Repository navigation
fix(wework): reduce abnormal CPU usage from composer input - #2407
sdadunderscoresdad wants to merge 22 commits into
Conversation
- Unwrap Windows PowerShell wrapper commands in toolBlockActivity so that collapsed tool labels show search/read summaries instead of the raw pwsh path. - Add semantic command labels for file reads and git status in ToolBlockItem. - Introduce executor/src/local/git.rs with a cross-platform GitCommandRunner that invokes git directly via argv on Windows, avoiding cmd /C hangs. - Route all git_* device commands through native git calls on Windows and extend PATH with common Git installation directories. - Add i18n keys and unit tests for both frontend label summarization and executor git detection.
…apping in ToolBlockItem
- Remove dead run_git_async and fix git_stdout to run git only once. - Scope Windows-only PathBuf import and git path helpers with cfg(windows). - Keep DEFAULT_DEVICE_ID cross-platform and use NUL on Windows. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Introduce getInnerShellCommand to unify shell wrapper unwrapping. - Share regex constants and improve Get-ChildItem classification. - Update stale test expectations and clean up blank lines. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Avoid syncing draft input to React state on every keystroke. - Sync draft to parent on blur, compositionend, and submit instead. - Defer autocomplete trigger detection and debug snapshot updates. - Use ComposerTextarea ref for submit value so stale value prop does not matter. - Fix placeholder visibility by tracking editor internal content state. - Wrap non-urgent state updates in startTransition.
…nto fix/wework-abnormal-cpu-usage
The deferred sync introduced in the composer perf commit caused the controlled value prop to drift from ProseMirror's internal state. This broke insertion appending, scope-switch composer reset, and submit clearing. Restore per-keystroke state and onChange sync while keeping the ComposerTextarea-level optimizations intact. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Keep BufferedChatInput's local draftState synchronous so the controlled value prop stays in sync with ProseMirror. Defer the parent onChange via a 300ms debounce to avoid re-rendering the Workbench tree on every keystroke. Flush the draft immediately on blur, compositionend, submit, and scope change. Update the one test that expected per-keystroke onChange to trigger blur and wait for the flush. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…eact render path Expose getValue/setValue through forwardRef on ChatInput, CompactChatComposer, and ProjectChatComposer so BufferedChatInput can update the editor directly without forcing parent re-renders on every keystroke. Keeps the existing tests passing by syncing external value changes and flushing pending drafts on blur. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- BufferedChatInput: keep local draft state in sync while still debouncing parent onChange; flush pending draft only on scope change/unmount. - CompactChatComposer: use the composer handle value for interrupt-and-send. - toolBlockActivity: loop stripShellPrefix to handle quoted paths and chains. - ToolBlockItem: unwrap the full shell command instead of only the first line. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add async wrappers around the synchronous git runner and await them in the Windows builtin device command handler. This prevents git subprocess I/O from blocking the tokio runtime threads. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… typing performance The synchronous setDraftState update in the previous commit reintroduced per-keystroke React re-renders and undid the ref-based composer handle optimization, causing input lag. Restore the version that keeps typing off the render path while keeping the other composer/label fixes intact. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…-cpu-usage # Conflicts: # wework/src/components/chat/ChatInput.tsx # wework/src/components/chat/composer/ProjectChatComposer.tsx
📝 WalkthroughWalkthroughThis PR adds asynchronous Git handling for Windows, ref-based chat composer synchronization, PowerShell and Git command classification, debounced draft and workbench snapshot updates, localized activity labels, and Tauri configuration reformatting. ChangesLocal Git Execution
Chat Composer State
Tool Activity Classification
Workbench and Configuration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WindowsBuiltins
participant GitHelpers
participant GitProcess
WindowsBuiltins->>GitHelpers: run_git_async(arguments, environment)
GitHelpers->>GitProcess: execute Git with sanitized environment
GitProcess-->>GitHelpers: stdout, stderr, exit status, duration
GitHelpers-->>WindowsBuiltins: CommandResult
sequenceDiagram
participant ChatInput
participant Composer
participant ComposerTextarea
participant ProseMirrorEditor
ChatInput->>Composer: forward value and lifecycle callbacks
Composer->>ComposerTextarea: forward ref and callbacks
ComposerTextarea->>ProseMirrorEditor: read or set editor value
ProseMirrorEditor-->>ComposerTextarea: content snapshot and blur event
ComposerTextarea-->>ChatInput: imperative value handle
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
executor/src/local/git.rs (3)
132-138: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
is_worktreereturns true for locations that are not work trees.The
||fallback accepts any path whererev-parse --git-dirsucceeds. That includes a bare repository and a plain directory nested under an unrelated repository, because Git walks up to the parent. The first check already answers the question, so the fallback only widens the result.If the fallback exists to cover an older Git version, document that reason. Otherwise rely on
--is-inside-work-treealone.♻️ Proposed simplification
pub fn is_worktree(path: &str, extra_env: &HashMap<String, String>) -> bool { git_stdout(path, &["rev-parse", "--is-inside-work-tree"], extra_env) .map(|output| output == "true") .unwrap_or(false) - || git_stdout(path, &["rev-parse", "--git-dir"], extra_env).is_some() }Apply the same change to
is_worktree_asyncat lines 216-224.As per coding guidelines: "Delete dead code and do not add compatibility shims or fallback paths without agreement; correct the primary path."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/src/local/git.rs` around lines 132 - 138, Remove the `git_stdout(..., &["rev-parse", "--git-dir"], ...)` fallback from both `is_worktree` and `is_worktree_async`, relying solely on `rev-parse --is-inside-work-tree` so bare repositories and directories nested under unrelated repositories return false.Source: Coding guidelines
319-335: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
run_git_or_panicand make setup failures visible.
run_git_or_panicnever panics. It returns stdout and discards the exit code and stderr. Ifgit initfails,detects_worktreefails later with no diagnostic output.Assert success in the helper so the failure names the failing Git command.
💚 Proposed test helper fix
- fn run_git_or_panic(cwd: &str, args: &[&str]) -> String { - run_git(cwd, args, &HashMap::new()).stdout - } + fn run_git_or_panic(cwd: &str, args: &[&str]) -> String { + let output = run_git(cwd, args, &HashMap::new()); + assert!( + output.success(), + "git {args:?} failed: exit={:?} stderr={}", + output.exit_code, + output.stderr + ); + output.stdout + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/src/local/git.rs` around lines 319 - 335, Update the test helper run_git_or_panic to assert that the Git command succeeds, including the command’s stderr or equivalent diagnostic in the assertion, and rename it to reflect that it validates success rather than panicking. Keep detects_worktree using this helper for git init so setup failures identify the failing command.
119-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused synchronous Git helpers
git_stdout,is_worktree,resolve_merge_base, andgit_rev_parse_verifyare only used withinexecutor/src/local/git.rstests or by the async wrapper paths. Keep the async Git helpers plusrun_git, and remove the synchronous helpers so callers do not maintain duplicated logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/src/local/git.rs` around lines 119 - 173, Remove the synchronous helper functions git_stdout, is_worktree, resolve_merge_base, and git_rev_parse_verify from the Git module, along with any tests or wrapper paths that depend exclusively on them. Preserve run_git and the async Git helpers, and update remaining callers to use the async implementations rather than duplicated synchronous logic.Source: Coding guidelines
executor/src/local/app_ipc.rs (2)
1594-1601: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd the
--separator consistently to the merge-base diff arguments.In
git_branch_diff_shortstattheNonebranch uses["diff", "--shortstat", "HEAD", "--"], but theSome(base)branch omits--.git_branch_diffat lines 1619-1622 repeats the same asymmetry. Without--, Git can treat the revision argument ambiguously when a path of the same name exists.Append
"--"in bothSome(base)branches so all four argument lists follow one form.♻️ Proposed change
let args: Vec<&str> = match base.as_deref() { - Some(base) => vec!["diff", "--shortstat", base], + Some(base) => vec!["diff", "--shortstat", base, "--"], None => vec!["diff", "--shortstat", "HEAD", "--"], };Also applies to: 1619-1622
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/src/local/app_ipc.rs` around lines 1594 - 1601, Update both `git_branch_diff_shortstat` and `git_branch_diff` so their `Some(base)` argument lists append the `"--"` separator after the merge-base revision, matching the existing `None` branches and ensuring all diff invocations use the same revision/path boundary.
1631-1648: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the untracked-file diff loop into one helper.
Lines 1631-1648 in
git_branch_diffand lines 1670-1687 ingit_diffare identical: list untracked files, then run onegit diff --no-indexper file and append successful output. Therev-parse --verify HEADprobe plus tracked-diff selection is also near-identical.Extract a single async helper, for example
append_untracked_diffs(path, env, &mut output), and call it from both arms. That keeps one code path when the diff format changes.Note one scaling property while you are there: the loop spawns one Git process per untracked file. A repository with many unignored files produces many sequential process spawns on a single IPC request. Consider capping the number of files or the total output size.
As per coding guidelines: "Before adding code, search for and reuse existing components, services, utilities, and patterns; extract shared logic instead of duplicating it."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/src/local/app_ipc.rs` around lines 1631 - 1648, Extract the duplicated untracked-file processing from git_branch_diff and git_diff into one async append_untracked_diffs helper that lists untracked files, runs the no-index diff, and appends successful output. Replace both inline loops with calls to this helper, preserving existing behavior, and add a reasonable cap on processed files or accumulated output to prevent excessive sequential Git processes per IPC request.Source: Coding guidelines
wework/src/components/layout/BufferedChatInput.test.tsx (1)
69-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the debounce window and for composition end.
This test now proves that blur flushes the draft. Two new behaviors remain untested:
onChangemust not fire during the 300 ms window, andonCompositionEndmust flush. The debounce is the central behavior of this PR, so a regression that removes the delay or the composition flush would pass the current suite.Add one test with fake timers that types, asserts
onChangewas not called, advances 300 ms, and then asserts the call. Add a second test that firescompositionEndinstead ofblur.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/components/layout/BufferedChatInput.test.tsx` around lines 69 - 75, Add coverage in the BufferedChatInput tests for both debounce timing and composition flushing: use fake timers to type a draft, verify onChange has not fired during the 300 ms window, advance 300 ms, then verify the change; add a separate test that triggers compositionEnd instead of blur and verifies onChange receives the draft.wework/src/components/layout/BufferedChatInput.tsx (1)
76-85: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThis cleanup runs on every
valuechange, so it emits a duplicateonChangeafter each flush.The effect lists
valuein its dependencies, so React runs the cleanup on everyvaluechange and not only on unmount. Trace one flush: the timer callsonChange(d); the parent re-renders withvalue === d; the cleanup for the previousvaluethen compares that oldervalueagainstdraftRef.current, which already holdsd, so it callsonChange(d)a second time.The extra call is idempotent, so no draft is lost. It does add one redundant parent update per flush, which works against the CPU goal of this PR. Read
valuethrough a ref and keep the effect dependencies empty so the flush runs only on unmount.♻️ Proposed refactor
+ const valueRef = useRef(value) + valueRef.current = value + // Flush pending draft when value changes or on unmount. useEffect(() => { return () => { const pendingDraft = draftRef.current - if (pendingDraft !== value) { + if (pendingDraft !== valueRef.current) { cancelPendingFlush() - onChange(pendingDraft) + onChangeRef.current(pendingDraft) } } - }, [cancelPendingFlush, onChange, value]) + }, [cancelPendingFlush])This needs an
onChangeRefthat mirrorsonChange, so the unmount path always calls the latest callback. Also update the comment, because the effect then flushes on unmount only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/components/layout/BufferedChatInput.tsx` around lines 76 - 85, Update the cleanup effect in BufferedChatInput to run only on unmount by removing value-dependent reruns and using a ref that mirrors the latest value for comparison; also add an onChangeRef mirroring the latest callback so unmount uses current props. Update the comment to state that pending drafts flush on unmount only, while preserving cancelPendingFlush and avoiding duplicate onChange calls after timer flushes.wework/src/components/chat/ChatInput.tsx (1)
297-305: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThree forwarded handles depend on
[value]only to support a pre-mount fallback. EachuseImperativeHandlereadscomposerRef.currentfirst and falls back tovalue. The child composer is always mounted after the first commit, so the fallback only applies before mount, yet the[value]dependency rebuilds the handle object on every value change.ComposerTextareaat lines 127-137 already shows the stable pattern: keep the latest value in a ref and use an empty dependency array.
wework/src/components/chat/ChatInput.tsx#L297-L305: storevaluein avalueRef, read it ingetValue, and change the dependency array to[].wework/src/components/chat/composer/ProjectChatComposer.tsx#L174-L182: apply the samevalueReffallback and empty dependency array.wework/src/components/chat/composer/CompactChatComposer.tsx#L156-L164: apply the samevalueReffallback and empty dependency array.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/components/chat/ChatInput.tsx` around lines 297 - 305, Stabilize all three forwarded imperative handles by storing the latest value in a valueRef and reading that ref only for the pre-mount fallback, then use an empty dependency array. Apply this in ChatInput.tsx lines 297-305, ProjectChatComposer.tsx lines 174-182, and CompactChatComposer.tsx lines 156-164; update each getValue implementation while preserving the composerRef.current-first behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@executor/src/local/app_ipc.rs`:
- Around line 1543-1563: Update the git_push branch-discovery flow around
run_git_async so it checks the branch command’s success status before treating
empty stdout as detached HEAD. For command failures, return the actual Git error
through the existing command-result path; only emit “Cannot push detached HEAD”
when the command succeeds with an empty branch name.
In `@executor/src/local/git.rs`:
- Around line 29-32: Harden run_git so Git commands cannot block indefinitely:
spawn the process, poll for completion with an explicit deadline, and on expiry
kill it and return a timed-out GitOutput. Configure the command with
stdin(Stdio::null()) and GIT_TERMINAL_PROMPT=0 to prevent interactive credential
prompts, and remove or update the stale timeout documentation above
WINDOWS_GIT_PATHS to match the implemented behavior.
- Around line 91-97: The prepend_windows_git_paths function reads from the
executor process PATH via std::env::var, which overwrites the PATH set by
build_env(extra_env). Refactor prepend_windows_git_paths to accept the command
as a parameter and read the current PATH value from the command's environment
(which was set by build_env), then append the Git directories and set the
combined value back on the command, avoiding any read from the executor's own
environment variables.
In `@wework/src/components/chat/blocks/toolBlockActivity.ts`:
- Around line 727-729: Update SHELL_EXECUTABLE_RE in
wework/src/components/chat/blocks/toolBlockActivity.ts:727-729 to accept both
forward- and backslash-separated shell paths while preserving existing
executable matching. In
wework/src/components/chat/blocks/toolBlockActivity.test.ts:220-232, replace the
duplicate PowerShell assertions with coverage for /bin/zsh -c or /bin/bash -c
wrappers.
In `@wework/src/components/chat/blocks/ToolBlockItem.tsx`:
- Around line 1011-1017: Update the Git command parsing near the subcommand
detection to tokenize with splitShellWords instead of trim().split(/\s+/), then
skip supported Git global options and their associated values before assigning
subcommand. Preserve the dedicated genericLabels.gitStatus result for status
commands and compact formatting for other commands, including paths containing
spaces.
In `@wework/src/components/chat/composer/CompactChatComposer.tsx`:
- Around line 533-536: Use separate refs for the compact and fullscreen
ComposerTextarea instances instead of sharing composerRef. Add a
fullscreen-specific composer handle ref for the instance near
fullscreenInputOpen, then select that ref when fullscreenInputOpen is true and
composerRef otherwise so parent methods always target the active editor,
including submit and setValue operations.
In `@wework/src/components/chat/composer/ProjectChatComposer.tsx`:
- Around line 227-252: The insertion handlers must use the live composer content
instead of the debounced value prop. In
wework/src/components/chat/composer/ProjectChatComposer.tsx lines 227-252,
update handleShowTextAttachment and handleQuickPhraseSelect to read
composerRef.current?.getValue() ?? value before appending, and use that value
for applyWorkspacePathTransfer in handleDrop at line 224. In
wework/src/components/chat/composer/CompactChatComposer.tsx lines 192-217, apply
the same live-value lookup in the attachment and quick-phrase insertion
handlers.
- Around line 306-318: Update the ProjectChatComposer onSubmit handler to
compute the exact value from composerRef.current?.getValue() ?? value before
calling debugComposerEvent, then use that same submitted value for
submittedValue and the subsequent onSubmit call. Keep the existing canSend guard
and other diagnostics unchanged.
---
Nitpick comments:
In `@executor/src/local/app_ipc.rs`:
- Around line 1594-1601: Update both `git_branch_diff_shortstat` and
`git_branch_diff` so their `Some(base)` argument lists append the `"--"`
separator after the merge-base revision, matching the existing `None` branches
and ensuring all diff invocations use the same revision/path boundary.
- Around line 1631-1648: Extract the duplicated untracked-file processing from
git_branch_diff and git_diff into one async append_untracked_diffs helper that
lists untracked files, runs the no-index diff, and appends successful output.
Replace both inline loops with calls to this helper, preserving existing
behavior, and add a reasonable cap on processed files or accumulated output to
prevent excessive sequential Git processes per IPC request.
In `@executor/src/local/git.rs`:
- Around line 132-138: Remove the `git_stdout(..., &["rev-parse", "--git-dir"],
...)` fallback from both `is_worktree` and `is_worktree_async`, relying solely
on `rev-parse --is-inside-work-tree` so bare repositories and directories nested
under unrelated repositories return false.
- Around line 319-335: Update the test helper run_git_or_panic to assert that
the Git command succeeds, including the command’s stderr or equivalent
diagnostic in the assertion, and rename it to reflect that it validates success
rather than panicking. Keep detects_worktree using this helper for git init so
setup failures identify the failing command.
- Around line 119-173: Remove the synchronous helper functions git_stdout,
is_worktree, resolve_merge_base, and git_rev_parse_verify from the Git module,
along with any tests or wrapper paths that depend exclusively on them. Preserve
run_git and the async Git helpers, and update remaining callers to use the async
implementations rather than duplicated synchronous logic.
In `@wework/src/components/chat/ChatInput.tsx`:
- Around line 297-305: Stabilize all three forwarded imperative handles by
storing the latest value in a valueRef and reading that ref only for the
pre-mount fallback, then use an empty dependency array. Apply this in
ChatInput.tsx lines 297-305, ProjectChatComposer.tsx lines 174-182, and
CompactChatComposer.tsx lines 156-164; update each getValue implementation while
preserving the composerRef.current-first behavior.
In `@wework/src/components/layout/BufferedChatInput.test.tsx`:
- Around line 69-75: Add coverage in the BufferedChatInput tests for both
debounce timing and composition flushing: use fake timers to type a draft,
verify onChange has not fired during the 300 ms window, advance 300 ms, then
verify the change; add a separate test that triggers compositionEnd instead of
blur and verifies onChange receives the draft.
In `@wework/src/components/layout/BufferedChatInput.tsx`:
- Around line 76-85: Update the cleanup effect in BufferedChatInput to run only
on unmount by removing value-dependent reruns and using a ref that mirrors the
latest value for comparison; also add an onChangeRef mirroring the latest
callback so unmount uses current props. Update the comment to state that pending
drafts flush on unmount only, while preserving cancelPendingFlush and avoiding
duplicate onChange calls after timer flushes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c01ed3a-85a9-493a-ba12-09047d8131ce
📒 Files selected for processing (22)
executor/src/local/app_ipc.rsexecutor/src/local/git.rsexecutor/src/local/mod.rswework/src-tauri/tauri.conf.jsonwework/src/components/chat/ChatInput.tsxwework/src/components/chat/blocks/ToolBlockItem.test.tsxwework/src/components/chat/blocks/ToolBlockItem.tsxwework/src/components/chat/blocks/ToolBlocksDisplay.test.tsxwework/src/components/chat/blocks/toolBlockActivity.test.tswework/src/components/chat/blocks/toolBlockActivity.tswework/src/components/chat/composer/CompactChatComposer.tsxwework/src/components/chat/composer/ComposerProseMirrorEditor.tsxwework/src/components/chat/composer/ComposerTextarea.tsxwework/src/components/chat/composer/ProjectChatComposer.tsxwework/src/components/chat/composer/composerTextareaTypes.tswework/src/components/chat/composer/useAutoResizeTextarea.tswework/src/components/layout/BufferedChatInput.test.tsxwework/src/components/layout/BufferedChatInput.tsxwework/src/components/layout/useWorkbenchPaneSession.tswework/src/features/workbench/WorkbenchProvider.tsxwework/src/i18n/locales/en/chat.jsonwework/src/i18n/locales/zh-CN/chat.json
| /// Maximum time we are willing to wait for a single Git device command. | ||
| /// Common installation directories for Git on Windows. These are appended to the process PATH | ||
| /// so that `git` can be resolved even when the executor is started from an environment that | ||
| /// does not include them. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Remove the stale timeout doc line, or add the timeout it describes.
Line 29 documents a "Maximum time we are willing to wait for a single Git device command", but no timeout constant exists and run_git calls command.output() without any deadline. The doc comment also attaches to WINDOWS_GIT_PATHS, which is unrelated.
This is more than a comment defect. The module header states the previous failure mode was hanging Git calls and "command timed out" errors. run_git can now block forever, for example when a credential helper waits for input. Through run_git_async each hang consumes a Tokio blocking-pool thread permanently, so repeated Git device commands can exhaust the pool.
Add an explicit wait bound, for example spawn with Command::spawn, poll with a deadline, then kill and report a timed-out GitOutput. Also set stdin(Stdio::null()) and GIT_TERMINAL_PROMPT=0 so Git fails fast instead of waiting for input.
🔧 Minimum hardening for the spawn path
let mut command = Command::new("git");
command
.arg("-C")
.arg(cwd)
.args(args)
.env_clear()
.envs(build_env(extra_env))
+ .env("GIT_TERMINAL_PROMPT", "0")
+ .stdin(Stdio::null())
.stdout(Stdio::piped())
.stderr(Stdio::piped());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor/src/local/git.rs` around lines 29 - 32, Harden run_git so Git
commands cannot block indefinitely: spawn the process, poll for completion with
an explicit deadline, and on expiry kill it and return a timed-out GitOutput.
Configure the command with stdin(Stdio::null()) and GIT_TERMINAL_PROMPT=0 to
prevent interactive credential prompts, and remove or update the stale timeout
documentation above WINDOWS_GIT_PATHS to match the implemented behavior.
| .env_clear() | ||
| .envs(build_env(extra_env)) | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::piped()); | ||
|
|
||
| #[cfg(windows)] | ||
| prepend_windows_git_paths(&mut command); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect build_env to see how it handles PATH.
fd -t f 'command.rs' executor/src | while IFS= read -r f; do
ast-grep outline "$f" --match 'build_env'
done
rg -nP -C 15 'fn build_env' executor/src
rg -nP -C 3 '"PATH"|PATH' executor/src/local/command.rsRepository: wecode-ai/Wegent
Length of output: 2198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate process_env implementation"
rg -n -C 15 'process_environment::process_env|fn process_env|process_env\(' executor/src
echo
echo "Search path handling across executor/src"
rg -n -C 5 'PATH|path|Paths|prepend_windows_git_paths|build_env' executor/src -g '*.rs'
echo
echo "Inspect executor/src/local/git.rs around run_git"
sed -n '1,150p' executor/src/local/git.rs
echo
echo "Inspect executor/src/local/command.rs around process_command/build_env"
sed -n '200,260p' executor/src/local/command.rsRepository: wecode-ai/Wegent
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files containing process_environment"
rg -n --hidden 'process_environment' Cargo.toml Cargo.lock executor/Cargo.toml 2>/dev/null || true
rg -n 'process_environment::process_env|use std::process::environment|process_environment' Cargo.lock 2>/dev/null | head -50
echo
echo "Process env implementation if present in lock"
rg -n --hidden 'name = "process_environment"|version =.' Cargo.lock | head -100
echo
echo "Locate git.rs"
fd -t f 'git.rs' executor/src
echo
if [ -f executor/src/local/git.rs ]; then
wc -l executor/src/local/git.rs
sed -n '1,140p' executor/src/local/git.rs
fi
echo
echo "Local command.rs env/build_env context"
if [ -f executor/src/local/command.rs ]; then
sed -n '1,80p;220,245p;300,380p' executor/src/local/command.rs
fi
echo
echo "Exact prepend_windows_git_paths references"
rg -n -C 8 'prepend_windows_git_paths|run_git|env_clear|build_env' executor/src -g '*.rs' | head -200Repository: wecode-ai/Wegent
Length of output: 191
🌐 Web query:
Rust std::process::Command::envs HashMap iter order envs before env PATH overwrites
💡 Result:
In Rust's std::process::Command, environment variables are managed internally using a BTreeMap, not a HashMap [1][2][3]. Because it uses a BTreeMap, the order of environment variables is deterministic (sorted by key), rather than insertion-ordered [1][2][3]. Regarding overwriting behavior, explicit changes made via.env or.envs always take precedence over inherited environment variables [4][5][6]. When you use.envs, you are providing a collection of key-value pairs that are inserted into or updated within the internal BTreeMap [4][5][3]. If multiple keys exist in your input to.envs, or if you call.env multiple times for the same key, the last value set for that specific key will be the one used by the spawned process [4][5][6]. Specifically concerning the PATH environment variable: 1. Overwriting: If you explicitly set PATH using.env("PATH",...) or via.envs, this value will completely override the parent process's PATH in the child environment [4][5][6]. 2. Resolution: The standard library tracks whether the PATH environment variable has been modified (via a saw_path flag in the internal CommandEnv struct) [1][2][3]. This is relevant because, on some platforms (notably Windows), the standard library uses this flag to decide whether to search the system-defined paths or rely solely on the provided PATH variable when spawning a child process [7][3]. If you need to preserve the parent's PATH while adding to it, you must manually fetch the current PATH, append your new entries, and set the full string explicitly: use std::env; use std::process::Command; let current_path = env::var_os("PATH").unwrap_or_default; let mut new_path = current_path; // Append your new paths here... Command::new("your_command").env("PATH", new_path).spawn?;
Citations:
- 1: https://doc.rust-lang.org/src/std/sys/process/env.rs.html
- 2: https://doc.rust-lang.org/1.66.1/src/std/sys_common/process.rs.html
- 3: https://github.com/rust-lang/rust/blob/d2f24127/library/std/src/sys/process/env.rs
- 4: https://dev-doc.rust-lang.org/stable/std/process/struct.Command.html
- 5: https://doc.rust-lang.org/nightly/std/process/struct.Command.html
- 6: https://doc.rust-lang.org/std/process/struct.Command.html?mut=
- 7: std::process::Command sometimes ignores PATH env variable on Windows rust-lang/rust#122660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'working dir\n'
pwd
printf 'tracked git.rs candidates\n'
git ls-files | rg '(^|/)git\.rs$|local/command\.rs$|Cargo\.lock$' || true
printf 'process_environment exact candidates\n'
rg -n --hidden -C 8 'process_environment|process_env\(' . -g '*.rs' -g '*.toml' | head -200 || true
printf 'git.rs snippets\n'
for f in $(git ls-files | rg 'git\.rs$'); do
echo "--- $f"
rg -n -C 8 'prepend_windows_git_paths|run_git|env_clear|build_env' "$f" || true
done
printf 'local command build_env snippets\n'
for f in $(git ls-files | rg 'command\.rs$'); do
echo "--- $f"
rg -n -C 15 'fn build_env|build_env\(|process_env' "$f" || true
doneRepository: wecode-ai/Wegent
Length of output: 25048
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'wework process_environment files\n'
git ls-files wework/src-tauri/src | rg 'process_environment|local_executor|lib\.rs' || true
printf 'process_environment.rs\n'
if [ -f wework/src-tauri/src/process_environment.rs ]; then
wc -l wework/src-tauri/src/process_environment.rs
rg -n -C 8 'pub.*process_env|pub.*normalized_current_path|pub.*sanitized_current_environment|normalized_current_path|sanitized_current_environment|PATH|path' wework/src-tauri/src/process_environment.rs
sed -n '220,380p' wework/src-tauri/src/process_environment.rs
fiRepository: wecode-ai/Wegent
Length of output: 4535
Use the command PATH for Windows Git path prepending.
prepend_windows_git_paths reads std::env::var("PATH") and calls command.env("PATH", joined), overwriting PATH set by build_env(extra_env). Get the PATH from build_env(extra_env) directly, append the Git directories, and set that value instead of reading the executor process PATH.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor/src/local/git.rs` around lines 91 - 97, The
prepend_windows_git_paths function reads from the executor process PATH via
std::env::var, which overwrites the PATH set by build_env(extra_env). Refactor
prepend_windows_git_paths to accept the command as a parameter and read the
current PATH value from the command's environment (which was set by build_env),
then append the Git directories and set the combined value back on the command,
avoiding any read from the executor's own environment variables.
Address CodeRabbit review feedback on PR 2407: - BufferedChatInput: flush pending drafts on scope switch or unmount only, using a committed-value baseline to avoid a duplicate onChange after every debounced flush. - CompactChatComposer: route parent value reads through the active (compact or fullscreen) composer handle instead of a shared ref. - ProjectChatComposer/CompactChatComposer: read the live editor value in attachment, quick-phrase, and drop insertion handlers instead of the debounced value prop, and log the actual submitted value. - ToolBlockItem: tokenize git commands with splitShellWords and skip git global options so `git -C <path> status` keeps its status label. - toolBlockActivity: recognize absolute POSIX shell paths (/bin/bash -c) when unwrapping shell wrappers; add debounce and composition-end BufferedChatInput tests.
Address CodeRabbit review feedback on PR 2407: - git_push surfaces the real git error when `branch --show-current` fails instead of misreporting a detached HEAD. - run_git sets GIT_TERMINAL_PROMPT=0 so git fails fast instead of waiting for interactive input, and drops the stale timeout doc line.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wework/src/components/layout/BufferedChatInput.test.tsx`:
- Around line 91-102: Update the “flushes the draft on composition end” test to
control or mock the debounce timer, keep it pending past its deadline, dispatch
compositionEnd, and assert onChange receives “draft” before the debounce timer
fires, proving the composition-end handler performs the flush.
- Around line 78-89: Update the test “debounces onChange during the 300ms
window” to use controlled/fake timers, assert onChange has not been called
before 300 ms, then advance the timer through the 300 ms boundary and assert it
receives “draft”. Remove the waitFor-based eventual-only verification while
preserving the existing typing setup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 15345ce1-2f5d-4389-b17f-62ed9b2ed54a
📒 Files selected for processing (9)
executor/src/local/app_ipc.rsexecutor/src/local/git.rswework/src/components/chat/blocks/ToolBlockItem.tsxwework/src/components/chat/blocks/toolBlockActivity.test.tswework/src/components/chat/blocks/toolBlockActivity.tswework/src/components/chat/composer/CompactChatComposer.tsxwework/src/components/chat/composer/ProjectChatComposer.tsxwework/src/components/layout/BufferedChatInput.test.tsxwework/src/components/layout/BufferedChatInput.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- wework/src/components/chat/composer/ProjectChatComposer.tsx
- wework/src/components/chat/blocks/toolBlockActivity.ts
- executor/src/local/app_ipc.rs
- wework/src/components/chat/blocks/ToolBlockItem.tsx
- wework/src/components/chat/blocks/toolBlockActivity.test.ts
- wework/src/components/chat/composer/CompactChatComposer.tsx
- wework/src/components/layout/BufferedChatInput.tsx
- executor/src/local/git.rs
| test('debounces onChange during the 300ms window', async () => { | ||
| const onChange = vi.fn() | ||
| render(<BufferedChatInput value="" onChange={onChange} onSubmit={vi.fn()} disabled={false} />) | ||
|
|
||
| await userEvent.type(screen.getByTestId('chat-message-input'), 'draft') | ||
| // onChange must not fire synchronously with typing; it is deferred by the debounce. | ||
| expect(onChange).not.toHaveBeenCalled() | ||
|
|
||
| await waitFor(() => { | ||
| expect(onChange).toHaveBeenCalledWith('draft') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify the 300 ms debounce boundary.
waitFor only proves eventual delivery. It does not prove that onChange remains deferred for 300 ms. Control the timer, assert no call before the deadline, then assert the callback after the deadline.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/components/layout/BufferedChatInput.test.tsx` around lines 78 -
89, Update the test “debounces onChange during the 300ms window” to use
controlled/fake timers, assert onChange has not been called before 300 ms, then
advance the timer through the 300 ms boundary and assert it receives “draft”.
Remove the waitFor-based eventual-only verification while preserving the
existing typing setup.
| test('flushes the draft on composition end', async () => { | ||
| const onChange = vi.fn() | ||
| render(<BufferedChatInput value="" onChange={onChange} onSubmit={vi.fn()} disabled={false} />) | ||
|
|
||
| const input = screen.getByTestId('chat-message-input') | ||
| await userEvent.type(input, 'draft') | ||
| fireEvent.compositionEnd(input) | ||
|
|
||
| await waitFor(() => { | ||
| expect(onChange).toHaveBeenLastCalledWith('draft') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Isolate the composition-end flush from the normal debounce.
This test can pass when compositionEnd is ignored. Hold the debounce timer below its deadline, dispatch compositionEnd, and assert that onChange is called before the timer would expire.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/components/layout/BufferedChatInput.test.tsx` around lines 91 -
102, Update the “flushes the draft on composition end” test to control or mock
the debounce timer, keep it pending past its deadline, dispatch compositionEnd,
and assert onChange receives “draft” before the debounce timer fires, proving
the composition-end handler performs the flush.
What changed
ChatInput,ProjectChatComposer, andComposerTextareaexpose ref-based handles viaforwardRef+useImperativeHandle, so handlers read the latest value without a re-render on every keystroke.onChangepropagation while keeping a local draft so composer state stays responsive, and keep ref-based typing performance inBufferedChatInputwith only the needed state sync.pwsh -Commandwrappers,cd &&prefixes, and leading$prompts.gitdirectly via argv on Windows, avoidingcmd /Changs; Windowsgit_*device commands run off the tokio worker threads.Why
Typing in the composer triggered per-keystroke React re-renders and state churn that caused high CPU usage and input lag, especially in long conversations. Running-tool blocks also showed raw PowerShell wrapper commands instead of readable labels, and Windows git device commands could hang under
cmd /C.Impact
git_*device commands run through a native git runner withoutcmd /Changs.Validation
pnpm --filter wework typecheckpnpm --filter wework test— focused chat/composer suites (ChatInput, ComposerToolbar, ToolBlockItem, ToolBlocksDisplay, BufferedChatInput)Summary by CodeRabbit
New Features
Bug Fixes
Performance