fix(ui): Remove dead dirs state and unused hook parameter from InputPrompt#2891
fix(ui): Remove dead dirs state and unused hook parameter from InputPrompt#2891tanzhenxin merged 3 commits intoQwenLM:mainfrom
Conversation
getDirectories() returns a new array reference each call, causing the useEffect dependency check to fail on every render. Move the call inside the effect body and use stable dependencies [config, dirs] so the effect only re-runs when they actually change.
tanzhenxin
left a comment
There was a problem hiding this comment.
Review: fix(ui): prevent useEffect from running every render in InputPrompt
Overview
Fixes a performance issue where a useEffect in InputPrompt fired on every render because getDirectories() returns Array.from(this.directories) — a new array reference each call — placed directly in the dependency array. The fix moves the call inside the effect body and uses [config, dirs] as dependencies.
Critical Issues
1. Effect no longer detects external directory changes (Medium-High)
Both config (likely a stable singleton) and dirs (local state, only changes via setDirs inside this effect) are stable references. After the initial render, the effect has no external trigger. If directories are added/removed externally (e.g., via /add-dir, which mutates WorkspaceContext in place at directoryCommand.tsx:135), neither config nor dirs changes identity, so the effect never re-runs.
The old code handled this correctly (albeit wastefully) — by calling getDirectories() during render and putting the result in deps.
Recommendations:
- Subscribe to
workspaceContext.onDirectoriesChanged()if such an API exists. - Or use a serialized dep key:
const dirKey = config.getWorkspaceContext().getDirectories().join('\0')and use[dirKey]as the dependency — still callsgetDirectories()every render but the string comparison is cheap and stable. - Or remove the
dirsstate entirely if nothing actually consumes it.
2. Self-triggering cycle (Low)
dirs in the dependency array means every real directory change causes the effect to run twice — once to detect, once after setDirs updates. The inner comparison stops infinite loops, but it's unnecessary work.
Positives
- Correct diagnosis of the
Array.from()new-reference problem. - Improved comparison: old code only compared
dirs.length; new code does element-wise.every(). - Clean, minimal diff.
Verdict
REQUEST_CHANGES — The fix solves the performance problem but introduces a functional regression where directory changes after mount go undetected. The effect's dependency array needs a mechanism to observe external directory mutations.
Move from [config, dirs] deps (both stable refs that miss external changes) to a dirKey string (join of current directories). This preserves the perf fix (no new array ref in deps) while still detecting directory additions/removals from /add-dir etc.
|
the original fix with [config, dirs] deps would miss external directory changes because both are
This keeps the perf fix (no new array ref in deps) while correctly detecting external mutations from /add-dir and similar commands. |
tanzhenxin
left a comment
There was a problem hiding this comment.
Review
The dirs parameter passed to useCommandCompletion() is never actually read inside that hook — it's declared on line 42 but not referenced anywhere in the function body. This makes the dirs state and its syncing effect dead code.
Rather than optimizing the sync, consider removing the dirs state and the unused parameter entirely.
The dirs parameter passed to useCommandCompletion() was never read inside that hook, making the dirs state and sync effect in InputPrompt dead code. Remove the parameter, the state, the effect, and all test call-site args.
Oh wow, good catch! Updated PR description and code. Thanks! |
tanzhenxin
left a comment
There was a problem hiding this comment.
Clean dead-code removal. The dirs parameter in useCommandCompletion() was declared but never read — removing it along with the state and sync effect in InputPrompt is the right call. LGTM.
Remove dead
dirsstate and unused hook parameter from InputPrompt.TLDR
The
dirsparameter passed touseCommandCompletion()was never referenced inside the hook body — only declared on line 42. Thedirsstate, its syncuseEffect, and the parameter itself were all dead code. This PR removes them entirely instead of optimizing the sync effect.Screenshots / Video Demo
N/A — no user-facing change. Pure dead-code removal.
Dive Deeper
Thanks to the reviewer for catching this. The original PR tried to fix an effect that was running on every render (because
getDirectories()returns a new array each call). The first attempted fix used a serialized dependency key to stabilize the dep. The reviewer correctly pointed out that the whole thing was pointless:Verified:
grep dirsinuseCommandCompletion.tsxreturns only the parameter declaration.So instead of optimizing dead code, this PR deletes it:
useCommandCompletion.tsx— removed thedirs: readonly string[]parameterInputPrompt.tsx— removed thedirsstate, thecurrentDirs/dirKeylocals, the syncuseEffect, and thedirsargument at the call siteuseCommandCompletion.test.ts— removedtestDirslocal and thetestDirs,argument from all 17 call sitesInputPrompt.test.tsx— removed the['/test/project/src']second argument from alltoHaveBeenCalledWithassertionsNet: 40 lines deleted, 0 lines added. No behavior change — the hook never used the value.
Modified files:
packages/cli/src/ui/components/InputPrompt.tsxpackages/cli/src/ui/components/InputPrompt.test.tsxpackages/cli/src/ui/hooks/useCommandCompletion.tsxpackages/cli/src/ui/hooks/useCommandCompletion.test.tsReviewer Test Plan
useCommandCompletiontests:pnpm --filter=@qwen-code/qwen-code exec npx vitest run src/ui/hooks/useCommandCompletion.test.ts— 17 passInputPrompttests:pnpm --filter=@qwen-code/qwen-code exec npx vitest run src/ui/components/InputPrompt.test.tsx— 103 pass, 2 skippedgrep -r "useCommandCompletion" packages/cli/src— all call sites match the new 8-arg signature/add-dirduring a session — prompt still works correctly (directory state lives inconfig.getWorkspaceContext(), not in the removed local state)Testing Matrix
Linked issues / bugs
Linked to bug report issue for useEffect running every render in InputPrompt. The reviewer correctly identified that the root cause was dead code, not a missing optimization.