From f1dd8d32a81a3c342d65b8d74602b05ab93e365a Mon Sep 17 00:00:00 2001 From: Adam Durham Date: Fri, 31 Jul 2026 13:49:48 -0500 Subject: [PATCH] test(desktop): extract ProfileRail focus/visibilitychange wiring into a tested hook Addresses review feedback from the hermes-sweeper (salvageability=high, keep_open): "The new focus/visibility listener behavior lacks a runtime UI regression test... no ProfileRail test." Rendering the full ProfileRail component for this would drag in drag-and-drop, dialogs, hotkeys, and i18n unrelated to what needs testing. Instead, extracted the focus/visibilitychange wiring into its own use-profile-rail-refresh-on-active hook, matching this exact directory's own established convention (use-profile-prewarm.ts is the same shape: a small side-effect hook pulled out of ProfileRail specifically so it's unit-testable in isolation). Added 6 tests covering exactly what the review asked for: refresh on mount, refresh on window focus, refresh on visibilitychange while visible, NO refresh on visibilitychange while hidden, listener cleanup on unmount, and no listener accumulation across repeated mount/unmount cycles. Verified the tests have real teeth: simulated the exact bug this PR originally fixed (dropped the cleanup return, leaving listeners attached after unmount) and confirmed 4 of 6 tests correctly fail against it -- including "no accumulate listeners" showing 7 calls instead of 1, the exact leaked-listener signature. Restored the real fix and all 6 pass. ProfileRail itself is otherwise unchanged in behavior -- this is a pure extraction (same effect, same dependencies, same cleanup), not a behavior change. Full sidebar test suite: 93 passed across 12 files (up from 87 across 11), 0 regressions. Python side unaffected: 158 passed. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/app/chat/sidebar/profile-switcher.tsx | 29 +----- ...use-profile-rail-refresh-on-active.test.ts | 91 +++++++++++++++++++ .../use-profile-rail-refresh-on-active.ts | 38 ++++++++ 3 files changed, 134 insertions(+), 24 deletions(-) create mode 100644 apps/desktop/src/app/chat/sidebar/use-profile-rail-refresh-on-active.test.ts create mode 100644 apps/desktop/src/app/chat/sidebar/use-profile-rail-refresh-on-active.ts diff --git a/apps/desktop/src/app/chat/sidebar/profile-switcher.tsx b/apps/desktop/src/app/chat/sidebar/profile-switcher.tsx index 24cec7297c..5074df3ce5 100644 --- a/apps/desktop/src/app/chat/sidebar/profile-switcher.tsx +++ b/apps/desktop/src/app/chat/sidebar/profile-switcher.tsx @@ -69,6 +69,7 @@ import { RenameProfileDialog } from '../../profiles/rename-profile-dialog' import { PROFILES_ROUTE, SETTINGS_ROUTE } from '../../routes' import { useProfilePrewarm } from './use-profile-prewarm' +import { useProfileRailRefreshOnActive } from './use-profile-rail-refresh-on-active' const RAIL_GAP = 4 // px — matches gap-1 between squares. @@ -207,30 +208,10 @@ export function ProfileRail() { // Re-pull the running profile + list on mount, and again whenever the window // regains focus/visibility -- a profile created, deleted, or renamed by // another surface (Manage Profiles, another window, the CLI) leaves this - // rail's cached $profiles stale until something re-fetches it. Without this, - // a deleted profile's square lingered in the rail until the user happened to - // open Manage Profiles (whose own refresh() call was the only other reader). - // Cheap and best-effort, matching the focus/visibilitychange refresh pattern - // used elsewhere in the sidebar (see refreshProjects/refreshProjectTree). - useEffect(() => { - void refreshActiveProfile() - - const onActive = () => { - if (document.visibilityState === 'hidden') { - return - } - - void refreshActiveProfile() - } - - window.addEventListener('focus', onActive) - document.addEventListener('visibilitychange', onActive) - - return () => { - window.removeEventListener('focus', onActive) - document.removeEventListener('visibilitychange', onActive) - } - }, []) + // rail's cached $profiles stale until something re-fetches it. See + // use-profile-rail-refresh-on-active.ts for the extracted (and tested) + // wiring. + useProfileRailRefreshOnActive() // Open the create dialog when the `profile.create` hotkey fires (the dialog // state lives here, so the global keybind bumps a request atom we watch). diff --git a/apps/desktop/src/app/chat/sidebar/use-profile-rail-refresh-on-active.test.ts b/apps/desktop/src/app/chat/sidebar/use-profile-rail-refresh-on-active.test.ts new file mode 100644 index 0000000000..9f15a2ab9c --- /dev/null +++ b/apps/desktop/src/app/chat/sidebar/use-profile-rail-refresh-on-active.test.ts @@ -0,0 +1,91 @@ +import { act, renderHook } from '@testing-library/react' +import { afterEach, describe, expect, it, vi } from 'vitest' + +const { refreshActiveProfile } = vi.hoisted(() => ({ + refreshActiveProfile: vi.fn().mockResolvedValue(undefined) +})) + +vi.mock('@/store/profile', () => ({ refreshActiveProfile })) + +import { useProfileRailRefreshOnActive } from './use-profile-rail-refresh-on-active' + +describe('useProfileRailRefreshOnActive', () => { + afterEach(() => { + refreshActiveProfile.mockClear() + vi.restoreAllMocks() + }) + + it('refreshes once on mount', () => { + renderHook(() => useProfileRailRefreshOnActive()) + + expect(refreshActiveProfile).toHaveBeenCalledTimes(1) + }) + + it('refreshes again when the window regains focus', async () => { + renderHook(() => useProfileRailRefreshOnActive()) + refreshActiveProfile.mockClear() + + await act(async () => { + window.dispatchEvent(new Event('focus')) + }) + + expect(refreshActiveProfile).toHaveBeenCalledTimes(1) + }) + + it('refreshes when visibilitychange fires while the document is visible', async () => { + vi.spyOn(document, 'visibilityState', 'get').mockReturnValue('visible') + renderHook(() => useProfileRailRefreshOnActive()) + refreshActiveProfile.mockClear() + + await act(async () => { + document.dispatchEvent(new Event('visibilitychange')) + }) + + expect(refreshActiveProfile).toHaveBeenCalledTimes(1) + }) + + it('does NOT refresh when visibilitychange fires while the document is hidden', async () => { + // Backgrounding/tab-switching away must not trigger a redundant refresh + // -- only becoming visible/focused again should. + vi.spyOn(document, 'visibilityState', 'get').mockReturnValue('hidden') + renderHook(() => useProfileRailRefreshOnActive()) + refreshActiveProfile.mockClear() + + await act(async () => { + document.dispatchEvent(new Event('visibilitychange')) + }) + + expect(refreshActiveProfile).not.toHaveBeenCalled() + }) + + it('removes both listeners on unmount, so a later focus/visibilitychange event does not refresh', async () => { + vi.spyOn(document, 'visibilityState', 'get').mockReturnValue('visible') + const { unmount } = renderHook(() => useProfileRailRefreshOnActive()) + refreshActiveProfile.mockClear() + + unmount() + + await act(async () => { + window.dispatchEvent(new Event('focus')) + document.dispatchEvent(new Event('visibilitychange')) + }) + + expect(refreshActiveProfile).not.toHaveBeenCalled() + }) + + it('does not accumulate listeners across repeated mount/unmount cycles', async () => { + // A leaked listener from a prior mount would double- (or N-times-) + // refresh on a single focus event after remounting. + const first = renderHook(() => useProfileRailRefreshOnActive()) + first.unmount() + + renderHook(() => useProfileRailRefreshOnActive()) + refreshActiveProfile.mockClear() + + await act(async () => { + window.dispatchEvent(new Event('focus')) + }) + + expect(refreshActiveProfile).toHaveBeenCalledTimes(1) + }) +}) diff --git a/apps/desktop/src/app/chat/sidebar/use-profile-rail-refresh-on-active.ts b/apps/desktop/src/app/chat/sidebar/use-profile-rail-refresh-on-active.ts new file mode 100644 index 0000000000..685ba3deee --- /dev/null +++ b/apps/desktop/src/app/chat/sidebar/use-profile-rail-refresh-on-active.ts @@ -0,0 +1,38 @@ +import { useEffect } from 'react' + +import { refreshActiveProfile } from '@/store/profile' + +/** + * Re-pull the running profile + list on mount, and again whenever the window + * regains focus/visibility -- a profile created, deleted, or renamed by + * another surface (Manage Profiles, another window, the CLI) leaves the + * rail's cached $profiles stale until something re-fetches it. Without this, + * a deleted profile's square lingers in the rail until the user happens to + * open Manage Profiles (whose own refresh() call was the only other reader). + * + * Cheap and best-effort, matching the focus/visibilitychange refresh pattern + * used elsewhere in the sidebar (see refreshProjects/refreshProjectTree). + * Extracted into its own hook (rather than left inline in ProfileRail) so the + * focus/visibility wiring is unit-testable without rendering the whole rail. + */ +export function useProfileRailRefreshOnActive(): void { + useEffect(() => { + void refreshActiveProfile() + + const onActive = () => { + if (document.visibilityState === 'hidden') { + return + } + + void refreshActiveProfile() + } + + window.addEventListener('focus', onActive) + document.addEventListener('visibilitychange', onActive) + + return () => { + window.removeEventListener('focus', onActive) + document.removeEventListener('visibilitychange', onActive) + } + }, []) +}