Remove wasm-pack from dev tooling, replacing it with custom tooling#4228
Open
Keavon wants to merge 5 commits into
Open
Remove wasm-pack from dev tooling, replacing it with custom tooling#4228Keavon wants to merge 5 commits into
Keavon wants to merge 5 commits into
Conversation
…ributes with graphic List<T> types (#4111) * Add conversion from Fill to Table<Graphic> * Refactor Vector vello renderer for Gradient / Color # Conflicts: # node-graph/libraries/rendering/src/renderer.rs * Refactor Vector SVG renderer for Gradient / Color * Fix conflicts * Add basic clipping-based fill for SVG rendering * Use Cow to avoid cloning graphic list for fill * Cleanup for Cow usage * format code * Use `<pattern>` instead of `<clipPath>` for clip This simplifies the future implementation of clipping-based rendering for strokes, as the stroke does not support the use of a clip path but rather paint sources from a paint server. * Move svg pattern rendering function to RenderExt * Fix comment * Fix empty fill list rendering as default black Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> * Move opaque check function to Graphic impl * Add color converter and debug node to use graphic * WIP: Use List<Graphic> to render Color & Gradient * Use `Arc<List<Vector>>` for vector_data metadata This exposes List's attributes to message handlers, enabling them to access the necessary attribute data such as ATTR_STROKE_PAINT_GRAPHIC as `Fill` and `Stroke` will not have paint information in the future. * Recurse opacity checks on nested `Graphic` Also extracts `fill_graphic_list_at` / `stroke_paint_graphic_list_at` to share the row-attribute lookup across the existing call sites. * Fix fill and stroke visibility check degradation * Fix clipping based stroke paint positioning * Refactor vello renderer for stroke to use graphic * Reduce `Fill` / `Stroke.color` to `List<Graphic>` allocations * Revert "Use `Arc<List<Vector>>` for vector_data metadata" This reverts commit 4285243 * Expose paint row attributes as dedicated metadata for vectors Add `fill_attributes` / `stroke_paint_attributes` to `DocumentMetadata` so the `ExpandFillStrokeOnSelectedLayers` handler can read row paint visibility without exposing entire `List<Vector>`. * Fix transparency check to consider fill opacity * Fix consistency of gradient placement for SVG stroke * Rename `stroke_paint_..` to `stroke_..` * Remove debug nodes * Allow to use any graphic type without casting * Rename `fill_graphic` / `stroke_graphic` to `fill` / `stroke` * Fix SVG pattern placement when stroke transform differs from item transform * Fix click target fill check for empty list in graphic * Fix blank fill/stroke attribute masking legacy style * Fix SVG's paint order trick for vector/raster fills * Add zero-division guard for pattern wraparound prevention * Code review --------- Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> Co-authored-by: Keavon Chambers <keavon@keavon.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 13 files
Confidence score: 3/5
- In
tools/cargo-run/src/wasm.rs,target_dir()andwatch_shell_commands()use inconsistent target-path logic (CARGO_TARGET_DIRnot normalized in one place, hardcoded../../targetin another), so builds/watch mode can look for wasm artifacts in the wrong location and fail for configured workspaces—unify both call sites on a single workspace-root-aware target-dir resolver before merging. - In
tools/cargo-run/src/requirements.rs, the platform check treats Windows ARM64 as unsupported for Binaryen, which can cause wasm-opt auto-install to fail on a supported platform and block developer workflows—update the support matrix/detection logic for Windows ARM64 and verify with a platform-specific test path. - In
tools/cargo-run/src/requirements.rs, Binaryen archives are downloaded and extracted without checksum verification, so a tampered artifact could be installed into the managed toolchain path—add checksum/signature validation for release assets before extraction prior to merge. - In
tools/cargo-run/src/lib.rs, Windows Ctrl+C state is not reset between runs, which can make later dev-server executions appear successful when they were interrupted—reset the Ctrl+C flag during shutdown/startup to avoid false-success reporting.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request replaces wasm-pack with a custom Rust-based build process in tools/cargo-run that directly manages cargo build, wasm-bindgen, and wasm-opt (Binaryen) installations. It also improves process supervision and Ctrl+C handling on Windows. Feedback on these changes highlights two issues: first, hardcoding the relative path ../../target in watch_shell_commands will break builds when a custom CARGO_TARGET_DIR is configured; second, SetConsoleCtrlHandler could register duplicate handlers if install_handler is invoked multiple times, which can be resolved using std::sync::Once.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…duction The devDependencies hold the build tooling (Vite, etc.), which npm omits in environments that set NODE_ENV=production, like CI does for the Vite build. The install timestamp check now also covers package-installer.js itself so changes to the install process trigger a reinstall.
Member
Author
|
!build desktop (Run ID 27447741636) |
|
|
|
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
2 participants
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.
Closes #2919