From dbd3454ee19ac2b037193fdfc7a0ff5ccf52faac Mon Sep 17 00:00:00 2001 From: Kelvin Amoaba <97001695+AmoabaKelvin@users.noreply.github.com> Date: Wed, 11 Nov 2026 04:18:55 +0000 Subject: [PATCH] feat(mobile): GitHub-style diff rows with a pinned gutter and sideways scroll (#40961) --- .../src/components/MobileDiffReviewLine.tsx | 190 ++++++++++++----- .../src/components/MobileDiffReviewLines.tsx | 191 +++++++++++++----- .../mobile-diff-review-layout-styles.ts | 34 +++- .../review/mobile-diff-review-rows.test.ts | 31 +++ .../review/mobile-diff-review-rows.ts | 57 +++++- mobile/src/theme/mobile-theme.ts | 31 ++- 6 files changed, 417 insertions(+), 117 deletions(-) create mode 100644 mobile/src/source-control/review/mobile-diff-review-rows.test.ts diff --git a/mobile/src/components/MobileDiffReviewLine.tsx b/mobile/src/components/MobileDiffReviewLine.tsx index 0bd1bbf872e..27cbb73605a 100644 --- a/mobile/src/components/MobileDiffReviewLine.tsx +++ b/mobile/src/components/MobileDiffReviewLine.tsx @@ -1,5 +1,5 @@ import { memo } from 'react' -import { Pressable, StyleSheet, Text, View } from 'react-native' +import { Platform, Pressable, Text, View, type ViewStyle } from 'react-native' import { MessageSquare } from 'lucide-react-native' import type { Translate } from '../i18n/mobile-i18n' import { useMobileTranslation } from '../i18n/use-mobile-translation' @@ -7,7 +7,7 @@ import type { DiffComment } from '../../../src/shared/diff-comment-types' import type { MobileDiffLine } from '../source-control/review/mobile-diff-lines' import type { MobileHighlightedDiffLine } from '../files/session-documents/mobile-file-syntax' import { mobileDiffLineNumber, mobileDiffLinePrefix } from '../source-control/mobile-diff-format' -import { colors, spacing, typography } from '../theme/mobile-theme' +import { colors, typography } from '../theme/mobile-theme' import { themedStyleSheet } from '../theme/themed-styles' import { useMobileColorScheme } from '../theme/use-mobile-color-scheme' import { MobileSyntaxSegments } from './MobileSyntaxSegments' @@ -21,6 +21,8 @@ type Props = { staleCommentIds: ReadonlySet /** Part of the lines picked for a note. */ selected?: boolean + /** Fits the file's longest line number, so every row's gutter lines up. */ + numberWidth: number /** Absent where lines take no notes, e.g. a past commit's files. */ onAddNote?: (lineNumber: number) => void onEditNote: (comment: DiffComment) => void @@ -52,6 +54,7 @@ export const MobileDiffReviewLine = memo(function MobileDiffReviewLine({ comments, staleCommentIds, selected = false, + numberWidth, onAddNote, onEditNote }: Props) { @@ -59,23 +62,45 @@ export const MobileDiffReviewLine = memo(function MobileDiffReviewLine({ useMobileColorScheme() const lineNumber = mobileDiffLineNumber(line) const canComment = onAddNote !== undefined && canCommentOnLine(line) + const ground = [ + line.kind === 'add' && styles.addedRow, + line.kind === 'delete' && styles.deletedRow, + selected && styles.selectedRow + ] return ( - {mobileDiffLinePrefix(line.kind)} - {lineNumber ? String(lineNumber) : ''} + {/* Pinned while the file scrolls sideways, so it repaints the row's tint over an opaque ground. */} + + + + {lineNumber ? String(lineNumber) : ''} + + + {mobileDiffLinePrefix(line.kind)} + + + [styles.code, pressed && canComment && styles.codePressed]} disabled={!canComment} @@ -98,41 +123,83 @@ export const MobileDiffReviewLine = memo(function MobileDiffReviewLine({ {comments.length > 0 ? ( - - {comments.map((comment) => ( - [styles.noteButton, pressed && styles.noteButtonPressed]} - onPress={() => onEditNote(comment)} - accessibilityRole="button" - accessibilityLabel={t( - 'components.diffReviewLine.editNoteOnLine', - 'Edit note on line {{number}}', - { - number: comment.lineNumber - } - )} - > - - - ))} + + + {comments.map((comment) => ( + [styles.noteButton, pressed && styles.noteButtonPressed]} + onPress={() => onEditNote(comment)} + accessibilityRole="button" + accessibilityLabel={t( + 'components.diffReviewLine.editNoteOnLine', + 'Edit note on line {{number}}', + { + number: comment.lineNumber + } + )} + > + + + ))} + ) : null} ) }) +/** Renders one code line's worth of a known string, so the list can size its columns to the code font. */ +export function MobileDiffReviewCharWidthProbe({ + onCharWidth +}: { + onCharWidth: (width: number) => void +}) { + return ( + onCharWidth(event.nativeEvent.layout.width / PROBE_TEXT.length)} + > + {PROBE_TEXT} + + ) +} + +const PROBE_TEXT = '0'.repeat(100) +const SIGN_WIDTH = 14 +// Room either side of the line number. +const NUMBER_PADDING = 12 + +export function mobileDiffReviewGutterWidth(numberWidth: number): number { + return numberWidth + SIGN_WIDTH +} + +export function mobileDiffReviewNumberWidth(digits: number, charWidth: number): number { + return Math.ceil(digits * charWidth + NUMBER_PADDING) +} + +// The page lets long lines run and the file scroll sideways under pinned cells (Kelvin's choice over wrapping). +const webPinned: ViewStyle = + Platform.OS === 'web' + ? { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: RN Web renders CSS sticky; this branch never reaches native. + position: 'sticky' as ViewStyle['position'], + zIndex: 1 + } + : {} + // Use 18px lineHeight alone without padding/minHeight to match desktop diff density (STA-1239). const styles = themedStyleSheet(() => ({ row: { flexDirection: 'row', - alignItems: 'stretch', - borderBottomWidth: StyleSheet.hairlineWidth, - borderBottomColor: colors.borderSubtle + alignItems: 'stretch' }, addedRow: { backgroundColor: colors.diffAddedBg @@ -144,38 +211,71 @@ const styles = themedStyleSheet(() => ({ selectedRow: { backgroundColor: colors.skillTokenSurface }, - prefix: { - width: 18, - textAlign: 'center', - color: colors.textMuted, + pinned: { + ...webPinned, + backgroundColor: colors.bgBase + }, + pinnedLeft: { + left: 0 + }, + pinnedRight: { + right: 0 + }, + gutter: { + flex: 1, + flexDirection: 'row' + }, + lineNumber: { + paddingRight: 6, + textAlign: 'right', + color: colors.diffLineNumber, fontFamily: typography.monoFamily, fontSize: typography.metaSize, lineHeight: 18 }, - lineNumber: { - width: 44, - paddingRight: spacing.xs, - textAlign: 'right', - color: colors.textMuted, + addedNumber: { + color: colors.textPrimary, + backgroundColor: colors.diffAddedGutter + }, + deletedNumber: { + color: colors.textPrimary, + backgroundColor: colors.diffDeletedGutter + }, + sign: { + width: SIGN_WIDTH, + textAlign: 'center', + color: colors.diffLineNumber, fontFamily: typography.monoFamily, fontSize: typography.metaSize, lineHeight: 18 }, + addedSign: { + color: colors.diffAddedSign + }, + deletedSign: { + color: colors.diffDeletedSign + }, code: { flex: 1, minWidth: 0, - paddingHorizontal: spacing.sm + paddingRight: 10 }, codePressed: { backgroundColor: colors.bgRaised }, codeText: { + ...(Platform.OS === 'web' ? { whiteSpace: 'pre' as const } : {}), color: colors.textPrimary, fontFamily: typography.monoFamily, fontSize: 12, lineHeight: 18 }, + probe: { + position: 'absolute', + opacity: 0 + }, notes: { + flex: 1, width: 40, alignItems: 'center', justifyContent: 'center', diff --git a/mobile/src/components/MobileDiffReviewLines.tsx b/mobile/src/components/MobileDiffReviewLines.tsx index 04ddb7398eb..09fb1e9d3f3 100644 --- a/mobile/src/components/MobileDiffReviewLines.tsx +++ b/mobile/src/components/MobileDiffReviewLines.tsx @@ -1,11 +1,17 @@ import { useCallback, useEffect, useMemo, useRef, useState } from 'react' -import { FlatList, Platform, Pressable, Text, View } from 'react-native' +import { FlatList, Platform, Pressable, Text, View, type ViewStyle } from 'react-native' import { Gesture, GestureDetector } from 'react-native-gesture-handler' -import { ChevronsUpDown } from 'lucide-react-native' +import { UnfoldVertical } from 'lucide-react-native' import type { DiffComment } from '../../../src/shared/diff-comment-types' import { useMobileColorScheme } from '../theme/use-mobile-color-scheme' import { useMobileTranslation } from '../i18n/use-mobile-translation' -import { DIFF_LINE_ID_PREFIX, MobileDiffReviewLine } from './MobileDiffReviewLine' +import { + DIFF_LINE_ID_PREFIX, + MobileDiffReviewCharWidthProbe, + MobileDiffReviewLine, + mobileDiffReviewGutterWidth, + mobileDiffReviewNumberWidth +} from './MobileDiffReviewLine' import type { ReviewDiffLine } from '../source-control/review/mobile-diff-review-screen-model' import { buildMobileDiffReviewRows, @@ -25,6 +31,18 @@ const RANGE_PICK_LONG_PRESS_MS = 350 const MAX_SCROLL_ATTEMPTS = 10 const NO_LINE_NOTES: readonly DiffComment[] = [] const SCROLL_RETRY_MS = 100 +// The page's default tab-size, so a tabbed line's width matches what it draws. +const TAB_COLUMNS = 8 +const CODE_END_PADDING = 10 +const NOTES_WIDTH = 40 + +function lineColumns(text: string): number { + let columns = 0 + for (const char of text) { + columns = char === '\t' ? (Math.floor(columns / TAB_COLUMNS) + 1) * TAB_COLUMNS : columns + 1 + } + return columns +} export type LineRange = { startLine: number; line: number } @@ -77,6 +95,44 @@ export function MobileDiffReviewLines({ setOpened({ lines, indexes: next }) } + // Long lines run on one line and the whole file scrolls sideways, so every row is as wide as the longest. + const [charWidth, setCharWidth] = useState(0) + const [viewportWidth, setViewportWidth] = useState(0) + const { numberWidth, codeColumns } = useMemo(() => { + let digits = 1 + let columns = 0 + for (const line of lines) { + digits = Math.max(digits, String(line.newLineNumber ?? line.oldLineNumber ?? '').length) + columns = Math.max(columns, lineColumns(line.text)) + } + return { numberWidth: mobileDiffReviewNumberWidth(digits, charWidth), codeColumns: columns } + }, [charWidth, lines]) + const gutterWidth = mobileDiffReviewGutterWidth(numberWidth) + const rowFrame: ViewStyle = + Platform.OS === 'web' && charWidth > 0 + ? { + width: Math.max( + viewportWidth, + Math.ceil( + gutterWidth + + codeColumns * charWidth + + CODE_END_PADDING + + (commentsByLine.size > 0 ? NOTES_WIDTH : 0) + ) + ) + } + : {} + // Folds, threads and the footer hold still in the viewport while the code slides under them. + const viewportRow: ViewStyle = + Platform.OS === 'web' && viewportWidth > 0 + ? { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: RN Web renders CSS sticky; this branch never reaches native. + position: 'sticky' as ViewStyle['position'], + left: 0, + width: viewportWidth + } + : {} + const listRef = useRef>>(null) const scrollFailedRef = useRef(false) const [scrollAttempt, setScrollAttempt] = useState(0) @@ -204,10 +260,16 @@ export function MobileDiffReviewLines({ return ( - + setViewportWidth(event.nativeEvent.layout.width)} + > + > ref={listRef} data={rows} + style={styles.diffList} // The list holds still while lines are being picked, so the drag selects instead of scrolling. scrollEnabled={dragRange === null} onScrollToIndexFailed={(info) => { @@ -218,60 +280,77 @@ export function MobileDiffReviewLines({ }) }} keyExtractor={(row) => row.key} - renderItem={({ item: row }) => - row.kind === 'fold' ? ( - [styles.foldRow, pressed && styles.buttonPressed]} - onPress={() => openFold(row.startIndex, row.endIndex)} - accessibilityRole="button" - accessibilityLabel={t( - 'components.diffReviewBody.showUnchangedLines', - 'Show {{count}} unchanged lines', - { count: row.hiddenCount } - )} - > - - - {row.hiddenCount === 1 - ? t( - 'components.diffReviewBody.unchangedLines_one', - '{{count}} unchanged line', - { - count: row.hiddenCount - } - ) - : t( - 'components.diffReviewBody.unchangedLines_other', - '{{count}} unchanged lines', - { count: row.hiddenCount } - )} - - - ) : row.kind === 'thread' ? ( - - - - ) : ( - = highlighted.startLine && - row.line.newLineNumber <= highlighted.line - } - {...(onPickLines ? { onAddNote: pickOneLine } : {})} - onEditNote={onEditNote} - /> - ) - } - contentContainerStyle={styles.diffList} + renderItem={({ item: row }) => ( + + {row.kind === 'fold' ? ( + [ + styles.foldRow, + viewportRow, + pressed && styles.buttonPressed + ]} + onPress={() => openFold(row.startIndex, row.endIndex)} + accessibilityRole="button" + accessibilityLabel={t( + 'components.diffReviewBody.showUnchangedLines', + 'Show {{count}} unchanged lines', + { count: row.hiddenCount } + )} + > + + + + + + {row.hiddenCount === 1 + ? t( + 'components.diffReviewBody.unchangedLines_one', + '{{count}} unchanged line', + { + count: row.hiddenCount + } + ) + : t( + 'components.diffReviewBody.unchangedLines_other', + '{{count}} unchanged lines', + { count: row.hiddenCount } + )} + + {row.enclosingLine ? ( + + {row.enclosingLine} + + ) : null} + + + ) : row.kind === 'thread' ? ( + + + + ) : ( + = highlighted.startLine && + row.line.newLineNumber <= highlighted.line + } + {...(onPickLines ? { onAddNote: pickOneLine } : {})} + onEditNote={onEditNote} + /> + )} + + )} + contentContainerStyle={styles.diffListContent} + ListFooterComponentStyle={rowFrame} ListFooterComponent={ truncated ? ( - + {t('components.diffReviewBody.diffTruncated', 'Diff truncated for mobile preview.')} ) : null diff --git a/mobile/src/components/mobile-diff-review-layout-styles.ts b/mobile/src/components/mobile-diff-review-layout-styles.ts index 8204ed0f69a..c9ac1229ed9 100644 --- a/mobile/src/components/mobile-diff-review-layout-styles.ts +++ b/mobile/src/components/mobile-diff-review-layout-styles.ts @@ -1,4 +1,4 @@ -import { StyleSheet } from 'react-native' +import { Platform, StyleSheet } from 'react-native' import { colors, radii, spacing, typography } from '../theme/mobile-theme' import { themedStyleSheet } from '../theme/themed-styles' @@ -59,13 +59,31 @@ export const mobileDiffReviewLayoutStyles = themedStyleSheet(() => ({ foldRow: { minHeight: 32, flexDirection: 'row', + alignItems: 'stretch', + backgroundColor: colors.diffFoldBg + }, + foldIcon: { alignItems: 'center', justifyContent: 'center', - gap: spacing.xs, - backgroundColor: colors.bgPanel + backgroundColor: colors.diffFoldGutter + }, + foldBody: { + flex: 1, + minWidth: 0, + flexDirection: 'row', + alignItems: 'center', + gap: spacing.sm, + paddingHorizontal: spacing.sm }, foldText: { + flexShrink: 0, + color: colors.diffFoldText, + fontSize: typography.metaSize + }, + foldContext: { + flexShrink: 1, color: colors.textMuted, + fontFamily: typography.monoFamily, fontSize: typography.metaSize }, diffListFrame: { @@ -77,8 +95,14 @@ export const mobileDiffReviewLayoutStyles = themedStyleSheet(() => ({ backgroundColor: colors.bgBase }, diffList: { - paddingBottom: 140, - backgroundColor: colors.editorSurface + flex: 1, + backgroundColor: colors.bgBase + }, + // On the page the rows run as wide as the longest line and scroll sideways together. A sideways scroller inside + // the vertical list keeps each swipe on one axis, as nested scrollers do in WebKit. + diffListContent: { + ...(Platform.OS === 'web' ? { overflowX: 'auto' as const } : {}), + paddingBottom: 140 }, truncatedText: { color: colors.textMuted, diff --git a/mobile/src/source-control/review/mobile-diff-review-rows.test.ts b/mobile/src/source-control/review/mobile-diff-review-rows.test.ts new file mode 100644 index 00000000000..e0acbecf7cc --- /dev/null +++ b/mobile/src/source-control/review/mobile-diff-review-rows.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from 'vitest' +import type { MobileDiffLine } from './mobile-diff-lines' +import { foldEnclosingLine } from './mobile-diff-review-rows' + +const context = (text: string): MobileDiffLine => ({ kind: 'context', text }) + +describe('foldEnclosingLine', () => { + it('names the nearest line above the fold that is indented less than the line after it', () => { + const lines: MobileDiffLine[] = [ + context("describe('queue', () => {"), + context(" it('orders files', () => {"), + context(' const queue = build()'), + context(''), + context(' expect(queue).toHaveLength(3)'), + { kind: 'delete', text: ' removed()' }, + context(' expect(queue[0].scope).toBe(1)') + ] + expect(foldEnclosingLine(lines, 6)).toBe("it('orders files', () => {") + }) + + it('skips blank lines after the fold and returns null when nothing encloses it', () => { + const lines = [ + context('const a = 1'), + context('const b = 2'), + context(''), + context('const c = 3') + ] + expect(foldEnclosingLine(lines, 2)).toBeNull() + expect(foldEnclosingLine(lines, 4)).toBeNull() + }) +}) diff --git a/mobile/src/source-control/review/mobile-diff-review-rows.ts b/mobile/src/source-control/review/mobile-diff-review-rows.ts index 9d140cd4881..1699890b564 100644 --- a/mobile/src/source-control/review/mobile-diff-review-rows.ts +++ b/mobile/src/source-control/review/mobile-diff-review-rows.ts @@ -7,7 +7,15 @@ const MINIMUM_FOLDED_LINES = 3 export type MobileDiffReviewRow = | { kind: 'line'; key: string; line: Line; lineIndex: number } - | { kind: 'fold'; key: string; hiddenCount: number; startIndex: number; endIndex: number } + | { + kind: 'fold' + key: string + hiddenCount: number + startIndex: number + endIndex: number + /** The line the folded code sits inside, shown on the fold like GitHub's hunk header. */ + enclosingLine: string | null + } | { kind: 'thread'; key: string; thread: MobileDiffReviewThread } /** @@ -72,7 +80,8 @@ export function buildMobileDiffReviewRows( key: `fold:${foldStart}`, hiddenCount: foldEnd - foldStart, startIndex: foldStart, - endIndex: foldEnd + endIndex: foldEnd, + enclosingLine: foldEnclosingLine(lines, foldEnd) }) lineIndex = foldEnd - 1 continue @@ -82,3 +91,47 @@ export function buildMobileDiffReviewRows( } return rows } + +const TAB_COLUMNS = 4 + +function indentColumns(text: string): number { + let columns = 0 + for (const char of text) { + if (char === ' ') { + columns += 1 + } else if (char === '\t') { + columns += TAB_COLUMNS + } else { + break + } + } + return columns +} + +/** + * The phone's diff has no `@@` headers, so the fold names its scope itself: the nearest new-file line at or + * above the fold's end that is indented less than the first line shown after it. Blank lines don't count. + */ +export function foldEnclosingLine( + lines: readonly MobileDiffLine[], + foldEnd: number +): string | null { + const counts = (line: MobileDiffLine | undefined): line is MobileDiffLine => + line !== undefined && line.kind !== 'delete' && line.text.trim() !== '' + let nextIndex = foldEnd + while (nextIndex < lines.length && !counts(lines[nextIndex])) { + nextIndex += 1 + } + const next = lines[nextIndex] + if (!counts(next)) { + return null + } + const nextIndent = indentColumns(next.text) + for (let index = foldEnd - 1; index >= 0; index -= 1) { + const line = lines[index] + if (counts(line) && indentColumns(line.text) < nextIndent) { + return line.text.trim() + } + } + return null +} diff --git a/mobile/src/theme/mobile-theme.ts b/mobile/src/theme/mobile-theme.ts index bcd4c8f9686..7d53f8422eb 100644 --- a/mobile/src/theme/mobile-theme.ts +++ b/mobile/src/theme/mobile-theme.ts @@ -57,11 +57,18 @@ const darkColors = { statusPurple: '#d8b4fe', gitDecorationAdded: '#81b88b', gitDecorationDeleted: '#c74e39', - // Desktop's `--diff-*-ground` and `--diff-*-gutter` mixes. - diffAddedBg: 'rgba(129, 184, 139, 0.16)', - diffDeletedBg: 'rgba(199, 78, 57, 0.18)', - diffAddedGutter: 'rgba(129, 184, 139, 0.3)', - diffDeletedGutter: 'rgba(199, 78, 57, 0.32)', + // GitHub's (Primer) diff colours, Kelvin's choice for every phone diff: row ground, line-number cell over it, + // the +/− sign, unchanged line numbers, and the blue "unchanged lines" fold band. + diffAddedBg: 'rgba(46, 160, 67, 0.15)', + diffDeletedBg: 'rgba(248, 81, 73, 0.13)', + diffAddedGutter: 'rgba(63, 185, 80, 0.3)', + diffDeletedGutter: 'rgba(248, 81, 73, 0.3)', + diffAddedSign: '#3fb950', + diffDeletedSign: '#f85149', + diffLineNumber: '#6e7681', + diffFoldBg: 'rgba(56, 139, 253, 0.1)', + diffFoldGutter: 'rgba(56, 139, 253, 0.22)', + diffFoldText: '#4493f8', // Desktop's `--chat-diff-*`: decoration mixed toward foreground for tinted rows. chatDiffAdded: '#a5ccac', chatDiffRemoved: '#d68273', @@ -124,10 +131,16 @@ const lightColors: MobilePalette = { statusPurple: '#9333ea', gitDecorationAdded: '#587c0c', gitDecorationDeleted: '#ad0707', - diffAddedBg: 'rgba(88, 124, 12, 0.13)', - diffDeletedBg: 'rgba(173, 7, 7, 0.11)', - diffAddedGutter: 'rgba(88, 124, 12, 0.26)', - diffDeletedGutter: 'rgba(173, 7, 7, 0.22)', + diffAddedBg: '#dafbe1', + diffDeletedBg: '#ffebe9', + diffAddedGutter: '#aceebb', + diffDeletedGutter: '#ffcecb', + diffAddedSign: '#1a7f37', + diffDeletedSign: '#cf222e', + diffLineNumber: '#6e7781', + diffFoldBg: '#ddf4ff', + diffFoldGutter: 'rgba(84, 174, 255, 0.4)', + diffFoldText: '#0969da', chatDiffAdded: '#415a0b', chatDiffRemoved: '#7c0808',