diff --git a/apps/mobile/src/app/(app)/_layout.tsx b/apps/mobile/src/app/(app)/_layout.tsx index d0c907a5a25..59103441dd5 100644 --- a/apps/mobile/src/app/(app)/_layout.tsx +++ b/apps/mobile/src/app/(app)/_layout.tsx @@ -1,7 +1,6 @@ import { Stack } from 'expo-router'; import { appUnlockScreenLayout } from '@/components/app-unlock-screen'; -import { useEffect } from 'react'; -import { AppState } from 'react-native'; +import { useEffect, useRef } from 'react'; import { UserWebConnectionProvider } from '@/components/agents/user-web-connection-provider'; import { KiloChatPresenceMount } from '@/components/kilo-chat/kilo-chat-presence-mount'; @@ -21,6 +20,7 @@ import { import { useFormSheetScreenOptions } from '@/lib/form-sheet'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; import { useCurrentUserId } from '@/lib/hooks/use-current-user-id'; +import { useAppLifecycle } from '@/lib/hooks/use-app-lifecycle'; import { useRouteForegroundRefresh } from '@/lib/hooks/use-route-foreground-refresh'; import { useSecurityLifecycleInvalidation } from '@/lib/hooks/use-security-lifecycle-invalidation'; import { CachePersistenceMount } from '@/lib/persist/cache-persistence-mount'; @@ -29,67 +29,56 @@ import { ToolSummaryTranslationRetryMount } from '@/lib/tool-summary-translation import { useTRPC } from '@/lib/trpc'; /** - * Attempts failed logout cleanup on every "next authenticated opportunity": - * once `user.getMe` has resolved on the authenticated mount, and on each - * AppState return to `active` while authenticated. The attempt itself is - * single-flight with 60 s spacing, so foreground flaps do not hammer the - * server and a transient failure retries on the next foreground. + * The single owner of the app's authenticated foreground reconciliation work, + * replacing the two independent `AppState` listeners the logout and push + * mounts used to register. Every pass runs the same ordered queue: failed + * logout cleanup first, then push-token ownership. + * + * It fires on the same triggers as before — once `user.getMe` has resolved on + * the authenticated mount (plus the push-token rotation subscription), and on + * each return to the foreground — but the foreground path is the app's shared + * `useAppLifecycle()` store, so the whole tree keeps one `AppState` listener + * and the pair runs only on the background -> active edge. The store seeds + * `isActive` as `true`, so a mount while already active is not an edge and + * only the mount attempt runs. + * + * No minimum interval is added at this layer: both attempts are already + * single-flight with 60 s spacing, and + * `attemptPushRegistrationReconciliation` additionally skips when the stored + * token, locale and app version already match the device + * (`src/lib/auth/push-registration-reconciliation.ts`). Re-gating them here + * would duplicate that contract without adding a skip. */ -function LogoutReconciliationMount() { +function ForegroundReconciliationMount() { const { userId, isLoading, isError } = useCurrentUserId(); + const { isActive } = useAppLifecycle(); + const wasActiveRef = useRef(isActive); + // Mount attempt and push-token rotation subscription. useEffect(() => { if (!userId || isLoading || isError) { return undefined; } void attemptLogoutReconciliation(userId); - - const subscription = AppState.addEventListener('change', nextState => { - if (nextState === 'active') { - void attemptLogoutReconciliation(userId); - } - }); - - return () => { - subscription.remove(); - }; - }, [userId, isLoading, isError]); - - return null; -} - -/** - * Ensures the signed-in user owns the device's Expo push token on every - * authenticated opportunity: once `user.getMe` has resolved on the - * authenticated mount, on each AppState return to `active`, and on each push - * token rotation. The attempt itself is single-flight with 60 s spacing, so - * foreground flaps do not hammer the server and a transient failure retries - * on the next foreground. - */ -function PushRegistrationMount() { - const { userId, isLoading, isError } = useCurrentUserId(); - - useEffect(() => { - if (!userId || isLoading || isError) { - return undefined; - } - void attemptPushRegistrationReconciliation(userId); - - const appState = AppState.addEventListener('change', next => { - if (next === 'active') { - void attemptPushRegistrationReconciliation(userId); - } - }); const unsubscribeRotation = subscribeToPushTokenRotation(userId); return () => { - appState.remove(); unsubscribeRotation(); }; }, [userId, isLoading, isError]); + // Foreground regain: the false -> true edge only, so an active -> active + // echo (or a duplicate `active` from the OS) never re-runs the pair. + useEffect(() => { + if (!wasActiveRef.current && isActive && userId && !isLoading && !isError) { + void attemptLogoutReconciliation(userId); + void attemptPushRegistrationReconciliation(userId); + } + wasActiveRef.current = isActive; + }, [isActive, userId, isLoading, isError]); + return null; } @@ -126,8 +115,7 @@ export default function AppLayout() { - - + diff --git a/apps/mobile/src/app/(app)/agent-chat/[session-id].mounted.test.tsx b/apps/mobile/src/app/(app)/agent-chat/[session-id].mounted.test.tsx index 9af4510a1bb..e189ab0823c 100644 --- a/apps/mobile/src/app/(app)/agent-chat/[session-id].mounted.test.tsx +++ b/apps/mobile/src/app/(app)/agent-chat/[session-id].mounted.test.tsx @@ -43,6 +43,8 @@ vi.mock('@/components/ui/refresh-control', () => ({ RefreshControl: 'RefreshCont const useRouterMock = vi.hoisted(() => vi.fn()); const useQueryMock = vi.hoisted(() => vi.fn()); const queryOptionsMock = vi.hoisted(() => vi.fn()); +// The route builds its narrowed foreground key from this session-scoped input. +const cliSessionGetQueryKeyMock = vi.hoisted(() => vi.fn(() => [['cliSessionsV2', 'get']])); const organizations = vi.hoisted(() => [ { organizationId: 'org-a', organizationName: 'Session organization' }, ]); @@ -168,8 +170,11 @@ vi.mock('@tanstack/react-query', async importOriginal => ({ })); // Foreground query refresh is separate from route parsing and provider lifetime. +const useRouteForegroundRefreshMock = vi.hoisted(() => + vi.fn<(queryKeys: readonly (readonly unknown[])[]) => void>() +); vi.mock('@/lib/hooks/use-route-foreground-refresh', () => ({ - useRouteForegroundRefresh: vi.fn(), + useRouteForegroundRefresh: useRouteForegroundRefreshMock, })); vi.mock('@/lib/auth/auth-context', () => ({ @@ -196,7 +201,7 @@ vi.mock('@/lib/trpc', () => ({ cliSessionsV2: { get: { queryOptions: queryOptionsMock, - queryKey: () => [['cliSessionsV2', 'get']], + queryKey: cliSessionGetQueryKeyMock, }, }, }), @@ -488,6 +493,8 @@ beforeEach(() => { ); useLocalSearchParamsMock.mockReset(); useRouterMock.mockReset(); + useRouteForegroundRefreshMock.mockClear(); + cliSessionGetQueryKeyMock.mockClear(); navigationRoutes.splice(0, navigationRoutes.length, 'session-detail'); useRouterMock.mockReturnValue({ canGoBack: () => navigationRoutes.length > 1, @@ -938,6 +945,23 @@ describe('SessionDetailScreen valid session-id', () => { }); }); +describe('SessionDetailScreen foreground refresh scope', () => { + it('invalidates only the opened session metadata and model preferences', async () => { + useLocalSearchParamsMock.mockReturnValue({ 'session-id': 'sess-1' }); + await mountRoute(); + + // The key is built from the opened session's own input, not a procedure-wide + // prefix and not the empty-id fallback. + expect(cliSessionGetQueryKeyMock).toHaveBeenCalledWith({ session_id: 'sess-1' }); + expect(useRouteForegroundRefreshMock).toHaveBeenCalled(); + const keys = useRouteForegroundRefreshMock.mock.calls.at(-1)?.[0]; + expect(keys).toEqual([[['cliSessionsV2', 'get']], [['modelPreferences']]]); + // The whole-procedure prefix would refetch every retained `cliSessionsV2` + // query — the stored `cliSessionsV2.list` pages behind this pushed route. + expect(keys).not.toContainEqual([['cliSessionsV2']]); + }); +}); + function transcriptPage(sessionId: KiloSessionId, messageId: string, text: string) { return { kind: 'success', diff --git a/apps/mobile/src/app/(app)/agent-chat/[session-id].tsx b/apps/mobile/src/app/(app)/agent-chat/[session-id].tsx index 63fb9af733d..bb8466064f9 100644 --- a/apps/mobile/src/app/(app)/agent-chat/[session-id].tsx +++ b/apps/mobile/src/app/(app)/agent-chat/[session-id].tsx @@ -98,7 +98,16 @@ export default function SessionDetailScreen() { if (openStart.current.sessionId !== rawSessionId) { openStart.current = { sessionId: rawSessionId, startedAt: Date.now() }; } - useRouteForegroundRefresh([[['cliSessionsV2']], [['modelPreferences']]]); + // Narrow the metadata invalidation to this session's own query key. The whole + // `cliSessionsV2` procedure prefix would also invalidate the stored + // `cliSessionsV2.list` pages behind this pushed route, so returning to the app + // (or refocusing the route) refetched every retained list page alongside the + // metadata. `SessionDetailContent` already refetches `getWithRuntimeState` on + // focus, and this key still refreshes the opened session's metadata. + useRouteForegroundRefresh([ + trpc.cliSessionsV2.get.queryKey({ session_id: sessionId ?? '' }), + [['modelPreferences']], + ]); const sessionQuery = useQuery({ ...trpc.cliSessionsV2.get.queryOptions( { session_id: sessionId ?? '' }, diff --git a/apps/mobile/src/app/(app)/app-layout-foreground.mounted.test.tsx b/apps/mobile/src/app/(app)/app-layout-foreground.mounted.test.tsx new file mode 100644 index 00000000000..649c9d2a36b --- /dev/null +++ b/apps/mobile/src/app/(app)/app-layout-foreground.mounted.test.tsx @@ -0,0 +1,268 @@ +import { createElement, type ReactNode } from 'react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import AppLayout from '@/app/(app)/_layout'; +import { attemptLogoutReconciliation } from '@/lib/auth/logout-reconciliation'; +import { + attemptPushRegistrationReconciliation, + subscribeToPushTokenRotation, +} from '@/lib/auth/push-registration-reconciliation'; +import { act, TestRenderer } from '@/test/renderer'; + +// AppState is the single emitter the shared `useAppLifecycle()` store wraps. +// The harness counts registrations so the assertion is "the whole (app) tree +// keeps one listener", and broadcasts to every listener so a second private +// subscription would be visible as a registration and as an extra pass. +const appState = vi.hoisted(() => { + const listeners = new Set<(state: string) => void>(); + return { + listeners, + registrationCount: 0, + currentState: 'active' as string, + addEventListener: (_event: string, listener: (state: string) => void) => { + appState.registrationCount += 1; + listeners.add(listener); + return { + remove: () => { + listeners.delete(listener); + }, + }; + }, + emit: (state: string) => { + appState.currentState = state; + for (const listener of listeners) { + listener(state); + } + }, + }; +}); + +// One ordered log across both reconciliations, so a pass is asserted as the +// logout -> push sequence and a leaked second pass is visible in the order. +const passes = vi.hoisted(() => ({ order: [] as string[] })); +const rotation = vi.hoisted(() => ({ unsubscribe: vi.fn() })); + +vi.mock('react-native', () => ({ + AppState: { + get currentState() { + return appState.currentState; + }, + addEventListener: appState.addEventListener, + }, +})); + +vi.mock('expo-router', async () => { + const { createElement: create } = await import('react'); + const Stack = ({ + children, + screenLayout, + screenOptions, + }: { + children?: ReactNode; + screenLayout?: unknown; + screenOptions?: unknown; + }) => create('Stack', { screenLayout, screenOptions }, children); + return { Stack: Object.assign(Stack, { Screen: 'StackScreen' }) }; +}); + +vi.mock('@/components/app-unlock-screen', () => ({ appUnlockScreenLayout: () => null })); +vi.mock('@/components/agents/user-web-connection-provider', () => ({ + UserWebConnectionProvider: 'UserWebConnectionProvider', +})); +vi.mock('@/components/kilo-chat/kilo-chat-provider', () => ({ + KiloChatProvider: 'KiloChatProvider', +})); +vi.mock('@/components/kilo-chat/kilo-chat-presence-mount', () => ({ + KiloChatPresenceMount: 'KiloChatPresenceMount', +})); +vi.mock('@/components/launcher-surfaces-mount', () => ({ LauncherSurfacesMount: () => null })); +vi.mock('@/components/share/share-payload-navigator', () => ({ + SharePayloadNavigator: 'SharePayloadNavigator', +})); +vi.mock('@/components/tour/tour-auto-open', () => ({ TourAutoOpen: 'TourAutoOpen' })); +vi.mock('@/lib/active-sessions-live-sync-mount', () => ({ + ActiveSessionsLiveSyncMount: 'ActiveSessionsLiveSyncMount', +})); +vi.mock('@/lib/artifacts/artifact-mirror-sync-mount', () => ({ + ArtifactMirrorSyncMount: 'ArtifactMirrorSyncMount', +})); +vi.mock('@/lib/glanceable/mount', () => ({ GlanceablePublisherMount: () => null })); +vi.mock('@/lib/glanceable/org-fence', () => ({ useGlanceableOrgFence: vi.fn() })); +vi.mock('@/lib/form-sheet', () => ({ useFormSheetScreenOptions: () => ({}) })); +vi.mock('@/lib/hooks/use-theme-colors', () => ({ + useThemeColors: () => ({ background: '#ffffff', foreground: '#000000' }), +})); +vi.mock('@/lib/hooks/use-current-user-id', () => ({ + useCurrentUserId: () => ({ + userId: 'user-1', + email: 'user@example.com', + isLoading: false, + isError: false, + }), +})); +vi.mock('@/lib/hooks/use-route-foreground-refresh', () => ({ + useRouteForegroundRefresh: vi.fn(), +})); +vi.mock('@/lib/hooks/use-security-lifecycle-invalidation', () => ({ + useSecurityLifecycleInvalidation: vi.fn(), +})); +vi.mock('@/lib/persist/cache-persistence-mount', () => ({ + CachePersistenceMount: 'CachePersistenceMount', +})); +vi.mock('@/lib/system-search-index-mount', () => ({ + SystemSearchIndexMount: 'SystemSearchIndexMount', +})); +vi.mock('@/lib/tool-summary-translation/tool-summary-translation-retry-mount', () => ({ + ToolSummaryTranslationRetryMount: 'ToolSummaryTranslationRetryMount', +})); +vi.mock('@/lib/trpc', () => ({ + useTRPC: () => ({ + user: { getMe: { queryKey: () => ['user', 'getMe'] } }, + organizations: { list: { queryKey: () => ['organizations', 'list'] } }, + }), +})); +vi.mock('@/lib/auth/logout-reconciliation', () => ({ + attemptLogoutReconciliation: vi.fn(() => { + passes.order.push('logout'); + return { kind: 'no-tombstone' }; + }), +})); +vi.mock('@/lib/auth/push-registration-reconciliation', () => ({ + attemptPushRegistrationReconciliation: vi.fn(() => { + passes.order.push('push'); + return { kind: 'already-registered' }; + }), + subscribeToPushTokenRotation: vi.fn(() => rotation.unsubscribe), +})); + +const mountedRenderers: TestRenderer.ReactTestRenderer[] = []; + +function mountAppLayout(): TestRenderer.ReactTestRenderer { + const rendererRef: { current: TestRenderer.ReactTestRenderer | undefined } = { + current: undefined, + }; + act(() => { + rendererRef.current = TestRenderer.create(createElement(AppLayout)); + }); + const renderer = rendererRef.current; + if (!renderer) { + throw new Error('renderer was not created'); + } + mountedRenderers.push(renderer); + return renderer; +} + +function unmountAppLayout(renderer: TestRenderer.ReactTestRenderer): void { + act(() => { + renderer.unmount(); + }); + const index = mountedRenderers.indexOf(renderer); + if (index !== -1) { + mountedRenderers.splice(index, 1); + } +} + +describe('AppLayout foreground reconciliation', () => { + beforeEach(() => { + vi.clearAllMocks(); + appState.listeners.clear(); + appState.registrationCount = 0; + appState.currentState = 'active'; + passes.order.length = 0; + }); + + afterEach(() => { + act(() => { + for (const renderer of mountedRenderers) { + renderer.unmount(); + } + }); + mountedRenderers.length = 0; + appState.listeners.clear(); + }); + + it('registers exactly one AppState listener for the whole tree', () => { + mountAppLayout(); + + expect(appState.registrationCount).toBe(1); + expect(appState.listeners.size).toBe(1); + }); + + it('runs each reconciliation once at mount, logout before push', () => { + mountAppLayout(); + + expect(passes.order).toEqual(['logout', 'push']); + expect(attemptLogoutReconciliation).toHaveBeenCalledTimes(1); + expect(attemptLogoutReconciliation).toHaveBeenCalledWith('user-1'); + expect(attemptPushRegistrationReconciliation).toHaveBeenCalledTimes(1); + expect(attemptPushRegistrationReconciliation).toHaveBeenCalledWith('user-1'); + }); + + it('runs each reconciliation exactly once on a background -> active edge', () => { + mountAppLayout(); + passes.order.length = 0; + + act(() => { + appState.emit('background'); + }); + expect(passes.order).toEqual([]); + + act(() => { + appState.emit('active'); + }); + expect(passes.order).toEqual(['logout', 'push']); + + // A repeated `active` while the app is already active is not a new edge: + // the shared store drops it before React sees a change. + act(() => { + appState.emit('active'); + }); + expect(passes.order).toEqual(['logout', 'push']); + + // A second full cycle is still one pass per edge. + act(() => { + appState.emit('background'); + }); + act(() => { + appState.emit('active'); + }); + expect(passes.order).toEqual(['logout', 'push', 'logout', 'push']); + }); + + it('does not run a pass on an active -> active echo', () => { + mountAppLayout(); + passes.order.length = 0; + // Drop the mount pass from the mock call history so the assertion is + // about the echo alone. + vi.mocked(attemptLogoutReconciliation).mockClear(); + vi.mocked(attemptPushRegistrationReconciliation).mockClear(); + + act(() => { + appState.emit('active'); + }); + + expect(passes.order).toEqual([]); + expect(attemptLogoutReconciliation).not.toHaveBeenCalled(); + expect(attemptPushRegistrationReconciliation).not.toHaveBeenCalled(); + expect(appState.listeners.size).toBe(1); + }); + + it('keeps one push-token rotation subscription and releases it on unmount', () => { + const renderer = mountAppLayout(); + + expect(subscribeToPushTokenRotation).toHaveBeenCalledWith('user-1'); + + act(() => { + appState.emit('background'); + }); + act(() => { + appState.emit('active'); + }); + + expect(subscribeToPushTokenRotation).toHaveBeenCalledTimes(1); + expect(rotation.unsubscribe).not.toHaveBeenCalled(); + + unmountAppLayout(renderer); + expect(rotation.unsubscribe).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/mobile/src/app/_layout.tsx b/apps/mobile/src/app/_layout.tsx index 1a139b46411..46e1fb0dc73 100644 --- a/apps/mobile/src/app/_layout.tsx +++ b/apps/mobile/src/app/_layout.tsx @@ -42,7 +42,7 @@ import { ShareIntentProvider, useShareIntentContext } from 'expo-share-intent'; import { StatusBar } from 'expo-status-bar'; import { useEffect, useRef, useState, useSyncExternalStore } from 'react'; import { useTranslation } from 'react-i18next'; -import { AppState, View } from 'react-native'; +import { View } from 'react-native'; import Animated, { useAnimatedStyle } from 'react-native-reanimated'; import { toast } from 'sonner-native'; @@ -69,6 +69,7 @@ import { markStartup, markStartupComplete } from '@/lib/startup-timing'; import { prefetchCurrentUser } from '@/lib/startup-prefetch'; import { useAnalyticsConsentGate } from '@/lib/hooks/use-analytics-consent-gate'; import { useForceUpdate } from '@/lib/hooks/use-force-update'; +import { useAppLifecycle } from '@/lib/hooks/use-app-lifecycle'; import { useCurrentUserId } from '@/lib/hooks/use-current-user-id'; import { useRestoreErrorHold } from '@/lib/hooks/use-restore-error-hold'; import { useScreenTracking } from '@/lib/hooks/use-screen-tracking'; @@ -1124,26 +1125,32 @@ function RootLayout() { // native listener instead of leaving it alive past the tree that uses it. useSystemSearchOpenListener(); - // Reap expired temp files at cold start and whenever the app returns to the - // foreground, deferred past the current interaction frame so a navigation - // never waits on it. AppState is already `active` at launch, so the change - // listener alone never reaps in a launch/use/kill cycle. + // Reap expired temp files at cold start, deferred past the current + // interaction frame so a navigation never waits on it. AppState is already + // `active` at launch, so the foreground edge alone never reaps in a + // launch/use/kill cycle. useEffect(() => { scheduleCacheMaintenance(() => { reapTempFiles(); }); - const subscription = AppState.addEventListener('change', nextState => { - if (nextState === 'active') { - scheduleCacheMaintenance(() => { - reapTempFiles(); - }); - } - }); - return () => { - subscription.remove(); - }; }, []); + // The foreground half of the same reap. The app's shared `useAppLifecycle()` + // store is the single foreground subscription for this work, so the edge + // fires only on background -> active and never on an active -> active echo. + // The reap is idempotent, so dropping the layout's own `AppState` listener + // for this loses nothing. + const { isActive } = useAppLifecycle(); + const wasActiveRef = useRef(isActive); + useEffect(() => { + if (!wasActiveRef.current && isActive) { + scheduleCacheMaintenance(() => { + reapTempFiles(); + }); + } + wasActiveRef.current = isActive; + }, [isActive]); + return ( diff --git a/apps/mobile/src/components/agents/session-list-content.mounted.test.tsx b/apps/mobile/src/components/agents/session-list-content.mounted.test.tsx index 74b215ebcc9..0b792ed974d 100644 --- a/apps/mobile/src/components/agents/session-list-content.mounted.test.tsx +++ b/apps/mobile/src/components/agents/session-list-content.mounted.test.tsx @@ -29,6 +29,8 @@ type ListProps = { }>; extraData: number; onEndReached: () => void; + onScrollBeginDrag: () => void; + onTouchMove: () => void; }; const controls = vi.hoisted(() => ({ scrollResets: 0, @@ -287,6 +289,87 @@ describe('AgentSessionListContent liveness', () => { } ); + it('parks pagination after a programmatic page-one reset and replays the end-reach on the user drag', () => { + const onEndReached = vi.fn<() => void>(); + const retained = Array.from({ length: 12 }, (_, index) => session(`row-${index}`)); + const pageOne = retained.slice(0, 4); + const props = contentProps({ + sections: [{ title: 'Today', data: retained }], + onEndReached, + }); + const renderer = mount(props); + // The foreground reconcile drops the retained pages, so the list shrinks + // under the viewport once the new page one lands and FlashList reports that + // shrink as an end-reach. Honoring it is the page-per-retained-page + // refetch device defect e2 measured. + act(() => { + renderer.update( + createElement(AgentSessionListContent, { + ...props, + sections: [{ title: 'Today', data: pageOne }], + }) + ); + }); + const shrunk = renderer.root.find(node => isHost(node, 'FlashList')); + act(() => { + (shrunk.props as ListProps).onEndReached(); + }); + expect(onEndReached).not.toHaveBeenCalled(); + // FlashList reports the end once per data change, so the drag the user makes + // to browse both lifts the park and replays the end-reach the park dropped. + act(() => { + (shrunk.props as ListProps).onScrollBeginDrag(); + }); + expect(onEndReached).toHaveBeenCalledTimes(1); + // The parked report is replayed once; later reports pass straight through. + act(() => { + (shrunk.props as ListProps).onEndReached(); + }); + expect(onEndReached).toHaveBeenCalledTimes(2); + }); + + it('releases the pagination park on touch move and replays the dropped end-reach when the list fits the viewport', () => { + const onEndReached = vi.fn<() => void>(); + const retained = Array.from({ length: 12 }, (_, index) => session(`row-${index}`)); + // The shrink leaves fewer rows than fill the viewport, so FlashList never + // emits `onScrollBeginDrag` (there is nothing to scroll) and reports the end + // once per data change: only the finger's own `onTouchMove` can both open + // the park and replay the end-reach it dropped. + const pageOne = retained.slice(0, 4); + const props = contentProps({ + sections: [{ title: 'Today', data: retained }], + onEndReached, + }); + const renderer = mount(props); + act(() => { + renderer.update( + createElement(AgentSessionListContent, { + ...props, + sections: [{ title: 'Today', data: pageOne }], + }) + ); + }); + const shrunk = renderer.root.find(node => isHost(node, 'FlashList')); + act(() => { + (shrunk.props as ListProps).onEndReached(); + }); + expect(onEndReached).not.toHaveBeenCalled(); + act(() => { + (shrunk.props as ListProps).onTouchMove(); + }); + expect(onEndReached).toHaveBeenCalledTimes(1); + // A further touch move replays nothing: the parked report was consumed. + act(() => { + (shrunk.props as ListProps).onTouchMove(); + }); + expect(onEndReached).toHaveBeenCalledTimes(1); + // The park is gone, so a later end-reach passes straight through. + act(() => { + (shrunk.props as ListProps).onEndReached(); + }); + expect(onEndReached).toHaveBeenCalledTimes(2); + }); + it.each([false, true])('preserves retry recovery for searching=%s', isSearching => { const recovered = contentProps({ sections: [{ title: 'Today', data: [session('recovered')] }], @@ -483,6 +566,27 @@ describe('AgentSessionListContent liveness', () => { } ); + it('reserves the clearance between the refresh band and the rows', () => { + // The band is in flow above the list, so the container's `gap-2` is the + // only clearance between them: without it the list's top edge sits flush + // on the band and a row resting across that edge is cut mid-text directly + // under "Couldn't refresh" (device defect p5). The clearance matches the + // live surface's `mx-4 gap-2` band-to-list gap. + const renderer = mount( + contentProps({ + isError: true, + sections: [{ title: 'Today', data: [session('cached')] }], + }) + ); + const surface = hosts(renderer, 'AnimatedView').find( + node => (node.props as { className?: string }).className === 'flex-1 gap-2' + ); + expect(surface).toBeDefined(); + // The band and the list are both children of that surface. + expect(surface?.findAll(node => isHost(node, 'AccessibleStatus'))).toHaveLength(1); + expect(surface?.findAll(node => isHost(node, 'FlashList'))).toHaveLength(1); + }); + it.each([ { hasAnySessions: false }, { hasAnySessions: false, isError: true }, diff --git a/apps/mobile/src/components/agents/session-list-content.tsx b/apps/mobile/src/components/agents/session-list-content.tsx index c8fb28ef661..f9ff3dec581 100644 --- a/apps/mobile/src/components/agents/session-list-content.tsx +++ b/apps/mobile/src/components/agents/session-list-content.tsx @@ -234,6 +234,53 @@ export function AgentSessionListContent({ [showLoadingSkeletons, sections] ); + // Pagination is user-driven. A programmatic page-one reset (the foreground, + // focus and pull reconciles) empties the retained pages, so the list shrinks + // under the viewport once the new page one lands, and FlashList reports that + // shrink as an end-reach. Honoring it re-fetched one page per retained page + // around a foreground transition instead of the single page the reconcile + // re-issued (device defect e2). A shrink therefore parks pagination until the + // next user drag; a query change that resets the list parks it too, and the + // drag the user makes to browse resumes it. + const paginationParkedRef = useRef(false); + const previousRowCountRef = useRef(rows.length); + useEffect(() => { + if (rows.length < previousRowCountRef.current) { + paginationParkedRef.current = true; + } + previousRowCountRef.current = rows.length; + }, [rows.length]); + // FlashList reports the end once per data change, so the end-reach the shrink + // produces is not followed by another one on its own. Hold it while parked and + // replay it when the user's movement lifts the park: a list whose rows fit the + // viewport never reaches the end again, so without the replay it would stay + // parked for good and never load older sessions. + const parkedEndReachRef = useRef(false); + const handleEndReached = useCallback(() => { + if (paginationParkedRef.current) { + parkedEndReachRef.current = true; + return; + } + onEndReached(); + }, [onEndReached]); + // The park must be released by the user's own movement, and `onScrollBeginDrag` + // only fires when the list can scroll: a shrink that leaves fewer rows than + // fill the viewport (a filter or search change, or a reconcile collapsing the + // list to page one) has nothing to scroll, so a drag never opened the park and + // `hasNextPage` stayed true behind it. `onTouchMove` is the finger's own + // movement, which a viewport-fitting list still reports; the PR-review diff + // file list wires the same two events for the same reason. + const releasePaginationPark = useCallback(() => { + if (!paginationParkedRef.current) { + return; + } + paginationParkedRef.current = false; + if (parkedEndReachRef.current) { + parkedEndReachRef.current = false; + onEndReached(); + } + }, [onEndReached]); + const renderItem = useCallback( ({ item }: ListRenderItemInfo) => { if (item.kind === 'skeleton') { @@ -350,8 +397,17 @@ export function AgentSessionListContent({ // 200ms fade reads as a blank list area on a fast cold open (the skeleton // phase would live entirely inside the fade). The skeleton → rows swap is // a same-pitch data update inside the mounted list. + // + // `gap-2` is the clearance between the status band and the rows — the same + // gap the live surface leaves below its band (`mx-4 gap-2` in + // `AgentSessionsSection`). Without it the list's top edge sits flush on the + // band, and a row resting across that edge is cut mid-text directly under + // "Couldn't refresh", so the two read as colliding (device defect p5). The + // gap lives on the container rather than as the band's padding on purpose: + // the clearance must not depend on whether the status line renders, or the + // failure line would push the rows down by the gap as it appears. return ( - + {/* The band's height is allocated whenever the rows show, so the in-flight spinner and the failure line replace empty space instead of pushing the rows down (device defect uxs1). */} @@ -379,8 +435,10 @@ export function AgentSessionListContent({ } contentContainerStyle={tabBarOnlyClearanceStyle} keyboardDismissMode="on-drag" - onEndReached={onEndReached} + onEndReached={handleEndReached} onEndReachedThreshold={0.5} + onScrollBeginDrag={releasePaginationPark} + onTouchMove={releasePaginationPark} refreshControl={rowsControl} maintainVisibleContentPosition={{ autoscrollToTopThreshold: 10 }} /> diff --git a/apps/mobile/src/components/kilo-pass/kilo-pass-subscription-card.mounted.test.tsx b/apps/mobile/src/components/kilo-pass/kilo-pass-subscription-card.mounted.test.tsx new file mode 100644 index 00000000000..a433cd3f57d --- /dev/null +++ b/apps/mobile/src/components/kilo-pass/kilo-pass-subscription-card.mounted.test.tsx @@ -0,0 +1,293 @@ +import { createElement } from 'react'; +import { act, TestRenderer } from '@/test/renderer'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { KiloPassSubscriptionCard } from '@/components/kilo-pass/kilo-pass-subscription-card'; + +// The card must not own a foreground listener: the profile route layout +// invalidates the `[['kiloPass']]` prefix through `useRouteForegroundRefresh`, +// which is focus-gated and de-duplicated by key. `addEventListener` is a spy so +// a reintroduced listener fails the test, and `emit` still drives any listener a +// future change registers, so a foreground refetch cannot slip through. +const appState = vi.hoisted(() => { + const listeners = new Set<(state: string) => void>(); + const addEventListener = vi.fn((_event: string, listener: (state: string) => void) => { + listeners.add(listener); + return { + remove: () => { + listeners.delete(listener); + }, + }; + }); + return { + listeners, + addEventListener, + emit: (state: string): void => { + for (const listener of listeners) { + listener(state); + } + }, + }; +}); + +const haptics = vi.hoisted(() => ({ selectionAsync: vi.fn() })); + +const devRefund = vi.hoisted(() => ({ value: null as string | null })); + +// Inert query results with refetch spies. `useQuery` dispatches on the marker +// the mocked tRPC `queryOptions` carries, so the card's three queries each get +// a stable shape regardless of hook-call order across renders. +const queryState = vi.hoisted(() => ({ + presentation: vi.fn(), + presentationData: undefined as unknown, + presentationIsError: false, + presentationIsPending: false, + state: vi.fn(), + stateData: undefined as unknown, + stateIsError: false, + stateIsPending: false, + productsData: undefined as unknown, + productsRefetch: vi.fn(), +})); + +const trpc = vi.hoisted(() => ({ + kiloPass: { + getCreditHistory: { pathFilter: () => ({ queryKey: [['kiloPass']] }) }, + getMobileStoreProducts: { + queryOptions: () => ({ __name: 'mobileStoreProducts' }), + }, + getPurchasePresentation: { queryOptions: () => ({ __name: 'presentation' }) }, + getState: { + pathFilter: () => ({ queryKey: [['kiloPass']] }), + queryOptions: () => ({ __name: 'state' }), + }, + }, + user: { + getContextBalance: { pathFilter: () => ({ queryKey: [['user']] }) }, + getCreditBlocks: { pathFilter: () => ({ queryKey: [['user']] }) }, + }, +})); + +vi.mock('react-native', () => ({ + AppState: { addEventListener: appState.addEventListener }, + Linking: { openURL: vi.fn() }, + Platform: { OS: 'ios' }, + Pressable: 'Pressable', + View: 'View', + // The card reads the window width to pick its stacked narrow presentation + // (`narrow-layout.ts`). 390 dp keeps the standard side-by-side row, the same + // width the narrow-layout test uses for its non-narrow cases. + useWindowDimensions: () => ({ width: 390, height: 844, fontScale: 1, scale: 2 }), +})); + +vi.mock('expo-router', () => ({ + useRouter: () => ({ push: vi.fn() }), +})); + +vi.mock('expo-haptics', () => ({ selectionAsync: haptics.selectionAsync })); + +vi.mock('react-i18next', () => ({ + useTranslation: () => ({ t: (key: string) => key }), +})); + +vi.mock('@/i18n', () => ({ + i18n: { language: 'en', t: (key: string) => key }, +})); + +vi.mock('@/lib/hooks/use-language-preference', () => ({ + getResolvedLanguage: () => 'en', +})); + +vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); +vi.mock('@/components/kilo-pass/kilo-pass-icon', () => ({ KiloPassIcon: 'KiloPassIcon' })); +vi.mock('@/components/ui/skeleton', () => ({ Skeleton: 'Skeleton' })); + +vi.mock('@/lib/hooks/use-theme-colors', () => ({ + useThemeColors: () => ({ primary: '#000000' }), +})); + +vi.mock('@/lib/trpc', () => ({ useTRPC: () => trpc })); + +vi.mock('@/lib/kilo-pass/dev-storekit-refund', () => ({ + getDevStoreKitRefundAppleProductId: () => devRefund.value, +})); + +vi.mock('@tanstack/react-query', () => ({ + useQuery: (options: { __name?: string }) => { + if (options.__name === 'presentation') { + return { + data: queryState.presentationData, + isError: queryState.presentationIsError, + isPending: queryState.presentationIsPending, + refetch: queryState.presentation, + }; + } + if (options.__name === 'state') { + return { + data: queryState.stateData, + isError: queryState.stateIsError, + isPending: queryState.stateIsPending, + refetch: queryState.state, + }; + } + return { + data: queryState.productsData, + isError: false, + isPending: false, + refetch: queryState.productsRefetch, + }; + }, + useQueryClient: () => ({ invalidateQueries: vi.fn() }), +})); + +const mountedRenderers: TestRenderer.ReactTestRenderer[] = []; + +async function renderCard(): Promise { + const ref: { current: TestRenderer.ReactTestRenderer | undefined } = { current: undefined }; + await act(async () => { + await Promise.resolve(); + ref.current = TestRenderer.create(createElement(KiloPassSubscriptionCard)); + }); + const renderer = ref.current; + if (!renderer) { + throw new Error('renderer was not created'); + } + mountedRenderers.push(renderer); + return renderer; +} + +async function flush(): Promise { + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); +} + +function collectText(value: unknown, out: string[] = []): string[] { + if (typeof value === 'string') { + out.push(value); + return out; + } + if (Array.isArray(value)) { + for (const item of value) { + collectText(item, out); + } + return out; + } + if (value && typeof value === 'object') { + const children = (value as { children?: unknown[] }).children; + if (Array.isArray(children)) { + collectText(children, out); + } + } + return out; +} + +describe('KiloPassSubscriptionCard mounted', () => { + beforeEach(() => { + (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + vi.stubGlobal('__DEV__', false); + appState.addEventListener.mockClear(); + appState.listeners.clear(); + haptics.selectionAsync.mockClear(); + devRefund.value = null; + queryState.presentation.mockClear(); + queryState.state.mockClear(); + queryState.productsRefetch.mockClear(); + queryState.presentationData = undefined; + queryState.presentationIsError = false; + queryState.presentationIsPending = false; + queryState.stateData = undefined; + queryState.stateIsError = false; + queryState.stateIsPending = false; + queryState.productsData = undefined; + }); + + afterEach(() => { + act(() => { + for (const renderer of mountedRenderers) { + renderer.unmount(); + } + }); + mountedRenderers.length = 0; + appState.listeners.clear(); + vi.unstubAllGlobals(); + }); + + it('never registers an AppState listener while rendering', async () => { + queryState.presentationIsPending = true; + queryState.stateIsPending = true; + + const renderer = await renderCard(); + + expect(appState.addEventListener).not.toHaveBeenCalled(); + // The loading state still renders its single skeleton slot. + expect( + renderer.root.findAllByProps({ accessibilityLabel: 'kiloPass.subscriptionLoading' }) + ).toHaveLength(1); + }); + + it('an active foreground transition triggers no refetch on either query', async () => { + queryState.presentationData = { kind: 'unavailable' }; + queryState.stateData = { subscription: null }; + + await renderCard(); + + expect(appState.addEventListener).not.toHaveBeenCalled(); + expect(appState.listeners.size).toBe(0); + + act(() => { + appState.emit('background'); + appState.emit('active'); + }); + await flush(); + + expect(queryState.presentation).not.toHaveBeenCalled(); + expect(queryState.state).not.toHaveBeenCalled(); + }); + + it('renders the error state and keeps Retry reachable', async () => { + queryState.presentationIsError = true; + + const renderer = await renderCard(); + const retry = renderer.root.findByProps({ accessibilityHint: 'kiloPass.retryHint' }); + const onRetryPress = retry.props.onPress as () => void; + + act(() => { + onRetryPress(); + }); + + expect(haptics.selectionAsync).toHaveBeenCalledTimes(1); + expect(queryState.state).toHaveBeenCalledTimes(1); + expect(queryState.presentation).toHaveBeenCalledTimes(1); + }); + + it('renders the card presentation state', async () => { + queryState.presentationData = { kind: 'unavailable' }; + queryState.stateData = { subscription: null }; + + const renderer = await renderCard(); + + expect(renderer.root.findAllByProps({ testID: 'kilo-pass-unavailable-card' })).toHaveLength(1); + }); + + it('hides the dev-refund control when no refundable product is present', async () => { + queryState.presentationData = { kind: 'unavailable' }; + queryState.stateData = { subscription: null }; + devRefund.value = null; + + const renderer = await renderCard(); + + expect(collectText(renderer.toJSON())).not.toContain('kiloPass.devRefund'); + }); + + it('shows the dev-refund control when a refundable product is present', async () => { + queryState.presentationData = { kind: 'unavailable' }; + queryState.stateData = { subscription: null }; + devRefund.value = 'kilo_pass_apple_monthly'; + + const renderer = await renderCard(); + + expect(collectText(renderer.toJSON())).toContain('kiloPass.devRefund'); + }); +}); diff --git a/apps/mobile/src/components/kilo-pass/kilo-pass-subscription-card.tsx b/apps/mobile/src/components/kilo-pass/kilo-pass-subscription-card.tsx index 6229c0121dd..0e1c647db74 100644 --- a/apps/mobile/src/components/kilo-pass/kilo-pass-subscription-card.tsx +++ b/apps/mobile/src/components/kilo-pass/kilo-pass-subscription-card.tsx @@ -1,7 +1,7 @@ import { type Href, useRouter } from 'expo-router'; import * as Haptics from 'expo-haptics'; -import { type ReactNode, useEffect, useRef } from 'react'; -import { AppState, Linking, Platform, Pressable, useWindowDimensions, View } from 'react-native'; +import { type ReactNode } from 'react'; +import { Linking, Platform, Pressable, useWindowDimensions, View } from 'react-native'; import { useTranslation } from 'react-i18next'; import { useQuery, useQueryClient } from '@tanstack/react-query'; @@ -82,30 +82,6 @@ export function KiloPassSubscriptionCard({ ]); }; - // Returning to the app may follow a store-management trip (App Store or Play); - // refetch both the presentation and state so the card reflects any change. - const refetchRef = useRef({ - presentation: presentationQuery.refetch, - state: stateQuery.refetch, - }); - useEffect(() => { - refetchRef.current = { - presentation: presentationQuery.refetch, - state: stateQuery.refetch, - }; - }, [presentationQuery.refetch, stateQuery.refetch]); - useEffect(() => { - const appStateSubscription = AppState.addEventListener('change', state => { - if (state === 'active') { - void refetchRef.current.presentation(); - void refetchRef.current.state(); - } - }); - return () => { - appStateSubscription.remove(); - }; - }, []); - const handlePress = () => { if (contentState.kind !== 'card') { return; diff --git a/apps/mobile/src/lib/artifacts/artifact-mirror-sync-mount.mounted.test.tsx b/apps/mobile/src/lib/artifacts/artifact-mirror-sync-mount.mounted.test.tsx index 9ed36b3d424..f63dffb5c04 100644 --- a/apps/mobile/src/lib/artifacts/artifact-mirror-sync-mount.mounted.test.tsx +++ b/apps/mobile/src/lib/artifacts/artifact-mirror-sync-mount.mounted.test.tsx @@ -1,7 +1,10 @@ import { createElement } from 'react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { ArtifactMirrorSyncMount } from '@/lib/artifacts/artifact-mirror-sync-mount'; +import { + ArtifactMirrorSyncMount, + MIRROR_BURST_SETTLE_MS, +} from '@/lib/artifacts/artifact-mirror-sync-mount'; import { act, TestRenderer } from '@/test/renderer'; // The mount reads three process-wide inputs: the auth session, the resolved @@ -82,6 +85,13 @@ async function mount(): Promise { ref.current = TestRenderer.create(createElement(ArtifactMirrorSyncMount)); await Promise.resolve(); }); + // The mount's first run waits for the launch burst to settle + // (`MIRROR_BURST_SETTLE_MS`); advancing the clock past that edge is what + // a launch does once the tree is up and its own requests have gone out. + await act(async () => { + vi.advanceTimersByTime(MIRROR_BURST_SETTLE_MS); + await Promise.resolve(); + }); const renderer = ref.current; if (!renderer) { throw new Error('ArtifactMirrorSyncMount did not mount'); @@ -97,6 +107,7 @@ function unmountAll(): void { beforeEach(() => { vi.clearAllMocks(); + vi.useFakeTimers(); clock.now = MOUNT_TIME; vi.spyOn(Date, 'now').mockImplementation(() => clock.now); appState.reset(); @@ -108,6 +119,7 @@ afterEach(() => { unmountAll(); appState.reset(); vi.restoreAllMocks(); + vi.useRealTimers(); }); describe('ArtifactMirrorSyncMount', () => { @@ -137,7 +149,7 @@ describe('ArtifactMirrorSyncMount', () => { expect(mocks.registerArtifactsProviderDomain).not.toHaveBeenCalled(); }); - it('re-runs on a foreground regain only after the interval', async () => { + it('re-runs on a foreground regain only after the interval, past the burst', async () => { await mount(); appState.emit('background'); @@ -150,11 +162,37 @@ describe('ArtifactMirrorSyncMount', () => { clock.now = MOUNT_TIME + mocks.MIRROR_SYNC_MIN_INTERVAL_MS; appState.emit('active'); + // The transition is accepted, but the run waits out the transition's own + // request burst so the mirror's session-list read does not join the app's + // foreground refresh (device check p2). + expect(mocks.syncArtifactMirror).toHaveBeenCalledTimes(1); + + await act(async () => { + vi.advanceTimersByTime(MIRROR_BURST_SETTLE_MS); + await Promise.resolve(); + }); expect(mocks.syncArtifactMirror).toHaveBeenCalledTimes(2); // Not forced: the engine's persisted `lastRunAt` stays the authority. expect(mocks.syncArtifactMirror).toHaveBeenLastCalledWith(); }); + it('drops a scheduled foreground run when the app leaves the foreground', async () => { + await mount(); + + clock.now = MOUNT_TIME + mocks.MIRROR_SYNC_MIN_INTERVAL_MS; + appState.emit('active'); + appState.emit('background'); + + await act(async () => { + vi.advanceTimersByTime(MIRROR_BURST_SETTLE_MS); + await Promise.resolve(); + }); + + // The trigger is foreground-only: a run scheduled by a regain that ended + // before the burst settled never starts. + expect(mocks.syncArtifactMirror).toHaveBeenCalledTimes(1); + }); + it('stops listening on unmount', async () => { await mount(); expect(appState.count()).toBe(1); diff --git a/apps/mobile/src/lib/artifacts/artifact-mirror-sync-mount.tsx b/apps/mobile/src/lib/artifacts/artifact-mirror-sync-mount.tsx index b6ea18cb0dd..3f3bf87d483 100644 --- a/apps/mobile/src/lib/artifacts/artifact-mirror-sync-mount.tsx +++ b/apps/mobile/src/lib/artifacts/artifact-mirror-sync-mount.tsx @@ -1,4 +1,4 @@ -import { useEffect, useMemo, useRef } from 'react'; +import { useEffect, useMemo, useRef, useState } from 'react'; import { AppState } from 'react-native'; import { @@ -10,17 +10,53 @@ import { useAuth } from '@/lib/auth/auth-context'; import { isSignOutActive } from '@/lib/auth/sign-out-state'; import { useCurrentUserId } from '@/lib/hooks/use-current-user-id'; +/** + * How long the mirror waits after the app's own request burst starts. + * + * The mirror is derived data with no progress UI and nothing the user sees + * waits on it, so its `cliSessionsV2.list` read must not join the burst of + * requests the app issues on the same transition: + * + * - the launch burst, which is what paints the first frame (device check e12 + * counts the requests before the first frame); + * - the foreground-regain burst, where the stored history's page-one reconcile + * issues its own `cliSessionsV2.list` in the first moments after `AppState` + * goes `active` (device check p2 counts the requests around the transition). + * + * One interaction frame is not enough for either: the burst runs for seconds + * after the tree mounts or regains the foreground. The run still happens once + * per signed-in context and once per accepted foreground transition, just after + * the burst has settled. + */ +export const MIRROR_BURST_SETTLE_MS = 5000; + +/** True once the launch's own request burst has settled. */ +function useAfterLaunchSettle(): boolean { + const [settled, setSettled] = useState(false); + useEffect(() => { + const timer = setTimeout(() => { + setSettled(true); + }, MIRROR_BURST_SETTLE_MS); + return () => { + clearTimeout(timer); + }; + }, []); + return settled; +} + /** * Keeps the artifact mirror in step with the signed-in user while the app runs. * * The mirror has no progress UI, so every trigger is fire-and-forget and lands * on the engine's own single-flight and minimum-interval gates: * - * - once per signed-in context (forced), so a cold start mirrors promptly and - * the iOS File Provider domain is registered; + * - once per signed-in context (forced), deferred past the launch's own request + * burst so the burst and the first frame it paints do not carry the mirror's + * session-list read, and the iOS File Provider domain is registered; * - on each `AppState` `active` transition the {@link shouldSyncOnForeground} * policy accepts, so a return to the foreground after the interval picks up - * what changed while the app was away. + * what changed while the app was away. The run waits out the same transition's + * own request burst ({@link MIRROR_BURST_SETTLE_MS}) for the same reason. * * A token alone is not enough to start: the engine resolves the user from the * `user.getMe` cache, so the first run waits for `useCurrentUserId` instead of @@ -84,7 +120,18 @@ function useArtifactMirrorSync(): void { const currentScope = useRef(scope); currentScope.current = scope; + // The first run waits for the launch's request burst to settle. The mirror is + // derived data with no progress UI, so the burst — and the first frame it + // paints — must not carry its session-list read; the cold start otherwise + // issued a `cliSessionsV2.list` request before the app had painted. The + // foreground listener is registered on the same settled edge, because it only + // reacts to a later transition. + const launchSettled = useAfterLaunchSettle(); + useEffect(() => { + if (!launchSettled) { + return undefined; + } if (!scope.signedIn || scope.signingOut) { return undefined; } @@ -96,7 +143,20 @@ function useArtifactMirrorSync(): void { lastRunAt.current = Date.now(); void syncArtifactMirror({ force: true }); + // The transition is accepted here, but the run itself waits out the + // foreground-regain burst: the stored history's page-one reconcile issues + // its own `cliSessionsV2.list` right after `active`, and the mirror's read + // must not join it (device check p2 counts the requests around the + // transition). The timer is cancelled when the app leaves the foreground + // or the mount goes away, so this stays a foreground-only trigger, and the + // run claims `lastRunAt` only when it actually fires — a cancelled + // transition is not a run. + let foregroundRun: ReturnType | null = null; const subscription = AppState.addEventListener('change', nextState => { + if (foregroundRun !== null) { + clearTimeout(foregroundRun); + foregroundRun = null; + } if (nextState !== 'active') { return; } @@ -110,15 +170,25 @@ function useArtifactMirrorSync(): void { ) { return; } - lastRunAt.current = now; - // Not forced: the engine's persisted `lastRunAt` is the authority. - void syncArtifactMirror(); + foregroundRun = setTimeout(() => { + foregroundRun = null; + const live = currentScope.current; + if (!live.signedIn || live.signingOut) { + return; + } + lastRunAt.current = Date.now(); + // Not forced: the engine's persisted `lastRunAt` is the authority. + void syncArtifactMirror(); + }, MIRROR_BURST_SETTLE_MS); }); return () => { subscription.remove(); + if (foregroundRun !== null) { + clearTimeout(foregroundRun); + } }; - }, [scope, authEpoch]); + }, [launchSettled, scope, authEpoch]); } export function ArtifactMirrorSyncMount(): null { diff --git a/apps/mobile/src/lib/hooks/use-agent-sessions.combined.mounted.test.tsx b/apps/mobile/src/lib/hooks/use-agent-sessions.combined.mounted.test.tsx index de83de2dc56..e9906e6e9c4 100644 --- a/apps/mobile/src/lib/hooks/use-agent-sessions.combined.mounted.test.tsx +++ b/apps/mobile/src/lib/hooks/use-agent-sessions.combined.mounted.test.tsx @@ -1,4 +1,5 @@ /* eslint-disable max-lines -- DOM-free React Native hook integration; the combined/live refresh matrix exceeds the default line limit */ +import { type AppStateStatus } from 'react-native'; import { act } from '@/test/renderer'; import { onlineManager } from '@tanstack/react-query'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -21,9 +22,18 @@ import { useAgentSessions, useLiveAgentSessions } from '@/lib/hooks/use-agent-se const state = vi.hoisted(() => ({ organizationId: null as string | null, + refetchOnWindowFocus: undefined as boolean | undefined, active: vi.fn(), stored: vi.fn(), maintenance: [] as (() => void)[], + // The shared `useAppLifecycle` store registers here; tests read the handler + // back to fire a foreground edge. + appStateAddEventListener: vi.fn((_event: string, _listener: (next: AppStateStatus) => void) => ({ + remove: vi.fn(), + })), + // Every stored-row set the mounted hook rendered, so a test can prove the + // list never blanked across a page-one reset. + storedTitles: [] as (string | null)[][], })); const STORED_KEY = [['cliSessionsV2', 'list'], { type: 'infinite' }] as const; vi.mock('@/lib/auth/auth-context', () => ({ @@ -73,6 +83,7 @@ vi.mock('react-native', () => ({ state.maintenance.push(run); }, }, + AppState: { currentState: 'active', addEventListener: state.appStateAddEventListener }, })); vi.mock('@/components/agents/user-web-connection-provider', () => ({ useUserWebConnection: vi.fn(), @@ -94,11 +105,23 @@ function history(title: string) { return { cliSessions: [stored(title)], nextCursor: null }; } function Probe() { - latest = useAgentSessions({ organizationId: state.organizationId }); + latest = useAgentSessions({ + organizationId: state.organizationId, + refetchOnWindowFocus: state.refetchOnWindowFocus, + }); live = useLiveAgentSessions({ organizationId: state.organizationId }); + state.storedTitles.push(latest.storedSessions.map(row => row.title)); return null; } const { render, unmount } = createQueryProbe(Probe, () => client); +/** The app-foreground listener the shared `useAppLifecycle` store registered. */ +function latestAppStateListener(): (next: AppStateStatus) => void { + const handler = state.appStateAddEventListener.mock.calls.at(-1)?.[1]; + if (!handler) { + throw new Error('AppState listener was not registered'); + } + return handler; +} function combined() { if (!latest) { throw new Error('Missing combined hook'); @@ -118,7 +141,10 @@ beforeEach(() => { (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; setSignOutActive(false); state.organizationId = null; + state.refetchOnWindowFocus = undefined; state.maintenance.length = 0; + state.storedTitles.length = 0; + state.appStateAddEventListener.mockClear(); state.active.mockReset().mockResolvedValue({ sessions: [makeCached({ id: 'fresh', createdOnPlatform: 'cli', organizationId: null })], }); @@ -257,7 +283,12 @@ describe('combined and live refresh callers', () => { await flush(); }); expect(state.stored.mock.calls).toHaveLength(2); - expect(combined().storedSessions.map(row => row.title)).toEqual(['Old history', 'Next page']); + // The queued refetch is a page-one reconcile, so it resets the cache as + // soon as the next page settles. The list holds the rows it had already + // painted (page two was never rendered before the reset) and refetches + // page one from `initialPageParam`. + expect(state.stored.mock.calls[1]?.[0]).toMatchObject({ pageParam: null }); + expect(combined().storedSessions.map(row => row.title)).toEqual(['Old history']); await act(async () => { firstPage.resolve(history('New history')); await refresh; @@ -321,4 +352,215 @@ describe('combined and live refresh callers', () => { } } ); + + it('reconciles page one on the app-foreground edge without refetching retained pages', async () => { + // Three retained pages: React Query's native focus refetch would re-request + // all of them, the foreground reconcile must request only page one. + client.setQueryData(STORED_KEY, { + pages: [ + { cliSessions: [stored('Page one', 'p1')], nextCursor: 'c1' }, + { cliSessions: [stored('Page two', 'p2')], nextCursor: 'c2' }, + { cliSessions: [stored('Page three', 'p3')], nextCursor: null }, + ], + pageParams: [null, 'c1', 'c2'], + }); + await render(); + expect(combined().storedSessions.map(row => row.title)).toEqual([ + 'Page one', + 'Page two', + 'Page three', + ]); + + const listener = latestAppStateListener(); + await act(async () => { + listener('background'); + await flush(); + }); + // Backgrounding alone is not a foreground edge: no reconcile. + expect(state.stored.mock.calls).toHaveLength(0); + + state.stored.mockClear(); + const page = deferred>(); + state.stored.mockReturnValueOnce(page.promise); + await act(async () => { + listener('active'); + await flush(); + }); + + // Exactly one `cliSessionsV2.list` request, from `initialPageParam`. + expect(state.stored.mock.calls).toHaveLength(1); + expect(state.stored.mock.calls[0]?.[0]).toMatchObject({ pageParam: null }); + // The cached pages were reset... + expect(client.getQueryData(STORED_KEY)).toEqual({ pages: [], pageParams: [] }); + // ...but the rows already painted were held, never blanked, across the reset. + expect(combined().storedSessions.map(row => row.title)).toEqual([ + 'Page one', + 'Page two', + 'Page three', + ]); + expect(state.storedTitles.every(titles => titles.length > 0)).toBe(true); + + await act(async () => { + page.resolve(history('Refreshed')); + await flush(); + }); + expect(combined().storedSessions.map(row => row.title)).toEqual(['Refreshed']); + expect(client.getQueryData(STORED_KEY)).toEqual({ + pages: [history('Refreshed')], + pageParams: [null], + }); + }); + + it('does not reconcile on the foreground edge when the caller drives foreground itself', async () => { + state.refetchOnWindowFocus = false; + await render(); + const listener = latestAppStateListener(); + await act(async () => { + listener('background'); + await flush(); + }); + state.stored.mockClear(); + await act(async () => { + listener('active'); + await flush(); + }); + expect(state.stored.mock.calls).toHaveLength(0); + expect(combined().storedSessions.map(row => row.title)).toEqual(['Old history']); + }); + + it('holds the painted rows when a foreground reconcile is paused offline', async () => { + // The reconcile empties the cache before refetching page one. Offline the + // refetch is paused, not fetching, so the hold must cover the unresolved + // refetch too: the list has to keep the rows the user was reading instead + // of blanking into its empty state until the network returns. + client.setQueryData(STORED_KEY, { + pages: [ + { cliSessions: [stored('Page one', 'p1')], nextCursor: 'c1' }, + { cliSessions: [stored('Page two', 'p2')], nextCursor: 'c2' }, + { cliSessions: [stored('Page three', 'p3')], nextCursor: null }, + ], + pageParams: [null, 'c1', 'c2'], + }); + await render(); + const listener = latestAppStateListener(); + await act(async () => { + listener('background'); + await flush(); + }); + + onlineManager.setOnline(false); + state.stored.mockClear(); + await act(async () => { + listener('active'); + await flush(); + }); + + // No request ran: the page-one fetch is paused, not fetching. + expect(state.stored.mock.calls).toHaveLength(0); + expect(client.getQueryData(STORED_KEY)).toEqual({ pages: [], pageParams: [] }); + expect(combined().storedIsPaused).toBe(true); + // The reset did not blank the list: the painted rows are still rendered. + expect(combined().storedSessions.map(row => row.title)).toEqual([ + 'Page one', + 'Page two', + 'Page three', + ]); + expect(state.storedTitles.every(titles => titles.length > 0)).toBe(true); + + // Recovering the network refetches page one and releases the hold. + const page = deferred>(); + state.stored.mockReturnValueOnce(page.promise); + await act(async () => { + onlineManager.setOnline(true); + await flush(); + }); + expect(combined().storedSessions.map(row => row.title)).toEqual([ + 'Page one', + 'Page two', + 'Page three', + ]); + await act(async () => { + page.resolve(history('Reconnected')); + await flush(); + }); + expect(combined().storedSessions.map(row => row.title)).toEqual(['Reconnected']); + }); + + it('holds the painted rows when a foreground reconcile fails', async () => { + // A failed page-one refetch leaves the cache empty and the query errored. + // The hold must survive it so the screen can show the inline error over the + // rows the user was reading instead of blanking into its full-screen error. + client.setQueryData(STORED_KEY, { + pages: [ + { cliSessions: [stored('Page one', 'p1')], nextCursor: 'c1' }, + { cliSessions: [stored('Page two', 'p2')], nextCursor: 'c2' }, + { cliSessions: [stored('Page three', 'p3')], nextCursor: null }, + ], + pageParams: [null, 'c1', 'c2'], + }); + await render(); + const listener = latestAppStateListener(); + await act(async () => { + listener('background'); + await flush(); + }); + + state.stored.mockClear(); + state.stored.mockRejectedValueOnce(new Error('network')); + await act(async () => { + listener('active'); + await flush(); + }); + + expect(state.stored.mock.calls).toHaveLength(1); + expect(combined().storedIsError).toBe(true); + expect(combined().storedSessions.map(row => row.title)).toEqual([ + 'Page one', + 'Page two', + 'Page three', + ]); + expect(state.storedTitles.every(titles => titles.length > 0)).toBe(true); + }); + + it('does not report rows cached by an earlier mount as delivered when the refetch fails', async () => { + // The focus refetch routes through `storedReconcile`, whose + // `reconcileFirstPage` reset is a cache write that advances `dataUpdatedAt` + // without delivering. The contract must still hold: rows only cached by an + // earlier mount are not "fetched since mount", so a failed fresh open shows + // the retryable full-screen error instead of stale rows as loaded. + client.setQueryData( + STORED_KEY, + { pages: [history('Old history')], pageParams: [null] }, + { updatedAt: Date.now() - 60_000 } + ); + await render(); + expect(combined().storedFetchedSinceMount).toBe(false); + + state.stored.mockClear(); + state.stored.mockRejectedValueOnce(new Error('offline')); + await act(async () => { + await combined().refetch(); + await flush(); + }); + + expect(combined().storedIsError).toBe(true); + expect(combined().storedFetchedSinceMount).toBe(false); + }); + + it('reports rows the refetch delivered this mount as fresh', async () => { + client.setQueryData( + STORED_KEY, + { pages: [history('Old history')], pageParams: [null] }, + { updatedAt: Date.now() - 60_000 } + ); + await render(); + expect(combined().storedFetchedSinceMount).toBe(false); + + await act(async () => { + await combined().refetch(); + await flush(); + }); + + expect(combined().storedFetchedSinceMount).toBe(true); + }); }); diff --git a/apps/mobile/src/lib/hooks/use-agent-sessions.test.ts b/apps/mobile/src/lib/hooks/use-agent-sessions.test.ts index 95f700e3a0e..31780583285 100644 --- a/apps/mobile/src/lib/hooks/use-agent-sessions.test.ts +++ b/apps/mobile/src/lib/hooks/use-agent-sessions.test.ts @@ -34,6 +34,7 @@ vi.mock('@/lib/active-sessions-live-sync', () => ({ vi.mock('react-native', () => ({ InteractionManager: { runAfterInteractions: vi.fn() }, + AppState: { currentState: 'active', addEventListener: vi.fn(() => ({ remove: vi.fn() })) }, })); function createTrpcStub(infiniteQueryOptions: unknown) { @@ -184,23 +185,28 @@ describe('buildAgentSessionSearchInput', () => { }); describe('buildStoredSessionsQueryOptions', () => { - it('keeps the native window-focus refetch by default (Home and Share Gate)', () => { + it('leaves the native window-focus refetch off by default so the hook owns the foreground reconcile', () => { const infiniteQueryOptions = vi.fn((_input: unknown, options: object) => options); const result = buildStoredSessionsQueryOptions(createTrpcStub(infiniteQueryOptions), {}); + // The hook registers its own `useAppLifecycle` foreground edge and + // reconciles page one; React Query's native window-focus refetch would + // instead re-request every retained page. expect(infiniteQueryOptions).toHaveBeenCalledWith( expect.objectContaining({ limit: 30 }), - expect.objectContaining({ refetchOnWindowFocus: true }) + expect.objectContaining({ refetchOnWindowFocus: false }) ); - expect(result.refetchOnWindowFocus).toBe(true); + expect(result.refetchOnWindowFocus).toBe(false); }); - it('disables the native window-focus refetch only for the Agents list configuration', () => { + it('keeps the native window-focus refetch off when the caller opts out of the hook edge', () => { const infiniteQueryOptions = vi.fn((_input: unknown, options: object) => options); const result = buildStoredSessionsQueryOptions(createTrpcStub(infiniteQueryOptions), { refetchOnWindowFocus: false, }); + // The Agents list drives foreground through its own callback, so the + // native fan-out stays off there too. expect(infiniteQueryOptions).toHaveBeenCalledWith( expect.objectContaining({ limit: 30 }), expect.objectContaining({ refetchOnWindowFocus: false }) diff --git a/apps/mobile/src/lib/hooks/use-agent-sessions.ts b/apps/mobile/src/lib/hooks/use-agent-sessions.ts index 667452e1efa..da7e4136414 100644 --- a/apps/mobile/src/lib/hooks/use-agent-sessions.ts +++ b/apps/mobile/src/lib/hooks/use-agent-sessions.ts @@ -36,6 +36,7 @@ import { DEFAULT_AGENT_SESSION_SORT, parseAgentSessionSortBy, } from '@/lib/agent-session-sort'; +import { useAppLifecycle } from '@/lib/hooks/use-app-lifecycle'; import { useLiveSessionsHold } from '@/lib/hooks/use-live-sessions-hold'; import { reconcileFirstPage, withInfiniteRetention } from '@/lib/query/infinite-retention'; import { scheduleCacheMaintenance } from '@/lib/query/schedule-cache-maintenance'; @@ -60,13 +61,18 @@ export type UseAgentSessionsOptions = { */ sortBy?: AgentSessionSortBy; /** - * Native window-focus (OS app foreground) refetch for the stored-sessions - * query. Defaults to React Query's native behavior (`true`) so Home and - * the Share Gate keep their foreground refresh. The Agents list passes - * `false`: its screen drives app-foreground refresh through an AppState - * callback that runs the wrapped `refetch` behind the shared operation - * coordinator, so the native query lifecycle must not start a stored - * refetch that bypasses that queue (see `buildStoredSessionsQueryOptions`). + * Foreground reconcile ownership for the stored-sessions query. + * + * The hook owns the OS app-foreground refresh: on the shared + * `useAppLifecycle` false -> true edge it reconciles page one (resets the + * cached pages and refetches from `initialPageParam`) instead of letting + * React Query's native window-focus refetch fan out over every retained + * page. That edge is registered unless the caller passes `false`; the native + * query option stays off by default for the same reason. + * + * The Agents list passes `false`: its screen drives app-foreground refresh + * through its own AppState callback that runs the wrapped `refetch`, so the + * hook must not reconcile a second time. */ refetchOnWindowFocus?: boolean; }; @@ -120,10 +126,11 @@ function getUpdatedSince(days: number): string { * server that keeps handing out cursors from growing the cache without bound. * * `maxPages` also bounds a single refetch: React Query re-requests every page - * retained in the cache on `refetch()`, and the history list refetches on focus - * return and app foreground. Staying near the browsable requirement (instead of - * a 100-page bound) keeps one focus/foreground refetch to a small, bounded - * number of page requests, matching `INBOX_MAX_PAGES`. + * retained in the cache on `refetch()`. The foreground and refresh paths + * reconcile page one instead (see `storedReconcile`), but the bound still caps + * the fan-out of any full refetch and the memory the history holds. Staying + * near the browsable requirement (instead of a 100-page bound) keeps that + * fan-out small, matching `INBOX_MAX_PAGES`. */ const SESSION_HISTORY_MAX_PAGES = 20; @@ -137,20 +144,20 @@ export function buildStoredSessionsQueryOptions( trpc: ReturnType, options?: UseAgentSessionsOptions ) { - return withInfiniteRetention( - trpc.cliSessionsV2.list.infiniteQueryOptions(buildAgentSessionListInput(options ?? {}), { + const base = trpc.cliSessionsV2.list.infiniteQueryOptions( + buildAgentSessionListInput(options ?? {}), + { staleTime: 30_000, enabled: options?.enabled, getNextPageParam: lastPage => lastPage.nextCursor, - // Native window-focus refetch stays on by default so Home and the Share - // Gate keep their OS-foreground refresh. The Agents list opts out: its - // screen runs an AppState 'active' callback through the wrapped refetch, - // so the native query lifecycle must not start a stored refetch that - // bypasses the operation coordinator shared with backfill and departure. - refetchOnWindowFocus: options?.refetchOnWindowFocus ?? true, - }), - SESSION_HISTORY_MAX_PAGES + // The hook owns the OS-foreground refresh through its `useAppLifecycle` + // edge (see `storedReconcile`), so the native window-focus refetch stays + // off by default: React Query's native path re-requests every retained + // page, which is exactly the burst the page-one reconcile replaces. + refetchOnWindowFocus: options?.refetchOnWindowFocus ?? false, + } ); + return withInfiniteRetention(base, SESSION_HISTORY_MAX_PAGES); } function useStoredSessions(options?: UseAgentSessionsOptions) { @@ -237,32 +244,65 @@ export function useAgentSessions(options?: UseAgentSessionsOptions) { const queryClient = useQueryClient(); const stored = useStoredSessions(options); const active = useActiveSessions(options); + // The app's one shared AppState store (seeded `true`), so a mount never + // invents a background -> active edge. Used below for the foreground + // reconcile that replaced React Query's native window-focus fan-out. + const { isActive } = useAppLifecycle(); // Rows delivered since this hook mounted, as opposed to rows restored from // the query cache (in-memory or the encrypted read cache). `dataUpdatedAt` - // only advances when the stored query successfully delivers data, so a fresh - // screen mount that cannot reach the API has no fresh rows even when a - // previous mount left rows cached. The Agents history screen uses this to - // show its retryable full-screen error on a failed fresh open instead of - // presenting stale rows as loaded, while a failure after rows were actually - // delivered this mount keeps them with the inline error. + // advances when the stored query successfully delivers data, but + // `reconcileFirstPage` (the foreground edge, pull-to-refresh, retry and + // departure reconciles) is also a cache write: it empties `pages` and + // advances `dataUpdatedAt` without a delivery, so requiring at least one + // delivered page tells a delivery apart from that reset. The flag latches + // once true, because a later reconcile empties `pages` again and must not + // make rows already delivered this mount read as stale. A fresh screen mount + // that cannot reach the API therefore has no fresh rows even when a previous + // mount left rows cached. The Agents history screen uses this to show its + // retryable full-screen error on a failed fresh open instead of presenting + // stale rows as loaded, while a failure after rows were actually delivered + // this mount keeps them with the inline error. const [storedDataUpdatedAtAtMount] = useState(stored.dataUpdatedAt); - const storedFetchedSinceMount = stored.dataUpdatedAt > storedDataUpdatedAtAtMount; + const storedFetchedSinceMountRef = useRef(false); + if ( + !storedFetchedSinceMountRef.current && + stored.dataUpdatedAt > storedDataUpdatedAtAtMount && + (stored.data?.pages.length ?? 0) > 0 + ) { + storedFetchedSinceMountRef.current = true; + } + const storedFetchedSinceMount = storedFetchedSinceMountRef.current; // One coordinator per hook instance, shared by the stored list's next-page - // fetch and every stored refetch (focus return, pull-to-refresh, retry, + // fetch and every stored reconcile (foreground edge, pull-to-refresh, retry, // departure trigger). The backfill selector's `isFetching` gate only sees - // the previous render, so the backfill effect and the focus effect can fire - // in the same commit; serializing both operations guarantees they never + // the previous render, so the backfill effect and the foreground effect can + // fire in the same commit; serializing both operations guarantees they never // overlap the same infinite query. The query methods are aliased because // React Query memoizes them, and listing the object itself would rebuild the // callbacks every render (the backfill effect depends on their stability). const enqueueStoredOperation = useMemo(() => createOperationCoordinator(), []); - const storedRefetchFn = stored.refetch; const storedFetchNextPage = stored.fetchNextPage; - const storedRefetch = useCallback(async () => { - await enqueueStoredOperation(storedRefetchFn); - }, [enqueueStoredOperation, storedRefetchFn]); + // Foreground/refresh reconcile. `reconcileFirstPage` resets the cached pages + // and invalidates the prefix, so page one refetches from `initialPageParam` + // instead of React Query re-requesting every retained page (up to + // `SESSION_HISTORY_MAX_PAGES`). `cancelRefetch: false` is required: after the + // reset `query.state.data` is still defined, so a default `cancelRefetch: + // true` would cancel the page-one fetch the invalidate just started and issue + // a second request. + const storedReconcile = useCallback(async () => { + await enqueueStoredOperation(async () => { + reconcileFirstPage(queryClient, trpc.cliSessionsV2.list.pathFilter().queryKey); + // `cancelRefetch` is a `refetchQueries` option (second argument), not a + // query filter: `false` makes it await the page-one fetch the invalidate + // just started instead of cancelling it and issuing a second request. + await queryClient.refetchQueries( + { queryKey: trpc.cliSessionsV2.list.pathFilter().queryKey, type: 'active' }, + { cancelRefetch: false } + ); + }); + }, [enqueueStoredOperation, queryClient, trpc]); const fetchNextPage = useCallback(async () => { await enqueueStoredOperation(storedFetchNextPage); }, [enqueueStoredOperation, storedFetchNextPage]); @@ -272,19 +312,27 @@ export function useAgentSessions(options?: UseAgentSessionsOptions) { // using the shared collection helper. const storedSessions = useMemo(() => collectUnfilteredPages(stored.data?.pages), [stored.data]); - // Render hold: `reconcileFirstPage` (departure effect below, mutation - // settle via `invalidateAgentSessionQueries`) empties the cached pages - // before refetching page one. Keep rendering the last non-empty rows for - // the same query key until the refetch delivers, so the SectionList never - // unmounts and scroll survives. A key change (filter/sort) or a settled - // empty result releases the hold. + // Render hold: `reconcileFirstPage` (foreground edge and refresh below, + // departure effect, mutation settle via `invalidateAgentSessionQueries`) + // empties the cached pages before refetching page one. Keep rendering the + // last non-empty rows for the same query key until the refetch delivers, so + // the SectionList never unmounts and scroll survives. A key change + // (filter/sort) or a settled empty result releases the hold. + // + // The hold gate spans the whole unresolved refetch, not just the in-flight + // one: after the reset `isFetching` is false while the page-one fetch is + // paused (offline) and after it fails (`isError`), and in both cases the + // cache holds zero rows while the user's rows exist only in the hold. + // Releasing there would blank the list into its empty or full-screen error + // state. `resolveStoredSessionsHold` only gates on a caller-supplied boolean, + // so the composite is computed here. const storedQueryKeyJson = JSON.stringify( trpc.cliSessionsV2.list.infiniteQueryKey(buildAgentSessionListInput(options ?? {})) ); const storedHoldRef = useRef | null>(null); const resolvedStored = resolveStoredSessionsHold({ current: storedSessions, - isFetching: stored.isFetching, + isFetching: stored.isFetching || stored.isPaused || stored.isError, queryKeyJson: storedQueryKeyJson, previousHold: storedHoldRef.current, }); @@ -365,6 +413,22 @@ export function useAgentSessions(options?: UseAgentSessionsOptions) { } }, [active.canRead, activeSessionIds, queryClient, trpc]); + // App-foreground edge. The native window-focus refetch is off (see the + // builder), so the hook owns the OS-foreground refresh and reconciles page + // one instead of fanning a refetch out over every retained page. + // `wasActiveRef` mirrors the store's seeded `true`, so a mount while active + // never invents an edge. Callers that drive their own foreground refresh + // (the Agents list) pass `refetchOnWindowFocus: false` and are not + // reconciled twice. + const wasActiveRef = useRef(isActive); + const ownsForegroundReconcile = options?.refetchOnWindowFocus !== false; + useEffect(() => { + if (!wasActiveRef.current && isActive && ownsForegroundReconcile) { + void storedReconcile(); + } + wasActiveRef.current = isActive; + }, [isActive, ownsForegroundReconcile, storedReconcile]); + return { storedSessions: renderedStoredSessions, activeSessions, @@ -413,8 +477,12 @@ export function useAgentSessions(options?: UseAgentSessionsOptions) { hasNextPage: stored.hasNextPage, isFetchingNextPage: stored.isFetchingNextPage, fetchNextPage, + // Pull-to-refresh, retry and the Share Gate's refresh reconcile page one + // as well: after `maxPages` evicts page one, a plain stored `refetch()` + // would only refresh the pages still in cache (the departure trigger below + // reconciles for the same reason). refetch: async () => { - await Promise.all([storedRefetch(), active.refetch()]); + await Promise.all([storedReconcile(), active.refetch()]); }, }; } diff --git a/apps/mobile/src/lib/hooks/use-code-reviews.test.ts b/apps/mobile/src/lib/hooks/use-code-reviews.test.ts index 7ed0124227a..776a8dca099 100644 --- a/apps/mobile/src/lib/hooks/use-code-reviews.test.ts +++ b/apps/mobile/src/lib/hooks/use-code-reviews.test.ts @@ -2,11 +2,18 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { + buildReviewFirstPageQueryOptions, + buildReviewListQueryKey, buildReviewListQueryOptions, cancelReviewMutationFn, createManualReviewMutationFn, + mergeReviewFirstPage, + nextReviewPollFailureStreak, retriggerReviewMutationFn, + REVIEW_LIST_MAX_PAGES, REVIEW_PAGE_SIZE, + REVIEW_POLL_MAX_CONSECUTIVE_FAILURES, + selectReviewFirstPageAction, useCancelReview, useCreateManualReview, useRetriggerReview, @@ -347,6 +354,15 @@ function makePage(count: number, hasMore = false, status = 'completed'): ReviewP }; } +function makePageWithIds(ids: string[], hasMore = false): ReviewPage { + return { + success: true, + reviews: ids.map(id => ({ id, status: 'running' })), + total: ids.length, + hasMore, + }; +} + // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule function callQueryFn(options: ReviewListOptions, pageParam: number): Promise { const queryFn = options.queryFn as unknown as (context: { @@ -365,11 +381,13 @@ function readGetNextPageParam( ) => number | undefined; } -function readRefetchInterval( - options: ReviewListOptions -): (query: { state: { data?: { pages: ReviewPage[] } } }) => number | false { +type ReviewProbeOptions = ReturnType; + +function readProbeRefetchInterval( + options: ReviewProbeOptions +): (query: { state: { data?: ReviewPage } }) => number | false { return options.refetchInterval as unknown as (query: { - state: { data?: { pages: ReviewPage[] } }; + state: { data?: ReviewPage }; }) => number | false; } @@ -415,6 +433,24 @@ describe('buildReviewListQueryOptions (offset pagination)', () => { expect(getNextPageParam(page, [page], 50)).toBe(100); }); + it('stops forward paging at the retention bound instead of evicting the newest page', () => { + const options = buildReviewListQueryOptions(createReviewTrpcStub(), 'personal'); + const getNextPageParam = readGetNextPageParam(options); + const page = makePage(50, true); + const atBound = Array.from({ length: REVIEW_LIST_MAX_PAGES }, () => page); + const belowBound = atBound.slice(0, REVIEW_LIST_MAX_PAGES - 1); + + // The list is newest-first, so React Query's `maxPages` trim would drop the + // front (newest) page. Refusing the next page keeps the newest page retained + // and makes `mergeReviewFirstPage`'s page-one guard reachable forever. + expect( + getNextPageParam(page, atBound, (REVIEW_LIST_MAX_PAGES - 1) * REVIEW_PAGE_SIZE) + ).toBeUndefined(); + expect(getNextPageParam(page, belowBound, (REVIEW_LIST_MAX_PAGES - 2) * REVIEW_PAGE_SIZE)).toBe( + (REVIEW_LIST_MAX_PAGES - 1) * REVIEW_PAGE_SIZE + ); + }); + it('stops pagination when hasMore is false or the page failed', () => { const options = buildReviewListQueryOptions(createReviewTrpcStub(), 'personal'); const getNextPageParam = readGetNextPageParam(options); @@ -480,21 +516,226 @@ describe('buildReviewListQueryOptions (offset pagination)', () => { expect(getNextPageParam(page2, [page1, page2], 50)).toBeUndefined(); }); - it('polls every 5s while the first page holds a running review', () => { + it('bounds retention with maxPages and keeps the poll off the infinite query', () => { const options = buildReviewListQueryOptions(createReviewTrpcStub(), 'personal'); - const refetchInterval = readRefetchInterval(options); - expect(refetchInterval({ state: { data: { pages: [makePage(50, true, 'running')] } } })).toBe( - 5000 + expect(options.maxPages).toBe(REVIEW_LIST_MAX_PAGES); + expect(options.maxPages).toBeGreaterThan(0); + expect(options).not.toHaveProperty('refetchInterval'); + }); +}); + +// --------------------------------------------------------------------------- +// buildReviewFirstPageQueryOptions: the off-the-infinite-query page-one poll +// --------------------------------------------------------------------------- + +type ReviewListCacheParam = Parameters[0]; +type ReviewListPageParam = Parameters[1]; + +function asListCache(pages: ReviewPage[], pageParams: number[]): NonNullable { + return { pages, pageParams } as unknown as NonNullable; +} + +function asReviewPage(page: ReviewPage): NonNullable { + return page as unknown as NonNullable; +} + +describe('buildReviewFirstPageQueryOptions (page-one probe)', () => { + it('keys the probe under the list key so prefix invalidation still matches', () => { + const trpc = createReviewTrpcStub(); + const listKey = buildReviewListQueryKey(trpc, 'personal'); + const options = buildReviewFirstPageQueryOptions(trpc, 'personal', { enabled: true }); + + expect(listKey).toEqual(['codeReviews', 'listForUser']); + expect(options.queryKey.slice(0, listKey.length)).toEqual(listKey); + expect(options.queryKey.length).toBeGreaterThan(listKey.length); + }); + + it('keys the org probe under the org list key', () => { + const trpc = createReviewTrpcStub(); + const listKey = buildReviewListQueryKey(trpc, 'org_42'); + const options = buildReviewFirstPageQueryOptions(trpc, 'org_42', { enabled: true }); + + expect(listKey).toEqual(['codeReviews', 'listForOrganization', { organizationId: 'org_42' }]); + expect(options.queryKey.slice(0, listKey.length)).toEqual(listKey); + expect(options.queryKey.length).toBeGreaterThan(listKey.length); + }); + + it('fetches offset 0 only, with the caller enabled flag and a zero staleTime', async () => { + const trpc = createReviewTrpcStub(); + const options = buildReviewFirstPageQueryOptions(trpc, 'personal', { enabled: true }); + + expect(options.staleTime).toBe(0); + expect(options.enabled).toBe(true); + expect(buildReviewFirstPageQueryOptions(trpc, 'personal', { enabled: false }).enabled).toBe( + false ); + + listForUserQueryMock.mockResolvedValueOnce(makePage(1)); + await options.queryFn(); + + expect(listForUserQueryMock).toHaveBeenCalledTimes(1); + expect(listForUserQueryMock).toHaveBeenCalledWith({ limit: 50, offset: 0 }); }); - it('does not poll for a terminal first page or when no page is loaded', () => { - const options = buildReviewListQueryOptions(createReviewTrpcStub(), 'personal'); - const refetchInterval = readRefetchInterval(options); + it('polls page one every 5s only while that page holds a running review', () => { + const options = buildReviewFirstPageQueryOptions(createReviewTrpcStub(), 'personal', { + enabled: true, + }); + const refetchInterval = readProbeRefetchInterval(options); + + expect(refetchInterval({ state: { data: makePage(50, true, 'running') } })).toBe(5000); + // A settled page with no running review is the one state that stops it. + expect(refetchInterval({ state: { data: makePage(50, false) } })).toBe(false); + }); + + it('keeps polling through a transient failure instead of clearing the interval', () => { + const options = buildReviewFirstPageQueryOptions(createReviewTrpcStub(), 'personal', { + enabled: true, + }); + const refetchInterval = readProbeRefetchInterval(options); + + // No page delivered yet (the first probe fetch failed): the review may still + // be running, so the interval must not clear on that one blip. + expect(refetchInterval({ state: {} })).toBe(5000); + // The last successful page is still held after a failed refetch (React Query + // keeps `data` on error), and a defensive failure payload keeps it polling. + expect( + refetchInterval({ state: { data: { success: false, reviews: [], error: 'boom' } } }) + ).toBe(5000); + expect(refetchInterval({ state: { data: makePage(50, true, 'running') } })).toBe(5000); + }); + + it('stops polling once the consecutive-failure bound is reached', () => { + const withoutPage = { state: {} }; + const withRunningReview = { state: { data: makePage(50, true, 'running') } }; + const belowBound = buildReviewFirstPageQueryOptions(createReviewTrpcStub(), 'personal', { + enabled: true, + failedPolls: REVIEW_POLL_MAX_CONSECUTIVE_FAILURES - 1, + }); + const atBound = buildReviewFirstPageQueryOptions(createReviewTrpcStub(), 'personal', { + enabled: true, + failedPolls: REVIEW_POLL_MAX_CONSECUTIVE_FAILURES, + }); + + // Below the bound a running review keeps re-arming, with or without a page. + expect(readProbeRefetchInterval(belowBound)(withoutPage)).toBe(5000); + expect(readProbeRefetchInterval(belowBound)(withRunningReview)).toBe(5000); + // At the bound every poll has failed: stop the loop until a refresh restarts it. + expect(readProbeRefetchInterval(atBound)(withoutPage)).toBe(false); + expect(readProbeRefetchInterval(atBound)(withRunningReview)).toBe(false); + }); + + it('counts consecutive failed polls and clears them on a delivered page', () => { + // No failure has settled yet: the streak stays empty. + expect(nextReviewPollFailureStreak(0, { dataUpdatedAt: 0, errorUpdatedAt: 0 })).toBe(0); + // A delivered page is the newer settle: the streak restarts even though the + // probe still holds the page it delivered before. + expect(nextReviewPollFailureStreak(2, { dataUpdatedAt: 200, errorUpdatedAt: 100 })).toBe(0); + // A failed poll is the newer settle: advance. This is the retained-page case + // — `dataUpdatedAt` stays put on a failed refetch. + expect(nextReviewPollFailureStreak(0, { dataUpdatedAt: 100, errorUpdatedAt: 200 })).toBe(1); + expect(nextReviewPollFailureStreak(1, { dataUpdatedAt: 100, errorUpdatedAt: 300 })).toBe(2); + }); + + it('rejects a resolved failure page like the list builder so a handler error never becomes probe data', async () => { + const options = buildReviewFirstPageQueryOptions(createReviewTrpcStub(), 'personal', { + enabled: true, + }); + listForUserQueryMock.mockResolvedValueOnce({ success: false, reviews: [], error: 'boom' }); + + await expect(options.queryFn()).rejects.toThrow('boom'); + }); +}); + +describe('mergeReviewFirstPage', () => { + it('replaces page one and leaves pageParams and later pages untouched', () => { + const oldFirst = makePage(2, true); + const oldSecond = makePage(2, false); + const fresh = makePage(3, true); + const existing = asListCache([oldFirst, oldSecond], [0, 2]); + + const merged = mergeReviewFirstPage(existing, asReviewPage(fresh)); + + expect(merged?.pages).toEqual([fresh, oldSecond]); + expect(merged?.pageParams).toEqual([0, 2]); + }); + + it('returns the existing cache unchanged when it has no pages', () => { + const existing = asListCache([], []); + + expect(mergeReviewFirstPage(existing, asReviewPage(makePage(1)))).toBe(existing); + }); + + it('leaves the list untouched when the cache does not start at offset 0', () => { + // `buildReviewListQueryOptions` caps forward paging so page one is never + // evicted from the front; the guard is the defensive no-op for any cache + // whose head is not offset 0. + const retainedFirst = makePage(2, true, 'running'); + const retainedSecond = makePage(2, false); + const existing = asListCache([retainedFirst, retainedSecond], [50, 100]); + + expect(mergeReviewFirstPage(existing, asReviewPage(makePage(3, true)))).toBe(existing); + }); + + it('leaves the list untouched when the probe page is not successful', () => { + const existing = asListCache([makePage(1)], [0]); + + expect( + mergeReviewFirstPage(existing, asReviewPage({ success: false, reviews: [], error: 'boom' })) + ).toBe(existing); + }); + + it('no-ops on a missing cache', () => { + expect(mergeReviewFirstPage(undefined, asReviewPage(makePage(1)))).toBeUndefined(); + expect(mergeReviewFirstPage(undefined, undefined)).toBeUndefined(); + }); +}); + +describe('selectReviewFirstPageAction', () => { + it('merges when the fresh page one holds the same rows in the same order', () => { + const existing = asListCache( + [makePageWithIds(['a', 'b'], true), makePageWithIds(['c'])], + [0, 2] + ); + + expect( + selectReviewFirstPageAction(existing, asReviewPage(makePageWithIds(['a', 'b'], true))) + ).toBe('merge'); + }); + + it('refetches the retained pages when a review added at the top shifts the boundary', () => { + // `created_at desc`: page one is [a, b] and page two starts at offset 2. A + // new review at the top makes page one [new, a], so the retained page two + // (offset 2) now starts at the row that was page one's last entry — merging + // would skip the row that moved across the boundary. + const existing = asListCache( + [makePageWithIds(['a', 'b'], true), makePageWithIds(['c'])], + [0, 2] + ); + + expect( + selectReviewFirstPageAction(existing, asReviewPage(makePageWithIds(['new', 'a'], true))) + ).toBe('refetch'); + }); + + it('merges when only one page is retained, whatever the probe holds', () => { + // No later page to desync: the next page param is recomputed from the new + // page one. + const existing = asListCache([makePageWithIds(['a', 'b'], true)], [0]); + + expect( + selectReviewFirstPageAction(existing, asReviewPage(makePageWithIds(['new', 'a'], true))) + ).toBe('merge'); + }); - expect(refetchInterval({ state: { data: { pages: [makePage(50, false)] } } })).toBe(false); - expect(refetchInterval({ state: { data: { pages: [] } } })).toBe(false); - expect(refetchInterval({ state: {} })).toBe(false); + it('merges (no-op) when there is no cache or the probe failed', () => { + expect(selectReviewFirstPageAction(undefined, asReviewPage(makePage(1)))).toBe('merge'); + expect( + selectReviewFirstPageAction( + asListCache([makePage(1), makePage(1)], [0, 1]), + asReviewPage({ success: false, reviews: [], error: 'boom' }) + ) + ).toBe('merge'); }); }); diff --git a/apps/mobile/src/lib/hooks/use-code-reviews.ts b/apps/mobile/src/lib/hooks/use-code-reviews.ts index a75e1ee0893..a785b2e799d 100644 --- a/apps/mobile/src/lib/hooks/use-code-reviews.ts +++ b/apps/mobile/src/lib/hooks/use-code-reviews.ts @@ -1,10 +1,18 @@ -import { useInfiniteQuery, useMutation, useQuery, useQueryClient } from '@tanstack/react-query'; +import { useEffect, useMemo, useRef, useState } from 'react'; +import { + type InfiniteData, + useInfiniteQuery, + useMutation, + useQuery, + useQueryClient, +} from '@tanstack/react-query'; import { hasInFlightReview, isInFlightReviewStatus } from '@kilocode/app-shared/code-review'; import { type inferRouterInputs, type MobileRouter } from '@kilocode/trpc/mobile'; import { i18n } from '@/i18n'; import { announcingToast } from '@/lib/a11y/announcing-toast'; import { PERSONAL_SCOPE } from '@/lib/hooks/use-code-reviewer'; +import { withInfiniteRetention } from '@/lib/query/infinite-retention'; import { trpcClient, useTRPC } from '@/lib/trpc'; function isPersonal(scope: string) { @@ -13,8 +21,88 @@ function isPersonal(scope: string) { export const REVIEW_PAGE_SIZE = 50; +/** + * Retention bound for the review-list infinite query. + * + * The list is browsed by scroll, so a bounded window of pages keeps the + * browsable range reachable. The bound matters most on invalidation and app + * foreground: a refetch of an infinite query re-requests every retained page, + * so the page count is what caps that burst. Ten pages of 50 reviews is far more + * than a user scrolls back through, while still stopping an unbounded history + * from accumulating for the life of the screen. + * + * `listCodeReviews` orders `created_at desc`, so page one is the NEWEST page. + * React Query's `maxPages` trims the front element on a forward fetch + * (`addToEnd(..., maxPages)`), which would evict the newest reviews from the top + * of the list. `buildReviewListQueryOptions` therefore also stops forward paging + * at this bound, so the bound is reached by refusing the next page and `maxPages` + * never has to trim. + */ +export const REVIEW_LIST_MAX_PAGES = 10; + +/** Poll cadence for page one while a review on it is still running. */ +const REVIEW_POLL_INTERVAL_MS = 5000; + +/** + * Consecutive failed polls the page-one probe tolerates before its interval + * stops. + * + * The interval must survive a failed poll: one transient error must not stop + * live updates for the rest of the review. It must not re-arm forever either — + * `probing` is derived from the list's cached page one, and only a delivered + * probe page updates that page, so a probe whose endpoint keeps failing can + * never turn itself off. Past this bound the poll stops until an external + * refresh (foreground/route invalidate, pull-to-refresh, cancel/retrigger) + * brings it back. See AGENTS.md "Failure UX": retry within a bound. + */ +export const REVIEW_POLL_MAX_CONSECUTIVE_FAILURES = 3; + +/** + * Next consecutive-failed-poll count for the page-one probe. A poll that settled + * with a delivered page clears the count; a poll that settled with an error + * advances it. + * + * The settle timestamps decide, not the presence of `data`: a failed refetch + * keeps the last delivered page (and its `dataUpdatedAt`), so "has data" is not + * the same as "the last poll delivered". `dataUpdatedAt >= errorUpdatedAt` means + * the last settle delivered a page, or nothing has failed yet. + */ +export function nextReviewPollFailureStreak( + previous: number, + probe: { dataUpdatedAt: number; errorUpdatedAt: number } +): number { + return probe.dataUpdatedAt >= probe.errorUpdatedAt ? 0 : previous + 1; +} + +/** Child segment that keeps the page-one probe under the list's invalidate prefix. */ +const REVIEW_FIRST_PAGE_KEY = 'firstPage'; + type ReviewListPage = Awaited>; +type ReviewListData = InfiniteData; + +/** Fetch one page of the recent-reviews list at `offset` (personal or org scope). */ +async function fetchReviewListPage(scope: string, offset: number): Promise { + const page = isPersonal(scope) + ? await trpcClient.codeReviews.listForUser.query({ limit: REVIEW_PAGE_SIZE, offset }) + : await trpcClient.codeReviews.listForOrganization.query({ + organizationId: scope, + limit: REVIEW_PAGE_SIZE, + offset, + }); + return page; +} + +/** + * The review-list query key for `scope` — exactly the key `useInvalidateReviews` + * invalidates and the key the list infinite query is stored under. + */ +export function buildReviewListQueryKey(trpc: ReturnType, scope: string) { + return isPersonal(scope) + ? trpc.codeReviews.listForUser.queryKey() + : trpc.codeReviews.listForOrganization.queryKey({ organizationId: scope }); +} + /** * Build the review-list infinite-query options. Kept as a pure builder so the * offset paging is testable without mounting the hook. The query key is exactly @@ -23,50 +111,253 @@ type ReviewListPage = Awaited, scope: string) { - const personal = isPersonal(scope); + return withInfiniteRetention( + { + queryKey: buildReviewListQueryKey(trpc, scope), + initialPageParam: 0, + queryFn: async ({ pageParam }: { pageParam: number }): Promise => { + const page = await fetchReviewListPage(scope, pageParam); + // The list endpoints resolve handler errors as `{ success: false, error }` + // instead of throwing (no tRPC client link converts them). Reject here so + // React Query marks the page as an error: a failed first page drives the + // screen's transient QueryError, and a failed next page drives the retry + // footer via `isFetchNextPageError` while keeping the loaded rows. + if (!page.success) { + throw new Error(page.error); + } + return page; + }, + getNextPageParam: ( + lastPage: ReviewListPage, + pages: ReviewListPage[], + lastPageParam: number + ) => + lastPage.success && lastPage.hasMore && pages.length < REVIEW_LIST_MAX_PAGES + ? lastPageParam + lastPage.reviews.length + : undefined, + }, + REVIEW_LIST_MAX_PAGES + ); +} + +/** + * Build the page-one probe used while a review is in flight. It fetches offset 0 + * only, so one poll tick is one request instead of one per retained page; the + * result is merged back into the list cache by `useReviewList`. + * + * The key is a child of the list key, so `useInvalidateReviews` and the + * route-level `[['codeReviews']]` foreground invalidate still refresh the probe + * along with the list. + * + * @param probe.enabled Whether a running review on the cached page one makes the + * probe worth running at all. + * @param probe.failedPolls Consecutive failed polls so far, counted by + * `useReviewList`; the interval stops at + * `REVIEW_POLL_MAX_CONSECUTIVE_FAILURES`. + */ +export function buildReviewFirstPageQueryOptions( + trpc: ReturnType, + scope: string, + probe: { enabled: boolean; failedPolls?: number } +) { + const { enabled, failedPolls = 0 } = probe; return { - queryKey: personal - ? trpc.codeReviews.listForUser.queryKey() - : trpc.codeReviews.listForOrganization.queryKey({ organizationId: scope }), - initialPageParam: 0, - queryFn: async ({ pageParam }: { pageParam: number }): Promise => { - const page = personal - ? await trpcClient.codeReviews.listForUser.query({ - limit: REVIEW_PAGE_SIZE, - offset: pageParam, - }) - : await trpcClient.codeReviews.listForOrganization.query({ - organizationId: scope, - limit: REVIEW_PAGE_SIZE, - offset: pageParam, - }); - // The list endpoints resolve handler errors as `{ success: false, error }` - // instead of throwing (no tRPC client link converts them). Reject here so - // React Query marks the page as an error: a failed first page drives the - // screen's transient QueryError, and a failed next page drives the retry - // footer via `isFetchNextPageError` while keeping the loaded rows. + queryKey: [...buildReviewListQueryKey(trpc, scope), REVIEW_FIRST_PAGE_KEY], + queryFn: async (): Promise => { + const page = await fetchReviewListPage(scope, 0); + // Same contract as the list builder: the endpoints resolve handler errors + // as `{ success: false, error }`, and a resolved failure must not become + // this query's `data`. Throwing keeps the last successful page (React + // Query keeps `data` across a failed refetch), so the interval below still + // sees the running review it is polling for. if (!page.success) { throw new Error(page.error); } return page; }, - getNextPageParam: (lastPage: ReviewListPage, _pages: ReviewListPage[], lastPageParam: number) => - lastPage.success && lastPage.hasMore ? lastPageParam + lastPage.reviews.length : undefined, - refetchInterval: (query: { state: { data?: { pages: ReviewListPage[] } } }) => { - const firstPage = query.state.data?.pages[0]; - if (!firstPage?.success) { + staleTime: 0, + enabled, + refetchInterval: (query: { state: { data?: ReviewListPage } }) => { + // Bounded retry: this probe is the only writer of the list's page one, so + // an endpoint that keeps failing would otherwise keep one request (plus + // this query's own retries) going every interval for the life of the + // screen. `useReviewList` counts consecutive failed polls and restarts the + // count on a delivered list or probe page, on a scope change, and on a + // review starting or ending. + if (failedPolls >= REVIEW_POLL_MAX_CONSECUTIVE_FAILURES) { return false; } - return hasInFlightReview(firstPage.reviews) ? 5000 : false; + // Poll until a successful page one with no running review arrives: the + // interval must survive a transient failure (no page delivered yet, or + // the last successful page still held after a failed refetch) instead of + // clearing on it, or one blip stops the poll for the rest of the review. + const page = query.state.data; + if (!page?.success) { + return REVIEW_POLL_INTERVAL_MS; + } + return hasInFlightReview(page.reviews) ? REVIEW_POLL_INTERVAL_MS : false; }, }; } +/** + * Whether the probe's fresh page one holds the same rows, in the same order, as + * the cached page one — i.e. replacing it cannot shift the page-one/page-two + * boundary. + * + * `listCodeReviews` orders `created_at desc`, so a review added or removed at + * the top shifts every later page's offset by one. `mergeReviewFirstPage` keeps + * `pageParams` and the later pages, so a changed row set would skip the row that + * moved across the boundary, and `dedupeById` cannot recover a gap (the row is + * simply absent, not duplicated). When the row set changed the caller refetches + * the retained pages instead, re-requesting every page at its offset. + */ +function reviewFirstPageBoundaryMatches( + existing: ReviewListData | undefined, + page: ReviewListPage | undefined +): boolean { + const cachedFirst = existing?.pages[0]; + if (!cachedFirst?.success || !page?.success) { + return false; + } + return ( + cachedFirst.reviews.length === page.reviews.length && + cachedFirst.reviews.every((review, index) => review.id === page.reviews[index]?.id) + ); +} + +/** + * What the page-one probe should do with its fresh page. Pure so the decision is + * unit-testable. + * + * `merge` swaps page one in place. `refetch` is for the shifted-boundary case: + * with more than one retained page, a changed row set means every later page + * starts at a stale offset, so the retained pages are refetched (each at its own + * param) instead of skipping the row that moved across the boundary. A single + * retained page has no boundary to desync — the next page param is recomputed + * from the new page one — so it always merges. + */ +export function selectReviewFirstPageAction( + existing: ReviewListData | undefined, + page: ReviewListPage | undefined +): 'merge' | 'refetch' { + if (!page?.success) { + return 'merge'; + } + if (existing && existing.pages.length > 1 && !reviewFirstPageBoundaryMatches(existing, page)) { + return 'refetch'; + } + return 'merge'; +} + +/** + * Replace page one of a cached review list with a fresh page. Pure so the merge + * is unit-testable. Returns `existing` unchanged when there is no cache, no + * page one, or the incoming page failed — pageParams and later pages are kept. + * + * The `pageParams[0] === 0` guard keeps the write on the first page. It is also + * the guard that would no-op the merge if page one were ever evicted from the + * front; `buildReviewListQueryOptions` caps forward paging at + * `REVIEW_LIST_MAX_PAGES` so that cannot happen on this list. + */ +export function mergeReviewFirstPage( + existing: ReviewListData | undefined, + page: ReviewListPage | undefined +): ReviewListData | undefined { + if (!existing || existing.pages.length === 0 || existing.pageParams[0] !== 0 || !page?.success) { + return existing; + } + return { ...existing, pages: [page, ...existing.pages.slice(1)] }; +} + export function useReviewList(scope: string) { const trpc = useTRPC(); - return useInfiniteQuery(buildReviewListQueryOptions(trpc, scope)); + const queryClient = useQueryClient(); + const list = useInfiniteQuery(buildReviewListQueryOptions(trpc, scope)); + + // A running review is created newest-first, so it lands on page one; that + // page decides whether a page-one probe is worth running at all. The screen + // renders every retained page, but only page one is polled so one tick is one + // request instead of one per retained page. + const firstPage = list.data?.pages[0]; + const probing = firstPage?.success === true && hasInFlightReview(firstPage.reviews); + + // Consecutive failed polls for the probe. The interval survives a failed poll + // (one transient error must not stop live updates for the rest of the review) + // but stops at the bound, so a permanently failing endpoint cannot leave a + // request loop running for the life of the screen. React Query keeps no + // consecutive-failure counter (`fetchFailureCount` restarts with every fetch), + // so the count is advanced here off the probe's own settle signals. + // + // The budget restarts whenever the surface behind the poll is refreshed: a + // delivered list page (a foreground or pull-to-refresh list refetch), the + // probe turning on or off (a review starting or ending), or a new scope. A + // restart lets a poll that hit the bound recover without remounting. + const [failedProbePolls, setFailedProbePolls] = useState(0); + const probe = useQuery( + buildReviewFirstPageQueryOptions(trpc, scope, { + enabled: probing, + failedPolls: failedProbePolls, + }) + ); + const probeBudgetKey = `${scope}|${probing}|${list.dataUpdatedAt}`; + const probeBudgetRef = useRef(probeBudgetKey); + // Read the settle signals as primitives for the same reason as `listRefetch` + // below: the query result object is fresh each render, and an effect that ran + // on every render would advance the streak on every render too. + const probeDataUpdatedAt = probe.dataUpdatedAt; + const probeErrorUpdatedAt = probe.errorUpdatedAt; + useEffect(() => { + const budgetChanged = probeBudgetRef.current !== probeBudgetKey; + probeBudgetRef.current = probeBudgetKey; + // Settle deps: `dataUpdatedAt` moves when the probe delivers a page, + // `errorUpdatedAt` when a poll fails. + setFailedProbePolls(previous => + budgetChanged + ? 0 + : nextReviewPollFailureStreak(previous, { + dataUpdatedAt: probeDataUpdatedAt, + errorUpdatedAt: probeErrorUpdatedAt, + }) + ); + }, [probeBudgetKey, probeDataUpdatedAt, probeErrorUpdatedAt]); + + // Memoised: the builder returns a fresh array on every call, and the effect + // below must not re-run every render because of a new key reference. + const listKey = useMemo( + () => buildReviewListQueryKey(trpc, scope), + [trpc, scope] + ); + // React Query memoizes `refetch`, but the result object is fresh each render; + // listing the method keeps the effect from re-running on every render. + const listRefetch = list.refetch; + + useEffect(() => { + const page = probe.data; + if (!page?.success) { + return; + } + const existing = queryClient.getQueryData(listKey); + if (selectReviewFirstPageAction(existing, page) === 'refetch') { + // The probe's page one holds a different row set, so the retained later + // pages start at stale offsets. Re-request every retained page at its + // offset (`cancelRefetch: false` joins an in-flight refetch instead of + // cancelling and re-issuing it) so the list stays contiguous. + void listRefetch({ cancelRefetch: false }); + return; + } + queryClient.setQueryData(listKey, current => + mergeReviewFirstPage(current, page) + ); + }, [probe.data, queryClient, listKey, listRefetch]); + + return list; } export function useReviewDetail(reviewId: string) { diff --git a/apps/mobile/src/lib/hooks/use-organization-queries.test.ts b/apps/mobile/src/lib/hooks/use-organization-queries.test.ts new file mode 100644 index 00000000000..454224a29f4 --- /dev/null +++ b/apps/mobile/src/lib/hooks/use-organization-queries.test.ts @@ -0,0 +1,225 @@ +import { InfiniteQueryObserver, QueryClient } from '@tanstack/react-query'; +import { describe, expect, it, vi } from 'vitest'; + +import { + buildOrgCreditTransactionsPageQueryOptions, + buildOrgInvoicesPageQueryOptions, +} from '@/lib/hooks/use-organization-queries'; +import { INFINITE_QUERY_MAX_PAGES } from '@/lib/query/infinite-retention'; + +// The hook module imports its context providers (and the real tRPC client) +// transitively; the options builders are pure, so only the module-load chain +// needs these mocks. No hook is mounted. +vi.mock('@/lib/trpc', () => ({ + useTRPC: vi.fn(), +})); +vi.mock('@/lib/auth/auth-context', () => ({ useAuth: vi.fn() })); +vi.mock('@/lib/organization-context', () => ({ useOrganization: vi.fn() })); + +type Page = { hasMore: boolean; nextCursor: string | null }; + +const echoInfiniteQueryOptions = (input: unknown, options: object) => ({ ...options, input }); + +type EchoedOptions = { + maxPages: number; + enabled: boolean; + input: { organizationId: string; period?: string }; + getNextPageParam: (lastPage: Page, pages: Page[]) => string | undefined; +}; + +/** + * Fake tRPC router whose `infiniteQueryOptions` echoes the options it was + * given alongside the input, so a builder's retention bound and the exact + * input each procedure receives can be asserted without mounting the hook. + */ +function createTrpcStub() { + const stub = { + organizations: { + creditTransactionsPage: { infiniteQueryOptions: echoInfiniteQueryOptions }, + invoicesPage: { infiniteQueryOptions: echoInfiniteQueryOptions }, + }, + }; + return stub as never; +} + +function readOptions(options: object): EchoedOptions { + return options as unknown as EchoedOptions; +} + +describe('buildOrgCreditTransactionsPageQueryOptions', () => { + it('applies the shared retention bound', () => { + const options = readOptions( + buildOrgCreditTransactionsPageQueryOptions(createTrpcStub(), 'org-1') + ); + + expect(options.maxPages).toBe(INFINITE_QUERY_MAX_PAGES); + }); + + it('keeps the enabled passthrough (disabled for a null organizationId)', () => { + expect( + readOptions(buildOrgCreditTransactionsPageQueryOptions(createTrpcStub(), 'org-1')).enabled + ).toBe(true); + expect( + readOptions(buildOrgCreditTransactionsPageQueryOptions(createTrpcStub(), null)).enabled + ).toBe(false); + }); + + it('passes the organizationId through to the procedure input', () => { + expect( + readOptions(buildOrgCreditTransactionsPageQueryOptions(createTrpcStub(), 'org-1')).input + ).toEqual({ organizationId: 'org-1' }); + expect( + readOptions(buildOrgCreditTransactionsPageQueryOptions(createTrpcStub(), null)).input + ).toEqual({ organizationId: '' }); + }); + + it('returns the next cursor while hasMore, and stops on the last page', () => { + const { getNextPageParam } = readOptions( + buildOrgCreditTransactionsPageQueryOptions(createTrpcStub(), 'org-1') + ); + + expect(getNextPageParam({ hasMore: true, nextCursor: 'cursor-1' }, [])).toBe('cursor-1'); + expect(getNextPageParam({ hasMore: false, nextCursor: 'cursor-2' }, [])).toBeUndefined(); + expect(getNextPageParam({ hasMore: true, nextCursor: null }, [])).toBeUndefined(); + }); + + it('stops forward paging at the retention bound instead of evicting the newest page', () => { + const { getNextPageParam } = readOptions( + buildOrgCreditTransactionsPageQueryOptions(createTrpcStub(), 'org-1') + ); + const page: Page = { hasMore: true, nextCursor: 'cursor-1' }; + const atBound = Array.from({ length: INFINITE_QUERY_MAX_PAGES }, () => page); + const belowBound = atBound.slice(0, INFINITE_QUERY_MAX_PAGES - 1); + + // The list is newest-first, so React Query's `maxPages` trim on a forward + // fetch (`addToEnd` slices index 0) would drop page one — the newest page. + // Refusing the next page keeps every loaded page, newest rows included. + expect(getNextPageParam(page, atBound)).toBeUndefined(); + expect(getNextPageParam(page, belowBound)).toBe('cursor-1'); + }); +}); + +describe('buildOrgInvoicesPageQueryOptions', () => { + it('applies the shared retention bound', () => { + const options = readOptions(buildOrgInvoicesPageQueryOptions(createTrpcStub(), 'org-1')); + + expect(options.maxPages).toBe(INFINITE_QUERY_MAX_PAGES); + }); + + it('keeps the enabled passthrough (disabled for a null organizationId)', () => { + expect(readOptions(buildOrgInvoicesPageQueryOptions(createTrpcStub(), 'org-1')).enabled).toBe( + true + ); + expect(readOptions(buildOrgInvoicesPageQueryOptions(createTrpcStub(), null)).enabled).toBe( + false + ); + }); + + it("keeps the fixed period: 'year' input and the organizationId", () => { + expect(readOptions(buildOrgInvoicesPageQueryOptions(createTrpcStub(), 'org-1')).input).toEqual({ + organizationId: 'org-1', + period: 'year', + }); + }); + + it('returns the next cursor while hasMore, and stops on the last page', () => { + const { getNextPageParam } = readOptions( + buildOrgInvoicesPageQueryOptions(createTrpcStub(), 'org-1') + ); + + expect(getNextPageParam({ hasMore: true, nextCursor: 'cursor-1' }, [])).toBe('cursor-1'); + expect(getNextPageParam({ hasMore: false, nextCursor: 'cursor-2' }, [])).toBeUndefined(); + expect(getNextPageParam({ hasMore: true, nextCursor: null }, [])).toBeUndefined(); + }); + + it('stops forward paging at the retention bound instead of evicting the newest page', () => { + const { getNextPageParam } = readOptions( + buildOrgInvoicesPageQueryOptions(createTrpcStub(), 'org-1') + ); + const page: Page = { hasMore: true, nextCursor: 'cursor-1' }; + const atBound = Array.from({ length: INFINITE_QUERY_MAX_PAGES }, () => page); + const belowBound = atBound.slice(0, INFINITE_QUERY_MAX_PAGES - 1); + + // Stripe lists invoices newest-first, so `maxPages` would trim page one (the + // newest page) on a forward fetch. Refusing the next page at the bound keeps + // the newest invoices retained. + expect(getNextPageParam(page, atBound)).toBeUndefined(); + expect(getNextPageParam(page, belowBound)).toBe('cursor-1'); + }); +}); + +/** + * Drive the builder's real `getNextPageParam` and `maxPages` through React + * Query's infinite-query behavior, so the front-trim hazard the finding + * describes is reproduced against the cache rather than asserted on the + * callback alone: without the refusal bound the sixth forward fetch runs + * `addToEnd(pages, page, 5)`, which drops index 0 — page one, the newest page. + */ +describe('org infinite-list retention', () => { + type RetentionPage = { + entries: { id: string }[]; + hasMore: boolean; + nextCursor: string | null; + }; + + type RetentionGetNextPageParam = ( + lastPage: RetentionPage, + pages: RetentionPage[] + ) => string | undefined; + + async function collectPageIds( + getNextPageParam: RetentionGetNextPageParam, + forwards: number + ): Promise<{ ids: string[]; fetched: number; hasNextPage: boolean }> { + const queryClient = new QueryClient(); + let fetched = 0; + const observer = new InfiniteQueryObserver(queryClient, { + queryKey: ['org-list-retention-test'], + initialPageParam: '0', + // eslint-disable-next-line require-await -- the recipe pins an async queryFn; the body needs no await. + queryFn: async ({ pageParam }: { pageParam: string }) => { + const index = Number(pageParam); + fetched += 1; + return { + entries: [{ id: `entry-${index}` }], + hasMore: true, + nextCursor: String(index + 1), + }; + }, + getNextPageParam, + maxPages: INFINITE_QUERY_MAX_PAGES, + staleTime: Infinity, + retry: false, + }); + // eslint-disable-next-line no-empty-function -- a real listener is required to activate the observer; the body is intentionally empty. + const unsubscribe = observer.subscribe(() => {}); + + await observer.refetch(); + for (let loaded = 1; loaded <= forwards; loaded += 1) { + // eslint-disable-next-line no-await-in-loop -- each page's cursor is the previous page's result. + await observer.fetchNextPage(); + } + + const result = observer.getCurrentResult(); + const ids = (result.data?.pages ?? []).flatMap(page => page.entries.map(entry => entry.id)); + unsubscribe(); + return { ids, fetched, hasNextPage: result.hasNextPage }; + } + + it('keeps the newest page instead of letting maxPages front-trim it', async () => { + const { getNextPageParam } = readOptions( + buildOrgCreditTransactionsPageQueryOptions(createTrpcStub(), 'org-1') + ) as unknown as { getNextPageParam: RetentionGetNextPageParam }; + + const { ids, fetched, hasNextPage } = await collectPageIds( + getNextPageParam, + INFINITE_QUERY_MAX_PAGES + 2 + ); + + // Page one (`entry-0`, the newest entry) is still at the front, every page + // that was loaded is retained, and no sixth request was issued. + expect(ids).toEqual(['entry-0', 'entry-1', 'entry-2', 'entry-3', 'entry-4']); + expect(fetched).toBe(INFINITE_QUERY_MAX_PAGES); + expect(hasNextPage).toBe(false); + }); +}); diff --git a/apps/mobile/src/lib/hooks/use-organization-queries.ts b/apps/mobile/src/lib/hooks/use-organization-queries.ts index a57a2ea27ae..7c2a5e8446d 100644 --- a/apps/mobile/src/lib/hooks/use-organization-queries.ts +++ b/apps/mobile/src/lib/hooks/use-organization-queries.ts @@ -5,6 +5,7 @@ import { useMemo } from 'react'; import { useOrganizationsList } from '@/lib/hooks/use-organizations-list'; import { useOrganization } from '@/lib/organization-context'; +import { INFINITE_QUERY_MAX_PAGES, withInfiniteRetention } from '@/lib/query/infinite-retention'; import { useTRPC } from '@/lib/trpc'; type RouterOutputs = inferRouterOutputs; @@ -135,28 +136,61 @@ export type CreditTransaction = RouterOutputs['organizations']['creditTransactionsPage']['entries'][number]; /** - * Cursor-paginated credit transactions for an organization. Mirrors the legacy - * `useOrgCreditTransactions` surface (flat `entries`) but pages through - * `organizations.creditTransactionsPage` with `useInfiniteQuery` so the screen - * can offer "Load more" instead of scanning every row at once. + * Whether the newest-first org lists have reached their retention bound. + * + * Both lists order `created_at desc`, so page one holds the newest entries and + * each screen renders every retained page as one flattened list. The shared + * `INFINITE_QUERY_MAX_PAGES` retention bound therefore has to be reached by + * refusing the next page rather than by React Query's `maxPages` trim: a + * forward fetch runs `addToEnd(pages, page, maxPages)`, which drops index 0 + * once the bound is exceeded, and index 0 is page one — the newest page. Left + * to `maxPages` alone that silently removes the newest rows from the top of + * the screen once a sixth page loads. Refusing the next page keeps every + * loaded page retained; the hooks read `hasNextPage`, so "Load more" + * disappears at the bound instead of paging into a silent no-op. */ -export function useOrgCreditTransactionsPage(organizationId: string | null) { - const trpc = useTRPC(); - const query = useInfiniteQuery( +function hasReachedRetentionBound(pageCount: number): boolean { + return pageCount >= INFINITE_QUERY_MAX_PAGES; +} + +/** + * Build the credit-transactions infinite-query options. Kept as a pure builder + * so the retention bound and the cursor passthrough are testable without + * mounting the hook. + */ +export function buildOrgCreditTransactionsPageQueryOptions( + trpc: ReturnType, + organizationId: string | null +) { + return withInfiniteRetention( trpc.organizations.creditTransactionsPage.infiniteQueryOptions( { organizationId: organizationId ?? '' }, { enabled: organizationId != null, - getNextPageParam: lastPage => - lastPage.hasMore ? (lastPage.nextCursor ?? undefined) : undefined, + getNextPageParam: (lastPage, pages) => + lastPage.hasMore && !hasReachedRetentionBound(pages.length) + ? (lastPage.nextCursor ?? undefined) + : undefined, } ) ); +} + +/** + * Cursor-paginated credit transactions for an organization. Mirrors the legacy + * `useOrgCreditTransactions` surface (flat `entries`) but pages through + * `organizations.creditTransactionsPage` with `useInfiniteQuery` so the screen + * can offer \"Load more\" instead of scanning every row at once. + */ +export function useOrgCreditTransactionsPage(organizationId: string | null) { + const trpc = useTRPC(); + const query = useInfiniteQuery(buildOrgCreditTransactionsPageQueryOptions(trpc, organizationId)); const pages = query.data?.pages; const entries = useMemo(() => (pages ?? []).flatMap(page => page.entries), [pages]); - const lastPage = pages != null && pages.length > 0 ? pages.at(-1) : undefined; - const hasMore = lastPage?.hasMore ?? false; + // `hasNextPage` folds the builder's refusal bound into the server's last-page + // `hasMore`, so the "Load more" control hides at the retention bound. + const hasMore = query.hasNextPage; return { query, entries, hasMore }; } @@ -164,28 +198,43 @@ export function useOrgCreditTransactionsPage(organizationId: string | null) { export type OrgInvoice = RouterOutputs['organizations']['invoicesPage']['entries'][number]; /** - * Cursor-paginated invoices for an organization. Mirrors the legacy - * `useOrgInvoices` surface (flat `entries`) but pages through - * `organizations.invoicesPage` with `useInfiniteQuery` so the screen can offer - * "Load more" instead of loading every invoice at once. + * Build the invoices infinite-query options. Kept as a pure builder so the + * retention bound, the fixed `period: 'year'` input, and the cursor + * passthrough are testable without mounting the hook. */ -export function useOrgInvoicesPage(organizationId: string | null) { - const trpc = useTRPC(); - const query = useInfiniteQuery( +export function buildOrgInvoicesPageQueryOptions( + trpc: ReturnType, + organizationId: string | null +) { + return withInfiniteRetention( trpc.organizations.invoicesPage.infiniteQueryOptions( { organizationId: organizationId ?? '', period: 'year' }, { enabled: organizationId != null, - getNextPageParam: lastPage => - lastPage.hasMore ? (lastPage.nextCursor ?? undefined) : undefined, + getNextPageParam: (lastPage, pages) => + lastPage.hasMore && !hasReachedRetentionBound(pages.length) + ? (lastPage.nextCursor ?? undefined) + : undefined, } ) ); +} + +/** + * Cursor-paginated invoices for an organization. Mirrors the legacy + * `useOrgInvoices` surface (flat `entries`) but pages through + * `organizations.invoicesPage` with `useInfiniteQuery` so the screen can offer + * \"Load more\" instead of loading every invoice at once. + */ +export function useOrgInvoicesPage(organizationId: string | null) { + const trpc = useTRPC(); + const query = useInfiniteQuery(buildOrgInvoicesPageQueryOptions(trpc, organizationId)); const pages = query.data?.pages; const entries = useMemo(() => (pages ?? []).flatMap(page => page.entries), [pages]); - const lastPage = pages != null && pages.length > 0 ? pages.at(-1) : undefined; - const hasMore = lastPage?.hasMore ?? false; + // `hasNextPage` folds the builder's refusal bound into the server's last-page + // `hasMore`, so the "Load more" control hides at the retention bound. + const hasMore = query.hasNextPage; return { query, entries, hasMore }; } diff --git a/apps/mobile/src/lib/query-client.test.ts b/apps/mobile/src/lib/query-client.test.ts index cb9316e97a9..c59d539f4a7 100644 --- a/apps/mobile/src/lib/query-client.test.ts +++ b/apps/mobile/src/lib/query-client.test.ts @@ -3,11 +3,13 @@ import { CancelledError, QueryObserver } from '@tanstack/react-query'; import { deferred, + flushQueryUpdates, makeTestQueryClient as makeClient, QUERY_KEY, } from './active-sessions-live-sync.test-helpers'; import { createKiloAppQueryClient, + DEFAULT_QUERY_STALE_TIME_MS, getActiveSessionsQueryMetadata, subscribeActiveSessionsQueryMetadata, } from './query-client'; @@ -90,6 +92,7 @@ describe('permission-denied query removal', () => { await expect( queryClient.fetchQuery({ queryKey: org1Key, + staleTime: 0, queryFn: () => { throw error; }, @@ -122,6 +125,7 @@ describe('permission-denied query removal', () => { await expect( queryClient.fetchQuery({ queryKey: key, + staleTime: 0, queryFn: () => { throw error; }, @@ -186,6 +190,7 @@ describe('permission-denied query removal', () => { await expect( queryClient.fetchQuery({ queryKey: key, + staleTime: 0, queryFn: () => { throw error; }, @@ -437,3 +442,80 @@ describe('app error reporting', () => { }); }); }); + +describe('default query stale time', () => { + // Deliberately one microtask deep so a refetch is observable, mirroring the + // shared makeQueryFn helper. + const makeResolvingQueryFn = () => + vi.fn(async () => { + const result = await Promise.resolve({ ok: true }); + return result; + }); + + it('defaults new queries to the shared stale time', () => { + const client = createKiloAppQueryClient(); + expect(client.getDefaultOptions().queries?.staleTime).toBe(DEFAULT_QUERY_STALE_TIME_MS); + }); + + it('serves a fresh query from cache on remount without calling the query function again', async () => { + const client = createKiloAppQueryClient(); + const queryKey = ['session', 'stale-time-default']; + const queryFn = makeResolvingQueryFn(); + + const first = new QueryObserver(client, { queryKey, queryFn }); + const unsubscribeFirst = first.subscribe(() => undefined); + await vi.waitFor(() => { + expect(first.getCurrentResult().data).toEqual({ ok: true }); + }); + expect(queryFn).toHaveBeenCalledTimes(1); + unsubscribeFirst(); + + const second = new QueryObserver(client, { queryKey, queryFn }); + const unsubscribeSecond = second.subscribe(() => undefined); + await flushQueryUpdates(); + // The cached payload is still fresh, so remounting must not refetch it. + expect(queryFn).toHaveBeenCalledTimes(1); + expect(second.getCurrentResult().data).toEqual({ ok: true }); + unsubscribeSecond(); + }); + + it('still refetches on remount when the query overrides staleTime with 0', async () => { + const client = createKiloAppQueryClient(); + const queryKey = ['session', 'stale-time-zero']; + const queryFn = makeResolvingQueryFn(); + + const first = new QueryObserver(client, { queryKey, queryFn, staleTime: 0 }); + const unsubscribeFirst = first.subscribe(() => undefined); + await vi.waitFor(() => { + expect(first.getCurrentResult().data).toEqual({ ok: true }); + }); + expect(queryFn).toHaveBeenCalledTimes(1); + unsubscribeFirst(); + + const second = new QueryObserver(client, { queryKey, queryFn, staleTime: 0 }); + const unsubscribeSecond = second.subscribe(() => undefined); + await vi.waitFor(() => { + expect(queryFn).toHaveBeenCalledTimes(2); + }); + unsubscribeSecond(); + }); + + it('still refetches a fresh query when it is invalidated', async () => { + const client = createKiloAppQueryClient(); + const queryKey = ['session', 'stale-time-invalidate']; + const queryFn = makeResolvingQueryFn(); + + const observer = new QueryObserver(client, { queryKey, queryFn }); + const unsubscribe = observer.subscribe(() => undefined); + await vi.waitFor(() => { + expect(observer.getCurrentResult().data).toEqual({ ok: true }); + }); + expect(queryFn).toHaveBeenCalledTimes(1); + + await client.invalidateQueries({ queryKey }); + await vi.waitFor(() => { + expect(queryFn).toHaveBeenCalledTimes(2); + }); + unsubscribe(); + }); +}); diff --git a/apps/mobile/src/lib/query-client.ts b/apps/mobile/src/lib/query-client.ts index 82d4b911a05..dd063aebc6a 100644 --- a/apps/mobile/src/lib/query-client.ts +++ b/apps/mobile/src/lib/query-client.ts @@ -220,10 +220,23 @@ function removePermissionDeniedQueries( queryClient.removeQueries({ queryKey: query.queryKey }); } +/** + * Default freshness for app queries. Screens paint persisted-cache rows before + * the network answers (U4); this keeps a mount or focus regain from re-requesting + * the same payload for 30 s. Explicit `staleTime: 0` call sites still override it. + */ +export const DEFAULT_QUERY_STALE_TIME_MS = 30_000; + export function createKiloAppQueryClient(): QueryClient { const queryClient = new QueryClient({ defaultOptions: { queries: { + // Keep the persisted-cache first paint: cached rows are fresh for the + // default window, so a mount or focus regain does not immediately + // re-request what is already on screen. Every explicit invalidation path + // (invalidateQueries, refetchQueries, refetchInterval, pull-to-refresh) + // still refetches, and call sites that pass `staleTime: 0` still override. + staleTime: DEFAULT_QUERY_STALE_TIME_MS, // Foreground freshness is owned per route by useRouteForegroundRefresh mounts; the blanket focusManager refetch also woke frozen background tabs. refetchOnWindowFocus: false, retry: (failureCount, error) => {