From 17ba28b0d6084f0488deb721f89926982c40b4b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Wed, 23 Sep 2026 11:01:42 +0200 Subject: [PATCH] perf(mobile): cut redundant repo, retry, search and presign work https://github.com/Kilo-Org/cloud/pull/6631 --- .../agents/file-part-url-resolver.test.ts | 103 +++++++++++- .../agents/file-part-url-resolver.ts | 96 +++++++++-- .../new-session-repository-section.test.ts | 4 + .../agents/new-session-repository-section.tsx | 4 + .../components/query-error.mounted.test.tsx | 56 +++++++ apps/mobile/src/components/query-error.tsx | 13 +- .../src/lib/system-search-collect.test.ts | 55 ++++++ apps/mobile/src/lib/system-search-collect.ts | 79 ++++++++- ...e-translated-tool-summary.mounted.test.tsx | 157 ++++++++++++++++-- .../use-translated-tool-summary.ts | 140 +++++++++++++--- .../src/lib/use-new-session-repos.test.ts | 65 +++++++- apps/mobile/src/lib/use-new-session-repos.ts | 68 ++++++-- 12 files changed, 757 insertions(+), 83 deletions(-) create mode 100644 apps/mobile/src/components/query-error.mounted.test.tsx diff --git a/apps/mobile/src/components/agents/file-part-url-resolver.test.ts b/apps/mobile/src/components/agents/file-part-url-resolver.test.ts index fcf0ebb8f6..f51a9209c3 100644 --- a/apps/mobile/src/components/agents/file-part-url-resolver.test.ts +++ b/apps/mobile/src/components/agents/file-part-url-resolver.test.ts @@ -182,8 +182,9 @@ afterEach(() => { }); describe('useResolvedFilePartUrl sweeper', () => { - it('starts one shared 30s interval for every subscriber', async () => { + it('arms one shared timer for every subscriber', async () => { const setIntervalSpy = vi.spyOn(globalThis, 'setInterval'); + const setTimeoutSpy = vi.spyOn(globalThis, 'setTimeout'); const due = Date.now() + 900_000; const parts = [ { id: 'part-1', uuid: '11111111-1111-4111-8111-111111111111', filename: 'a.png' }, @@ -196,8 +197,104 @@ describe('useResolvedFilePartUrl sweeper', () => { await mountProbe(makeFilePart(id, uuid, filename)); } - expect(setIntervalSpy).toHaveBeenCalledTimes(1); - expect(setIntervalSpy).toHaveBeenCalledWith(expect.any(Function), 30_000); + // One timeout armed at the earliest due renew, and no fixed 30 s interval. + expect(setIntervalSpy).not.toHaveBeenCalled(); + expect(vi.getTimerCount()).toBe(1); + expect(setTimeoutSpy).toHaveBeenLastCalledWith(expect.any(Function), 900_000 - 120_000); + }); + + it('waits for the trusted far-future expiry instead of sweeping every 30s', async () => { + vi.spyOn(globalThis, 'setInterval'); + const uuid = '11111111-1111-4111-8111-111111111111'; + const expiresAt = Date.now() + 900_000; + cacheRenewableEntry( + 'part-1', + { uuid, filename: 'a.png' }, + { url: 'https://r2.example/old', urlExpiresAt: expiresAt } + ); + getAttachmentDownloadUrlMutate.mockResolvedValue({ + signedUrl: 'https://r2.example/fresh', + key: 'k', + expiresAt: '2040-01-01T00:00:00Z', + }); + + await mountProbe(makeFilePart('part-1', uuid, 'a.png')); + + advance(30_000); + + // A 900 s URL is nowhere near its 120 s renew window, so nothing sweeps. + expect(getAttachmentDownloadUrlMutate).not.toHaveBeenCalled(); + + advance(expiresAt - Date.now() - 120_000); + + expect(getAttachmentDownloadUrlMutate).toHaveBeenCalledTimes(1); + await flushMicrotasks(); + + expect(getFilePartCacheEntry('part-1')?.url).toBe('https://r2.example/fresh'); + expect(getFilePartCacheEntry('part-1')?.urlExpiresAt).toBe(Date.parse('2040-01-01T00:00:00Z')); + }); + + it('caps a far-future expiry at the largest timeout the runtime accepts', async () => { + const setTimeoutSpy = vi.spyOn(globalThis, 'setTimeout'); + const uuid = '11111111-1111-4111-8111-111111111111'; + // A delay above 2^31 - 1 ms overflows and would fire immediately; the + // sweep must re-check at the ceiling instead of busy-looping. + cacheRenewableEntry( + 'part-1', + { uuid, filename: 'a.png' }, + { url: 'https://r2.example/old', urlExpiresAt: Date.parse('2099-01-01T00:00:00Z') } + ); + + await mountProbe(makeFilePart('part-1', uuid, 'a.png')); + + expect(setTimeoutSpy).toHaveBeenLastCalledWith(expect.any(Function), 2_147_483_647); + }); + + it('renews a cached entry whose expiry is not finite instead of spinning at the floor', async () => { + const uuid = '11111111-1111-4111-8111-111111111111'; + // An unparseable server `expiresAt` lands as NaN: parseTimestamp(...).getTime(). + cacheRenewableEntry('part-1', { uuid, filename: 'a.png' }, { urlExpiresAt: Number.NaN }); + + await mountProbe(makeFilePart('part-1', uuid, 'a.png')); + await flushMicrotasks(); + + // A NaN delay would make setTimeout fire after ~1 ms and the sweep re-arm + // forever, and a NaN expiry never compares as due, so the malformed entry is + // renewed at once and its finite replacement expiry governs the sweep again. + expect(getAttachmentDownloadUrlMutate).toHaveBeenCalledTimes(1); + expect(getFilePartCacheEntry('part-1')?.urlExpiresAt).toBe(Date.parse('2099-01-01T00:00:00Z')); + }); + + it('lets a finite sibling deadline govern after renewing a malformed entry', async () => { + const setTimeoutSpy = vi.spyOn(globalThis, 'setTimeout'); + const badUuid = '11111111-1111-4111-8111-111111111111'; + const farUuid = '22222222-2222-4222-8222-222222222222'; + // The malformed entry is stored first so it is visited before any finite + // one: its NaN used to be the last word on `soonest`, pinning the whole + // shared sweep to the 30 s floor and never renewing itself. + cacheRenewableEntry( + 'part-1', + { uuid: badUuid, filename: 'a.png' }, + { url: 'https://r2.example/bad', urlExpiresAt: Number.NaN } + ); + const farDue = Date.now() + 900_000; + cacheRenewableEntry( + 'part-2', + { uuid: farUuid, filename: 'b.png' }, + { url: 'https://r2.example/far', urlExpiresAt: farDue } + ); + await mountProbe(makeFilePart('part-1', badUuid, 'a.png')); + await mountProbe(makeFilePart('part-2', farUuid, 'b.png')); + await flushMicrotasks(); + + // Only the malformed entry is renewed; the far sibling is untouched. + expect(getAttachmentDownloadUrlMutate).toHaveBeenCalledTimes(1); + expect(getAttachmentDownloadUrlMutate).toHaveBeenCalledWith({ + messageUuid: badUuid, + filename: 'a.png', + }); + // The sweep waits for the sibling's real deadline, not the 30 s floor. + expect(setTimeoutSpy.mock.calls.map(call => call[1])).toContain(farDue - Date.now() - 120_000); }); it('renews each due entry exactly once per sweep', async () => { diff --git a/apps/mobile/src/components/agents/file-part-url-resolver.ts b/apps/mobile/src/components/agents/file-part-url-resolver.ts index fc00889e5f..8602bed7d0 100644 --- a/apps/mobile/src/components/agents/file-part-url-resolver.ts +++ b/apps/mobile/src/components/agents/file-part-url-resolver.ts @@ -22,8 +22,13 @@ import { type CloudAgentAttachmentRef, parseCloudAgentAttachmentUrl } from './fi /** Start a renew when the presigned lifetime drops under two minutes. */ const RENEW_THRESHOLD_MS = 120_000; -/** Sweep the cache for near-expiry URLs every thirty seconds. */ +/** Floor before the next renew sweep: an entry whose renew keeps failing is + * retried no more often than every thirty seconds. */ const RENEW_INTERVAL_MS = 30_000; +/** Longest `setTimeout` delay the JS runtime accepts. A delay above 2^31 - 1 ms + * overflows and fires immediately, so a far-future expiry is re-checked here + * instead of busy-looping the sweep. */ +const MAX_TIMER_DELAY_MS = 2_147_483_647; /** Part IDs with an on-demand presign in flight. Dedupes a StrictMode * double-mount, a leave/reopen during the mutate, and the renewal sweep. */ @@ -31,7 +36,7 @@ const inFlight = new Map boolean>(); // One module-level sweeper serves every mounted subscriber across the app. let renewSubscribers = 0; -let renewTimer: ReturnType | undefined = undefined; +let renewTimer: ReturnType | undefined = undefined; export type ResolvedFilePartUrl = { status: 'ready' | 'resolving' | 'unavailable' | 'error'; @@ -78,6 +83,9 @@ async function presignAttachment( ...(entry.filename ? { filename: entry.filename } : {}), urlExpiresAt: parseTimestamp(result.expiresAt).getTime(), }); + // The trusted expiry moved: re-arm the sweep so the fresh URL is renewed + // before it lapses instead of waiting on the old deadline. + scheduleRenewSweep(); return true; } catch { if (isCurrent()) { @@ -95,36 +103,102 @@ async function presignAttachment( } } +/** Milliseconds until a presigned entry is due for a renew. A missing or + * non-finite `urlExpiresAt` (an unparseable server `expiresAt` stored as NaN) + * is due at once: its URL has no trusted deadline to wait on, so the sweep + * re-presigns it instead of letting a NaN poison the earliest-due reduction or + * stranding the entry forever. */ +function renewDueInMs(entry: FilePartCacheEntry, now: number): number { + if (entry.urlExpiresAt === undefined || !Number.isFinite(entry.urlExpiresAt)) { + return 0; + } + return entry.urlExpiresAt - now - RENEW_THRESHOLD_MS; +} + /** True when a presigned entry needs a renew now: a ref and URL exist and the - * expiry is missing (old entry) or under the renew threshold. */ + * expiry is missing, non-finite, or at/reached the renew threshold. */ function isRenewDue(entry: FilePartCacheEntry, now: number): boolean { return ( - entry.attachmentRef !== undefined && - entry.url !== undefined && - (entry.urlExpiresAt === undefined || entry.urlExpiresAt - now < RENEW_THRESHOLD_MS) + entry.attachmentRef !== undefined && entry.url !== undefined && renewDueInMs(entry, now) <= 0 ); } -function renewDueEntries(): void { +/** + * Delay until the earliest entry is due for a renew, floored at + * RENEW_INTERVAL_MS, or null when no cached entry carries both an attachment + * ref and a URL (nothing to renew). `renewDueInMs` never returns NaN, so one + * malformed expiry cannot swallow a later entry's deadline or arm a ~1 ms + * timeout that re-schedules the sweep forever. + */ +function nextRenewDelayMs(now: number): number | null { + let soonest: number | undefined = undefined; + for (const { entry } of listFilePartCacheEntries()) { + if (entry.attachmentRef !== undefined && entry.url !== undefined) { + const dueIn = renewDueInMs(entry, now); + soonest = soonest === undefined || dueIn < soonest ? dueIn : soonest; + } + } + return soonest === undefined ? null : Math.max(RENEW_INTERVAL_MS, soonest); +} + +/** Re-presign every cached entry that is due now and return the in-flight + * renewals so the sweep can re-arm once they settle. */ +function renewDueEntries(): Promise[] { const now = Date.now(); + const renewals: Promise[] = []; for (const { partId, entry } of listFilePartCacheEntries()) { const ref = entry.attachmentRef; if (ref !== undefined && isRenewDue(entry, now)) { - void presignAttachment(partId, { ...entry, attachmentRef: ref }, true); + renewals.push(presignAttachment(partId, { ...entry, attachmentRef: ref }, true)); } } + return renewals; } -function startRenewTimer(): void { +/** + * Run one sweep, then re-arm: a successful renew moves the deadline out to the + * new far-future expiry, a failed one leaves the entry due at the 30 s floor. + */ +async function runRenewSweep(): Promise { + const renewals = renewDueEntries(); + if (renewals.length > 0) { + await Promise.allSettled(renewals); + } + scheduleRenewSweep(); +} + +/** + * Arm the single shared timeout at the earliest due renew, replacing any armed + * handle. No subscriber and no ref+URL entry both mean no timer at all. + */ +function scheduleRenewSweep(): void { if (renewTimer !== undefined) { + clearTimeout(renewTimer); + renewTimer = undefined; + } + if (renewSubscribers === 0) { return; } - renewTimer = setInterval(renewDueEntries, RENEW_INTERVAL_MS); + const delay = nextRenewDelayMs(Date.now()); + if (delay === null) { + return; + } + renewTimer = setTimeout( + () => { + renewTimer = undefined; + void runRenewSweep(); + }, + Math.min(delay, MAX_TIMER_DELAY_MS) + ); +} + +function startRenewTimer(): void { + scheduleRenewSweep(); } function stopRenewTimer(): void { if (renewTimer !== undefined) { - clearInterval(renewTimer); + clearTimeout(renewTimer); renewTimer = undefined; } } diff --git a/apps/mobile/src/components/agents/new-session-repository-section.test.ts b/apps/mobile/src/components/agents/new-session-repository-section.test.ts index 9274043a9b..af2a3c6596 100644 --- a/apps/mobile/src/components/agents/new-session-repository-section.test.ts +++ b/apps/mobile/src/components/agents/new-session-repository-section.test.ts @@ -401,6 +401,10 @@ describe('NewSessionRepositorySection connect cards after selection', () => { }); const error = renderer.root.findByType('QueryError' as never); expect(error.props.title).toBe(i18n.t('agentChat.newSession.couldNotLoadGitlabRepositories')); + // The error row's action is the provider-list refresh, so it carries that + // name rather than the generic "Retry" (scenario e7: the digest must show + // the 'Refresh repositories' control on the error row). + expect(error.props.retryLabel).toBe(i18n.t('agentChat.newSession.refreshRepositories')); act(() => { (error.props.onRetry as () => void)(); }); diff --git a/apps/mobile/src/components/agents/new-session-repository-section.tsx b/apps/mobile/src/components/agents/new-session-repository-section.tsx index b1fae54acb..02c054589e 100644 --- a/apps/mobile/src/components/agents/new-session-repository-section.tsx +++ b/apps/mobile/src/components/agents/new-session-repository-section.tsx @@ -189,6 +189,10 @@ export function NewSessionRepositorySection({ title={t(PROVIDER_COPY[platform].errorTitle)} message={t('organization.boundary.loadErrorMessage')} onRetry={onRefreshRepos} + // The error row's action is the same provider-list refresh the + // connect and connected-empty cards offer, so it carries the same + // name instead of the generic "Retry". + retryLabel={t('agentChat.newSession.refreshRepositories')} isRetrying={isRetrying} /> diff --git a/apps/mobile/src/components/query-error.mounted.test.tsx b/apps/mobile/src/components/query-error.mounted.test.tsx new file mode 100644 index 0000000000..61677fdb75 --- /dev/null +++ b/apps/mobile/src/components/query-error.mounted.test.tsx @@ -0,0 +1,56 @@ +import { createElement, type ReactNode } from 'react'; +import { describe, expect, it, vi } from 'vitest'; + +import { renderWithProviders } from '@/test/render-with-providers'; + +import { QueryError } from './query-error'; + +// The retry action is the only stateful part under test: EmptyState (and its +// measured placement) and the primitives are stubbed so the label and the +// press handler are asserted on the button the component itself builds. +vi.mock('@/components/empty-state', () => ({ + EmptyState: ({ action }: { action?: ReactNode }) => createElement('EmptyState', null, action), +})); +vi.mock('@/components/ui/button', () => ({ Button: 'Button' })); +vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); +vi.mock('@/components/ui/accessible-status', () => ({ AccessibleStatus: 'AccessibleStatus' })); +vi.mock('@/components/ui/icons', () => ({ + AlertCircle: () => null, + Lock: () => null, + SearchX: () => null, + ServerCrash: () => null, + WifiOff: () => null, +})); +vi.mock('react-i18next', () => ({ useTranslation: () => ({ t: (key: string) => key }) })); + +/** The retry button's props, including the label it renders. */ +type RetryButtonProps = { onPress: () => void; children: { props: { children: string } } }; + +describe('QueryError retry action', () => { + it('defaults the label to the generic Retry for callers that pass none', async () => { + const onRetry = vi.fn<() => void>(); + const mounted = await renderWithProviders(); + + const retry = mounted.renderer.root.findByProps({ accessibilityLabel: 'common.retry' }) + .props as RetryButtonProps; + expect(retry.children.props.children).toBe('common.retry'); + retry.onPress(); + expect(onRetry).toHaveBeenCalledOnce(); + mounted.unmount(); + }); + + it('uses the caller label on both the visible and the accessibility label', async () => { + const mounted = await renderWithProviders( + void>()} retryLabel="Refresh repositories" /> + ); + + const retry = mounted.renderer.root.findByProps({ + accessibilityLabel: 'Refresh repositories', + }).props as RetryButtonProps; + expect(retry.children.props.children).toBe('Refresh repositories'); + expect( + mounted.renderer.root.findAllByProps({ accessibilityLabel: 'common.retry' }) + ).toHaveLength(0); + mounted.unmount(); + }); +}); diff --git a/apps/mobile/src/components/query-error.tsx b/apps/mobile/src/components/query-error.tsx index 89108b4423..04a48f0055 100644 --- a/apps/mobile/src/components/query-error.tsx +++ b/apps/mobile/src/components/query-error.tsx @@ -49,6 +49,13 @@ type QueryErrorProps = { title?: string; message?: string; onRetry?: () => void; + /** + * The retry action's label. A caller whose refresh action has a more precise + * name than "Retry" passes it here, so the visible label and the + * accessibility label stay the same string; the default is the generic + * `common.retry`. + */ + retryLabel?: string; isRetrying?: boolean; className?: string; placement?: 'center' | 'top' | 'static'; @@ -63,6 +70,7 @@ export function QueryError({ title, message, onRetry, + retryLabel, isRetrying = false, className, placement = 'center', @@ -72,6 +80,7 @@ export function QueryError({ const meta = variantMeta(t, variant); const titleText = title ?? meta.title; const descriptionText = message ?? meta.description; + const retryText = retryLabel ?? t('common.retry'); return ( - {t('common.retry')} + {retryText} ) } diff --git a/apps/mobile/src/lib/system-search-collect.test.ts b/apps/mobile/src/lib/system-search-collect.test.ts index de8a689976..378fedc0fc 100644 --- a/apps/mobile/src/lib/system-search-collect.test.ts +++ b/apps/mobile/src/lib/system-search-collect.test.ts @@ -650,4 +650,59 @@ describe('collectSystemSearchDocuments', () => { ]); expect(queryFn).not.toHaveBeenCalled(); }); + + it('reuses a query’s documents while its payload identity is unchanged', async () => { + const client = new QueryClient(); + seed(client); + + const first = await collectSystemSearchDocuments(client); + const second = await collectSystemSearchDocuments(client); + + // The very same document objects come back: the second collect reused the + // memo entry instead of re-decoding the payload and re-fingerprinting + // every document. + expect(second.documents[0]).toBe(first.documents[0]); + expect(second.documents).toEqual(first.documents); + }); + + it('rebuilds a query’s documents when the cached payload changes', async () => { + const client = new QueryClient(); + seed(client); + + const first = await collectSystemSearchDocuments(client); + client.setQueryData(SESSION_LIST_KEY, { + pages: [ + { cliSessions: [{ ...sessionRow, title: 'Fix login bug (renamed)' }], nextCursor: null }, + ], + }); + const second = await collectSystemSearchDocuments(client); + + const renamed = second.documents.find( + document => document.id === '/(app)/agent-chat/sess-1?organizationId=org-1' + ); + // The changed payload invalidated the memo entry, so the document carries + // the new title and is a fresh object rather than the reused one. + expect(renamed?.title).toBe('Fix login bug (renamed)'); + expect(renamed).not.toBe( + first.documents.find( + document => document.id === '/(app)/agent-chat/sess-1?organizationId=org-1' + ) + ); + }); + + it('keeps one client’s memoized documents out of another client', async () => { + const firstClient = new QueryClient(); + const secondClient = new QueryClient(); + // Both clients hold the same query keys and the same payload objects, so a + // memo keyed by query hash alone would hand the first client's documents to + // the second. + seed(firstClient); + seed(secondClient); + + const first = await collectSystemSearchDocuments(firstClient); + const second = await collectSystemSearchDocuments(secondClient); + + expect(second.documents[0]).not.toBe(first.documents[0]); + expect(second.documents).toEqual(first.documents); + }); }); diff --git a/apps/mobile/src/lib/system-search-collect.ts b/apps/mobile/src/lib/system-search-collect.ts index 3666eaddfe..e93cb5ed20 100644 --- a/apps/mobile/src/lib/system-search-collect.ts +++ b/apps/mobile/src/lib/system-search-collect.ts @@ -133,13 +133,39 @@ export type SystemSearchCollection = { observedSources: Set; }; +/** + * One query's collected documents, memoized against the payload identity that + * produced them. `data`, `status` and `maxPages` are exactly the query fields + * the decode reads, so a change to any of them invalidates the entry and the + * query is decoded once more. + */ +type QueryCollectionMemo = { + data: unknown; + status: Query['state']['status']; + maxPages: unknown; + documents: SystemSearchDocument[]; + observedSource: string | null; +}; + +/** + * Per-client memo of each cached query's documents, keyed by `queryHash`. Keyed + * by the client so two clients holding the same query key never read each + * other's documents (production has one `QueryClient`; the tests build a new + * one per case), and WeakMap so the memo goes with the client it describes. + */ +const collectionsByClient = new WeakMap>(); + export async function collectSystemSearchDocuments( queryClient: QueryClient ): Promise { const documents: SystemSearchDocument[] = []; const observedSources = new Set(); + const collections = collectionsFor(queryClient); + const seen = new Set(); for (const query of queryClient.getQueryCache().getAll()) { - const collected = documentsFromQuery(query); + const queryHash = query.queryHash; + seen.add(queryHash); + const collected = collectOrReuse(collections, query, queryHash); documents.push(...collected.documents); // Only a successful query that fully enumerated a source scope is // authoritative. A query can hold the family path without enumerating it — @@ -152,6 +178,13 @@ export async function collectSystemSearchDocuments( observedSources.add(collected.observedSource); } } + // Drop the queries the cache no longer holds, so a removed-then-re-added + // query decodes its new payload and the memo stays bounded to the cache. + for (const queryHash of collections.keys()) { + if (!seen.has(queryHash)) { + collections.delete(queryHash); + } + } const recents = await recentPrDocuments(); documents.push(...recents.documents); for (const source of recents.observedSources) { @@ -160,6 +193,50 @@ export async function collectSystemSearchDocuments( return { documents: dedupeBy(documents, document => document.id), observedSources }; } +/** The query memo belonging to one client, created on first use. */ +function collectionsFor(queryClient: QueryClient): Map { + const existing = collectionsByClient.get(queryClient); + if (existing !== undefined) { + return existing; + } + const created = new Map(); + collectionsByClient.set(queryClient, created); + return created; +} + +/** + * One query's documents: reused from the memo when the payload identity it + * decoded is unchanged, decoded and memoized otherwise. `data`, `status` and + * `maxPages` are the fields the decode reads, so comparing them is enough to + * know the cached entry describes the query in front of us. + */ +function collectOrReuse( + collections: Map, + query: Query, + queryHash: string +): QueryDocuments { + const memo = collections.get(queryHash); + if ( + memo !== undefined && + memo.data === query.state.data && + memo.status === query.state.status && + memo.maxPages === query.options.maxPages + ) { + // The payload is the same object this entry decoded, so its documents and + // their fingerprints are reused rather than rebuilt. + return memo; + } + const collected = documentsFromQuery(query); + collections.set(queryHash, { + data: query.state.data, + status: query.state.status, + maxPages: query.options.maxPages, + documents: collected.documents, + observedSource: collected.observedSource, + }); + return collected; +} + /** * What one cached query carries: the documents it enumerates, and the source * scope key it fully enumerated, or null when it did not enumerate one. The diff --git a/apps/mobile/src/lib/tool-summary-translation/use-translated-tool-summary.mounted.test.tsx b/apps/mobile/src/lib/tool-summary-translation/use-translated-tool-summary.mounted.test.tsx index 044650542d..a9f37cc24f 100644 --- a/apps/mobile/src/lib/tool-summary-translation/use-translated-tool-summary.mounted.test.tsx +++ b/apps/mobile/src/lib/tool-summary-translation/use-translated-tool-summary.mounted.test.tsx @@ -1,4 +1,5 @@ -/* eslint-disable max-lines -- the string-row states and the pending/interval retry suite share one hook and one client mock harness. */ +/* eslint-disable max-lines -- the string-row states and the pending/shared-retry suite share one hook and one client mock harness. */ +import { onlineManager } from '@tanstack/react-query'; import { createElement } from 'react'; import { act, TestRenderer } from '@/test/renderer'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -48,6 +49,23 @@ function Probe({ return null; } +/** + * The renderer of the case currently running. The shared retry keeps module-level + * state (subscriber count, timer handle, backoff delay) that only a successful + * `unmount()` unwinds, so a case that throws before its own final `unmount()` + * leaves the module armed: `vi.useRealTimers()` drops the fake timer but not its + * stale handle, and every later case's `armRetryTimer()` would early-return on + * `retryTimer !== undefined`. `afterEach` unmounts whatever is still mounted, so + * one real failure cannot cascade into unrelated ones. + */ +let activeUnmount: (() => void) | undefined = undefined; + +function cleanupMountedRenderer(): void { + const unmount = activeUnmount; + activeUnmount = undefined; + unmount?.(); +} + function mountProbes(specs: ProbeSpec[]): { latest: (index: number) => string; unmount: () => void; @@ -76,13 +94,16 @@ function mountProbes(specs: ProbeSpec[]): { if (!renderer) { throw new Error('renderer was not created'); } + const unmount = () => { + activeUnmount = undefined; + act(() => { + renderer.unmount(); + }); + }; + activeUnmount = unmount; return { latest: index => current[index] ?? '', - unmount: () => { - act(() => { - renderer.unmount(); - }); - }, + unmount, }; } @@ -118,6 +139,10 @@ beforeEach(() => { }); describe('useTranslatedToolSummary', () => { + afterEach(() => { + cleanupMountedRenderer(); + }); + it('renders the raw text when the preference is off', async () => { requestMock.mockResolvedValue(['translated']); setConfig({ enabled: false, model: MODEL }); @@ -258,6 +283,13 @@ describe('useTranslatedToolSummary', () => { createElement('View', null, probe('partial', 0), probe('partial', 1)) ); }); + const unmountRenderer = () => { + activeUnmount = undefined; + act(() => { + ref.renderer?.unmount(); + }); + }; + activeUnmount = unmountRenderer; await settle(); expect(requestMock).toHaveBeenCalledTimes(1); @@ -293,9 +325,7 @@ describe('useTranslatedToolSummary', () => { expect(current[1]).toBe('de:final'); expect(requestMock).toHaveBeenCalledTimes(3); - act(() => { - ref.renderer?.unmount(); - }); + unmountRenderer(); }); }); @@ -343,13 +373,16 @@ function mountPending(text: string): { latest: () => ToolSummaryTranslation; unm if (!renderer) { throw new Error('renderer was not created'); } + const unmount = () => { + activeUnmount = undefined; + act(() => { + renderer.unmount(); + }); + }; + activeUnmount = unmount; return { latest: () => current, - unmount: () => { - act(() => { - renderer.unmount(); - }); - }, + unmount, }; } @@ -365,6 +398,11 @@ describe('useToolSummaryTranslation retries an unresolved summary', () => { }); afterEach(() => { + // Unwind any case that threw before its own unmount before the fake timers + // are discarded, then restore the module-global connectivity gate: a case + // that flips it must not leave the shared retry paused for the next one. + cleanupMountedRenderer(); + onlineManager.setOnline(true); vi.useRealTimers(); }); @@ -414,4 +452,95 @@ describe('useToolSummaryTranslation retries an unresolved summary', () => { await advance(TOOL_SUMMARY_TRANSLATION_RETRY_MS * 3); expect(requestMock).toHaveBeenCalledTimes(2); }); + + it('arms one shared timer instead of an interval per mounted row', async () => { + const setIntervalSpy = vi.spyOn(globalThis, 'setInterval'); + requestMock.mockResolvedValue([null]); + setConfig({ enabled: true, model: MODEL }); + const { latest, unmount } = mountProbes([ + { text: 'Shared retry one', itemId: 'part-a' }, + { text: 'Shared retry two', itemId: 'part-b' }, + { text: 'Shared retry three', itemId: 'part-c' }, + ]); + + await advance(BATCH_WINDOW_SETTLE_MS); + + // Every row is unresolved, yet the fix must never arm a per-row interval. + expect(requestMock).toHaveBeenCalledTimes(1); + expect(latest(0)).toBe('Shared retry one'); + expect(setIntervalSpy).not.toHaveBeenCalled(); + unmount(); + setIntervalSpy.mockRestore(); + }); + + it('doubles the wait after each replay', async () => { + requestMock.mockResolvedValue([null]); + setConfig({ enabled: true, model: MODEL }); + const { latest, unmount } = mountPending('Backoff target summary'); + + await advance(BATCH_WINDOW_SETTLE_MS); + expect(requestMock).toHaveBeenCalledTimes(1); + + // The first replay is still at the base delay. + await advance(TOOL_SUMMARY_TRANSLATION_RETRY_MS); + expect(requestMock).toHaveBeenCalledTimes(2); + expect(latest()).toEqual({ text: 'Backoff target summary', pending: true }); + + // The next wake is at 2x the base: one base window later nothing new. + await advance(TOOL_SUMMARY_TRANSLATION_RETRY_MS); + expect(requestMock).toHaveBeenCalledTimes(2); + + await advance(TOOL_SUMMARY_TRANSLATION_RETRY_MS); + expect(requestMock).toHaveBeenCalledTimes(3); + unmount(); + }); + + it('pauses the shared retry while offline and replays once on reconnect', async () => { + requestMock.mockResolvedValue([null]); + setConfig({ enabled: true, model: MODEL }); + // Offline before the failure: the shared timer must not wake, and the + // reconnection edge below must be the only thing that resumes it. + onlineManager.setOnline(false); + const { latest, unmount } = mountPending('Offline target summary'); + + await advance(BATCH_WINDOW_SETTLE_MS); + expect(requestMock).toHaveBeenCalledTimes(1); + + await advance(TOOL_SUMMARY_TRANSLATION_RETRY_MS * 2); + // No wakeup and no gateway call while offline: only the mount request. + expect(requestMock).toHaveBeenCalledTimes(1); + expect(latest()).toEqual({ text: 'Offline target summary', pending: true }); + + onlineManager.setOnline(true); + await advance(TOOL_SUMMARY_TRANSLATION_RETRY_MS + BATCH_WINDOW_SETTLE_MS); + // Exactly one replay on the reconnection edge, at the base delay. + expect(requestMock).toHaveBeenCalledTimes(2); + unmount(); + }); + + it('reschedules an armed backoff timer when the device reconnects', async () => { + requestMock.mockResolvedValue([null]); + setConfig({ enabled: true, model: MODEL }); + const { unmount } = mountPending('Reconnect mid-wait summary'); + + await advance(BATCH_WINDOW_SETTLE_MS); + expect(requestMock).toHaveBeenCalledTimes(1); + + // Two replays grow the wait to 4x the base, leaving that long timer armed. + await advance(TOOL_SUMMARY_TRANSLATION_RETRY_MS); + expect(requestMock).toHaveBeenCalledTimes(2); + await advance(TOOL_SUMMARY_TRANSLATION_RETRY_MS * 2); + expect(requestMock).toHaveBeenCalledTimes(3); + + // A network blip inside that wait: offline, then back online before the + // armed 4x timer fires. + onlineManager.setOnline(false); + onlineManager.setOnline(true); + + // The reconnect must reschedule the replay at the base delay instead of + // letting the stale armed timer stand. + await advance(TOOL_SUMMARY_TRANSLATION_RETRY_MS + BATCH_WINDOW_SETTLE_MS); + expect(requestMock).toHaveBeenCalledTimes(4); + unmount(); + }); }); diff --git a/apps/mobile/src/lib/tool-summary-translation/use-translated-tool-summary.ts b/apps/mobile/src/lib/tool-summary-translation/use-translated-tool-summary.ts index 303c0258cd..5b62ab3fce 100644 --- a/apps/mobile/src/lib/tool-summary-translation/use-translated-tool-summary.ts +++ b/apps/mobile/src/lib/tool-summary-translation/use-translated-tool-summary.ts @@ -1,3 +1,4 @@ +import { onlineManager } from '@tanstack/react-query'; import { useEffect, useSyncExternalStore } from 'react'; import { useTranslation } from 'react-i18next'; @@ -6,6 +7,7 @@ import { getConfig, getTranslation, releaseTranslationInterest, + retryUnresolvedTranslations, subscribe, } from './tool-summary-translation-runtime'; @@ -16,20 +18,105 @@ export type ToolSummaryTranslation = { }; /** - * How long an unresolved row waits before it asks the gateway again. Every - * client failure (no token, non-2xx, timeout, malformed body) resolves to - * `null` and caches nothing, so a request that settled without a translation - * would otherwise leave `pending` true for the rest of the mount: a condensed - * label would keep its count alone and never show the last summary again, and a - * plain row would keep the original text. The retry belongs to the mounted row, - * so it stops when the translation lands (`translated` clears the timer) or the - * row unmounts, and `ensureTranslation` drops each tick while a request is in - * flight, a request is already queued behind the concurrency limit, or the - * translation is already cached, so rows sharing a summary ask the gateway once - * per cadence. + * The first wait before an unresolved summary is asked for again, and the delay + * the shared retry returns to on the reconnection edge. Every client failure (no + * token, non-2xx, timeout, malformed body) resolves to `null` and caches + * nothing, so a request that settled without a translation would otherwise + * leave `pending` true for the rest of the mount: a condensed label would keep + * its count alone and never show the last summary again, and a plain row would + * keep the original text. The retry is shared by every mounted unresolved row + * (one timer, not one per row), so `ensureTranslation`'s de-duplication keeps + * the gateway work to one request per cadence per distinct summary. */ export const TOOL_SUMMARY_TRANSLATION_RETRY_MS = 10_000; +/** + * The longest the shared retry wait may grow to. The wait doubles after every + * replay, from the 10 s base to this cap, so a gateway that stays down costs a + * handful of attempts instead of one every ten seconds for the life of the + * mount. + */ +const TRANSLATION_RETRY_BACKOFF_CAP_MS = 300_000; + +/** + * One shared, backed-off scheduler for every mounted row whose summary is still + * unresolved. A per-row interval woke the JS thread once per failed row and kept + * issuing gateway calls while offline; this module arms a single timer for all + * of them, backs the wait off after every replay, and never wakes while + * `onlineManager` reports offline. `onlineManager` is already the app's single + * online source of truth (NetInfo drives `onlineManager.setOnline` in + * `query-client-lifecycle.tsx`), so no new dependency or platform branch is + * needed. When the last row disarms, the timer is cleared and the wait resets, + * so nothing outlives the account: the transcript unmounting on sign-out clears + * the timer, and the runtime's own memory clear drops the pending work. + */ +let retrySubscribers = 0; +let retryTimer: ReturnType | undefined = undefined; +let retryDelayMs = TOOL_SUMMARY_TRANSLATION_RETRY_MS; +let onlineListenerInstalled = false; + +function clearRetryTimer(): void { + if (retryTimer !== undefined) { + clearTimeout(retryTimer); + retryTimer = undefined; + } +} + +/** + * Arm the one shared timer when a row is subscribed and the device is online. + * The tick clears its handle, returns without replaying when there is nothing + * to serve or the device went offline (the online edge below re-arms), and + * otherwise replays the unresolved keys once, doubles the wait up to the cap and + * arms again. + */ +function armRetryTimer(): void { + if (retrySubscribers === 0 || retryTimer !== undefined || !onlineManager.isOnline()) { + return; + } + retryTimer = setTimeout(() => { + retryTimer = undefined; + if (retrySubscribers === 0 || !onlineManager.isOnline()) { + return; + } + retryUnresolvedTranslations(); + retryDelayMs = Math.min(retryDelayMs * 2, TRANSLATION_RETRY_BACKOFF_CAP_MS); + armRetryTimer(); + }, retryDelayMs); +} + +/** + * Register one mounted unresolved row with the shared retry and return the + * closure that deregisters it. The first row installs the single online + * listener; the reconnect edge resets the wait to the base delay and re-arms, + * so the first replay after coming back online happens at the base cadence + * rather than the backed-off one. The armed handle is cleared first: a + * reconnect inside an already-armed long backed-off wait must replace it, or + * `armRetryTimer`'s early return would leave the stale wait standing. The last + * disarmed row clears the timer and resets the delay, so a later mount starts + * from the base again. + */ +function armTranslationRetry(): () => void { + retrySubscribers += 1; + if (!onlineListenerInstalled) { + onlineListenerInstalled = true; + onlineManager.subscribe(online => { + if (online && retrySubscribers > 0) { + retryDelayMs = TOOL_SUMMARY_TRANSLATION_RETRY_MS; + clearRetryTimer(); + armRetryTimer(); + } + }); + } + armRetryTimer(); + return () => { + retrySubscribers -= 1; + if (retrySubscribers === 0) { + clearRetryTimer(); + retryDelayMs = TOOL_SUMMARY_TRANSLATION_RETRY_MS; + } + }; +} + /** * The translation for a tool summary and whether it is still on its way. The * subscription also carries the preference that decides whether to translate at @@ -60,10 +147,10 @@ export const TOOL_SUMMARY_TRANSLATION_RETRY_MS = 10_000; * single-line, so the swap cannot shift layout. * * `pending` is true only while translation is on for this text and the runtime - * holds no translation for it, but the row keeps asking while it stays mounted: - * a request that failed (or timed out) leaves the runtime uncached, so the row - * retries it rather than reporting the missing summary as final. A row that - * embeds the summary in a sentence of its own (`CondensedToolRunRow`) has + * holds no translation for it, but the row joins the shared retry while it stays + * mounted: a request that failed (or timed out) leaves the runtime uncached, so + * the row retries it rather than reporting the missing summary as final. A row + * that embeds the summary in a sentence of its own (`CondensedToolRunRow`) has * nowhere to put the original English while it waits, so it reads this flag * instead of the fallback text. */ @@ -94,19 +181,18 @@ export function useToolSummaryTranslation( return release; } // A request that settled without a translation is not final: while the row - // stays mounted it asks again, so a transient gateway failure resolves once - // the gateway recovers instead of stranding the label for the session. The - // runtime drops each tick while the key is cached, in flight or already - // queued, so the tick never stacks a second copy of the same summary. The - // tick releases its interest first because it is the same surface re-asking, - // not a second one, so the runtime's presence count stays at one per mounted - // surface instead of growing with every tick. - const retry = setInterval(() => { - releaseTranslationInterest({ itemId: id, text, language, model }); - ensureTranslation({ itemId: id, text, language, model }); - }, TOOL_SUMMARY_TRANSLATION_RETRY_MS); + // stays mounted it joins the one shared, backed-off retry, so a transient + // gateway failure resolves once the gateway recovers instead of stranding + // the label for the session. The row keeps its runtime interest while it + // waits, so the shared scheduler replays exactly the mounted unresolved keys + // in one batch, and the runtime still drops each replay while the key is + // cached, in flight or already queued, so no replay stacks a second copy of + // the same summary. The shared timer never wakes while the device is + // offline, and it stops as soon as the translation lands (this cleanup + // disarms it) or the last mounted row unmounts. + const disarm = armTranslationRetry(); return () => { - clearInterval(retry); + disarm(); release(); }; }, [active, itemId, text, language, config.model, translated]); diff --git a/apps/mobile/src/lib/use-new-session-repos.test.ts b/apps/mobile/src/lib/use-new-session-repos.test.ts index d67808ef53..3927eb6265 100644 --- a/apps/mobile/src/lib/use-new-session-repos.test.ts +++ b/apps/mobile/src/lib/use-new-session-repos.test.ts @@ -23,7 +23,7 @@ const mocks = vi.hoisted(() => ({ }) ), /** Every `useQuery` call in mount order, so a test can read the branch query's options. */ - queryCalls: [] as { queryKey?: unknown[]; enabled?: boolean }[], + queryCalls: [] as { queryKey?: unknown[]; enabled?: boolean; staleTime?: number }[], /** Result the branch query (the one with a `queryFn`) reports. */ branchQueryResult: emptyQueryResult(), })); @@ -40,7 +40,12 @@ vi.mock('@tanstack/react-query', () => ({ // Records every call so the branch query's key and `enabled` can be asserted. // The provider queries have no `queryFn`; the branch query does, and only it // reads `branchQueryResult`. - useQuery: (options: { queryKey?: unknown[]; enabled?: boolean; queryFn?: unknown }) => { + useQuery: (options: { + queryKey?: unknown[]; + enabled?: boolean; + staleTime?: number; + queryFn?: unknown; + }) => { mocks.queryCalls.push(options); const base = { data: undefined, @@ -89,12 +94,19 @@ vi.mock('@/lib/trpc', () => ({ useTRPC: () => ({ cloudAgentNext: { listGitHubRepositories: { - queryOptions: () => ({ queryKey: ['github'] }), + queryOptions: (_input: unknown, opts?: Record) => ({ + queryKey: ['github'], + ...opts, + }), queryKey: () => ['github'], }, listGitLabRepositories: { - queryOptions: ({ forceRefresh }: { forceRefresh: boolean }) => ({ + queryOptions: ( + { forceRefresh }: { forceRefresh: boolean }, + opts?: Record + ) => ({ queryKey: ['gitlab', forceRefresh], + ...opts, }), queryKey: ({ forceRefresh }: { forceRefresh: boolean }) => ['gitlab', forceRefresh], }, @@ -105,18 +117,29 @@ vi.mock('@/lib/trpc', () => ({ organizations: { cloudAgentNext: { listGitHubRepositories: { - queryOptions: () => ({ queryKey: ['github'] }), + queryOptions: (_input: unknown, opts?: Record) => ({ + queryKey: ['github'], + ...opts, + }), queryKey: () => ['github'], }, listGitLabRepositories: { - queryOptions: ({ forceRefresh }: { forceRefresh: boolean }) => ({ + queryOptions: ( + { forceRefresh }: { forceRefresh: boolean }, + opts?: Record + ) => ({ queryKey: ['gitlab', forceRefresh], + ...opts, }), queryKey: ({ forceRefresh }: { forceRefresh: boolean }) => ['gitlab', forceRefresh], }, listBitbucketRepositories: { - queryOptions: ({ forceRefresh }: { forceRefresh: boolean }) => ({ + queryOptions: ( + { forceRefresh }: { forceRefresh: boolean }, + opts?: Record + ) => ({ queryKey: ['bitbucket', forceRefresh], + ...opts, }), queryKey: ({ forceRefresh }: { forceRefresh: boolean }) => ['bitbucket', forceRefresh], }, @@ -216,6 +239,34 @@ describe('useNewSessionRepos force-fresh Bitbucket cache write', () => { }); }); +describe('useNewSessionRepos provider staleTime', () => { + // The three provider list queries are expensive, so they must not refetch on + // every mount of the new-session form. The force-fresh flows above write + // fresh results into these same keys. + function expectProviderStaleTime() { + expect(mocks.queryCalls.map(options => options.queryKey?.[0])).toEqual([ + 'github', + 'gitlab', + 'bitbucket', + ]); + for (const options of mocks.queryCalls) { + expect(options.staleTime).toBe(300_000); + } + } + + it('gives every provider repository query a five-minute staleTime (personal)', () => { + mountRepos(undefined); + + expectProviderStaleTime(); + }); + + it('gives every provider repository query a five-minute staleTime (organization)', () => { + mountRepos('org-1'); + + expectProviderStaleTime(); + }); +}); + // ── Branches of the selected repository ────────────────────────────── type BranchesResult = ReturnType; diff --git a/apps/mobile/src/lib/use-new-session-repos.ts b/apps/mobile/src/lib/use-new-session-repos.ts index 9b8598120b..81dfb9bc05 100644 --- a/apps/mobile/src/lib/use-new-session-repos.ts +++ b/apps/mobile/src/lib/use-new-session-repos.ts @@ -42,6 +42,23 @@ type UseNewSessionReposResult = { refreshReposForceFresh: () => Promise; }; +/** + * How long a provider's repository list stays fresh. Without it the three + * queries take the query client's `staleTime: 0` default and refetch a full + * repository list per provider on every mount of the new-session form, for + * data that changes at the rate of a repository being connected. + * + * This window is never the only way the lists update: every explicit + * invalidation path still writes fresh results into the same `forceRefresh: + * false` keys, so the next mount reads them instead of refetching — + * `refreshReposForceFresh` (through `fetchQuery` on its own `forceRefresh: + * true` keys at `staleTime: 0`), `forceFreshGitLab` and `forceFreshBitbucket` + * (`setQueryData` on the normal keys), and the connect/return flow + * (`useGitHubReposRefresh.performForceFresh` and + * `openAuthorizationAndWaitForReturn`). + */ +const NEW_SESSION_REPOS_STALE_TIME_MS = 5 * 60 * 1000; + export function useNewSessionRepos({ organizationId, }: UseNewSessionReposArgs): UseNewSessionReposResult { @@ -50,32 +67,47 @@ export function useNewSessionRepos({ const githubQuery = useQuery( organizationId - ? trpc.organizations.cloudAgentNext.listGitHubRepositories.queryOptions({ - organizationId, - forceRefresh: false, - }) - : trpc.cloudAgentNext.listGitHubRepositories.queryOptions({ - forceRefresh: false, - }) + ? trpc.organizations.cloudAgentNext.listGitHubRepositories.queryOptions( + { + organizationId, + forceRefresh: false, + }, + { staleTime: NEW_SESSION_REPOS_STALE_TIME_MS } + ) + : trpc.cloudAgentNext.listGitHubRepositories.queryOptions( + { + forceRefresh: false, + }, + { staleTime: NEW_SESSION_REPOS_STALE_TIME_MS } + ) ); const gitlabQuery = useQuery( organizationId - ? trpc.organizations.cloudAgentNext.listGitLabRepositories.queryOptions({ - organizationId, - forceRefresh: false, - }) - : trpc.cloudAgentNext.listGitLabRepositories.queryOptions({ - forceRefresh: false, - }) + ? trpc.organizations.cloudAgentNext.listGitLabRepositories.queryOptions( + { + organizationId, + forceRefresh: false, + }, + { staleTime: NEW_SESSION_REPOS_STALE_TIME_MS } + ) + : trpc.cloudAgentNext.listGitLabRepositories.queryOptions( + { + forceRefresh: false, + }, + { staleTime: NEW_SESSION_REPOS_STALE_TIME_MS } + ) ); // Bitbucket is organization-only: the query is disabled without an org. const bitbucketQuery = useQuery({ - ...trpc.organizations.cloudAgentNext.listBitbucketRepositories.queryOptions({ - organizationId: organizationId ?? '', - forceRefresh: false, - }), + ...trpc.organizations.cloudAgentNext.listBitbucketRepositories.queryOptions( + { + organizationId: organizationId ?? '', + forceRefresh: false, + }, + { staleTime: NEW_SESSION_REPOS_STALE_TIME_MS } + ), enabled: Boolean(organizationId), });