fix: address review comments from Bun→Node migration PRs#989
Merged
Conversation
Contributor
|
Contributor
Codecov Results 📊✅ 7014 passed | Total: 7014 | Pass Rate: 100% | Execution Time: 0ms 📊 Comparison with Base Branch
✨ No test changes detected All tests are passing successfully. ❌ Patch coverage is 61.29%. Project has 14233 uncovered lines. Files with missing lines (4)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 77.10% 77.11% +0.01%
==========================================
Files 322 322 —
Lines 62163 62169 +6
Branches 0 0 —
==========================================
+ Hits 47923 47936 +13
- Misses 14240 14233 -7
- Partials 0 0 —Generated by Codecov Action |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fe0fae4. Configure here.
Consolidates fixes for review comments across PRs #967, #970, #984, #986. Changes: - Bump engines.node from >=22.12 to >=22.15 (zstd APIs require 22.15+, Node 20 is EOL). Update dist/bin.cjs runtime version check to match. - Simplify zstd imports: replace feature-detection dance with direct imports from node:zlib now that >=22.15 is guaranteed. - Fix spawn error handling in upgrade.ts: propagate error object to catch block so ENOENT/EBUSY detection works (was silently resolving with 1). - Fix sqlite.ts transaction(): wrap ROLLBACK in try/catch so original error is preserved if ROLLBACK itself fails. - Guard semver.compare calls in delta-upgrade.ts with semver.valid() to avoid throwing on invalid version strings. - Narrow catch in shell.ts addToGitHubPath to ENOENT only, re-throw other errors (EACCES, EPERM) instead of silently swallowing. - Add WriteStream backpressure handling in upgrade.ts streamDecompressToFile: check write() return value, await drain. - Add setup-node@v6 with Node 22 to E2E CI job. - Fix whichSync Windows CRLF: split on /\r?\n/ instead of \n. 7014 tests pass, 0 failures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Consolidates fixes for review comments across PRs #967, #970, #984, #986, superseding #988.
Critical
engines.nodeto>=22.15—node:zlibzstd APIs require 22.15+. Node 20 is EOL. Updateddist/bin.cjsruntime version check to match.import { zstdCompress } from "node:zlib"now that >=22.15 is guaranteed.High
upgrade.ts—proc.on("error", () => resolve(1))discarded the error object, making ENOENT/EBUSY detection dead code. Now properly rejects with the error.Medium
sqlite.tsROLLBACK — if ROLLBACK throws, the original transaction error was lost. Now wrapped in try/catch.semver.compareindelta-upgrade.tswithsemver.valid()to avoid throwing on invalid version strings.shell.tsaddToGitHubPath— only catch ENOENT, re-throw EACCES/EPERM.upgrade.tsstreamDecompressToFile— checkwrite()return value, await'drain'.setup-node@v6with Node 22 to E2E CI job.Low
whichSyncWindows CRLF — split on/\r?\n/instead of\nto strip trailing\rfromwhere.exeoutput.Test results
All 7014 tests pass, 0 failures.