Conversation
📝 WalkthroughWalkthroughThis PR removes Windows platform support from the hypersync-client-node package by eliminating Windows build targets from CI workflows, deleting Windows-specific npm packages, and bumping all remaining platform package versions from 1.0.1 to 1.1.0 alongside corresponding version check updates. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/publish_to_npm.yaml (2)
277-283:⚠️ Potential issue | 🟠 Major
test-macOS-bindingis missing frompublish.needs— macOS failures won't gate the release.After renaming
test-macOS-windows-binding→test-macOS-binding, the job was not added back to thepublishjob'sneedslist. As a result, a macOS binding breakage can silently slip through and still produce a published package.🐛 Proposed fix
publish: name: Publish runs-on: ubuntu-latest needs: + - test-macOS-binding - test-linux-x64-gnu-binding - test-linux-x64-musl-binding - test-linux-aarch64-gnu-binding🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish_to_npm.yaml around lines 277 - 283, The publish job's dependency list is missing the renamed macOS test job; update the publish job (symbol: publish) to include the new dependency name test-macOS-binding in its needs array so macOS CI failures will gate the release—add "test-macOS-binding" alongside the existing entries under the publish job's needs.
48-54:⚠️ Potential issue | 🟡 MinorDuplicate
yarn set version 3inaarch64-apple-darwinbuild step.
yarn set version 3appears twice consecutively (lines 51–52) — one is a copy-paste artifact.🔧 Proposed fix
- host: macos-latest target: aarch64-apple-darwin build: | yarn set version 3 - yarn set version 3 yarn build --target aarch64-apple-darwin strip -x *.node🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish_to_npm.yaml around lines 48 - 54, In the publish_to_npm workflow's aarch64 job (the block where target: aarch64-apple-darwin and build: are defined) remove the duplicate command so only a single "yarn set version 3" remains in the build script; update the build block to have the sequence "yarn set version 3", "yarn build --target aarch64-apple-darwin", "strip -x *.node" to eliminate the copy-paste artifact.index.js (1)
106-175:⚠️ Potential issue | 🟠 MajorRegenerate index.js from updated package.json — Windows paths are stale.
The Windows platform block (lines 106–175) currently exists in index.js, but Windows targets are no longer present in
package.json's napi configuration (only Linux and macOS targets remain: aarch64-apple-darwin, aarch64-unknown-linux-gnu, x86_64-unknown-linux-musl, x86_64-unknown-linux-gnu, x86_64-apple-darwin). Since this file is auto-generated by NAPI-RS, it must be regenerated to reflect the updated napi targets. Run:npm run artifactsor
napi artifactsto regenerate index.js with the current configuration. The Windows stubs will be removed, eliminating the dead code paths and ensuring the generated binding loader matches the published package targets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.js` around lines 106 - 175, index.js still contains the old Windows loading block (the branch checking process.platform === 'win32' and its process.arch cases) which is stale because the package's napi targets no longer include Windows; regenerate the auto-generated binding loader to remove these dead Windows paths by running the artifact generation command (e.g., "npm run artifacts" or "napi artifacts") so index.js is rebuilt to match the current package.json napi configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/publish_to_npm.yaml:
- Around line 277-283: The publish job's dependency list is missing the renamed
macOS test job; update the publish job (symbol: publish) to include the new
dependency name test-macOS-binding in its needs array so macOS CI failures will
gate the release—add "test-macOS-binding" alongside the existing entries under
the publish job's needs.
- Around line 48-54: In the publish_to_npm workflow's aarch64 job (the block
where target: aarch64-apple-darwin and build: are defined) remove the duplicate
command so only a single "yarn set version 3" remains in the build script;
update the build block to have the sequence "yarn set version 3", "yarn build
--target aarch64-apple-darwin", "strip -x *.node" to eliminate the copy-paste
artifact.
In `@index.js`:
- Around line 106-175: index.js still contains the old Windows loading block
(the branch checking process.platform === 'win32' and its process.arch cases)
which is stale because the package's napi targets no longer include Windows;
regenerate the auto-generated binding loader to remove these dead Windows paths
by running the artifact generation command (e.g., "npm run artifacts" or "napi
artifacts") so index.js is rebuilt to match the current package.json napi
configuration.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/publish_to_npm.yamlindex.jsnpm/darwin-arm64/package.jsonnpm/darwin-x64/package.jsonnpm/linux-arm64-gnu/package.jsonnpm/linux-x64-gnu/package.jsonnpm/linux-x64-musl/package.jsonnpm/win32-arm64-msvc/README.mdnpm/win32-arm64-msvc/package.jsonnpm/win32-x64-msvc/README.mdnpm/win32-x64-msvc/package.jsonpackage.json
💤 Files with no reviewable changes (4)
- npm/win32-x64-msvc/package.json
- npm/win32-arm64-msvc/README.md
- npm/win32-arm64-msvc/package.json
- npm/win32-x64-msvc/README.md
1 participant
The windows tests etc are too flakey and I don't think we have any users of the hypersync client on windows. We don't support it on hyperindex anyhow.
Summary by CodeRabbit
Version Update
Removed Features