ci: try TARGET_CC env instead for new napi-rs#182
Merged
Conversation
WalkthroughThis update modifies the GitHub Actions release workflow by renaming an environment variable related to the C compiler and adjusting build steps. It updates dependency versions in Rust and JavaScript configuration files. Additionally, it changes the Changes
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #182 +/- ##
=======================================
Coverage 95.10% 95.10%
=======================================
Files 12 12
Lines 2818 2818
=======================================
Hits 2680 2680
Misses 138 138 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR adjusts CI logic for N-API builds and bumps related package versions:
- Updates
@napi-rs/cliand Rustnapicrates to their latest alpha/beta releases. - Switches the compiler environment variable from
CCtoTARGET_CC. - Comments out version-check gating on build jobs and moves the publish job’s
ifcondition.
Reviewed Changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| package.json | Bumped @napi-rs/cli version to 3.0.0-alpha.92 |
| napi/Cargo.toml | Updated napi and napi-derive crate versions to 3.0.0-beta.12 |
| .github/workflows/release-napi.yml | Replaced CC with TARGET_CC, commented out build if checks, and adjusted publish job gating |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (4)
.github/workflows/release-napi.yml:200
- Misaligned indentation for the
ifkey could cause the condition to be ignored; align it with sibling keys (e.g., add two more spaces) to match theneedsindentation level.
if: needs.check.outputs.version_changed == 'true'
.github/workflows/release-napi.yml:45
- [nitpick] Consider removing or documenting the reason for commenting out this conditional, to avoid confusion and keep the workflow file clean.
# if: needs.check.outputs.version_changed == 'true'
.github/workflows/release-napi.yml:147
- [nitpick] Provide a rationale or remove this commented-out
ifstatement to reduce clutter and clarify CI intent.
# if: needs.check.outputs.version_changed == 'true'
.github/workflows/release-napi.yml:134
- Ensure that downstream build steps and scripts are updated to respect
TARGET_CCinstead of the standardCCvariable; otherwise, the intended compiler override may not take effect.
TARGET_CC: clang # for mimalloc
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 124ca0c in 1 minute and 33 seconds. Click for details.
- Reviewed
119lines of code in4files - Skipped
1files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/release-napi.yml:43
- Draft comment:
Commenting out the version check condition may lead to unintended unguarded builds. Confirm this change is intentional. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The change appears intentional and well-thought-out. The version check is still present in the publish step, which is the critical control point. Running builds unconditionally could be useful for testing/verification purposes. The comment is asking for confirmation rather than pointing out a clear issue. I might be missing security implications of running builds without version checks. There could be resource usage concerns I haven't considered. The build step is just creating artifacts - any security or resource concerns are mitigated by the version check still being present in the publish step, which controls actual releases. The comment should be deleted as it's asking for confirmation rather than pointing out a clear issue, and the change appears to be an intentional restructuring of the workflow controls.
2. .github/workflows/release-napi.yml:134
- Draft comment:
Switching from 'CC' to 'TARGET_CC: clang' aligns with the new napi‑rs expectations. Verify that no other toolchain conflicts arise. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
3. Cargo.lock:556
- Draft comment:
The dependency 'windows-targets' was downgraded from 0.53.2 to 0.52.6. Ensure compatibility with the rest of the toolchain. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. Cargo.lock:623
- Draft comment:
Bumping 'napi', 'napi-derive', and 'napi-derive-backend' to beta.12 appears consistent. Check that all dependent crates are updated accordingly. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%None
5. napi/Cargo.toml:25
- Draft comment:
Updated the 'napi' and 'napi-derive' versions to beta.12. Ensure these changes are in sync with Cargo.lock and downstream consumers. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
6. package.json:31
- Draft comment:
The '@napi-rs/cli' package is updated to 3.0.0-alpha.92; verify compatibility with the changes in napi‑rs. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
Workflow ID: wflow_S36nAuShh838Dq5F
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
CodSpeed Performance ReportMerging #182 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
.github/workflows/release-napi.yml (1)
90-96:TARGET_CCis set, but the RISC-V sub-script still exportsCC– build will pick the wrong compiler.
mimalloc(and the new napi-rs build system) look forTARGET_CC.
Inside the RISC-V build command we still export the legacy vars:export CC=riscv64-linux-gnu-gcc export CXX=riscv64-linux-gnu-g++These no longer have any effect; the step-level env leaves
TARGET_CC=clang, so the cross build will use the host compiler and fail.Diff fix:
- export CC=riscv64-linux-gnu-gcc && - export CXX=riscv64-linux-gnu-g++ && + export TARGET_CC=riscv64-linux-gnu-gcc && + export TARGET_CXX=riscv64-linux-gnu-g++ &&Also applies to: 134-135
🧹 Nitpick comments (2)
.github/workflows/release-napi.yml (2)
45-46: Un-scoping the build job increases CI load.Removing the
if:guard means every push tomainnow triggers the full, very heavy, cross-platform N-API matrix.
If that is intentional – ignore. If not, please restore the condition or switch to a reusable workflow that can be called only when the version changes.
147-148: Same unconditional run for FreeBSD job.The guard is merely commented out, not removed. This reads as dead code and is easy to forget.
Either drop the commented line or re-enable it for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
.github/workflows/release-napi.yml(4 hunks)napi/Cargo.toml(1 hunks)package.json(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: JounQin
PR: unrs/unrs-resolver#144
File: napi/patch.mjs:5-5
Timestamp: 2025-06-10T13:53:02.983Z
Learning: Build-time scripts in napi/patch.mjs and similar build pipeline contexts don't require error handling for reading expected files like index.js, as these files are guaranteed to be present during the build process.
napi/Cargo.toml (6)
Learnt from: JounQin
PR: unrs/unrs-resolver#136
File: src/tests/extension_alias.rs:36-46
Timestamp: 2025-06-05T18:52:30.363Z
Learning: The fixtures/yarn directory in the unrs-resolver project requires running `yarn install` to install TypeScript dependencies before tests can run, similar to other fixture directories like fixtures/pnp. The test in src/tests/extension_alias.rs depends on the installed TypeScript package providing lib/typescript.d.ts and related files.
Learnt from: JounQin
PR: unrs/unrs-resolver#136
File: src/tests/extension_alias.rs:36-46
Timestamp: 2025-06-05T18:52:30.363Z
Learning: The fixtures/yarn directory in the unrs-resolver project requires running `yarn install` to install TypeScript dependencies before tests can run, similar to other fixture directories like fixtures/pnp. The test in src/tests/extension_alias.rs depends on the installed TypeScript package providing lib/typescript.d.ts and related files.
Learnt from: JounQin
PR: unrs/unrs-resolver#129
File: src/tests/pnp.rs:6-6
Timestamp: 2025-06-02T15:57:50.888Z
Learning: In the unrs-resolver project, std::env is acceptable for test cases even when deprecated, as the user prefers simpler approaches in test code compared to production code standards.
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: src/tests/pnp.rs:116-133
Timestamp: 2025-04-21T08:17:34.175Z
Learning: Test cases in unrs-resolver that intentionally document failing behavior should make the intent clear by either: 1) asserting on the expected error condition rather than the desired behavior, 2) using #[should_panic] for tests expected to panic, or 3) using #[ignore] with a reason to skip known failing tests in CI.
Learnt from: JounQin
PR: unrs/unrs-resolver#40
File: src/tests/resolve.rs:121-136
Timestamp: 2025-03-29T04:14:03.715Z
Learning: For tests involving npm/yarn package resolution in the unrs-resolver project, dependencies must be installed first using the appropriate package manager commands (pnpm i, yarn) in both the root directory and fixture directories like fixtures/pnp and fixtures/misc/nested-package-json.
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: src/tests/pnp.rs:116-133
Timestamp: 2025-04-21T08:13:59.358Z
Learning: In the unrs-resolver project, tests that intentionally document failing behavior (regression tests) should either use #[should_panic], assert on the expected error, or include clear documentation indicating they are intentional failing test cases.
package.json (6)
Learnt from: JounQin
PR: unrs/unrs-resolver#144
File: napi/patch.mjs:5-5
Timestamp: 2025-06-10T13:53:02.983Z
Learning: Build-time scripts in napi/patch.mjs and similar build pipeline contexts don't require error handling for reading expected files like index.js, as these files are guaranteed to be present during the build process.
Learnt from: JounQin
PR: unrs/unrs-resolver#136
File: src/tests/extension_alias.rs:36-46
Timestamp: 2025-06-05T18:52:30.363Z
Learning: The fixtures/yarn directory in the unrs-resolver project requires running `yarn install` to install TypeScript dependencies before tests can run, similar to other fixture directories like fixtures/pnp. The test in src/tests/extension_alias.rs depends on the installed TypeScript package providing lib/typescript.d.ts and related files.
Learnt from: JounQin
PR: unrs/unrs-resolver#136
File: src/tests/extension_alias.rs:36-46
Timestamp: 2025-06-05T18:52:30.363Z
Learning: The fixtures/yarn directory in the unrs-resolver project requires running `yarn install` to install TypeScript dependencies before tests can run, similar to other fixture directories like fixtures/pnp. The test in src/tests/extension_alias.rs depends on the installed TypeScript package providing lib/typescript.d.ts and related files.
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: fixtures/pnp/package.json:11-14
Timestamp: 2025-04-21T08:11:41.469Z
Learning: The project uses a justfile with an `install` task that includes running yarn in the fixtures/pnp directory to install dependencies with the command: `cd fixtures/pnp && yarn`.
Learnt from: JounQin
PR: unrs/unrs-resolver#40
File: src/tests/resolve.rs:121-136
Timestamp: 2025-03-29T04:14:03.715Z
Learning: For tests involving npm/yarn package resolution in the unrs-resolver project, dependencies must be installed first using the appropriate package manager commands (pnpm i, yarn) in both the root directory and fixture directories like fixtures/pnp and fixtures/misc/nested-package-json.
Learnt from: JounQin
PR: unrs/unrs-resolver#115
File: tests/resolve_test.rs:251-263
Timestamp: 2025-05-28T02:21:41.547Z
Learning: In pnpm workspaces, `pnpm install` must be run first to initialize dependencies and create the proper node_modules/.pnpm structure before tests can pass. Tests that rely on resolving packages will fail if dependencies haven't been installed yet.
.github/workflows/release-napi.yml (1)
Learnt from: JounQin
PR: unrs/unrs-resolver#144
File: napi/patch.mjs:5-5
Timestamp: 2025-06-10T13:53:02.983Z
Learning: Build-time scripts in napi/patch.mjs and similar build pipeline contexts don't require error handling for reading expected files like index.js, as these files are guaranteed to be present during the build process.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Benchmark
🔇 Additional comments (3)
napi/Cargo.toml (1)
25-26: Dependency bump looks good but remember to update the lock-file.The direct version bump from
beta.11→beta.12is fine, howevercargo update -p napi --workspace(or a fullcargo update) must be run so thatCargo.lockcaptures the new versions; otherwise CI may still restorebeta.11from the lock-file layer.package.json (1)
31-31: Consistent tool-chain bump – LGTM.Nothing else in the JS tool-chain depends on
@napi-rs/clipre-release APIs, so this minor alpha bump is safe..github/workflows/release-napi.yml (1)
197-200:publishnow depends oncheck– good catch.Adding
checktoneedsprevents publishing when the version didn’t change; behaviour matches theif:guard. ✔️
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed ffaaef1 in 1 minute and 48 seconds. Click for details.
- Reviewed
153lines of code in5files - Skipped
1files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/release-napi.yml:134
- Draft comment:
Changed env variable from 'CC' to 'TARGET_CC' for mimalloc. Ensure that all build scripts and downstream tooling expect this update. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that all build scripts and downstream tooling expect the update, which violates the rule against asking the author to ensure behavior is intended or tested. It doesn't provide a specific suggestion or ask for a specific test to be written.
2. napi/Cargo.toml:25
- Draft comment:
Updated napi and napi-derive versions to 3.0.0-beta.12. Confirm that these version bumps are compatible with any changes in the upstream napi‑rs API. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to confirm compatibility with upstream changes, which violates the rule against asking for confirmation or verification. It does not provide a specific code suggestion or point out a specific issue with the code.
3. package.json:23
- Draft comment:
Verify that the 'postinstall' script version (1.11.0) matches the package version to avoid publish discrepancies. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. napi/resolver.wasi.cjs:73
- Draft comment:
Typo in comment: "a worker is consist of two handles: kPublicPort and kHandle." should be corrected to "a worker consists of two handles: kPublicPort and kHandle." - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the grammar correction is technically correct, it's a very minor issue in a comment that doesn't affect code functionality. The meaning of the comment is still clear despite the grammatical error. The rules state we should not make purely informative comments or comments that are obvious/unimportant. The grammar error could potentially confuse non-native English speakers. Also, maintaining high quality documentation is important for code maintainability. While documentation quality is important, this is such a minor grammatical issue that it doesn't significantly impact understanding. The technical content of the comment is clear regardless. Delete this comment as it's purely informative and doesn't affect code functionality or understanding.
Workflow ID: wflow_v8rQCDd5Dxu1DQQI
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
napi/Cargo.toml (1)
25-26: Remember to update theCargo.lockfileEnsure that the
Cargo.lockfile is updated alongside theseCargo.tomlchanges so that the new crate versions are locked and reproducible.
🧹 Nitpick comments (1)
napi/resolver.wasi.cjs (1)
75-90: Add defensive programming for symbol lookupThe current implementation assumes the symbols will always be found, but this may not be guaranteed across all Node.js versions.
Consider adding defensive checks:
{ const kPublicPort = Object.getOwnPropertySymbols(worker).find(s => s.toString().includes("kPublicPort") ); if (kPublicPort) { worker[kPublicPort].ref = () => {}; + } else { + console.warn('Warning: kPublicPort symbol not found. Worker may prevent process exit.'); } const kHandle = Object.getOwnPropertySymbols(worker).find(s => s.toString().includes("kHandle") ); - if (kPublicPort) { + if (kHandle) { worker[kHandle].ref = () => {}; + } else { + console.warn('Warning: kHandle symbol not found. Worker may prevent process exit.'); } worker.unref(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
.github/workflows/release-napi.yml(4 hunks)napi/Cargo.toml(1 hunks)napi/resolver.wasi.cjs(1 hunks)package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/release-napi.yml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: JounQin
PR: unrs/unrs-resolver#72
File: fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json:0-0
Timestamp: 2025-04-21T06:11:46.964Z
Learning: JounQin's fix for the dot alias issue in PR #72 addresses the problem by correctly normalizing paths in the `extend_tsconfig` method, ensuring that base URLs are properly resolved as absolute paths when extending tsconfigs, rather than adding special case handling for dot aliases in the resolver.
Learnt from: JounQin
PR: unrs/unrs-resolver#136
File: src/tests/extension_alias.rs:36-46
Timestamp: 2025-06-05T18:52:30.363Z
Learning: The fixtures/yarn directory in the unrs-resolver project requires running `yarn install` to install TypeScript dependencies before tests can run, similar to other fixture directories like fixtures/pnp. The test in src/tests/extension_alias.rs depends on the installed TypeScript package providing lib/typescript.d.ts and related files.
Learnt from: JounQin
PR: unrs/unrs-resolver#136
File: src/tests/extension_alias.rs:36-46
Timestamp: 2025-06-05T18:52:30.363Z
Learning: The fixtures/yarn directory in the unrs-resolver project requires running `yarn install` to install TypeScript dependencies before tests can run, similar to other fixture directories like fixtures/pnp. The test in src/tests/extension_alias.rs depends on the installed TypeScript package providing lib/typescript.d.ts and related files.
Learnt from: JounQin
PR: unrs/unrs-resolver#144
File: napi/patch.mjs:5-5
Timestamp: 2025-06-10T13:53:02.983Z
Learning: Build-time scripts in napi/patch.mjs and similar build pipeline contexts don't require error handling for reading expected files like index.js, as these files are guaranteed to be present during the build process.
napi/Cargo.toml (6)
Learnt from: JounQin
PR: unrs/unrs-resolver#136
File: src/tests/extension_alias.rs:36-46
Timestamp: 2025-06-05T18:52:30.363Z
Learning: The fixtures/yarn directory in the unrs-resolver project requires running `yarn install` to install TypeScript dependencies before tests can run, similar to other fixture directories like fixtures/pnp. The test in src/tests/extension_alias.rs depends on the installed TypeScript package providing lib/typescript.d.ts and related files.
Learnt from: JounQin
PR: unrs/unrs-resolver#136
File: src/tests/extension_alias.rs:36-46
Timestamp: 2025-06-05T18:52:30.363Z
Learning: The fixtures/yarn directory in the unrs-resolver project requires running `yarn install` to install TypeScript dependencies before tests can run, similar to other fixture directories like fixtures/pnp. The test in src/tests/extension_alias.rs depends on the installed TypeScript package providing lib/typescript.d.ts and related files.
Learnt from: JounQin
PR: unrs/unrs-resolver#129
File: src/tests/pnp.rs:6-6
Timestamp: 2025-06-02T15:57:50.888Z
Learning: In the unrs-resolver project, std::env is acceptable for test cases even when deprecated, as the user prefers simpler approaches in test code compared to production code standards.
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: src/tests/pnp.rs:116-133
Timestamp: 2025-04-21T08:17:34.175Z
Learning: Test cases in unrs-resolver that intentionally document failing behavior should make the intent clear by either: 1) asserting on the expected error condition rather than the desired behavior, 2) using #[should_panic] for tests expected to panic, or 3) using #[ignore] with a reason to skip known failing tests in CI.
Learnt from: JounQin
PR: unrs/unrs-resolver#40
File: src/tests/resolve.rs:121-136
Timestamp: 2025-03-29T04:14:03.715Z
Learning: For tests involving npm/yarn package resolution in the unrs-resolver project, dependencies must be installed first using the appropriate package manager commands (pnpm i, yarn) in both the root directory and fixture directories like fixtures/pnp and fixtures/misc/nested-package-json.
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: src/tests/pnp.rs:116-133
Timestamp: 2025-04-21T08:13:59.358Z
Learning: In the unrs-resolver project, tests that intentionally document failing behavior (regression tests) should either use #[should_panic], assert on the expected error, or include clear documentation indicating they are intentional failing test cases.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Benchmark
🔇 Additional comments (1)
napi/Cargo.toml (1)
25-27: Dependency updates look goodThe updates to
napiandnapi-deriveto version3.0.0-beta.12are properly aligned, and the new optionaltracing-subscriberdependency is correctly configured with minimal features to reduce binary size.
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed d9c2778 in 1 minute and 57 seconds. Click for details.
- Reviewed
162lines of code in5files - Skipped
1files when reviewing. - Skipped posting
8draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/release-napi.yml:92
- Draft comment:
Switch from CC to TARGET_CC for the riscv build; ensure the build tools honor TARGET_CC. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. .github/workflows/release-napi.yml:134
- Draft comment:
Use TARGET_CC (not CC) for mimalloc; verify that dependent builds expect TARGET_CC consistently. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%<= threshold50%The comment suggests using TARGET_CC instead of CC for mimalloc, which is a specific code suggestion. However, it also asks to verify that dependent builds expect TARGET_CC consistently, which is against the rules as it asks for verification. The first part of the comment is useful, but the second part is not allowed.
3. .github/workflows/release-napi.yml:44
- Draft comment:
Disabled version check condition may trigger builds on every commit; confirm this is intentional. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. Cargo.lock:556
- Draft comment:
Downgrade of windows-targets from 0.53.2 to 0.52.6; verify that this change is compatible with other dependencies. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to verify compatibility with other dependencies, which is against the rules. It does not provide a specific suggestion or ask for a specific test to be written. It also comments on a dependency change, which is not allowed unless it provides a specific actionable insight.
5. Cargo.lock:625
- Draft comment:
Bump of NAPI-related crates to beta.12; ensure API changes are compatible with existing consumers. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is related to dependency changes, specifically bumping NAPI-related crates to a new version. The comment suggests ensuring API changes are compatible with existing consumers, which falls under the rule of not asking the PR author to ensure compatibility or test changes. Therefore, this comment should be removed.
6. napi/Cargo.toml:25
- Draft comment:
NAPI dependencies updated to beta.12; confirm this aligns with Cargo.lock and the overall API. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm alignment with Cargo.lock and the overall API, which violates the rule against asking for confirmation or verification. It does not provide a specific suggestion or point out a specific issue.
7. package.json:31
- Draft comment:
Updated @napi-rs/cli to version 3.0.0-alpha.92; verify the CLI changes don't adversely affect the build process. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to verify that the CLI changes don't adversely affect the build process, which is against the rules. It is not making a specific code suggestion or asking for a specific test to be written. Therefore, it should be removed.
8. napi/resolver.wasi.cjs:73
- Draft comment:
Typo in comment: "a worker is consist of two handles: kPublicPort and kHandle." should be "a worker consists of two handles: kPublicPort and kHandle." - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about the grammar, it's a very minor issue in a comment that doesn't affect code functionality. The existing comment is still perfectly understandable despite the grammatical error. Comments about pure documentation/grammar issues that don't affect code understanding should generally be avoided unless they significantly impact clarity. The grammar error could potentially confuse non-native English speakers. Also, maintaining high quality even in comments shows attention to detail. The meaning is still clear despite the minor grammatical error, and we should focus review comments on substantive code issues rather than minor documentation fixes. Delete this comment as it addresses a trivial grammatical issue that doesn't impact code understanding or functionality.
Workflow ID: wflow_QbxxZuTpJy5RnPFV
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed b3e9434 in 1 minute and 22 seconds. Click for details.
- Reviewed
138lines of code in5files - Skipped
1files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. napi/resolver.wasi.cjs:73
- Draft comment:
Typographical error: In the comment "a worker is consist of two handles", it should read "a worker consists of two handles". - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold50%This comment is pointing out a typographical error in a comment within the code. While it is not directly related to the functionality of the code, correcting typographical errors can improve code readability and maintainability. However, it does not suggest a change in the code logic or functionality, nor does it ask for tests or confirmation of intent. It is more of an informative comment about a typo.
Workflow ID: wflow_wg5WyHxJDUrDLN4C
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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
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.
Important
Switch to
TARGET_CCfor compiler specification and update dependencies for improved build stability and worker thread handling.CCtoTARGET_CCin.github/workflows/release-napi.ymlfor specifying the compiler.Cargo.lockto usenapiversion3.0.0-beta.12andnapi-deriveversion3.0.0-beta.12.@napi-rs/clito3.0.0-alpha.92andemnapito1.4.4inpackage.json.resolver.wasi.cjsto improve worker thread handling, preventing process hangs by unreferencing workers.This description was created by
for b3e9434. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit