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 dd0dd7dd8e..b3a6901320 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,6 @@ 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'; @@ -318,16 +314,15 @@ describe('PrReviewFileList write affordances per provider', () => { }); }); - it('reserves the bar-sized gap under a provider diff list too', () => { + it('keeps the small footer gap under a provider diff list too', () => { const githubPadding = listBottomPadding(mountList()); const gitlabPadding = listBottomPadding( mountListInScope({ platform: 'gitlab', projectPath: 'group/repo', mrIid: 12 }) ); - // The bar 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); + // The bar is an in-flow footer below the list (spot check e3), so no + // row can ever scroll under it; the list only keeps a 12-point gap + // between its last row and the footer's top edge, on every provider. + expect(githubPadding).toBe(12); + expect(gitlabPadding).toBe(12); }); }); diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-file-list.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-file-list.tsx index 6d50c19dbf..dff27d09a8 100644 --- a/apps/mobile/src/components/pr-review/diff/pr-diff-file-list.tsx +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-file-list.tsx @@ -12,8 +12,9 @@ // * S7a adds diff-line selection: tapping a line runs the pure // `selectLine` reducer; the result is mirrored into the // `diff-selection-bridge` (so the comment composer can read it on -// mount) and a floating action bar (`PrDiffFloatingActions`) -// hosts the "Comment" and "Finish review" affordances. +// mount) and a footer action bar (`PrDiffFloatingActions`) rendered +// in-flow below the list hosts the "Comment" and "Finish review" +// affordances. // // Cold first paint: FlashList mounts only after the first page of files is // present. The first-load waiting state is a plain skeleton outside the list @@ -41,7 +42,6 @@ import { 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'; @@ -60,6 +60,9 @@ import { clearDiffSelection } from '@/lib/pr-review/diff-selection-bridge'; import { CenteredState } from '@/components/centered-state'; import { useIsTablet } from '@/lib/hooks/use-is-tablet'; +// Gap between the last diff row and the in-flow footer bar's top edge. +const PR_DIFF_LIST_FOOTER_GAP = 12; + type PrReviewFileListProps = { readonly owner: string; readonly repo: string; @@ -127,30 +130,13 @@ export function PrReviewFileList({ [owner, repo, number] ); - // Measured floating-bar height (null until the first layout event). - const [barHeight, setBarHeight] = useState(null); - - // Stable callback: ignore sub-one-point noise to avoid unnecessary - // re-renders. Layout events can fire with fractional-pixel deltas. - const handleHeightChange = useCallback((height: number) => { - setBarHeight(prev => { - if (prev !== null && Math.abs(prev - height) < 1) { - return prev; - } - return height; - }); - }, []); - // The write bar renders on every provider (s6): its two routes — the // comment composer and the review-submit sheet — are siblings of the // GitHub route AND of the provider route, so the bar pushes the sheet - // inside the scope its queries run under. The 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] - ); + // inside the scope its queries run under. The bar is an in-flow footer + // below the list (spot check e3), so the list only keeps a small gap + // between its last row and the footer's top edge. + const listContentStyle = useMemo(() => ({ paddingBottom: PR_DIFF_LIST_FOOTER_GAP }), []); // Which provider's words the terminal and empty states use. const copy = usePrDiffStateCopy({ owner, repo, number }); @@ -377,7 +363,6 @@ export function PrReviewFileList({ viewMode={effectiveViewMode} selection={selection} onClearSelection={clearSelection} - onHeightChange={handleHeightChange} /> 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 7f7e79c319..51c844330c 100644 --- a/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.test.tsx +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.test.tsx @@ -310,18 +310,13 @@ describe('PrDiffFloatingActions bottom inset (plan §6)', () => { function findRootBar(): React.ReactElement | null { // eslint-disable-next-line new-cap const element = PrDiffFloatingActions(baseProps); - return findElement({ - node: element, - type: 'View', - prop: 'onLayout', - value: (element.props as { onLayout?: unknown }).onLayout, - }); + return element; } function rootPaddingBottom(): number | undefined { const root = findRootBar(); if (!root) { - throw new Error('floating action bar root not found'); + throw new Error('footer action bar root not found'); } return (root.props as { style?: { paddingBottom?: number } }).style?.paddingBottom; } @@ -335,27 +330,17 @@ describe('PrDiffFloatingActions bottom inset (plan §6)', () => { expect(rootPaddingBottom()).toBe(58); }); - it('reports the measured layout height through onHeightChange', () => { - const onHeightChange = vi.fn(() => undefined); - // eslint-disable-next-line new-cap - const element = PrDiffFloatingActions({ ...baseProps, onHeightChange }); - const root = findElement({ - node: element, - type: 'View', - prop: 'onLayout', - value: (element.props as { onLayout?: unknown }).onLayout, - }); + it('renders in-flow, not as an overlay over the list', () => { + // Spot check e3: the bar used to sit `absolute inset-x-0 bottom-0` over + // the FlashList, so a partly-scrolled diff row was clipped at its top + // edge. As an in-flow footer the list ends above it at every scroll + // position. + const root = findRootBar(); if (!root) { - throw new Error('floating action bar root not found'); + throw new Error('footer action bar root not found'); } - const onLayout = ( - root.props as { - onLayout?: (event: { nativeEvent: { layout: { height: number } } }) => void; - } - ).onLayout; - onLayout?.({ nativeEvent: { layout: { height: 150 } } }); - - expect(onHeightChange).toHaveBeenCalledTimes(1); - expect(onHeightChange).toHaveBeenCalledWith(150); + const classes = ((root.props as { className?: string }).className ?? '').split(' '); + expect(classes).not.toContain('absolute'); + expect(classes).toContain('w-full'); }); }); diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.tsx index 5834bd9e3a..b0a211f719 100644 --- a/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.tsx +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.tsx @@ -1,4 +1,7 @@ -// Floating action bar rendered over the PR diff FlashList. Hosts: +// Footer action bar rendered in-flow below the PR diff FlashList. The list +// ends at its top edge, so a diff row is never clipped by it at any scroll +// position (spot check e3: the bar floated over the list and cut the last +// src/beta.ts line). Hosts: // - The "Comment" affordance that pushes the comment-composer route // when a diff-line selection exists, plus a "Clear" button that // drops the selection. @@ -13,7 +16,7 @@ import { type Href, useRouter } from 'expo-router'; import { MessageCirclePlus } from '@/components/ui/icons'; import { useTranslation } from 'react-i18next'; -import { type LayoutChangeEvent, View } from 'react-native'; +import { View } from 'react-native'; import { useSafeAreaInsets } from 'react-native-safe-area-context'; import { Button } from '@/components/ui/button'; @@ -47,8 +50,6 @@ type PrDiffFloatingActionsProps = Readonly<{ selection: SelectionState | null; /** Setter for the parent's selection state — `null` clears. */ onClearSelection: () => void; - /** Optional callback for the measured root layout height (points). */ - onHeightChange?: (height: number) => void; }>; export function PrDiffFloatingActions({ @@ -59,18 +60,15 @@ export function PrDiffFloatingActions({ viewMode, selection, onClearSelection, - onHeightChange, }: PrDiffFloatingActionsProps) { const router = useRouter(); const colors = useThemeColors(); const { t } = useTranslation(); const pending = usePendingReview(); - // The bar sits on the bottom edge, so its bottom padding must include the - // Android system inset. The measured height (onLayout) therefore already - // includes the inset, which `prDiffListBottomPadding` reserves for the list. - // The 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. + // The footer sits on the bottom edge, so its bottom padding must include + // the Android system inset. The container is opaque (`bg-background`) and + // in-flow: the diff list ends at its top edge, so no row is ever clipped + // by it and nothing shows through around the card. const insets = useSafeAreaInsets(); const showSelectionAction = viewMode === 'unified' && selection !== null; @@ -115,10 +113,7 @@ export function PrDiffFloatingActions({ return ( { - onHeightChange?.(event.nativeEvent.layout.height); - }} - className="absolute inset-x-0 bottom-0 items-center gap-2 bg-background px-4 pt-3" + className="w-full items-center gap-2 bg-background px-4 pt-3" style={{ paddingBottom: 24 + insets.bottom }} > diff --git a/apps/mobile/src/components/pr-review/merge/pr-merge-sheet.test.tsx b/apps/mobile/src/components/pr-review/merge/pr-merge-sheet.test.tsx index 7af27b5987..3e2ca3324c 100644 --- a/apps/mobile/src/components/pr-review/merge/pr-merge-sheet.test.tsx +++ b/apps/mobile/src/components/pr-review/merge/pr-merge-sheet.test.tsx @@ -606,6 +606,37 @@ describe('PrMergeSheet provider merge arm (s6)', () => { findElement({ node: element, type: 'Button', prop: 'accessibilityLabel', value: 'Cancel' }) ).not.toBeNull(); }); + + it('pins the blocked-arm footer to the sheet bottom (spot check e7)', () => { + // eslint-disable-next-line new-cap + const element = PrMergeSheet({ + ...baseProps, + prRef: GITLAB_REF, + mergeState: mergeState({ + canMerge: false, + blockedReasons: [{ code: 'failing_pipeline', message: 'Pipeline #123 failed' }], + }), + }); + // The sheet opens at the full detent; a growing spacer plus a content + // container that fills the viewport keep Cancel on the sheet's bottom + // edge instead of floating mid-sheet over an empty region. + const scroll = findElement({ + node: element, + type: 'ScrollView', + prop: 'className', + value: 'flex-1 bg-background', + }); + expect(scroll).not.toBeNull(); + if (!scroll) { + return; + } + expect( + (scroll.props as { contentContainerStyle?: Record }).contentContainerStyle + ).toEqual({ flexGrow: 1, paddingBottom: 4 }); + expect( + findElement({ node: scroll, type: 'View', prop: 'className', value: 'flex-1' }) + ).not.toBeNull(); + }); }); // ── s6f: reviewer blocking findings ────────────────────────────────── 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 6c9539bc7f..fb8ae13eeb 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 @@ -519,6 +519,16 @@ export function PrMergeSheet(props: PrMergeSheetProps) { onDismiss(); } + // The short arms (blocked merge, capability banner, provider auto-merge) + // carry a small body; the sheet still opens at the full detent, so their + // footers are pinned to the sheet's bottom edge with a growing spacer + // (spot check e7: Cancel sat mid-sheet over a large empty region). The + // ScrollView's content container grows to at least the viewport, so the + // spacer only expands when the body is shorter than the sheet. + function bodySpacer() { + return ; + } + // 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 @@ -548,6 +558,7 @@ export function PrMergeSheet(props: PrMergeSheetProps) { + {bodySpacer()} {cancelOnlyFooter()} ); @@ -556,6 +567,7 @@ export function PrMergeSheet(props: PrMergeSheetProps) { return ( <> + {bodySpacer()}