fix: retry giveFocus on next animation frame for newly created blocks#3100
Conversation
When a new block is created, giveFocus() may fail because the view's DOM element isn't mounted yet. This causes focus to fall back to the hidden dummy input, which cannot handle arrow key scrolling. Retry giveFocus() via requestAnimationFrame after the fallback, giving React a chance to flush pending renders before the retry. Fixes wavetermdev#2926 Signed-off-by: majiayu000 <1835304752@qq.com>
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughIn Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Reviewed by minimax-m2.5-20260211 · 360,462 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/app/block/block.tsx (1)
229-231: Consider cancelling therequestAnimationFrameon cleanup or re-invocation.The scheduled rAF callback is not cancelled if:
- The component unmounts before the frame fires
setFocusTargetis called again before the previous frame executesWhile the optional chaining prevents crashes, calling
giveFocus()on a disposed viewModel or queuing multiple redundant callbacks is not ideal.♻️ Suggested improvement using a ref to track and cancel pending rAF
Add a ref to track the animation frame ID at the component level (around line 144):
const pendingFocusRafRef = useRef<number | null>(null);Then update
setFocusTargetto cancel any pending frame before scheduling a new one:const setFocusTarget = useCallback(() => { + if (pendingFocusRafRef.current != null) { + cancelAnimationFrame(pendingFocusRafRef.current); + pendingFocusRafRef.current = null; + } const ok = viewModel?.giveFocus?.(); if (ok) { return; } focusElemRef.current?.focus({ preventScroll: true }); - requestAnimationFrame(() => { + pendingFocusRafRef.current = requestAnimationFrame(() => { + pendingFocusRafRef.current = null; viewModel?.giveFocus?.(); }); }, [viewModel]);And add cleanup on unmount (e.g., in a
useEffect):useEffect(() => { return () => { if (pendingFocusRafRef.current != null) { cancelAnimationFrame(pendingFocusRafRef.current); } }; }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/block/block.tsx` around lines 229 - 231, The requestAnimationFrame scheduled in setFocusTarget calls viewModel?.giveFocus() but is never cancelled, so add a component-level ref (e.g., pendingFocusRafRef) to store the RAF id, cancel any existing RAF via cancelAnimationFrame(pendingFocusRafRef.current) before scheduling a new one in setFocusTarget, assign the new id to pendingFocusRafRef.current, and add a cleanup useEffect that cancels the pending RAF on unmount; reference the existing requestAnimationFrame call and viewModel?.giveFocus() when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/app/block/block.tsx`:
- Around line 229-231: The requestAnimationFrame scheduled in setFocusTarget
calls viewModel?.giveFocus() but is never cancelled, so add a component-level
ref (e.g., pendingFocusRafRef) to store the RAF id, cancel any existing RAF via
cancelAnimationFrame(pendingFocusRafRef.current) before scheduling a new one in
setFocusTarget, assign the new id to pendingFocusRafRef.current, and add a
cleanup useEffect that cancels the pending RAF on unmount; reference the
existing requestAnimationFrame call and viewModel?.giveFocus() when making these
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b3006749-8cfc-4710-9b33-13e3d345d387
📒 Files selected for processing (1)
frontend/app/block/block.tsx
Address CodeRabbit review feedback on PR wavetermdev#3100: track the requestAnimationFrame ID via a ref and cancel it before scheduling a new one or when the component unmounts. Signed-off-by: majiayu000 <1835304752@qq.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/block/block.tsx`:
- Around line 232-246: The retry RAF currently calls viewModel?.giveFocus?()
unconditionally, causing duplicate subscriptions; update setFocusTarget so the
RAF callback first checks whether focus is already on the block (e.g. check
focusElemRef.current and document.activeElement or a viewModel method) and only
calls viewModel?.giveFocus?() if focus is still not within focusElemRef.current;
keep the immediate giveFocus call as-is, still clear pendingFocusRafRef, and
ensure pendingFocusRafRef is nulled whether or not the retry runs to avoid
dangling refs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8695f4ac-9ef8-4d6b-b103-7d8164d0241f
📒 Files selected for processing (1)
frontend/app/block/block.tsx
| const setFocusTarget = useCallback(() => { | ||
| if (pendingFocusRafRef.current != null) { | ||
| cancelAnimationFrame(pendingFocusRafRef.current); | ||
| pendingFocusRafRef.current = null; | ||
| } | ||
| const ok = viewModel?.giveFocus?.(); | ||
| if (ok) { | ||
| return; | ||
| } | ||
| focusElemRef.current?.focus({ preventScroll: true }); | ||
| pendingFocusRafRef.current = requestAnimationFrame(() => { | ||
| pendingFocusRafRef.current = null; | ||
| viewModel?.giveFocus?.(); | ||
| }); | ||
| }, [viewModel]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all giveFocus implementations to assess side-effect patterns
# Search for giveFocus method definitions
ast-grep --pattern $'giveFocus($_) {
$$$
}'
# Also search for giveFocus with no params
ast-grep --pattern $'giveFocus() {
$$$
}'Repository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
# First, find the block.tsx file and examine the code around lines 232-246
find . -name "block.tsx" -path "*/frontend/*" | head -5Repository: wavetermdev/waveterm
Length of output: 94
🏁 Script executed:
# Search for giveFocus implementations using rg
rg "giveFocus\s*\(" --type ts --type tsx -A 5 -B 1 | head -100Repository: wavetermdev/waveterm
Length of output: 91
🏁 Script executed:
# Also search for giveFocus method definitions more broadly
rg "giveFocus\s*[:\(]" --type ts --type tsx | head -50Repository: wavetermdev/waveterm
Length of output: 91
🏁 Script executed:
# Check file size first
wc -l ./frontend/app/block/block.tsxRepository: wavetermdev/waveterm
Length of output: 98
🏁 Script executed:
# Read the specific lines from block.tsx
sed -n '232,246p' ./frontend/app/block/block.tsxRepository: wavetermdev/waveterm
Length of output: 619
🏁 Script executed:
# Search for giveFocus without type filters
rg "giveFocus\s*\(" -A 5 -B 1 | head -150Repository: wavetermdev/waveterm
Length of output: 8613
🏁 Script executed:
# Also find webview.tsx file
find . -name "webview.tsx" | head -5Repository: wavetermdev/waveterm
Length of output: 103
🏁 Script executed:
# Find the giveFocus method in webview.tsx and get more context
rg "giveFocus" ./frontend/app/view/webview/webview.tsx -A 30 -B 2Repository: wavetermdev/waveterm
Length of output: 1248
🏁 Script executed:
# Check the webview.tsx file size first
wc -l ./frontend/app/view/webview/webview.tsxRepository: wavetermdev/waveterm
Length of output: 108
🏁 Script executed:
# Read lines around 504-527 as mentioned in the review
sed -n '500,540p' ./frontend/app/view/webview/webview.tsxRepository: wavetermdev/waveterm
Length of output: 1439
🏁 Script executed:
# Search for focusedBlockId function
rg "focusedBlockId" --type-listRepository: wavetermdev/waveterm
Length of output: 453
🏁 Script executed:
# Search more broadly for focusedBlockId
rg "focusedBlockId" -A 3 -B 1 | head -50Repository: wavetermdev/waveterm
Length of output: 3132
🏁 Script executed:
# Check the setFocusTarget context in block.tsx - get more lines to see available variables
sed -n '200,250p' ./frontend/app/block/block.tsxRepository: wavetermdev/waveterm
Length of output: 2003
🏁 Script executed:
# Search for nodeModel usage in block.tsx
rg "nodeModel" ./frontend/app/block/block.tsx -B 1 -A 1 | head -30Repository: wavetermdev/waveterm
Length of output: 1182
Double giveFocus() invocation creates duplicate subscriptions in webview.
The retry logic calls giveFocus() twice: immediately (line 237) and in the RAF callback (line 244). The webview implementation has problematic side effects when ctrlShiftState is true—it sets up a globalStore.sub() subscription and returns false. If the RAF callback fires while ctrlShiftState is still true, a second subscription is created. Both subscriptions respond to the state change, causing duplicate unsubscribe calls, double focus() invocations, and potential memory leaks.
Guard the RAF callback to skip the retry if focus is already on the block:
Suggested guard to prevent redundant retry
focusElemRef.current?.focus({ preventScroll: true });
pendingFocusRafRef.current = requestAnimationFrame(() => {
pendingFocusRafRef.current = null;
+ if (focusedBlockId() === nodeModel.blockId) {
+ return;
+ }
viewModel?.giveFocus?.();
});
- }, [viewModel]);
+ }, [viewModel, nodeModel.blockId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/block/block.tsx` around lines 232 - 246, The retry RAF currently
calls viewModel?.giveFocus?() unconditionally, causing duplicate subscriptions;
update setFocusTarget so the RAF callback first checks whether focus is already
on the block (e.g. check focusElemRef.current and document.activeElement or a
viewModel method) and only calls viewModel?.giveFocus?() if focus is still not
within focusElemRef.current; keep the immediate giveFocus call as-is, still
clear pendingFocusRafRef, and ensure pendingFocusRafRef is nulled whether or not
the retry runs to avoid dangling refs.
Skip the requestAnimationFrame retry of giveFocus() if focus has already moved outside the block, preventing unnecessary calls when the user has clicked elsewhere between scheduling and execution. Signed-off-by: majiayu000 <1835304752@qq.com>
Cherry-pick from wavetermdev#3100. Fixes keyboard scrolling not working in newly opened blocks until mouse click — race condition where giveFocus() is called before the view's DOM element is mounted.
|
@majiayu000 thanks for the submission. lgtm! coderabbit often gets over eager about flagging things that aren't actually bugs. |
3 participants
Fixes #2926
Problem
Keyboard scrolling (arrow keys) does not work in newly opened blocks until the user mouse-clicks. This is a race condition in BlockFull's focus handling.
When
isFocusedbecomes true for a newly created block,setFocusTarget()callsviewModel.giveFocus(). But the view's DOM element (terminal, Monaco editor, webview) may not be mounted yet, sogiveFocus()returns false and focus falls back to the hidden dummy<input>element — which cannot handle arrow key scrolling.Fix
After falling back to the dummy focus element, schedule a
requestAnimationFramecallback that retriesviewModel.giveFocus(). This gives React one more frame to flush pending renders and mount the view's DOM, so focus transfers to the real element once it's ready.Test Plan
npx tsc --noEmitpasses clean