From dae4689dc1e28972a13d9908fc50f9d856c64e1f Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 14 Dec 2025 12:00:20 -0600 Subject: [PATCH 1/8] =?UTF-8?q?=F0=9F=A4=96=20perf:=20cache=20review=20pan?= =?UTF-8?q?el=20diff/tree?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../RightSidebar/CodeReview/ReviewPanel.tsx | 348 +++++++++++++----- .../utils/review/reviewPanelCache.test.ts | 166 +++++++++ src/browser/utils/review/reviewPanelCache.ts | 176 +++++++++ 3 files changed, 588 insertions(+), 102 deletions(-) create mode 100644 src/browser/utils/review/reviewPanelCache.test.ts create mode 100644 src/browser/utils/review/reviewPanelCache.ts diff --git a/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx b/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx index 9d6774c4be..9b4cf9d293 100644 --- a/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx +++ b/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx @@ -39,6 +39,19 @@ import type { } from "@/common/types/review"; import type { FileTreeNode } from "@/common/utils/git/numstatParser"; import { matchesKeybind, KEYBINDS, formatKeybind } from "@/browser/utils/ui/keybinds"; +import { + getCachedReviewDiff, + getCachedReviewFileTree, + getInFlightReviewDiff, + getInFlightReviewFileTree, + makeReviewDiffCacheKey, + makeReviewFileTreeCacheKey, + setCachedReviewDiff, + setCachedReviewFileTree, + setInFlightReviewDiff, + setInFlightReviewFileTree, +} from "@/browser/utils/review/reviewPanelCache"; +import type { ReviewPanelDiagnosticInfo } from "@/browser/utils/review/reviewPanelCache"; import { applyFrontendFilters } from "@/browser/utils/review/filterHunks"; import { cn } from "@/common/lib/utils"; import { useAPI } from "@/browser/contexts/API"; @@ -67,12 +80,7 @@ interface ReviewSearchState { matchCase: boolean; } -interface DiagnosticInfo { - command: string; - outputLength: number; - fileDiffCount: number; - hunkCount: number; -} +type DiagnosticInfo = ReviewPanelDiagnosticInfo; /** * Discriminated union for diff loading state. @@ -109,9 +117,15 @@ export const ReviewPanel: React.FC = ({ const [isPanelFocused, setIsPanelFocused] = useState(false); const [refreshTrigger, setRefreshTrigger] = useState(0); const [fileTree, setFileTree] = useState(null); + // Map of hunkId -> toggle function for expand/collapse const toggleExpandFnsRef = useRef void>>(new Map()); + // Track refresh trigger changes so we can distinguish initial mount vs manual refresh. + // Each effect gets its own ref to avoid cross-effect interference. + const lastDiffRefreshTriggerRef = useRef(null); + const lastFileTreeRefreshTriggerRef = useRef(null); + // Unified search state (per-workspace persistence) const [searchState, setSearchState] = usePersistedState( getReviewSearchStateKey(workspaceId), @@ -177,40 +191,96 @@ export const ReviewPanel: React.FC = ({ if (!api || isCreating) return; let cancelled = false; - const loadFileTree = async () => { + const prevRefreshTrigger = lastFileTreeRefreshTriggerRef.current; + lastFileTreeRefreshTriggerRef.current = refreshTrigger; + const isManualRefresh = prevRefreshTrigger !== null && prevRefreshTrigger !== refreshTrigger; + + const cacheKey = makeReviewFileTreeCacheKey({ + workspaceId, + workspacePath, + diffBase: filters.diffBase, + includeUncommitted: filters.includeUncommitted, + }); + + // Fast path: use cached tree when switching workspaces (unless user explicitly refreshed). + if (!isManualRefresh) { + const cached = getCachedReviewFileTree(cacheKey); + if (cached) { + setFileTree(cached.fileTree); + setIsLoadingTree(false); + return () => { + cancelled = true; + }; + } + } + + // If another panel instance is already loading this tree, reuse it. + const inFlight = getInFlightReviewFileTree(cacheKey); + if (inFlight) { setIsLoadingTree(true); - try { - const numstatCommand = buildGitDiffCommand( - filters.diffBase, - filters.includeUncommitted, - "", // No path filter for file tree - "numstat" - ); - - const numstatResult = await api.workspace.executeBash({ - workspaceId, - script: numstatCommand, - options: { timeout_secs: 30 }, + void inFlight + .then((data) => { + if (cancelled) return; + setFileTree(data.fileTree); + }) + .catch((err) => { + console.error("Failed to load file tree:", err); + }) + .finally(() => { + if (cancelled) return; + setIsLoadingTree(false); }); - if (cancelled) return; + return () => { + cancelled = true; + }; + } - if (numstatResult.success) { - const numstatOutput = numstatResult.data.output ?? ""; - const fileStats = parseNumstat(numstatOutput); + setIsLoadingTree(true); - // Build tree with original paths (needed for git commands) - const tree = buildFileTree(fileStats); - setFileTree(tree); - } - } catch (err) { - console.error("Failed to load file tree:", err); - } finally { - setIsLoadingTree(false); + const loadPromise = (async () => { + const numstatCommand = buildGitDiffCommand( + filters.diffBase, + filters.includeUncommitted, + "", // No path filter for file tree + "numstat" + ); + + const numstatResult = await api.workspace.executeBash({ + workspaceId, + script: numstatCommand, + options: { timeout_secs: 30 }, + }); + + if (!numstatResult.success) { + throw new Error(numstatResult.error ?? "Unknown error"); } - }; - void loadFileTree(); + const numstatOutput = numstatResult.data.output ?? ""; + const fileStats = parseNumstat(numstatOutput); + + // Build tree with original paths (needed for git commands) + const tree = buildFileTree(fileStats); + + const value = { fileTree: tree }; + setCachedReviewFileTree(cacheKey, value); + return value; + })(); + + setInFlightReviewFileTree(cacheKey, loadPromise); + + void loadPromise + .then((data) => { + if (cancelled) return; + setFileTree(data.fileTree); + }) + .catch((err) => { + console.error("Failed to load file tree:", err); + }) + .finally(() => { + if (cancelled) return; + setIsLoadingTree(false); + }); return () => { cancelled = true; @@ -231,97 +301,171 @@ export const ReviewPanel: React.FC = ({ if (!api || isCreating) return; let cancelled = false; - // Transition to appropriate loading state: - // - "refreshing" if we have data (keeps UI stable during refresh) - // - "loading" if no data yet - setDiffState((prev) => { - if (prev.status === "loaded" || prev.status === "refreshing") { - return { - status: "refreshing", - hunks: prev.hunks, - truncationWarning: prev.truncationWarning, - }; - } - return { status: "loading" }; + const prevRefreshTrigger = lastDiffRefreshTriggerRef.current; + lastDiffRefreshTriggerRef.current = refreshTrigger; + const isManualRefresh = prevRefreshTrigger !== null && prevRefreshTrigger !== refreshTrigger; + + const cacheKey = makeReviewDiffCacheKey({ + workspaceId, + workspacePath, + diffBase: filters.diffBase, + includeUncommitted: filters.includeUncommitted, + selectedFilePath, }); - const loadDiff = async () => { - try { - // Git-level filters (affect what data is fetched): - // - diffBase: what to diff against - // - includeUncommitted: include working directory changes - // - selectedFilePath: ESSENTIAL for truncation - if full diff is cut off, - // path filter lets us retrieve specific file's hunks - const pathFilter = selectedFilePath ? ` -- "${extractNewPath(selectedFilePath)}"` : ""; - - const diffCommand = buildGitDiffCommand( - filters.diffBase, - filters.includeUncommitted, - pathFilter, - "diff" - ); - - // Fetch diff - const diffResult = await api.workspace.executeBash({ - workspaceId, - script: diffCommand, - options: { timeout_secs: 30 }, + const inFlight = getInFlightReviewDiff(cacheKey); + + // Fast path: show cached diff immediately when switching workspaces. + // If a refresh is already in-flight, keep the cached diff visible and update when it completes. + if (!isManualRefresh) { + const cached = getCachedReviewDiff(cacheKey); + if (cached) { + setDiagnosticInfo(cached.diagnosticInfo); + setDiffState({ + status: inFlight ? "refreshing" : "loaded", + hunks: cached.hunks, + truncationWarning: cached.truncationWarning, }); - if (cancelled) return; - - if (!diffResult.success) { - // Real error (not truncation-related) - console.error("Git diff failed:", diffResult.error); - setDiffState({ status: "error", message: diffResult.error ?? "Unknown error" }); - setDiagnosticInfo(null); - return; + if (cached.hunks.length > 0) { + setSelectedHunkId((prev) => prev ?? cached.hunks[0].id); } - const diffOutput = diffResult.data.output ?? ""; - const truncationInfo = - "truncated" in diffResult.data ? diffResult.data.truncated : undefined; + if (!inFlight) { + return () => { + cancelled = true; + }; + } + } + } - const fileDiffs = parseDiff(diffOutput); - const allHunks = extractAllHunks(fileDiffs); + const transitionToLoadingState = () => { + // - "refreshing" if we have data (keeps UI stable during refresh) + // - "loading" if no data yet + setDiffState((prev) => { + if (prev.status === "loaded" || prev.status === "refreshing") { + return { + status: "refreshing", + hunks: prev.hunks, + truncationWarning: prev.truncationWarning, + }; + } + return { status: "loading" }; + }); + }; - // Store diagnostic info - setDiagnosticInfo({ - command: diffCommand, - outputLength: diffOutput.length, - fileDiffCount: fileDiffs.length, - hunkCount: allHunks.length, + // If another panel instance is already loading this diff, reuse it. + if (inFlight) { + transitionToLoadingState(); + + void inFlight + .then((data) => { + if (cancelled) return; + setDiagnosticInfo(data.diagnosticInfo); + setDiffState({ + status: "loaded", + hunks: data.hunks, + truncationWarning: data.truncationWarning, + }); + + if (data.hunks.length > 0) { + setSelectedHunkId((prev) => prev ?? data.hunks[0].id); + } + }) + .catch((err) => { + if (cancelled) return; + const errorMsg = `Failed to load diff: ${err instanceof Error ? err.message : String(err)}`; + console.error(errorMsg); + setDiffState({ status: "error", message: errorMsg }); + setDiagnosticInfo(null); }); - // Build truncation warning (only when not filtering by path) - const truncationWarning = - truncationInfo && !selectedFilePath - ? `Diff truncated (${truncationInfo.reason}). Filter by file to see more.` - : null; + return () => { + cancelled = true; + }; + } - // Single atomic state update with all data - setDiffState({ status: "loaded", hunks: allHunks, truncationWarning }); + transitionToLoadingState(); + + const loadPromise = (async () => { + // Git-level filters (affect what data is fetched): + // - diffBase: what to diff against + // - includeUncommitted: include working directory changes + // - selectedFilePath: ESSENTIAL for truncation - if full diff is cut off, + // path filter lets us retrieve specific file's hunks + const pathFilter = selectedFilePath ? ` -- "${extractNewPath(selectedFilePath)}"` : ""; + + const diffCommand = buildGitDiffCommand( + filters.diffBase, + filters.includeUncommitted, + pathFilter, + "diff" + ); + + // Fetch diff + const diffResult = await api.workspace.executeBash({ + workspaceId, + script: diffCommand, + options: { timeout_secs: 30 }, + }); - // Auto-select first hunk if none selected - if (allHunks.length > 0 && !selectedHunkId) { - setSelectedHunkId(allHunks[0].id); + if (!diffResult.success) { + throw new Error(diffResult.error ?? "Unknown error"); + } + + const diffOutput = diffResult.data.output ?? ""; + const truncationInfo = "truncated" in diffResult.data ? diffResult.data.truncated : undefined; + + const fileDiffs = parseDiff(diffOutput); + const allHunks = extractAllHunks(fileDiffs); + + const diagnosticInfo: DiagnosticInfo = { + command: diffCommand, + outputLength: diffOutput.length, + fileDiffCount: fileDiffs.length, + hunkCount: allHunks.length, + }; + + // Build truncation warning (only when not filtering by path) + const truncationWarning = + truncationInfo && !selectedFilePath + ? `Diff truncated (${truncationInfo.reason}). Filter by file to see more.` + : null; + + const value = { hunks: allHunks, truncationWarning, diagnosticInfo }; + setCachedReviewDiff(cacheKey, value); + return value; + })(); + + setInFlightReviewDiff(cacheKey, loadPromise); + + void loadPromise + .then((data) => { + if (cancelled) return; + setDiagnosticInfo(data.diagnosticInfo); + setDiffState({ + status: "loaded", + hunks: data.hunks, + truncationWarning: data.truncationWarning, + }); + + if (data.hunks.length > 0) { + setSelectedHunkId((prev) => prev ?? data.hunks[0].id); } - } catch (err) { + }) + .catch((err) => { if (cancelled) return; const errorMsg = `Failed to load diff: ${err instanceof Error ? err.message : String(err)}`; console.error(errorMsg); setDiffState({ status: "error", message: errorMsg }); - } - }; - - void loadDiff(); + setDiagnosticInfo(null); + }); return () => { cancelled = true; }; - // selectedHunkId intentionally omitted - only auto-select on initial load, not on every selection change - // eslint-disable-next-line react-hooks/exhaustive-deps }, [ + api, workspaceId, workspacePath, filters.diffBase, diff --git a/src/browser/utils/review/reviewPanelCache.test.ts b/src/browser/utils/review/reviewPanelCache.test.ts new file mode 100644 index 0000000000..d231b76a6c --- /dev/null +++ b/src/browser/utils/review/reviewPanelCache.test.ts @@ -0,0 +1,166 @@ +import { beforeEach, describe, expect, test } from "bun:test"; +import type { DiffHunk } from "@/common/types/review"; +import type { FileTreeNode } from "@/common/utils/git/numstatParser"; +import { + clearReviewPanelCaches, + getCachedReviewDiff, + getCachedReviewFileTree, + getInFlightReviewDiff, + getInFlightReviewFileTree, + makeReviewDiffCacheKey, + makeReviewFileTreeCacheKey, + setCachedReviewDiff, + setCachedReviewFileTree, + setInFlightReviewDiff, + setInFlightReviewFileTree, +} from "./reviewPanelCache"; + +describe("reviewPanelCache", () => { + beforeEach(() => { + clearReviewPanelCaches(); + }); + + test("makeReviewDiffCacheKey is sensitive to selectedFilePath", () => { + const base = { + workspaceId: "ws", + workspacePath: "/tmp/ws", + diffBase: "HEAD", + includeUncommitted: false, + }; + + const keyA = makeReviewDiffCacheKey({ ...base, selectedFilePath: null }); + const keyB = makeReviewDiffCacheKey({ ...base, selectedFilePath: "src/a.ts" }); + + expect(keyA).not.toEqual(keyB); + }); + + test("makeReviewFileTreeCacheKey does not include selectedFilePath", () => { + const key = makeReviewFileTreeCacheKey({ + workspaceId: "ws", + workspacePath: "/tmp/ws", + diffBase: "HEAD", + includeUncommitted: true, + }); + + expect(key).toContain("review-panel-tree"); + }); + + test("diff cache round-trips", () => { + const key = makeReviewDiffCacheKey({ + workspaceId: "ws", + workspacePath: "/tmp/ws", + diffBase: "HEAD", + includeUncommitted: false, + selectedFilePath: null, + }); + + const hunk: DiffHunk = { + id: "h1", + filePath: "src/a.ts", + oldStart: 1, + oldLines: 1, + newStart: 1, + newLines: 1, + content: "+console.log('hi')", + header: "@@ -1 +1 @@", + changeType: "modified", + }; + + setCachedReviewDiff(key, { + hunks: [hunk], + truncationWarning: null, + diagnosticInfo: { + command: "git diff", + outputLength: 123, + fileDiffCount: 1, + hunkCount: 1, + }, + }); + + expect(getCachedReviewDiff(key)?.hunks[0].id).toEqual("h1"); + }); + + test("file tree cache round-trips", () => { + const key = makeReviewFileTreeCacheKey({ + workspaceId: "ws", + workspacePath: "/tmp/ws", + diffBase: "HEAD", + includeUncommitted: false, + }); + + const tree: FileTreeNode = { + name: "", + path: "", + isDirectory: true, + children: [], + totalStats: { filePath: "", additions: 0, deletions: 0 }, + }; + + setCachedReviewFileTree(key, { fileTree: tree }); + + expect(getCachedReviewFileTree(key)?.fileTree.isDirectory).toEqual(true); + }); + + test("inFlight diff is cleared after settle", async () => { + const key = makeReviewDiffCacheKey({ + workspaceId: "ws", + workspacePath: "/tmp/ws", + diffBase: "HEAD", + includeUncommitted: false, + selectedFilePath: null, + }); + + let resolve!: (value: { + hunks: DiffHunk[]; + truncationWarning: string | null; + diagnosticInfo: null; + }) => void; + + const promise = new Promise<{ + hunks: DiffHunk[]; + truncationWarning: string | null; + diagnosticInfo: null; + }>((r) => { + resolve = r; + }); + + setInFlightReviewDiff(key, promise); + expect(getInFlightReviewDiff(key)).toBe(promise); + + resolve({ hunks: [], truncationWarning: null, diagnosticInfo: null }); + await promise; + + expect(getInFlightReviewDiff(key)).toBeNull(); + }); + + test("inFlight file tree is cleared after settle", async () => { + const key = makeReviewFileTreeCacheKey({ + workspaceId: "ws", + workspacePath: "/tmp/ws", + diffBase: "HEAD", + includeUncommitted: false, + }); + + let resolve!: (value: { fileTree: FileTreeNode }) => void; + + const tree: FileTreeNode = { + name: "", + path: "", + isDirectory: true, + children: [], + totalStats: { filePath: "", additions: 0, deletions: 0 }, + }; + + const promise = new Promise<{ fileTree: FileTreeNode }>((r) => { + resolve = r; + }); + + setInFlightReviewFileTree(key, promise); + expect(getInFlightReviewFileTree(key)).toBe(promise); + + resolve({ fileTree: tree }); + await promise; + + expect(getInFlightReviewFileTree(key)).toBeNull(); + }); +}); diff --git a/src/browser/utils/review/reviewPanelCache.ts b/src/browser/utils/review/reviewPanelCache.ts new file mode 100644 index 0000000000..a885706d45 --- /dev/null +++ b/src/browser/utils/review/reviewPanelCache.ts @@ -0,0 +1,176 @@ +/** + * In-memory caches for the Code Review panel. + * + * Motivation: switching workspaces can trigger expensive git diff/numstat calls. + * We keep the most recent results in an LRU so returning to a workspace is instant. + * + * Refresh semantics: + * - Cache entries are updated on successful loads. + * - Consumers can bypass the cache on manual refresh (Ctrl/Cmd+R). + */ + +import { LRUCache } from "lru-cache"; +import type { DiffHunk } from "@/common/types/review"; +import type { FileTreeNode } from "@/common/utils/git/numstatParser"; + +export interface ReviewPanelDiagnosticInfo { + command: string; + outputLength: number; + fileDiffCount: number; + hunkCount: number; +} + +export interface ReviewPanelDiffCacheValue { + hunks: DiffHunk[]; + truncationWarning: string | null; + diagnosticInfo: ReviewPanelDiagnosticInfo | null; +} + +export interface ReviewPanelFileTreeCacheValue { + fileTree: FileTreeNode; +} + +interface CacheEntry { + value: T; + cachedAt: number; +} + +function estimateHunksSizeBytes(hunks: DiffHunk[]): number { + // Rough bytes for JS strings (UTF-16) + a small constant per object. + let bytes = hunks.length * 64; + for (const hunk of hunks) { + bytes += + (hunk.id.length + + hunk.filePath.length + + hunk.content.length + + hunk.header.length + + (hunk.oldPath?.length ?? 0) + + (hunk.changeType?.length ?? 0)) * + 2; + } + return bytes; +} + +function estimateFileTreeSizeBytes(node: FileTreeNode): number { + // Rough bytes for JS strings + a constant per node. + let bytes = 64 + (node.name.length + node.path.length) * 2; + if (node.stats) { + bytes += node.stats.filePath.length * 2; + } + if (node.totalStats) { + bytes += node.totalStats.filePath.length * 2; + } + for (const child of node.children) { + bytes += estimateFileTreeSizeBytes(child); + } + return bytes; +} + +const DIFF_CACHE_MAX_SIZE_BYTES = 16 * 1024 * 1024; // 16MB +const FILE_TREE_CACHE_MAX_SIZE_BYTES = 4 * 1024 * 1024; // 4MB + +const diffCache = new LRUCache>({ + max: 25, + maxSize: DIFF_CACHE_MAX_SIZE_BYTES, + sizeCalculation: (entry) => estimateHunksSizeBytes(entry.value.hunks), +}); + +const fileTreeCache = new LRUCache>({ + max: 25, + maxSize: FILE_TREE_CACHE_MAX_SIZE_BYTES, + sizeCalculation: (entry) => estimateFileTreeSizeBytes(entry.value.fileTree), +}); + +const inFlightDiffLoads = new Map>(); +const inFlightFileTreeLoads = new Map>(); + +export function makeReviewDiffCacheKey(params: { + workspaceId: string; + workspacePath: string; + diffBase: string; + includeUncommitted: boolean; + selectedFilePath: string | null; +}): string { + // Use a null byte separator to avoid accidental collisions. + return [ + "review-panel-diff:v1", + params.workspaceId, + params.workspacePath, + params.diffBase, + params.includeUncommitted ? "1" : "0", + params.selectedFilePath ?? "", + ].join("\u0000"); +} + +export function makeReviewFileTreeCacheKey(params: { + workspaceId: string; + workspacePath: string; + diffBase: string; + includeUncommitted: boolean; +}): string { + return [ + "review-panel-tree:v1", + params.workspaceId, + params.workspacePath, + params.diffBase, + params.includeUncommitted ? "1" : "0", + ].join("\u0000"); +} + +export function getCachedReviewDiff(key: string): ReviewPanelDiffCacheValue | null { + return diffCache.get(key)?.value ?? null; +} + +export function setCachedReviewDiff(key: string, value: ReviewPanelDiffCacheValue): void { + diffCache.set(key, { value, cachedAt: Date.now() }); +} + +export function getCachedReviewFileTree(key: string): ReviewPanelFileTreeCacheValue | null { + return fileTreeCache.get(key)?.value ?? null; +} + +export function setCachedReviewFileTree(key: string, value: ReviewPanelFileTreeCacheValue): void { + fileTreeCache.set(key, { value, cachedAt: Date.now() }); +} + +export function getInFlightReviewDiff(key: string): Promise | null { + return inFlightDiffLoads.get(key) ?? null; +} + +export function setInFlightReviewDiff( + key: string, + promise: Promise +): void { + inFlightDiffLoads.set(key, promise); + void promise.finally(() => { + if (inFlightDiffLoads.get(key) === promise) { + inFlightDiffLoads.delete(key); + } + }); +} + +export function getInFlightReviewFileTree( + key: string +): Promise | null { + return inFlightFileTreeLoads.get(key) ?? null; +} + +export function setInFlightReviewFileTree( + key: string, + promise: Promise +): void { + inFlightFileTreeLoads.set(key, promise); + void promise.finally(() => { + if (inFlightFileTreeLoads.get(key) === promise) { + inFlightFileTreeLoads.delete(key); + } + }); +} + +/** For tests / debugging. */ +export function clearReviewPanelCaches(): void { + diffCache.clear(); + fileTreeCache.clear(); + inFlightDiffLoads.clear(); + inFlightFileTreeLoads.clear(); +} From f5cd8894f989b957d3da2202fcbc6d3f1385b8bc Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 14 Dec 2025 12:15:52 -0600 Subject: [PATCH 2/8] =?UTF-8?q?=F0=9F=A4=96=20perf:=20simplify=20review=20?= =?UTF-8?q?panel=20caching?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../RightSidebar/CodeReview/ReviewPanel.tsx | 372 +++++++++--------- .../utils/review/reviewPanelCache.test.ts | 166 -------- src/browser/utils/review/reviewPanelCache.ts | 176 --------- 3 files changed, 181 insertions(+), 533 deletions(-) delete mode 100644 src/browser/utils/review/reviewPanelCache.test.ts delete mode 100644 src/browser/utils/review/reviewPanelCache.ts diff --git a/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx b/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx index 9b4cf9d293..df5b0753e5 100644 --- a/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx +++ b/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx @@ -39,19 +39,6 @@ import type { } from "@/common/types/review"; import type { FileTreeNode } from "@/common/utils/git/numstatParser"; import { matchesKeybind, KEYBINDS, formatKeybind } from "@/browser/utils/ui/keybinds"; -import { - getCachedReviewDiff, - getCachedReviewFileTree, - getInFlightReviewDiff, - getInFlightReviewFileTree, - makeReviewDiffCacheKey, - makeReviewFileTreeCacheKey, - setCachedReviewDiff, - setCachedReviewFileTree, - setInFlightReviewDiff, - setInFlightReviewFileTree, -} from "@/browser/utils/review/reviewPanelCache"; -import type { ReviewPanelDiagnosticInfo } from "@/browser/utils/review/reviewPanelCache"; import { applyFrontendFilters } from "@/browser/utils/review/filterHunks"; import { cn } from "@/common/lib/utils"; import { useAPI } from "@/browser/contexts/API"; @@ -80,7 +67,12 @@ interface ReviewSearchState { matchCase: boolean; } -type DiagnosticInfo = ReviewPanelDiagnosticInfo; +interface DiagnosticInfo { + command: string; + outputLength: number; + fileDiffCount: number; + hunkCount: number; +} /** * Discriminated union for diff loading state. @@ -95,6 +87,69 @@ type DiffState = | { status: "loaded"; hunks: DiffHunk[]; truncationWarning: string | null } | { status: "error"; message: string }; +const REVIEW_PANEL_CACHE_MAX_ENTRIES = 10; + +interface ReviewPanelDiffCacheValue { + hunks: DiffHunk[]; + truncationWarning: string | null; + diagnosticInfo: DiagnosticInfo | null; +} + +const reviewPanelDiffCache = new Map(); +const reviewPanelFileTreeCache = new Map(); + +function evictOldestCacheEntry(map: Map): void { + const first = map.keys().next(); + if (!first.done) { + map.delete(first.value); + } +} + +function setCacheWithEviction(map: Map, key: string, value: T): void { + // Refresh insertion order so eviction behaves roughly like LRU. + if (map.has(key)) { + map.delete(key); + } + map.set(key, value); + + if (map.size > REVIEW_PANEL_CACHE_MAX_ENTRIES) { + evictOldestCacheEntry(map); + } +} + +function makeReviewDiffCacheKey(params: { + workspaceId: string; + workspacePath: string; + diffBase: string; + includeUncommitted: boolean; + selectedFilePath: string | null; +}): string { + // Null byte separator avoids accidental collisions. + return [ + "review-panel-diff:v1", + params.workspaceId, + params.workspacePath, + params.diffBase, + params.includeUncommitted ? "1" : "0", + params.selectedFilePath ?? "", + ].join("\u0000"); +} + +function makeReviewFileTreeCacheKey(params: { + workspaceId: string; + workspacePath: string; + diffBase: string; + includeUncommitted: boolean; +}): string { + return [ + "review-panel-tree:v1", + params.workspaceId, + params.workspacePath, + params.diffBase, + params.includeUncommitted ? "1" : "0", + ].join("\u0000"); +} + export const ReviewPanel: React.FC = ({ workspaceId, workspacePath, @@ -204,9 +259,9 @@ export const ReviewPanel: React.FC = ({ // Fast path: use cached tree when switching workspaces (unless user explicitly refreshed). if (!isManualRefresh) { - const cached = getCachedReviewFileTree(cacheKey); - if (cached) { - setFileTree(cached.fileTree); + const cachedTree = reviewPanelFileTreeCache.get(cacheKey); + if (cachedTree) { + setFileTree(cachedTree); setIsLoadingTree(false); return () => { cancelled = true; @@ -214,73 +269,46 @@ export const ReviewPanel: React.FC = ({ } } - // If another panel instance is already loading this tree, reuse it. - const inFlight = getInFlightReviewFileTree(cacheKey); - if (inFlight) { + const loadFileTree = async () => { setIsLoadingTree(true); - void inFlight - .then((data) => { - if (cancelled) return; - setFileTree(data.fileTree); - }) - .catch((err) => { - console.error("Failed to load file tree:", err); - }) - .finally(() => { - if (cancelled) return; - setIsLoadingTree(false); + try { + const numstatCommand = buildGitDiffCommand( + filters.diffBase, + filters.includeUncommitted, + "", // No path filter for file tree + "numstat" + ); + + const numstatResult = await api.workspace.executeBash({ + workspaceId, + script: numstatCommand, + options: { timeout_secs: 30 }, }); - return () => { - cancelled = true; - }; - } - - setIsLoadingTree(true); - - const loadPromise = (async () => { - const numstatCommand = buildGitDiffCommand( - filters.diffBase, - filters.includeUncommitted, - "", // No path filter for file tree - "numstat" - ); - - const numstatResult = await api.workspace.executeBash({ - workspaceId, - script: numstatCommand, - options: { timeout_secs: 30 }, - }); - - if (!numstatResult.success) { - throw new Error(numstatResult.error ?? "Unknown error"); - } - - const numstatOutput = numstatResult.data.output ?? ""; - const fileStats = parseNumstat(numstatOutput); + if (!numstatResult.success) { + throw new Error(numstatResult.error ?? "Unknown error"); + } - // Build tree with original paths (needed for git commands) - const tree = buildFileTree(fileStats); + const numstatOutput = numstatResult.data.output ?? ""; + const fileStats = parseNumstat(numstatOutput); - const value = { fileTree: tree }; - setCachedReviewFileTree(cacheKey, value); - return value; - })(); + // Build tree with original paths (needed for git commands) + const tree = buildFileTree(fileStats); - setInFlightReviewFileTree(cacheKey, loadPromise); + setCacheWithEviction(reviewPanelFileTreeCache, cacheKey, tree); - void loadPromise - .then((data) => { if (cancelled) return; - setFileTree(data.fileTree); - }) - .catch((err) => { + setFileTree(tree); + } catch (err) { console.error("Failed to load file tree:", err); - }) - .finally(() => { - if (cancelled) return; - setIsLoadingTree(false); - }); + } finally { + if (!cancelled) { + setIsLoadingTree(false); + } + } + }; + + void loadFileTree(); return () => { cancelled = true; @@ -313,16 +341,13 @@ export const ReviewPanel: React.FC = ({ selectedFilePath, }); - const inFlight = getInFlightReviewDiff(cacheKey); - - // Fast path: show cached diff immediately when switching workspaces. - // If a refresh is already in-flight, keep the cached diff visible and update when it completes. + // Fast path: use cached diff when switching workspaces (unless user explicitly refreshed). if (!isManualRefresh) { - const cached = getCachedReviewDiff(cacheKey); + const cached = reviewPanelDiffCache.get(cacheKey); if (cached) { setDiagnosticInfo(cached.diagnosticInfo); setDiffState({ - status: inFlight ? "refreshing" : "loaded", + status: "loaded", hunks: cached.hunks, truncationWarning: cached.truncationWarning, }); @@ -331,135 +356,100 @@ export const ReviewPanel: React.FC = ({ setSelectedHunkId((prev) => prev ?? cached.hunks[0].id); } - if (!inFlight) { - return () => { - cancelled = true; - }; - } + return () => { + cancelled = true; + }; } } - const transitionToLoadingState = () => { - // - "refreshing" if we have data (keeps UI stable during refresh) - // - "loading" if no data yet - setDiffState((prev) => { - if (prev.status === "loaded" || prev.status === "refreshing") { - return { - status: "refreshing", - hunks: prev.hunks, - truncationWarning: prev.truncationWarning, - }; - } - return { status: "loading" }; - }); - }; + // Transition to appropriate loading state: + // - "refreshing" if we have data (keeps UI stable during refresh) + // - "loading" if no data yet + setDiffState((prev) => { + if (prev.status === "loaded" || prev.status === "refreshing") { + return { + status: "refreshing", + hunks: prev.hunks, + truncationWarning: prev.truncationWarning, + }; + } + return { status: "loading" }; + }); - // If another panel instance is already loading this diff, reuse it. - if (inFlight) { - transitionToLoadingState(); - - void inFlight - .then((data) => { - if (cancelled) return; - setDiagnosticInfo(data.diagnosticInfo); - setDiffState({ - status: "loaded", - hunks: data.hunks, - truncationWarning: data.truncationWarning, - }); - - if (data.hunks.length > 0) { - setSelectedHunkId((prev) => prev ?? data.hunks[0].id); - } - }) - .catch((err) => { - if (cancelled) return; - const errorMsg = `Failed to load diff: ${err instanceof Error ? err.message : String(err)}`; - console.error(errorMsg); - setDiffState({ status: "error", message: errorMsg }); - setDiagnosticInfo(null); + const loadDiff = async () => { + try { + // Git-level filters (affect what data is fetched): + // - diffBase: what to diff against + // - includeUncommitted: include working directory changes + // - selectedFilePath: ESSENTIAL for truncation - if full diff is cut off, + // path filter lets us retrieve specific file's hunks + const pathFilter = selectedFilePath ? ` -- "${extractNewPath(selectedFilePath)}"` : ""; + + const diffCommand = buildGitDiffCommand( + filters.diffBase, + filters.includeUncommitted, + pathFilter, + "diff" + ); + + // Fetch diff + const diffResult = await api.workspace.executeBash({ + workspaceId, + script: diffCommand, + options: { timeout_secs: 30 }, }); - return () => { - cancelled = true; - }; - } - - transitionToLoadingState(); - - const loadPromise = (async () => { - // Git-level filters (affect what data is fetched): - // - diffBase: what to diff against - // - includeUncommitted: include working directory changes - // - selectedFilePath: ESSENTIAL for truncation - if full diff is cut off, - // path filter lets us retrieve specific file's hunks - const pathFilter = selectedFilePath ? ` -- "${extractNewPath(selectedFilePath)}"` : ""; - - const diffCommand = buildGitDiffCommand( - filters.diffBase, - filters.includeUncommitted, - pathFilter, - "diff" - ); - - // Fetch diff - const diffResult = await api.workspace.executeBash({ - workspaceId, - script: diffCommand, - options: { timeout_secs: 30 }, - }); - - if (!diffResult.success) { - throw new Error(diffResult.error ?? "Unknown error"); - } - - const diffOutput = diffResult.data.output ?? ""; - const truncationInfo = "truncated" in diffResult.data ? diffResult.data.truncated : undefined; + if (cancelled) return; - const fileDiffs = parseDiff(diffOutput); - const allHunks = extractAllHunks(fileDiffs); + if (!diffResult.success) { + // Real error (not truncation-related) + console.error("Git diff failed:", diffResult.error); + setDiffState({ status: "error", message: diffResult.error ?? "Unknown error" }); + setDiagnosticInfo(null); + return; + } - const diagnosticInfo: DiagnosticInfo = { - command: diffCommand, - outputLength: diffOutput.length, - fileDiffCount: fileDiffs.length, - hunkCount: allHunks.length, - }; + const diffOutput = diffResult.data.output ?? ""; + const truncationInfo = + "truncated" in diffResult.data ? diffResult.data.truncated : undefined; - // Build truncation warning (only when not filtering by path) - const truncationWarning = - truncationInfo && !selectedFilePath - ? `Diff truncated (${truncationInfo.reason}). Filter by file to see more.` - : null; + const fileDiffs = parseDiff(diffOutput); + const allHunks = extractAllHunks(fileDiffs); - const value = { hunks: allHunks, truncationWarning, diagnosticInfo }; - setCachedReviewDiff(cacheKey, value); - return value; - })(); + const nextDiagnosticInfo: DiagnosticInfo = { + command: diffCommand, + outputLength: diffOutput.length, + fileDiffCount: fileDiffs.length, + hunkCount: allHunks.length, + }; - setInFlightReviewDiff(cacheKey, loadPromise); + // Build truncation warning (only when not filtering by path) + const truncationWarning = + truncationInfo && !selectedFilePath + ? `Diff truncated (${truncationInfo.reason}). Filter by file to see more.` + : null; - void loadPromise - .then((data) => { - if (cancelled) return; - setDiagnosticInfo(data.diagnosticInfo); - setDiffState({ - status: "loaded", - hunks: data.hunks, - truncationWarning: data.truncationWarning, + setCacheWithEviction(reviewPanelDiffCache, cacheKey, { + hunks: allHunks, + truncationWarning, + diagnosticInfo: nextDiagnosticInfo, }); - if (data.hunks.length > 0) { - setSelectedHunkId((prev) => prev ?? data.hunks[0].id); + setDiagnosticInfo(nextDiagnosticInfo); + setDiffState({ status: "loaded", hunks: allHunks, truncationWarning }); + + if (allHunks.length > 0) { + setSelectedHunkId((prev) => prev ?? allHunks[0].id); } - }) - .catch((err) => { + } catch (err) { if (cancelled) return; const errorMsg = `Failed to load diff: ${err instanceof Error ? err.message : String(err)}`; console.error(errorMsg); setDiffState({ status: "error", message: errorMsg }); - setDiagnosticInfo(null); - }); + } + }; + + void loadDiff(); return () => { cancelled = true; diff --git a/src/browser/utils/review/reviewPanelCache.test.ts b/src/browser/utils/review/reviewPanelCache.test.ts deleted file mode 100644 index d231b76a6c..0000000000 --- a/src/browser/utils/review/reviewPanelCache.test.ts +++ /dev/null @@ -1,166 +0,0 @@ -import { beforeEach, describe, expect, test } from "bun:test"; -import type { DiffHunk } from "@/common/types/review"; -import type { FileTreeNode } from "@/common/utils/git/numstatParser"; -import { - clearReviewPanelCaches, - getCachedReviewDiff, - getCachedReviewFileTree, - getInFlightReviewDiff, - getInFlightReviewFileTree, - makeReviewDiffCacheKey, - makeReviewFileTreeCacheKey, - setCachedReviewDiff, - setCachedReviewFileTree, - setInFlightReviewDiff, - setInFlightReviewFileTree, -} from "./reviewPanelCache"; - -describe("reviewPanelCache", () => { - beforeEach(() => { - clearReviewPanelCaches(); - }); - - test("makeReviewDiffCacheKey is sensitive to selectedFilePath", () => { - const base = { - workspaceId: "ws", - workspacePath: "/tmp/ws", - diffBase: "HEAD", - includeUncommitted: false, - }; - - const keyA = makeReviewDiffCacheKey({ ...base, selectedFilePath: null }); - const keyB = makeReviewDiffCacheKey({ ...base, selectedFilePath: "src/a.ts" }); - - expect(keyA).not.toEqual(keyB); - }); - - test("makeReviewFileTreeCacheKey does not include selectedFilePath", () => { - const key = makeReviewFileTreeCacheKey({ - workspaceId: "ws", - workspacePath: "/tmp/ws", - diffBase: "HEAD", - includeUncommitted: true, - }); - - expect(key).toContain("review-panel-tree"); - }); - - test("diff cache round-trips", () => { - const key = makeReviewDiffCacheKey({ - workspaceId: "ws", - workspacePath: "/tmp/ws", - diffBase: "HEAD", - includeUncommitted: false, - selectedFilePath: null, - }); - - const hunk: DiffHunk = { - id: "h1", - filePath: "src/a.ts", - oldStart: 1, - oldLines: 1, - newStart: 1, - newLines: 1, - content: "+console.log('hi')", - header: "@@ -1 +1 @@", - changeType: "modified", - }; - - setCachedReviewDiff(key, { - hunks: [hunk], - truncationWarning: null, - diagnosticInfo: { - command: "git diff", - outputLength: 123, - fileDiffCount: 1, - hunkCount: 1, - }, - }); - - expect(getCachedReviewDiff(key)?.hunks[0].id).toEqual("h1"); - }); - - test("file tree cache round-trips", () => { - const key = makeReviewFileTreeCacheKey({ - workspaceId: "ws", - workspacePath: "/tmp/ws", - diffBase: "HEAD", - includeUncommitted: false, - }); - - const tree: FileTreeNode = { - name: "", - path: "", - isDirectory: true, - children: [], - totalStats: { filePath: "", additions: 0, deletions: 0 }, - }; - - setCachedReviewFileTree(key, { fileTree: tree }); - - expect(getCachedReviewFileTree(key)?.fileTree.isDirectory).toEqual(true); - }); - - test("inFlight diff is cleared after settle", async () => { - const key = makeReviewDiffCacheKey({ - workspaceId: "ws", - workspacePath: "/tmp/ws", - diffBase: "HEAD", - includeUncommitted: false, - selectedFilePath: null, - }); - - let resolve!: (value: { - hunks: DiffHunk[]; - truncationWarning: string | null; - diagnosticInfo: null; - }) => void; - - const promise = new Promise<{ - hunks: DiffHunk[]; - truncationWarning: string | null; - diagnosticInfo: null; - }>((r) => { - resolve = r; - }); - - setInFlightReviewDiff(key, promise); - expect(getInFlightReviewDiff(key)).toBe(promise); - - resolve({ hunks: [], truncationWarning: null, diagnosticInfo: null }); - await promise; - - expect(getInFlightReviewDiff(key)).toBeNull(); - }); - - test("inFlight file tree is cleared after settle", async () => { - const key = makeReviewFileTreeCacheKey({ - workspaceId: "ws", - workspacePath: "/tmp/ws", - diffBase: "HEAD", - includeUncommitted: false, - }); - - let resolve!: (value: { fileTree: FileTreeNode }) => void; - - const tree: FileTreeNode = { - name: "", - path: "", - isDirectory: true, - children: [], - totalStats: { filePath: "", additions: 0, deletions: 0 }, - }; - - const promise = new Promise<{ fileTree: FileTreeNode }>((r) => { - resolve = r; - }); - - setInFlightReviewFileTree(key, promise); - expect(getInFlightReviewFileTree(key)).toBe(promise); - - resolve({ fileTree: tree }); - await promise; - - expect(getInFlightReviewFileTree(key)).toBeNull(); - }); -}); diff --git a/src/browser/utils/review/reviewPanelCache.ts b/src/browser/utils/review/reviewPanelCache.ts deleted file mode 100644 index a885706d45..0000000000 --- a/src/browser/utils/review/reviewPanelCache.ts +++ /dev/null @@ -1,176 +0,0 @@ -/** - * In-memory caches for the Code Review panel. - * - * Motivation: switching workspaces can trigger expensive git diff/numstat calls. - * We keep the most recent results in an LRU so returning to a workspace is instant. - * - * Refresh semantics: - * - Cache entries are updated on successful loads. - * - Consumers can bypass the cache on manual refresh (Ctrl/Cmd+R). - */ - -import { LRUCache } from "lru-cache"; -import type { DiffHunk } from "@/common/types/review"; -import type { FileTreeNode } from "@/common/utils/git/numstatParser"; - -export interface ReviewPanelDiagnosticInfo { - command: string; - outputLength: number; - fileDiffCount: number; - hunkCount: number; -} - -export interface ReviewPanelDiffCacheValue { - hunks: DiffHunk[]; - truncationWarning: string | null; - diagnosticInfo: ReviewPanelDiagnosticInfo | null; -} - -export interface ReviewPanelFileTreeCacheValue { - fileTree: FileTreeNode; -} - -interface CacheEntry { - value: T; - cachedAt: number; -} - -function estimateHunksSizeBytes(hunks: DiffHunk[]): number { - // Rough bytes for JS strings (UTF-16) + a small constant per object. - let bytes = hunks.length * 64; - for (const hunk of hunks) { - bytes += - (hunk.id.length + - hunk.filePath.length + - hunk.content.length + - hunk.header.length + - (hunk.oldPath?.length ?? 0) + - (hunk.changeType?.length ?? 0)) * - 2; - } - return bytes; -} - -function estimateFileTreeSizeBytes(node: FileTreeNode): number { - // Rough bytes for JS strings + a constant per node. - let bytes = 64 + (node.name.length + node.path.length) * 2; - if (node.stats) { - bytes += node.stats.filePath.length * 2; - } - if (node.totalStats) { - bytes += node.totalStats.filePath.length * 2; - } - for (const child of node.children) { - bytes += estimateFileTreeSizeBytes(child); - } - return bytes; -} - -const DIFF_CACHE_MAX_SIZE_BYTES = 16 * 1024 * 1024; // 16MB -const FILE_TREE_CACHE_MAX_SIZE_BYTES = 4 * 1024 * 1024; // 4MB - -const diffCache = new LRUCache>({ - max: 25, - maxSize: DIFF_CACHE_MAX_SIZE_BYTES, - sizeCalculation: (entry) => estimateHunksSizeBytes(entry.value.hunks), -}); - -const fileTreeCache = new LRUCache>({ - max: 25, - maxSize: FILE_TREE_CACHE_MAX_SIZE_BYTES, - sizeCalculation: (entry) => estimateFileTreeSizeBytes(entry.value.fileTree), -}); - -const inFlightDiffLoads = new Map>(); -const inFlightFileTreeLoads = new Map>(); - -export function makeReviewDiffCacheKey(params: { - workspaceId: string; - workspacePath: string; - diffBase: string; - includeUncommitted: boolean; - selectedFilePath: string | null; -}): string { - // Use a null byte separator to avoid accidental collisions. - return [ - "review-panel-diff:v1", - params.workspaceId, - params.workspacePath, - params.diffBase, - params.includeUncommitted ? "1" : "0", - params.selectedFilePath ?? "", - ].join("\u0000"); -} - -export function makeReviewFileTreeCacheKey(params: { - workspaceId: string; - workspacePath: string; - diffBase: string; - includeUncommitted: boolean; -}): string { - return [ - "review-panel-tree:v1", - params.workspaceId, - params.workspacePath, - params.diffBase, - params.includeUncommitted ? "1" : "0", - ].join("\u0000"); -} - -export function getCachedReviewDiff(key: string): ReviewPanelDiffCacheValue | null { - return diffCache.get(key)?.value ?? null; -} - -export function setCachedReviewDiff(key: string, value: ReviewPanelDiffCacheValue): void { - diffCache.set(key, { value, cachedAt: Date.now() }); -} - -export function getCachedReviewFileTree(key: string): ReviewPanelFileTreeCacheValue | null { - return fileTreeCache.get(key)?.value ?? null; -} - -export function setCachedReviewFileTree(key: string, value: ReviewPanelFileTreeCacheValue): void { - fileTreeCache.set(key, { value, cachedAt: Date.now() }); -} - -export function getInFlightReviewDiff(key: string): Promise | null { - return inFlightDiffLoads.get(key) ?? null; -} - -export function setInFlightReviewDiff( - key: string, - promise: Promise -): void { - inFlightDiffLoads.set(key, promise); - void promise.finally(() => { - if (inFlightDiffLoads.get(key) === promise) { - inFlightDiffLoads.delete(key); - } - }); -} - -export function getInFlightReviewFileTree( - key: string -): Promise | null { - return inFlightFileTreeLoads.get(key) ?? null; -} - -export function setInFlightReviewFileTree( - key: string, - promise: Promise -): void { - inFlightFileTreeLoads.set(key, promise); - void promise.finally(() => { - if (inFlightFileTreeLoads.get(key) === promise) { - inFlightFileTreeLoads.delete(key); - } - }); -} - -/** For tests / debugging. */ -export function clearReviewPanelCaches(): void { - diffCache.clear(); - fileTreeCache.clear(); - inFlightDiffLoads.clear(); - inFlightFileTreeLoads.clear(); -} From 910ec6574e6a04cf3f7013cec057be3fc705f56c Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 14 Dec 2025 12:21:15 -0600 Subject: [PATCH 3/8] =?UTF-8?q?=F0=9F=A4=96=20perf:=20use=20LRU=20cache=20?= =?UTF-8?q?for=20review=20panel?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../RightSidebar/CodeReview/ReviewPanel.tsx | 70 ++++++++++++++----- 1 file changed, 54 insertions(+), 16 deletions(-) diff --git a/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx b/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx index df5b0753e5..8bea6e553c 100644 --- a/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx +++ b/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx @@ -22,6 +22,7 @@ * - Frontend filtering is fast even for 1000+ hunks (<5ms) */ +import { LRUCache } from "lru-cache"; import React, { useState, useEffect, useMemo, useCallback, useRef } from "react"; import { HunkViewer } from "./HunkViewer"; import { ReviewControls } from "./ReviewControls"; @@ -88,6 +89,8 @@ type DiffState = | { status: "error"; message: string }; const REVIEW_PANEL_CACHE_MAX_ENTRIES = 10; +const REVIEW_PANEL_DIFF_CACHE_MAX_SIZE_BYTES = 16 * 1024 * 1024; // 16MB +const REVIEW_PANEL_TREE_CACHE_MAX_SIZE_BYTES = 4 * 1024 * 1024; // 4MB interface ReviewPanelDiffCacheValue { hunks: DiffHunk[]; @@ -95,28 +98,63 @@ interface ReviewPanelDiffCacheValue { diagnosticInfo: DiagnosticInfo | null; } -const reviewPanelDiffCache = new Map(); -const reviewPanelFileTreeCache = new Map(); - -function evictOldestCacheEntry(map: Map): void { - const first = map.keys().next(); - if (!first.done) { - map.delete(first.value); +function estimateHunksSizeBytes(hunks: DiffHunk[]): number { + // Rough bytes for JS strings (UTF-16) + a small constant per object. + let bytes = hunks.length * 64; + for (const hunk of hunks) { + bytes += + (hunk.id.length + + hunk.filePath.length + + hunk.content.length + + hunk.header.length + + (hunk.oldPath?.length ?? 0) + + (hunk.changeType?.length ?? 0)) * + 2; } + return bytes; } -function setCacheWithEviction(map: Map, key: string, value: T): void { - // Refresh insertion order so eviction behaves roughly like LRU. - if (map.has(key)) { - map.delete(key); +function estimateDiffCacheValueSizeBytes(value: ReviewPanelDiffCacheValue): number { + let bytes = estimateHunksSizeBytes(value.hunks); + bytes += (value.truncationWarning?.length ?? 0) * 2; + if (value.diagnosticInfo) { + bytes += + (value.diagnosticInfo.command.length + + value.diagnosticInfo.outputLength.toString().length + + value.diagnosticInfo.fileDiffCount.toString().length + + value.diagnosticInfo.hunkCount.toString().length) * + 2; } - map.set(key, value); + return bytes; +} - if (map.size > REVIEW_PANEL_CACHE_MAX_ENTRIES) { - evictOldestCacheEntry(map); +function estimateFileTreeSizeBytes(node: FileTreeNode): number { + // Rough bytes for JS strings + a constant per node. + let bytes = 64 + (node.name.length + node.path.length) * 2; + if (node.stats) { + bytes += node.stats.filePath.length * 2; + } + if (node.totalStats) { + bytes += node.totalStats.filePath.length * 2; } + for (const child of node.children) { + bytes += estimateFileTreeSizeBytes(child); + } + return bytes; } +const reviewPanelDiffCache = new LRUCache({ + max: REVIEW_PANEL_CACHE_MAX_ENTRIES, + maxSize: REVIEW_PANEL_DIFF_CACHE_MAX_SIZE_BYTES, + sizeCalculation: (value) => estimateDiffCacheValueSizeBytes(value), +}); + +const reviewPanelFileTreeCache = new LRUCache({ + max: REVIEW_PANEL_CACHE_MAX_ENTRIES, + maxSize: REVIEW_PANEL_TREE_CACHE_MAX_SIZE_BYTES, + sizeCalculation: (node) => estimateFileTreeSizeBytes(node), +}); + function makeReviewDiffCacheKey(params: { workspaceId: string; workspacePath: string; @@ -295,7 +333,7 @@ export const ReviewPanel: React.FC = ({ // Build tree with original paths (needed for git commands) const tree = buildFileTree(fileStats); - setCacheWithEviction(reviewPanelFileTreeCache, cacheKey, tree); + reviewPanelFileTreeCache.set(cacheKey, tree); if (cancelled) return; setFileTree(tree); @@ -429,7 +467,7 @@ export const ReviewPanel: React.FC = ({ ? `Diff truncated (${truncationInfo.reason}). Filter by file to see more.` : null; - setCacheWithEviction(reviewPanelDiffCache, cacheKey, { + reviewPanelDiffCache.set(cacheKey, { hunks: allHunks, truncationWarning, diagnosticInfo: nextDiagnosticInfo, From c8ca6401bb4a3c51633c9fc1603a8544174ee3c8 Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 14 Dec 2025 12:29:35 -0600 Subject: [PATCH 4/8] =?UTF-8?q?=F0=9F=A4=96=20refactor:=20key=20review=20p?= =?UTF-8?q?anel=20cache=20by=20git=20command?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../RightSidebar/CodeReview/ReviewPanel.tsx | 78 +++++++------------ 1 file changed, 27 insertions(+), 51 deletions(-) diff --git a/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx b/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx index 8bea6e553c..11a4e27cc9 100644 --- a/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx +++ b/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx @@ -155,37 +155,14 @@ const reviewPanelFileTreeCache = new LRUCache({ sizeCalculation: (node) => estimateFileTreeSizeBytes(node), }); -function makeReviewDiffCacheKey(params: { +function makeReviewPanelCacheKey(params: { + scope: "review-panel-diff:v1" | "review-panel-tree:v1"; workspaceId: string; workspacePath: string; - diffBase: string; - includeUncommitted: boolean; - selectedFilePath: string | null; + gitCommand: string; }): string { - // Null byte separator avoids accidental collisions. - return [ - "review-panel-diff:v1", - params.workspaceId, - params.workspacePath, - params.diffBase, - params.includeUncommitted ? "1" : "0", - params.selectedFilePath ?? "", - ].join("\u0000"); -} - -function makeReviewFileTreeCacheKey(params: { - workspaceId: string; - workspacePath: string; - diffBase: string; - includeUncommitted: boolean; -}): string { - return [ - "review-panel-tree:v1", - params.workspaceId, - params.workspacePath, - params.diffBase, - params.includeUncommitted ? "1" : "0", - ].join("\u0000"); + // Key off the actual git command to avoid forgetting to include new inputs. + return [params.scope, params.workspaceId, params.workspacePath, params.gitCommand].join("\u0000"); } export const ReviewPanel: React.FC = ({ @@ -288,11 +265,18 @@ export const ReviewPanel: React.FC = ({ lastFileTreeRefreshTriggerRef.current = refreshTrigger; const isManualRefresh = prevRefreshTrigger !== null && prevRefreshTrigger !== refreshTrigger; - const cacheKey = makeReviewFileTreeCacheKey({ + const numstatCommand = buildGitDiffCommand( + filters.diffBase, + filters.includeUncommitted, + "", // No path filter for file tree + "numstat" + ); + + const cacheKey = makeReviewPanelCacheKey({ + scope: "review-panel-tree:v1", workspaceId, workspacePath, - diffBase: filters.diffBase, - includeUncommitted: filters.includeUncommitted, + gitCommand: numstatCommand, }); // Fast path: use cached tree when switching workspaces (unless user explicitly refreshed). @@ -310,13 +294,6 @@ export const ReviewPanel: React.FC = ({ const loadFileTree = async () => { setIsLoadingTree(true); try { - const numstatCommand = buildGitDiffCommand( - filters.diffBase, - filters.includeUncommitted, - "", // No path filter for file tree - "numstat" - ); - const numstatResult = await api.workspace.executeBash({ workspaceId, script: numstatCommand, @@ -371,12 +348,20 @@ export const ReviewPanel: React.FC = ({ lastDiffRefreshTriggerRef.current = refreshTrigger; const isManualRefresh = prevRefreshTrigger !== null && prevRefreshTrigger !== refreshTrigger; - const cacheKey = makeReviewDiffCacheKey({ + const pathFilter = selectedFilePath ? ` -- "${extractNewPath(selectedFilePath)}"` : ""; + + const diffCommand = buildGitDiffCommand( + filters.diffBase, + filters.includeUncommitted, + pathFilter, + "diff" + ); + + const cacheKey = makeReviewPanelCacheKey({ + scope: "review-panel-diff:v1", workspaceId, workspacePath, - diffBase: filters.diffBase, - includeUncommitted: filters.includeUncommitted, - selectedFilePath, + gitCommand: diffCommand, }); // Fast path: use cached diff when switching workspaces (unless user explicitly refreshed). @@ -421,15 +406,6 @@ export const ReviewPanel: React.FC = ({ // - includeUncommitted: include working directory changes // - selectedFilePath: ESSENTIAL for truncation - if full diff is cut off, // path filter lets us retrieve specific file's hunks - const pathFilter = selectedFilePath ? ` -- "${extractNewPath(selectedFilePath)}"` : ""; - - const diffCommand = buildGitDiffCommand( - filters.diffBase, - filters.includeUncommitted, - pathFilter, - "diff" - ); - // Fetch diff const diffResult = await api.workspace.executeBash({ workspaceId, From 348831204ded132d11c87994587fdcee51bd1477 Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 14 Dec 2025 13:08:15 -0600 Subject: [PATCH 5/8] =?UTF-8?q?=F0=9F=A4=96=20refactor:=20simplify=20revie?= =?UTF-8?q?w=20panel=20cache=20key?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../RightSidebar/CodeReview/ReviewPanel.tsx | 140 ++++++++++-------- 1 file changed, 81 insertions(+), 59 deletions(-) diff --git a/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx b/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx index 11a4e27cc9..61d47b55ba 100644 --- a/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx +++ b/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx @@ -42,7 +42,7 @@ import type { FileTreeNode } from "@/common/utils/git/numstatParser"; import { matchesKeybind, KEYBINDS, formatKeybind } from "@/browser/utils/ui/keybinds"; import { applyFrontendFilters } from "@/browser/utils/review/filterHunks"; import { cn } from "@/common/lib/utils"; -import { useAPI } from "@/browser/contexts/API"; +import { useAPI, type APIClient } from "@/browser/contexts/API"; /** Stats reported to parent for tab display */ interface ReviewPanelStats { @@ -156,13 +156,44 @@ const reviewPanelFileTreeCache = new LRUCache({ }); function makeReviewPanelCacheKey(params: { - scope: "review-panel-diff:v1" | "review-panel-tree:v1"; workspaceId: string; workspacePath: string; gitCommand: string; }): string { // Key off the actual git command to avoid forgetting to include new inputs. - return [params.scope, params.workspaceId, params.workspacePath, params.gitCommand].join("\u0000"); + return [ + "review-panel-cache:v1", + params.workspaceId, + params.workspacePath, + params.gitCommand, + ].join("\u0000"); +} + +type ExecuteBashResult = Awaited>; +type ExecuteBashSuccess = Extract; + +async function executeWorkspaceBashAndCache(params: { + api: APIClient; + workspaceId: string; + script: string; + cacheKey: string; + cache: LRUCache; + timeoutSecs: number; + parse: (result: ExecuteBashSuccess) => T; +}): Promise { + const result = await params.api.workspace.executeBash({ + workspaceId: params.workspaceId, + script: params.script, + options: { timeout_secs: params.timeoutSecs }, + }); + + if (!result.success) { + throw new Error(result.error ?? "Unknown error"); + } + + const value = params.parse(result); + params.cache.set(params.cacheKey, value); + return value; } export const ReviewPanel: React.FC = ({ @@ -273,7 +304,6 @@ export const ReviewPanel: React.FC = ({ ); const cacheKey = makeReviewPanelCacheKey({ - scope: "review-panel-tree:v1", workspaceId, workspacePath, gitCommand: numstatCommand, @@ -294,24 +324,22 @@ export const ReviewPanel: React.FC = ({ const loadFileTree = async () => { setIsLoadingTree(true); try { - const numstatResult = await api.workspace.executeBash({ + const tree = await executeWorkspaceBashAndCache({ + api, workspaceId, script: numstatCommand, - options: { timeout_secs: 30 }, + cacheKey, + cache: reviewPanelFileTreeCache, + timeoutSecs: 30, + parse: (result) => { + const numstatOutput = result.data.output ?? ""; + const fileStats = parseNumstat(numstatOutput); + + // Build tree with original paths (needed for git commands) + return buildFileTree(fileStats); + }, }); - if (!numstatResult.success) { - throw new Error(numstatResult.error ?? "Unknown error"); - } - - const numstatOutput = numstatResult.data.output ?? ""; - const fileStats = parseNumstat(numstatOutput); - - // Build tree with original paths (needed for git commands) - const tree = buildFileTree(fileStats); - - reviewPanelFileTreeCache.set(cacheKey, tree); - if (cancelled) return; setFileTree(tree); } catch (err) { @@ -358,7 +386,6 @@ export const ReviewPanel: React.FC = ({ ); const cacheKey = makeReviewPanelCacheKey({ - scope: "review-panel-diff:v1", workspaceId, workspacePath, gitCommand: diffCommand, @@ -406,60 +433,55 @@ export const ReviewPanel: React.FC = ({ // - includeUncommitted: include working directory changes // - selectedFilePath: ESSENTIAL for truncation - if full diff is cut off, // path filter lets us retrieve specific file's hunks - // Fetch diff - const diffResult = await api.workspace.executeBash({ + const data = await executeWorkspaceBashAndCache({ + api, workspaceId, script: diffCommand, - options: { timeout_secs: 30 }, + cacheKey, + cache: reviewPanelDiffCache, + timeoutSecs: 30, + parse: (result) => { + const diffOutput = result.data.output ?? ""; + const truncationInfo = "truncated" in result.data ? result.data.truncated : undefined; + + const fileDiffs = parseDiff(diffOutput); + const allHunks = extractAllHunks(fileDiffs); + + const diagnosticInfo: DiagnosticInfo = { + command: diffCommand, + outputLength: diffOutput.length, + fileDiffCount: fileDiffs.length, + hunkCount: allHunks.length, + }; + + // Build truncation warning (only when not filtering by path) + const truncationWarning = + truncationInfo && !selectedFilePath + ? `Diff truncated (${truncationInfo.reason}). Filter by file to see more.` + : null; + + return { hunks: allHunks, truncationWarning, diagnosticInfo }; + }, }); if (cancelled) return; - if (!diffResult.success) { - // Real error (not truncation-related) - console.error("Git diff failed:", diffResult.error); - setDiffState({ status: "error", message: diffResult.error ?? "Unknown error" }); - setDiagnosticInfo(null); - return; - } - - const diffOutput = diffResult.data.output ?? ""; - const truncationInfo = - "truncated" in diffResult.data ? diffResult.data.truncated : undefined; - - const fileDiffs = parseDiff(diffOutput); - const allHunks = extractAllHunks(fileDiffs); - - const nextDiagnosticInfo: DiagnosticInfo = { - command: diffCommand, - outputLength: diffOutput.length, - fileDiffCount: fileDiffs.length, - hunkCount: allHunks.length, - }; - - // Build truncation warning (only when not filtering by path) - const truncationWarning = - truncationInfo && !selectedFilePath - ? `Diff truncated (${truncationInfo.reason}). Filter by file to see more.` - : null; - - reviewPanelDiffCache.set(cacheKey, { - hunks: allHunks, - truncationWarning, - diagnosticInfo: nextDiagnosticInfo, + setDiagnosticInfo(data.diagnosticInfo); + setDiffState({ + status: "loaded", + hunks: data.hunks, + truncationWarning: data.truncationWarning, }); - setDiagnosticInfo(nextDiagnosticInfo); - setDiffState({ status: "loaded", hunks: allHunks, truncationWarning }); - - if (allHunks.length > 0) { - setSelectedHunkId((prev) => prev ?? allHunks[0].id); + if (data.hunks.length > 0) { + setSelectedHunkId((prev) => prev ?? data.hunks[0].id); } } catch (err) { if (cancelled) return; const errorMsg = `Failed to load diff: ${err instanceof Error ? err.message : String(err)}`; console.error(errorMsg); setDiffState({ status: "error", message: errorMsg }); + setDiagnosticInfo(null); } }; From a3428a58cd5367b89d02a801db4a0b57f31b4615 Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 14 Dec 2025 13:24:26 -0600 Subject: [PATCH 6/8] =?UTF-8?q?=F0=9F=A4=96=20refactor:=20simplify=20revie?= =?UTF-8?q?w=20panel=20cache=20sizing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../RightSidebar/CodeReview/ReviewPanel.tsx | 53 ++++--------------- 1 file changed, 9 insertions(+), 44 deletions(-) diff --git a/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx b/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx index 61d47b55ba..8f62dd07ec 100644 --- a/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx +++ b/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx @@ -98,61 +98,26 @@ interface ReviewPanelDiffCacheValue { diagnosticInfo: DiagnosticInfo | null; } -function estimateHunksSizeBytes(hunks: DiffHunk[]): number { - // Rough bytes for JS strings (UTF-16) + a small constant per object. - let bytes = hunks.length * 64; - for (const hunk of hunks) { - bytes += - (hunk.id.length + - hunk.filePath.length + - hunk.content.length + - hunk.header.length + - (hunk.oldPath?.length ?? 0) + - (hunk.changeType?.length ?? 0)) * - 2; +function estimateJsonSizeBytes(value: unknown): number { + // Rough bytes for JS strings (UTF-16). Used only for LRU sizing. + try { + return JSON.stringify(value).length * 2; + } catch { + // If we ever hit an unserializable structure, treat it as huge so it won't stick in cache. + return Number.MAX_SAFE_INTEGER; } - return bytes; -} - -function estimateDiffCacheValueSizeBytes(value: ReviewPanelDiffCacheValue): number { - let bytes = estimateHunksSizeBytes(value.hunks); - bytes += (value.truncationWarning?.length ?? 0) * 2; - if (value.diagnosticInfo) { - bytes += - (value.diagnosticInfo.command.length + - value.diagnosticInfo.outputLength.toString().length + - value.diagnosticInfo.fileDiffCount.toString().length + - value.diagnosticInfo.hunkCount.toString().length) * - 2; - } - return bytes; -} - -function estimateFileTreeSizeBytes(node: FileTreeNode): number { - // Rough bytes for JS strings + a constant per node. - let bytes = 64 + (node.name.length + node.path.length) * 2; - if (node.stats) { - bytes += node.stats.filePath.length * 2; - } - if (node.totalStats) { - bytes += node.totalStats.filePath.length * 2; - } - for (const child of node.children) { - bytes += estimateFileTreeSizeBytes(child); - } - return bytes; } const reviewPanelDiffCache = new LRUCache({ max: REVIEW_PANEL_CACHE_MAX_ENTRIES, maxSize: REVIEW_PANEL_DIFF_CACHE_MAX_SIZE_BYTES, - sizeCalculation: (value) => estimateDiffCacheValueSizeBytes(value), + sizeCalculation: (value) => estimateJsonSizeBytes(value), }); const reviewPanelFileTreeCache = new LRUCache({ max: REVIEW_PANEL_CACHE_MAX_ENTRIES, maxSize: REVIEW_PANEL_TREE_CACHE_MAX_SIZE_BYTES, - sizeCalculation: (node) => estimateFileTreeSizeBytes(node), + sizeCalculation: (node) => estimateJsonSizeBytes(node), }); function makeReviewPanelCacheKey(params: { From bf3f37de0336858e92f6731092957a4d2332addb Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 14 Dec 2025 13:38:10 -0600 Subject: [PATCH 7/8] =?UTF-8?q?=F0=9F=A4=96=20refactor:=20unify=20review?= =?UTF-8?q?=20panel=20caches=20into=20single=20LRU=20instance?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove redundant cache prefix from key (git command already distinguishes) - Merge reviewPanelDiffCache and reviewPanelFileTreeCache into one - Remove cache param from executeWorkspaceBashAndCache helper --- _Generated with `mux` • Model: `anthropic:claude-opus-4-5` • Thinking: `high`_ --- .../RightSidebar/CodeReview/ReviewPanel.tsx | 35 ++++++------------- 1 file changed, 11 insertions(+), 24 deletions(-) diff --git a/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx b/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx index 8f62dd07ec..03ba67a8d8 100644 --- a/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx +++ b/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx @@ -88,9 +88,8 @@ type DiffState = | { status: "loaded"; hunks: DiffHunk[]; truncationWarning: string | null } | { status: "error"; message: string }; -const REVIEW_PANEL_CACHE_MAX_ENTRIES = 10; -const REVIEW_PANEL_DIFF_CACHE_MAX_SIZE_BYTES = 16 * 1024 * 1024; // 16MB -const REVIEW_PANEL_TREE_CACHE_MAX_SIZE_BYTES = 4 * 1024 * 1024; // 4MB +const REVIEW_PANEL_CACHE_MAX_ENTRIES = 20; +const REVIEW_PANEL_CACHE_MAX_SIZE_BYTES = 20 * 1024 * 1024; // 20MB interface ReviewPanelDiffCacheValue { hunks: DiffHunk[]; @@ -98,6 +97,8 @@ interface ReviewPanelDiffCacheValue { diagnosticInfo: DiagnosticInfo | null; } +type ReviewPanelCacheValue = ReviewPanelDiffCacheValue | FileTreeNode; + function estimateJsonSizeBytes(value: unknown): number { // Rough bytes for JS strings (UTF-16). Used only for LRU sizing. try { @@ -108,41 +109,29 @@ function estimateJsonSizeBytes(value: unknown): number { } } -const reviewPanelDiffCache = new LRUCache({ +const reviewPanelCache = new LRUCache({ max: REVIEW_PANEL_CACHE_MAX_ENTRIES, - maxSize: REVIEW_PANEL_DIFF_CACHE_MAX_SIZE_BYTES, + maxSize: REVIEW_PANEL_CACHE_MAX_SIZE_BYTES, sizeCalculation: (value) => estimateJsonSizeBytes(value), }); -const reviewPanelFileTreeCache = new LRUCache({ - max: REVIEW_PANEL_CACHE_MAX_ENTRIES, - maxSize: REVIEW_PANEL_TREE_CACHE_MAX_SIZE_BYTES, - sizeCalculation: (node) => estimateJsonSizeBytes(node), -}); - function makeReviewPanelCacheKey(params: { workspaceId: string; workspacePath: string; gitCommand: string; }): string { // Key off the actual git command to avoid forgetting to include new inputs. - return [ - "review-panel-cache:v1", - params.workspaceId, - params.workspacePath, - params.gitCommand, - ].join("\u0000"); + return [params.workspaceId, params.workspacePath, params.gitCommand].join("\u0000"); } type ExecuteBashResult = Awaited>; type ExecuteBashSuccess = Extract; -async function executeWorkspaceBashAndCache(params: { +async function executeWorkspaceBashAndCache(params: { api: APIClient; workspaceId: string; script: string; cacheKey: string; - cache: LRUCache; timeoutSecs: number; parse: (result: ExecuteBashSuccess) => T; }): Promise { @@ -157,7 +146,7 @@ async function executeWorkspaceBashAndCache(params: { } const value = params.parse(result); - params.cache.set(params.cacheKey, value); + reviewPanelCache.set(params.cacheKey, value); return value; } @@ -276,7 +265,7 @@ export const ReviewPanel: React.FC = ({ // Fast path: use cached tree when switching workspaces (unless user explicitly refreshed). if (!isManualRefresh) { - const cachedTree = reviewPanelFileTreeCache.get(cacheKey); + const cachedTree = reviewPanelCache.get(cacheKey) as FileTreeNode | undefined; if (cachedTree) { setFileTree(cachedTree); setIsLoadingTree(false); @@ -294,7 +283,6 @@ export const ReviewPanel: React.FC = ({ workspaceId, script: numstatCommand, cacheKey, - cache: reviewPanelFileTreeCache, timeoutSecs: 30, parse: (result) => { const numstatOutput = result.data.output ?? ""; @@ -358,7 +346,7 @@ export const ReviewPanel: React.FC = ({ // Fast path: use cached diff when switching workspaces (unless user explicitly refreshed). if (!isManualRefresh) { - const cached = reviewPanelDiffCache.get(cacheKey); + const cached = reviewPanelCache.get(cacheKey) as ReviewPanelDiffCacheValue | undefined; if (cached) { setDiagnosticInfo(cached.diagnosticInfo); setDiffState({ @@ -403,7 +391,6 @@ export const ReviewPanel: React.FC = ({ workspaceId, script: diffCommand, cacheKey, - cache: reviewPanelDiffCache, timeoutSecs: 30, parse: (result) => { const diffOutput = result.data.output ?? ""; From dba7aa7c36cdb802971b829f12ae26ae55af660e Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 14 Dec 2025 13:45:54 -0600 Subject: [PATCH 8/8] =?UTF-8?q?=F0=9F=A4=96=20refactor:=20simplify=20revie?= =?UTF-8?q?w=20panel=20cache=20-=20remove=20sync=20init?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove getInitialDiffState and readPersistedState usage. Cache hits in the effect are fast enough (~16ms) that the brief loading state is not user-visible. This removes duplicated cache-key logic and simplifies the code. --- _Generated with `mux` • Model: `anthropic:claude-opus-4-5` • Thinking: `high`_ --- src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx b/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx index 03ba67a8d8..888d84f16b 100644 --- a/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx +++ b/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx @@ -163,7 +163,7 @@ export const ReviewPanel: React.FC = ({ const searchInputRef = useRef(null); // Unified diff state - discriminated union makes invalid states unrepresentable - // Note: Parent renders with key={workspaceId}, so component remounts on workspace change + // Note: Parent renders with key={workspaceId}, so component remounts on workspace change. const [diffState, setDiffState] = useState({ status: "loading" }); const [selectedHunkId, setSelectedHunkId] = useState(null);