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/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/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/pr-diff-file-list-header.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-file-list-header.tsx index 6695116faa..73eb8d1ea1 100644 --- a/apps/mobile/src/components/pr-review/diff/pr-diff-file-list-header.tsx +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-file-list-header.tsx @@ -21,6 +21,7 @@ import { formatNumber } from '@/lib/format'; import { useIsTablet } from '@/lib/hooks/use-is-tablet'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; import { type DiffViewMode } from '@/lib/pr-review/diff/pr-diff-list-items'; +import { providerPrChildRoutePath, useProviderPrScope } from '@/lib/pr-review/provider-pr-ref'; import { cn } from '@/lib/utils'; type PrDiffFileListHeaderProps = { @@ -34,8 +35,6 @@ type PrDiffFileListHeaderProps = { readonly onViewModeChange: (mode: DiffViewMode) => 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 a8b16de647..dd0dd7dd8e 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,21 @@ import { type RefreshControlProps } from 'react-native'; import TestRenderer, { act } from 'react-test-renderer'; import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { + PR_DIFF_FLOATING_ACTIONS_FALLBACK_HEIGHT, + PR_DIFF_LIST_FOOTER_GAP, +} from '@/lib/pr-review/diff/pr-diff-list-bottom-padding'; +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, @@ -88,7 +99,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, @@ -129,6 +143,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[] { @@ -211,3 +248,86 @@ 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('reserves the bar-sized gap under a provider diff list too', () => { + const githubPadding = listBottomPadding(mountList()); + const gitlabPadding = listBottomPadding( + mountListInScope({ platform: 'gitlab', projectPath: 'group/repo', mrIid: 12 }) + ); + // The bar renders on every provider (s6), so every list reserves the + // bar's fallback height plus the footer gap — the last diff row is + // never hidden under the bar. + const expected = PR_DIFF_FLOATING_ACTIONS_FALLBACK_HEIGHT + PR_DIFF_LIST_FOOTER_GAP; + expect(githubPadding).toBe(expected); + expect(gitlabPadding).toBe(expected); + }); +}); 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 b6108f3fa9..6d50c19dbf 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 @@ -26,7 +26,7 @@ import { FlashList, type FlashListRef } from '@shopify/flash-list'; import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { useTranslation } from 'react-i18next'; -import { RefreshControl, View, type ViewStyle } from 'react-native'; +import { RefreshControl, View } from 'react-native'; import { QueryError } from '@/components/query-error'; import { @@ -40,11 +40,13 @@ 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 { prDiffListBottomPadding } from '@/lib/pr-review/diff/pr-diff-list-bottom-padding'; +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'; @@ -90,7 +92,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>({}); @@ -134,11 +141,20 @@ export function PrReviewFileList({ }); }, []); - const contentContainerStyle = useMemo( + // 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 list's bottom padding + // reserves the bar's space at its measured (or fallback) height, so the + // last diff row is never hidden under it. + const listContentStyle = 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; for (const file of files) { @@ -261,19 +277,11 @@ export function PrReviewFileList({ if (files.length === 0) { if (firstPageErrorState?.kind === 'not-found') { - return ( - - ); + return ; } if (firstPageErrorState?.kind === 'permission') { return ( - + ); } if (firstPageErrorState?.kind === 'reconnect') { @@ -300,6 +308,7 @@ export function PrReviewFileList({ return ( 0 ? ( @@ -356,7 +365,7 @@ export function PrReviewFileList({ } }} onEndReachedThreshold={0.5} - contentContainerStyle={contentContainerStyle} + contentContainerStyle={listContentStyle} ItemSeparatorComponent={null} /> )} @@ -364,6 +373,7 @@ export function PrReviewFileList({ owner={owner} repo={repo} number={number} + prRef={scope.ref.platform === 'github' ? undefined : scope.ref} viewMode={effectiveViewMode} selection={selection} onClearSelection={clearSelection} 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 30bb7ad0bc..6623809403 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 })); @@ -152,7 +158,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, })); @@ -570,3 +579,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 7950d05e08..eed1cb2364 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 @@ -49,6 +49,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` @@ -114,7 +115,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.test.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.test.tsx index 565b69700a..31bbf4c71f 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,67 @@ 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({ + pathname: '/(app)/pr-review/gitlab/group/sub/repo/12/comment-composer', + params: { path: 'src/lib.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, expectedPathname) => { + pressButtonWith(prRef, 'Finish review'); + + expect(routerPush).toHaveBeenCalledTimes(1); + expect(routerPush).toHaveBeenCalledWith({ pathname: expectedPathname, params: {} }); + }); +}); + describe('PrDiffFloatingActions bottom inset (plan §6)', () => { beforeEach(() => { insets.bottom = 0; 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..08186f03d9 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 @@ -18,9 +18,11 @@ 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,6 +34,13 @@ 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. */ @@ -46,6 +55,7 @@ export function PrDiffFloatingActions({ owner, repo, number, + prRef, viewMode, selection, onClearSelection, @@ -70,22 +80,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 }, 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-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 9bbd0574fa..3600706c67 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 e34b5dbe56..6670790de5 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 @@ -63,6 +63,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', })); @@ -98,6 +101,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: () => ({}) }, + }, }), })); 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 97fa9e45f9..27c5a9ea52 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 @@ -15,12 +15,38 @@ 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. +type TerminalNounKey = 'common.mergeRequest' | 'common.pullRequest'; // i18n-dup-ok: one copy, two senses: mid-sentence lowercase noun vs capitalized standalone label; languages case-decline them apart. + +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..6ce4e0b65b --- /dev/null +++ b/apps/mobile/src/components/pr-review/merge/pr-merge-section-provider.test.tsx @@ -0,0 +1,165 @@ +/* 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' })); + +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({ + pathname: '/(app)/pr-review/gitlab/group/sub/repo/12/merge', + params: { 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({ + pathname: '/(app)/pr-review/gitlab/group/sub/repo/12/merge', + params: { 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,504 @@ 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(); + }); +}); + +// ── 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..d9b1067fe1 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,11 +58,16 @@ 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'; @@ -69,15 +91,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. + */ +export 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 @@ -113,22 +187,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 +242,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 +290,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 +310,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 +337,7 @@ export function PrMergeSheet(props: PrMergeSheetProps) { setInlineErrorKind('retryable'); } } - }, [lastError, t]); + }, [lastError, t, providerPlatform]); useEffect(() => { const sub = Keyboard.addListener('keyboardDidShow', () => { @@ -242,7 +354,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 +389,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 +468,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 +485,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 +499,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,7 +519,116 @@ 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 ( + <> + + + + {cancelOnlyFooter()} + + ); + } + if (providerAutoMerge) { + return ( + <> + + + + + + + ); + } + if (mergeState && !mergeState.canMerge) { + // A blocked merge state replaces the form with the restrictions list + // (nothing to submit). + return ( + <> + + + + {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 ( <> @@ -376,30 +643,112 @@ export function PrMergeSheet(props: PrMergeSheetProps) { setScrollViewportHeight(event.nativeEvent.layout.height); }} > - {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-review-capability-banner.mounted.test.tsx b/apps/mobile/src/components/pr-review/pr-review-capability-banner.mounted.test.tsx new file mode 100644 index 0000000000..456b4794d1 --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-capability-banner.mounted.test.tsx @@ -0,0 +1,82 @@ +/* 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) */ +import { createElement } from 'react'; +import TestRenderer, { act } from 'react-test-renderer'; +import { describe, expect, it, vi } from 'vitest'; + +import '@/i18n'; +import { type ProviderReviewCapability } from '@kilocode/app-shared/provider-review'; +import type * as ReactI18next from 'react-i18next'; +import { PrReviewCapabilityBanner } from './pr-review-capability-banner'; + +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', () => ({ 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 a08f8b69f9..6fec4d266c 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,11 @@ /* eslint-disable typescript-eslint/no-deprecated -- react-test-renderer is the repository's native-free mounted test tool. */ import { createElement } from 'react'; import TestRenderer, { act } from 'react-test-renderer'; -import { describe, expect, it, vi } from 'vitest'; +import { 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(() => ({ @@ -83,7 +85,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', () => { @@ -117,3 +122,85 @@ 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(); + }); +}); 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 705e51c57b..5be63a710f 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 @@ -9,7 +9,6 @@ import { MinusCircle, XCircle, } from '@/components/ui/icons'; -import { useMemo } from 'react'; import { useTranslation } from 'react-i18next'; import { Pressable, View } from 'react-native'; @@ -18,10 +17,12 @@ import { Button } from '@/components/ui/button'; 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'; @@ -179,17 +180,13 @@ 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. @@ -273,6 +270,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 }; @@ -286,18 +288,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 d03bfb59b1..2eeae002e6 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 { ActivityIndicator, Alert } from 'react-native'; @@ -13,6 +13,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 = { @@ -24,30 +32,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); @@ -82,6 +136,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} @@ -103,6 +158,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} 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..ec50ea7e99 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 @@ -14,6 +14,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(); @@ -153,6 +154,18 @@ vi.mock('@/lib/pr-review/pending-review-provider', () => ({ })); vi.mock('@/lib/pr-review/use-pr-review-mutations', () => ({ + formatPendingCommentBody: (item: { + path: string; + line: number; + startLine?: number; + body: string; + }) => { + const location = + item.startLine !== undefined && item.startLine !== item.line + ? `${item.path}:L${item.startLine}–L${item.line}` + : `${item.path}:L${item.line}`; + return `${location}\n\n${item.body}`; + }, useCreateReviewCommentMutation: () => ({ mutateAsync: createCommentMocks.mutateAsync, isPending: createCommentMocks.isPending, @@ -287,3 +300,55 @@ 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 body-only 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 body-only through the provider arm, with the location anchored in the text', async () => { + createCommentMocks.mutateAsync.mockResolvedValueOnce({}); + const element = mountProviderComposer(); + typeBody(element, 'hello'); + footerProp(element, 'onCommentNow')?.(); + await flushMicrotasks(); + + expect(createCommentMocks.mutateAsync).toHaveBeenCalledWith({ + body: 'src/a.ts:L10\n\nhello', + }); + }); + + 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..31bac6df50 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 @@ -35,7 +35,11 @@ import { useFencedDraftLoad } from '@/lib/persist/use-draft-load'; 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 { + formatPendingCommentBody, + 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 +49,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 location + * anchored in the body, 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 +74,7 @@ export function PrReviewCommentComposer(props: PrReviewCommentComposerProps) { owner, repo, number, + prRef, mode, path, side, @@ -74,13 +87,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 +217,23 @@ 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 body-only: the inline position rides in the + // text, because the provider APIs have no comment position. + await createComment.mutateAsync( + prRef + ? { body: formatPendingCommentBody({ path, line, startLine, body }) } + : { + 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 deleted file mode 100644 index fd304a7bec..0000000000 --- a/apps/mobile/src/components/pr-review/pr-review-connect-gate-view.test.ts +++ /dev/null @@ -1,187 +0,0 @@ -import * as React from 'react'; -import { describe, expect, it, vi } from 'vitest'; - -import '@/i18n'; -import type * as ReactI18next from 'react-i18next'; -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(); - return { - ...actual, - useTranslation: () => { - const i18n = actual.getI18n(); - return { t: i18n.t.bind(i18n), i18n }; - }, - }; -}); - -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, -// so `isLoading` is false and the gate would otherwise fall through to -// Connect on a cold launch before NetInfo settles. This pins that wiring: -// a revert to `isLoading` would make the paused query render Connect and -// fail the assertions below. -// -// 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. - -let authorizationQueryResult = { - data: undefined as unknown, - isPending: true, - isLoading: false, - isError: false, - isFetching: false, - refetch: vi.fn(), -}; - -vi.mock('react', async () => { - const actual = await vi.importActual('react'); - return { - ...actual, - useCallback: vi.fn( unknown>(fn: T) => fn), - useState: vi.fn((initial: T) => [initial, vi.fn() as () => void] as [T, (value: T) => void]), - useRef: vi.fn((initial: T) => ({ current: initial })), - useEffect: vi.fn(), - }; -}); - -vi.mock('@tanstack/react-query', () => ({ - useQuery: () => authorizationQueryResult, - useMutation: () => ({ mutateAsync: vi.fn() }), - useQueryClient: () => ({ invalidateQueries: vi.fn() }), -})); - -vi.mock('@/lib/trpc', () => ({ - useTRPC: () => ({ - githubApps: { - getUserAuthorization: { queryOptions: () => ({}), queryKey: () => [] }, - connectUserAuthorization: { mutationOptions: () => ({}) }, - }, - }), -})); - -vi.mock('@/lib/hooks/use-theme-colors', () => ({ - useThemeColors: () => ({ mutedForeground: '#6F6A61', primaryForeground: '#FFFFFF' }), -})); - -vi.mock('react-native-safe-area-context', () => ({ - useSafeAreaInsets: () => ({ bottom: 0 }), -})); - -vi.mock('sonner-native', () => ({ toast: { error: vi.fn() } })); - -vi.mock('@/lib/pr-review/connect-gate-platform', () => ({ - openAuthorizationAndWaitForReturn: vi.fn(), -})); - -vi.mock('@/components/ui/icons', () => ({ - PlugZap: 'PlugZap', - RefreshCcw: 'RefreshCcw', - ShieldAlert: 'ShieldAlert', -})); - -vi.mock('@/components/centered-state', () => ({ CenteredState: 'CenteredState' })); -vi.mock('@/components/empty-state', () => ({ EmptyState: 'EmptyState' })); -vi.mock('@/components/query-error', () => ({ QueryError: 'QueryError' })); -vi.mock('@/components/screen-header', () => ({ ScreenHeader: 'ScreenHeader' })); -vi.mock('@/components/ui/button', () => ({ Button: 'Button' })); -vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); - -vi.mock('react-native', () => ({ - ActivityIndicator: 'ActivityIndicator', - AppState: { addEventListener: vi.fn(() => ({ remove: vi.fn() })) }, - Platform: { OS: 'ios' }, - View: 'View', -})); - -function containsType(node: unknown, type: string): boolean { - if (Array.isArray(node)) { - return node.some(child => containsType(child, type)); - } - if (React.isValidElement(node)) { - const element = node; - if (element.type === type) { - return true; - } - const props = element.props as { children?: unknown }; - return containsType(props.children, type); - } - return false; -} - -describe('PrReviewConnectGate wiring', () => { - it('shows loading, not Connect, for a paused authorization query with no data', () => { - authorizationQueryResult = { - data: undefined, - isPending: true, - isLoading: false, - isError: false, - isFetching: false, - refetch: vi.fn(), - }; - - // eslint-disable-next-line new-cap - const tree = PrReviewConnectGate({ children: null }); - - expect(containsType(tree, 'ActivityIndicator')).toBe(true); - expect(containsType(tree, 'EmptyState')).toBe(false); - }); -}); 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 474c0dcc2f..e457816bc8 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,8 +1,9 @@ import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query'; import { PlugZap, RefreshCcw, ShieldAlert } from '@/components/ui/icons'; -import { type ReactNode, useCallback, useState } from 'react'; +import { type ReactNode, useCallback, useMemo, useState } from 'react'; import { useTranslation } from 'react-i18next'; import { ActivityIndicator, Platform, View } from 'react-native'; +import { usePathname } from 'expo-router'; import { CenteredState } from '@/components/centered-state'; import { toast } from 'sonner-native'; @@ -11,36 +12,79 @@ 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'; 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(); @@ -97,6 +141,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) @@ -106,6 +151,7 @@ export function PrReviewConnectGate({ children }: PrReviewConnectGateProps) { isLoading: authorization.isPending, connected: authorization.data?.connected === true, revoked: authorization.data?.revoked === true, + organizationId: null, }); if (view === 'error') { @@ -172,3 +218,176 @@ export function PrReviewConnectGate({ children }: PrReviewConnectGateProps) { 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.test.tsx b/apps/mobile/src/components/pr-review/pr-review-discussion-tab.test.tsx index c29a18c02d..a2922dc3f9 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 })); @@ -56,10 +58,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) { @@ -168,6 +179,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..5b5cd6735c 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 @@ -70,6 +70,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 +101,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); @@ -219,7 +225,14 @@ export function PrReviewDiscussionTab({ if (view.kind === 'permission') { return ( - + ); } if (view.kind === 'not-found') { @@ -227,7 +240,11 @@ export function PrReviewDiscussionTab({ ); } @@ -278,7 +295,11 @@ export function PrReviewDiscussionTab({ ({ + push: vi.fn(), + alert: vi.fn(), + toastError: vi.fn(), + clipboard: { current: '' as string }, +})); + +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: (cb: () => (() => void) | void) => { + cb(); + }, + useRouter: () => ({ push: mocks.push }), +})); + +vi.mock('expo-clipboard', () => ({ + getStringAsync: async () => mocks.clipboard.current, +})); + +// The store doubles as the recents disk: tests seed it directly and assert +// removals by reading it back. +const store = new Map(); +vi.mock('expo-secure-store', () => ({ + getItemAsync: async (key: string) => store.get(key) ?? null, + setItemAsync: async (key: string, value: string) => { + store.set(key, value); + }, + deleteItemAsync: async (key: string) => { + 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) => write(), + deleteAccountMetadata: async (key: string) => { + store.delete(key); + }, +})); + +vi.mock('react-native', () => ({ + ActivityIndicator: 'ActivityIndicator', + Alert: { alert: mocks.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/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: mocks.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++; + 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++; + if (!(index in hookSlots)) { + hookSlots[index] = { current: initial }; + } + return hookSlots[index]; + }, + useCallback: (fn: unknown) => fn, + useMemo: (factory: () => unknown) => factory(), + }; +}); + +// Imported after the mocks so the module graph sees them. +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); + } +} + +function findAll(tree: unknown, typeName: string): El[] { + const out: El[] = []; + collect(tree, typeName, out); + return out; +} + +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; +} + +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); +} + +function render(): unknown { + hookIndex = 0; + return PrReviewEntryScreen(); +} + +/** Flush the mocked SecureStore's microtask chain to completion. */ +function flush(): Promise { + return new Promise(resolve => { + setTimeout(resolve, 0); + }); +} + +async function renderLoaded(): Promise { + render(); + // Flush the focus-effect recents load. + await flush(); + return render(); +} + +function seedRecents(entries: RecentPr[]): void { + store.set('pr-review-recents', JSON.stringify(entries)); +} + +function storedRecents(): RecentPr[] { + return JSON.parse(store.get('pr-review-recents') ?? '[]') as RecentPr[]; +} + +const SAME_TRIPLE = { owner: 'acme', repo: 'api', number: 7, lastOpenedAt: 1_700_000_000_000 }; + +beforeEach(() => { + vi.clearAllMocks(); + hookSlots = []; + store.clear(); + mocks.clipboard.current = ''; +}); + +afterEach(() => { + store.clear(); +}); + +describe('provider-neutral URL field', () => { + it('labels and placeholders name both review nouns, no provider host', async () => { + seedRecents([]); + 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 () => { + seedRecents([]); + const tree = await renderLoaded(); + const input = find(tree, 'TextInput', () => true); + (input.props?.onChangeText as (value: string) => void)( + 'https://github.com/octocat/hello-world/pull/42' + ); + const open = render(); + ( + find(open, 'Button', p => p.accessibilityLabel === 'Open pull request or merge request') + .props?.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 () => { + seedRecents([]); + const tree = await renderLoaded(); + const input = find(tree, 'TextInput', () => true); + (input.props?.onChangeText as (value: string) => void)( + 'https://gitlab.example.com/group/sub/repo/-/merge_requests/9' + ); + const open = render(); + ( + find(open, 'Button', p => p.accessibilityLabel === 'Open pull request or merge request') + .props?.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 () => { + seedRecents([]); + const tree = await renderLoaded(); + const input = find(tree, 'TextInput', () => true); + (input.props?.onChangeText as (value: string) => void)( + 'https://bitbucket.org/acme/api/pull-requests/7/overview' + ); + const open = render(); + ( + find(open, 'Button', p => p.accessibilityLabel === 'Open pull request or merge request') + .props?.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 () => { + seedRecents([]); + const tree = await renderLoaded(); + const input = find(tree, 'TextInput', () => true); + (input.props?.onChangeText as (value: string) => void)('https://example.com/blog/post'); + const open = render(); + ( + find(open, 'Button', p => p.accessibilityLabel === 'Open pull request or merge request') + .props?.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 () => { + seedRecents([]); + 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' + ); + (paste.props?.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 () => { + seedRecents([]); + 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' + ); + (paste.props?.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 () => { + seedRecents([]); + const before = await renderLoaded(); + expect(findAll(before, 'Pressable').some(p => p.props?.accessibilityLabel === 'Clear link')).toBe( + false + ); + const input = find(before, 'TextInput', () => true); + (input.props?.onChangeText as (value: string) => void)('anything'); + const after = render(); + expect( + find(after, 'Pressable', p => p.accessibilityLabel === 'Clear link') + ).toBeTruthy(); + }); +}); + +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(); + (row.props?.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' + ); + (removeGitLab.props?.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 destructive = ( + mocks.alert.mock.calls[0]?.[2] as { text: string; onPress?: () => void }[] + ).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'); + (retry.props?.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.tsx b/apps/mobile/src/components/pr-review/pr-review-entry-screen.tsx index e8d5dd83dd..ae3da8edce 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 @@ -8,24 +8,31 @@ import { ActivityIndicator, Alert, Pressable, TextInput, View } from 'react-nati 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, + recentPrKey, + type RecentPr, + removeRecentPr, +} from '@/lib/pr-review/recent-prs'; export function PrReviewEntryScreen() { const router = useRouter(); @@ -71,16 +78,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 () => { @@ -104,9 +113,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) => { @@ -156,11 +167,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} @@ -239,7 +257,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..a95accfa67 --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-file-navigator-screen.test.tsx @@ -0,0 +1,132 @@ +/* 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' })); +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 1201218e85..a842acd26d 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 @@ -6,11 +6,14 @@ import { ActivityIndicator, View } from 'react-native'; 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; @@ -22,29 +25,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(); @@ -76,9 +88,8 @@ export function PrReviewFileNavigatorScreen() { ) : ( - { void pr.refetch(); }} @@ -88,3 +99,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.tsx b/apps/mobile/src/components/pr-review/pr-review-merge-screen.tsx index b11bfebafd..3352b86fcc 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 'react-native'; @@ -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,112 @@ 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 settle, so its content never shifts: + // loading → content happens in the screen body, not inside the sheet. + const isProviderArm = scope.ref.platform !== 'github'; + const mergeStateSettled = + !isProviderArm || (!mergeStateQuery.isLoading && !mergeStateQuery.isPending); + const capabilitiesSettled = !needsAutoMergeCapability || !capabilitiesQuery.isPending; + + if (pr.data && mergeStateSettled && capabilitiesSettled) { return ( { await pr.refetch(); }} @@ -85,20 +180,23 @@ 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) ? ( + + + + ) : ( + { + void pr.refetch(); + }} + isRetrying={pr.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..4892bd0182 --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-provider-noun.ts @@ -0,0 +1,14 @@ +// 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' { + 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..a8e19d4dab --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-provider-sheet-href.ts @@ -0,0 +1,33 @@ +// 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('/'); + const instance = + ref.platform === 'gitlab' && ref.instanceHint ? { instance: ref.instanceHint } : {}; + return { + pathname: `/(app)/pr-review/${platform}/${encoded}/${sheet}`, + params: { ...instance, ...params }, + }; +} 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..eefea32abe 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,56 @@ 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(); + const title = + platform === 'gitlab' + ? t('prReview.reconnectNotice.gitlabTitle') + : platform === 'bitbucket' + ? t('prReview.reconnectNotice.bitbucketTitle') + : t('prReview.reconnectNotice.title'); + 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}