From 0d5bfb6437e77c0271402e8d4ce1e32fde2af074 Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 22 Dec 2025 17:48:08 -0600 Subject: [PATCH 01/10] perf: 1s debounce for active workspace git status Active workspace now uses 1s debounce for git status refresh after file-modifying tools complete, while background workspaces keep 3s. This makes the visible git indicators feel more responsive without adding load from background workspace refreshes. --- src/browser/App.tsx | 6 + .../RightSidebar/CodeReview/ReviewPanel.tsx | 42 ++- .../hooks/useReviewRefreshController.ts | 263 ++++-------------- src/browser/stores/GitStatusStore.ts | 34 ++- src/browser/utils/RefreshController.test.ts | 43 +++ src/browser/utils/RefreshController.ts | 53 +++- 6 files changed, 221 insertions(+), 220 deletions(-) diff --git a/src/browser/App.tsx b/src/browser/App.tsx index 3267245f95..c6739f217e 100644 --- a/src/browser/App.tsx +++ b/src/browser/App.tsx @@ -17,6 +17,7 @@ import { buildSortedWorkspacesByProject } from "./utils/ui/workspaceFiltering"; import { useResumeManager } from "./hooks/useResumeManager"; import { useUnreadTracking } from "./hooks/useUnreadTracking"; import { useWorkspaceStoreRaw, useWorkspaceRecency } from "./stores/WorkspaceStore"; +import { setActiveWorkspace } from "./stores/GitStatusStore"; import { useStableReference, compareMaps } from "./hooks/useStableReference"; import { CommandRegistryProvider, useCommandRegistry } from "./contexts/CommandRegistryContext"; @@ -133,6 +134,11 @@ function AppInner() { prevWorkspaceRef.current = selectedWorkspace; }, [selectedWorkspace, telemetry]); + // Tell GitStatusStore which workspace is active (for prioritized 1s refresh) + useEffect(() => { + setActiveWorkspace(selectedWorkspace?.workspaceId ?? null); + }, [selectedWorkspace?.workspaceId]); + // Track last-read timestamps for unread indicators const { lastReadTimestamps, onToggleUnread } = useUnreadTracking(selectedWorkspace); diff --git a/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx b/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx index 8f4cf8b434..9777e55d61 100644 --- a/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx +++ b/src/browser/components/RightSidebar/CodeReview/ReviewPanel.tsx @@ -94,6 +94,31 @@ type DiffState = const REVIEW_PANEL_CACHE_MAX_ENTRIES = 20; const REVIEW_PANEL_CACHE_MAX_SIZE_BYTES = 20 * 1024 * 1024; // 20MB +/** + * Preserve object references for unchanged hunks to prevent re-renders. + * Compares by ID and content - if a hunk exists in prev with same content, reuse it. + */ +function preserveHunkReferences(prev: DiffHunk[], next: DiffHunk[]): DiffHunk[] { + if (prev.length === 0) return next; + + const prevById = new Map(prev.map((h) => [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; @@ -458,10 +483,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) { diff --git a/src/browser/hooks/useReviewRefreshController.ts b/src/browser/hooks/useReviewRefreshController.ts index c8cf2291cf..c66a51f9d7 100644 --- a/src/browser/hooks/useReviewRefreshController.ts +++ b/src/browser/hooks/useReviewRefreshController.ts @@ -1,6 +1,7 @@ -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 } from "@/browser/utils/RefreshController"; /** Debounce delay for auto-refresh after tool completion */ const TOOL_REFRESH_DEBOUNCE_MS = 3000; @@ -47,17 +48,11 @@ export interface ReviewRefreshController { /** * 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: + * - Origin branch fetch before refresh + * - Scroll position preservation + * - User interaction pause state */ export function useReviewRefreshController( options: UseReviewRefreshControllerOptions @@ -65,211 +60,68 @@ export function useReviewRefreshController( const { workspaceId, api, isCreating, onRefresh, scrollContainerRef, onGitStatusRefresh } = options; - // Store diffBase in a ref so we always read the latest value + // Refs for values that executeRefresh needs at call time (avoid stale closures) 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; - // State refs (avoid re-renders, just track state for refresh logic) - const isRefreshingRef = useRef(false); - const isInteractingRef = useRef(false); - const debounceTimerRef = useRef | null>(null); - - // Pending flags - track why refresh was deferred - const pendingBecauseHiddenRef = useRef(false); - const pendingBecauseInteractingRef = useRef(false); - const pendingBecauseInFlightRef = useRef(false); - // 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?.(); + // User interaction state (pauses auto-refresh) + const isInteractingRef = useRef(false); - if (pendingBecauseInFlightRef.current) { - pendingBecauseInFlightRef.current = false; - setTimeout(() => tryRefresh("in-flight-followup"), 0); + // Create RefreshController once, with stable callbacks via refs + const controller = useMemo(() => { + const ctrl = new RefreshController({ + debounceMs: TOOL_REFRESH_DEBOUNCE_MS, + isPaused: () => isInteractingRef.current, + onRefresh: async () => { + if (!api || isCreating) return; + + // Save scroll position before refresh + savedScrollTopRef.current = scrollContainerRef.current?.scrollTop ?? null; + + const originBranch = getOriginBranchForFetch(diffBaseRef.current); + if (originBranch) { + try { + await api.workspace.executeBash({ + workspaceId, + script: `git fetch origin ${originBranch} --quiet || true`, + options: { timeout_secs: 30 }, + }); + } catch (err) { + console.debug("ReviewPanel origin fetch failed", err); } - }); - - 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); - }; - - /** - * 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"); - } - }; + } + + onRefresh(); + onGitStatusRefreshRef.current?.(); + }, + }); + ctrl.bindListeners(); + return ctrl; + // workspaceId/api/isCreating changes require new controller with updated closure + }, [workspaceId, api, isCreating, onRefresh, scrollContainerRef]); + + // 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,24 +130,19 @@ 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; }, }; } diff --git a/src/browser/stores/GitStatusStore.ts b/src/browser/stores/GitStatusStore.ts index 3df58aeca1..ea9494a725 100644 --- a/src/browser/stores/GitStatusStore.ts +++ b/src/browser/stores/GitStatusStore.ts @@ -19,7 +19,7 @@ import { RefreshController } from "@/browser/utils/RefreshController"; * - Lives outside React lifecycle (stable references) * - Event-driven updates (no polling): * - Initial subscription triggers immediate fetch - * - File-modifying tools trigger debounced refresh (3s) + * - File-modifying tools trigger debounced refresh (1s active, 3s background) * - Window focus triggers refresh for visible workspaces * - Explicit invalidation (branch switch, etc.) * - Manages git fetch with exponential backoff @@ -31,6 +31,7 @@ import { RefreshController } from "@/browser/utils/RefreshController"; // Configuration const MAX_CONCURRENT_GIT_OPS = 5; +const ACTIVE_WORKSPACE_DEBOUNCE_MS = 1000; // 1s for active workspace (vs 3s background) // Fetch configuration - aggressive intervals for fresh data const FETCH_BASE_INTERVAL_MS = 3 * 1000; // 3 seconds @@ -53,6 +54,9 @@ export class GitStatusStore { // File modification subscription private fileModifyUnsubscribe: (() => void) | null = null; + // Active workspace ID for prioritized refresh (1s vs 3s debounce) + private activeWorkspaceId: string | null = null; + // RefreshController handles debouncing, focus/visibility, and in-flight guards private readonly refreshController: RefreshController; @@ -64,7 +68,8 @@ export class GitStatusStore { // Create refresh controller with proactive focus refresh (catches external git changes) this.refreshController = new RefreshController({ onRefresh: () => this.updateGitStatus(), - debounceMs: 3000, // Same as TOOL_REFRESH_DEBOUNCE_MS in ReviewPanel + debounceMs: 3000, // Background workspaces + priorityDebounceMs: ACTIVE_WORKSPACE_DEBOUNCE_MS, // Active workspace gets faster refresh refreshOnFocus: true, // Proactively refresh on focus to catch external changes focusDebounceMs: 500, // Prevent spam from rapid alt-tabbing }); @@ -113,6 +118,14 @@ export class GitStatusStore { }); } + /** + * Set the active workspace for prioritized refresh (1s debounce vs 3s). + * Call when workspace selection changes. + */ + setActiveWorkspace(workspaceId: string | null): void { + this.activeWorkspaceId = workspaceId; + } + /** * Invalidate status for a workspace, clearing cache and triggering immediate refresh. * Call after operations that change git state (e.g., branch switch). @@ -490,8 +503,13 @@ export class GitStatusStore { return; } - // RefreshController handles debouncing, focus gating, and in-flight guards - this.refreshController.schedule(); + // Active workspace gets faster refresh (1s) via priority debounce + if (workspaceId === this.activeWorkspaceId) { + this.refreshController.schedulePriority(); + } else { + // Background workspaces use standard 3s debounce + this.refreshController.schedule(); + } }); } } @@ -542,3 +560,11 @@ export function invalidateGitStatus(workspaceId: string): void { const store = getGitStoreInstance(); store.invalidateWorkspace(workspaceId); } + +/** + * Set the active workspace for prioritized git status refresh (1s vs 3s debounce). + */ +export function setActiveWorkspace(workspaceId: string | null): void { + const store = getGitStoreInstance(); + store.setActiveWorkspace(workspaceId); +} diff --git a/src/browser/utils/RefreshController.test.ts b/src/browser/utils/RefreshController.test.ts index e4aa56babc..ab844fc7c5 100644 --- a/src/browser/utils/RefreshController.test.ts +++ b/src/browser/utils/RefreshController.test.ts @@ -108,4 +108,47 @@ 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(); + }); }); diff --git a/src/browser/utils/RefreshController.ts b/src/browser/utils/RefreshController.ts index 0ba0501a75..c5c42eef99 100644 --- a/src/browser/utils/RefreshController.ts +++ b/src/browser/utils/RefreshController.ts @@ -18,6 +18,9 @@ export interface RefreshControllerOptions { /** 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,18 +30,27 @@ 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; } export class RefreshController { private readonly onRefresh: () => Promise | void; private readonly debounceMs: number; + private readonly priorityDebounceMs: number; private readonly refreshOnFocus: boolean; private readonly focusDebounceMs: number; + private readonly isPaused: (() => boolean) | null; private debounceTimer: ReturnType | null = null; private inFlight = false; private pendingBecauseHidden = false; private pendingBecauseInFlight = false; + private pendingBecausePaused = false; private lastFocusRefreshMs = 0; private disposed = false; @@ -50,14 +62,27 @@ export class RefreshController { constructor(options: RefreshControllerOptions) { this.onRefresh = options.onRefresh; 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; } /** * Schedule a debounced refresh. Multiple calls within debounceMs coalesce. */ schedule(): void { + this.scheduleWithDelay(this.debounceMs); + } + + /** + * Schedule with priority (shorter) debounce. Used for active workspace. + */ + schedulePriority(): void { + this.scheduleWithDelay(this.priorityDebounceMs); + } + + private scheduleWithDelay(delayMs: number): void { if (this.disposed) return; if (this.debounceTimer) { @@ -67,11 +92,12 @@ export class RefreshController { this.debounceTimer = setTimeout(() => { this.debounceTimer = null; this.tryRefresh(); - }, this.debounceMs); + }, 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,13 +108,13 @@ export class RefreshController { this.debounceTimer = null; } - this.tryRefresh(); + this.tryRefresh({ bypassPause: true }); } /** * Attempt refresh, respecting pause conditions. */ - private tryRefresh(): void { + private tryRefresh(options?: { bypassPause?: boolean }): void { if (this.disposed) return; // Hidden → queue for visibility @@ -97,6 +123,13 @@ export class RefreshController { return; } + // Custom pause (e.g., user interacting) → queue for unpause + // Bypassed for manual refresh (user explicitly requested) + if (!options?.bypassPause && this.isPaused?.()) { + this.pendingBecausePaused = true; + return; + } + // In-flight → queue for completion if (this.inFlight) { this.pendingBecauseInFlight = true; @@ -158,6 +191,18 @@ export class RefreshController { } } + /** + * 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; + this.tryRefresh(); + } + } + /** * Bind focus/visibility listeners. Call once after construction. * Safe to call in non-browser environments (no-op). From 97d45c984f6103c6786690b274ee788829a076a2 Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 22 Dec 2025 19:59:36 -0600 Subject: [PATCH 02/10] feat: show last refresh time and trigger in tooltip Adds debugging visibility to ReviewPanel's refresh button tooltip: - Shows how long ago the last refresh occurred (e.g., '5s ago', '2m ago') - Shows what triggered the refresh (manual click, tool completion, focus, etc.) - Persisted per-workspace so it survives workspace switches RefreshController now uses rate-limiting instead of debouncing: - First event starts timer; subsequent events don't reset it - Guarantees refresh fires within N seconds of first trigger - Events during in-flight refresh queue a follow-up after completion - Better UX during rapid tool calls (refreshes periodically vs never) Fixes controller recreation bug: onRefresh callback was causing useMemo to recreate the controller on every render, disposing active timers. Now uses refs for callbacks to maintain stable dependencies. Debug logging improvements: - RefreshController accepts debugLabel for human-readable logs - WorkspaceStore.getWorkspaceName() helper for log-friendly workspace names - Logs now show workspace names instead of opaque IDs Adds tests for file-modifying tool subscription mechanism. --- .../RightSidebar/CodeReview/RefreshButton.tsx | 34 +++- .../CodeReview/ReviewControls.tsx | 12 +- .../RightSidebar/CodeReview/ReviewPanel.tsx | 1 + .../hooks/useReviewRefreshController.ts | 46 ++++-- src/browser/stores/WorkspaceStore.test.ts | 147 ++++++++++++++++++ src/browser/stores/WorkspaceStore.ts | 9 ++ src/browser/utils/RefreshController.test.ts | 104 ++++++++++++- src/browser/utils/RefreshController.ts | 119 +++++++++++--- src/browser/utils/ui/dateTime.ts | 13 ++ 9 files changed, 446 insertions(+), 39 deletions(-) diff --git a/src/browser/components/RightSidebar/CodeReview/RefreshButton.tsx b/src/browser/components/RightSidebar/CodeReview/RefreshButton.tsx index fa7eb24a68..00c2c06059 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); @@ -76,9 +92,19 @@ export const RefreshButton: React.FC = ({ onClick, isLoading - {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]} + + )} + + )} ); 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 && ( + + )} = ({ 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 c66a51f9d7..36702ce1ee 100644 --- a/src/browser/hooks/useReviewRefreshController.ts +++ b/src/browser/hooks/useReviewRefreshController.ts @@ -1,7 +1,8 @@ import { useEffect, useRef, useMemo } from "react"; import { workspaceStore } from "@/browser/stores/WorkspaceStore"; import type { APIClient } from "@/browser/contexts/API"; -import { RefreshController } from "@/browser/utils/RefreshController"; +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; @@ -43,6 +44,8 @@ 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; } /** @@ -57,15 +60,17 @@ export interface ReviewRefreshController { export function useReviewRefreshController( options: UseReviewRefreshControllerOptions ): ReviewRefreshController { - const { workspaceId, api, isCreating, onRefresh, scrollContainerRef, onGitStatusRefresh } = - options; + const { workspaceId, api, isCreating, scrollContainerRef } = options; // Refs for values that executeRefresh needs at call time (avoid stale closures) const diffBaseRef = useRef(options.diffBase); diffBaseRef.current = options.diffBase; - const onGitStatusRefreshRef = useRef(onGitStatusRefresh); - onGitStatusRefreshRef.current = onGitStatusRefresh; + const onRefreshRef = useRef(options.onRefresh); + onRefreshRef.current = options.onRefresh; + + const onGitStatusRefreshRef = useRef(options.onGitStatusRefresh); + onGitStatusRefreshRef.current = options.onGitStatusRefresh; // Scroll position to restore after refresh const savedScrollTopRef = useRef(null); @@ -73,11 +78,19 @@ 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 + ); + // Create RefreshController once, with stable callbacks via refs const controller = useMemo(() => { + const wsName = workspaceStore.getWorkspaceName(workspaceId); const ctrl = new RefreshController({ debounceMs: TOOL_REFRESH_DEBOUNCE_MS, isPaused: () => isInteractingRef.current, + debugLabel: wsName, onRefresh: async () => { if (!api || isCreating) return; @@ -97,14 +110,16 @@ export function useReviewRefreshController( } } - onRefresh(); + onRefreshRef.current(); onGitStatusRefreshRef.current?.(); }, + onRefreshComplete: setLastRefreshInfo, }); ctrl.bindListeners(); return ctrl; // workspaceId/api/isCreating changes require new controller with updated closure - }, [workspaceId, api, isCreating, onRefresh, scrollContainerRef]); + // Note: options.onRefresh is accessed via ref to avoid recreating controller on every render + }, [workspaceId, api, isCreating, scrollContainerRef, setLastRefreshInfo]); // Cleanup on unmount or when controller changes useEffect(() => { @@ -115,12 +130,18 @@ export function useReviewRefreshController( useEffect(() => { if (!api || isCreating) return; - const unsubscribe = workspaceStore.subscribeFileModifyingTool( - () => controller.schedule(), - workspaceId - ); + const wsName = workspaceStore.getWorkspaceName(workspaceId); + console.debug(`[ReviewRefresh] subscribing for "${wsName}"`); + + const unsubscribe = workspaceStore.subscribeFileModifyingTool(() => { + console.debug(`[ReviewRefresh] tool completed in "${wsName}", scheduling refresh`); + controller.schedule(); + }, workspaceId); - return unsubscribe; + return () => { + console.debug(`[ReviewRefresh] unsubscribing for "${wsName}"`); + unsubscribe(); + }; }, [api, workspaceId, isCreating, controller]); // Public API @@ -144,6 +165,7 @@ export function useReviewRefreshController( get isRefreshing() { return controller.isRefreshing; }, + lastRefreshInfo, }; } diff --git a/src/browser/stores/WorkspaceStore.test.ts b/src/browser/stores/WorkspaceStore.test.ts index 34d33a418b..41c0b7b45d 100644 --- a/src/browser/stores/WorkspaceStore.test.ts +++ b/src/browser/stores/WorkspaceStore.test.ts @@ -773,4 +773,151 @@ describe("WorkspaceStore", () => { expect(live).toBeNull(); }); }); + + describe("file-modifying tool subscription", () => { + it("notifies subscribers when bash tool completes", async () => { + const workspaceId = "file-modify-test-1"; + const notifications: string[] = []; + + // Subscribe BEFORE creating workspace + const unsubscribe = store.subscribeFileModifyingTool((wsId) => { + notifications.push(wsId); + }, workspaceId); + + mockOnChat.mockImplementation(async function* (): AsyncGenerator< + WorkspaceChatMessage, + void, + unknown + > { + yield { type: "caught-up" }; + await Promise.resolve(); + yield { + type: "tool-call-end", + workspaceId, + messageId: "m1", + toolCallId: "call-1", + toolName: "bash", + result: { success: true, output: "done", exitCode: 0, wall_duration_ms: 1 }, + timestamp: 1, + }; + }); + + createAndAddWorkspace(store, workspaceId); + await new Promise((resolve) => setTimeout(resolve, 10)); + + expect(notifications).toContain(workspaceId); + expect(notifications.length).toBe(1); + + unsubscribe(); + }); + + it("notifies subscribers when file_edit tool completes", async () => { + const workspaceId = "file-modify-test-2"; + const notifications: string[] = []; + + const unsubscribe = store.subscribeFileModifyingTool((wsId) => { + notifications.push(wsId); + }, workspaceId); + + mockOnChat.mockImplementation(async function* (): AsyncGenerator< + WorkspaceChatMessage, + void, + unknown + > { + yield { type: "caught-up" }; + await Promise.resolve(); + yield { + type: "tool-call-end", + workspaceId, + messageId: "m1", + toolCallId: "call-1", + toolName: "file_edit_replace_string", + result: { success: true }, + timestamp: 1, + }; + }); + + createAndAddWorkspace(store, workspaceId); + await new Promise((resolve) => setTimeout(resolve, 10)); + + expect(notifications).toContain(workspaceId); + unsubscribe(); + }); + + it("does NOT notify for non-file-modifying tools", async () => { + const workspaceId = "file-modify-test-3"; + const notifications: string[] = []; + + const unsubscribe = store.subscribeFileModifyingTool((wsId) => { + notifications.push(wsId); + }, workspaceId); + + mockOnChat.mockImplementation(async function* (): AsyncGenerator< + WorkspaceChatMessage, + void, + unknown + > { + yield { type: "caught-up" }; + await Promise.resolve(); + yield { + type: "tool-call-end", + workspaceId, + messageId: "m1", + toolCallId: "call-1", + toolName: "web_search", // Not file-modifying + result: { success: true }, + timestamp: 1, + }; + }); + + createAndAddWorkspace(store, workspaceId); + await new Promise((resolve) => setTimeout(resolve, 10)); + + expect(notifications.length).toBe(0); + unsubscribe(); + }); + + it("only notifies subscribers for the specific workspace", async () => { + const workspaceId1 = "file-modify-test-4a"; + const workspaceId2 = "file-modify-test-4b"; + const notifications1: string[] = []; + const notifications2: string[] = []; + + const unsubscribe1 = store.subscribeFileModifyingTool((wsId) => { + notifications1.push(wsId); + }, workspaceId1); + + const unsubscribe2 = store.subscribeFileModifyingTool((wsId) => { + notifications2.push(wsId); + }, workspaceId2); + + // Only emit tool completion for workspace1 + mockOnChat.mockImplementation(async function* (): AsyncGenerator< + WorkspaceChatMessage, + void, + unknown + > { + yield { type: "caught-up" }; + await Promise.resolve(); + yield { + type: "tool-call-end", + workspaceId: workspaceId1, + messageId: "m1", + toolCallId: "call-1", + toolName: "bash", + result: { success: true, output: "done", exitCode: 0, wall_duration_ms: 1 }, + timestamp: 1, + }; + }); + + createAndAddWorkspace(store, workspaceId1); + await new Promise((resolve) => setTimeout(resolve, 10)); + + expect(notifications1.length).toBe(1); + expect(notifications2.length).toBe(0); // Not notified - different workspace + + unsubscribe1(); + unsubscribe2(); + }); + }); }); diff --git a/src/browser/stores/WorkspaceStore.ts b/src/browser/stores/WorkspaceStore.ts index 3fc72ea576..eddba112fb 100644 --- a/src/browser/stores/WorkspaceStore.ts +++ b/src/browser/stores/WorkspaceStore.ts @@ -830,6 +830,13 @@ export class WorkspaceStore { * * REQUIRES: Workspace must have been added via addWorkspace() first. */ + /** + * Get workspace name for logging/debugging. Returns "name" or falls back to ID. + */ + getWorkspaceName(workspaceId: string): string { + return this.workspaceMetadata.get(workspaceId)?.name ?? workspaceId; + } + getWorkspaceState(workspaceId: string): WorkspaceState { return this.states.get(workspaceId, () => { const aggregator = this.assertGet(workspaceId); @@ -1829,6 +1836,8 @@ export const workspaceStore = { getStoreInstance().getFileModifyingToolMs(workspaceId), clearFileModifyingToolMs: (workspaceId: string) => getStoreInstance().clearFileModifyingToolMs(workspaceId), + /** Get workspace name for logging/debugging */ + getWorkspaceName: (workspaceId: string) => getStoreInstance().getWorkspaceName(workspaceId), }; /** diff --git a/src/browser/utils/RefreshController.test.ts b/src/browser/utils/RefreshController.test.ts index ab844fc7c5..1fc94d1cdf 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,36 @@ 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 (via setTimeout 0 in onComplete) + jest.advanceTimersByTime(1); + expect(onRefresh).toHaveBeenCalledTimes(2); + + controller.dispose(); + }); + it("isRefreshing reflects in-flight state", () => { let resolveRefresh: () => void; const refreshPromise = new Promise((resolve) => { @@ -151,4 +196,57 @@ describe("RefreshController", () => { 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(100); + expect(controller.lastRefreshInfo!.trigger).toBe("scheduled"); + + // Priority refresh should record "priority" trigger + controller.schedulePriority(); + jest.advanceTimersByTime(100); + 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(100); + 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 c5c42eef99..07551103ac 100644 --- a/src/browser/utils/RefreshController.ts +++ b/src/browser/utils/RefreshController.ts @@ -11,10 +11,30 @@ * 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; @@ -36,15 +56,20 @@ export interface RefreshControllerOptions { * 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; } 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 inFlight = false; @@ -54,6 +79,10 @@ export class RefreshController { private lastFocusRefreshMs = 0; private disposed = false; + // Track last refresh for debugging + private _lastRefreshInfo: LastRefreshInfo | null = null; + private pendingTrigger: RefreshTrigger | null = null; + // Track if listeners are bound (for cleanup) private listenersBound = false; private boundHandleVisibility: (() => void) | null = null; @@ -61,37 +90,70 @@ 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 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); + this.scheduleWithDelay(this.debounceMs, "scheduled"); } /** - * Schedule with priority (shorter) debounce. Used for active workspace. + * Schedule with priority (shorter) rate limit. Used for active workspace. */ schedulePriority(): void { - this.scheduleWithDelay(this.priorityDebounceMs); + this.scheduleWithDelay(this.priorityDebounceMs, "priority"); } - private scheduleWithDelay(delayMs: number): void { + private scheduleWithDelay(delayMs: number, trigger: RefreshTrigger): void { if (this.disposed) return; + // Always update pending trigger (use priority if upgrading from scheduled) + if (!this.pendingTrigger || trigger === "priority") { + this.pendingTrigger = 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(); + const t = this.pendingTrigger ?? trigger; + this.pendingTrigger = null; + this.debug(`timer fired, refreshing (${t})`); + this.tryRefresh({ trigger: t }); }, delayMs); } @@ -108,18 +170,21 @@ export class RefreshController { this.debounceTimer = null; } - this.tryRefresh({ bypassPause: true }); + this.tryRefresh({ bypassPause: true, trigger: "manual" }); } /** * Attempt refresh, respecting pause conditions. */ - private tryRefresh(options?: { bypassPause?: boolean }): void { + private tryRefresh(options?: { bypassPause?: boolean; trigger?: RefreshTrigger }): void { if (this.disposed) return; + const trigger = options?.trigger ?? this.pendingTrigger ?? "scheduled"; + // Hidden → queue for visibility if (typeof document !== "undefined" && document.hidden) { this.pendingBecauseHidden = true; + this.pendingTrigger = trigger; return; } @@ -127,36 +192,44 @@ export class RefreshController { // Bypassed for manual refresh (user explicitly requested) if (!options?.bypassPause && this.isPaused?.()) { this.pendingBecausePaused = true; + this.pendingTrigger = trigger; return; } // In-flight → queue for completion if (this.inFlight) { this.pendingBecauseInFlight = true; + this.pendingTrigger = trigger; return; } - this.executeRefresh(); + this.executeRefresh(trigger); } /** * Execute the refresh, tracking in-flight state. */ - private executeRefresh(): void { + private executeRefresh(trigger: RefreshTrigger): void { if (this.disposed) return; 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); } }; @@ -170,14 +243,14 @@ 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(); + this.tryRefresh({ trigger }); return; // Don't double-refresh with proactive } @@ -186,7 +259,7 @@ export class RefreshController { const now = Date.now(); if (now - this.lastFocusRefreshMs >= this.focusDebounceMs) { this.lastFocusRefreshMs = now; - this.tryRefresh(); + this.tryRefresh({ trigger }); } } } @@ -199,7 +272,7 @@ export class RefreshController { if (this.disposed) return; if (this.pendingBecausePaused) { this.pendingBecausePaused = false; - this.tryRefresh(); + this.tryRefresh({ trigger: "unpaused" }); } } @@ -215,12 +288,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); @@ -234,6 +307,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. */ 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 edc1f0890aeecf3582ea14a9bbda4f09435de612 Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 22 Dec 2025 21:00:14 -0600 Subject: [PATCH 03/10] debug: add logging to manual refresh path for diagnosis --- .../RightSidebar/CodeReview/RefreshButton.tsx | 8 ++++- .../RightSidebar/CodeReview/ReviewPanel.tsx | 8 +++++ .../hooks/useReviewRefreshController.ts | 31 +++++++++++++++++-- src/browser/utils/RefreshController.ts | 20 ++++++++++-- 4 files changed, 60 insertions(+), 7 deletions(-) diff --git a/src/browser/components/RightSidebar/CodeReview/RefreshButton.tsx b/src/browser/components/RightSidebar/CodeReview/RefreshButton.tsx index 00c2c06059..a90c6fef09 100644 --- a/src/browser/components/RightSidebar/CodeReview/RefreshButton.tsx +++ b/src/browser/components/RightSidebar/CodeReview/RefreshButton.tsx @@ -18,6 +18,7 @@ interface RefreshButtonProps { /** Human-readable trigger labels */ const TRIGGER_LABELS: Record = { + initial: "initial load", manual: "manual click", scheduled: "tool completion", priority: "tool completion (priority)", @@ -29,6 +30,11 @@ const TRIGGER_LABELS: Record = { export const RefreshButton: React.FC = (props) => { const { onClick, isLoading = false, lastRefreshInfo } = props; + + const handleClick = () => { + console.debug("[RefreshButton] clicked"); + onClick(); + }; // Track animation state for graceful stopping const [animationState, setAnimationState] = useState<"idle" | "spinning" | "stopping">("idle"); const spinOnceTimeoutRef = useRef(null); @@ -67,7 +73,7 @@ export const RefreshButton: React.FC = (props) => {