From 30fef765319433221c79e2235325d5b5f41e4e6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Fri, 21 Aug 2026 03:25:57 +0200 Subject: [PATCH 01/14] feat(cloud-agent-sdk): add reconnect exhaustion signal and retry Add an additive two-edge reconnect-exhaustion signal so mobile can show an explicit recovery action instead of an indefinite Reconnecting label. The auth-failure stop path fires the same terminal edge. --- ...r-web-connection-provider.mounted.test.tsx | 3 + .../src/base-connection.test.ts | 82 +++++++++++++++++++ .../cloud-agent-sdk/src/base-connection.ts | 44 +++++++++- .../src/session-routing.test.ts | 6 ++ .../src/session-transport.test.ts | 3 + packages/cloud-agent-sdk/src/session.test.ts | 6 ++ .../src/user-web-connection.test.ts | 68 +++++++++++++++ .../src/user-web-connection.ts | 33 ++++++++ 8 files changed, 243 insertions(+), 2 deletions(-) diff --git a/apps/mobile/src/components/agents/user-web-connection-provider.mounted.test.tsx b/apps/mobile/src/components/agents/user-web-connection-provider.mounted.test.tsx index 8f461abf55..8702427664 100644 --- a/apps/mobile/src/components/agents/user-web-connection-provider.mounted.test.tsx +++ b/apps/mobile/src/components/agents/user-web-connection-provider.mounted.test.tsx @@ -25,6 +25,9 @@ vi.mock('@kilocode/cloud-agent-sdk/user-web-connection', () => ({ destroy: vi.fn(), isConnected: vi.fn(() => false), onConnectionChange: vi.fn(() => vi.fn()), + isReconnectExhausted: vi.fn(() => false), + onReconnectExhaustionChange: vi.fn(() => vi.fn()), + retryConnection: vi.fn(), subscribeToCliSession: vi.fn(() => vi.fn()), sendCommand: vi.fn(), sendCommandToConnection: vi.fn(), diff --git a/packages/cloud-agent-sdk/src/base-connection.test.ts b/packages/cloud-agent-sdk/src/base-connection.test.ts index 24aa92c109..1d631c0d2f 100644 --- a/packages/cloud-agent-sdk/src/base-connection.test.ts +++ b/packages/cloud-agent-sdk/src/base-connection.test.ts @@ -503,6 +503,84 @@ describe('createBaseConnection – stale WebSocket recovery', () => { }); }); + describe('reconnect exhaustion signal', () => { + function exhaustWithCap(onReconnectExhaustionChange: jest.Mock) { + jest.spyOn(Math, 'random').mockReturnValue(0); + const { connection } = createTestConnection({ + maxReconnectAttempts: 2, + onReconnectExhaustionChange, + }); + connection.connect(); + + closeSocket(0); + jest.advanceTimersByTime(60_000); + closeSocket(1); + jest.advanceTimersByTime(60_000); + closeSocket(2); + + return connection; + } + + it('fires true exactly once at the cap', () => { + const onReconnectExhaustionChange = jest.fn(); + const connection = exhaustWithCap(onReconnectExhaustionChange); + + expect(onReconnectExhaustionChange).toHaveBeenCalledTimes(1); + expect(onReconnectExhaustionChange).toHaveBeenCalledWith(true); + + connection.destroy(); + }); + + it('retryReconnect fires false and reconnects', () => { + const onReconnectExhaustionChange = jest.fn(); + const connection = exhaustWithCap(onReconnectExhaustionChange); + const socketsAfterExhaustion = sockets.length; + + connection.retryReconnect(); + + expect(onReconnectExhaustionChange).toHaveBeenLastCalledWith(false); + expect(sockets.length).toBe(socketsAfterExhaustion + 1); + + connection.destroy(); + }); + + it('online event after exhaustion fires false and reconnects', () => { + const onReconnectExhaustionChange = jest.fn(); + const connection = exhaustWithCap(onReconnectExhaustionChange); + const socketsAfterExhaustion = sockets.length; + + sockets[sockets.length - 1].readyState = 3; // WebSocket.CLOSED + mockWindow.dispatchEvent(new Event('online')); + + expect(onReconnectExhaustionChange).toHaveBeenLastCalledWith(false); + expect(sockets.length).toBe(socketsAfterExhaustion + 1); + + connection.destroy(); + }); + + it('successful message after exhaustion fires false', () => { + const onReconnectExhaustionChange = jest.fn(); + const connection = exhaustWithCap(onReconnectExhaustionChange); + + connectSocket(2); + + expect(onReconnectExhaustionChange).toHaveBeenLastCalledWith(false); + + connection.destroy(); + }); + + it('connect() while exhausted fires false', () => { + const onReconnectExhaustionChange = jest.fn(); + const connection = exhaustWithCap(onReconnectExhaustionChange); + + connection.connect(); + + expect(onReconnectExhaustionChange).toHaveBeenLastCalledWith(false); + + connection.destroy(); + }); + }); + describe('onReconnected vs onConnected', () => { it('fires onConnected on first successful connection', () => { const { connection, onConnected, onReconnected } = createTestConnection(); @@ -564,9 +642,11 @@ describe('createBaseConnection – stale WebSocket recovery', () => { it('notifies route loss when the refreshed socket also closes for auth failure', async () => { const refreshAuth = jest.fn(() => Promise.resolve()); const onReplacingConnection = jest.fn(); + const onReconnectExhaustionChange = jest.fn(); const { connection } = createTestConnection({ refreshAuth, onReplacingConnection, + onReconnectExhaustionChange, isAuthFailure: event => event.code === 4001 || event.code === 1008, }); connection.connect(); @@ -580,6 +660,8 @@ describe('createBaseConnection – stale WebSocket recovery', () => { expect(onReplacingConnection).toHaveBeenCalledTimes(2); expect(refreshAuth).toHaveBeenCalledTimes(1); + expect(onReconnectExhaustionChange).toHaveBeenCalledTimes(1); + expect(onReconnectExhaustionChange).toHaveBeenCalledWith(true); jest.advanceTimersByTime(60_000); expect(sockets).toHaveLength(2); connection.destroy(); diff --git a/packages/cloud-agent-sdk/src/base-connection.ts b/packages/cloud-agent-sdk/src/base-connection.ts index ccd7e77ce8..864a6d0132 100644 --- a/packages/cloud-agent-sdk/src/base-connection.ts +++ b/packages/cloud-agent-sdk/src/base-connection.ts @@ -43,12 +43,20 @@ export type BaseConnectionConfig = { * For browser usage, use `createBrowserLifecycleHooks()`. * For CLI usage, omit this or provide custom hooks. */ lifecycleHooks?: ConnectionLifecycleHooks | undefined; + /** Cap on automatic reconnect attempts before the connection is considered + * exhausted. Defaults to `MAX_RECONNECT_ATTEMPTS`. */ + maxReconnectAttempts?: number | undefined; + /** Fires on both edges of the reconnect-exhaustion state: `true` when the + * automatic retry cap is reached (or a terminal auth failure stops retries), + * and `false` when any recovery path resets the state. */ + onReconnectExhaustionChange?: ((exhausted: boolean) => void) | undefined; }; export type Connection = { connect: () => void; disconnect: () => void; reconnectWithRefreshedAuth?: () => void; + retryReconnect: () => void; destroy: () => void; }; @@ -72,6 +80,7 @@ export function createBaseConnection(config: BaseConnectionConfig): Connec let authRefreshAttempted = false; let connected = false; let reconnectAttempt = 0; + let exhausted = false; let generation = 0; let hasConnectedOnce = false; let stalenessTimeoutId: ReturnType | null = null; @@ -79,6 +88,7 @@ export function createBaseConnection(config: BaseConnectionConfig): Connec let hiddenAt = 0; let preconnectAuthRefreshAttempted = false; const stalenessTimeoutMs = config.stalenessTimeoutMs ?? DEFAULT_STALENESS_TIMEOUT_MS; + const maxReconnectAttempts = config.maxReconnectAttempts ?? MAX_RECONNECT_ATTEMPTS; // Cleanup functions returned by lifecycle hooks const cleanupFns: Array<() => void> = []; @@ -98,6 +108,13 @@ export function createBaseConnection(config: BaseConnectionConfig): Connec } } + function clearExhausted(): void { + if (exhausted) { + exhausted = false; + config.onReconnectExhaustionChange?.(false); + } + } + function notifyReplacingConnection(expectedGeneration = generation): boolean { config.onReplacingConnection?.(); return !destroyed && !intentionalDisconnect && expectedGeneration === generation; @@ -160,7 +177,11 @@ export function createBaseConnection(config: BaseConnectionConfig): Connec function scheduleReconnect(attempt: number, expectedGeneration: number) { if (destroyed || intentionalDisconnect || expectedGeneration !== generation) return; - if (attempt >= MAX_RECONNECT_ATTEMPTS) { + if (attempt >= maxReconnectAttempts) { + if (!exhausted) { + exhausted = true; + config.onReconnectExhaustionChange?.(true); + } return; } @@ -234,6 +255,7 @@ export function createBaseConnection(config: BaseConnectionConfig): Connec // Reset auth refresh flag on successful message authRefreshAttempted = false; reconnectAttempt = 0; + clearExhausted(); if (!connected) { connected = true; @@ -285,6 +307,10 @@ export function createBaseConnection(config: BaseConnectionConfig): Connec // The current physical route is gone even though no new socket follows. if (isAuthFailure && authRefreshAttempted) { notifyReplacingConnection(expectedGeneration); + if (!exhausted) { + exhausted = true; + config.onReconnectExhaustionChange?.(true); + } return; } @@ -319,6 +345,7 @@ export function createBaseConnection(config: BaseConnectionConfig): Connec // Tab became visible reconnectAttempt = 0; + clearExhausted(); const wasHiddenSince = hiddenAt; hiddenAt = 0; @@ -365,6 +392,7 @@ export function createBaseConnection(config: BaseConnectionConfig): Connec // BFCache restore - WebSocket is guaranteed dead reconnectAttempt = 0; + clearExhausted(); clearReconnectTimer(); clearStalenessTimeout(); if (!notifyReplacingConnection()) return; @@ -388,6 +416,7 @@ export function createBaseConnection(config: BaseConnectionConfig): Connec if (connected && ws !== null && ws.readyState === WebSocket.OPEN) return; reconnectAttempt = 0; + clearExhausted(); clearReconnectTimer(); void refreshAndConnect(generation); } @@ -423,6 +452,7 @@ export function createBaseConnection(config: BaseConnectionConfig): Connec preconnectAuthRefreshAttempted = false; connected = false; reconnectAttempt = 0; + clearExhausted(); hasConnectedOnce = false; lastMessageTime = 0; generation += 1; @@ -456,6 +486,7 @@ export function createBaseConnection(config: BaseConnectionConfig): Connec if (destroyed || intentionalDisconnect) return; reconnectAttempt = 0; + clearExhausted(); clearReconnectTimer(); clearStalenessTimeout(); if (!notifyReplacingConnection()) return; @@ -472,6 +503,15 @@ export function createBaseConnection(config: BaseConnectionConfig): Connec void refreshAndConnect(generation); } + function retryReconnect() { + if (destroyed || intentionalDisconnect) return; + + clearReconnectTimer(); + reconnectAttempt = 0; + clearExhausted(); + void refreshAndConnect(generation); + } + function destroy() { destroyed = true; generation += 1; @@ -490,7 +530,7 @@ export function createBaseConnection(config: BaseConnectionConfig): Connec connected = false; } - return { connect, disconnect, reconnectWithRefreshedAuth, destroy }; + return { connect, disconnect, reconnectWithRefreshedAuth, retryReconnect, destroy }; } export function createBrowserLifecycleHooks(): ConnectionLifecycleHooks { diff --git a/packages/cloud-agent-sdk/src/session-routing.test.ts b/packages/cloud-agent-sdk/src/session-routing.test.ts index 64e035f1c0..5c52370741 100644 --- a/packages/cloud-agent-sdk/src/session-routing.test.ts +++ b/packages/cloud-agent-sdk/src/session-routing.test.ts @@ -129,6 +129,9 @@ describe('session transport routing', () => { destroy: jest.fn(), isConnected: jest.fn(() => false), onConnectionChange: jest.fn(() => jest.fn()), + isReconnectExhausted: jest.fn(() => false), + onReconnectExhaustionChange: jest.fn(() => jest.fn()), + retryConnection: jest.fn(), subscribeToCliSession: jest.fn(() => release), sendCommand: jest.fn(() => Promise.resolve()), sendCommandToConnection: jest.fn(() => Promise.resolve()), @@ -219,6 +222,9 @@ describe('session transport routing', () => { destroy: jest.fn(), isConnected: jest.fn(() => false), onConnectionChange: jest.fn(() => jest.fn()), + isReconnectExhausted: jest.fn(() => false), + onReconnectExhaustionChange: jest.fn(() => jest.fn()), + retryConnection: jest.fn(), subscribeToCliSession: jest.fn(() => subscribeRelease), sendCommand: jest.fn(() => Promise.resolve()), sendCommandToConnection: jest.fn(() => Promise.resolve()), diff --git a/packages/cloud-agent-sdk/src/session-transport.test.ts b/packages/cloud-agent-sdk/src/session-transport.test.ts index 936f1838a2..7436c3951f 100644 --- a/packages/cloud-agent-sdk/src/session-transport.test.ts +++ b/packages/cloud-agent-sdk/src/session-transport.test.ts @@ -100,6 +100,9 @@ function createUserWebConnection() { destroy: jest.fn(), isConnected: jest.fn(() => false), onConnectionChange: jest.fn(() => jest.fn()), + isReconnectExhausted: jest.fn(() => false), + onReconnectExhaustionChange: jest.fn(() => jest.fn()), + retryConnection: jest.fn(), subscribeToCliSession: jest.fn(() => jest.fn()), sendCommand: jest.fn((_sessionId: string, command: string) => Promise.resolve( diff --git a/packages/cloud-agent-sdk/src/session.test.ts b/packages/cloud-agent-sdk/src/session.test.ts index 0d7b8686f3..615ba000c8 100644 --- a/packages/cloud-agent-sdk/src/session.test.ts +++ b/packages/cloud-agent-sdk/src/session.test.ts @@ -375,6 +375,9 @@ describe('remote session transport state', () => { destroy: jest.fn(), isConnected: jest.fn(() => false), onConnectionChange: jest.fn(() => jest.fn()), + isReconnectExhausted: jest.fn(() => false), + onReconnectExhaustionChange: jest.fn(() => jest.fn()), + retryConnection: jest.fn(), subscribeToCliSession: jest.fn(() => jest.fn()), sendCommand: jest.fn(() => Promise.resolve({ @@ -448,6 +451,9 @@ describe('remote session create and retry commands', () => { destroy: jest.fn(), isConnected: jest.fn(() => false), onConnectionChange: jest.fn(() => jest.fn()), + isReconnectExhausted: jest.fn(() => false), + onReconnectExhaustionChange: jest.fn(() => jest.fn()), + retryConnection: jest.fn(), subscribeToCliSession: jest.fn(() => jest.fn()), sendCommand: jest.fn, [string, string, unknown, string?]>(() => Promise.resolve({ diff --git a/packages/cloud-agent-sdk/src/user-web-connection.test.ts b/packages/cloud-agent-sdk/src/user-web-connection.test.ts index 2f07ce7f74..4cd1b84ea2 100644 --- a/packages/cloud-agent-sdk/src/user-web-connection.test.ts +++ b/packages/cloud-agent-sdk/src/user-web-connection.test.ts @@ -1927,3 +1927,71 @@ describe('createUserWebConnection connection-state API', () => { client.destroy(); }); }); + +describe('createUserWebConnection reconnect-exhaustion API', () => { + it('reports false initially and subscribes/unsubscribes listeners', () => { + const client = createUserWebConnection({ websocketUrl: WS_URL, getAuthToken: () => 'token' }); + const listener = jest.fn(); + const unsubscribe = client.onReconnectExhaustionChange(listener); + + expect(client.isReconnectExhausted()).toBe(false); + expect(listener).not.toHaveBeenCalled(); + + unsubscribe(); + client.destroy(); + }); + + it('fires the listener on both edges and retryConnection resets the snapshot', async () => { + jest.useFakeTimers(); + jest.spyOn(Math, 'random').mockReturnValue(0); + try { + const listener = jest.fn(); + const client = createUserWebConnection({ + websocketUrl: WS_URL, + getAuthToken: () => 'token', + maxReconnectAttempts: 2, + }); + client.onReconnectExhaustionChange(listener); + const release = client.retain(); + + // Drive the base connection to exhaustion without ever opening a socket + // (no successful message), so `hasEverOpened` stays false and reconnects + // skip the auth refresh. + sockets[0].onclose?.({ code: 1006 } as CloseEvent); + jest.advanceTimersByTime(60_000); + sockets[1].onclose?.({ code: 1006 } as CloseEvent); + jest.advanceTimersByTime(60_000); + sockets[2].onclose?.({ code: 1006 } as CloseEvent); + + expect(listener).toHaveBeenCalledTimes(1); + expect(listener).toHaveBeenCalledWith(true); + expect(client.isReconnectExhausted()).toBe(true); + + client.retryConnection(); + await Promise.resolve(); + await Promise.resolve(); + + expect(listener).toHaveBeenCalledTimes(2); + expect(listener).toHaveBeenLastCalledWith(false); + expect(client.isReconnectExhausted()).toBe(false); + expect(sockets).toHaveLength(4); + + release(); + client.destroy(); + } finally { + jest.useRealTimers(); + } + }); + + it('ignores exhaustion listeners registered after destroy()', () => { + const client = createUserWebConnection({ websocketUrl: WS_URL, getAuthToken: () => 'token' }); + client.destroy(); + const listener = jest.fn(); + + const unsubscribe = client.onReconnectExhaustionChange(listener); + unsubscribe(); + + expect(listener).not.toHaveBeenCalled(); + expect(client.isReconnectExhausted()).toBe(false); + }); +}); diff --git a/packages/cloud-agent-sdk/src/user-web-connection.ts b/packages/cloud-agent-sdk/src/user-web-connection.ts index c0a429afd9..b8c41aa7c9 100644 --- a/packages/cloud-agent-sdk/src/user-web-connection.ts +++ b/packages/cloud-agent-sdk/src/user-web-connection.ts @@ -60,6 +60,7 @@ type UserWebConnectionConfig = { onError?: (message: string) => void; onReconnect?: () => void; lifecycleHooks?: ConnectionLifecycleHooks; + maxReconnectAttempts?: number; }; type SendCommandToConnectionInput = { @@ -91,6 +92,10 @@ type UserWebConnection = { * never invoked and the unsubscribe is a no-op. */ onConnectionChange: (listener: (connected: boolean) => void) => () => void; + // The boolean readiness API (old form) stays the readiness source for existing consumers; the exhaustion signal is additive and owns only recovery UI. Removal condition: none — the boolean API is permanent. + isReconnectExhausted: () => boolean; + onReconnectExhaustionChange: (listener: (exhausted: boolean) => void) => () => void; + retryConnection: () => void; subscribeToCliSession: (sessionId: string) => () => void; sendCommand: ( sessionId: string, @@ -193,6 +198,19 @@ function createUserWebConnection( for (const listener of connectionChangeListeners) listener(value); } + // Wrapper-owned reconnect-exhaustion snapshot, mirroring `connected`. The base + // connection fires the two-edge callback; the wrapper keeps a synchronous + // snapshot so `isReconnectExhausted()` reads it without crossing the base + // boundary and listeners fire only on value changes. + let exhausted = false; + const exhaustionChangeListeners = new Set<(exhausted: boolean) => void>(); + + function setExhausted(value: boolean): void { + if (exhausted === value) return; + exhausted = value; + for (const listener of exhaustionChangeListeners) listener(value); + } + function hasLifetime(): boolean { return retainCount > 0; } @@ -431,6 +449,8 @@ function createUserWebConnection( hasEverOpened = false; baseConnection = createBaseConnection({ lifecycleHooks: createLifecycleHooks(), + maxReconnectAttempts: config.maxReconnectAttempts, + onReconnectExhaustionChange: setExhausted, buildUrl, parseMessage: (data: unknown) => { if (typeof data !== 'string') return null; @@ -541,6 +561,7 @@ function createUserWebConnection( // callback, so the wrapper must drive the disconnected transition itself // before the next `startConnection()` reopens from a false baseline. setConnected(false); + setExhausted(false); } function connect(): void { @@ -646,6 +667,7 @@ function createUserWebConnection( reconnectListeners.clear(); sessionListeners.clear(); connectionChangeListeners.clear(); + exhaustionChangeListeners.clear(); }, isConnected: () => connected, onConnectionChange(listener) { @@ -655,6 +677,17 @@ function createUserWebConnection( connectionChangeListeners.delete(listener); }; }, + isReconnectExhausted: () => exhausted, + onReconnectExhaustionChange(listener) { + if (destroyed) return () => {}; + exhaustionChangeListeners.add(listener); + return () => { + exhaustionChangeListeners.delete(listener); + }; + }, + retryConnection() { + baseConnection?.retryReconnect(); + }, subscribeToCliSession(sessionId) { if (destroyed) return () => {}; const releaseConnection = retainConnection(); From fcf5fbbfb91def6276ace154686d0428928da08d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Fri, 21 Aug 2026 03:35:17 +0200 Subject: [PATCH 02/14] feat(mobile): show explicit recovery action on reconnect exhaustion Render Connection lost with a Retry action when the user-web transport exhausts reconnect attempts, instead of an indefinite Reconnecting label. --- ...session-connection-indicator-state.test.ts | 103 +++++++++++++++++- .../session-connection-indicator-state.ts | 8 +- ...sion-connection-indicator.mounted.test.tsx | 73 ++++++++++++- .../agents/session-connection-indicator.tsx | 33 +++++- .../hooks/use-user-web-connection-state.ts | 24 ++++ 5 files changed, 231 insertions(+), 10 deletions(-) diff --git a/apps/mobile/src/components/agents/session-connection-indicator-state.test.ts b/apps/mobile/src/components/agents/session-connection-indicator-state.test.ts index 4f5e62e11f..849c1e967b 100644 --- a/apps/mobile/src/components/agents/session-connection-indicator-state.test.ts +++ b/apps/mobile/src/components/agents/session-connection-indicator-state.test.ts @@ -14,8 +14,14 @@ function resolve(input: { activeSessionType: 'remote' | 'cloud-agent' | 'read-only' | null; agentStatusType: StatusType; userWebConnected: boolean; + reconnectExhausted?: boolean; }): SessionConnectionState { - return resolveSessionConnectionState(input); + return resolveSessionConnectionState({ + activeSessionType: input.activeSessionType, + agentStatusType: input.agentStatusType, + userWebConnected: input.userWebConnected, + reconnectExhausted: input.reconnectExhausted ?? false, + }); } describe('resolveSessionConnectionState - remote', () => { @@ -91,3 +97,98 @@ describe('resolveSessionConnectionState - no transport', () => { } }); }); + +describe('resolveSessionConnectionState - exhausted', () => { + it('reports exhausted for a remote session when the user-web leg is down and reconnects are exhausted', () => { + expect( + resolve({ + activeSessionType: 'remote', + agentStatusType: 'idle', + userWebConnected: false, + reconnectExhausted: true, + }) + ).toBe('exhausted'); + }); + + it('reports exhausted for a remote disconnected agent status while reconnects are exhausted', () => { + expect( + resolve({ + activeSessionType: 'remote', + agentStatusType: 'disconnected', + userWebConnected: true, + reconnectExhausted: true, + }) + ).toBe('exhausted'); + }); + + it('reports down (not exhausted) for a remote session while the user-web leg is down and reconnects remain', () => { + expect( + resolve({ + activeSessionType: 'remote', + agentStatusType: 'idle', + userWebConnected: false, + reconnectExhausted: false, + }) + ).toBe('down'); + }); + + it('reports up for a connected remote session even while reconnects are exhausted', () => { + for (const status of STATUSES.filter(item => item !== 'disconnected')) { + expect( + resolve({ + activeSessionType: 'remote', + agentStatusType: status, + userWebConnected: true, + reconnectExhausted: true, + }) + ).toBe('up'); + } + }); +}); + +describe('resolveSessionConnectionState - exhausted precedence', () => { + it('never overrides none for read-only sessions while reconnects are exhausted', () => { + for (const status of STATUSES) { + for (const userWebConnected of CONNECTION_VALUES) { + expect( + resolve({ + activeSessionType: 'read-only', + agentStatusType: status, + userWebConnected, + reconnectExhausted: true, + }) + ).toBe('none'); + } + } + }); + + it('never overrides none for unresolved session types while reconnects are exhausted', () => { + for (const status of STATUSES) { + for (const userWebConnected of CONNECTION_VALUES) { + expect( + resolve({ + activeSessionType: null, + agentStatusType: status, + userWebConnected, + reconnectExhausted: true, + }) + ).toBe('none'); + } + } + }); + + it('never applies to cloud-agent sessions', () => { + for (const status of STATUSES) { + for (const userWebConnected of CONNECTION_VALUES) { + expect( + resolve({ + activeSessionType: 'cloud-agent', + agentStatusType: status, + userWebConnected, + reconnectExhausted: true, + }) + ).toBe(status === 'disconnected' ? 'down' : 'up'); + } + } + }); +}); diff --git a/apps/mobile/src/components/agents/session-connection-indicator-state.ts b/apps/mobile/src/components/agents/session-connection-indicator-state.ts index 56a4c2f34a..44822d27f0 100644 --- a/apps/mobile/src/components/agents/session-connection-indicator-state.ts +++ b/apps/mobile/src/components/agents/session-connection-indicator-state.ts @@ -1,14 +1,18 @@ import { type AgentStatus, type ResolvedSession } from '@kilocode/cloud-agent-sdk'; -export type SessionConnectionState = 'up' | 'down' | 'none'; +export type SessionConnectionState = 'up' | 'down' | 'exhausted' | 'none'; export function resolveSessionConnectionState(input: { activeSessionType: ResolvedSession['type'] | null; agentStatusType: AgentStatus['type']; userWebConnected: boolean; + reconnectExhausted: boolean; }): SessionConnectionState { if (input.activeSessionType === 'remote') { - return !input.userWebConnected || input.agentStatusType === 'disconnected' ? 'down' : 'up'; + if (!input.userWebConnected || input.agentStatusType === 'disconnected') { + return input.reconnectExhausted ? 'exhausted' : 'down'; + } + return 'up'; } if (input.activeSessionType === 'cloud-agent') { return input.agentStatusType === 'disconnected' ? 'down' : 'up'; diff --git a/apps/mobile/src/components/agents/session-connection-indicator.mounted.test.tsx b/apps/mobile/src/components/agents/session-connection-indicator.mounted.test.tsx index f487e11317..0099e70c97 100644 --- a/apps/mobile/src/components/agents/session-connection-indicator.mounted.test.tsx +++ b/apps/mobile/src/components/agents/session-connection-indicator.mounted.test.tsx @@ -5,10 +5,15 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { SessionConnectionIndicator } from './session-connection-indicator'; -const connection = vi.hoisted(() => ({ connected: true })); +const connection = vi.hoisted(() => ({ + connected: true, + exhausted: false, + retryConnection: vi.fn(), +})); vi.mock('react-native', () => ({ View: 'View', + Pressable: 'Pressable', })); vi.mock('@/components/ui/icons', () => ({ WifiOff: 'WifiOff', @@ -20,7 +25,13 @@ vi.mock('@/lib/hooks/use-theme-colors', () => ({ useThemeColors: () => ({ mutedForeground: '#666666' }), })); vi.mock('@/lib/hooks/use-user-web-connection-state', () => ({ - useUserWebConnectionState: () => connection.connected, + useUserWebConnectionHealth: () => ({ + isConnected: connection.connected, + reconnectExhausted: connection.exhausted, + }), +})); +vi.mock('@/components/agents/user-web-connection-provider', () => ({ + useUserWebConnection: () => ({ retryConnection: connection.retryConnection }), })); type IndicatorProps = Parameters[0]; @@ -59,6 +70,8 @@ function findHost( describe('SessionConnectionIndicator mounted', () => { beforeEach(() => { connection.connected = true; + connection.exhausted = false; + connection.retryConnection.mockClear(); }); it('renders a blank fixed row with no text for default (pending/error) props', async () => { @@ -146,4 +159,60 @@ describe('SessionConnectionIndicator mounted', () => { expect(findHost(renderer.root, 'Text')).toHaveLength(0); expect(findHost(renderer.root, 'WifiOff')).toHaveLength(0); }); + + it('renders Connection lost with a Retry action when reconnects are exhausted', async () => { + connection.connected = false; + connection.exhausted = true; + const renderer = await mount({ activeSessionType: 'remote', agentStatusType: 'idle' }); + + const view = findHost(renderer.root, 'View')[0]; + expect(view).toBeDefined(); + if (!view) { + throw new Error('view not found'); + } + expect(view.props.accessibilityElementsHidden).toBe(false); + expect(findHost(renderer.root, 'WifiOff')).toHaveLength(1); + const texts = findHost(renderer.root, 'Text'); + expect(texts.some(node => node.props.children === 'Connection lost')).toBe(true); + expect(texts.some(node => node.props.children === 'Retry')).toBe(true); + expect(findHost(renderer.root, 'Pressable')).toHaveLength(1); + }); + + it('calls retryConnection when the Retry action is pressed', async () => { + connection.connected = false; + connection.exhausted = true; + const renderer = await mount({ activeSessionType: 'remote', agentStatusType: 'idle' }); + + const pressables = findHost(renderer.root, 'Pressable'); + expect(pressables).toHaveLength(1); + const pressable = pressables[0]; + expect(pressable).toBeDefined(); + if (!pressable) { + throw new Error('pressable not found'); + } + + await act(async () => { + await Promise.resolve(); + (pressable.props.onPress as () => void)(); + }); + + expect(connection.retryConnection).toHaveBeenCalledTimes(1); + }); + + it('clears the label when the exhaustion edge flips false and the transport recovers', async () => { + connection.connected = false; + connection.exhausted = true; + const renderer = await mount({ activeSessionType: 'remote', agentStatusType: 'idle' }); + expect( + findHost(renderer.root, 'Text').some(node => node.props.children === 'Connection lost') + ).toBe(true); + + connection.exhausted = false; + connection.connected = true; + await update(renderer, { activeSessionType: 'remote', agentStatusType: 'idle' }); + + expect(findHost(renderer.root, 'Text')).toHaveLength(0); + expect(findHost(renderer.root, 'WifiOff')).toHaveLength(0); + expect(findHost(renderer.root, 'Pressable')).toHaveLength(0); + }); }); diff --git a/apps/mobile/src/components/agents/session-connection-indicator.tsx b/apps/mobile/src/components/agents/session-connection-indicator.tsx index c74170b7c2..3e4923c9cf 100644 --- a/apps/mobile/src/components/agents/session-connection-indicator.tsx +++ b/apps/mobile/src/components/agents/session-connection-indicator.tsx @@ -1,10 +1,11 @@ import { type AgentStatus, type ResolvedSession } from '@kilocode/cloud-agent-sdk'; import { WifiOff } from '@/components/ui/icons'; import { useEffect, useRef } from 'react'; -import { View } from 'react-native'; +import { Pressable, View } from 'react-native'; import { Text } from '@/components/ui/text'; -import { useUserWebConnectionState } from '@/lib/hooks/use-user-web-connection-state'; +import { useUserWebConnection } from '@/components/agents/user-web-connection-provider'; +import { useUserWebConnectionHealth } from '@/lib/hooks/use-user-web-connection-state'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; import { resolveSessionConnectionState } from './session-connection-indicator-state'; @@ -18,12 +19,14 @@ export function SessionConnectionIndicator({ activeSessionType = null, agentStatusType = 'idle', }: Readonly) { - const userWebConnected = useUserWebConnectionState(); + const { isConnected: userWebConnected, reconnectExhausted } = useUserWebConnectionHealth(); + const connection = useUserWebConnection(); const colors = useThemeColors(); const state = resolveSessionConnectionState({ activeSessionType, agentStatusType, userWebConnected, + reconnectExhausted, }); // "Ever up" is a committed-state ref (written in an effect), so a drop // after the first committed up reads "Reconnecting…" while a cold start @@ -38,19 +41,39 @@ export function SessionConnectionIndicator({ let label: string | null = null; if (state === 'down') { label = wasUpRef.current ? 'Reconnecting…' : 'Connecting…'; + } else if (state === 'exhausted') { + label = 'Connection lost'; } + // The exhausted state adds an interactive `Retry` action. The row stays a + // single accessibility element for the non-interactive labels only; with a + // pressable child the label text and the action must stay separately + // reachable for assistive technology. + const interactive = state === 'exhausted'; return ( {label !== null ? ( <> {label} + {interactive ? ( + { + connection.retryConnection(); + }} + hitSlop={8} + className="active:opacity-70" + accessibilityRole="button" + accessibilityLabel="Retry connection" + > + Retry + + ) : null} ) : null} diff --git a/apps/mobile/src/lib/hooks/use-user-web-connection-state.ts b/apps/mobile/src/lib/hooks/use-user-web-connection-state.ts index 7865958d6b..8858f0abd4 100644 --- a/apps/mobile/src/lib/hooks/use-user-web-connection-state.ts +++ b/apps/mobile/src/lib/hooks/use-user-web-connection-state.ts @@ -17,3 +17,27 @@ export function useUserWebConnectionState(): boolean { () => connection.isConnected() ); } + +type UserWebConnectionHealth = { + isConnected: boolean; + reconnectExhausted: boolean; +}; + +/** + * Reactive binding to both user-web transport signals: readiness and + * reconnect exhaustion. Readiness keeps driving the automatic + * `Reconnecting…` / `Connecting…` labels; exhaustion drives the explicit + * recovery UI (`Connection lost` + `Retry`). + */ +export function useUserWebConnectionHealth(): UserWebConnectionHealth { + const connection = useUserWebConnection(); + const isConnected = useSyncExternalStore( + listener => connection.onConnectionChange(listener), + () => connection.isConnected() + ); + const reconnectExhausted = useSyncExternalStore( + listener => connection.onReconnectExhaustionChange(listener), + () => connection.isReconnectExhausted() + ); + return { isConnected, reconnectExhausted }; +} From 895eea016f617c891c32cd739d6ffcb8021ac0e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Fri, 21 Aug 2026 03:57:00 +0200 Subject: [PATCH 03/14] fix(mobile): distinguish security capability loading, error, and offline The settings overview now renders a skeleton, a permissions error with retry, or the offline variant instead of permission-denied copy when the org-role query is unresolved or paused. --- .../settings-overview-screen.mounted.test.tsx | 160 +++++++++++++----- .../settings-overview-screen.tsx | 34 +++- .../use-offline-banner-state.mounted.test.tsx | 110 ++++++++---- .../src/lib/hooks/use-offline-banner-state.ts | 32 ++-- .../src/lib/hooks/use-security-agent.test.ts | 126 ++++++++++++++ .../src/lib/hooks/use-security-agent.ts | 29 +++- .../src/lib/offline-banner-state.test.ts | 28 ++- apps/mobile/src/lib/offline-banner-state.ts | 22 ++- 8 files changed, 429 insertions(+), 112 deletions(-) create mode 100644 apps/mobile/src/lib/hooks/use-security-agent.test.ts diff --git a/apps/mobile/src/components/security-agent/settings-overview-screen.mounted.test.tsx b/apps/mobile/src/components/security-agent/settings-overview-screen.mounted.test.tsx index 43a681e2f8..7444cf36fd 100644 --- a/apps/mobile/src/components/security-agent/settings-overview-screen.mounted.test.tsx +++ b/apps/mobile/src/components/security-agent/settings-overview-screen.mounted.test.tsx @@ -17,15 +17,18 @@ const config = vi.hoisted(() => ({ data: null as unknown, isLoading: false, isError: false, + fetchStatus: 'idle' as 'fetching' | 'paused' | 'idle', refetch: vi.fn(), })); const capability = vi.hoisted(() => ({ canManage: true, - isLoading: false, + status: 'allowed' as 'loading' | 'error' | 'denied' | 'allowed', isError: false, - isFetching: false, refetch: vi.fn(), })); +const committedConnectivity = vi.hoisted(() => ({ + status: 'online' as 'online' | 'offline' | 'unknown', +})); const repositories = vi.hoisted(() => ({ data: null as unknown[] | null, isLoading: false, @@ -44,6 +47,10 @@ const configureRows = vi.hoisted(() => ({ rows: [] as { title: string; subtitle?: string; onPress?: () => void }[], })); +const platformErrorScreens = vi.hoisted(() => ({ + screens: [] as { variant?: string; message?: string; onRetry?: () => void }[], +})); + vi.mock('react-native', () => ({ View: 'View', Switch: 'Switch', @@ -68,13 +75,21 @@ vi.mock('@/lib/hooks/use-security-agent', () => ({ useSetSecurityAgentEnabled: () => setEnabled, useTrackSecurityAgentInteraction: () => trackInteraction, })); +vi.mock('@/lib/hooks/use-offline-banner-state', () => ({ + useCommittedConnectivityStatus: () => committedConnectivity.status, +})); vi.mock('@/lib/security-agent', () => ({ getSecurityAgentPath: (scope: string, section: string) => `/security/${scope}/${section}`, })); vi.mock('@/components/security-agent/audit-report-button', () => ({ AuditReportButton: () => null, })); -vi.mock('@/components/platform-error-screen', () => ({ PlatformErrorScreen: () => null })); +vi.mock('@/components/platform-error-screen', () => ({ + PlatformErrorScreen: (props: { variant?: string; message?: string; onRetry?: () => void }) => { + platformErrorScreens.screens.push(props); + return null; + }, +})); vi.mock('@/components/screen-header', () => ({ ScreenHeader: () => null })); vi.mock('@/components/ui/configure-row', () => ({ ConfigureRow: (props: { title: string; subtitle?: string; onPress?: () => void }) => { @@ -130,6 +145,15 @@ function findSwitch(root: I): I { return n; } +function hasSwitch(root: I): boolean { + return ( + root.findAll(n => typeof n.type === 'string' && (n.type as string) === 'Switch').length > 0 + ); +} + +const denialCopy = + 'Security Agent is disabled. Only organization owners and billing managers can turn it on.'; + function renderedTexts(root: I): string[] { return root .findAll( @@ -141,20 +165,24 @@ function renderedTexts(root: I): string[] { .map(n => n.props.children as string); } +function resetMocks() { + config.data = null; + config.isLoading = false; + config.isError = false; + config.fetchStatus = 'idle'; + capability.canManage = true; + capability.status = 'allowed'; + capability.isError = false; + committedConnectivity.status = 'online'; + repositories.data = []; + repositories.isLoading = false; + repositories.isError = false; + configureRows.rows = []; + platformErrorScreens.screens = []; +} + describe('SettingsOverviewScreen disabled switch', () => { - beforeEach(() => { - config.data = null; - config.isLoading = false; - config.isError = false; - capability.canManage = true; - repositories.data = []; - repositories.isLoading = false; - repositories.isError = false; - setEnabled.isPending = false; - setEnabled.mutate.mockClear(); - trackInteraction.mutate.mockClear(); - configureRows.rows = []; - }); + beforeEach(resetMocks); it('disables the switch while disabled with no effective repo selection', () => { config.data = disabledConfig(); @@ -182,19 +210,7 @@ describe('SettingsOverviewScreen disabled switch', () => { }); describe('SettingsOverviewScreen disabled copy and CTA', () => { - beforeEach(() => { - config.data = null; - config.isLoading = false; - config.isError = false; - capability.canManage = true; - repositories.data = []; - repositories.isLoading = false; - repositories.isError = false; - setEnabled.isPending = false; - setEnabled.mutate.mockClear(); - trackInteraction.mutate.mockClear(); - configureRows.rows = []; - }); + beforeEach(resetMocks); it('shows the empty-selection copy while disabled with no effective repo', () => { config.data = disabledConfig(); @@ -227,6 +243,7 @@ describe('SettingsOverviewScreen disabled copy and CTA', () => { it('does not offer the CTA to a non-manager', () => { capability.canManage = false; + capability.status = 'denied'; config.data = disabledConfig(); repositories.data = [{ id: 1 }]; renderScreen(); @@ -236,19 +253,7 @@ describe('SettingsOverviewScreen disabled copy and CTA', () => { }); describe('SettingsOverviewScreen repository query loading and error', () => { - beforeEach(() => { - config.data = null; - config.isLoading = false; - config.isError = false; - capability.canManage = true; - repositories.data = []; - repositories.isLoading = false; - repositories.isError = false; - setEnabled.isPending = false; - setEnabled.mutate.mockClear(); - trackInteraction.mutate.mockClear(); - configureRows.rows = []; - }); + beforeEach(resetMocks); it('does not read a loading repo query as empty in all mode', () => { config.data = disabledConfig({ repositorySelectionMode: 'all' }); @@ -280,3 +285,74 @@ describe('SettingsOverviewScreen repository query loading and error', () => { expect(configureRows.rows.map(r => r.title)).toContain('Select repositories'); }); }); + +describe('SettingsOverviewScreen capability and connectivity states', () => { + beforeEach(resetMocks); + + it('renders the skeleton while the capability is loading', () => { + config.data = disabledConfig(); + capability.status = 'loading'; + const root = renderScreen(); + + expect(platformErrorScreens.screens).toEqual([]); + expect(hasSwitch(root.root)).toBe(false); + expect(renderedTexts(root.root)).not.toContain(denialCopy); + }); + + it('shows the permissions error with retry and no denial copy when capability errors', () => { + config.data = disabledConfig(); + capability.status = 'error'; + const root = renderScreen(); + + expect(platformErrorScreens.screens[0]?.message).toBe('Could not load permissions'); + expect(platformErrorScreens.screens[0]?.onRetry).toBeTypeOf('function'); + expect(renderedTexts(root.root)).not.toContain(denialCopy); + }); + + it('keeps the resolved branch when a background refetch fails with a settled role', () => { + config.data = disabledConfig(); + capability.isError = true; + const root = renderScreen(); + + expect(platformErrorScreens.screens).toEqual([]); + expect(hasSwitch(root.root)).toBe(true); + }); + + it('shows denial copy and hides the switch when denied', () => { + config.data = disabledConfig(); + capability.status = 'denied'; + capability.canManage = false; + const root = renderScreen(); + + expect(hasSwitch(root.root)).toBe(false); + expect(renderedTexts(root.root)).toContain(denialCopy); + }); + + it('shows the switch when allowed', () => { + config.data = disabledConfig(); + const root = renderScreen(); + + expect(hasSwitch(root.root)).toBe(true); + }); + + it('shows the offline variant when paused with committed offline', () => { + config.data = null; + config.fetchStatus = 'paused'; + committedConnectivity.status = 'offline'; + const root = renderScreen(); + + expect(platformErrorScreens.screens[0]?.variant).toBe('offline'); + expect(platformErrorScreens.screens[0]?.message).toBe('Could not load Security Agent settings'); + expect(hasSwitch(root.root)).toBe(false); + }); + + it('renders the skeleton when paused but connectivity is unknown', () => { + config.data = null; + config.fetchStatus = 'paused'; + committedConnectivity.status = 'unknown'; + const root = renderScreen(); + + expect(platformErrorScreens.screens).toEqual([]); + expect(hasSwitch(root.root)).toBe(false); + }); +}); diff --git a/apps/mobile/src/components/security-agent/settings-overview-screen.tsx b/apps/mobile/src/components/security-agent/settings-overview-screen.tsx index 0a88a63d60..49f7e5c20e 100644 --- a/apps/mobile/src/components/security-agent/settings-overview-screen.tsx +++ b/apps/mobile/src/components/security-agent/settings-overview-screen.tsx @@ -18,6 +18,7 @@ import { useSetSecurityAgentEnabled, useTrackSecurityAgentInteraction, } from '@/lib/hooks/use-security-agent'; +import { useCommittedConnectivityStatus } from '@/lib/hooks/use-offline-banner-state'; import { getSecurityAgentPath } from '@/lib/security-agent'; import { capitalize } from '@/lib/utils'; @@ -52,7 +53,8 @@ export function SettingsOverviewScreen({ }: Readonly<{ scope: string; presentation?: SettingsOverviewPresentation }>) { const router = useRouter(); const config = useSecurityAgentConfig(scope); - const canManage = useSecurityAgentCapability(scope).canManage; + const capability = useSecurityAgentCapability(scope); + const committedConnectivity = useCommittedConnectivityStatus(); const setEnabled = useSetSecurityAgentEnabled(scope); const trackInteraction = useTrackSecurityAgentInteraction(scope); const repositories = useSecurityAgentRepositories(scope); @@ -82,7 +84,26 @@ export function SettingsOverviewScreen({ /> ); } - if (config.isLoading || !config.data) { + if (!config.data && config.fetchStatus === 'paused' && committedConnectivity === 'offline') { + return ( + void config.refetch()} + /> + ); + } + if (capability.status === 'error') { + return ( + void capability.refetch()} + /> + ); + } + if (config.isLoading || !config.data || capability.status === 'loading') { return ; } @@ -105,7 +126,8 @@ export function SettingsOverviewScreen({ // offer a direct path to the repo picker. Hide the CTA only when the repo set // is settled and empty (nothing to select); a loading or failed repo query // keeps the CTA reachable. - const showRepoCta = !data.isEnabled && canManage && !hasEffectiveRepo && !repositoriesEmpty; + const showRepoCta = + !data.isEnabled && capability.canManage && !hasEffectiveRepo && !repositoriesEmpty; const repoCountLabel = data.repositorySelectionMode === 'all' ? 'All repositories' @@ -155,7 +177,7 @@ export function SettingsOverviewScreen({ // connected-but-disabled counterpart: settings-overview-screen is where // scope-entry redirects once the agent is disabled, so the same action // needs to be reachable here too. - const auditAction = canManage ? : null; + const auditAction = capability.canManage ? : null; // Render the disabled-agent copy in three states: a still-loading repo set // (skeleton), a failed repo set (error + Retry), or a settled set (copy). @@ -180,7 +202,7 @@ export function SettingsOverviewScreen({ } return ( - {getDisabledCopy(canManage, hasEffectiveRepo)} + {getDisabledCopy(capability.canManage, hasEffectiveRepo)} ); }; @@ -196,7 +218,7 @@ export function SettingsOverviewScreen({ {data.isEnabled ? repoCountLabel : 'Disabled'} - {canManage ? ( + {capability.canManage ? ( ({ addEventListener: netinfo.addEventListener, })); -function Probe() { - const isOffline = useOfflineBannerState(); - return createElement('ProbeText', null, String(isOffline)); +type Hooks = { + useOfflineBannerState: () => boolean; + useCommittedConnectivityStatus: () => BannerState; +}; + +async function loadHooks(): Promise { + const hooks = await import('@/lib/hooks/use-offline-banner-state'); + return hooks; +} + +function Probe({ hooks }: { hooks: Hooks }) { + const isOffline = hooks.useOfflineBannerState(); + const status = hooks.useCommittedConnectivityStatus(); + return createElement('ProbeText', null, `${String(isOffline)}:${status}`); } function textChildren(renderer: TestRenderer.ReactTestRenderer): string[] | null { @@ -46,13 +56,13 @@ function textChildren(renderer: TestRenderer.ReactTestRenderer): string[] | null return json.children?.filter((child): child is string => typeof child === 'string') ?? null; } -async function renderProbe(): Promise { +async function renderProbe(hooks: Hooks): Promise { const rendererRef: { current: TestRenderer.ReactTestRenderer | undefined } = { current: undefined, }; await act(async () => { await Promise.resolve(); - rendererRef.current = TestRenderer.create(createElement(Probe)); + rendererRef.current = TestRenderer.create(createElement(Probe, { hooks })); }); const renderer = rendererRef.current; if (!renderer) { @@ -61,57 +71,91 @@ async function renderProbe(): Promise { return renderer; } -describe('useOfflineBannerState mounted', () => { +describe('useOfflineBannerState and useCommittedConnectivityStatus mounted', () => { beforeEach(() => { // React 19 requires the act environment flag before `act` supports // updates scheduled from effects and external stores. (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + // A fresh module per test gives a fresh module-level store singleton. + vi.resetModules(); + netinfo.listeners.clear(); }); afterEach(() => { netinfo.listeners.clear(); vi.useRealTimers(); }); - it('subscribes once, shows after the delay, hides at once, and cleans up on unmount', async () => { + it('subscribes once, shows after the delay, hides at once, and reports the tri-state', async () => { vi.useFakeTimers(); - const renderer = await renderProbe(); + const hooks = await loadHooks(); + const renderer = await renderProbe(hooks); expect(netinfo.listeners.size).toBe(1); - expect(textChildren(renderer)).toEqual(['false']); + expect(textChildren(renderer)).toEqual(['false:unknown']); act(() => { netinfo.emit({ isConnected: false, isInternetReachable: false }); }); - expect(textChildren(renderer)).toEqual(['false']); + expect(textChildren(renderer)).toEqual(['false:unknown']); act(() => { vi.advanceTimersByTime(OFFLINE_BANNER_SHOW_DELAY_MS); }); - expect(textChildren(renderer)).toEqual(['true']); + expect(textChildren(renderer)).toEqual(['true:offline']); act(() => { netinfo.emit({ isConnected: true, isInternetReachable: true }); }); - expect(textChildren(renderer)).toEqual(['false']); - - const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => undefined); - try { - act(() => { - renderer.unmount(); - }); - - expect(netinfo.listeners.size).toBe(0); - - act(() => { - netinfo.emit({ isConnected: false, isInternetReachable: false }); - vi.advanceTimersByTime(OFFLINE_BANNER_SHOW_DELAY_MS); - }); - - expect(renderer.toJSON()).toBeNull(); - expect(consoleErrorSpy).not.toHaveBeenCalled(); - } finally { - consoleErrorSpy.mockRestore(); - } + expect(textChildren(renderer)).toEqual(['false:online']); + }); + + it('reports the unknown → online edge even though the banner stays hidden', async () => { + vi.useFakeTimers(); + + const hooks = await loadHooks(); + const renderer = await renderProbe(hooks); + + expect(textChildren(renderer)).toEqual(['false:unknown']); + + act(() => { + netinfo.emit({ isConnected: true, isInternetReachable: true }); + }); + expect(textChildren(renderer)).toEqual(['false:online']); + }); + + it('shares one store and one NetInfo subscription across callers', async () => { + vi.useFakeTimers(); + + const hooks = await loadHooks(); + const first = await renderProbe(hooks); + const second = await renderProbe(hooks); + + expect(netinfo.listeners.size).toBe(1); + + act(() => { + netinfo.emit({ isConnected: false, isInternetReachable: false }); + }); + act(() => { + vi.advanceTimersByTime(OFFLINE_BANNER_SHOW_DELAY_MS); + }); + + expect(textChildren(first)).toEqual(['true:offline']); + expect(textChildren(second)).toEqual(['true:offline']); + }); + + it('keeps the shared store alive after unmount (never destroyed)', async () => { + vi.useFakeTimers(); + + const hooks = await loadHooks(); + const renderer = await renderProbe(hooks); + + expect(netinfo.listeners.size).toBe(1); + + act(() => { + renderer.unmount(); + }); + + expect(netinfo.listeners.size).toBe(1); }); }); diff --git a/apps/mobile/src/lib/hooks/use-offline-banner-state.ts b/apps/mobile/src/lib/hooks/use-offline-banner-state.ts index 4ba615ac16..d83a9626ee 100644 --- a/apps/mobile/src/lib/hooks/use-offline-banner-state.ts +++ b/apps/mobile/src/lib/hooks/use-offline-banner-state.ts @@ -1,17 +1,14 @@ import { addEventListener } from '@react-native-community/netinfo'; -import { useCallback, useEffect, useState, useSyncExternalStore } from 'react'; +import { useSyncExternalStore } from 'react'; import { + type BannerState, type ConnectivitySource, createOfflineBannerStore, type OfflineBannerStore, type OfflineBannerTimer, } from '@/lib/offline-banner-state'; -const NOOP_SUBSCRIBE = (): (() => void) => () => { - // No store on the first render; the effect creates one and destroys it on unmount. -}; - const netInfoSource: ConnectivitySource = { subscribe: listener => addEventListener(listener), }; @@ -27,15 +24,20 @@ const defaultTimer: OfflineBannerTimer = { }, }; +// One store per app, created lazily on first use and never destroyed, so +// every caller of the two hooks below shares a single NetInfo subscription +// (the app must not grow a second connectivity system). +let store: OfflineBannerStore | null = null; + +function getStore(): OfflineBannerStore { + store ??= createOfflineBannerStore({ source: netInfoSource, timer: defaultTimer }); + return store; +} + export function useOfflineBannerState(): boolean { - const [store, setStore] = useState(null); - useEffect(() => { - const created = createOfflineBannerStore({ source: netInfoSource, timer: defaultTimer }); - setStore(created); - return () => { - created.destroy(); - }; - }, []); - const getSnapshot = useCallback(() => store?.isOffline() ?? false, [store]); - return useSyncExternalStore(store?.subscribe ?? NOOP_SUBSCRIBE, getSnapshot); + return useSyncExternalStore(getStore().subscribe, getStore().isOffline); +} + +export function useCommittedConnectivityStatus(): BannerState { + return useSyncExternalStore(getStore().subscribe, getStore().state); } diff --git a/apps/mobile/src/lib/hooks/use-security-agent.test.ts b/apps/mobile/src/lib/hooks/use-security-agent.test.ts new file mode 100644 index 0000000000..7f2e2f8245 --- /dev/null +++ b/apps/mobile/src/lib/hooks/use-security-agent.test.ts @@ -0,0 +1,126 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { useSecurityAgentCapability } from './use-security-agent'; + +// use-security-agent.ts re-exports the mutation hooks from +// use-security-agent-mutations.ts; stub that module so loading this file in +// node does not pull in the mutation graph (expo-router, outbox, etc.). +vi.mock('@/lib/hooks/use-security-agent-mutations', () => ({ + useSaveSecurityAgentConfig: () => ({}), + useSetSecurityAgentEnabled: () => ({}), + useTrackSecurityAgentInteraction: () => ({}), + useTriggerSecuritySync: () => ({}), +})); + +type OrgListEntry = { organizationId: string; role: string }; + +const queryState = vi.hoisted(() => ({ + data: undefined as OrgListEntry[] | undefined, + isLoading: false, + isError: false, + isFetching: false, + isPending: false, + refetch: vi.fn(), +})); + +vi.mock('@tanstack/react-query', () => ({ + useQuery: () => queryState, +})); + +vi.mock('@/lib/trpc', () => ({ + useTRPC: () => ({ + organizations: { + list: { queryOptions: () => ({}) }, + }, + }), +})); + +function capabilityFor(scope: string) { + // eslint-disable-next-line react-hooks/rules-of-hooks -- the mocked hooks have no React state; this mirrors use-code-reviewer.test.ts + return useSecurityAgentCapability(scope); +} + +beforeEach(() => { + queryState.data = undefined; + queryState.isLoading = false; + queryState.isError = false; + queryState.isFetching = false; + queryState.isPending = false; + queryState.refetch.mockReset(); +}); + +afterEach(() => { + vi.clearAllMocks(); +}); + +describe('useSecurityAgentCapability status derivation', () => { + it('returns allowed for the personal scope', () => { + const capability = capabilityFor('personal'); + + expect(capability.status).toBe('allowed'); + expect(capability.canManage).toBe(true); + }); + + it('returns allowed for an org with a settled owner role', () => { + queryState.data = [{ organizationId: 'org_123', role: 'owner' }]; + const capability = capabilityFor('org_123'); + + expect(capability.status).toBe('allowed'); + expect(capability.canManage).toBe(true); + }); + + it('returns denied for an org with a settled member role', () => { + queryState.data = [{ organizationId: 'org_123', role: 'member' }]; + const capability = capabilityFor('org_123'); + + expect(capability.status).toBe('denied'); + expect(capability.canManage).toBe(false); + }); + + it('returns error when the org role query fails with no data', () => { + queryState.data = undefined; + queryState.isError = true; + queryState.isPending = false; + const capability = capabilityFor('org_123'); + + expect(capability.status).toBe('error'); + }); + + it('returns loading when the org role query is pending with no data', () => { + queryState.data = undefined; + queryState.isError = false; + queryState.isPending = true; + const capability = capabilityFor('org_123'); + + expect(capability.status).toBe('loading'); + }); + + it('falls back to loading for any other unresolved combination', () => { + queryState.data = undefined; + queryState.isError = false; + queryState.isPending = false; + const capability = capabilityFor('org_123'); + + expect(capability.status).toBe('loading'); + }); + + it('keeps a settled owner role authoritative when a background refetch fails', () => { + queryState.data = [{ organizationId: 'org_123', role: 'owner' }]; + queryState.isError = true; + queryState.isPending = false; + const capability = capabilityFor('org_123'); + + expect(capability.status).toBe('allowed'); + expect(capability.canManage).toBe(true); + }); + + it('keeps a settled member role denied when a background refetch fails', () => { + queryState.data = [{ organizationId: 'org_123', role: 'member' }]; + queryState.isError = true; + queryState.isPending = false; + const capability = capabilityFor('org_123'); + + expect(capability.status).toBe('denied'); + expect(capability.canManage).toBe(false); + }); +}); diff --git a/apps/mobile/src/lib/hooks/use-security-agent.ts b/apps/mobile/src/lib/hooks/use-security-agent.ts index b92daaf3d1..2dad6cf760 100644 --- a/apps/mobile/src/lib/hooks/use-security-agent.ts +++ b/apps/mobile/src/lib/hooks/use-security-agent.ts @@ -125,6 +125,8 @@ function useSecurityAgentOrgRoleQuery(scope: string) { isError: false, isFetching: false, refetch: query.refetch, + hasData: true, + isPending: false, }; } return { @@ -133,16 +135,39 @@ function useSecurityAgentOrgRoleQuery(scope: string) { isError: query.isError, isFetching: query.isFetching, refetch: query.refetch, + hasData: query.data !== undefined, + isPending: query.isPending, }; } // Discriminated capability state for consumers (e.g. audit-report access) // that must distinguish "still loading"/"failed to load" from "resolved: // no access" instead of treating an undefined role as permission-denied. +export type SecurityAgentCapabilityStatus = 'loading' | 'error' | 'denied' | 'allowed'; + export function useSecurityAgentCapability(scope: string) { - const { role, isLoading, isError, isFetching, refetch } = useSecurityAgentOrgRoleQuery(scope); + const { role, isLoading, isError, isFetching, refetch, hasData, isPending } = + useSecurityAgentOrgRoleQuery(scope); + const canManage = canManageSecurityAgent(scope, role); + + let status: SecurityAgentCapabilityStatus = 'loading'; + if (isPersonalSecurityScope(scope)) { + status = 'allowed'; + } else if (hasData) { + // A settled role stays authoritative: a failed background refetch must + // never demote an already-resolved capability to 'error'. + status = canManage ? 'allowed' : 'denied'; + } else if (isError) { + status = 'error'; + } else if (isPending) { + // Covers an offline-paused cold launch: pending with no data yet. + status = 'loading'; + } + // Any other unresolved combination keeps the initial 'loading'. + return { - canManage: canManageSecurityAgent(scope, role), + canManage, + status, isLoading, isError, isFetching, diff --git a/apps/mobile/src/lib/offline-banner-state.test.ts b/apps/mobile/src/lib/offline-banner-state.test.ts index 2850d9148b..a55cfd4fa5 100644 --- a/apps/mobile/src/lib/offline-banner-state.test.ts +++ b/apps/mobile/src/lib/offline-banner-state.test.ts @@ -90,6 +90,7 @@ describe('createOfflineBannerStore', () => { const { store } = createStore(); expect(store.isOffline()).toBe(false); + expect(store.state()).toBe('unknown'); }); it('does not show the offline banner while connectivity is unknown', () => { @@ -104,7 +105,7 @@ describe('createOfflineBannerStore', () => { expect(listener).not.toHaveBeenCalled(); }); - it('does not notify on unknown → online (the banner stays hidden)', () => { + it('notifies on unknown → online (the banner stays hidden)', () => { const { store, source } = createStore(); const listener = vi.fn(() => undefined); store.subscribe(listener); @@ -113,7 +114,8 @@ describe('createOfflineBannerStore', () => { source.emit(onlineState); expect(store.isOffline()).toBe(false); - expect(listener).not.toHaveBeenCalled(); + expect(store.state()).toBe('online'); + expect(listener).toHaveBeenCalledTimes(1); }); it('commits offline only after the show delay and notifies once', () => { @@ -134,7 +136,7 @@ describe('createOfflineBannerStore', () => { expect(listener).toHaveBeenCalledTimes(1); }); - it('never commits when the state returns online inside the window', () => { + it('cancels the pending offline commit when the state returns online inside the window', () => { const { store, source, timer } = createStore(); const listener = vi.fn(() => undefined); store.subscribe(listener); @@ -145,7 +147,8 @@ describe('createOfflineBannerStore', () => { timer.firePending(); expect(store.isOffline()).toBe(false); - expect(listener).not.toHaveBeenCalled(); + expect(store.state()).toBe('online'); + expect(listener).toHaveBeenCalledTimes(1); }); it('hides immediately when the connection returns after a committed offline', () => { @@ -179,7 +182,9 @@ describe('createOfflineBannerStore', () => { timer.firePending(); expect(store.isOffline()).toBe(true); - expect(listener).toHaveBeenCalledTimes(1); + // One notification for the unknown → online commit, one for the final + // offline commit — the intermediate online commits are no-ops. + expect(listener).toHaveBeenCalledTimes(2); }); it('destroy with a pending commit cancels the timer and unsubscribes the source', () => { @@ -221,4 +226,17 @@ describe('createOfflineBannerStore', () => { expect(listener).not.toHaveBeenCalled(); }); + + it('exposes the committed state via state()', () => { + const { store, source, timer } = createStore(); + + expect(store.state()).toBe('unknown'); + + source.emit(onlineState); + expect(store.state()).toBe('online'); + + source.emit(offlineState); + timer.firePending(); + expect(store.state()).toBe('offline'); + }); }); diff --git a/apps/mobile/src/lib/offline-banner-state.ts b/apps/mobile/src/lib/offline-banner-state.ts index 6ad421c03c..2ec57ee773 100644 --- a/apps/mobile/src/lib/offline-banner-state.ts +++ b/apps/mobile/src/lib/offline-banner-state.ts @@ -19,11 +19,12 @@ export type ConnectivitySource = { export type OfflineBannerStore = { subscribe: (listener: () => void) => () => void; isOffline: () => boolean; + state: () => BannerState; destroy: () => void; }; /** The banner's committed connectivity state. */ -type BannerState = 'online' | 'offline' | 'unknown'; +export type BannerState = 'online' | 'offline' | 'unknown'; export function createOfflineBannerStore(options: { source: ConnectivitySource; @@ -45,14 +46,15 @@ export function createOfflineBannerStore(options: { } function commit(next: BannerState): void { - const wasOffline = state === 'offline'; + if (state === next) { + return; + } state = next; - // Notify only when the observable `isOffline` value changes; an - // unknown → online transition leaves it false and must not re-render. - if (wasOffline !== (next === 'offline')) { - for (const listener of listeners) { - listener(); - } + // Notify on every committed state change. The banner's `getSnapshot` + // (`isOffline`) is unchanged on an unknown → online edge, so it does not + // re-render there; the tri-state hook's `getSnapshot` (`state`) does. + for (const listener of listeners) { + listener(); } } @@ -87,11 +89,13 @@ export function createOfflineBannerStore(options: { const isOffline = (): boolean => state === 'offline'; + const getState = (): BannerState => state; + const destroy = (): void => { cancelPending(); unsubscribeSource(); listeners.clear(); }; - return { subscribe, isOffline, destroy }; + return { subscribe, isOffline, state: getState, destroy }; } From ba4d54126b0a9499becb2675f7b20cf1970987e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Fri, 21 Aug 2026 04:50:11 +0200 Subject: [PATCH 04/14] fix(mobile): guard optimistic rollback by mutation generation Add a per-key generation guard so a failing older mutation cannot roll back cache state a newer mutation owns, and serialize same-entity mutations with scope ids for security, org, and model-preferences hooks. --- .../lib/hooks/mutation-generations.test.ts | 30 +++ .../src/lib/hooks/mutation-generations.ts | 27 +++ .../lib/hooks/use-model-preferences.test.ts | 158 +++++++++++++ .../src/lib/hooks/use-model-preferences.ts | 23 +- .../hooks/use-organization-mutations.test.ts | 218 ++++++++++++++++++ .../lib/hooks/use-organization-mutations.ts | 48 +++- .../use-security-agent-mutations.test.ts | 144 +++++++++++- .../lib/hooks/use-security-agent-mutations.ts | 39 +++- .../hooks/use-security-remediation.test.ts | 189 +++++++++++++++ .../src/lib/hooks/use-security-remediation.ts | 16 +- 10 files changed, 864 insertions(+), 28 deletions(-) create mode 100644 apps/mobile/src/lib/hooks/mutation-generations.test.ts create mode 100644 apps/mobile/src/lib/hooks/mutation-generations.ts create mode 100644 apps/mobile/src/lib/hooks/use-model-preferences.test.ts create mode 100644 apps/mobile/src/lib/hooks/use-organization-mutations.test.ts create mode 100644 apps/mobile/src/lib/hooks/use-security-remediation.test.ts diff --git a/apps/mobile/src/lib/hooks/mutation-generations.test.ts b/apps/mobile/src/lib/hooks/mutation-generations.test.ts new file mode 100644 index 0000000000..2a59981c10 --- /dev/null +++ b/apps/mobile/src/lib/hooks/mutation-generations.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, it } from 'vitest'; + +import { isLatestMutationGeneration, nextMutationGeneration } from './mutation-generations'; + +describe('mutation-generations', () => { + it('increments per key and returns the new generation', () => { + expect(nextMutationGeneration('a')).toBe(1); + expect(nextMutationGeneration('a')).toBe(2); + expect(nextMutationGeneration('b')).toBe(1); + }); + + it('tracks keys independently', () => { + nextMutationGeneration('x'); + nextMutationGeneration('y'); + nextMutationGeneration('y'); + expect(isLatestMutationGeneration('x', 1)).toBe(true); + expect(isLatestMutationGeneration('y', 2)).toBe(true); + }); + + it('isLatestMutationGeneration is true only for the latest generation', () => { + const first = nextMutationGeneration('key'); + const second = nextMutationGeneration('key'); + expect(isLatestMutationGeneration('key', second)).toBe(true); + expect(isLatestMutationGeneration('key', first)).toBe(false); + }); + + it('returns false for a key with no generation yet', () => { + expect(isLatestMutationGeneration('untouched', 1)).toBe(false); + }); +}); diff --git a/apps/mobile/src/lib/hooks/mutation-generations.ts b/apps/mobile/src/lib/hooks/mutation-generations.ts new file mode 100644 index 0000000000..9fabaa506e --- /dev/null +++ b/apps/mobile/src/lib/hooks/mutation-generations.ts @@ -0,0 +1,27 @@ +// Pure per-key generation counter, kept dependency-free so it can be +// vitest'd in node env without pulling in react-native transitively +// (same reasoning as save-chain.ts). + +const generations = new Map(); + +/** + * Increments and returns the per-key generation counter. Call this in a + * mutation's `onMutate` to stamp the optimistic write it is about to make. + */ +export function nextMutationGeneration(key: string): number { + const next = (generations.get(key) ?? 0) + 1; + generations.set(key, next); + return next; +} + +/** + * True when the per-key counter still equals `generation`, i.e. no newer + * mutation has stamped the same cache since. An older mutation's failure + * must not roll back cache state a newer mutation already owns; the newer + * mutation's settle-time invalidation reconciles with server truth. + * Accepted residual: when both fail, the older optimistic value can show + * until the settle invalidation refetches. + */ +export function isLatestMutationGeneration(key: string, generation: number): boolean { + return generations.get(key) === generation; +} diff --git a/apps/mobile/src/lib/hooks/use-model-preferences.test.ts b/apps/mobile/src/lib/hooks/use-model-preferences.test.ts new file mode 100644 index 0000000000..f941272bc1 --- /dev/null +++ b/apps/mobile/src/lib/hooks/use-model-preferences.test.ts @@ -0,0 +1,158 @@ +/* eslint-disable require-await, @typescript-eslint/require-await -- the fake chainSave factories settle without await because they resolve immediately */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import type * as React from 'react'; + +import { useModelPreferences } from './use-model-preferences'; + +type MutationOptions = { + mutationFn?: (vars: unknown) => Promise; + onMutate?: (vars: unknown) => Promise | unknown; + onError?: (error: unknown, vars: unknown, context: unknown) => void; + onSuccess?: (result: unknown, vars: unknown) => void; + onSettled?: (data?: unknown, error?: unknown, vars?: unknown) => Promise | void; + scope?: { id: string }; +}; + +// useModelPreferences registers its mutations in this order: +// setLastSelected, clearLastSelected, addFavorite, removeFavorite, setFavorites. +const capturedMutations: (MutationOptions | null)[] = []; +const invalidateQueriesMock = vi.fn(); +const getQueryDataMock = vi.fn(); +const setQueryDataMock = vi.fn(); +const cancelQueriesMock = vi.fn(); +const toastErrorMock = vi.fn(); +const setFavoritesErrorMock = vi.hoisted(() => vi.fn()); +const chainSaveMock = vi.hoisted(() => + vi.fn(async (_key: string, op: () => Promise) => op()) +); + +vi.mock('react', async () => { + const actual = await vi.importActual('react'); + return { + ...actual, + useState: vi.fn((initial: unknown) => [initial, setFavoritesErrorMock] as const), + useCallback: vi.fn( unknown>(fn: T) => fn), + useMemo: vi.fn((fn: () => T) => fn()), + }; +}); + +vi.mock('@tanstack/react-query', () => ({ + useMutation: (opts: MutationOptions) => { + capturedMutations.push(opts); + return { mutate: vi.fn(), mutateAsync: vi.fn(), isPending: false, isError: false }; + }, + useQuery: () => ({ data: { favorites: [], lastSelected: null }, isLoading: false }), + useQueryClient: () => ({ + invalidateQueries: (...args: unknown[]) => { + invalidateQueriesMock(...args); + }, + getQueryData: (...args: unknown[]) => getQueryDataMock(...args), + setQueryData: (...args: unknown[]) => setQueryDataMock(...args), + cancelQueries: (...args: unknown[]) => cancelQueriesMock(...args), + }), + hashKey: (key: unknown) => JSON.stringify(key), +})); + +vi.mock('sonner-native', () => ({ + toast: { error: (msg: string) => toastErrorMock(msg) }, +})); + +vi.mock('@/lib/hooks/save-chain', () => ({ + chainSave: async (key: string, op: () => Promise) => chainSaveMock(key, op), +})); + +vi.mock('@/lib/trpc', () => ({ + useTRPC: () => ({ + modelPreferences: { + get: { + queryOptions: () => ({ queryKey: ['modelPreferences', 'get'], queryFn: () => undefined }), + queryKey: (input: unknown) => ['modelPreferences', 'get', input], + }, + setLastSelected: { mutationOptions: (opts: MutationOptions) => opts }, + clearLastSelected: { mutationOptions: (opts: MutationOptions) => opts }, + setFavorites: { mutationOptions: (opts: MutationOptions) => opts }, + }, + }), + trpcClient: { + modelPreferences: { + addFavorite: { mutate: vi.fn() }, + removeFavorite: { mutate: vi.fn() }, + }, + }, +})); + +describe('useModelPreferences (generation guard)', () => { + beforeEach(() => { + capturedMutations.length = 0; + invalidateQueriesMock.mockReset(); + getQueryDataMock.mockReset(); + setQueryDataMock.mockReset(); + cancelQueriesMock.mockReset(); + toastErrorMock.mockReset(); + setFavoritesErrorMock.mockReset(); + chainSaveMock.mockClear(); + chainSaveMock.mockImplementation(async (_key, op) => op()); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + it('serializes favorite mutations through chainSave (rule 1) and adds no scope', async () => { + useModelPreferences(undefined); + const addFavorite = capturedMutations[2]; + const removeFavorite = capturedMutations[3]; + + await addFavorite?.mutationFn?.({ model: 'm1' }); + expect(chainSaveMock).toHaveBeenCalledWith('model-preferences-favorites', expect.any(Function)); + expect(addFavorite?.scope).toBeUndefined(); + expect(removeFavorite?.scope).toBeUndefined(); + }); + + it('a failing older addFavorite does not roll back while a newer one owns the cache', async () => { + getQueryDataMock.mockReturnValue({ favorites: [], lastSelected: null }); + useModelPreferences(undefined); + const addFavorite = capturedMutations[2]; + const older = await addFavorite?.onMutate?.({ model: 'm1' }); + const newer = await addFavorite?.onMutate?.({ model: 'm2' }); + + setQueryDataMock.mockClear(); + addFavorite?.onError?.(new Error('boom'), { model: 'm1' }, older); + expect(setQueryDataMock).not.toHaveBeenCalled(); + + addFavorite?.onError?.(new Error('boom'), { model: 'm2' }, newer); + expect(setQueryDataMock).toHaveBeenCalledTimes(1); + // Favorites surface the error inline (no toast). + expect(toastErrorMock).not.toHaveBeenCalled(); + expect(setFavoritesErrorMock).toHaveBeenCalledWith('boom'); + }); + + it('a failing latest removeFavorite rolls back its snapshot', async () => { + getQueryDataMock.mockReturnValue({ favorites: ['m1'], lastSelected: null }); + useModelPreferences(undefined); + const removeFavorite = capturedMutations[3]; + const context = await removeFavorite?.onMutate?.({ model: 'm1' }); + + setQueryDataMock.mockClear(); + removeFavorite?.onError?.(new Error('boom'), { model: 'm1' }, context); + expect(setQueryDataMock).toHaveBeenCalledTimes(1); + expect(toastErrorMock).not.toHaveBeenCalled(); + expect(setFavoritesErrorMock).toHaveBeenCalledWith('boom'); + }); + + it('surfaces the inline error even when the stale generation skips the rollback', async () => { + getQueryDataMock.mockReturnValue({ favorites: [], lastSelected: null }); + useModelPreferences(undefined); + const addFavorite = capturedMutations[2]; + const older = await addFavorite?.onMutate?.({ model: 'm1' }); + await addFavorite?.onMutate?.({ model: 'm2' }); + + setQueryDataMock.mockClear(); + setFavoritesErrorMock.mockClear(); + addFavorite?.onError?.(new Error('boom'), { model: 'm1' }, older); + expect(setQueryDataMock).not.toHaveBeenCalled(); + expect(setFavoritesErrorMock).toHaveBeenCalledWith('boom'); + }); +}); diff --git a/apps/mobile/src/lib/hooks/use-model-preferences.ts b/apps/mobile/src/lib/hooks/use-model-preferences.ts index fea113467f..ed7c55b07a 100644 --- a/apps/mobile/src/lib/hooks/use-model-preferences.ts +++ b/apps/mobile/src/lib/hooks/use-model-preferences.ts @@ -1,8 +1,12 @@ -import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query'; +import { hashKey, useMutation, useQuery, useQueryClient } from '@tanstack/react-query'; import { type inferRouterOutputs, type MobileRouter } from '@kilocode/trpc/mobile'; import { useCallback, useMemo, useState } from 'react'; import { toast } from 'sonner-native'; +import { + isLatestMutationGeneration, + nextMutationGeneration, +} from '@/lib/hooks/mutation-generations'; import { chainSave } from '@/lib/hooks/save-chain'; import { trpcClient, useTRPC } from '@/lib/trpc'; @@ -38,6 +42,7 @@ export function useModelPreferences(organizationId: string | undefined) { async (update: (favorites: string[]) => string[]) => { const queryKey = trpc.modelPreferences.get.queryKey(input); await queryClient.cancelQueries({ queryKey }); + const generation = nextMutationGeneration(hashKey(queryKey)); const previous = queryClient.getQueryData(queryKey); if (previous) { queryClient.setQueryData(queryKey, { @@ -45,15 +50,19 @@ export function useModelPreferences(organizationId: string | undefined) { favorites: update(previous.favorites), }); } - return { previous }; + return { previous, generation }; }, [queryClient, trpc.modelPreferences.get, input] ); const rollbackFavorites = useCallback( - (error: { message: string }, context: { previous?: ModelPreferences } | undefined) => { - if (context?.previous) { - queryClient.setQueryData(trpc.modelPreferences.get.queryKey(input), context.previous); + ( + error: { message: string }, + context: { previous?: ModelPreferences; generation: number } | undefined + ) => { + const queryKey = trpc.modelPreferences.get.queryKey(input); + if (context?.previous && isLatestMutationGeneration(hashKey(queryKey), context.generation)) { + queryClient.setQueryData(queryKey, context.previous); } setFavoritesError(error.message || 'Could not update favorites'); }, @@ -79,6 +88,8 @@ export function useModelPreferences(organizationId: string | undefined) { // out of order and the earlier response can stomp the later one's result. // Chaining onto the prior in-flight request keeps them in order — simple // FIFO, no dedupe/coalescing (see save-chain.ts). + // onError policy: roll back the onMutate snapshot (latest generation only); + // the caller renders the error inline (no toast). const addFavorite = useMutation({ mutationFn: (vars: { model: string }) => // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule @@ -97,6 +108,8 @@ export function useModelPreferences(organizationId: string | undefined) { }); const removeFavorite = useMutation({ + // onError policy: roll back the onMutate snapshot (latest generation only); + // the caller renders the error inline (no toast). mutationFn: (vars: { model: string }) => // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule chainSave(FAVORITES_CHAIN_KEY, () => trpcClient.modelPreferences.removeFavorite.mutate(vars)), diff --git a/apps/mobile/src/lib/hooks/use-organization-mutations.test.ts b/apps/mobile/src/lib/hooks/use-organization-mutations.test.ts new file mode 100644 index 0000000000..a1e4674325 --- /dev/null +++ b/apps/mobile/src/lib/hooks/use-organization-mutations.test.ts @@ -0,0 +1,218 @@ +/* eslint-disable require-await, @typescript-eslint/require-await -- the second mutationFn resolves immediately without awaiting */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import type * as ReactQuery from '@tanstack/react-query'; + +import { useOrganizationMutations } from './use-organization-mutations'; + +type MutationOptions = { + mutationFn?: (vars: unknown) => Promise; + onMutate?: (vars: unknown) => Promise | unknown; + onError?: (error: unknown, vars: unknown, context: unknown) => void; + onSuccess?: (result: unknown, vars: unknown) => void; + onSettled?: (data?: unknown, error?: unknown, vars?: unknown) => Promise | void; + scope?: { id: string }; +}; + +// useOrganizationMutations registers its mutations in this order: +// rename, invite, updateMember, removeMember, deleteInvite, updateMinimumBalanceAlert. +const capturedMutations: (MutationOptions | null)[] = []; +const invalidateQueriesMock = vi.fn(); +const getQueryDataMock = vi.fn(); +const setQueryDataMock = vi.fn(); +const cancelQueriesMock = vi.fn(); +const toastErrorMock = vi.fn(); + +vi.mock('@tanstack/react-query', () => ({ + useMutation: (opts: MutationOptions) => { + capturedMutations.push(opts); + return { mutate: vi.fn(), mutateAsync: vi.fn(), isPending: false, isError: false }; + }, + useQueryClient: () => ({ + invalidateQueries: (...args: unknown[]) => { + invalidateQueriesMock(...args); + }, + getQueryData: (...args: unknown[]) => getQueryDataMock(...args), + setQueryData: (...args: unknown[]) => setQueryDataMock(...args), + cancelQueries: (...args: unknown[]) => cancelQueriesMock(...args), + }), + hashKey: (key: unknown) => JSON.stringify(key), +})); + +vi.mock('@/lib/a11y/announcing-toast', () => ({ + announcingToast: { + error: (msg: string) => toastErrorMock(msg), + success: vi.fn(), + warning: vi.fn(), + }, +})); + +vi.mock('@/lib/trpc', () => ({ + useTRPC: () => ({ + organizations: { + withMembers: { + queryKey: ({ organizationId }: { organizationId: string }) => [ + 'organizations', + 'withMembers', + { organizationId }, + ], + }, + list: { queryKey: () => ['organizations', 'list'] }, + }, + }), + trpcClient: { + organizations: { + update: { mutate: vi.fn() }, + members: { + invite: { mutate: vi.fn() }, + update: { mutate: vi.fn() }, + remove: { mutate: vi.fn() }, + deleteInvite: { mutate: vi.fn() }, + }, + settings: { + updateMinimumBalanceAlert: { mutate: vi.fn() }, + }, + }, + }, +})); + +const ORG_ID = 'aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa'; + +describe('useOrganizationMutations (generation guard + scope)', () => { + beforeEach(() => { + capturedMutations.length = 0; + invalidateQueriesMock.mockReset(); + getQueryDataMock.mockReset(); + setQueryDataMock.mockReset(); + cancelQueriesMock.mockReset(); + toastErrorMock.mockReset(); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + it('scopes all five optimistic mutations and leaves invite unscoped', () => { + useOrganizationMutations(ORG_ID); + const expectedScope = { id: `organization:${ORG_ID}` }; + // rename + expect(capturedMutations[0]?.scope).toEqual(expectedScope); + // invite + expect(capturedMutations[1]?.scope).toBeUndefined(); + // updateMember + expect(capturedMutations[2]?.scope).toEqual(expectedScope); + // removeMember + expect(capturedMutations[3]?.scope).toEqual(expectedScope); + // deleteInvite + expect(capturedMutations[4]?.scope).toEqual(expectedScope); + // updateMinimumBalanceAlert + expect(capturedMutations[5]?.scope).toEqual(expectedScope); + }); + + it('a failing older updateMember does not roll back while a newer one owns the cache', async () => { + getQueryDataMock.mockReturnValue({ members: [] }); + useOrganizationMutations(ORG_ID); + const updateMember = capturedMutations[2]; + const older = await updateMember?.onMutate?.({ memberId: 'm1', role: 'admin' }); + const newer = await updateMember?.onMutate?.({ memberId: 'm2', role: 'member' }); + + setQueryDataMock.mockClear(); + updateMember?.onError?.(new Error('boom'), { memberId: 'm1', role: 'admin' }, older); + expect(setQueryDataMock).not.toHaveBeenCalled(); + + updateMember?.onError?.(new Error('boom'), { memberId: 'm2', role: 'member' }, newer); + expect(setQueryDataMock).toHaveBeenCalledTimes(1); + // updateMember toasts by default (not silent). + expect(toastErrorMock).toHaveBeenCalledTimes(2); + }); + + it('a failing latest updateMember rolls back its snapshot', async () => { + getQueryDataMock.mockReturnValue({ members: [] }); + useOrganizationMutations(ORG_ID); + const updateMember = capturedMutations[2]; + const context = await updateMember?.onMutate?.({ memberId: 'm1', role: 'admin' }); + + setQueryDataMock.mockClear(); + updateMember?.onError?.(new Error('boom'), { memberId: 'm1', role: 'admin' }, context); + expect(setQueryDataMock).toHaveBeenCalledTimes(1); + expect(toastErrorMock).toHaveBeenCalledWith('boom'); + }); + + it('updateMember stays inline (no toast) when silenceUpdateMemberToast is set', async () => { + getQueryDataMock.mockReturnValue({ members: [] }); + useOrganizationMutations(ORG_ID, { silenceUpdateMemberToast: true }); + const updateMember = capturedMutations[2]; + const context = await updateMember?.onMutate?.({ memberId: 'm1', role: 'admin' }); + + updateMember?.onError?.(new Error('boom'), { memberId: 'm1', role: 'admin' }, context); + expect(toastErrorMock).not.toHaveBeenCalled(); + }); + + it('rename: a failing older rename does not roll back while a newer rename owns the cache', async () => { + getQueryDataMock.mockReturnValue({ name: 'Old', members: [] }); + useOrganizationMutations(ORG_ID); + const rename = capturedMutations[0]; + const older = await rename?.onMutate?.({ name: 'A' }); + const newer = await rename?.onMutate?.({ name: 'B' }); + + setQueryDataMock.mockClear(); + rename?.onError?.(new Error('boom'), { name: 'A' }, older); + expect(setQueryDataMock).not.toHaveBeenCalled(); + + rename?.onError?.(new Error('boom'), { name: 'B' }, newer); + // rename rolls back both the withMembers and the list caches. + expect(setQueryDataMock).toHaveBeenCalledTimes(2); + // rename renders inline (no toast). + expect(toastErrorMock).not.toHaveBeenCalled(); + }); + + it('updateMinimumBalanceAlert renders inline (no toast) and rolls back the latest snapshot', async () => { + getQueryDataMock.mockReturnValue({ settings: {} }); + useOrganizationMutations(ORG_ID); + const updateMinimumBalanceAlert = capturedMutations[5]; + const context = await updateMinimumBalanceAlert?.onMutate?.({ enabled: true }); + + setQueryDataMock.mockClear(); + updateMinimumBalanceAlert?.onError?.(new Error('boom'), { enabled: true }, context); + expect(setQueryDataMock).toHaveBeenCalledTimes(1); + expect(toastErrorMock).not.toHaveBeenCalled(); + }); +}); + +describe('scope.id network serialization (real MutationCache)', () => { + it('starts the second same-scope org mutationFn only after the first settles', async () => { + const { MutationCache, QueryClient } = + await vi.importActual('@tanstack/react-query'); + const cache = new MutationCache(); + const client = new QueryClient({ mutationCache: cache }); + const order: string[] = []; + const gate = Promise.withResolvers(); + + const first = cache.build(client, { + mutationFn: async () => { + order.push('first-start'); + await gate.promise; + order.push('first-end'); + return 'first'; + }, + scope: { id: `organization:${ORG_ID}` }, + }); + const second = cache.build(client, { + mutationFn: async () => { + order.push('second-start'); + return 'second'; + }, + scope: { id: `organization:${ORG_ID}` }, + }); + + const p1 = first.execute({}); + const p2 = second.execute({}); + await Promise.resolve(); + await Promise.resolve(); + expect(order).toEqual(['first-start']); + + gate.resolve(null); + await Promise.all([p1, p2]); + expect(order).toEqual(['first-start', 'first-end', 'second-start']); + }); +}); diff --git a/apps/mobile/src/lib/hooks/use-organization-mutations.ts b/apps/mobile/src/lib/hooks/use-organization-mutations.ts index 2b6b9afa16..3e12692e9f 100644 --- a/apps/mobile/src/lib/hooks/use-organization-mutations.ts +++ b/apps/mobile/src/lib/hooks/use-organization-mutations.ts @@ -1,6 +1,10 @@ -import { useMutation, useQueryClient } from '@tanstack/react-query'; +import { hashKey, useMutation, useQueryClient } from '@tanstack/react-query'; import { announcingToast } from '@/lib/a11y/announcing-toast'; +import { + isLatestMutationGeneration, + nextMutationGeneration, +} from '@/lib/hooks/mutation-generations'; import { type OrgListEntry, type OrgRole, @@ -47,6 +51,8 @@ export function useOrganizationMutations( // Every optimistic mutation here only touches the withMembers cache, so the // key is fixed rather than threaded through like use-kiloclaw-mutations.ts // (which juggles many caches across a personal/org split). + // onError policy: roll back the onMutate snapshot (latest generation only); + // toast error.message, or the caller renders the error inline when silent. function optimistic( updater: (old: OrgWithMembers, input: TInput) => OrgWithMembers, { silent }: { silent?: boolean } = {} @@ -54,18 +60,22 @@ export function useOrganizationMutations( return { onMutate: async (input: TInput) => { await queryClient.cancelQueries({ queryKey: withMembersKey }); + const generation = nextMutationGeneration(hashKey(withMembersKey)); const previous = queryClient.getQueryData(withMembersKey); queryClient.setQueryData(withMembersKey, old => old ? updater(old, input) : old ); - return { previous }; + return { previous, generation }; }, onError: ( error: { message: string }, _input: TInput, - context?: { previous?: OrgWithMembers } + context?: { previous?: OrgWithMembers; generation: number } ) => { - if (context?.previous) { + if ( + context?.previous && + isLatestMutationGeneration(hashKey(withMembersKey), context.generation) + ) { queryClient.setQueryData(withMembersKey, context.previous); } if (!silent) { @@ -77,6 +87,8 @@ export function useOrganizationMutations( } return { + // onError policy: roll back the onMutate snapshot (latest generation only); + // the caller renders the error inline (no toast). rename: useMutation({ // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule mutationFn: (input: { name: string }) => @@ -86,6 +98,7 @@ export function useOrganizationMutations( queryClient.cancelQueries({ queryKey: withMembersKey }), queryClient.cancelQueries({ queryKey: listKey }), ]); + const generation = nextMutationGeneration(hashKey(withMembersKey)); const previousWithMembers = queryClient.getQueryData(withMembersKey); const previousList = queryClient.getQueryData(listKey); queryClient.setQueryData(withMembersKey, old => @@ -100,23 +113,36 @@ export function useOrganizationMutations( ) : old ); - return { previousWithMembers, previousList }; + return { previousWithMembers, previousList, generation }; }, // No onMutationError toast here: RenameModal (the only caller) shows // the error inline while it stays open (see Pattern P2). onError: ( _error: { message: string }, _input, - context?: { previousWithMembers?: OrgWithMembers; previousList?: OrgListEntry[] } + context?: { + previousWithMembers?: OrgWithMembers; + previousList?: OrgListEntry[]; + generation: number; + } ) => { - if (context?.previousWithMembers) { + if ( + context?.previousWithMembers && + isLatestMutationGeneration(hashKey(withMembersKey), context.generation) + ) { queryClient.setQueryData(withMembersKey, context.previousWithMembers); } - if (context?.previousList) { + if ( + context?.previousList && + isLatestMutationGeneration(hashKey(withMembersKey), context.generation) + ) { queryClient.setQueryData(listKey, context.previousList); } }, onSettled: invalidateAll, + // Serialize rename against the other optimistic org writes so the + // network calls land in order; the generation guard orders rollbacks. + scope: { id: `organization:${organizationId}` }, }), // No onMutationError toast here: invite-member-sheet (the only caller) @@ -153,6 +179,7 @@ export function useOrganizationMutations( }), { silent: silenceUpdateMemberToast } ), + scope: { id: `organization:${organizationId}` }, }), removeMember: useMutation({ @@ -165,6 +192,7 @@ export function useOrganizationMutations( member => !(member.status === 'active' && member.id === input.memberId) ), })), + scope: { id: `organization:${organizationId}` }, }), deleteInvite: useMutation({ @@ -177,10 +205,13 @@ export function useOrganizationMutations( member => !(member.status === 'invited' && member.inviteId === input.inviteId) ), })), + scope: { id: `organization:${organizationId}` }, }), // No onMutationError toast here: low-balance-alert-sheet (the only // caller) shows the error inline while it stays open (see Pattern P2). + // onError policy: roll back the onMutate snapshot (latest generation only); + // the caller renders the error inline (no toast). updateMinimumBalanceAlert: useMutation({ // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule mutationFn: (input: { @@ -217,6 +248,7 @@ export function useOrganizationMutations( }, { silent: true } ), + scope: { id: `organization:${organizationId}` }, }), }; } diff --git a/apps/mobile/src/lib/hooks/use-security-agent-mutations.test.ts b/apps/mobile/src/lib/hooks/use-security-agent-mutations.test.ts index 81d028dd0b..c16276ead3 100644 --- a/apps/mobile/src/lib/hooks/use-security-agent-mutations.test.ts +++ b/apps/mobile/src/lib/hooks/use-security-agent-mutations.test.ts @@ -1,17 +1,26 @@ -// P1-A-08e wiring tests for `useTriggerSecuritySync`. +// Tests for the security-agent mutation hooks and their helpers. // -// The dashboard owns the sync button and its toasts; these tests assert the -// HOOK WIRING: the `mutationFn` delegates to the matching +// The dashboard owns the sync button and its toasts; the sync suite asserts +// the HOOK WIRING: the `mutationFn` delegates to the matching // `trpcClient.(organizations.)securityAgent.triggerSync.mutate`, the hoisted // operation key is merged into the input, and the key rotation policy (real // `isSecuritySyncRetryable` + `mapSecuritySyncOperationError`) runs inside // `mutationFn`. Only `useHoistedOperationKey` is mocked (it holds React ref // state that needs a mounted renderer). -/* eslint-disable max-lines -- cohesive suite for sync wiring, retryability matrix, and the reconcile-first outbox path */ +// +// Suites: `useTriggerSecuritySync` wiring, retryability, and reconcile-first +// outbox; `useSaveSecurityAgentConfig` expectedRevision and generation +// guard+scope; `useSetSecurityAgentEnabled` generation guard+scope; `scope.id` +// network serialization (real MutationCache); `securitySyncIntentFingerprint`; +// `isSecuritySyncRetryable`; in-progress copy per surface; and +// `isSecurityConfigurationError`. +/* eslint-disable max-lines -- one file for the useTriggerSecuritySync wiring/retryability/outbox, useSaveSecurityAgentConfig expectedRevision + generation-guard/scope, useSetSecurityAgentEnabled generation-guard/scope, scope.id serialization (real MutationCache), securitySyncIntentFingerprint, isSecuritySyncRetryable, in-progress copy per surface, and isSecurityConfigurationError suites */ /* eslint-disable require-await, @typescript-eslint/require-await -- the fake outbox factories settle without await because they resolve immediately */ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import type * as ReactQuery from '@tanstack/react-query'; + import type * as OperationKeyModule from '@/lib/operation-key'; import { isSecurityConfigurationError, @@ -21,6 +30,7 @@ import { SECURITY_SERVICE_NOT_CONFIGURED_MESSAGE, securitySyncIntentFingerprint, useSaveSecurityAgentConfig, + useSetSecurityAgentEnabled, useTriggerSecuritySync, } from './use-security-agent-mutations'; @@ -62,9 +72,11 @@ vi.mock('@/lib/hooks/use-security-agent-commands', () => ({ type MutationOptions = { mutationFn?: (vars: unknown) => Promise; - onError?: (error: unknown) => void; + onMutate?: (vars: unknown) => Promise | unknown; + onError?: (error: unknown, vars?: unknown, context?: unknown) => void; onSuccess?: (result: unknown, vars: unknown) => void; onSettled?: (data?: unknown, error?: unknown, vars?: unknown) => Promise | void; + scope?: { id: string }; }; let lastCapturedOptions: MutationOptions | null = null; @@ -92,6 +104,7 @@ vi.mock('@tanstack/react-query', () => ({ setQueryData: (...args: unknown[]) => setQueryDataMock(...args), cancelQueries: (...args: unknown[]) => cancelQueriesMock(...args), }), + hashKey: (key: unknown) => JSON.stringify(key), })); vi.mock('@/lib/trpc', () => ({ @@ -346,6 +359,127 @@ describe('useSaveSecurityAgentConfig (expectedRevision)', () => { }); }); +describe('useSaveSecurityAgentConfig (generation guard + scope)', () => { + beforeEach(() => { + lastCapturedOptions = null; + getQueryDataMock.mockReset(); + setQueryDataMock.mockReset(); + cancelQueriesMock.mockReset(); + toastErrorMock.mockReset(); + }); + + it('scopes the mutation to serialize against a toggle on the same config', () => { + useSaveSecurityAgentConfig('personal'); + expect(lastCapturedOptions?.scope).toEqual({ id: 'security-agent-config:personal' }); + }); + + it('a failing older save does not roll back while a newer save owns the config', async () => { + getQueryDataMock.mockReturnValue({ isEnabled: false, slaEnabled: false }); + useSaveSecurityAgentConfig('personal'); + const older = await lastCapturedOptions?.onMutate?.({ slaEnabled: true }); + const newer = await lastCapturedOptions?.onMutate?.({ slaEnabled: false }); + + setQueryDataMock.mockClear(); + lastCapturedOptions?.onError?.(new Error('boom'), { slaEnabled: true }, older); + expect(setQueryDataMock).not.toHaveBeenCalled(); + + lastCapturedOptions?.onError?.(new Error('boom'), { slaEnabled: false }, newer); + expect(setQueryDataMock).toHaveBeenCalledTimes(1); + // The toast fires regardless of which generation failed. + expect(toastErrorMock).toHaveBeenCalledTimes(2); + }); + + it('a failing latest save rolls back its snapshot', async () => { + getQueryDataMock.mockReturnValue({ isEnabled: false, slaEnabled: false }); + useSaveSecurityAgentConfig('personal'); + const context = await lastCapturedOptions?.onMutate?.({ slaEnabled: true }); + + setQueryDataMock.mockClear(); + lastCapturedOptions?.onError?.(new Error('boom'), { slaEnabled: true }, context); + expect(setQueryDataMock).toHaveBeenCalledTimes(1); + expect(toastErrorMock).toHaveBeenCalledWith('boom'); + }); +}); + +describe('useSetSecurityAgentEnabled (generation guard + scope)', () => { + beforeEach(() => { + lastCapturedOptions = null; + getQueryDataMock.mockReset(); + setQueryDataMock.mockReset(); + cancelQueriesMock.mockReset(); + toastErrorMock.mockReset(); + }); + + it('scopes the mutation to serialize against a save on the same config', () => { + useSetSecurityAgentEnabled('personal'); + expect(lastCapturedOptions?.scope).toEqual({ id: 'security-agent-config:personal' }); + }); + + it('a failing older toggle does not roll back while a newer toggle owns the config', async () => { + getQueryDataMock.mockReturnValue({ isEnabled: false, configRevision: 1 }); + useSetSecurityAgentEnabled('personal'); + const older = await lastCapturedOptions?.onMutate?.({ isEnabled: true }); + const newer = await lastCapturedOptions?.onMutate?.({ isEnabled: false }); + + setQueryDataMock.mockClear(); + lastCapturedOptions?.onError?.(new Error('boom'), { isEnabled: true }, older); + expect(setQueryDataMock).not.toHaveBeenCalled(); + + lastCapturedOptions?.onError?.(new Error('boom'), { isEnabled: false }, newer); + expect(setQueryDataMock).toHaveBeenCalledTimes(1); + expect(toastErrorMock).toHaveBeenCalledTimes(2); + }); + + it('a failing latest toggle rolls back its snapshot', async () => { + getQueryDataMock.mockReturnValue({ isEnabled: false, configRevision: 1 }); + useSetSecurityAgentEnabled('personal'); + const context = await lastCapturedOptions?.onMutate?.({ isEnabled: true }); + + setQueryDataMock.mockClear(); + lastCapturedOptions?.onError?.(new Error('boom'), { isEnabled: true }, context); + expect(setQueryDataMock).toHaveBeenCalledTimes(1); + expect(toastErrorMock).toHaveBeenCalledWith('boom'); + }); +}); + +describe('scope.id network serialization (real MutationCache)', () => { + it('starts the second same-scope mutationFn only after the first settles', async () => { + const { MutationCache, QueryClient } = + await vi.importActual('@tanstack/react-query'); + const cache = new MutationCache(); + const client = new QueryClient({ mutationCache: cache }); + const order: string[] = []; + const gate = Promise.withResolvers(); + + const first = cache.build(client, { + mutationFn: async () => { + order.push('first-start'); + await gate.promise; + order.push('first-end'); + return 'first'; + }, + scope: { id: 'security-agent-config:personal' }, + }); + const second = cache.build(client, { + mutationFn: async () => { + order.push('second-start'); + return 'second'; + }, + scope: { id: 'security-agent-config:personal' }, + }); + + const p1 = first.execute({}); + const p2 = second.execute({}); + await Promise.resolve(); + await Promise.resolve(); + expect(order).toEqual(['first-start']); + + gate.resolve(null); + await Promise.all([p1, p2]); + expect(order).toEqual(['first-start', 'first-end', 'second-start']); + }); +}); + describe('securitySyncIntentFingerprint (P1-A-08e changed-input)', () => { it('stays stable for a retry of the same scope+repo and rotates when the repo or scope changes', () => { const original = securitySyncIntentFingerprint(ORG_ID, 'kilo/repo'); diff --git a/apps/mobile/src/lib/hooks/use-security-agent-mutations.ts b/apps/mobile/src/lib/hooks/use-security-agent-mutations.ts index 8b8a2f0215..a1f016445b 100644 --- a/apps/mobile/src/lib/hooks/use-security-agent-mutations.ts +++ b/apps/mobile/src/lib/hooks/use-security-agent-mutations.ts @@ -1,7 +1,11 @@ import { isPersonalSecurityScope } from '@kilocode/app-shared/security-agent'; -import { useMutation, useQueryClient } from '@tanstack/react-query'; +import { hashKey, useMutation, useQueryClient } from '@tanstack/react-query'; import { announcingToast } from '@/lib/a11y/announcing-toast'; +import { + isLatestMutationGeneration, + nextMutationGeneration, +} from '@/lib/hooks/mutation-generations'; import { trackSecurityAgentCommand } from '@/lib/hooks/use-security-agent-commands'; import { isOperationInProgress, @@ -124,6 +128,8 @@ export function useSaveSecurityAgentConfig(scope: string) { const queryClient = useQueryClient(); const configQueryKey = useSecurityAgentConfigQueryKey(scope); + // onError policy: roll back the onMutate snapshot (latest generation only) + // and toast error.message. return useMutation({ // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule mutationFn: (patch: Omit) => { @@ -141,14 +147,18 @@ export function useSaveSecurityAgentConfig(scope: string) { }, onMutate: async patch => { await queryClient.cancelQueries({ queryKey: configQueryKey }); + const generation = nextMutationGeneration(hashKey(configQueryKey)); const previous = queryClient.getQueryData(configQueryKey); queryClient.setQueryData(configQueryKey, old => old ? { ...old, ...patch } : old ); - return { previous, patch }; + return { previous, patch, generation }; }, onError: (error, _patch, context) => { - if (context?.previous) { + if ( + context?.previous && + isLatestMutationGeneration(hashKey(configQueryKey), context.generation) + ) { const keys = Object.keys(context.patch) as (keyof SecurityAgentConfig)[]; const restoredFields = pick(context.previous, keys); queryClient.setQueryData(configQueryKey, old => @@ -185,6 +195,9 @@ export function useSaveSecurityAgentConfig(scope: string) { ); }); }, + // Serialize a save against a toggle on the same config so the network + // calls land in order; the generation guard above orders the rollbacks. + scope: { id: `security-agent-config:${scope}` }, }); } @@ -193,6 +206,8 @@ export function useSetSecurityAgentEnabled(scope: string) { const queryClient = useQueryClient(); const configQueryKey = useSecurityAgentConfigQueryKey(scope); + // onError policy: roll back the onMutate snapshot (latest generation only) + // and toast error.message. return useMutation({ // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule mutationFn: (vars: Parameters[0]) => @@ -204,16 +219,23 @@ export function useSetSecurityAgentEnabled(scope: string) { }), onMutate: async vars => { await queryClient.cancelQueries({ queryKey: configQueryKey }); + const generation = nextMutationGeneration(hashKey(configQueryKey)); const previous = queryClient.getQueryData(configQueryKey); queryClient.setQueryData(configQueryKey, old => old ? { ...old, isEnabled: vars.isEnabled } : old ); - return { previous }; + return { previous, generation }; }, onError: (error, _vars, context) => { - queryClient.setQueryData(configQueryKey, old => - old && context?.previous ? { ...old, isEnabled: context.previous.isEnabled } : old - ); + if ( + context?.previous && + isLatestMutationGeneration(hashKey(configQueryKey), context.generation) + ) { + const previous = context.previous; + queryClient.setQueryData(configQueryKey, old => + old ? { ...old, isEnabled: previous.isEnabled } : old + ); + } announcingToast.error(error.message); }, onSuccess: result => { @@ -252,6 +274,9 @@ export function useSetSecurityAgentEnabled(scope: string) { }), ]); }, + // Serialize a toggle against a save on the same config so the network + // calls land in order; the generation guard above orders the rollbacks. + scope: { id: `security-agent-config:${scope}` }, }); } diff --git a/apps/mobile/src/lib/hooks/use-security-remediation.test.ts b/apps/mobile/src/lib/hooks/use-security-remediation.test.ts new file mode 100644 index 0000000000..a1ccfb6f5c --- /dev/null +++ b/apps/mobile/src/lib/hooks/use-security-remediation.test.ts @@ -0,0 +1,189 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { useCancelSecurityRemediation } from './use-security-remediation'; + +type MutationOptions = { + mutationFn?: (vars: unknown) => Promise; + onMutate?: (vars: unknown) => Promise | unknown; + onError?: (error: unknown, vars: unknown, context: unknown) => void; + onSuccess?: (result: unknown, vars: unknown) => void; + onSettled?: (data?: unknown, error?: unknown, vars?: unknown) => Promise | void; + scope?: { id: string }; +}; + +let lastCapturedOptions: MutationOptions | null = null; +const cancelMutateMock = vi.fn(); +const orgCancelMutateMock = vi.fn(); +const invalidateQueriesMock = vi.fn(); +const getQueryDataMock = vi.fn(); +const setQueryDataMock = vi.fn(); +const cancelQueriesMock = vi.fn(); +const toastErrorMock = vi.fn(); +const toastSuccessMock = vi.fn(); + +vi.mock('@tanstack/react-query', () => ({ + useMutation: (opts: MutationOptions) => { + lastCapturedOptions = opts; + return { mutate: vi.fn(), mutateAsync: vi.fn(), isPending: false, isError: false }; + }, + useQueryClient: () => ({ + invalidateQueries: (...args: unknown[]) => { + invalidateQueriesMock(...args); + }, + getQueryData: (...args: unknown[]) => getQueryDataMock(...args), + setQueryData: (...args: unknown[]) => setQueryDataMock(...args), + cancelQueries: (...args: unknown[]) => cancelQueriesMock(...args), + }), + hashKey: (key: unknown) => JSON.stringify(key), +})); + +vi.mock('react-native', () => ({ + InteractionManager: { runAfterInteractions: vi.fn() }, +})); + +vi.mock('sonner-native', () => ({ + toast: { + error: (msg: string) => toastErrorMock(msg), + success: (msg: string) => toastSuccessMock(msg), + }, +})); + +vi.mock('@kilocode/app-shared/security-agent', () => ({ + isPersonalSecurityScope: (scope: string) => scope === 'personal', + getRemediationUnavailableCopy: () => null, +})); + +vi.mock('@/lib/trpc', () => ({ + useTRPC: () => ({ + securityAgent: { + getAnalysis: { + queryKey: ({ findingId }: { findingId: string }) => [ + 'securityAgent', + 'getAnalysis', + { findingId }, + ], + }, + getFinding: { queryKey: () => ['securityAgent', 'getFinding'] }, + getDashboardStats: { queryKey: () => ['securityAgent', 'getDashboardStats'] }, + listFindings: { queryKey: () => ['securityAgent', 'listFindings'] }, + }, + organizations: { + securityAgent: { + getAnalysis: { + queryKey: ({ + organizationId, + findingId, + }: { + organizationId: string; + findingId: string; + }) => ['organizations', 'securityAgent', 'getAnalysis', { organizationId, findingId }], + }, + getFinding: { queryKey: () => ['organizations', 'securityAgent', 'getFinding'] }, + getDashboardStats: { + queryKey: () => ['organizations', 'securityAgent', 'getDashboardStats'], + }, + listFindings: { queryKey: () => ['organizations', 'securityAgent', 'listFindings'] }, + }, + }, + }), + trpcClient: { + securityAgent: { + cancelRemediation: { mutate: (vars: unknown) => cancelMutateMock(vars) }, + }, + organizations: { + securityAgent: { + cancelRemediation: { mutate: (vars: unknown) => orgCancelMutateMock(vars) }, + }, + }, + }, +})); + +const ORG_ID = 'aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa'; + +describe('useCancelSecurityRemediation (generation guard)', () => { + beforeEach(() => { + lastCapturedOptions = null; + cancelMutateMock.mockReset(); + orgCancelMutateMock.mockReset(); + invalidateQueriesMock.mockReset(); + getQueryDataMock.mockReset(); + setQueryDataMock.mockReset(); + cancelQueriesMock.mockReset(); + toastErrorMock.mockReset(); + toastSuccessMock.mockReset(); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + it('has no scope (rule 4: concurrent cancels write different findings)', () => { + useCancelSecurityRemediation('personal'); + expect(lastCapturedOptions?.scope).toBeUndefined(); + }); + + it('a failing older cancel does not roll back while a newer cancel owns the finding', async () => { + getQueryDataMock.mockReturnValue({ + remediationAttempts: [{ id: 'a1', cancellationRequestedAt: null }], + }); + useCancelSecurityRemediation('personal'); + const older = await lastCapturedOptions?.onMutate?.({ attemptId: 'a1', findingId: 'f1' }); + const newer = await lastCapturedOptions?.onMutate?.({ attemptId: 'a1', findingId: 'f1' }); + + setQueryDataMock.mockClear(); + lastCapturedOptions?.onError?.(new Error('boom'), { attemptId: 'a1', findingId: 'f1' }, older); + expect(setQueryDataMock).not.toHaveBeenCalled(); + + lastCapturedOptions?.onError?.(new Error('boom'), { attemptId: 'a1', findingId: 'f1' }, newer); + expect(setQueryDataMock).toHaveBeenCalledTimes(1); + // The toast fires regardless of which generation failed. + expect(toastErrorMock).toHaveBeenCalledTimes(2); + }); + + it('a failing latest cancel rolls back its snapshot', async () => { + getQueryDataMock.mockReturnValue({ + remediationAttempts: [{ id: 'a1', cancellationRequestedAt: null }], + }); + useCancelSecurityRemediation('personal'); + const context = await lastCapturedOptions?.onMutate?.({ attemptId: 'a1', findingId: 'f1' }); + + setQueryDataMock.mockClear(); + lastCapturedOptions?.onError?.( + new Error('boom'), + { attemptId: 'a1', findingId: 'f1' }, + context + ); + expect(setQueryDataMock).toHaveBeenCalledTimes(1); + expect(toastErrorMock).toHaveBeenCalledWith('boom'); + }); + + it('keys the generation guard per finding', async () => { + getQueryDataMock.mockReturnValue({ remediationAttempts: [] }); + useCancelSecurityRemediation('personal'); + // Different findings write different analysis query keys, so the first + // finding's failure still rolls back after a second finding was written. + const findingA = await lastCapturedOptions?.onMutate?.({ attemptId: 'a1', findingId: 'f1' }); + const findingB = await lastCapturedOptions?.onMutate?.({ attemptId: 'a2', findingId: 'f2' }); + void findingB; + + setQueryDataMock.mockClear(); + lastCapturedOptions?.onError?.( + new Error('boom'), + { attemptId: 'a1', findingId: 'f1' }, + findingA + ); + expect(setQueryDataMock).toHaveBeenCalledTimes(1); + }); + + it('delegates an org cancel to organizations.securityAgent.cancelRemediation', async () => { + orgCancelMutateMock.mockResolvedValueOnce({ status: 'cancelled' }); + useCancelSecurityRemediation(ORG_ID); + + await lastCapturedOptions?.mutationFn?.({ attemptId: 'a1', findingId: 'f1' }); + + expect(orgCancelMutateMock).toHaveBeenCalledWith({ + organizationId: ORG_ID, + attemptId: 'a1', + }); + }); +}); diff --git a/apps/mobile/src/lib/hooks/use-security-remediation.ts b/apps/mobile/src/lib/hooks/use-security-remediation.ts index 52e35eac66..50c0ae0ebd 100644 --- a/apps/mobile/src/lib/hooks/use-security-remediation.ts +++ b/apps/mobile/src/lib/hooks/use-security-remediation.ts @@ -5,9 +5,13 @@ import { getRemediationUnavailableCopy, isPersonalSecurityScope, } from '@kilocode/app-shared/security-agent'; -import { useMutation, useQueryClient } from '@tanstack/react-query'; +import { hashKey, useMutation, useQueryClient } from '@tanstack/react-query'; import { toast } from 'sonner-native'; +import { + isLatestMutationGeneration, + nextMutationGeneration, +} from '@/lib/hooks/mutation-generations'; import { reconcileFirstPage } from '@/lib/query/infinite-retention'; import { scheduleCacheMaintenance } from '@/lib/query/schedule-cache-maintenance'; import { type SecurityAnalysis } from '@/lib/security-agent'; @@ -141,6 +145,8 @@ function getSecurityAnalysisQueryKey( export function useCancelSecurityRemediation(scope: string) { const trpc = useTRPC(); const queryClient = useQueryClient(); + // onError policy: roll back the onMutate snapshot (latest generation only) + // and toast error.message. return useMutation({ // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule mutationFn: (vars: { attemptId: string; findingId: string }) => @@ -153,6 +159,7 @@ export function useCancelSecurityRemediation(scope: string) { onMutate: async vars => { const analysisQueryKey = getSecurityAnalysisQueryKey(trpc, scope, vars.findingId); await queryClient.cancelQueries({ queryKey: analysisQueryKey }); + const generation = nextMutationGeneration(hashKey(analysisQueryKey)); const previous = queryClient.getQueryData(analysisQueryKey); queryClient.setQueryData(analysisQueryKey, old => old @@ -166,10 +173,13 @@ export function useCancelSecurityRemediation(scope: string) { } : old ); - return { previous, analysisQueryKey }; + return { previous, analysisQueryKey, generation }; }, onError: (error, _vars, context) => { - if (context?.previous) { + if ( + context?.previous && + isLatestMutationGeneration(hashKey(context.analysisQueryKey), context.generation) + ) { queryClient.setQueryData(context.analysisQueryKey, context.previous); } toast.error(error.message); From 12225f61154fe41d8141a1654df73197291bd8fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Fri, 21 Aug 2026 05:40:30 +0200 Subject: [PATCH 05/14] fix(mobile): guard session, reviewer, discussion, and notification mutations Add the generation guard and the named network-order mechanisms to the remaining mutation hooks: session list, code-reviewer, PR discussion threads, and push-token/notification preferences. --- .../notifications-screen.mounted.test.tsx | 86 +++++- .../src/components/notifications-screen.tsx | 48 ++- .../src/lib/hooks/use-code-reviewer.test.ts | 137 ++++++++- .../mobile/src/lib/hooks/use-code-reviewer.ts | 78 +++-- .../lib/hooks/use-session-mutations.test.ts | 106 ++++++- .../src/lib/hooks/use-session-mutations.ts | 31 +- .../use-review-discussion-mutations.test.ts | 274 +++++++++++++++++- .../use-review-discussion-mutations.ts | 160 ++++++---- 8 files changed, 782 insertions(+), 138 deletions(-) diff --git a/apps/mobile/src/components/notifications-screen.mounted.test.tsx b/apps/mobile/src/components/notifications-screen.mounted.test.tsx index 64611ad6ce..1d476a7680 100644 --- a/apps/mobile/src/components/notifications-screen.mounted.test.tsx +++ b/apps/mobile/src/components/notifications-screen.mounted.test.tsx @@ -21,6 +21,8 @@ const registerTokenMutationFn = vi.hoisted(() => vi.fn()); const getNotificationPermissionStatus = vi.hoisted(() => vi.fn()); const getDevicePushToken = vi.hoisted(() => vi.fn()); const toastError = vi.hoisted(() => vi.fn()); +const registerTokenOptions = vi.hoisted(() => vi.fn()); +const setPreferenceOptions = vi.hoisted(() => vi.fn()); const useKiloClawTabVisible = vi.hoisted(() => vi.fn(() => true)); vi.mock('@/lib/hooks/use-kiloclaw-tab-visible', () => ({ @@ -87,18 +89,24 @@ vi.mock('@/lib/trpc', () => ({ queryOptions: () => ({ queryKey: ['getNotificationPreferences'], queryFn: prefsQueryFn }), }, registerPushToken: { - mutationOptions: (opts: object) => ({ - ...opts, - mutationFn: registerTokenMutationFn, - mutationKey: ['registerPushToken'], - }), + mutationOptions: (opts: object) => { + registerTokenOptions(opts); + return { + ...opts, + mutationFn: registerTokenMutationFn, + mutationKey: ['registerPushToken'], + }; + }, }, setNotificationPreferences: { - mutationOptions: (opts: object) => ({ - ...opts, - mutationFn: setPreferenceMutationFn, - mutationKey: ['setNotificationPreferences'], - }), + mutationOptions: (opts: object) => { + setPreferenceOptions(opts); + return { + ...opts, + mutationFn: setPreferenceMutationFn, + mutationKey: ['setNotificationPreferences'], + }; + }, }, }, }), @@ -319,3 +327,61 @@ describe('NotificationsScreen KiloClaw activity row', () => { expect(skeletonCount(renderer.root)).toBeGreaterThan(0); }); }); + +describe('NotificationsScreen mutation serialization (scope.id + generation guard)', () => { + beforeEach(() => { + vi.clearAllMocks(); + getNotificationPermissionStatus.mockResolvedValue('granted'); + getDevicePushToken.mockResolvedValue('device-token'); + pushTokensQueryFn.mockResolvedValue([{ token: 'device-token', platform: 'android' }]); + setPreferenceMutationFn.mockResolvedValue({}); + registerTokenMutationFn.mockResolvedValue({ success: true }); + }); + + it('scopes both mutations to their fixed cache entries', async () => { + prefsQueryFn.mockResolvedValue(fullPrefs()); + const { renderer } = await renderScreen(); + await waitForEnabledSwitch(renderer, 'Chat messages'); + + expect(registerTokenOptions).toHaveBeenCalledWith( + expect.objectContaining({ scope: { id: 'push-tokens' } }) + ); + expect(setPreferenceOptions).toHaveBeenCalledWith( + expect.objectContaining({ scope: { id: 'notification-preferences' } }) + ); + }); + + it('a failing older category mutation does not roll back while a newer one owns the cache', async () => { + prefsQueryFn.mockResolvedValue(fullPrefs({ chatMessages: true, agentAttention: true })); + const { renderer, queryClient } = await renderScreen(); + await waitForEnabledSwitch(renderer, 'Chat messages'); + + const opts = setPreferenceOptions.mock.calls[0]?.[0] as + | { + onMutate?: (vars: Record) => Promise; + onError?: (error: Error, vars: Record, context: unknown) => void; + } + | undefined; + if (!opts?.onMutate || !opts.onError) { + throw new Error('setNotificationPreferences options not captured'); + } + + const older = await opts.onMutate({ chatMessages: false }); + const newer = await opts.onMutate({ agentAttention: false }); + + // The older failure must not restore its snapshot over the newer write. + opts.onError(new Error('boom'), { chatMessages: false }, older); + const afterOlder = queryClient.getQueryData>([ + 'getNotificationPreferences', + ]); + expect(afterOlder?.agentAttention).toBe(false); + + // The newer failure (latest generation) rolls back its own snapshot. + opts.onError(new Error('boom'), { agentAttention: false }, newer); + const afterNewer = queryClient.getQueryData>([ + 'getNotificationPreferences', + ]); + expect(afterNewer?.agentAttention).toBe(true); + expect(toastError).toHaveBeenCalledTimes(2); + }); +}); diff --git a/apps/mobile/src/components/notifications-screen.tsx b/apps/mobile/src/components/notifications-screen.tsx index 86e198d7e3..d0828caaf5 100644 --- a/apps/mobile/src/components/notifications-screen.tsx +++ b/apps/mobile/src/components/notifications-screen.tsx @@ -4,7 +4,7 @@ * has seven keys; the KiloClaw row is hidden when useKiloClawTabVisible is false. * Extracting subcomponents would re-encode the same hooks. The screen stays a * single rendered surface. */ -import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query'; +import { hashKey, useMutation, useQuery, useQueryClient } from '@tanstack/react-query'; import * as Application from 'expo-application'; import * as Notifications from 'expo-notifications'; import { @@ -43,6 +43,10 @@ import { rollbackAgentPushOptimistic, } from '@/lib/hooks/agent-push-preference'; import { useAppLifecycle } from '@/lib/hooks/use-app-lifecycle'; +import { + isLatestMutationGeneration, + nextMutationGeneration, +} from '@/lib/hooks/mutation-generations'; import { useKiloClawTabVisible } from '@/lib/hooks/use-kiloclaw-tab-visible'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; import { @@ -305,10 +309,13 @@ export function NotificationsScreen() { void queryClient.invalidateQueries({ queryKey: preferencesQueryKey }); }, [queryClient, pushTokensQueryKey, preferencesQueryKey]); + // onError policy: roll back the onMutate snapshot (latest generation only) + // and toast error.message. const registerToken = useMutation( trpc.user.registerPushToken.mutationOptions({ onMutate: async () => { await queryClient.cancelQueries({ queryKey: pushTokensQueryKey }); + const generation = nextMutationGeneration(hashKey(pushTokensQueryKey)); const previous = queryClient.getQueryData(pushTokensQueryKey); if (deviceToken) { queryClient.setQueryData(pushTokensQueryKey, (old: typeof pushTokens) => [ @@ -316,15 +323,21 @@ export function NotificationsScreen() { { token: deviceToken, platform: getPlatform() }, ]); } - return { previous }; + return { previous, generation }; }, onError: (error, _vars, context) => { - if (context?.previous) { + if ( + context?.previous && + isLatestMutationGeneration(hashKey(pushTokensQueryKey), context.generation) + ) { queryClient.setQueryData(pushTokensQueryKey, context.previous); } toast.error(error.message); }, onSettled: invalidateAll, + // One fixed cache entry, so a single static scope id serializes the + // network call (rule 2). + scope: { id: 'push-tokens' }, }) ); @@ -332,12 +345,11 @@ export function NotificationsScreen() { // category. We pass ONE key per call so the server-side partial update // only touches the column the user is flipping; the optimistic helper // scopes its in-memory flip to that same key. + // onError policy: roll back the onMutate snapshot (latest generation only) + // and toast error.message. const setPreference = useMutation( trpc.user.setNotificationPreferences.mutationOptions({ - // react-query's onMutate signature requires either an async function or - // a plain return of a Promise; we need async semantics so the optimistic - // write commits before the mutation body runs. - // eslint-disable-next-line require-await, typescript-eslint/return-await + // async so the optimistic write commits before the mutation body runs. onMutate: async variables => { const category = categoryFromVariables(variables); if (category == null) { @@ -347,19 +359,26 @@ export function NotificationsScreen() { if (next === undefined) { return undefined; } - return applyAgentPushOptimistic({ + const generation = nextMutationGeneration(hashKey(preferencesQueryKey)); + const context = await applyAgentPushOptimistic({ queryClient, queryKey: preferencesQueryKey, next, category, }); + return { ...context, generation }; }, onError: (error, variables, context) => { - rollbackAgentPushOptimistic({ - queryClient, - queryKey: preferencesQueryKey, - context, - }); + if ( + context && + isLatestMutationGeneration(hashKey(preferencesQueryKey), context.generation) + ) { + rollbackAgentPushOptimistic({ + queryClient, + queryKey: preferencesQueryKey, + context, + }); + } toast.error(error.message); if (variables.notificationPreviews !== undefined) { setPreviewErrorCode(readTrpcErrorField(error, 'code')); @@ -382,6 +401,9 @@ export function NotificationsScreen() { } void queryClient.invalidateQueries({ queryKey: preferencesQueryKey }); }, + // One fixed cache entry, so a single static scope id serializes the + // network call (rule 2). + scope: { id: 'notification-preferences' }, }) ); diff --git a/apps/mobile/src/lib/hooks/use-code-reviewer.test.ts b/apps/mobile/src/lib/hooks/use-code-reviewer.test.ts index 637d7de844..28c78b5657 100644 --- a/apps/mobile/src/lib/hooks/use-code-reviewer.test.ts +++ b/apps/mobile/src/lib/hooks/use-code-reviewer.test.ts @@ -1,14 +1,17 @@ +/* eslint-disable max-lines -- one file for the save/toggle serialization and generation-guard rollback suites */ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { type ConfigPatch, PERSONAL_SCOPE } from '@/lib/code-reviewer-config'; -import { useSaveReviewConfig } from './use-code-reviewer'; +import { useSaveReviewConfig, useToggleReviewer } from './use-code-reviewer'; type MutationOptions = { mutationFn?: (vars: unknown) => Promise; - onError?: (error: unknown) => void; + onMutate?: (vars: unknown) => Promise | unknown; + onError?: (error: unknown, vars?: unknown, context?: unknown) => void; onSettled?: () => void; onSuccess?: (data: unknown) => void; + scope?: { id: string }; }; type PersonalPatch = { @@ -37,6 +40,8 @@ const personalPatchMutateMock = vi.fn(); const orgPatchMutateMock = vi.fn(); const personalSaveMutateMock = vi.fn(); const orgSaveMutateMock = vi.fn(); +const personalToggleMutateMock = vi.fn(); +const orgToggleMutateMock = vi.fn(); const invalidateQueriesMock = vi.fn(); const cancelQueriesMock = vi.fn(); const getQueryDataMock = vi.fn(); @@ -57,6 +62,7 @@ vi.mock('@tanstack/react-query', () => ({ setQueryData: setQueryDataMock, invalidateQueries: invalidateQueriesMock, }), + hashKey: (key: unknown) => JSON.stringify(key), })); vi.mock('@/lib/trpc', () => ({ @@ -76,6 +82,8 @@ vi.mock('@/lib/trpc', () => ({ patchReviewConfig: { mutate: (vars: unknown) => personalPatchMutateMock(vars) }, // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule saveReviewConfig: { mutate: (vars: unknown) => personalSaveMutateMock(vars) }, + // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule + toggleReviewAgent: { mutate: (vars: unknown) => personalToggleMutateMock(vars) }, }, organizations: { reviewAgent: { @@ -83,6 +91,8 @@ vi.mock('@/lib/trpc', () => ({ patchReviewConfig: { mutate: (vars: unknown) => orgPatchMutateMock(vars) }, // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule saveReviewConfig: { mutate: (vars: unknown) => orgSaveMutateMock(vars) }, + // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule + toggleReviewAgent: { mutate: (vars: unknown) => orgToggleMutateMock(vars) }, }, }, }, @@ -114,12 +124,28 @@ function getSaveOptions( return lastCapturedOptions; } +function getToggleOptions( + scope: string, + platform: 'github' | 'gitlab' | 'bitbucket' +): MutationOptions { + lastCapturedOptions = null; + // eslint-disable-next-line react-hooks/rules-of-hooks + useToggleReviewer(scope, platform); + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + if (!lastCapturedOptions) { + throw new Error('mutation options for useToggleReviewer were not captured'); + } + return lastCapturedOptions; +} + beforeEach(() => { lastCapturedOptions = null; personalPatchMutateMock.mockReset(); orgPatchMutateMock.mockReset(); personalSaveMutateMock.mockReset(); orgSaveMutateMock.mockReset(); + personalToggleMutateMock.mockReset(); + orgToggleMutateMock.mockReset(); invalidateQueriesMock.mockReset(); cancelQueriesMock.mockReset(); getQueryDataMock.mockReset(); @@ -129,6 +155,8 @@ beforeEach(() => { // override per-case when they need a different outcome. personalPatchMutateMock.mockResolvedValue({ success: true, webhookSync: null }); orgPatchMutateMock.mockResolvedValue({ success: true, webhookSync: null }); + personalToggleMutateMock.mockResolvedValue({ success: true }); + orgToggleMutateMock.mockResolvedValue({ success: true }); }); afterEach(() => { @@ -366,3 +394,108 @@ describe('useSaveReviewConfig onError', () => { expect(toastErrorMock).toHaveBeenCalledWith('Network unreachable'); }); }); + +describe('useSaveReviewConfig (generation guard)', () => { + beforeEach(() => { + lastCapturedOptions = null; + getQueryDataMock.mockReset(); + setQueryDataMock.mockReset(); + cancelQueriesMock.mockReset(); + toastErrorMock.mockReset(); + }); + + it('a failing older save does not roll back while a newer save owns the config', async () => { + getQueryDataMock.mockReturnValue({ reviewStyle: 'lenient' }); + const opts = getSaveOptions(PERSONAL_SCOPE, 'github'); + const older = await opts.onMutate?.({ reviewStyle: 'strict' }); + const newer = await opts.onMutate?.({ reviewStyle: 'lenient' }); + + setQueryDataMock.mockClear(); + opts.onError?.(new Error('boom'), { reviewStyle: 'strict' }, older); + expect(setQueryDataMock).not.toHaveBeenCalled(); + + opts.onError?.(new Error('boom'), { reviewStyle: 'lenient' }, newer); + expect(setQueryDataMock).toHaveBeenCalledTimes(1); + expect(toastErrorMock).toHaveBeenCalledTimes(2); + }); + + it('a failing latest save rolls back its snapshot and toasts', async () => { + getQueryDataMock.mockReturnValue({ reviewStyle: 'lenient' }); + const opts = getSaveOptions(PERSONAL_SCOPE, 'github'); + const context = await opts.onMutate?.({ reviewStyle: 'strict' }); + + setQueryDataMock.mockClear(); + opts.onError?.(new Error('boom'), { reviewStyle: 'strict' }, context); + expect(setQueryDataMock).toHaveBeenCalledTimes(1); + expect(toastErrorMock).toHaveBeenCalledWith('boom'); + }); +}); + +describe('useToggleReviewer (generation guard + chain join)', () => { + beforeEach(() => { + lastCapturedOptions = null; + getQueryDataMock.mockReset(); + setQueryDataMock.mockReset(); + cancelQueriesMock.mockReset(); + toastErrorMock.mockReset(); + personalToggleMutateMock.mockReset(); + personalToggleMutateMock.mockResolvedValue({ success: true }); + }); + + it('joins the save chain key (rule 1) and adds no scope.id', async () => { + const opts = getToggleOptions(PERSONAL_SCOPE, 'github'); + await opts.mutationFn?.({ isEnabled: true }); + expect(personalToggleMutateMock).toHaveBeenCalledWith( + expect.objectContaining({ platform: 'github', isEnabled: true }) + ); + expect(opts.scope).toBeUndefined(); + }); + + it('serializes the toggle behind an in-flight save on the same chain key', async () => { + const saveOpts = getSaveOptions(PERSONAL_SCOPE, 'github'); + const toggleOpts = getToggleOptions(PERSONAL_SCOPE, 'github'); + + const saveGate = Promise.withResolvers(); + personalPatchMutateMock.mockReset(); + personalPatchMutateMock.mockReturnValueOnce(saveGate.promise); + personalToggleMutateMock.mockResolvedValue({ success: true }); + + const savePromise = saveOpts.mutationFn?.({ reviewStyle: 'strict' }); + const togglePromise = toggleOpts.mutationFn?.({ isEnabled: true }); + + await Promise.resolve(); + await Promise.resolve(); + // The toggle's network call must not start while the save is in flight. + expect(personalToggleMutateMock).not.toHaveBeenCalled(); + + saveGate.resolve({ success: true, webhookSync: null }); + await Promise.all([savePromise, togglePromise]); + expect(personalToggleMutateMock).toHaveBeenCalledTimes(1); + }); + + it('a failing older toggle does not roll back while a newer toggle owns the config', async () => { + getQueryDataMock.mockReturnValue({ isEnabled: false }); + const opts = getToggleOptions(PERSONAL_SCOPE, 'github'); + const older = await opts.onMutate?.({ isEnabled: true }); + const newer = await opts.onMutate?.({ isEnabled: false }); + + setQueryDataMock.mockClear(); + opts.onError?.(new Error('boom'), { isEnabled: true }, older); + expect(setQueryDataMock).not.toHaveBeenCalled(); + + opts.onError?.(new Error('boom'), { isEnabled: false }, newer); + expect(setQueryDataMock).toHaveBeenCalledTimes(1); + expect(toastErrorMock).toHaveBeenCalledTimes(2); + }); + + it('a failing latest toggle rolls back its snapshot and toasts', async () => { + getQueryDataMock.mockReturnValue({ isEnabled: false }); + const opts = getToggleOptions(PERSONAL_SCOPE, 'github'); + const context = await opts.onMutate?.({ isEnabled: true }); + + setQueryDataMock.mockClear(); + opts.onError?.(new Error('boom'), { isEnabled: true }, context); + expect(setQueryDataMock).toHaveBeenCalledTimes(1); + expect(toastErrorMock).toHaveBeenCalledWith('boom'); + }); +}); diff --git a/apps/mobile/src/lib/hooks/use-code-reviewer.ts b/apps/mobile/src/lib/hooks/use-code-reviewer.ts index 141f7c561b..0036dde01e 100644 --- a/apps/mobile/src/lib/hooks/use-code-reviewer.ts +++ b/apps/mobile/src/lib/hooks/use-code-reviewer.ts @@ -1,4 +1,10 @@ -import { useMutation, useQuery, useQueryClient, type UseQueryResult } from '@tanstack/react-query'; +import { + hashKey, + useMutation, + useQuery, + useQueryClient, + type UseQueryResult, +} from '@tanstack/react-query'; import { announcingToast } from '@/lib/a11y/announcing-toast'; import { @@ -7,6 +13,10 @@ import { type ReviewConfigData, type ReviewerPlatform, } from '@/lib/code-reviewer-config'; +import { + isLatestMutationGeneration, + nextMutationGeneration, +} from '@/lib/hooks/mutation-generations'; import { chainSave } from '@/lib/hooks/save-chain'; import { trpcClient, useTRPC } from '@/lib/trpc'; import { pick } from '@/lib/utils'; @@ -141,40 +151,51 @@ export function useReviewConfigCacheReader(scope: string, platform: ReviewerPlat export function useToggleReviewer(scope: string, platform: ReviewerPlatform) { const queryClient = useQueryClient(); const queryKey = useReviewConfigQueryKey(scope, platform); + const toggleChainKey = `${scope}:${platform}`; + // onError policy: roll back the onMutate snapshot (latest generation only) + // and toast error.message. return useMutation({ - mutationFn: async (vars: { isEnabled: boolean }) => { - const result = isPersonal(scope) - ? await trpcClient.personalReviewAgent.toggleReviewAgent.mutate({ - platform: toPersonalPlatform(platform), - isEnabled: vars.isEnabled, - }) - : await trpcClient.organizations.reviewAgent.toggleReviewAgent.mutate({ - organizationId: scope, - platform, - isEnabled: vars.isEnabled, - }); - // The output type widens `success` to `boolean` (not a `true` - // literal), so a domain failure here must not be treated as a - // resolved mutation — throwing routes it to onError (toast) instead - // of letting callers' onSuccess fire haptics/navigation as if it worked. - if (!result.success) { - throw new Error('Failed to update reviewer'); - } - return result; - }, + // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule + mutationFn: (vars: { isEnabled: boolean }) => + // The toggle joins the same chain key the config save uses, so a toggle + // and a save for the same scope+platform serialize their network calls. + chainSave(toggleChainKey, async () => { + const result = isPersonal(scope) + ? await trpcClient.personalReviewAgent.toggleReviewAgent.mutate({ + platform: toPersonalPlatform(platform), + isEnabled: vars.isEnabled, + }) + : await trpcClient.organizations.reviewAgent.toggleReviewAgent.mutate({ + organizationId: scope, + platform, + isEnabled: vars.isEnabled, + }); + // The output type widens `success` to `boolean` (not a `true` + // literal), so a domain failure here must not be treated as a + // resolved mutation — throwing routes it to onError (toast) instead + // of letting callers' onSuccess fire haptics/navigation as if it worked. + if (!result.success) { + throw new Error('Failed to update reviewer'); + } + return result; + }), onMutate: async vars => { await queryClient.cancelQueries({ queryKey }); + const generation = nextMutationGeneration(hashKey(queryKey)); const previous = queryClient.getQueryData(queryKey); queryClient.setQueryData(queryKey, old => old ? { ...old, isEnabled: vars.isEnabled } : old ); - return { previous }; + return { previous, generation }; }, onError: (error, _vars, context) => { - queryClient.setQueryData(queryKey, old => - old && context?.previous ? { ...old, isEnabled: context.previous.isEnabled } : old - ); + if (context?.previous && isLatestMutationGeneration(hashKey(queryKey), context.generation)) { + const previous = context.previous; + queryClient.setQueryData(queryKey, old => + old ? { ...old, isEnabled: previous.isEnabled } : old + ); + } announcingToast.error(error.message); }, // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule @@ -215,6 +236,8 @@ export function useSaveReviewConfig(scope: string, platform: ReviewerPlatform) { const webhookWarningQueryKey = gitLabWebhookWarningQueryKey(scope, platform); const saveChainKey = `${scope}:${platform}`; + // onError policy: roll back the onMutate snapshot (latest generation only) + // and toast error.message. return useMutation({ // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule mutationFn: (patch: ConfigPatch) => @@ -315,14 +338,15 @@ export function useSaveReviewConfig(scope: string, platform: ReviewerPlatform) { }), onMutate: async patch => { await queryClient.cancelQueries({ queryKey }); + const generation = nextMutationGeneration(hashKey(queryKey)); const previous = queryClient.getQueryData(queryKey); queryClient.setQueryData(queryKey, old => old ? { ...old, ...patch } : old ); - return { previous, patch }; + return { previous, patch, generation }; }, onError: (error, _patch, context) => { - if (context?.previous) { + if (context?.previous && isLatestMutationGeneration(hashKey(queryKey), context.generation)) { const keys = Object.keys(context.patch) as (keyof ConfigPatch)[]; const restoredFields = pick(context.previous, keys); queryClient.setQueryData(queryKey, old => diff --git a/apps/mobile/src/lib/hooks/use-session-mutations.test.ts b/apps/mobile/src/lib/hooks/use-session-mutations.test.ts index 1b912261d5..c37341884e 100644 --- a/apps/mobile/src/lib/hooks/use-session-mutations.test.ts +++ b/apps/mobile/src/lib/hooks/use-session-mutations.test.ts @@ -1,3 +1,4 @@ +/* eslint-disable max-lines -- one file for the rename/delete optimistic mutation wiring and rollback suites */ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { useSessionMutations } from './use-session-mutations'; @@ -6,6 +7,7 @@ type MutationOptions = { onMutate?: (input: { session_id: string; title?: string }) => Promise | unknown; onError?: (error: Error, input: unknown, context: unknown) => void; onSettled?: () => unknown; + scope?: { id: string }; [key: string]: unknown; }; type TrpcMock = { @@ -63,6 +65,7 @@ vi.mock('@tanstack/react-query', () => ({ setQueryData: setQueryDataMock, invalidateQueries: invalidateQueriesMock, }), + hashKey: (key: unknown) => JSON.stringify(key), })); vi.mock('@/lib/trpc', () => ({ @@ -222,28 +225,30 @@ describe('useSessionMutations', () => { expect(context).toEqual({ previous: storedSnapshot, previousActive: activeSnapshot, + generation: expect.any(Number), }); }); - it('onError restores both snapshots and toasts the error', () => { + it('onError restores both snapshots and toasts the error', async () => { + const storedSnapshot: [unknown, unknown][] = [[['stored-key'], { pages: ['stored'] }]]; + const activeSnapshot: [unknown, unknown][] = [ + [['active-key'], { sessions: [{ id: 's1', title: 'Old' }] }], + ]; + getQueriesDataMock.mockReturnValueOnce(storedSnapshot).mockReturnValueOnce(activeSnapshot); + useSessionMutations(); const options = capturedOptions.rename; - const storedKey = ['stored-key'] as const; - const activeKey = ['active-key'] as const; - const previous = [[storedKey, { pages: ['stored'] }]] as [unknown, unknown][]; - const previousActive = [[activeKey, { sessions: [{ id: 's1', title: 'Old' }] }]] as [ - unknown, - unknown, - ][]; - - options?.onError?.( - new Error('rename failed'), - { session_id: 's1', title: 'New' }, - { previous, previousActive } - ); + const onMutate = options?.onMutate; + if (!onMutate) { + throw new Error('expected rename onMutate'); + } + const context = await onMutate({ session_id: 's1', title: 'New' }); + + setQueryDataMock.mockClear(); + options.onError?.(new Error('rename failed'), { session_id: 's1', title: 'New' }, context); - expect(setQueryDataMock).toHaveBeenCalledWith(storedKey, { pages: ['stored'] }); - expect(setQueryDataMock).toHaveBeenCalledWith(activeKey, { + expect(setQueryDataMock).toHaveBeenCalledWith(['stored-key'], { pages: ['stored'] }); + expect(setQueryDataMock).toHaveBeenCalledWith(['active-key'], { sessions: [{ id: 's1', title: 'Old' }], }); expect(toastErrorMock).toHaveBeenCalledWith('rename failed'); @@ -284,6 +289,75 @@ describe('useSessionMutations', () => { }); }); + describe('generation guard (shared cliSessionsV2.list cache)', () => { + it('adds no scope.id (callers already serialize per session via chainSave)', () => { + useSessionMutations(); + expect(capturedOptions.delete?.scope).toBeUndefined(); + expect(capturedOptions.rename?.scope).toBeUndefined(); + }); + + it('a failing older delete does not roll back while a newer rename owns the shared list cache', async () => { + const deleteSnapshot: [unknown, unknown][] = [[['delete-key'], { pages: ['delete'] }]]; + const renameStoredSnapshot: [unknown, unknown][] = [[['stored-key'], { pages: ['stored'] }]]; + const renameActiveSnapshot: [unknown, unknown][] = [ + [['active-key'], { sessions: [{ id: 's2', title: 'Old' }] }], + ]; + getQueriesDataMock + .mockReturnValueOnce(deleteSnapshot) + .mockReturnValueOnce(renameStoredSnapshot) + .mockReturnValueOnce(renameActiveSnapshot); + + useSessionMutations(); + const deleteOnMutate = capturedOptions.delete?.onMutate; + const renameOnMutate = capturedOptions.rename?.onMutate; + if (!deleteOnMutate || !renameOnMutate) { + throw new Error('expected onMutate'); + } + + const olderDelete = await deleteOnMutate({ session_id: 's1' }); + const newerRename = await renameOnMutate({ session_id: 's2', title: 'New' }); + + setQueryDataMock.mockClear(); + capturedOptions.delete?.onError?.( + new Error('delete failed'), + { session_id: 's1' }, + olderDelete + ); + // The older delete's rollback must not restore its snapshot over the + // newer rename's optimistic write. + expect(setQueryDataMock).not.toHaveBeenCalled(); + expect(toastErrorMock).toHaveBeenCalledWith('delete failed'); + + capturedOptions.rename?.onError?.( + new Error('rename failed'), + { session_id: 's2', title: 'New' }, + newerRename + ); + expect(setQueryDataMock).toHaveBeenCalledWith(['stored-key'], { pages: ['stored'] }); + expect(setQueryDataMock).toHaveBeenCalledWith(['active-key'], { + sessions: [{ id: 's2', title: 'Old' }], + }); + expect(toastErrorMock).toHaveBeenCalledWith('rename failed'); + }); + + it('a failing latest delete rolls back its snapshot and toasts', async () => { + const deleteSnapshot: [unknown, unknown][] = [[['delete-key'], { pages: ['delete'] }]]; + getQueriesDataMock.mockReturnValue(deleteSnapshot); + + useSessionMutations(); + const onMutate = capturedOptions.delete?.onMutate; + if (!onMutate) { + throw new Error('expected delete onMutate'); + } + const context = await onMutate({ session_id: 's1' }); + + setQueryDataMock.mockClear(); + capturedOptions.delete?.onError?.(new Error('delete failed'), { session_id: 's1' }, context); + expect(setQueryDataMock).toHaveBeenCalledWith(['delete-key'], { pages: ['delete'] }); + expect(toastErrorMock).toHaveBeenCalledWith('delete failed'); + }); + }); + describe('deleteSession completion callback', () => { it('toasts success and invokes onDeleted after a successful delete', async () => { mutateAsyncMock.mockResolvedValue(undefined); diff --git a/apps/mobile/src/lib/hooks/use-session-mutations.ts b/apps/mobile/src/lib/hooks/use-session-mutations.ts index 685cbbf089..5734cb7d27 100644 --- a/apps/mobile/src/lib/hooks/use-session-mutations.ts +++ b/apps/mobile/src/lib/hooks/use-session-mutations.ts @@ -1,8 +1,12 @@ -import { type QueryKey, useMutation, useQueryClient } from '@tanstack/react-query'; +import { hashKey, type QueryKey, useMutation, useQueryClient } from '@tanstack/react-query'; import { invalidateAgentSessionQueries } from '@/lib/agent-session-cache'; import { applyActiveSessionTitle, type CachedActiveSessionsData } from '@/lib/active-sessions-live'; import { announcingToast } from '@/lib/a11y/announcing-toast'; +import { + isLatestMutationGeneration, + nextMutationGeneration, +} from '@/lib/hooks/mutation-generations'; import { chainSave } from '@/lib/hooks/save-chain'; import { scheduleCacheMaintenance } from '@/lib/query/schedule-cache-maintenance'; import { @@ -61,31 +65,42 @@ export function useSessionMutations() { } }; + // onError policy: roll back the onMutate snapshot (latest generation only) + // and toast error.message. const deleteSessionMutation = useMutation( trpc.cliSessionsV2.delete.mutationOptions({ - // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule - onMutate: ({ session_id }) => - snapshotAndUpdate(data => removeStoredSession(data, session_id)), + onMutate: async ({ session_id }) => { + const generation = nextMutationGeneration(hashKey(listKey)); + const { previous } = await snapshotAndUpdate(data => removeStoredSession(data, session_id)); + return { previous, generation }; + }, onError: (error, _input, context) => { - rollback(context?.previous); + if (context && isLatestMutationGeneration(hashKey(listKey), context.generation)) { + rollback(context.previous); + } onError(error); }, onSettled: invalidateSessions, }) ); + // onError policy: roll back the onMutate snapshot (latest generation only) + // and toast error.message. const renameSessionMutation = useMutation( trpc.cliSessionsV2.rename.mutationOptions({ onMutate: async ({ session_id, title }) => { + const generation = nextMutationGeneration(hashKey(listKey)); const { previous } = await snapshotAndUpdate(data => mapStoredSessions(data, session_id, session => ({ ...session, title })) ); const previousActive = await snapshotAndUpdateActive(session_id, title); - return { previous, previousActive }; + return { previous, previousActive, generation }; }, onError: (error, _input, context) => { - rollback(context?.previous); - rollback(context?.previousActive); + if (context && isLatestMutationGeneration(hashKey(listKey), context.generation)) { + rollback(context.previous); + rollback(context.previousActive); + } onError(error); }, onSettled: invalidateSessions, diff --git a/apps/mobile/src/lib/pr-review/discussion/use-review-discussion-mutations.test.ts b/apps/mobile/src/lib/pr-review/discussion/use-review-discussion-mutations.test.ts index 13fa6251d1..dc26f01ad4 100644 --- a/apps/mobile/src/lib/pr-review/discussion/use-review-discussion-mutations.test.ts +++ b/apps/mobile/src/lib/pr-review/discussion/use-review-discussion-mutations.test.ts @@ -9,12 +9,20 @@ // `mutationFn`. Only `useHoistedOperationKey` is mocked (it holds React // ref state that needs a mounted renderer, covered by // `operation-key.mounted.test.tsx`). +/* eslint-disable max-lines -- one file for the reply wiring, the resolve/unresolve/reaction generation guard + chainSave/scope serialization, and the real-MutationCache scope.id serialization suites */ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import type * as OperationKeyModule from '@/lib/operation-key'; +import type * as ReactQuery from '@tanstack/react-query'; import { prIntentFingerprint } from '@kilocode/app-shared/pr-review'; -import { useReplyToCommentMutation } from './use-review-discussion-mutations'; +import { + useAddReactionMutation, + useRemoveReactionMutation, + useReplyToCommentMutation, + useResolveThreadMutation, + useUnresolveThreadMutation, +} from './use-review-discussion-mutations'; const hoistedKeys = vi.hoisted(() => ({ getKey: vi.fn(() => 'hoisted-op-key'), @@ -32,13 +40,31 @@ vi.mock('@/lib/operation-key', async importOriginal => { type MutationOptions = { mutationFn?: (vars: unknown) => Promise; - onError?: (error: unknown) => void; + onMutate?: (vars: unknown) => Promise | unknown; + onError?: (error: unknown, vars?: unknown, context?: unknown) => void; onSettled?: (data?: unknown, error?: unknown, vars?: unknown) => Promise | void; + scope?: { id: string }; }; +function captureOptions(run: () => unknown): MutationOptions { + lastCapturedOptions = null; + run(); + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + if (!lastCapturedOptions) { + throw new Error('mutation options not captured'); + } + return lastCapturedOptions; +} + let lastCapturedOptions: MutationOptions | null = null; const replyMutateMock = vi.fn(); +const resolveMutateMock = vi.fn(); +const unresolveMutateMock = vi.fn(); const invalidateQueriesMock = vi.fn(); +const cancelQueriesMock = vi.fn(); +const getQueriesDataMock = vi.fn(); +const setQueriesDataMock = vi.fn(); +const setQueryDataMock = vi.fn(); const toastErrorMock = vi.fn(); vi.mock('@tanstack/react-query', () => ({ @@ -50,6 +76,12 @@ vi.mock('@tanstack/react-query', () => ({ invalidateQueries: (...args: unknown[]) => { invalidateQueriesMock(...args); }, + cancelQueries: (...args: unknown[]) => { + cancelQueriesMock(...args); + }, + getQueriesData: (...args: unknown[]) => getQueriesDataMock(...args), + setQueriesData: (...args: unknown[]) => setQueriesDataMock(...args), + setQueryData: (...args: unknown[]) => setQueryDataMock(...args), }), })); @@ -57,12 +89,18 @@ vi.mock('@/lib/trpc', () => ({ useTRPC: () => ({ githubPrReview: { listReviewThreads: { pathFilter: () => ['githubPrReview', 'listReviewThreads'] }, + addReaction: { mutationOptions: (opts: MutationOptions) => opts }, + removeReaction: { mutationOptions: (opts: MutationOptions) => opts }, }, }), trpcClient: { githubPrReview: { // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule replyToComment: { mutate: (vars: unknown) => replyMutateMock(vars) }, + // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule + resolveThread: { mutate: (vars: unknown) => resolveMutateMock(vars) }, + // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule + unresolveThread: { mutate: (vars: unknown) => unresolveMutateMock(vars) }, }, }, })); @@ -83,7 +121,13 @@ describe('useReplyToCommentMutation (P1-A-08c wiring)', () => { beforeEach(() => { lastCapturedOptions = null; replyMutateMock.mockReset(); + resolveMutateMock.mockReset(); + unresolveMutateMock.mockReset(); invalidateQueriesMock.mockReset(); + cancelQueriesMock.mockReset(); + getQueriesDataMock.mockReset(); + setQueriesDataMock.mockReset(); + setQueryDataMock.mockReset(); toastErrorMock.mockReset(); hoistedKeys.getKey.mockClear(); hoistedKeys.rotateKey.mockClear(); @@ -197,3 +241,229 @@ describe('reply_comment fingerprint (P1-A-08c changed-input)', () => { expect(otherComment).not.toBe(original); }); }); + +describe('useResolveThreadMutation (generation guard + chainSave)', () => { + beforeEach(() => { + lastCapturedOptions = null; + resolveMutateMock.mockReset(); + cancelQueriesMock.mockReset(); + getQueriesDataMock.mockReset(); + setQueriesDataMock.mockReset(); + setQueryDataMock.mockReset(); + toastErrorMock.mockReset(); + }); + + it('wraps the tRPC call in chainSave keyed by threadId (rule 3) and adds no scope.id', async () => { + resolveMutateMock.mockResolvedValue({ threadId: 't1', isResolved: true }); + useResolveThreadMutation(); + const opts = lastCapturedOptions; + if (!opts) { + throw new Error('resolve options not captured'); + } + + await opts.mutationFn?.({ threadId: 't1' }); + expect(resolveMutateMock).toHaveBeenCalledWith({ threadId: 't1' }); + expect(opts.scope).toBeUndefined(); + }); + + it('serializes two resolves for the same thread (second mutationFn starts after the first settles)', async () => { + useResolveThreadMutation(); + const opts = lastCapturedOptions; + if (!opts) { + throw new Error('resolve options not captured'); + } + + const gate = Promise.withResolvers<{ threadId: string; isResolved: boolean }>(); + resolveMutateMock + .mockReturnValueOnce(gate.promise) + .mockResolvedValueOnce({ threadId: 't1', isResolved: true }); + + const first = opts.mutationFn?.({ threadId: 't1' }); + const second = opts.mutationFn?.({ threadId: 't1' }); + + await Promise.resolve(); + await Promise.resolve(); + expect(resolveMutateMock).toHaveBeenCalledTimes(1); + + gate.resolve({ threadId: 't1', isResolved: true }); + await Promise.all([first, second]); + expect(resolveMutateMock).toHaveBeenCalledTimes(2); + }); + + it('a failing older resolve does not roll back while a newer reaction owns the threads cache', async () => { + const resolveOpts = captureOptions(() => useResolveThreadMutation()); + const reactionOpts = captureOptions(() => useAddReactionMutation('t2')); + + getQueriesDataMock + .mockReturnValueOnce([['k1', { pages: [] }]]) + .mockReturnValueOnce([['k2', { pages: [] }]]); + + const older = await resolveOpts.onMutate?.({ threadId: 't1' }); + const newer = await reactionOpts.onMutate?.({ commentNodeId: 'c1', content: 'THUMBS_UP' }); + + setQueryDataMock.mockClear(); + resolveOpts.onError?.(new Error('boom'), { threadId: 't1' }, older); + // The older resolve's rollback must not restore its snapshot over the + // newer reaction's optimistic write to the same procedure-wide cache. + expect(setQueryDataMock).not.toHaveBeenCalled(); + expect(toastErrorMock).toHaveBeenCalledWith('boom'); + + reactionOpts.onError?.(new Error('boom'), { commentNodeId: 'c1', content: 'THUMBS_UP' }, newer); + expect(setQueryDataMock).toHaveBeenCalledTimes(1); + expect(toastErrorMock).toHaveBeenCalledWith('boom'); + }); + + it('a failing latest resolve rolls back its snapshot and toasts', async () => { + useResolveThreadMutation(); + const opts = lastCapturedOptions; + if (!opts) { + throw new Error('resolve options not captured'); + } + + getQueriesDataMock.mockReturnValueOnce([['k1', { pages: [] }]]); + const context = await opts.onMutate?.({ threadId: 't1' }); + + setQueryDataMock.mockClear(); + opts.onError?.(new Error('boom'), { threadId: 't1' }, context); + expect(setQueryDataMock).toHaveBeenCalledTimes(1); + expect(toastErrorMock).toHaveBeenCalledWith('boom'); + }); +}); + +describe('useUnresolveThreadMutation (generation guard + chainSave)', () => { + beforeEach(() => { + lastCapturedOptions = null; + unresolveMutateMock.mockReset(); + getQueriesDataMock.mockReset(); + setQueryDataMock.mockReset(); + toastErrorMock.mockReset(); + }); + + it('wraps the tRPC call in chainSave keyed by threadId and adds no scope.id', async () => { + unresolveMutateMock.mockResolvedValue({ threadId: 't1', isResolved: false }); + useUnresolveThreadMutation(); + const opts = lastCapturedOptions; + if (!opts) { + throw new Error('unresolve options not captured'); + } + + await opts.mutationFn?.({ threadId: 't1' }); + expect(unresolveMutateMock).toHaveBeenCalledWith({ threadId: 't1' }); + expect(opts.scope).toBeUndefined(); + }); + + it('a failing latest unresolve rolls back its snapshot and toasts', async () => { + useUnresolveThreadMutation(); + const opts = lastCapturedOptions; + if (!opts) { + throw new Error('unresolve options not captured'); + } + + getQueriesDataMock.mockReturnValueOnce([['k1', { pages: [] }]]); + const context = await opts.onMutate?.({ threadId: 't1' }); + + setQueryDataMock.mockClear(); + opts.onError?.(new Error('boom'), { threadId: 't1' }, context); + expect(setQueryDataMock).toHaveBeenCalledTimes(1); + expect(toastErrorMock).toHaveBeenCalledWith('boom'); + }); +}); + +describe('useAddReactionMutation (generation guard + scope.id)', () => { + beforeEach(() => { + lastCapturedOptions = null; + getQueriesDataMock.mockReset(); + setQueryDataMock.mockReset(); + toastErrorMock.mockReset(); + }); + + it('scopes the mutation per thread from the hook closure (rule 2)', () => { + useAddReactionMutation('t1'); + expect(lastCapturedOptions?.scope).toEqual({ id: 'pr-thread:t1' }); + }); + + it('a failing latest reaction rolls back its snapshot and toasts', async () => { + useAddReactionMutation('t1'); + const opts = lastCapturedOptions; + if (!opts) { + throw new Error('reaction options not captured'); + } + + getQueriesDataMock.mockReturnValueOnce([['k1', { pages: [] }]]); + const context = await opts.onMutate?.({ commentNodeId: 'c1', content: 'THUMBS_UP' }); + + setQueryDataMock.mockClear(); + opts.onError?.(new Error('boom'), { commentNodeId: 'c1', content: 'THUMBS_UP' }, context); + expect(setQueryDataMock).toHaveBeenCalledTimes(1); + expect(toastErrorMock).toHaveBeenCalledWith('boom'); + }); +}); + +describe('useRemoveReactionMutation (generation guard + scope.id)', () => { + beforeEach(() => { + lastCapturedOptions = null; + getQueriesDataMock.mockReset(); + setQueryDataMock.mockReset(); + toastErrorMock.mockReset(); + }); + + it('scopes the mutation per thread from the hook closure (rule 2)', () => { + useRemoveReactionMutation('t1'); + expect(lastCapturedOptions?.scope).toEqual({ id: 'pr-thread:t1' }); + }); + + it('a failing latest reaction removal rolls back its snapshot and toasts', async () => { + useRemoveReactionMutation('t1'); + const opts = lastCapturedOptions; + if (!opts) { + throw new Error('reaction options not captured'); + } + + getQueriesDataMock.mockReturnValueOnce([['k1', { pages: [] }]]); + const context = await opts.onMutate?.({ commentNodeId: 'c1', content: 'THUMBS_UP' }); + + setQueryDataMock.mockClear(); + opts.onError?.(new Error('boom'), { commentNodeId: 'c1', content: 'THUMBS_UP' }, context); + expect(setQueryDataMock).toHaveBeenCalledTimes(1); + expect(toastErrorMock).toHaveBeenCalledWith('boom'); + }); +}); + +describe('scope.id network serialization (real MutationCache)', () => { + it('starts the second same-scope mutationFn only after the first settles', async () => { + const { MutationCache, QueryClient } = + await vi.importActual('@tanstack/react-query'); + const cache = new MutationCache(); + const client = new QueryClient({ mutationCache: cache }); + const order: string[] = []; + const gate = Promise.withResolvers(); + + const first = cache.build(client, { + mutationFn: async () => { + order.push('first-start'); + await gate.promise; + order.push('first-end'); + return 'first'; + }, + scope: { id: 'pr-thread:t1' }, + }); + const second = cache.build(client, { + // eslint-disable-next-line require-await, typescript-eslint/require-await -- MutationFunction requires a Promise return + mutationFn: async () => { + order.push('second-start'); + return 'second'; + }, + scope: { id: 'pr-thread:t1' }, + }); + + const p1 = first.execute({}); + const p2 = second.execute({}); + await Promise.resolve(); + await Promise.resolve(); + expect(order).toEqual(['first-start']); + + gate.resolve(null); + await Promise.all([p1, p2]); + expect(order).toEqual(['first-start', 'first-end', 'second-start']); + }); +}); diff --git a/apps/mobile/src/lib/pr-review/discussion/use-review-discussion-mutations.ts b/apps/mobile/src/lib/pr-review/discussion/use-review-discussion-mutations.ts index d2662d9c80..647d88ccd5 100644 --- a/apps/mobile/src/lib/pr-review/discussion/use-review-discussion-mutations.ts +++ b/apps/mobile/src/lib/pr-review/discussion/use-review-discussion-mutations.ts @@ -38,6 +38,11 @@ import { toast } from 'sonner-native'; import { prIntentFingerprint } from '@kilocode/app-shared/pr-review'; +import { + isLatestMutationGeneration, + nextMutationGeneration, +} from '@/lib/hooks/mutation-generations'; +import { chainSave } from '@/lib/hooks/save-chain'; import { trpcClient, useTRPC } from '@/lib/trpc'; import { useHoistedOperationKey } from '@/lib/operation-key'; import { @@ -60,6 +65,11 @@ function useDiscussionKeys() { }; } +// Every discussion mutation snapshots the same procedure-wide +// `listReviewThreads` cache through its path filter, so one shared generation +// key guards all rollbacks across resolve/unresolve/reaction writes. +const LIST_REVIEW_THREADS_GENERATION_KEY = 'githubPrReview.listReviewThreads'; + async function invalidateDiscussionCaches( queryClient: ReturnType, keys: ReturnType @@ -110,71 +120,83 @@ export function useReplyToCommentMutation() { // ── Resolve / unresolve (optimistic) ────────────────────────────────── export function useResolveThreadMutation() { - const trpc = useTRPC(); const queryClient = useQueryClient(); const keys = useDiscussionKeys(); - return useMutation( - trpc.githubPrReview.resolveThread.mutationOptions({ + // onError policy: roll back the onMutate snapshot (latest generation only) + // and toast error.message. + return useMutation({ + mutationFn: (vars: { threadId: string }) => // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule - onMutate: async ({ threadId }) => { - await queryClient.cancelQueries(keys.listReviewThreadsPath); - const previous = queryClient.getQueriesData( - keys.listReviewThreadsPath - ); - queryClient.setQueriesData(keys.listReviewThreadsPath, old => - applyResolveToggle(old, threadId, true) - ); - return { previous }; - }, - onError: (error, _input, context) => { - const previous = context?.previous; - if (previous) { - for (const [key, data] of previous) { - queryClient.setQueryData(key, data); - } + chainSave(`pr-thread:${vars.threadId}`, () => + trpcClient.githubPrReview.resolveThread.mutate(vars) + ), + onMutate: async ({ threadId }) => { + await queryClient.cancelQueries(keys.listReviewThreadsPath); + const generation = nextMutationGeneration(LIST_REVIEW_THREADS_GENERATION_KEY); + const previous = queryClient.getQueriesData( + keys.listReviewThreadsPath + ); + queryClient.setQueriesData(keys.listReviewThreadsPath, old => + applyResolveToggle(old, threadId, true) + ); + return { previous, generation }; + }, + onError: (error, _input, context) => { + if ( + context?.previous && + isLatestMutationGeneration(LIST_REVIEW_THREADS_GENERATION_KEY, context.generation) + ) { + for (const [key, data] of context.previous) { + queryClient.setQueryData(key, data); } - toast.error(error.message); - }, - onSettled: async () => { - await invalidateDiscussionCaches(queryClient, keys); - }, - }) - ); + } + toast.error(error.message); + }, + onSettled: async () => { + await invalidateDiscussionCaches(queryClient, keys); + }, + }); } export function useUnresolveThreadMutation() { - const trpc = useTRPC(); const queryClient = useQueryClient(); const keys = useDiscussionKeys(); - return useMutation( - trpc.githubPrReview.unresolveThread.mutationOptions({ + // onError policy: roll back the onMutate snapshot (latest generation only) + // and toast error.message. + return useMutation({ + mutationFn: (vars: { threadId: string }) => // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule - onMutate: async ({ threadId }) => { - await queryClient.cancelQueries(keys.listReviewThreadsPath); - const previous = queryClient.getQueriesData( - keys.listReviewThreadsPath - ); - queryClient.setQueriesData(keys.listReviewThreadsPath, old => - applyResolveToggle(old, threadId, false) - ); - return { previous }; - }, - onError: (error, _input, context) => { - const previous = context?.previous; - if (previous) { - for (const [key, data] of previous) { - queryClient.setQueryData(key, data); - } + chainSave(`pr-thread:${vars.threadId}`, () => + trpcClient.githubPrReview.unresolveThread.mutate(vars) + ), + onMutate: async ({ threadId }) => { + await queryClient.cancelQueries(keys.listReviewThreadsPath); + const generation = nextMutationGeneration(LIST_REVIEW_THREADS_GENERATION_KEY); + const previous = queryClient.getQueriesData( + keys.listReviewThreadsPath + ); + queryClient.setQueriesData(keys.listReviewThreadsPath, old => + applyResolveToggle(old, threadId, false) + ); + return { previous, generation }; + }, + onError: (error, _input, context) => { + if ( + context?.previous && + isLatestMutationGeneration(LIST_REVIEW_THREADS_GENERATION_KEY, context.generation) + ) { + for (const [key, data] of context.previous) { + queryClient.setQueryData(key, data); } - toast.error(error.message); - }, - onSettled: async () => { - await invalidateDiscussionCaches(queryClient, keys); - }, - }) - ); + } + toast.error(error.message); + }, + onSettled: async () => { + await invalidateDiscussionCaches(queryClient, keys); + }, + }); } // ── Reactions (optimistic) ──────────────────────────────────────────── @@ -188,11 +210,14 @@ export function useAddReactionMutation(threadId: string) { const queryClient = useQueryClient(); const keys = useDiscussionKeys(); + // onError policy: roll back the onMutate snapshot (latest generation only) + // and toast error.message. return useMutation( trpc.githubPrReview.addReaction.mutationOptions({ // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule onMutate: async ({ commentNodeId, content }) => { await queryClient.cancelQueries(keys.listReviewThreadsPath); + const generation = nextMutationGeneration(LIST_REVIEW_THREADS_GENERATION_KEY); const previous = queryClient.getQueriesData( keys.listReviewThreadsPath ); @@ -204,12 +229,14 @@ export function useAddReactionMutation(threadId: string) { content: content as ReviewReactionContent, }) ); - return { previous }; + return { previous, generation }; }, onError: (error, _input, context) => { - const previous = context?.previous; - if (previous) { - for (const [key, data] of previous) { + if ( + context?.previous && + isLatestMutationGeneration(LIST_REVIEW_THREADS_GENERATION_KEY, context.generation) + ) { + for (const [key, data] of context.previous) { queryClient.setQueryData(key, data); } } @@ -218,6 +245,10 @@ export function useAddReactionMutation(threadId: string) { onSettled: async () => { await invalidateDiscussionCaches(queryClient, keys); }, + // The reaction DTO carries only {commentNodeId, content}; the owning + // threadId comes from the hook closure, so scope.id serializes network + // calls per thread (rule 2). + scope: { id: `pr-thread:${threadId}` }, }) ); } @@ -227,11 +258,14 @@ export function useRemoveReactionMutation(threadId: string) { const queryClient = useQueryClient(); const keys = useDiscussionKeys(); + // onError policy: roll back the onMutate snapshot (latest generation only) + // and toast error.message. return useMutation( trpc.githubPrReview.removeReaction.mutationOptions({ // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule onMutate: async ({ commentNodeId, content }) => { await queryClient.cancelQueries(keys.listReviewThreadsPath); + const generation = nextMutationGeneration(LIST_REVIEW_THREADS_GENERATION_KEY); const previous = queryClient.getQueriesData( keys.listReviewThreadsPath ); @@ -243,12 +277,14 @@ export function useRemoveReactionMutation(threadId: string) { content: content as ReviewReactionContent, }) ); - return { previous }; + return { previous, generation }; }, onError: (error, _input, context) => { - const previous = context?.previous; - if (previous) { - for (const [key, data] of previous) { + if ( + context?.previous && + isLatestMutationGeneration(LIST_REVIEW_THREADS_GENERATION_KEY, context.generation) + ) { + for (const [key, data] of context.previous) { queryClient.setQueryData(key, data); } } @@ -257,6 +293,10 @@ export function useRemoveReactionMutation(threadId: string) { onSettled: async () => { await invalidateDiscussionCaches(queryClient, keys); }, + // The reaction DTO carries only {commentNodeId, content}; the owning + // threadId comes from the hook closure, so scope.id serializes network + // calls per thread (rule 2). + scope: { id: `pr-thread:${threadId}` }, }) ); } From bd98738f10bd1fe56f99d682405e43d7e34b03fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Fri, 21 Aug 2026 05:57:40 +0200 Subject: [PATCH 06/14] feat(mobile): add route-scoped foreground refresh hook Add useRouteForegroundRefresh to invalidate route query keys on the focused AppState foreground transition and on focus regain after the first. Disable the blanket refetchOnWindowFocus default so frozen background tabs no longer refetch. --- ...-route-foreground-refresh.mounted.test.tsx | 154 ++++++++++++++++++ .../lib/hooks/use-route-foreground-refresh.ts | 59 +++++++ apps/mobile/src/lib/query-client.ts | 2 + 3 files changed, 215 insertions(+) create mode 100644 apps/mobile/src/lib/hooks/use-route-foreground-refresh.mounted.test.tsx create mode 100644 apps/mobile/src/lib/hooks/use-route-foreground-refresh.ts diff --git a/apps/mobile/src/lib/hooks/use-route-foreground-refresh.mounted.test.tsx b/apps/mobile/src/lib/hooks/use-route-foreground-refresh.mounted.test.tsx new file mode 100644 index 0000000000..7b893b634a --- /dev/null +++ b/apps/mobile/src/lib/hooks/use-route-foreground-refresh.mounted.test.tsx @@ -0,0 +1,154 @@ +/* eslint-disable typescript-eslint/no-deprecated -- react-test-renderer is the DOM-free renderer used to mount React/RN trees under vitest (same pattern as src/lib/hooks/use-force-update.mounted.test.tsx) */ +import { createElement } from 'react'; +import TestRenderer, { act } from 'react-test-renderer'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { useRouteForegroundRefresh } from '@/lib/hooks/use-route-foreground-refresh'; + +const invalidateQueries = vi.hoisted(() => vi.fn()); + +const appState = vi.hoisted(() => { + const listeners = new Set<(state: string) => void>(); + return { + listeners, + addEventListener: (_event: string, listener: (state: string) => void) => { + listeners.add(listener); + return { + remove: () => { + listeners.delete(listener); + }, + }; + }, + emit: (state: string): void => { + for (const listener of listeners) { + listener(state); + } + }, + }; +}); + +const focusState = vi.hoisted(() => ({ + isFocused: true, +})); + +// Captures the useFocusEffect callback so a test can simulate a focus event. +const focusEffect = vi.hoisted(() => ({ + effect: undefined as (() => void) | undefined, +})); + +vi.mock('react-native', () => ({ + AppState: { addEventListener: appState.addEventListener }, +})); + +vi.mock('@tanstack/react-query', () => ({ + useQueryClient: () => ({ invalidateQueries }), +})); + +vi.mock('expo-router', () => ({ + useIsFocused: () => focusState.isFocused, + useFocusEffect: (effect: () => void) => { + focusEffect.effect = effect; + }, +})); + +const KEYS: readonly (readonly unknown[])[] = [['securityAgent'], ['organizations']]; + +function Probe({ queryKeys }: { queryKeys: readonly (readonly unknown[])[] }) { + useRouteForegroundRefresh(queryKeys); + return createElement('ProbeText', null, 'probe'); +} + +const mountedRenderers: TestRenderer.ReactTestRenderer[] = []; + +async function renderProbe( + queryKeys: readonly (readonly unknown[])[] +): Promise { + const rendererRef: { current: TestRenderer.ReactTestRenderer | undefined } = { + current: undefined, + }; + await act(async () => { + await Promise.resolve(); + rendererRef.current = TestRenderer.create(createElement(Probe, { queryKeys })); + }); + const renderer = rendererRef.current; + if (!renderer) { + throw new Error('renderer was not created'); + } + mountedRenderers.push(renderer); + return renderer; +} + +function backgroundThenActive(): void { + act(() => { + appState.emit('background'); + }); + act(() => { + appState.emit('active'); + }); +} + +describe('useRouteForegroundRefresh mounted', () => { + beforeEach(() => { + (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + focusState.isFocused = true; + focusEffect.effect = undefined; + invalidateQueries.mockReset(); + }); + + afterEach(() => { + act(() => { + for (const renderer of mountedRenderers) { + renderer.unmount(); + } + }); + mountedRenderers.length = 0; + appState.listeners.clear(); + focusEffect.effect = undefined; + }); + + it('invalidates exactly the listed keys on the focused foreground transition', async () => { + await renderProbe(KEYS); + + backgroundThenActive(); + + expect(invalidateQueries).toHaveBeenCalledTimes(2); + expect(invalidateQueries).toHaveBeenNthCalledWith(1, { queryKey: ['securityAgent'] }); + expect(invalidateQueries).toHaveBeenNthCalledWith(2, { queryKey: ['organizations'] }); + }); + + it('invalidates nothing on the foreground transition while unfocused', async () => { + focusState.isFocused = false; + await renderProbe(KEYS); + + backgroundThenActive(); + + expect(invalidateQueries).not.toHaveBeenCalled(); + }); + + it('invalidates nothing on the first focus (mount)', async () => { + await renderProbe(KEYS); + + act(() => { + focusEffect.effect?.(); + }); + + expect(invalidateQueries).not.toHaveBeenCalled(); + }); + + it('invalidates the keys on a later focus regain', async () => { + await renderProbe(KEYS); + + act(() => { + focusEffect.effect?.(); + }); + expect(invalidateQueries).not.toHaveBeenCalled(); + + act(() => { + focusEffect.effect?.(); + }); + + expect(invalidateQueries).toHaveBeenCalledTimes(2); + expect(invalidateQueries).toHaveBeenNthCalledWith(1, { queryKey: ['securityAgent'] }); + expect(invalidateQueries).toHaveBeenNthCalledWith(2, { queryKey: ['organizations'] }); + }); +}); diff --git a/apps/mobile/src/lib/hooks/use-route-foreground-refresh.ts b/apps/mobile/src/lib/hooks/use-route-foreground-refresh.ts new file mode 100644 index 0000000000..dc6e693c3a --- /dev/null +++ b/apps/mobile/src/lib/hooks/use-route-foreground-refresh.ts @@ -0,0 +1,59 @@ +import { useQueryClient } from '@tanstack/react-query'; +import { useFocusEffect, useIsFocused } from 'expo-router'; +import { useCallback, useEffect, useRef } from 'react'; + +import { useAppLifecycle } from '@/lib/hooks/use-app-lifecycle'; + +/** + * Invalidates the given tRPC path-prefix query keys when the route regains + * foreground freshness: + * + * - on the AppState false -> true transition to active while the route is + * focused, and + * - on every route focus regain after the first (the mount focus is already + * covered by refetchOnMount). + * + * Keys are tRPC path-prefix arrays (e.g. `[['securityAgent']]`), which match + * every procedure and scope variant under the prefix; only mounted observers + * refetch. + */ +export function useRouteForegroundRefresh(queryKeys: readonly (readonly unknown[])[]): void { + const queryClient = useQueryClient(); + const { isActive } = useAppLifecycle(); + const isFocused = useIsFocused(); + + // Keep the latest keys in a ref so the focus effect callback stays stable: + // the owner passes an inline array literal, which is a fresh reference every + // render, and a changing useFocusEffect callback would re-run on every render. + const queryKeysRef = useRef(queryKeys); + queryKeysRef.current = queryKeys; + + const wasActiveRef = useRef(isActive); + const focusedRef = useRef(isFocused); + const firstFocusRef = useRef(true); + + useEffect(() => { + focusedRef.current = isFocused; + }, [isFocused]); + + useEffect(() => { + if (!wasActiveRef.current && isActive && focusedRef.current) { + for (const queryKey of queryKeysRef.current) { + void queryClient.invalidateQueries({ queryKey }); + } + } + wasActiveRef.current = isActive; + }, [isActive, queryClient]); + + useFocusEffect( + useCallback(() => { + if (firstFocusRef.current) { + firstFocusRef.current = false; + return; + } + for (const queryKey of queryKeysRef.current) { + void queryClient.invalidateQueries({ queryKey }); + } + }, [queryClient]) + ); +} diff --git a/apps/mobile/src/lib/query-client.ts b/apps/mobile/src/lib/query-client.ts index 5a861b4b57..a90b4b8305 100644 --- a/apps/mobile/src/lib/query-client.ts +++ b/apps/mobile/src/lib/query-client.ts @@ -98,6 +98,8 @@ export function createKiloAppQueryClient(): QueryClient { const queryClient = new QueryClient({ defaultOptions: { queries: { + // Foreground freshness is owned per route by useRouteForegroundRefresh mounts; the blanket focusManager refetch also woke frozen background tabs. + refetchOnWindowFocus: false, retry: (failureCount, error) => { const code = (error as { data?: { code?: string } } | null)?.data?.code; if (code !== undefined && PERMANENT_CODES.has(code)) { From 5130066b6d6d36277e1004db7d603bc5c1fd5878 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Fri, 21 Aug 2026 06:29:14 +0200 Subject: [PATCH 07/14] feat(mobile): mount route-scoped foreground refresh owners Mount useRouteForegroundRefresh at the app-wide, home-tab, profile-tab, and pushed-route surfaces from plan section 10. Each path-prefix key uses the nested tRPC form so invalidation prefix-matches tRPC v11 keys. Remove the profile credits card's bespoke focus invalidation, now subsumed by the profile-tab owner. Add a real-QueryClient key-matching test. --- .../src/app/(app)/(tabs)/(0_home)/_layout.tsx | 3 + .../app/(app)/(tabs)/(3_profile)/_layout.tsx | 9 ++ apps/mobile/src/app/(app)/_layout.tsx | 21 +++++ .../src/app/(app)/agent-chat/[session-id].tsx | 2 + .../src/app/(app)/agent-chat/model-picker.tsx | 2 + apps/mobile/src/app/(app)/device-sessions.tsx | 2 + apps/mobile/src/app/(app)/kilo-pass.tsx | 2 + .../[owner]/[repo]/[number]/_layout.tsx | 2 + apps/mobile/src/app/(app)/pr-review/index.tsx | 2 + .../src/components/profile-credits-card.tsx | 22 +---- ...-route-foreground-refresh.mounted.test.tsx | 88 ++++++++++++++++++- 11 files changed, 131 insertions(+), 24 deletions(-) diff --git a/apps/mobile/src/app/(app)/(tabs)/(0_home)/_layout.tsx b/apps/mobile/src/app/(app)/(tabs)/(0_home)/_layout.tsx index 74598f7148..7c93bb9743 100644 --- a/apps/mobile/src/app/(app)/(tabs)/(0_home)/_layout.tsx +++ b/apps/mobile/src/app/(app)/(tabs)/(0_home)/_layout.tsx @@ -1,9 +1,12 @@ import { Stack } from 'expo-router'; +import { useRouteForegroundRefresh } from '@/lib/hooks/use-route-foreground-refresh'; + export const unstable_settings = { initialRouteName: 'index', }; export default function HomeLayout() { + useRouteForegroundRefresh([[['activeSessions']]]); return ; } diff --git a/apps/mobile/src/app/(app)/(tabs)/(3_profile)/_layout.tsx b/apps/mobile/src/app/(app)/(tabs)/(3_profile)/_layout.tsx index a452d1e276..e77c1584b5 100644 --- a/apps/mobile/src/app/(app)/(tabs)/(3_profile)/_layout.tsx +++ b/apps/mobile/src/app/(app)/(tabs)/(3_profile)/_layout.tsx @@ -1,9 +1,18 @@ import { Stack } from 'expo-router'; +import { useRouteForegroundRefresh } from '@/lib/hooks/use-route-foreground-refresh'; + export const unstable_settings = { initialRouteName: 'index', }; export default function ProfileLayout() { + useRouteForegroundRefresh([ + [['user']], + [['organizations']], + [['personalReviewAgent']], + [['securityAgent']], + [['kiloPass']], + ]); return ; } diff --git a/apps/mobile/src/app/(app)/_layout.tsx b/apps/mobile/src/app/(app)/_layout.tsx index 449437e0f9..caa1580303 100644 --- a/apps/mobile/src/app/(app)/_layout.tsx +++ b/apps/mobile/src/app/(app)/_layout.tsx @@ -15,7 +15,9 @@ import { import { useFormSheetDetents } from '@/lib/form-sheet'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; import { useCurrentUserId } from '@/lib/hooks/use-current-user-id'; +import { useRouteForegroundRefresh } from '@/lib/hooks/use-route-foreground-refresh'; import { CachePersistenceMount } from '@/lib/persist/cache-persistence-mount'; +import { useTRPC } from '@/lib/trpc'; /** * Attempts failed logout cleanup on every "next authenticated opportunity": @@ -82,6 +84,24 @@ function PushRegistrationMount() { return null; } +/** + * Refreshes app-wide freshness on foreground regain: the signed-in user, + * their organizations, and kilo-chat conversations. The root `(app)` layout + * is always focused, so the hook's focus gate never blocks this mount. + */ +function AppWideFreshnessMount() { + const trpc = useTRPC(); + useRouteForegroundRefresh([ + trpc.user.getMe.queryKey(), + trpc.organizations.list.queryKey(), + // Kilo-chat keys are FLAT (['kilo-chat', 'conversations', …]), so the + // partial key is the flat ['kilo-chat']; the nested tRPC form + // [['kilo-chat']] does not prefix-match flat keys. + ['kilo-chat'], + ]); + return null; +} + export default function AppLayout() { const colors = useThemeColors(); const { fullSheetDetent } = useFormSheetDetents(); @@ -92,6 +112,7 @@ export default function AppLayout() { + 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 3785289d63..b989c2b13a 100644 --- a/apps/mobile/src/app/(app)/agent-chat/[session-id].tsx +++ b/apps/mobile/src/app/(app)/agent-chat/[session-id].tsx @@ -16,6 +16,7 @@ import { QueryError } from '@/components/query-error'; import { ScreenHeader } from '@/components/screen-header'; import { Button } from '@/components/ui/button'; import { Text } from '@/components/ui/text'; +import { useRouteForegroundRefresh } from '@/lib/hooks/use-route-foreground-refresh'; import { shouldRetryNotFoundOnSpawnedRoute } from '@/lib/spawned-not-found-retry'; import { useTRPC } from '@/lib/trpc'; @@ -54,6 +55,7 @@ export default function SessionDetailScreen() { const spawnedMode = Array.isArray(modeParam) ? modeParam[0] : modeParam; const trpc = useTRPC(); const router = useRouter(); + useRouteForegroundRefresh([[['cliSessionsV2']], [['modelPreferences']]]); const sessionQuery = useQuery({ ...trpc.cliSessionsV2.get.queryOptions( { session_id: sessionId }, diff --git a/apps/mobile/src/app/(app)/agent-chat/model-picker.tsx b/apps/mobile/src/app/(app)/agent-chat/model-picker.tsx index 392a6fd043..00397247c8 100644 --- a/apps/mobile/src/app/(app)/agent-chat/model-picker.tsx +++ b/apps/mobile/src/app/(app)/agent-chat/model-picker.tsx @@ -1,5 +1,7 @@ import { ModelPickerContent } from '@/components/agents/model-picker-content'; +import { useRouteForegroundRefresh } from '@/lib/hooks/use-route-foreground-refresh'; export default function ModelPickerScreen() { + useRouteForegroundRefresh([[['modelPreferences']]]); return ; } diff --git a/apps/mobile/src/app/(app)/device-sessions.tsx b/apps/mobile/src/app/(app)/device-sessions.tsx index 04bd86e9d5..daf269c140 100644 --- a/apps/mobile/src/app/(app)/device-sessions.tsx +++ b/apps/mobile/src/app/(app)/device-sessions.tsx @@ -1,5 +1,7 @@ import { DeviceSessionsScreen } from '@/components/device-sessions-screen'; +import { useRouteForegroundRefresh } from '@/lib/hooks/use-route-foreground-refresh'; export default function DeviceSessionsRoute() { + useRouteForegroundRefresh([[['user']]]); return ; } diff --git a/apps/mobile/src/app/(app)/kilo-pass.tsx b/apps/mobile/src/app/(app)/kilo-pass.tsx index 0612882c4e..e645ec78a8 100644 --- a/apps/mobile/src/app/(app)/kilo-pass.tsx +++ b/apps/mobile/src/app/(app)/kilo-pass.tsx @@ -1,5 +1,7 @@ import { KiloPassSubscriptionScreen } from '@/components/kilo-pass/kilo-pass-subscription-screen'; +import { useRouteForegroundRefresh } from '@/lib/hooks/use-route-foreground-refresh'; export default function KiloPassRoute() { + useRouteForegroundRefresh([[['kiloPass']]]); return ; } diff --git a/apps/mobile/src/app/(app)/pr-review/[owner]/[repo]/[number]/_layout.tsx b/apps/mobile/src/app/(app)/pr-review/[owner]/[repo]/[number]/_layout.tsx index 3d8b4cc16d..99ff431b52 100644 --- a/apps/mobile/src/app/(app)/pr-review/[owner]/[repo]/[number]/_layout.tsx +++ b/apps/mobile/src/app/(app)/pr-review/[owner]/[repo]/[number]/_layout.tsx @@ -3,6 +3,7 @@ import { type Href, Stack, useLocalSearchParams } from 'expo-router'; import { InvalidRouteState } from '@/components/invalid-route-state'; import { PrReviewConnectGate } from '@/components/pr-review/pr-review-connect-gate'; import { useCurrentUserId } from '@/lib/hooks/use-current-user-id'; +import { useRouteForegroundRefresh } from '@/lib/hooks/use-route-foreground-refresh'; import { pendingReviewDraftKey, PendingReviewProvider, @@ -39,6 +40,7 @@ export default function PrReviewNumberLayout() { const number = rawNumber ? Number.parseInt(rawNumber, 10) : Number.NaN; const { fullSheetDetent } = useFormSheetDetents(); const { userId } = useCurrentUserId(); + useRouteForegroundRefresh([[['githubPrReview']]]); if (!owner || !repo || !Number.isInteger(number) || number <= 0) { return ; diff --git a/apps/mobile/src/app/(app)/pr-review/index.tsx b/apps/mobile/src/app/(app)/pr-review/index.tsx index 2514d0c877..737b6e45e0 100644 --- a/apps/mobile/src/app/(app)/pr-review/index.tsx +++ b/apps/mobile/src/app/(app)/pr-review/index.tsx @@ -1,7 +1,9 @@ import { PrReviewConnectGate } from '@/components/pr-review/pr-review-connect-gate'; import { PrReviewEntryScreen } from '@/components/pr-review/pr-review-entry-screen'; +import { useRouteForegroundRefresh } from '@/lib/hooks/use-route-foreground-refresh'; export default function PrReviewIndexRoute() { + useRouteForegroundRefresh([[['githubPrReview']]]); return ( diff --git a/apps/mobile/src/components/profile-credits-card.tsx b/apps/mobile/src/components/profile-credits-card.tsx index 5b0be1d43d..4cb9ec358b 100644 --- a/apps/mobile/src/components/profile-credits-card.tsx +++ b/apps/mobile/src/components/profile-credits-card.tsx @@ -1,8 +1,6 @@ import { useActionSheet } from '@expo/react-native-action-sheet'; import { formatDollars, fromMicrodollars } from '@kilocode/app-shared/utils'; -import { keepPreviousData, useQuery, useQueryClient } from '@tanstack/react-query'; -import { useFocusEffect } from 'expo-router'; -import { useCallback, useRef } from 'react'; +import { keepPreviousData, useQuery } from '@tanstack/react-query'; import { ChevronDown } from '@/components/ui/icons'; import { ActivityIndicator, Platform, Pressable, View } from 'react-native'; import { useSafeAreaInsets } from 'react-native-safe-area-context'; @@ -31,24 +29,6 @@ export function CreditsCard({ enabled, orgs }: Readonly) { const { bottom } = useSafeAreaInsets(); const { organizationId, setOrganizationId } = useOrganization(); const selectedOrgId = organizationId ?? undefined; - const queryClient = useQueryClient(); - - // Credits and pass state also change from webhooks (renewal, refund, expiry), - // which no client action invalidates. Refetching when the tab regains focus - // catches those without polling in the background. The first focus is the - // mount fetch, so it is skipped. - const tabFocusedBeforeRef = useRef(false); - useFocusEffect( - useCallback(() => { - if (!tabFocusedBeforeRef.current) { - tabFocusedBeforeRef.current = true; - return; - } - void queryClient.invalidateQueries(trpc.user.getContextBalance.pathFilter()); - void queryClient.invalidateQueries(trpc.user.getCreditBlocks.pathFilter()); - void queryClient.invalidateQueries(trpc.kiloPass.getState.pathFilter()); - }, [queryClient, trpc]) - ); const { data: balance, diff --git a/apps/mobile/src/lib/hooks/use-route-foreground-refresh.mounted.test.tsx b/apps/mobile/src/lib/hooks/use-route-foreground-refresh.mounted.test.tsx index 7b893b634a..60e03520bf 100644 --- a/apps/mobile/src/lib/hooks/use-route-foreground-refresh.mounted.test.tsx +++ b/apps/mobile/src/lib/hooks/use-route-foreground-refresh.mounted.test.tsx @@ -1,5 +1,7 @@ /* eslint-disable typescript-eslint/no-deprecated -- react-test-renderer is the DOM-free renderer used to mount React/RN trees under vitest (same pattern as src/lib/hooks/use-force-update.mounted.test.tsx) */ import { createElement } from 'react'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import type * as TanStackReactQuery from '@tanstack/react-query'; import TestRenderer, { act } from 'react-test-renderer'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -7,6 +9,11 @@ import { useRouteForegroundRefresh } from '@/lib/hooks/use-route-foreground-refr const invalidateQueries = vi.hoisted(() => vi.fn()); +// When true, the mocked useQueryClient delegates to the real hook, so a probe +// mounted under a real QueryClientProvider reads the provider's client. The +// existing mocked blocks keep this false and get the pass-through spy. +const useRealQueryClient = vi.hoisted(() => ({ value: false })); + const appState = vi.hoisted(() => { const listeners = new Set<(state: string) => void>(); return { @@ -40,9 +47,14 @@ vi.mock('react-native', () => ({ AppState: { addEventListener: appState.addEventListener }, })); -vi.mock('@tanstack/react-query', () => ({ - useQueryClient: () => ({ invalidateQueries }), -})); +vi.mock('@tanstack/react-query', async importOriginal => { + const actual = await importOriginal(); + return { + ...actual, + useQueryClient: () => + useRealQueryClient.value ? actual.useQueryClient() : { invalidateQueries }, + }; +}); vi.mock('expo-router', () => ({ useIsFocused: () => focusState.isFocused, @@ -78,6 +90,31 @@ async function renderProbe( return renderer; } +async function renderProbeWithProvider( + queryClient: QueryClient, + queryKeys: readonly (readonly unknown[])[] +): Promise { + const rendererRef: { current: TestRenderer.ReactTestRenderer | undefined } = { + current: undefined, + }; + await act(async () => { + await Promise.resolve(); + rendererRef.current = TestRenderer.create( + createElement( + QueryClientProvider, + { client: queryClient }, + createElement(Probe, { queryKeys }) + ) + ); + }); + const renderer = rendererRef.current; + if (!renderer) { + throw new Error('renderer was not created'); + } + mountedRenderers.push(renderer); + return renderer; +} + function backgroundThenActive(): void { act(() => { appState.emit('background'); @@ -152,3 +189,48 @@ describe('useRouteForegroundRefresh mounted', () => { expect(invalidateQueries).toHaveBeenNthCalledWith(2, { queryKey: ['organizations'] }); }); }); + +describe('useRouteForegroundRefresh key matching with a real QueryClient', () => { + beforeEach(() => { + (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + focusState.isFocused = true; + focusEffect.effect = undefined; + useRealQueryClient.value = true; + }); + + afterEach(() => { + useRealQueryClient.value = false; + act(() => { + for (const renderer of mountedRenderers) { + renderer.unmount(); + } + }); + mountedRenderers.length = 0; + appState.listeners.clear(); + focusEffect.effect = undefined; + }); + + it('invalidates a tRPC-shaped query key with the nested prefix form', async () => { + const queryClient = new QueryClient(); + const queryFn = vi.fn().mockResolvedValue('sentinel'); + await queryClient.prefetchQuery({ queryKey: [['user', 'getMe']], queryFn }); + + await renderProbeWithProvider(queryClient, [[['user']]]); + + backgroundThenActive(); + + expect(queryClient.getQueryState([['user', 'getMe']])?.isInvalidated).toBe(true); + }); + + it('does not invalidate a tRPC-shaped query key with the flat prefix form', async () => { + const queryClient = new QueryClient(); + const queryFn = vi.fn().mockResolvedValue('sentinel'); + await queryClient.prefetchQuery({ queryKey: [['user', 'getMe']], queryFn }); + + await renderProbeWithProvider(queryClient, [['user']]); + + backgroundThenActive(); + + expect(queryClient.getQueryState([['user', 'getMe']])?.isInvalidated).toBe(false); + }); +}); From d522fc2f1c23b3bb354d55d7b7cdcb01bb0671bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Fri, 21 Aug 2026 06:29:17 +0200 Subject: [PATCH 08/14] feat(mobile): gate Agents foreground refresh on tab focus Gate the Agents list AppState foreground listener on the tab's focus state so a frozen background tab no longer refetches. The focused tab refreshes the stored list and invalidates the active-sessions tray. Add a mounted test covering focused, unfocused, and blur-after-mount foreground transitions. --- .../session-list-screen.mounted.test.tsx | 234 ++++++++++++++++++ .../components/agents/session-list-screen.tsx | 18 +- 2 files changed, 248 insertions(+), 4 deletions(-) create mode 100644 apps/mobile/src/components/agents/session-list-screen.mounted.test.tsx diff --git a/apps/mobile/src/components/agents/session-list-screen.mounted.test.tsx b/apps/mobile/src/components/agents/session-list-screen.mounted.test.tsx new file mode 100644 index 0000000000..496f9ffcc3 --- /dev/null +++ b/apps/mobile/src/components/agents/session-list-screen.mounted.test.tsx @@ -0,0 +1,234 @@ +/* eslint-disable typescript-eslint/no-deprecated -- react-test-renderer is the DOM-free renderer used to mount React/RN trees under vitest (same pattern as session-list-body-empty.mounted.test.tsx) */ +import { createElement } from 'react'; +import TestRenderer, { act } from 'react-test-renderer'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { AgentSessionListScreen } from './session-list-screen'; + +const appState = vi.hoisted(() => { + const listeners = new Set<(state: string) => void>(); + return { + listeners, + addEventListener: (_event: string, listener: (state: string) => void) => { + listeners.add(listener); + return { + remove: () => { + listeners.delete(listener); + }, + }; + }, + emit: (state: string): void => { + for (const listener of listeners) { + listener(state); + } + }, + }; +}); + +const focusState = vi.hoisted(() => ({ current: true as boolean })); +const focusCallback = vi.hoisted(() => ({ + current: undefined as (() => void) | undefined, +})); +const refetchSpy = vi.hoisted(() => vi.fn()); +const invalidateQueries = vi.hoisted(() => vi.fn()); + +vi.mock('react-native', () => ({ + Platform: { OS: 'ios' }, + Pressable: 'Pressable', + View: 'View', + useWindowDimensions: () => ({ fontScale: 1 }), + AppState: { addEventListener: appState.addEventListener }, +})); +vi.mock('react-native-reanimated', () => ({ + __esModule: true, + default: { View: 'AnimatedView' }, + LinearTransition: 'LinearTransition', +})); +vi.mock('react-native-safe-area-context', () => ({ + useSafeAreaInsets: () => ({ bottom: 0 }), +})); +vi.mock('expo-router', () => ({ + useIsFocused: () => focusState.current, + useFocusEffect: (effect: () => void) => { + focusCallback.current = effect; + }, + useRouter: () => ({ push: vi.fn() }), +})); +vi.mock('@tanstack/react-query', () => ({ + useQueryClient: () => ({ invalidateQueries }), +})); + +vi.mock('@/components/ui/icons', () => ({ + Plus: 'Plus', +})); +vi.mock('@/components/agents/active-now-section', () => ({ + ActiveNowSection: 'ActiveNowSection', +})); +vi.mock('@/components/agents/session-list-content', () => ({ + AgentSessionListContent: 'AgentSessionListContent', + FAB_MARGIN: 0, + FAB_SIZE: 0, +})); +vi.mock('@/components/agents/session-list-header-actions', () => ({ + SessionListHeaderActions: 'SessionListHeaderActions', +})); +vi.mock('@/components/agents/session-list-search-header', () => ({ + SessionListSearchHeader: 'SessionListSearchHeader', +})); +vi.mock('@/components/agents/platform-filter-modal', () => ({ + SessionFilterChips: 'SessionFilterChips', + SessionFilterModal: 'SessionFilterModal', +})); +vi.mock('@/components/screen-header', () => ({ + ScreenHeader: 'ScreenHeader', +})); +vi.mock('@/components/agents/use-session-search-input', () => ({ + useSessionSearchInput: () => ({ + searchQuery: '', + searchInputRef: { current: null }, + hasText: false, + awaitingCommit: false, + handleSearchInputChange: vi.fn(), + handleClearSearchInput: vi.fn(), + clearSearchInput: vi.fn(), + searchController: { clearBroadly: vi.fn() }, + }), +})); +vi.mock('@/components/agents/use-agent-session-navigator', () => ({ + useAgentSessionNavigator: () => vi.fn(), +})); +vi.mock('@/components/agents/use-agent-session-list-data', () => ({ + useAgentSessionListData: () => ({ + storedSessions: [], + activeSessions: [], + activeIsError: false, + isLoading: false, + paging: {}, + refetch: refetchSpy, + handleRetry: vi.fn(), + handleRefetch: vi.fn(), + isSearching: false, + search: { isFetching: false }, + projectOptions: [], + contentIsError: false, + pinnedActive: [], + sections: [], + }), +})); +vi.mock('@/lib/hooks/use-persisted-agent-session-filters', () => ({ + usePersistedAgentSessionFilters: () => ({ + platformFilter: [], + projectFilter: [], + sortBy: 'updated', + hasLoaded: true, + setFilters: vi.fn(), + setPlatformFilter: vi.fn(), + setProjectFilter: vi.fn(), + }), +})); +vi.mock('@/lib/organization-context', () => ({ + useOrganization: () => ({ organizationId: null, isLoaded: true }), +})); +vi.mock('@/lib/hooks/use-theme-colors', () => ({ + useThemeColors: () => ({ primaryForeground: '#ffffff' }), +})); + +const mountedRenderers: TestRenderer.ReactTestRenderer[] = []; + +async function renderScreen(): Promise { + const rendererRef: { current: TestRenderer.ReactTestRenderer | undefined } = { + current: undefined, + }; + await act(async () => { + await Promise.resolve(); + rendererRef.current = TestRenderer.create(createElement(AgentSessionListScreen)); + }); + const renderer = rendererRef.current; + if (!renderer) { + throw new Error('renderer was not created'); + } + mountedRenderers.push(renderer); + return renderer; +} + +describe('AgentSessionListScreen foreground refresh', () => { + beforeEach(() => { + (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + focusState.current = true; + focusCallback.current = undefined; + refetchSpy.mockClear(); + invalidateQueries.mockClear(); + }); + + afterEach(() => { + act(() => { + for (const renderer of mountedRenderers) { + renderer.unmount(); + } + }); + mountedRenderers.length = 0; + appState.listeners.clear(); + vi.restoreAllMocks(); + }); + + it('refetches and invalidates the active-sessions tray on foreground while focused', async () => { + await renderScreen(); + + // The route-focus refetch fires once on mount focus. + act(() => { + focusCallback.current?.(); + }); + expect(refetchSpy).toHaveBeenCalledTimes(1); + + act(() => { + appState.emit('background'); + }); + act(() => { + appState.emit('active'); + }); + + expect(refetchSpy).toHaveBeenCalledTimes(2); + expect(invalidateQueries).toHaveBeenCalledWith({ queryKey: [['activeSessions']] }); + }); + + it('does not refetch or invalidate on foreground while unfocused', async () => { + focusState.current = false; + await renderScreen(); + + act(() => { + appState.emit('background'); + }); + act(() => { + appState.emit('active'); + }); + + expect(refetchSpy).not.toHaveBeenCalled(); + expect(invalidateQueries).not.toHaveBeenCalled(); + }); + + it('does not refetch or invalidate on foreground after focus is lost post-mount', async () => { + const renderer = await renderScreen(); + + // The route-focus refetch fires once on mount focus. + act(() => { + focusCallback.current?.(); + }); + expect(refetchSpy).toHaveBeenCalledTimes(1); + + // Blur the tab after mount. The sync effect must move focusedRef to false. + act(() => { + focusState.current = false; + renderer.update(createElement(AgentSessionListScreen)); + }); + + act(() => { + appState.emit('background'); + }); + act(() => { + appState.emit('active'); + }); + + expect(refetchSpy).toHaveBeenCalledTimes(1); + expect(invalidateQueries).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/mobile/src/components/agents/session-list-screen.tsx b/apps/mobile/src/components/agents/session-list-screen.tsx index bc850d50dc..a514254c62 100644 --- a/apps/mobile/src/components/agents/session-list-screen.tsx +++ b/apps/mobile/src/components/agents/session-list-screen.tsx @@ -1,5 +1,6 @@ import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { AppState, Platform, Pressable, useWindowDimensions, View } from 'react-native'; +import { useQueryClient } from '@tanstack/react-query'; import Animated, { LinearTransition } from 'react-native-reanimated'; import { useSafeAreaInsets } from 'react-native-safe-area-context'; import { Plus } from '@/components/ui/icons'; @@ -26,10 +27,16 @@ import { useOrganization } from '@/lib/organization-context'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; import { getEffectiveTabBarHeight } from '@/lib/tab-bar-layout'; -import { type Href, useFocusEffect, useRouter } from 'expo-router'; +import { type Href, useFocusEffect, useIsFocused, useRouter } from 'expo-router'; export function AgentSessionListScreen() { const router = useRouter(); + const queryClient = useQueryClient(); + const isFocused = useIsFocused(); + const focusedRef = useRef(isFocused); + useEffect(() => { + focusedRef.current = isFocused; + }, [isFocused]); const colors = useThemeColors(); const { bottom } = useSafeAreaInsets(); const { fontScale } = useWindowDimensions(); @@ -102,17 +109,20 @@ export function AgentSessionListScreen() { // from the list hook), so an OS foreground transition must be driven here — // through the same wrapped `refetch` as navigation focus — to keep every // stored refetch serialized by the shared operation coordinator (backfill - // and departure never overlap a refetch). + // and departure never overlap a refetch). A frozen (unfocused) Agents tab + // must NOT refetch on foreground: only the focused tab refreshes the stored + // list and invalidates the active-sessions tray. useEffect(() => { const subscription = AppState.addEventListener('change', nextState => { - if (nextState === 'active') { + if (nextState === 'active' && focusedRef.current) { void refetchRef.current(); + void queryClient.invalidateQueries({ queryKey: [['activeSessions']] }); } }); return () => { subscription.remove(); }; - }, []); + }, [queryClient]); const showSearchBusy = selectShowSearchBusy({ awaitingCommit, From cccdad5eec6bf1332613dc1f0f7ae3bf301f6ada Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Fri, 21 Aug 2026 06:42:45 +0200 Subject: [PATCH 09/14] test(mobile): add offline flapping and process-kill chaos evidence Extend the offline banner, query-client lifecycle, and mutation outbox tests with EV-03-style chaos scenarios: NetInfo flapping drives onlineManager and the banner debounce correctly, and an outbox row survives a simulated relaunch with its operationKey reused. --- .../src/lib/offline-banner-state.test.ts | 30 +++++++++++++++++++ .../src/lib/persist/mutation-outbox.test.ts | 17 +++++++++++ .../src/lib/query-client-lifecycle.test.ts | 27 +++++++++++++++++ 3 files changed, 74 insertions(+) diff --git a/apps/mobile/src/lib/offline-banner-state.test.ts b/apps/mobile/src/lib/offline-banner-state.test.ts index a55cfd4fa5..8e11d5f45b 100644 --- a/apps/mobile/src/lib/offline-banner-state.test.ts +++ b/apps/mobile/src/lib/offline-banner-state.test.ts @@ -187,6 +187,36 @@ describe('createOfflineBannerStore', () => { expect(listener).toHaveBeenCalledTimes(2); }); + it('flapping unknown → offline → online → offline shows only after the debounce and hides immediately on online', () => { + const { store, source, timer } = createStore(); + const listener = vi.fn(() => undefined); + store.subscribe(listener); + + source.emit(unknownState); + timer.firePending(); + expect(store.isOffline()).toBe(false); + expect(listener).not.toHaveBeenCalled(); + + source.emit(offlineState); + // Not committed yet: the banner waits out the show delay. + expect(store.isOffline()).toBe(false); + expect(timer.scheduled[0]?.delayMs).toBe(OFFLINE_BANNER_SHOW_DELAY_MS); + + source.emit(onlineState); + // Hides immediately: the pending offline commit was cancelled. + expect(store.isOffline()).toBe(false); + expect(store.state()).toBe('online'); + + source.emit(offlineState); + timer.firePending(); + expect(store.isOffline()).toBe(true); + expect(store.state()).toBe('offline'); + + // One notification for the unknown → online commit, one for the final + // offline commit. + expect(listener).toHaveBeenCalledTimes(2); + }); + it('destroy with a pending commit cancels the timer and unsubscribes the source', () => { const { store, source, timer } = createStore(); const listener = vi.fn(() => undefined); diff --git a/apps/mobile/src/lib/persist/mutation-outbox.test.ts b/apps/mobile/src/lib/persist/mutation-outbox.test.ts index a1a48e785c..2a1c680362 100644 --- a/apps/mobile/src/lib/persist/mutation-outbox.test.ts +++ b/apps/mobile/src/lib/persist/mutation-outbox.test.ts @@ -147,6 +147,23 @@ describe('round trip and absent load', () => { }); }); +describe('process kill and relaunch', () => { + it('recovers a written safe-retry row after a simulated relaunch and keeps its stored operationKey', async () => { + // Crash mid-flight: the row was persisted before the POST. + await writeOutboxRow('u1', safeRetryRow({ operationKey: 'op-key-1', fingerprint: 'fp-1' })); + + // Relaunch: the launch load lists the rows again from the encrypted KV. + const rows = await listOutboxRows('u1'); + + expect(rows).toHaveLength(1); + expect(rows?.[0]).toMatchObject({ + taxonomy: 'safe-retry', + operationKey: 'op-key-1', + fingerprint: 'fp-1', + }); + }); +}); + describe('remove and list', () => { it('removes a stored row', async () => { await writeOutboxRow('u1', safeRetryRow({ fingerprint: 'fp' })); diff --git a/apps/mobile/src/lib/query-client-lifecycle.test.ts b/apps/mobile/src/lib/query-client-lifecycle.test.ts index 0d445120d6..33bc7fdadb 100644 --- a/apps/mobile/src/lib/query-client-lifecycle.test.ts +++ b/apps/mobile/src/lib/query-client-lifecycle.test.ts @@ -122,4 +122,31 @@ describe('installQueryClientNativeLifecycle', () => { expect(setOnline).toHaveBeenNthCalledWith(3, false); expect(setOnline).toHaveBeenNthCalledWith(4, true); }); + + it('flapping unknown → offline → online → offline is online only on the confirmed online state', () => { + const native = createSources(); + const setFocused = createBooleanSetterMock(); + const setOnline = createBooleanSetterMock(); + + const cleanup = installQueryClientNativeLifecycle({ + sources: native.sources, + managers: { + focus: { setFocused }, + online: { setOnline }, + }, + }); + + native.setConnectivity({ isConnected: null, isInternetReachable: null }); + native.setConnectivity({ isConnected: false, isInternetReachable: false }); + native.setConnectivity({ isConnected: true, isInternetReachable: true }); + native.setConnectivity({ isConnected: false, isInternetReachable: false }); + cleanup(); + + // Only the confirmed online state resumes React Query; unknown and every + // offline state pause it, so a paused query never surfaces an error state. + expect(setOnline).toHaveBeenNthCalledWith(1, false); + expect(setOnline).toHaveBeenNthCalledWith(2, false); + expect(setOnline).toHaveBeenNthCalledWith(3, true); + expect(setOnline).toHaveBeenNthCalledWith(4, false); + }); }); From cc7735f7420c0c87aefc146bc0054079c4fe58e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Fri, 21 Aug 2026 06:50:45 +0200 Subject: [PATCH 10/14] fix(mobile): make security capability status type local --- apps/mobile/src/lib/hooks/use-security-agent.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/mobile/src/lib/hooks/use-security-agent.ts b/apps/mobile/src/lib/hooks/use-security-agent.ts index 2dad6cf760..5ad730b383 100644 --- a/apps/mobile/src/lib/hooks/use-security-agent.ts +++ b/apps/mobile/src/lib/hooks/use-security-agent.ts @@ -143,7 +143,7 @@ function useSecurityAgentOrgRoleQuery(scope: string) { // Discriminated capability state for consumers (e.g. audit-report access) // that must distinguish "still loading"/"failed to load" from "resolved: // no access" instead of treating an undefined role as permission-denied. -export type SecurityAgentCapabilityStatus = 'loading' | 'error' | 'denied' | 'allowed'; +type SecurityAgentCapabilityStatus = 'loading' | 'error' | 'denied' | 'allowed'; export function useSecurityAgentCapability(scope: string) { const { role, isLoading, isError, isFetching, refetch, hasData, isPending } = From 0010a39ea5405de5213d9f7cad987e1d4cbb4cfb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Fri, 21 Aug 2026 07:44:01 +0200 Subject: [PATCH 11/14] refactor(mobile): merge duplicate offline branch in security settings --- .../security-agent/settings-overview-screen.tsx | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/apps/mobile/src/components/security-agent/settings-overview-screen.tsx b/apps/mobile/src/components/security-agent/settings-overview-screen.tsx index 49f7e5c20e..b2e4ef8cc1 100644 --- a/apps/mobile/src/components/security-agent/settings-overview-screen.tsx +++ b/apps/mobile/src/components/security-agent/settings-overview-screen.tsx @@ -74,17 +74,10 @@ export function SettingsOverviewScreen({ trackRef.current({ interaction: 'settings_config_viewed' }); }, []); - if (config.isError && !config.data) { - return ( - void config.refetch()} - /> - ); - } - if (!config.data && config.fetchStatus === 'paused' && committedConnectivity === 'offline') { + if ( + !config.data && + (config.isError || (config.fetchStatus === 'paused' && committedConnectivity === 'offline')) + ) { return ( Date: Fri, 21 Aug 2026 08:06:17 +0200 Subject: [PATCH 12/14] fix(cloud-agent-sdk): reach reconnect exhaustion cap on live sessions Thread the reconnect attempt through refreshAndConnect so reconnects that refresh auth before connecting no longer reset the attempt counter to zero. Live user-web sessions now reach the exhaustion cap and fire the two-edge signal, instead of showing Reconnecting forever. --- .../src/base-connection.test.ts | 46 +++++++++++++++++++ .../cloud-agent-sdk/src/base-connection.ts | 6 +-- 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/packages/cloud-agent-sdk/src/base-connection.test.ts b/packages/cloud-agent-sdk/src/base-connection.test.ts index 1d631c0d2f..8673553d46 100644 --- a/packages/cloud-agent-sdk/src/base-connection.test.ts +++ b/packages/cloud-agent-sdk/src/base-connection.test.ts @@ -579,6 +579,52 @@ describe('createBaseConnection – stale WebSocket recovery', () => { connection.destroy(); }); + + it('reaches the cap for a live session whose reconnects all go through refreshAndConnect', async () => { + const refreshAuth = jest.fn(() => Promise.resolve()); + const onReconnectExhaustionChange = jest.fn(); + const { connection } = createTestConnection({ + refreshAuth, + shouldRefreshAuthBeforeConnect: () => true, + maxReconnectAttempts: 2, + onReconnectExhaustionChange, + }); + + connection.connect(); + await Promise.resolve(); + await Promise.resolve(); + connectSocket(0); + + // Live session loses its server. Every reconnect now goes through + // refreshAndConnect because shouldRefreshAuthBeforeConnect is true. + closeSocket(0); + jest.advanceTimersByTime(60_000); + await Promise.resolve(); + await Promise.resolve(); + + closeSocket(1); + jest.advanceTimersByTime(60_000); + await Promise.resolve(); + await Promise.resolve(); + + closeSocket(2); + + expect(onReconnectExhaustionChange).toHaveBeenCalledTimes(1); + expect(onReconnectExhaustionChange).toHaveBeenCalledWith(true); + + // The attempt counter now accumulates through refreshAndConnect, so the + // socket count grows past the cap attempt (2). + expect(sockets.length).toBeGreaterThan(2); + + // A recovery path resets the counter and fires false. + connection.retryReconnect(); + await Promise.resolve(); + await Promise.resolve(); + + expect(onReconnectExhaustionChange).toHaveBeenLastCalledWith(false); + + connection.destroy(); + }); }); describe('onReconnected vs onConnected', () => { diff --git a/packages/cloud-agent-sdk/src/base-connection.ts b/packages/cloud-agent-sdk/src/base-connection.ts index 864a6d0132..ee689ae262 100644 --- a/packages/cloud-agent-sdk/src/base-connection.ts +++ b/packages/cloud-agent-sdk/src/base-connection.ts @@ -146,7 +146,7 @@ export function createBaseConnection(config: BaseConnectionConfig): Connec } } - async function refreshAndConnect(expectedGeneration: number): Promise { + async function refreshAndConnect(expectedGeneration: number, attempt = 0): Promise { preconnectAuthRefreshAttempted = true; try { @@ -166,7 +166,7 @@ export function createBaseConnection(config: BaseConnectionConfig): Connec // refresh resolves. if (connected && ws !== null && ws.readyState === WebSocket.OPEN) return; } - connectInternal(0, expectedGeneration, true); + connectInternal(attempt, expectedGeneration, true); } finally { if (expectedGeneration === generation) { preconnectAuthRefreshAttempted = false; @@ -205,7 +205,7 @@ export function createBaseConnection(config: BaseConnectionConfig): Connec !skipAuthRefresh && !preconnectAuthRefreshAttempted ) { - void refreshAndConnect(expectedGeneration); + void refreshAndConnect(expectedGeneration, attempt); return; } From 55ac7098fbd0cb3621a5849a6341af13513e23fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Fri, 21 Aug 2026 08:06:36 +0200 Subject: [PATCH 13/14] fix(mobile): stamp session mutation generation after cancelQueries Stamp the generation inside snapshotAndUpdate after cancelQueries resolves, so the stamp order matches the write order and an older mutation cannot clobber a newer list write. --- apps/mobile/src/lib/hooks/use-session-mutations.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/apps/mobile/src/lib/hooks/use-session-mutations.ts b/apps/mobile/src/lib/hooks/use-session-mutations.ts index 5734cb7d27..a2901fea9a 100644 --- a/apps/mobile/src/lib/hooks/use-session-mutations.ts +++ b/apps/mobile/src/lib/hooks/use-session-mutations.ts @@ -36,13 +36,14 @@ export function useSessionMutations() { const snapshotAndUpdate = async ( update: (data: SessionsListData) => SessionsListData - ): Promise<{ previous: SessionsListSnapshot }> => { + ): Promise<{ previous: SessionsListSnapshot; generation: number }> => { await queryClient.cancelQueries({ queryKey: listKey }); + const generation = nextMutationGeneration(hashKey(listKey)); const previous = queryClient.getQueriesData({ queryKey: listKey }); queryClient.setQueriesData({ queryKey: listKey }, old => old ? update(old) : old ); - return { previous }; + return { previous, generation }; }; /** @@ -70,8 +71,9 @@ export function useSessionMutations() { const deleteSessionMutation = useMutation( trpc.cliSessionsV2.delete.mutationOptions({ onMutate: async ({ session_id }) => { - const generation = nextMutationGeneration(hashKey(listKey)); - const { previous } = await snapshotAndUpdate(data => removeStoredSession(data, session_id)); + const { previous, generation } = await snapshotAndUpdate(data => + removeStoredSession(data, session_id) + ); return { previous, generation }; }, onError: (error, _input, context) => { @@ -89,8 +91,7 @@ export function useSessionMutations() { const renameSessionMutation = useMutation( trpc.cliSessionsV2.rename.mutationOptions({ onMutate: async ({ session_id, title }) => { - const generation = nextMutationGeneration(hashKey(listKey)); - const { previous } = await snapshotAndUpdate(data => + const { previous, generation } = await snapshotAndUpdate(data => mapStoredSessions(data, session_id, session => ({ ...session, title })) ); const previousActive = await snapshotAndUpdateActive(session_id, title); From 56b335e23a2734bbb82db8c1103d8016c9ad92b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Fri, 21 Aug 2026 08:06:51 +0200 Subject: [PATCH 14/14] fix(mobile): read Agents tab focus live on foreground Read navigation.isFocused() in the AppState callback instead of a focusedRef that a frozen tab never updates. A frozen unfocused Agents tab no longer refetches or invalidates the active-sessions tray on foreground. --- .../agents/session-list-screen.mounted.test.tsx | 12 +++++------- .../src/components/agents/session-list-screen.tsx | 15 ++++++--------- 2 files changed, 11 insertions(+), 16 deletions(-) diff --git a/apps/mobile/src/components/agents/session-list-screen.mounted.test.tsx b/apps/mobile/src/components/agents/session-list-screen.mounted.test.tsx index 496f9ffcc3..3e9ff1d5d4 100644 --- a/apps/mobile/src/components/agents/session-list-screen.mounted.test.tsx +++ b/apps/mobile/src/components/agents/session-list-screen.mounted.test.tsx @@ -48,7 +48,7 @@ vi.mock('react-native-safe-area-context', () => ({ useSafeAreaInsets: () => ({ bottom: 0 }), })); vi.mock('expo-router', () => ({ - useIsFocused: () => focusState.current, + useNavigation: () => ({ isFocused: () => focusState.current }), useFocusEffect: (effect: () => void) => { focusCallback.current = effect; }, @@ -207,7 +207,7 @@ describe('AgentSessionListScreen foreground refresh', () => { }); it('does not refetch or invalidate on foreground after focus is lost post-mount', async () => { - const renderer = await renderScreen(); + await renderScreen(); // The route-focus refetch fires once on mount focus. act(() => { @@ -215,11 +215,9 @@ describe('AgentSessionListScreen foreground refresh', () => { }); expect(refetchSpy).toHaveBeenCalledTimes(1); - // Blur the tab after mount. The sync effect must move focusedRef to false. - act(() => { - focusState.current = false; - renderer.update(createElement(AgentSessionListScreen)); - }); + // Blur the tab after mount WITHOUT re-rendering: a frozen (unfocused) tab + // does not re-render, so the AppState callback must read focus live. + focusState.current = false; act(() => { appState.emit('background'); diff --git a/apps/mobile/src/components/agents/session-list-screen.tsx b/apps/mobile/src/components/agents/session-list-screen.tsx index a514254c62..386a8e2815 100644 --- a/apps/mobile/src/components/agents/session-list-screen.tsx +++ b/apps/mobile/src/components/agents/session-list-screen.tsx @@ -27,16 +27,12 @@ import { useOrganization } from '@/lib/organization-context'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; import { getEffectiveTabBarHeight } from '@/lib/tab-bar-layout'; -import { type Href, useFocusEffect, useIsFocused, useRouter } from 'expo-router'; +import { type Href, useFocusEffect, useNavigation, useRouter } from 'expo-router'; export function AgentSessionListScreen() { const router = useRouter(); + const navigation = useNavigation(); const queryClient = useQueryClient(); - const isFocused = useIsFocused(); - const focusedRef = useRef(isFocused); - useEffect(() => { - focusedRef.current = isFocused; - }, [isFocused]); const colors = useThemeColors(); const { bottom } = useSafeAreaInsets(); const { fontScale } = useWindowDimensions(); @@ -111,10 +107,11 @@ export function AgentSessionListScreen() { // stored refetch serialized by the shared operation coordinator (backfill // and departure never overlap a refetch). A frozen (unfocused) Agents tab // must NOT refetch on foreground: only the focused tab refreshes the stored - // list and invalidates the active-sessions tray. + // list and invalidates the active-sessions tray. Focus is read live via + // `navigation.isFocused()` because a frozen tree does not re-render. useEffect(() => { const subscription = AppState.addEventListener('change', nextState => { - if (nextState === 'active' && focusedRef.current) { + if (nextState === 'active' && navigation.isFocused()) { void refetchRef.current(); void queryClient.invalidateQueries({ queryKey: [['activeSessions']] }); } @@ -122,7 +119,7 @@ export function AgentSessionListScreen() { return () => { subscription.remove(); }; - }, [queryClient]); + }, [queryClient, navigation]); const showSearchBusy = selectShowSearchBusy({ awaitingCommit,