From 81d973ffc4be5bec005c9561d17f0059d8a41acd Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Thu, 4 Jun 2026 19:44:45 +0700 Subject: [PATCH] Fix diff gutter row alignment --- packages/app/e2e/diff-row-alignment.spec.ts | 505 ++++++++++++++++++++ packages/app/src/git/diff-pane.tsx | 264 +++++----- packages/app/src/review/surface.tsx | 38 +- 3 files changed, 679 insertions(+), 128 deletions(-) create mode 100644 packages/app/e2e/diff-row-alignment.spec.ts diff --git a/packages/app/e2e/diff-row-alignment.spec.ts b/packages/app/e2e/diff-row-alignment.spec.ts new file mode 100644 index 000000000..61a8ecbfd --- /dev/null +++ b/packages/app/e2e/diff-row-alignment.spec.ts @@ -0,0 +1,505 @@ +import { writeFile } from "node:fs/promises"; +import path from "node:path"; +import { type Page } from "@playwright/test"; +import { buildHostWorkspaceRoute, buildSettingsSectionRoute } from "../src/utils/host-routes"; +import { test, expect } from "./fixtures"; +import { getServerId } from "./helpers/server-id"; +import { connectSeedClient } from "./helpers/seed-client"; +import { createTempGitRepo } from "./helpers/workspace"; +import { waitForWorkspaceTabsVisible } from "./helpers/workspace-tabs"; + +interface DirtyWorkspace { + id: string; +} + +interface CleanupTask { + run: () => Promise; +} + +const cleanupTasks: CleanupTask[] = []; +const APP_SETTINGS_KEY = "@paseo:app-settings"; +const CHANGES_PREFERENCES_KEY = "@paseo:changes-preferences"; + +const BEFORE = `import { useLayoutEffect, useMemo, useRef, useState } from "react"; + +interface UseMountedTabSetInput { + activeTabId: string | null; + allTabIds: string[]; + cap: number; +} + +interface UseMountedTabSetResult { + mountedTabIds: Set; +} + +function createInitialMountedTabIds(input: UseMountedTabSetInput): Set { + if (!input.activeTabId || !input.allTabIds.includes(input.activeTabId)) { + return new Set(); + } + return new Set([input.activeTabId]); +} + +function setsEqual(left: Set, right: Set): boolean { + if (left.size !== right.size) { + return false; + } + for (const value of left) { + if (!right.has(value)) { + return false; + } + } + return true; +} + +export function useMountedTabSet(input: UseMountedTabSetInput): UseMountedTabSetResult { + const { activeTabId, allTabIds, cap } = input; + const allTabIdsKey = allTabIds.join("\\u0000"); + const availableTabIds = useMemo(() => { + void allTabIdsKey; + return new Set(allTabIds); + }, [allTabIds, allTabIdsKey]); + const [mountedTabIds, setMountedTabIds] = useState(() => createInitialMountedTabIds(input)); + const lruRef = useRef(activeTabId && allTabIds.includes(activeTabId) ? [activeTabId] : []); + + useLayoutEffect(() => { + const nextLru = lruRef.current.filter((tabId) => availableTabIds.has(tabId)); + if (activeTabId && availableTabIds.has(activeTabId)) { + const existingIndex = nextLru.indexOf(activeTabId); + if (existingIndex >= 0) { + nextLru.splice(existingIndex, 1); + } + nextLru.unshift(activeTabId); + } + if (nextLru.length > cap) { + nextLru.length = cap; + } + + lruRef.current = nextLru; + setMountedTabIds((previousMountedTabIds) => { + const nextMountedTabIds = new Set(nextLru); + return setsEqual(previousMountedTabIds, nextMountedTabIds) + ? previousMountedTabIds + : nextMountedTabIds; + }); + }, [activeTabId, availableTabIds, cap]); + + return { mountedTabIds }; +} +`; + +const AFTER = `import { useLayoutEffect, useMemo, useRef, useState } from "react"; + +interface UseMountedTabSetInput { + activeTabId: string | null; + allTabIds: string[]; + cap: number; +} + +interface UseMountedTabSetResult { + mountedTabIds: Set; +} + +interface DeriveRenderMountedTabIdsInput { + activeTabId: string | null; + availableTabIds: Set; + cap: number; + mountedTabIds: Set; +} + +function createInitialMountedTabIds(input: UseMountedTabSetInput): Set { + if (!input.activeTabId || !input.allTabIds.includes(input.activeTabId)) { + return new Set(); + } + return new Set([input.activeTabId]); +} + +function setsEqual(left: Set, right: Set): boolean { + if (left.size !== right.size) { + return false; + } + for (const value of left) { + if (!right.has(value)) { + return false; + } + } + return true; +} + +function deriveRenderMountedTabIds(input: DeriveRenderMountedTabIdsInput): Set { + const { activeTabId, availableTabIds, cap, mountedTabIds } = input; + if (!activeTabId || !availableTabIds.has(activeTabId) || mountedTabIds.has(activeTabId)) { + return mountedTabIds; + } + + const next = new Set([activeTabId]); + const maxSize = Math.max(1, cap); + for (const tabId of mountedTabIds) { + if (next.size >= maxSize) { + break; + } + if (availableTabIds.has(tabId)) { + next.add(tabId); + } + } + return next; +} + +export function useMountedTabSet(input: UseMountedTabSetInput): UseMountedTabSetResult { + const { activeTabId, allTabIds, cap } = input; + const allTabIdsKey = allTabIds.join("\\u0000"); + const availableTabIds = useMemo(() => { + void allTabIdsKey; + return new Set(allTabIds); + }, [allTabIds, allTabIdsKey]); + const [mountedTabIds, setMountedTabIds] = useState(() => createInitialMountedTabIds(input)); + const lruRef = useRef(activeTabId && allTabIds.includes(activeTabId) ? [activeTabId] : []); + const renderMountedTabIds = useMemo( + () => + deriveRenderMountedTabIds({ + activeTabId, + availableTabIds, + cap, + mountedTabIds, + }), + [activeTabId, availableTabIds, cap, mountedTabIds], + ); + + useLayoutEffect(() => { + const nextLru = lruRef.current.filter((tabId) => availableTabIds.has(tabId)); + if (activeTabId && availableTabIds.has(activeTabId)) { + const existingIndex = nextLru.indexOf(activeTabId); + if (existingIndex >= 0) { + nextLru.splice(existingIndex, 1); + } + nextLru.unshift(activeTabId); + } + if (nextLru.length > cap) { + nextLru.length = cap; + } + + lruRef.current = nextLru; + setMountedTabIds((previousMountedTabIds) => { + const nextMountedTabIds = new Set(nextLru); + return setsEqual(previousMountedTabIds, nextMountedTabIds) + ? previousMountedTabIds + : nextMountedTabIds; + }); + }, [activeTabId, availableTabIds, cap]); + + return { mountedTabIds: renderMountedTabIds }; +} +`; + +test.afterEach(async () => { + for (const task of cleanupTasks.splice(0)) { + await task.run(); + } +}); + +test("changes diff keeps code rows aligned with the gutter", async ({ page }) => { + const workspace = await createWorkspaceWithMountedTabDiff(); + await useCodeFont(page, 9); + await useUnwrappedDiffLines(page); + await openWorkspaceChanges(page, workspace); + + await expectDiffCodeFontSize(page, 9); + await expectVisibleDiffRowsAligned(page); + await expectDiffCodeTextAlignedWithGutterText(page, [ + { + codeText: "function createInitialMountedTabIds(input: UseMountedTabSetInput)", + lineNumber: "20", + }, + { codeText: "return next;", lineNumber: "55" }, + { codeText: "useLayoutEffect(() => {", lineNumber: "78" }, + ]); + await expectHoverCommentButtonAlignedWithCodeLine(page, { + codeText: "function createInitialMountedTabIds(input: UseMountedTabSetInput)", + lineNumber: "20", + }); +}); + +test("changes diff keeps unwrapped gutter and code rows aligned after code size changes", async ({ + page, +}) => { + const workspace = await createWorkspaceWithMountedTabDiff(); + await useCodeFont(page, 12); + await useUnwrappedDiffLines(page); + await openWorkspaceChanges(page, workspace); + + await changeCodeFontSizeFromSettings(page, 18); + await returnToWorkspaceChanges(page); + await scrollToLowerUnwrappedDiffRows(page); + + await expectDiffCodeFontSize(page, 18); + await expectVisibleDiffRowsShareTypography(page); + await expectVisibleDiffRowsAligned(page); +}); + +async function useCodeFont(page: Page, codeFontSize: number): Promise { + await page.addInitScript( + ({ settingsKey, fontSize }) => { + localStorage.setItem( + settingsKey, + JSON.stringify({ + theme: "dark", + sendBehavior: "interrupt", + serviceUrlBehavior: "ask", + terminalScrollbackLines: 10_000, + uiFontFamily: "", + monoFontFamily: "", + uiFontSize: 16, + codeFontSize: fontSize, + syntaxTheme: "one", + }), + ); + }, + { settingsKey: APP_SETTINGS_KEY, fontSize: codeFontSize }, + ); +} + +async function useUnwrappedDiffLines(page: Page): Promise { + await page.addInitScript( + ({ preferencesKey }) => { + localStorage.setItem( + preferencesKey, + JSON.stringify({ layout: "unified", wrapLines: false, hideWhitespace: false }), + ); + }, + { preferencesKey: CHANGES_PREFERENCES_KEY }, + ); +} + +async function expectDiffCodeFontSize(page: Page, fontSize: number): Promise { + const actualFontSize = await page + .getByTestId("diff-code-text-1") + .evaluate((text) => Number.parseFloat(getComputedStyle(text).fontSize)); + expect(actualFontSize).toBe(fontSize); +} + +async function expectVisibleDiffRowsAligned(page: Page): Promise { + const geometry = await readVisibleDiffRowGeometry(page); + expect(geometry.maxDelta, JSON.stringify(geometry.rows, null, 2)).toBeLessThanOrEqual(1); +} + +async function expectVisibleDiffRowsShareTypography(page: Page): Promise { + const geometry = await readVisibleDiffRowGeometry(page); + expect(geometry.mismatchedTypography, JSON.stringify(geometry, null, 2)).toEqual([]); +} + +async function readVisibleDiffRowGeometry(page: Page): Promise<{ + maxDelta: number; + mismatchedTypography: { index: number; gutterLineHeight: number; codeLineHeight: number }[]; + rows: { + index: number; + gutterTop: number; + codeTop: number; + delta: number; + gutterLineHeight: number; + codeLineHeight: number; + }[]; +}> { + return page.locator("body").evaluate(({ ownerDocument }) => { + const root = ownerDocument.querySelector('[data-testid="diff-file-0-body"]'); + if (!root) { + throw new Error("Expanded diff body is not mounted"); + } + + const readRows = (prefix: string, textPrefix: string) => + Array.from(root.querySelectorAll(`[data-testid^="${prefix}"]`)).map((row) => { + const testId = row.getAttribute("data-testid") ?? ""; + const index = Number(testId.slice(prefix.length)); + const rect = row.getBoundingClientRect(); + const text = root.querySelector(`[data-testid="${textPrefix}${index}"]`); + const lineHeight = text ? Number.parseFloat(getComputedStyle(text).lineHeight) : 0; + return { index, top: rect.top, height: rect.height, lineHeight }; + }); + + const gutters = new Map( + readRows("diff-gutter-row-", "diff-gutter-text-").map((row) => [row.index, row]), + ); + const codes = readRows("diff-code-row-", "diff-code-text-"); + const rows = codes + .map((code) => { + const gutter = gutters.get(code.index); + if (!gutter) { + throw new Error(`Missing gutter row ${code.index}`); + } + return { + index: code.index, + gutterTop: gutter.top, + codeTop: code.top, + delta: Math.abs(code.top - gutter.top), + gutterLineHeight: gutter.lineHeight, + codeLineHeight: code.lineHeight, + }; + }) + .filter((row) => row.gutterTop >= 0 && row.codeTop >= 0); + + return { + maxDelta: Math.max(...rows.map((row) => row.delta)), + mismatchedTypography: rows + .filter((row) => Math.abs(row.gutterLineHeight - row.codeLineHeight) > 0.5) + .map((row) => ({ + index: row.index, + gutterLineHeight: row.gutterLineHeight, + codeLineHeight: row.codeLineHeight, + })), + rows, + }; + }); +} + +async function createWorkspaceWithMountedTabDiff(): Promise { + const repo = await createTempGitRepo("diff-row-alignment-", { + files: [{ path: "src/use-mounted-tab-set.ts", content: BEFORE }], + }); + const client = await connectSeedClient(); + cleanupTasks.push({ + run: async () => { + await client.close().catch(() => undefined); + await repo.cleanup().catch(() => undefined); + }, + }); + + await writeFile(path.join(repo.path, "src/use-mounted-tab-set.ts"), AFTER); + const opened = await client.openProject(repo.path); + if (!opened.workspace) { + throw new Error(opened.error ?? `Failed to open project ${repo.path}`); + } + return { id: opened.workspace.id }; +} + +async function openWorkspaceChanges(page: Page, workspace: DirtyWorkspace): Promise { + await page.setViewportSize({ width: 1400, height: 900 }); + await page.goto(buildHostWorkspaceRoute(getServerId(), workspace.id)); + await waitForWorkspaceTabsVisible(page); + await page.getByRole("button", { name: "Open explorer" }).click(); + await openChangesInVisibleExplorer(page); + await page.getByTestId("diff-file-0").click(); + await expectExpandedMountedTabDiff(page); +} + +async function openChangesInVisibleExplorer(page: Page): Promise { + await expect(page.getByTestId("explorer-tab-changes")).toBeVisible({ timeout: 30_000 }); + await expect(page.getByText("use-mounted-tab-set.ts")).toBeVisible({ timeout: 30_000 }); +} + +async function expectExpandedMountedTabDiff(page: Page): Promise { + await expect(page.getByTestId("diff-file-0-body")).toBeVisible({ timeout: 30_000 }); + await expect(page.getByText("function createInitialMountedTabIds")).toBeVisible({ + timeout: 30_000, + }); +} + +async function changeCodeFontSizeFromSettings(page: Page, codeFontSize: number): Promise { + await page.getByTestId("sidebar-settings").click(); + await expect(page).toHaveURL(new RegExp(`${buildSettingsSectionRoute("general")}|/settings$`)); + await page.getByRole("button", { name: "Appearance" }).click(); + await page.getByLabel("Code font size").fill(String(codeFontSize)); + await page.getByLabel("Code font size").press("Enter"); + await expect(page.getByLabel("Code font size")).toHaveValue(String(codeFontSize)); +} + +async function returnToWorkspaceChanges(page: Page): Promise { + await page.getByTestId("settings-back-to-workspace").click(); + await waitForWorkspaceTabsVisible(page); + await openChangesInVisibleExplorer(page); + await expectExpandedMountedTabDiff(page); +} + +async function scrollToLowerUnwrappedDiffRows(page: Page): Promise { + const lastRowIndex = await page.getByTestId("diff-file-0-body").evaluate((root) => { + const rows = Array.from(root.querySelectorAll('[data-testid^="diff-code-row-"]')); + if (rows.length === 0) { + throw new Error("No unwrapped code rows are mounted"); + } + return Math.max( + ...rows.map((row) => Number((row.getAttribute("data-testid") ?? "").slice(14))), + ); + }); + await page.getByTestId(`diff-code-row-${lastRowIndex}`).scrollIntoViewIfNeeded(); + await expect(page.getByTestId(`diff-code-row-${lastRowIndex}`)).toBeVisible(); +} + +async function expectDiffCodeTextAlignedWithGutterText( + page: Page, + lines: { codeText: string; lineNumber: string }[], +): Promise { + const geometries = await readDiffTextGeometry(page, lines); + for (const geometry of geometries) { + expect(geometry.codeTop, geometry.codeText).toBeCloseTo(geometry.gutterTop, 0); + } +} + +async function expectHoverCommentButtonAlignedWithCodeLine( + page: Page, + line: { codeText: string; lineNumber: string }, +): Promise { + const target = await readDiffTextGeometry(page, [line]).then((rows) => rows[0]); + if (!target) { + throw new Error(`Could not find target line ${line.lineNumber}`); + } + await page.getByTestId(`diff-code-row-${target.index}`).hover(); + const geometry = await page + .getByTestId(`diff-gutter-action-${target.index}`) + .evaluate((action, expectedCodeCenterY) => { + const rect = action.getBoundingClientRect(); + return { + actionCenterY: rect.top + rect.height / 2, + codeCenterY: expectedCodeCenterY, + }; + }, target.codeCenterY); + expect(geometry.actionCenterY).toBeCloseTo(geometry.codeCenterY, 0); +} + +async function readDiffTextGeometry( + page: Page, + lines: { codeText: string; lineNumber: string }[], +): Promise< + { index: number; codeText: string; codeTop: number; gutterTop: number; codeCenterY: number }[] +> { + return page.locator("body").evaluate(({ ownerDocument }, targets) => { + const root = ownerDocument.querySelector('[data-testid="explorer-content-area"]'); + if (!root) { + throw new Error("Changes panel is not mounted"); + } + + const readIndexedElements = (prefix: string) => + Array.from(root.querySelectorAll(`[data-testid^="${prefix}"]`)).map( + (element) => { + const testId = element.getAttribute("data-testid") ?? ""; + return { index: Number(testId.slice(prefix.length)), element }; + }, + ); + + const gutterTexts = readIndexedElements("diff-gutter-text-"); + const codeTexts = readIndexedElements("diff-code-text-"); + + return targets.map((target) => { + const gutter = gutterTexts.find( + ({ element }) => (element.textContent ?? "").trim() === target.lineNumber, + ); + if (!gutter) { + throw new Error(`Could not find gutter line ${target.lineNumber}`); + } + const code = codeTexts.find( + ({ index, element }) => + index === gutter.index && (element.textContent ?? "").includes(target.codeText), + ); + if (!code) { + throw new Error(`Could not find code row ${target.codeText}`); + } + + const codeRect = code.element.getBoundingClientRect(); + const gutterRect = gutter.element.getBoundingClientRect(); + + return { + index: gutter.index, + codeText: target.codeText, + codeTop: codeRect.top, + gutterTop: gutterRect.top, + codeCenterY: codeRect.top + codeRect.height / 2, + }; + }); + }, lines); +} diff --git a/packages/app/src/git/diff-pane.tsx b/packages/app/src/git/diff-pane.tsx index e115de412..3c74a3f58 100644 --- a/packages/app/src/git/diff-pane.tsx +++ b/packages/app/src/git/diff-pane.tsx @@ -25,9 +25,9 @@ import { type ViewStyle, type TextStyle, } from "react-native"; -import { StyleSheet, useUnistyles, withUnistyles } from "react-native-unistyles"; -import { ICON_SIZE, type Theme } from "@/styles/theme"; -import { useIsCompactFormFactor } from "@/constants/layout"; +import { StyleSheet, withUnistyles } from "react-native-unistyles"; +import { BORDER_WIDTH, ICON_SIZE, SPACING, type Theme } from "@/styles/theme"; +import { useIsCompactFormFactor, WORKSPACE_SECONDARY_HEADER_HEIGHT } from "@/constants/layout"; import { AlignJustify, Archive, @@ -55,10 +55,10 @@ import { import { useCheckoutStatusQuery } from "@/git/use-status-query"; import { useCheckoutPrStatusQuery } from "@/git/use-pr-status-query"; import { useChangesPreferences } from "@/hooks/use-changes-preferences"; +import { useAppSettings } from "@/hooks/use-settings"; import { DiffScroll } from "@/components/diff-scroll"; import { syntaxTokenStyleFor } from "@/styles/syntax-token-styles"; import { CODE_SURFACE_DATASET } from "@/styles/code-surface"; -import { WORKSPACE_SECONDARY_HEADER_HEIGHT } from "@/constants/layout"; import { shouldAnchorHeaderBeforeCollapse } from "@/git/diff-scroll"; import { buildSplitDiffRows, @@ -123,6 +123,7 @@ function fileHeaderPressableStyle({ pressed }: PressableStateCallbackType) { interface HighlightedTextProps { tokens: HighlightToken[]; wrapLines?: boolean; + testID?: string; } type WrappedWebTextStyle = TextStyle & { @@ -143,13 +144,10 @@ function HighlightedToken({ token }: { token: HighlightToken }) { return {token.text}; } -function HighlightedText({ tokens, wrapLines = false }: HighlightedTextProps) { - const { theme } = useUnistyles(); - const lineHeight = theme.lineHeight.diff; - +function HighlightedText({ tokens, wrapLines = false, testID }: HighlightedTextProps) { const containerStyle = useMemo( - () => [styles.diffLineText, { lineHeight, ...getWrappedTextStyle(wrapLines) }], - [lineHeight, wrapLines], + () => [styles.diffTextMetrics, styles.diffLineText, getWrappedTextStyle(wrapLines)], + [wrapLines], ); const keyedTokens = useMemo( @@ -158,7 +156,7 @@ function HighlightedText({ tokens, wrapLines = false }: HighlightedTextProps) { ); return ( - + {keyedTokens.map(({ key, token }) => ( ))} @@ -252,6 +250,8 @@ function DiffGutterCell({ reviewActions, isLineHovered, style, + textTestID, + actionTestID, }: { lineNumber: number | null; type: DiffLine["type"] | undefined | null; @@ -260,6 +260,8 @@ function DiffGutterCell({ reviewActions?: InlineReviewActions; isLineHovered?: boolean; style?: StyleProp; + textTestID?: string; + actionTestID?: string; }) { const containerStyle = useMemo( () => [ @@ -272,6 +274,7 @@ function DiffGutterCell({ ); const textStyle = useMemo( () => [ + styles.diffTextMetrics, styles.lineNumberText, type === "add" && styles.addLineNumberText, type === "remove" && styles.removeLineNumberText, @@ -296,8 +299,9 @@ function DiffGutterCell({ isLineHovered={isLineHovered} onStartComment={onStartComment} style={containerStyle} + actionTestID={actionTestID} > - + {formatDiffGutterText(lineNumber)} @@ -312,6 +316,7 @@ function DiffTextLine({ onHoverChange, hoverTargetKey, onHoverTargetChange, + textTestID, }: { line: DiffLine; wrapLines: boolean; @@ -320,6 +325,7 @@ function DiffTextLine({ onHoverChange?: (hovered: boolean) => void; hoverTargetKey?: string | null; onHoverTargetChange?: (key: string | null) => void; + textTestID?: string; }) { const visibleTokens = hasVisibleDiffTokens(line.tokens) ? line.tokens : null; @@ -329,6 +335,7 @@ function DiffTextLine({ ); const textStyle = useMemo( () => [ + styles.diffTextMetrics, styles.diffLineText, getWrappedTextStyle(wrapLines), line.type === "add" && styles.addLineText, @@ -349,9 +356,11 @@ function DiffTextLine({ style={containerStyle} > {line.type !== "header" && visibleTokens ? ( - + ) : ( - {formatDiffContentText(line.content)} + + {formatDiffContentText(line.content)} + )} ); @@ -380,6 +389,7 @@ function SplitTextLine({ ); const textStyle = useMemo( () => [ + styles.diffTextMetrics, styles.diffLineText, getWrappedTextStyle(wrapLines), line?.type === "add" && styles.addLineText, @@ -432,6 +442,7 @@ function DiffLineView({ ); const textStyle = useMemo( () => [ + styles.diffTextMetrics, styles.diffLineText, getWrappedTextStyle(wrapLines), line.type === "add" && styles.addLineText, @@ -487,6 +498,7 @@ function SplitDiffLine({ ); const textStyle = useMemo( () => [ + styles.diffTextMetrics, styles.diffLineText, getWrappedTextStyle(wrapLines), line?.type === "add" && styles.addLineText, @@ -896,6 +908,7 @@ function DiffFileBody({ file, layout, wrapLines, + codeFontSize, reviewActions, onBodyHeightChange, testID, @@ -903,6 +916,7 @@ function DiffFileBody({ file: ParsedDiffFile; layout: "unified" | "split"; wrapLines: boolean; + codeFontSize: number; reviewActions?: InlineReviewActions; onBodyHeightChange?: (file: ParsedDiffFile, height: number) => void; testID?: string; @@ -910,7 +924,6 @@ function DiffFileBody({ const [scrollViewWidth, setScrollViewWidth] = useState(0); const [bodyWidth, setBodyWidth] = useState(0); const [hoveredReviewTargetKey, setHoveredReviewTargetKey] = useState(null); - const { theme } = useUnistyles(); const handleLayout = useCallback( (event: LayoutChangeEvent) => { @@ -950,7 +963,7 @@ function DiffFileBody({ hunk.newStart + hunk.newCount, ); } - const gutterWidth = lineNumberGutterWidth(maxLineNo, theme.fontSize.code); + const gutterWidth = lineNumberGutterWidth(maxLineNo, codeFontSize); if (layout === "split") { const rows = buildSplitDiffRows(file); @@ -981,8 +994,8 @@ function DiffFileBody({ return ( - {computedLines.map(({ line, lineNumber, key, reviewTarget }) => ( - + {computedLines.map(({ line, lineNumber, key, reviewTarget }, index) => ( + - {computedLines.map(({ line, lineNumber, key, reviewTarget }) => ( - + {computedLines.map(({ line, lineNumber, key, reviewTarget }, index) => ( + - {computedLines.map(({ line, key, reviewTarget }) => ( - + {computedLines.map(({ line, key, reviewTarget }, index) => ( + StyleProp; +const foregroundIconColorMapping = (theme: Theme) => ({ color: theme.colors.foreground }); +const foregroundMutedIconColorMapping = (theme: Theme) => ({ color: theme.colors.foregroundMuted }); + +const ThemedActivityIndicator = withUnistyles(ActivityIndicator); +const ThemedAlignJustify = withUnistyles(AlignJustify); +const ThemedColumns2 = withUnistyles(Columns2); +const ThemedPilcrow = withUnistyles(Pilcrow); +const ThemedWrapText = withUnistyles(WrapText); +const ThemedListChevronsDownUp = withUnistyles(ListChevronsDownUp); +const ThemedListChevronsUpDown = withUnistyles(ListChevronsUpDown); +const ThemedGitCommitHorizontal = withUnistyles(GitCommitHorizontal); +const ThemedDownload = withUnistyles(Download); +const ThemedUpload = withUnistyles(Upload); +const ThemedArrowDownUp = withUnistyles(ArrowDownUp); +const ThemedGitHubIcon = withUnistyles(GitHubIcon); +const ThemedGitMerge = withUnistyles(GitMerge); +const ThemedRefreshCcw = withUnistyles(RefreshCcw); +const ThemedArchive = withUnistyles(Archive); +const ThemedGitBranch = withUnistyles(GitBranch); +const ThemedChevronDown = withUnistyles(ChevronDown); + interface DiffLayoutToggleGroupProps { layout: "unified" | "split"; unifiedToggleStyle: PressableStyleFn; @@ -1089,7 +1126,6 @@ function DiffLayoutToggleGroup({ onUnified, onSplit, }: DiffLayoutToggleGroupProps) { - const { theme } = useUnistyles(); return ( @@ -1101,9 +1137,11 @@ function DiffLayoutToggleGroup({ onPress={onUnified} style={unifiedToggleStyle} > - @@ -1120,9 +1158,11 @@ function DiffLayoutToggleGroup({ onPress={onSplit} style={splitToggleStyle} > - @@ -1147,7 +1187,6 @@ function DiffWhitespaceToggle({ toggleStyle, onToggle, }: DiffWhitespaceToggleProps) { - const { theme } = useUnistyles(); return ( @@ -1158,9 +1197,9 @@ function DiffWhitespaceToggle({ style={toggleStyle} onPress={onToggle} > - @@ -1190,15 +1229,14 @@ function DiffFilesToolbar({ onToggleWrapLines, onToggleExpandAll, }: DiffFilesToolbarProps) { - const { theme } = useUnistyles(); return ( - @@ -1212,9 +1250,15 @@ function DiffFilesToolbar({ {allExpanded ? ( - + ) : ( - + )} @@ -1236,7 +1280,6 @@ interface DiffRefreshButtonProps { const ThemedRotateCw = withUnistyles(RotateCw); const ThemedLoadingSpinner = withUnistyles(LoadingSpinner); -const refreshIconColorMapping = (theme: Theme) => ({ color: theme.colors.foregroundMuted }); function DiffRefreshButton({ isRefreshing, toggleStyle, onPress }: DiffRefreshButtonProps) { return ( @@ -1252,9 +1295,12 @@ function DiffRefreshButton({ isRefreshing, toggleStyle, onPress }: DiffRefreshBu > {isRefreshing ? ( - + ) : ( - + )} @@ -1322,7 +1368,6 @@ interface DiffBodyContentProps { handleDiffListScroll: (event: NativeSyntheticEvent) => void; onContentSizeChange: (width: number, height: number) => void; showDesktopWebScrollbar: boolean; - foregroundMutedColor: string; } function DiffBodyContent({ @@ -1344,12 +1389,11 @@ function DiffBodyContent({ handleDiffListScroll, onContentSizeChange, showDesktopWebScrollbar, - foregroundMutedColor, }: DiffBodyContentProps) { if (isStatusLoading) { return ( - + Checking repository... ); @@ -1371,7 +1415,7 @@ function DiffBodyContent({ if (isDiffLoading) { return ( - + ); } @@ -1483,30 +1527,27 @@ function computePrErrorMessage( return prPayloadError?.message ?? null; } -function buildDiffModeTriggerStyle(surfaceColor: string): PressableStyleFn { +function buildDiffModeTriggerStyle(): PressableStyleFn { return ({ hovered, pressed, open }) => [ styles.diffModeTrigger, - (Boolean(hovered) || pressed || Boolean(open)) && - inlineUnistylesStyle({ backgroundColor: surfaceColor }), + (Boolean(hovered) || pressed || Boolean(open)) && styles.diffModeTriggerHovered, ]; } -function buildExpandAllButtonStyle(surfaceColor: string): PressableStyleFn { +function buildExpandAllButtonStyle(): PressableStyleFn { return ({ hovered, pressed }) => [ styles.expandAllButton, - (Boolean(hovered) || pressed) && inlineUnistylesStyle({ backgroundColor: surfaceColor }), + (Boolean(hovered) || pressed) && styles.toggleButtonSelected, ]; } function buildToggleButtonStyle( selected: boolean, baseStyles: StyleProp | StyleProp[], - surfaceColor: string, ): PressableStyleFn { return ({ hovered, pressed }) => [ baseStyles, - (selected || Boolean(hovered) || pressed) && - inlineUnistylesStyle({ backgroundColor: surfaceColor }), + (selected || Boolean(hovered) || pressed) && styles.toggleButtonSelected, ]; } @@ -1521,7 +1562,7 @@ export function GitDiffPane({ hideHeaderRow, enabled, }: GitDiffPaneProps) { - const { theme } = useUnistyles(); + const { settings: appSettings } = useAppSettings(); const isMobile = useIsCompactFormFactor(); const showDesktopWebScrollbar = isWeb && !isMobile; const canUseSplitLayout = isWeb && !isMobile; @@ -1557,56 +1598,44 @@ export function GitDiffPane({ handleLayoutChange("split"); }, [handleLayoutChange]); - const controlSurfaceColor = theme.colors.surface2; - const diffModeTriggerStyle = useMemo( - () => buildDiffModeTriggerStyle(controlSurfaceColor), - [controlSurfaceColor], + const codeFontSize = appSettings.codeFontSize; + const diffBodyLineHeight = Math.round(codeFontSize * 1.5); + const diffBodyTypographyKey = [appSettings.monoFontFamily, codeFontSize, diffBodyLineHeight].join( + ":", ); + const diffModeTriggerStyle = useMemo(() => buildDiffModeTriggerStyle(), []); const unifiedToggleStyle = useMemo( () => - buildToggleButtonStyle( - changesPreferences.layout === "unified", - [styles.toggleButton, styles.toggleButtonGroupStart], - controlSurfaceColor, - ), - [changesPreferences.layout, controlSurfaceColor], + buildToggleButtonStyle(changesPreferences.layout === "unified", [ + styles.toggleButton, + styles.toggleButtonGroupStart, + ]), + [changesPreferences.layout], ); const splitToggleStyle = useMemo( () => - buildToggleButtonStyle( - changesPreferences.layout === "split", - [styles.toggleButton, styles.toggleButtonGroupEnd], - controlSurfaceColor, - ), - [changesPreferences.layout, controlSurfaceColor], + buildToggleButtonStyle(changesPreferences.layout === "split", [ + styles.toggleButton, + styles.toggleButtonGroupEnd, + ]), + [changesPreferences.layout], ); const hideWhitespaceToggleStyle = useMemo( - () => - buildToggleButtonStyle( - changesPreferences.hideWhitespace, - styles.expandAllButton, - controlSurfaceColor, - ), - [changesPreferences.hideWhitespace, controlSurfaceColor], + () => buildToggleButtonStyle(changesPreferences.hideWhitespace, styles.expandAllButton), + [changesPreferences.hideWhitespace], ); const wrapLinesToggleStyle = useMemo( - () => buildToggleButtonStyle(wrapLines, styles.expandAllButton, controlSurfaceColor), - [wrapLines, controlSurfaceColor], + () => buildToggleButtonStyle(wrapLines, styles.expandAllButton), + [wrapLines], ); - const expandAllToggleStyle = useMemo( - () => buildExpandAllButtonStyle(controlSurfaceColor), - [controlSurfaceColor], - ); + const expandAllToggleStyle = useMemo(() => buildExpandAllButtonStyle(), []); - const refreshToggleStyle = useMemo( - () => buildExpandAllButtonStyle(controlSurfaceColor), - [controlSurfaceColor], - ); + const refreshToggleStyle = useMemo(() => buildExpandAllButtonStyle(), []); const toast = useToast(); const refreshSupported = useSessionStore( @@ -1754,9 +1783,8 @@ export function GitDiffPane({ const bodyHeightByKeyRef = useRef>({}); const defaultHeaderHeightRef = useRef(44); const [heightVersion, setHeightVersion] = useState(0); - const diffBodyLineHeight = theme.lineHeight.diff; - const diffBodyChromeHeight = theme.borderWidth[1] * 2; - const statusBodyHeightEstimate = diffBodyChromeHeight + theme.spacing[4] * 2 + diffBodyLineHeight; + const diffBodyChromeHeight = BORDER_WIDTH[1] * 2; + const statusBodyHeightEstimate = diffBodyChromeHeight + SPACING[4] * 2 + diffBodyLineHeight; const { flatItems, stickyHeaderIndices } = useMemo(() => { const items: DiffFlatItem[] = []; const stickyIndices: number[] = []; @@ -1777,12 +1805,13 @@ export function GitDiffPane({ const getBodyHeightKey = useCallback( (file: ParsedDiffFile): string => { if (file.status === "too_large" || file.status === "binary") { - return `${effectiveLayout}:${wrapLines ? "wrap" : "scroll"}:${file.path}:${file.status}`; + return `${effectiveLayout}:${wrapLines ? "wrap" : "scroll"}:${diffBodyTypographyKey}:${file.path}:${file.status}`; } return [ effectiveLayout, wrapLines ? "wrap" : "scroll", + diffBodyTypographyKey, file.path, file.status ?? "ok", file.additions, @@ -1792,7 +1821,7 @@ export function GitDiffPane({ getDiffContentLength(file), ].join(":"); }, - [effectiveLayout, wrapLines], + [diffBodyTypographyKey, effectiveLayout, wrapLines], ); const estimateBodyHeight = useCallback( @@ -1961,6 +1990,7 @@ export function GitDiffPane({ file={item.file} layout={effectiveLayout} wrapLines={wrapLines} + codeFontSize={codeFontSize} reviewActions={reviewActions} onBodyHeightChange={handleBodyHeightChange} testID={`diff-file-${item.fileIndex}-body`} @@ -1968,6 +1998,7 @@ export function GitDiffPane({ ); }, [ + codeFontSize, effectiveLayout, handleBodyHeightChange, handleHeaderHeightChange, @@ -2015,34 +2046,41 @@ export function GitDiffPane({ () => ({ expandedPathsArray, effectiveLayout, + diffBodyTypographyKey, heightVersion, wrapLines, reviewActions, }), - [expandedPathsArray, effectiveLayout, heightVersion, wrapLines, reviewActions], + [ + expandedPathsArray, + effectiveLayout, + diffBodyTypographyKey, + heightVersion, + wrapLines, + reviewActions, + ], ); const hasChanges = files.length > 0; const diffErrorMessage = diffPayloadError?.message ?? null; const prErrorMessage = computePrErrorMessage(githubFeaturesEnabled, prPayloadError); const baseRefLabel = useMemo(() => computeBaseRefLabel(baseRef), [baseRef]); - const iconColor = theme.colors.foregroundMuted; const gitActionsIcons = useMemo( () => ({ - commit: , - pull: , - push: , - pullAndPush: , - viewPr: , - createPr: , - mergePrSquash: , - mergePrMerge: , - mergePrRebase: , - merge: , - mergeFromBase: , - archive: , + commit: , + pull: , + push: , + pullAndPush: , + viewPr: , + createPr: , + mergePrSquash: , + mergePrMerge: , + mergePrRebase: , + merge: , + mergeFromBase: , + archive: , }), - [iconColor], + [], ); const { gitActions, branchLabel } = useGitActions({ serverId, cwd, icons: gitActionsIcons }); const committedDiffDescription = useMemo( @@ -2076,7 +2114,6 @@ export function GitDiffPane({ handleDiffListScroll={handleDiffListScroll} onContentSizeChange={scrollbar.onContentSizeChange} showDesktopWebScrollbar={showDesktopWebScrollbar} - foregroundMutedColor={theme.colors.foregroundMuted} /> ); @@ -2085,7 +2122,7 @@ export function GitDiffPane({ {!hideHeaderRow ? ( - + {branchLabel} @@ -2107,7 +2144,7 @@ export function GitDiffPane({ {diffMode === "uncommitted" ? "Uncommitted" : "Committed"} - + ({ elevation: 4, overflow: "visible", }, + diffTextMetrics: { + fontSize: theme.fontSize.code, + lineHeight: theme.lineHeight.diff, + fontFamily: theme.fontFamily.mono, + }, lineNumberText: { width: "100%", textAlign: "right", paddingRight: theme.spacing[2], - fontSize: theme.fontSize.code, - lineHeight: theme.lineHeight.diff, - fontFamily: theme.fontFamily.mono, color: theme.colors.foregroundMuted, userSelect: "none", }, @@ -2570,9 +2609,6 @@ const styles = StyleSheet.create((theme) => ({ diffLineText: { flex: 1, paddingRight: theme.spacing[3], - fontSize: theme.fontSize.code, - lineHeight: theme.lineHeight.diff, - fontFamily: theme.fontFamily.mono, color: theme.colors.foreground, userSelect: "text", }, @@ -2621,7 +2657,7 @@ const styles = StyleSheet.create((theme) => ({ }, })); -const HEADER_LINE_TEXT_STYLE = [styles.diffLineText, styles.headerLineText]; +const HEADER_LINE_TEXT_STYLE = [styles.diffTextMetrics, styles.diffLineText, styles.headerLineText]; const FILE_SECTION_BODY_STYLE = [styles.fileSectionBodyContainer, styles.fileSectionBorder]; const DIFF_CONTENT_SPLIT_ROW_STYLE = [styles.diffContent, styles.splitRow]; const DIFF_CONTENT_ROW_STYLE = [styles.diffContent, styles.diffContentRow]; diff --git a/packages/app/src/review/surface.tsx b/packages/app/src/review/surface.tsx index 1ea505e38..ffc2764eb 100644 --- a/packages/app/src/review/surface.tsx +++ b/packages/app/src/review/surface.tsx @@ -11,11 +11,12 @@ import { type StyleProp, type ViewStyle, } from "react-native"; -import { StyleSheet, useUnistyles } from "react-native-unistyles"; +import { StyleSheet, withUnistyles } from "react-native-unistyles"; import { Button } from "@/components/ui/button"; import { Shortcut } from "@/components/ui/shortcut"; import { isWeb } from "@/constants/platform"; import { inlineUnistylesStyle } from "@/styles/unistyles-inline-style"; +import type { Theme } from "@/styles/theme"; import type { ShortcutKey } from "@/utils/format-shortcut"; import { useWorkspaceFocusRestoration } from "@/workspace/focus"; import { useReviewDraftComments, useReviewDraftStore, type ReviewDraftComment } from "./store"; @@ -77,6 +78,14 @@ const INLINE_REVIEW_GAP = 6; export const SMALL_ACTION_HIT_SLOP = 8; const REVIEW_CANCEL_SHORTCUT_KEYS: ShortcutKey[] = ["Esc"]; const REVIEW_SAVE_SHORTCUT_KEYS: ShortcutKey[] = ["mod", "Enter"]; +const foregroundMutedIconColorMapping = (theme: Theme) => ({ color: theme.colors.foregroundMuted }); +const destructiveIconColorMapping = (theme: Theme) => ({ color: theme.colors.destructive }); +const accentForegroundIconColorMapping = (theme: Theme) => ({ + color: theme.colors.accentForeground, +}); +const ThemedPencil = withUnistyles(Pencil); +const ThemedPlus = withUnistyles(Plus); +const ThemedTrash2 = withUnistyles(Trash2); export interface InlineReviewEditorState { target: ReviewableDiffTarget; @@ -274,6 +283,7 @@ export function InlineReviewGutterCell({ isLineHovered = false, onStartComment, style, + actionTestID, }: { children: ReactNode; reviewTarget: ReviewableDiffTarget | null | undefined; @@ -282,8 +292,8 @@ export function InlineReviewGutterCell({ isLineHovered?: boolean; onStartComment: (target: ReviewableDiffTarget) => void; style?: StyleProp; + actionTestID?: string; }) { - const { theme } = useUnistyles(); const canComment = Boolean(reviewTarget); const hasComments = comments.length > 0; const [isGutterHovered, setIsGutterHovered] = useState(false); @@ -345,8 +355,8 @@ export function InlineReviewGutterCell({ {children} {showAction ? ( - - + + ) : null} @@ -429,8 +439,6 @@ function CommentRow({ onEditComment: (target: ReviewableDiffTarget, comment: ReviewDraftComment) => void; onDeleteComment: (id: string) => void; }) { - const { theme } = useUnistyles(); - const handleEdit = useCallback( () => onEditComment(reviewTarget, comment), [onEditComment, reviewTarget, comment], @@ -455,7 +463,7 @@ function CommentRow({ onPress={handleEdit} style={iconButtonStyle} > - + - + @@ -499,7 +507,6 @@ export function InlineReviewEditor({ onSave: (body: string) => void; testID?: string; }) { - const { theme } = useUnistyles(); const inputRef = useRef(null); const focus = useWorkspaceFocusRestoration(); const canShowKeyboardHints = useCanShowReviewKeyboardHints(); @@ -577,7 +584,7 @@ export function InlineReviewEditor({ accessibilityLabel="Review comment" testID={testID ? `${testID}-input` : undefined} placeholder="Leave a comment" - placeholderTextColor={theme.colors.foregroundMuted} + placeholderTextColor={styles.placeholderColor.color} multiline value={body} onChangeText={setBody} @@ -617,16 +624,16 @@ export function InlineReviewEditor({ const styles = StyleSheet.create((theme) => ({ gutterInner: { minHeight: theme.lineHeight.diff, - alignItems: "center", - justifyContent: "center", + alignItems: "stretch", + justifyContent: "flex-start", overflow: "visible", }, gutterLabel: { width: "100%", minWidth: 0, height: theme.lineHeight.diff, - alignItems: "center", - justifyContent: "center", + alignItems: "stretch", + justifyContent: "flex-start", position: "relative", overflow: "visible", }, @@ -646,6 +653,9 @@ const styles = StyleSheet.create((theme) => ({ zIndex: 10, elevation: 10, }, + placeholderColor: { + color: theme.colors.foregroundMuted, + }, threadContainer: { flex: 1, minWidth: 0,