Skip to content

Commit 4089f3c

Browse files
committed
🤖 fix: eliminate debounce race conditions in ReviewPanel auto-refresh
Refactored the file-modifying tool debounce to prevent stale closures from causing unreliable refresh behavior. ## Changes - Moved debounce timer to a ref (debounceTimerRef) so it's accessible from any code path without closure capture - Unified duplicate refresh logic (performRefresh + handleRefreshRef) into a single handleRefreshRef that's updated on every render - Timer callback now invokes through ref to read latest state at execution time - Removed filters.diffBase from effect dependencies since the ref-based approach handles it correctly ## Race condition fixes 1. **Stale closure capture**: Previously, the timer captured `filters.diffBase` at schedule time. Now it reads `filters.diffBase` at execution time via ref. 2. **Multiple timers**: The ref-based timer ensures only one timer exists, preventing duplicate refreshes when dependencies change during debounce window. 3. **Cleanup reliability**: Timer ref can be cleared from cleanup without depending on closure variable still being in scope. --- _Generated with `mux` • Model: `anthropic:claude-opus-4-5` • Thinking: `high`_
1 parent 473bbbf commit 4089f3c

File tree

1 file changed

+65
-84
lines changed

1 file changed

+65
-84
lines changed

src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx

Lines changed: 65 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -259,65 +259,86 @@ export const ReviewPanel: React.FC<ReviewPanelProps> = ({
259259
includeUncommitted: includeUncommitted,
260260
});
261261

262-
// Auto-refresh on file-modifying tool completions (debounced 3s).
263-
// Respects user interaction - if user is focused on review input, queues refresh for after blur.
264-
useEffect(() => {
262+
// Stable refresh handler via ref - always reads latest state at execution time.
263+
// This avoids stale closure issues when debounced timers fire.
264+
const handleRefreshRef = useRef<() => void>(() => {
265+
console.debug("ReviewPanel handleRefreshRef called before init");
266+
});
267+
handleRefreshRef.current = () => {
265268
if (!api || isCreating) return;
266269

267-
let debounceTimer: ReturnType<typeof setTimeout> | null = null;
270+
// Skip if document not visible (user switched tabs/windows)
271+
if (document.hidden) return;
268272

269-
const performRefresh = () => {
270-
// Skip if document not visible (user switched tabs/windows)
271-
if (document.hidden) return;
273+
// Skip if user is actively entering a review note
274+
if (isUserInteractingRef.current) {
275+
pendingRefreshRef.current = true;
276+
return;
277+
}
272278

273-
// Skip if user is actively entering a review note
274-
if (isUserInteractingRef.current) {
275-
pendingRefreshRef.current = true;
276-
return;
277-
}
279+
// Skip if already refreshing (for origin/* bases with fetch)
280+
if (isRefreshingRef.current) return;
278281

279-
// Skip if already refreshing (for origin/* bases with fetch)
280-
if (isRefreshingRef.current) return;
281-
282-
// Save scroll position before refresh
283-
savedScrollTopRef.current = scrollContainerRef.current?.scrollTop ?? null;
284-
285-
const originBranch = getOriginBranchForFetch(filters.diffBase);
286-
if (originBranch) {
287-
// Remote base: fetch before refreshing diff
288-
isRefreshingRef.current = true;
289-
api.workspace
290-
.executeBash({
291-
workspaceId,
292-
script: `git fetch origin ${originBranch} --quiet || true`,
293-
options: { timeout_secs: 30 },
294-
})
295-
.catch((err) => {
296-
console.debug("ReviewPanel origin fetch failed", err);
297-
})
298-
.finally(() => {
299-
isRefreshingRef.current = false;
300-
setRefreshTrigger((prev) => prev + 1);
301-
});
302-
} else {
303-
// Local base: just refresh diff
304-
setRefreshTrigger((prev) => prev + 1);
305-
}
306-
};
282+
// Save scroll position before refresh
283+
savedScrollTopRef.current = scrollContainerRef.current?.scrollTop ?? null;
284+
285+
const originBranch = getOriginBranchForFetch(filters.diffBase);
286+
if (originBranch) {
287+
isRefreshingRef.current = true;
288+
289+
api.workspace
290+
.executeBash({
291+
workspaceId,
292+
script: `git fetch origin ${originBranch} --quiet || true`,
293+
options: { timeout_secs: 30 },
294+
})
295+
.catch((err) => {
296+
console.debug("ReviewPanel origin fetch failed", err);
297+
})
298+
.finally(() => {
299+
isRefreshingRef.current = false;
300+
setRefreshTrigger((prev) => prev + 1);
301+
});
302+
303+
return;
304+
}
305+
306+
setRefreshTrigger((prev) => prev + 1);
307+
};
308+
309+
const handleRefresh = () => {
310+
handleRefreshRef.current();
311+
};
312+
313+
// Debounce timer ref - lives outside effect to prevent stale closure issues.
314+
// Using a ref allows cleanup from any code path without closure capture.
315+
const debounceTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
316+
317+
// Auto-refresh on file-modifying tool completions (debounced 3s).
318+
// Respects user interaction - if user is focused on review input, queues refresh for after blur.
319+
useEffect(() => {
320+
if (!api || isCreating) return;
307321

308322
const scheduleRefresh = () => {
309-
if (debounceTimer) clearTimeout(debounceTimer);
310-
debounceTimer = setTimeout(performRefresh, TOOL_REFRESH_DEBOUNCE_MS);
323+
if (debounceTimerRef.current) clearTimeout(debounceTimerRef.current);
324+
// Call through ref to get latest state at execution time
325+
debounceTimerRef.current = setTimeout(() => {
326+
debounceTimerRef.current = null;
327+
handleRefreshRef.current();
328+
}, TOOL_REFRESH_DEBOUNCE_MS);
311329
};
312330

313331
// Subscribe to file-modifying tool completions for this workspace
314332
const unsubscribe = workspaceStore.subscribeFileModifyingTool(workspaceId, scheduleRefresh);
315333

316334
return () => {
317335
unsubscribe();
318-
if (debounceTimer) clearTimeout(debounceTimer);
336+
if (debounceTimerRef.current) {
337+
clearTimeout(debounceTimerRef.current);
338+
debounceTimerRef.current = null;
339+
}
319340
};
320-
}, [api, workspaceId, filters.diffBase, isCreating]);
341+
}, [api, workspaceId, isCreating]);
321342

322343
// Sync panel focus with interaction tracking; fire pending refresh on blur
323344
useEffect(() => {
@@ -343,46 +364,6 @@ export const ReviewPanel: React.FC<ReviewPanelProps> = ({
343364
}, [diffState.status]);
344365

345366
// Focus panel when focusTrigger changes (preserves current hunk selection)
346-
347-
const handleRefreshRef = useRef<() => void>(() => {
348-
console.debug("ReviewPanel handleRefreshRef called before init");
349-
});
350-
handleRefreshRef.current = () => {
351-
if (!api || isCreating) return;
352-
353-
// Skip if already refreshing (for origin/* bases with fetch)
354-
if (isRefreshingRef.current) {
355-
setRefreshTrigger((prev) => prev + 1);
356-
return;
357-
}
358-
359-
const originBranch = getOriginBranchForFetch(filters.diffBase);
360-
if (originBranch) {
361-
isRefreshingRef.current = true;
362-
363-
api.workspace
364-
.executeBash({
365-
workspaceId,
366-
script: `git fetch origin ${originBranch} --quiet || true`,
367-
options: { timeout_secs: 30 },
368-
})
369-
.catch((err) => {
370-
console.debug("ReviewPanel origin fetch failed", err);
371-
})
372-
.finally(() => {
373-
isRefreshingRef.current = false;
374-
setRefreshTrigger((prev) => prev + 1);
375-
});
376-
377-
return;
378-
}
379-
380-
setRefreshTrigger((prev) => prev + 1);
381-
};
382-
383-
const handleRefresh = () => {
384-
handleRefreshRef.current();
385-
};
386367
useEffect(() => {
387368
if (focusTrigger && focusTrigger > 0) {
388369
panelRef.current?.focus();

0 commit comments

Comments
 (0)