Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRenames package to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Consumer Code
participant Loader as index.js (binding loader)
participant Native as `@rush-fs/core-`<platform>-<arch>
participant WASI as `@rush-fs/core-wasm32-wasi`
App->>Loader: require('@rush-fs/core')
Loader->>Native: attempt platform-specific binding load
alt binding loaded
Native-->>Loader: native binding (v)
Loader->>Loader: validate v == 0.1.0-beta.1
alt version mismatch
Loader-->>App: throw version mismatch error
else version match
Loader-->>App: export native binding
end
else load fails
Loader->>WASI: attempt WASI fallback
alt WASI available
WASI-->>Loader: wasi binding
Loader-->>App: export wasi binding
else
Loader-->>App: throw aggregated "cannot find native binding" error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
CoderSerio
force-pushed
the
feat/carbon-dev
branch
from
8c904ec to
9496680
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
README.zh-CN.md (1)
34-37:⚠️ Potential issue | 🟡 MinorManual installation instructions still reference the old
@rush-fs/rush-fs-*binding names.These platform-package names (
@rush-fs/rush-fs-darwin-arm64, etc.) correspond tonapi.binaryName: "rush-fs", but after renaming to@rush-fs/corethe intention is to publish@rush-fs/core-*packages (@rush-fs/core-darwin-arm64, etc.). OncebinaryNameis changed to"core"and packages are re-published under that name, these instructions will direct users to non-existent packages.📝 Proposed update
- **macOS ARM：** `pnpm add `@rush-fs/rush-fs-darwin-arm64`` - **macOS x64：** `pnpm add `@rush-fs/rush-fs-darwin-x64`` - **Windows x64：** `pnpm add `@rush-fs/rush-fs-win32-x64-msvc`` - **Linux x64 (glibc)：** `pnpm add `@rush-fs/rush-fs-linux-x64-gnu`` + **macOS ARM：** `pnpm add `@rush-fs/core-darwin-arm64`` + **macOS x64：** `pnpm add `@rush-fs/core-darwin-x64`` + **Windows x64：** `pnpm add `@rush-fs/core-win32-x64-msvc`` + **Linux x64 (glibc)：** `pnpm add `@rush-fs/core-linux-x64-gnu``🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.zh-CN.md` around lines 34 - 37, The README's manual install lines still reference the old binding packages like `@rush-fs/rush-fs-darwin-arm64`, `@rush-fs/rush-fs-win32-x64-msvc`, etc.; update those to the new package naming scheme (e.g., `@rush-fs/core-darwin-arm64`, `@rush-fs/core-darwin-x64`, `@rush-fs/core-win32-x64-msvc`, `@rush-fs/core-linux-x64-gnu`) and ensure the package metadata uses napi.binaryName: "core" so the published artifacts match these names; replace each instance of `@rush-fs/rush-fs-`* in the README with the corresponding `@rush-fs/core-`* name and verify the npm install examples (pnpm add ...) reflect the new names.README.md (1)
34-38:⚠️ Potential issue | 🟡 MinorSame platform binding name mismatch as in
README.zh-CN.md.The manual-install commands still reference
@rush-fs/rush-fs-*(matchingbinaryName: "rush-fs") instead of@rush-fs/core-*(matching the intendedbinaryName: "core").📝 Proposed update
- **macOS ARM:** `pnpm add `@rush-fs/rush-fs-darwin-arm64`` - **macOS x64:** `pnpm add `@rush-fs/rush-fs-darwin-x64`` - **Windows x64:** `pnpm add `@rush-fs/rush-fs-win32-x64-msvc`` - **Linux x64 (glibc):** `pnpm add `@rush-fs/rush-fs-linux-x64-gnu`` + **macOS ARM:** `pnpm add `@rush-fs/core-darwin-arm64`` + **macOS x64:** `pnpm add `@rush-fs/core-darwin-x64`` + **Windows x64:** `pnpm add `@rush-fs/core-win32-x64-msvc`` + **Linux x64 (glibc):** `pnpm add `@rush-fs/core-linux-x64-gnu``🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 34 - 38, The README's manual-install package names reference `@rush-fs/rush-fs-`* but the project uses binaryName "core", so update each platform line to the correct package namespace `@rush-fs/core-`*, e.g. replace occurrences like `@rush-fs/rush-fs-darwin-arm64`, `@rush-fs/rush-fs-darwin-x64`, `@rush-fs/rush-fs-win32-x64-msvc`, `@rush-fs/rush-fs-linux-x64-gnu` with `@rush-fs/core-darwin-arm64`, `@rush-fs/core-darwin-x64`, `@rush-fs/core-win32-x64-msvc`, `@rush-fs/core-linux-x64-gnu` respectively so the README matches binaryName "core".index.js (1)
80-92:⚠️ Potential issue | 🔴 CriticalBroken brace structure causes a JavaScript
SyntaxErrorthroughoutindex.js— direct cause of all CI export failures.This is an auto-generated file (
/* auto-generated by NAPI-RS */). The new lines were hand-inserted inside the existingif (bindingPackageVersion !== '0.0.5') { throw }block after removing its closing}, producing the following malformed structure (darwin-arm64 shown; the identical defect repeats in every platform section):try { // depth 1 const binding = require('rush-fs-…') … if (… !== '0.0.5' …) { // depth 2 (if-A) throw new Error(…) // ← always throws here when condition is true if (… !== '0.0.3' …) { // depth 3 (if-B – unreachable after the throw above) throw new Error(…) // ↑ if-B is never closed with } before the next const const binding = require('@rush-fs/core-…') // still inside if-B const bindingPackageVersion = … // still inside if-B if (… !== '0.1.0' …) { // depth 4 (if-C) throw new Error(…) } // closes if-C ← line 273 / 88 return binding // still inside if-B } catch (e) { // } closes if-B → if-A + try still open → SyntaxErrorBecause
} catchcloses if-B rather than thetryblock, thecatchclause appears in an invalid position. Node.js cannot parse the file, socjs-module-lexerreports zero named exports, which is exactly what every CI error reflects:
SyntaxError: The requested module '../index.js' does not provide an export named 'access'The correct fix is to regenerate
index.jsvianapi build, not to patch it by hand. After resolving thebinaryNameinconsistency flagged inpackage.json, run:pnpm build # or: napi build --platformIf a manual structure is required in the interim, each platform section should have two independent
tryblocks — one for the oldrush-fs-*package and one for the new@rush-fs/core-*package:📐 Correct structure for one section (android-arm64) as a reference
try { const binding = require('rush-fs-android-arm64') const bindingPackageVersion = require('rush-fs-android-arm64/package.json').version - if (bindingPackageVersion !== '0.0.5' && process.env.NAPI_RS_ENFORCE_VERSION_CHECK && process.env.NAPI_RS_ENFORCE_VERSION_CHECK !== '0') { - throw new Error(`Native binding package version mismatch, expected 0.0.5 but got ${bindingPackageVersion}. You can reinstall dependencies to fix this issue.`) - if (bindingPackageVersion !== '0.0.3' && process.env.NAPI_RS_ENFORCE_VERSION_CHECK && process.env.NAPI_RS_ENFORCE_VERSION_CHECK !== '0') { - throw new Error(`Native binding package version mismatch, expected 0.0.3 but got ${bindingPackageVersion}. You can reinstall dependencies to fix this issue.`) - const binding = require('@rush-fs/core-android-arm64') - const bindingPackageVersion = require('@rush-fs/core-android-arm64/package.json').version - if (bindingPackageVersion !== '0.1.0' && process.env.NAPI_RS_ENFORCE_VERSION_CHECK && process.env.NAPI_RS_ENFORCE_VERSION_CHECK !== '0') { - throw new Error(`Native binding package version mismatch, expected 0.1.0 but got ${bindingPackageVersion}. You can reinstall dependencies to fix this issue.`) - } + if (bindingPackageVersion !== '0.0.5' && process.env.NAPI_RS_ENFORCE_VERSION_CHECK && process.env.NAPI_RS_ENFORCE_VERSION_CHECK !== '0') { + throw new Error(`Native binding package version mismatch, expected 0.0.5 but got ${bindingPackageVersion}. You can reinstall dependencies to fix this issue.`) + } return binding } catch (e) { loadErrors.push(e) } + try { + const binding = require('@rush-fs/core-android-arm64') + const bindingPackageVersion = require('@rush-fs/core-android-arm64/package.json').version + if (bindingPackageVersion !== '0.1.0-alpha.0' && process.env.NAPI_RS_ENFORCE_VERSION_CHECK && process.env.NAPI_RS_ENFORCE_VERSION_CHECK !== '0') { + throw new Error(`Native binding package version mismatch, expected 0.1.0-alpha.0 but got ${bindingPackageVersion}. You can reinstall dependencies to fix this issue.`) + } + return binding + } catch (e) { + loadErrors.push(e) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.js` around lines 80 - 92, index.js has malformed brace/try-catch structure around the platform sections causing a SyntaxError: the original if block checking bindingPackageVersion (e.g. the '0.0.5' check) was not closed before adding the new code paths (the successive bindingPackageVersion checks and require('@rush-fs/core-android-arm64')), so the catch clause is misplaced; fix by regenerating the file via napi build after correcting the binaryName inconsistency in package.json, or if you must patch manually, ensure each platform section uses two independent try blocks (one for the legacy rush-fs require and version check, and a separate try for the `@rush-fs/core` require and its bindingPackageVersion checks '0.0.3'/'0.1.0'), and close the original if and try blocks (the if checking bindingPackageVersion !== '0.0.5' and its corresponding }) before adding the new require and new version checks so the catch belongs to the correct try.package.json (1)
34-35:⚠️ Potential issue | 🟠 MajorChange
binaryNamefrom"rush-fs"to"core"in package.json.With the main package name as
@rush-fs/coreandbinaryNameset to"rush-fs", there is a mismatch. NAPI-RS derives platform-specific package names from the main package name (e.g.,@rush-fs/core-linux-x64-gnu), and thebinaryNameshould correspond to the unscoped part of the main package name. Since the package is@rush-fs/core, thebinaryNamemust be"core"to align with the scoped platform packages thatindex.jsis already trying to require.After changing
binaryName, regenerateindex.jsvianapi buildto ensure the loader properly references the correct binary names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 34 - 35, Update package.json's napi.binaryName from "rush-fs" to "core" so it matches the unscoped part of the main package name (`@rush-fs/core`); then run `napi build` to regenerate the native loader (index.js) so the require calls and generated platform package names (e.g., `@rush-fs/core-`*) align with the new binaryName and the loader’s expected module names.
🧹 Nitpick comments (2)
.github/workflows/Release.yml (2)
12-12: Consider settingCARGO_INCREMENTALto'0'for CI.Incremental compilation in CI rarely pays off: any dependency change invalidates large portions of the incremental state, the cargo
target/cache already handles full-artifact reuse, andCARGO_INCREMENTAL=1bloats that cache. The standard pattern for CI pipelines isCARGO_INCREMENTAL: '0'.♻️ Suggested change
- CARGO_INCREMENTAL: '1' + CARGO_INCREMENTAL: '0'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/Release.yml at line 12, Change the CI environment variable CARGO_INCREMENTAL from '1' to '0' in the workflow so incremental compilation is disabled; locate the CARGO_INCREMENTAL entry in the Release.yml workflow and set CARGO_INCREMENTAL: '0' (replace the current CARGO_INCREMENTAL: '1').
1-6: Residual Chinese comments in the publish step body are inconsistent with the new English header.The inline comments at lines 152, 163, 180, and 183 are still in Chinese. The comment on line 183 also references
pnpm i rush-fs, which is the old, now-renamed package. These are pre-existing lines outside this diff, but worth cleaning up in a follow-on to keep the file uniformly English.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/Release.yml around lines 1 - 6, The publish step in the Release.yml workflow contains residual Chinese comments and an outdated package reference; locate the publish job/step (the block describing the publish action) and replace the Chinese inline comments at the publish step body with concise English comments, and update any commands or references like "pnpm i rush-fs" to the current package name/command (e.g., use the correct `@rush-fs/core` install/publish invocation or remove the obsolete line), ensuring all comments in the publish step are consistently English and the package name is current.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.js`:
- Around line 86-87: The version check compares bindingPackageVersion to the
hardcoded string '0.1.0' which fails for the published pre-release
'0.1.0-alpha.0'; update the check in the blocks that reference
bindingPackageVersion and NAPI_RS_ENFORCE_VERSION_CHECK so the expected version
matches the package's declared version (e.g., use the exact '0.1.0-alpha.0'
value, read the correct version dynamically from package metadata, or allow
pre-release by comparing the base semver like startsWith('0.1.0')) and update
the error message text to reflect the same expected-version string so the
condition and thrown message remain consistent (ensure changes are applied to
every platform section that uses bindingPackageVersion and the error throw).
In `@package.json`:
- Around line 2-6: The package.json contains duplicate top-level "name" and
"version" keys; remove the stale entries so only the intended values remain:
delete the "name": "rush-fs" entry and both older "version": "0.0.5" and
"version": "0.0.4" entries, leaving only "name": "@rush-fs/core" and "version":
"0.1.0-alpha.0"; ensure the resulting JSON is valid (no duplicate keys or
trailing commas).
---
Outside diff comments:
In `@index.js`:
- Around line 80-92: index.js has malformed brace/try-catch structure around the
platform sections causing a SyntaxError: the original if block checking
bindingPackageVersion (e.g. the '0.0.5' check) was not closed before adding the
new code paths (the successive bindingPackageVersion checks and
require('@rush-fs/core-android-arm64')), so the catch clause is misplaced; fix
by regenerating the file via napi build after correcting the binaryName
inconsistency in package.json, or if you must patch manually, ensure each
platform section uses two independent try blocks (one for the legacy rush-fs
require and version check, and a separate try for the `@rush-fs/core` require and
its bindingPackageVersion checks '0.0.3'/'0.1.0'), and close the original if and
try blocks (the if checking bindingPackageVersion !== '0.0.5' and its
corresponding }) before adding the new require and new version checks so the
catch belongs to the correct try.
In `@package.json`:
- Around line 34-35: Update package.json's napi.binaryName from "rush-fs" to
"core" so it matches the unscoped part of the main package name (`@rush-fs/core`);
then run `napi build` to regenerate the native loader (index.js) so the require
calls and generated platform package names (e.g., `@rush-fs/core-`*) align with
the new binaryName and the loader’s expected module names.
In `@README.md`:
- Around line 34-38: The README's manual-install package names reference
`@rush-fs/rush-fs-`* but the project uses binaryName "core", so update each
platform line to the correct package namespace `@rush-fs/core-`*, e.g. replace
occurrences like `@rush-fs/rush-fs-darwin-arm64`, `@rush-fs/rush-fs-darwin-x64`,
`@rush-fs/rush-fs-win32-x64-msvc`, `@rush-fs/rush-fs-linux-x64-gnu` with
`@rush-fs/core-darwin-arm64`, `@rush-fs/core-darwin-x64`,
`@rush-fs/core-win32-x64-msvc`, `@rush-fs/core-linux-x64-gnu` respectively so the
README matches binaryName "core".
In `@README.zh-CN.md`:
- Around line 34-37: The README's manual install lines still reference the old
binding packages like `@rush-fs/rush-fs-darwin-arm64`,
`@rush-fs/rush-fs-win32-x64-msvc`, etc.; update those to the new package naming
scheme (e.g., `@rush-fs/core-darwin-arm64`, `@rush-fs/core-darwin-x64`,
`@rush-fs/core-win32-x64-msvc`, `@rush-fs/core-linux-x64-gnu`) and ensure the
package metadata uses napi.binaryName: "core" so the published artifacts match
these names; replace each instance of `@rush-fs/rush-fs-`* in the README with the
corresponding `@rush-fs/core-`* name and verify the npm install examples (pnpm add
...) reflect the new names.
---
Nitpick comments:
In @.github/workflows/Release.yml:
- Line 12: Change the CI environment variable CARGO_INCREMENTAL from '1' to '0'
in the workflow so incremental compilation is disabled; locate the
CARGO_INCREMENTAL entry in the Release.yml workflow and set CARGO_INCREMENTAL:
'0' (replace the current CARGO_INCREMENTAL: '1').
- Around line 1-6: The publish step in the Release.yml workflow contains
residual Chinese comments and an outdated package reference; locate the publish
job/step (the block describing the publish action) and replace the Chinese
inline comments at the publish step body with concise English comments, and
update any commands or references like "pnpm i rush-fs" to the current package
name/command (e.g., use the correct `@rush-fs/core` install/publish invocation or
remove the obsolete line), ensuring all comments in the publish step are
consistently English and the package name is current.
9496680 to
dff4af5
Compare
…a.0; enhance documentation for migration and usage
CoderSerio
force-pushed
the
feat/carbon-dev
branch
from
dff4af5 to
9f385ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
README.md (1)
31-38:⚠️ Potential issue | 🟡 MinorStale platform binding package names in manual installation instructions.
Line 31 (newly written) and lines 35–38 still reference the old
@rush-fs/rush-fs-*naming, butindex.jsnow loads@rush-fs/core-*packages. Users following the fallback install steps would install non-existent packages.📝 Proposed fix
-When you install `@rush-fs/core`, the package manager should automatically install the **platform-specific native binding** for your OS/arch via `optionalDependencies` (e.g. `@rush-fs/rush-fs-darwin-arm64` on macOS ARM). +When you install `@rush-fs/core`, the package manager should automatically install the **platform-specific native binding** for your OS/arch via `optionalDependencies` (e.g. `@rush-fs/core-darwin-arm64` on macOS ARM).- **macOS ARM:** `pnpm add `@rush-fs/rush-fs-darwin-arm64`` - **macOS x64:** `pnpm add `@rush-fs/rush-fs-darwin-x64`` - **Windows x64:** `pnpm add `@rush-fs/rush-fs-win32-x64-msvc`` - **Linux x64 (glibc):** `pnpm add `@rush-fs/rush-fs-linux-x64-gnu`` + **macOS ARM:** `pnpm add `@rush-fs/core-darwin-arm64`` + **macOS x64:** `pnpm add `@rush-fs/core-darwin-x64`` + **Windows x64:** `pnpm add `@rush-fs/core-win32-x64-msvc`` + **Linux x64 (glibc):** `pnpm add `@rush-fs/core-linux-x64-gnu``🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 31 - 38, Update the README fallback install steps to use the current platform package naming (change occurrences of `@rush-fs/rush-fs-`* to `@rush-fs/core-`*) so they match how index.js loads the native bindings; specifically replace examples like `@rush-fs/rush-fs-darwin-arm64`, `@rush-fs/rush-fs-darwin-x64`, `@rush-fs/rush-fs-win32-x64-msvc`, and `@rush-fs/rush-fs-linux-x64-gnu` with `@rush-fs/core-darwin-arm64`, `@rush-fs/core-darwin-x64`, `@rush-fs/core-win32-x64-msvc`, and `@rush-fs/core-linux-x64-gnu` respectively so the documented pnpm add commands match the loader in index.js..github/workflows/Release.yml (2)
136-144:⚠️ Potential issue | 🟡 Minor
console.logfires before the guard — logs misleadingly even when no rename occursThe
console.logon line 140 prints"Updating package name from X to@rush-fs/X"unconditionally, before theif (!pkg.name.startsWith('@rush-fs/'))check. Move it inside the guard.♻️ Proposed fix
node -e " const fs = require('fs'); const pkgPath = process.env.PKG_PATH; const pkg = JSON.parse(fs.readFileSync(pkgPath, 'utf8')); - console.log('Updating package name from', pkg.name, 'to `@rush-fs/`' + pkg.name); if (!pkg.name.startsWith('@rush-fs/')) { + console.log('Updating package name from', pkg.name, 'to `@rush-fs/`' + pkg.name); pkg.name = '@rush-fs/' + pkg.name; fs.writeFileSync(pkgPath, JSON.stringify(pkg, null, 2)); } "🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/Release.yml around lines 136 - 144, The console.log currently runs unconditionally before the guard, producing misleading output; move the console.log call so it executes only inside the if (!pkg.name.startsWith('@rush-fs/')) block (using the same pkg and pkgPath variables) so it logs "Updating package name from ... to `@rush-fs/`..." only when pkg.name is actually changed, and leave no log when the package already starts with '@rush-fs/'.
181-182:⚠️ Potential issue | 🟠 MajorSilent-failure risk:
sedno-ops ifindex.jsuses different require patternsThe
sedcommands exit0even when no substitution occurs. Inspection of the actual generatedindex.jsshows it already uses scoped@rush-fs/core-*requires and relative paths (./rush-fs...), not the unscopedrush-fs-*pattern that the sed commands target. This means both substitutions will silently fail, leaving no CI signal. A published@rush-fs/corepackage with broken requires would only be discovered by end users.Add an explicit verification after the substitutions:
🛡️ Proposed fix — add sanity check
sed -i "s/require('rush-fs-/require('@rush-fs\/rush-fs-/g" index.js sed -i 's/require(\"rush-fs-/require(\"@rush-fs\/rush-fs-/g' index.js + # Abort if none of the scoped requires ended up in index.js + if ! grep -q "require('@rush-fs/" index.js && ! grep -q 'require("@rush-fs/' index.js; then + echo "ERROR: index.js still contains no scoped `@rush-fs/` requires after sed; aborting publish." >&2 + exit 1 + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/Release.yml around lines 181 - 182, The CI currently runs two sed substitutions targeting unscoped "require('rush-fs-" patterns in index.js but sed silently succeeds even when no replacements occur; update the workflow to verify the replacements by checking index.js after the sed commands (e.g., search for remaining unscoped require patterns like require('rush-fs- or require("rush-fs-) and fail the job if any are found) so the step that edits index.js aborts with a non-zero exit when the expected substitution did not happen; reference the existing sed commands that modify index.js to locate where to add this post-check.
🧹 Nitpick comments (3)
.github/workflows/Release.yml (3)
12-12: Consider settingCARGO_INCREMENTAL: '0'in CIThe cargo cache key (line 71) is
${{ matrix.settings.target }}-cargo-${{ matrix.settings.host }}— it doesn't include a Cargo.lock or dependency hash. When dependencies change, stale incremental artifacts can remain in the cache and cause subtle linker failures. The standard Rust CI recommendation is to disable incremental compilation in CI environments.♻️ Suggested change
- CARGO_INCREMENTAL: '1' + CARGO_INCREMENTAL: '0'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/Release.yml at line 12, The CI sets CARGO_INCREMENTAL: '1' which can cause stale incremental artifacts to be cached and produce linker errors; change the environment variable CARGO_INCREMENTAL to '0' in the workflow (replace the CARGO_INCREMENTAL: '1' entry with CARGO_INCREMENTAL: '0') so incremental compilation is disabled in CI and cached artifacts won't cause subtle build failures.
184-196: HardcodedoptionalDependencieswill silently drift if the build matrix growsAdding a new target to the
matrix.settings(e.g.,aarch64-unknown-linux-gnu) requires a matching manual update here; omitting it means the platform package gets published but is never auto-installed for those users.Auto-derive from the actual
npm/directories instead:♻️ Proposed refactor
- node -e " - const fs = require('fs'); - const p = JSON.parse(fs.readFileSync('package.json', 'utf8')); - const v = p.version; - p.optionalDependencies = { - '@rush-fs/rush-fs-win32-x64-msvc': v, - '@rush-fs/rush-fs-darwin-x64': v, - '@rush-fs/rush-fs-linux-x64-gnu': v, - '@rush-fs/rush-fs-darwin-arm64': v - }; - fs.writeFileSync('package.json', JSON.stringify(p, null, 2)); - " + node -e " + const fs = require('fs'); + const p = JSON.parse(fs.readFileSync('package.json', 'utf8')); + const v = p.version; + // Auto-derive from whichever platform packages were actually built and renamed + const optDeps = {}; + for (const dir of fs.readdirSync('npm')) { + const pkgPath = \`npm/\${dir}/package.json\`; + if (!fs.existsSync(pkgPath)) continue; + const name = JSON.parse(fs.readFileSync(pkgPath, 'utf8')).name; + if (name) optDeps[name] = v; + } + p.optionalDependencies = optDeps; + fs.writeFileSync('package.json', JSON.stringify(p, null, 2)); + "🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/Release.yml around lines 184 - 196, Replace the hardcoded optionalDependencies object assignment so it is built dynamically from the actual npm/ package directories: read the npm/ directory (e.g., using fs.readdirSync('npm')), filter for platform package folders, map each folder name to the scoped package name (e.g., `@rush-fs/${folder}`) and assign the current version variable v as the value, then set p.optionalDependencies to that generated object before writing package.json; keep using the existing p and v variables and ensure the logic replaces the static block that lists '@rush-fs/...'.
148-196: Inline comments in the publish step remain in Chinese after the header was translatedLines 152, 163, 180, and 183 still carry Chinese comments. Given that the header block was specifically updated to English in this PR, translating these would keep the file consistent for future contributors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/Release.yml around lines 148 - 196, In the "Publish to npm" job, replace the remaining Chinese inline comments with English equivalents to match the rest of the file: update the comment before git fetch/tags ("# 先 fetch tags，然后检查对应的 tag 是否已存在"), the one before the npm/* loop ("# prepublish 之后再次更新包名（因为 prepublish 可能会更新 package.json）"), the index.js require explanation ("# 主包 index.js 里 require 的是 rush-fs-xxx，改为 `@rush-fs/rush-fs-xxx` 才能找到已发布的平台包"), and the optionalDependencies note ("# optionalDependencies 必须在 prepublish 之后注入，否则会被 prepublish 覆盖，导致用户 pnpm i rush-fs 时不会自动安装平台包") with concise English comments that convey the same intent in the Publish to npm step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.zh-CN.md`:
- Line 39: Update the version string in README.zh-CN.md to match the CHANGELOG
anchor: change "0.1.0-alpha" to the full "0.1.0-alpha.0" in the migration note
so the prose and the [CHANGELOG.md](./CHANGELOG.md#010-alpha0) link align.
---
Outside diff comments:
In @.github/workflows/Release.yml:
- Around line 136-144: The console.log currently runs unconditionally before the
guard, producing misleading output; move the console.log call so it executes
only inside the if (!pkg.name.startsWith('@rush-fs/')) block (using the same pkg
and pkgPath variables) so it logs "Updating package name from ... to
`@rush-fs/`..." only when pkg.name is actually changed, and leave no log when the
package already starts with '@rush-fs/'.
- Around line 181-182: The CI currently runs two sed substitutions targeting
unscoped "require('rush-fs-" patterns in index.js but sed silently succeeds even
when no replacements occur; update the workflow to verify the replacements by
checking index.js after the sed commands (e.g., search for remaining unscoped
require patterns like require('rush-fs- or require("rush-fs-) and fail the job
if any are found) so the step that edits index.js aborts with a non-zero exit
when the expected substitution did not happen; reference the existing sed
commands that modify index.js to locate where to add this post-check.
In `@README.md`:
- Around line 31-38: Update the README fallback install steps to use the current
platform package naming (change occurrences of `@rush-fs/rush-fs-`* to
`@rush-fs/core-`*) so they match how index.js loads the native bindings;
specifically replace examples like `@rush-fs/rush-fs-darwin-arm64`,
`@rush-fs/rush-fs-darwin-x64`, `@rush-fs/rush-fs-win32-x64-msvc`, and
`@rush-fs/rush-fs-linux-x64-gnu` with `@rush-fs/core-darwin-arm64`,
`@rush-fs/core-darwin-x64`, `@rush-fs/core-win32-x64-msvc`, and
`@rush-fs/core-linux-x64-gnu` respectively so the documented pnpm add commands
match the loader in index.js.
---
Nitpick comments:
In @.github/workflows/Release.yml:
- Line 12: The CI sets CARGO_INCREMENTAL: '1' which can cause stale incremental
artifacts to be cached and produce linker errors; change the environment
variable CARGO_INCREMENTAL to '0' in the workflow (replace the
CARGO_INCREMENTAL: '1' entry with CARGO_INCREMENTAL: '0') so incremental
compilation is disabled in CI and cached artifacts won't cause subtle build
failures.
- Around line 184-196: Replace the hardcoded optionalDependencies object
assignment so it is built dynamically from the actual npm/ package directories:
read the npm/ directory (e.g., using fs.readdirSync('npm')), filter for platform
package folders, map each folder name to the scoped package name (e.g.,
`@rush-fs/${folder}`) and assign the current version variable v as the value,
then set p.optionalDependencies to that generated object before writing
package.json; keep using the existing p and v variables and ensure the logic
replaces the static block that lists '@rush-fs/...'.
- Around line 148-196: In the "Publish to npm" job, replace the remaining
Chinese inline comments with English equivalents to match the rest of the file:
update the comment before git fetch/tags ("# 先 fetch tags，然后检查对应的 tag 是否已存在"),
the one before the npm/* loop ("# prepublish 之后再次更新包名（因为 prepublish 可能会更新
package.json）"), the index.js require explanation ("# 主包 index.js 里 require 的是
rush-fs-xxx，改为 `@rush-fs/rush-fs-xxx` 才能找到已发布的平台包"), and the optionalDependencies
note ("# optionalDependencies 必须在 prepublish 之后注入，否则会被 prepublish 覆盖，导致用户 pnpm i
rush-fs 时不会自动安装平台包") with concise English comments that convey the same intent
in the Publish to npm step.
Owner
Author
|
🦊 LGTM, but it needs a little more time to make sure it's stable for the most to use |
CoderSerio
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 11: The changelog currently calls out "**This is an alpha release.**"
which conflicts with the published pre-release identifier `0.1.0-beta.0`; update
that sentence in CHANGELOG.md to read "This is a beta release." or "This is a
pre-release." (or otherwise mention `beta`) so the wording matches the
`0.1.0-beta.0` tag and avoids confusion.
- Line 9: The changelog section heading "## [0.1.0] - (release date TBD)"
doesn't match the link definition "[0.1.0-beta.0]"; update the section heading
to use the actual released version by renaming "## [0.1.0] - (release date TBD)"
to "## [0.1.0-beta.0] - (release date TBD)" so the header and the link reference
(the "[0.1.0-beta.0]" link definition) match and the MD053 linter warning is
resolved.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 11: The README mismatch: CHANGELOG.md says "beta" but README.md and
README.zh-CN.md still show "alpha"; update README.md's status-alpha badge to
status-beta (or the equivalent badge string) and replace the explicit "alpha"
text in README.zh-CN.md ("当前为 **alpha** 版本") with "beta" so all docs match the
0.1.0-beta.0 release tag; search for the literal strings "status-alpha",
"alpha", and the README badge lines to locate and edit the entries in README.md
and README.zh-CN.md.
In `@README.md`:
- Line 38: The README references a broken CHANGELOG anchor `#010-alpha0`; update
the link target in README.md (and README.zh-CN.md) to match the actual CHANGELOG
heading you'll use — if you change the CHANGELOG heading to `## [0.1.0-beta.0]`
(replacing the current `## [0.1.0]`), change the anchor to `#010-beta0`;
alternatively, if you keep the current `## [0.1.0]` heading, change the README
anchors to `#010`. Ensure the anchor string in README.md and README.zh-CN.md
matches the chosen CHANGELOG heading (`## [0.1.0]` or `## [0.1.0-beta.0]`) so
the `#010-beta0` (or `#010`) link resolves.
---
Duplicate comments:
In `@CHANGELOG.md`:
- Line 9: The changelog heading "## [0.1.0]" does not match the link reference
"[0.1.0-beta.0]" (the unused/orphaned reference at the bottom), so update one to
match the other; either rename the heading to "## [0.1.0-beta.0]" or change the
link reference "[0.1.0-beta.0]" to "[0.1.0]" so the heading and link identifier
are identical (search for the header "## [0.1.0]" and the link reference
"[0.1.0-beta.0]" to locate where to make the change).
In `@README.md`:
- Line 11: Update the status badge in README.md from "status-alpha" to
"status-beta" (or remove it) so it matches the release tag 0.1.0-beta.0; do the
same change in README.zh-CN.md at the corresponding badge (around line 19) —
locate the <img> tag with src containing "status-alpha" and either change the
URL/alt to "status-beta" or delete the tag, and commit both files so badges
align with the CHANGELOG and release version.
In `@README.zh-CN.md`:
- Line 39: Update the version string and the CHANGELOG anchor in the README
zh-CN migration note: change the text "0.1.0-alpha" to "0.1.0-beta.0" in the
sentence that starts with "从 `rush-fs` 迁移：…主包更名为 `@rush-fs/core`", and update
the linked anchor "./CHANGELOG.md#010-alpha0" to "./CHANGELOG.md#010-beta0" so
it matches the corrected CHANGELOG heading `## [0.1.0-beta.0]`.
- Line 11: Update the README.zh-CN.md to replace the remaining "alpha"
references with "beta": change the shield badge name from "status-alpha" (the
img line with alt="Alpha") to "status-beta" and update the corresponding
paragraph that currently announces "alpha" (the <p> block referenced on line 19)
so both the badge and text read "beta" to match version 0.1.0-beta.0/CHANGELOG.
b0f8ba5 to
cb91348
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
__test__/glob.spec.ts (1)
10-19:makeDirFixturenever cleans up created temp directories.Each test that calls
makeDirFixtureleaves a directory under the systemtmpdir. While the random suffix prevents collisions, running the suite repeatedly accumulates stale directories. Adding a teardown registration would keep the temp filesystem clean.♻️ Suggested addition
-function makeDirFixture(): string { +function makeDirFixture(t: { teardown: (fn: () => void) => void }): string { const base = join(tmpdir(), `hyper-glob-test-${Date.now()}-${Math.random().toString(36).slice(2)}`) nodeFs.mkdirSync(join(base, 'src/sub'), { recursive: true }) nodeFs.writeFileSync(join(base, 'src/a.ts'), '') nodeFs.writeFileSync(join(base, 'src/b.ts'), '') nodeFs.writeFileSync(join(base, 'src/sub/c.ts'), '') nodeFs.mkdirSync(join(base, 'dist'), { recursive: true }) nodeFs.writeFileSync(join(base, 'dist/out.js'), '') + t.teardown(() => nodeFs.rmSync(base, { recursive: true, force: true })) return base }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__test__/glob.spec.ts` around lines 10 - 19, makeDirFixture creates temp directories but never removes them; update it to register cleanup so tests don't leak tmp files: after creating the base dir in makeDirFixture, register a teardown (e.g., via afterEach/afterAll or a test framework cleanup helper) that removes the returned directory using nodeFs.rmSync(base, { recursive: true, force: true }) or equivalent, or maintain a global list of created bases and remove them in an afterEach/afterAll handler; reference the makeDirFixture function and the created base variable when adding the cleanup registration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 9: The CHANGELOG section heading "## [0.1.0]" is incorrect for the
pre-release; change that heading to "## [0.1.0-beta.1]" so it matches the
existing link target "v0.1.0-beta.1" and Keep a Changelog conventions, and then
update the anchor links referenced in README.md and README.zh-CN.md (currently
"#010-alpha0") to "#010-beta1" so the README anchor targets match the new
heading.
In `@src/glob.rs`:
- Around line 13-20: The extract_path_prefix function currently only looks for
the first '*' so patterns where '?' or '[' appear before '*' produce an invalid
walk_root; update extract_path_prefix to locate the earliest glob metacharacter
(at least '*', '?', '[' — you can scan the pattern byte-by-byte or take the min
index of pattern.find('*'), pattern.find('?'), pattern.find('[')) and use that
index instead of only the '*' index; keep the existing trimming logic and None
returns when the prefix is empty or "." and return Some((prefix.to_string(),
pattern[first_metachar..].to_string())) so walk_root never contains unhandled
glob metacharacters.
---
Duplicate comments:
In `@README.md`:
- Line 11: The README's status badge uses "status-alpha" but the project release
label in CHANGELOG.md is "beta"; update the badge to match by replacing the
"status-alpha" token (the img src/alt that currently reads status-alpha) with
"status-beta" so the README badge and CHANGELOG.md are consistent, or
alternatively align CHANGELOG.md to "alpha" if the intended project status is
alpha—ensure the badge src and alt text both reflect the chosen label.
- Line 38: Update the broken CHANGELOG anchor in the README: change the link
target in the "**Migration from `rush-fs`:**" line from
"./CHANGELOG.md#010-alpha0" to the correct anchor "./CHANGELOG.md#010-beta1"
(this matches the updated CHANGELOG heading `## [0.1.0-beta.1]`). Ensure the
text around the link remains unchanged and only the fragment identifier is
replaced.
In `@README.zh-CN.md`:
- Line 11: The status badge in README.zh-CN.md is showing "status-alpha" while
CHANGELOG.md says this release is beta; update the <img> badge to reflect beta
(e.g., change the badge URL/label from "status-alpha" to "status-beta" and
adjust the alt text from "Alpha" to "Beta") so the README badge matches the
CHANGELOG release label.
- Line 36: Update the migration note line to reference the new prerelease
version and correct CHANGELOG anchor: change the version text from "0.1.0-alpha"
to "0.1.0-beta.1" and update the link anchor from "./CHANGELOG.md#010-alpha0" to
"./CHANGELOG.md#010-beta1" (ensure the CHANGELOG heading is updated to "##
[0.1.0-beta.1]" so the anchor matches); edit the line containing the migration
string "**从 `rush-fs` 迁移：** 自 0.1.0-alpha 起主包更名为 `@rush-fs/core`..."
accordingly.
---
Nitpick comments:
In `@__test__/glob.spec.ts`:
- Around line 10-19: makeDirFixture creates temp directories but never removes
them; update it to register cleanup so tests don't leak tmp files: after
creating the base dir in makeDirFixture, register a teardown (e.g., via
afterEach/afterAll or a test framework cleanup helper) that removes the returned
directory using nodeFs.rmSync(base, { recursive: true, force: true }) or
equivalent, or maintain a global list of created bases and remove them in an
afterEach/afterAll handler; reference the makeDirFixture function and the
created base variable when adding the cleanup registration.
cb91348 to
bc268a7
Compare
CoderSerio
force-pushed
the
feat/carbon-dev
branch
from
bc268a7 to
9665726
Compare
CoderSerio
force-pushed
the
feat/carbon-dev
branch
from
9665726 to
d816d8d
Compare
CoderSerio
force-pushed
the
feat/carbon-dev
branch
from
d816d8d to
379fcd7
Compare
CoderSerio
force-pushed
the
feat/carbon-dev
branch
from
379fcd7 to
ab0ff0b
Compare
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.
Description
Type of change
Checklist
.rsfile undersrc/(if adding an API)src/lib.rs(if adding an API)pnpm build:debugpasses with no warnings__test__/(functional + parity with node:fs + error cases, if applicable)pnpm testpassesSee CONTRIBUTING.md for the full development guide.
Summary by CodeRabbit
Chores
Documentation
Bug Fixes