Skip to content

Commit c69368c

Browse files
committed
perf: 1s debounce for active workspace git status
Active workspace now uses 1s debounce for git status refresh after file-modifying tools complete, while background workspaces keep 3s. This makes the visible git indicators feel more responsive without adding load from background workspace refreshes.
1 parent 70dc5cc commit c69368c

File tree

5 files changed

+173
-217
lines changed

5 files changed

+173
-217
lines changed

src/browser/App.tsx

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import { buildSortedWorkspacesByProject } from "./utils/ui/workspaceFiltering";
1717
import { useResumeManager } from "./hooks/useResumeManager";
1818
import { useUnreadTracking } from "./hooks/useUnreadTracking";
1919
import { useWorkspaceStoreRaw, useWorkspaceRecency } from "./stores/WorkspaceStore";
20+
import { setActiveWorkspace } from "./stores/GitStatusStore";
2021

2122
import { useStableReference, compareMaps } from "./hooks/useStableReference";
2223
import { CommandRegistryProvider, useCommandRegistry } from "./contexts/CommandRegistryContext";
@@ -133,6 +134,11 @@ function AppInner() {
133134
prevWorkspaceRef.current = selectedWorkspace;
134135
}, [selectedWorkspace, telemetry]);
135136

137+
// Tell GitStatusStore which workspace is active (for prioritized 1s refresh)
138+
useEffect(() => {
139+
setActiveWorkspace(selectedWorkspace?.workspaceId ?? null);
140+
}, [selectedWorkspace?.workspaceId]);
141+
136142
// Track last-read timestamps for unread indicators
137143
const { lastReadTimestamps, onToggleUnread } = useUnreadTracking(selectedWorkspace);
138144

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

Lines changed: 38 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,31 @@ type DiffState =
9494
const REVIEW_PANEL_CACHE_MAX_ENTRIES = 20;
9595
const REVIEW_PANEL_CACHE_MAX_SIZE_BYTES = 20 * 1024 * 1024; // 20MB
9696

97+
/**
98+
* Preserve object references for unchanged hunks to prevent re-renders.
99+
* Compares by ID and content - if a hunk exists in prev with same content, reuse it.
100+
*/
101+
function preserveHunkReferences(prev: DiffHunk[], next: DiffHunk[]): DiffHunk[] {
102+
if (prev.length === 0) return next;
103+
104+
const prevById = new Map(prev.map((h) => [h.id, h]));
105+
let allSame = prev.length === next.length;
106+
107+
const result = next.map((hunk, i) => {
108+
const prevHunk = prevById.get(hunk.id);
109+
// Fast path: same ID and content means unchanged (content hash is part of ID)
110+
if (prevHunk && prevHunk.content === hunk.content) {
111+
if (allSame && prev[i]?.id !== hunk.id) allSame = false;
112+
return prevHunk;
113+
}
114+
allSame = false;
115+
return hunk;
116+
});
117+
118+
// If all hunks are reused in same order, return prev array to preserve top-level reference
119+
return allSame ? prev : result;
120+
}
121+
97122
interface ReviewPanelDiffCacheValue {
98123
hunks: DiffHunk[];
99124
truncationWarning: string | null;
@@ -458,10 +483,19 @@ export const ReviewPanel: React.FC<ReviewPanelProps> = ({
458483
if (cancelled) return;
459484

460485
setDiagnosticInfo(data.diagnosticInfo);
461-
setDiffState({
462-
status: "loaded",
463-
hunks: data.hunks,
464-
truncationWarning: data.truncationWarning,
486+
487+
// Preserve object references for unchanged hunks to prevent unnecessary re-renders.
488+
// HunkViewer is memoized on hunk object identity, so reusing references avoids
489+
// re-rendering (and re-highlighting) hunks that haven't actually changed.
490+
setDiffState((prev) => {
491+
const prevHunks =
492+
prev.status === "loaded" || prev.status === "refreshing" ? prev.hunks : [];
493+
const hunks = preserveHunkReferences(prevHunks, data.hunks);
494+
return {
495+
status: "loaded",
496+
hunks,
497+
truncationWarning: data.truncationWarning,
498+
};
465499
});
466500

467501
if (data.hunks.length > 0) {

src/browser/hooks/useReviewRefreshController.ts

Lines changed: 55 additions & 208 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
1-
import { useEffect, useRef } from "react";
1+
import { useEffect, useRef, useMemo } from "react";
22
import { workspaceStore } from "@/browser/stores/WorkspaceStore";
33
import type { APIClient } from "@/browser/contexts/API";
4+
import { RefreshController } from "@/browser/utils/RefreshController";
45

56
/** Debounce delay for auto-refresh after tool completion */
67
const TOOL_REFRESH_DEBOUNCE_MS = 3000;
@@ -47,229 +48,80 @@ export interface ReviewRefreshController {
4748
/**
4849
* Controls ReviewPanel auto-refresh triggered by file-modifying tool completions.
4950
*
50-
* Handles:
51-
* - Debouncing rapid tool completions (3s window)
52-
* - Pausing while user is interacting (review note input focused)
53-
* - Pausing while tab is hidden (flush on visibility change)
54-
* - Coalescing requests while origin fetch is in-flight
55-
* - Preserving scroll position across refreshes
56-
*
57-
* Architecture:
58-
* - All refresh logic flows through a single ref-based handler to avoid stale closures
59-
* - Pending flags track deferred refreshes for various pause conditions
60-
* - visibilitychange listener ensures hidden-tab refreshes aren't lost
51+
* Delegates debouncing, visibility/focus handling, and in-flight guards to RefreshController.
52+
* Keeps ReviewPanel-specific logic:
53+
* - Origin branch fetch before refresh
54+
* - Scroll position preservation
55+
* - User interaction pause state
6156
*/
6257
export function useReviewRefreshController(
6358
options: UseReviewRefreshControllerOptions
6459
): ReviewRefreshController {
6560
const { workspaceId, api, isCreating, onRefresh, scrollContainerRef, onGitStatusRefresh } =
6661
options;
6762

68-
// Store diffBase in a ref so we always read the latest value
63+
// Refs for values that executeRefresh needs at call time (avoid stale closures)
6964
const diffBaseRef = useRef(options.diffBase);
7065
diffBaseRef.current = options.diffBase;
7166

72-
// Store onGitStatusRefresh in a ref to avoid stale closures
7367
const onGitStatusRefreshRef = useRef(onGitStatusRefresh);
7468
onGitStatusRefreshRef.current = onGitStatusRefresh;
7569

76-
// State refs (avoid re-renders, just track state for refresh logic)
77-
const isRefreshingRef = useRef(false);
78-
const isInteractingRef = useRef(false);
79-
const debounceTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
80-
81-
// Pending flags - track why refresh was deferred
82-
const pendingBecauseHiddenRef = useRef(false);
83-
const pendingBecauseInteractingRef = useRef(false);
84-
const pendingBecauseInFlightRef = useRef(false);
85-
8670
// Scroll position to restore after refresh
8771
const savedScrollTopRef = useRef<number | null>(null);
8872

89-
// Expose isRefreshing for UI (e.g. disable refresh button)
90-
// We use a ref but also track in a simple way for the return value
91-
const isRefreshingForReturn = useRef(false);
92-
93-
/**
94-
* Core refresh execution - handles origin fetch if needed, then triggers onRefresh.
95-
* Always reads latest state from refs at execution time.
96-
*/
97-
const executeRefresh = useRef(() => {
98-
if (!api || isCreating) return;
99-
100-
// Save scroll position before refresh
101-
savedScrollTopRef.current = scrollContainerRef.current?.scrollTop ?? null;
102-
103-
const originBranch = getOriginBranchForFetch(diffBaseRef.current);
104-
if (originBranch) {
105-
isRefreshingRef.current = true;
106-
isRefreshingForReturn.current = true;
107-
108-
api.workspace
109-
.executeBash({
110-
workspaceId,
111-
script: `git fetch origin ${originBranch} --quiet || true`,
112-
options: { timeout_secs: 30 },
113-
})
114-
.catch((err) => {
115-
console.debug("ReviewPanel origin fetch failed", err);
116-
})
117-
.finally(() => {
118-
isRefreshingRef.current = false;
119-
isRefreshingForReturn.current = false;
120-
onRefresh();
121-
onGitStatusRefreshRef.current?.();
122-
123-
// If another refresh was requested while we were fetching, do it now
124-
if (pendingBecauseInFlightRef.current) {
125-
pendingBecauseInFlightRef.current = false;
126-
// Use setTimeout to avoid recursive call stack
127-
setTimeout(() => tryRefresh("in-flight-followup"), 0);
128-
}
129-
});
130-
131-
return;
132-
}
133-
134-
// Local base - just trigger refresh immediately
135-
onRefresh();
136-
onGitStatusRefreshRef.current?.();
137-
});
138-
139-
// Update executeRefresh closure dependencies
140-
executeRefresh.current = () => {
141-
if (!api || isCreating) return;
142-
143-
savedScrollTopRef.current = scrollContainerRef.current?.scrollTop ?? null;
144-
145-
const originBranch = getOriginBranchForFetch(diffBaseRef.current);
146-
if (originBranch) {
147-
isRefreshingRef.current = true;
148-
isRefreshingForReturn.current = true;
149-
150-
api.workspace
151-
.executeBash({
152-
workspaceId,
153-
script: `git fetch origin ${originBranch} --quiet || true`,
154-
options: { timeout_secs: 30 },
155-
})
156-
.catch((err) => {
157-
console.debug("ReviewPanel origin fetch failed", err);
158-
})
159-
.finally(() => {
160-
isRefreshingRef.current = false;
161-
isRefreshingForReturn.current = false;
162-
onRefresh();
163-
onGitStatusRefreshRef.current?.();
73+
// User interaction state (pauses auto-refresh)
74+
const isInteractingRef = useRef(false);
16475

165-
if (pendingBecauseInFlightRef.current) {
166-
pendingBecauseInFlightRef.current = false;
167-
setTimeout(() => tryRefresh("in-flight-followup"), 0);
76+
// Create RefreshController once, with stable callbacks via refs
77+
const controller = useMemo(() => {
78+
const ctrl = new RefreshController({
79+
debounceMs: TOOL_REFRESH_DEBOUNCE_MS,
80+
isPaused: () => isInteractingRef.current,
81+
onRefresh: async () => {
82+
if (!api || isCreating) return;
83+
84+
// Save scroll position before refresh
85+
savedScrollTopRef.current = scrollContainerRef.current?.scrollTop ?? null;
86+
87+
const originBranch = getOriginBranchForFetch(diffBaseRef.current);
88+
if (originBranch) {
89+
try {
90+
await api.workspace.executeBash({
91+
workspaceId,
92+
script: `git fetch origin ${originBranch} --quiet || true`,
93+
options: { timeout_secs: 30 },
94+
});
95+
} catch (err) {
96+
console.debug("ReviewPanel origin fetch failed", err);
16897
}
169-
});
170-
171-
return;
172-
}
173-
174-
onRefresh();
175-
onGitStatusRefreshRef.current?.();
176-
};
177-
178-
/**
179-
* Attempt to refresh, respecting all pause conditions.
180-
* If paused, sets the appropriate pending flag.
181-
*/
182-
const tryRefresh = (_reason: string) => {
183-
if (!api || isCreating) return;
184-
185-
// Check pause conditions in order of priority
186-
187-
// 1. Tab hidden - queue for visibility change
188-
if (document.hidden) {
189-
pendingBecauseHiddenRef.current = true;
190-
return;
191-
}
192-
193-
// 2. User interacting - queue for blur
194-
if (isInteractingRef.current) {
195-
pendingBecauseInteractingRef.current = true;
196-
return;
197-
}
198-
199-
// 3. Already refreshing (origin fetch in-flight) - queue for completion
200-
if (isRefreshingRef.current) {
201-
pendingBecauseInFlightRef.current = true;
202-
return;
203-
}
204-
205-
// All clear - execute refresh
206-
executeRefresh.current();
207-
};
208-
209-
/**
210-
* Schedule a debounced refresh (for tool completions).
211-
*/
212-
const scheduleRefresh = () => {
213-
if (debounceTimerRef.current) {
214-
clearTimeout(debounceTimerRef.current);
215-
}
216-
debounceTimerRef.current = setTimeout(() => {
217-
debounceTimerRef.current = null;
218-
tryRefresh("tool-completion");
219-
}, TOOL_REFRESH_DEBOUNCE_MS);
220-
};
221-
222-
/**
223-
* Flush any pending refresh (called when pause condition clears).
224-
*/
225-
const flushPending = (clearedCondition: "hidden" | "interacting") => {
226-
if (clearedCondition === "hidden" && pendingBecauseHiddenRef.current) {
227-
pendingBecauseHiddenRef.current = false;
228-
tryRefresh("visibility-restored");
229-
} else if (clearedCondition === "interacting" && pendingBecauseInteractingRef.current) {
230-
pendingBecauseInteractingRef.current = false;
231-
tryRefresh("interaction-ended");
232-
}
233-
};
98+
}
99+
100+
onRefresh();
101+
onGitStatusRefreshRef.current?.();
102+
},
103+
});
104+
ctrl.bindListeners();
105+
return ctrl;
106+
// workspaceId/api/isCreating changes require new controller with updated closure
107+
}, [workspaceId, api, isCreating, onRefresh, scrollContainerRef]);
108+
109+
// Cleanup on unmount or when controller changes
110+
useEffect(() => {
111+
return () => controller.dispose();
112+
}, [controller]);
234113

235114
// Subscribe to file-modifying tool completions
236115
useEffect(() => {
237116
if (!api || isCreating) return;
238117

239-
const unsubscribe = workspaceStore.subscribeFileModifyingTool(scheduleRefresh, workspaceId);
240-
241-
return () => {
242-
unsubscribe();
243-
if (debounceTimerRef.current) {
244-
clearTimeout(debounceTimerRef.current);
245-
debounceTimerRef.current = null;
246-
}
247-
};
248-
// scheduleRefresh is stable (only uses refs internally)
249-
// eslint-disable-next-line react-hooks/exhaustive-deps
250-
}, [api, workspaceId, isCreating]);
118+
const unsubscribe = workspaceStore.subscribeFileModifyingTool(
119+
() => controller.schedule(),
120+
workspaceId
121+
);
251122

252-
// Handle visibility/focus changes - flush pending refresh when user returns.
253-
// Uses both visibilitychange (for browser tab hidden state) and window focus
254-
// (for Electron app focus) since visibilitychange alone is unreliable in Electron
255-
// when the app is behind other windows or on a different desktop/space.
256-
useEffect(() => {
257-
const handleReturn = () => {
258-
// Only flush if document is actually visible
259-
if (!document.hidden) {
260-
flushPending("hidden");
261-
}
262-
};
263-
264-
document.addEventListener("visibilitychange", handleReturn);
265-
window.addEventListener("focus", handleReturn);
266-
return () => {
267-
document.removeEventListener("visibilitychange", handleReturn);
268-
window.removeEventListener("focus", handleReturn);
269-
};
270-
// flushPending is stable (only uses refs internally)
271-
// eslint-disable-next-line react-hooks/exhaustive-deps
272-
}, []);
123+
return unsubscribe;
124+
}, [api, workspaceId, isCreating, controller]);
273125

274126
// Public API
275127
const setInteracting = (interacting: boolean) => {
@@ -278,24 +130,19 @@ export function useReviewRefreshController(
278130

279131
// If interaction ended, flush any pending refresh
280132
if (wasInteracting && !interacting) {
281-
flushPending("interacting");
133+
controller.notifyUnpaused();
282134
}
283135
};
284136

285137
const requestManualRefresh = () => {
286-
// Manual refresh bypasses debounce but still respects in-flight check
287-
if (isRefreshingRef.current) {
288-
pendingBecauseInFlightRef.current = true;
289-
return;
290-
}
291-
executeRefresh.current();
138+
controller.requestImmediate();
292139
};
293140

294141
return {
295142
requestManualRefresh,
296143
setInteracting,
297144
get isRefreshing() {
298-
return isRefreshingForReturn.current;
145+
return controller.isRefreshing;
299146
},
300147
};
301148
}

0 commit comments

Comments
 (0)