Skip to content

Commit b493236

Browse files
ammar-agentThomasK33
authored andcommitted
🤖 fix: make code review diff refresh robust and predictable (#1296)
Makes the Code Review "diff refresh" mechanism robust and predictable, driven by file-modifying tool-call events (`bash`, `file_edit_*`) rather than polling or filesystem watchers. ## Key Changes ### RefreshController improvements - **Coalescing rate-limit** instead of resetting debounce — prevents starvation when tool events are frequent - **Manual refresh always works** — `requestImmediate()` bypasses pause, hidden, and min-interval checks - **Loop prevention** — 500ms minimum interval between refreshes (scheduled events only, not manual) - **Trigger tracking** — `LastRefreshInfo` records what caused each refresh ### Review panel refresh - **Non-blocking manual refresh** — immediately bumps `refreshTrigger`, origin fetch runs in background - **Origin fetch deduplication** — file tree and diff loads share the same `git fetch` call - **Refresh tooltip** — shows "Last: 2m ago via manual" for debugging ### GitStatusStore - **Active workspace priority** — 1s debounce vs 3s for background workspaces - **File modification subscription** — refreshes on tool completion events ## Bug Fixes - Fixed storybook test failure: `requestImmediate()` was blocked by MIN_REFRESH_INTERVAL when components subscribed immediately after `syncWorkspaces()` — now bypasses interval check for manual/immediate requests Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with `mux` • Model: `anthropic:claude-opus-4-5` • Thinking: `high`_
1 parent 5eff3c6 commit b493236

File tree

12 files changed

+779
-266
lines changed

12 files changed

+779
-266
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/RefreshButton.tsx

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,30 @@
55
import React, { useState, useRef, useEffect } from "react";
66
import { Tooltip, TooltipTrigger, TooltipContent } from "@/browser/components/ui/tooltip";
77
import { formatKeybind, KEYBINDS } from "@/browser/utils/ui/keybinds";
8+
import { formatRelativeTimeCompact } from "@/browser/utils/ui/dateTime";
89
import { cn } from "@/common/lib/utils";
10+
import type { LastRefreshInfo, RefreshTrigger } from "@/browser/utils/RefreshController";
911

1012
interface RefreshButtonProps {
1113
onClick: () => void;
1214
isLoading?: boolean;
15+
/** Debug info about last refresh (timestamp and trigger) */
16+
lastRefreshInfo?: LastRefreshInfo | null;
1317
}
1418

15-
export const RefreshButton: React.FC<RefreshButtonProps> = ({ onClick, isLoading = false }) => {
19+
/** Human-readable trigger labels */
20+
const TRIGGER_LABELS: Record<RefreshTrigger, string> = {
21+
manual: "manual click",
22+
scheduled: "tool completion",
23+
priority: "tool completion (priority)",
24+
focus: "window focus",
25+
visibility: "tab visible",
26+
unpaused: "interaction ended",
27+
"in-flight-followup": "queued followup",
28+
};
29+
30+
export const RefreshButton: React.FC<RefreshButtonProps> = (props) => {
31+
const { onClick, isLoading = false, lastRefreshInfo } = props;
1632
// Track animation state for graceful stopping
1733
const [animationState, setAnimationState] = useState<"idle" | "spinning" | "stopping">("idle");
1834
const spinOnceTimeoutRef = useRef<number | null>(null);
@@ -76,9 +92,19 @@ export const RefreshButton: React.FC<RefreshButtonProps> = ({ onClick, isLoading
7692
</button>
7793
</TooltipTrigger>
7894
<TooltipContent side="bottom" align="start">
79-
{animationState !== "idle"
80-
? "Refreshing..."
81-
: `Refresh diff (${formatKeybind(KEYBINDS.REFRESH_REVIEW)})`}
95+
{animationState !== "idle" ? (
96+
"Refreshing..."
97+
) : (
98+
<span>
99+
Refresh diff ({formatKeybind(KEYBINDS.REFRESH_REVIEW)})
100+
{lastRefreshInfo && (
101+
<span className="text-muted block text-[10px]">
102+
Last: {formatRelativeTimeCompact(lastRefreshInfo.timestamp)} via{" "}
103+
{TRIGGER_LABELS[lastRefreshInfo.trigger] ?? lastRefreshInfo.trigger}
104+
</span>
105+
)}
106+
</span>
107+
)}
82108
</TooltipContent>
83109
</Tooltip>
84110
);

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

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import React, { useState } from "react";
66
import { usePersistedState } from "@/browser/hooks/usePersistedState";
77
import type { ReviewFilters, ReviewStats } from "@/common/types/review";
8+
import type { LastRefreshInfo } from "@/browser/utils/RefreshController";
89
import { RefreshButton } from "./RefreshButton";
910
import { UntrackedStatus } from "./UntrackedStatus";
1011

@@ -17,6 +18,8 @@ interface ReviewControlsProps {
1718
workspaceId: string;
1819
workspacePath: string;
1920
refreshTrigger?: number;
21+
/** Debug info about last refresh */
22+
lastRefreshInfo?: LastRefreshInfo | null;
2023
}
2124

2225
export const ReviewControls: React.FC<ReviewControlsProps> = ({
@@ -28,6 +31,7 @@ export const ReviewControls: React.FC<ReviewControlsProps> = ({
2831
workspaceId,
2932
workspacePath,
3033
refreshTrigger,
34+
lastRefreshInfo,
3135
}) => {
3236
// Local state for input value - only commit on blur/Enter
3337
const [inputValue, setInputValue] = useState(filters.diffBase);
@@ -83,7 +87,13 @@ export const ReviewControls: React.FC<ReviewControlsProps> = ({
8387

8488
return (
8589
<div className="bg-separator border-border-light flex flex-wrap items-center gap-3 border-b px-3 py-2 text-[11px]">
86-
{onRefresh && <RefreshButton onClick={onRefresh} isLoading={isLoading} />}
90+
{onRefresh && (
91+
<RefreshButton
92+
onClick={onRefresh}
93+
isLoading={isLoading}
94+
lastRefreshInfo={lastRefreshInfo}
95+
/>
96+
)}
8797
<label className="text-muted font-medium whitespace-nowrap">Base:</label>
8898
<input
8999
type="text"

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

Lines changed: 105 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;
@@ -118,6 +143,53 @@ const reviewPanelCache = new LRUCache<string, ReviewPanelCacheValue>({
118143
sizeCalculation: (value) => estimateJsonSizeBytes(value),
119144
});
120145

146+
function getOriginBranchForFetch(diffBase: string): string | null {
147+
const trimmed = diffBase.trim();
148+
if (!trimmed.startsWith("origin/")) return null;
149+
150+
const branch = trimmed.slice("origin/".length);
151+
152+
// Avoid shell injection; diffBase is user-controlled.
153+
if (!/^[0-9A-Za-z._/-]+$/.test(branch)) return null;
154+
155+
return branch;
156+
}
157+
158+
interface OriginFetchState {
159+
key: string;
160+
promise: Promise<void>;
161+
}
162+
163+
async function ensureOriginFetched(params: {
164+
api: APIClient;
165+
workspaceId: string;
166+
diffBase: string;
167+
refreshToken: number;
168+
originFetchRef: React.MutableRefObject<OriginFetchState | null>;
169+
}): Promise<void> {
170+
const originBranch = getOriginBranchForFetch(params.diffBase);
171+
if (!originBranch) return;
172+
173+
const key = [params.workspaceId, params.diffBase, String(params.refreshToken)].join("\u0000");
174+
const existing = params.originFetchRef.current;
175+
if (existing?.key === key) {
176+
await existing.promise;
177+
return;
178+
}
179+
180+
// Ensure manual refresh doesn't hang on credential prompts.
181+
const promise = params.api.workspace
182+
.executeBash({
183+
workspaceId: params.workspaceId,
184+
script: `GIT_TERMINAL_PROMPT=0 git fetch origin ${originBranch} --quiet || true`,
185+
options: { timeout_secs: 30 },
186+
})
187+
.then(() => undefined)
188+
.catch(() => undefined);
189+
190+
params.originFetchRef.current = { key, promise };
191+
await promise;
192+
}
121193
function makeReviewPanelCacheKey(params: {
122194
workspaceId: string;
123195
workspacePath: string;
@@ -161,6 +233,7 @@ export const ReviewPanel: React.FC<ReviewPanelProps> = ({
161233
isCreating = false,
162234
onStatsChange,
163235
}) => {
236+
const originFetchRef = useRef<OriginFetchState | null>(null);
164237
const { api } = useAPI();
165238
const panelRef = useRef<HTMLDivElement>(null);
166239
const scrollContainerRef = useRef<HTMLDivElement>(null);
@@ -309,6 +382,15 @@ export const ReviewPanel: React.FC<ReviewPanelProps> = ({
309382
const loadFileTree = async () => {
310383
setIsLoadingTree(true);
311384
try {
385+
await ensureOriginFetched({
386+
api,
387+
workspaceId,
388+
diffBase: filters.diffBase,
389+
refreshToken: refreshTrigger,
390+
originFetchRef,
391+
});
392+
if (cancelled) return;
393+
312394
const tree = await executeWorkspaceBashAndCache({
313395
api,
314396
workspaceId,
@@ -420,6 +502,15 @@ export const ReviewPanel: React.FC<ReviewPanelProps> = ({
420502

421503
const loadDiff = async () => {
422504
try {
505+
await ensureOriginFetched({
506+
api,
507+
workspaceId,
508+
diffBase: filters.diffBase,
509+
refreshToken: refreshTrigger,
510+
originFetchRef,
511+
});
512+
if (cancelled) return;
513+
423514
// Git-level filters (affect what data is fetched):
424515
// - diffBase: what to diff against
425516
// - includeUncommitted: include working directory changes
@@ -458,10 +549,19 @@ export const ReviewPanel: React.FC<ReviewPanelProps> = ({
458549
if (cancelled) return;
459550

460551
setDiagnosticInfo(data.diagnosticInfo);
461-
setDiffState({
462-
status: "loaded",
463-
hunks: data.hunks,
464-
truncationWarning: data.truncationWarning,
552+
553+
// Preserve object references for unchanged hunks to prevent unnecessary re-renders.
554+
// HunkViewer is memoized on hunk object identity, so reusing references avoids
555+
// re-rendering (and re-highlighting) hunks that haven't actually changed.
556+
setDiffState((prev) => {
557+
const prevHunks =
558+
prev.status === "loaded" || prev.status === "refreshing" ? prev.hunks : [];
559+
const hunks = preserveHunkReferences(prevHunks, data.hunks);
560+
return {
561+
status: "loaded",
562+
hunks,
563+
truncationWarning: data.truncationWarning,
564+
};
465565
});
466566

467567
if (data.hunks.length > 0) {
@@ -811,6 +911,7 @@ export const ReviewPanel: React.FC<ReviewPanelProps> = ({
811911
workspaceId={workspaceId}
812912
workspacePath={workspacePath}
813913
refreshTrigger={refreshTrigger}
914+
lastRefreshInfo={refreshController.lastRefreshInfo}
814915
/>
815916

816917
{diffState.status === "error" ? (

0 commit comments

Comments
 (0)