fix(napi): skip debug buffer tracking on wasm targets#3078
fix(napi): skip debug buffer tracking on wasm targets#3078graphite-app[bot] merged 1 commit intomainfrom
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdded WebAssembly exclusion to several debug-only conditional compilation attributes so buffer/ArrayBuffer debug validation blocks and a thread-local buffer declaration are omitted when compiling for WASM targets; no public APIs or function signatures were changed. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (40)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/napi/src/bindgen_runtime/js_values/arraybuffer.rscrates/napi/src/bindgen_runtime/js_values/buffer.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi]` attribute on impl blocks to define classes exported to JavaScript/TypeScript
Applied to files:
crates/napi/src/bindgen_runtime/js_values/buffer.rscrates/napi/src/bindgen_runtime/js_values/arraybuffer.rs
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi(js_name = "...")]` attribute to rename Rust items in JavaScript/TypeScript
Applied to files:
crates/napi/src/bindgen_runtime/js_values/buffer.rscrates/napi/src/bindgen_runtime/js_values/arraybuffer.rs
⏰ 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). (7)
- GitHub Check: aarch64-pc-windows-msvc - node@24 - toolchain@ stable
- GitHub Check: aarch64-pc-windows-msvc - node@22 - toolchain@ stable
- GitHub Check: Build only test - aarch64-unknown-linux-ohos
- GitHub Check: Zig-Cross-Compile-On-Linux (aarch64-apple-darwin)
- GitHub Check: ASAN - ubuntu-24.04
- GitHub Check: Memory leak detect job
- GitHub Check: Cursor Bugbot
🔇 Additional comments (1)
crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs (1)
151-151: LGTM - Consistent WASM exclusion for debug buffer validation.All six cfg attribute changes consistently add
not(target_family = "wasm")to the existing debug assertion conditions forBUFFER_DATAusage sites. The changes correctly prevent buffer tracking checks from compiling on WASM targets.However, these changes depend on the fix for the critical issue in
buffer.rswhere the usage sites also need to be updated to match theBUFFER_DATAdefinition.Also applies to: 232-232, 919-919, 1024-1024, 1638-1638, 1735-1735
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Pull request overview
This PR adds not(target_family = "wasm") to conditional compilation attributes for debug assertion code that uses thread_local! storage. The changes prevent compilation of thread-local buffer tracking code on WASM targets, where thread-local storage is problematic.
- Updated
BUFFER_DATAthread_local declaration in buffer.rs to exclude WASM targets - Updated all 6 usages of
BUFFER_DATAin arraybuffer.rs to exclude WASM targets
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/napi/src/bindgen_runtime/js_values/buffer.rs | Updated cfg attribute on BUFFER_DATA thread_local declaration to exclude WASM targets |
| crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs | Updated cfg attributes on all 6 usages of BUFFER_DATA to exclude WASM targets, ensuring consistency with the declaration in buffer.rs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Brooooooklyn
force-pushed
the
bisect/cfg-only
branch
from
791b076 to
2a58dd5
Compare
Brooooooklyn
changed the title
Brooooooklyn
force-pushed
the
bisect/cfg-only
branch
from
2a58dd5 to
ac6a276
Compare
Brooooooklyn
changed the title
Merge activity
|
<!-- CURSOR_SUMMARY --> > [!NOTE] > Adjusts debug-only buffer tracking to exclude WebAssembly targets. > > - Adds `not(target_family = "wasm")` to `#[cfg(all(debug_assertions, not(windows)))]` guards > - Applies to `BUFFER_DATA` thread_local in `buffer.rs` and duplicate-pointer checks in `arraybuffer.rs` (including typed array slices and `Uint8ClampedSlice`) > - Prevents wasm builds from using debug TLS-based pointer checks/panics > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit ac6a276. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Refined platform-specific compilation settings for WebAssembly builds to optimize build handling. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
graphite-app
bot
force-pushed
the
bisect/cfg-only
branch
from
ac6a276 to
e55bc34
Compare
1 participant
Note
Adjusts debug-only buffer tracking to exclude WebAssembly targets.
not(target_family = "wasm")to#[cfg(all(debug_assertions, not(windows)))]guardsBUFFER_DATAthread_local inbuffer.rsand duplicate-pointer checks inarraybuffer.rs(including typed array slices andUint8ClampedSlice)Written by Cursor Bugbot for commit ac6a276. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.