diff --git a/apps/mobile/src/app/(app)/_layout.tsx b/apps/mobile/src/app/(app)/_layout.tsx index b906648f86..ed169ee76b 100644 --- a/apps/mobile/src/app/(app)/_layout.tsx +++ b/apps/mobile/src/app/(app)/_layout.tsx @@ -158,6 +158,15 @@ export default function AppLayout() { headerShown: false, }} /> + ({ back: vi.fn() })); +const slot = vi.hoisted(() => ({ bridge: undefined as BranchPickerBridge | undefined })); + +vi.mock('expo-router', () => ({ + useRouter: () => router, +})); +vi.mock('react-native', () => ({ + Pressable: 'Pressable', + ScrollView: 'ScrollView', + View: 'View', +})); +vi.mock('@/components/picker-sheet', () => ({ + // The fake shell renders the header contract (title + both dismiss + // controls) and the rows below it, so a test can assert the header + // controls and the rows in one tree. + PickerSheet: (props: { + title: string; + onDone: () => void; + onCancel?: () => void; + expired?: boolean; + children?: React.ReactNode; + }) => + createElement( + 'PickerSheet', + { + title: props.title, + expired: props.expired === true, + onCancel: props.onCancel, + onDone: props.onDone, + }, + props.children + ), +})); +vi.mock('@/components/ui/text', async () => { + const React = await import('react'); + return { Text: 'Text', TextClassContext: React.createContext(undefined) }; +}); +vi.mock('@/components/ui/icons', () => ({ Check: 'Check' })); +vi.mock('@/lib/hooks/use-theme-colors', () => ({ + useThemeColors: () => ({ primary: '#0a84ff' }), +})); +vi.mock('@/lib/route-registry', () => ({ + UNFENCED_ROUTE_KEY: 'unscoped', + useRouteRegistry: vi.fn(), + branchPickerSlot: { + get: () => slot.bridge, + clear: vi.fn(), + }, +})); + +function texts(renderer: TestRenderer.ReactTestRenderer): string[] { + return renderer.root + .findAllByType('Text' as never) + .flatMap(node => node.children) + .filter((child): child is string => typeof child === 'string'); +} + +function branchLabel(branch: string): string { + return i18n.t('agentChat.newSession.branchAccessibility', { label: branch }); +} + +function branchRow(renderer: TestRenderer.ReactTestRenderer, branch: string) { + return renderer.root.findAll( + node => + node.props.accessibilityLabel === branchLabel(branch) && + typeof node.props.onPress === 'function' + )[0]; +} + +/** Fire a node's `onPress`, the way a tap would. */ +function press(node: TestRenderer.ReactTestInstance | undefined) { + act(() => { + (node?.props.onPress as (() => void) | undefined)?.(); + }); +} + +/** Mount the screen inside act, so i18n's subscription settles inside it. */ +function mount(): TestRenderer.ReactTestRenderer { + const ref: { current: TestRenderer.ReactTestRenderer | null } = { current: null }; + act(() => { + ref.current = TestRenderer.create(createElement(BranchPickerScreen)); + }); + const created = ref.current; + if (created === null) { + throw new Error('the branch picker route did not render'); + } + return created; +} + +function setBridge(overrides: Partial = {}) { + slot.bridge = { + branches: ['main', 'release/2.0'], + defaultBranch: 'main', + selectedBranch: 'main', + onSelect: vi.fn(() => undefined), + ...overrides, + }; +} + +beforeEach(() => { + (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + slot.bridge = undefined; + router.back.mockClear(); +}); + +describe('BranchPickerScreen', () => { + it('renders the header shell with both dismiss controls and one row per branch', () => { + setBridge(); + const renderer = mount(); + + const shell = renderer.root.findByType('PickerSheet' as never); + expect(shell.props.title).toBe(i18n.t('agentChat.newSession.branchPickerTitle')); + expect(typeof shell.props.onCancel).toBe('function'); + expect(typeof shell.props.onDone).toBe('function'); + + expect(branchRow(renderer, 'main')).toBeDefined(); + expect(branchRow(renderer, 'release/2.0')).toBeDefined(); + }); + + it('marks the provider default row and the selected row', () => { + setBridge({ selectedBranch: 'release/2.0' }); + const renderer = mount(); + + expect(texts(renderer)).toContain(i18n.t('agentChat.newSession.branchDefault')); + expect(branchRow(renderer, 'release/2.0')?.props.accessibilityState).toEqual({ + selected: true, + }); + expect(branchRow(renderer, 'main')?.props.accessibilityState).toEqual({ selected: false }); + }); + + it('hands the picked branch name back and dismisses', () => { + const onSelect = vi.fn(() => undefined); + setBridge({ onSelect }); + const renderer = mount(); + + press(branchRow(renderer, 'release/2.0')); + + expect(onSelect).toHaveBeenCalledWith('release/2.0'); + expect(router.back).toHaveBeenCalledTimes(1); + }); + + it('hands the default branch name back too — the trigger owns the override decision', () => { + const onSelect = vi.fn(() => undefined); + setBridge({ onSelect }); + const renderer = mount(); + + press(branchRow(renderer, 'main')); + + expect(onSelect).toHaveBeenCalledWith('main'); + }); + + it('dismisses from the header Cancel without reporting a pick', () => { + const onSelect = vi.fn(() => undefined); + setBridge({ onSelect }); + const renderer = mount(); + + const shell = renderer.root.findByType('PickerSheet' as never); + act(() => { + (shell.props.onCancel as () => void)(); + }); + + expect(router.back).toHaveBeenCalledTimes(1); + expect(onSelect).not.toHaveBeenCalled(); + }); + + it('renders the standard expired shell when the slot is gone', () => { + const renderer = mount(); + + const shell = renderer.root.findByType('PickerSheet' as never); + expect(shell.props.expired).toBe(true); + expect(texts(renderer)).not.toContain('main'); + }); +}); diff --git a/apps/mobile/src/app/(app)/agent-chat/branch-picker.tsx b/apps/mobile/src/app/(app)/agent-chat/branch-picker.tsx new file mode 100644 index 0000000000..2585e7a9be --- /dev/null +++ b/apps/mobile/src/app/(app)/agent-chat/branch-picker.tsx @@ -0,0 +1,85 @@ +import { useRouter } from 'expo-router'; +import { Check } from '@/components/ui/icons'; +import { useState } from 'react'; +import { Pressable, View } from 'react-native'; +import { useTranslation } from 'react-i18next'; + +import { PickerSheet } from '@/components/picker-sheet'; +import { Text } from '@/components/ui/text'; +import { useThemeColors } from '@/lib/hooks/use-theme-colors'; +import { type BranchPickerBridge } from '@/lib/picker-bridge'; +import { branchPickerSlot, UNFENCED_ROUTE_KEY, useRouteRegistry } from '@/lib/route-registry'; + +/** + * The new-session branch picker, presented as the standard formSheet (same + * shell as the repo/mode/model pickers). The shell's header carries the + * dismiss controls and the rows render below it, so a Cancel control can + * never float over — or drift away from — the branch rows. + */ +export default function BranchPickerScreen() { + const router = useRouter(); + const colors = useThemeColors(); + const { t } = useTranslation(); + useRouteRegistry(UNFENCED_ROUTE_KEY); + // Lazy init reads the slot synchronously on first render — no effect, no + // "Options expired" flash before a later effect populates state. + const [bridge] = useState(() => branchPickerSlot.get(UNFENCED_ROUTE_KEY)); + + function close() { + router.back(); + } + + function handleSelect(picker: BranchPickerBridge, branch: string) { + picker.onSelect(branch); + branchPickerSlot.clear(UNFENCED_ROUTE_KEY); + router.back(); + } + + if (!bridge) { + return ( + + ); + } + + return ( + + + {bridge.branches.map(branch => { + const isSelected = branch === bridge.selectedBranch; + const isDefault = branch === bridge.defaultBranch; + return ( + { + handleSelect(bridge, branch); + }} + > + + {branch} + + {isDefault ? ( + + {t('agentChat.newSession.branchDefault')} + + ) : null} + {isSelected ? : null} + + ); + })} + + + ); +} diff --git a/apps/mobile/src/app/(app)/agent-chat/repo-picker.tsx b/apps/mobile/src/app/(app)/agent-chat/repo-picker.tsx index 3caa8c6961..432c31f82d 100644 --- a/apps/mobile/src/app/(app)/agent-chat/repo-picker.tsx +++ b/apps/mobile/src/app/(app)/agent-chat/repo-picker.tsx @@ -2,9 +2,8 @@ import { useFocusEffect, useRouter } from 'expo-router'; import * as Haptics from 'expo-haptics'; import { Check, Info, Lock, Search, SearchX, Unlock } from '@/components/ui/icons'; import { useCallback, useMemo, useRef, useState } from 'react'; -import { FlatList, Pressable, TextInput, View } from 'react-native'; +import { Pressable, TextInput, View } from 'react-native'; import { useTranslation } from 'react-i18next'; -import { useSafeAreaInsets } from 'react-native-safe-area-context'; import { EmptyState } from '@/components/empty-state'; import { PickerSheet } from '@/components/picker-sheet'; @@ -21,7 +20,6 @@ type PickerListItem = export default function RepoPickerScreen() { const router = useRouter(); const colors = useThemeColors(); - const { bottom } = useSafeAreaInsets(); const { t } = useTranslation(); const [search, setSearch] = useState(''); const [bridge, setBridge] = useState(() => repoPickerSlot.get(UNFENCED_ROUTE_KEY)); @@ -102,7 +100,6 @@ export default function RepoPickerScreen() { @@ -136,17 +133,18 @@ export default function RepoPickerScreen() { } /> ) : ( - item.key} - keyboardShouldPersistTaps="handled" - keyboardDismissMode="on-drag" - contentContainerStyle={{ paddingBottom: bottom }} - renderItem={({ item }) => { + // Mapped rows inside the shell ScrollView instead of a FlatList: the + // FlatList stretches into the space the formSheet offers and its rows + // painted over the pinned search header while scrolling. The shell + // scroll view starts below the header, so a row can never overlap it. + + {listItems.map(item => { if (item.kind === 'header') { return ( - + {t(item.titleKey)} ); @@ -156,6 +154,7 @@ export default function RepoPickerScreen() { const rowLabel = `${platformName} ${repo.fullName}`; return ( { handleSelect(`${repo.platform}:${repo.fullName}`); @@ -182,9 +181,33 @@ export default function RepoPickerScreen() { ) : null} ); - }} - /> + })} + {renderBitbucketNote()} + )} ); + + /** + * Personal Bitbucket never lists repositories (organization-only), so the + * grouped list would end at GitLab with nothing explaining the gap. The + * note renders once, after the provider sections, whenever the picker has + * rows but no Bitbucket section; a connected org's rows suppress it. + */ + function renderBitbucketNote() { + if (search.trim() || !bridge) { + return null; + } + if (bridge.sections.some(section => section.key === 'bitbucket')) { + return null; + } + return ( + + + {t('agentChat.repoPicker.platformBitbucket')} + + {t('agentChat.newSession.bitbucketOrganizationsOnly')} + + ); + } } diff --git a/apps/mobile/src/app/(app)/pr-review/[platform]/[...identity]/_layout.tsx b/apps/mobile/src/app/(app)/pr-review/[platform]/[...identity]/_layout.tsx new file mode 100644 index 0000000000..5c917ab7f9 --- /dev/null +++ b/apps/mobile/src/app/(app)/pr-review/[platform]/[...identity]/_layout.tsx @@ -0,0 +1,125 @@ +import { type Href, Redirect, Stack, useLocalSearchParams } from 'expo-router'; +import { useMemo } from 'react'; + +import { appUnlockScreenLayout } from '@/components/app-unlock-screen'; +import { InvalidRouteState } from '@/components/invalid-route-state'; +import { PrReviewConnectGate } from '@/components/pr-review/pr-review-connect-gate'; +import { useFormSheetDetents } from '@/lib/form-sheet'; +import { useCurrentUserId } from '@/lib/hooks/use-current-user-id'; +import { useRouteForegroundRefresh } from '@/lib/hooks/use-route-foreground-refresh'; +import { useOrganization } from '@/lib/organization-context'; +import { + pendingReviewDraftKey, + PendingReviewProvider, +} from '@/lib/pr-review/pending-review-provider'; +import { + parseProviderPrRoute, + providerPrRefKey, + providerPrRoutePath, + ProviderPrScopeProvider, + providerPrTriple, +} from '@/lib/pr-review/provider-pr-ref'; +import { parseParam } from '@/lib/route-params'; + +type Params = { + platform: string; + identity: string[]; + instance?: string; +}; + +/** + * Param guard + scope hoist for the provider PR-review surface. + * + * The route is `[platform]/[...identity]`, where the LAST identity segment is + * the number (a GitLab MR iid, a Bitbucket PR id) and everything before it is + * the project path — a GitLab FULL nested path (`group/sub/repo`) or a + * Bitbucket `workspace/repo`. `parseProviderPrRoute` validates every segment, + * so a hand-built deep link with a missing, repeated or non-numeric segment + * never reaches a query. + * + * GitHub keeps its original `[owner]/[repo]/[number]` route untouched — this + * layout only redirects a hand-built `/pr-review/github/...` link there so + * that surface, its connect gate and its write sheets stay exactly as they + * were. + * + * The scope (ref + organization) is published in context rather than threaded + * through props: the diff list, the file navigator and the discussion tree + * take the GitHub-shaped `owner`/`repo`/`number` triple, and reading the real + * ref from context moves their queries to the right provider without a + * per-provider copy of that tree. + */ +export default function ProviderPrReviewLayout() { + const params = useLocalSearchParams(); + const platform = parseParam(params.platform) ?? ''; + // A catch-all param is a fresh array on every render; the joined form is a + // stable dependency, and a `/` inside a segment is percent-encoded by + // `providerPrRoutePath`, so splitting it back is lossless. + const identity = Array.isArray(params.identity) + ? params.identity.join('/') + : (parseParam(params.identity) ?? ''); + const instance = parseParam(params.instance) ?? ''; + const { organizationId } = useOrganization(); + const { fullSheetDetent } = useFormSheetDetents(); + const { userId } = useCurrentUserId(); + useRouteForegroundRefresh([[['providerReview']]]); + + const ref = useMemo( + () => + parseProviderPrRoute({ + platform, + identity: identity.split('/'), + instance: instance || undefined, + }), + [platform, identity, instance] + ); + const scope = useMemo(() => (ref ? { ref, organizationId } : null), [ref, organizationId]); + + if (!ref || !scope) { + return ; + } + + if (ref.platform === 'github') { + return ; + } + + // One draft queue per PR/MR: the GitHub-shaped key the store already uses, + // suffixed with the s1 collision-free ref identity so a GitLab MR and a + // GitHub PR that share `owner/repo#number` — and one project reached on two + // GitLab instances — never share a queue. + const triple = providerPrTriple(ref); + const draftEntityKey = `${pendingReviewDraftKey(triple.owner, triple.repo, triple.number)}@${providerPrRefKey(ref)}`; + + const sheetOptions = { + presentation: 'formSheet' as const, + sheetAllowedDetents: [0.5, fullSheetDetent] as [number, number], + sheetInitialDetentIndex: 'last' as const, + sheetGrabberVisible: true, + headerShown: false, + }; + + return ( + + + {/* The provider-aware connect gate (s7): a disconnected reader can + never reach the authenticated queries and mutations below, and a + Bitbucket personal scope gets the terminal org-only explanation + instead of a retry that could not succeed. */} + + + {/* The three write sheets (s6) are siblings of the GitHub route's + sheets: they mount inside this layout, so they see the provider + scope and this PR's single `PendingReviewProvider` queue. */} + + + + + + + + + ); +} diff --git a/apps/mobile/src/app/(app)/pr-review/[platform]/[...identity]/comment-composer.tsx b/apps/mobile/src/app/(app)/pr-review/[platform]/[...identity]/comment-composer.tsx new file mode 100644 index 0000000000..4a141c120b --- /dev/null +++ b/apps/mobile/src/app/(app)/pr-review/[platform]/[...identity]/comment-composer.tsx @@ -0,0 +1,5 @@ +import { PrReviewCommentComposerScreen } from '@/components/pr-review/pr-review-comment-composer-screen'; + +export default function ProviderPrReviewCommentComposerRoute() { + return ; +} diff --git a/apps/mobile/src/app/(app)/pr-review/[platform]/[...identity]/file-navigator.tsx b/apps/mobile/src/app/(app)/pr-review/[platform]/[...identity]/file-navigator.tsx new file mode 100644 index 0000000000..f5e1922b64 --- /dev/null +++ b/apps/mobile/src/app/(app)/pr-review/[platform]/[...identity]/file-navigator.tsx @@ -0,0 +1,5 @@ +import { PrReviewFileNavigatorScreen } from '@/components/pr-review/pr-review-file-navigator-screen'; + +export default function ProviderPrReviewFileNavigatorRoute() { + return ; +} diff --git a/apps/mobile/src/app/(app)/pr-review/[platform]/[...identity]/index.tsx b/apps/mobile/src/app/(app)/pr-review/[platform]/[...identity]/index.tsx new file mode 100644 index 0000000000..33d1ce0458 --- /dev/null +++ b/apps/mobile/src/app/(app)/pr-review/[platform]/[...identity]/index.tsx @@ -0,0 +1,40 @@ +import { type Href, Stack, useLocalSearchParams } from 'expo-router'; + +import { InvalidRouteState } from '@/components/invalid-route-state'; +import { PrReviewScreen } from '@/components/pr-review/pr-review-screen'; +import { parseProviderPrRoute, providerPrTriple } from '@/lib/pr-review/provider-pr-ref'; +import { parseParam } from '@/lib/route-params'; + +type Params = { + platform: string; + identity: string[]; + instance?: string; +}; + +/** + * The provider PR/MR detail screen. The layout above already validated the + * route and published the scope, so the screen renders through the same tree + * GitHub uses; the triple it takes is the GitHub-shaped identity its stores + * are keyed on, while its queries follow the ref from the scope. + */ +export default function ProviderPrReviewIndexRoute() { + const params = useLocalSearchParams(); + const ref = parseProviderPrRoute({ + platform: parseParam(params.platform) ?? '', + identity: params.identity, + instance: params.instance, + }); + + if (!ref) { + return ; + } + + const { owner, repo, number } = providerPrTriple(ref); + + return ( + <> + + + + ); +} diff --git a/apps/mobile/src/app/(app)/pr-review/[platform]/[...identity]/merge.tsx b/apps/mobile/src/app/(app)/pr-review/[platform]/[...identity]/merge.tsx new file mode 100644 index 0000000000..e337d7891b --- /dev/null +++ b/apps/mobile/src/app/(app)/pr-review/[platform]/[...identity]/merge.tsx @@ -0,0 +1,5 @@ +import { PrReviewMergeScreen } from '@/components/pr-review/pr-review-merge-screen'; + +export default function ProviderPrReviewMergeRoute() { + return ; +} diff --git a/apps/mobile/src/app/(app)/pr-review/[platform]/[...identity]/review-submit.tsx b/apps/mobile/src/app/(app)/pr-review/[platform]/[...identity]/review-submit.tsx new file mode 100644 index 0000000000..a6f5f35433 --- /dev/null +++ b/apps/mobile/src/app/(app)/pr-review/[platform]/[...identity]/review-submit.tsx @@ -0,0 +1,5 @@ +import { PrReviewReviewSubmitScreen } from '@/components/pr-review/pr-review-review-submit-screen'; + +export default function ProviderPrReviewSubmitRoute() { + return ; +} diff --git a/apps/mobile/src/components/agents/new-session-configure-form.tsx b/apps/mobile/src/components/agents/new-session-configure-form.tsx index 6ebae1b378..33d9762546 100644 --- a/apps/mobile/src/components/agents/new-session-configure-form.tsx +++ b/apps/mobile/src/components/agents/new-session-configure-form.tsx @@ -217,6 +217,7 @@ export function NewSessionConfigureForm({ contentContainerClassName="flex-grow px-4 pb-8 pt-4" keyboardShouldPersistTaps="handled" automaticallyAdjustKeyboardInsets + keyboardDismissMode="on-drag" > ({ + ActivityIndicator: 'ActivityIndicator', + View: 'View', +})); +vi.mock('@/components/ui/text', async () => { + const React = await import('react'); + return { Text: 'Text', TextClassContext: React.createContext(undefined) }; +}); +vi.mock('@/components/ui/button', () => ({ Button: 'Button' })); +vi.mock('@/components/ui/icons', () => ({ ExternalLink: 'ExternalLink', RefreshCw: 'RefreshCw' })); +vi.mock('@/components/ui/activity-indicator', () => ({ ActivityIndicator: 'ActivityIndicator' })); +vi.mock('@/components/query-error', () => ({ QueryError: 'QueryError' })); +vi.mock('@/components/agents/repo-selector', () => ({ RepoSelector: 'RepoSelector' })); +vi.mock('@/components/agents/repository-branch-selector', () => ({ + RepositoryBranchSelector: 'RepositoryBranchSelector', +})); +vi.mock('@/lib/hooks/use-theme-colors', () => ({ + useThemeColors: () => ({ foreground: '#000000', mutedForeground: '#777777' }), +})); + +const githubRow: NewSessionRepository = { + platform: 'github', + fullName: 'owner/repo', + isPrivate: false, +}; +const gitlabRow: NewSessionRepository = { + platform: 'gitlab', + fullName: 'owner/repo', + isPrivate: false, +}; + +const group = ( + key: RepositoryGroup['key'], + status: RepositoryGroup['status'], + repositories: NewSessionRepository[] = [] +): RepositoryGroup => ({ key, status, repositories }); + +function mountSection(overrides: { + value?: string; + repositories?: NewSessionRepository[]; + groups?: RepositoryGroup[]; +}) { + const renderer: { current: TestRenderer.ReactTestRenderer | null } = { current: null }; + act(() => { + renderer.current = TestRenderer.create( + createElement(NewSessionRepositorySection, { + disabled: false, + isRetrying: false, + onChange: vi.fn(() => undefined), + onConnect: vi.fn(() => undefined), + onRefreshRepos: vi.fn(() => undefined), + repositories: overrides.repositories ?? [githubRow, gitlabRow], + recents: [], + groups: overrides.groups ?? [group('github', 'repos'), group('gitlab', 'repos')], + value: overrides.value ?? '', + }) + ); + }); + const created = renderer.current; + if (created === null) { + throw new Error('the section did not render'); + } + return created; +} + +function branchSelectorProps(renderer: TestRenderer.ReactTestRenderer) { + return renderer.root.findAllByType('RepositoryBranchSelector' as never)[0]?.props as { + repository: NewSessionRepository | null; + disabled: boolean; + }; +} + +function renderedText(renderer: TestRenderer.ReactTestRenderer): string[] { + return renderer.root + .findAllByType('Text' as never) + .flatMap(node => node.children) + .filter((child): child is string => typeof child === 'string'); +} + +beforeEach(() => { + resetSelectedBranchOverrides(); +}); + +describe('NewSessionRepositorySection branch row', () => { + it('hands the branch selector the resolved repository row', () => { + const renderer = mountSection({ value: 'github:owner/repo' }); + + expect(branchSelectorProps(renderer).repository).toEqual(githubRow); + }); + + it('keeps same-named rows on two providers distinct', () => { + const renderer = mountSection({ value: 'gitlab:owner/repo' }); + + expect(branchSelectorProps(renderer).repository).toEqual(gitlabRow); + }); + + it('offers no branch row until a repository is selected', () => { + const renderer = mountSection({ value: '' }); + + expect(branchSelectorProps(renderer).repository).toBeNull(); + }); + + it('clears a stale branch override when the section mounts', () => { + setSelectedBranchOverride(githubRow, 'release/2.0'); + + mountSection({ value: 'github:owner/repo' }); + + expect(getSelectedBranchOverride(githubRow)).toBeNull(); + }); + + it('clears the branch override when the section unmounts', () => { + const renderer = mountSection({ value: 'github:owner/repo' }); + setSelectedBranchOverride(githubRow, 'release/2.0'); + + act(() => { + renderer.unmount(); + }); + + expect(getSelectedBranchOverride(githubRow)).toBeNull(); + }); +}); + +describe('NewSessionRepositorySection Bitbucket connect card', () => { + it('states outright that Bitbucket is organizations-only', () => { + const renderer = mountSection({ + groups: [group('github', 'repos'), group('gitlab', 'repos'), group('bitbucket', 'connect')], + }); + + expect(renderedText(renderer)).toContain( + i18n.t('agentChat.newSession.bitbucketOrganizationsOnly') + ); + }); + + it('leaves the GitHub connect card free of the Bitbucket restriction', () => { + const renderer = mountSection({ + groups: [group('github', 'connect'), group('gitlab', 'repos')], + }); + + expect(renderedText(renderer)).not.toContain( + i18n.t('agentChat.newSession.bitbucketOrganizationsOnly') + ); + }); +}); diff --git a/apps/mobile/src/components/agents/new-session-repository-section.tsx b/apps/mobile/src/components/agents/new-session-repository-section.tsx index 0022bb02d6..415bff7def 100644 --- a/apps/mobile/src/components/agents/new-session-repository-section.tsx +++ b/apps/mobile/src/components/agents/new-session-repository-section.tsx @@ -1,4 +1,4 @@ -import { Fragment, type ReactElement } from 'react'; +import { Fragment, type ReactElement, useEffect } from 'react'; import { View } from 'react-native'; import { ActivityIndicator } from '@/components/ui/activity-indicator'; import { useTranslation } from 'react-i18next'; @@ -8,11 +8,13 @@ import { Button } from '@/components/ui/button'; import { Text } from '@/components/ui/text'; import { QueryError } from '@/components/query-error'; import { RepoSelector } from '@/components/agents/repo-selector'; +import { RepositoryBranchSelector } from '@/components/agents/repository-branch-selector'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; import { type NewSessionRepository, type RepositoryGroup, type RepositoryPlatform, + resetSelectedBranchOverrides, } from './new-session-repository-state'; type NewSessionRepositorySectionProps = { @@ -65,6 +67,16 @@ const PROVIDER_COPY = { } >; +/** + * The restriction a provider's connect card must state outright. Bitbucket + * connects for an organization, never for a personal account, so "connect it" + * can never read as a promise that a personal session will get Bitbucket + * repositories — or their branches. + */ +function connectNoteKey(platform: RepositoryPlatform): string | undefined { + return platform === 'bitbucket' ? 'agentChat.newSession.bitbucketOrganizationsOnly' : undefined; +} + /** * Provider-aware repository section. One group per provider renders its own * connect/empty/error state independently, and the picker trigger lists every @@ -87,6 +99,22 @@ export function NewSessionRepositorySection({ const hasRepos = repositories.length > 0; const anyLoading = groups.some(group => group.status === 'loading'); + // The picker reports `platform:fullName`; resolve it to the row so the branch + // selector queries (and keys) the full repository identity. The prefill seeds + // the same platform-qualified key, so no bare-fullName fallback is needed — + // one would bind a same-named row on another provider. + const selectedRepository = + repositories.find(repository => `${repository.platform}:${repository.fullName}` === value) ?? + null; + + // The branch choice belongs to this screen: clear it when the section mounts + // and when it goes away, so a branch picked for one draft can never reach the + // next one. + useEffect(() => { + resetSelectedBranchOverrides(); + return resetSelectedBranchOverrides; + }, []); + return ( @@ -104,6 +132,8 @@ export function NewSessionRepositorySection({ /> )} + + {groups.map(group => ( {renderGroupCard(group.key, group.status)} ))} @@ -149,11 +179,13 @@ export function NewSessionRepositorySection({ function renderConnectCard(platform: RepositoryPlatform): ReactElement | null { const copy = PROVIDER_COPY[platform]; + const noteKey = connectNoteKey(platform); return ( {t(copy.connectTitle)} {t(copy.connectDescription)} + {noteKey ? {t(noteKey)} : null} + + ); + } + // Empty: the repository stays selected and the session starts on whatever + // the provider checks out — there is no override to offer. + if (branches.branches.length === 0) { + return renderNote(t('agentChat.newSession.branchEmpty')); + } + return renderTrigger(); + } + + function renderNote(message: string) { + return ( + + {message} + + ); + } + + function renderTrigger() { + // A provider can list branches without naming a default (a mirror with no + // HEAD, a repository whose default was deleted). The picker below still + // lists every branch, so the row asks for a choice rather than claiming + // the list is empty, and nothing is marked as the default. + const label = selectedBranch ?? t('agentChat.newSession.branchPlaceholder'); + const isDefault = selectedBranch !== null && selectedBranch === branches.defaultBranch; + return ( + + + {label} + + {isDefault ? ( + + {t('agentChat.newSession.branchDefault')} + + ) : null} + + + ); + } + + function openPicker() { + if (!repository || disabled) { + return; + } + // The keyboard belongs to the form; the sheet must not slide up over an + // open keyboard (the form keeps first responder across taps). + Keyboard.dismiss(); + branchPickerSlot.set(UNFENCED_ROUTE_KEY, { + branches: branches.branches, + defaultBranch: branches.defaultBranch, + selectedBranch, + onSelect: branch => { + // The provider default is stored as "no override", so the create body + // only carries `upstreamBranch` for a real, non-default choice. + setSelectedBranchOverride(repository, branch === branches.defaultBranch ? null : branch); + }, + }); + router.push('/(app)/agent-chat/branch-picker' as Href); + } +} diff --git a/apps/mobile/src/components/agents/session-detail-content.tsx b/apps/mobile/src/components/agents/session-detail-content.tsx index f8b911b998..8e772793a3 100644 --- a/apps/mobile/src/components/agents/session-detail-content.tsx +++ b/apps/mobile/src/components/agents/session-detail-content.tsx @@ -736,7 +736,8 @@ export function SessionDetailContent({ if (kept.length === 0) { return base; } - return [...base, ...kept].toSorted((a, b) => { + // eslint-disable-next-line unicorn/no-array-sort -- Hermes does not implement Array.prototype.toSorted; the spread already copies so nothing shared is mutated + return [...base, ...kept].sort((a, b) => { if (a.info.id < b.info.id) { return -1; } diff --git a/apps/mobile/src/components/agents/session-pr-badge.test.ts b/apps/mobile/src/components/agents/session-pr-badge.test.ts index bd751d1a1f..263cf7a75f 100644 --- a/apps/mobile/src/components/agents/session-pr-badge.test.ts +++ b/apps/mobile/src/components/agents/session-pr-badge.test.ts @@ -243,7 +243,7 @@ describe('SessionPrBadge mounted', () => { expect(mocks.openExternalUrl).not.toHaveBeenCalled(); }); - it('opens the browser for a GitLab PR on press', async () => { + it('opens the in-app merge request route for a GitLab MR on press', async () => { const renderer = await renderBadge({ pr: pr({ platform: 'gitlab', @@ -255,11 +255,26 @@ describe('SessionPrBadge mounted', () => { const pressable = findHost(renderer.root, 'Pressable')[0]; pressable?.props.onPress(); - expect(mocks.openExternalUrl).toHaveBeenCalledWith( - 'https://gitlab.com/octocat/hello-world/-/merge_requests/42', - { label: 'pull request' } + expect(mocks.push).toHaveBeenCalledWith( + '/(app)/pr-review/gitlab/octocat/hello-world/42?instance=https%3A%2F%2Fgitlab.com' ); - expect(mocks.push).not.toHaveBeenCalled(); + expect(mocks.openExternalUrl).not.toHaveBeenCalled(); + }); + + it('opens the in-app pull request route for a Bitbucket PR on press', async () => { + const renderer = await renderBadge({ + pr: pr({ + platform: 'bitbucket', + url: 'https://bitbucket.org/acme/api/pull-requests/42', + }), + loading: false, + }); + + const pressable = findHost(renderer.root, 'Pressable')[0]; + pressable?.props.onPress(); + + expect(mocks.push).toHaveBeenCalledWith('/(app)/pr-review/bitbucket/acme/api/42'); + expect(mocks.openExternalUrl).not.toHaveBeenCalled(); }); it('opens the browser for a GitHub PR when the PR review flag is off', async () => { diff --git a/apps/mobile/src/components/agents/session-pr-badge.tsx b/apps/mobile/src/components/agents/session-pr-badge.tsx index bf335c7850..b557d3004f 100644 --- a/apps/mobile/src/components/agents/session-pr-badge.tsx +++ b/apps/mobile/src/components/agents/session-pr-badge.tsx @@ -93,7 +93,6 @@ export function SessionPrBadge(props: SessionPrBadgeProps) { } const target = resolveSessionPrTapTarget({ url: pr.url, - number: pr.number, }); if (target.kind === 'in-app') { router.push(target.href); diff --git a/apps/mobile/src/components/agents/use-new-session-creator.test.ts b/apps/mobile/src/components/agents/use-new-session-creator.test.ts index a31d48ae45..58a967248e 100644 --- a/apps/mobile/src/components/agents/use-new-session-creator.test.ts +++ b/apps/mobile/src/components/agents/use-new-session-creator.test.ts @@ -6,7 +6,11 @@ import TestRenderer, { act } from 'react-test-renderer'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { type AgentMode } from '@/components/agents/mode-selector'; -import { type NewSessionRepository } from './new-session-repository-state'; +import { + type NewSessionRepository, + resetSelectedBranchOverrides, + setSelectedBranchOverride, +} from './new-session-repository-state'; import { useNewSessionCreator } from './use-new-session-creator'; import { clearDraft, flushDraft, loadDraft } from '@/lib/persist/drafts'; import { useFencedDraftLoad, useRemoteSpawnDraftCleanup } from '@/lib/persist/use-draft-load'; @@ -1238,3 +1242,111 @@ describe('useRemoteSpawnDraftCleanup remote-spawn clear', () => { expect(vi.mocked(flushDraft)).not.toHaveBeenCalled(); }); }); + +describe('useNewSessionCreator upstream branch', () => { + const githubRow: NewSessionRepository = { + platform: 'github', + fullName: 'owner/repo', + isPrivate: false, + }; + const gitlabRow: NewSessionRepository = { + platform: 'gitlab', + fullName: 'group/project', + isPrivate: true, + }; + const bitbucketRow: NewSessionRepository = { + platform: 'bitbucket', + fullName: 'workspace/repo', + isPrivate: true, + workspaceUuid: 'ws-1234', + repositoryUuid: 'repo-5678', + }; + + beforeEach(() => { + resetSelectedBranchOverrides(); + }); + + it('omits upstreamBranch when the provider default is in effect', async () => { + prepareSessionMutate.mockResolvedValue(sessionResult()); + const creator = runCreator({ selectedRepository: githubRow }); + + creator.promptRef.current = 'hello'; + await creator.createSessionFromDraft(); + + expect(prepareSessionMutate.mock.calls[0]?.[0]).not.toHaveProperty('upstreamBranch'); + }); + + it('sends the chosen branch for a GitHub row', async () => { + prepareSessionMutate.mockResolvedValue(sessionResult()); + setSelectedBranchOverride(githubRow, 'release/2.0'); + const creator = runCreator({ selectedRepository: githubRow }); + + creator.promptRef.current = 'hello'; + await creator.createSessionFromDraft(); + + expect(prepareSessionMutate.mock.calls[0]?.[0]).toMatchObject({ + githubRepo: 'owner/repo', + upstreamBranch: 'release/2.0', + }); + }); + + it('sends the chosen branch for a GitLab row', async () => { + prepareSessionMutate.mockResolvedValue(sessionResult()); + setSelectedBranchOverride(gitlabRow, 'feature/x'); + const creator = runCreator({ selectedRepository: gitlabRow }); + + creator.promptRef.current = 'hello'; + await creator.createSessionFromDraft(); + + expect(prepareSessionMutate.mock.calls[0]?.[0]).toMatchObject({ + gitlabProject: 'group/project', + upstreamBranch: 'feature/x', + }); + }); + + it('sends the chosen branch for a Bitbucket row', async () => { + prepareSessionMutate.mockResolvedValue(sessionResult()); + setSelectedBranchOverride(bitbucketRow, 'develop'); + const creator = runCreator({ organizationId: 'org-1', selectedRepository: bitbucketRow }); + + creator.promptRef.current = 'hello'; + await creator.createSessionFromDraft(); + + expect(prepareSessionMutate.mock.calls[0]?.[0]).toMatchObject({ + bitbucketRepo: { fullName: 'workspace/repo' }, + upstreamBranch: 'develop', + }); + }); + + it('never carries a branch chosen for another repository', async () => { + prepareSessionMutate.mockResolvedValue(sessionResult()); + setSelectedBranchOverride(gitlabRow, 'feature/x'); + const creator = runCreator({ selectedRepository: githubRow }); + + creator.promptRef.current = 'hello'; + await creator.createSessionFromDraft(); + + expect(prepareSessionMutate.mock.calls[0]?.[0]).not.toHaveProperty('upstreamBranch'); + }); + + it('keeps the retry fingerprint repository-scoped when the branch changes', async () => { + prepareSessionMutate.mockResolvedValue(sessionResult()); + const first = runCreator({ selectedRepository: githubRow }); + first.promptRef.current = 'hello'; + await first.createSessionFromDraft(); + const defaultBranchFingerprint = outboxMock.writeSafeRetry.mock.calls[0]?.[0].fingerprint; + + setSelectedBranchOverride(githubRow, 'release/2.0'); + const second = runCreator({ selectedRepository: githubRow }); + second.promptRef.current = 'hello'; + await second.createSessionFromDraft(); + const overrideFingerprint = outboxMock.writeSafeRetry.mock.calls[1]?.[0].fingerprint; + + // Same intent, same retry key: a branch change must not fork the safe-retry + // row, or one submit could replay as two sessions. + expect(overrideFingerprint).toBe(defaultBranchFingerprint); + expect(prepareSessionMutate.mock.calls[1]?.[0]).toMatchObject({ + upstreamBranch: 'release/2.0', + }); + }); +}); diff --git a/apps/mobile/src/components/agents/use-new-session-creator.ts b/apps/mobile/src/components/agents/use-new-session-creator.ts index d83e1ff960..2c2a804948 100644 --- a/apps/mobile/src/components/agents/use-new-session-creator.ts +++ b/apps/mobile/src/components/agents/use-new-session-creator.ts @@ -7,6 +7,7 @@ import { toast } from 'sonner-native'; import { i18n } from '@/i18n'; import { type AgentMode } from '@/components/agents/mode-selector'; import { + getSelectedBranchOverride, type NewSessionRepository, type RepositoryPlatform, } from '@/components/agents/new-session-repository-state'; @@ -50,6 +51,8 @@ type PrepareSessionInput = { githubRepo?: string; gitlabProject?: string; bitbucketRepo?: { fullName: string; workspaceUuid: string; repositoryUuid: string }; + /** The chosen non-default branch; omitted, the provider's default is checked out. */ + upstreamBranch?: string; autoCommit: boolean; autoInitiate: boolean; operationKey: string; @@ -327,9 +330,16 @@ function resolveRepoFingerprint(repository: NewSessionRepository | null): { /** * Write exactly one repository field into the create body, matching the - * selected row's platform. Bitbucket requires workspace + run ids, so it - * contributes nothing when those are missing (which cannot happen for a row - * that came from `listBitbucketRepositories`). + * selected row's platform, plus the branch the user picked for that exact + * repository. Bitbucket requires workspace + run ids, so it contributes + * nothing when those are missing (which cannot happen for a row that came + * from `listBitbucketRepositories`). + * + * The branch is read by repository identity, so a branch chosen for another + * repository can never ride along; only a non-default choice is stored, so an + * unset `upstreamBranch` means "check out the provider's own default". It is + * deliberately absent from the retry fingerprint: the retry key stays + * repository-scoped, and changing the branch must not fork it. */ function setRepositoryField( input: PrepareSessionInput, @@ -340,10 +350,12 @@ function setRepositoryField( } if (repository.platform === 'github') { input.githubRepo = repository.fullName; + setUpstreamBranch(input, repository); return; } if (repository.platform === 'gitlab') { input.gitlabProject = repository.fullName; + setUpstreamBranch(input, repository); return; } if (repository.workspaceUuid && repository.repositoryUuid) { @@ -352,5 +364,14 @@ function setRepositoryField( workspaceUuid: repository.workspaceUuid, repositoryUuid: repository.repositoryUuid, }; + setUpstreamBranch(input, repository); + } +} + +/** Carry the branch only when a repository field was written for it. */ +function setUpstreamBranch(input: PrepareSessionInput, repository: NewSessionRepository): void { + const branch = getSelectedBranchOverride(repository); + if (branch !== null) { + input.upstreamBranch = branch; } } diff --git a/apps/mobile/src/components/picker-sheet.tsx b/apps/mobile/src/components/picker-sheet.tsx index 42ab734c2b..f4c5b8e8af 100644 --- a/apps/mobile/src/components/picker-sheet.tsx +++ b/apps/mobile/src/components/picker-sheet.tsx @@ -68,7 +68,14 @@ export function PickerSheet({ {headerContent} {scrollable && !expired ? ( - {body} + // keyboardShouldPersistTaps keeps a first tap on a row working while + // a picker's search field holds the keyboard open. + + {body} + ) : ( body )} diff --git a/apps/mobile/src/components/pr-review/composer-inline-error.tsx b/apps/mobile/src/components/pr-review/composer-inline-error.tsx index 73c9500a8b..a5fa43772d 100644 --- a/apps/mobile/src/components/pr-review/composer-inline-error.tsx +++ b/apps/mobile/src/components/pr-review/composer-inline-error.tsx @@ -90,7 +90,7 @@ export function useComposerInlineError(error: unknown, isEdit: boolean) { })(); return; } - const display = mutationErrorDisplay('composer', classification, error); + const display = mutationErrorDisplay('composer', classification, { rawError: error }); setInlineError(display.message); setInlineErrorKind(display.kind); setInlineErrorIsLocal(false); diff --git a/apps/mobile/src/components/pr-review/diff/diff-line.mounted.test.tsx b/apps/mobile/src/components/pr-review/diff/diff-line.mounted.test.tsx new file mode 100644 index 0000000000..840eac4046 --- /dev/null +++ b/apps/mobile/src/components/pr-review/diff/diff-line.mounted.test.tsx @@ -0,0 +1,108 @@ +/* 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 pr-diff-hunk-rows.test.tsx) */ +import { createElement } from 'react'; +import TestRenderer, { act } from 'react-test-renderer'; +import { describe, expect, it, vi } from 'vitest'; + +import '@/i18n'; +import { DiffLine } from './diff-line'; +import { type ParsedDiffLine } from '@/lib/pr-review/diff/parse-patch'; + +vi.mock('react-native', () => ({ + Pressable: 'Pressable', + Text: 'RNText', + View: 'View', +})); +vi.mock('@/lib/hooks/use-theme-colors', () => ({ + useThemeColors: () => ({ + background: '#FFFFFF', + foreground: '#111111', + good: '#0a0', + destructive: '#d00', + mutedForeground: '#777777', + }), +})); + +function line(overrides: Partial = {}): ParsedDiffLine { + return { + type: 'context', + oldLine: 12, + newLine: 12, + text: 'const value = computeSomething(x);', + noNewlineAtEndOfFile: false, + ...overrides, + }; +} + +/** Mount a DiffLine inside act, so subscription updates stay inside it. */ +function mountLine(props: { + line: ParsedDiffLine; + language: string | null; + keyId: string; +}): TestRenderer.ReactTestRenderer { + const ref: { current: TestRenderer.ReactTestRenderer | null } = { current: null }; + act(() => { + ref.current = TestRenderer.create(createElement(DiffLine, props)); + }); + const created = ref.current; + if (created === null) { + throw new Error('the diff line did not render'); + } + return created; +} + +/** The row is the only `flex-row items-stretch` View in a DiffLine. */ +function findRow(renderer: TestRenderer.ReactTestRenderer): TestRenderer.ReactTestInstance { + const rows = renderer.root.findAll( + node => + node.type === ('View' as never) && + typeof node.props.className === 'string' && + node.props.className.includes('flex-row items-stretch') + ); + const [row] = rows; + if (rows.length !== 1 || row === undefined) { + throw new Error(`expected exactly one diff row, found ${rows.length}`); + } + return row; +} + +describe('DiffLine gutter alignment', () => { + // The row is `flex-row items-stretch`, so the gutter View stretches to the + // row's full height. A long code line wraps and makes the row several + // visual lines tall; the line number must sit on the FIRST visual line — + // aligned with the code's first line via the same top padding the code + // container uses — never centered onto a later visual line. + it('aligns the gutter number with the start of the row on a wrapped line', () => { + const renderer = mountLine({ + line: line({ + text: 'const wrappedValue = someVeryLongExpression(thatDoesNotFitOnOneLine, atPhoneWidth) + trailingOperand;', + }), + language: null, + keyId: 'line-12', + }); + + const row = findRow(renderer); + const [gutter, code] = row.props.children as [ + TestRenderer.ReactTestInstance, + TestRenderer.ReactTestInstance, + ]; + + expect(gutter.props.className).toContain('justify-start'); + expect(gutter.props.className).not.toContain('justify-center'); + expect((gutter.props.style as { paddingTop: number }).paddingTop).toBe(2); + // The code container pads by the same amount, so the gutter's first line + // and the code's first visual line share one baseline. + expect((code.props.style as { paddingVertical: number }).paddingVertical).toBe(2); + }); + + it('keeps the same alignment for add and delete rows', () => { + for (const type of ['add', 'del', 'context'] as const) { + const renderer = mountLine({ line: line({ type }), language: null, keyId: `k-${type}` }); + const row = findRow(renderer); + const [gutter] = row.props.children as [ + TestRenderer.ReactTestInstance, + TestRenderer.ReactTestInstance, + ]; + expect(gutter.props.className).toContain('justify-start'); + } + }); +}); diff --git a/apps/mobile/src/components/pr-review/diff/diff-line.tsx b/apps/mobile/src/components/pr-review/diff/diff-line.tsx index 07001afad6..795ab003fa 100644 --- a/apps/mobile/src/components/pr-review/diff/diff-line.tsx +++ b/apps/mobile/src/components/pr-review/diff/diff-line.tsx @@ -121,6 +121,11 @@ function DiffLineImpl({ line, language, onTap, isSelected }: Readonly - + {/* eslint-disable-next-line react-native/no-inline-styles, react-native/no-color-literals -- dynamic theme color + mono font for gutter */} void; }; -const FILE_NAVIGATOR_PATH = '/(app)/pr-review/[owner]/[repo]/[number]/file-navigator' as const; - export function PrDiffFileListHeader({ owner, repo, @@ -51,10 +50,14 @@ export function PrDiffFileListHeader({ const colors = useThemeColors(); const { t } = useTranslation(); - const navigatorHref = useMemo( - () => ({ pathname: FILE_NAVIGATOR_PATH, params: { owner, repo, number } }), - [owner, repo, number] - ); + // The sheet is a sibling of the screen it was opened from, so its href is + // built from the live scope: a GitHub ref keeps the original + // `[owner]/[repo]/[number]/file-navigator` path, a GitLab or Bitbucket ref + // opens the sheet inside the provider layout — the only place its scope is + // published, and therefore the only place the sheet can query the right + // provider. + const { ref } = useProviderPrScope({ owner, repo, number }); + const navigatorHref = useMemo(() => providerPrChildRoutePath(ref, 'file-navigator'), [ref]); const handleOpenNavigator = useCallback(() => { router.push(navigatorHref); diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-file-list.test.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-file-list.test.tsx index 3de14a9903..14c521b0f3 100644 --- a/apps/mobile/src/components/pr-review/diff/pr-diff-file-list.test.tsx +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-file-list.test.tsx @@ -4,10 +4,17 @@ import { type RefreshControlProps } from 'react-native'; import TestRenderer, { act } from 'react-test-renderer'; import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { ProviderPrScopeProvider } from '@/lib/pr-review/provider-pr-ref'; + import { PrReviewFileList } from './pr-diff-file-list'; const insetsState = vi.hoisted(() => ({ top: 0, bottom: 0, left: 0, right: 0 })); +// Records every (ref, headSha) the list hands to the viewed-files hook, so +// the provider-scoped keying (s6, identity rule 17) is proven at the call +// site rather than only in the store's unit tests. +const viewedFilesCalls = vi.hoisted(() => [] as unknown[][]); + const listQueryState = vi.hoisted(() => ({ query: { isLoading: false, @@ -89,7 +96,10 @@ vi.mock('@/lib/pr-review/diff/use-pr-diff-context-loader', () => ({ })); vi.mock('@/lib/pr-review/diff/pr-review-file-list-state', () => ({ usePrReviewFileListQuery: () => listQueryState, - usePrReviewViewedFiles: () => ({ isViewed: () => false, toggle: vi.fn(), isLoading: false }), + usePrReviewViewedFiles: (...args: unknown[]) => { + viewedFilesCalls.push(args); + return { isViewed: () => false, toggle: vi.fn(), isLoading: false }; + }, useFetchToCompletion: () => ({ run: vi.fn(), isRunning: false, @@ -130,6 +140,29 @@ function mountList(changedFiles = BASE_PROPS.changedFiles): TestRenderer.ReactTe return renderer; } +function mountListInScope( + ref: Parameters[0]['value']['ref'] +): TestRenderer.ReactTestRenderer { + const holder: { current: TestRenderer.ReactTestRenderer | undefined } = { current: undefined }; + act(() => { + holder.current = TestRenderer.create( + + + + ); + }); + const renderer = holder.current; + if (!renderer) { + throw new Error('renderer was not created'); + } + return renderer; +} + +function listBottomPadding(renderer: TestRenderer.ReactTestRenderer): number { + const list = renderer.root.find(node => String(node.type) === 'FlashList'); + return (list.props.contentContainerStyle as { paddingBottom: number }).paddingBottom; +} + function bottomPaddedViews( renderer: TestRenderer.ReactTestRenderer ): TestRenderer.ReactTestInstance[] { @@ -212,3 +245,85 @@ describe('PrReviewFileList full-body states', () => { expect(renderer.root.findAll(node => String(node.type) === 'QueryError')).toHaveLength(0); }); }); + +// The comment composer and the review-submit sheet are route siblings on +// every provider (s6): the write bar renders on a GitLab MR / Bitbucket PR +// too, and the bar carries the provider ref so it pushes the sheet inside +// the ref's own route — never the GitHub sibling. +describe('PrReviewFileList write affordances per provider', () => { + beforeEach(() => { + insetsState.bottom = 0; + resetState(); + listQueryState.files = [{ path: 'src/file.ts' }]; + viewedFilesCalls.length = 0; + }); + + it('keeps the write bar on a GitHub pull request', () => { + const renderer = mountList(); + const bar = renderer.root.find(node => String(node.type) === 'PrDiffFloatingActions'); + expect(bar.props.prRef).toBeUndefined(); + }); + + // The viewed set must be keyed by the live provider ref (s6, identity + // rule 17): the store folds `providerPrRefKey` into the key only when the + // call site hands it a ref, so the bare triple would silently collide. + it('keys the viewed set by the live ref, never the bare triple', () => { + mountList(); + expect(viewedFilesCalls[0]).toEqual([ + { platform: 'github', owner: 'octocat', repo: 'hello-world', number: 7 }, + 'sha', + ]); + mountListInScope({ platform: 'gitlab', projectPath: 'group/sub/repo', mrIid: 12 }); + expect(viewedFilesCalls[1]).toEqual([ + { platform: 'gitlab', projectPath: 'group/sub/repo', mrIid: 12 }, + 'sha', + ]); + mountListInScope({ platform: 'bitbucket', workspace: 'acme', repoSlug: 'api', prId: 42 }); + expect(viewedFilesCalls[2]).toEqual([ + { platform: 'bitbucket', workspace: 'acme', repoSlug: 'api', prId: 42 }, + 'sha', + ]); + }); + + it('keeps the write bar on a GitLab merge request, carrying the provider ref', () => { + const renderer = mountListInScope({ + platform: 'gitlab', + projectPath: 'group/sub/repo', + mrIid: 12, + }); + const bar = renderer.root.find(node => String(node.type) === 'PrDiffFloatingActions'); + expect(bar.props.prRef).toEqual({ + platform: 'gitlab', + projectPath: 'group/sub/repo', + mrIid: 12, + }); + }); + + it('keeps the write bar on a Bitbucket pull request, carrying the provider ref', () => { + const renderer = mountListInScope({ + platform: 'bitbucket', + workspace: 'acme', + repoSlug: 'api', + prId: 42, + }); + const bar = renderer.root.find(node => String(node.type) === 'PrDiffFloatingActions'); + expect(bar.props.prRef).toEqual({ + platform: 'bitbucket', + workspace: 'acme', + repoSlug: 'api', + prId: 42, + }); + }); + + it('keeps the small footer gap under a provider diff list too', () => { + const githubPadding = listBottomPadding(mountList()); + const gitlabPadding = listBottomPadding( + mountListInScope({ platform: 'gitlab', projectPath: 'group/repo', mrIid: 12 }) + ); + // The bar is an in-flow footer below the list (spot check e3), so no + // row can ever scroll under it; the list only keeps a 12-point gap + // between its last row and the footer's top edge, on every provider. + expect(githubPadding).toBe(12); + expect(gitlabPadding).toBe(12); + }); +}); diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-file-list.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-file-list.tsx index db42a09621..3540a51fd4 100644 --- a/apps/mobile/src/components/pr-review/diff/pr-diff-file-list.tsx +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-file-list.tsx @@ -12,8 +12,9 @@ // * S7a adds diff-line selection: tapping a line runs the pure // `selectLine` reducer; the result is mirrored into the // `diff-selection-bridge` (so the comment composer can read it on -// mount) and a floating action bar (`PrDiffFloatingActions`) -// hosts the "Comment" and "Finish review" affordances. +// mount) and a footer action bar (`PrDiffFloatingActions`) rendered +// in-flow below the list hosts the "Comment" and "Finish review" +// affordances. // // Cold first paint: FlashList mounts only after the first page of files is // present. The first-load waiting state is a plain skeleton outside the list @@ -26,7 +27,7 @@ import { FlashList, type FlashListRef } from '@shopify/flash-list'; import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { useTranslation } from 'react-i18next'; -import { View, type ViewStyle } from 'react-native'; +import { View } from 'react-native'; import { RefreshControl } from '@/components/ui/refresh-control'; import { QueryError } from '@/components/query-error'; @@ -41,11 +42,12 @@ import { } from '@/components/pr-review/diff/pr-diff-file-list-header'; import { PrDiffFileListLoading } from '@/components/pr-review/diff/pr-diff-file-list-loading'; import { PrDiffFloatingActions } from '@/components/pr-review/diff/pr-diff-floating-actions'; +import { usePrDiffStateCopy } from '@/components/pr-review/diff/pr-diff-state-copy'; +import { useProviderPrScope } from '@/lib/pr-review/provider-pr-ref'; import { useDiffRenderItem } from '@/components/pr-review/diff/pr-diff-file-list-render'; import { useDiffSelection } from '@/components/pr-review/diff/use-diff-selection'; import { EmptyFilesView, TabStateMessage } from '@/components/pr-review/diff/pr-diff-rows'; import { buildFileItems, buildPaginationItem } from '@/lib/pr-review/diff/pr-diff-list-builder'; -import { prDiffListBottomPadding } from '@/lib/pr-review/diff/pr-diff-list-bottom-padding'; import { itemTypeFor, type ListItem } from '@/lib/pr-review/diff/pr-diff-list-items'; import { stickyFileHeaderIndices } from '@/lib/pr-review/diff/sticky-file-headers'; import { usePrDiffContextLoader } from '@/lib/pr-review/diff/use-pr-diff-context-loader'; @@ -59,6 +61,9 @@ import { clearDiffSelection } from '@/lib/pr-review/diff-selection-bridge'; import { CenteredState } from '@/components/centered-state'; import { useIsTablet } from '@/lib/hooks/use-is-tablet'; +// Gap between the last diff row and the in-flow footer bar's top edge. +const PR_DIFF_LIST_FOOTER_GAP = 12; + type PrReviewFileListProps = { readonly owner: string; readonly repo: string; @@ -91,7 +96,12 @@ export function PrReviewFileList({ number, enabled: true, }); - const viewed = usePrReviewViewedFiles({ owner, repo, number }, headSha); + // The live provider scope: the viewed set is keyed by the ref (s6, + // identity rule 17), so a GitLab MR and a same-numbered GitHub PR — or + // one project on two GitLab instances — never share a set. On the GitHub + // route the fallback ref is the triple itself, keeping the legacy bytes. + const scope = useProviderPrScope({ owner, repo, number }); + const viewed = usePrReviewViewedFiles(scope.ref, headSha); const fetchToCompletion = useFetchToCompletion(query, changedFiles); const [expanded, setExpanded] = useState>({}); @@ -121,24 +131,16 @@ export function PrReviewFileList({ [owner, repo, number] ); - // Measured floating-bar height (null until the first layout event). - const [barHeight, setBarHeight] = useState(null); - - // Stable callback: ignore sub-one-point noise to avoid unnecessary - // re-renders. Layout events can fire with fractional-pixel deltas. - const handleHeightChange = useCallback((height: number) => { - setBarHeight(prev => { - if (prev !== null && Math.abs(prev - height) < 1) { - return prev; - } - return height; - }); - }, []); + // The write bar renders on every provider (s6): its two routes — the + // comment composer and the review-submit sheet — are siblings of the + // GitHub route AND of the provider route, so the bar pushes the sheet + // inside the scope its queries run under. The bar is an in-flow footer + // below the list (spot check e3), so the list only keeps a small gap + // between its last row and the footer's top edge. + const listContentStyle = useMemo(() => ({ paddingBottom: PR_DIFF_LIST_FOOTER_GAP }), []); - const contentContainerStyle = useMemo( - () => ({ paddingBottom: prDiffListBottomPadding(barHeight) }), - [barHeight] - ); + // Which provider's words the terminal and empty states use. + const copy = usePrDiffStateCopy({ owner, repo, number }); const viewedCount = useMemo(() => { let count = 0; @@ -262,19 +264,11 @@ export function PrReviewFileList({ if (files.length === 0) { if (firstPageErrorState?.kind === 'not-found') { - return ( - - ); + return ; } if (firstPageErrorState?.kind === 'permission') { return ( - + ); } if (firstPageErrorState?.kind === 'reconnect') { @@ -301,6 +295,7 @@ export function PrReviewFileList({ return ( 0 ? ( @@ -357,7 +352,7 @@ export function PrReviewFileList({ } }} onEndReachedThreshold={0.5} - contentContainerStyle={contentContainerStyle} + contentContainerStyle={listContentStyle} ItemSeparatorComponent={null} /> )} @@ -365,10 +360,10 @@ export function PrReviewFileList({ owner={owner} repo={repo} number={number} + prRef={scope.ref.platform === 'github' ? undefined : scope.ref} viewMode={effectiveViewMode} selection={selection} onClearSelection={clearSelection} - onHeightChange={handleHeightChange} /> 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 index d8c25eb1bd..e51f73d7f7 100644 --- 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 @@ -16,6 +16,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import '@/i18n'; import { PrDiffFileNavigator } from '@/components/pr-review/diff/pr-diff-file-navigator'; import { type PrReviewFile } from '@/lib/pr-review/diff/pr-review-file-types'; +import { type ProviderPrRef, ProviderPrScopeProvider } from '@/lib/pr-review/provider-pr-ref'; import { renderWithProviders } from '@/test/render-with-providers'; // ── Hoisted mocks ────────────────────────────────────────────────────────── @@ -31,6 +32,11 @@ const rowRenders = vi.hoisted( () => [] as { path: string; onSelect: () => void; onToggleViewed: () => void }[] ); +// Records every (ref, headSha) the navigator hands to the viewed-files hook, +// so the provider-scoped keying (s6, identity rule 17) is proven at the call +// site rather than only in the store's unit tests. +const viewedFilesCalls = vi.hoisted(() => [] as unknown[][]); + // Captures the latest FlashList props so tests can read `onEndReached`. const flashListProps = vi.hoisted(() => ({ current: null as null | Record })); @@ -153,7 +159,10 @@ let fetchAllResult: FetchAllResult = { vi.mock('@/lib/pr-review/diff/pr-review-file-list-state', () => ({ usePrReviewFileListQuery: () => listQueryResult, - usePrReviewViewedFiles: () => viewedResult, + usePrReviewViewedFiles: (...args: unknown[]) => { + viewedFilesCalls.push(args); + return viewedResult; + }, useFetchToCompletion: () => fetchAllResult, })); @@ -571,3 +580,60 @@ describe('PrDiffFileNavigator list bottom inset (plan §6)', () => { expect(listContentStyle()).toEqual({ paddingBottom: 66, paddingTop: 8 }); }); }); + +// s6 (identity rule 17): the sheet toggling a file here and the diff list +// behind it share the viewed store, so both must key it by the LIVE provider +// ref. A bare triple would silently collide across providers and GitLab +// instances; the store only folds the collision-free key when it receives +// the ref. +describe('PrDiffFileNavigator viewed-set provider keying (s6)', () => { + const GITLAB_REF: ProviderPrRef = { + platform: 'gitlab', + projectPath: 'group/sub/repo', + mrIid: 12, + }; + + beforeEach(() => { + viewedFilesCalls.length = 0; + 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('keys the viewed set by the provider ref under the provider scope', async () => { + await renderWithProviders( + + + + ); + + expect(viewedFilesCalls[0]).toEqual([GITLAB_REF, 'sha']); + }); + + it('falls back to the GitHub ref triple on the GitHub route', async () => { + await renderWithProviders(); + + expect(viewedFilesCalls[0]).toEqual([ + { platform: 'github', owner: 'octocat', repo: 'hello-world', number: 7 }, + 'sha', + ]); + }); +}); 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 1b68b7e368..8b041df532 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 @@ -43,6 +43,7 @@ import { } 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 { useProviderPrScope } from '@/lib/pr-review/provider-pr-ref'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; // Memoized row so recycled cells do not re-render on every keystroke: `file` @@ -108,7 +109,11 @@ export function PrDiffFileNavigator({ number, enabled: true, }); - const viewed = usePrReviewViewedFiles({ owner, repo, number }, headSha); + // The viewed set is keyed by the live provider ref (s6, identity rule 17), + // so the sheet toggling a file here and the diff list behind it read and + // write the SAME provider-scoped set — and never a same-numbered PR's. + const scope = useProviderPrScope({ owner, repo, number }); + const viewed = usePrReviewViewedFiles(scope.ref, headSha); const fetchAll = useFetchToCompletion(query, changedFiles); const hasActiveSearch = searchRef.current.trim().length > 0; diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.backdrop.test.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.backdrop.test.tsx new file mode 100644 index 0000000000..7338b83730 --- /dev/null +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.backdrop.test.tsx @@ -0,0 +1,102 @@ +// Spot check e2-expand.png: the Finish review island floated over the +// unified diff with deleted-line text still visible around and below the +// button. The card was opaque but the bar's padding ring was not, so diff +// rows scrolled under it showed through. The bar container itself must +// carry the screen background and swallow touches in that ring. +// (Extracted from pr-diff-floating-actions.test.tsx to keep that file +// inside the max-lines budget.) + +import * as React from 'react'; +import { describe, expect, it, vi } from 'vitest'; + +import '@/i18n'; +import type * as ReactI18next from 'react-i18next'; +import { PrDiffFloatingActions } from './pr-diff-floating-actions'; +import { type SelectionState } from '@/lib/pr-review/diff-selection'; + +vi.mock('react-i18next', async importOriginal => { + const actual = await importOriginal(); + return { + ...actual, + useTranslation: () => { + const i18n = actual.getI18n(); + return { t: i18n.t.bind(i18n), i18n }; + }, + }; +}); + +vi.mock('expo-router', () => ({ + useRouter: () => ({ push: vi.fn() }), +})); + +vi.mock('react-native', () => ({ + View: 'View', + Platform: { OS: 'ios' }, +})); + +vi.mock('react-native-safe-area-context', () => ({ + useSafeAreaInsets: () => ({ top: 0, bottom: 0, left: 0, right: 0 }), +})); + +vi.mock('@/components/ui/icons', () => ({ + MessageCirclePlus: () => null, +})); + +vi.mock('@/lib/hooks/use-theme-colors', () => ({ + useThemeColors: () => ({ + primaryForeground: '#FFFFFF', + foreground: '#000000', + mutedForeground: '#6F6A61', + }), +})); + +vi.mock('@/components/ui/button', () => ({ Button: 'Button' })); +vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); +vi.mock('@/lib/pr-review/diff-selection-bridge', () => ({ + clearDiffSelection: vi.fn(), +})); + +vi.mock('@/lib/pr-review/pending-review-provider', () => ({ + usePendingReview: () => ({ + items: [], + addComment: vi.fn(() => undefined), + updateComment: vi.fn(() => undefined), + removeComment: vi.fn(() => undefined), + clear: vi.fn(() => undefined), + }), +})); + +const baseProps = { + owner: 'octocat', + repo: 'hello', + number: 7, + viewMode: 'unified' as const, + selection: null as SelectionState | null, + onClearSelection: vi.fn(), +}; + +function renderBar(): React.ReactElement { + // eslint-disable-next-line new-cap -- plain function component, no hooks state needed for the container props + return PrDiffFloatingActions(baseProps); +} + +describe('PrDiffFloatingActions opaque backdrop (spot check e2)', () => { + it('paints the bar container with the screen background and swallows touches', () => { + // With a transparent container the diff rows scrolled under the bar + // stayed visible around and below the button. The container carries the + // screen background, and the removed `pointerEvents="box-none"` means a + // tap in the padding ring can never reach a diff row hidden behind it. + const props = renderBar().props as { className?: string; pointerEvents?: string }; + expect(props.pointerEvents).toBeUndefined(); + expect((props.className ?? '').split(' ')).toContain('bg-background'); + }); + + it('keeps the action card on the same background inside the bar', () => { + const card = (renderBar().props as { children?: React.ReactElement }).children; + if (!card) { + throw new Error('floating action card not found'); + } + const classes = (card.props as { className?: string }).className ?? ''; + expect(classes.split(' ')).toContain('bg-background'); + }); +}); diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.badge.mounted.test.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.badge.mounted.test.tsx new file mode 100644 index 0000000000..381fc20647 --- /dev/null +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.badge.mounted.test.tsx @@ -0,0 +1,252 @@ +/* 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 pr-diff-file-list.test.tsx) */ +// Spot check e1-select-line / e1-line1-comment: the Finish review count badge +// rode the label's top-right corner (`absolute -right-2.5 -top-2.5`), so the +// opaque pill drew over the last glyphs of the label. The earlier repairs +// pinned the bar's container and footer classes, never the badge's own +// placement inside the button, so the overlap survived them. This file mounts +// the real `PrDiffFloatingActions` inside the real `Button` (the composed +// render path the e1 screenshots show: a GitLab MR, a line selected so the +// Comment row is up, a non-empty pending queue) and pins what makes the +// overlap structurally impossible: the badge is an in-flow sibling AFTER the +// label in the button's `flex-row items-center justify-center gap-2` line, +// no node in the whole tree is absolute, and a two-digit count behaves the +// same. Mutation-inversion gate: re-wrapping the label in a `relative` View +// with an `absolute` badge must fail tests 1–3. +import TestRenderer, { act } from 'react-test-renderer'; +import { describe, expect, it, vi } from 'vitest'; + +import '@/i18n'; +import type * as ReactI18next from 'react-i18next'; +import { type PendingReviewItem } from '@/lib/pr-review/pending-review-provider'; +import { type ProviderPrRef } from '@/lib/pr-review/provider-pr-ref'; +import { type SelectionState } from '@/lib/pr-review/diff-selection'; +import { PrDiffFloatingActions } from './pr-diff-floating-actions'; + +vi.mock('react-i18next', async importOriginal => { + const actual = await importOriginal(); + return { + ...actual, + useTranslation: () => { + const i18n = actual.getI18n(); + return { t: i18n.t.bind(i18n), i18n }; + }, + }; +}); + +const insets = vi.hoisted(() => ({ top: 0, bottom: 0, left: 0, right: 0 })); + +const pendingState = vi.hoisted((): { items: PendingReviewItem[] } => ({ items: [] })); + +// The real Button and the real Text mount against these host stubs, so the +// button's own `flex-row items-center justify-center gap-2` classes — the gap +// that separates label from badge — are the ones under test. +vi.mock('react-native', () => ({ + View: 'View', + Pressable: 'Pressable', + ActivityIndicator: 'ActivityIndicator', + Text: 'RNText', + Platform: { OS: 'ios' }, + I18nManager: { isRTL: false, doLeftAndRightSwapInRTL: false }, +})); +vi.mock('@rn-primitives/slot', () => ({ Text: 'SlotText', View: 'SlotView' })); +vi.mock('expo-router', () => ({ + useRouter: () => ({ push: vi.fn() }), +})); +vi.mock('react-native-safe-area-context', () => ({ + useSafeAreaInsets: () => insets, +})); +vi.mock('@/components/ui/icons', () => ({ + MessageCirclePlus: () => null, +})); +// The real Button reaches the UI spinner through its loading arm; the real +// spinner pulls the motion policy (expo-battery), which this harness does not +// mount. The badge placement under test never renders the spinner. +vi.mock('@/components/ui/activity-indicator', () => ({ ActivityIndicator: 'ActivityIndicator' })); +vi.mock('@/lib/hooks/use-theme-colors', () => ({ + useThemeColors: () => ({ + primaryForeground: '#FFFFFF', + foreground: '#000000', + mutedForeground: '#6F6A61', + }), +})); +vi.mock('@/lib/pr-review/diff-selection-bridge', () => ({ + clearDiffSelection: vi.fn(), +})); +vi.mock('@/lib/pr-review/pending-review-provider', () => ({ + usePendingReview: () => ({ + items: pendingState.items, + addComment: vi.fn(() => undefined), + updateComment: vi.fn(() => undefined), + removeComment: vi.fn(() => undefined), + clear: vi.fn(() => undefined), + }), +})); + +const GITLAB_REF: ProviderPrRef = { + platform: 'gitlab', + projectPath: 'group/sub/repo', + mrIid: 12, +}; + +// The e1 moment: a README line is selected, so the bar shows the selection +// row (Comment + Clear) above the Finish review button. +const SELECTION: SelectionState = { + path: 'README.md', + side: 'RIGHT', + hunkKey: 'README.md:0', + startLine: 5, + line: 5, + selectedText: '- old readme line', +}; + +function makeItem(index: number): PendingReviewItem { + return { + id: `id-${index}`, + path: 'README.md', + side: 'RIGHT', + line: index + 1, + body: 'comment', + commitSha: 'head-1', + }; +} + +function classesOf(node: TestRenderer.ReactTestInstance): string[] { + return typeof node.props.className === 'string' ? node.props.className.split(' ') : []; +} + +function styleOf(node: TestRenderer.ReactTestInstance): Record { + const style = node.props.style; + return style != null && typeof style === 'object' && !Array.isArray(style) + ? (style as Record) + : {}; +} + +/** The instance children of a node, dropping raw text nodes. */ +function instanceChildren(node: TestRenderer.ReactTestInstance): TestRenderer.ReactTestInstance[] { + return node.children.filter( + (child): child is TestRenderer.ReactTestInstance => typeof child !== 'string' + ); +} + +/** Resolve a child to its rendered host node: the label mounts as the real + * Text composite, so the button's child is the composite, not the RNText. */ +function hostRoot(node: TestRenderer.ReactTestInstance): TestRenderer.ReactTestInstance { + let current = node; + while (typeof current.type !== 'string') { + const [first] = instanceChildren(current); + if (!first) { + throw new Error('composite rendered nothing'); + } + current = first; + } + return current; +} + +/** The instance child at `index`, or a thrown error naming the tree. */ +function childAt( + node: TestRenderer.ReactTestInstance, + index: number +): TestRenderer.ReactTestInstance { + const kids = instanceChildren(node); + const kid = kids[index]; + if (!kid) { + throw new Error(`expected a child at index ${index}, got ${kids.length} children`); + } + return kid; +} + +function hostText(node: TestRenderer.ReactTestInstance): string { + return node.children.filter((child): child is string => typeof child === 'string').join(''); +} + +function mountBar(pendingCount: number): TestRenderer.ReactTestRenderer { + pendingState.items = Array.from({ length: pendingCount }, (_, index) => makeItem(index)); + const ref: { current: TestRenderer.ReactTestRenderer | undefined } = { current: undefined }; + act(() => { + ref.current = TestRenderer.create( + undefined)} + /> + ); + }); + const renderer = ref.current; + if (!renderer) { + throw new Error('renderer was not created'); + } + return renderer; +} + +function finishReviewButton( + renderer: TestRenderer.ReactTestRenderer +): TestRenderer.ReactTestInstance { + return renderer.root.find( + node => String(node.type) === 'Pressable' && node.props.accessibilityLabel === 'Finish review' + ); +} + +describe('Finish review count badge placement (spot check e1)', () => { + it('lays the badge out in-flow after the label, never over it', () => { + const renderer = mountBar(3); + const button = finishReviewButton(renderer); + const buttonClasses = classesOf(button); + // The button is the row that spaces label and badge apart. + expect(buttonClasses).toContain('flex-row'); + expect(buttonClasses).toContain('items-center'); + expect(buttonClasses).toContain('justify-center'); + expect(buttonClasses).toContain('gap-2'); + + const kids = instanceChildren(button); + expect(kids).toHaveLength(2); + const label = hostRoot(childAt(button, 0)); + expect(String(label.type)).toBe('RNText'); + expect(hostText(label)).toBe('Finish review'); + const badge = childAt(button, 1); + expect(String(badge.type)).toBe('View'); + expect(classesOf(badge)).toContain('rounded-full'); + // The defect: the badge was anchored to the label's corner with negative + // offsets, so it drew over the last glyphs. In-flow it cannot. + expect(classesOf(badge)).not.toContain('absolute'); + expect(styleOf(badge).position).not.toBe('absolute'); + expect(styleOf(badge).right).toBeUndefined(); + expect(styleOf(badge).top).toBeUndefined(); + // The badge is a direct sibling of the label in the button row, after it. + expect(badge.parent).toBe(button); + expect(hostText(hostRoot(childAt(badge, 0)))).toBe('3'); + }); + + it('renders no absolutely positioned node anywhere in the bar', () => { + const renderer = mountBar(3); + const absolutes = renderer.root.findAll(node => { + if (classesOf(node).includes('absolute')) { + return true; + } + return styleOf(node).position === 'absolute'; + }); + expect(absolutes).toHaveLength(0); + }); + + it('keeps the badge in-flow for a two-digit pending count', () => { + const renderer = mountBar(12); + const button = finishReviewButton(renderer); + expect(instanceChildren(button)).toHaveLength(2); + const badge = childAt(button, 1); + expect(String(badge.type)).toBe('View'); + expect(classesOf(badge)).not.toContain('absolute'); + expect(hostText(hostRoot(childAt(badge, 0)))).toBe('12'); + }); + + it('renders the label alone when the pending queue is empty', () => { + const renderer = mountBar(0); + const button = finishReviewButton(renderer); + expect(instanceChildren(button)).toHaveLength(1); + expect(hostText(hostRoot(childAt(button, 0)))).toBe('Finish review'); + const badges = renderer.root.findAll(node => classesOf(node).includes('rounded-full')); + expect(badges).toHaveLength(0); + }); +}); diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.test.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.test.tsx index 565b69700a..51c844330c 100644 --- a/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.test.tsx +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.test.tsx @@ -18,6 +18,7 @@ import '@/i18n'; import type * as ReactI18next from 'react-i18next'; import { PrDiffFloatingActions } from './pr-diff-floating-actions'; import { type PendingReviewItem } from '@/lib/pr-review/pending-review-provider'; +import { type ProviderPrRef } from '@/lib/pr-review/provider-pr-ref'; import { type SelectionState } from '@/lib/pr-review/diff-selection'; vi.mock('react-i18next', async importOriginal => { @@ -241,6 +242,66 @@ describe('PrDiffFloatingActions submit reachability (P1-F-46b)', () => { }); }); +// ── Provider arms (s6) ─────────────────────────────────────────────── +// +// The two sheets are route siblings on every provider, so a bar holding a +// provider ref pushes the sheet inside the ref's own route (the provider +// scope the layout publishes), never the GitHub sibling. + +const GITLAB_REF: ProviderPrRef = { platform: 'gitlab', projectPath: 'group/sub/repo', mrIid: 12 }; +const BITBUCKET_REF: ProviderPrRef = { + platform: 'bitbucket', + workspace: 'acme', + repoSlug: 'api', + prId: 42, +}; + +describe('PrDiffFloatingActions provider routes (s6)', () => { + const selection: SelectionState = { + path: 'src/lib.ts', + side: 'RIGHT', + hunkKey: 'h1', + startLine: 3, + line: 5, + selectedText: 'x', + }; + + function pressButtonWith(prRef: ProviderPrRef | undefined, label: string): void { + routerPush.mockClear(); + // eslint-disable-next-line new-cap + const element = PrDiffFloatingActions({ ...baseProps, prRef, selection }); + const button = findElement({ + node: element, + type: 'Button', + prop: 'accessibilityLabel', + value: label, + }); + if (!button) { + throw new Error(`${label} button not found`); + } + (button.props as { onPress?: () => void }).onPress?.(); + } + + it('pushes the comment composer inside the GitLab ref route with the line params', () => { + pressButtonWith(GITLAB_REF, 'Comment on selected lines'); + + expect(routerPush).toHaveBeenCalledTimes(1); + expect(routerPush).toHaveBeenCalledWith( + '/(app)/pr-review/gitlab/group/sub/repo/12/comment-composer?path=src%2Flib.ts&side=RIGHT&line=5&startLine=3' + ); + }); + + it.each<[ProviderPrRef, string]>([ + [GITLAB_REF, '/(app)/pr-review/gitlab/group/sub/repo/12/review-submit'], + [BITBUCKET_REF, '/(app)/pr-review/bitbucket/acme/api/42/review-submit'], + ])('pushes the review-submit sheet inside the %s ref route', (prRef, expectedHref) => { + pressButtonWith(prRef, 'Finish review'); + + expect(routerPush).toHaveBeenCalledTimes(1); + expect(routerPush).toHaveBeenCalledWith(expectedHref); + }); +}); + describe('PrDiffFloatingActions bottom inset (plan §6)', () => { beforeEach(() => { insets.bottom = 0; @@ -249,18 +310,13 @@ describe('PrDiffFloatingActions bottom inset (plan §6)', () => { function findRootBar(): React.ReactElement | null { // eslint-disable-next-line new-cap const element = PrDiffFloatingActions(baseProps); - return findElement({ - node: element, - type: 'View', - prop: 'pointerEvents', - value: 'box-none', - }); + return element; } function rootPaddingBottom(): number | undefined { const root = findRootBar(); if (!root) { - throw new Error('floating action bar root not found'); + throw new Error('footer action bar root not found'); } return (root.props as { style?: { paddingBottom?: number } }).style?.paddingBottom; } @@ -274,27 +330,17 @@ describe('PrDiffFloatingActions bottom inset (plan §6)', () => { expect(rootPaddingBottom()).toBe(58); }); - it('reports the measured layout height through onHeightChange', () => { - const onHeightChange = vi.fn(() => undefined); - // eslint-disable-next-line new-cap - const element = PrDiffFloatingActions({ ...baseProps, onHeightChange }); - const root = findElement({ - node: element, - type: 'View', - prop: 'pointerEvents', - value: 'box-none', - }); + it('renders in-flow, not as an overlay over the list', () => { + // Spot check e3: the bar used to sit `absolute inset-x-0 bottom-0` over + // the FlashList, so a partly-scrolled diff row was clipped at its top + // edge. As an in-flow footer the list ends above it at every scroll + // position. + const root = findRootBar(); if (!root) { - throw new Error('floating action bar root not found'); + throw new Error('footer action bar root not found'); } - const onLayout = ( - root.props as { - onLayout?: (event: { nativeEvent: { layout: { height: number } } }) => void; - } - ).onLayout; - onLayout?.({ nativeEvent: { layout: { height: 150 } } }); - - expect(onHeightChange).toHaveBeenCalledTimes(1); - expect(onHeightChange).toHaveBeenCalledWith(150); + const classes = ((root.props as { className?: string }).className ?? '').split(' '); + expect(classes).not.toContain('absolute'); + expect(classes).toContain('w-full'); }); }); diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.tsx index a2b47ab08c..46ff49f6b9 100644 --- a/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.tsx +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.tsx @@ -1,4 +1,7 @@ -// Floating action bar rendered over the PR diff FlashList. Hosts: +// Footer action bar rendered in-flow below the PR diff FlashList. The list +// ends at its top edge, so a diff row is never clipped by it at any scroll +// position (spot check e3: the bar floated over the list and cut the last +// src/beta.ts line). Hosts: // - The "Comment" affordance that pushes the comment-composer route // when a diff-line selection exists, plus a "Clear" button that // drops the selection. @@ -13,14 +16,16 @@ import { type Href, useRouter } from 'expo-router'; import { MessageCirclePlus } from '@/components/ui/icons'; import { useTranslation } from 'react-i18next'; -import { type LayoutChangeEvent, View } from 'react-native'; +import { View } from 'react-native'; import { useSafeAreaInsets } from 'react-native-safe-area-context'; import { Button } from '@/components/ui/button'; import { Text } from '@/components/ui/text'; +import { providerPrSheetHref } from '@/components/pr-review/pr-review-provider-sheet-href'; import { clearDiffSelection } from '@/lib/pr-review/diff-selection-bridge'; import { type SelectionState } from '@/lib/pr-review/diff-selection'; import { type DiffViewMode } from '@/lib/pr-review/diff/pr-diff-list-items'; +import { type ProviderPrRef } from '@/lib/pr-review/provider-pr-ref'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; import { usePendingReview } from '@/lib/pr-review/pending-review-provider'; import { cn } from '@/lib/utils'; @@ -32,32 +37,38 @@ type PrDiffFloatingActionsProps = Readonly<{ owner: string; repo: string; number: number; + /** + * The provider ref when the diff renders under a GitLab / Bitbucket scope + * (s6). The two sheets are route siblings on every provider, so the bar + * pushes the sheet inside the ref's own route — pushing the GitHub sibling + * would leave the provider scope and write to the wrong provider. + */ + prRef?: ProviderPrRef; /** Unified (default) or side-by-side (tablet only). */ viewMode: DiffViewMode; /** `null` when no selection exists. Drives the "Comment" affordance. */ selection: SelectionState | null; /** Setter for the parent's selection state — `null` clears. */ onClearSelection: () => void; - /** Optional callback for the measured root layout height (points). */ - onHeightChange?: (height: number) => void; }>; export function PrDiffFloatingActions({ owner, repo, number, + prRef, viewMode, selection, onClearSelection, - onHeightChange, }: PrDiffFloatingActionsProps) { const router = useRouter(); const colors = useThemeColors(); const { t } = useTranslation(); const pending = usePendingReview(); - // The bar sits on the bottom edge, so its bottom padding must include the - // Android system inset. The measured height (onLayout) therefore already - // includes the inset, which `prDiffListBottomPadding` reserves for the list. + // The footer sits on the bottom edge, so its bottom padding must include + // the Android system inset. The container is opaque (`bg-background`) and + // in-flow: the diff list ends at its top edge, so no row is ever clipped + // by it and nothing shows through around the card. const insets = useSafeAreaInsets(); const showSelectionAction = viewMode === 'unified' && selection !== null; @@ -70,22 +81,29 @@ export function PrDiffFloatingActions({ if (!selection) { return; } + const lineParams = { + path: selection.path, + side: selection.side, + line: selection.line, + ...(selection.startLine !== selection.line ? { startLine: selection.startLine } : {}), + }; + if (prRef) { + router.push(providerPrSheetHref(prRef, 'comment-composer', lineParams)); + return; + } const href: Href = { pathname: COMMENT_COMPOSER_PATH, - params: { - owner, - repo, - number, - path: selection.path, - side: selection.side, - line: selection.line, - ...(selection.startLine !== selection.line ? { startLine: selection.startLine } : {}), - }, + // The bracketed GitHub pathname needs the route segments as params. + params: { owner, repo, number, ...lineParams }, }; router.push(href); } function openReviewSubmit() { + if (prRef) { + router.push(providerPrSheetHref(prRef, 'review-submit')); + return; + } const href: Href = { pathname: REVIEW_SUBMIT_PATH, params: { owner, repo, number }, @@ -95,11 +113,7 @@ export function PrDiffFloatingActions({ return ( { - onHeightChange?.(event.nativeEvent.layout.height); - }} - pointerEvents="box-none" - className="absolute inset-x-0 bottom-0 items-center gap-2 px-4 pt-3" + className="w-full items-center gap-2 bg-background px-4 pt-3" style={{ paddingBottom: 24 + insets.bottom }} > @@ -129,19 +143,23 @@ export function PrDiffFloatingActions({ ) : null} + {/* The Button row is `flex-row items-center justify-center gap-2`, so + the count badge is an in-flow pill AFTER the label. It used to ride + the label's top-right corner (`absolute -right-2.5 -top-2.5`), + which drew the opaque badge over the last glyphs of the label + (spot check e1-select-line / e1-line1-comment). In-flow the badge + can never cover the label, at any pending count or font scale. */} diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-hunk-rows.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-hunk-rows.tsx index c592db7706..c11d423ca9 100644 --- a/apps/mobile/src/components/pr-review/diff/pr-diff-hunk-rows.tsx +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-hunk-rows.tsx @@ -273,10 +273,13 @@ export function TabStateMessage({ title, message }: { title: string; message: st export function EmptyFilesView({ changedFiles, + noChangesDescription, onRequestOverview, refreshControl, }: { changedFiles: number; + /** Provider wording for the 0-changed-files case; GitHub copy when absent. */ + noChangesDescription?: string; onRequestOverview?: () => void; refreshControl?: ScrollViewProps['refreshControl']; }) { @@ -291,7 +294,7 @@ export function EmptyFilesView({ {changedFiles === 0 - ? t('prReview.noFilesChangedDescription') + ? (noChangesDescription ?? t('prReview.noFilesChangedDescription')) : t('prReview.hunkRows.filesStillLoading')} {onRequestOverview ? ( diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-side-by-side-row.mounted.test.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-side-by-side-row.mounted.test.tsx new file mode 100644 index 0000000000..203a827ce7 --- /dev/null +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-side-by-side-row.mounted.test.tsx @@ -0,0 +1,86 @@ +/* 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 diff-line.mounted.test.tsx) */ +import { createElement } from 'react'; +import TestRenderer, { act } from 'react-test-renderer'; +import { describe, expect, it, vi } from 'vitest'; + +import '@/i18n'; +import { SideBySideRow } from './pr-diff-side-by-side-row'; +import { type ParsedDiffLine } from '@/lib/pr-review/diff/parse-patch'; +import { type SideBySideRow as SideBySideRowData } from '@/lib/pr-review/diff/side-by-side'; + +vi.mock('react-native', () => ({ + Text: 'RNText', + View: 'View', +})); +vi.mock('@/components/ui/text', async () => { + const React = await import('react'); + return { Text: 'Text', TextClassContext: React.createContext(undefined) }; +}); +vi.mock('@/lib/hooks/use-theme-colors', () => ({ + useThemeColors: () => ({ + background: '#FFFFFF', + foreground: '#111111', + mutedForeground: '#777777', + }), +})); + +function line(overrides: Partial = {}): ParsedDiffLine { + return { + type: 'context', + oldLine: 7, + newLine: 7, + text: 'const value = computeSomething(x);', + noNewlineAtEndOfFile: false, + ...overrides, + }; +} + +function row(overrides: Partial = {}): SideBySideRowData { + return { left: { line: line(overrides) }, right: { line: line(overrides) } }; +} + +function mountRow(data: SideBySideRowData): TestRenderer.ReactTestRenderer { + const ref: { current: TestRenderer.ReactTestRenderer | null } = { current: null }; + act(() => { + ref.current = TestRenderer.create( + createElement(SideBySideRow, { row: data, language: null, rowKeyId: 'row-7' }) + ); + }); + const created = ref.current; + if (created === null) { + throw new Error('the side-by-side row did not render'); + } + return created; +} + +describe('SideBySideRow gutter alignment', () => { + // Same defect class as the unified DiffLine gutter: a wrapped code line + // makes the column several visual lines tall, and a centered number would + // drift onto a later visual line instead of the column's start. + it('top-aligns both column gutters with the code first line', () => { + const renderer = mountRow( + row({ + text: 'const wrappedValue = someVeryLongExpression(thatDoesNotFitOnOneLine, atPhoneWidth);', + }) + ); + + const columns = renderer.root.findAll( + node => + node.type === ('View' as never) && + typeof node.props.className === 'string' && + node.props.className.includes('flex-1 flex-row items-stretch') + ); + expect(columns).toHaveLength(2); + + for (const column of columns) { + const children = column.props.children as TestRenderer.ReactTestInstance[]; + const gutter = children[0]; + if (gutter === undefined) { + throw new Error('the column rendered without a gutter'); + } + expect(gutter.props.className).toContain('justify-start'); + expect(gutter.props.className).not.toContain('justify-center'); + expect((gutter.props.style as { paddingTop: number }).paddingTop).toBe(2); + } + }); +}); diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-side-by-side-row.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-side-by-side-row.tsx index 8dde148dac..31ce207518 100644 --- a/apps/mobile/src/components/pr-review/diff/pr-diff-side-by-side-row.tsx +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-side-by-side-row.tsx @@ -90,6 +90,9 @@ function SideColumnImpl({ line, side, language, isDark, foreground }: SideColumn const gutterStyle: ViewStyle = { width: COLUMN_GUTTER_WIDTH, minHeight: metrics.rowMinHeight, + // Top-aligned with the code's first line (see DiffLine's gutter): a + // centered number drifts onto a later visual line when the code wraps. + paddingTop: VERTICAL_PADDING, }; const codeContainerStyle: ViewStyle = { paddingVertical: VERTICAL_PADDING }; const codeBaseStyle: TextStyle = { @@ -114,7 +117,7 @@ function SideColumnImpl({ line, side, language, isDark, foreground }: SideColumn style={rowStyle} > {/* eslint-disable-next-line react-native/no-inline-styles, react-native/no-color-literals -- dynamic theme muted color */} diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-state-copy.ts b/apps/mobile/src/components/pr-review/diff/pr-diff-state-copy.ts new file mode 100644 index 0000000000..740d3f2625 --- /dev/null +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-state-copy.ts @@ -0,0 +1,41 @@ +// Provider wording for the file list's terminal and empty states. +// +// GitLab calls this a merge request, and its copy must not name the Kilo +// GitHub App or "this pull request"; GitHub and Bitbucket both say pull +// request, so the provider term alone switches the strings and the list keeps +// one set of states for all three providers. + +import { useMemo } from 'react'; +import { useTranslation } from 'react-i18next'; + +import { type ProviderPrTriple, useProviderPrScope } from '@/lib/pr-review/provider-pr-ref'; + +type PrDiffStateCopy = { + readonly unavailableTitle: string; + readonly unavailableMessage: string; + readonly accessDeniedMessage: string; + /** Undefined keeps the GitHub wording inside `EmptyFilesView`. */ + readonly noChangesDescription: string | undefined; +}; + +export function usePrDiffStateCopy(triple: ProviderPrTriple): PrDiffStateCopy { + const { t } = useTranslation(); + const isMergeRequest = useProviderPrScope(triple).ref.platform === 'gitlab'; + return useMemo( + () => ({ + unavailableTitle: isMergeRequest + ? t('prReview.terms.mergeRequestUnavailable') + : t('prReview.pullRequestUnavailable'), + unavailableMessage: isMergeRequest + ? t('prReview.terms.unavailableDescription') + : t('prReview.pullRequestUnavailableDescription'), + accessDeniedMessage: isMergeRequest + ? t('prReview.terms.accessDeniedMergeRequest') + : t('prReview.accessDeniedDescription'), + noChangesDescription: isMergeRequest + ? t('prReview.terms.noFilesChangedDescription') + : undefined, + }), + [isMergeRequest, t] + ); +} diff --git a/apps/mobile/src/components/pr-review/discussion/comment-row.test.tsx b/apps/mobile/src/components/pr-review/discussion/comment-row.test.tsx index c6e6481844..5f53a7fa87 100644 --- a/apps/mobile/src/components/pr-review/discussion/comment-row.test.tsx +++ b/apps/mobile/src/components/pr-review/discussion/comment-row.test.tsx @@ -302,3 +302,43 @@ describe('CommentRow overflow actions', () => { renderer.unmount(); }); }); + +describe('CommentRow reactions capability gate (s6)', () => { + afterEach(() => { + vi.clearAllMocks(); + }); + + async function renderWithCapabilities( + reactionsSupported: boolean + ): Promise { + let renderer: TestRenderer.ReactTestRenderer | null = null; + await act(async () => { + await Promise.resolve(); + renderer = TestRenderer.create( + createElement(CommentRow, { + comment: makeComment(), + onToggleReaction: vi.fn<() => void>(), + readOnly: true, + reactionsSupported, + }) + ); + }); + // eslint-disable-next-line typescript-eslint/no-unnecessary-condition + if (!renderer) { + throw new Error('Failed to create test renderer'); + } + return renderer; + } + + it('supported (default): renders the reactions row', async () => { + const renderer = await renderWithCapabilities(true); + expect(renderer.root.findAll(node => (node.type as string) === 'ReactionsRow')).toHaveLength(1); + renderer.unmount(); + }); + + it('unsupported: renders no reactions row at all — never an empty or failing one', async () => { + const renderer = await renderWithCapabilities(false); + expect(renderer.root.findAll(node => (node.type as string) === 'ReactionsRow')).toHaveLength(0); + renderer.unmount(); + }); +}); diff --git a/apps/mobile/src/components/pr-review/discussion/comment-row.tsx b/apps/mobile/src/components/pr-review/discussion/comment-row.tsx index 18fa553fc4..fa034f1439 100644 --- a/apps/mobile/src/components/pr-review/discussion/comment-row.tsx +++ b/apps/mobile/src/components/pr-review/discussion/comment-row.tsx @@ -43,6 +43,13 @@ type CommentRowProps = { readonly onToggleReaction: (content: ReviewReactionContent) => void; readonly reactionsDisabled?: boolean; readonly readOnly?: boolean; + /** + * The `capabilities.reactions.supported` flag (s6). False renders NO + * reactions row at all — the provider has no reaction affordance to + * offer, so the row shows nothing instead of an empty or failing one. + * Defaults to true, so the GitHub call sites are unchanged. + */ + readonly reactionsSupported?: boolean; /** The viewer's GitHub login, used to disable self-target moderation. */ readonly viewerLogin?: string | null; }; @@ -96,6 +103,7 @@ export function CommentRow({ onToggleReaction, reactionsDisabled, readOnly, + reactionsSupported = true, viewerLogin = null, }: Readonly) { const authorName = selectCommentAuthorName(comment.author); @@ -257,12 +265,14 @@ export function CommentRow({ - + {reactionsSupported ? ( + + ) : null} ); } diff --git a/apps/mobile/src/components/pr-review/discussion/discussion-thread.provider-gate.test.tsx b/apps/mobile/src/components/pr-review/discussion/discussion-thread.provider-gate.test.tsx new file mode 100644 index 0000000000..a408a3490f --- /dev/null +++ b/apps/mobile/src/components/pr-review/discussion/discussion-thread.provider-gate.test.tsx @@ -0,0 +1,243 @@ +/* eslint-disable typescript-eslint/no-deprecated -- react-test-renderer is the DOM-free renderer used to test React/RN structure under vitest */ +import { createElement } from 'react'; +import TestRenderer, { act } from 'react-test-renderer'; +import { describe, expect, it, vi } from 'vitest'; + +import { type ReviewThread } from '@/lib/pr-review/discussion/review-discussion-types'; +import { type ProviderPrRef, ProviderPrScopeProvider } from '@/lib/pr-review/provider-pr-ref'; + +import { DiscussionThread } from './discussion-thread'; + +// The s6 write arm of the provider discussion surface. The reply and resolve +// mutations route through the `providerReview` seam on a GitLab MR / Bitbucket +// PR (the hooks pick the arm from the provider scope the layout publishes), so +// the thread OFFERS resolve and reply on every platform whose capabilities +// allow it — with the provider-native ids (discussion id / verbatim comment +// id) riding on the wire. Reactions have no seam write path and no provider +// read layer returns reaction data, so a provider comment row stays read-only: +// the row shows nothing rather than a dead or failing affordance, and the +// capability flag removes the reaction row entirely where the provider says +// unsupported (Bitbucket). The read-only facts the providers DO report — the +// Resolved badge — stay rendered. + +const GITLAB_REF: ProviderPrRef = { platform: 'gitlab', projectPath: 'group/sub/repo', mrIid: 12 }; +const BITBUCKET_REF: ProviderPrRef = { + platform: 'bitbucket', + workspace: 'acme', + repoSlug: 'api', + prId: 42, +}; + +function makeThread(overrides: Partial = {}): ReviewThread { + return { + threadId: 'D-12', + isResolved: false, + isOutdated: false, + subjectType: 'LINE', + path: 'src/index.ts', + line: 10, + startLine: null, + originalLine: null, + originalStartLine: null, + diffSide: 'RIGHT', + diffHunk: null, + comments: [ + { + commentId: 12, + nodeId: '12', + author: { login: 'alice', avatarUrl: null }, + bodyMarkdown: 'hello', + createdAt: '2024-01-01T00:00:00Z', + reactions: [], + }, + ], + ...overrides, + }; +} + +const baseProps = { + owner: 'group/sub', + repo: 'repo', + number: 12, + onToggleExpand: vi.fn<() => void>(), +}; + +vi.mock('react-native', () => ({ + Pressable: 'Pressable', + View: 'View', +})); +vi.mock('expo-haptics', () => ({ selectionAsync: vi.fn() })); +vi.mock('@/components/ui/icons', () => ({ + Check: 'Check', + CheckCheck: 'CheckCheck', + ChevronDown: 'ChevronDown', + ChevronUp: 'ChevronUp', +})); +vi.mock('@/components/pr-review/discussion/comment-row', () => ({ CommentRow: 'CommentRow' })); +vi.mock('@/components/pr-review/discussion/reply-input', () => ({ ReplyInput: 'ReplyInput' })); +vi.mock('@/components/pr-review/discussion/thread-diff-snippet', () => ({ + ThreadDiffSnippet: 'ThreadDiffSnippet', +})); +vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); +vi.mock('@/lib/hooks/use-theme-colors', () => ({ + useThemeColors: () => ({ mutedForeground: '#6F6A61', good: '#22C55E' }), +})); +const { resolveMutate } = vi.hoisted(() => ({ resolveMutate: vi.fn<() => void>() })); + +vi.mock('@/lib/pr-review/discussion/use-review-discussion-mutations', () => ({ + useAddReactionMutation: () => ({ mutate: vi.fn(), isPending: false }), + useRemoveReactionMutation: () => ({ mutate: vi.fn(), isPending: false }), + useReplyToCommentMutation: () => ({ mutate: vi.fn(), isPending: false }), + useResolveThreadMutation: () => ({ mutate: resolveMutate, isPending: false }), + useUnresolveThreadMutation: () => ({ mutate: vi.fn(), isPending: false }), +})); + +function countByType( + root: TestRenderer.ReactTestInstance, + type: string, + match?: (props: Record) => boolean +): number { + return root.findAll( + node => + typeof node.type === 'string' && + (node.type as string) === type && + (match === undefined || match(node.props as Record)) + ).length; +} + +async function renderThread( + ref: ProviderPrRef | null, + expanded: boolean, + thread: ReviewThread +): Promise { + const card = createElement(DiscussionThread, { ...baseProps, thread, expanded }); + const tree = ref ? ( + {card} + ) : ( + card + ); + let renderer: TestRenderer.ReactTestRenderer | null = null; + await act(async () => { + await Promise.resolve(); + renderer = TestRenderer.create(tree); + }); + // Runtime safety: act() could theoretically fail without assigning. + // eslint-disable-next-line typescript-eslint/no-unnecessary-condition + if (!renderer) { + throw new Error('Failed to create test renderer'); + } + return renderer; +} + +function pressableCount(root: TestRenderer.ReactTestInstance, label: string): number { + return countByType( + root, + 'Pressable', + props => props.accessibilityLabel === label || props.children === label + ); +} + +/** Press the resolve toggle (its own nested pressable in the header). */ +function pressResolve(root: TestRenderer.ReactTestInstance): void { + const resolveToggle = root.findAll( + node => + typeof node.type === 'string' && + (node.type as string) === 'Pressable' && + (node.props as Record).accessibilityLabel === 'Resolve thread' + )[0]; + if (!resolveToggle) { + throw new Error('Resolve toggle not found'); + } + (resolveToggle.props as { onPress?: () => void }).onPress?.(); +} + +describe('DiscussionThread provider write arm (s6)', () => { + it.each<[string, ProviderPrRef]>([ + ['gitlab', GITLAB_REF], + ['bitbucket', BITBUCKET_REF], + ])('offers resolve and reply through the seam on a %s thread', async (_platform, ref) => { + const renderer = await renderThread(ref, true, makeThread()); + try { + // Resolve is offered and carries the provider-native thread id. + expect(pressableCount(renderer.root, 'Resolve thread')).toBe(1); + pressResolve(renderer.root); + expect(resolveMutate).toHaveBeenCalledWith({ threadId: 'D-12' }); + + // Reply is offered with the provider target (discussion id + verbatim + // comment id), so the seam posts it for this provider identity. + expect(countByType(renderer.root, 'ReplyInput')).toBe(1); + const replyInput = renderer.root.find( + node => typeof node.type === 'string' && (node.type as string) === 'ReplyInput' + ); + expect(replyInput.props.provider).toEqual({ + ref, + threadId: 'D-12', + commentNodeId: '12', + }); + + expect(countByType(renderer.root, 'CommentRow')).toBe(1); + } finally { + renderer.unmount(); + } + }); + + it('makes thread comments read-only on a provider scope (no reaction write path)', async () => { + const renderer = await renderThread(GITLAB_REF, true, makeThread()); + try { + const row = renderer.root.find( + node => typeof node.type === 'string' && (node.type as string) === 'CommentRow' + ); + expect(row.props.readOnly).toBe(true); + // GitLab's capability says reactions are supported, so the row is + // gated on the flag (true) — but it is read-only and the read layer + // returns no reaction data, so nothing renders. + expect(row.props.reactionsSupported).toBe(true); + } finally { + renderer.unmount(); + } + }); + + it('drops the reaction row entirely on a Bitbucket thread (capability unsupported)', async () => { + const renderer = await renderThread(BITBUCKET_REF, true, makeThread()); + try { + const row = renderer.root.find( + node => typeof node.type === 'string' && (node.type as string) === 'CommentRow' + ); + expect(row.props.reactionsSupported).toBe(false); + expect(row.props.readOnly).toBe(true); + } finally { + renderer.unmount(); + } + }); + + it('keeps the resolve toggle, reply input and writable reactions on GitHub', async () => { + const renderer = await renderThread(null, true, makeThread()); + try { + expect(pressableCount(renderer.root, 'Resolve thread')).toBe(1); + expect(countByType(renderer.root, 'ReplyInput')).toBe(1); + const replyInput = renderer.root.find( + node => typeof node.type === 'string' && (node.type as string) === 'ReplyInput' + ); + // GitHub carries no provider target — the exact pre-s6 call shape. + expect(replyInput.props.provider).toBeUndefined(); + const row = renderer.root.find( + node => typeof node.type === 'string' && (node.type as string) === 'CommentRow' + ); + expect(row.props.readOnly).toBe(false); + } finally { + renderer.unmount(); + } + }); + + it('keeps the read-only Resolved badge on a provider thread', async () => { + const renderer = await renderThread(GITLAB_REF, false, makeThread({ isResolved: true })); + try { + // Collapsed provider thread: the unresolve affordance is offered + // through the seam, and the read-only Resolved badge stays. + expect(pressableCount(renderer.root, 'Unresolve thread')).toBe(1); + expect(countByType(renderer.root, 'Text', props => props.children === 'Resolved')).toBe(1); + } finally { + renderer.unmount(); + } + }); +}); diff --git a/apps/mobile/src/components/pr-review/discussion/discussion-thread.tsx b/apps/mobile/src/components/pr-review/discussion/discussion-thread.tsx index c6d172fc45..c8f24d0403 100644 --- a/apps/mobile/src/components/pr-review/discussion/discussion-thread.tsx +++ b/apps/mobile/src/components/pr-review/discussion/discussion-thread.tsx @@ -55,6 +55,7 @@ import { useResolveThreadMutation, useUnresolveThreadMutation, } from '@/lib/pr-review/discussion/use-review-discussion-mutations'; +import { providerPrCapabilities, useProviderPrScope } from '@/lib/pr-review/provider-pr-ref'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; import { cn, parseTimestamp, timeAgo } from '@/lib/utils'; @@ -79,6 +80,19 @@ export function DiscussionThread({ onToggleExpand, viewerLogin = null, }: Readonly) { + // s6: the reply and resolve writes route through the `providerReview` seam + // on a GitLab MR / Bitbucket PR (the mutation hooks pick the arm from the + // provider scope the layout publishes), so what this card offers is decided + // by the capability list, not by the platform. Reactions stay behind the + // GitHub write path: the seam has no reaction procedure and no provider + // read layer returns reaction data, so a provider comment row renders + // read-only — the row shows nothing rather than a dead or failing + // affordance. + const scope = useProviderPrScope({ owner, repo, number }); + const capabilities = providerPrCapabilities(scope.ref.platform); + const isGithub = scope.ref.platform === 'github'; + const canReply = capabilities.canComment; + const canResolve = capabilities.canResolveThreads; const resolve = useResolveThreadMutation(); const unresolve = useUnresolveThreadMutation(); const addReaction = useAddReactionMutation(thread.threadId); @@ -131,11 +145,23 @@ export function DiscussionThread({ firstTimestamp: firstComment?.createdAt ?? null, expanded, onToggleResolve, + canResolve, resolveDisabled: isResolving, onToggleExpand, } as const; if (expanded) { + // The provider reply target carries the provider-native ids the seam + // needs: the thread's discussion/root-comment id and the first comment's + // verbatim provider id (kept in `nodeId` by the read layer). + const providerReply = + !isGithub && firstComment + ? { + ref: scope.ref, + threadId: thread.threadId, + commentNodeId: firstComment.nodeId, + } + : undefined; return ( { onToggleReaction(comment, content); @@ -157,13 +185,14 @@ export function DiscussionThread({ ))} - {firstComment ? ( + {firstComment && canReply ? ( ) : null} @@ -195,6 +224,8 @@ type ThreadHeaderProps = { readonly expanded: boolean; readonly onToggleExpand: () => void; readonly onToggleResolve: () => void; + /** False on a GitLab/Bitbucket scope: the resolve control is withheld. */ + readonly canResolve: boolean; readonly resolveDisabled: boolean; }; @@ -208,6 +239,7 @@ function ThreadHeader({ expanded, onToggleExpand, onToggleResolve, + canResolve, resolveDisabled, }: Readonly) { const colors = useThemeColors(); @@ -237,7 +269,9 @@ function ThreadHeader({ {anchorLabel} - + {canResolve ? ( + + ) : null} {resolved ? ( diff --git a/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.mounted.test.tsx b/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.mounted.test.tsx new file mode 100644 index 0000000000..e29a6d8fa7 --- /dev/null +++ b/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.mounted.test.tsx @@ -0,0 +1,162 @@ +/* eslint-disable typescript-eslint/no-deprecated -- react-test-renderer is the repository's native-free mounted test tool. */ +import { createElement, Fragment, type ReactNode } from 'react'; +import TestRenderer, { act } from 'react-test-renderer'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { type DiscussionListItem } from '@/lib/pr-review/discussion/review-discussion-types'; +import { ProviderPrScopeProvider } from '@/lib/pr-review/provider-pr-ref'; +import { PrReviewDiscussionList } from './pr-review-discussion-list'; + +type TaggedOptions = { tag: string; input?: unknown }; + +const observed = vi.hoisted(() => ({ options: [] as TaggedOptions[] })); + +// Records every query the list mounts and answers the overview with a viewer +// login, so a test can assert BOTH which namespace was asked and what the +// answer drives. +vi.mock('@tanstack/react-query', () => ({ + useQuery: (options: TaggedOptions) => { + observed.options.push(options); + if (options.tag === 'moderation') { + return { data: { blockedLogins: [], mutedLogins: [] } }; + } + return { data: { repo: { viewerLogin: 'octocat' } } }; + }, +})); +vi.mock('@/lib/trpc', () => ({ + useTRPC: () => ({ + moderation: { listHiddenUsers: { queryOptions: () => ({ tag: 'moderation' }) } }, + githubPrReview: { + getPullRequest: { + queryOptions: (input: unknown) => ({ tag: 'githubPrReview.getPullRequest', input }), + }, + }, + providerReview: { + getPullRequest: { + queryOptions: (input: unknown) => ({ tag: 'providerReview.getPullRequest', input }), + }, + }, + }), +})); +vi.mock('react-i18next', () => ({ useTranslation: () => ({ t: (key: string) => key }) })); +vi.mock('react-native', () => ({ View: 'View' })); +vi.mock('@shopify/flash-list', () => ({ + FlashList: ({ + data, + renderItem, + }: { + data: readonly unknown[]; + renderItem: (args: { item: unknown; index: number }) => ReactNode; + }) => + createElement( + Fragment, + null, + data.map((item, index) => + createElement(Fragment, { key: index }, renderItem({ item, index })) + ) + ), +})); +vi.mock('@/components/pr-review/discussion/comment-row', () => ({ CommentRow: 'CommentRow' })); +vi.mock('@/components/pr-review/discussion/discussion-thread', () => ({ + DiscussionThread: 'DiscussionThread', +})); +vi.mock('@/components/ui/button', () => ({ Button: 'Button' })); +vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); +vi.mock('@/lib/screen-insets', () => ({ useDetailScreenBottomPadding: () => 0 })); + +function noop(): void { + // The viewer query is what this file asserts; the list callbacks are inert. +} + +const listItems: readonly DiscussionListItem[] = [ + { + kind: 'comment', + comment: { + commentId: 1, + nodeId: 'c1', + author: { login: 'octocat', avatarUrl: null }, + bodyMarkdown: 'hello', + createdAt: '2026-01-01T00:00:00Z', + reactions: [], + }, + }, +]; + +function mountList(scope?: { + ref: { platform: 'gitlab'; projectPath: string; mrIid: number }; + organizationId: string | null; +}) { + const list = ( + + ); + const created: { current: TestRenderer.ReactTestRenderer | undefined } = { current: undefined }; + act(() => { + created.current = TestRenderer.create( + scope ? {list} : list + ); + }); + const renderer = created.current; + if (!renderer) { + throw new Error('renderer was not created'); + } + return renderer; +} + +describe('PrReviewDiscussionList viewer query', () => { + beforeEach(() => { + observed.options = []; + }); + + it('reads the viewer login from the GitHub procedure on a GitHub scope', () => { + const renderer = mountList(); + + expect(observed.options.map(options => options.tag)).toEqual([ + 'moderation', + 'githubPrReview.getPullRequest', + ]); + expect(observed.options[1]?.input).toEqual({ owner: 'group/sub', repo: 'repo', number: 12 }); + expect( + renderer.root.findAll(node => String(node.type) === 'CommentRow')[0]?.props.viewerLogin + ).toBe('octocat'); + + act(() => { + renderer.unmount(); + }); + }); + + it('never fires the GitHub procedure under a GitLab scope', () => { + const renderer = mountList({ + ref: { platform: 'gitlab', projectPath: 'group/sub/repo', mrIid: 12 }, + organizationId: null, + }); + + expect(observed.options.map(options => options.tag)).toEqual([ + 'moderation', + 'providerReview.getPullRequest', + ]); + expect(observed.options[1]?.input).toMatchObject({ + platform: 'gitlab', + projectPath: 'group/sub/repo', + mrIid: 12, + }); + + act(() => { + renderer.unmount(); + }); + }); +}); diff --git a/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.tsx b/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.tsx index 88fdca7364..61e28a5d96 100644 --- a/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.tsx +++ b/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.tsx @@ -17,6 +17,7 @@ import { type ReviewThread, } from '@/lib/pr-review/discussion/review-discussion-types'; import { expandedForThread } from '@/lib/pr-review/discussion/thread-expansion'; +import { useProviderPrQueries } from '@/lib/pr-review/provider-pr-queries'; import { useDetailScreenBottomPadding } from '@/lib/screen-insets'; import { useTRPC } from '@/lib/trpc'; @@ -61,8 +62,14 @@ export function PrReviewDiscussionList({ const trpc = useTRPC(); // Account-local hidden users (blocked + muted GitHub logins) filter rows. const hiddenUsers = useQuery(trpc.moderation.listHiddenUsers.queryOptions()); - // Viewer login for self-target gating on the comment overflow menu. - const pr = useQuery(trpc.githubPrReview.getPullRequest.queryOptions({ owner, repo, number })); + // Viewer login for self-target gating on the comment overflow menu. The + // overview goes through the provider seam, not `githubPrReview` directly: + // this list also renders under a GitLab MR / Bitbucket PR scope, where the + // GitHub-shaped triple is a synthesized stand-in and a GitHub call with it + // would fail on every render. On GitHub the key is unchanged, so this + // still dedupes with the screen's own overview query. + const queries = useProviderPrQueries({ owner, repo, number }); + const pr = useQuery(queries.overviewOptions()); const viewerLogin = pr.data?.repo.viewerLogin ?? null; const hiddenLogins = useMemo(() => { @@ -118,6 +125,9 @@ export function PrReviewDiscussionList({ diff --git a/apps/mobile/src/components/pr-review/discussion/reply-input.test.ts b/apps/mobile/src/components/pr-review/discussion/reply-input.test.ts index 1862348e6b..6eba07309b 100644 --- a/apps/mobile/src/components/pr-review/discussion/reply-input.test.ts +++ b/apps/mobile/src/components/pr-review/discussion/reply-input.test.ts @@ -18,6 +18,7 @@ import type * as ReactI18next from 'react-i18next'; import { ensureTermsAcceptedOutcome, ReplyInput } from './reply-input'; import { clearDraft } from '@/lib/persist/drafts'; import { type useReplyToCommentMutation } from '@/lib/pr-review/discussion/use-review-discussion-mutations'; +import { type ProviderPrRef, providerPrRefKey } from '@/lib/pr-review/provider-pr-ref'; vi.mock('react-i18next', async importOriginal => { const actual = await importOriginal(); @@ -111,11 +112,21 @@ vi.mock('@/lib/hooks/use-current-user-id', () => ({ // the React hook primitives are stubbed to no-op/simple versions, mirroring // pr-merge-sheet.test.tsx. The pure `ensureTermsAcceptedOutcome` tests above // do not touch these. +// +// The useState mock records every setter it hands out: the error effect +// writes its inline copy through one of them, so a test can observe the +// state write even though the no-op mock never re-renders. +const stateSetters = vi.hoisted(() => [] as { mock: { calls: unknown[][] } }[]); + vi.mock('react', async () => { const actual = await vi.importActual('react'); return { ...actual, - useState: vi.fn((initial: T) => [initial, vi.fn() as () => void] as [T, (value: T) => void]), + useState: vi.fn((initial: T) => { + const setter = vi.fn(); + stateSetters.push(setter); + return [initial, setter as () => void] as [T, (value: T) => void]; + }), useMemo: vi.fn((factory: () => T) => factory()), useRef: vi.fn((initial: T) => { const ref: React.RefObject = { current: initial }; @@ -418,6 +429,82 @@ describe('ReplyInput seeds the field from the settled draft during render', () = }); }); +// ── Provider arm (s6) ──────────────────────────────────────────────── + +const GITLAB_REF: ProviderPrRef = { platform: 'gitlab', projectPath: 'octocat/hello', mrIid: 1 }; + +describe('ReplyInput provider arm (s6)', () => { + beforeEach(() => { + alertCalls.length = 0; + getTermsStatusMock.mockReset(); + acceptTermsMock.mockReset(); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + function mountProviderReply(mutate: unknown): void { + // eslint-disable-next-line new-cap + const element = ReplyInput({ + owner: 'octocat', + repo: 'hello', + number: 1, + commentId: 42, + reply: makeReply(mutate), + provider: { ref: GITLAB_REF, threadId: 'D-77', commentNodeId: '9001' }, + }); + const input = findElement({ + node: element, + type: 'TextInput', + prop: 'accessibilityLabel', + value: 'Reply body', + }); + if (!input) { + throw new Error('Reply body TextInput not found'); + } + (input.props as { onChangeText?: (value: string) => void }).onChangeText?.('hello'); + const button = findElement({ + node: element, + type: 'Button', + prop: 'accessibilityLabel', + value: 'Submit reply', + }); + if (!button) { + throw new Error('Submit reply Button not found'); + } + (button.props as { onPress?: () => void }).onPress?.(); + } + + it('posts the seam vars (provider-native ids), never the GitHub-shaped input', async () => { + getTermsStatusMock.mockResolvedValue({ accepted: true, currentVersion: 'v1' }); + const mutate = vi.fn((_input: unknown, options: { onSuccess?: () => void }) => { + options.onSuccess?.(); + }); + mountProviderReply(mutate); + await flush(); + + expect(mutate).toHaveBeenCalledWith( + { threadId: 'D-77', commentNodeId: '9001', body: 'hello' }, + expect.anything() + ); + }); + + it('folds the provider ref identity into the durable reply draft key', async () => { + getTermsStatusMock.mockResolvedValue({ accepted: true, currentVersion: 'v1' }); + const mutate = vi.fn((_input: unknown, options: { onSuccess?: () => void }) => { + options.onSuccess?.(); + }); + mountProviderReply(mutate); + await flush(); + + // The mocked prReplyDraftKey answers 'pr-reply:key'; the provider arm + // appends the collision-free ref identity (identity rule 17) so a + // same-numbered GitHub PR can never share this reply's draft. + expect(clearDraft).toHaveBeenCalledWith('u1', `pr-reply:key@${providerPrRefKey(GITLAB_REF)}`); + }); +}); + describe('ReplyInput gates input on draft settle', () => { it('hides the input and disables submit until the draft settles', () => { draftLoadMock.mockReturnValue({ settled: false, value: null }); @@ -467,3 +554,63 @@ describe('ReplyInput gates input on draft settle', () => { ).not.toBeNull(); }); }); + +// ── s6f: refused-reply wording ─────────────────────────────────────── + +/** True when the error effect wrote `value` into any state slot. */ +function stateValueWritten(value: string): boolean { + return stateSetters.some(setter => setter.mock.calls.some(call => call[0] === value)); +} + +function forbiddenError(): Error { + return Object.assign(new Error('403 Forbidden'), { data: { code: 'FORBIDDEN' } }); +} + +describe('ReplyInput refused-reply wording (s6f)', () => { + beforeEach(() => { + stateSetters.length = 0; + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + function mountWithForbiddenError(provider?: { + ref: ProviderPrRef; + threadId: string; + commentNodeId: string; + }): void { + const errored = { + mutate: vi.fn(), + isPending: false, + error: forbiddenError(), + } as unknown as ReplyMutation; + // eslint-disable-next-line new-cap + ReplyInput({ + owner: 'octocat', + repo: 'hello', + number: 1, + commentId: 42, + reply: errored, + provider, + }); + } + + it('words a refused provider reply after the merge-request noun', () => { + mountWithForbiddenError({ ref: GITLAB_REF, threadId: 'D-77', commentNodeId: '9001' }); + // The provider 403 must never read "pull request" on a merge request. + expect(stateValueWritten("You don't have permission to reply to this merge request.")).toBe( + true + ); + expect(stateValueWritten("You don't have permission to reply to this pull request.")).toBe( + false + ); + }); + + it('keeps the exact pre-s6 forbidden copy on the GitHub arm', () => { + mountWithForbiddenError(); + expect(stateValueWritten("You don't have permission to reply to this pull request.")).toBe( + true + ); + }); +}); diff --git a/apps/mobile/src/components/pr-review/discussion/reply-input.tsx b/apps/mobile/src/components/pr-review/discussion/reply-input.tsx index 3d6902ddee..64ac0e6b33 100644 --- a/apps/mobile/src/components/pr-review/discussion/reply-input.tsx +++ b/apps/mobile/src/components/pr-review/discussion/reply-input.tsx @@ -10,6 +10,7 @@ import * as WebBrowser from 'expo-web-browser'; import { UGC_AGE_POSTURE } from '@kilocode/app-shared/moderation'; import { PrReviewReconnectNotice } from '@/components/pr-review/pr-review-reconnect-notice'; +import { providerPrNounKey } from '@/components/pr-review/pr-review-provider-noun'; import { Button } from '@/components/ui/button'; import { Text } from '@/components/ui/text'; import { i18n } from '@/i18n'; @@ -22,6 +23,7 @@ import { useFencedDraftLoad } from '@/lib/persist/use-draft-load'; import { classifyPrReviewMutationError } from '@/lib/pr-review/classify-pr-review-query-state'; import { type useReplyToCommentMutation } from '@/lib/pr-review/discussion/use-review-discussion-mutations'; import { isPrOperationPersistenceFailed } from '@/lib/pr-review/merge/pr-operation-ledger'; +import { type ProviderPrRef, providerPrRefKey } from '@/lib/pr-review/provider-pr-ref'; import { trpcClient } from '@/lib/trpc'; /** @@ -139,9 +141,32 @@ type ReplyInputProps = { readonly number: number; readonly commentId: number; readonly reply: ReturnType; + /** + * The provider arm (s6). Present on a GitLab MR / Bitbucket PR thread: + * the reply posts `{ threadId, commentNodeId, body }` through the + * `providerReview` seam (GitLab answers inside the discussion, Bitbucket + * attaches to the root comment), and the durable draft key folds the + * collision-free ref identity so the same-numbered PR on another provider + * can never share this reply's draft (identity rule 17). Absent on GitHub, + * which keeps the exact pre-s6 call and key bytes. + */ + readonly provider?: + | { + readonly ref: ProviderPrRef; + readonly threadId: string; + readonly commentNodeId: string; + } + | undefined; }; -export function ReplyInput({ owner, repo, number, commentId, reply }: Readonly) { +export function ReplyInput({ + owner, + repo, + number, + commentId, + reply, + provider, +}: Readonly) { const colors = useThemeColors(); const { t } = useTranslation(); const bodyRef = useRef(''); @@ -151,11 +176,19 @@ export function ReplyInput({ owner, repo, number, commentId, reply }: Readonly(null); const [resetKey, setResetKey] = useState(0); + // The provider platform as a stable primitive: the error effect words a + // refusal after it without depending on the `provider` object identity. + const providerPlatform = provider?.ref.platform; // Durable reply draft, keyed by account and thread. Nothing is saved or // restored while the user id is unknown. const { userId, isLoading: isIdentityLoading } = useCurrentUserId(); - const replyDraftKey = prReplyDraftKey(owner, repo, number, commentId); + const positionReplyDraftKey = prReplyDraftKey(owner, repo, number, commentId); + // Provider arms fold the collision-free ref identity into the key (identity + // rule 17); the GitHub bytes stay exactly as stored before this slice. + const replyDraftKey = provider + ? `${positionReplyDraftKey}@${providerPrRefKey(provider.ref)}` + : positionReplyDraftKey; const draft = useFencedDraftLoad({ userId, isIdentityLoading, entityKey: replyDraftKey }); useDraftFlushOnBackground(userId, replyDraftKey, true); @@ -205,7 +238,15 @@ export function ReplyInput({ owner, repo, number, commentId, reply }: Readonly { const body = bodyRef.current.trim(); @@ -238,7 +279,12 @@ export function ReplyInput({ owner, repo, number, commentId, reply }: Readonly { bodyRef.current = ''; diff --git a/apps/mobile/src/components/pr-review/full-surface-states.mounted.test.tsx b/apps/mobile/src/components/pr-review/full-surface-states.mounted.test.tsx index c783ff16c5..84a4167f54 100644 --- a/apps/mobile/src/components/pr-review/full-surface-states.mounted.test.tsx +++ b/apps/mobile/src/components/pr-review/full-surface-states.mounted.test.tsx @@ -24,16 +24,22 @@ vi.mock('@tanstack/react-query', async importOriginal => ({ ...(await importOriginal()), useQuery: () => query, })); -vi.mock('expo-router', () => ({ - useRouter: () => ({ back: vi.fn(), push: vi.fn() }), - useLocalSearchParams: () => ({ +// Mutable so one suite can hand the composer route a malformed param set +// without re-mocking `expo-router` per test. +const routeParams = vi.hoisted(() => { + const current: Record = { owner: 'org', repo: 'repo', number: '1', path: 'src/a.ts', line: '1', side: 'RIGHT', - }), + }; + return { current }; +}); +vi.mock('expo-router', () => ({ + useRouter: () => ({ back: vi.fn(), push: vi.fn(), replace: vi.fn() }), + useLocalSearchParams: () => routeParams.current, })); vi.mock('react-native', () => ({ View: 'View', @@ -63,6 +69,9 @@ vi.mock('@/components/pr-review/diff/pr-diff-file-navigator', () => ({ vi.mock('@/components/pr-review/merge/pr-merge-section', () => ({ PrMergeSection: 'PrMergeSection', })); +vi.mock('@/components/pr-review/merge/pr-merge-section-provider', () => ({ + PrMergeSectionProvider: 'PrMergeSectionProvider', +})); vi.mock('@/components/pr-review/pr-review-checks-section', () => ({ PrReviewChecksSection: 'PrReviewChecksSection', })); @@ -102,6 +111,13 @@ vi.mock('@/lib/trpc', () => ({ useTRPC: () => ({ githubPrReview: { getPullRequest: { queryOptions: () => ({}) } }, githubApps: { getUserAuthorization: { queryKey: () => [] } }, + // s6: the submit/merge screens build the provider seam's capability and + // merge-state options even on the GitHub arm (the queries register + // disabled and never fetch), so the router mock carries the namespace. + providerReview: { + getCapabilities: { queryOptions: () => ({}) }, + getMergeState: { queryOptions: () => ({}) }, + }, }), })); @@ -110,6 +126,14 @@ beforeEach(() => { query.isError = true; query.isLoading = false; query.error.data.code = 'INTERNAL_SERVER_ERROR'; + routeParams.current = { + owner: 'org', + repo: 'repo', + number: '1', + path: 'src/a.ts', + line: '1', + side: 'RIGHT', + }; vi.clearAllMocks(); }); @@ -199,3 +223,22 @@ describe.each([ unmount(); }); }); + +describe('composer malformed route', () => { + it('renders the terminal invalid state without the Add-comment chrome', async () => { + // A hand-built or restored composer link with no valid comment target + // failed at the ROUTE, not at the comment: the sheet must not announce + // "Add comment" over a "Page not found" body. + routeParams.current = { owner: 'org', repo: 'repo', number: '1' }; + const { renderer, unmount } = await renderWithProviders( + createElement(PrReviewCommentComposerScreen) + ); + expect(renderer.root.findAll(node => String(node.type) === 'InvalidRouteState')).toHaveLength( + 1 + ); + expect(renderer.root.findAll(node => String(node.type) === 'PrFormSheetHeader')).toHaveLength( + 0 + ); + unmount(); + }); +}); diff --git a/apps/mobile/src/components/pr-review/merge/pr-merge-section-parts.tsx b/apps/mobile/src/components/pr-review/merge/pr-merge-section-parts.tsx index 4c6539c888..e8042acbc3 100644 --- a/apps/mobile/src/components/pr-review/merge/pr-merge-section-parts.tsx +++ b/apps/mobile/src/components/pr-review/merge/pr-merge-section-parts.tsx @@ -16,12 +16,40 @@ import { type PrOverviewDto, } from '@/lib/pr-review/merge/merge-blocked-reasons'; -export function TerminalChip({ state }: Readonly<{ state: PrOverviewDto['state'] }>) { +// Mid-sentence lowercase noun (common.*) vs the capitalized standalone +// label (prReview.terms.*) — see providerPrTermKey. +// i18n-dup-ok: one copy, two senses: mid-sentence lowercase noun vs +// capitalized standalone label; languages case-decline them apart. +type TerminalNounKey = 'common.mergeRequest' | 'common.pullRequest'; + +function terminalLabelKey( + state: PrOverviewDto['state'], + nounKey: TerminalNounKey +): + | 'prReview.merge.terminal.alreadyMerged' + | 'prReview.merge.terminal.closedMergeRequest' + | 'prReview.merge.terminal.closed' { + if (state === 'merged') { + return 'prReview.merge.terminal.alreadyMerged'; + } + return nounKey === 'common.mergeRequest' + ? 'prReview.merge.terminal.closedMergeRequest' + : 'prReview.merge.terminal.closed'; +} + +export function TerminalChip({ + state, + nounKey = 'common.pullRequest', +}: Readonly<{ + state: PrOverviewDto['state']; + /** + * The provider's own noun for the closed sentence (s6): a GitLab merge + * request says "merge request", GitHub and Bitbucket say "pull request". + */ + nounKey?: TerminalNounKey; +}>) { const { t } = useTranslation(); - const label = - state === 'merged' - ? t('prReview.merge.terminal.alreadyMerged') - : t('prReview.merge.terminal.closed'); + const label = t(terminalLabelKey(state, nounKey)); return ( diff --git a/apps/mobile/src/components/pr-review/merge/pr-merge-section-provider.test.tsx b/apps/mobile/src/components/pr-review/merge/pr-merge-section-provider.test.tsx new file mode 100644 index 0000000000..2f07d94580 --- /dev/null +++ b/apps/mobile/src/components/pr-review/merge/pr-merge-section-provider.test.tsx @@ -0,0 +1,166 @@ +/* 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 comment-row.test.tsx) */ +// The provider merge section (s6): the overview's merge affordance on a +// GitLab MR / Bitbucket PR. The merge CTA always pushes the ref's own sheet +// route (the sheet renders the s2/s3 restrictions); the auto-merge row +// follows the capability list — GitLab gets the enable CTA, Bitbucket gets +// the explicit capability banner instead of a dead button or a silent +// absence. A terminal PR renders the terminal chip and no CTAs. + +import * as React from 'react'; +import { createElement } from 'react'; +import TestRenderer, { act } from 'react-test-renderer'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import '@/i18n'; + +import { PrMergeSectionProvider } from './pr-merge-section-provider'; +import { type ProviderPrRef } from '@/lib/pr-review/provider-pr-ref'; + +const routerPush = vi.fn(); + +vi.mock('expo-router', () => ({ + useRouter: () => ({ push: routerPush }), +})); + +vi.mock('react-native', () => ({ + View: 'View', + ActivityIndicator: 'ActivityIndicator', +})); + +vi.mock('@/components/ui/icons', () => ({ + AlertTriangle: 'AlertTriangle', + GitBranch: 'GitBranch', + GitMerge: 'GitMerge', + GitPullRequest: 'GitPullRequest', + RefreshCw: 'RefreshCw', + ShieldAlert: 'ShieldAlert', + XCircle: 'XCircle', +})); + +vi.mock('@/components/ui/button', () => ({ Button: 'Button' })); +vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); +// The section parts render the UI spinner; the real one reaches the motion +// policy (expo-battery), which stays unmocked in this pure harness. +vi.mock('@/components/ui/activity-indicator', () => ({ ActivityIndicator: 'ActivityIndicator' })); + +vi.mock('@/lib/hooks/use-theme-colors', () => ({ + useThemeColors: () => ({ + primaryForeground: '#FFFFFF', + foreground: '#000000', + mutedForeground: '#6F6A61', + destructive: '#DC2626', + }), +})); + +const GITLAB_REF: ProviderPrRef = { + platform: 'gitlab', + projectPath: 'group/sub/repo', + mrIid: 12, +}; +const BITBUCKET_REF: ProviderPrRef = { + platform: 'bitbucket', + workspace: 'acme', + repoSlug: 'api', + prId: 42, +}; + +async function mount( + prRef: ProviderPrRef, + state: 'open' | 'closed' | 'merged' +): Promise { + let renderer: TestRenderer.ReactTestRenderer | null = null; + await act(async () => { + await Promise.resolve(); + renderer = TestRenderer.create(createElement(PrMergeSectionProvider, { prRef, state })); + }); + // eslint-disable-next-line typescript-eslint/no-unnecessary-condition -- the closure assignment cannot cross into TS's narrow + if (!renderer) { + throw new Error('renderer was not created'); + } + return renderer; +} + +function findButtons(renderer: TestRenderer.ReactTestRenderer, label: string): number { + return renderer.root.findAll( + node => + String(node.type) === 'Button' && + (node.props as Record).accessibilityLabel === label + ).length; +} + +function findButton(renderer: TestRenderer.ReactTestRenderer, label: string) { + const button = renderer.root.find( + node => + String(node.type) === 'Button' && + (node.props as Record).accessibilityLabel === label + ); + return (button.props as { onPress?: () => void }).onPress; +} + +describe('PrMergeSectionProvider (s6)', () => { + beforeEach(() => { + routerPush.mockClear(); + }); + + it('offers merge and enable-auto-merge on a GitLab merge request (capability supported)', async () => { + const renderer = await mount(GITLAB_REF, 'open'); + expect(findButtons(renderer, 'Merge merge request')).toBe(1); + expect(findButtons(renderer, 'Enable auto-merge')).toBe(1); + renderer.unmount(); + }); + + it('offers merge with the explicit capability banner on Bitbucket (auto-merge unsupported)', async () => { + const renderer = await mount(BITBUCKET_REF, 'open'); + expect(findButtons(renderer, 'Merge pull request')).toBe(1); + expect(findButtons(renderer, 'Enable auto-merge')).toBe(0); + const banner = renderer.root.find( + node => + typeof node.type === 'function' && + (node.type as { name?: string }).name === 'PrReviewCapabilityBanner' + ); + expect( + (banner.props as { capability: { supported: boolean; reason: string } }).capability + ).toEqual({ + supported: false, + reason: 'Bitbucket Cloud does not expose auto-merge in its API', + }); + renderer.unmount(); + }); + + it('pushes the merge sheet inside the GitLab ref route on press', async () => { + const renderer = await mount(GITLAB_REF, 'open'); + act(() => { + findButton(renderer, 'Merge merge request')?.(); + }); + expect(routerPush).toHaveBeenCalledWith( + '/(app)/pr-review/gitlab/group/sub/repo/12/merge?mode=merge' + ); + renderer.unmount(); + }); + + it('pushes the auto-merge arm inside the ref route on the enable CTA', async () => { + const renderer = await mount(GITLAB_REF, 'open'); + act(() => { + findButton(renderer, 'Enable auto-merge')?.(); + }); + expect(routerPush).toHaveBeenCalledWith( + '/(app)/pr-review/gitlab/group/sub/repo/12/merge?mode=enable-auto-merge' + ); + renderer.unmount(); + }); + + it.each<['closed' | 'merged', string]>([ + ['merged', 'Already merged'], + ['closed', 'This merge request is closed'], + ])( + 'renders only the terminal chip with provider wording on a %s GitLab merge request', + async (state, label) => { + const renderer = await mount(GITLAB_REF, state); + expect(renderer.root.findAll(node => String(node.type) === 'Button')).toHaveLength(0); + expect( + renderer.root.findAll(node => String(node.type) === 'Text' && node.props.children === label) + ).toHaveLength(1); + renderer.unmount(); + } + ); +}); diff --git a/apps/mobile/src/components/pr-review/merge/pr-merge-section-provider.tsx b/apps/mobile/src/components/pr-review/merge/pr-merge-section-provider.tsx new file mode 100644 index 0000000000..cdd9093068 --- /dev/null +++ b/apps/mobile/src/components/pr-review/merge/pr-merge-section-provider.tsx @@ -0,0 +1,82 @@ +// The provider merge section (s6). The GitHub section derives its gate from +// the GitHub overview DTO; a GitLab MR / Bitbucket PR normalizes `mergeable` +// to null, so this arm offers the merge affordance directly and lets the +// confirmation sheet — which reads `providerReview.getMergeState` — render +// the restrictions list and refuse the submit. Auto-merge follows the +// capability list: GitLab (supported) gets the enable CTA, Bitbucket gets +// the explicit capability banner with the provider's reason — never a dead +// button and never a silent absence. + +import { useRouter } from 'expo-router'; +import { useTranslation } from 'react-i18next'; +import { View } from 'react-native'; + +import { PrReviewCapabilityBanner } from '@/components/pr-review/pr-review-capability-banner'; +import { TerminalChip } from '@/components/pr-review/merge/pr-merge-section-parts'; +import { providerPrNounKey } from '@/components/pr-review/pr-review-provider-noun'; +import { providerPrSheetHref } from '@/components/pr-review/pr-review-provider-sheet-href'; +import { Button } from '@/components/ui/button'; +import { GitMerge } from '@/components/ui/icons'; +import { Text } from '@/components/ui/text'; +import { useThemeColors } from '@/lib/hooks/use-theme-colors'; +import { providerPrCapabilities, type ProviderPrRef } from '@/lib/pr-review/provider-pr-ref'; + +type PrMergeSectionProviderProps = Readonly<{ + /** The provider ref the section pushes its sheet route under. */ + prRef: ProviderPrRef; + /** The overview lifecycle state; `open` is the only mergeable one. */ + state: 'open' | 'closed' | 'merged'; +}>; + +export function PrMergeSectionProvider({ prRef, state }: PrMergeSectionProviderProps) { + const router = useRouter(); + const colors = useThemeColors(); + const { t } = useTranslation(); + + if (state !== 'open') { + // Provider wording (s6): a closed GitLab merge request says "merge + // request"; Bitbucket keeps "pull request" — that is its own noun. + return ; + } + + const autoMerge = providerPrCapabilities(prRef.platform).autoMerge; + const mergeLabel = t('prReview.merge.mergeTermTitle', { + term: t(providerPrNounKey(prRef.platform)), + }); + + return ( + + + {t('prReview.merge.merge')} + + + {autoMerge.supported ? ( + + ) : ( + + )} + + ); +} diff --git a/apps/mobile/src/components/pr-review/merge/pr-merge-sheet-parts.test.tsx b/apps/mobile/src/components/pr-review/merge/pr-merge-sheet-parts.test.tsx new file mode 100644 index 0000000000..e73c0272cf --- /dev/null +++ b/apps/mobile/src/components/pr-review/merge/pr-merge-sheet-parts.test.tsx @@ -0,0 +1,121 @@ +/* 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 */ +// MergeSheetFormBody field contract (s6f): the Bitbucket merge arm passes +// showTitle=false because the provider merge takes only the message — no +// commit-title input may exist on that arm whose value would be silently +// dropped on submit. The GitLab and GitHub arms keep the field. + +import * as React from 'react'; +import TestRenderer from 'react-test-renderer'; +import { describe, expect, it, vi } from 'vitest'; + +import '@/i18n'; +import type * as ReactI18next from 'react-i18next'; +import { MergeSheetFormBody } from './pr-merge-sheet-parts'; +import { type AllowedMergeMethod } from '@/lib/pr-review/merge/merge-blocked-reasons'; + +vi.mock('react-i18next', async importOriginal => { + const actual = await importOriginal(); + return { + ...actual, + useTranslation: () => { + const i18n = actual.getI18n(); + return { t: i18n.t.bind(i18n), i18n }; + }, + }; +}); + +vi.mock('react-native', () => ({ + Pressable: 'Pressable', + Switch: 'Switch', + TextInput: 'TextInput', + View: 'View', +})); + +vi.mock('expo-haptics', () => ({ + selectionAsync: vi.fn(), +})); + +vi.mock('@/components/pr-review/pr-form-sheet-chrome', () => ({ + PrFormSheetFooter: 'PrFormSheetFooter', + useFormSheetKeyboardVisible: () => false, +})); +vi.mock('@/components/ui/button', () => ({ Button: 'Button' })); +vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); +vi.mock('@/components/ui/radio-group', () => ({ + RadioGroup: 'RadioGroup', + radioItemA11y: (props: unknown) => props, +})); +vi.mock('@/components/ui/accessible-status', () => ({ AccessibleStatus: 'AccessibleStatus' })); +vi.mock('@/lib/hooks/use-theme-colors', () => ({ + useThemeColors: () => ({ mutedForeground: '#000000' }), +})); +vi.mock('@/components/pr-review/pr-review-reconnect-notice', () => ({ + PrReviewReconnectNotice: 'PrReviewReconnectNotice', +})); + +function formBodyProps(showTitle: boolean) { + const titleRef = { current: 'Merge pull request #1 from Feature' }; + const messageRef = { current: '' }; + return { + noMethodsAllowed: false, + methodOptions: [{ value: 'merge' as AllowedMergeMethod, label: 'Merge', icon: 'merge' }], + method: 'merge' as AllowedMergeMethod, + isMutating: false, + onMethodChange: vi.fn(), + titleRef, + titleInputRef: { current: null }, + titlePlaceholder: 'Merge pull request #1 from Feature', + showTitle, + messageRef, + messageInputRef: { current: null }, + isHalfDetent: false, + showDeleteBranchToggle: false, + deleteBranch: false, + onDeleteBranchChange: vi.fn(), + inlineError: null, + inlineErrorKind: null, + submitLabel: 'Merge', + onConfirm: vi.fn(), + onDismiss: vi.fn(), + }; +} + +function inputByA11yLabel( + renderer: TestRenderer.ReactTestRenderer, + label: string +): TestRenderer.ReactTestInstance | null { + const found = renderer.root.findAll(node => node.props.accessibilityLabel === label); + return found[0] ?? null; +} + +describe('MergeSheetFormBody commit-title field (s6f)', () => { + it('renders no commit-title input when showTitle is false (Bitbucket arm)', () => { + let renderer: TestRenderer.ReactTestRenderer | undefined = undefined; + TestRenderer.act(() => { + renderer = TestRenderer.create(); + }); + // eslint-disable-next-line typescript-eslint/no-unnecessary-condition -- the guard proves it, the cast cannot + if (!renderer) { + throw new Error('Failed to mount MergeSheetFormBody'); + } + expect(inputByA11yLabel(renderer, 'Commit title')).toBeNull(); + // The message field stays: the Bitbucket merge takes the message. + expect(inputByA11yLabel(renderer, 'Commit message')).not.toBeNull(); + }); + + it('keeps the commit-title input when showTitle is true (GitLab and GitHub arms)', () => { + let renderer: TestRenderer.ReactTestRenderer | undefined = undefined; + TestRenderer.act(() => { + renderer = TestRenderer.create(); + }); + // eslint-disable-next-line typescript-eslint/no-unnecessary-condition -- the guard proves it, the cast cannot + if (!renderer) { + throw new Error('Failed to mount MergeSheetFormBody'); + } + const title = inputByA11yLabel(renderer, 'Commit title'); + if (!title) { + throw new Error('Commit title input not found'); + } + expect(title.props.defaultValue).toBe('Merge pull request #1 from Feature'); + }); +}); diff --git a/apps/mobile/src/components/pr-review/merge/pr-merge-sheet-parts.tsx b/apps/mobile/src/components/pr-review/merge/pr-merge-sheet-parts.tsx index a578c3529e..2d4b1d7449 100644 --- a/apps/mobile/src/components/pr-review/merge/pr-merge-sheet-parts.tsx +++ b/apps/mobile/src/components/pr-review/merge/pr-merge-sheet-parts.tsx @@ -205,6 +205,12 @@ export function MergeSheetFormBody(props: { titleRef: RefObject; titleInputRef: RefObject; titlePlaceholder: string; + /** + * False on the Bitbucket merge arm (s6f): the provider merge takes only + * the message, so no commit-title input exists whose value would be + * silently dropped on submit. + */ + showTitle: boolean; messageRef: RefObject; messageInputRef: RefObject; isHalfDetent: boolean; @@ -226,6 +232,7 @@ export function MergeSheetFormBody(props: { titleRef, titleInputRef, titlePlaceholder, + showTitle, messageRef, messageInputRef, isHalfDetent, @@ -260,12 +267,14 @@ export function MergeSheetFormBody(props: { onChange={onMethodChange} /> )} - + {showTitle ? ( + + ) : null} ({ error: null as Error | null, })); +// Every useState setter the mocked hook primitives hand out, in call order. +// The error effect writes its inline copy through one of them, so a test can +// observe the state write even though the no-op mock never re-renders. +const stateSetters = vi.hoisted(() => [] as { mock: { calls: unknown[][] } }[]); + vi.mock('react', async () => { const actual = await vi.importActual('react'); return { ...actual, - useState: vi.fn( - (initial: T) => [initial, vi.fn() as () => void] as [T, (value: T) => void] - ), + useState: vi.fn((initial: T) => { + const setter = vi.fn(); + stateSetters.push(setter); + return [initial, setter as () => void] as [T, (value: T) => void]; + }), useMemo: vi.fn((factory: () => T) => factory()), useRef: vi.fn((initial: T) => { const ref: React.RefObject = { current: initial }; @@ -57,14 +75,17 @@ vi.mock('react', async () => { }; }); +const alertCalls = vi.hoisted(() => [] as { title: string; message: string }[]); + vi.mock('react-native', () => ({ Alert: { alert: vi.fn( ( - _title: string, - _message: string, + title: string, + message: string, buttons: readonly { style?: string; onPress?: () => void }[] ) => { + alertCalls.push({ title, message }); const destructive = buttons.find(b => b.style === 'destructive'); destructive?.onPress?.(); } @@ -117,6 +138,7 @@ vi.mock('@/components/pr-review/pr-review-reconnect-notice', () => ({ })); vi.mock('@/components/pr-review/pr-form-sheet-chrome', () => ({ PrFormSheetHeader: 'PrFormSheetHeader', + PrFormSheetFooter: 'PrFormSheetFooter', })); vi.mock('@/components/pr-review/merge/pr-merge-icons', () => ({ defaultMergeMethodOptionFor: () => 'squash', @@ -239,7 +261,7 @@ function findElement({ node, type, prop, value }: FindElementArgs): React.ReactE return null; } -function pressMerge(props: typeof baseProps) { +function pressMerge(props: Parameters[0]) { // eslint-disable-next-line new-cap const element = PrMergeSheet(props); // The submit CTA lives inside MergeSheetFormBody (mocked as a string @@ -388,3 +410,535 @@ describe('PrMergeSheet performSubmit wiring (P0-B-08)', () => { expect(onDismiss).toHaveBeenCalledTimes(1); }); }); + +// ── Provider arms (s6) ─────────────────────────────────────────────── + +/** Find an element whose type is a real (unmocked) component — by identity. */ +function findComponent(node: unknown, component: unknown): React.ReactElement | null { + if (React.isValidElement(node)) { + if (node.type === component) { + return node; + } + const children = (node.props as Record).children; + const found = findComponent(children, component); + if (found) { + return found; + } + } + if (Array.isArray(node)) { + for (const child of node) { + const found = findComponent(child, component); + if (found) { + return found; + } + } + } + return null; +} + +/** findComponent, but fails the test loudly when the component is absent. */ +function requireComponent(node: unknown, component: unknown): React.ReactElement { + const found = findComponent(node, component); + if (!found) { + throw new Error(`component ${(component as { name?: string }).name ?? '?'} not found`); + } + return found; +} + +/** Every string a directly-invoked component function rendered, in order. */ +function collectTexts(node: unknown): string[] { + const out: string[] = []; + const walk = (value: unknown): void => { + if (typeof value === 'string') { + out.push(value); + return; + } + if (Array.isArray(value)) { + for (const child of value) { + walk(child); + } + return; + } + if (React.isValidElement(value)) { + walk((value.props as Record).children); + } + }; + walk(node); + return out; +} + +const GITLAB_REF: ProviderPrRef = { platform: 'gitlab', projectPath: 'octocat/hello', mrIid: 1 }; +const BITBUCKET_REF: ProviderPrRef = { + platform: 'bitbucket', + workspace: 'acme', + repoSlug: 'hello', + prId: 1, +}; + +function mergeState(overrides: Partial = {}): ProviderPrMergeState { + return { + canMerge: true, + approvalsRequired: 0, + pipelineMustSucceed: false, + conflicts: false, + blockedReasons: [], + ...overrides, + }; +} + +const AUTO_MERGE_SUPPORTED: ProviderReviewCapability = { supported: true, reason: '' }; +const AUTO_MERGE_UNSUPPORTED: ProviderReviewCapability = { + supported: false, + reason: 'Bitbucket Cloud does not expose auto-merge in its API', +}; + +describe('PrMergeSheet provider merge arm (s6)', () => { + beforeEach(() => { + alertCalls.length = 0; + __resetMergePartialSuccessStoreForTests(); + mergeMutationMocks.mutateAsync.mockReset(); + mergeMutationMocks.isPending = false; + mergeMutationMocks.error = null; + autoMergeMutationMocks.mutateAsync.mockReset(); + autoMergeMutationMocks.isPending = false; + autoMergeMutationMocks.error = null; + vi.clearAllMocks(); + }); + + it('fences the merge on the head and folds the method into squash for GitLab', () => { + mergeMutationMocks.mutateAsync.mockResolvedValueOnce({ + merged: true, + sha: 'mergedsha', + branchDeleted: true, + }); + pressMerge({ ...baseProps, prRef: GITLAB_REF, mergeState: mergeState() }); + + // The confirm dialog speaks the connected provider's noun, in sentence + // form: "Merge merge request?", never "Merge Merge request?". + expect(alertCalls[0]).toEqual({ + title: 'Merge merge request?', + message: 'This will merge your changes into the base branch.', + }); + expect(mergeMutationMocks.mutateAsync).toHaveBeenCalledWith({ + expectedHeadSha: 'a'.repeat(40), + squash: true, + deleteBranch: true, + // The commit-title field rides the merge: GitLab takes the title the + // user sees (here the mocked default). + commitTitle: 'Merge pull request #1 from Feature', + }); + }); + + it('keeps the GitHub arm confirm copy and full input unchanged', () => { + mergeMutationMocks.mutateAsync.mockResolvedValueOnce({ + merged: true, + sha: 'mergedsha', + branchDeleted: true, + }); + pressMerge(baseProps); + + expect(alertCalls[0]?.title).toBe('Merge pull request?'); + expect(mergeMutationMocks.mutateAsync).toHaveBeenCalledWith( + expect.objectContaining({ + owner: 'octocat', + repo: 'hello', + number: 1, + method: 'squash', + expectedHeadSha: 'a'.repeat(40), + }) + ); + }); + + it('folds the provider ref identity into the durable merge draft key', async () => { + mergeMutationMocks.mutateAsync.mockResolvedValueOnce({ + merged: true, + sha: 'mergedsha', + branchDeleted: true, + }); + pressMerge({ ...baseProps, prRef: GITLAB_REF, mergeState: mergeState() }); + await flushMicrotasks(); + + expect(clearDraft).toHaveBeenCalledWith( + 'u1', + `pr-merge:octocat/hello#1@${providerPrRefKey(GITLAB_REF)}` + ); + }); + + it('shows the restrictions list above the form when the merge is allowed', () => { + // eslint-disable-next-line new-cap + const element = PrMergeSheet({ + ...baseProps, + prRef: GITLAB_REF, + mergeState: mergeState({ approvalsRequired: 2, pipelineMustSucceed: true }), + }); + expect( + findElement({ + node: element, + type: 'MergeSheetFormBody', + prop: 'submitLabel', + value: 'Merge', + }) + ).not.toBeNull(); + expect(findComponent(element, MergeRestrictionsList)).not.toBeNull(); + }); + + it('replaces the form with the restrictions list when the merge is blocked', () => { + // eslint-disable-next-line new-cap + const element = PrMergeSheet({ + ...baseProps, + prRef: GITLAB_REF, + mergeState: mergeState({ + canMerge: false, + blockedReasons: [{ code: 'failing_pipeline', message: 'Pipeline #123 failed' }], + }), + }); + // Nothing to submit: the form is gone and only Cancel remains. + expect( + findElement({ + node: element, + type: 'MergeSheetFormBody', + prop: 'submitLabel', + value: 'Merge', + }) + ).toBeNull(); + expect(findComponent(element, MergeRestrictionsList)).not.toBeNull(); + expect( + findElement({ node: element, type: 'Button', prop: 'accessibilityLabel', value: 'Cancel' }) + ).not.toBeNull(); + }); + + it('pins the blocked-arm footer to the sheet bottom (spot check e7)', () => { + // eslint-disable-next-line new-cap + const element = PrMergeSheet({ + ...baseProps, + prRef: GITLAB_REF, + mergeState: mergeState({ + canMerge: false, + blockedReasons: [{ code: 'failing_pipeline', message: 'Pipeline #123 failed' }], + }), + }); + // The sheet opens at the full detent; a growing spacer plus a content + // container that fills the viewport keep Cancel on the sheet's bottom + // edge instead of floating mid-sheet over an empty region. + const scroll = findElement({ + node: element, + type: 'ScrollView', + prop: 'className', + value: 'flex-1 bg-background', + }); + expect(scroll).not.toBeNull(); + if (!scroll) { + return; + } + expect( + (scroll.props as { contentContainerStyle?: Record }).contentContainerStyle + ).toEqual({ flexGrow: 1, paddingBottom: 4 }); + expect( + findElement({ node: scroll, type: 'View', prop: 'className', value: 'flex-1' }) + ).not.toBeNull(); + }); +}); + +// ── s6f: reviewer blocking findings ────────────────────────────────── + +function forbiddenError(): Error { + return Object.assign(new Error('403 Forbidden'), { data: { code: 'FORBIDDEN' } }); +} + +/** True when the error effect wrote `value` into any state slot. */ +function stateValueWritten(value: string): boolean { + return stateSetters.some(setter => setter.mock.calls.some(call => call[0] === value)); +} + +describe('PrMergeSheet commit-title arm (s6f)', () => { + beforeEach(() => { + stateSetters.length = 0; + mergeMutationMocks.mutateAsync.mockReset(); + mergeMutationMocks.isPending = false; + mergeMutationMocks.error = null; + vi.clearAllMocks(); + }); + + function formBodyProps(props: Parameters[0]) { + // eslint-disable-next-line new-cap + const element = PrMergeSheet(props); + const formBody = findElement({ + node: element, + type: 'MergeSheetFormBody', + prop: 'submitLabel', + value: 'Merge', + }); + if (!formBody) { + throw new Error('MergeSheetFormBody not found in rendered tree'); + } + return formBody.props as { showTitle?: boolean }; + } + + it('hides the commit-title field on the Bitbucket arm (the provider takes only the message)', () => { + expect( + formBodyProps({ ...baseProps, prRef: BITBUCKET_REF, mergeState: mergeState() }).showTitle + ).toBe(false); + }); + + it('keeps the commit-title field on the GitLab and GitHub arms', () => { + expect( + formBodyProps({ ...baseProps, prRef: GITLAB_REF, mergeState: mergeState() }).showTitle + ).toBe(true); + expect(formBodyProps(baseProps).showTitle).toBe(true); + }); + + it('never carries a commitTitle on the Bitbucket merge input', () => { + mergeMutationMocks.mutateAsync.mockResolvedValueOnce({ + merged: true, + sha: 'mergedsha', + branchDeleted: true, + }); + pressMerge({ ...baseProps, prRef: BITBUCKET_REF, mergeState: mergeState() }); + expect(mergeMutationMocks.mutateAsync).toHaveBeenCalledWith( + expect.not.objectContaining({ commitTitle: expect.anything() }) + ); + }); +}); + +describe('PrMergeSheet refused-merge wording (s6f)', () => { + beforeEach(() => { + stateSetters.length = 0; + mergeMutationMocks.mutateAsync.mockReset(); + mergeMutationMocks.isPending = false; + mergeMutationMocks.error = null; + vi.clearAllMocks(); + }); + + it('words a refused GitLab merge after the merge-request noun', () => { + mergeMutationMocks.error = forbiddenError(); + // eslint-disable-next-line new-cap + PrMergeSheet({ ...baseProps, prRef: GITLAB_REF, mergeState: mergeState() }); + // The provider 403 must never read "pull request" on a merge request. + expect(stateValueWritten("You don't have permission to merge this merge request.")).toBe(true); + expect(stateValueWritten("You don't have permission to merge this pull request.")).toBe(false); + }); + + it('keeps the exact pre-s6 forbidden copy on the GitHub arm', () => { + mergeMutationMocks.error = forbiddenError(); + // eslint-disable-next-line new-cap + PrMergeSheet(baseProps); + expect(stateValueWritten("You don't have permission to merge this pull request.")).toBe(true); + }); +}); + +describe('PrMergeSheet provider auto-merge arms (s6)', () => { + beforeEach(() => { + alertCalls.length = 0; + __resetMergePartialSuccessStoreForTests(); + mergeMutationMocks.mutateAsync.mockReset(); + autoMergeMutationMocks.mutateAsync.mockReset(); + autoMergeMutationMocks.isPending = false; + autoMergeMutationMocks.error = null; + vi.clearAllMocks(); + }); + + function autoMergeProps(ref: ProviderPrRef, capability: ProviderReviewCapability) { + return { + ...baseProps, + mode: 'enable-auto-merge' as const, + prRef: ref, + mergeState: mergeState({ approvalsRequired: 2 }), + autoMergeCapability: capability, + }; + } + + function pressSubmit(element: React.ReactElement): () => void { + const submit = findElement({ + node: element, + type: 'Button', + prop: 'accessibilityLabel', + value: 'Enable auto-merge', + }); + if (!submit) { + throw new Error('auto-merge submit button not found'); + } + const onPress = (submit.props as { onPress?: () => void }).onPress; + // eslint-disable-next-line typescript-eslint/no-unnecessary-condition -- the guard above proves it, the cast cannot + if (!onPress) { + throw new Error('auto-merge submit has no onPress'); + } + return onPress; + } + + it('arms GitLab auto-merge through the head fence with the provider confirm copy', () => { + autoMergeMutationMocks.mutateAsync.mockResolvedValueOnce({ supported: true, reason: '' }); + // eslint-disable-next-line new-cap + const element = PrMergeSheet(autoMergeProps(GITLAB_REF, AUTO_MERGE_SUPPORTED)); + expect(findComponent(element, ProviderAutoMergeBody)).not.toBeNull(); + + pressSubmit(element)(); + expect(alertCalls[0]?.message).toBe( + 'The merge request will merge automatically once its pipeline succeeds.' + ); + expect(autoMergeMutationMocks.mutateAsync).toHaveBeenCalledWith({ + expectedHeadSha: 'a'.repeat(40), + }); + }); + + it('shows the capability banner with the provider reason for Bitbucket, with nothing to submit', () => { + // eslint-disable-next-line new-cap + const element = PrMergeSheet(autoMergeProps(BITBUCKET_REF, AUTO_MERGE_UNSUPPORTED)); + // The banner carries the server's explicit reason; no submit CTA exists. + const banner = requireComponent(element, PrReviewCapabilityBanner); + expect((banner.props as { capability?: ProviderReviewCapability }).capability).toBe( + AUTO_MERGE_UNSUPPORTED + ); + expect( + findElement({ + node: element, + type: 'Button', + prop: 'accessibilityLabel', + value: 'Enable auto-merge', + }) + ).toBeNull(); + expect(autoMergeMutationMocks.mutateAsync).not.toHaveBeenCalled(); + }); +}); + +describe('MergeRestrictionsList (s6)', () => { + it('lists the policy flags in localized copy', () => { + // eslint-disable-next-line new-cap + const tree = MergeRestrictionsList({ + mergeState: mergeState({ + canMerge: false, + approvalsRequired: 2, + pipelineMustSucceed: true, + conflicts: true, + }), + term: 'merge request', + }); + const texts = collectTexts(tree); + expect(texts).toContain('Merge restrictions'); + expect(texts).toContain('Resolve the merge conflicts on this branch before merging.'); + expect(texts).toContain('2 approvals required'); + expect(texts).toContain('The pipeline must succeed before merging.'); + }); + + it('localizes known provider reasons and keeps the server message for `other`', () => { + // eslint-disable-next-line new-cap + const tree = MergeRestrictionsList({ + mergeState: mergeState({ + canMerge: false, + blockedReasons: [ + { code: 'failing_pipeline', message: 'Pipeline #123 failed' }, + { code: 'pending_pipeline', message: 'Pipeline #124 running' }, + { code: 'permission', message: '403 Forbidden' }, + { code: 'other', message: 'A merge is already running' }, + ], + }), + term: 'merge request', + }); + const texts = collectTexts(tree); + expect(texts).toContain('The pipeline is failing on the latest commit.'); + expect(texts).toContain('The pipeline is still running on the latest commit.'); + expect(texts).toContain("You don't have permission to merge this merge request."); + // The `other` code keeps the provider's own message verbatim. + expect(texts).toContain('A merge is already running'); + }); + + it('does not repeat a flag the reasons list already carries', () => { + // eslint-disable-next-line new-cap + const tree = MergeRestrictionsList({ + mergeState: mergeState({ + canMerge: false, + approvalsRequired: 3, + pipelineMustSucceed: true, + conflicts: true, + blockedReasons: [ + { code: 'conflicts', message: 'conflicts exist' }, + { code: 'failing_pipeline', message: 'Pipeline #123 failed' }, + ], + }), + term: 'merge request', + }); + const texts = collectTexts(tree); + const conflictRows = texts.filter( + text => text === 'Resolve the merge conflicts on this branch before merging.' + ); + expect(conflictRows).toHaveLength(1); + // The pipeline flag row is suppressed: the failing-pipeline reason says it. + expect(texts).not.toContain('The pipeline must succeed before merging.'); + expect(texts).toContain('3 approvals required'); + }); + + it('maps the draft reason onto the provider noun', () => { + // eslint-disable-next-line new-cap + const tree = MergeRestrictionsList({ + mergeState: mergeState({ + canMerge: false, + blockedReasons: [{ code: 'draft', message: 'Draft status' }], + }), + term: 'merge request', + }); + expect(collectTexts(tree)).toContain( + 'Mark the merge request as ready for review before merging.' + ); + }); + + it('renders nothing when no restriction applies', () => { + // eslint-disable-next-line new-cap + expect(MergeRestrictionsList({ mergeState: mergeState(), term: 'merge request' })).toBeNull(); + }); +}); + +describe('ProviderAutoMergeBody (s6)', () => { + it('explains the arm in the provider noun and repeats the restrictions', () => { + // eslint-disable-next-line new-cap + const tree = ProviderAutoMergeBody({ + mergeState: mergeState({ approvalsRequired: 2 }), + term: 'merge request', + }); + // The restrictions render as a mounted MergeRestrictionsList child. + const restrictions = requireComponent(tree, MergeRestrictionsList); + expect( + (restrictions.props as { mergeState?: ProviderPrMergeState }).mergeState?.approvalsRequired + ).toBe(2); + expect(collectTexts(tree)).toContain( + 'GitLab merges this merge request automatically once its pipeline succeeds.' + ); + }); + + it('renders just the explanation while the merge state has not loaded', () => { + // eslint-disable-next-line new-cap + const tree = ProviderAutoMergeBody({ mergeState: null, term: 'merge request' }); + expect(collectTexts(tree)).toEqual([ + 'GitLab merges this merge request automatically once its pipeline succeeds.', + ]); + }); +}); + +function conflictError(message: string): Error { + return Object.assign(new Error(message), { data: { code: 'CONFLICT' } }); +} + +describe('staleHeadRejectionMessage (s6)', () => { + it('returns the provider reason for a moved head', () => { + const message = + 'The merge request changed since it was loaded. Reload the merge request and try again.'; + expect(staleHeadRejectionMessage(conflictError(message))).toBe(message); + }); + + it('returns the reason for a target closed without merging', () => { + const message = 'The merge request was closed without merging.'; + expect(staleHeadRejectionMessage(conflictError(message))).toBe(message); + }); + + it('leaves other conflicts to the generic classification', () => { + expect(staleHeadRejectionMessage(conflictError('A merge is already running'))).toBeNull(); + }); + + it('ignores errors that are not conflicts', () => { + expect( + staleHeadRejectionMessage( + Object.assign(new Error('The merge request changed since it was loaded.'), { + data: { code: 'FORBIDDEN' }, + }) + ) + ).toBeNull(); + }); +}); diff --git a/apps/mobile/src/components/pr-review/merge/pr-merge-sheet.tsx b/apps/mobile/src/components/pr-review/merge/pr-merge-sheet.tsx index 2de1156233..d7b63c5ec1 100644 --- a/apps/mobile/src/components/pr-review/merge/pr-merge-sheet.tsx +++ b/apps/mobile/src/components/pr-review/merge/pr-merge-sheet.tsx @@ -14,19 +14,36 @@ // dismisses (cancel) or the mutation succeeds (auto-dismiss). import * as Haptics from 'expo-haptics'; -import { Alert, Keyboard, ScrollView, type TextInput, useWindowDimensions } from 'react-native'; -import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; +import { + Alert, + Keyboard, + ScrollView, + type TextInput, + useWindowDimensions, + View, +} from 'react-native'; +import { type ReactNode, useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { useTranslation } from 'react-i18next'; -import { type inferRouterInputs, type MobileRouter } from '@kilocode/trpc/mobile'; +import { + type ProviderPrMergeBlockedReason, + type ProviderPrMergeState, + type ProviderPrPlatform, + type ProviderReviewCapability, +} from '@kilocode/app-shared/provider-review'; -import { PrFormSheetHeader } from '@/components/pr-review/pr-form-sheet-chrome'; +import { PrFormSheetFooter, PrFormSheetHeader } from '@/components/pr-review/pr-form-sheet-chrome'; +import { PrReviewCapabilityBanner } from '@/components/pr-review/pr-review-capability-banner'; +import { Button } from '@/components/ui/button'; +import { Text } from '@/components/ui/text'; import { type AllowedMergeMethod, type PrMergeMethod, type PrOverviewRepoSettings, } from '@/lib/pr-review/merge/merge-blocked-reasons'; import { + type EnableAutoMergeVars, + type MergeVars, useEnableAutoMergeMutation, useMergePullRequestMutation, } from '@/lib/pr-review/merge/use-pr-merge-mutations'; @@ -41,17 +58,26 @@ import { mergeMethodOptionsFor, } from '@/components/pr-review/merge/pr-merge-icons'; import { MergeSheetFormBody } from '@/components/pr-review/merge/pr-merge-sheet-parts'; +import { providerPrNounKey } from '@/components/pr-review/pr-review-provider-noun'; import { defaultCommitMessage, defaultCommitTitle, } from '@/lib/pr-review/merge/merge-commit-defaults'; import { useCurrentUserId } from '@/lib/hooks/use-current-user-id'; +import { formatNumber } from '@/lib/format'; +import { i18n } from '@/i18n'; +import { type ProviderPrRef, providerPrRefKey } from '@/lib/pr-review/provider-pr-ref'; +import { readTrpcErrorField } from '@/lib/trpc-error'; import { clearDraft, isMergeDraft, prMergeDraftKey, saveDraft } from '@/lib/persist/drafts'; import { useDraftFlushOnBackground } from '@/lib/persist/use-draft-flush'; import { useFencedDraftLoad } from '@/lib/persist/use-draft-load'; type PrMergeSheetMode = 'merge' | 'enable-auto-merge'; +// flexGrow makes the ScrollView's content container at least the viewport +// tall, so a short arm's spacer can push its footer to the sheet's bottom. +const SHEET_SCROLL_CONTENT_STYLE = { flexGrow: 1, paddingBottom: 4 }; + type PrMergeSheetProps = Readonly<{ owner: string; /** The GitHub repository name (the `repo` path segment, not the settings object). */ @@ -69,15 +95,67 @@ type PrMergeSheetProps = Readonly<{ mode: PrMergeSheetMode; sheetTitle: string; eyebrow: string; + /** + * The provider ref (s6). Present on the GitLab/Bitbucket surface: the merge + * posts through `providerReview.mergePullRequest` with the head fence, the + * method list comes from the provider (not the GitHub repo settings), and + * the draft key folds the ref identity. Absent on GitHub, which keeps the + * exact pre-s6 path. + */ + prRef?: ProviderPrRef; + /** + * The provider merge gate from `providerReview.getMergeState` (s2/s3). + * Rendered as the restrictions list and gates the submit; null on GitHub, + * whose gate derives from the overview DTO in the merge section. + */ + mergeState?: ProviderPrMergeState | null; + /** + * The auto-merge capability (provider arms). A `supported: false` answer + * (Bitbucket) renders the explicit capability banner instead of the form. + */ + autoMergeCapability?: ProviderReviewCapability; /** Called after a successful merge / auto-merge enable so the orchestrator can refetch. */ onRefetch: () => Promise; /** Called when the user cancels or after a successful submit. */ onDismiss: () => void; }>; -type RouterInputs = inferRouterInputs; -type MergePullRequestInput = RouterInputs['githubPrReview']['mergePullRequest']; -type AutoMergeInput = RouterInputs['githubPrReview']['enableAutoMerge']; +/** + * The provider's own noun in sentence form (s6). Defined in + * `pr-review-provider-noun.ts` (shared with the overview's provider merge + * arm) and re-exported here for the sheet's callers. + */ +export { providerPrNounKey } from '@/components/pr-review/pr-review-provider-noun'; + +/** + * The explicit stale-head rejection (s6): the server refuses a merge whose + * head moved (or whose target closed) with CONFLICT carrying the provider's + * own reason. There is nothing to retry against the old head, so the sheet + * keeps that reason inline and stays put — no redirect, no retry affordance. + * Returns the reason to show, or null when the error is not a stale-head. + */ +export function staleHeadRejectionMessage(error: unknown): string | null { + if (readTrpcErrorField(error, 'code') !== 'CONFLICT') { + return null; + } + const message = error instanceof Error ? error.message : ''; + return /changed since it was loaded|closed without merging/.test(message) ? message : null; +} + +/** + * The inline copy for a refused merge (s6f): the provider arm words the + * refusal after the connected provider (merge request vs pull request) + * through the existing term-parameterized key; GitHub keeps the exact + * pre-s6 copy. + */ +function mergeForbiddenCopy( + platform: ProviderPrPlatform | undefined, + t: ReturnType['t'] +): string { + return platform + ? t('prReview.merge.providerBlocked.permission', { term: t(providerPrNounKey(platform)) }) + : t('prReview.merge.forbidden'); +} /** * Wraps an uncontrolled-input ref so every `.current` write (the parts file's @@ -98,6 +176,16 @@ function savingRef(target: { current: T }, onWrite: () => void) { }); } +// The short arms (blocked merge, capability banner, provider auto-merge) +// carry a small body; the sheet still opens at the full detent, so their +// footers are pinned to the sheet's bottom edge with a growing spacer +// (spot check e7: Cancel sat mid-sheet over a large empty region). The +// ScrollView's content container grows to at least the viewport, so the +// spacer only expands when the body is shorter than the sheet. +function bodySpacer() { + return ; +} + export function PrMergeSheet(props: PrMergeSheetProps) { const { owner, @@ -113,22 +201,45 @@ export function PrMergeSheet(props: PrMergeSheetProps) { mode, sheetTitle, eyebrow, + prRef, + mergeState, + autoMergeCapability, onRefetch, onDismiss, } = props; const { t } = useTranslation(); - const methodOptions = useMemo(() => mergeMethodOptionsFor(repoSettings), [repoSettings]); + // Provider arms derive the method list from the platform, not the GitHub + // repo settings: GitLab offers merge + squash, Bitbucket Cloud only the + // merge commit. GitHub keeps the repo-settings list unchanged. + const providerMethodOptions = useMemo(() => { + if (!prRef) { + return null; + } + return mergeMethodOptionsFor({ + ...repoSettings, + allowMergeCommit: true, + allowSquashMerge: prRef.platform === 'gitlab', + allowRebaseMerge: false, + allowAutoMerge: prRef.platform === 'gitlab', + }); + }, [prRef, repoSettings]); + const methodOptions = useMemo( + () => providerMethodOptions ?? mergeMethodOptionsFor(repoSettings), + [providerMethodOptions, repoSettings] + ); const safeInitial: AllowedMergeMethod = useMemo( () => methodOptions.find(o => o.value === initialMethod)?.value ?? - defaultMergeMethodOptionFor(repoSettings), - [initialMethod, methodOptions, repoSettings] + (providerMethodOptions + ? (providerMethodOptions[0]?.value ?? defaultMergeMethodOptionFor(repoSettings)) + : defaultMergeMethodOptionFor(repoSettings)), + [initialMethod, methodOptions, providerMethodOptions, repoSettings] ); const [method, setMethod] = useState(safeInitial); - const showDeleteBranchToggle = !isCrossRepo; + const showDeleteBranchToggle = prRef ? true : !isCrossRepo; const [deleteBranch, setDeleteBranch] = useState(repoSettings.deleteBranchOnMerge); // iOS uncontrolled-input pattern: store text in a ref via onChangeText, @@ -145,7 +256,10 @@ export function PrMergeSheet(props: PrMergeSheetProps) { // read while the user id is unknown. The inputs render only once the draft // settles, seeded from the stored value or today's defaults. const { userId, isLoading: isIdentityLoading } = useCurrentUserId(); - const mergeDraftKey = prMergeDraftKey(owner, repoName, number); + const positionDraftKey = prMergeDraftKey(owner, repoName, number); + // Provider arms fold the collision-free ref identity into the key (identity + // rule 17); the GitHub bytes stay exactly as stored before this slice. + const mergeDraftKey = prRef ? `${positionDraftKey}@${providerPrRefKey(prRef)}` : positionDraftKey; const draft = useFencedDraftLoad<{ title: string; message: string }>({ userId, isIdentityLoading, @@ -190,13 +304,16 @@ export function PrMergeSheet(props: PrMergeSheetProps) { [owner, repoName, number] ); - const mergeMutation = useMergePullRequestMutation(ref); - const enableAutoMergeMutation = useEnableAutoMergeMutation(ref); + const mergeMutation = useMergePullRequestMutation(prRef ?? ref); + const enableAutoMergeMutation = useEnableAutoMergeMutation(prRef ?? ref); const isMutating = (mode === 'merge' && mergeMutation.isPending) || (mode === 'enable-auto-merge' && enableAutoMergeMutation.isPending); const lastError = mode === 'merge' ? mergeMutation.error : enableAutoMergeMutation.error; + // The provider platform as a stable primitive: the error effect words a + // refusal after it without depending on the `prRef` object identity. + const providerPlatform = prRef?.platform; useEffect(() => { if (lastError) { @@ -207,11 +324,20 @@ export function PrMergeSheet(props: PrMergeSheetProps) { setInlineErrorKind('non-retryable'); return; } + // A moved head is the explicit stale-head rejection (s6): the server + // answers CONFLICT with the provider's own reason, there is nothing to + // retry against the old head, and the sheet stays put showing it. + const staleHead = staleHeadRejectionMessage(lastError); + if (staleHead !== null) { + setInlineError(staleHead); + setInlineErrorKind('non-retryable'); + return; + } const classification = classifyPrReviewMutationError(lastError); if (classification.kind === 'bad-request' || classification.kind === 'forbidden') { setInlineError( classification.kind === 'forbidden' - ? t('prReview.merge.forbidden') + ? mergeForbiddenCopy(providerPlatform, t) : t('prReview.merge.cannotMerge') ); setInlineErrorKind('non-retryable'); @@ -225,7 +351,7 @@ export function PrMergeSheet(props: PrMergeSheetProps) { setInlineErrorKind('retryable'); } } - }, [lastError, t]); + }, [lastError, t, providerPlatform]); useEffect(() => { const sub = Keyboard.addListener('keyboardDidShow', () => { @@ -242,7 +368,29 @@ export function PrMergeSheet(props: PrMergeSheetProps) { setMethod(next); } - function buildMergeInput(): MergePullRequestInput { + function buildMergeInput(): MergeVars { + if (prRef) { + // The provider merge carries the head fence (the server refuses a moved + // head before any merge call); GitLab folds the method into `squash` + // and takes a commit title, Bitbucket only the message — and the form + // hides the title input on that arm (s6f), so no typed value is ever + // dropped. The term rides the fingerprint so a retried intent re-merges + // the same revision. + const commitMessage = messageRef.current.trim(); + return { + expectedHeadSha: headSha, + ...(prRef.platform === 'gitlab' + ? { + squash: method === 'squash', + ...(titleRef.current.trim().length > 0 + ? { commitTitle: titleRef.current.trim() } + : {}), + } + : {}), + deleteBranch: showDeleteBranchToggle ? deleteBranch : false, + ...(commitMessage.length > 0 ? { commitMessage } : {}), + }; + } return { owner, repo: repoName, @@ -255,7 +403,13 @@ export function PrMergeSheet(props: PrMergeSheetProps) { }; } - function buildAutoMergeInput(): AutoMergeInput { + function buildAutoMergeInput(): EnableAutoMergeVars { + if (prRef) { + // GitLab arms merge-when-pipeline-succeeds fenced on the head; the + // method rides the server-side squash handling. Bitbucket never gets + // here: the capability banner replaces the form. + return { expectedHeadSha: headSha }; + } const autoMethod: 'MERGE' | 'SQUASH' | 'REBASE' = (() => { if (method === 'merge') { return 'MERGE'; @@ -328,8 +482,16 @@ export function PrMergeSheet(props: PrMergeSheetProps) { void performSubmit(); }; + // Provider arms word the nouns after the connected provider (merge + // request vs pull request); GitHub keeps its exact pre-s6 copy. if (mode === 'merge') { - Alert.alert(t('prReview.merge.confirmTitle'), t('prReview.merge.confirmMessage'), [ + const [confirmTitle, confirmMessage] = prRef + ? [ + t('prReview.merge.confirmTitleTerm', { term: t(providerPrNounKey(prRef.platform)) }), + t('prReview.merge.confirmMessage'), + ] + : [t('prReview.merge.confirmTitle'), t('prReview.merge.confirmMessage')]; + Alert.alert(confirmTitle, confirmMessage, [ { text: t('common.cancel'), style: 'cancel' }, { text: t('prReview.merge.merge'), style: 'destructive', onPress: submit }, ]); @@ -337,7 +499,11 @@ export function PrMergeSheet(props: PrMergeSheetProps) { } Alert.alert( t('prReview.merge.enableAutoMergeConfirmTitle'), - t('prReview.merge.enableAutoMergeConfirmMessage'), + prRef + ? t('prReview.merge.enableAutoMergeConfirmMessageTerm', { + term: t(providerPrNounKey(prRef.platform)), + }) + : t('prReview.merge.enableAutoMergeConfirmMessage'), [ { text: t('common.cancel'), style: 'cancel' }, { text: t('prReview.merge.enableAutoMerge'), style: 'destructive', onPress: submit }, @@ -347,6 +513,12 @@ export function PrMergeSheet(props: PrMergeSheetProps) { const submitLabel = mode === 'merge' ? t('prReview.merge.merge') : t('prReview.merge.enableAutoMerge'); + // Provider arms (s6): the provider's own noun for the confirm copy, and the + // two auto-merge shapes — GitLab arms through the seam, Bitbucket Cloud has + // no auto-merge API and opens onto the capability banner instead. + const providerTerm = prRef ? t(providerPrNounKey(prRef.platform)) : ''; + const providerAutoMerge = Boolean(prRef) && mode === 'enable-auto-merge'; + const autoMergeUnsupported = providerAutoMerge && autoMergeCapability?.supported === false; // A repository can (rarely) have every merge method disabled. GitHub would // reject any submission, so surface it explicitly and block the action // rather than sending a method the repo does not allow. @@ -361,14 +533,126 @@ export function PrMergeSheet(props: PrMergeSheetProps) { onDismiss(); } + // The body a settled draft renders: the arm the provider state selects — + // the Bitbucket auto-merge capability banner, the GitLab auto-merge body, + // a blocked merge state's restrictions, or the form. The cancel-only + // arms share the ghost footer button. + function cancelOnlyFooter() { + return ( + + + + ); + } + + const settledBody = ((): ReactNode => { + if (autoMergeUnsupported) { + // A Bitbucket auto-merge opens onto the capability banner: the provider + // has no API to arm, so there is nothing to submit or retry. + return ( + <> + + + + {bodySpacer()} + {cancelOnlyFooter()} + + ); + } + if (providerAutoMerge) { + return ( + <> + + {bodySpacer()} + + + + + + ); + } + if (mergeState && !mergeState.canMerge) { + // A blocked merge state replaces the form with the restrictions list + // (nothing to submit). + return ( + <> + + + + {bodySpacer()} + {cancelOnlyFooter()} + + ); + } + return ( + <> + {mergeState ? ( + + + + ) : null} + + + ); + })(); + // PickerSheet invariant: [header, ScrollView]; footer is trailing content. + // Provider arms (s6): the s2/s3 merge state renders as the restrictions + // list; a blocked state replaces the form with that list (nothing to + // submit), and a Bitbucket auto-merge opens onto the capability banner. return ( <> - {draft.settled ? ( - - ) : null} + {draft.settled ? settledBody : null} ); } + +/** The localized copy for one provider blocked reason; `other` keeps the server's message. */ +function providerBlockedReasonText( + reason: ProviderPrMergeBlockedReason, + term: string, + t: ReturnType['t'] +): string { + // Literal keys, never a template: the catalog check scans the source for + // the keys a lookup passes on, and a computed key is invisible to it. + const KEY_BY_CODE = { + conflicts: 'prReview.merge.blocked.conflictsDetail', + required_approvals: 'prReview.merge.blocked.requiredReviewsDetail', + failing_pipeline: 'prReview.merge.providerBlocked.failingPipeline', + pending_pipeline: 'prReview.merge.providerBlocked.pendingPipeline', + draft: 'prReview.merge.providerBlocked.draft', + permission: 'prReview.merge.providerBlocked.permission', + other: null, + } satisfies Record; + const key = KEY_BY_CODE[reason.code]; + if (key === null) { + return reason.message; + } + return t(key, { term }); +} + +/** + * The s2/s3 merge state as an explicit restrictions list (s6): the branch + * policy flags first, then the provider's concrete blocked reasons. Rows the + * reasons list already carries are not repeated from the flags. + */ +export function MergeRestrictionsList({ + mergeState, + term, +}: Readonly<{ mergeState: ProviderPrMergeState; term: string }>) { + const { t } = useTranslation(); + const hasConflictReason = mergeState.blockedReasons.some(reason => reason.code === 'conflicts'); + const hasApprovalsReason = mergeState.blockedReasons.some( + reason => reason.code === 'required_approvals' + ); + const hasPipelineReason = mergeState.blockedReasons.some( + reason => reason.code === 'failing_pipeline' || reason.code === 'pending_pipeline' + ); + const rows: { id: string; text: string }[] = []; + if (mergeState.conflicts && !hasConflictReason) { + rows.push({ id: 'conflicts', text: t('prReview.merge.blocked.conflictsDetail') }); + } + if (mergeState.approvalsRequired > 0 && !hasApprovalsReason) { + rows.push({ + id: 'approvals', + text: t('prReview.merge.restrictions.approvalsRequired', { + count: mergeState.approvalsRequired, + displayCount: formatNumber(mergeState.approvalsRequired, i18n.language), + }), + }); + } + if (mergeState.pipelineMustSucceed && !hasPipelineReason) { + rows.push({ + id: 'pipeline', + text: t('prReview.merge.restrictions.pipelineMustSucceed'), + }); + } + for (const reason of mergeState.blockedReasons) { + rows.push({ + id: `blocked:${reason.code}:${reason.message}`, + text: providerBlockedReasonText(reason, term, t), + }); + } + if (rows.length === 0) { + return null; + } + return ( + + + {t('prReview.merge.restrictions.title')} + + {rows.map(row => ( + + {'• '} + {row.text} + + ))} + + ); +} + +/** + * The GitLab auto-merge body (s6): the merge state's restrictions plus one + * plain explanation. No form fields exist on this arm — arming rides only + * the head fence. + */ +export function ProviderAutoMergeBody({ + mergeState, + term, +}: Readonly<{ mergeState: ProviderPrMergeState | null | undefined; term: string }>) { + const { t } = useTranslation(); + return ( + + {mergeState ? : null} + + {t('prReview.merge.enableAutoMergeDescriptionTerm', { term })} + + + ); +} diff --git a/apps/mobile/src/components/pr-review/pr-form-sheet-chrome.tsx b/apps/mobile/src/components/pr-review/pr-form-sheet-chrome.tsx index 10138e03fa..1c222e1be5 100644 --- a/apps/mobile/src/components/pr-review/pr-form-sheet-chrome.tsx +++ b/apps/mobile/src/components/pr-review/pr-form-sheet-chrome.tsx @@ -49,6 +49,9 @@ export function useFormSheetKeyboardVisible(): boolean { export function PrFormSheetHeader(props: { title: string; eyebrow: string; onBack: () => void }) { return ( + {/* Left-aligned heading on the back row: `centerTitle` would split the + header into a centered title row and a second row holding a lone + dismiss chevron, which read as a stray control under the title. */} { + const actual = await importOriginal(); + return { + ...actual, + useTranslation: () => { + const i18n = actual.getI18n(); + return { t: i18n.t.bind(i18n), i18n }; + }, + }; +}); + +vi.mock('react-native', () => ({ View: 'View' })); +vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); + +const UNSUPPORTED: ProviderReviewCapability = { + supported: false, + reason: 'Bitbucket Cloud does not expose auto-merge in its API', +}; +const SUPPORTED: ProviderReviewCapability = { supported: true, reason: '' }; + +function renderBanner( + capability: ProviderReviewCapability | undefined +): TestRenderer.ReactTestRenderer { + let renderer: TestRenderer.ReactTestRenderer | null = null; + act(() => { + renderer = TestRenderer.create(createElement(PrReviewCapabilityBanner, { capability })); + }); + // eslint-disable-next-line typescript-eslint/no-unnecessary-condition -- the act() callback runs synchronously; this narrows the definite assignment + if (!renderer) { + throw new Error('Failed to create test renderer'); + } + return renderer; +} + +function textsOf(renderer: TestRenderer.ReactTestRenderer): string[] { + return renderer.root + .findAll(node => (node.type as string) === 'Text') + .flatMap(node => [node.props.children as string]) + .flat() + .filter((child): child is string => typeof child === 'string'); +} + +describe('PrReviewCapabilityBanner', () => { + it('renders the localized title and the provider reason for a supported:false capability', () => { + const renderer = renderBanner(UNSUPPORTED); + const texts = textsOf(renderer); + expect(texts).toContain('Not available on this provider'); + expect(texts).toContain(UNSUPPORTED.reason); + renderer.unmount(); + }); + + it('announces title and reason together for accessibility', () => { + const renderer = renderBanner(UNSUPPORTED); + const view = renderer.root.find(node => (node.type as string) === 'View'); + expect(view.props.accessibilityLabel).toBe( + `Not available on this provider: ${UNSUPPORTED.reason}` + ); + renderer.unmount(); + }); + + it('renders nothing for a supported capability — the affordance itself shows', () => { + const renderer = renderBanner(SUPPORTED); + expect(renderer.toJSON()).toBeNull(); + renderer.unmount(); + }); + + it('renders nothing while the capability is not loaded (undefined)', () => { + const renderer = renderBanner(undefined); + expect(renderer.toJSON()).toBeNull(); + renderer.unmount(); + }); +}); diff --git a/apps/mobile/src/components/pr-review/pr-review-capability-banner.tsx b/apps/mobile/src/components/pr-review/pr-review-capability-banner.tsx new file mode 100644 index 0000000000..1dcda5690c --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-capability-banner.tsx @@ -0,0 +1,37 @@ +// The explicit capability explanation (s6). A provider that cannot do +// something answers `{ supported: false, reason }`; this banner renders that +// answer as a visible, localized explanation — never a silent absence and +// never a generic failure. Any review surface holding a capability object +// (the merge sheet's Bitbucket auto-merge arm today, the discussion +// limitations after it) renders it through here so the wording stays one. + +import { useTranslation } from 'react-i18next'; +import { View } from 'react-native'; + +import { type ProviderReviewCapability } from '@kilocode/app-shared/provider-review'; + +import { Text } from '@/components/ui/text'; + +export function PrReviewCapabilityBanner({ + capability, +}: Readonly<{ capability: ProviderReviewCapability | undefined }>) { + const { t } = useTranslation(); + // A supported (or not-yet-loaded) capability has nothing to explain: the + // surface renders the affordance itself, so the banner draws nothing. + if (capability === undefined || capability.supported) { + return null; + } + return ( + + + {t('prReview.capabilities.banner.title')} + + {capability.reason} + + ); +} diff --git a/apps/mobile/src/components/pr-review/pr-review-checks-section.mounted.test.tsx b/apps/mobile/src/components/pr-review/pr-review-checks-section.mounted.test.tsx index 3c2fac4fdb..cae7521cd3 100644 --- a/apps/mobile/src/components/pr-review/pr-review-checks-section.mounted.test.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-checks-section.mounted.test.tsx @@ -1,9 +1,14 @@ /* eslint-disable typescript-eslint/no-deprecated -- react-test-renderer is the repository's native-free mounted test tool. */ +// eslint-disable-next-line import/no-nodejs-modules -- vitest-only theme-token guard, runs in node, never bundled into the app +import { readFileSync } from 'node:fs'; + import { createElement } from 'react'; import TestRenderer, { act } from 'react-test-renderer'; -import { describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { SpinningIcon } from '@/components/ui/spinning-icon'; +import { ProviderPrScopeProvider } from '@/lib/pr-review/provider-pr-ref'; +import { openExternalUrl } from '@/lib/external-link'; import { PrReviewChecksSection } from './pr-review-checks-section'; const query = vi.hoisted(() => ({ @@ -60,6 +65,7 @@ vi.mock('@/components/ui/icons', () => ({ XCircle: 'XCircle', })); vi.mock('@/components/ui/spinning-icon', () => ({ SpinningIcon: 'SpinningIcon' })); +vi.mock('@/components/ui/skeleton', () => ({ Skeleton: 'Skeleton' })); vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); vi.mock('@/components/pr-review/pr-review-reconnect-notice', () => ({ PrReviewReconnectNotice: 'PrReviewReconnectNotice', @@ -83,7 +89,10 @@ vi.mock('@/lib/pr-review/classify-pr-review-query-state', () => ({ classifyPrReviewQueryState: () => ({ kind: 'retryable' }), })); vi.mock('@/lib/trpc', () => ({ - useTRPC: () => ({ githubPrReview: { listChecks: { queryOptions: () => ({}) } } }), + useTRPC: () => ({ + githubPrReview: { listChecks: { queryOptions: () => ({}) } }, + providerReview: { listChecks: { queryOptions: () => ({}) } }, + }), })); describe('PrReviewChecksSection check status icons', () => { @@ -123,3 +132,211 @@ describe('PrReviewChecksSection check status icons', () => { }); }); }); + +describe('PrReviewChecksSection view-on-provider link', () => { + const gitlabScope = { + ref: { + platform: 'gitlab' as const, + projectPath: 'group/sub/repo', + mrIid: 12, + instanceHint: 'https://gitlab.example.com', + }, + organizationId: null, + }; + + function mountSection(scope?: { ref: typeof gitlabScope.ref; organizationId: string | null }) { + const previousData = query.data; + query.data = { + checkRuns: [], + rollup: { total: 0, success: 0, failure: 0, pending: 0, skipped: 0 }, + }; + const section = ( + + ); + const renderer: { current: TestRenderer.ReactTestRenderer | undefined } = { + current: undefined, + }; + act(() => { + renderer.current = TestRenderer.create( + scope ? {section} : section + ); + }); + const created = renderer.current; + if (!created) { + throw new Error('renderer was not created'); + } + return { + renderer: created, + restore: () => { + query.data = previousData; + }, + }; + } + + beforeEach(() => { + vi.mocked(openExternalUrl).mockClear(); + }); + + function pressViewButton(renderer: TestRenderer.ReactTestRenderer) { + const button = renderer.root.findAllByType('Button' as never)[0]; + if (!button) { + throw new Error('the view-on-provider button did not render'); + } + act(() => { + (button.props.onPress as () => void)(); + }); + } + + it('labels the opened link "pull request" on the GitHub arm', () => { + const { renderer, restore } = mountSection(); + pressViewButton(renderer); + + expect(openExternalUrl).toHaveBeenCalledWith('https://github.com/group/sub/repo/pull/12', { + label: 'prReview.terms.pullRequest', + }); + act(() => { + renderer.unmount(); + }); + restore(); + }); + + it('labels the same link "merge request" on a GitLab scope', () => { + const { renderer, restore } = mountSection(gitlabScope); + pressViewButton(renderer); + + expect(openExternalUrl).toHaveBeenCalledWith( + 'https://gitlab.example.com/group/sub/repo/-/merge_requests/12', + { label: 'prReview.terms.mergeRequest' } + ); + act(() => { + renderer.unmount(); + }); + restore(); + }); +}); + +// Spot check e1-nav-mr.png: the CHECKS section rendered as an empty gray +// block — no loading indicator, no empty copy, no error copy. The screen +// was in the loading state, and the state was invisible: the skeleton bars +// carried `bg-muted` inside a `bg-secondary` card, and `--muted` equals +// `--secondary` in BOTH themes (apps/mobile/src/global.css), so the bars +// painted the card's own colour. This test pins the fixed render path: the +// card holds three shared Skeleton bars in `bg-muted-soft` — the one gray +// that differs from the card in both themes — and no bar keeps the +// collision token. The CSS guard below proves the collision is real +// (`--muted` == `--secondary`) and that the token the bars now use is not +// the collision token, so a future theme change that reintroduces the +// collision fails here instead of shipping another empty gray block. +describe('PrReviewChecksSection loading state is visible on the card', () => { + const previous = { isLoading: false, data: query.data }; + + beforeEach(() => { + query.isLoading = true; + }); + + afterEach(() => { + query.isLoading = previous.isLoading; + query.data = previous.data; + }); + + function mountLoading() { + const renderer: { current: TestRenderer.ReactTestRenderer | undefined } = { + current: undefined, + }; + act(() => { + renderer.current = TestRenderer.create( + createElement(PrReviewChecksSection, { + owner: 'group/sub', + repo: 'repo', + number: 12, + headSha: 'head', + }) + ); + }); + const created = renderer.current; + if (!created) { + throw new Error('renderer was not created'); + } + return created; + } + + function findCard(renderer: TestRenderer.ReactTestRenderer) { + const cards = renderer.root.findAll( + node => + String(node.type) === 'View' && + typeof node.props.className === 'string' && + node.props.className.split(/\s+/).includes('bg-secondary') + ); + expect(cards).toHaveLength(1); + const card = cards[0]; + if (!card) { + throw new Error('the checks card did not render'); + } + return card; + } + + it('paints three animated skeleton bars in a colour distinct from the card', () => { + const renderer = mountLoading(); + const card = findCard(renderer); + + // The shared Skeleton component — the app's loading indicator (pulse + + // shimmer), not a static block. + const bars = card.findAll(node => String(node.type) === 'Skeleton'); + expect(bars).toHaveLength(3); + for (const bar of bars) { + const className = String(bar.props.className ?? ''); + expect(className.split(/\s+/)).toContain('bg-muted-soft'); + } + + // The defect itself: no node in the card may keep the bare `bg-muted` + // token — on this card it is the card's own colour, i.e. invisible. + const invisible = card.findAll( + node => + typeof node.props.className === 'string' && + /(^|\s)bg-muted(\s|$)/.test(node.props.className) + ); + expect(invisible).toHaveLength(0); + + // The state is announced, not only shown. + expect(card.props.accessibilityRole).toBe('progressbar'); + expect(card.props.accessibilityLabel).toBe('common.loading'); + + act(() => { + renderer.unmount(); + }); + }); + + it('keeps the CHECKS heading rendered while loading, so the block is labelled', () => { + const renderer = mountLoading(); + const headings = renderer.root.findAll( + node => String(node.type) === 'Text' && node.props.children === 'prReview.checks.title' + ); + expect(headings).toHaveLength(1); + act(() => { + renderer.unmount(); + }); + }); + + it('guards the theme tokens: bg-muted collides with the card, bg-muted-soft does not', () => { + const css = readFileSync(new URL('../../global.css', import.meta.url), 'utf8'); + const values = (name: string) => + [...css.matchAll(new RegExp(`--${name}:\\s*([^;]+);`, 'g'))].map(match => + (match[1] ?? '').trim().toLowerCase() + ); + const secondary = values('secondary'); + const muted = values('muted'); + const mutedSoft = values('muted-soft'); + + // Both theme blocks are present. + expect(secondary.length).toBeGreaterThanOrEqual(2); + expect(muted).toHaveLength(secondary.length); + expect(mutedSoft).toHaveLength(secondary.length); + // The collision that made the old skeleton invisible, pinned so the + // test above stays meaningful. + expect(muted).toEqual(secondary); + // The fix token must contrast with the card in every theme. + for (const [index, soft] of mutedSoft.entries()) { + expect(soft).not.toBe(secondary[index]); + } + }); +}); diff --git a/apps/mobile/src/components/pr-review/pr-review-checks-section.test.tsx b/apps/mobile/src/components/pr-review/pr-review-checks-section.test.tsx index 810930ca99..7e3377f252 100644 --- a/apps/mobile/src/components/pr-review/pr-review-checks-section.test.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-checks-section.test.tsx @@ -61,6 +61,9 @@ vi.mock('@/components/ui/icons', () => ({ })); vi.mock('@/components/ui/spinning-icon', () => ({ SpinningIcon: 'SpinningIcon' })); vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); +// The section's loading card renders the UI skeleton; the real one reaches +// expo-linear-gradient and the reanimated worklets, which stay unmocked here. +vi.mock('@/components/ui/skeleton', () => ({ Skeleton: 'Skeleton' })); vi.mock('@/i18n', () => ({ i18n: { language: 'en', t: (key: string) => key } })); vi.mock('@/lib/external-link', () => ({ openExternalUrl: vi.fn() })); vi.mock('@/lib/format', () => ({ diff --git a/apps/mobile/src/components/pr-review/pr-review-checks-section.tsx b/apps/mobile/src/components/pr-review/pr-review-checks-section.tsx index 7b3f5bce5f..9c0b1ef608 100644 --- a/apps/mobile/src/components/pr-review/pr-review-checks-section.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-checks-section.tsx @@ -1,3 +1,4 @@ +/* eslint-disable max-lines -- the section owns every CHECKS state in one file: the card shell, the tone/rollup helpers and the rows share one surface, and splitting the visible-loading fix away from the states it must match scatters it across callers. */ import { useQuery } from '@tanstack/react-query'; import { type inferRouterOutputs, type MobileRouter } from '@kilocode/trpc/mobile'; import { @@ -9,19 +10,21 @@ import { MinusCircle, XCircle, } from '@/components/ui/icons'; -import { useMemo } from 'react'; import { useTranslation } from 'react-i18next'; import { Pressable, View } from 'react-native'; import { PrReviewReconnectNotice } from '@/components/pr-review/pr-review-reconnect-notice'; import { Button } from '@/components/ui/button'; +import { Skeleton } from '@/components/ui/skeleton'; import { SpinningIcon } from '@/components/ui/spinning-icon'; import { Text } from '@/components/ui/text'; import { i18n } from '@/i18n'; +import { reviewerPlatformLabel } from '@/lib/code-reviewer-config'; import { formatList, formatNumber } from '@/lib/format'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; import { classifyPrReviewQueryState } from '@/lib/pr-review/classify-pr-review-query-state'; -import { useTRPC } from '@/lib/trpc'; +import { useProviderPrQueries } from '@/lib/pr-review/provider-pr-queries'; +import { providerPrTermKey, providerPrWebUrl } from '@/lib/pr-review/provider-pr-ref'; import { cn } from '@/lib/utils'; import { openExternalUrl } from '@/lib/external-link'; @@ -175,30 +178,36 @@ export function PrReviewChecksSection({ number, headSha, }: PrReviewChecksSectionProps) { - const trpc = useTRPC(); + const queries = useProviderPrQueries({ owner, repo, number }); const colors = useThemeColors(); const { t } = useTranslation(); - const prUrl = useMemo( - () => `https://github.com/${owner}/${repo}/pull/${number}`, - [owner, repo, number] - ); + // Null on a GitLab ref with no instance hint: no host, so no link out. + const prUrl = providerPrWebUrl(queries.ref); - const checks = useQuery( - trpc.githubPrReview.listChecks.queryOptions({ owner, repo, ref: headSha }) - ); + const checks = useQuery(queries.checksOptions(headSha)); // Loading (first time, no cached data): show three skeleton rows in a // card so the section matches the final dimensions once the data lands. + // The bars must NOT be `bg-muted` here: `--muted` and `--secondary` are + // the same colour in both themes (apps/mobile/src/global.css), so a + // `bg-muted` bar inside this `bg-secondary` card paints nothing and the + // section reads as an empty gray block (spot check e1-nav-mr). The shared + // Skeleton gives the pulse + shimmer, and `bg-muted-soft` is the one gray + // that contrasts with the card in both themes. if (checks.isLoading) { return ( {t('prReview.checks.title')} - - - - + + + + ); @@ -269,6 +278,11 @@ export function PrReviewChecksSection({ ); } + const viewOnProviderLabel = + queries.platform === 'github' + ? t('prReview.checks.viewOnGitHub') + : t('prReview.terms.viewOnProvider', { provider: reviewerPlatformLabel(queries.platform) }); + const data = checks.data; const runList = data?.checkRuns ?? []; const rollup = data?.rollup ?? { total: 0, success: 0, failure: 0, pending: 0, skipped: 0 }; @@ -282,18 +296,20 @@ export function PrReviewChecksSection({ {rollupLine} - + {prUrl ? ( + + ) : null} ); diff --git a/apps/mobile/src/components/pr-review/pr-review-comment-composer-screen.tsx b/apps/mobile/src/components/pr-review/pr-review-comment-composer-screen.tsx index 26541d6c69..9848fb2468 100644 --- a/apps/mobile/src/components/pr-review/pr-review-comment-composer-screen.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-comment-composer-screen.tsx @@ -1,6 +1,6 @@ import { useQuery } from '@tanstack/react-query'; import { useLocalSearchParams, useRouter } from 'expo-router'; -import { type ReactNode, useEffect, useRef } from 'react'; +import { type ReactNode, useEffect, useMemo, useRef } from 'react'; import { useTranslation } from 'react-i18next'; import { Alert } from 'react-native'; import { ActivityIndicator } from '@/components/ui/activity-indicator'; @@ -14,6 +14,14 @@ import { InvalidRouteState } from '@/components/invalid-route-state'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; import { parseComposerParams } from '@/lib/pr-review/comment-composer-params'; import { usePendingReview } from '@/lib/pr-review/pending-review-provider'; +import { buildPrOverviewQueryOptions } from '@/lib/pr-review/provider-pr-queries'; +import { + isProviderScopeReady, + parseProviderPrRoute, + providerPrRefLabel, + providerPrTriple, + useProviderPrScope, +} from '@/lib/pr-review/provider-pr-ref'; import { useTRPC } from '@/lib/trpc'; type Params = { @@ -25,30 +33,76 @@ type Params = { line: string; startLine?: string; pendingId?: string; + // Provider route shape (`[platform]/[...identity]/comment-composer`). + platform?: string; + identity?: string[] | string; + instance?: string; }; +/** + * Comment-composer formSheet, mounted by BOTH routes: the GitHub + * `[owner]/[repo]/[number]/comment-composer` route and the provider + * `[platform]/[...identity]/comment-composer` route (s6). The route decides + * the ref; the provider layout publishes the scope the queries run under. + */ export function PrReviewCommentComposerScreen() { const router = useRouter(); const colors = useThemeColors(); const { t } = useTranslation(); const params = useLocalSearchParams(); - const parsed = parseComposerParams(params); const pending = usePendingReview(); + // The provider route carries the identity segments; the GitHub route the + // plain triple. Exactly one parses — the provider layout redirects a + // hand-built `/pr-review/github/...` link to the GitHub route. + const providerRef = useMemo( + () => + parseProviderPrRoute({ + platform: params.platform, + identity: params.identity, + instance: params.instance, + }), + [params.platform, params.identity, params.instance] + ); + const providerTriple = providerRef ? providerPrTriple(providerRef) : null; + const parsed = useMemo( + () => + parseComposerParams( + providerTriple + ? { + ...params, + owner: providerTriple.owner, + repo: providerTriple.repo, + number: String(providerTriple.number), + } + : params + ), + // `providerTriple` is derived from `providerRef`; the segments it reads + // are the same memo inputs. + [params, providerTriple] + ); + const pendingId = parsed?.pendingId; const isEdit = pendingId !== undefined; const pendingItem = isEdit ? pending.items.find(item => item.id === pendingId) : undefined; const title = isEdit ? t('prReview.composer.editTitle') : t('prReview.composer.addTitle'); - const eyebrow = parsed ? `${parsed.owner}/${parsed.repo}#${parsed.number}` : ''; + const githubEyebrow = parsed ? `${parsed.owner}/${parsed.repo}#${parsed.number}` : ''; + const eyebrow = providerRef ? providerPrRefLabel(providerRef) : githubEyebrow; + + // The scope the overview runs under: the layout publishes the provider + // scope in context; the GitHub route falls back to the parsed triple, so + // the GitHub query key is byte-identical to the pre-s6 one. + const scope = useProviderPrScope(parsed ?? { owner: '', repo: '', number: 0 }); // Edit mode is local-only: do not fire getPullRequest and do not gate on it. + // The readiness gate rides the same predicate the provider reads use, so a + // Bitbucket scope without its organization waits instead of querying. const trpc = useTRPC(); - const pr = useQuery( - trpc.githubPrReview.getPullRequest.queryOptions( - { owner: parsed?.owner ?? '', repo: parsed?.repo ?? '', number: parsed?.number ?? 0 }, - { enabled: parsed !== null && !isEdit } - ) - ); + const overviewOptions = useMemo(() => buildPrOverviewQueryOptions(trpc, scope), [trpc, scope]); + const pr = useQuery({ + ...overviewOptions, + enabled: parsed !== null && !isEdit && isProviderScopeReady(scope), + }); // Missing pending item: alert above the formSheet and back out once. const missingAlertedRef = useRef(false); @@ -83,6 +137,7 @@ export function PrReviewCommentComposerScreen() { owner={parsed.owner} repo={parsed.repo} number={parsed.number} + prRef={providerRef && providerRef.platform !== 'github' ? providerRef : undefined} mode={{ kind: 'edit', pendingItemId: pendingItem.id }} path={parsed.path} side={parsed.side} @@ -104,6 +159,7 @@ export function PrReviewCommentComposerScreen() { owner={parsed.owner} repo={parsed.repo} number={parsed.number} + prRef={providerRef && providerRef.platform !== 'github' ? providerRef : undefined} mode={{ kind: 'create', headSha: pr.data.headSha }} path={parsed.path} side={parsed.side} @@ -116,10 +172,17 @@ export function PrReviewCommentComposerScreen() { ); } - let body: ReactNode = null; + // A route with no valid comment target is not a broken comment flow: the + // route failed, nothing the composer could post. The "Add comment" chrome + // over a "Page not found" body read as a comment sheet that cannot save, so + // the terminal invalid state renders alone — no misleading title, no lone + // dismiss chevron — and carries its own Go back to the shared inbox. if (!parsed) { - body = ; - } else if (isEdit) { + return ; + } + + let body: ReactNode = null; + if (isEdit) { body = null; } else if (pr.isLoading) { body = ( diff --git a/apps/mobile/src/components/pr-review/pr-review-comment-composer.test.tsx b/apps/mobile/src/components/pr-review/pr-review-comment-composer.test.tsx index 1a1820b71d..f79cb0ca9c 100644 --- a/apps/mobile/src/components/pr-review/pr-review-comment-composer.test.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-comment-composer.test.tsx @@ -1,3 +1,4 @@ +/* eslint-disable max-lines -- the composer suite covers the draft clear rules and the provider-arm anchored post in one cohesive file */ // Clear-rule coverage for the comment composer's durable draft. The composer // clears its draft on three committed outcomes — comment post, add-to-review, // and a confirmed discard — and keeps it on a dismissed-without-confirmation @@ -14,6 +15,7 @@ import '@/i18n'; import type * as ReactI18next from 'react-i18next'; import { PrReviewCommentComposer } from './pr-review-comment-composer'; import { clearDraft } from '@/lib/persist/drafts'; +import { providerPrRefKey } from '@/lib/pr-review/provider-pr-ref'; vi.mock('react-i18next', async importOriginal => { const actual = await importOriginal(); @@ -256,6 +258,18 @@ describe('PrReviewCommentComposer draft clear rules', () => { footerProp(element, 'onCommentNow')?.(); await flushMicrotasks(); + // The GitHub arm keeps its exact pre-s6 variables: the position rides + // the flat input fields, never the provider `anchor` shape (c3). + expect(createCommentMocks.mutateAsync).toHaveBeenCalledWith({ + owner: 'octocat', + repo: 'hello', + number: 1, + body: 'hello', + path: 'src/a.ts', + line: 10, + side: 'RIGHT', + commitSha: 'a'.repeat(40), + }); expect(clearDraft).toHaveBeenCalledWith('u1', 'pr-comment:key'); }); @@ -287,3 +301,73 @@ describe('PrReviewCommentComposer draft clear rules', () => { expect(clearDraft).not.toHaveBeenCalled(); }); }); + +describe('PrReviewCommentComposer provider arm (s6)', () => { + // A GitLab MR with the same owner/repo/number triple as the GitHub + // fixtures: the folded draft key and the anchored post must differ from + // the GitHub arm in both bytes. + const gitlabRef = { platform: 'gitlab' as const, projectPath: 'octocat/hello', mrIid: 1 }; + const providerProps = { ...baseProps, prRef: gitlabRef }; + + function mountProviderComposer(): React.ReactElement { + // eslint-disable-next-line new-cap + return PrReviewCommentComposer(providerProps); + } + + beforeEach(() => { + createCommentMocks.mutateAsync.mockReset(); + createCommentMocks.isPending = false; + createCommentMocks.error = null; + vi.clearAllMocks(); + }); + + it('posts the tapped diff position as the real anchor through the provider arm (c3)', async () => { + createCommentMocks.mutateAsync.mockResolvedValueOnce({}); + const element = mountProviderComposer(); + typeBody(element, 'hello'); + footerProp(element, 'onCommentNow')?.(); + await flushMicrotasks(); + + expect(createCommentMocks.mutateAsync).toHaveBeenCalledWith({ + body: 'hello', + anchor: { path: 'src/a.ts', side: 'RIGHT', line: 10 }, + }); + }); + + it('carries a multi-line range into the anchor (c3)', async () => { + createCommentMocks.mutateAsync.mockResolvedValueOnce({}); + // eslint-disable-next-line new-cap + const element = PrReviewCommentComposer({ + ...providerProps, + startLine: 8, + }); + typeBody(element, 'hello'); + footerProp(element, 'onCommentNow')?.(); + await flushMicrotasks(); + + expect(createCommentMocks.mutateAsync).toHaveBeenCalledWith({ + body: 'hello', + anchor: { path: 'src/a.ts', side: 'RIGHT', line: 10, startLine: 8 }, + }); + }); + + it('keeps the provider comment out of the pending queue path (comment now is direct)', () => { + createCommentMocks.mutateAsync.mockResolvedValueOnce({}); + const element = mountProviderComposer(); + typeBody(element, 'hello'); + footerProp(element, 'onAddToReview')?.(); + + // Add-to-review stays provider-agnostic: the queue is local and the + // submit sheet folds it into the review body later. + expect(createCommentMocks.mutateAsync).not.toHaveBeenCalled(); + }); + + it('folds the provider ref identity into the durable comment draft key', () => { + const element = mountProviderComposer(); + typeBody(element, 'hello'); + footerProp(element, 'onAddToReview')?.(); + + const expected = `pr-comment:key@${providerPrRefKey(gitlabRef)}`; + expect(clearDraft).toHaveBeenCalledWith('u1', expected); + }); +}); diff --git a/apps/mobile/src/components/pr-review/pr-review-comment-composer.tsx b/apps/mobile/src/components/pr-review/pr-review-comment-composer.tsx index 9b37ca9e58..1371ce4a39 100644 --- a/apps/mobile/src/components/pr-review/pr-review-comment-composer.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-comment-composer.tsx @@ -36,6 +36,7 @@ import { buildSuggestionFence } from '@/lib/pr-review/build-suggestion-fence'; import { getDiffSelection } from '@/lib/pr-review/diff-selection-bridge'; import { usePendingReview } from '@/lib/pr-review/pending-review-provider'; import { useCreateReviewCommentMutation } from '@/lib/pr-review/use-pr-review-mutations'; +import { type ProviderPrRef, providerPrRefKey } from '@/lib/pr-review/provider-pr-ref'; type CommentComposerMode = | { kind: 'create'; headSha: string } @@ -45,6 +46,14 @@ type PrReviewCommentComposerProps = Readonly<{ owner: string; repo: string; number: number; + /** + * The provider ref (s6). Present on the GitLab/Bitbucket surface: the + * comment posts through `providerReview.addComment` with the tapped diff + * position as a real anchor (c3), and the durable comment draft key folds + * the ref identity so a same-numbered GitHub PR never shares this sheet's + * draft. Absent on GitHub, which keeps the exact pre-s6 write path. + */ + prRef?: ProviderPrRef; mode: CommentComposerMode; path: string; side: 'LEFT' | 'RIGHT'; @@ -62,6 +71,7 @@ export function PrReviewCommentComposer(props: PrReviewCommentComposerProps) { owner, repo, number, + prRef, mode, path, side, @@ -74,13 +84,18 @@ export function PrReviewCommentComposer(props: PrReviewCommentComposerProps) { } = props; const pending = usePendingReview(); const { t } = useTranslation(); - const createComment = useCreateReviewCommentMutation({ owner, repo, number }); + const createComment = useCreateReviewCommentMutation(prRef ?? { owner, repo, number }); const isEdit = mode.kind === 'edit'; // Durable comment draft (create mode only). Edit mode edits an already-queued // item, durable through the pending-review provider, so no draft there. const { userId, isLoading: isIdentityLoading } = useCurrentUserId(); - const commentDraftKey = prCommentDraftKey(owner, repo, number, path, side, line, startLine); + const positionDraftKey = prCommentDraftKey(owner, repo, number, path, side, line, startLine); + // Provider arms fold the collision-free ref identity into the key (identity + // rule 17); the GitHub bytes stay exactly as stored before this slice. + const commentDraftKey = prRef + ? `${positionDraftKey}@${providerPrRefKey(prRef)}` + : positionDraftKey; const draftUserId = isEdit ? undefined : userId; const draft = useFencedDraftLoad({ userId: draftUserId, @@ -199,17 +214,32 @@ export function PrReviewCommentComposer(props: PrReviewCommentComposerProps) { return; } try { - await createComment.mutateAsync({ - owner, - repo, - number, - body, - path, - line, - side, - ...(startLine !== undefined ? { startLine, startSide: side } : {}), - commitSha: mode.headSha, - }); + // Provider arms post the real diff position (c3): the sheet's + // path/side/line selection rides the `anchor` the provider router + // turns into an inline discussion / inline comment. + await createComment.mutateAsync( + prRef + ? { + body, + anchor: { + path, + side, + line, + ...(startLine !== undefined ? { startLine } : {}), + }, + } + : { + owner, + repo, + number, + body, + path, + line, + side, + ...(startLine !== undefined ? { startLine, startSide: side } : {}), + commitSha: mode.headSha, + } + ); if (draftUserId) { void clearDraft(draftUserId, commentDraftKey); } diff --git a/apps/mobile/src/components/pr-review/pr-review-connect-gate-view.test.ts b/apps/mobile/src/components/pr-review/pr-review-connect-gate-view.test.ts index c18825f1af..b3aa79454d 100644 --- a/apps/mobile/src/components/pr-review/pr-review-connect-gate-view.test.ts +++ b/apps/mobile/src/components/pr-review/pr-review-connect-gate-view.test.ts @@ -1,14 +1,9 @@ import * as React from 'react'; import { describe, expect, it, vi } from 'vitest'; -import '@/i18n'; import type * as ReactI18next from 'react-i18next'; +import '@/i18n'; import { PrReviewConnectGate } from './pr-review-connect-gate'; -import { - type PrReviewGateView, - selectPrReviewGateView, - type SelectPrReviewGateViewInput, -} from './pr-review-connect-gate-view'; vi.mock('react-i18next', async importOriginal => { const actual = await importOriginal(); @@ -21,57 +16,6 @@ vi.mock('react-i18next', async importOriginal => { }; }); -const base: SelectPrReviewGateViewInput = { - isError: false, - isLoading: false, - connected: true, - revoked: false, -}; - -function viewFor(patch: Partial): PrReviewGateView { - return selectPrReviewGateView({ ...base, ...patch }); -} - -describe('selectPrReviewGateView', () => { - it('returns loading while the query is loading', () => { - expect(viewFor({ isLoading: true })).toBe('loading'); - }); - - it('returns error when the query failed and is not loading', () => { - expect(viewFor({ isError: true })).toBe('error'); - }); - - it('returns error when both error and loading are true', () => { - expect(selectPrReviewGateView({ ...base, isError: true, isLoading: true })).toBe('error'); - }); - - it('returns connect when not connected and not revoked', () => { - expect(viewFor({ connected: false })).toBe('connect'); - }); - - it('returns reconnect when the connection was revoked', () => { - expect(viewFor({ connected: false, revoked: true })).toBe('reconnect'); - }); - - it('returns children when connected', () => { - expect(viewFor({ connected: true })).toBe('children'); - }); - - it('exposes only one happy view and four non-happy header-bearing views', () => { - const inputs: Partial[] = [ - { isLoading: true }, - { isError: true }, - { connected: false }, - { connected: false, revoked: true }, - { connected: true }, - ]; - const views = inputs.map(patch => viewFor(patch)); - expect(views.filter(view => view === 'children')).toHaveLength(1); - expect(views.filter(view => view !== 'children')).toHaveLength(4); - expect(new Set(views).size).toBe(views.length); - }); -}); - // The gate passes `authorization.isPending` (no data yet) to the view // selector, not `isLoading` (isPending && isFetching). A paused query // (offline/unknown connectivity, empty cache) is pending but not fetching, @@ -80,6 +24,9 @@ describe('selectPrReviewGateView', () => { // a revert to `isLoading` would make the paused query render Connect and // fail the assertions below. // +// The view selector itself lives in `@/lib/pr-review/pr-review-connect-gate-view` +// with its decision table beside it. +// // Rendered as a plain function call (same pattern as pr-review-screen.test.tsx) // with hooks and child components stubbed so the tree walk stays deterministic. @@ -118,6 +65,13 @@ vi.mock('@/lib/trpc', () => ({ }), })); +vi.mock('@/lib/config', () => ({ WEB_BASE_URL: 'https://web.example' })); + +vi.mock('expo-router', () => ({ + // Any non-entry pathname reaches the GitHub arm. + usePathname: () => '/pr-review/github/owner/repo/1', +})); + vi.mock('@/lib/hooks/use-theme-colors', () => ({ useThemeColors: () => ({ mutedForeground: '#6F6A61', primaryForeground: '#FFFFFF' }), })); @@ -164,6 +118,14 @@ function containsType(node: unknown, type: string): boolean { if (element.type === type) { return true; } + // A function component element (the gate dispatches to GitHubConnectGate) + // is walked by calling it: hooks are stubbed, so a plain call renders. + if ( + typeof element.type === 'function' && + containsType((element.type as (props: unknown) => unknown)(element.props), type) + ) { + return true; + } return Object.values(element.props as Record).some(value => containsType(value, type) ); diff --git a/apps/mobile/src/components/pr-review/pr-review-connect-gate-view.ts b/apps/mobile/src/components/pr-review/pr-review-connect-gate-view.ts deleted file mode 100644 index b8f0267f8b..0000000000 --- a/apps/mobile/src/components/pr-review/pr-review-connect-gate-view.ts +++ /dev/null @@ -1,29 +0,0 @@ -export type PrReviewGateView = 'error' | 'loading' | 'connect' | 'reconnect' | 'children'; - -export type SelectPrReviewGateViewInput = { - readonly isError: boolean; - readonly isLoading: boolean; - readonly connected: boolean; - readonly revoked: boolean; -}; - -/** - * Pure view selector for the PR-review connect gate. - * - * The gate is intentionally a simple priority ladder: error → loading → - * not-connected → children. This mirrors the original component's branch - * order and keeps every non-happy outcome in a fixed set of header-bearing - * states. - */ -export function selectPrReviewGateView(args: SelectPrReviewGateViewInput): PrReviewGateView { - if (args.isError) { - return 'error'; - } - if (args.isLoading) { - return 'loading'; - } - if (!args.connected) { - return args.revoked ? 'reconnect' : 'connect'; - } - return 'children'; -} diff --git a/apps/mobile/src/components/pr-review/pr-review-connect-gate.tsx b/apps/mobile/src/components/pr-review/pr-review-connect-gate.tsx index 530c49ae4e..f0bbabdcb3 100644 --- a/apps/mobile/src/components/pr-review/pr-review-connect-gate.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-connect-gate.tsx @@ -1,9 +1,10 @@ import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query'; -import { PlugZap, ShieldAlert } from '@/components/ui/icons'; -import { type ReactNode, useCallback, useState } from 'react'; +import { PlugZap, RefreshCcw, ShieldAlert } from '@/components/ui/icons'; +import { type ReactNode, useCallback, useMemo, useState } from 'react'; import { useTranslation } from 'react-i18next'; import { Platform, View } from 'react-native'; import { ActivityIndicator } from '@/components/ui/activity-indicator'; +import { usePathname } from 'expo-router'; import { CenteredState } from '@/components/centered-state'; import { toast } from 'sonner-native'; @@ -13,36 +14,86 @@ import { QueryError } from '@/components/query-error'; import { ScreenHeader } from '@/components/screen-header'; import { Button } from '@/components/ui/button'; import { Text } from '@/components/ui/text'; +import { WEB_BASE_URL } from '@/lib/config'; +import { getBitbucketIntegrationUrl, getGitLabIntegrationUrl } from '@/lib/integration-urls'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; import { useExternalAuthReturn } from '@/lib/external-auth/use-external-auth-return'; import { openAuthorizationAndWaitForReturn } from '@/lib/pr-review/connect-gate-platform'; -import { selectPrReviewGateView } from './pr-review-connect-gate-view'; +import { selectPrReviewGateView } from '@/lib/pr-review/pr-review-connect-gate-view'; +import { type ProviderPrPlatform } from '@/lib/pr-review/provider-pr-ref'; import { useTRPC } from '@/lib/trpc'; +/** + * Cold deep links land straight on a gate state with no navigation history, + * so `ScreenHeader` would render without a back control. The provider-neutral + * inbox is the one exit every PR-review surface shares. + */ +const PR_REVIEW_ENTRY_HREF = '/(app)/pr-review' as const; + type PrReviewConnectGateProps = { readonly children: ReactNode; + /** + * Which provider's connection this mount checks. The GitHub detail route + * and every pre-s7 mount keep the default; the provider layout passes the + * route's platform. + */ + readonly platform?: ProviderPrPlatform; + /** The selected organization for a provider check; null = personal scope. */ + readonly organizationId?: string | null; }; /** - * Wraps every PR-review surface. The user's GitHub identity (separate from - * a per-org GitHub App installation) is required to post review comments - * via the mobile app — without it, every mutation would 401 in the same - * way. The gate is the single place that handles: + * Wraps every PR-review surface, with one arm per provider: * + * - GitHub: the user's GitHub identity (separate from a per-org GitHub App + * installation) is required to post review comments — the existing + * `getUserAuthorization` check and `connectUserAuthorization` CTA. + * - GitLab (personal + org) and Bitbucket (org): the integration status the + * s4 endpoints expose; the CTA opens the web integration page and the + * status refetches when the app returns. + * - Bitbucket personal: Cloud has no personal review scope, so the gate + * shows the org-only explanation with no CTA — nothing a retry could fix. + * + * Each arm handles the same states: * - happy: connected → render children - * - retryable: getUserAuthorization fails → QueryError + Retry - * - empty: not connected / revoked → EmptyState CTA - * - non-retryable: structurally n/a (this is a configuration gate, not a - * transient server failure). + * - retryable: the status check fails → QueryError + Retry + * - empty: not connected → EmptyState CTA into the provider's connect flow + * - non-retryable: Bitbucket personal → org-only explanation, no CTA + * + * The entry screen is provider-neutral — a pasted GitLab or Bitbucket link + * must work for a user with no GitHub connection — so the gate is a + * pass-through there; the provider gates protect each detail route. * - * The CTA calls `githubApps.connectUserAuthorization` and opens the + * The GitHub CTA calls `githubApps.connectUserAuthorization` and opens the * returned URL with the platform-appropriate browser launcher (iOS native * auth session that resolves on sheet close; Android custom tab that - * resolves on app-foreground via AppState). Cancellation on either - * platform simply leaves the gate showing — there's nothing to roll - * back because the auth flow is server-driven. + * resolves on app-foreground via AppState). Cancellation on either platform + * simply leaves the gate showing — there's nothing to roll back because the + * auth flow is server-driven. */ -export function PrReviewConnectGate({ children }: PrReviewConnectGateProps) { +export function PrReviewConnectGate({ + children, + platform = 'github', + organizationId = null, +}: PrReviewConnectGateProps) { + const pathname = usePathname(); + // The entry route (`(app)/pr-review` → pathname `/pr-review`) is the one + // PR-review surface that serves all three providers at once; gating it on + // any single connection would lock out the other two. + if (pathname === '/pr-review') { + return <>{children}; + } + if (platform === 'github') { + return {children}; + } + return ( + + {children} + + ); +} + +function GitHubConnectGate({ children }: Readonly<{ children: ReactNode }>) { const trpc = useTRPC(); const queryClient = useQueryClient(); const colors = useThemeColors(); @@ -99,6 +150,7 @@ export function PrReviewConnectGate({ children }: PrReviewConnectGateProps) { }; const view = selectPrReviewGateView({ + platform: 'github', isError: authorization.isError, // `isPending` (no data yet) rather than `isLoading` (isPending && // isFetching): a paused query (offline/unknown connectivity, empty cache) @@ -108,12 +160,13 @@ export function PrReviewConnectGate({ children }: PrReviewConnectGateProps) { isLoading: authorization.isPending, connected: authorization.data?.connected === true, revoked: authorization.data?.revoked === true, + organizationId: null, }); if (view === 'error') { return ( - + - + @@ -142,7 +195,7 @@ export function PrReviewConnectGate({ children }: PrReviewConnectGateProps) { const revoked = view === 'reconnect'; return ( - + {children}; } + +/** + * GitLab / Bitbucket arm. The status query is the s4 integration status the + * connect flow updates; a Bitbucket personal scope disables it entirely + * because there is nothing to check — the selector renders the terminal + * org-only explanation for that case. + */ +function ProviderConnectGate({ + platform, + organizationId, + children, +}: Readonly<{ + platform: Exclude; + organizationId: string | null; + children: ReactNode; +}>) { + const trpc = useTRPC(); + const colors = useThemeColors(); + const { t } = useTranslation(); + + // The two providers answer with different status shapes, so each arm + // subscribes to its own query and only one is enabled per mount — a + // union of the two queryOptions types is not assignable to one useQuery. + const gitlabOptions = useMemo( + () => + organizationId + ? trpc.organizations.reviewAgent.getGitLabStatus.queryOptions({ organizationId }) + : trpc.personalReviewAgent.getGitLabStatus.queryOptions(), + [trpc, organizationId] + ); + const bitbucketOptions = useMemo( + () => + trpc.organizations.reviewAgent.getBitbucketReadiness.queryOptions({ + organizationId: organizationId ?? '', + }), + [trpc, organizationId] + ); + // Bitbucket Cloud is organization-context only (s4): with no organization + // selected there is no endpoint to ask, so the query stays disabled and the + // gate renders the org-only explanation. + const gitlabStatus = useQuery({ ...gitlabOptions, enabled: platform === 'gitlab' }); + const bitbucketStatus = useQuery({ + ...bitbucketOptions, + enabled: platform === 'bitbucket' && organizationId !== null, + }); + const status = platform === 'gitlab' ? gitlabStatus : bitbucketStatus; + + const refetchStatus = useCallback(() => { + void status.refetch(); + }, [status]); + const { markLaunched, clearLaunch } = useExternalAuthReturn(refetchStatus); + const [connecting, setConnecting] = useState(false); + + const handleConnect = async () => { + setConnecting(true); + try { + // The provider connections are web-side integrations: open the + // existing integration page and re-check the status when the app + // returns (pattern: `openAuthorizationAndWaitForReturn`). + markLaunched(); + const integrationUrl = + platform === 'gitlab' + ? getGitLabIntegrationUrl(WEB_BASE_URL, organizationId ?? undefined) + : getBitbucketIntegrationUrl(WEB_BASE_URL, organizationId ?? ''); + const trigger = await openAuthorizationAndWaitForReturn(Platform.OS, integrationUrl); + if (trigger === 'sheet-close') { + clearLaunch(); + await status.refetch(); + } + // Android: the AppState listener in `useExternalAuthReturn` refetches + // when the app returns to foreground; the sentinel stays set until it + // consumes the launch. + } catch { + // The browser failed to open — clear the sentinel so a later unrelated + // foreground doesn't trigger a stray refetch, and keep the gate showing. + clearLaunch(); + } finally { + setConnecting(false); + } + }; + + const view = selectPrReviewGateView({ + platform, + isError: status.isError, + isLoading: status.isPending, + connected: status.data?.connected === true, + // `revoked` is the GitHub App's vocabulary; provider statuses only ever + // answer connected / not connected. + revoked: false, + organizationId, + }); + + if (view === 'org-only') { + return ( + + + + + ); + } + + if (view === 'error') { + return ( + + + { + void status.refetch(); + }} + isRetrying={status.isFetching} + /> + + ); + } + + if (view === 'loading') { + return ( + + + + + + + ); + } + + if (view === 'connect') { + const title = platform === 'gitlab' ? t('common.connectGitlab') : t('common.connectBitbucket'); + return ( + + + { + void handleConnect(); + }} + > + {connecting ? ( + + ) : ( + + )} + {title} + + } + /> + + ); + } + + return <>{children}; +} 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 index d25d35521a..ad7450159e 100644 --- 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 @@ -5,6 +5,7 @@ import { selectDiscussionTabView } from './pr-review-discussion-tab-view'; const base = { firstPageErrorState: null, isPending: false, + isPaused: false, isEmpty: false, }; @@ -37,6 +38,15 @@ describe('selectDiscussionTabView', () => { expect(selectDiscussionTabView({ ...base, isPending: true })).toEqual({ kind: 'loading' }); }); + it('returns retryable when the first page is pending but paused', () => { + // A paused fetch (offline, or never started) has no end: the skeleton + // would sit there with no comments, empty state, or error (spot check + // e7). The retryable state carries the working Retry CTA instead. + expect( + selectDiscussionTabView({ ...base, isPending: true, isPaused: true, isEmpty: true }) + ).toEqual({ kind: 'retryable' }); + }); + it('returns empty when there is no error, no pending, and no items', () => { expect(selectDiscussionTabView({ ...base, isEmpty: true })).toEqual({ kind: 'empty' }); }); @@ -50,6 +60,7 @@ describe('selectDiscussionTabView', () => { selectDiscussionTabView({ firstPageErrorState: { kind: 'permission' }, isPending: true, + isPaused: false, isEmpty: true, }) ).toEqual({ kind: 'permission' }); 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 index 326b006f9c..5b47a60830 100644 --- 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 @@ -11,15 +11,24 @@ export type DiscussionTabView = { export function selectDiscussionTabView(args: { firstPageErrorState: { kind: 'permission' | 'not-found' | 'reconnect' | 'retryable' } | null; isPending: boolean; + /** + * The pending first page is paused — offline, or a fetch that will never + * start (spot check e7). + */ + isPaused: boolean; isEmpty: boolean; }): DiscussionTabView { - const { firstPageErrorState, isPending, isEmpty } = args; + const { firstPageErrorState, isPending, isPaused, isEmpty } = args; if (firstPageErrorState) { return { kind: firstPageErrorState.kind }; } if (isPending) { - return { kind: 'loading' }; + // A paused page has no end: the skeleton would sit there with no + // comments, no empty state, and no error. Surface the retryable state so + // the tab always carries an escape. A page in flight — and the one frame + // before the fetch starts — keeps the skeleton. + return { kind: isPaused ? 'retryable' : 'loading' }; } if (isEmpty) { return { kind: 'empty' }; diff --git a/apps/mobile/src/components/pr-review/pr-review-discussion-tab.test.tsx b/apps/mobile/src/components/pr-review/pr-review-discussion-tab.test.tsx index c29a18c02d..c5356983e9 100644 --- a/apps/mobile/src/components/pr-review/pr-review-discussion-tab.test.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-discussion-tab.test.tsx @@ -3,6 +3,8 @@ import { createElement } from 'react'; import TestRenderer, { act } from 'react-test-renderer'; import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { type ProviderPrRef, ProviderPrScopeProvider } from '@/lib/pr-review/provider-pr-ref'; + import { PrReviewDiscussionTab } from './pr-review-discussion-tab'; const insetsState = vi.hoisted(() => ({ top: 0, bottom: 0, left: 0, right: 0 })); @@ -11,6 +13,7 @@ const discussionState = vi.hoisted(() => ({ query: { isPending: false, isFetching: false, + isPaused: false, hasNextPage: false, isFetchingNextPage: false, fetchNextPage: vi.fn(), @@ -56,10 +59,19 @@ const BASE_PROPS = { onRequestFiles: vi.fn(() => undefined), }; -function mountTab(): TestRenderer.ReactTestRenderer { +/** Mounts the tab, optionally under a provider scope (no scope = GitHub). */ +function mountTab(scopeRef?: ProviderPrRef): TestRenderer.ReactTestRenderer { + const tab = createElement(PrReviewDiscussionTab, BASE_PROPS); + const tree = scopeRef ? ( + + {tab} + + ) : ( + tab + ); const ref: { current: TestRenderer.ReactTestRenderer | undefined } = { current: undefined }; act(() => { - ref.current = TestRenderer.create(createElement(PrReviewDiscussionTab, BASE_PROPS)); + ref.current = TestRenderer.create(tree); }); const renderer = ref.current; if (!renderer) { @@ -94,6 +106,7 @@ function expectSinglePadding(renderer: TestRenderer.ReactTestRenderer, expected: function resetState(): void { discussionState.query.isPending = false; discussionState.query.isFetching = false; + discussionState.query.isPaused = false; discussionState.query.hasNextPage = false; discussionState.query.isFetchingNextPage = false; discussionState.threads = []; @@ -137,6 +150,23 @@ describe('PrReviewDiscussionTab full-body states', () => { expectSinglePadding(mountTab(), 32); }); + it('escapes a stuck skeleton when the first page is paused, not in flight', () => { + // Spot check e7: the tab showed only skeleton cards — no comments, no + // empty state, no error. A pending page whose fetch is paused has no + // end, so the tab must render the retryable state with a working Retry + // CTA instead of the permanent skeleton. + discussionState.query.isPending = true; + discussionState.query.isPaused = true; + const renderer = mountTab(); + + expect(renderer.root.findAll(node => String(node.type) === 'Skeleton')).toHaveLength(0); + const error = renderer.root.find(node => String(node.type) === 'QueryError'); + act(() => { + (error.props.onRetry as () => void)(); + }); + expect(discussionState.query.refetch).toHaveBeenCalled(); + }); + it('lets EmptyState own the empty body and keeps its Files action', () => { const renderer = mountTab(); const empty = renderer.root.find(node => String(node.type) === 'EmptyState'); @@ -168,6 +198,41 @@ describe('PrReviewDiscussionTab full-body states', () => { ).toHaveLength(0); }); + // Wording, not layout: the same three states below must never call a GitLab + // merge request a "pull request". Every other state's copy is already + // provider-neutral, so it stays on one key. + const GITLAB_REF: ProviderPrRef = { + platform: 'gitlab', + projectPath: 'group/sub/repo', + mrIid: 12, + }; + + function errorMessage(scopeRef?: ProviderPrRef): unknown { + return mountTab(scopeRef).root.find(node => String(node.type) === 'QueryError').props.message; + } + + it.each(['permission', 'not-found'])('names a merge request in the %s copy', kind => { + discussionState.firstPageErrorState = { kind }; + expect(errorMessage(GITLAB_REF)).toContain('merge request'); + expect(errorMessage(GITLAB_REF)).not.toContain('pull request'); + }); + + it.each(['permission', 'not-found'])( + 'keeps the pull request copy on the %s state without a provider scope', + kind => { + discussionState.firstPageErrorState = { kind }; + expect(errorMessage()).not.toContain('merge request'); + } + ); + + it('names a merge request in the empty state description', () => { + const description = (scopeRef?: ProviderPrRef) => + mountTab(scopeRef).root.find(node => String(node.type) === 'EmptyState').props + .description as string; + expect(description(GITLAB_REF)).toContain('merge request'); + expect(description()).toContain('pull request'); + }); + it('renders the happy list without a chrome wrapper', () => { discussionState.conversation = [{ nodeId: 'c1', createdAt: null }]; const renderer = mountTab(); 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 baca0fc176..a5e0f1c296 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 @@ -7,7 +7,11 @@ // the entire loaded set on every update (R4: a // later page can insert rows mid-list). // - loading: first page in flight; render `Skeleton` -// placeholders matching the row dimensions. +// placeholders matching the row dimensions. A first +// page that is pending but PAUSED (offline, or a fetch +// that will never start) is not "in flight": it falls +// to the retryable state below so the tab never sits +// on a skeleton with no escape (spot check e7). // - retryable: first page failed with a transient error; // render `QueryError` with the standard Retry // CTA wired to `refetch()`. @@ -70,6 +74,7 @@ import { toggleThreadExpanded, } from '@/lib/pr-review/discussion/thread-expansion'; import { usePrReviewDiscussionThreads } from '@/lib/pr-review/discussion/use-pr-review-discussion-threads'; +import { useProviderPrScope } from '@/lib/pr-review/provider-pr-ref'; import { selectDiscussionTabView } from '@/components/pr-review/pr-review-discussion-tab-view'; import { useDetailScreenBottomPadding } from '@/lib/screen-insets'; @@ -100,6 +105,11 @@ export function PrReviewDiscussionTab({ }); const { t } = useTranslation(); + // GitLab calls this a merge request; GitHub and Bitbucket both say pull + // request, so the three provider-named strings below switch on that term + // alone rather than forking the tab per provider. + const { ref } = useProviderPrScope({ owner, repo, number }); + const isMergeRequest = ref.platform === 'gitlab'; const [expansion, setExpansion] = useState>({}); const [suppressContentPosition, setSuppressContentPosition] = useState(false); @@ -214,12 +224,23 @@ export function PrReviewDiscussionTab({ const view = selectDiscussionTabView({ firstPageErrorState: retainedContentError ? null : firstPageErrorState, isPending: query.isPending && isEmpty, + // A pending page whose fetch is paused (offline, or a fetch that will + // never start) has no end — the retryable state, not the skeleton (spot + // check e7). + isPaused: query.isPaused, isEmpty, }); if (view.kind === 'permission') { return ( - + ); } if (view.kind === 'not-found') { @@ -227,7 +248,11 @@ export function PrReviewDiscussionTab({ ); } @@ -278,7 +303,11 @@ export function PrReviewDiscussionTab({ { + vi.clearAllMocks(); + resetHookSlots(); + store.clear(); +}); + +describe('recents identity across providers', () => { + beforeEach(() => { + seedRecents([ + { ...SAME_TRIPLE, title: 'GitHub one' }, + { + ...SAME_TRIPLE, + title: 'GitLab one', + platform: 'gitlab', + instanceHint: 'https://gitlab.example.com', + }, + { ...SAME_TRIPLE, title: 'Bitbucket one', platform: 'bitbucket' }, + ]); + }); + + it('renders one row per provider with a provider label', async () => { + const tree = await renderLoaded(); + const rows = findAll(tree, 'View').filter(p => p.props?.testID === 'recent-row'); + expect(rows).toHaveLength(3); + const labels = textValues(tree); + expect(labels).toContain('GitHub'); + expect(labels).toContain('GitLab'); + expect(labels).toContain('Bitbucket'); + // The GitLab row writes the MR identity with the provider's own separator. + expect(labels).toContain('acme/api!7'); + expect(labels).toContain('acme/api#7'); + }); + + it("row presses navigate to the row's own provider route", async () => { + const tree = await renderLoaded(); + const rowPressables = findAll(tree, 'Pressable').filter( + p => p.props?.accessibilityLabel == null + ); + expect(rowPressables).toHaveLength(3); + const pushes: unknown[] = []; + for (const row of rowPressables) { + mocks.push.mockClear(); + (propsOf(row).onPress as () => void)(); + pushes.push(mocks.push.mock.calls[0]?.[0]); + } + // Seed order is stored order: GitHub, GitLab, Bitbucket. + expect(pushes).toEqual([ + '/(app)/pr-review/acme/api/7', + '/(app)/pr-review/gitlab/acme/api/7?instance=https%3A%2F%2Fgitlab.example.com', + '/(app)/pr-review/bitbucket/acme/api/7', + ]); + }); + + it('remove confirms with provider-neutral copy and deletes only the targeted row', async () => { + const tree = await renderLoaded(); + const removeGitLab = find( + tree, + 'Button', + p => p.accessibilityLabel === 'Remove acme/api!7 from recents' + ); + (propsOf(removeGitLab).onPress as () => void)(); + expect(mocks.alert).toHaveBeenCalledWith( + 'Remove from recents?', + 'This review will be removed from your recents.', + expect.arrayContaining([ + expect.objectContaining({ text: 'Cancel' }), + expect.objectContaining({ text: 'Remove' }), + ]) + ); + const alertCall = mocks.alert.mock.calls[0] as + | [string, string, { text: string; onPress?: () => void }[]] + | undefined; + const destructive = alertCall?.[2].find(button => button.text === 'Remove'); + destructive?.onPress?.(); + await flush(); + const remaining = storedRecents(); + expect(remaining).toHaveLength(2); + expect(remaining.map(entry => recentPrKey(entry))).toEqual([ + 'github||acme/api#7', + 'bitbucket||acme/api#7', + ]); + }); + + it('a failed row keeps the retry CTA and provider label', async () => { + store.clear(); + seedRecents([ + { + ...SAME_TRIPLE, + title: 'Broken MR', + platform: 'gitlab', + instanceHint: 'https://gl.acme.dev', + lastResult: 'failed', + }, + ]); + const tree = await renderLoaded(); + expect(textValues(tree)).toContain("Couldn't load"); + const retry = find(tree, 'Button', p => p.accessibilityLabel === 'Retry'); + (propsOf(retry).onPress as () => void)(); + expect(mocks.push).toHaveBeenCalledWith( + '/(app)/pr-review/gitlab/acme/api/7?instance=https%3A%2F%2Fgl.acme.dev' + ); + }); + + it('an empty recents store shows the neutral empty state', async () => { + store.clear(); + seedRecents([]); + const tree = await renderLoaded(); + const empty = find(tree, 'EmptyState', () => true); + expect(empty.props?.title).toBe('No recent reviews'); + expect(empty.props?.description).toBe( + "Paste a link above to start a review — it'll show up here next time." + ); + }); + + it('the recents load renders into an indicator while pending', () => { + // getRecentPrs is still in flight on the first call: the body is the + // spinner, never a blank that would jump the layout on arrival. + store.set('pr-review-recents', JSON.stringify([{ ...SAME_TRIPLE, title: 'X' }])); + const tree = render(); + expect(findAll(tree, 'ActivityIndicator').length).toBeGreaterThan(0); + }); +}); diff --git a/apps/mobile/src/components/pr-review/pr-review-entry-screen-test-utils.ts b/apps/mobile/src/components/pr-review/pr-review-entry-screen-test-utils.ts new file mode 100644 index 0000000000..47350bb3a1 --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-entry-screen-test-utils.ts @@ -0,0 +1,253 @@ +// Shared plain-function-call harness for the pr-review entry screen tests. +// The entry screen is rendered without a React renderer: state and refs live +// in a per-test slot array, so calling the component again re-reads what the +// previous call's setters wrote. The vi.mock registrations live here so the +// two test files (URL field, recents) share one wiring; a test file must +// import this module before anything that pulls the screen in. + +import { vi } from 'vitest'; + +import type * as ReactI18next from 'react-i18next'; +import type * as ReactNamespace from 'react'; + +import { PrReviewEntryScreen } from './pr-review-entry-screen'; +import { type RecentPr } from '@/lib/pr-review/recent-prs'; + +import '@/i18n'; + +const harnessMocks = vi.hoisted(() => ({ + push: vi.fn(), + alert: vi.fn(), + toastError: vi.fn(), + clipboard: { current: '' as string }, +})); + +// A hoisted binding cannot be exported directly; alias it for the test files. +export const mocks = harnessMocks; + +// The store doubles as the recents disk: tests seed it directly and assert +// removals by reading it back. +export const store = new Map(); + +vi.mock('react-i18next', async importOriginal => { + const actual = await importOriginal(); + return { + ...actual, + useTranslation: () => { + const i18n = actual.getI18n(); + return { t: i18n.t.bind(i18n), i18n }; + }, + }; +}); + +vi.mock('expo-router', () => ({ + useFocusEffect: (effect: () => (() => void) | undefined) => { + effect(); + }, + useRouter: () => ({ push: harnessMocks.push }), +})); + +vi.mock('expo-clipboard', () => ({ + getStringAsync: async () => { + await Promise.resolve(); + return harnessMocks.clipboard.current; + }, +})); + +vi.mock('expo-secure-store', () => ({ + getItemAsync: async (key: string) => { + await Promise.resolve(); + return store.get(key) ?? null; + }, + setItemAsync: async (key: string, value: string) => { + await Promise.resolve(); + store.set(key, value); + }, + deleteItemAsync: async (key: string) => { + await Promise.resolve(); + store.delete(key); + }, +})); +vi.mock('@/lib/storage-keys', () => ({ PR_REVIEW_RECENTS_KEY: 'pr-review-recents' })); +vi.mock('@/lib/auth/account-metadata-write', () => ({ + writeAccountMetadata: async (_key: string, write: () => Promise) => { + await write(); + }, + deleteAccountMetadata: async (key: string) => { + await Promise.resolve(); + store.delete(key); + }, +})); + +vi.mock('react-native', () => ({ + ActivityIndicator: 'ActivityIndicator', + Alert: { alert: harnessMocks.alert }, + Pressable: 'Pressable', + TextInput: 'TextInput', + View: 'View', +})); + +vi.mock('@/components/empty-state', () => ({ EmptyState: 'EmptyState' })); +vi.mock('@/components/screen-header', () => ({ ScreenHeader: 'ScreenHeader' })); +vi.mock('@/components/ui/button', () => ({ Button: 'Button' })); +vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); +vi.mock('@/components/ui/activity-indicator', () => ({ ActivityIndicator: 'ActivityIndicator' })); +vi.mock('@/components/ui/icons', () => ({ + Clipboard: 'ClipboardIcon', + Link2: 'Link2', + SearchX: 'SearchX', + X: 'X', +})); +vi.mock('@/components/ui/directional-icons', () => ({ + DirectionalChevronRight: 'DirectionalChevronRight', +})); +vi.mock('@/components/pr-review/pr-review-inbox-list', () => ({ + PrReviewInboxList: 'PrReviewInboxList', +})); +vi.mock('@/lib/hooks/use-theme-colors', () => ({ + useThemeColors: () => ({ mutedForeground: '#6F6A61', primaryForeground: '#FFFFFF' }), +})); +vi.mock('@/lib/a11y/announcing-toast', () => ({ + announcingToast: { error: harnessMocks.toastError }, +})); + +// The screen's hooks run without a React renderer: state and refs live in a +// per-test slot array, so calling the component again re-reads what the +// previous call's setters wrote. +let hookSlots: unknown[] = []; +let hookIndex = 0; + +vi.mock('react', async importOriginal => { + const actual = await importOriginal(); + return { + ...actual, + useState: (initial: unknown) => { + const index = hookIndex; + hookIndex += 1; + if (!(index in hookSlots)) { + hookSlots[index] = initial; + } + return [ + hookSlots[index], + (next: unknown) => { + hookSlots[index] = + typeof next === 'function' + ? (next as (prev: unknown) => unknown)(hookSlots[index]) + : next; + }, + ]; + }, + useRef: (initial: unknown) => { + const index = hookIndex; + hookIndex += 1; + if (!(index in hookSlots)) { + hookSlots[index] = { current: initial }; + } + return hookSlots[index]; + }, + useCallback: (fn: unknown) => fn, + useMemo: (factory: () => unknown) => factory(), + }; +}); + +export type El = { + type?: unknown; + props?: Record; +}; + +function isElement(value: unknown): value is El { + return typeof value === 'object' && value !== null && 'type' in value && 'props' in value; +} + +function collect(node: unknown, typeName: string, out: El[]): void { + if (node == null) { + return; + } + if (Array.isArray(node)) { + for (const child of node) { + collect(child, typeName, out); + } + return; + } + if (!isElement(node)) { + return; + } + if (node.type === typeName) { + out.push(node); + } + for (const value of Object.values(node.props ?? {})) { + collect(value, typeName, out); + } +} + +export function findAll(tree: unknown, typeName: string): El[] { + const out: El[] = []; + collect(tree, typeName, out); + return out; +} + +export function find( + tree: unknown, + typeName: string, + where: (props: Record) => boolean +): El { + const match = findAll(tree, typeName).find(el => where(el.props ?? {})); + if (!match) { + throw new Error(`no ${typeName} matched the predicate`); + } + return match; +} + +/** A matched element's props, guaranteed present — call handlers through this. */ +export function propsOf(el: El): Record { + if (!el.props) { + throw new Error('element has no props'); + } + return el.props; +} + +export function textValues(tree: unknown): string[] { + return findAll(tree, 'Text') + .map(el => { + const child = el.props?.children; + return typeof child === 'string' ? child : ''; + }) + .filter(value => value.length > 0); +} + +export function render(): unknown { + // Re-rendering restarts the hook order but keeps the slot values, so a + // second call re-reads what the previous call's setters wrote. + hookIndex = 0; + // The component is a plain function call in this harness, not a constructor. + // eslint-disable-next-line new-cap + return PrReviewEntryScreen(); +} + +/** Drop all hook state so the next render starts from its initial values. */ +export function resetHookSlots(): void { + hookSlots = []; + hookIndex = 0; +} + +/** Flush the mocked SecureStore's microtask chain to completion. */ +export async function flush(): Promise { + await new Promise(resolve => { + setTimeout(resolve, 0); + }); +} + +export async function renderLoaded(): Promise { + render(); + // Flush the focus-effect recents load. + await flush(); + return render(); +} + +export function seedRecents(entries: RecentPr[]): void { + store.set('pr-review-recents', JSON.stringify(entries)); +} + +export function storedRecents(): RecentPr[] { + return JSON.parse(store.get('pr-review-recents') ?? '[]') as RecentPr[]; +} diff --git a/apps/mobile/src/components/pr-review/pr-review-entry-screen.test.ts b/apps/mobile/src/components/pr-review/pr-review-entry-screen.test.ts new file mode 100644 index 0000000000..e589d7b1ee --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-entry-screen.test.ts @@ -0,0 +1,138 @@ +// The entry screen is the first route into a review for every provider +// (s7): the field must accept a GitHub PR, a GitLab MR (gitlab.com or a +// self-managed host) and a Bitbucket PR. The URL-field arm of the tests; +// the recents arm lives in pr-review-entry-recents.test.ts and the shared +// plain-function-call harness in pr-review-entry-screen-test-utils.ts. + +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { + find, + findAll, + flush, + mocks, + propsOf, + render, + renderLoaded, + resetHookSlots, + seedRecents, +} from './pr-review-entry-screen-test-utils'; + +beforeEach(() => { + vi.clearAllMocks(); + resetHookSlots(); + mocks.clipboard.current = ''; + seedRecents([]); +}); + +describe('provider-neutral URL field', () => { + it('labels and placeholders name both review nouns, no provider host', async () => { + const tree = await renderLoaded(); + const input = find(tree, 'TextInput', () => true); + expect(input.props?.placeholder).toBe('Pull request or merge request URL'); + expect(input.props?.accessibilityLabel).toBe('Enter a pull request or merge request URL'); + expect(String(input.props?.placeholder)).not.toContain('github'); + }); + + it('opens a GitHub PR URL on the GitHub route', async () => { + const tree = await renderLoaded(); + const input = find(tree, 'TextInput', () => true); + (propsOf(input).onChangeText as (value: string) => void)( + 'https://github.com/octocat/hello-world/pull/42' + ); + const open = render(); + ( + propsOf( + find(open, 'Button', p => p.accessibilityLabel === 'Open pull request or merge request') + ).onPress as () => void + )(); + expect(mocks.push).toHaveBeenCalledWith('/(app)/pr-review/octocat/hello-world/42'); + }); + + it('opens a self-managed GitLab MR on the provider route with its instance', async () => { + const tree = await renderLoaded(); + const input = find(tree, 'TextInput', () => true); + (propsOf(input).onChangeText as (value: string) => void)( + 'https://gitlab.example.com/group/sub/repo/-/merge_requests/9' + ); + const open = render(); + ( + propsOf( + find(open, 'Button', p => p.accessibilityLabel === 'Open pull request or merge request') + ).onPress as () => void + )(); + expect(mocks.push).toHaveBeenCalledWith( + '/(app)/pr-review/gitlab/group/sub/repo/9?instance=https%3A%2F%2Fgitlab.example.com' + ); + }); + + it('opens a Bitbucket PR on the provider route', async () => { + const tree = await renderLoaded(); + const input = find(tree, 'TextInput', () => true); + (propsOf(input).onChangeText as (value: string) => void)( + 'https://bitbucket.org/acme/api/pull-requests/7/overview' + ); + const open = render(); + ( + propsOf( + find(open, 'Button', p => p.accessibilityLabel === 'Open pull request or merge request') + ).onPress as () => void + )(); + expect(mocks.push).toHaveBeenCalledWith('/(app)/pr-review/bitbucket/acme/api/7'); + }); + + it('toasts the provider-neutral invalid copy for a link no provider serves', async () => { + const tree = await renderLoaded(); + const input = find(tree, 'TextInput', () => true); + (propsOf(input).onChangeText as (value: string) => void)('https://example.com/blog/post'); + const open = render(); + ( + propsOf( + find(open, 'Button', p => p.accessibilityLabel === 'Open pull request or merge request') + ).onPress as () => void + )(); + expect(mocks.toastError).toHaveBeenCalledWith('Not a pull request or merge request link'); + expect(mocks.push).not.toHaveBeenCalled(); + }); + + it('paste replaces the field and opens a GitLab MR straight away', async () => { + mocks.clipboard.current = 'https://gitlab.com/acme/api/-/merge_requests/3'; + const tree = await renderLoaded(); + const paste = find( + tree, + 'Pressable', + p => p.accessibilityLabel === 'Paste pull request or merge request link' + ); + await (propsOf(paste).onPress as () => Promise)(); + await flush(); + expect(mocks.push).toHaveBeenCalledWith( + '/(app)/pr-review/gitlab/acme/api/3?instance=https%3A%2F%2Fgitlab.com' + ); + expect(mocks.toastError).not.toHaveBeenCalled(); + }); + + it('paste of plain text keeps the invalid toast without navigating', async () => { + mocks.clipboard.current = 'just some notes'; + const tree = await renderLoaded(); + const paste = find( + tree, + 'Pressable', + p => p.accessibilityLabel === 'Paste pull request or merge request link' + ); + await (propsOf(paste).onPress as () => Promise)(); + await flush(); + expect(mocks.toastError).toHaveBeenCalledWith('Not a pull request or merge request link'); + expect(mocks.push).not.toHaveBeenCalled(); + }); + + it('shows the clear control only once the field has text', async () => { + const before = await renderLoaded(); + expect( + findAll(before, 'Pressable').some(p => p.props?.accessibilityLabel === 'Clear link') + ).toBe(false); + const input = find(before, 'TextInput', () => true); + (propsOf(input).onChangeText as (value: string) => void)('anything'); + const after = render(); + expect(find(after, 'Pressable', p => p.accessibilityLabel === 'Clear link')).toBeTruthy(); + }); +}); diff --git a/apps/mobile/src/components/pr-review/pr-review-entry-screen.tsx b/apps/mobile/src/components/pr-review/pr-review-entry-screen.tsx index 58e42ee93e..cf177f2d42 100644 --- a/apps/mobile/src/components/pr-review/pr-review-entry-screen.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-entry-screen.tsx @@ -9,24 +9,28 @@ import { ActivityIndicator } from '@/components/ui/activity-indicator'; import { EmptyState } from '@/components/empty-state'; import { PrReviewInboxList } from '@/components/pr-review/pr-review-inbox-list'; -import { selectRecentPrRowState } from '@/components/pr-review/recent-pr-row-state'; +import { selectRecentPrRowState } from '@/lib/pr-review/recent-pr-row-state'; import { ScreenHeader } from '@/components/screen-header'; import { Button } from '@/components/ui/button'; import { Text } from '@/components/ui/text'; import { announcingToast } from '@/lib/a11y/announcing-toast'; -import { parseGitHubPrUrl } from '@/lib/github-pr-url'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; -import { getPrReviewPath } from '@/lib/profile-agent-navigation'; +import { providerPrRefLabel, providerPrRoutePath } from '@/lib/pr-review/provider-pr-ref'; import { consumePrLinkInputEcho, pushPrLinkInputEcho } from '@/lib/pr-review/pr-link-input-echo'; import { + decidePrLinkOpen, decidePrLinkPaste, prLinkToastClipboardEmptyCopy, prLinkToastInvalidCopy, selectPrLinkClearButtonVisible, } from '@/lib/pr-review/pr-link-paste'; -import { getRecentPrs, type RecentPr, removeRecentPr } from '@/lib/pr-review/recent-prs'; - -const URL_PLACEHOLDER = 'https://github.com/owner/repo/pull/123'; +import { + getRecentPrs, + providerRefFromRecentPr, + type RecentPr, + recentPrKey, + removeRecentPr, +} from '@/lib/pr-review/recent-prs'; export function PrReviewEntryScreen() { const router = useRouter(); @@ -72,16 +76,18 @@ export function PrReviewEntryScreen() { }; const handleSubmit = () => { - const raw = inputValueRef.current; - const parsed = parseGitHubPrUrl(raw.trim()); - if (!parsed) { + const decision = decidePrLinkOpen(inputValueRef.current); + if (decision.kind === 'invalid') { announcingToast.error(prLinkToastInvalidCopy()); return; } - // 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)); + // Navigate straight to the ref's own provider route. Recents are written + // only after an authorized payload (the review screen's backfill effect), + // so a failed or unauthorized open never persists an entry. A + // self-managed GitLab host parses to a ref whose instanceHint rides as + // the route's `instance` param — the server re-derives the authoritative + // instance, so a mismatched host lands on the clear not-authorized state. + router.push(providerPrRoutePath(decision.ref)); }; const handlePaste = async () => { @@ -105,9 +111,11 @@ export function PrReviewEntryScreen() { }; 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)); + // Navigate only. The review screen's backfill effect updates + // `lastOpenedAt` (and `lastResult`) once an authorized payload loads. + // The entry's platform (legacy entries: GitHub) decides the route, so a + // GitLab row opens the GitLab surface, never a same-named GitHub PR. + router.push(providerPrRoutePath(providerRefFromRecentPr(entry))); }; const handleRemoveRecent = (entry: RecentPr) => { @@ -157,11 +165,12 @@ export function PrReviewEntryScreen() { const isLast = index === recent.length - 1; const rowState = selectRecentPrRowState(entry); const removeLabel = t('prReview.entry.removeRecentAccessibility', { - repo: `${entry.owner}/${entry.repo}#${entry.number}`, + repo: providerPrRefLabel(providerRefFromRecentPr(entry)), }); return ( + + {rowState.provider} + {rowState.primary} @@ -240,7 +255,7 @@ export function PrReviewEntryScreen() { {t('prReview.entry.open')} diff --git a/apps/mobile/src/components/pr-review/pr-review-file-navigator-screen.test.tsx b/apps/mobile/src/components/pr-review/pr-review-file-navigator-screen.test.tsx new file mode 100644 index 0000000000..932337e136 --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-file-navigator-screen.test.tsx @@ -0,0 +1,135 @@ +/* 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 pr-review-discussion-tab.test.tsx) */ +import { createElement } from 'react'; +import TestRenderer, { act } from 'react-test-renderer'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { type ProviderPrRef } from '@/lib/pr-review/provider-pr-ref'; + +import { PrReviewFileNavigatorScreen } from './pr-review-file-navigator-screen'; + +const queryState = vi.hoisted(() => ({ + data: null as { headSha: string; counts: { changedFiles: number } } | null, + isLoading: false, + isError: false, + isFetching: false, + error: null as unknown, + refetch: vi.fn(), +})); + +const scopeState: { ref: ProviderPrRef; isReady: boolean } = vi.hoisted(() => ({ + ref: { platform: 'gitlab', projectPath: 'group/sub/repo', mrIid: 12 }, + isReady: true, +})); + +vi.mock('react-native', () => ({ View: 'View', ActivityIndicator: 'ActivityIndicator' })); +vi.mock('@tanstack/react-query', () => ({ useQuery: () => queryState })); +vi.mock('expo-router', () => ({ + useLocalSearchParams: () => ({}), + useRouter: () => ({ back: vi.fn() }), +})); +vi.mock('@/lib/pr-review/provider-pr-queries', () => ({ + useProviderPrQueries: () => ({ ...scopeState, overviewOptions: () => ({}) }), +})); +vi.mock('@/lib/hooks/use-theme-colors', () => ({ + useThemeColors: () => ({ mutedForeground: '#888' }), +})); +vi.mock('@/components/centered-state', () => ({ CenteredState: 'CenteredState' })); +vi.mock('@/components/query-error', () => ({ QueryError: 'QueryError' })); +vi.mock('@/components/screen-header', () => ({ ScreenHeader: 'ScreenHeader' })); +// The screen renders the UI spinner while loading; the real one reaches the +// motion policy (expo-battery), which stays unmocked in this pure harness. +vi.mock('@/components/ui/activity-indicator', () => ({ ActivityIndicator: 'ActivityIndicator' })); +vi.mock('@/components/pr-review/pr-review-reconnect-notice', () => ({ + PrReviewReconnectNotice: 'PrReviewReconnectNotice', +})); +vi.mock('@/components/pr-review/diff/pr-diff-file-navigator', () => ({ + PrDiffFileNavigator: 'PrDiffFileNavigator', +})); + +function mountScreen(): TestRenderer.ReactTestRenderer { + const ref: { current: TestRenderer.ReactTestRenderer | undefined } = { current: undefined }; + act(() => { + ref.current = TestRenderer.create(createElement(PrReviewFileNavigatorScreen)); + }); + const renderer = ref.current; + if (!renderer) { + throw new Error('renderer was not created'); + } + return renderer; +} + +function find(renderer: TestRenderer.ReactTestRenderer, type: string) { + return renderer.root.find(node => String(node.type) === type); +} + +function trpcError(code: string): unknown { + return Object.assign(new Error(code), { data: { code }, shape: { data: { code } } }); +} + +describe('PrReviewFileNavigatorScreen states', () => { + beforeEach(() => { + queryState.data = null; + queryState.isLoading = false; + queryState.isError = false; + queryState.isFetching = false; + queryState.error = null; + queryState.refetch.mockClear(); + scopeState.ref = { platform: 'gitlab', projectPath: 'group/sub/repo', mrIid: 12 }; + scopeState.isReady = true; + }); + + it('titles the sheet with the provider ref, not a GitHub triple', () => { + expect(find(mountScreen(), 'ScreenHeader').props.eyebrow).toBe('group/sub/repo!12'); + }); + + it('shows one loading indicator while the first load is in flight', () => { + queryState.isLoading = true; + const renderer = mountScreen(); + expect(renderer.root.findAll(node => String(node.type) === 'ActivityIndicator')).toHaveLength( + 1 + ); + expect(renderer.root.findAll(node => String(node.type) === 'QueryError')).toHaveLength(0); + }); + + it('hands the navigator the resolved provider identity on the happy path', () => { + queryState.data = { headSha: 'abc123', counts: { changedFiles: 4 } }; + const navigator = find(mountScreen(), 'PrDiffFileNavigator'); + expect(navigator.props).toMatchObject({ + owner: 'group/sub', + repo: 'repo', + number: 12, + headSha: 'abc123', + changedFiles: 4, + }); + }); + + it('offers a retry only for a transient failure', () => { + queryState.isError = true; + queryState.error = trpcError('INTERNAL_SERVER_ERROR'); + const error = find(mountScreen(), 'QueryError'); + expect(error.props.variant).toBe('server'); + act(() => { + (error.props.onRetry as () => void)(); + }); + expect(queryState.refetch).toHaveBeenCalled(); + }); + + it.each([ + ['FORBIDDEN', 'permission'], + ['NOT_FOUND', 'not-found'], + ])('renders %s as a terminal state with no retry', (code, variant) => { + queryState.isError = true; + queryState.error = trpcError(code); + const error = find(mountScreen(), 'QueryError'); + expect(error.props.variant).toBe(variant); + expect(error.props.onRetry).toBeUndefined(); + }); + + it('points a broken connection at the reconnect notice instead of a retry', () => { + queryState.isError = true; + queryState.error = trpcError('PRECONDITION_FAILED'); + const renderer = mountScreen(); + expect(find(renderer, 'PrReviewReconnectNotice')).toBeDefined(); + expect(renderer.root.findAll(node => String(node.type) === 'QueryError')).toHaveLength(0); + }); +}); diff --git a/apps/mobile/src/components/pr-review/pr-review-file-navigator-screen.tsx b/apps/mobile/src/components/pr-review/pr-review-file-navigator-screen.tsx index 2fa40b135c..d05acc8567 100644 --- a/apps/mobile/src/components/pr-review/pr-review-file-navigator-screen.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-file-navigator-screen.tsx @@ -7,11 +7,14 @@ import { ActivityIndicator } from '@/components/ui/activity-indicator'; import { CenteredState } from '@/components/centered-state'; import { PrDiffFileNavigator } from '@/components/pr-review/diff/pr-diff-file-navigator'; +import { PrReviewReconnectNotice } from '@/components/pr-review/pr-review-reconnect-notice'; import { QueryError } from '@/components/query-error'; import { ScreenHeader } from '@/components/screen-header'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; +import { classifyPrReviewQueryState } from '@/lib/pr-review/classify-pr-review-query-state'; +import { useProviderPrQueries } from '@/lib/pr-review/provider-pr-queries'; +import { providerPrRefLabel, providerPrTriple } from '@/lib/pr-review/provider-pr-ref'; import { parseParam } from '@/lib/route-params'; -import { useTRPC } from '@/lib/trpc'; type Params = { owner: string; @@ -23,29 +26,38 @@ type Params = { * File-navigator formSheet route. Fetches the PR overview for the head SHA * (the navigator keys viewed state + the diff list on it) and mounts the S6c * navigator content. Rendered inside the `[number]` layout's formSheet stack. + * + * Provider-agnostic (s5): the same sheet is a sibling of the provider route's + * stack, where the identity comes from the published scope rather than from + * `owner`/`repo`/`number` params. The GitHub params below stay the fallback + * scope, so the GitHub route reaches the exact same query it did before. */ export function PrReviewFileNavigatorScreen() { const router = useRouter(); const colors = useThemeColors(); const { t } = useTranslation(); const params = useLocalSearchParams(); - const owner = parseParam(params.owner) ?? ''; - const repo = parseParam(params.repo) ?? ''; const rawNumber = parseParam(params.number) ?? ''; - const number = Number.parseInt(rawNumber, 10); + const queries = useProviderPrQueries({ + owner: parseParam(params.owner) ?? '', + repo: parseParam(params.repo) ?? '', + number: Number.parseInt(rawNumber, 10), + }); + const { owner, repo, number } = providerPrTriple(queries.ref); - const trpc = useTRPC(); - const pr = useQuery( - trpc.githubPrReview.getPullRequest.queryOptions( - { owner, repo, number }, - { enabled: Boolean(owner) && Boolean(repo) && Number.isInteger(number) && number > 0 } - ) - ); + // The route layout above validates the identity before this sheet can + // mount; the guard stays as the belt-and-braces it always was, ANDed with + // the scope readiness a Bitbucket ref carries. + const hasIdentity = Boolean(owner) && Boolean(repo) && Number.isInteger(number) && number > 0; + const pr = useQuery({ + ...queries.overviewOptions(), + enabled: queries.isReady && hasIdentity, + }); const header = ( { router.back(); @@ -77,9 +89,8 @@ export function PrReviewFileNavigatorScreen() { ) : ( - { void pr.refetch(); }} @@ -89,3 +100,39 @@ export function PrReviewFileNavigatorScreen() { ); } + +/** + * The sheet's failure states, split the same way the Overview and Discussion + * bodies split them: a permission denial, a missing PR/MR and a broken + * connection are terminal and carry no retry, because retrying the identical + * request cannot change any of them. Only a transient failure gets the CTA. + */ +function NavigatorError({ + error, + onRetry, + isRetrying, +}: Readonly<{ error: unknown; onRetry: () => void; isRetrying: boolean }>) { + const { t } = useTranslation(); + const state = error === null ? null : classifyPrReviewQueryState(error); + if (state?.kind === 'permission') { + return ; + } + if (state?.kind === 'not-found') { + return ; + } + if (state?.kind === 'reconnect') { + return ( + + + + ); + } + return ( + + ); +} diff --git a/apps/mobile/src/components/pr-review/pr-review-files-tab.tsx b/apps/mobile/src/components/pr-review/pr-review-files-tab.tsx index 87f015aa75..07de0b67b7 100644 --- a/apps/mobile/src/components/pr-review/pr-review-files-tab.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-files-tab.tsx @@ -16,6 +16,12 @@ type PrReviewFilesTabProps = { * Files tab: hosts the S6b diff file list (a virtualized FlashList, so the * screen renders this outside its Overview ScrollView). S6c layers the file * navigator sheet and the tablet unified/side-by-side toggle on top of this. + * + * Provider-agnostic (s5): the identity below is the GitHub-shaped triple the + * list and its stores are written against, while the list's own queries + * (`usePrReviewFileListQuery`, `usePrDiffContextLoader`) resolve the real + * `ProviderPrRef` from the provider scope, so the same list renders GitLab + * and Bitbucket diffs without a per-provider copy of this tree. */ export function PrReviewFilesTab({ owner, 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 index 8d01d02197..5f9d26e9ab 100644 --- a/apps/mobile/src/components/pr-review/pr-review-inbox-list.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-inbox-list.tsx @@ -24,13 +24,17 @@ import { DirectionalChevronRight } from '@/components/ui/directional-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 { + providerPrRefLabel, + providerPrRoutePath, + providerPrTermKey, +} from '@/lib/pr-review/provider-pr-ref'; +import { type ProviderInboxRow, useProviderInbox } from '@/lib/pr-review/use-provider-inbox'; import { parseTimestamp, timeAgo } from '@/lib/utils'; const SKELETON_ROW_COUNT = 5; -type InboxItem = ReturnType['items'][number]; +type InboxItem = ProviderInboxRow; type PrReviewInboxListProps = { /** The "Paste a PR link" block, rendered above the Inbox eyebrow. */ @@ -40,18 +44,33 @@ type PrReviewInboxListProps = { }; export function PrReviewInboxList({ header, recents }: Readonly) { - const { query, items, firstPageErrorState, laterPageError } = usePrInbox(true); + const inbox = useProviderInbox(true); + // Two different retries, because they recover two different failures: the + // empty-state CTA re-runs the inbox from scratch, while the footer CTA must + // load only the page (or the provider) that failed — re-fetching pages the + // list already shows would never load the missing one. + const handleRetry = () => { + inbox.refetch(); + }; + const handleRetryMore = () => { + inbox.retryFailedPages(); + }; const view = selectPrInboxView({ - isLoading: query.isPending, - itemCount: items.length, - firstPageErrorState, - laterPageError, + isLoading: inbox.isPending, + itemCount: inbox.items.length, + firstPageErrorState: inbox.firstPageErrorState, + laterPageError: inbox.laterPageError, }); + // A provider outage while the merged list happens to be empty is still a + // retryable failure, not "no review requests": keep the footer retry so the + // failing provider has a CTA the empty state itself must not carry. + const showLoadMoreRetry = + view.showLoadMoreRetry || (view.kind === 'empty' && inbox.laterPageError); return ( `${item.owner}/${item.repo}#${item.number}`} + data={view.kind === 'happy' ? inbox.items : []} + keyExtractor={item => item.key} renderItem={({ item }) => } ListHeaderComponent={ @@ -60,30 +79,18 @@ export function PrReviewInboxList({ header, recents }: Readonly } ListEmptyComponent={ - { - void query.refetch(); - }} - isRetrying={query.isFetching} - /> + } ListFooterComponent={ - {view.showLoadMoreRetry ? ( - { - void query.fetchNextPage(); - }} - /> - ) : null} + {showLoadMoreRetry ? : null} {recents} } onEndReached={() => { - if (query.hasNextPage && !query.isFetchingNextPage) { - void query.fetchNextPage(); + if (inbox.hasNextPage && !inbox.isFetchingNextPage) { + inbox.fetchNextPage(); } }} onEndReachedThreshold={0.5} @@ -124,12 +131,14 @@ function InboxRow({ item }: Readonly<{ item: InboxItem }>) { const colors = useThemeColors(); const { t } = useTranslation(); const updatedLabel = timeAgo(parseTimestamp(item.updatedAt)); - const rowLabel = `${item.owner}/${item.repo}#${item.number}`; + // `group/sub/repo!12` on GitLab, `owner/repo#7` elsewhere — the row says + // which provider it came from before the term chip repeats it in words. + const rowLabel = providerPrRefLabel(item.ref); return ( { - router.push(getPrReviewPath(item.owner, item.repo, item.number)); + router.push(providerPrRoutePath(item.ref)); }} accessibilityRole="button" accessibilityLabel={rowLabel} @@ -141,15 +150,10 @@ function InboxRow({ item }: Readonly<{ item: InboxItem }>) { - {item.owner}/{item.repo}#{item.number} · {updatedLabel} + {rowLabel} · {updatedLabel} - {item.isDraft ? ( - - - {t('common.draft')} - - - ) : null} + + {item.isDraft ? : null} @@ -157,6 +161,16 @@ function InboxRow({ item }: Readonly<{ item: InboxItem }>) { ); } +function InboxChip({ label }: Readonly<{ label: string }>) { + return ( + + + {label} + + + ); +} + function InboxEmpty({ view, onRetry, diff --git a/apps/mobile/src/components/pr-review/pr-review-merge-screen.mounted.test.tsx b/apps/mobile/src/components/pr-review/pr-review-merge-screen.mounted.test.tsx new file mode 100644 index 0000000000..5efa4cd26b --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-merge-screen.mounted.test.tsx @@ -0,0 +1,297 @@ +// The merge screen gates its sheet on the provider reads' DATA (ux2): an +// errored `providerReview.getMergeState` must not mount the GitLab sheet +// without its restrictions list, and an errored `providerReview.getCapabilities` +// must not mount the Bitbucket auto-merge sheet without its capability banner. +// The gate is data presence, not `isSuccess`: a foreground refresh whose +// refetch fails retains the last good read, and the open sheet must stay +// mounted on that retained data rather than drop the user's typed message. +// The failure body's Retry refetches the failed provider reads alongside the +// overview. Mounted with a keyed `useQuery` mock so each read can settle into a +// different state than its siblings. + +/* 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/components/pr-review/pr-review-submit.test.tsx */ +/* eslint-disable require-await, @typescript-eslint/require-await -- the fake refetch factories settle without await because they resolve immediately */ + +import type * as ReactQuery from '@tanstack/react-query'; +import { type ReactNode } from 'react'; +import { type ReactTestInstance, type ReactTestRenderer } from 'react-test-renderer'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import '@/i18n'; +import { PrReviewMergeScreen } from './pr-review-merge-screen'; +import { type ProviderPrScope, ProviderPrScopeProvider } from '@/lib/pr-review/provider-pr-ref'; +import { renderWithProviders } from '@/test/render-with-providers'; + +type MockQueryResult = { + data: unknown; + isLoading: boolean; + isPending: boolean; + isSuccess: boolean; + isError: boolean; + isFetching: boolean; + refetch: () => Promise; +}; + +type ResultKey = 'overview' | 'mergeState' | 'capabilities'; + +type MockState = { + results: Partial>; + params: Record; + scope: ProviderPrScope | null; +}; + +const mock = vi.hoisted( + (): MockState => ({ + results: {}, + params: {}, + scope: null, + }) +); + +function mockResult(overrides: Partial = {}): MockQueryResult { + return { + data: undefined, + isLoading: false, + isPending: false, + isSuccess: true, + isError: false, + isFetching: false, + refetch: vi.fn(async () => undefined), + ...overrides, + }; +} + +function resultOf(key: ResultKey): MockQueryResult { + const query = mock.results[key]; + if (query === undefined) { + throw new Error(`the test did not set a mock result for the ${key} read`); + } + return query; +} + +vi.mock('@tanstack/react-query', async importOriginal => ({ + ...(await importOriginal()), + useQuery: (options: { queryKey: readonly unknown[] }) => + mock.results[String(options.queryKey[0]) as ResultKey], +})); +vi.mock('expo-router', () => ({ + useRouter: () => ({ back: vi.fn(), push: vi.fn() }), + useLocalSearchParams: () => mock.params, +})); +vi.mock('react-native', () => ({ + View: 'View', + ActivityIndicator: 'ActivityIndicator', +})); +vi.mock('@/components/centered-state', () => ({ CenteredState: 'CenteredState' })); +vi.mock('@/components/query-error', () => ({ QueryError: 'QueryError' })); +// The screen renders the UI spinner while the overview loads; the real one +// reaches the motion policy (expo-battery), unmocked in this harness. +vi.mock('@/components/ui/activity-indicator', () => ({ ActivityIndicator: 'ActivityIndicator' })); +vi.mock('@/components/pr-review/pr-form-sheet-chrome', () => ({ + PrFormSheetHeader: 'PrFormSheetHeader', +})); +vi.mock('@/components/pr-review/merge/pr-merge-sheet', () => ({ + PrMergeSheet: 'PrMergeSheet', + providerPrNounKey: (platform: string) => + platform === 'gitlab' ? 'common.mergeRequest' : 'common.pullRequest', +})); +vi.mock('@/lib/hooks/use-theme-colors', () => ({ useThemeColors: () => ({}) })); +vi.mock('@/lib/trpc', () => ({ + trpcClient: {}, + useTRPC: () => ({ + githubPrReview: { getPullRequest: { queryOptions: () => ({ queryKey: ['overview'] }) } }, + providerReview: { + getPullRequest: { queryOptions: () => ({ queryKey: ['overview'] }) }, + getMergeState: { queryOptions: () => ({ queryKey: ['mergeState'] }) }, + getCapabilities: { queryOptions: () => ({ queryKey: ['capabilities'] }) }, + }, + }), +})); + +function ScopeWrapper({ children }: Readonly<{ children: ReactNode }>) { + const { scope } = mock; + if (scope === null) { + throw new Error('the test did not set a provider scope'); + } + return {children}; +} + +const gitlabMergeState = { + canMerge: false, + approvalsRequired: 2, + pipelineMustSucceed: true, + conflicts: false, + blockedReasons: [{ code: 'approvals', message: '2 approvals required' }], +}; + +const overviewData = { + headSha: 'abc123', + headRef: 'feature', + isCrossRepo: false, + prNodeId: 'gitlab:group/repo!12', + title: 'Ship it', + bodyMarkdown: '', + baseRef: 'main', + repo: { allowMergeCommit: true, allowSquashMerge: true, allowRebaseMerge: false }, +}; + +const autoMergeUnsupported = { supported: false, reason: 'Workspace has no auto-merge' }; + +function gitlabMergeParams() { + mock.params = { platform: 'gitlab', identity: ['group', 'repo', '12'] }; + mock.scope = { + ref: { platform: 'gitlab', projectPath: 'group/repo', mrIid: 12 }, + organizationId: null, + }; +} + +function bitbucketAutoMergeParams() { + mock.params = { platform: 'bitbucket', identity: ['ws', 'repo', '7'], mode: 'enable-auto-merge' }; + mock.scope = { + ref: { platform: 'bitbucket', workspace: 'ws', repoSlug: 'repo', prId: 7 }, + organizationId: 'org-1', + }; +} + +async function renderScreen() { + return renderWithProviders(, { wrapper: ScopeWrapper }); +} + +function findSheet(renderer: ReactTestRenderer) { + return renderer.root.findAll(node => String(node.type) === 'PrMergeSheet'); +} + +function oneSheet(renderer: ReactTestRenderer): ReactTestInstance { + const [sheet] = findSheet(renderer); + if (sheet === undefined) { + throw new Error('the merge sheet did not mount'); + } + return sheet; +} + +function findError(renderer: ReactTestRenderer) { + return renderer.root.findAll(node => String(node.type) === 'QueryError'); +} + +function oneError(renderer: ReactTestRenderer): ReactTestInstance { + const [error] = findError(renderer); + if (error === undefined) { + throw new Error('the failure body did not render'); + } + return error; +} + +beforeEach(() => { + vi.clearAllMocks(); + mock.results = { + overview: mockResult({ data: overviewData }), + mergeState: mockResult({ data: gitlabMergeState }), + capabilities: mockResult({ data: { autoMerge: autoMergeUnsupported } }), + }; +}); + +describe('PrReviewMergeScreen provider-read gating', () => { + it('keeps the GitLab sheet unmounted when getMergeState fails while the overview succeeds', async () => { + gitlabMergeParams(); + mock.results.mergeState = mockResult({ data: undefined, isSuccess: false, isError: true }); + const { renderer, unmount } = await renderScreen(); + expect(findSheet(renderer)).toHaveLength(0); + expect(findError(renderer)).toHaveLength(1); + unmount(); + }); + + it('refetches the failed merge-state read alongside the overview on Retry', async () => { + gitlabMergeParams(); + mock.results.mergeState = mockResult({ data: undefined, isSuccess: false, isError: true }); + const { renderer, unmount } = await renderScreen(); + const error = oneError(renderer); + (error.props.onRetry as () => void)(); + await vi.waitFor(() => { + expect(resultOf('overview').refetch).toHaveBeenCalledOnce(); + expect(resultOf('mergeState').refetch).toHaveBeenCalledOnce(); + }); + expect(resultOf('capabilities').refetch).not.toHaveBeenCalled(); + unmount(); + }); + + it('keeps the Bitbucket auto-merge sheet unmounted when getCapabilities fails', async () => { + bitbucketAutoMergeParams(); + mock.results.mergeState = mockResult({ data: { ...gitlabMergeState, canMerge: true } }); + mock.results.capabilities = mockResult({ data: undefined, isSuccess: false, isError: true }); + const { renderer, unmount } = await renderScreen(); + expect(findSheet(renderer)).toHaveLength(0); + expect(findError(renderer)).toHaveLength(1); + const error = oneError(renderer); + (error.props.onRetry as () => void)(); + await vi.waitFor(() => { + expect(resultOf('overview').refetch).toHaveBeenCalledOnce(); + expect(resultOf('capabilities').refetch).toHaveBeenCalledOnce(); + }); + // The merge-state read succeeded, so Retry does not refetch it. + expect(resultOf('mergeState').refetch).not.toHaveBeenCalled(); + unmount(); + }); + + it('mounts the sheet with the restrictions list and the capability banner once both reads succeed', async () => { + bitbucketAutoMergeParams(); + mock.results.mergeState = mockResult({ data: { ...gitlabMergeState, canMerge: true } }); + const { renderer, unmount } = await renderScreen(); + expect(findSheet(renderer)).toHaveLength(1); + const sheet = oneSheet(renderer); + expect(sheet.props.mergeState).toEqual({ ...gitlabMergeState, canMerge: true }); + expect(sheet.props.autoMergeCapability).toEqual(autoMergeUnsupported); + expect(findError(renderer)).toHaveLength(0); + unmount(); + }); + + it('shows one loading body while the merge-state read is in flight, not a sheet without it', async () => { + gitlabMergeParams(); + mock.results.mergeState = mockResult({ + data: undefined, + isSuccess: false, + isPending: true, + isLoading: true, + }); + const { renderer, unmount } = await renderScreen(); + expect(findSheet(renderer)).toHaveLength(0); + expect(findError(renderer)).toHaveLength(0); + expect(renderer.root.findAll(node => String(node.type) === 'CenteredState')).toHaveLength(1); + unmount(); + }); + + it('keeps the open GitLab sheet mounted when a refresh refetch of getMergeState fails on retained data', async () => { + // A foreground refresh invalidates the provider reads; the refetch errors + // but the query keeps its last good data. Gating on `isSuccess` would + // unmount the sheet and drop the commit message the user typed. + gitlabMergeParams(); + mock.results.mergeState = mockResult({ + data: gitlabMergeState, + isSuccess: false, + isError: true, + isFetching: true, + }); + const { renderer, unmount } = await renderScreen(); + expect(findSheet(renderer)).toHaveLength(1); + const sheet = oneSheet(renderer); + expect(sheet.props.mergeState).toEqual(gitlabMergeState); + expect(findError(renderer)).toHaveLength(0); + unmount(); + }); + + it('keeps the open Bitbucket auto-merge sheet mounted when a refresh refetch of getCapabilities fails on retained data', async () => { + bitbucketAutoMergeParams(); + mock.results.mergeState = mockResult({ data: { ...gitlabMergeState, canMerge: true } }); + mock.results.capabilities = mockResult({ + data: { autoMerge: autoMergeUnsupported }, + isSuccess: false, + isError: true, + isFetching: true, + }); + const { renderer, unmount } = await renderScreen(); + expect(findSheet(renderer)).toHaveLength(1); + const sheet = oneSheet(renderer); + expect(sheet.props.autoMergeCapability).toEqual(autoMergeUnsupported); + expect(findError(renderer)).toHaveLength(0); + unmount(); + }); +}); diff --git a/apps/mobile/src/components/pr-review/pr-review-merge-screen.tsx b/apps/mobile/src/components/pr-review/pr-review-merge-screen.tsx index 5689accd15..90c99d32c8 100644 --- a/apps/mobile/src/components/pr-review/pr-review-merge-screen.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-merge-screen.tsx @@ -1,6 +1,6 @@ import { useQuery } from '@tanstack/react-query'; import { useLocalSearchParams, useRouter } from 'expo-router'; -import { type ReactNode } from 'react'; +import { type ReactNode, useMemo } from 'react'; import { useTranslation } from 'react-i18next'; import { ActivityIndicator } from '@/components/ui/activity-indicator'; @@ -8,10 +8,23 @@ import { CenteredState } from '@/components/centered-state'; import { PrFormSheetHeader } from '@/components/pr-review/pr-form-sheet-chrome'; import { QueryError } from '@/components/query-error'; -import { PrMergeSheet } from '@/components/pr-review/merge/pr-merge-sheet'; +import { PrMergeSheet, providerPrNounKey } from '@/components/pr-review/merge/pr-merge-sheet'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; import { type PrMergeMethod } from '@/lib/pr-review/merge/merge-blocked-reasons'; import { parseParam } from '@/lib/route-params'; +import { + buildPrMergeStateQueryOptions, + buildPrOverviewQueryOptions, + providerCapabilitiesIdentity, + selectProviderCapabilitiesData, +} from '@/lib/pr-review/provider-pr-queries'; +import { + isProviderScopeReady, + parseProviderPrRoute, + providerPrRefLabel, + providerPrTriple, + useProviderPrScope, +} from '@/lib/pr-review/provider-pr-ref'; import { useTRPC } from '@/lib/trpc'; type Params = { @@ -20,6 +33,10 @@ type Params = { number: string; mode?: string; method?: string; + // Provider route shape (`[platform]/[...identity]/merge`). + platform?: string; + identity?: string[] | string; + instance?: string; }; const MERGE_METHODS = new Set(['merge', 'squash', 'rebase']); @@ -27,39 +44,118 @@ const MERGE_METHODS = new Set(['merge', 'squash', 'rebase']); /** * Merge formSheet route. Reads the PR + mode/method from params, fetches the * overview so the sheet has the repo settings + head SHA fence, and mounts the - * S8 merge sheet. Rendered inside the `[number]` layout's formSheet stack. + * merge sheet. Rendered inside BOTH the GitHub `[number]` layout and the + * provider `[...identity]` layout (s6): on provider arms the s2/s3 merge + * state rides along as the confirmation sheet's restrictions list, and the + * wording follows the connected provider (merge request vs pull request). */ export function PrReviewMergeScreen() { const router = useRouter(); const colors = useThemeColors(); const { t } = useTranslation(); const params = useLocalSearchParams(); - const owner = parseParam(params.owner) ?? ''; - const repo = parseParam(params.repo) ?? ''; - const rawNumber = parseParam(params.number) ?? ''; + + // The provider route carries the identity segments; the GitHub route the + // plain triple. Exactly one parses — the provider layout redirects a + // hand-built `/pr-review/github/...` link to the GitHub route. + const providerRef = useMemo( + () => + parseProviderPrRoute({ + platform: params.platform, + identity: params.identity, + instance: params.instance, + }), + [params.platform, params.identity, params.instance] + ); + const providerTriple = providerRef ? providerPrTriple(providerRef) : null; + const owner = providerTriple ? providerTriple.owner : (parseParam(params.owner) ?? ''); + const repo = providerTriple ? providerTriple.repo : (parseParam(params.repo) ?? ''); + const rawNumber = providerTriple + ? String(providerTriple.number) + : (parseParam(params.number) ?? ''); const number = Number.parseInt(rawNumber, 10); + const mode = params.mode === 'enable-auto-merge' ? 'enable-auto-merge' : 'merge'; const method: PrMergeMethod = MERGE_METHODS.has(params.method as PrMergeMethod) ? (params.method as PrMergeMethod) : 'merge'; - const sheetTitle = - mode === 'enable-auto-merge' - ? t('prReview.merge.enableAutoMerge') - : t('prReview.merge.mergePullRequest'); - const eyebrow = `${owner}/${repo}#${rawNumber}`; + const sheetTitle = (() => { + if (mode === 'enable-auto-merge') { + return t('prReview.merge.enableAutoMerge'); + } + if (providerRef && providerRef.platform !== 'github') { + return t('prReview.merge.mergeTermTitle', { + term: t(providerPrNounKey(providerRef.platform)), + }); + } + return t('prReview.merge.mergePullRequest'); + })(); + const eyebrow = providerRef ? providerPrRefLabel(providerRef) : `${owner}/${repo}#${rawNumber}`; const dismiss = () => { router.back(); }; + // The scope the reads run under: the layout publishes the provider scope in + // context; the GitHub route falls back to the parsed triple, so the GitHub + // query key is byte-identical to the pre-s6 one. + const scope = useProviderPrScope( + owner && repo && Number.isInteger(number) && number > 0 + ? { owner, repo, number } + : { owner: '', repo: '', number: 0 } + ); + const trpc = useTRPC(); - const pr = useQuery( - trpc.githubPrReview.getPullRequest.queryOptions( - { owner, repo, number }, - { enabled: Boolean(owner) && Boolean(repo) && Number.isInteger(number) && number > 0 } - ) + const overviewOptions = useMemo(() => buildPrOverviewQueryOptions(trpc, scope), [trpc, scope]); + const paramsValid = Boolean(owner) && Boolean(repo) && Number.isInteger(number) && number > 0; + const pr = useQuery({ + ...overviewOptions, + enabled: paramsValid && isProviderScopeReady(scope), + }); + + // The auto-merge capability, provider auto-merge arms only: a + // `supported: false` answer (Bitbucket) renders the capability banner. + // The GitHub arm keeps its query disabled — it never touches the + // `providerReview` namespace over the network. + const needsAutoMergeCapability = + providerRef !== null && providerRef.platform !== 'github' && mode === 'enable-auto-merge'; + // The options call sits directly in the body and the result is bound + // before `useQuery`: wrapped in a helper/useMemo closure the linter's + // inference loses the option type, and passed inline the type-checker's + // inference collapses every `.data` read. The data itself is read back + // through the seam's typed selector. + const capabilitiesOptions = trpc.providerReview.getCapabilities.queryOptions( + providerCapabilitiesIdentity(scope), + { + enabled: + needsAutoMergeCapability && scope.ref.platform !== 'github' && isProviderScopeReady(scope), + } ); + const capabilitiesQuery = useQuery(capabilitiesOptions); + const capabilitiesData = selectProviderCapabilitiesData(capabilitiesQuery.data); - if (pr.data) { + // The s2/s3 merge gate, provider arms only (GitHub's gate derives from the + // overview DTO in the merge section — the GitHub arm's query registers + // disabled and never touches the `providerReview` namespace). + const mergeStateOptions = useMemo( + () => buildPrMergeStateQueryOptions(trpc, scope), + [trpc, scope] + ); + const mergeStateQuery = useQuery(mergeStateOptions); + + // The sheet mounts only once its reads HAVE DATA, so its content never + // shifts and a doomed submit is never offered: loading → content happens in + // the screen body, not inside the sheet, and a first-load failure of a + // provider read (no data yet) keeps the sheet unmounted rather than losing + // the restrictions list (getMergeState) or the capability banner + // (getCapabilities). Data presence, not `isSuccess`: a foreground refresh + // whose refetch fails RETAINS the last good read, and gating on success + // would unmount an open sheet — dropping the commit message the user typed + // — over data the sheet still has. + const isProviderArm = scope.ref.platform !== 'github'; + const mergeStateReady = !isProviderArm || mergeStateQuery.data !== undefined; + const capabilitiesReady = !needsAutoMergeCapability || capabilitiesQuery.data !== undefined; + + if (pr.data && mergeStateReady && capabilitiesReady) { return ( { await pr.refetch(); }} @@ -85,20 +186,38 @@ export function PrReviewMergeScreen() { ); } - const body: ReactNode = pr.isLoading ? ( - - - - ) : ( - { - void pr.refetch(); - }} - isRetrying={pr.isFetching} - /> - ); + const body: ReactNode = + pr.isLoading || + (isProviderArm && mergeStateQuery.isLoading) || + (needsAutoMergeCapability && capabilitiesQuery.isLoading) ? ( + + + + ) : ( + { + // Retry recovers every read the screen is waiting on: the overview + // and, on the provider arms, the errored merge gate / capability + // read — a Retry that refetched only the overview could never + // clear the failure that kept the sheet from mounting. Each + // refetch starts as it is called, so the reads run concurrently. + void pr.refetch(); + if (isProviderArm && mergeStateQuery.isError) { + void mergeStateQuery.refetch(); + } + if (needsAutoMergeCapability && capabilitiesQuery.isError) { + void capabilitiesQuery.refetch(); + } + }} + isRetrying={ + pr.isFetching || + (isProviderArm && mergeStateQuery.isFetching) || + (needsAutoMergeCapability && capabilitiesQuery.isFetching) + } + /> + ); return ( <> diff --git a/apps/mobile/src/components/pr-review/pr-review-overview-parts.tsx b/apps/mobile/src/components/pr-review/pr-review-overview-parts.tsx index 6e982720af..832fac0fa6 100644 --- a/apps/mobile/src/components/pr-review/pr-review-overview-parts.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-overview-parts.tsx @@ -180,7 +180,8 @@ export function PrCountsLine({ additions, deletions, }: Readonly<{ - commits: number; + /** Null where the provider reports no commit count; the chip is dropped. */ + commits: number | null; changedFiles: number; additions: number; deletions: number; @@ -189,12 +190,15 @@ export function PrCountsLine({ const { t } = useTranslation(); return ( - - - - {formatNumber(commits, i18n.language)} {t('prReview.overview.commit', { count: commits })} - - + {commits === null ? null : ( + + + + {formatNumber(commits, i18n.language)}{' '} + {t('prReview.overview.commit', { count: commits })} + + + )} diff --git a/apps/mobile/src/components/pr-review/pr-review-overview.tsx b/apps/mobile/src/components/pr-review/pr-review-overview.tsx index 0647bf47bc..d17a1b8e01 100644 --- a/apps/mobile/src/components/pr-review/pr-review-overview.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-overview.tsx @@ -14,6 +14,7 @@ import { MarkdownText } from '@/components/agents/markdown-text'; import { PrOverviewMeta } from '@/components/pr-review/pr-review-meta-parts'; import { PrReviewChecksSection } from '@/components/pr-review/pr-review-checks-section'; import { PrMergeSection } from '@/components/pr-review/merge/pr-merge-section'; +import { PrMergeSectionProvider } from '@/components/pr-review/merge/pr-merge-section-provider'; import { describePrState, formatPrCounts, @@ -28,8 +29,9 @@ import { getGitHubIntegrationUrl } from '@/lib/agent-github-integration'; import { WEB_BASE_URL } from '@/lib/config'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; import { classifyPrReviewQueryState } from '@/lib/pr-review/classify-pr-review-query-state'; +import { useProviderPrQueries } from '@/lib/pr-review/provider-pr-queries'; import { useCheckGitHubConnection } from '@/lib/pr-review/use-check-github-connection'; -import { trpcClient, useTRPC } from '@/lib/trpc'; +import { trpcClient } from '@/lib/trpc'; const REVIEW_SUBMIT_PATH = '/(app)/pr-review/[owner]/[repo]/[number]/review-submit' as const; @@ -76,13 +78,19 @@ export function PrReviewOverview({ isActive: _isActive, refreshControl, }: PrReviewOverviewProps) { - const trpc = useTRPC(); + const queries = useProviderPrQueries({ owner, repo, number }); const connection = useCheckGitHubConnection(); const router = useRouter(); const colors = useThemeColors(); const { t } = useTranslation(); - const pr = useQuery(trpc.githubPrReview.getPullRequest.queryOptions({ owner, repo, number })); + // GitHub-only affordances: the install/reconnect CTAs and the review-submit + // sheet are GitHub App flows and GitHub-route siblings. On GitLab and + // Bitbucket the same states render without a CTA that cannot work. + const isGitHub = queries.platform === 'github'; + const isMergeRequest = queries.platform === 'gitlab'; + + const pr = useQuery(queries.overviewOptions()); const handleOpenReviewSubmit = useCallback(() => { const href: Href = { @@ -112,12 +120,22 @@ export function PrReviewOverview({ - {t('prReview.installKiloGitHubApp')} - + isGitHub ? ( + + ) : null } /> ); @@ -129,7 +147,11 @@ export function PrReviewOverview({ refreshControl={refreshControl} icon={GitPullRequest} title={t('common.accessDenied')} - description={t('prReview.accessDeniedDescription')} + description={ + isMergeRequest + ? t('prReview.terms.accessDeniedMergeRequest') + : t('prReview.accessDeniedDescription') + } /> ); } @@ -141,15 +163,17 @@ export function PrReviewOverview({ title={t('prReview.reconnectNotice.title')} description={t('prReview.reconnectNotice.message')} action={ - + isGitHub ? ( + + ) : null } /> ); @@ -159,7 +183,11 @@ export function PrReviewOverview({ { void pr.refetch(); }} @@ -232,30 +260,38 @@ export function PrReviewOverview({ - - - {t('prReview.review')} - - - + {isGitHub ? ( + + + {t('prReview.review')} + + + + ) : null} - { - await pr.refetch(); - }} - isRefetching={pr.isFetching} - /> + {isGitHub ? ( + { + await pr.refetch(); + }} + isRefetching={pr.isFetching} + /> + ) : ( + // The provider merge arm (s6): the merge affordance and the + // capability-gated auto-merge row, pushing the ref's own sheet route. + + )} {t('prReview.headLine', { diff --git a/apps/mobile/src/components/pr-review/pr-review-provider-noun.ts b/apps/mobile/src/components/pr-review/pr-review-provider-noun.ts new file mode 100644 index 0000000000..37c2f27219 --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-provider-noun.ts @@ -0,0 +1,15 @@ +// The provider's own noun in sentence form (s6). The `prReview.terms.*` +// labels are capitalized for chips and headers, so a mid-sentence +// `{{term}}` interpolation rides the lowercase `common.*` nouns instead — +// "Merge Merge request?" never renders. Standalone (no React imports) so +// the merge sheet, the merge screen and the overview's provider merge arm +// share the one mapping without pulling each other's bundles together. + +import { type ProviderPrPlatform } from '@kilocode/app-shared/provider-review'; + +export function providerPrNounKey( + platform: ProviderPrPlatform +): 'common.mergeRequest' | 'common.pullRequest' { + // i18n-dup-ok: mid-sentence lowercase noun for {{term}} interpolation vs the capitalized standalone prReview.terms.* label; languages case-decline them apart. + return platform === 'gitlab' ? 'common.mergeRequest' : 'common.pullRequest'; +} diff --git a/apps/mobile/src/components/pr-review/pr-review-provider-sheet-href.ts b/apps/mobile/src/components/pr-review/pr-review-provider-sheet-href.ts new file mode 100644 index 0000000000..c787e4dfd4 --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-provider-sheet-href.ts @@ -0,0 +1,40 @@ +// The write-sheet routes inside the provider layout (s6). The comment +// composer, the review-submit sheet and the merge sheet are children of the +// PR's own route on every provider: the provider scope and the +// `PendingReviewProvider` queue are published by the provider layout, so a +// provider sheet must be reached through the provider route — pushing the +// GitHub sibling would leave that scope and write to the wrong provider. The +// GitHub route keeps its own literal paths untouched. +import { type Href } from 'expo-router'; + +import { type ProviderPrRef, providerPrRouteSegments } from '@/lib/pr-review/provider-pr-ref'; + +export type ProviderPrSheetRoute = 'comment-composer' | 'review-submit' | 'merge'; + +/** + * The href for one sheet under the ref's own route. A GitLab `instanceHint` + * rides as the `instance` query param — the same param the provider layout + * reads for the base route — so the sheet stays on the instance the reader + * opened. Extra params (composer position, merge mode) ride as query params. + */ +export function providerPrSheetHref( + ref: ProviderPrRef, + sheet: ProviderPrSheetRoute, + params: Record = {} +): Href { + const { platform, identity } = providerPrRouteSegments(ref); + const encoded = identity.map(segment => encodeURIComponent(segment)).join('/'); + // The path is built at runtime, so it never appears in the generated + // typed-routes literal union; like `providerPrHref` (provider-pr-ref.ts), + // the params ride in an encoded query string and the href is the cast + // string. + const queryParts: string[] = []; + if (ref.platform === 'gitlab' && ref.instanceHint) { + queryParts.push(`instance=${encodeURIComponent(ref.instanceHint)}`); + } + for (const [key, value] of Object.entries(params)) { + queryParts.push(`${encodeURIComponent(key)}=${encodeURIComponent(String(value))}`); + } + const search = queryParts.length > 0 ? `?${queryParts.join('&')}` : ''; + return `/(app)/pr-review/${platform}/${encoded}/${sheet}${search}` as Href; +} diff --git a/apps/mobile/src/components/pr-review/pr-review-reconnect-notice.tsx b/apps/mobile/src/components/pr-review/pr-review-reconnect-notice.tsx index 1ea7172108..87a8bc34e5 100644 --- a/apps/mobile/src/components/pr-review/pr-review-reconnect-notice.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-reconnect-notice.tsx @@ -3,24 +3,58 @@ import { View } from 'react-native'; import { Button } from '@/components/ui/button'; import { Text } from '@/components/ui/text'; +import { useProviderPrScopeOrNull } from '@/lib/pr-review/provider-pr-ref'; import { useCheckGitHubConnection } from '@/lib/pr-review/use-check-github-connection'; +import { useCheckProviderConnection } from '@/lib/pr-review/use-check-provider-connection'; +/** + * The mid-session recovery notice for an expired provider connection. The + * surface that caught the precondition failure renders it; this component + * decides WHICH connection to re-check from the provider scope the route + * published — the GitHub route publishes none, so the GitHub arm (and the + * original copy) is the fallback. A GitLab or Bitbucket surface re-checks + * its own integration status instead, because a GitHub retry can never fix + * an expired GitLab connection. + */ export function PrReviewReconnectNotice() { const connection = useCheckGitHubConnection(); + const providerConnection = useCheckProviderConnection(); + const scope = useProviderPrScopeOrNull(); + const platform = scope?.ref.platform ?? 'github'; + const organizationId = scope?.organizationId ?? null; + const providerPlatform = platform === 'github' ? null : platform; const { t } = useTranslation(); + let title = t('prReview.reconnectNotice.title'); + if (platform === 'gitlab') { + title = t('prReview.reconnectNotice.gitlabTitle'); + } else if (platform === 'bitbucket') { + title = t('prReview.reconnectNotice.bitbucketTitle'); + } + const message = + providerPlatform === null + ? t('prReview.reconnectNotice.message') + : t('prReview.reconnectNotice.providerMessage', { + provider: + platform === 'gitlab' + ? t('common.gitlab') + : t('agentChat.repoPicker.platformBitbucket'), + }); + return ( - - {t('prReview.reconnectNotice.title')} - - {t('prReview.reconnectNotice.message')} + {title} + {message}