diff --git a/apps/mobile/src/components/agents/repository-branch-selector.mounted.test.tsx b/apps/mobile/src/components/agents/repository-branch-selector.mounted.test.tsx index 18f623894b..d326d5c17c 100644 --- a/apps/mobile/src/components/agents/repository-branch-selector.mounted.test.tsx +++ b/apps/mobile/src/components/agents/repository-branch-selector.mounted.test.tsx @@ -196,6 +196,71 @@ describe('RepositoryBranchSelector', () => { expect(pressableWithLabel(loaded, branchLabel('main'))?.props.className).toContain('h-12'); }); + // Spot check e4-branch-03/04: the screenshots showed a blue "refreshing" + // overlay floating over the prompt card and over the screen header while + // branches loaded. That overlay belonged to the old inline picker; the row + // is now an in-flow fixed-height row and the picker is its own opaque + // formSheet route. These two tests pin the replacement: the row never + // carries an absolute-positioned node in any state (nothing can cover the + // card above or the header), and the three fixed states share one height. + it('renders every branch-row state in-flow — no node overlays the form', () => { + const states: RepositoryBranchesState[] = [ + branchesState({ isLoading: true }), + branchesState(), + branchesState({ isRetryableError: true }), + branchesState({ isPermanentError: true }), + branchesState({ branches: [], defaultBranch: null }), + branchesState({ isEnabled: false }), + ]; + for (const state of states) { + const renderer = mountSelector(githubRow, state); + const floating = renderer.root.findAll(node => { + const classes = + typeof node.props.className === 'string' ? node.props.className.split(' ') : []; + const style = node.props.style as { position?: string } | undefined; + return classes.includes('absolute') || style?.position === 'absolute'; + }); + expect(floating).toHaveLength(0); + } + }); + + it('keeps skeleton, trigger, and error row at the same fixed height', () => { + for (const state of [ + branchesState({ isLoading: true }), + branchesState(), + branchesState({ isRetryableError: true }), + ]) { + const renderer = mountSelector(githubRow, state); + const row = renderer.root.findAll(node => { + const classes = + typeof node.props.className === 'string' ? node.props.className.split(' ') : []; + return classes.includes('h-12'); + }); + expect(row).toHaveLength(1); + } + }); + + it('shows the branch name and chevron once a retry loads the branches', () => { + // Spot check e4-retry-loaded: after tapping Retry the row rendered as an + // empty field — no branch name, no chevron, no skeleton. The loaded + // trigger carries both. + const renderer = mountSelector(githubRow, branchesState({ isRetryableError: true })); + press(renderer.root.findAllByType('Button' as never)[0]); + expect(retry).toHaveBeenCalledTimes(1); + + vi.mocked(useRepositoryBranches).mockReturnValue(branchesState()); + act(() => { + renderer.update( + createElement(RepositoryBranchSelector, { repository: githubRow, disabled: false }) + ); + }); + + expect(pressableWithLabel(renderer, branchLabel('main'))).toBeDefined(); + expect(texts(renderer)).toContain('main'); + expect(renderer.root.findAllByType('ChevronDown' as never).length).toBeGreaterThan(0); + expect(renderer.root.findAllByType('Skeleton' as never)).toHaveLength(0); + }); + it('offers a retry for a transient failure', () => { const renderer = mountSelector(githubRow, branchesState({ isRetryableError: true })); diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.backdrop.test.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.backdrop.test.tsx new file mode 100644 index 0000000000..7338b83730 --- /dev/null +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.backdrop.test.tsx @@ -0,0 +1,102 @@ +// Spot check e2-expand.png: the Finish review island floated over the +// unified diff with deleted-line text still visible around and below the +// button. The card was opaque but the bar's padding ring was not, so diff +// rows scrolled under it showed through. The bar container itself must +// carry the screen background and swallow touches in that ring. +// (Extracted from pr-diff-floating-actions.test.tsx to keep that file +// inside the max-lines budget.) + +import * as React from 'react'; +import { describe, expect, it, vi } from 'vitest'; + +import '@/i18n'; +import type * as ReactI18next from 'react-i18next'; +import { PrDiffFloatingActions } from './pr-diff-floating-actions'; +import { type SelectionState } from '@/lib/pr-review/diff-selection'; + +vi.mock('react-i18next', async importOriginal => { + const actual = await importOriginal(); + return { + ...actual, + useTranslation: () => { + const i18n = actual.getI18n(); + return { t: i18n.t.bind(i18n), i18n }; + }, + }; +}); + +vi.mock('expo-router', () => ({ + useRouter: () => ({ push: vi.fn() }), +})); + +vi.mock('react-native', () => ({ + View: 'View', + Platform: { OS: 'ios' }, +})); + +vi.mock('react-native-safe-area-context', () => ({ + useSafeAreaInsets: () => ({ top: 0, bottom: 0, left: 0, right: 0 }), +})); + +vi.mock('@/components/ui/icons', () => ({ + MessageCirclePlus: () => null, +})); + +vi.mock('@/lib/hooks/use-theme-colors', () => ({ + useThemeColors: () => ({ + primaryForeground: '#FFFFFF', + foreground: '#000000', + mutedForeground: '#6F6A61', + }), +})); + +vi.mock('@/components/ui/button', () => ({ Button: 'Button' })); +vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); +vi.mock('@/lib/pr-review/diff-selection-bridge', () => ({ + clearDiffSelection: vi.fn(), +})); + +vi.mock('@/lib/pr-review/pending-review-provider', () => ({ + usePendingReview: () => ({ + items: [], + addComment: vi.fn(() => undefined), + updateComment: vi.fn(() => undefined), + removeComment: vi.fn(() => undefined), + clear: vi.fn(() => undefined), + }), +})); + +const baseProps = { + owner: 'octocat', + repo: 'hello', + number: 7, + viewMode: 'unified' as const, + selection: null as SelectionState | null, + onClearSelection: vi.fn(), +}; + +function renderBar(): React.ReactElement { + // eslint-disable-next-line new-cap -- plain function component, no hooks state needed for the container props + return PrDiffFloatingActions(baseProps); +} + +describe('PrDiffFloatingActions opaque backdrop (spot check e2)', () => { + it('paints the bar container with the screen background and swallows touches', () => { + // With a transparent container the diff rows scrolled under the bar + // stayed visible around and below the button. The container carries the + // screen background, and the removed `pointerEvents="box-none"` means a + // tap in the padding ring can never reach a diff row hidden behind it. + const props = renderBar().props as { className?: string; pointerEvents?: string }; + expect(props.pointerEvents).toBeUndefined(); + expect((props.className ?? '').split(' ')).toContain('bg-background'); + }); + + it('keeps the action card on the same background inside the bar', () => { + const card = (renderBar().props as { children?: React.ReactElement }).children; + if (!card) { + throw new Error('floating action card not found'); + } + const classes = (card.props as { className?: string }).className ?? ''; + expect(classes.split(' ')).toContain('bg-background'); + }); +}); diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.test.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.test.tsx index de318e55d8..7f7e79c319 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 @@ -313,8 +313,8 @@ describe('PrDiffFloatingActions bottom inset (plan §6)', () => { return findElement({ node: element, type: 'View', - prop: 'pointerEvents', - value: 'box-none', + prop: 'onLayout', + value: (element.props as { onLayout?: unknown }).onLayout, }); } @@ -342,8 +342,8 @@ describe('PrDiffFloatingActions bottom inset (plan §6)', () => { const root = findElement({ node: element, type: 'View', - prop: 'pointerEvents', - value: 'box-none', + prop: 'onLayout', + value: (element.props as { onLayout?: unknown }).onLayout, }); if (!root) { throw new Error('floating action bar root not found'); 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 08186f03d9..5834bd9e3a 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 @@ -68,6 +68,9 @@ export function PrDiffFloatingActions({ // The bar sits on the bottom edge, so its bottom padding must include the // Android system inset. The measured height (onLayout) therefore already // includes the inset, which `prDiffListBottomPadding` reserves for the list. + // The container itself is opaque (`bg-background`): the card floats inside + // a transparent ring of padding, and without a solid backdrop the diff text + // of rows scrolled under the bar shows through around and below the button. const insets = useSafeAreaInsets(); const showSelectionAction = viewMode === 'unified' && selection !== null; @@ -115,8 +118,7 @@ export function PrDiffFloatingActions({ onLayout={(event: LayoutChangeEvent) => { onHeightChange?.(event.nativeEvent.layout.height); }} - pointerEvents="box-none" - className="absolute inset-x-0 bottom-0 items-center gap-2 px-4 pt-3" + className="absolute inset-x-0 bottom-0 items-center gap-2 bg-background px-4 pt-3" style={{ paddingBottom: 24 + insets.bottom }} > diff --git a/apps/mobile/src/components/pr-review/pr-review-discussion-tab-view.test.ts b/apps/mobile/src/components/pr-review/pr-review-discussion-tab-view.test.ts index d25d35521a..fb28722228 100644 --- a/apps/mobile/src/components/pr-review/pr-review-discussion-tab-view.test.ts +++ b/apps/mobile/src/components/pr-review/pr-review-discussion-tab-view.test.ts @@ -5,6 +5,7 @@ import { selectDiscussionTabView } from './pr-review-discussion-tab-view'; const base = { firstPageErrorState: null, isPending: false, + isPaused: false, isEmpty: false, }; @@ -37,6 +38,15 @@ describe('selectDiscussionTabView', () => { expect(selectDiscussionTabView({ ...base, isPending: true })).toEqual({ kind: 'loading' }); }); + it('returns retryable when the first page is pending but paused', () => { + // A paused fetch (offline, or never started) has no end: the skeleton + // would sit there with no comments, empty state, or error (spot check + // e7). The retryable state carries the working Retry CTA instead. + expect(selectDiscussionTabView({ ...base, isPending: true, isPaused: true, isEmpty: true })).toEqual( + { kind: 'retryable' } + ); + }); + it('returns empty when there is no error, no pending, and no items', () => { expect(selectDiscussionTabView({ ...base, isEmpty: true })).toEqual({ kind: 'empty' }); }); @@ -50,6 +60,7 @@ describe('selectDiscussionTabView', () => { selectDiscussionTabView({ firstPageErrorState: { kind: 'permission' }, isPending: true, + isPaused: false, isEmpty: true, }) ).toEqual({ kind: 'permission' }); diff --git a/apps/mobile/src/components/pr-review/pr-review-discussion-tab-view.ts b/apps/mobile/src/components/pr-review/pr-review-discussion-tab-view.ts index 326b006f9c..5b47a60830 100644 --- a/apps/mobile/src/components/pr-review/pr-review-discussion-tab-view.ts +++ b/apps/mobile/src/components/pr-review/pr-review-discussion-tab-view.ts @@ -11,15 +11,24 @@ export type DiscussionTabView = { export function selectDiscussionTabView(args: { firstPageErrorState: { kind: 'permission' | 'not-found' | 'reconnect' | 'retryable' } | null; isPending: boolean; + /** + * The pending first page is paused — offline, or a fetch that will never + * start (spot check e7). + */ + isPaused: boolean; isEmpty: boolean; }): DiscussionTabView { - const { firstPageErrorState, isPending, isEmpty } = args; + const { firstPageErrorState, isPending, isPaused, isEmpty } = args; if (firstPageErrorState) { return { kind: firstPageErrorState.kind }; } if (isPending) { - return { kind: 'loading' }; + // A paused page has no end: the skeleton would sit there with no + // comments, no empty state, and no error. Surface the retryable state so + // the tab always carries an escape. A page in flight — and the one frame + // before the fetch starts — keeps the skeleton. + return { kind: isPaused ? 'retryable' : 'loading' }; } if (isEmpty) { return { kind: 'empty' }; diff --git a/apps/mobile/src/components/pr-review/pr-review-discussion-tab.test.tsx b/apps/mobile/src/components/pr-review/pr-review-discussion-tab.test.tsx index a2922dc3f9..c5356983e9 100644 --- a/apps/mobile/src/components/pr-review/pr-review-discussion-tab.test.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-discussion-tab.test.tsx @@ -13,6 +13,7 @@ const discussionState = vi.hoisted(() => ({ query: { isPending: false, isFetching: false, + isPaused: false, hasNextPage: false, isFetchingNextPage: false, fetchNextPage: vi.fn(), @@ -105,6 +106,7 @@ function expectSinglePadding(renderer: TestRenderer.ReactTestRenderer, expected: function resetState(): void { discussionState.query.isPending = false; discussionState.query.isFetching = false; + discussionState.query.isPaused = false; discussionState.query.hasNextPage = false; discussionState.query.isFetchingNextPage = false; discussionState.threads = []; @@ -148,6 +150,23 @@ describe('PrReviewDiscussionTab full-body states', () => { expectSinglePadding(mountTab(), 32); }); + it('escapes a stuck skeleton when the first page is paused, not in flight', () => { + // Spot check e7: the tab showed only skeleton cards — no comments, no + // empty state, no error. A pending page whose fetch is paused has no + // end, so the tab must render the retryable state with a working Retry + // CTA instead of the permanent skeleton. + discussionState.query.isPending = true; + discussionState.query.isPaused = true; + const renderer = mountTab(); + + expect(renderer.root.findAll(node => String(node.type) === 'Skeleton')).toHaveLength(0); + const error = renderer.root.find(node => String(node.type) === 'QueryError'); + act(() => { + (error.props.onRetry as () => void)(); + }); + expect(discussionState.query.refetch).toHaveBeenCalled(); + }); + it('lets EmptyState own the empty body and keeps its Files action', () => { const renderer = mountTab(); const empty = renderer.root.find(node => String(node.type) === 'EmptyState'); 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 5b5cd6735c..a5e0f1c296 100644 --- a/apps/mobile/src/components/pr-review/pr-review-discussion-tab.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-discussion-tab.tsx @@ -7,7 +7,11 @@ // the entire loaded set on every update (R4: a // later page can insert rows mid-list). // - loading: first page in flight; render `Skeleton` -// placeholders matching the row dimensions. +// placeholders matching the row dimensions. A first +// page that is pending but PAUSED (offline, or a fetch +// that will never start) is not "in flight": it falls +// to the retryable state below so the tab never sits +// on a skeleton with no escape (spot check e7). // - retryable: first page failed with a transient error; // render `QueryError` with the standard Retry // CTA wired to `refetch()`. @@ -220,6 +224,10 @@ export function PrReviewDiscussionTab({ const view = selectDiscussionTabView({ firstPageErrorState: retainedContentError ? null : firstPageErrorState, isPending: query.isPending && isEmpty, + // A pending page whose fetch is paused (offline, or a fetch that will + // never start) has no end — the retryable state, not the skeleton (spot + // check e7). + isPaused: query.isPaused, isEmpty, }); diff --git a/apps/mobile/src/lib/pr-review/provider-pr-queries.test.ts b/apps/mobile/src/lib/pr-review/provider-pr-queries.test.ts index 4d2e7844fd..fd1f9b9f32 100644 --- a/apps/mobile/src/lib/pr-review/provider-pr-queries.test.ts +++ b/apps/mobile/src/lib/pr-review/provider-pr-queries.test.ts @@ -299,6 +299,19 @@ describe('normalizeProviderThreadsPage', () => { expect(first(first(mapped).threads).threadId).toBe('disc-1'); expect(normalizePrThreadsPages('gitlab', undefined)).toEqual([]); }); + + it('drops a discussion whose notes are all system events', () => { + // GitLab reports every MR event (pushes, assignments) as a discussion, + // and the read layer strips the system notes — what remains carries no + // discussion content. Kept, it would count as content and the tab would + // render a blank list with no comments, no empty state, and no error + // (spot check e7). + const page = normalizeProviderThreadsPage({ + threads: [thread, { ...thread, threadId: 'disc-system', comments: [] }], + nextCursor: null, + }); + expect(page.threads.map(entry => entry.threadId)).toEqual(['disc-1']); + }); }); describe('normalizeProviderOverview', () => { diff --git a/apps/mobile/src/lib/pr-review/provider-pr-queries.ts b/apps/mobile/src/lib/pr-review/provider-pr-queries.ts index 862fcddac8..ae0c4248db 100644 --- a/apps/mobile/src/lib/pr-review/provider-pr-queries.ts +++ b/apps/mobile/src/lib/pr-review/provider-pr-queries.ts @@ -171,7 +171,16 @@ type ProviderThreadsPage = { threads: ProviderPrThread[]; nextCursor: string | n export function normalizeProviderThreadsPage(page: ProviderThreadsPage): PrThreadsPageModel { return { - threads: page.threads.map(normalizeProviderThread), + // GitLab reports every MR event (pushes, assignments, "requested review") + // as a discussion; the read layer drops the system notes, which leaves + // threads with no comments. Those are not discussion content: counted as + // content they keep the tab out of its empty state, and filtered by the + // list they leave a blank screen — comments, empty state, and error all + // absent at once (spot check e7). Drop them here, where the provider + // shape is read, so the tab's empty check sees the truth. + threads: page.threads + .filter(thread => thread.comments.length > 0) + .map(thread => normalizeProviderThread(thread)), // Providers have no separate conversation leg: an unanchored discussion // is already a thread with `path: null`. conversation: [],