From f9d5b6bb024479026ee53d87e54ff96aedda6d96 Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Thu, 17 Sep 2026 23:40:00 -0700 Subject: [PATCH] fix(renderer): dispose global listeners during HMR (#20908) * fix(renderer): dispose combined diff cache listener on HMR * fix(renderer): dispose contextual tour key guard on HMR * fix(renderer): dispose activity pagehide listener on HMR * fix(renderer): dispose keyboard layout hooks on HMR * fix(renderer): dispose input quiet listeners on HMR * fix(renderer): dispose desync sentinel listener on HMR * fix: address memory PR review regressions and withdraw false positives --------- Co-authored-by: m4air Co-authored-by: m4air --- .../activity-clear-completed-hmr.test.ts | 177 ++++++++++++++++++ .../activity/activity-clear-completed.ts | 23 +++ .../ContextualTourOverlaySurface.tsx | 20 ++ .../combined-diff-view-memory.ts | 34 +++- .../terminal-render-desync-trigger.ts | 9 + src/renderer/src/lib/input-quiet-scheduler.ts | 20 ++ .../keyboard-layout/layout-base-character.ts | 16 ++ 7 files changed, 292 insertions(+), 7 deletions(-) create mode 100644 src/renderer/src/components/activity/activity-clear-completed-hmr.test.ts diff --git a/src/renderer/src/components/activity/activity-clear-completed-hmr.test.ts b/src/renderer/src/components/activity/activity-clear-completed-hmr.test.ts new file mode 100644 index 00000000000..b1698551c5d --- /dev/null +++ b/src/renderer/src/components/activity/activity-clear-completed-hmr.test.ts @@ -0,0 +1,177 @@ +// @vitest-environment happy-dom +import { afterEach, beforeEach, expect, it, vi } from 'vitest' +import type { RetainedAgentEntry } from '@/store/slices/agent-status' +import type { ActivityEvent, AgentPaneThread } from './activity-thread-types' +import { makeTab, makeWorktree } from './ActivityPrototypePage-test-fixtures' + +const mockStore = vi.hoisted(() => { + const activityClearedAtByPaneKey: Record = {} + const agentStatusByPaneKey: Record = {} + const retainedAgentsByPaneKey: Record = {} + const retentionSuppressedPaneKeys: Record = {} + const state = { + activityClearedAtByPaneKey, + agentStatusByPaneKey, + retainedAgentsByPaneKey, + retentionSuppressedPaneKeys, + applyActivityClearedAt: vi.fn((patch: Record) => { + const next = { ...state.activityClearedAtByPaneKey } + for (const [key, value] of Object.entries(patch)) { + if (value === null) { + delete next[key] + } else { + next[key] = value + } + } + state.activityClearedAtByPaneKey = next + }), + dismissRetainedAgents: vi.fn((paneKeys: readonly string[]) => { + const next = { ...state.retainedAgentsByPaneKey } + for (const key of paneKeys) { + if (state.agentStatusByPaneKey[key]) { + state.retentionSuppressedPaneKeys[key] = true + } + delete next[key] + } + state.retainedAgentsByPaneKey = next + }), + clearRetentionSuppressedPaneKeys: vi.fn((paneKeys: string[]) => { + for (const key of paneKeys) { + delete state.retentionSuppressedPaneKeys[key] + } + }), + retainAgents: vi.fn((entries: RetainedAgentEntry[]) => { + const next = { ...state.retainedAgentsByPaneKey } + for (const retained of entries) { + next[retained.entry.paneKey] = retained + } + state.retainedAgentsByPaneKey = next + }) + } + return state +}) + +const toastSpy = vi.hoisted(() => vi.fn()) + +vi.mock('@/store', () => ({ + useAppStore: { getState: () => mockStore } +})) +vi.mock('sonner', () => ({ toast: toastSpy })) + +import { + CLEAR_COMPLETED_EVICTION_FALLBACK_MS, + disposePendingClearCompletedEvictionListener, + clearCompletedActivity, + flushPendingClearCompletedEvictions +} from './activity-clear-completed' + +function makeThread(paneKey: string, overrides: Partial = {}): AgentPaneThread { + return { + paneKey, + tab: makeTab(), + worktree: makeWorktree(), + repo: null, + currentAgentState: null, + currentAgentEntry: null, + latestEvent: null, + latestTimestamp: 5_000, + agentType: 'claude', + unread: false, + paneTitle: `Agent ${paneKey}`, + responsePreview: '', + events: [], + ...overrides + } +} + +function doneEvent(interrupted: boolean): ActivityEvent { + return { + id: 'evt', + state: 'done', + timestamp: 5_000, + worktree: makeWorktree(), + repo: null, + entry: { ...makeRetained('t-done:1').entry, interrupted }, + tab: makeTab(), + agentType: 'claude', + agentAlive: false, + unread: false + } +} + +const doneThread = makeThread('t-done:1', { latestEvent: doneEvent(false) }) + +function makeRetained(paneKey: string): RetainedAgentEntry { + return { + entry: { + state: 'done', + prompt: 'retained run', + updatedAt: 5_000, + stateStartedAt: 5_000, + paneKey, + stateHistory: [], + agentType: 'claude' + }, + worktreeId: 'wt-1', + tab: makeTab(), + agentType: 'claude', + startedAt: 5_000 + } +} + +const drop = vi.fn() +const domWindow = window +beforeEach(() => { + vi.useFakeTimers() + Object.assign(domWindow, { api: { agentStatus: { dropPersistedBatch: drop } } }) + mockStore.retainedAgentsByPaneKey = { 't-done:1': makeRetained('t-done:1') } + mockStore.activityClearedAtByPaneKey = {} + mockStore.retentionSuppressedPaneKeys = {} +}) +afterEach(() => { + flushPendingClearCompletedEvictions() + vi.useRealTimers() + vi.clearAllMocks() +}) + +it('preserves the pending pagehide flush through HMR and removes the listener when drained', () => { + const remove = vi.spyOn(domWindow, 'removeEventListener') + try { + clearCompletedActivity([doneThread]) + disposePendingClearCompletedEvictionListener() + expect(remove).not.toHaveBeenCalledWith('pagehide', flushPendingClearCompletedEvictions) + domWindow.dispatchEvent(new Event('pagehide')) + expect(drop).toHaveBeenCalledOnce() + expect(remove).toHaveBeenCalledWith('pagehide', flushPendingClearCompletedEvictions) + } finally { + remove.mockRestore() + } +}) + +it('preserves Undo after HMR without dropping the restored persisted row', () => { + clearCompletedActivity([doneThread]) + disposePendingClearCompletedEvictionListener() + const options = toastSpy.mock.calls.at(-1)?.[1] + options.action.onClick() + domWindow.dispatchEvent(new Event('pagehide')) + vi.advanceTimersByTime(CLEAR_COMPLETED_EVICTION_FALLBACK_MS) + expect(drop).not.toHaveBeenCalled() + expect(mockStore.retainedAgentsByPaneKey['t-done:1']).toBeDefined() +}) + +it('releases the retired listener when its fallback settles and supports a late stale handler', () => { + const remove = vi.spyOn(domWindow, 'removeEventListener') + try { + clearCompletedActivity([doneThread]) + disposePendingClearCompletedEvictionListener() + vi.advanceTimersByTime(CLEAR_COMPLETED_EVICTION_FALLBACK_MS) + expect(drop).toHaveBeenCalledOnce() + expect(remove).toHaveBeenCalledWith('pagehide', flushPendingClearCompletedEvictions) + mockStore.retainedAgentsByPaneKey = { 't-done:1': makeRetained('t-done:1') } + clearCompletedActivity([doneThread]) + domWindow.dispatchEvent(new Event('pagehide')) + expect(drop).toHaveBeenCalledTimes(2) + } finally { + remove.mockRestore() + } +}) diff --git a/src/renderer/src/components/activity/activity-clear-completed.ts b/src/renderer/src/components/activity/activity-clear-completed.ts index 3c6e743a8bb..2bb39fdff43 100644 --- a/src/renderer/src/components/activity/activity-clear-completed.ts +++ b/src/renderer/src/components/activity/activity-clear-completed.ts @@ -68,16 +68,34 @@ export function planClearCompletedActivity( // Deferred evictions whose undo toast is still open; flushed on pagehide because the toast's // close callbacks never fire on quit/reload, which would let cleared rows replay next launch. const pendingDiskEvictions = new Set<() => void>() +let evictionListenerRetired = false export function flushPendingClearCompletedEvictions(): void { // Set iteration tolerates the self-delete each evict() performs. for (const evict of pendingDiskEvictions) { evict() } } +export function disposePendingClearCompletedEvictionListener(): void { + evictionListenerRetired = true + releaseRetiredEvictionListener() +} + +function releaseRetiredEvictionListener(): void { + if (evictionListenerRetired && pendingDiskEvictions.size === 0 && typeof window !== 'undefined') { + window.removeEventListener('pagehide', flushPendingClearCompletedEvictions) + } +} + if (typeof window !== 'undefined') { window.addEventListener('pagehide', flushPendingClearCompletedEvictions) } +if (import.meta !== undefined && import.meta.hot) { + // Vite can replace this module without a full renderer reload. Remove the + // pagehide hook so dev sessions do not retain stale eviction closures. + import.meta.hot.dispose(disposePendingClearCompletedEvictionListener) +} + // Why a fallback: sonner only fires onDismiss/onAutoClose for the toast's own close paths; a // `toast.dismiss()` from another caller leaves the eviction pending until pagehide. export const CLEAR_COMPLETED_EVICTION_FALLBACK_MS = 60_000 @@ -143,6 +161,7 @@ export function clearCompletedActivity(threads: readonly AgentPaneThread[]): boo let fallbackTimer: ReturnType | null = null const dropRetainedFromDiskCache = (): void => { pendingDiskEvictions.delete(dropRetainedFromDiskCache) + releaseRetiredEvictionListener() if (fallbackTimer !== null) { clearTimeout(fallbackTimer) fallbackTimer = null @@ -154,6 +173,9 @@ export function clearCompletedActivity(threads: readonly AgentPaneThread[]): boo evictPersistedStatuses(plan.cacheIdentities) } pendingDiskEvictions.add(dropRetainedFromDiskCache) + if (evictionListenerRetired && typeof window !== 'undefined') { + window.addEventListener('pagehide', flushPendingClearCompletedEvictions) + } fallbackTimer = setTimeout(dropRetainedFromDiskCache, CLEAR_COMPLETED_EVICTION_FALLBACK_MS) toast( plan.clearedThreadCount === 1 @@ -169,6 +191,7 @@ export function clearCompletedActivity(threads: readonly AgentPaneThread[]): boo onClick: () => { undone = true pendingDiskEvictions.delete(dropRetainedFromDiskCache) + releaseRetiredEvictionListener() if (fallbackTimer !== null) { clearTimeout(fallbackTimer) fallbackTimer = null diff --git a/src/renderer/src/components/contextual-tours/ContextualTourOverlaySurface.tsx b/src/renderer/src/components/contextual-tours/ContextualTourOverlaySurface.tsx index 5b6284ae648..1c176502613 100644 --- a/src/renderer/src/components/contextual-tours/ContextualTourOverlaySurface.tsx +++ b/src/renderer/src/components/contextual-tours/ContextualTourOverlaySurface.tsx @@ -60,6 +60,20 @@ type ContextualTourOverlaySurfaceProps = { onOverlayKeyDownCapture: (event: KeyboardEvent) => void } +function disposeContextualTourGlobalKeyGuard(): void { + if (typeof window === 'undefined') { + return + } + const guardedWindow = window as Window & { + __orcaContextualTourGlobalKeyGuardInstalled?: boolean + } + if (!guardedWindow.__orcaContextualTourGlobalKeyGuardInstalled) { + return + } + window.removeEventListener('keydown', handleContextualTourGlobalKeyDown, true) + delete guardedWindow.__orcaContextualTourGlobalKeyGuardInstalled +} + if (typeof window !== 'undefined') { const guardedWindow = window as Window & { __orcaContextualTourGlobalKeyGuardInstalled?: boolean @@ -70,6 +84,12 @@ if (typeof window !== 'undefined') { } } +if (import.meta !== undefined && import.meta.hot) { + // Vite can replace this module without a full renderer reload. Remove the + // global key guard so dev sessions do not retain stale module closures. + import.meta.hot.dispose(disposeContextualTourGlobalKeyGuard) +} + const PANEL_BASE_CLASSES = 'orca-contextual-tour-panel rounded-lg border border-border text-popover-foreground backdrop-blur-[2px]' diff --git a/src/renderer/src/components/editor/combined-diff/remember-view/combined-diff-view-memory.ts b/src/renderer/src/components/editor/combined-diff/remember-view/combined-diff-view-memory.ts index 8360fa40b33..8c1df7e86d6 100644 --- a/src/renderer/src/components/editor/combined-diff/remember-view/combined-diff-view-memory.ts +++ b/src/renderer/src/components/editor/combined-diff/remember-view/combined-diff-view-memory.ts @@ -34,12 +34,32 @@ function invalidateCombinedDiffCachesForRelativePath(relativePath: string): void } } +function handleCombinedDiffExternalFileChange(event: Event): void { + const detail = (event as CustomEvent).detail + if (detail?.relativePath) { + // Why: inactive combined-diff tabs are unmounted, so only a module-level cache bust stops a remount replaying stale bodies. + invalidateCombinedDiffCachesForRelativePath(detail.relativePath) + } +} + +export function disposeCombinedDiffViewMemory(): void { + if (typeof window !== 'undefined') { + window.removeEventListener( + ORCA_EDITOR_EXTERNAL_FILE_CHANGE_EVENT, + handleCombinedDiffExternalFileChange + ) + } +} + if (typeof window !== 'undefined') { - window.addEventListener(ORCA_EDITOR_EXTERNAL_FILE_CHANGE_EVENT, (event) => { - const detail = (event as CustomEvent).detail - if (detail?.relativePath) { - // Why: inactive combined-diff tabs are unmounted, so only a module-level cache bust stops a remount replaying stale bodies. - invalidateCombinedDiffCachesForRelativePath(detail.relativePath) - } - }) + window.addEventListener( + ORCA_EDITOR_EXTERNAL_FILE_CHANGE_EVENT, + handleCombinedDiffExternalFileChange + ) +} + +if (import.meta !== undefined && import.meta.hot) { + // Vite can replace this module without a full renderer reload. Remove the + // global cache-bust listener so dev sessions do not accumulate handlers. + import.meta.hot.dispose(disposeCombinedDiffViewMemory) } diff --git a/src/renderer/src/components/terminal-pane/terminal-render-desync-trigger.ts b/src/renderer/src/components/terminal-pane/terminal-render-desync-trigger.ts index e65b19030e7..fa872efde43 100644 --- a/src/renderer/src/components/terminal-pane/terminal-render-desync-trigger.ts +++ b/src/renderer/src/components/terminal-pane/terminal-render-desync-trigger.ts @@ -118,3 +118,12 @@ function removeClickListener(): void { clickListener = null } } + +if (import.meta !== undefined && import.meta.hot) { + // Vite can replace this module without a full renderer reload. Remove the + // opt-in gesture hook so dev sessions do not retain stale pane closures. + import.meta.hot.dispose(() => { + removeClickListener() + stopRenderDesyncSampleBurst() + }) +} diff --git a/src/renderer/src/lib/input-quiet-scheduler.ts b/src/renderer/src/lib/input-quiet-scheduler.ts index 66ed6c6d90f..c968fe38a8b 100644 --- a/src/renderer/src/lib/input-quiet-scheduler.ts +++ b/src/renderer/src/lib/input-quiet-scheduler.ts @@ -15,6 +15,7 @@ const INPUT_QUIET_EVENTS: readonly (keyof WindowEventMap)[] = [ ] let listenersInstalled = false +let listenersWindow: Window | null = null let lastInputAt = Number.NEGATIVE_INFINITY function now(): number { @@ -49,6 +50,25 @@ function ensureInputQuietListeners(targetWindow: Window): void { for (const eventName of INPUT_QUIET_EVENTS) { targetWindow.addEventListener(eventName, recordInput, options) } + listenersWindow = targetWindow +} + +function disposeInputQuietListeners(): void { + if (!listenersWindow) { + return + } + const options: AddEventListenerOptions = { capture: true } + for (const eventName of INPUT_QUIET_EVENTS) { + listenersWindow.removeEventListener(eventName, recordInput, options) + } + listenersWindow = null + listenersInstalled = false +} + +if (import.meta !== undefined && import.meta.hot) { + // Vite can replace this module without a full renderer reload. Remove the + // global input hooks so dev sessions do not retain stale scheduler closures. + import.meta.hot.dispose(disposeInputQuietListeners) } function scheduleIdleCallback(targetWindow: Window, callback: () => void, timeout: number): number { diff --git a/src/renderer/src/lib/keyboard-layout/layout-base-character.ts b/src/renderer/src/lib/keyboard-layout/layout-base-character.ts index 8f05f9ba827..3fdc8206c9a 100644 --- a/src/renderer/src/lib/keyboard-layout/layout-base-character.ts +++ b/src/renderer/src/lib/keyboard-layout/layout-base-character.ts @@ -108,6 +108,22 @@ function refreshOnFocus(): void { void refreshLayoutMap() } +function disposeLayoutCharacterPrefetch(): void { + if (attachedWindow) { + attachedWindow.removeEventListener('focus', refreshOnFocus) + attachedWindow = null + } + unsubscribeLayoutChange?.() + unsubscribeLayoutChange = null + focusListenerAttached = false +} + +if (import.meta !== undefined && import.meta.hot) { + // Vite can replace this module without a full renderer reload. Remove the + // global focus/layout hooks so dev sessions do not accumulate listeners. + import.meta.hot.dispose(disposeLayoutCharacterPrefetch) +} + /** A layout map entry is usable as a kitty base key only if it is a single * printable codepoint (dead keys report names like 'Dead'; some entries are * empty). Exposed for tests. */