Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 100 additions & 3 deletions apps/mobile/src/components/agents/file-part-url-resolver.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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' },
Expand All @@ -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 () => {
Expand Down
96 changes: 85 additions & 11 deletions apps/mobile/src/components/agents/file-part-url-resolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,21 @@ 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. */
const inFlight = new Map<string, () => boolean>();

// One module-level sweeper serves every mounted subscriber across the app.
let renewSubscribers = 0;
let renewTimer: ReturnType<typeof setInterval> | undefined = undefined;
let renewTimer: ReturnType<typeof setTimeout> | undefined = undefined;

export type ResolvedFilePartUrl = {
status: 'ready' | 'resolving' | 'unavailable' | 'error';
Expand Down Expand Up @@ -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()) {
Expand All @@ -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;
Comment thread
iscekic marked this conversation as resolved.
}
}
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<boolean>[] {
const now = Date.now();
const renewals: Promise<boolean>[] = [];
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<void> {
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;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}
/>
</View>
Expand Down
56 changes: 56 additions & 0 deletions apps/mobile/src/components/query-error.mounted.test.tsx
Original file line number Diff line number Diff line change
@@ -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(<QueryError onRetry={onRetry} />);

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(
<QueryError onRetry={vi.fn<() => 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();
});
});
13 changes: 11 additions & 2 deletions apps/mobile/src/components/query-error.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -63,6 +70,7 @@ export function QueryError({
title,
message,
onRetry,
retryLabel,
isRetrying = false,
className,
placement = 'center',
Expand All @@ -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 (
<EmptyState
Expand All @@ -93,9 +102,9 @@ export function QueryError({
variant="outline"
onPress={onRetry}
loading={isRetrying}
accessibilityLabel={t('common.retry')}
accessibilityLabel={retryText}
>
<Text>{t('common.retry')}</Text>
<Text>{retryText}</Text>
</Button>
)
}
Expand Down
Loading
Loading