From 5bc7e39a11a6cfbe5b1843974518a821eec2a799 Mon Sep 17 00:00:00 2001 From: Ammar Date: Tue, 23 Dec 2025 15:51:31 -0600 Subject: [PATCH 1/2] fix: restore code review diff refresh with manual feedback --- .../RightSidebar/CodeReview/RefreshButton.tsx | 50 ++- .../CodeReview/ReviewControls.tsx | 12 +- .../RightSidebar/CodeReview/ReviewPanel.tsx | 109 ++++++- .../hooks/useReviewRefreshController.ts | 289 ++++-------------- src/browser/utils/RefreshController.test.ts | 149 ++++++++- src/browser/utils/RefreshController.ts | 241 +++++++++++++-- src/browser/utils/ui/dateTime.ts | 13 + 7 files changed, 601 insertions(+), 262 deletions(-) diff --git a/src/browser/components/RightSidebar/CodeReview/RefreshButton.tsx b/src/browser/components/RightSidebar/CodeReview/RefreshButton.tsx index fa7eb24a68..0983104aa1 100644 --- a/src/browser/components/RightSidebar/CodeReview/RefreshButton.tsx +++ b/src/browser/components/RightSidebar/CodeReview/RefreshButton.tsx @@ -5,14 +5,30 @@ import React, { useState, useRef, useEffect } from "react"; import { Tooltip, TooltipTrigger, TooltipContent } from "@/browser/components/ui/tooltip"; import { formatKeybind, KEYBINDS } from "@/browser/utils/ui/keybinds"; +import { formatRelativeTimeCompact } from "@/browser/utils/ui/dateTime"; import { cn } from "@/common/lib/utils"; +import type { LastRefreshInfo, RefreshTrigger } from "@/browser/utils/RefreshController"; interface RefreshButtonProps { onClick: () => void; isLoading?: boolean; + /** Debug info about last refresh (timestamp and trigger) */ + lastRefreshInfo?: LastRefreshInfo | null; } -export const RefreshButton: React.FC = ({ onClick, isLoading = false }) => { +/** Human-readable trigger labels */ +const TRIGGER_LABELS: Record = { + manual: "manual click", + scheduled: "tool completion", + priority: "tool completion (priority)", + focus: "window focus", + visibility: "tab visible", + unpaused: "interaction ended", + "in-flight-followup": "queued followup", +}; + +export const RefreshButton: React.FC = (props) => { + const { onClick, isLoading = false, lastRefreshInfo } = props; // Track animation state for graceful stopping const [animationState, setAnimationState] = useState<"idle" | "spinning" | "stopping">("idle"); const spinOnceTimeoutRef = useRef(null); @@ -47,11 +63,25 @@ export const RefreshButton: React.FC = ({ onClick, isLoading }; }, []); + const handleClick = () => { + // Manual refresh should always provide immediate feedback, even if the refresh + // ends up being a no-op or resolves too quickly for isLoading to visibly flip. + if (!isLoading) { + setAnimationState("spinning"); + if (spinOnceTimeoutRef.current) { + clearTimeout(spinOnceTimeoutRef.current); + spinOnceTimeoutRef.current = null; + } + } + + onClick(); + }; + return ( - {animationState !== "idle" - ? "Refreshing..." - : `Refresh diff (${formatKeybind(KEYBINDS.REFRESH_REVIEW)})`} + {animationState !== "idle" ? ( + "Refreshing..." + ) : ( + + Refresh diff ({formatKeybind(KEYBINDS.REFRESH_REVIEW)}) + {lastRefreshInfo && ( + + Last: {formatRelativeTimeCompact(lastRefreshInfo.timestamp)} via{" "} + {TRIGGER_LABELS[lastRefreshInfo.trigger] ?? lastRefreshInfo.trigger} + + )} + + )} ); diff --git a/src/browser/components/RightSidebar/CodeReview/ReviewControls.tsx b/src/browser/components/RightSidebar/CodeReview/ReviewControls.tsx index 6946a1332e..beb87c4250 100644 --- a/src/browser/components/RightSidebar/CodeReview/ReviewControls.tsx +++ b/src/browser/components/RightSidebar/CodeReview/ReviewControls.tsx @@ -5,6 +5,7 @@ import React, { useState } from "react"; import { usePersistedState } from "@/browser/hooks/usePersistedState"; import type { ReviewFilters, ReviewStats } from "@/common/types/review"; +import type { LastRefreshInfo } from "@/browser/utils/RefreshController"; import { RefreshButton } from "./RefreshButton"; import { UntrackedStatus } from "./UntrackedStatus"; @@ -17,6 +18,8 @@ interface ReviewControlsProps { workspaceId: string; workspacePath: string; refreshTrigger?: number; + /** Debug info about last refresh */ + lastRefreshInfo?: LastRefreshInfo | null; } export const ReviewControls: React.FC = ({ @@ -28,6 +31,7 @@ export const ReviewControls: React.FC = ({ workspaceId, workspacePath, refreshTrigger, + lastRefreshInfo, }) => { // Local state for input value - only commit on blur/Enter const [inputValue, setInputValue] = useState(filters.diffBase); @@ -83,7 +87,13 @@ export const ReviewControls: React.FC = ({ return (
- {onRefresh && } + {onRefresh && ( + + )} [h.id, h])); + let allSame = prev.length === next.length; + + const result = next.map((hunk, i) => { + const prevHunk = prevById.get(hunk.id); + // Fast path: same ID and content means unchanged (content hash is part of ID) + if (prevHunk && prevHunk.content === hunk.content) { + if (allSame && prev[i]?.id !== hunk.id) allSame = false; + return prevHunk; + } + allSame = false; + return hunk; + }); + + // If all hunks are reused in same order, return prev array to preserve top-level reference + return allSame ? prev : result; +} + interface ReviewPanelDiffCacheValue { hunks: DiffHunk[]; truncationWarning: string | null; @@ -118,6 +143,53 @@ const reviewPanelCache = new LRUCache({ sizeCalculation: (value) => estimateJsonSizeBytes(value), }); +function getOriginBranchForFetch(diffBase: string): string | null { + const trimmed = diffBase.trim(); + if (!trimmed.startsWith("origin/")) return null; + + const branch = trimmed.slice("origin/".length); + + // Avoid shell injection; diffBase is user-controlled. + if (!/^[0-9A-Za-z._/-]+$/.test(branch)) return null; + + return branch; +} + +interface OriginFetchState { + key: string; + promise: Promise; +} + +async function ensureOriginFetched(params: { + api: APIClient; + workspaceId: string; + diffBase: string; + refreshToken: number; + originFetchRef: React.MutableRefObject; +}): Promise { + const originBranch = getOriginBranchForFetch(params.diffBase); + if (!originBranch) return; + + const key = [params.workspaceId, params.diffBase, String(params.refreshToken)].join("\u0000"); + const existing = params.originFetchRef.current; + if (existing?.key === key) { + await existing.promise; + return; + } + + // Ensure manual refresh doesn't hang on credential prompts. + const promise = params.api.workspace + .executeBash({ + workspaceId: params.workspaceId, + script: `GIT_TERMINAL_PROMPT=0 git fetch origin ${originBranch} --quiet || true`, + options: { timeout_secs: 30 }, + }) + .then(() => undefined) + .catch(() => undefined); + + params.originFetchRef.current = { key, promise }; + await promise; +} function makeReviewPanelCacheKey(params: { workspaceId: string; workspacePath: string; @@ -161,6 +233,7 @@ export const ReviewPanel: React.FC = ({ isCreating = false, onStatsChange, }) => { + const originFetchRef = useRef(null); const { api } = useAPI(); const panelRef = useRef(null); const scrollContainerRef = useRef(null); @@ -309,6 +382,15 @@ export const ReviewPanel: React.FC = ({ const loadFileTree = async () => { setIsLoadingTree(true); try { + await ensureOriginFetched({ + api, + workspaceId, + diffBase: filters.diffBase, + refreshToken: refreshTrigger, + originFetchRef, + }); + if (cancelled) return; + const tree = await executeWorkspaceBashAndCache({ api, workspaceId, @@ -420,6 +502,15 @@ export const ReviewPanel: React.FC = ({ const loadDiff = async () => { try { + await ensureOriginFetched({ + api, + workspaceId, + diffBase: filters.diffBase, + refreshToken: refreshTrigger, + originFetchRef, + }); + if (cancelled) return; + // Git-level filters (affect what data is fetched): // - diffBase: what to diff against // - includeUncommitted: include working directory changes @@ -458,10 +549,19 @@ export const ReviewPanel: React.FC = ({ if (cancelled) return; setDiagnosticInfo(data.diagnosticInfo); - setDiffState({ - status: "loaded", - hunks: data.hunks, - truncationWarning: data.truncationWarning, + + // Preserve object references for unchanged hunks to prevent unnecessary re-renders. + // HunkViewer is memoized on hunk object identity, so reusing references avoids + // re-rendering (and re-highlighting) hunks that haven't actually changed. + setDiffState((prev) => { + const prevHunks = + prev.status === "loaded" || prev.status === "refreshing" ? prev.hunks : []; + const hunks = preserveHunkReferences(prevHunks, data.hunks); + return { + status: "loaded", + hunks, + truncationWarning: data.truncationWarning, + }; }); if (data.hunks.length > 0) { @@ -811,6 +911,7 @@ export const ReviewPanel: React.FC = ({ workspaceId={workspaceId} workspacePath={workspacePath} refreshTrigger={refreshTrigger} + lastRefreshInfo={refreshController.lastRefreshInfo} /> {diffState.status === "error" ? ( diff --git a/src/browser/hooks/useReviewRefreshController.ts b/src/browser/hooks/useReviewRefreshController.ts index c8cf2291cf..f2b3613247 100644 --- a/src/browser/hooks/useReviewRefreshController.ts +++ b/src/browser/hooks/useReviewRefreshController.ts @@ -1,26 +1,12 @@ -import { useEffect, useRef } from "react"; +import { useEffect, useRef, useMemo } from "react"; import { workspaceStore } from "@/browser/stores/WorkspaceStore"; import type { APIClient } from "@/browser/contexts/API"; +import { RefreshController, type LastRefreshInfo } from "@/browser/utils/RefreshController"; +import { usePersistedState } from "@/browser/hooks/usePersistedState"; /** Debounce delay for auto-refresh after tool completion */ const TOOL_REFRESH_DEBOUNCE_MS = 3000; -/** - * Extract branch name from an "origin/..." diff base for git fetch. - * Returns null if not an origin ref or if branch name is unsafe for shell. - */ -function getOriginBranchForFetch(diffBase: string): string | null { - const trimmed = diffBase.trim(); - if (!trimmed.startsWith("origin/")) return null; - - const branch = trimmed.slice("origin/".length); - - // Avoid shell injection; diffBase is user-controlled. - if (!/^[0-9A-Za-z._/-]+$/.test(branch)) return null; - - return branch; -} - export interface UseReviewRefreshControllerOptions { workspaceId: string; api: APIClient | null; @@ -42,234 +28,79 @@ export interface ReviewRefreshController { setInteracting: (interacting: boolean) => void; /** Whether a git fetch is currently in-flight */ isRefreshing: boolean; + /** Info about the last completed refresh (for debugging) */ + lastRefreshInfo: LastRefreshInfo | null; } /** * Controls ReviewPanel auto-refresh triggered by file-modifying tool completions. * - * Handles: - * - Debouncing rapid tool completions (3s window) - * - Pausing while user is interacting (review note input focused) - * - Pausing while tab is hidden (flush on visibility change) - * - Coalescing requests while origin fetch is in-flight - * - Preserving scroll position across refreshes - * - * Architecture: - * - All refresh logic flows through a single ref-based handler to avoid stale closures - * - Pending flags track deferred refreshes for various pause conditions - * - visibilitychange listener ensures hidden-tab refreshes aren't lost + * Delegates debouncing, visibility/focus handling, and in-flight guards to RefreshController. + * Keeps ReviewPanel-specific logic: + * - Scroll position preservation + * - User interaction pause state */ export function useReviewRefreshController( options: UseReviewRefreshControllerOptions ): ReviewRefreshController { - const { workspaceId, api, isCreating, onRefresh, scrollContainerRef, onGitStatusRefresh } = - options; - - // Store diffBase in a ref so we always read the latest value - const diffBaseRef = useRef(options.diffBase); - diffBaseRef.current = options.diffBase; - - // Store onGitStatusRefresh in a ref to avoid stale closures - const onGitStatusRefreshRef = useRef(onGitStatusRefresh); - onGitStatusRefreshRef.current = onGitStatusRefresh; + const { workspaceId, api, isCreating, scrollContainerRef } = options; - // State refs (avoid re-renders, just track state for refresh logic) - const isRefreshingRef = useRef(false); - const isInteractingRef = useRef(false); - const debounceTimerRef = useRef | null>(null); + // Refs for values that executeRefresh needs at call time (avoid stale closures) + const onRefreshRef = useRef(options.onRefresh); + onRefreshRef.current = options.onRefresh; - // Pending flags - track why refresh was deferred - const pendingBecauseHiddenRef = useRef(false); - const pendingBecauseInteractingRef = useRef(false); - const pendingBecauseInFlightRef = useRef(false); + const onGitStatusRefreshRef = useRef(options.onGitStatusRefresh); + onGitStatusRefreshRef.current = options.onGitStatusRefresh; // Scroll position to restore after refresh const savedScrollTopRef = useRef(null); - // Expose isRefreshing for UI (e.g. disable refresh button) - // We use a ref but also track in a simple way for the return value - const isRefreshingForReturn = useRef(false); - - /** - * Core refresh execution - handles origin fetch if needed, then triggers onRefresh. - * Always reads latest state from refs at execution time. - */ - const executeRefresh = useRef(() => { - if (!api || isCreating) return; - - // Save scroll position before refresh - savedScrollTopRef.current = scrollContainerRef.current?.scrollTop ?? null; - - const originBranch = getOriginBranchForFetch(diffBaseRef.current); - if (originBranch) { - isRefreshingRef.current = true; - isRefreshingForReturn.current = true; - - api.workspace - .executeBash({ - workspaceId, - script: `git fetch origin ${originBranch} --quiet || true`, - options: { timeout_secs: 30 }, - }) - .catch((err) => { - console.debug("ReviewPanel origin fetch failed", err); - }) - .finally(() => { - isRefreshingRef.current = false; - isRefreshingForReturn.current = false; - onRefresh(); - onGitStatusRefreshRef.current?.(); - - // If another refresh was requested while we were fetching, do it now - if (pendingBecauseInFlightRef.current) { - pendingBecauseInFlightRef.current = false; - // Use setTimeout to avoid recursive call stack - setTimeout(() => tryRefresh("in-flight-followup"), 0); - } - }); - - return; - } - - // Local base - just trigger refresh immediately - onRefresh(); - onGitStatusRefreshRef.current?.(); - }); - - // Update executeRefresh closure dependencies - executeRefresh.current = () => { - if (!api || isCreating) return; - - savedScrollTopRef.current = scrollContainerRef.current?.scrollTop ?? null; - - const originBranch = getOriginBranchForFetch(diffBaseRef.current); - if (originBranch) { - isRefreshingRef.current = true; - isRefreshingForReturn.current = true; - - api.workspace - .executeBash({ - workspaceId, - script: `git fetch origin ${originBranch} --quiet || true`, - options: { timeout_secs: 30 }, - }) - .catch((err) => { - console.debug("ReviewPanel origin fetch failed", err); - }) - .finally(() => { - isRefreshingRef.current = false; - isRefreshingForReturn.current = false; - onRefresh(); - onGitStatusRefreshRef.current?.(); - - if (pendingBecauseInFlightRef.current) { - pendingBecauseInFlightRef.current = false; - setTimeout(() => tryRefresh("in-flight-followup"), 0); - } - }); - - return; - } - - onRefresh(); - onGitStatusRefreshRef.current?.(); - }; - - /** - * Attempt to refresh, respecting all pause conditions. - * If paused, sets the appropriate pending flag. - */ - const tryRefresh = (_reason: string) => { - if (!api || isCreating) return; - - // Check pause conditions in order of priority - - // 1. Tab hidden - queue for visibility change - if (document.hidden) { - pendingBecauseHiddenRef.current = true; - return; - } - - // 2. User interacting - queue for blur - if (isInteractingRef.current) { - pendingBecauseInteractingRef.current = true; - return; - } - - // 3. Already refreshing (origin fetch in-flight) - queue for completion - if (isRefreshingRef.current) { - pendingBecauseInFlightRef.current = true; - return; - } - - // All clear - execute refresh - executeRefresh.current(); - }; - - /** - * Schedule a debounced refresh (for tool completions). - */ - const scheduleRefresh = () => { - if (debounceTimerRef.current) { - clearTimeout(debounceTimerRef.current); - } - debounceTimerRef.current = setTimeout(() => { - debounceTimerRef.current = null; - tryRefresh("tool-completion"); - }, TOOL_REFRESH_DEBOUNCE_MS); - }; + // User interaction state (pauses auto-refresh) + const isInteractingRef = useRef(false); - /** - * Flush any pending refresh (called when pause condition clears). - */ - const flushPending = (clearedCondition: "hidden" | "interacting") => { - if (clearedCondition === "hidden" && pendingBecauseHiddenRef.current) { - pendingBecauseHiddenRef.current = false; - tryRefresh("visibility-restored"); - } else if (clearedCondition === "interacting" && pendingBecauseInteractingRef.current) { - pendingBecauseInteractingRef.current = false; - tryRefresh("interaction-ended"); - } - }; + // Track last refresh info - persisted per workspace so it survives workspace switches + const [lastRefreshInfo, setLastRefreshInfo] = usePersistedState( + `review-last-refresh:${workspaceId}`, + null + ); + + // Create RefreshController once, with stable callbacks via refs + const controller = useMemo(() => { + const ctrl = new RefreshController({ + debounceMs: TOOL_REFRESH_DEBOUNCE_MS, + isPaused: () => isInteractingRef.current, + onRefresh: () => { + if (isCreating) return; + + // Save scroll position before refresh + savedScrollTopRef.current = scrollContainerRef.current?.scrollTop ?? null; + + onRefreshRef.current(); + onGitStatusRefreshRef.current?.(); + }, + onRefreshComplete: setLastRefreshInfo, + }); + ctrl.bindListeners(); + return ctrl; + // isCreating/scrollContainerRef/setLastRefreshInfo changes require a new controller. + // Note: options.onRefresh is accessed via ref to avoid recreating controller on every render. + }, [isCreating, scrollContainerRef, setLastRefreshInfo]); + + // Cleanup on unmount or when controller changes + useEffect(() => { + return () => controller.dispose(); + }, [controller]); // Subscribe to file-modifying tool completions useEffect(() => { if (!api || isCreating) return; - const unsubscribe = workspaceStore.subscribeFileModifyingTool(scheduleRefresh, workspaceId); - - return () => { - unsubscribe(); - if (debounceTimerRef.current) { - clearTimeout(debounceTimerRef.current); - debounceTimerRef.current = null; - } - }; - // scheduleRefresh is stable (only uses refs internally) - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [api, workspaceId, isCreating]); + const unsubscribe = workspaceStore.subscribeFileModifyingTool(() => { + controller.schedule(); + }, workspaceId); - // Handle visibility/focus changes - flush pending refresh when user returns. - // Uses both visibilitychange (for browser tab hidden state) and window focus - // (for Electron app focus) since visibilitychange alone is unreliable in Electron - // when the app is behind other windows or on a different desktop/space. - useEffect(() => { - const handleReturn = () => { - // Only flush if document is actually visible - if (!document.hidden) { - flushPending("hidden"); - } - }; - - document.addEventListener("visibilitychange", handleReturn); - window.addEventListener("focus", handleReturn); - return () => { - document.removeEventListener("visibilitychange", handleReturn); - window.removeEventListener("focus", handleReturn); - }; - // flushPending is stable (only uses refs internally) - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + return unsubscribe; + }, [api, workspaceId, isCreating, controller]); // Public API const setInteracting = (interacting: boolean) => { @@ -278,25 +109,21 @@ export function useReviewRefreshController( // If interaction ended, flush any pending refresh if (wasInteracting && !interacting) { - flushPending("interacting"); + controller.notifyUnpaused(); } }; const requestManualRefresh = () => { - // Manual refresh bypasses debounce but still respects in-flight check - if (isRefreshingRef.current) { - pendingBecauseInFlightRef.current = true; - return; - } - executeRefresh.current(); + controller.requestImmediate(); }; return { requestManualRefresh, setInteracting, get isRefreshing() { - return isRefreshingForReturn.current; + return controller.isRefreshing; }, + lastRefreshInfo, }; } diff --git a/src/browser/utils/RefreshController.test.ts b/src/browser/utils/RefreshController.test.ts index e4aa56babc..a976269b5f 100644 --- a/src/browser/utils/RefreshController.test.ts +++ b/src/browser/utils/RefreshController.test.ts @@ -10,7 +10,22 @@ describe("RefreshController", () => { jest.useRealTimers(); }); - it("debounces multiple schedule() calls", () => { + it("rate-limits multiple schedule() calls (doesn't reset timer)", () => { + const onRefresh = jest.fn<() => void>(); + const controller = new RefreshController({ onRefresh, debounceMs: 100 }); + + controller.schedule(); + jest.advanceTimersByTime(50); + controller.schedule(); // Shouldn't reset timer + jest.advanceTimersByTime(50); + + // Should fire at 100ms from first call, not 150ms + expect(onRefresh).toHaveBeenCalledTimes(1); + + controller.dispose(); + }); + + it("coalesces calls during rate-limit window", () => { const onRefresh = jest.fn<() => void>(); const controller = new RefreshController({ onRefresh, debounceMs: 100 }); @@ -27,7 +42,7 @@ describe("RefreshController", () => { controller.dispose(); }); - it("requestImmediate() bypasses debounce", () => { + it("requestImmediate() bypasses rate-limit timer", () => { const onRefresh = jest.fn<() => void>(); const controller = new RefreshController({ onRefresh, debounceMs: 100 }); @@ -37,7 +52,7 @@ describe("RefreshController", () => { controller.requestImmediate(); expect(onRefresh).toHaveBeenCalledTimes(1); - // Original debounce timer should be cleared + // Original timer should be cleared jest.advanceTimersByTime(100); expect(onRefresh).toHaveBeenCalledTimes(1); @@ -64,6 +79,38 @@ describe("RefreshController", () => { controller.dispose(); }); + it("schedule() during in-flight queues refresh for after completion", async () => { + let resolveRefresh: () => void; + const onRefresh = jest.fn( + () => + new Promise((resolve) => { + resolveRefresh = resolve; + }) + ); + + const controller = new RefreshController({ onRefresh, debounceMs: 100 }); + + // Start first refresh + controller.requestImmediate(); + expect(onRefresh).toHaveBeenCalledTimes(1); + + // schedule() while in-flight should queue, not start timer + controller.schedule(); + + // Complete the first refresh and let .finally() run + resolveRefresh!(); + await Promise.resolve(); + await Promise.resolve(); // Extra tick for .finally() + + // Should trigger follow-up refresh, but never more frequently than the minimum interval. + // First tick runs the post-flight setTimeout(0), then we wait out the min interval. + jest.advanceTimersByTime(0); + jest.advanceTimersByTime(500); + expect(onRefresh).toHaveBeenCalledTimes(2); + + controller.dispose(); + }); + it("isRefreshing reflects in-flight state", () => { let resolveRefresh: () => void; const refreshPromise = new Promise((resolve) => { @@ -108,4 +155,100 @@ describe("RefreshController", () => { expect(onRefresh).not.toHaveBeenCalled(); }); + + it("requestImmediate() bypasses isPaused check (for manual refresh)", () => { + const onRefresh = jest.fn<() => void>(); + const paused = true; + const controller = new RefreshController({ + onRefresh, + debounceMs: 100, + isPaused: () => paused, + }); + + // schedule() should be blocked by isPaused + controller.schedule(); + jest.advanceTimersByTime(100); + expect(onRefresh).not.toHaveBeenCalled(); + + // requestImmediate() should bypass isPaused (manual refresh) + controller.requestImmediate(); + expect(onRefresh).toHaveBeenCalledTimes(1); + + controller.dispose(); + }); + + it("schedule() respects isPaused and flushes on notifyUnpaused", () => { + const onRefresh = jest.fn<() => void>(); + let paused = true; + const controller = new RefreshController({ + onRefresh, + debounceMs: 100, + isPaused: () => paused, + }); + + // schedule() should queue but not execute while paused + controller.schedule(); + jest.advanceTimersByTime(100); + expect(onRefresh).not.toHaveBeenCalled(); + + // Unpausing should flush the pending refresh + paused = false; + controller.notifyUnpaused(); + expect(onRefresh).toHaveBeenCalledTimes(1); + + controller.dispose(); + }); + + it("lastRefreshInfo tracks trigger and timestamp", () => { + const onRefresh = jest.fn<() => void>(); + const controller = new RefreshController({ onRefresh, debounceMs: 100 }); + + expect(controller.lastRefreshInfo).toBeNull(); + + // Manual refresh should record "manual" trigger + const beforeManual = Date.now(); + controller.requestImmediate(); + expect(controller.lastRefreshInfo).not.toBeNull(); + expect(controller.lastRefreshInfo!.trigger).toBe("manual"); + expect(controller.lastRefreshInfo!.timestamp).toBeGreaterThanOrEqual(beforeManual); + + // Scheduled refresh should record "scheduled" trigger + controller.schedule(); + jest.advanceTimersByTime(500); + expect(controller.lastRefreshInfo!.trigger).toBe("scheduled"); + + // Priority refresh should record "priority" trigger + controller.schedulePriority(); + jest.advanceTimersByTime(500); + expect(controller.lastRefreshInfo!.trigger).toBe("priority"); + + controller.dispose(); + }); + + it("onRefreshComplete callback is called with refresh info", () => { + const onRefresh = jest.fn<() => void>(); + const onRefreshComplete = jest.fn<(info: { trigger: string; timestamp: number }) => void>(); + const controller = new RefreshController({ + onRefresh, + onRefreshComplete, + debounceMs: 100, + }); + + expect(onRefreshComplete).not.toHaveBeenCalled(); + + controller.requestImmediate(); + expect(onRefreshComplete).toHaveBeenCalledTimes(1); + expect(onRefreshComplete).toHaveBeenCalledWith( + expect.objectContaining({ trigger: "manual", timestamp: expect.any(Number) }) + ); + + controller.schedule(); + jest.advanceTimersByTime(500); + expect(onRefreshComplete).toHaveBeenCalledTimes(2); + expect(onRefreshComplete).toHaveBeenLastCalledWith( + expect.objectContaining({ trigger: "scheduled" }) + ); + + controller.dispose(); + }); }); diff --git a/src/browser/utils/RefreshController.ts b/src/browser/utils/RefreshController.ts index 0ba0501a75..6ad4e7e977 100644 --- a/src/browser/utils/RefreshController.ts +++ b/src/browser/utils/RefreshController.ts @@ -11,13 +11,36 @@ * Used by GitStatusStore and useReviewRefreshController. */ +/** Reason that triggered a refresh - useful for debugging */ +export type RefreshTrigger = + | "manual" // User clicked refresh button + | "scheduled" // Debounced tool completion + | "priority" // Priority debounced (active workspace) + | "focus" // Window regained focus + | "visibility" // Tab became visible + | "unpaused" // Interaction ended, flushing pending + | "in-flight-followup"; // Queued while previous refresh was running + +export interface LastRefreshInfo { + /** Timestamp of last refresh completion */ + timestamp: number; + /** What triggered the refresh */ + trigger: RefreshTrigger; +} + export interface RefreshControllerOptions { /** Called to execute the actual refresh. Can be async. */ onRefresh: () => Promise | void; + /** Called after refresh completes with info about the refresh (for state updates) */ + onRefreshComplete?: (info: LastRefreshInfo) => void; + /** Debounce delay for triggered refreshes (ms). Default: 3000 */ debounceMs?: number; + /** Priority debounce delay (ms). Used by schedulePriority(). Default: same as debounceMs */ + priorityDebounceMs?: number; + /** * Whether to proactively refresh on focus, not just flush pending. * Enable for stores that need to catch external changes (e.g., git status). @@ -27,21 +50,46 @@ export interface RefreshControllerOptions { /** Minimum interval between focus-triggered refreshes (ms). Default: 500 */ focusDebounceMs?: number; + + /** + * Optional callback to check if refresh should be paused (e.g., user is interacting). + * If returns true, refresh is deferred until the condition clears. + */ + isPaused?: () => boolean; + + /** Label for debug logging (e.g., workspace name). If set, enables debug logs. */ + debugLabel?: string; } +/** Minimum ms between refresh executions - hard guard against loops */ +const MIN_REFRESH_INTERVAL_MS = 500; + export class RefreshController { private readonly onRefresh: () => Promise | void; + private readonly onRefreshComplete: ((info: LastRefreshInfo) => void) | null; private readonly debounceMs: number; + private readonly priorityDebounceMs: number; private readonly refreshOnFocus: boolean; private readonly focusDebounceMs: number; + private readonly isPaused: (() => boolean) | null; + private readonly debugLabel: string | null; private debounceTimer: ReturnType | null = null; + private cooldownTimer: ReturnType | null = null; private inFlight = false; private pendingBecauseHidden = false; private pendingBecauseInFlight = false; + private pendingBecausePaused = false; private lastFocusRefreshMs = 0; private disposed = false; + // Track last refresh for debugging + private _lastRefreshInfo: LastRefreshInfo | null = null; + private pendingTrigger: RefreshTrigger | null = null; + + // Timestamp of last refresh START (not completion) + private lastRefreshStartMs = 0; + // Track if listeners are bound (for cleanup) private listenersBound = false; private boundHandleVisibility: (() => void) | null = null; @@ -49,29 +97,94 @@ export class RefreshController { constructor(options: RefreshControllerOptions) { this.onRefresh = options.onRefresh; + this.onRefreshComplete = options.onRefreshComplete ?? null; this.debounceMs = options.debounceMs ?? 3000; + this.priorityDebounceMs = options.priorityDebounceMs ?? this.debounceMs; this.refreshOnFocus = options.refreshOnFocus ?? false; this.focusDebounceMs = options.focusDebounceMs ?? 500; + this.isPaused = options.isPaused ?? null; + this.debugLabel = options.debugLabel ?? null; + } + + private updatePendingTrigger(trigger: RefreshTrigger): void { + const priorities: Record = { + manual: 3, + priority: 2, + scheduled: 1, + focus: 0, + visibility: 0, + unpaused: 0, + "in-flight-followup": 0, + }; + + if (!this.pendingTrigger) { + this.pendingTrigger = trigger; + return; + } + + if (priorities[trigger] >= priorities[this.pendingTrigger]) { + this.pendingTrigger = trigger; + } + } + private debug(message: string): void { + if (this.debugLabel) { + console.debug(`[RefreshController:${this.debugLabel}] ${message}`); + } } /** - * Schedule a debounced refresh. Multiple calls within debounceMs coalesce. + * Schedule a rate-limited refresh. Multiple calls within the rate limit window + * coalesce, but don't reset the timer (unlike pure debounce). + * + * Behavior: + * - First call starts timer for delayMs + * - Subsequent calls mark "pending" but don't reset timer + * - When timer fires, refresh runs + * - If calls came in during refresh, a new timer starts after completion */ schedule(): void { + this.scheduleWithDelay(this.debounceMs, "scheduled"); + } + + /** + * Schedule with priority (shorter) rate limit. Used for active workspace. + */ + schedulePriority(): void { + this.scheduleWithDelay(this.priorityDebounceMs, "priority"); + } + + private scheduleWithDelay(delayMs: number, trigger: RefreshTrigger): void { if (this.disposed) return; + // Always update pending trigger (manual > priority > scheduled) + this.updatePendingTrigger(trigger); + + // If refresh is in-flight, mark pending and let onComplete handle scheduling + if (this.inFlight) { + this.debug("in-flight, queueing for completion"); + this.pendingBecauseInFlight = true; + return; + } + + // Rate-limit: if timer already running, don't reset it if (this.debounceTimer) { - clearTimeout(this.debounceTimer); + this.debug("timer running, coalescing"); + return; } + this.debug(`starting ${delayMs}ms timer (${trigger})`); this.debounceTimer = setTimeout(() => { this.debounceTimer = null; - this.tryRefresh(); - }, this.debounceMs); + const t = this.pendingTrigger ?? trigger; + this.pendingTrigger = null; + this.debug(`timer fired, refreshing (${t})`); + this.tryRefresh({ trigger: t }); + }, delayMs); } /** - * Request immediate refresh, bypassing debounce but respecting in-flight guard. + * Request immediate refresh, bypassing debounce and pause checks. + * Use for manual refresh (user clicked button) which should always execute. */ requestImmediate(): void { if (this.disposed) return; @@ -82,48 +195,111 @@ export class RefreshController { this.debounceTimer = null; } - this.tryRefresh(); + this.tryRefresh({ bypassPause: true, bypassHidden: true, trigger: "manual" }); } /** * Attempt refresh, respecting pause conditions. */ - private tryRefresh(): void { + private tryRefresh(options?: { + bypassPause?: boolean; + bypassHidden?: boolean; + bypassMinInterval?: boolean; + trigger?: RefreshTrigger; + }): void { if (this.disposed) return; - // Hidden → queue for visibility - if (typeof document !== "undefined" && document.hidden) { + const trigger = options?.trigger ?? this.pendingTrigger ?? "scheduled"; + const bypassHidden = (options?.bypassHidden ?? false) || trigger === "manual"; + const bypassPause = (options?.bypassPause ?? false) || trigger === "manual"; + const bypassMinInterval = (options?.bypassMinInterval ?? false) || trigger === "manual"; + + // Hidden → queue for visibility (unless bypassed) + if (!bypassHidden && typeof document !== "undefined" && document.hidden) { this.pendingBecauseHidden = true; + this.updatePendingTrigger(trigger); + return; + } + + // Custom pause (e.g., user interacting) → queue for unpause + // Bypassed for manual refresh (user explicitly requested) + if (!bypassPause && this.isPaused?.()) { + this.pendingBecausePaused = true; + this.updatePendingTrigger(trigger); return; } // In-flight → queue for completion if (this.inFlight) { this.pendingBecauseInFlight = true; + this.updatePendingTrigger(trigger); return; } - this.executeRefresh(); + // Hard guard: enforce minimum interval between refresh starts. + // Rather than dropping the request, schedule it for the earliest allowed time. + // Bypassed for manual refresh (user/component explicitly requested). + if (!bypassMinInterval && this.lastRefreshStartMs > 0) { + const now = Date.now(); + const elapsed = now - this.lastRefreshStartMs; + if (elapsed < MIN_REFRESH_INTERVAL_MS) { + this.updatePendingTrigger(trigger); + + if (this.cooldownTimer) { + this.debug("cooldown timer running, coalescing"); + return; + } + + const delayMs = MIN_REFRESH_INTERVAL_MS - elapsed; + const t = this.pendingTrigger ?? trigger; + this.debug(`cooldown: delaying ${delayMs}ms (${t})`); + + this.cooldownTimer = setTimeout(() => { + this.cooldownTimer = null; + const cooldownTrigger = this.pendingTrigger ?? t; + this.pendingTrigger = null; + this.tryRefresh({ trigger: cooldownTrigger }); + }, delayMs); + + return; + } + } + + this.executeRefresh(trigger); } /** * Execute the refresh, tracking in-flight state. */ - private executeRefresh(): void { + private executeRefresh(trigger: RefreshTrigger): void { if (this.disposed) return; + // Record refresh start; min-interval enforcement happens in tryRefresh(). + this.lastRefreshStartMs = Date.now(); + + if (this.cooldownTimer) { + clearTimeout(this.cooldownTimer); + this.cooldownTimer = null; + } + this.inFlight = true; + this.pendingTrigger = null; const maybePromise = this.onRefresh(); const onComplete = () => { this.inFlight = false; + this._lastRefreshInfo = { timestamp: Date.now(), trigger }; + + // Notify listener (for React state updates) + this.onRefreshComplete?.(this._lastRefreshInfo); // Process any queued refresh if (this.pendingBecauseInFlight) { this.pendingBecauseInFlight = false; - // Defer to avoid recursive stack - setTimeout(() => this.tryRefresh(), 0); + // Defer to avoid recursive stack; use the queued trigger + const followupTrigger = this.pendingTrigger ?? "in-flight-followup"; + setTimeout(() => this.tryRefresh({ trigger: followupTrigger }), 0); } }; @@ -137,14 +313,16 @@ export class RefreshController { /** * Handle focus/visibility return. Call from visibility/focus listeners. */ - private handleReturn(): void { + private handleReturn(trigger: "focus" | "visibility"): void { if (this.disposed) return; if (typeof document !== "undefined" && document.hidden) return; // Flush pending hidden refresh if (this.pendingBecauseHidden) { this.pendingBecauseHidden = false; - this.tryRefresh(); + const pendingTrigger = this.pendingTrigger ?? trigger; + this.pendingTrigger = null; + this.tryRefresh({ trigger: pendingTrigger }); return; // Don't double-refresh with proactive } @@ -153,11 +331,25 @@ export class RefreshController { const now = Date.now(); if (now - this.lastFocusRefreshMs >= this.focusDebounceMs) { this.lastFocusRefreshMs = now; - this.tryRefresh(); + this.tryRefresh({ trigger }); } } } + /** + * Notify that a pause condition has cleared (e.g., user stopped interacting). + * Flushes any pending refresh that was deferred due to isPaused(). + */ + notifyUnpaused(): void { + if (this.disposed) return; + if (this.pendingBecausePaused) { + this.pendingBecausePaused = false; + const pendingTrigger = this.pendingTrigger ?? "unpaused"; + this.pendingTrigger = null; + this.tryRefresh({ trigger: pendingTrigger }); + } + } + /** * Bind focus/visibility listeners. Call once after construction. * Safe to call in non-browser environments (no-op). @@ -170,12 +362,12 @@ export class RefreshController { this.boundHandleVisibility = () => { if (document.visibilityState === "visible") { - this.handleReturn(); + this.handleReturn("visibility"); } }; this.boundHandleFocus = () => { - this.handleReturn(); + this.handleReturn("focus"); }; document.addEventListener("visibilitychange", this.boundHandleVisibility); @@ -189,6 +381,14 @@ export class RefreshController { return this.inFlight; } + /** + * Info about the last completed refresh (timestamp and trigger reason). + * Useful for debugging refresh behavior. + */ + get lastRefreshInfo(): LastRefreshInfo | null { + return this._lastRefreshInfo; + } + /** * Clean up timers and listeners. */ @@ -200,6 +400,11 @@ export class RefreshController { this.debounceTimer = null; } + if (this.cooldownTimer) { + clearTimeout(this.cooldownTimer); + this.cooldownTimer = null; + } + if (this.listenersBound) { if (this.boundHandleVisibility) { document.removeEventListener("visibilitychange", this.boundHandleVisibility); diff --git a/src/browser/utils/ui/dateTime.ts b/src/browser/utils/ui/dateTime.ts index a668dc4b1b..6e869981c4 100644 --- a/src/browser/utils/ui/dateTime.ts +++ b/src/browser/utils/ui/dateTime.ts @@ -75,3 +75,16 @@ export function formatRelativeTime(timestamp: number): string { return years === 1 ? "1 year ago" : `${years} years ago`; } } + +/** + * Compact relative time format for space-constrained UI (tooltips, badges). + * Examples: "5s ago", "2m ago", "3h ago", "1d ago" + */ +export function formatRelativeTimeCompact(timestamp: number): string { + const elapsed = Math.floor((Date.now() - timestamp) / 1000); + if (elapsed < 0) return "now"; + if (elapsed < 60) return `${elapsed}s ago`; + if (elapsed < 3600) return `${Math.floor(elapsed / 60)}m ago`; + if (elapsed < 86400) return `${Math.floor(elapsed / 3600)}h ago`; + return `${Math.floor(elapsed / 86400)}d ago`; +} From ead684a1b505e0904c3eceda3ffdd2c6a69ede86 Mon Sep 17 00:00:00 2001 From: Ammar Date: Tue, 23 Dec 2025 16:11:59 -0600 Subject: [PATCH 2/2] fix: make review last refresh info session-scoped --- .../hooks/useReviewRefreshController.ts | 33 +++++++++++++------ 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/src/browser/hooks/useReviewRefreshController.ts b/src/browser/hooks/useReviewRefreshController.ts index f2b3613247..e25de3760f 100644 --- a/src/browser/hooks/useReviewRefreshController.ts +++ b/src/browser/hooks/useReviewRefreshController.ts @@ -1,10 +1,14 @@ -import { useEffect, useRef, useMemo } from "react"; +import { useEffect, useRef, useMemo, useState } from "react"; import { workspaceStore } from "@/browser/stores/WorkspaceStore"; import type { APIClient } from "@/browser/contexts/API"; import { RefreshController, type LastRefreshInfo } from "@/browser/utils/RefreshController"; -import { usePersistedState } from "@/browser/hooks/usePersistedState"; /** Debounce delay for auto-refresh after tool completion */ + +// Session-scoped cache of last refresh info per workspace. +// We intentionally do NOT persist this across window reloads, since that can +// show confusing/stale values (e.g. "4h ago") after restarting the app. +const lastRefreshInfoByWorkspaceId = new Map(); const TOOL_REFRESH_DEBOUNCE_MS = 3000; export interface UseReviewRefreshControllerOptions { @@ -58,11 +62,14 @@ export function useReviewRefreshController( // User interaction state (pauses auto-refresh) const isInteractingRef = useRef(false); - // Track last refresh info - persisted per workspace so it survives workspace switches - const [lastRefreshInfo, setLastRefreshInfo] = usePersistedState( - `review-last-refresh:${workspaceId}`, - null - ); + const [lastRefreshInfo, setLastRefreshInfo] = useState(() => { + return lastRefreshInfoByWorkspaceId.get(workspaceId) ?? null; + }); + + // If workspaceId changes without a full remount, load any cached value. + useEffect(() => { + setLastRefreshInfo(lastRefreshInfoByWorkspaceId.get(workspaceId) ?? null); + }, [workspaceId]); // Create RefreshController once, with stable callbacks via refs const controller = useMemo(() => { @@ -78,13 +85,16 @@ export function useReviewRefreshController( onRefreshRef.current(); onGitStatusRefreshRef.current?.(); }, - onRefreshComplete: setLastRefreshInfo, + onRefreshComplete: (info) => { + lastRefreshInfoByWorkspaceId.set(workspaceId, info); + setLastRefreshInfo(info); + }, }); ctrl.bindListeners(); return ctrl; - // isCreating/scrollContainerRef/setLastRefreshInfo changes require a new controller. + // workspaceId/isCreating/scrollContainerRef changes require a new controller. // Note: options.onRefresh is accessed via ref to avoid recreating controller on every render. - }, [isCreating, scrollContainerRef, setLastRefreshInfo]); + }, [workspaceId, isCreating, scrollContainerRef]); // Cleanup on unmount or when controller changes useEffect(() => { @@ -114,6 +124,9 @@ export function useReviewRefreshController( }; const requestManualRefresh = () => { + const info: LastRefreshInfo = { timestamp: Date.now(), trigger: "manual" }; + lastRefreshInfoByWorkspaceId.set(workspaceId, info); + setLastRefreshInfo(info); controller.requestImmediate(); };