diff --git a/apps/mobile/src/components/organization/members-list-items.test.ts b/apps/mobile/src/components/organization/members-list-items.test.ts new file mode 100644 index 0000000000..10678db35c --- /dev/null +++ b/apps/mobile/src/components/organization/members-list-items.test.ts @@ -0,0 +1,100 @@ +import { describe, expect, it } from 'vitest'; + +import { type ActiveOrgMember, type InvitedOrgMember } from '@/lib/hooks/use-organization-queries'; + +import { buildMembersListItems } from './members-list-items'; + +function activeMember(id: string): ActiveOrgMember { + return { + id, + name: `Member ${id}`, + email: `${id}@example.com`, + role: 'member', + status: 'active', + inviteDate: null, + dailyUsageLimitUsd: null, + currentDailyUsageUsd: null, + }; +} + +function invitedMember(id: string): InvitedOrgMember { + return { + email: `${id}@example.com`, + role: 'member', + inviteDate: null, + inviteToken: `token-${id}`, + inviteId: `invite-${id}`, + status: 'invited', + inviteUrl: `https://example.com/invite/${id}`, + emailStatus: null, + dailyUsageLimitUsd: null, + currentDailyUsageUsd: null, + }; +} + +describe('buildMembersListItems', () => { + it('returns [] when both lists are empty', () => { + expect(buildMembersListItems({ activeMembers: [], invitedMembers: [] })).toEqual([]); + }); + + it('builds members only with a Members section and a last flag', () => { + const items = buildMembersListItems({ + activeMembers: [activeMember('a'), activeMember('b')], + invitedMembers: [], + }); + expect(items).toEqual([ + { kind: 'section', title: 'Members' }, + { kind: 'member', member: activeMember('a'), last: false }, + { kind: 'member', member: activeMember('b'), last: true }, + ]); + }); + + it('builds members plus invites with both sections and correct last flags', () => { + const items = buildMembersListItems({ + activeMembers: [activeMember('a')], + invitedMembers: [invitedMember('i1'), invitedMember('i2')], + }); + expect(items).toEqual([ + { kind: 'section', title: 'Members' }, + { kind: 'member', member: activeMember('a'), last: true }, + { kind: 'section', title: 'Pending invitations' }, + { kind: 'invite', invite: invitedMember('i1'), last: false }, + { kind: 'invite', invite: invitedMember('i2'), last: true }, + ]); + }); + + it('omits the Pending invitations header when there are no invites', () => { + const items = buildMembersListItems({ + activeMembers: [activeMember('a')], + invitedMembers: [], + }); + expect( + items.some(item => item.kind === 'section' && item.title === 'Pending invitations') + ).toBe(false); + expect(items.some(item => item.kind === 'invite')).toBe(false); + }); + + it('starts with members-empty when there are no active members but invites exist', () => { + const items = buildMembersListItems({ + activeMembers: [], + invitedMembers: [invitedMember('i1')], + }); + expect(items[0]).toEqual({ kind: 'members-empty' }); + expect(items).toEqual([ + { kind: 'members-empty' }, + { kind: 'section', title: 'Pending invitations' }, + { kind: 'invite', invite: invitedMember('i1'), last: true }, + ]); + }); + + it('flags the final row of each group as last', () => { + const items = buildMembersListItems({ + activeMembers: [activeMember('a'), activeMember('b')], + invitedMembers: [invitedMember('i1')], + }); + const members = items.filter(item => item.kind === 'member'); + const invites = items.filter(item => item.kind === 'invite'); + expect(members.map(item => item.last)).toEqual([false, true]); + expect(invites.map(item => item.last)).toEqual([true]); + }); +}); diff --git a/apps/mobile/src/components/organization/members-list-items.ts b/apps/mobile/src/components/organization/members-list-items.ts new file mode 100644 index 0000000000..eb1156bb6a --- /dev/null +++ b/apps/mobile/src/components/organization/members-list-items.ts @@ -0,0 +1,55 @@ +// Pure builder for the organization Members screen's FlashList items. +// +// One flat array drives a single FlashList. The composition rules are exact so +// the empty state can never go missing: +// +// - both lists empty -> `[]` (the list renders `ListEmptyComponent`) +// - active members present -> `section: 'Members'` + one `member` row each +// - no active, invites -> `members-empty` + the invites section +// - invites present -> `section: 'Pending invitations'` + one +// `invite` row each +// +// `last` marks the final row of each group so the existing hairline rule is +// preserved. + +import { type ActiveOrgMember, type InvitedOrgMember } from '@/lib/hooks/use-organization-queries'; + +export type MembersListItem = + | { kind: 'section'; title: string } + | { kind: 'members-empty' } + | { kind: 'member'; member: ActiveOrgMember; last: boolean } + | { kind: 'invite'; invite: InvitedOrgMember; last: boolean }; + +export function buildMembersListItems(args: { + activeMembers: ActiveOrgMember[]; + invitedMembers: InvitedOrgMember[]; +}): MembersListItem[] { + const { activeMembers, invitedMembers } = args; + const items: MembersListItem[] = []; + + if (activeMembers.length > 0) { + items.push({ kind: 'section', title: 'Members' }); + for (const [index, member] of activeMembers.entries()) { + items.push({ + kind: 'member', + member, + last: index === activeMembers.length - 1, + }); + } + } else if (invitedMembers.length > 0) { + items.push({ kind: 'members-empty' }); + } + + if (invitedMembers.length > 0) { + items.push({ kind: 'section', title: 'Pending invitations' }); + for (const [index, invite] of invitedMembers.entries()) { + items.push({ + kind: 'invite', + invite, + last: index === invitedMembers.length - 1, + }); + } + } + + return items; +} diff --git a/apps/mobile/src/components/organization/members-screen.mounted.test.tsx b/apps/mobile/src/components/organization/members-screen.mounted.test.tsx new file mode 100644 index 0000000000..e9684dcd36 --- /dev/null +++ b/apps/mobile/src/components/organization/members-screen.mounted.test.tsx @@ -0,0 +1,169 @@ +/* eslint-disable typescript-eslint/no-deprecated -- react-test-renderer is the DOM-free renderer used to mount React/RN trees under vitest (node env, no jsdom); same pattern as src/test/render-with-providers.tsx. */ + +// Screen-level empty-state precedence regression: when the member query errors +// with no data, both member arrays are empty, so the list's empty component +// must render the QueryError — not "No members yet". The item builder and the +// error selector are unit-tested separately; this proves the loading → error → +// empty precedence in the screen JSX itself. + +import { type ComponentType, createElement, type ReactElement } from 'react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { renderWithProviders } from '@/test/render-with-providers'; + +import { OrganizationMembersScreen } from './members-screen'; + +const withMembersQuery = vi.hoisted(() => ({ + data: undefined as unknown, + isLoading: false, + isFetching: false, + isError: false, + error: null as unknown, + refetch: vi.fn(), +})); + +vi.mock('@/lib/hooks/use-organization-queries', () => ({ + isMoneyRole: () => true, + useOrgBoundary: () => ({ + organizationId: 'org-1', + role: 'owner', + org: { organizationId: 'org-1', role: 'owner' }, + isResolving: false, + }), + useOrgWithMembers: () => withMembersQuery, +})); + +vi.mock('@shopify/flash-list', () => ({ + FlashList: (props: { + data?: unknown[]; + ListEmptyComponent?: ComponentType | ReactElement | null; + }) => { + const data = props.data ?? []; + if (data.length === 0) { + const Empty = props.ListEmptyComponent; + if (typeof Empty === 'function') { + return createElement(Empty); + } + return Empty ?? null; + } + return null; + }, +})); + +vi.mock('expo-router', () => ({ + useRouter: () => ({ push: vi.fn() }), +})); + +vi.mock('@/components/ui/icons', () => ({ + UserPlus: 'UserPlus', + Users: 'Users', +})); + +vi.mock('@/components/empty-state', () => ({ + EmptyState: ({ title }: { title: string }) => `EMPTY_STATE:${title}`, +})); + +vi.mock('@/components/organization/invited-member-row', () => ({ + InvitedMemberRow: () => null, +})); + +vi.mock('@/components/organization/member-row', () => ({ + MemberRow: () => null, +})); + +vi.mock('@/components/organization/organization-boundary', () => ({ + OrganizationBoundary: () => null, +})); + +vi.mock('@/components/query-error', () => ({ + QueryError: () => 'QUERY_ERROR', +})); + +vi.mock('@/components/screen-header', () => ({ + ScreenHeader: () => null, +})); + +vi.mock('@/components/ui/button', () => ({ + Button: 'Button', +})); + +vi.mock('@/components/ui/skeleton', () => ({ + Skeleton: 'Skeleton', +})); + +vi.mock('@/components/ui/text', () => ({ + Text: 'Text', +})); + +vi.mock('@/components/tab-screen', () => ({ + useTabBarBottomPadding: () => 0, +})); + +vi.mock('@/lib/hooks/use-current-user-id', () => ({ + useCurrentUserId: () => ({ userId: 'user-1' }), +})); + +vi.mock('@/lib/hooks/use-theme-colors', () => ({ + useThemeColors: () => ({ foreground: '#000000' }), +})); + +vi.mock('@/lib/utils', () => ({ + cn: (...args: unknown[]) => args.filter(Boolean).join(' '), + firstNonEmpty: (...args: (string | null | undefined)[]) => + args.find(value => value != null && value !== '') ?? '', + parseTimestamp: (value: string) => new Date(value), +})); + +vi.mock('react-native', () => ({ + Pressable: 'Pressable', + View: 'View', +})); + +function collectText(node: unknown): string[] { + if (node == null) { + return []; + } + if (typeof node === 'string') { + return [node]; + } + if (Array.isArray(node)) { + return node.flatMap(item => collectText(item)); + } + if (typeof node === 'object' && 'children' in node) { + return collectText((node as { children?: unknown }).children); + } + return []; +} + +async function renderScreen(): Promise { + const { renderer } = await renderWithProviders(createElement(OrganizationMembersScreen)); + return collectText(renderer.toJSON()); +} + +beforeEach(() => { + withMembersQuery.data = undefined; + withMembersQuery.isLoading = false; + withMembersQuery.isFetching = false; + withMembersQuery.isError = false; + withMembersQuery.error = null; + withMembersQuery.refetch.mockClear(); +}); + +describe('OrganizationMembersScreen empty-state precedence', () => { + it('renders QueryError, not "No members yet", when an error leaves both member arrays empty', async () => { + withMembersQuery.isError = true; + withMembersQuery.error = { data: { code: 'INTERNAL_SERVER_ERROR' } }; + + const texts = await renderScreen(); + + expect(texts).toContain('QUERY_ERROR'); + expect(texts).not.toContain('No members yet'); + }); + + it('renders "No members yet" when there is no error and both member arrays are empty', async () => { + const texts = await renderScreen(); + + expect(texts).not.toContain('QUERY_ERROR'); + expect(texts).toContain('EMPTY_STATE:No members yet'); + }); +}); diff --git a/apps/mobile/src/components/organization/members-screen.tsx b/apps/mobile/src/components/organization/members-screen.tsx index b7650d4008..37d54789ae 100644 --- a/apps/mobile/src/components/organization/members-screen.tsx +++ b/apps/mobile/src/components/organization/members-screen.tsx @@ -1,8 +1,8 @@ +import { FlashList } from '@shopify/flash-list'; import { type Href, useRouter } from 'expo-router'; import { UserPlus, Users } from '@/components/ui/icons'; -import { type ReactNode } from 'react'; -import { Pressable, View } from 'react-native'; -import Animated, { FadeIn, FadeOut, LinearTransition } from 'react-native-reanimated'; +import { useMemo } from 'react'; +import { Pressable, View, type ViewStyle } from 'react-native'; import { EmptyState } from '@/components/empty-state'; import { InvitedMemberRow } from '@/components/organization/invited-member-row'; @@ -13,7 +13,7 @@ import { ScreenHeader } from '@/components/screen-header'; import { Button } from '@/components/ui/button'; import { Skeleton } from '@/components/ui/skeleton'; import { Text } from '@/components/ui/text'; -import { TabScreenScrollView } from '@/components/tab-screen'; +import { useTabBarBottomPadding } from '@/components/tab-screen'; import { useCurrentUserId } from '@/lib/hooks/use-current-user-id'; import { type ActiveOrgMember, @@ -23,7 +23,10 @@ import { useOrgWithMembers, } from '@/lib/hooks/use-organization-queries'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; -import { firstNonEmpty, parseTimestamp } from '@/lib/utils'; +import { cn, firstNonEmpty, parseTimestamp } from '@/lib/utils'; + +import { buildMembersListItems, type MembersListItem } from './members-list-items'; +import { selectOrgListErrorView } from './org-list-error-view'; function sortActiveMembers(members: ActiveOrgMember[]): ActiveOrgMember[] { // eslint-disable-next-line unicorn/no-array-sort -- toSorted() is not available in Hermes @@ -56,12 +59,28 @@ function MemberRowSkeleton({ last }: Readonly<{ last?: boolean }>) { ); } +const listStyle = { flex: 1 } satisfies ViewStyle; +const listContentContainerStyle = { paddingTop: 16, flexGrow: 1 } satisfies ViewStyle; + export function OrganizationMembersScreen() { const router = useRouter(); const colors = useThemeColors(); const { organizationId, role, org, isResolving } = useOrgBoundary(); const orgWithMembers = useOrgWithMembers(organizationId); const { userId: currentUserId } = useCurrentUserId(); + const paddingBottom = useTabBarBottomPadding(); + + const activeMembers = sortActiveMembers( + orgWithMembers.data?.members.filter(m => m.status === 'active') ?? [] + ); + const invitedMembers = sortInvitedMembers( + orgWithMembers.data?.members.filter(m => m.status === 'invited') ?? [] + ); + + const items = useMemo( + () => buildMembersListItems({ activeMembers, invitedMembers }), + [activeMembers, invitedMembers] + ); if (isResolving || organizationId == null || org == null) { return ; @@ -73,70 +92,115 @@ export function OrganizationMembersScreen() { const canInvite = isMoneyRole(role); const isOwner = role === 'owner'; - const activeMembers = sortActiveMembers( - orgWithMembers.data?.members.filter(m => m.status === 'active') ?? [] - ); - const invitedMembers = sortInvitedMembers( - orgWithMembers.data?.members.filter(m => m.status === 'invited') ?? [] + const errorView = isError ? selectOrgListErrorView(orgWithMembers.error) : null; + + const emptyState = ( + { + router.push('/(app)/(tabs)/(3_profile)/organization/invite-member' as Href); + }} + > + Invite member + + ) : undefined + } + /> ); - let membersBody: ReactNode = null; - if (isLoading) { - membersBody = ( - - - - - - ); - } else if (isError) { - membersBody = ( - void orgWithMembers.refetch()} - isRetrying={orgWithMembers.isFetching} - placement="top" - /> - ); - } else if (activeMembers.length === 0) { - membersBody = ( - { - router.push('/(app)/(tabs)/(3_profile)/organization/invite-member' as Href); - }} - > - Invite member - - ) : undefined - } - /> - ); - } else { - membersBody = ( - - {activeMembers.map((member, index) => ( - - ))} - - ); - } + // Loading, error, and empty are mutually exclusive and evaluated in this + // order. An error leaves both member arrays empty, so it must be checked + // before the empty branch — otherwise a 500 renders "No members yet". + const renderListEmpty = () => { + if (isLoading) { + return ( + + + + + + ); + } + if (errorView) { + return ( + void orgWithMembers.refetch() : undefined} + isRetrying={orgWithMembers.isFetching} + placement="top" + /> + ); + } + return emptyState; + }; + + const renderItem = ({ item, index }: { item: MembersListItem; index: number }) => { + switch (item.kind) { + case 'section': { + return ( + + {item.title} + + ); + } + case 'members-empty': { + return emptyState; + } + case 'member': { + const isFirst = index === 0 || items[index - 1]?.kind === 'section'; + return ( + + + + ); + } + case 'invite': { + const isFirst = index === 0 || items[index - 1]?.kind === 'section'; + return ( + + + + ); + } + default: { + const _exhaustive: never = item; + return _exhaustive; + } + } + }; return ( @@ -158,37 +222,36 @@ export function OrganizationMembersScreen() { ) : undefined } /> - { + switch (item.kind) { + case 'section': { + return `section:${item.title}`; + } + case 'members-empty': { + return 'members-empty'; + } + case 'member': { + return item.member.id; + } + case 'invite': { + return item.invite.inviteId; + } + default: { + const _exhaustive: never = item; + return _exhaustive; + } + } + }} + getItemType={item => item.kind} + ListEmptyComponent={renderListEmpty} + ListFooterComponent={} showsVerticalScrollIndicator={false} - > - - Members - {membersBody} - - - {!isLoading && !isError && invitedMembers.length > 0 && ( - - Pending invitations - - {invitedMembers.map((invite, index) => ( - - ))} - - - )} - + contentContainerStyle={listContentContainerStyle} + /> ); } diff --git a/apps/mobile/src/components/organization/org-list-error-view.test.ts b/apps/mobile/src/components/organization/org-list-error-view.test.ts new file mode 100644 index 0000000000..0a5e434ff0 --- /dev/null +++ b/apps/mobile/src/components/organization/org-list-error-view.test.ts @@ -0,0 +1,59 @@ +import { describe, expect, it } from 'vitest'; + +import { selectOrgListErrorView } from './org-list-error-view'; + +function makeTrpcError(code: string): unknown { + return { data: { code } }; +} + +describe('selectOrgListErrorView', () => { + it('maps FORBIDDEN to permission with no retry', () => { + expect(selectOrgListErrorView(makeTrpcError('FORBIDDEN'))).toEqual({ + variant: 'permission', + showRetry: false, + }); + }); + + it('maps UNAUTHORIZED to permission with no retry', () => { + expect(selectOrgListErrorView(makeTrpcError('UNAUTHORIZED'))).toEqual({ + variant: 'permission', + showRetry: false, + }); + }); + + it('maps NOT_FOUND to not-found with no retry', () => { + expect(selectOrgListErrorView(makeTrpcError('NOT_FOUND'))).toEqual({ + variant: 'not-found', + showRetry: false, + }); + }); + + it('maps BAD_REQUEST to server with no retry (terminal code)', () => { + expect(selectOrgListErrorView(makeTrpcError('BAD_REQUEST'))).toEqual({ + variant: 'server', + showRetry: false, + }); + }); + + it('maps UNAUTHORIZED-style UNPROCESSABLE_CONTENT to server with no retry', () => { + expect(selectOrgListErrorView(makeTrpcError('UNPROCESSABLE_CONTENT'))).toEqual({ + variant: 'server', + showRetry: false, + }); + }); + + it('maps a 500-class code to server with retry', () => { + expect(selectOrgListErrorView(makeTrpcError('INTERNAL_SERVER_ERROR'))).toEqual({ + variant: 'server', + showRetry: true, + }); + }); + + it('maps an unknown non-tRPC error to server with retry', () => { + expect(selectOrgListErrorView(new Error('network down'))).toEqual({ + variant: 'server', + showRetry: true, + }); + expect(selectOrgListErrorView(null)).toEqual({ variant: 'server', showRetry: true }); + }); +}); diff --git a/apps/mobile/src/components/organization/org-list-error-view.ts b/apps/mobile/src/components/organization/org-list-error-view.ts new file mode 100644 index 0000000000..de88f446d2 --- /dev/null +++ b/apps/mobile/src/components/organization/org-list-error-view.ts @@ -0,0 +1,27 @@ +// Pure error-view selector for the organization Members list. +// +// Unlike the PR Review classifier, this does NOT treat PRECONDITION_FAILED as +// a reconnect state: an org list has no connect gate. The variant comes from +// the tRPC code, and the retry decision is `!isTerminalTrpcCode(code)` — which +// also covers BAD_REQUEST and UNPROCESSABLE_CONTENT, so a Retry is never +// offered on a permanent error. + +import { isTerminalTrpcCode, readTrpcErrorField } from '@/lib/trpc-error'; + +export type OrgListErrorView = { + variant: 'permission' | 'not-found' | 'server'; + showRetry: boolean; +}; + +export function selectOrgListErrorView(error: unknown): OrgListErrorView { + const code = readTrpcErrorField(error, 'code'); + + let variant: 'permission' | 'not-found' | 'server' = 'server'; + if (code === 'FORBIDDEN' || code === 'UNAUTHORIZED') { + variant = 'permission'; + } else if (code === 'NOT_FOUND') { + variant = 'not-found'; + } + + return { variant, showRetry: !isTerminalTrpcCode(code) }; +} diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-file-navigator.test.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-file-navigator.test.tsx new file mode 100644 index 0000000000..3230438c1e --- /dev/null +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-file-navigator.test.tsx @@ -0,0 +1,455 @@ +// File-navigator repair coverage (S4 r1): the three impl-review findings. +// +// 1. A failed later page with no active search shows a retry CTA that +// re-fetches just the failed page (`query.fetchNextPage`). +// 2. The memoized row receives identity-stable `onSelect`/`onToggleViewed` +// callbacks, so a search keystroke does not re-render recycled cells. +// 3. `onEndReached` is gated to "no active search", so during a search only +// fetch-to-completion loads the remaining pages. + +/* eslint-disable max-lines -- cohesive component-test suite for the navigator fetch rules */ +/* eslint-disable typescript-eslint/no-deprecated -- react-test-renderer is the DOM-free renderer used to mount React/RN trees under vitest (same pattern as src/test/render-with-providers.tsx) */ +import { createElement, Fragment, type ReactElement } from 'react'; +import { act } from 'react-test-renderer'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { PrDiffFileNavigator } from '@/components/pr-review/diff/pr-diff-file-navigator'; +import { type PrReviewFile } from '@/lib/pr-review/diff/pr-review-file-types'; +import { renderWithProviders } from '@/test/render-with-providers'; + +// ── Hoisted mocks ────────────────────────────────────────────────────────── + +const fetchNextPageMock = vi.hoisted(() => vi.fn()); +const fetchAllRunMock = vi.hoisted(() => vi.fn()); + +// Records every `NavigatorFileRow` render. Because the navigator wraps the row +// in `memo`, a memo hit (stable callbacks) does NOT push a new entry. +const rowRenders = vi.hoisted( + () => [] as { path: string; onSelect: () => void; onToggleViewed: () => void }[] +); + +// Captures the latest FlashList props so tests can read `onEndReached`. +const flashListProps = vi.hoisted(() => ({ current: null as null | Record })); + +vi.mock('@shopify/flash-list', () => ({ + FlashList: (props: Record) => { + flashListProps.current = props; + const data = (props.data ?? []) as PrReviewFile[]; + const renderItem = props.renderItem as (args: { + item: PrReviewFile; + index: number; + }) => ReactElement; + return createElement( + Fragment, + null, + data.map((item, index) => renderItem({ item, index })) + ); + }, +})); + +vi.mock('expo-router', () => ({ + useRouter: () => ({ canGoBack: () => false, back: vi.fn() }), +})); + +vi.mock('@/components/ui/icons', () => ({ Search: 'Search' })); +vi.mock('@/components/empty-state', () => ({ EmptyState: 'EmptyState' })); +vi.mock('@/components/ui/skeleton', () => ({ Skeleton: 'Skeleton' })); +vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); +vi.mock('@/lib/hooks/use-theme-colors', () => ({ + useThemeColors: () => ({ mutedForeground: '#888888' }), +})); +vi.mock('@/lib/pr-review/file-navigator-bridge', () => ({ + requestScrollToFile: vi.fn(), +})); + +vi.mock('react-native', () => ({ + View: 'View', + Pressable: 'Pressable', + TextInput: 'TextInput', + ActivityIndicator: 'ActivityIndicator', +})); + +vi.mock('@/components/pr-review/diff/pr-diff-navigator-file-row', () => ({ + NavigatorFileRow: (props: { + file: PrReviewFile; + viewed: boolean; + onSelect: () => void; + onToggleViewed: () => void; + }) => { + rowRenders.push({ + path: props.file.path, + onSelect: props.onSelect, + onToggleViewed: props.onToggleViewed, + }); + return null; + }, +})); + +// ── Hook mocks (module-level mutable state, reset per test) ──────────────── + +type ListQueryResult = { + query: { + isLoading: boolean; + isFetching: boolean; + isFetchingNextPage: boolean; + hasNextPage: boolean; + fetchNextPage: () => unknown; + refetch: () => unknown; + }; + files: PrReviewFile[]; + firstPageErrorState: null; + laterPageError: boolean; +}; + +type ViewedResult = { + isViewed: (path: string) => boolean; + toggle: (path: string) => void; + isLoading: boolean; +}; + +type FetchAllResult = { + run: () => unknown; + isRunning: boolean; + loadedFiles: number; + totalFiles: null; + error: unknown; +}; + +let listQueryResult: ListQueryResult = { + query: { + isLoading: false, + isFetching: false, + isFetchingNextPage: false, + hasNextPage: false, + fetchNextPage: fetchNextPageMock, + refetch: vi.fn(), + }, + files: [], + firstPageErrorState: null, + laterPageError: false, +}; +let viewedResult: ViewedResult = { + isViewed: () => false, + toggle: () => undefined, + isLoading: false, +}; +let fetchAllResult: FetchAllResult = { + run: fetchAllRunMock, + isRunning: false, + loadedFiles: 0, + totalFiles: null, + error: null, +}; + +vi.mock('@/lib/pr-review/diff/pr-review-file-list-state', () => ({ + usePrReviewFileListQuery: () => listQueryResult, + usePrReviewViewedFiles: () => viewedResult, + useFetchToCompletion: () => fetchAllResult, +})); + +// ── Helpers ──────────────────────────────────────────────────────────────── + +function makeFile(path: string): PrReviewFile { + return { + path, + previousPath: null, + status: 'modified', + additions: 0, + deletions: 0, + patch: null, + patchMissing: false, + }; +} + +const BASE_PROPS = { + owner: 'octocat', + repo: 'hello-world', + number: 7, + headSha: 'sha', + changedFiles: 1, +}; + +async function mountNavigator() { + const result = await renderWithProviders(createElement(PrDiffFileNavigator, BASE_PROPS)); + return result; +} + +function findSearchInput(renderer: Awaited>['renderer']) { + return renderer.root.findByProps({ accessibilityLabel: 'Filter files by path' }); +} + +function typeSearch( + renderer: Awaited>['renderer'], + text: string +) { + const input = findSearchInput(renderer); + act(() => { + (input.props.onChangeText as (value: string) => void)(text); + }); +} + +function findRetryButton(renderer: Awaited>['renderer']) { + return renderer.root.findByProps({ accessibilityLabel: 'Retry loading more files' }); +} + +// ── Tests ────────────────────────────────────────────────────────────────── + +describe('PrDiffFileNavigator later-page retry (finding 1)', () => { + beforeEach(() => { + fetchNextPageMock.mockReset(); + fetchAllRunMock.mockReset(); + rowRenders.length = 0; + flashListProps.current = null; + listQueryResult = { + query: { + isLoading: false, + isFetching: false, + isFetchingNextPage: false, + hasNextPage: true, + fetchNextPage: fetchNextPageMock, + refetch: vi.fn(), + }, + files: [makeFile('src/a.ts')], + firstPageErrorState: null, + laterPageError: true, + }; + viewedResult = { isViewed: () => false, toggle: vi.fn(() => undefined), isLoading: false }; + fetchAllResult = { + run: fetchAllRunMock, + isRunning: false, + loadedFiles: 0, + totalFiles: null, + error: null, + }; + }); + + it('shows a retry CTA for a failed later page with no active search', async () => { + const { renderer } = await mountNavigator(); + + expect(findRetryButton(renderer)).toBeTruthy(); + }); + + it('retries the failed page via fetchNextPage when the CTA is pressed', async () => { + const { renderer } = await mountNavigator(); + + const button = findRetryButton(renderer); + act(() => { + (button.props.onPress as () => void)(); + }); + + expect(fetchNextPageMock).toHaveBeenCalledTimes(1); + }); + + it('does not show the later-page CTA when a search is active', async () => { + const { renderer } = await mountNavigator(); + + typeSearch(renderer, 'src'); + + expect( + renderer.root.findAllByProps({ accessibilityLabel: 'Retry loading more files' }) + ).toHaveLength(0); + }); +}); + +describe('PrDiffFileNavigator stable row callbacks (finding 2)', () => { + beforeEach(() => { + fetchNextPageMock.mockReset(); + fetchAllRunMock.mockReset(); + rowRenders.length = 0; + flashListProps.current = null; + listQueryResult = { + query: { + isLoading: false, + isFetching: false, + isFetchingNextPage: false, + hasNextPage: false, + fetchNextPage: fetchNextPageMock, + refetch: vi.fn(), + }, + files: [makeFile('src/a.ts')], + firstPageErrorState: null, + laterPageError: false, + }; + viewedResult = { isViewed: () => false, toggle: vi.fn(() => undefined), isLoading: false }; + fetchAllResult = { + run: fetchAllRunMock, + isRunning: false, + loadedFiles: 0, + totalFiles: null, + error: null, + }; + }); + + it('does not re-render the memoized row on a search keystroke', async () => { + const { renderer } = await mountNavigator(); + + expect(rowRenders).toHaveLength(1); + + typeSearch(renderer, 'src'); + + // The file still matches the search, so the row stays mounted. Stable + // callbacks make the memo hit, so the row does not render again. + expect(rowRenders).toHaveLength(1); + }); +}); + +describe('PrDiffFileNavigator onEndReached gating (finding 3)', () => { + beforeEach(() => { + fetchNextPageMock.mockReset(); + fetchAllRunMock.mockReset(); + rowRenders.length = 0; + flashListProps.current = null; + listQueryResult = { + query: { + isLoading: false, + isFetching: false, + isFetchingNextPage: false, + hasNextPage: true, + fetchNextPage: fetchNextPageMock, + refetch: vi.fn(), + }, + files: [makeFile('src/a.ts')], + firstPageErrorState: null, + laterPageError: false, + }; + viewedResult = { isViewed: () => false, toggle: vi.fn(() => undefined), isLoading: false }; + fetchAllResult = { + run: fetchAllRunMock, + isRunning: false, + loadedFiles: 0, + totalFiles: null, + error: null, + }; + }); + + it('loads the next page on end-reached with no active search', async () => { + await mountNavigator(); + + const onEndReached = flashListProps.current?.onEndReached as () => void; + onEndReached(); + + expect(fetchNextPageMock).toHaveBeenCalledTimes(1); + }); + + it('does not load the next page on end-reached during an active search', async () => { + const { renderer } = await mountNavigator(); + + typeSearch(renderer, 'src'); + + const onEndReached = flashListProps.current?.onEndReached as () => void; + onEndReached(); + + expect(fetchNextPageMock).not.toHaveBeenCalled(); + }); +}); + +describe('PrDiffFileNavigator active-search fetch-to-completion (finding 4)', () => { + beforeEach(() => { + fetchNextPageMock.mockReset(); + fetchAllRunMock.mockReset(); + rowRenders.length = 0; + flashListProps.current = null; + listQueryResult = { + query: { + isLoading: false, + isFetching: false, + isFetchingNextPage: false, + hasNextPage: true, + fetchNextPage: fetchNextPageMock, + refetch: vi.fn(), + }, + files: [makeFile('src/a.ts')], + firstPageErrorState: null, + laterPageError: false, + }; + viewedResult = { isViewed: () => false, toggle: vi.fn(() => undefined), isLoading: false }; + fetchAllResult = { + run: fetchAllRunMock, + isRunning: false, + loadedFiles: 0, + totalFiles: null, + error: null, + }; + }); + + it('starts fetch-to-completion when a search becomes active', async () => { + const { renderer } = await mountNavigator(); + + // No active search yet: fetch-to-completion must not have run. + expect(fetchAllRunMock).not.toHaveBeenCalled(); + + typeSearch(renderer, 'src'); + + expect(fetchAllRunMock).toHaveBeenCalledTimes(1); + }); + + it('shows the load-all retry when fetch-to-completion has an error', async () => { + fetchAllResult = { + run: fetchAllRunMock, + isRunning: false, + loadedFiles: 1, + totalFiles: null, + error: new Error('page failed'), + }; + const { renderer } = await mountNavigator(); + + expect( + renderer.root.findByProps({ accessibilityLabel: 'Retry loading all files' }) + ).toBeTruthy(); + }); +}); + +describe('PrDiffFileNavigator stale row callbacks (finding 5)', () => { + beforeEach(() => { + fetchNextPageMock.mockReset(); + fetchAllRunMock.mockReset(); + rowRenders.length = 0; + flashListProps.current = null; + listQueryResult = { + query: { + isLoading: false, + isFetching: false, + isFetchingNextPage: false, + hasNextPage: false, + fetchNextPage: fetchNextPageMock, + refetch: vi.fn(), + }, + files: [makeFile('src/a.ts')], + firstPageErrorState: null, + laterPageError: false, + }; + viewedResult = { isViewed: () => false, toggle: vi.fn(() => undefined), isLoading: false }; + fetchAllResult = { + run: fetchAllRunMock, + isRunning: false, + loadedFiles: 0, + totalFiles: null, + error: null, + }; + }); + + it('calls the latest viewed.toggle after the hook returns a new viewed object', async () => { + const toggleA = vi.fn(() => undefined); + viewedResult = { isViewed: () => false, toggle: toggleA, isLoading: false }; + + const { renderer } = await mountNavigator(); + + // The hook now returns a new `viewed` object (e.g. after a head-SHA change). + const toggleB = vi.fn(() => undefined); + viewedResult = { isViewed: () => false, toggle: toggleB, isLoading: false }; + typeSearch(renderer, 'src'); + + // The cached row callback must call the latest toggle, not the stale one. + const firstRender = rowRenders[0]; + if (!firstRender) { + throw new Error('expected a row render'); + } + const onToggleViewed = firstRender.onToggleViewed; + act(() => { + onToggleViewed(); + }); + + expect(toggleB).toHaveBeenCalledTimes(1); + expect(toggleA).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-file-navigator.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-file-navigator.tsx index fdfb32d5ab..70401a8bbb 100644 --- a/apps/mobile/src/components/pr-review/diff/pr-diff-file-navigator.tsx +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-file-navigator.tsx @@ -7,18 +7,21 @@ // - share `usePrReviewFileListQuery` with the mounted Files tab so // react-query dedupes by key and the navigator and the file list // stay in sync -// - drive `useFetchToCompletion(...).run()` on mount so the full -// listed file set is available for search/jump +// - virtualize the file rows with `FlashList`; with no active search, +// pages load on scroll via `onEndReached` +// - with an active search, drive `useFetchToCompletion(...).run()` so +// the filter still searches the full listed set // - render a search input (uncontrolled per iOS rules: ref + // onChangeText, no `value`) and a list of file rows // - on tap, `requestScrollToFile(...)` and dismiss // - render the four states: loading, retryable (fetch-to-completion // error), empty (0 listed files), happy +import { FlashList } from '@shopify/flash-list'; import { useRouter } from 'expo-router'; import { Search } from '@/components/ui/icons'; -import { useEffect, useMemo, useRef, useState } from 'react'; -import { ActivityIndicator, Pressable, ScrollView, TextInput, View } from 'react-native'; +import { memo, useCallback, useEffect, useMemo, useRef, useState } from 'react'; +import { ActivityIndicator, Pressable, TextInput, View, type ViewStyle } from 'react-native'; import { EmptyState } from '@/components/empty-state'; import { NavigatorFileRow } from '@/components/pr-review/diff/pr-diff-navigator-file-row'; @@ -31,8 +34,21 @@ import { usePrReviewViewedFiles, } from '@/lib/pr-review/diff/pr-review-file-list-state'; import { type PrReviewFile } from '@/lib/pr-review/diff/pr-review-file-types'; +import { filterNavigatorFiles } from '@/lib/pr-review/diff/navigator-file-filter'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; +// Memoized row so recycled cells do not re-render on every keystroke: `file` +// and `viewed` are stable across a search re-render, and the callbacks below +// are `useCallback`-stable. +const MemoNavigatorFileRow = memo(NavigatorFileRow); + +// Matches the previous ScrollView `contentContainerClassName="pb-8 pt-2"`. +const LIST_CONTENT_STYLE: ViewStyle = { paddingBottom: 32, paddingTop: 8 }; + +// Identity-stable per-path row callbacks (the row's `onSelect`/`onToggleViewed` +// take no args, so the path is bound once and cached in a ref map). +type RowCallbacks = { handleSelect: () => void; handleToggleViewed: () => void }; + type PrDiffFileNavigatorProps = { readonly owner: string; readonly repo: string; @@ -43,14 +59,6 @@ type PrDiffFileNavigatorProps = { readonly onDismiss?: () => void; }; -function filterFiles(files: PrReviewFile[], query: string): PrReviewFile[] { - const needle = query.trim().toLowerCase(); - if (needle.length === 0) { - return files; - } - return files.filter(file => file.path.toLowerCase().includes(needle)); -} - function countViewed(files: PrReviewFile[], isViewed: (path: string) => boolean): number { let count = 0; for (const file of files) { @@ -79,7 +87,7 @@ export function PrDiffFileNavigator({ const [searchVersion, setSearchVersion] = useState(0); const inputRef = useRef(null); - const { query, files, firstPageErrorState } = usePrReviewFileListQuery({ + const { query, files, firstPageErrorState, laterPageError } = usePrReviewFileListQuery({ owner, repo, number, @@ -88,21 +96,31 @@ export function PrDiffFileNavigator({ const viewed = usePrReviewViewedFiles({ owner, repo, number }, headSha); const fetchAll = useFetchToCompletion(query, changedFiles); - // Drive the query to completion so search/navigation cover the full listed - // set. `run()` no-ops while the first page is in flight, so re-run it - // reactively once the query becomes eligible (first page settled, more pages - // remain), and stop once complete or after a surfaced error (the user can - // then tap the "Failed to load all files" retry to resume). + const hasActiveSearch = searchRef.current.trim().length > 0; + + // Rule 2: an active search drives fetch-to-completion so the filter still + // searches the full listed set. `run()` no-ops while the first page is in + // flight, so re-run it reactively once the query becomes eligible (first + // page settled, more pages remain), and stop once complete or after a + // surfaced error (the user can then tap the "Failed to load all files" + // retry to resume). With no active search, pages load on scroll via + // `onEndReached` instead. const runRef = useRef(fetchAll.run); runRef.current = fetchAll.run; useEffect(() => { - if (!query.isFetching && query.hasNextPage && !fetchAll.isRunning && !fetchAll.error) { + if ( + hasActiveSearch && + !query.isFetching && + query.hasNextPage && + !fetchAll.isRunning && + !fetchAll.error + ) { void runRef.current(); } - }, [query.isFetching, query.hasNextPage, fetchAll.isRunning, fetchAll.error]); + }, [hasActiveSearch, query.isFetching, query.hasNextPage, fetchAll.isRunning, fetchAll.error]); const filtered = useMemo( - () => filterFiles(files, searchRef.current), + () => filterNavigatorFiles(files, searchRef.current), // `searchVersion` is the only thing that signals "the ref changed", // so it has to be in the dep list even though `files` is the only // real data input. @@ -112,16 +130,68 @@ export function PrDiffFileNavigator({ const viewedCount = useMemo(() => countViewed(files, viewed.isViewed), [files, viewed]); - const handleSelectFile = (path: string) => { - requestScrollToFile({ owner, repo, number, path }); - if (onDismiss) { - onDismiss(); - return; - } - if (router.canGoBack()) { - router.back(); + const handleSelectFile = useCallback( + (path: string) => { + requestScrollToFile({ owner, repo, number, path }); + if (onDismiss) { + onDismiss(); + return; + } + if (router.canGoBack()) { + router.back(); + } + }, + [owner, repo, number, onDismiss, router] + ); + + const handleToggleViewed = useCallback( + (path: string) => { + void viewed.toggle(path); + }, + [viewed] + ); + + // Cache per-path callbacks so `MemoNavigatorFileRow`'s memo hits across a + // search re-render. The closures read the latest handlers through refs, so + // they stay identity-stable (memo keeps hitting) but never go stale when + // `handleSelectFile` / `handleToggleViewed` change identity (e.g. a head-SHA + // change swaps `viewed.toggle`). + const handleSelectFileRef = useRef(handleSelectFile); + handleSelectFileRef.current = handleSelectFile; + const handleToggleViewedRef = useRef(handleToggleViewed); + handleToggleViewedRef.current = handleToggleViewed; + + const rowCallbacksRef = useRef(new Map()); + const getRowCallbacks = useCallback((path: string) => { + let callbacks = rowCallbacksRef.current.get(path); + if (!callbacks) { + callbacks = { + handleSelect: () => { + handleSelectFileRef.current(path); + }, + handleToggleViewed: () => { + handleToggleViewedRef.current(path); + }, + }; + rowCallbacksRef.current.set(path, callbacks); } - }; + return callbacks; + }, []); + + const renderItem = useCallback( + ({ item }: { item: PrReviewFile }) => { + const callbacks = getRowCallbacks(item.path); + return ( + + ); + }, + [viewed, getRowCallbacks] + ); if (firstPageErrorState?.kind === 'not-found') { return ( @@ -214,6 +284,13 @@ export function PrDiffFileNavigator({ } const showLoadAllRetry = Boolean(fetchAll.error) && !fetchAll.isRunning && query.hasNextPage; + // A scroll-triggered later-page failure (no search) retries just that page. + // `!fetchAll.error` keeps it mutually exclusive with the load-all banner. + const showLaterPageRetry = laterPageError && !hasActiveSearch && !fetchAll.error; + const showRetry = showLoadAllRetry || showLaterPageRetry; + const retryMessage = showLoadAllRetry ? 'Failed to load all files' : "Couldn't load more files"; + const retryLabel = showLoadAllRetry ? 'Retry loading all files' : 'Retry loading more files'; + const retryAction = showLoadAllRetry ? fetchAll.run : query.fetchNextPage; // Truncated when pagination hasn't finished, errored, or GitHub's 3,000-file // listing cap left fewer listed files than the overview's changed-file count. const isTruncated = query.hasNextPage || Boolean(fetchAll.error) || changedFiles > files.length; @@ -255,49 +332,42 @@ export function PrDiffFileNavigator({ ) : null} - {showLoadAllRetry ? ( + {showRetry ? ( - Failed to load all files + {retryMessage} { - void fetchAll.run(); - }} + onPress={() => void retryAction()} className="rounded-md border border-border bg-card px-3 py-1 active:opacity-70" accessibilityRole="button" - accessibilityLabel="Retry loading all files" + accessibilityLabel={retryLabel} > Retry ) : null} - file.path} keyboardShouldPersistTaps="handled" automaticallyAdjustKeyboardInsets - > - {filtered.length === 0 ? ( + contentContainerStyle={LIST_CONTENT_STYLE} + onEndReached={() => { + // During a search, fetch-to-completion loads pages; a scroll fetch would race it. + if (!hasActiveSearch && query.hasNextPage && !query.isFetchingNextPage) { + void query.fetchNextPage(); + } + }} + onEndReachedThreshold={0.5} + ListEmptyComponent={ No files match "{searchRef.current}" - ) : null} - {filtered.map(file => ( - { - handleSelectFile(file.path); - }} - onToggleViewed={() => { - void viewed.toggle(file.path); - }} - /> - ))} - + } + /> ); } diff --git a/apps/mobile/src/components/pr-review/pr-review-discussion-tab-view.test.ts b/apps/mobile/src/components/pr-review/pr-review-discussion-tab-view.test.ts new file mode 100644 index 0000000000..d25d35521a --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-discussion-tab-view.test.ts @@ -0,0 +1,63 @@ +import { describe, expect, it } from 'vitest'; + +import { selectDiscussionTabView } from './pr-review-discussion-tab-view'; + +const base = { + firstPageErrorState: null, + isPending: false, + isEmpty: false, +}; + +describe('selectDiscussionTabView', () => { + it('returns permission for a permission first-page error', () => { + expect( + selectDiscussionTabView({ ...base, firstPageErrorState: { kind: 'permission' } }) + ).toEqual({ kind: 'permission' }); + }); + + it('returns not-found for a not-found first-page error', () => { + expect( + selectDiscussionTabView({ ...base, firstPageErrorState: { kind: 'not-found' } }) + ).toEqual({ kind: 'not-found' }); + }); + + it('returns reconnect for a reconnect first-page error', () => { + expect( + selectDiscussionTabView({ ...base, firstPageErrorState: { kind: 'reconnect' } }) + ).toEqual({ kind: 'reconnect' }); + }); + + it('returns retryable for a retryable first-page error', () => { + expect( + selectDiscussionTabView({ ...base, firstPageErrorState: { kind: 'retryable' } }) + ).toEqual({ kind: 'retryable' }); + }); + + it('returns loading while the first page is pending', () => { + expect(selectDiscussionTabView({ ...base, isPending: true })).toEqual({ kind: 'loading' }); + }); + + it('returns empty when there is no error, no pending, and no items', () => { + expect(selectDiscussionTabView({ ...base, isEmpty: true })).toEqual({ kind: 'empty' }); + }); + + it('returns happy when there is no error, no pending, and items exist', () => { + expect(selectDiscussionTabView(base)).toEqual({ kind: 'happy' }); + }); + + it('prioritizes the error state over pending and empty', () => { + expect( + selectDiscussionTabView({ + firstPageErrorState: { kind: 'permission' }, + isPending: true, + isEmpty: true, + }) + ).toEqual({ kind: 'permission' }); + }); + + it('prioritizes pending over empty', () => { + expect(selectDiscussionTabView({ ...base, isPending: true, isEmpty: true })).toEqual({ + kind: 'loading', + }); + }); +}); diff --git a/apps/mobile/src/components/pr-review/pr-review-discussion-tab-view.ts b/apps/mobile/src/components/pr-review/pr-review-discussion-tab-view.ts new file mode 100644 index 0000000000..326b006f9c --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-discussion-tab-view.ts @@ -0,0 +1,28 @@ +// Pure state selector for the PR Review Discussion tab. +// +// Extracts the tab's existing branch chain (unchanged in behaviour) so the +// seven outcomes can be unit-tested. The tab renders exactly what it rendered +// before; this module only owns the decision. + +export type DiscussionTabView = { + kind: 'permission' | 'not-found' | 'reconnect' | 'retryable' | 'loading' | 'empty' | 'happy'; +}; + +export function selectDiscussionTabView(args: { + firstPageErrorState: { kind: 'permission' | 'not-found' | 'reconnect' | 'retryable' } | null; + isPending: boolean; + isEmpty: boolean; +}): DiscussionTabView { + const { firstPageErrorState, isPending, isEmpty } = args; + + if (firstPageErrorState) { + return { kind: firstPageErrorState.kind }; + } + if (isPending) { + return { kind: 'loading' }; + } + if (isEmpty) { + return { kind: 'empty' }; + } + return { kind: 'happy' }; +} diff --git a/apps/mobile/src/components/pr-review/pr-review-discussion-tab.tsx b/apps/mobile/src/components/pr-review/pr-review-discussion-tab.tsx index 1beacfe796..c024270bf1 100644 --- a/apps/mobile/src/components/pr-review/pr-review-discussion-tab.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-discussion-tab.tsx @@ -68,6 +68,7 @@ import { toggleThreadExpanded, } from '@/lib/pr-review/discussion/thread-expansion'; import { usePrReviewDiscussionThreads } from '@/lib/pr-review/discussion/use-pr-review-discussion-threads'; +import { selectDiscussionTabView } from '@/components/pr-review/pr-review-discussion-tab-view'; type PrReviewDiscussionTabProps = { readonly owner: string; @@ -200,33 +201,38 @@ export function PrReviewDiscussionTab({ }; // ── First-page error / terminal states ───────────────────────────── - if (firstPageErrorState) { - if (firstPageErrorState.kind === 'permission') { - return ( - - ); - } - if (firstPageErrorState.kind === 'not-found') { - return ( - - ); - } - if (firstPageErrorState.kind === 'reconnect') { - return ( - - - - ); - } - // retryable + const view = selectDiscussionTabView({ + firstPageErrorState, + isPending: query.isPending, + isEmpty: isDiscussionEmpty(threads, conversation), + }); + + if (view.kind === 'permission') { + return ( + + ); + } + if (view.kind === 'not-found') { + return ( + + ); + } + if (view.kind === 'reconnect') { + return ( + + + + ); + } + if (view.kind === 'retryable') { return ( {Array.from({ length: SKELETON_ROW_COUNT }).map((_, index) => ( @@ -258,7 +264,7 @@ export function PrReviewDiscussionTab({ } // ── Empty (neither threads nor conversation comments) ────────────── - if (isDiscussionEmpty(threads, conversation)) { + if (view.kind === 'empty') { return ( 0); }; - const handleSubmit = async () => { + const handleSubmit = () => { const raw = inputValueRef.current; const parsed = parseGitHubPrUrl(raw.trim()); if (!parsed) { announcingToast.error(PR_LINK_TOAST_INVALID_COPY); return; } - // Title is backfilled on first successful load (S5). - await upsertRecentPr({ - owner: parsed.owner, - repo: parsed.repo, - number: parsed.number, - title: '', - lastOpenedAt: Date.now(), - }); + // Navigate straight to the PR route. Recents are written only after an + // authorized payload (the PR screen's backfill effect), so a failed or + // unauthorized open never persists an entry. router.push(getPrReviewPath(parsed.owner, parsed.repo, parsed.number)); }; @@ -103,21 +93,36 @@ export function PrReviewEntryScreen() { announcingToast.error(PR_LINK_TOAST_INVALID_COPY); return; } - await handleSubmit(); + handleSubmit(); }; const focusInput = () => { inputRef.current?.focus(); }; - const handleRecentPress = async (entry: RecentPr) => { - await upsertRecentPr({ - ...entry, - lastOpenedAt: Date.now(), - }); + const handleRecentPress = (entry: RecentPr) => { + // Navigate only. The PR screen's backfill effect updates `lastOpenedAt` + // (and `lastResult`) once an authorized payload loads. router.push(getPrReviewPath(entry.owner, entry.repo, entry.number)); }; + const handleRemoveRecent = (entry: RecentPr) => { + Alert.alert('Remove from recents?', 'This pull request will be removed from your recents.', [ + { text: 'Cancel', style: 'cancel' }, + { + text: 'Remove', + style: 'destructive', + onPress: () => { + void (async () => { + await removeRecentPr(entry); + const list = await getRecentPrs(); + setRecent(list); + })(); + }, + }, + ]); + }; + const showClearButton = selectPrLinkClearButtonVisible({ hasInput }); let recentsBody: ReactNode = null; @@ -142,148 +147,163 @@ export function PrReviewEntryScreen() { {recent.map((entry, index) => { const isLast = index === recent.length - 1; + const rowState = selectRecentPrRowState(entry); + const removeLabel = `Remove ${entry.owner}/${entry.repo}#${entry.number} from recents`; return ( - { - void handleRecentPress(entry); - }} - className={`flex-row items-center gap-3 px-3 py-3 active:opacity-70 ${ - isLast ? '' : 'border-b-[0.5px] border-hair-soft' - }`} + className={isLast ? '' : 'border-b-[0.5px] border-hair-soft'} > - - - {entry.title || `${entry.owner}/${entry.repo}#${entry.number}`} - - - {entry.owner}/{entry.repo}#{entry.number} - + { + handleRecentPress(entry); + }} + className="flex-row items-center gap-3 px-3 py-3 active:opacity-70" + > + + + {rowState.primary} + + {rowState.secondary ? ( + + {rowState.secondary} + + ) : null} + + + + + {rowState.failed ? ( + + Couldn't load + + ) : ( + + )} + + {rowState.failed ? ( + + ) : null} + + - - + ); })} ); } - return ( - - - - - - - - Paste a PR link - - - - - - { - // Don't setState on every keystroke; track only whether the - // input has any text. The raw value lives in the ref so - // handleSubmit reads the latest text without re-rendering. - const decision = consumePrLinkInputEcho( - pendingProgrammaticTextsRef.current, - value - ); - pendingProgrammaticTextsRef.current = [...decision.pending]; - if (decision.kind === 'echo') { - // Echo of setNativeProps: inputValueRef already holds the - // intentional value from applyFieldText — do not clobber it - // with a delayed/stale echo. - return; - } - inputValueRef.current = value; - setHasInput(value.length > 0); - }} - // leading-[normal] so no lineHeight reaches the style: an explicit lineHeight - // makes iOS draw the placeholder lower than the typed text (see AGENTS.md). - className="min-w-0 flex-1 bg-transparent py-3 pl-3 pr-1 text-base text-foreground leading-[normal]" - accessibilityLabel="GitHub pull request URL" - returnKeyType="go" - onSubmitEditing={() => { - void handleSubmit(); - }} - /> - {showClearButton ? ( - // h-13 w-13 measures 45×45pt on device; h-12 is 42pt and h-11 is - // 38pt in this app — do not "simplify" back to h-11/w-11. - { - // clear() is the iOS-safe native empty after real typing. - // setNativeProps({ text: '' }) loses the most-recent-event-count - // race and leaves the typed text visible while React state - // thinks the field is empty. Do not route through - // applyFieldText('') (paste-only path) and do not push an - // echo for '' — a non-arriving echo would stale the FIFO. - inputValueRef.current = ''; - setHasInput(false); - inputRef.current?.clear(); - inputRef.current?.focus(); - }} - accessibilityRole="button" - accessibilityLabel="Clear pull request link" - className="h-13 w-13 items-center justify-center active:opacity-70" - > - - - ) : null} - + const pasteBlock = ( + + + + + Paste a PR link + + + + + + { + // Don't setState on every keystroke; track only whether the + // input has any text. The raw value lives in the ref so + // handleSubmit reads the latest text without re-rendering. + const decision = consumePrLinkInputEcho(pendingProgrammaticTextsRef.current, value); + pendingProgrammaticTextsRef.current = [...decision.pending]; + if (decision.kind === 'echo') { + // Echo of setNativeProps: inputValueRef already holds the + // intentional value from applyFieldText — do not clobber it + // with a delayed/stale echo. + return; + } + inputValueRef.current = value; + setHasInput(value.length > 0); + }} + // leading-[normal] so no lineHeight reaches the style: an explicit lineHeight + // makes iOS draw the placeholder lower than the typed text (see AGENTS.md). + className="min-w-0 flex-1 bg-transparent py-3 pl-3 pr-1 text-base text-foreground leading-[normal]" + accessibilityLabel="GitHub pull request URL" + returnKeyType="go" + onSubmitEditing={handleSubmit} + /> + {showClearButton ? ( + // h-13 w-13 measures 45×45pt on device; h-12 is 42pt and h-11 is + // 38pt in this app — do not "simplify" back to h-11/w-11. { - void handlePaste(); + // clear() is the iOS-safe native empty after real typing. + // setNativeProps({ text: '' }) loses the most-recent-event-count + // race and leaves the typed text visible while React state + // thinks the field is empty. Do not route through + // applyFieldText('') (paste-only path) and do not push an + // echo for '' — a non-arriving echo would stale the FIFO. + inputValueRef.current = ''; + setHasInput(false); + inputRef.current?.clear(); + inputRef.current?.focus(); }} accessibilityRole="button" - accessibilityLabel="Paste pull request link" - hitSlop={4} - className="h-11 w-11 items-center justify-center rounded-md border border-border bg-card active:opacity-70" + accessibilityLabel="Clear pull request link" + className="h-13 w-13 items-center justify-center active:opacity-70" > - + - - + ) : null} + { + void handlePaste(); + }} + accessibilityRole="button" + accessibilityLabel="Paste pull request link" + hitSlop={4} + className="h-11 w-11 items-center justify-center rounded-md border border-border bg-card active:opacity-70" + > + + + + + + ); - - - - - Recent - - - {recentsBody} - - + return ( + + + ); } diff --git a/apps/mobile/src/components/pr-review/pr-review-inbox-list.tsx b/apps/mobile/src/components/pr-review/pr-review-inbox-list.tsx new file mode 100644 index 0000000000..53fbd23c91 --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-inbox-list.tsx @@ -0,0 +1,224 @@ +// PR inbox list: the entry screen's single scroll container. +// +// The screen keeps its ScreenHeader and the paste-a-link + recents +// state; this component owns the ONE FlashList that composes the whole +// body. The paste block and recents are passed in and rendered in the +// list header/footer so they stay mounted in every inbox state (E4/E5 +// depend on reaching Recents right after a failed open). Inbox states +// render inside `ListEmptyComponent` — never as a replacement for the +// screen. + +import { FlashList } from '@shopify/flash-list'; +import { useRouter } from 'expo-router'; +import { type ReactNode } from 'react'; +import { Pressable, View } from 'react-native'; + +import { EmptyState } from '@/components/empty-state'; +import { QueryError } from '@/components/query-error'; +import { PrReviewReconnectNotice } from '@/components/pr-review/pr-review-reconnect-notice'; +import { type PrInboxView, selectPrInboxView } from '@/components/pr-review/pr-review-inbox-view'; +import { Button } from '@/components/ui/button'; +import { ChevronRight, Clock, GitPullRequest, Inbox } from '@/components/ui/icons'; +import { Skeleton } from '@/components/ui/skeleton'; +import { Text } from '@/components/ui/text'; +import { useThemeColors } from '@/lib/hooks/use-theme-colors'; +import { getPrReviewPath } from '@/lib/profile-agent-navigation'; +import { usePrInbox } from '@/lib/pr-review/use-pr-inbox'; +import { parseTimestamp, timeAgo } from '@/lib/utils'; + +const SKELETON_ROW_COUNT = 5; + +type InboxItem = ReturnType['items'][number]; + +type PrReviewInboxListProps = { + /** The "Paste a PR link" block, rendered above the Inbox eyebrow. */ + header: ReactNode; + /** The recents body, rendered below the pagination footer. */ + recents: ReactNode; +}; + +export function PrReviewInboxList({ header, recents }: Readonly) { + const { query, items, firstPageErrorState, laterPageError } = usePrInbox(true); + const view = selectPrInboxView({ + isLoading: query.isPending, + itemCount: items.length, + firstPageErrorState, + laterPageError, + }); + + return ( + `${item.owner}/${item.repo}#${item.number}`} + renderItem={({ item }) => } + ListHeaderComponent={ + + {header} + + + } + ListEmptyComponent={ + { + void query.refetch(); + }} + isRetrying={query.isFetching} + /> + } + ListFooterComponent={ + + {view.showLoadMoreRetry ? ( + { + void query.fetchNextPage(); + }} + /> + ) : null} + + {recents} + + } + onEndReached={() => { + if (query.hasNextPage && !query.isFetchingNextPage) { + void query.fetchNextPage(); + } + }} + onEndReachedThreshold={0.5} + keyboardShouldPersistTaps="handled" + automaticallyAdjustKeyboardInsets + /> + ); +} + +function InboxEyebrow() { + const colors = useThemeColors(); + return ( + + + + Inbox + + + ); +} + +function RecentEyebrow() { + const colors = useThemeColors(); + return ( + + + + Recent + + + ); +} + +function InboxRow({ item }: Readonly<{ item: InboxItem }>) { + const router = useRouter(); + const colors = useThemeColors(); + const updatedLabel = timeAgo(parseTimestamp(item.updatedAt)); + + return ( + { + router.push(getPrReviewPath(item.owner, item.repo, item.number)); + }} + accessibilityRole="button" + accessibilityLabel={`${item.owner}/${item.repo}#${item.number}`} + className="flex-row items-center gap-3 border-b-[0.5px] border-hair-soft px-6 py-3 active:opacity-70" + > + + + {item.title} + + + + {item.owner}/{item.repo}#{item.number} · {updatedLabel} + + {item.isDraft ? ( + + + Draft + + + ) : null} + + + + + ); +} + +function InboxEmpty({ + view, + onRetry, + isRetrying, +}: Readonly<{ view: PrInboxView; onRetry: () => void; isRetrying: boolean }>) { + if (view.kind === 'loading') { + return ( + + {Array.from({ length: SKELETON_ROW_COUNT }).map((_, index) => ( + // eslint-disable-next-line react/no-array-index-key -- skeleton placeholders have no stable id + + + + + ))} + + ); + } + + if (view.kind === 'empty') { + return ( + + ); + } + + if (view.kind === 'permission') { + return ; + } + + if (view.kind === 'not-found') { + return ; + } + + if (view.kind === 'reconnect') { + return ( + + + + ); + } + + // retryable + return ( + + ); +} + +function LoadMoreRetry({ onRetry }: Readonly<{ onRetry: () => void }>) { + return ( + + + Couldn't load more + + + + ); +} diff --git a/apps/mobile/src/components/pr-review/pr-review-inbox-view.test.ts b/apps/mobile/src/components/pr-review/pr-review-inbox-view.test.ts new file mode 100644 index 0000000000..0367fe7b0e --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-inbox-view.test.ts @@ -0,0 +1,108 @@ +import { describe, expect, it } from 'vitest'; + +import { selectPrInboxView } from './pr-review-inbox-view'; + +function state(kind: 'retryable' | 'permission' | 'not-found' | 'reconnect') { + return { kind }; +} + +describe('selectPrInboxView', () => { + it('selects loading when the first page is still in flight', () => { + expect( + selectPrInboxView({ + isLoading: true, + itemCount: 0, + firstPageErrorState: null, + laterPageError: false, + }) + ).toEqual({ kind: 'loading', showLoadMoreRetry: false }); + }); + + it('selects happy when rows are loaded', () => { + expect( + selectPrInboxView({ + isLoading: false, + itemCount: 3, + firstPageErrorState: null, + laterPageError: false, + }) + ).toEqual({ kind: 'happy', showLoadMoreRetry: false }); + }); + + it('selects empty when no rows and no error', () => { + expect( + selectPrInboxView({ + isLoading: false, + itemCount: 0, + firstPageErrorState: null, + laterPageError: false, + }) + ).toEqual({ kind: 'empty', showLoadMoreRetry: false }); + }); + + it('selects retryable for a transient first-page error', () => { + expect( + selectPrInboxView({ + isLoading: false, + itemCount: 0, + firstPageErrorState: state('retryable'), + laterPageError: false, + }) + ).toEqual({ kind: 'retryable', showLoadMoreRetry: false }); + }); + + it('selects permission for a FORBIDDEN first-page error', () => { + expect( + selectPrInboxView({ + isLoading: false, + itemCount: 0, + firstPageErrorState: state('permission'), + laterPageError: false, + }) + ).toEqual({ kind: 'permission', showLoadMoreRetry: false }); + }); + + it('selects not-found for a NOT_FOUND first-page error', () => { + expect( + selectPrInboxView({ + isLoading: false, + itemCount: 0, + firstPageErrorState: state('not-found'), + laterPageError: false, + }) + ).toEqual({ kind: 'not-found', showLoadMoreRetry: false }); + }); + + it('selects reconnect for a PRECONDITION_FAILED first-page error', () => { + expect( + selectPrInboxView({ + isLoading: false, + itemCount: 0, + firstPageErrorState: state('reconnect'), + laterPageError: false, + }) + ).toEqual({ kind: 'reconnect', showLoadMoreRetry: false }); + }); + + it('flags the load-more retry row only on a later-page failure with rows loaded', () => { + expect( + selectPrInboxView({ + isLoading: false, + itemCount: 5, + firstPageErrorState: null, + laterPageError: true, + }) + ).toEqual({ kind: 'happy', showLoadMoreRetry: true }); + }); + + it('never flags the load-more retry row outside the happy state', () => { + expect( + selectPrInboxView({ + isLoading: true, + itemCount: 0, + firstPageErrorState: null, + laterPageError: true, + }) + ).toEqual({ kind: 'loading', showLoadMoreRetry: false }); + }); +}); diff --git a/apps/mobile/src/components/pr-review/pr-review-inbox-view.ts b/apps/mobile/src/components/pr-review/pr-review-inbox-view.ts new file mode 100644 index 0000000000..49a34bbfab --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-inbox-view.ts @@ -0,0 +1,46 @@ +// Pure state selection for the PR inbox list. Kept out of JSX so every +// branch is unit-tested. The inbox has the same four-state matrix as the +// rest of the PR Review surface, plus a later-page failure that keeps +// already-loaded rows and offers an inline retry. + +import { type classifyPrReviewQueryState } from '@/lib/pr-review/classify-pr-review-query-state'; + +type PrReviewQueryState = ReturnType; + +type PrInboxViewKind = + | 'loading' + | 'happy' + | 'empty' + | 'retryable' + | 'permission' + | 'not-found' + | 'reconnect'; + +export type PrInboxView = { + kind: PrInboxViewKind; + /** Show the inline "Couldn't load more" + Retry footer row. */ + showLoadMoreRetry: boolean; +}; + +export function selectPrInboxView(args: { + isLoading: boolean; + itemCount: number; + firstPageErrorState: PrReviewQueryState | null; + laterPageError: boolean; +}): PrInboxView { + const { isLoading, itemCount, firstPageErrorState, laterPageError } = args; + + if (firstPageErrorState) { + return { kind: firstPageErrorState.kind, showLoadMoreRetry: false }; + } + + if (isLoading) { + return { kind: 'loading', showLoadMoreRetry: false }; + } + + if (itemCount === 0) { + return { kind: 'empty', showLoadMoreRetry: false }; + } + + return { kind: 'happy', showLoadMoreRetry: laterPageError }; +} diff --git a/apps/mobile/src/components/pr-review/pr-review-pending-comment-row.tsx b/apps/mobile/src/components/pr-review/pr-review-pending-comment-row.tsx index 1a820802a3..95461a347f 100644 --- a/apps/mobile/src/components/pr-review/pr-review-pending-comment-row.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-pending-comment-row.tsx @@ -18,6 +18,8 @@ type PrReviewPendingCommentRowProps = Readonly<{ onPress: () => void; onDelete: () => void; disabled?: boolean; + /** True when the comment was written against an older commit than the PR head. */ + stale?: boolean; }>; export function PrReviewPendingCommentRow({ @@ -25,6 +27,7 @@ export function PrReviewPendingCommentRow({ onPress, onDelete, disabled = false, + stale = false, }: PrReviewPendingCommentRowProps) { const colors = useThemeColors(); const location = pendingCommentLocationLabel(item); @@ -35,7 +38,11 @@ export function PrReviewPendingCommentRow({ onPress={onPress} disabled={disabled} accessibilityRole="button" - accessibilityLabel={`Edit pending comment on ${location}`} + accessibilityLabel={ + stale + ? `Edit outdated pending comment on ${location}` + : `Edit pending comment on ${location}` + } className="min-h-9 flex-1 gap-0.5 active:opacity-70" > @@ -44,6 +51,18 @@ export function PrReviewPendingCommentRow({ {item.body.trim().length > 0 ? item.body : '(empty)'} + {stale ? ( + + + + Outdated + + + + the PR moved since you wrote this. + + + ) : null} 0) { message = - 'Some comments may be outdated because the PR head changed after they were queued. Submission will use the current head.'; + 'Comments written against an older commit are not sent. They stay in your queue so you can edit or delete them.'; } else { message = 'All comments will be sent in a single batched request.'; } diff --git a/apps/mobile/src/components/pr-review/pr-review-screen.tsx b/apps/mobile/src/components/pr-review/pr-review-screen.tsx index 253d792d27..d060aabe43 100644 --- a/apps/mobile/src/components/pr-review/pr-review-screen.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-screen.tsx @@ -1,7 +1,7 @@ import { useQuery, useQueryClient } from '@tanstack/react-query'; import { type Href, useFocusEffect, useRouter } from 'expo-router'; import { Check, Share as ShareIcon } from '@/components/ui/icons'; -import { type ReactNode, useCallback, useEffect, useState } from 'react'; +import { type ReactNode, useCallback, useEffect, useRef, useState } from 'react'; import { Pressable, RefreshControl, ScrollView, Share, View } from 'react-native'; import { PrMergePartialSuccessBanner } from '@/components/pr-review/merge/pr-merge-partial-success-banner'; @@ -17,7 +17,7 @@ import { Button } from '@/components/ui/button'; import { Text } from '@/components/ui/text'; import { consumeMergePartialSuccess } from '@/lib/pr-review/merge/merge-result-banner-store'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; -import { upsertRecentPr } from '@/lib/pr-review/recent-prs'; +import { markRecentPrFailed, upsertRecentPr } from '@/lib/pr-review/recent-prs'; import { useTRPC } from '@/lib/trpc'; import { cn } from '@/lib/utils'; @@ -84,9 +84,10 @@ export function PrReviewScreen({ owner, repo, number }: PrReviewScreenProps) { // a single network round-trip even though both components subscribe. const pr = useQuery(trpc.githubPrReview.getPullRequest.queryOptions({ owner, repo, number })); - // Recents title backfill. S4b left the title empty so the recents row - // can be written before the PR loads. Once we have the real title, - // upsert it so the recents list shows it next time. + // Recents backfill. This is the ONLY writer that creates an entry: a + // successful load upserts the real title with `lastResult: 'ok'`, which + // also clears any previous `'failed'` marker. A never-authorized PR + // (no successful load) never gets an entry. useEffect(() => { const data = pr.data; if (!data?.title) { @@ -98,9 +99,28 @@ export function PrReviewScreen({ owner, repo, number }: PrReviewScreenProps) { number, title: data.title, lastOpenedAt: Date.now(), + lastResult: 'ok', }); }, [pr.data, owner, repo, number]); + // Mark an existing recents entry as failed exactly once per error. The + // ref guards against re-writing on re-render; `markRecentPrFailed` is a + // no-op when no entry exists, so a never-authorized PR stays out of + // recents. A success (isError false) or a PR identity change resets the + // guard so a later error marks the entry failed again. + const markedFailedRef = useRef(false); + useEffect(() => { + if (!pr.isError) { + markedFailedRef.current = false; + return; + } + if (markedFailedRef.current) { + return; + } + markedFailedRef.current = true; + void markRecentPrFailed({ owner, repo, number }); + }, [pr.isError, owner, repo, number]); + // Share the PR's public GitHub URL via the native share sheet. The URL comes // from the route params, so this works before the PR query resolves; the title // is added once it is known. Fire-and-forget, like the invite-link share in diff --git a/apps/mobile/src/components/pr-review/pr-review-submit-view.test.ts b/apps/mobile/src/components/pr-review/pr-review-submit-view.test.ts new file mode 100644 index 0000000000..0e9a6893a0 --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-submit-view.test.ts @@ -0,0 +1,38 @@ +import { describe, expect, it } from 'vitest'; + +import { + selectPartialSubmitMessage, + selectSubmitCtaLabel, +} from '@/components/pr-review/pr-review-submit-view'; + +describe('selectSubmitCtaLabel', () => { + it('uses the plain label when nothing is stale', () => { + expect(selectSubmitCtaLabel({ freshCount: 3, totalCount: 3 })).toBe('Submit review'); + }); + + it('uses the partial label when some items are stale', () => { + expect(selectSubmitCtaLabel({ freshCount: 2, totalCount: 5 })).toBe('Submit 2 of 5 comments'); + }); + + it('uses the partial label even when no item is fresh', () => { + expect(selectSubmitCtaLabel({ freshCount: 0, totalCount: 4 })).toBe('Submit 0 of 4 comments'); + }); +}); + +describe('selectPartialSubmitMessage', () => { + it('returns null when nothing is stale', () => { + expect(selectPartialSubmitMessage({ freshCount: 3, staleCount: 0 })).toBeNull(); + }); + + it('reports the posted and kept counts when some items are stale', () => { + expect(selectPartialSubmitMessage({ freshCount: 2, staleCount: 3 })).toBe( + 'Posted 2 comment(s). 3 comment(s) point at an older commit and stayed in your queue.' + ); + }); + + it('reports zero posted comments when every item is stale', () => { + expect(selectPartialSubmitMessage({ freshCount: 0, staleCount: 4 })).toBe( + 'Posted 0 comment(s). 4 comment(s) point at an older commit and stayed in your queue.' + ); + }); +}); diff --git a/apps/mobile/src/components/pr-review/pr-review-submit-view.ts b/apps/mobile/src/components/pr-review/pr-review-submit-view.ts new file mode 100644 index 0000000000..33ed2e2712 --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-submit-view.ts @@ -0,0 +1,22 @@ +/** + * Pure copy selectors for the review-submit sheet's stale-head path. Kept + * out of the component so the submit CTA label and the partial-result + * message are unit-testable without mounting the sheet. + */ + +export function selectSubmitCtaLabel(args: { freshCount: number; totalCount: number }): string { + if (args.totalCount > args.freshCount) { + return `Submit ${args.freshCount} of ${args.totalCount} comments`; + } + return 'Submit review'; +} + +export function selectPartialSubmitMessage(args: { + freshCount: number; + staleCount: number; +}): string | null { + if (args.staleCount === 0) { + return null; + } + return `Posted ${args.freshCount} comment(s). ${args.staleCount} comment(s) point at an older commit and stayed in your queue.`; +} diff --git a/apps/mobile/src/components/pr-review/pr-review-submit.test.tsx b/apps/mobile/src/components/pr-review/pr-review-submit.test.tsx new file mode 100644 index 0000000000..2e599d130f --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-submit.test.tsx @@ -0,0 +1,215 @@ +/* eslint-disable typescript-eslint/no-deprecated -- react-test-renderer is the DOM-free renderer used to mount React/RN trees under vitest (node env, no jsdom); see src/lib/pr-review/pending-review-provider.mounted.test.tsx */ +/* eslint-disable require-await, @typescript-eslint/require-await -- the fake mutation and drafts factories settle without await because they resolve immediately */ +import TestRenderer, { act } from 'react-test-renderer'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { PrReviewSubmit } from './pr-review-submit'; +import { + type PendingReviewItem, + PendingReviewProvider, + usePendingReview, +} from '@/lib/pr-review/pending-review-provider'; + +const submitMutationMock = vi.hoisted(() => ({ + mutateAsync: vi.fn(async (): Promise => undefined), +})); + +vi.mock('@/lib/pr-review/use-pr-review-mutations', () => ({ + useSubmitReviewMutation: () => ({ + mutateAsync: submitMutationMock.mutateAsync, + isPending: false, + error: null, + }), +})); + +vi.mock('@/components/pr-review/discussion/reply-input', () => ({ + ensureTermsAcceptedOutcome: vi.fn(async () => ({ kind: 'accepted' as const })), + TERMS_CHECK_RETRY_COPY: 'terms-check-retry', + TERMS_OUTDATED_COPY: 'terms-outdated', +})); + +vi.mock('@sentry/react-native', () => ({ captureException: vi.fn() })); + +vi.mock('@/lib/persist/drafts', () => ({ + loadDraft: vi.fn(async (): Promise => null), + saveDraft: vi.fn(async (): Promise => undefined), + clearDraft: vi.fn(async (): Promise => undefined), + prReviewDraftKey: (owner: string, repo: string, number: number) => + `pr-review:${owner}/${repo}#${number}`, +})); + +vi.mock('@/lib/persist/use-draft-flush', () => ({ + useDraftFlushOnBackground: vi.fn(), +})); + +// `mutation-error-display` imports the PR operation-ledger helpers, which +// import `expo-crypto` (and transitively expo-modules-core). Mock it so this +// suite stays node-only, same as the other ledger pure tests. +vi.mock('expo-crypto', () => ({ + randomUUID: () => 'not-used-in-pure-tests', +})); + +vi.mock('expo-haptics', () => ({ + notificationAsync: vi.fn(), +})); + +vi.mock('expo-router', () => ({ + useRouter: () => ({ push: vi.fn() }), +})); + +vi.mock('react-native', () => ({ + Alert: { alert: vi.fn() }, + Keyboard: { addListener: () => ({ remove: vi.fn() }) }, + ScrollView: 'ScrollView', + TextInput: 'TextInput', + View: 'View', +})); + +vi.mock('@/components/pr-review/pr-form-sheet-chrome', () => ({ + PrFormSheetFooter: 'PrFormSheetFooter', + PrFormSheetHeader: 'PrFormSheetHeader', + useFormSheetKeyboardVisible: () => false, +})); +vi.mock('@/components/pr-review/review-event-chips', () => ({ + ReviewEventChips: 'ReviewEventChips', +})); +vi.mock('@/components/ui/button', () => ({ Button: 'Button' })); +vi.mock('@/components/ui/accessible-status', () => ({ AccessibleStatus: 'AccessibleStatus' })); +vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); +vi.mock('@/components/pr-review/pr-review-pending-comment-row', () => ({ + focusAfterPendingCommentRemoval: vi.fn(), + PendingQueueHint: 'PendingQueueHint', + PrReviewPendingCommentRow: 'PrReviewPendingCommentRow', + ReviewSummaryField: 'ReviewSummaryField', +})); +vi.mock('@/components/pr-review/pr-review-reconnect-notice', () => ({ + PrReviewReconnectNotice: 'PrReviewReconnectNotice', +})); + +const ITEM_FRESH_A: PendingReviewItem = { + id: 'fresh-a', + path: 'src/a.ts', + side: 'RIGHT', + line: 1, + body: 'A', + commitSha: 'head-1', +}; +const ITEM_FRESH_B: PendingReviewItem = { + id: 'fresh-b', + path: 'src/b.ts', + side: 'RIGHT', + line: 2, + body: 'B', + commitSha: 'head-1', +}; +const ITEM_STALE: PendingReviewItem = { + id: 'stale-c', + path: 'src/c.ts', + side: 'RIGHT', + line: 3, + body: 'C', + commitSha: 'head-0', +}; + +let latestItems: PendingReviewItem[] = []; +let addCommentFn: ((item: PendingReviewItem) => void) | null = null; + +function Consumer() { + const value = usePendingReview(); + latestItems = value.items; + addCommentFn = value.addComment; + return null; +} + +function mount(): TestRenderer.ReactTestRenderer { + let renderer: TestRenderer.ReactTestRenderer | undefined = undefined; + act(() => { + renderer = TestRenderer.create( + + + undefined)} + /> + + ); + }); + // eslint-disable-next-line typescript-eslint/no-unnecessary-condition + if (!renderer) { + throw new Error('Failed to mount PrReviewSubmit'); + } + return renderer; +} + +async function flush(): Promise { + await act(async () => { + await new Promise(resolve => { + setTimeout(resolve, 0); + }); + }); +} + +function submitOnPress(renderer: TestRenderer.ReactTestRenderer): () => void { + const button = renderer.root.findAll( + node => node.props.accessibilityLabel === 'Submit 2 of 3 comments' + )[0]; + if (!button) { + throw new Error('Submit button not found'); + } + return button.props.onPress as () => void; +} + +beforeEach(() => { + latestItems = []; + addCommentFn = null; + submitMutationMock.mutateAsync.mockReset(); + submitMutationMock.mutateAsync.mockResolvedValue(undefined); +}); + +describe('PrReviewSubmit queue retention', () => { + it('keeps every queued item when a submit fails', async () => { + submitMutationMock.mutateAsync.mockRejectedValueOnce(new Error('network down')); + const renderer = mount(); + + act(() => { + addCommentFn?.(ITEM_FRESH_A); + addCommentFn?.(ITEM_FRESH_B); + addCommentFn?.(ITEM_STALE); + }); + expect(latestItems.map(item => item.id)).toEqual(['fresh-a', 'fresh-b', 'stale-c']); + + act(() => { + submitOnPress(renderer)(); + }); + await flush(); + + // A failed submit reaches the catch path, which drains nothing: the fresh + // and stale items all stay queued, with no optimistic removal. + expect(submitMutationMock.mutateAsync).toHaveBeenCalledTimes(1); + expect(latestItems.map(item => item.id)).toEqual(['fresh-a', 'fresh-b', 'stale-c']); + }); + + it('removes only the fresh items on success, leaving stale items queued', async () => { + const renderer = mount(); + + act(() => { + addCommentFn?.(ITEM_FRESH_A); + addCommentFn?.(ITEM_FRESH_B); + addCommentFn?.(ITEM_STALE); + }); + + act(() => { + submitOnPress(renderer)(); + }); + await flush(); + + // Success removes exactly the fresh ids; the stale item stays queued. + expect(submitMutationMock.mutateAsync).toHaveBeenCalledTimes(1); + expect(latestItems.map(item => item.id)).toEqual(['stale-c']); + }); +}); diff --git a/apps/mobile/src/components/pr-review/pr-review-submit.tsx b/apps/mobile/src/components/pr-review/pr-review-submit.tsx index 7955d1c8e4..8d25053eb2 100644 --- a/apps/mobile/src/components/pr-review/pr-review-submit.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-submit.tsx @@ -1,6 +1,8 @@ +/* eslint-disable max-lines -- the submit sheet composes the event radio, summary, pending-comments list, and the stale/fresh partition in one cohesive surface. */ // Review-submit content: event radio, optional summary, pending-comments -// list (view/edit/delete), and one batched submitReview call. Queue is -// cleared on success and retained on failure. +// list (view/edit/delete), and one batched submitReview call. On success the +// fresh comments are removed and stale comments stay queued; on failure the +// whole queue is retained. // // Disable lifetime: bad-request clears on event/summary change; forbidden // stays for the rest of the sheet session. Toasts paint behind formSheets @@ -40,7 +42,12 @@ import { import { classifyPrReviewMutationError } from '@/lib/pr-review/classify-pr-review-query-state'; import { mutationErrorDisplay } from '@/lib/pr-review/mutation-error-display'; import { type PendingReviewItem, usePendingReview } from '@/lib/pr-review/pending-review-provider'; +import { partitionPendingItems } from '@/lib/pr-review/partition-pending-items'; import { useSubmitReviewMutation } from '@/lib/pr-review/use-pr-review-mutations'; +import { + selectPartialSubmitMessage, + selectSubmitCtaLabel, +} from '@/components/pr-review/pr-review-submit-view'; const COMMENT_COMPOSER_PATH = '/(app)/pr-review/[owner]/[repo]/[number]/comment-composer' as const; @@ -66,6 +73,7 @@ export function PrReviewSubmit(props: PrReviewSubmitProps) { const [inlineErrorKind, setInlineErrorKind] = useState< 'retryable' | 'bad-request' | 'forbidden' | 'reconnect' | null >(null); + const [partialResult, setPartialResult] = useState(null); const bodyRef = useRef(''); const bodyInputRef = useRef(null); @@ -73,11 +81,16 @@ export function PrReviewSubmit(props: PrReviewSubmitProps) { const isSubmitting = submitReview.isPending; const queuedCount = pending.items.length; - const hasStaleItems = pending.items.some(item => item.commitSha !== headSha); + const { fresh, stale } = partitionPendingItems(pending.items, headSha); + const staleIds = new Set(stale.map(item => item.id)); const blockReason = reviewSubmitBlockReason({ event, hasSummary, - commentCount: queuedCount, + commentCount: fresh.length, + }); + const submitLabel = selectSubmitCtaLabel({ + freshCount: fresh.length, + totalCount: pending.items.length, }); useEffect(() => { @@ -130,6 +143,7 @@ export function PrReviewSubmit(props: PrReviewSubmitProps) { async function handleSubmit() { setInlineError(null); setInlineErrorKind(null); + setPartialResult(null); const outcome = await ensureTermsAcceptedOutcome(); if (outcome.kind === 'outdated') { setInlineError(TERMS_OUTDATED_COPY); @@ -149,12 +163,20 @@ export function PrReviewSubmit(props: PrReviewSubmitProps) { event, ...(body.length > 0 ? { body } : {}), commitSha: headSha, - items: pending.items, + items: fresh, }) ); - pending.clear(); + pending.removeComments(fresh.map(item => item.id)); void Haptics.notificationAsync(Haptics.NotificationFeedbackType.Success); - onDismiss(); + if (stale.length > 0) { + // Stale items stay queued: keep the sheet open and report the partial + // result instead of dismissing, so the user can edit or delete them. + setPartialResult( + selectPartialSubmitMessage({ freshCount: fresh.length, staleCount: stale.length }) + ); + } else { + onDismiss(); + } } catch { // Classified into inlineError by the effect above. } @@ -212,8 +234,8 @@ export function PrReviewSubmit(props: PrReviewSubmitProps) { let queueHint: ReactNode = null; if (blockReason !== null) { queueHint = ; - } else if (!keyboardVisible && (queuedCount === 0 || hasStaleItems)) { - queueHint = ; + } else if (!keyboardVisible && (queuedCount === 0 || stale.length > 0)) { + queueHint = ; } // PickerSheet invariant: [header, ScrollView]; footer is trailing content. @@ -262,6 +284,7 @@ export function PrReviewSubmit(props: PrReviewSubmitProps) { { openEditComposer(item); @@ -274,6 +297,10 @@ export function PrReviewSubmit(props: PrReviewSubmitProps) { : null} + {partialResult ? ( + + ) : null} + {inlineError && inlineErrorKind !== 'reconnect' ? ( {/* Mutation-classified errors are toast-owned (announcingToast); @@ -300,9 +327,9 @@ export function PrReviewSubmit(props: PrReviewSubmitProps) { }} loading={isSubmitting} disabled={submitDisabled} - accessibilityLabel="Submit review" + accessibilityLabel={submitLabel} > - Submit review + {submitLabel}