Skip to content

Commit 702cd47

Browse files
committed
Revert "🤖 fix: make code review diff refresh robust and predictable (#1296)"
This reverts commit 4a3de01.
1 parent 6cbe539 commit 702cd47

File tree

12 files changed

+266
-779
lines changed

12 files changed

+266
-779
lines changed

‎src/browser/App.tsx‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ 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";
2120

2221
import { useStableReference, compareMaps } from "./hooks/useStableReference";
2322
import { CommandRegistryProvider, useCommandRegistry } from "./contexts/CommandRegistryContext";
@@ -134,11 +133,6 @@ function AppInner() {
134133
prevWorkspaceRef.current = selectedWorkspace;
135134
}, [selectedWorkspace, telemetry]);
136135

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

‎src/browser/components/RightSidebar/CodeReview/RefreshButton.tsx‎

Lines changed: 4 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -5,30 +5,14 @@
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";
98
import { cn } from "@/common/lib/utils";
10-
import type { LastRefreshInfo, RefreshTrigger } from "@/browser/utils/RefreshController";
119

1210
interface RefreshButtonProps {
1311
onClick: () => void;
1412
isLoading?: boolean;
15-
/** Debug info about last refresh (timestamp and trigger) */
16-
lastRefreshInfo?: LastRefreshInfo | null;
1713
}
1814

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;
15+
export const RefreshButton: React.FC<RefreshButtonProps> = ({ onClick, isLoading = false }) => {
3216
// Track animation state for graceful stopping
3317
const [animationState, setAnimationState] = useState<"idle" | "spinning" | "stopping">("idle");
3418
const spinOnceTimeoutRef = useRef<number | null>(null);
@@ -92,19 +76,9 @@ export const RefreshButton: React.FC<RefreshButtonProps> = (props) => {
9276
</button>
9377
</TooltipTrigger>
9478
<TooltipContent side="bottom" align="start">
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-
)}
79+
{animationState !== "idle"
80+
? "Refreshing..."
81+
: `Refresh diff (${formatKeybind(KEYBINDS.REFRESH_REVIEW)})`}
10882
</TooltipContent>
10983
</Tooltip>
11084
);

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

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
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";
98
import { RefreshButton } from "./RefreshButton";
109
import { UntrackedStatus } from "./UntrackedStatus";
1110

@@ -18,8 +17,6 @@ interface ReviewControlsProps {
1817
workspaceId: string;
1918
workspacePath: string;
2019
refreshTrigger?: number;
21-
/** Debug info about last refresh */
22-
lastRefreshInfo?: LastRefreshInfo | null;
2320
}
2421

2522
export const ReviewControls: React.FC<ReviewControlsProps> = ({
@@ -31,7 +28,6 @@ export const ReviewControls: React.FC<ReviewControlsProps> = ({
3128
workspaceId,
3229
workspacePath,
3330
refreshTrigger,
34-
lastRefreshInfo,
3531
}) => {
3632
// Local state for input value - only commit on blur/Enter
3733
const [inputValue, setInputValue] = useState(filters.diffBase);
@@ -87,13 +83,7 @@ export const ReviewControls: React.FC<ReviewControlsProps> = ({
8783

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

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

Lines changed: 4 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -94,31 +94,6 @@ 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-
12297
interface ReviewPanelDiffCacheValue {
12398
hunks: DiffHunk[];
12499
truncationWarning: string | null;
@@ -143,53 +118,6 @@ const reviewPanelCache = new LRUCache<string, ReviewPanelCacheValue>({
143118
sizeCalculation: (value) => estimateJsonSizeBytes(value),
144119
});
145120

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-
}
193121
function makeReviewPanelCacheKey(params: {
194122
workspaceId: string;
195123
workspacePath: string;
@@ -233,7 +161,6 @@ export const ReviewPanel: React.FC<ReviewPanelProps> = ({
233161
isCreating = false,
234162
onStatsChange,
235163
}) => {
236-
const originFetchRef = useRef<OriginFetchState | null>(null);
237164
const { api } = useAPI();
238165
const panelRef = useRef<HTMLDivElement>(null);
239166
const scrollContainerRef = useRef<HTMLDivElement>(null);
@@ -382,15 +309,6 @@ export const ReviewPanel: React.FC<ReviewPanelProps> = ({
382309
const loadFileTree = async () => {
383310
setIsLoadingTree(true);
384311
try {
385-
await ensureOriginFetched({
386-
api,
387-
workspaceId,
388-
diffBase: filters.diffBase,
389-
refreshToken: refreshTrigger,
390-
originFetchRef,
391-
});
392-
if (cancelled) return;
393-
394312
const tree = await executeWorkspaceBashAndCache({
395313
api,
396314
workspaceId,
@@ -502,15 +420,6 @@ export const ReviewPanel: React.FC<ReviewPanelProps> = ({
502420

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

551460
setDiagnosticInfo(data.diagnosticInfo);
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-
};
461+
setDiffState({
462+
status: "loaded",
463+
hunks: data.hunks,
464+
truncationWarning: data.truncationWarning,
565465
});
566466

567467
if (data.hunks.length > 0) {
@@ -911,7 +811,6 @@ export const ReviewPanel: React.FC<ReviewPanelProps> = ({
911811
workspaceId={workspaceId}
912812
workspacePath={workspacePath}
913813
refreshTrigger={refreshTrigger}
914-
lastRefreshInfo={refreshController.lastRefreshInfo}
915814
/>
916815

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

0 commit comments

Comments
 (0)