diff --git a/apps/mobile/src/lib/pr-review/diff/highlight.test.ts b/apps/mobile/src/lib/pr-review/diff/highlight.test.ts index 4794d6a5721..b4979661858 100644 --- a/apps/mobile/src/lib/pr-review/diff/highlight.test.ts +++ b/apps/mobile/src/lib/pr-review/diff/highlight.test.ts @@ -96,14 +96,6 @@ describe('highlightLine', () => { expect(texts(tokens).join('')).toBe('// hello'); }); - it('re-runs the same line through the cache and returns the same tokens', () => { - const a = highlightLine('const x = 1;', 'typescript'); - const b = highlightLine('const x = 1;', 'typescript'); - // Reference equality is not required, but the tokenized output - // should match exactly so the UI doesn't see visual diffs. - expect(b).toEqual(a); - }); - it('falls back to plain text when the highlighter throws', () => { // unknown language — lowlight throws and we recover. const tokens = highlightLine('hello world', 'not-a-real-language'); diff --git a/apps/mobile/src/lib/pr-review/diff/parse-patch-accessibility.test.ts b/apps/mobile/src/lib/pr-review/diff/parse-patch-accessibility.test.ts index 58068017a08..0fdbc4e62dc 100644 --- a/apps/mobile/src/lib/pr-review/diff/parse-patch-accessibility.test.ts +++ b/apps/mobile/src/lib/pr-review/diff/parse-patch-accessibility.test.ts @@ -70,15 +70,4 @@ describe('buildDiffLineAccessibilityLabel', () => { buildDiffLineAccessibilityLabel(makeLine({ type: 'del', oldLine: 2, text: ' \t ' })) ).toBe('Deleted line 2: (empty)'); }); - - // Regression guard: if a future refactor decouples the label from the - // parsed type, screen readers could report "Added" for a deleted line - // — which would be worse than the color-only fallback it replaces. - it('is derived from the parsed type — same input yields the same label, type drives the word', () => { - const added = makeLine({ type: 'add', newLine: 1, text: 'x' }); - const deleted = makeLine({ type: 'del', oldLine: 1, text: 'x' }); - expect(buildDiffLineAccessibilityLabel(added)).toBe(buildDiffLineAccessibilityLabel(added)); - expect(buildDiffLineAccessibilityLabel(added).startsWith('Added')).toBe(true); - expect(buildDiffLineAccessibilityLabel(deleted).startsWith('Deleted')).toBe(true); - }); }); diff --git a/apps/mobile/src/lib/pr-review/diff/pr-diff-list-builder.test.ts b/apps/mobile/src/lib/pr-review/diff/pr-diff-list-builder.test.ts index 6eec8d1f43b..b677ecd8b86 100644 --- a/apps/mobile/src/lib/pr-review/diff/pr-diff-list-builder.test.ts +++ b/apps/mobile/src/lib/pr-review/diff/pr-diff-list-builder.test.ts @@ -1,16 +1,9 @@ -import { describe, expect, it, vi } from 'vitest'; +import { describe, expect, it } from 'vitest'; import { buildItems } from '@/lib/pr-review/diff/pr-diff-list-builder'; import { type BuildItemsArgs, type ListItem } from '@/lib/pr-review/diff/pr-diff-list-items'; -import { parsePatch } from '@/lib/pr-review/diff/parse-patch'; import { type PrReviewFile } from '@/lib/pr-review/diff/pr-review-file-types'; -// Wraps the real parser in a spy so a test can count parse calls. -vi.mock('@/lib/pr-review/diff/parse-patch', async importOriginal => { - const actual = await importOriginal<{ parsePatch: typeof parsePatch }>(); - return { ...actual, parsePatch: vi.fn(actual.parsePatch) }; -}); - type FilePatchMissingItem = Extract; type SeparatorItem = Extract; @@ -144,7 +137,12 @@ describe('buildItems gap separators', () => { ); const trailing = separatorFor(items, 1); expect(trailing).toMatchObject({ context: { gapIndex: 1, startLine: 8 }, state: 'idle' }); - expect(Number.isFinite(trailing?.context.endLine ?? 0)).toBe(false); + // The trailing end is unknown, so the expand row must take its + // `isUnknownEnd` branch (default window, no Expand-all affordance). + // Asserted as the builder hands it over; the bounded half of the same + // contract is covered by `uses totalLines to bound the trailing gap once + // known`. + expect(trailing?.context.endLine).toBe(Infinity); }); it('renders between-hunk separators for gaps larger than 3 lines', () => { @@ -298,15 +296,3 @@ describe('buildItems trailing gap totalLines', () => { }); }); }); - -describe('buildFileItems parsed patch cache', () => { - it('parses each file object once across two builds that differ only in viewed', () => { - const spy = vi.mocked(parsePatch); - spy.mockClear(); - const args = baseArgs({ files: [makeFile(singleHunkPatch)], expanded: { 'a.ts': true } }); - const build = (next: BuildItemsArgs) => - buildItems(next).find((i): i is DiffLineListItem => i.kind === 'diff-line'); - expect(build(args)?.parsed).toBe(build({ ...args, viewed: () => true })?.parsed); - expect(spy).toHaveBeenCalledTimes(1); - }); -}); diff --git a/apps/mobile/src/lib/pr-review/diff/pr-review-file-list-state.mounted.test.tsx b/apps/mobile/src/lib/pr-review/diff/pr-review-file-list-state.mounted.test.tsx new file mode 100644 index 00000000000..e790cd71ca9 --- /dev/null +++ b/apps/mobile/src/lib/pr-review/diff/pr-review-file-list-state.mounted.test.tsx @@ -0,0 +1,150 @@ +import { createElement } from 'react'; +import { act } from '@/test/renderer'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { + type PrReviewFileListQueryResult, + usePrReviewFileListQuery, +} from '@/lib/pr-review/diff/pr-review-file-list-state'; +import { renderWithProviders, waitFor } from '@/test/render-with-providers'; + +// ── Mocks ────────────────────────────────────────────────────────────────── + +const queryFnMock = vi.hoisted(() => vi.fn()); + +vi.mock('@/lib/trpc', () => ({ + useTRPC: () => ({ + githubPrReview: { + listFiles: { + infiniteQueryOptions: (input: unknown, opts: object) => ({ + ...opts, + queryKey: ['githubPrReview', 'listFiles', input], + queryFn: queryFnMock, + initialPageParam: undefined as string | undefined, + getNextPageParam: (lastPage: { nextCursor: string | null }) => + lastPage.nextCursor ?? undefined, + }), + }, + }, + }), +})); + +// The hook's import chain pulls the real viewed-files store (SecureStore); +// the store is not the subject here, so it is stubbed like the other mounted +// suites for this module do. +vi.mock('@/lib/pr-review/viewed-files', () => ({ + getViewedFiles: vi.fn(async () => { + await Promise.resolve(); + return [] as string[]; + }), + toggleViewedFile: vi.fn(async () => { + await Promise.resolve(); + }), +})); + +// ── Harness ──────────────────────────────────────────────────────────────── + +type Result = PrReviewFileListQueryResult; + +function Probe({ holder }: { holder: { current: Result | null } }) { + holder.current = usePrReviewFileListQuery({ + owner: 'octocat', + repo: 'hello', + number: 1, + enabled: true, + }); + return null; +} + +async function mountProbe() { + const holder: { current: Result | null } = { current: null }; + await renderWithProviders(createElement(Probe, { holder })); + return holder; +} + +function current(holder: { current: Result | null }): Result { + const result = holder.current; + if (!result) { + throw new Error('probe did not render'); + } + return result; +} + +const FILE_A = { path: 'a.ts' }; +const FILE_B = { path: 'b.ts' }; + +beforeEach(() => { + queryFnMock.mockReset(); +}); + +// ── Tests ────────────────────────────────────────────────────────────────── + +describe('usePrReviewFileListQuery error classification', () => { + it('a first-page failure surfaces firstPageErrorState and no laterPageError', async () => { + queryFnMock.mockRejectedValue(new Error('network down')); + const holder = await mountProbe(); + + await waitFor(() => current(holder).errorState !== null); + + const result = current(holder); + expect(result.errorState?.kind).toBe('retryable'); + expect(result.firstPageErrorState?.kind).toBe('retryable'); + expect(result.laterPageError).toBe(false); + expect(result.files).toEqual([]); + }); + + it('a NOT_FOUND first page classifies as not-found', async () => { + queryFnMock.mockRejectedValue({ data: { code: 'NOT_FOUND' } }); + const holder = await mountProbe(); + + await waitFor(() => current(holder).errorState !== null); + + const result = current(holder); + expect(result.errorState?.kind).toBe('not-found'); + expect(result.firstPageErrorState?.kind).toBe('not-found'); + expect(result.laterPageError).toBe(false); + }); + + it('a later-page failure keeps the loaded files and flips laterPageError', async () => { + queryFnMock + .mockResolvedValueOnce({ files: [FILE_A], nextCursor: 'cursor-2' }) + .mockRejectedValueOnce(new Error('page 2 failed')); + const holder = await mountProbe(); + + // Load page one, then let the hook auto-run fetch-to-completion? No: the + // hook itself only reads; drive page two through the query handle it + // exposes. + await waitFor(() => current(holder).files.length === 1); + expect(current(holder).files).toEqual([FILE_A]); + + await act(async () => { + await current(holder).query.fetchNextPage(); + }); + await waitFor(() => current(holder).errorState !== null); + + const result = current(holder); + expect(result.errorState?.kind).toBe('retryable'); + // A page-1 set survived: the tab must not blank on a later-page failure. + expect(result.firstPageErrorState).toBeNull(); + expect(result.laterPageError).toBe(true); + expect(result.files).toEqual([FILE_A]); + expect(queryFnMock).toHaveBeenCalledTimes(2); + }); + + it('loads pages through the provider page shape (sanity on the option wiring)', async () => { + queryFnMock + .mockResolvedValueOnce({ files: [FILE_A], nextCursor: 'cursor-2' }) + .mockResolvedValueOnce({ files: [FILE_B], nextCursor: null }); + const holder = await mountProbe(); + + await act(async () => { + await current(holder).query.fetchNextPage(); + }); + + await waitFor(() => current(holder).files.length === 2); + expect(current(holder).files).toEqual([FILE_A, FILE_B]); + expect(current(holder).errorState).toBeNull(); + expect(current(holder).firstPageErrorState).toBeNull(); + expect(current(holder).laterPageError).toBe(false); + }); +}); diff --git a/apps/mobile/src/lib/pr-review/diff/pr-review-file-list-state.test.ts b/apps/mobile/src/lib/pr-review/diff/pr-review-file-list-state.test.ts index 345770a119f..67369ff7fe1 100644 --- a/apps/mobile/src/lib/pr-review/diff/pr-review-file-list-state.test.ts +++ b/apps/mobile/src/lib/pr-review/diff/pr-review-file-list-state.test.ts @@ -1,7 +1,6 @@ import { describe, expect, it, vi } from 'vitest'; import { buildPrReviewFileListQueryOptions } from './pr-review-file-list-state'; -import { PR_REVIEW_MAX_PAGES } from './pr-review-file-types'; // The hook module transitively imports react-native (via // `@/lib/query/infinite-retention` and the viewed-files store) and the real @@ -27,7 +26,11 @@ function createTrpcStub(infiniteQueryOptions: unknown) { } describe('buildPrReviewFileListQueryOptions', () => { - it('carries a numeric maxPages bound to PR_REVIEW_MAX_PAGES', () => { + // The literal ceiling, not the constant: this is the documented server page + // cap (60 pages × 50/page = 3,000 files, the same as the truncation banner). + // Comparing the builder's output to the same constant it imports could + // never catch the constant drifting to an unsafe value. + it('caps retention at the server page ceiling of 60', () => { const infiniteQueryOptions = vi.fn((_input: unknown, options: object) => options); const result = buildPrReviewFileListQueryOptions(createTrpcStub(infiniteQueryOptions), { owner: 'octocat', @@ -36,6 +39,6 @@ describe('buildPrReviewFileListQueryOptions', () => { enabled: true, }); - expect(result.maxPages).toBe(PR_REVIEW_MAX_PAGES); + expect(result.maxPages).toBe(60); }); }); diff --git a/apps/mobile/src/lib/pr-review/diff/pr-review-file-list-state.ts b/apps/mobile/src/lib/pr-review/diff/pr-review-file-list-state.ts index 652b3cd02b3..70cae0c8ef2 100644 --- a/apps/mobile/src/lib/pr-review/diff/pr-review-file-list-state.ts +++ b/apps/mobile/src/lib/pr-review/diff/pr-review-file-list-state.ts @@ -71,6 +71,9 @@ export function buildPrReviewFileListQueryOptions( ); } +/** The mounted contract the Files tab renders from: the raw infinite query plus the classified error states. */ +export type PrReviewFileListQueryResult = ReturnType; + export function usePrReviewFileListQuery(args: { owner: string; repo: string; diff --git a/apps/mobile/src/lib/pr-review/diff/use-pr-diff-context-loader.mounted.test.tsx b/apps/mobile/src/lib/pr-review/diff/use-pr-diff-context-loader.mounted.test.tsx index d3c769bf165..17a50715afe 100644 --- a/apps/mobile/src/lib/pr-review/diff/use-pr-diff-context-loader.mounted.test.tsx +++ b/apps/mobile/src/lib/pr-review/diff/use-pr-diff-context-loader.mounted.test.tsx @@ -76,4 +76,67 @@ describe('usePrDiffContextLoader', () => { await waitFor(() => current(holder).expandedContext['src/index.ts']?.[0]?.status === 'partial'); expect(queryFnMock).toHaveBeenCalledTimes(1); }); + + it('a provider answer with lines lands as partial with those lines', async () => { + queryFnMock.mockResolvedValue({ lines: ['line-a'], totalLines: 1 }); + const holder = await mountProbe(); + + act(() => { + current(holder).handleLoadContext(ITEM, 20); + }); + + await waitFor(() => current(holder).expandedContext['src/index.ts']?.[0]?.status === 'partial'); + expect(current(holder).expandedContext['src/index.ts']?.[0]).toEqual({ + status: 'partial', + lines: ['line-a'], + totalLines: 1, + }); + }); + + it('a provider answer with no lines marks the gap unavailable', async () => { + queryFnMock.mockResolvedValue({ lines: [], totalLines: 0 }); + const holder = await mountProbe(); + + act(() => { + current(holder).handleLoadContext(ITEM, 20); + }); + + await waitFor( + () => current(holder).expandedContext['src/index.ts']?.[0]?.status === 'unavailable' + ); + expect(current(holder).expandedContext['src/index.ts']?.[0]).toEqual({ + status: 'unavailable', + }); + }); + + it('a NOT_FOUND rejection marks the gap unavailable, not error', async () => { + queryFnMock.mockRejectedValue({ data: { code: 'NOT_FOUND', message: 'no such ref' } }); + const holder = await mountProbe(); + + act(() => { + current(holder).handleLoadContext(ITEM, 20); + }); + + await waitFor( + () => current(holder).expandedContext['src/index.ts']?.[0]?.status === 'unavailable' + ); + expect(current(holder).expandedContext['src/index.ts']?.[0]).toEqual({ + status: 'unavailable', + }); + }); + + it('any other rejection marks the gap error', async () => { + queryFnMock.mockRejectedValue(new Error('boom')); + const holder = await mountProbe(); + + act(() => { + current(holder).handleLoadContext(ITEM, 20); + }); + + await waitFor(() => current(holder).expandedContext['src/index.ts']?.[0]?.status === 'error'); + expect(current(holder).expandedContext['src/index.ts']?.[0]).toEqual({ + status: 'error', + lines: [], + }); + }); }); diff --git a/apps/mobile/src/lib/pr-review/diff/use-pr-diff-page-gate.mounted.test.tsx b/apps/mobile/src/lib/pr-review/diff/use-pr-diff-page-gate.mounted.test.tsx new file mode 100644 index 00000000000..3b294d548a5 --- /dev/null +++ b/apps/mobile/src/lib/pr-review/diff/use-pr-diff-page-gate.mounted.test.tsx @@ -0,0 +1,216 @@ +import { createElement } from 'react'; +import { act } from '@/test/renderer'; +import { describe, expect, it, vi } from 'vitest'; + +import { usePrDiffPageGate } from '@/lib/pr-review/diff/use-pr-diff-page-gate'; +import { renderWithProviders } from '@/test/render-with-providers'; +import { QueryClientProvider } from '@tanstack/react-query'; + +// The gate's consumer-visible contract: FlashList reports end-reached as soon +// as content fits the viewport, so an end-reach before the user drags is held, +// and the drag that opens the gate consumes it. The gate resets when the +// rendered PR's identity changes so a drag on the previous PR cannot load the +// next PR's pages. + +// The gate reads only three fields of the query result, so the stub names +// those three and casts once at the hook boundary; the full result type stays +// TanStack's. +type QueryStub = { + hasNextPage: boolean; + isFetchingNextPage: boolean; + fetchNextPage: () => Promise; +}; + +type GateResult = ReturnType; + +type GateHolder = { current: GateResult | null }; + +function GateProbe({ + identity, + query, + holder, +}: { + identity: string; + query: QueryStub; + holder: GateHolder; +}) { + holder.current = usePrDiffPageGate( + query as unknown as Parameters[0], + identity + ); + return null; +} + +async function mountGate(identity: string, query: QueryStub) { + const holder: GateHolder = { current: null }; + const { renderer, queryClient } = await renderWithProviders( + createElement(GateProbe, { identity, query, holder }) + ); + // Rerenders for an identity change must keep the same root element type + // (QueryClientProvider, as renderWithProviders mounted), or the swapped + // root remounts the whole tree and every useRef resets — a vacuous pass. + const rerender = (nextIdentity: string) => { + act(() => { + renderer.update( + createElement( + QueryClientProvider, + { client: queryClient }, + createElement(GateProbe, { identity: nextIdentity, query, holder }) + ) + ); + }); + }; + return { holder, rerender }; +} + +function gateOf(holder: GateHolder): GateResult { + const gate = holder.current; + if (!gate) { + throw new Error('gate probe did not render'); + } + return gate; +} + +function queryStub(overrides: Partial = {}): QueryStub { + return { + hasNextPage: true, + isFetchingNextPage: false, + fetchNextPage: vi.fn(async () => { + await Promise.resolve(); + }), + ...overrides, + }; +} + +describe('usePrDiffPageGate hold-until-drag', () => { + it('holds an end-reach that arrives before any drag', async () => { + const query = queryStub(); + const { holder } = await mountGate('pr-1', query); + + act(() => { + gateOf(holder).onEndReached(); + }); + + expect(query.fetchNextPage).not.toHaveBeenCalled(); + }); + + it('releases the held end-reach on the next drag, once', async () => { + const query = queryStub(); + const { holder } = await mountGate('pr-1', query); + + act(() => { + gateOf(holder).onEndReached(); + }); + // onDragStart is wired to both the scroll-begin and touch-move paths, so + // one gesture can report it repeatedly; only the first consumes the hold. + act(() => { + gateOf(holder).onDragStart(); + gateOf(holder).onDragStart(); + }); + + expect(query.fetchNextPage).toHaveBeenCalledTimes(1); + }); + + it('never fetches from a drag alone', async () => { + const query = queryStub(); + const { holder } = await mountGate('pr-1', query); + + act(() => { + gateOf(holder).onDragStart(); + }); + + expect(query.fetchNextPage).not.toHaveBeenCalled(); + }); + + it('fetches directly on end-reach after the user has dragged', async () => { + const query = queryStub(); + const { holder } = await mountGate('pr-1', query); + + act(() => { + gateOf(holder).onDragStart(); + }); + act(() => { + gateOf(holder).onEndReached(); + }); + + expect(query.fetchNextPage).toHaveBeenCalledTimes(1); + }); + + it('blocks the fetch when the query reports no next page or a running page fetch', async () => { + const exhausted = queryStub({ hasNextPage: false }); + const { holder: exhaustedHolder } = await mountGate('pr-1', exhausted); + act(() => { + gateOf(exhaustedHolder).onDragStart(); + }); + act(() => { + gateOf(exhaustedHolder).onEndReached(); + }); + expect(exhausted.fetchNextPage).not.toHaveBeenCalled(); + + const busy = queryStub({ isFetchingNextPage: true }); + const { holder: busyHolder } = await mountGate('pr-1', busy); + act(() => { + gateOf(busyHolder).onDragStart(); + }); + act(() => { + gateOf(busyHolder).onEndReached(); + }); + expect(busy.fetchNextPage).not.toHaveBeenCalled(); + }); + + it('resets the gate when the PR identity changes', async () => { + const query = queryStub(); + const { holder, rerender } = await mountGate('pr-1', query); + + // Case 1: the reader dragged the previous PR, then switched. The reset + // must discard userDragged too, or the new PR's mount-time end-reach + // fetches immediately. + act(() => { + gateOf(holder).onDragStart(); + }); + rerender('pr-2'); + act(() => { + gateOf(holder).onEndReached(); + }); + expect(query.fetchNextPage).not.toHaveBeenCalled(); + }); + + it('discards a held end-reach from the previous PR on identity change', async () => { + const query = queryStub(); + const { holder, rerender } = await mountGate('pr-1', query); + + // Case 2: an end-reach arrived on the previous PR, then the reader + // switched. A drag on the PR the reader left must not load the new one. + act(() => { + gateOf(holder).onEndReached(); + }); + rerender('pr-2'); + act(() => { + gateOf(holder).onDragStart(); + }); + expect(query.fetchNextPage).not.toHaveBeenCalled(); + }); + + it('treats the switched-in PR as a fresh gate: its end-reach is held until a drag', async () => { + const query = queryStub(); + const { holder, rerender } = await mountGate('pr-1', query); + + act(() => { + gateOf(holder).onEndReached(); + }); + rerender('pr-2'); + + // The new PR's list reports its end before the reader drags (a + // viewport-fitting first page does this at mount): held again. + act(() => { + gateOf(holder).onEndReached(); + }); + expect(query.fetchNextPage).not.toHaveBeenCalled(); + + // Exactly one fetch: the drag consumed pr-2's hold, not pr-1's. + act(() => { + gateOf(holder).onDragStart(); + }); + expect(query.fetchNextPage).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/mobile/src/lib/pr-review/diff/use-pr-diff-page-gate.ts b/apps/mobile/src/lib/pr-review/diff/use-pr-diff-page-gate.ts index e9a1d84e4fd..c242aa2af78 100644 --- a/apps/mobile/src/lib/pr-review/diff/use-pr-diff-page-gate.ts +++ b/apps/mobile/src/lib/pr-review/diff/use-pr-diff-page-gate.ts @@ -1,8 +1,8 @@ import { useCallback, useRef } from 'react'; -import { type usePrReviewFileListQuery } from '@/lib/pr-review/diff/pr-review-file-list-state'; +import { type PrReviewFileListQueryResult } from '@/lib/pr-review/diff/pr-review-file-list-state'; -type FileListQuery = ReturnType['query']; +type FileListQuery = PrReviewFileListQueryResult['query']; type PageGate = { userDragged: boolean; heldEndReach: boolean }; diff --git a/apps/mobile/src/lib/pr-review/diff/use-pr-review-viewed-files.mounted.test.tsx b/apps/mobile/src/lib/pr-review/diff/use-pr-review-viewed-files.mounted.test.tsx index fc0534260bb..227f5cd19e3 100644 --- a/apps/mobile/src/lib/pr-review/diff/use-pr-review-viewed-files.mounted.test.tsx +++ b/apps/mobile/src/lib/pr-review/diff/use-pr-review-viewed-files.mounted.test.tsx @@ -2,8 +2,8 @@ // end. The store folds `providerPrRefKey` into the durable key only when it // receives the ref, so a GitLab MR and a same-numbered GitHub PR — or one // project on two GitLab instances — never share a viewed set. These tests -// pin the hook's pass-through; the store's keying itself is covered in -// `viewed-files.test.ts`. +// exercise the hook's mounted behavior; the store's keying itself is covered +// in `viewed-files.test.ts`. import { createElement } from 'react'; import { beforeEach, describe, expect, it, vi } from 'vitest'; @@ -27,20 +27,59 @@ vi.mock('@/lib/trpc', () => ({ const storeMocks = vi.hoisted(() => ({ getViewedFiles: vi.fn(), toggleViewedFile: vi.fn(), - // The real key is the provider-scoped identity (rule 17); the store only - // needs it to be stable per ref, so the stub stringifies the ref. - viewedFilesKey: vi.fn((ref: unknown) => JSON.stringify(ref)), })); -vi.mock('@/lib/pr-review/viewed-files', () => storeMocks); +// Only the durable I/O layer is faked; `viewedFilesKey` stays the real +// provider-scoped implementation so the tests exercise the identity rule the +// hook exists for. The real `viewed-files` module loads fine in this node +// environment once its SecureStore chain is stubbed. +vi.mock('expo-secure-store', () => ({ + getItemAsync: vi.fn(async () => { + await Promise.resolve(); + return null; + }), + setItemAsync: vi.fn(async () => { + await Promise.resolve(); + }), + deleteItemAsync: vi.fn(async () => { + await Promise.resolve(); + }), +})); + +vi.mock('@/lib/storage-keys', () => ({ + PR_REVIEW_VIEWED_KEY: 'pr-review-viewed', +})); +vi.mock('@/lib/pr-review/viewed-files', async importOriginal => { + const actual = await importOriginal<{ + getViewedFiles: unknown; + toggleViewedFile: unknown; + viewedFilesKey: unknown; + }>(); + return { + ...actual, + getViewedFiles: storeMocks.getViewedFiles, + toggleViewedFile: storeMocks.toggleViewedFile, + }; +}); + +// The two fixtures share the legacy `owner/repo#number` triple: only the +// provider-scoped key suffix (`providerPrRefKey`) separates their viewed +// sets, so a real key that dropped the suffix would fail this suite. const GITLAB_REF: ProviderPrRef = { platform: 'gitlab', - projectPath: 'group/sub/repo', + projectPath: 'octocat/hello-world', mrIid: 12, instanceHint: 'https://gitlab.example.com', }; +const GITHUB_REF_SAME_NUMBER: ProviderPrRef = { + platform: 'github', + owner: 'octocat', + repo: 'hello-world', + number: 12, +}; + type ViewedRefArg = Parameters[0]; type ProbeResult = { @@ -71,10 +110,6 @@ function viewedOf(holder: ProbeHolder): ProbeResult { return holder.current; } -async function mountProbe(viewRef: ViewedRefArg) { - return viewedOf(await mountProbeHolder(viewRef)); -} - beforeEach(() => { // The mounted renderers are not unmounted between tests, so drop the store's // subscriptions to make each test start from a fresh read. @@ -84,27 +119,43 @@ beforeEach(() => { }); describe('usePrReviewViewedFiles provider refs (s6)', () => { - it('reads the viewed set under the provider ref, not the bare triple', async () => { - await mountProbe(GITLAB_REF); - expect(storeMocks.getViewedFiles).toHaveBeenCalledWith(GITLAB_REF, 'sha-1'); - }); + it('keeps two provider identities independent through the same mount flow', async () => { + // Same number, different providers: the store keys by provider identity, + // so a toggle on the GitLab MR never lights up the GitHub PR. The fake + // durable layer records per-key sets so the post-write revalidation sees + // exactly what the optimistic publish showed. + const durable: Record = {}; + storeMocks.getViewedFiles.mockImplementation(async (ref: unknown) => { + await Promise.resolve(); + return [...(durable[JSON.stringify(ref)] ?? [])]; + }); + storeMocks.toggleViewedFile.mockImplementation( + async (input: { path: string; headSha: string } & Record) => { + await Promise.resolve(); + // getViewedFiles is called as (ref, headSha); the durable record key + // is the ref alone, so the toggle input's headSha drops out too. + const { path, headSha: _headSha, ...ref } = input; + const key = JSON.stringify(ref); + const current = durable[key] ?? []; + durable[key] = current.includes(path) + ? current.filter(p => p !== path) + : [...current, path]; + } + ); + + const gitlab = await mountProbeHolder(GITLAB_REF); + const github = await mountProbeHolder(GITHUB_REF_SAME_NUMBER); + + // One durable read per distinct key: each identity read its own set. + expect(storeMocks.getViewedFiles).toHaveBeenCalledTimes(2); - it('writes the toggle under the provider ref, folding the key identity', async () => { - const viewed = await mountProbe(GITLAB_REF); await act(async () => { - await viewed.toggle('src/a.ts'); + await viewedOf(gitlab).toggle('src/a.ts'); }); - expect(storeMocks.toggleViewedFile).toHaveBeenCalledWith({ - ...GITLAB_REF, - headSha: 'sha-1', - path: 'src/a.ts', - }); - }); - it('keeps the legacy GitHub triple working byte-for-byte', async () => { - const legacy = { owner: 'octocat', repo: 'hello-world', number: 7 }; - await mountProbe(legacy); - expect(storeMocks.getViewedFiles).toHaveBeenCalledWith(legacy, 'sha-1'); + // The GitLab probe shows the toggle; the GitHub probe does not. + await waitFor(() => viewedOf(gitlab).isViewed('src/a.ts')); + expect(viewedOf(github).isViewed('src/a.ts')).toBe(false); }); it('shares one read and one toggle across two mounted probes', async () => { diff --git a/apps/mobile/src/lib/pr-review/diff/viewed-files-store.test.ts b/apps/mobile/src/lib/pr-review/diff/viewed-files-store.test.ts index 3254fcb8286..2dd6ea7ac47 100644 --- a/apps/mobile/src/lib/pr-review/diff/viewed-files-store.test.ts +++ b/apps/mobile/src/lib/pr-review/diff/viewed-files-store.test.ts @@ -29,10 +29,35 @@ const OTHER_REF = { owner: 'octocat', repo: 'other-repo', number: 7 }; const SHA1 = 'sha-1'; const SHA2 = 'sha-2'; -/** Let the store's in-flight read resolve and publish. */ -async function settle(): Promise { - await Promise.resolve(); - await Promise.resolve(); +/** + * Flush one macrotask turn: every pending microtask chain (any await depth) + * settles before the timeout fires. Tests that assert a publish HAS happened + * poll an observable condition with `waitFor`; tests that assert nothing + * published yet use one `flushTurn()` so the assertion is not coupled to the + * read chain's await count. + */ +async function flushTurn(): Promise { + const { promise, resolve } = Promise.withResolvers(); + setTimeout(resolve, undefined); + await promise; +} + +/** + * Bounded poll for an observable store condition. Each attempt flushes a + * macrotask turn before re-checking, so it observes any settled publish + * regardless of how many awaits the read chain contains. + */ +async function waitFor(predicate: () => boolean, attempts = 50): Promise { + for (let index = 0; index < attempts; index += 1) { + if (predicate()) { + return; + } + // eslint-disable-next-line no-await-in-loop -- polling must flush and re-check sequentially between turns + await flushTurn(); + } + if (!predicate()) { + throw new Error('waitFor: condition not met after 50 attempts'); + } } /** A `() => void` listener stub (typed so a void-returning parameter accepts it). */ @@ -57,7 +82,7 @@ describe('viewed-files-store', () => { expect(listener).not.toHaveBeenCalled(); expect(getViewedFilesSnapshot(REF, SHA1)).toEqual({ paths: [], isLoading: true }); - await settle(); + await waitFor(() => listener.mock.calls.length > 0); expect(storeMocks.getViewedFiles).toHaveBeenCalledWith(REF, SHA1); expect(getViewedFilesSnapshot(REF, SHA1)).toEqual({ paths: ['a.ts'], isLoading: false }); expect(listener).toHaveBeenCalledTimes(1); @@ -69,7 +94,7 @@ describe('viewed-files-store', () => { storeMocks.getViewedFiles.mockRejectedValue(new Error('read failed')); subscribeViewedFiles(REF, SHA1, listenerStub()); - await settle(); + await waitFor(() => !getViewedFilesSnapshot(REF, SHA1).isLoading); expect(getViewedFilesSnapshot(REF, SHA1)).toEqual({ paths: [], isLoading: false }); }); @@ -78,7 +103,7 @@ describe('viewed-files-store', () => { subscribeViewedFiles(REF, SHA1, listenerStub()); expect(getViewedFilesSnapshot(REF, SHA1)).toBe(getViewedFilesSnapshot(REF, SHA1)); - await settle(); + await waitFor(() => !getViewedFilesSnapshot(REF, SHA1).isLoading); const loaded = getViewedFilesSnapshot(REF, SHA1); expect(loaded).toBe(getViewedFilesSnapshot(REF, SHA1)); @@ -97,7 +122,7 @@ describe('viewed-files-store', () => { subscribeViewedFiles(REF, SHA1, first); subscribeViewedFiles(REF, SHA1, second); - await settle(); + await waitFor(() => second.mock.calls.length > 0); expect(storeMocks.getViewedFiles).toHaveBeenCalledTimes(1); expect(first).toHaveBeenCalledTimes(1); expect(second).toHaveBeenCalledTimes(1); @@ -108,7 +133,7 @@ describe('viewed-files-store', () => { storeMocks.getViewedFiles.mockResolvedValue(['a.ts']); const unsubscribeFirst = subscribeViewedFiles(REF, SHA1, listenerStub()); const unsubscribeSecond = subscribeViewedFiles(REF, SHA1, listenerStub()); - await settle(); + await waitFor(() => !getViewedFilesSnapshot(REF, SHA1).isLoading); // One listener leaving keeps the entry (and its loaded snapshot) alive. unsubscribeFirst(); @@ -119,7 +144,7 @@ describe('viewed-files-store', () => { storeMocks.getViewedFiles.mockResolvedValue(['b.ts']); subscribeViewedFiles(REF, SHA1, listenerStub()); - await settle(); + await waitFor(() => getViewedFilesSnapshot(REF, SHA1).paths.includes('b.ts')); expect(storeMocks.getViewedFiles).toHaveBeenCalledTimes(2); expect(getViewedFilesSnapshot(REF, SHA1).paths).toEqual(['b.ts']); }); @@ -132,7 +157,7 @@ describe('viewed-files-store', () => { subscribeViewedFiles(REF, SHA1, own); subscribeViewedFiles(OTHER_REF, SHA1, otherRef); subscribeViewedFiles(REF, SHA2, otherSha); - await settle(); + await waitFor(() => !getViewedFilesSnapshot(REF, SHA2).isLoading); own.mockClear(); otherRef.mockClear(); @@ -153,13 +178,10 @@ describe('viewed-files-store', () => { const other = listenerStub(); subscribeViewedFiles(REF, SHA1, own); subscribeViewedFiles(OTHER_REF, SHA1, other); - await settle(); + await waitFor(() => !getViewedFilesSnapshot(OTHER_REF, SHA1).isLoading); - let releaseWrite: () => void = undefined as unknown as () => void; - const writeGate = new Promise(resolve => { - releaseWrite = resolve; - }); - storeMocks.toggleViewedFile.mockReturnValue(writeGate); + const writeGate = Promise.withResolvers(); + storeMocks.toggleViewedFile.mockReturnValue(writeGate.promise); storeMocks.getViewedFiles.mockClear(); own.mockClear(); other.mockClear(); @@ -170,9 +192,9 @@ describe('viewed-files-store', () => { expect(own).toHaveBeenCalledTimes(1); expect(other).not.toHaveBeenCalled(); - releaseWrite(); + writeGate.resolve(undefined); await toggling; - await settle(); + await waitFor(() => other.mock.calls.length > 0); expect(storeMocks.toggleViewedFile).toHaveBeenCalledWith({ ...REF, @@ -202,51 +224,55 @@ describe('viewed-files-store', () => { } }); subscribeViewedFiles(REF, SHA1, listenerStub()); - await settle(); + await waitFor(() => !getViewedFilesSnapshot(REF, SHA1).isLoading); await applyViewedFilesToggle(REF, SHA1, 'a.ts'); - await settle(); + await waitFor( + () => + !getViewedFilesSnapshot(REF, SHA1).isLoading && + !getViewedFilesSnapshot(REF, SHA1).paths.includes('a.ts') + ); expect(getViewedFilesSnapshot(REF, SHA1).paths).toEqual([]); }); it('revalidateAllViewedFiles re-reads every open key', async () => { - const durable = new Map(); + const durable: Record = {}; storeMocks.getViewedFiles.mockImplementation(async (ref: { owner: string; repo: string }) => { await Promise.resolve(); - return durable.get(`${ref.owner}/${ref.repo}`) ?? []; + return durable[`${ref.owner}/${ref.repo}`] ?? []; }); subscribeViewedFiles(REF, SHA1, listenerStub()); subscribeViewedFiles(OTHER_REF, SHA1, listenerStub()); - await settle(); + await waitFor(() => !getViewedFilesSnapshot(REF, SHA1).isLoading); - durable.set('octocat/hello-world', ['a.ts']); - durable.set('octocat/other-repo', ['b.ts']); + durable['octocat/hello-world'] = ['a.ts']; + durable['octocat/other-repo'] = ['b.ts']; revalidateAllViewedFiles(); - await settle(); + await waitFor( + () => + getViewedFilesSnapshot(REF, SHA1).paths.includes('a.ts') && + getViewedFilesSnapshot(OTHER_REF, SHA1).paths.includes('b.ts') + ); expect(getViewedFilesSnapshot(REF, SHA1).paths).toEqual(['a.ts']); expect(getViewedFilesSnapshot(OTHER_REF, SHA1).paths).toEqual(['b.ts']); }); it('does not resurrect an entry whose read resolves after the last unsubscribe', async () => { - let releaseRead: (paths: string[]) => void = undefined as unknown as (paths: string[]) => void; - const readGate = new Promise(resolve => { - releaseRead = resolve; - }); - storeMocks.getViewedFiles.mockReturnValue(readGate); + const readGate = Promise.withResolvers(); + storeMocks.getViewedFiles.mockReturnValue(readGate.promise); const unsubscribe = subscribeViewedFiles(REF, SHA1, listenerStub()); unsubscribe(); - releaseRead(['a.ts']); - await settle(); - + readGate.resolve(['a.ts']); + await flushTurn(); expect(getViewedFilesSnapshot(REF, SHA1)).toEqual({ paths: [], isLoading: true }); }); it('resetViewedFilesStoreForTests clears every entry', async () => { storeMocks.getViewedFiles.mockResolvedValue(['a.ts']); subscribeViewedFiles(REF, SHA1, listenerStub()); - await settle(); + await waitFor(() => !getViewedFilesSnapshot(REF, SHA1).isLoading); expect(getViewedFilesSnapshot(REF, SHA1).isLoading).toBe(false); resetViewedFilesStoreForTests(); diff --git a/apps/mobile/src/lib/pr-review/viewed-files.test.ts b/apps/mobile/src/lib/pr-review/viewed-files.test.ts index cdac3e7dc68..63faeb58208 100644 --- a/apps/mobile/src/lib/pr-review/viewed-files.test.ts +++ b/apps/mobile/src/lib/pr-review/viewed-files.test.ts @@ -87,7 +87,7 @@ describe('viewed-files', () => { it('caps the map at 20 PRs, evicting the least-recently-touched', async () => { const seed: Record = {}; for (let index = 0; index < 20; index += 1) { - seed[`octocat/repo-${index}#1`] = { headSha: SHA1, viewedPaths: [] }; + seed[`octocat/repo-${index}#1`] = { headSha: SHA1, viewedPaths: [`kept-${index}.ts`] }; } setStored(seed); await toggleViewedFile({ @@ -102,14 +102,21 @@ describe('viewed-files', () => { await expect( getViewedFiles({ owner: 'octocat', repo: 'brand-new-repo', number: 1 }, SHA1) ).resolves.toEqual(['a.ts']); - // The oldest (repo-0) was evicted; reading it yields an empty list. + // The oldest (repo-0) was evicted; its set is gone, not just emptied. await expect( getViewedFiles({ owner: 'octocat', repo: 'repo-0', number: 1 }, SHA1) ).resolves.toEqual([]); - // The most-recently-touched original (repo-19) is still present. + // The most-recently-touched original survives WITH its own set — an + // empty-seed eviction would make this assertion pass vacuously. await expect( getViewedFiles({ owner: 'octocat', repo: 'repo-19', number: 1 }, SHA1) - ).resolves.toEqual([]); + ).resolves.toEqual(['kept-19.ts']); + // The stored map itself holds exactly the cap: 20 entries after the + // 21st toggle, not 19 survivors plus the newcomer. + const stored = JSON.parse(store.get('pr-review-viewed') ?? '{}') as Record; + expect(Object.keys(stored)).toHaveLength(20); + expect(stored['octocat/repo-0#1']).toBeUndefined(); + expect(stored['octocat/repo-19#1']).toEqual({ headSha: SHA1, viewedPaths: ['kept-19.ts'] }); }); it('treats corrupt stored data as an empty map', async () => { diff --git a/apps/mobile/src/lib/query/infinite-retention.test.ts b/apps/mobile/src/lib/query/infinite-retention.test.ts index c348e8aabc3..dfdd369bb58 100644 --- a/apps/mobile/src/lib/query/infinite-retention.test.ts +++ b/apps/mobile/src/lib/query/infinite-retention.test.ts @@ -148,11 +148,11 @@ describe('reconcileFirstPage', () => { }); }); -describe('infinite-query refetch fan-out', () => { +describe('infinite-query refetch fan-out through withInfiniteRetention', () => { it('re-requests every retained page, so maxPages bounds a single refetch', async () => { const queryClient = new QueryClient(); let calls = 0; - const observer = new InfiniteQueryObserver(queryClient, { + const baseOptions = { queryKey: ['refetch-fanout'], // eslint-disable-next-line require-await -- the recipe pins an async queryFn; the body needs no await. queryFn: async ({ pageParam }: { pageParam: number }) => { @@ -160,10 +160,15 @@ describe('infinite-query refetch fan-out', () => { return page([{ id: String(pageParam), title: 'x' }]); }, initialPageParam: 0, - getNextPageParam: (_last, pages) => pages.length, - maxPages: 3, + getNextPageParam: (_lastPage: Page, pages: Page[]) => pages.length, staleTime: Infinity, retry: false, + } as const; + // The observer options come from this module's own helper, so the + // retention bound the app actually uses is what this test exercises. + const observer = new InfiniteQueryObserver(queryClient, { + ...baseOptions, + ...withInfiniteRetention(baseOptions, 3), }); // eslint-disable-next-line no-empty-function -- a real listener is required to activate the observer; the body is intentionally empty. const unsubscribe = observer.subscribe(() => {}); @@ -175,6 +180,13 @@ describe('infinite-query refetch fan-out', () => { } expect(observer.getCurrentResult().data?.pages).toHaveLength(3); + // A 4th page exists; the cap of 3 must evict the oldest page instead of + // retaining it. Without the helper's maxPages, the pages array would grow + // to 4 and the refetch below would fan out 4 requests. + // eslint-disable-next-line no-await-in-loop -- the cursor chain continues from page 3. + await observer.fetchNextPage(); + expect(observer.getCurrentResult().data?.pages).toHaveLength(3); + calls = 0; await observer.refetch();