diff --git a/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/ReviewDiffCanvasDrawing.kt b/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/ReviewDiffCanvasDrawing.kt index 6782e6894d99..64c98a9b6933 100644 --- a/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/ReviewDiffCanvasDrawing.kt +++ b/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/ReviewDiffCanvasDrawing.kt @@ -15,6 +15,7 @@ import kotlin.math.min internal class ReviewDiffCanvasDrawing(context: Context) { private val density = context.resources.displayMetrics.density var theme: DiffTheme = DiffTheme.fallback("light") + private val wordDiffRangesByRowId = mutableMapOf>() val backgroundPaint = Paint() val borderPaint = Paint(Paint.ANTI_ALIAS_FLAG) @@ -41,6 +42,14 @@ internal class ReviewDiffCanvasDrawing(context: Context) { uiPaint.typeface = Typeface.DEFAULT_BOLD } + fun mergeWordDiffRangesByRowId(patch: Map>) { + wordDiffRangesByRowId.putAll(patch) + } + + fun clearWordDiffRanges() { + wordDiffRangesByRowId.clear() + } + fun fileHeaderChevronRect(top: Int, bottom: Int, style: DiffStyle): RectF { val centerY = (top + bottom) / 2f val left = style.fileHeaderHorizontalPaddingPx @@ -205,14 +214,15 @@ internal class ReviewDiffCanvasDrawing(context: Context) { top: Int, bottom: Int ) { - if (row.wordDiffRanges.isEmpty() || (row.change != "add" && row.change != "delete")) return + val ranges = wordDiffRangesByRowId[row.id] ?: row.wordDiffRanges + if (ranges.isEmpty() || (row.change != "add" && row.change != "delete")) return val color = if (row.change == "add") theme.addBar else theme.deleteBar backgroundPaint.color = withAlpha(color, 71) val characterWidth = textPaint.measureText("M") val fontHeight = textPaint.fontMetrics.run { descent - ascent } val highlightHeight = max(4f * density, min(bottom - top - 4f * density, fontHeight)) val highlightTop = (top + bottom - highlightHeight) / 2f - row.wordDiffRanges.forEach { range -> + ranges.forEach { range -> val left = codeX + range.start * characterWidth val right = max(left + 2f * density, codeX + range.end * characterWidth) canvas.drawRoundRect( diff --git a/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/T3ReviewDiffView.kt b/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/T3ReviewDiffView.kt index 97e9f696db90..6b32c31fb625 100644 --- a/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/T3ReviewDiffView.kt +++ b/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/T3ReviewDiffView.kt @@ -61,8 +61,8 @@ class T3ReviewDiffView(context: Context, appContext: AppContext) : ExpoView(cont onDebug( mapOf( "message" to "visible-range", - "firstRowIndex" to first, - "lastRowIndex" to last, + "firstRowIndex" to (visibleRows.getOrNull(first)?.sourceIndex ?: first), + "lastRowIndex" to (visibleRows.getOrNull(last)?.sourceIndex ?: last), ), ) emitVisibleFile(first) @@ -78,6 +78,7 @@ class T3ReviewDiffView(context: Context, appContext: AppContext) : ExpoView(cont if (tokensResetKey == value) return tokensResetKey = value canvasView.tokensByRowId = emptyMap() + canvasView.drawing.clearWordDiffRanges() } fun setContentResetKey(value: String) { @@ -85,6 +86,7 @@ class T3ReviewDiffView(context: Context, appContext: AppContext) : ExpoView(cont contentResetKey = value tokensDecodeGeneration += 1 canvasView.tokensByRowId = emptyMap() + canvasView.drawing.clearWordDiffRanges() lastVisibleFileId = null pendingInitialScroll = true canvasView.setVerticalOffset(0) @@ -167,6 +169,7 @@ class T3ReviewDiffView(context: Context, appContext: AppContext) : ExpoView(cont } fun setTokensPatchJson(value: String) { + val expectedContentResetKey = contentResetKey payloadDecodeExecutor.execute { try { val payload = JSONObject(value) @@ -174,11 +177,19 @@ class T3ReviewDiffView(context: Context, appContext: AppContext) : ExpoView(cont val decodedTokens = parseTokensObject( payload.optJSONObject("tokensByRowId") ?: JSONObject(), ) + val decodedWordDiffRanges = parseWordDiffRangesObject( + payload.optJSONObject("wordDiffRangesByRowId") ?: JSONObject(), + ) post { + if (expectedContentResetKey != contentResetKey) return@post if (resetKey.isNotEmpty() && resetKey != tokensResetKey) return@post if (decodedTokens.isNotEmpty()) { canvasView.tokensByRowId = canvasView.tokensByRowId + decodedTokens } + if (decodedWordDiffRanges.isNotEmpty()) { + canvasView.drawing.mergeWordDiffRangesByRowId(decodedWordDiffRanges) + canvasView.invalidate() + } } } catch (_: Exception) { } @@ -434,6 +445,8 @@ private data class HorizontalPanTarget( ) internal data class DiffRow( + // JavaScript range requests use the full payload position, even when files are collapsed. + val sourceIndex: Int, val kind: String, val id: String, val fileId: String, @@ -646,7 +659,7 @@ internal data class DiffStyle( private class DiffCanvasView(context: Context) : View(context) { private val density = resources.displayMetrics.density - private val drawing = ReviewDiffCanvasDrawing(context) + val drawing = ReviewDiffCanvasDrawing(context) private val backgroundPaint = drawing.backgroundPaint private val borderPaint = drawing.borderPaint private val textPaint = drawing.textPaint @@ -1337,6 +1350,7 @@ private fun parseRows(value: String): List = try { List(array.length()) { index -> val row = array.getJSONObject(index) DiffRow( + sourceIndex = index, kind = row.optString("kind"), id = row.optString("id"), fileId = row.optString("fileId"), @@ -1371,6 +1385,17 @@ private fun parseWordDiffRanges(value: JSONArray): List = bui } } +private fun parseWordDiffRangesObject( + value: JSONObject +): Map> = buildMap { + val keys = value.keys() + while (keys.hasNext()) { + val rowId = keys.next() + val ranges = value.optJSONArray(rowId) ?: continue + put(rowId, parseWordDiffRanges(ranges)) + } +} + private fun parseTokensObject(value: String): Map> = try { parseTokensObject(JSONObject(value)) } catch (_: Exception) { diff --git a/apps/mobile/modules/t3-review-diff/ios/T3ReviewDiffView.swift b/apps/mobile/modules/t3-review-diff/ios/T3ReviewDiffView.swift index 9b36d60f94ec..c739170ca7de 100644 --- a/apps/mobile/modules/t3-review-diff/ios/T3ReviewDiffView.swift +++ b/apps/mobile/modules/t3-review-diff/ios/T3ReviewDiffView.swift @@ -36,6 +36,7 @@ private struct ReviewDiffNativeTokenPatch: Decodable, Sendable { let resetKey: String? let chunkIndex: Int? let tokensByRowId: [String: [ReviewDiffNativeToken]]? + let wordDiffRangesByRowId: [String: [ReviewDiffNativeWordDiffRange]]? } private struct ReviewDiffNativeThemePayload: Decodable { @@ -520,11 +521,17 @@ public final class T3ReviewDiffView: ExpoView, UIScrollViewDelegate { } let tokensByRowId = patch.tokensByRowId ?? [:] - if tokensByRowId.isEmpty { + let wordDiffRangesByRowId = patch.wordDiffRangesByRowId ?? [:] + if tokensByRowId.isEmpty && wordDiffRangesByRowId.isEmpty { return } - self.contentView.mergeTokensByRowId(tokensByRowId) + if !tokensByRowId.isEmpty { + self.contentView.mergeTokensByRowId(tokensByRowId) + } + if !wordDiffRangesByRowId.isEmpty { + self.contentView.mergeWordDiffRangesByRowId(wordDiffRangesByRowId) + } if let chunkIndex = patch.chunkIndex, chunkIndex < 5 || chunkIndex.isMultiple(of: 10) { self.emitDebug("tokens-patch-decoded", [ "chunkIndex": chunkIndex, @@ -549,6 +556,7 @@ public final class T3ReviewDiffView: ExpoView, UIScrollViewDelegate { self.tokensResetKey = tokensResetKey contentView.tokensByRowId = [:] + contentView.clearWordDiffRanges() emitDebug("tokens-reset", [ "resetKey": tokensResetKey, ]) @@ -563,6 +571,7 @@ public final class T3ReviewDiffView: ExpoView, UIScrollViewDelegate { rowsDecodeGeneration += 1 tokensDecodeGeneration += 1 contentView.tokensByRowId = [:] + contentView.clearWordDiffRanges() rows = [] contentView.rows = [] hasAppliedInitialRowIndex = false @@ -951,6 +960,18 @@ private final class ReviewDiffContentView: UIView, UIGestureRecognizerDelegate { setNeedsDisplayForVisibleBounds() } + private var wordDiffRangesByRowId: [String: [ReviewDiffNativeWordDiffRange]] = [:] + + func mergeWordDiffRangesByRowId(_ patch: [String: [ReviewDiffNativeWordDiffRange]]) { + wordDiffRangesByRowId.merge(patch) { _, next in next } + setNeedsDisplayForVisibleBounds() + } + + func clearWordDiffRanges() { + wordDiffRangesByRowId.removeAll() + setNeedsDisplayForVisibleBounds() + } + var collapsedFileIds: Set = [] { didSet { rebuildRowLayout() @@ -2268,7 +2289,7 @@ private final class ReviewDiffContentView: UIView, UIGestureRecognizerDelegate { context: CGContext, horizontalOffset: CGFloat ) { - guard let ranges = row.wordDiffRanges, !ranges.isEmpty else { + guard let ranges = wordDiffRangesByRowId[row.id] ?? row.wordDiffRanges, !ranges.isEmpty else { return } diff --git a/apps/mobile/package.json b/apps/mobile/package.json index b32117c03155..ebe59048379b 100644 --- a/apps/mobile/package.json +++ b/apps/mobile/package.json @@ -132,7 +132,9 @@ "@pierre/trees": "1.0.0-beta.4", "@types/react": "~19.2.0", "@types/react-dom": "~19.2.3", + "@types/react-test-renderer": "19.1.0", "babel-preset-expo": "~57.0.9", + "react-test-renderer": "19.2.3", "tailwindcss": "^4.0.0", "typescript": "catalog:" }, diff --git a/apps/mobile/src/features/diffs/nativeReviewDiffSurface.test.ts b/apps/mobile/src/features/diffs/nativeReviewDiffSurface.test.ts index 08ab53971b68..c729f505dfeb 100644 --- a/apps/mobile/src/features/diffs/nativeReviewDiffSurface.test.ts +++ b/apps/mobile/src/features/diffs/nativeReviewDiffSurface.test.ts @@ -1,4 +1,19 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vite-plus/test"; +import { act, createElement, forwardRef, useImperativeHandle, useLayoutEffect } from "react"; +import { create, type ReactTestRenderer } from "react-test-renderer"; +import type { NativeReviewDiffRow } from "./nativeReviewDiffSurface"; +import type { computeVisibleNativeReviewWordDiffRanges } from "../review/nativeReviewWordDiffs"; + +type WordDiffInput = Parameters[0]; +type WordDiffResult = Awaited>; + +const wordJobs = vi.hoisted( + () => + [] as Array<{ + readonly input: WordDiffInput; + readonly complete: () => Promise; + }>, +); const expoMocks = vi.hoisted(() => ({ requireNativeView: vi.fn(), @@ -16,15 +31,40 @@ vi.mock("expo", () => ({ requireNativeView: expoMocks.requireNativeView, })); +vi.mock("./nativeReviewDiffHighlighter", () => ({ + highlightNativeReviewDiffVisibleRows: async () => ({ tokensByRowId: {}, rowCount: 0 }), +})); + +vi.mock("../review/nativeReviewWordDiffs", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + computeVisibleNativeReviewWordDiffRanges: (input: WordDiffInput) => { + const deferred = Promise.withResolvers(); + wordJobs.push({ + input, + complete: async () => { + const result = await actual.computeVisibleNativeReviewWordDiffRanges(input); + deferred.resolve(result); + return result; + }, + }); + return deferred.promise; + }, + }; +}); + describe("resolveNativeReviewDiffView", () => { beforeEach(() => { vi.clearAllMocks(); vi.resetModules(); + wordJobs.length = 0; globalThis.expo = undefined as unknown as typeof globalThis.expo; }); afterEach(() => { globalThis.expo = originalExpo; + vi.unstubAllGlobals(); }); it("returns null when the native review diff view config is unavailable", async () => { @@ -65,6 +105,297 @@ describe("resolveNativeReviewDiffView", () => { ); expect(consoleError).toHaveBeenCalledTimes(1); }); + + it("keeps visible word ranges across delayed delivery and highlight resets", async () => { + setExpoViewConfigAvailable(); + vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); + const frames = new Map void>(); + let frameId = 0; + vi.stubGlobal("requestAnimationFrame", (callback: (time: number) => void) => { + frames.set(++frameId, callback); + return frameId; + }); + vi.stubGlobal("cancelAnimationFrame", (id: number) => frames.delete(id)); + const sent: string[] = []; + const nativeHandle = { + setRowsJson: async () => undefined, + setTokensJson: async () => undefined, + setTokensPatchJson: async (json: string) => { + sent.push(json); + }, + }; + const NativeMock = forwardRef(function NativeMock(_props, ref) { + useImperativeHandle(ref, () => nativeHandle, []); + return null; + }); + expoMocks.requireNativeView.mockReturnValue(NativeMock); + const { resolveNativeReviewDiffView } = await import("./nativeReviewDiffSurface"); + const { useNativeReviewDiffHighlighting } = + await import("../review/useNativeReviewDiffHighlighting"); + const NativeView = resolveNativeReviewDiffView(); + if (!NativeView) throw new Error("Expected the native payload sender"); + const pair = (id: string): NativeReviewDiffRow[] => [ + { + kind: "line", + id: `${id}:delete`, + fileId: id, + change: "delete", + content: 'const item = renderPanel({ title: "before", active: true });', + }, + { + kind: "line", + id: `${id}:add`, + fileId: id, + change: "add", + content: 'const item = renderPanel({ title: "after", active: true });', + }, + ]; + const rows: NativeReviewDiffRow[] = [ + ...pair("a"), + ...Array.from({ length: 500 }, (_, index) => ({ + kind: "hunk" as const, + id: `gap:${index}`, + fileId: "gap", + })), + ...pair("b"), + ]; + const input = { + files: [], + rows, + scheme: "dark" as const, + enabled: true, + collapsedFileIds: [], + }; + let changeRange: + | ReturnType["updateVisibleRange"] + | undefined; + function Harness({ + resetKey = "source", + contentResetKey = "view", + }: { + readonly resetKey?: string; + readonly contentResetKey?: string; + }) { + const result = useNativeReviewDiffHighlighting({ ...input, resetKey, contentResetKey }); + useLayoutEffect(() => { + changeRange = result.updateVisibleRange; + }, [result.updateVisibleRange]); + return createElement(NativeView!, { + appearanceScheme: "dark", + themeJson: "{}", + rowHeight: 20, + contentWidth: 1000, + rowsJson: "[]", + tokensResetKey: resetKey, + contentResetKey, + tokensPatchJson: result.tokensPatchJson, + wordDiffRangesPatchJson: result.wordDiffRangesPatchJson, + onWordDiffRangesPatchSent: result.onWordDiffRangesPatchSent, + }); + } + const flushFrames = () => + act(async () => { + const pending = [...frames.values()]; + frames.clear(); + for (const callback of pending) callback(performance.now()); + await Promise.resolve(); + }); + let renderer: ReactTestRenderer | undefined; + try { + await act(async () => { + renderer = create(createElement(Harness)); + }); + await flushFrames(); + await act(async () => { + await wordJobs.at(-1)!.complete(); + }); + expect(sent.some((json) => json.includes('"a:delete"'))).toBe(false); + await act(async () => { + changeRange!({ firstRowIndex: 502, lastRowIndex: 503 }); + }); + await act(async () => { + await wordJobs.at(-1)!.complete(); + }); + await flushFrames(); + expect(sent.some((json) => json.includes('"b:delete"'))).toBe(true); + expect(sent.some((json) => json.includes('"a:delete"'))).toBe(false); + await act(async () => { + changeRange!({ firstRowIndex: 0, lastRowIndex: 1 }); + }); + expect(wordJobs.at(-1)!.input.alreadyHighlightedRowIds?.has("a:delete")).toBe(false); + await act(async () => { + await wordJobs.at(-1)!.complete(); + }); + await flushFrames(); + expect(sent.some((json) => json.includes('"a:delete"'))).toBe(true); + await act(async () => { + changeRange!({ firstRowIndex: 502, lastRowIndex: 503 }); + }); + await act(async () => { + await wordJobs.at(-1)!.complete(); + }); + await act(async () => { + renderer!.update(createElement(Harness, { resetKey: "refreshed" })); + }); + expect(wordJobs.at(-1)!.input.firstRowIndex).toBe(502); + await act(async () => { + await wordJobs.at(-1)!.complete(); + }); + await flushFrames(); + expect( + sent.some((json) => json.includes('"resetKey":"refreshed"') && json.includes('"b:delete"')), + ).toBe(true); + await act(async () => { + renderer!.update( + createElement(Harness, { resetKey: "new-source", contentResetKey: "new-view" }), + ); + }); + expect(wordJobs.at(-1)!.input.firstRowIndex).toBe(0); + } finally { + await act(async () => renderer?.unmount()); + } + expect(frames.size).toBe(0); + }); + + it("requests new word ranges after many small scroll steps", async () => { + vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); + const { useNativeReviewDiffHighlighting } = + await import("../review/useNativeReviewDiffHighlighting"); + const rows: NativeReviewDiffRow[] = Array.from({ length: 2000 }, (_, index) => ({ + kind: "hunk" as const, + id: `gap:${index}`, + fileId: "gap", + })); + let changeRange: + | ReturnType["updateVisibleRange"] + | undefined; + function Harness() { + const result = useNativeReviewDiffHighlighting({ + files: [], + rows, + scheme: "dark", + enabled: true, + collapsedFileIds: [], + resetKey: "source", + contentResetKey: "view", + }); + useLayoutEffect(() => { + changeRange = result.updateVisibleRange; + }, [result.updateVisibleRange]); + return null; + } + let renderer: ReactTestRenderer | undefined; + try { + await act(async () => { + renderer = create(createElement(Harness)); + }); + const mountJobCount = wordJobs.length; + // Android reports the viewport one row at a time during a slow drag. + for (let step = 1; step <= 500; step += 1) { + await act(async () => { + changeRange!({ firstRowIndex: step, lastRowIndex: step + 40 }); + }); + } + const requestedStarts = wordJobs.slice(mountJobCount).map((job) => job.input.firstRowIndex); + expect(requestedStarts.length).toBeGreaterThan(0); + expect(requestedStarts.at(-1)).toBeGreaterThanOrEqual(490); + // Each request waits for the viewport to move 20 rows past the previous request. + expect(requestedStarts).toEqual( + Array.from({ length: requestedStarts.length }, (_, index) => 1 + index * 10), + ); + } finally { + await act(async () => renderer?.unmount()); + } + }); + + it("delivers word ranges for review comment cards as a keyed patch", async () => { + setExpoViewConfigAvailable(); + vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); + const frames = new Map void>(); + let frameId = 0; + vi.stubGlobal("requestAnimationFrame", (callback: (time: number) => void) => { + frames.set(++frameId, callback); + return frameId; + }); + vi.stubGlobal("cancelAnimationFrame", (id: number) => frames.delete(id)); + const sent: string[] = []; + const nativeProps: Array> = []; + const nativeHandle = { + setRowsJson: async () => undefined, + setTokensJson: async () => undefined, + setTokensPatchJson: async (json: string) => { + sent.push(json); + }, + }; + const NativeMock = forwardRef>( + function NativeMock(props, ref) { + nativeProps.push(props); + useImperativeHandle(ref, () => nativeHandle, []); + return null; + }, + ); + expoMocks.requireNativeView.mockReturnValue(NativeMock); + const { resolveNativeReviewDiffView } = await import("./nativeReviewDiffSurface"); + const { useNativeReviewCommentWordDiffs } = + await import("../review/useNativeReviewCommentWordDiffs"); + const NativeView = resolveNativeReviewDiffView(); + if (!NativeView) throw new Error("Expected the native payload sender"); + const rows: NativeReviewDiffRow[] = [ + { + kind: "line", + id: "c:snippet:0", + change: "delete", + content: 'const item = renderPanel({ title: "before", active: true });', + }, + { + kind: "line", + id: "c:snippet:1", + change: "add", + content: 'const item = renderPanel({ title: "after", active: true });', + }, + ]; + function Card() { + const { tokensResetKey, wordDiffRangesPatchJson } = useNativeReviewCommentWordDiffs({ + rows, + enabled: true, + }); + return createElement(NativeView!, { + appearanceScheme: "dark", + themeJson: "{}", + rowHeight: 20, + contentWidth: 1000, + rowsJson: "[]", + tokensResetKey, + wordDiffRangesPatchJson, + }); + } + let renderer: ReactTestRenderer | undefined; + try { + await act(async () => { + renderer = create(createElement(Card)); + }); + await act(async () => { + await wordJobs.at(-1)!.complete(); + }); + await act(async () => { + const pending = [...frames.values()]; + frames.clear(); + for (const callback of pending) callback(performance.now()); + await Promise.resolve(); + }); + const patch = sent.map((json) => JSON.parse(json)).find((p) => "wordDiffRangesByRowId" in p); + expect(patch).toBeDefined(); + expect(patch.resetKey).toBe(nativeProps.at(-1)!.tokensResetKey); + expect(Object.keys(patch.wordDiffRangesByRowId).sort()).toEqual([ + "c:snippet:0", + "c:snippet:1", + ]); + expect(patch.wordDiffRangesByRowId["c:snippet:0"]).toEqual([{ start: 35, end: 41 }]); + expect(patch.wordDiffRangesByRowId["c:snippet:1"]).toEqual([{ start: 35, end: 40 }]); + } finally { + await act(async () => renderer?.unmount()); + } + }); }); describe("isPendingNativeViewRegistration", () => { diff --git a/apps/mobile/src/features/diffs/nativeReviewDiffSurface.ts b/apps/mobile/src/features/diffs/nativeReviewDiffSurface.ts index fda734106687..4d3c723ffa00 100644 --- a/apps/mobile/src/features/diffs/nativeReviewDiffSurface.ts +++ b/apps/mobile/src/features/diffs/nativeReviewDiffSurface.ts @@ -110,6 +110,8 @@ export interface NativeReviewDiffViewProps extends ViewProps { readonly rowsJson: string; readonly tokensJson?: string; readonly tokensPatchJson?: string; + readonly wordDiffRangesPatchJson?: string; + readonly onWordDiffRangesPatchSent?: () => void; readonly tokensResetKey?: string; readonly contentResetKey?: string; readonly collapsedFileIdsJson?: string; @@ -163,7 +165,12 @@ interface NativeReviewDiffViewRef { type NativeReviewDiffRawViewProps = Omit< NativeReviewDiffViewProps, - "nativeViewRef" | "rowsJson" | "tokensJson" | "tokensPatchJson" + | "nativeViewRef" + | "rowsJson" + | "tokensJson" + | "tokensPatchJson" + | "wordDiffRangesPatchJson" + | "onWordDiffRangesPatchSent" > & { readonly ref?: Ref; }; @@ -186,6 +193,7 @@ function useNativeReviewDiffPayload( nativeRef: React.RefObject, method: NativeReviewDiffPayloadMethod, payload: string | undefined, + onSent?: () => void, ) { useEffect(() => { if (payload === undefined) { @@ -211,18 +219,23 @@ function useNativeReviewDiffPayload( return; } - void command.call(view, payload).catch((error: unknown) => { - if ( - !cancelled && - attempts < NATIVE_REVIEW_DIFF_PAYLOAD_RETRY_FRAMES && - isPendingNativeViewRegistration(error) - ) { - attempts += 1; - frame = requestAnimationFrame(dispatch); - return; - } - console.error(`[native-review-diff] ${method} failed`, error); - }); + void command + .call(view, payload) + .then(() => { + if (!cancelled) onSent?.(); + }) + .catch((error: unknown) => { + if ( + !cancelled && + attempts < NATIVE_REVIEW_DIFF_PAYLOAD_RETRY_FRAMES && + isPendingNativeViewRegistration(error) + ) { + attempts += 1; + frame = requestAnimationFrame(dispatch); + return; + } + console.error(`[native-review-diff] ${method} failed`, error); + }); }; // Fabric attaches the React ref before Expo registers the native tag used by @@ -235,7 +248,7 @@ function useNativeReviewDiffPayload( cancelAnimationFrame(frame); } }; - }, [method, nativeRef, payload]); + }, [method, nativeRef, onSent, payload]); } function getExpoViewConfig(moduleName: string) { @@ -245,11 +258,25 @@ function getExpoViewConfig(moduleName: string) { } function NativeReviewDiffView(props: NativeReviewDiffViewProps) { - const { nativeViewRef, rowsJson, tokensJson, tokensPatchJson, ...nativeProps } = props; + const { + nativeViewRef, + rowsJson, + tokensJson, + tokensPatchJson, + wordDiffRangesPatchJson, + onWordDiffRangesPatchSent, + ...nativeProps + } = props; const nativeRef = useRef(null); useNativeReviewDiffPayload(nativeRef, "setRowsJson", rowsJson); useNativeReviewDiffPayload(nativeRef, "setTokensJson", tokensJson); useNativeReviewDiffPayload(nativeRef, "setTokensPatchJson", tokensPatchJson); + useNativeReviewDiffPayload( + nativeRef, + "setTokensPatchJson", + wordDiffRangesPatchJson, + onWordDiffRangesPatchSent, + ); useImperativeHandle( nativeViewRef, () => ({ diff --git a/apps/mobile/src/features/review/ReviewCommentCard.tsx b/apps/mobile/src/features/review/ReviewCommentCard.tsx index ff348e1f2a97..ddf2502ccb40 100644 --- a/apps/mobile/src/features/review/ReviewCommentCard.tsx +++ b/apps/mobile/src/features/review/ReviewCommentCard.tsx @@ -17,6 +17,7 @@ import { buildReviewParsedDiff } from "./reviewModel"; import { REVIEW_MONO_FONT_FAMILY } from "./reviewDiffRendering"; import type { ReviewInlineComment } from "./reviewCommentSelection"; import { useMarkdownCodeHighlight } from "../threads/markdownCodeHighlightState"; +import { useNativeReviewCommentWordDiffs } from "./useNativeReviewCommentWordDiffs"; export interface ReviewCommentColors { readonly background: ColorValue; @@ -127,6 +128,10 @@ export const ReviewCommentCard = memo(function ReviewCommentCard(props: { [compactNativeRows.length, nativeReviewDiffStyle], ); const shouldRenderNativeDiff = NativeReviewDiffView != null && compactNativeRows.length > 0; + const { tokensResetKey, wordDiffRangesPatchJson } = useNativeReviewCommentWordDiffs({ + rows: compactNativeRows, + enabled: shouldRenderNativeDiff, + }); return ( diff --git a/apps/mobile/src/features/review/ReviewSheet.tsx b/apps/mobile/src/features/review/ReviewSheet.tsx index cb55570f9116..5d891a669894 100644 --- a/apps/mobile/src/features/review/ReviewSheet.tsx +++ b/apps/mobile/src/features/review/ReviewSheet.tsx @@ -773,7 +773,7 @@ export function ReviewSheet(props: ReviewSheetProps) { appearanceScheme={selectedTheme} collapsedFileIdsJson={nativeBridge.collapsedFileIdsJson} collapsedCommentIdsJson={nativeBridge.collapsedCommentIdsJson} - contentResetKey={`${reviewCache.threadKey}:${selectedSection.id}`} + contentResetKey={nativeBridge.contentResetKey} contentWidth={NATIVE_REVIEW_DIFF_CONTENT_WIDTH} nativeViewRef={nativeReviewDiffViewRef} rowHeight={nativeReviewDiffStyle.rowHeight} @@ -782,6 +782,8 @@ export function ReviewSheet(props: ReviewSheetProps) { styleJson={nativeBridge.styleJson} themeJson={nativeBridge.themeJson} tokensPatchJson={nativeBridge.tokensPatchJson} + wordDiffRangesPatchJson={nativeBridge.wordDiffRangesPatchJson} + onWordDiffRangesPatchSent={nativeBridge.onWordDiffRangesPatchSent} tokensResetKey={nativeBridge.tokensResetKey} viewedFileIdsJson={nativeBridge.viewedFileIdsJson} onDebug={handleNativeDebug} diff --git a/apps/mobile/src/features/review/nativeReviewDiffAdapter.test.ts b/apps/mobile/src/features/review/nativeReviewDiffAdapter.test.ts index 21c86a2beab4..9e9b92391720 100644 --- a/apps/mobile/src/features/review/nativeReviewDiffAdapter.test.ts +++ b/apps/mobile/src/features/review/nativeReviewDiffAdapter.test.ts @@ -18,6 +18,8 @@ import { import type { ReviewInlineComment } from "./reviewCommentSelection"; import { buildReviewParsedDiff } from "./reviewModel"; import * as ReviewWordDiffs from "./reviewWordDiffs"; +import { computeVisibleNativeReviewWordDiffRanges } from "./nativeReviewWordDiffs"; +import type { NativeReviewDiffRow } from "../diffs/nativeReviewDiffSurface"; const parsedDiff = buildReviewParsedDiff( [ @@ -138,13 +140,22 @@ describe("getCachedNativeReviewDiffData", () => { expect(changed.rows.find((row) => row.kind === "comment")?.commentText).toBe("Changed"); }); - it("reuses source rows and word matching when a file comment changes", () => { + it("defers word matching and reuses its results when a file comment changes", async () => { const diff = buildReviewParsedDiff(filesPatch(["example.ts", "second.ts"]), "comment-reuse"); const matchWords = vi.spyOn(ReviewWordDiffs, "computeWordAltDiffRanges"); try { const base = getCachedNativeReviewDiffData({ parsedDiff: diff }); + expect(matchWords).not.toHaveBeenCalled(); + expect(base.rows.filter((row) => row.wordDiffRanges?.length)).toHaveLength(0); + const initialRanges = await computeVisibleNativeReviewWordDiffRanges({ + rows: base.rows, + firstRowIndex: 0, + lastRowIndex: base.rows.length - 1, + }); expect(matchWords).toHaveBeenCalledTimes(4); - expect(base.rows.filter((row) => row.wordDiffRanges?.length)).toHaveLength(8); + expect( + Object.values(initialRanges.rangesByRowId).filter((ranges) => ranges.length), + ).toHaveLength(8); const firstComment = makeComment("First file comment"); const secondComment = { ...makeComment("Second file comment"), @@ -173,6 +184,12 @@ describe("getCachedNativeReviewDiffData", () => { } const removed = getCachedNativeReviewDiffData({ parsedDiff: diff, comments: [] }); expect(removed.rows).toEqual(base.rows); + const changedRanges = await computeVisibleNativeReviewWordDiffRanges({ + rows: changed.rows, + firstRowIndex: 0, + lastRowIndex: changed.rows.length - 1, + }); + expect(changedRanges).toEqual(initialRanges); expect(matchWords).toHaveBeenCalledTimes(4); expect(first.rows.find((row) => row.id === firstComment.id)?.commentText).toBe( "First file comment", @@ -239,6 +256,146 @@ describe("getCachedNativeReviewDiffData", () => { }); }); +describe("visible native word diffs", () => { + it("matches an offscreen counterpart without preparing unrelated pairs", async () => { + const data = buildNativeReviewDiffData( + buildReviewParsedDiff(filesPatch(["example.ts"]), "visible-pair"), + ); + const result = await computeVisibleNativeReviewWordDiffRanges({ + rows: data.rows, + firstRowIndex: 2, + lastRowIndex: 2, + overscanRows: 0, + }); + expect(result.pairCount).toBe(1); + expect(Object.keys(result.rangesByRowId)).toEqual([data.rows[2]!.id, data.rows[4]!.id]); + expect(result.rangesByRowId[data.rows[4]!.id]?.length).toBeGreaterThan(0); + expect( + await computeVisibleNativeReviewWordDiffRanges({ + rows: data.rows, + firstRowIndex: 4, + lastRowIndex: 4, + overscanRows: 0, + alreadyHighlightedRowIds: new Set(Object.keys(result.rangesByRowId)), + }), + ).toEqual({ rangesByRowId: {}, pairCount: 0 }); + }); + + it("does not spend the visible pair budget on collapsed files", async () => { + const data = buildNativeReviewDiffData( + buildReviewParsedDiff(filesPatch(["first.ts", "second.ts"]), "collapsed-pairs"), + ); + const collapsedFileIds = new Set([data.files[0]!.id]); + const result = await computeVisibleNativeReviewWordDiffRanges({ + rows: data.rows, + firstRowIndex: 0, + lastRowIndex: data.rows.length - 1, + collapsedFileIds, + overscanRows: 0, + maxPairs: 1, + }); + expect(result.pairCount).toBe(1); + expect(Object.keys(result.rangesByRowId)).toEqual([data.rows[8]!.id, data.rows[10]!.id]); + collapsedFileIds.clear(); + const reopened = await computeVisibleNativeReviewWordDiffRanges({ + rows: data.rows, + firstRowIndex: 0, + lastRowIndex: data.rows.length - 1, + collapsedFileIds, + overscanRows: 0, + maxPairs: 1, + }); + expect(Object.keys(reopened.rangesByRowId)).toEqual([data.rows[2]!.id, data.rows[4]!.id]); + }); + + it("yields and cancels while indexing rows without replacement pairs", async () => { + const rows: NativeReviewDiffRow[] = Array.from({ length: 2_000 }, (_, index) => ({ + kind: "line", + id: `context-${index}`, + fileId: "context", + change: "context", + content: "unchanged", + })); + let elapsed = 0; + const clock = vi.spyOn(performance, "now").mockImplementation(() => (elapsed += 5)); + const controller = new AbortController(); + try { + const pending = computeVisibleNativeReviewWordDiffRanges({ + rows, + firstRowIndex: 0, + lastRowIndex: rows.length - 1, + signal: controller.signal, + }); + setTimeout(() => controller.abort(), 0); + expect(await pending).toEqual({ rangesByRowId: {}, pairCount: 0 }); + expect(controller.signal.aborted).toBe(true); + } finally { + clock.mockRestore(); + } + }); + + it("discards partial results when cancelled between batches and reuses completed pairs", async () => { + const rows: NativeReviewDiffRow[] = (["delete", "add"] as const).flatMap((change) => + Array.from({ length: 65 }, (_, index) => ({ + kind: "line" as const, + id: `${change}-${index}`, + fileId: "file", + change, + content: `const row${index} = renderPanel({ label: "${change === "delete" ? "before" : "after"}", enabled: true });`, + })), + ); + const controller = new AbortController(); + const matchWords = vi.spyOn(ReviewWordDiffs, "computeWordAltDiffRanges"); + try { + const pending = computeVisibleNativeReviewWordDiffRanges({ + rows, + firstRowIndex: 0, + lastRowIndex: rows.length - 1, + signal: controller.signal, + }); + // Cancellation runs on the next event-loop turn, so word matching must yield to it. + setTimeout(() => controller.abort(), 0); + expect(await pending).toEqual({ rangesByRowId: {}, pairCount: 0 }); + expect(matchWords.mock.calls.length).toBeGreaterThan(0); + expect(matchWords.mock.calls.length).toBeLessThan(65); + const resumed = await computeVisibleNativeReviewWordDiffRanges({ + rows, + firstRowIndex: 0, + lastRowIndex: rows.length - 1, + }); + expect(resumed.pairCount).toBe(65); + expect(Object.keys(resumed.rangesByRowId)).toHaveLength(130); + expect(matchWords).toHaveBeenCalledTimes(65); + } finally { + matchWords.mockRestore(); + } + }); + + it("invalidates a cached pair when its same-ID counterpart changes", async () => { + const data = buildNativeReviewDiffData( + buildReviewParsedDiff(filesPatch(["example.ts"]), "changed-pair"), + ); + const first = await computeVisibleNativeReviewWordDiffRanges({ + rows: data.rows, + firstRowIndex: 2, + lastRowIndex: 2, + overscanRows: 0, + }); + const changedRow = { + ...data.rows[4]!, + content: data.rows[4]!.content!.replace("after", "updated-value"), + }; + const changed = await computeVisibleNativeReviewWordDiffRanges({ + rows: data.rows.map((row) => (row.id === changedRow.id ? changedRow : row)), + firstRowIndex: 2, + lastRowIndex: 2, + overscanRows: 0, + }); + expect(changed.pairCount).toBe(1); + expect(changed.rangesByRowId[changedRow.id]).not.toEqual(first.rangesByRowId[changedRow.id]); + }); +}); + describe("createNativeReviewDiffTheme", () => { it("serializes every native color as cross-platform opaque hex", () => { for (const themeId of MOBILE_THEME_IDS) { diff --git a/apps/mobile/src/features/review/nativeReviewDiffAdapter.ts b/apps/mobile/src/features/review/nativeReviewDiffAdapter.ts index b35d561a249d..e8cd792c0ec6 100644 --- a/apps/mobile/src/features/review/nativeReviewDiffAdapter.ts +++ b/apps/mobile/src/features/review/nativeReviewDiffAdapter.ts @@ -4,11 +4,9 @@ import type { NativeReviewDiffLanguage, } from "../diffs/nativeReviewDiffTypes"; import * as Arr from "effect/Array"; -import { pipe } from "effect/Function"; import type { ResolvedMobileCodeSurface } from "../../lib/appearancePreferences"; import { type MobileThemeId, type MobileThemeVariables } from "../../lib/mobileTheme"; import { getMobileTerminalTheme, type TerminalAppearanceScheme } from "../terminal/terminalTheme"; -import { computeWordAltDiffRanges } from "./reviewWordDiffs"; import { getReviewFilePreviewState, type ReviewParsedDiff, @@ -17,8 +15,6 @@ import { } from "./reviewModel"; import type { ReviewInlineComment } from "./reviewCommentSelection"; -const NATIVE_REVIEW_MAX_WORD_DIFF_RANGE_COUNT = 4; -const NATIVE_REVIEW_MAX_WORD_DIFF_COVERAGE = 0.45; const NATIVE_HEX_COLOR = /^#([\da-f]{2})([\da-f]{2})([\da-f]{2})([\da-f]{2})?$/i; const NATIVE_RGBA_COLOR = /^rgba?\(\s*([\d.]+)\s*,\s*([\d.]+)\s*,\s*([\d.]+)(?:\s*,\s*([\d.]+))?\s*\)$/; @@ -291,117 +287,6 @@ function noticeRowsForFile(file: ReviewRenderableFile): ReadonlyArray, -): NonNullable { - return pipe( - ranges, - Arr.flatMap((range) => { - let start = Math.max(0, range.start); - let end = Math.min(content.length, range.end); - - while (start < end && /\s/.test(content[start] ?? "")) { - start += 1; - } - while (end > start && /\s/.test(content[end - 1] ?? "")) { - end -= 1; - } - - return end > start ? [{ start, end }] : []; - }), - ); -} - -function nonWhitespaceLength(value: string) { - return value.replace(/\s/g, "").length; -} - -function shouldUseWordDiffRanges( - content: string, - ranges: NonNullable, -) { - if (ranges.length === 0 || ranges.length > NATIVE_REVIEW_MAX_WORD_DIFF_RANGE_COUNT) { - return false; - } - - const meaningfulLength = nonWhitespaceLength(content); - if (meaningfulLength === 0) { - return false; - } - - const highlightedLength = ranges.reduce( - (total, range) => total + nonWhitespaceLength(content.slice(range.start, range.end)), - 0, - ); - return highlightedLength / meaningfulLength <= NATIVE_REVIEW_MAX_WORD_DIFF_COVERAGE; -} - -function addNativeWordDiffRanges( - rows: ReadonlyArray, -): ReadonlyArray { - const nextRows = [...rows]; - let index = 0; - - while (index < nextRows.length) { - const deletedRowIndexes: number[] = []; - const addedRowIndexes: number[] = []; - const fileId = nextRows[index]?.fileId; - - while ( - nextRows[index]?.kind === "line" && - nextRows[index]?.change === "delete" && - nextRows[index]?.fileId === fileId - ) { - deletedRowIndexes.push(index); - index += 1; - } - - while ( - nextRows[index]?.kind === "line" && - nextRows[index]?.change === "add" && - nextRows[index]?.fileId === fileId - ) { - addedRowIndexes.push(index); - index += 1; - } - - const pairedCount = Math.min(deletedRowIndexes.length, addedRowIndexes.length); - for (let pairIndex = 0; pairIndex < pairedCount; pairIndex += 1) { - const deletedRowIndex = deletedRowIndexes[pairIndex]; - const addedRowIndex = addedRowIndexes[pairIndex]; - if (deletedRowIndex === undefined || addedRowIndex === undefined) { - continue; - } - const deletedRow = nextRows[deletedRowIndex]; - const addedRow = nextRows[addedRowIndex]; - if (!deletedRow?.content || !addedRow?.content) { - continue; - } - - const ranges = computeWordAltDiffRanges({ - deletionLine: deletedRow.content, - additionLine: addedRow.content, - }); - const deletionRanges = trimWordDiffRanges(deletedRow.content, ranges.deletion); - const additionRanges = trimWordDiffRanges(addedRow.content, ranges.addition); - - if (shouldUseWordDiffRanges(deletedRow.content, deletionRanges)) { - nextRows[deletedRowIndex] = { ...deletedRow, wordDiffRanges: deletionRanges }; - } - if (shouldUseWordDiffRanges(addedRow.content, additionRanges)) { - nextRows[addedRowIndex] = { ...addedRow, wordDiffRanges: additionRanges }; - } - } - - if (deletedRowIndexes.length === 0 && addedRowIndexes.length === 0) { - index += 1; - } - } - - return nextRows; -} - function mapLineRow( file: ReviewRenderableFile, row: ReviewRenderableLineRow, @@ -465,8 +350,7 @@ function prepareFileRows(file: ReviewRenderableFile): PreparedNativeReviewFileRo fileId: file.id, filePath: file.path, lineCount: lineRows.length, - // Comments must not split the source deletion/addition runs used for word matching. - rows: addNativeWordDiffRanges(rows), + rows, commentTargetsByRowId, rowIdByCommentLineId, commentedRows: null, @@ -601,7 +485,7 @@ export function buildNativeReviewDiffData( /** * Prepares source rows once per parsed diff, including its section-specific IDs. - * Comment edits reuse those rows, word ranges, and targets. Each file retains + * Comment edits reuse those rows and targets. Each file retains * only its latest comment overlay, and the weak key releases old parsed diffs. */ export function getCachedNativeReviewDiffData( diff --git a/apps/mobile/src/features/review/nativeReviewWordDiffs.ts b/apps/mobile/src/features/review/nativeReviewWordDiffs.ts new file mode 100644 index 000000000000..893ffd3b70c2 --- /dev/null +++ b/apps/mobile/src/features/review/nativeReviewWordDiffs.ts @@ -0,0 +1,251 @@ +import type { + NativeReviewDiffRow, + NativeReviewDiffWordDiffRange, +} from "../diffs/nativeReviewDiffSurface"; +import { computeWordAltDiffRanges } from "./reviewWordDiffs"; + +const MAX_WORD_DIFF_RANGE_COUNT = 4; +const MAX_WORD_DIFF_COVERAGE = 0.45; +const VISIBLE_OVERSCAN_ROWS = 160; +const VISIBLE_MAX_PAIRS = 360; +const MAX_PAIRS_PER_BATCH = 32; +const MAX_BATCH_MILLISECONDS = 4; +const SCAN_BUDGET_CHECK_INTERVAL = 256; + +interface WordDiffWorkBudget { + startedAt: number; + operations: number; +} + +interface WordDiffPair { + readonly deletion: NativeReviewDiffRow; + readonly addition: NativeReviewDiffRow; +} + +interface WordDiffPairRanges { + readonly deletion: ReadonlyArray; + readonly addition: ReadonlyArray; +} + +const pairsByRows = new WeakMap< + ReadonlyArray, + { + readonly collapsedFileIds: ReadonlySet; + readonly pairs: ReadonlyArray; + } +>(); +const rangesByDeletion = new WeakMap< + NativeReviewDiffRow, + { readonly addition: NativeReviewDiffRow; readonly ranges: WordDiffPairRanges } +>(); + +function trimWordDiffRanges( + content: string, + ranges: ReadonlyArray, +): ReadonlyArray { + return ranges.flatMap((range) => { + let start = Math.max(0, range.start); + let end = Math.min(content.length, range.end); + while (start < end && /\s/.test(content[start] ?? "")) start += 1; + while (end > start && /\s/.test(content[end - 1] ?? "")) end -= 1; + return end > start ? [{ start, end }] : []; + }); +} + +function shouldUseWordDiffRanges( + content: string, + ranges: ReadonlyArray, +): boolean { + if (ranges.length === 0 || ranges.length > MAX_WORD_DIFF_RANGE_COUNT) return false; + const meaningfulLength = content.replace(/\s/g, "").length; + if (meaningfulLength === 0) return false; + const highlightedLength = ranges.reduce( + (total, range) => total + content.slice(range.start, range.end).replace(/\s/g, "").length, + 0, + ); + return highlightedLength / meaningfulLength <= MAX_WORD_DIFF_COVERAGE; +} + +function shouldYieldScan(budget: WordDiffWorkBudget): boolean { + budget.operations += 1; + if (budget.operations < SCAN_BUDGET_CHECK_INTERVAL) return false; + budget.operations = 0; + return performance.now() - budget.startedAt >= MAX_BATCH_MILLISECONDS; +} + +function sameCollapsedFiles( + cached: ReadonlySet, + next: ReadonlySet | undefined, +): boolean { + if (cached.size !== (next?.size ?? 0)) return false; + for (const id of cached) { + if (!next?.has(id)) return false; + } + return true; +} + +async function pauseWordDiffWork(budget: WordDiffWorkBudget): Promise { + await yieldWordDiffWork(); + budget.startedAt = performance.now(); + budget.operations = 0; +} + +/** Index source pairs once. Comments do not change deletion/addition correspondence. */ +async function getWordDiffPairs( + rows: ReadonlyArray, + budget: WordDiffWorkBudget, + signal: AbortSignal | undefined, + collapsedFileIds: ReadonlySet | undefined, +): Promise | null> { + const cached = pairsByRows.get(rows); + if (cached && sameCollapsedFiles(cached.collapsedFileIds, collapsedFileIds)) return cached.pairs; + const pairs: Array = []; + pairs.length = rows.length; + let index = 0; + while (index < rows.length) { + if (shouldYieldScan(budget)) { + await pauseWordDiffWork(budget); + if (signal?.aborted) return null; + } + if ( + rows[index]!.kind !== "line" || + rows[index]!.change !== "delete" || + collapsedFileIds?.has(rows[index]!.fileId ?? "") + ) { + index += 1; + continue; + } + const deletedIndexes: number[] = []; + const addedIndexes: number[] = []; + const fileId = rows[index]!.fileId; + while (index < rows.length) { + if (shouldYieldScan(budget)) { + await pauseWordDiffWork(budget); + if (signal?.aborted) return null; + } + const row = rows[index]!; + if (row.kind === "comment") { + index += 1; + continue; + } + if (row.kind !== "line" || row.change !== "delete" || row.fileId !== fileId) break; + deletedIndexes.push(index); + index += 1; + } + while (index < rows.length) { + if (shouldYieldScan(budget)) { + await pauseWordDiffWork(budget); + if (signal?.aborted) return null; + } + const row = rows[index]!; + if (row.kind === "comment") { + index += 1; + continue; + } + if (row.kind !== "line" || row.change !== "add" || row.fileId !== fileId) break; + addedIndexes.push(index); + index += 1; + } + const pairedCount = Math.min(deletedIndexes.length, addedIndexes.length); + for (let pairIndex = 0; pairIndex < pairedCount; pairIndex += 1) { + if (shouldYieldScan(budget)) { + await pauseWordDiffWork(budget); + if (signal?.aborted) return null; + } + const deletionIndex = deletedIndexes[pairIndex]!; + const additionIndex = addedIndexes[pairIndex]!; + const pair = { deletion: rows[deletionIndex]!, addition: rows[additionIndex]! }; + pairs[deletionIndex] = pair; + pairs[additionIndex] = pair; + } + } + pairsByRows.set(rows, { pairs, collapsedFileIds: new Set(collapsedFileIds) }); + return pairs; +} + +function getWordDiffPairRanges(pair: WordDiffPair): WordDiffPairRanges { + const cached = rangesByDeletion.get(pair.deletion); + if (cached?.addition === pair.addition) return cached.ranges; + const deletionLine = pair.deletion.content ?? ""; + const additionLine = pair.addition.content ?? ""; + const ranges = + deletionLine && additionLine + ? computeWordAltDiffRanges({ deletionLine, additionLine }) + : { deletion: [], addition: [] }; + const deletion = trimWordDiffRanges(deletionLine, ranges.deletion); + const addition = trimWordDiffRanges(additionLine, ranges.addition); + const result = { + deletion: shouldUseWordDiffRanges(deletionLine, deletion) ? deletion : [], + addition: shouldUseWordDiffRanges(additionLine, addition) ? addition : [], + }; + rangesByDeletion.set(pair.deletion, { addition: pair.addition, ranges: result }); + return result; +} + +function yieldWordDiffWork(): Promise { + return new Promise((resolve) => setTimeout(resolve, 0)); +} + +/** Prepare visible pairs without waiting for syntax highlighting or changing source rows. */ +export async function computeVisibleNativeReviewWordDiffRanges(input: { + readonly rows: ReadonlyArray; + readonly firstRowIndex: number; + readonly lastRowIndex: number; + readonly collapsedFileIds?: ReadonlySet; + readonly alreadyHighlightedRowIds?: ReadonlySet; + readonly overscanRows?: number; + readonly maxPairs?: number; + readonly signal?: AbortSignal; +}): Promise<{ + readonly rangesByRowId: Record>; + readonly pairCount: number; +}> { + await yieldWordDiffWork(); + if (input.signal?.aborted) return { rangesByRowId: {}, pairCount: 0 }; + const budget = { startedAt: performance.now(), operations: 0 }; + const pairs = await getWordDiffPairs(input.rows, budget, input.signal, input.collapsedFileIds); + if (pairs === null) return { rangesByRowId: {}, pairCount: 0 }; + const overscanRows = input.overscanRows ?? VISIBLE_OVERSCAN_ROWS; + const start = Math.max(0, Math.floor(input.firstRowIndex - overscanRows)); + const end = Math.min(input.rows.length - 1, Math.ceil(input.lastRowIndex + overscanRows)); + const selectedPairs = new Set(); + const rangesByRowId: Record> = {}; + let batchCount = 0; + for ( + let index = start; + index <= end && selectedPairs.size < (input.maxPairs ?? VISIBLE_MAX_PAIRS); + index += 1 + ) { + if (shouldYieldScan(budget)) { + await pauseWordDiffWork(budget); + if (input.signal?.aborted) return { rangesByRowId: {}, pairCount: 0 }; + batchCount = 0; + } + const pair = pairs[index]; + if ( + !pair || + selectedPairs.has(pair) || + input.collapsedFileIds?.has(pair.deletion.fileId ?? "") || + (input.alreadyHighlightedRowIds?.has(pair.deletion.id) && + input.alreadyHighlightedRowIds.has(pair.addition.id)) + ) { + continue; + } + if ( + batchCount >= MAX_PAIRS_PER_BATCH || + performance.now() - budget.startedAt >= MAX_BATCH_MILLISECONDS + ) { + await pauseWordDiffWork(budget); + if (input.signal?.aborted) return { rangesByRowId: {}, pairCount: 0 }; + batchCount = 0; + } + const ranges = getWordDiffPairRanges(pair); + rangesByRowId[pair.deletion.id] = ranges.deletion; + rangesByRowId[pair.addition.id] = ranges.addition; + selectedPairs.add(pair); + batchCount += 1; + } + return input.signal?.aborted + ? { rangesByRowId: {}, pairCount: 0 } + : { rangesByRowId, pairCount: selectedPairs.size }; +} diff --git a/apps/mobile/src/features/review/useNativeReviewCommentWordDiffs.ts b/apps/mobile/src/features/review/useNativeReviewCommentWordDiffs.ts new file mode 100644 index 000000000000..b4632a4d1714 --- /dev/null +++ b/apps/mobile/src/features/review/useNativeReviewCommentWordDiffs.ts @@ -0,0 +1,61 @@ +import { useEffect, useMemo, useState } from "react"; + +import type { NativeReviewDiffRow } from "../diffs/nativeReviewDiffSurface"; +import { computeVisibleNativeReviewWordDiffRanges } from "./nativeReviewWordDiffs"; + +interface NativeReviewCommentWordDiffPatch { + readonly resetKey: string; + readonly wordDiffRangesByRowId: Awaited< + ReturnType + >["rangesByRowId"]; +} + +// Line numbers stay out of the key: they change the rows JSON without changing +// syntax tokens, and a reset would clear tokens native never gets resent. +function buildCommentRowsResetKey(rows: ReadonlyArray): string { + let hash = 5381; + for (const row of rows) { + const text = `${row.id}\n${row.content ?? ""}\n`; + for (let index = 0; index < text.length; index += 1) { + hash = (hash * 33) ^ text.charCodeAt(index); + } + } + return `${rows.length}:${(hash >>> 0).toString(36)}`; +} + +/** Word highlights for a comment card's rows, delivered as a patch like the review sheet. */ +export function useNativeReviewCommentWordDiffs(input: { + readonly rows: ReadonlyArray; + readonly enabled: boolean; +}) { + const { enabled, rows } = input; + const tokensResetKey = useMemo(() => buildCommentRowsResetKey(rows), [rows]); + const [patch, setPatch] = useState(() => ({ + resetKey: tokensResetKey, + wordDiffRangesByRowId: {}, + })); + const wordDiffRangesPatchJson = useMemo(() => JSON.stringify(patch), [patch]); + + useEffect(() => { + if (!enabled || rows.length === 0) return; + const abortController = new AbortController(); + void computeVisibleNativeReviewWordDiffRanges({ + rows, + firstRowIndex: 0, + lastRowIndex: rows.length - 1, + signal: abortController.signal, + }) + .then((result) => { + if (abortController.signal.aborted || result.pairCount === 0) return; + setPatch({ resetKey: tokensResetKey, wordDiffRangesByRowId: result.rangesByRowId }); + }) + .catch((error: unknown) => { + if (!abortController.signal.aborted && typeof __DEV__ !== "undefined" && __DEV__) { + console.log("[review-comment] word diff failed", { error, resetKey: tokensResetKey }); + } + }); + return () => abortController.abort(); + }, [enabled, rows, tokensResetKey]); + + return { tokensResetKey, wordDiffRangesPatchJson }; +} diff --git a/apps/mobile/src/features/review/useNativeReviewDiffBridge.ts b/apps/mobile/src/features/review/useNativeReviewDiffBridge.ts index 1728da662686..b03c2b7f920f 100644 --- a/apps/mobile/src/features/review/useNativeReviewDiffBridge.ts +++ b/apps/mobile/src/features/review/useNativeReviewDiffBridge.ts @@ -51,6 +51,7 @@ export function useNativeReviewDiffBridge(input: { ); const themeJson = useMemo(() => JSON.stringify(theme), [theme]); const styleJson = useMemo(() => JSON.stringify(nativeReviewDiffStyle), [nativeReviewDiffStyle]); + const contentResetKey = `${threadKey}:${sectionId}`; const tokensResetKey = useMemo( () => buildNativeReviewTokensResetKey({ @@ -63,12 +64,19 @@ export function useNativeReviewDiffBridge(input: { }), [data.files.length, data.rows.length, diff, scheme, sectionId, threadKey], ); - const { tokensPatchJson, updateVisibleRange } = useNativeReviewDiffHighlighting({ + const { + tokensPatchJson, + wordDiffRangesPatchJson, + onWordDiffRangesPatchSent, + updateVisibleRange, + } = useNativeReviewDiffHighlighting({ files: data.files, rows: data.rows, scheme, resetKey: tokensResetKey, + contentResetKey, enabled: canHighlight, + collapsedFileIds, }); const onDebug = useCallback( @@ -120,7 +128,10 @@ export function useNativeReviewDiffBridge(input: { themeJson, styleJson, tokensPatchJson, + wordDiffRangesPatchJson, + onWordDiffRangesPatchSent, tokensResetKey, + contentResetKey, onDebug, onToggleComment, }; diff --git a/apps/mobile/src/features/review/useNativeReviewDiffHighlighting.ts b/apps/mobile/src/features/review/useNativeReviewDiffHighlighting.ts index 35f06c263666..c5928b7b5502 100644 --- a/apps/mobile/src/features/review/useNativeReviewDiffHighlighting.ts +++ b/apps/mobile/src/features/review/useNativeReviewDiffHighlighting.ts @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useRef, useState } from "react"; +import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { highlightNativeReviewDiffVisibleRows, @@ -7,12 +7,22 @@ import { } from "../diffs/nativeReviewDiffHighlighter"; import type { NativeReviewDiffRow } from "../diffs/nativeReviewDiffSurface"; import type { NativeReviewDiffFile } from "../diffs/nativeReviewDiffTypes"; +import { computeVisibleNativeReviewWordDiffRanges } from "./nativeReviewWordDiffs"; interface NativeReviewVisibleRange { readonly firstRowIndex: number; readonly lastRowIndex: number; } +interface NativeReviewWordDiffPatch { + readonly resetKey: string; + readonly wordDiffRangesByRowId: Awaited< + ReturnType + >["rangesByRowId"]; +} + +const INITIAL_VISIBLE_RANGE: NativeReviewVisibleRange = { firstRowIndex: 0, lastRowIndex: 80 }; + function createEmptyTokenPatch(resetKey: string): string { return JSON.stringify({ resetKey, tokensByRowId: {} }); } @@ -39,27 +49,45 @@ export function useNativeReviewDiffHighlighting(input: { readonly rows: ReadonlyArray; readonly scheme: NativeReviewDiffHighlightScheme; readonly resetKey: string; + readonly contentResetKey: string; readonly enabled: boolean; + readonly collapsedFileIds: ReadonlyArray; }) { - const { enabled, files, resetKey, rows, scheme } = input; + const { collapsedFileIds, contentResetKey, enabled, files, resetKey, rows, scheme } = input; const highlightedRowIdsRef = useRef>(new Set()); - const visibleRangeRef = useRef({ - firstRowIndex: 0, - lastRowIndex: 80, - }); + const wordHighlightedRowIdsRef = useRef>(new Set()); + const contentResetKeyRef = useRef(contentResetKey); + const visibleRangeRef = useRef(INITIAL_VISIBLE_RANGE); + // The range the latest highlight request covered. Scroll distance is measured + // from here, not from the previous viewport event, so a slow scroll still adds up. + const requestedRangeRef = useRef(INITIAL_VISIBLE_RANGE); const visibleChunkIndexRef = useRef(0); const [tokensPatchJson, setTokensPatchJson] = useState(() => createEmptyTokenPatch(resetKey)); + const [wordDiffRangesPatch, setWordDiffRangesPatch] = useState(() => ({ + resetKey, + wordDiffRangesByRowId: {}, + })); + const wordDiffRangesPatchJson = useMemo( + () => JSON.stringify(wordDiffRangesPatch), + [wordDiffRangesPatch], + ); const [visibleHighlightRequest, setVisibleHighlightRequest] = useState(0); useEffect(() => { highlightedRowIdsRef.current = new Set(); + wordHighlightedRowIdsRef.current = new Set(); visibleChunkIndexRef.current = 0; - visibleRangeRef.current = { firstRowIndex: 0, lastRowIndex: 80 }; + if (contentResetKeyRef.current !== contentResetKey) { + contentResetKeyRef.current = contentResetKey; + visibleRangeRef.current = INITIAL_VISIBLE_RANGE; + } setTokensPatchJson(createEmptyTokenPatch(resetKey)); + setWordDiffRangesPatch({ resetKey, wordDiffRangesByRowId: {} }); if (enabled && rows.length > 0) { + requestedRangeRef.current = visibleRangeRef.current; setVisibleHighlightRequest((request) => request + 1); } - }, [enabled, resetKey, rows.length]); + }, [contentResetKey, enabled, resetKey, rows.length]); useEffect(() => { if (!enabled || rows.length === 0) { @@ -121,20 +149,56 @@ export function useNativeReviewDiffHighlighting(input: { return () => abortController.abort(); }, [enabled, files, resetKey, rows, scheme, visibleHighlightRequest]); + // Word ranges do not depend on the syntax engine. A syntax failure must not hide them. + useEffect(() => { + if (!enabled || rows.length === 0) return; + const abortController = new AbortController(); + const requestRange = visibleRangeRef.current; + void computeVisibleNativeReviewWordDiffRanges({ + rows, + firstRowIndex: requestRange.firstRowIndex, + lastRowIndex: requestRange.lastRowIndex, + collapsedFileIds: new Set(collapsedFileIds), + alreadyHighlightedRowIds: wordHighlightedRowIdsRef.current, + signal: abortController.signal, + }) + .then((result) => { + if (abortController.signal.aborted || result.pairCount === 0) return; + setWordDiffRangesPatch({ resetKey, wordDiffRangesByRowId: result.rangesByRowId }); + }) + .catch((error: unknown) => { + if (!abortController.signal.aborted) { + logReviewDiffDiagnostic("native visible word diff failed", { error, resetKey }); + } + }); + return () => abortController.abort(); + }, [collapsedFileIds, enabled, resetKey, rows, visibleHighlightRequest]); + + // A newer render can replace a patch before its native dispatch frame runs. + const onWordDiffRangesPatchSent = useCallback(() => { + if (wordDiffRangesPatch.resetKey !== resetKey) return; + for (const rowId of Object.keys(wordDiffRangesPatch.wordDiffRangesByRowId)) { + wordHighlightedRowIdsRef.current.add(rowId); + } + }, [resetKey, wordDiffRangesPatch]); + const updateVisibleRange = useCallback((nextRange: NativeReviewVisibleRange) => { - const previousRange = visibleRangeRef.current; + const requestedRange = requestedRangeRef.current; const movedRows = - Math.abs(nextRange.firstRowIndex - previousRange.firstRowIndex) + - Math.abs(nextRange.lastRowIndex - previousRange.lastRowIndex); + Math.abs(nextRange.firstRowIndex - requestedRange.firstRowIndex) + + Math.abs(nextRange.lastRowIndex - requestedRange.lastRowIndex); visibleRangeRef.current = nextRange; if (movedRows >= 20) { + requestedRangeRef.current = nextRange; setVisibleHighlightRequest((request) => request + 1); } }, []); return { tokensPatchJson, + wordDiffRangesPatchJson, + onWordDiffRangesPatchSent, updateVisibleRange, }; } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index a0daed3ff962..2e8ced3556ae 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -492,9 +492,15 @@ importers: '@types/react-dom': specifier: ~19.2.3 version: 19.2.3(@types/react@19.2.16) + '@types/react-test-renderer': + specifier: 19.1.0 + version: 19.1.0 babel-preset-expo: specifier: ~57.0.9 version: 57.0.9(@babel/core@7.29.7)(@babel/runtime@7.29.7)(expo-widgets@57.0.15)(expo@57.0.18)(react-refresh@0.14.2) + react-test-renderer: + specifier: 19.2.3 + version: 19.2.3(react@19.2.3) tailwindcss: specifier: 4.3.3 version: 4.3.3 @@ -9473,6 +9479,11 @@ packages: '@types/react': optional: true + react-test-renderer@19.2.3: + resolution: {integrity: sha512-TMR1LnSFiWZMJkCgNf5ATSvAheTT2NvKIwiVwdBPHxjBI7n/JbWd4gaZ16DVd9foAXdvDz+sB5yxZTwMjPRxpw==} + peerDependencies: + react: ^19.2.3 + react-test-renderer@19.2.6: resolution: {integrity: sha512-GbS6V23YduFTPiWJ5xICbKEjRcqx1Z90js/V5miqhz7qp/d6xSe9Dd6NjSQODFRdzdsqRMPW82E/sFpPRbY5Mw==} peerDependencies: @@ -20578,6 +20589,12 @@ snapshots: optionalDependencies: '@types/react': 19.2.16 + react-test-renderer@19.2.3(react@19.2.3): + dependencies: + react: 19.2.3 + react-is: 19.2.7 + scheduler: 0.27.0 + react-test-renderer@19.2.6(react@19.2.6): dependencies: react: 19.2.6