feat(mobile): bring GitLab and Bitbucket pull request review to GitHub parity (stack 24/30)

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 }));
 

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<typeof ReactI18next>();
+  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');
+  });
+});

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');

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 }}
     >
       <View className="w-full gap-2 rounded-2xl border border-border bg-background px-3 py-3 shadow-lg shadow-[#0000001A]">

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' });

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' };

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');

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,
   });
 

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', () => {

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: [],