From d0087d82766570a94a507d4dd8b8902246ad2c53 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 26 Oct 2026 23:36:07 +1100 Subject: [PATCH] perf(mobile): avoid code row scans while dragging file paths --- .../modules/t3reviewdiff/T3ReviewDiffView.kt | 3 +- .../t3-review-diff/ios/T3ReviewDiffView.swift | 4 +- .../t3reviewdiff/ReviewDiffGeometryTest.kt | 153 ++++++++++++++++++ .../t3-review-diff/tests/ios/geometry.swift | 95 +++++++++++ .../modules/t3-review-diff/tests/run-ios.sh | 10 ++ 5 files changed, 262 insertions(+), 3 deletions(-) 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 8426ddb565a40..404987da24655 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 @@ -1122,7 +1122,8 @@ private class DiffCanvasView(context: Context) : View(context) { } private fun maxHeaderPathOffset(fileId: String): Int { - val row = rows.firstOrNull { it.kind == "file" && it.resolvedFileId == fileId } ?: return 0 + val rowIndex = fileHeaderIndices.firstOrNull { rows[it].resolvedFileId == fileId } ?: return 0 + val row = rows[rowIndex] val top = 0 val bottom = rowHeight(row) val iconRect = drawing.fileHeaderIconRect(top, bottom, style) diff --git a/apps/mobile/modules/t3-review-diff/ios/T3ReviewDiffView.swift b/apps/mobile/modules/t3-review-diff/ios/T3ReviewDiffView.swift index 8d8a84c406df3..661a18982cd54 100644 --- a/apps/mobile/modules/t3-review-diff/ios/T3ReviewDiffView.swift +++ b/apps/mobile/modules/t3-review-diff/ios/T3ReviewDiffView.swift @@ -1909,8 +1909,8 @@ private final class ReviewDiffContentView: UIView, UIGestureRecognizerDelegate { private func maxHorizontalOffset(for target: (fileId: String, kind: ReviewDiffHorizontalPanKind)) -> CGFloat { if target.kind == .fileHeaderPath, - let row = rows.first(where: { resolvedFileId(for: $0) == target.fileId && $0.kind == "file" }) { - return maxHeaderPathOffset(for: row) + let rowIndex = fileHeaderRowIndex(forFileId: target.fileId) { + return maxHeaderPathOffset(for: rows[rowIndex]) } if codeWrapColumns != nil { return 0 diff --git a/apps/mobile/modules/t3-review-diff/tests/android/src/test/kotlin/expo/modules/t3reviewdiff/ReviewDiffGeometryTest.kt b/apps/mobile/modules/t3-review-diff/tests/android/src/test/kotlin/expo/modules/t3reviewdiff/ReviewDiffGeometryTest.kt index d6b4c7456392e..beac68f8e5357 100644 --- a/apps/mobile/modules/t3-review-diff/tests/android/src/test/kotlin/expo/modules/t3reviewdiff/ReviewDiffGeometryTest.kt +++ b/apps/mobile/modules/t3-review-diff/tests/android/src/test/kotlin/expo/modules/t3reviewdiff/ReviewDiffGeometryTest.kt @@ -219,6 +219,157 @@ class ReviewDiffGeometryTest { } } + @Test + fun headerPathLimitsKeepFirstMatchAndNeverReadCodeRows() { + val parent = T3ReviewDiffView(RuntimeEnvironment.getApplication(), AppContext()) + val reference = T3ReviewDiffView(RuntimeEnvironment.getApplication(), AppContext()) + val canvas = parent.getChildAt(0) + val oracle = reference.getChildAt(0) + val spec = View.MeasureSpec.makeMeasureSpec(390, View.MeasureSpec.EXACTLY) + for (view in listOf(canvas, oracle)) { + view.measure(spec, spec) + view.layout(0, 0, 390, 600) + } + val longPath = "long/directory/".repeat(12) + val ordinary = headerRows(longPath) + val input = listOf(row("impostor", "value").copy(fileId = "f9")) + ordinary + listOf( + row("duplicate:header", "").copy(kind = "file", fileId = "f9", filePath = "short.kt"), + row("id-fallback", "").copy(kind = "file", fileId = "", filePath = longPath + "fallback.kt"), + row("", "").copy(kind = "file", fileId = "", filePath = longPath + "empty.kt"), + row("orphan:line:0", "code ".repeat(100)).copy(fileId = "orphan"), + ) + try { + // Drive the production collapse path from its full decoded payload boundary. + parent.javaClass.getDeclaredField("rows").apply { isAccessible = true }.set(parent, input) + call(parent, "rebuildVisibleRows") + for (wrap in listOf(false, true)) { + parent.setStyleJson("{\"wordWrap\":$wrap}") + reference.setStyleJson("{\"wordWrap\":$wrap}") + for (collapsed in listOf("[]", "[\"f0\",\"f5\"]", "[]")) { + parent.setCollapsedFileIdsJson(collapsed) + checkHeaderLimits(canvas, oracle, input, wrap, collapsed) + } + } + parent.setStyleJson("{\"wordWrap\":false}") + val counted = CountingRows(ordinary) + call(canvas, "setRows", counted) + val target = panTarget(canvas, "f9", "FILE_HEADER_PATH") + call(canvas, "resetHorizontalOffsets") + counted.reads = 0 + counted.codeReads = 0 + repeat(120) { call(canvas, "scrollByHorizontal", 1, target) } + val panReads = counted.reads + val panCodeReads = counted.codeReads + counted.reads = 0 + counted.codeReads = 0 + repeat(120) { call(canvas, "clampHeaderPathOffsets") } + val clampReads = counted.reads + val clampCodeReads = counted.codeReads + println( + "HEADER_LOOKUP Android updates=120 panReads=$panReads panCodeReads=$panCodeReads " + + "clampReads=$clampReads clampCodeReads=$clampCodeReads" + ) + call( + canvas, + "setRows", + listOf(row("replacement", "").copy(kind = "file", fileId = "f9", filePath = "short.kt")) + ) + assertEquals(0, headerLimit(canvas, "f9")) + assertEquals(0, headerOffset(canvas, "f9")) + parent.setContentResetKey("new-content") + assertEquals(0, headerLimit(canvas, "f9")) + assertTrue((field(canvas, "fileHeaderIndices") as IntArray).isEmpty()) + assertTrue((field(canvas, "headerPathOffsetsByFileId") as Map<*, *>).isEmpty()) + println( + "Passed Android header limits: first/middle/last, duplicate, empty/missing IDs, rename, " + + "collapse/reexpand, replacement/reset, wrap and code bounds" + ) + assertEquals("Header pans must not inspect code rows", 0, panCodeReads) + assertEquals("Header clamps must not inspect code rows", 0, clampCodeReads) + assertTrue("Header pans must read only preceding headers", panReads <= 1320) + assertTrue("Header clamps must read only preceding headers", clampReads <= 1320) + } finally { + parent.cleanup() + reference.cleanup() + } + } + + private fun headerRows(longPath: String): List = (0 until 10).flatMap { file -> + listOf( + row("f$file:header", "").copy( + kind = "file", + fileId = "f$file", + filePath = "${longPath}new-$file.kt", + previousPath = "old-$file.kt", + additions = file + 1, + deletions = file * 2, + ) + ) + + List(100) { line -> row("f$file:line:$line", "code ".repeat(100)).copy(fileId = "f$file") } + } + + private fun checkHeaderLimits( + canvas: View, + oracle: View, + input: List, + wrap: Boolean, + collapsed: String + ) { + for ((caseIndex, id) in listOf( + "f0", + "f5", + "f9", + "id-fallback", + "", + "orphan", + "missing" + ).withIndex()) { + val header = input.firstOrNull { it.kind == "file" && it.resolvedFileId == id } + call(oracle, "setRows", listOfNotNull(header)) + val expected = headerLimit(oracle, id) + val actual = headerLimit(canvas, id) + println("HEADER_LIMIT Android wrap=$wrap collapsed=$collapsed case=$caseIndex limit=$actual") + assertEquals("wrap=$wrap collapsed=$collapsed id=$id", expected, actual) + if (header != null) assertTrue("Long rename/header path must overflow", expected > 0) + val target = panTarget(canvas, id, "FILE_HEADER_PATH") + assertEquals(expected, call(canvas, "maxHorizontalOffset", target)) + call(canvas, "scrollByHorizontal", 1_000_000, target) + assertEquals(expected, headerOffset(canvas, id)) + call(canvas, "scrollByHorizontal", -1_000_000, target) + assertEquals(0, headerOffset(canvas, id)) + call(canvas, "scrollByHorizontal", 1, target) + call(canvas, "clampHeaderPathOffsets") + assertEquals(minOf(1, expected), headerOffset(canvas, id)) + } + val codeTarget = panTarget(canvas, "f9", "CODE") + val codeLimit = call(canvas, "maxHorizontalOffset") as Int + assertEquals(codeLimit, call(canvas, "maxHorizontalOffset", codeTarget)) + if (wrap) assertEquals(0, codeLimit) else assertTrue(codeLimit > 0) + call(canvas, "scrollByHorizontal", 1_000_000, codeTarget) + assertEquals(codeLimit, call(canvas, "horizontalOffset")) + call(canvas, "scrollByHorizontal", -1_000_000, codeTarget) + assertEquals(0, call(canvas, "horizontalOffset")) + } + + private fun headerLimit(canvas: View, id: String): Int = canvas.javaClass + .getDeclaredMethod("maxHeaderPathOffset", String::class.java).apply { isAccessible = true } + .invoke(canvas, id) as Int + + private fun headerOffset(canvas: View, id: String): Int = + (field(canvas, "headerPathOffsetsByFileId") as Map<*, *>)[id] as Int? ?: 0 + + private fun panTarget(canvas: View, id: String, kind: String): Any { + val targetClass = canvas.javaClass.declaredMethods.single { + it.name == "scrollByHorizontal" + }.parameterTypes[1] + val constructor = targetClass.declaredConstructors.single().apply { isAccessible = true } + val enumValue = constructor.parameterTypes[1].enumConstants.single { + (it as Enum<*>).name == + kind + } + return constructor.newInstance(id, enumValue) + } + private fun call(target: Any, name: String, vararg args: Any): Any? = target.javaClass .declaredMethods.single { it.name == name && it.parameterCount == args.size } .apply { isAccessible = true }.invoke(target, *args) @@ -235,9 +386,11 @@ class ReviewDiffGeometryTest { private class CountingRows(private val rows: List) : AbstractList() { var reads = 0 + var codeReads = 0 override val size: Int get() = rows.size override fun get(index: Int): DiffRow { reads += 1 + if (rows[index].kind != "file") codeReads += 1 return rows[index] } } diff --git a/apps/mobile/modules/t3-review-diff/tests/ios/geometry.swift b/apps/mobile/modules/t3-review-diff/tests/ios/geometry.swift index bd213f694f0c7..ae40bfb17ef40 100644 --- a/apps/mobile/modules/t3-review-diff/tests/ios/geometry.swift +++ b/apps/mobile/modules/t3-review-diff/tests/ios/geometry.swift @@ -103,6 +103,101 @@ geometryView.mergeTokensByRowId(["a:line:29": [ReviewDiffNativeToken(content: ge geometryView.selectedRowIds = ["a:line:29"] check(geometryView.contentHeight == originalHeight, "Tokens and selection must preserve geometry") +// The runner instruments the production resolver in its temporary compile input only. +private enum HeaderLookupReads { + static var enabled = false + static var headers = 0 + static var code = 0 + static func record(_ row: ReviewDiffNativeRow) { + guard enabled else { return } + if row.kind == "file" { headers += 1 } else { code += 1 } + } +} + +private extension ReviewDiffContentView { + func checkHeaderPathLimits() throws { + viewportWidth = 390 + let longPath = String(repeating: "long/directory/", count: 12) + func decode(_ input: [[String: Any]]) throws -> [ReviewDiffNativeRow] { + try JSONDecoder().decode([ReviewDiffNativeRow].self, from: JSONSerialization.data(withJSONObject: input)) + } + var fixture: [[String: Any]] = [] + for file in 0..<10 { + fixture.append(["kind": "file", "id": "f\(file):header", "fileId": "f\(file)", + "filePath": longPath + "new-\(file).swift", "previousPath": "old-\(file).swift", + "additions": file + 1, "deletions": file * 2]) + for line in 0..<100 { + fixture.append(["kind": "line", "id": "f\(file):line:\(line)", "fileId": "f\(file)", + "content": String(repeating: "code ", count: 100)]) + } + } + // A code row sharing the target ID must not hide the first header. + fixture.insert(["kind": "line", "id": "impostor", "fileId": "f9", "content": "value"], at: 0) + fixture += [ + ["kind": "file", "id": "duplicate:header", "fileId": "f9", "filePath": "short.swift"], + ["kind": "file", "id": "empty:header", "fileId": "", "filePath": longPath + "empty.swift"], + ["kind": "file", "id": "derived:header", "filePath": longPath + "derived.swift"], + ["kind": "file", "id": "path-fallback", "filePath": longPath + "fallback.swift"], + ["kind": "file", "id": "id-only"], + ["kind": "line", "id": "orphan:line:0", "fileId": "orphan", "content": String(repeating: "code ", count: 100)] + ] + let input = try decode(fixture) + rows = input + for wrap in [false, true] { + style = try geometryStyle(wrap: wrap) + func verify(_ label: String) { + for (caseIndex, id) in ["f0", "f5", "f9", "", "derived", longPath + "fallback.swift", "id-only", "orphan", "missing"].enumerated() { + let header = rows.first { $0.kind == "file" && $0.resolvedFileId == id } + let expected = header.map { maxHeaderPathOffset(for: $0) } + ?? maxHorizontalOffset(for: (fileId: id, kind: .code)) + let actual = maxHorizontalOffset(for: (fileId: id, kind: .fileHeaderPath)) + print("HEADER_LIMIT iOS wrap=\(wrap) state=\(label) case=\(caseIndex) limit=\(actual)") + check(actual == expected, "\(label) wrap=\(wrap) id=\(id): header path limit changed") + if ["f0", "f5", "f9", "", "derived"].contains(id) { + check(actual > 0, "Long rename/header path must overflow in either wrap mode") + } + // This is the same clamp called after token, style, and viewport changes. + setHorizontalOffset(.greatestFiniteMagnitude, for: id, kind: .fileHeaderPath) + clampHorizontalOffsets() + check(horizontalOffset(for: id, kind: .fileHeaderPath) == expected, "Header clamp changed") + setHorizontalOffset(13, for: id, kind: .code) + clampHorizontalOffsets() + check(horizontalOffset(for: id, kind: .code) == min(13, maxHorizontalOffset(for: (id, .code))), + "Code bounds changed") + } + } + verify("expanded") + collapsedFileIds = ["f0", "f5"] + verify("collapsed") + collapsedFileIds = [] + verify("reexpanded") + } + // Known fixture: ten headers separated by 100 code rows; repeatedly clamp the last. + rows = try decode(Array(fixture[1..<1011])) + style = try geometryStyle(wrap: false) + headerPathOffsetsByFileId = ["f9": 1] + horizontalOffsetsByFileId = [:] + HeaderLookupReads.headers = 0 + HeaderLookupReads.code = 0 + HeaderLookupReads.enabled = true + for _ in 0..<120 { clampHorizontalOffsets() } + HeaderLookupReads.enabled = false + print("HEADER_LOOKUP iOS updates=120 headerVisits=\(HeaderLookupReads.headers) codeVisits=\(HeaderLookupReads.code)") + // Finish semantic checks before rejecting baseline scan work. + rows = try decode([["kind": "file", "id": "replacement:header", "fileId": "f9", "filePath": "short.swift"]]) + check(headerPathOffsetsByFileId.isEmpty && horizontalOffsetsByFileId.isEmpty, "Replacement clears offsets") + check(maxHorizontalOffset(for: ("f9", .fileHeaderPath)) == 0, "Replacement must use current header") + rows = [] + check(fileHeaderRowIndices.isEmpty && headerPathOffsetsByFileId.isEmpty, "Reset must clear old index/offsets") + check(maxHorizontalOffset(for: ("f9", .fileHeaderPath)) == maxHorizontalOffset(for: ("f9", .code)), + "Missing-header fallback after reset changed") + print("Passed iOS header limits: first/middle/last, duplicate, empty/absent IDs, rename, collapse/reexpand, replacement/reset, wrap and code bounds") + check(HeaderLookupReads.code == 0, "Header clamps must not inspect code rows") + check(HeaderLookupReads.headers == 1200, "Header clamps must visit only preceding headers") + } +} +try ReviewDiffContentView(frame: CGRect(x: 0, y: 0, width: 390, height: 600)).checkHeaderPathLimits() + // Configuration changes before publication must restart preparation from a fresh snapshot. private let geometryQueue = DispatchQueue(label: "review-diff.geometry-tests") geometryQueue.suspend() diff --git a/apps/mobile/modules/t3-review-diff/tests/run-ios.sh b/apps/mobile/modules/t3-review-diff/tests/run-ios.sh index a8ecdd88947d5..82d5c6a3d6cdd 100644 --- a/apps/mobile/modules/t3-review-diff/tests/run-ios.sh +++ b/apps/mobile/modules/t3-review-diff/tests/run-ios.sh @@ -12,6 +12,16 @@ from pathlib import Path module = Path(sys.argv[1]) source = (module / "ios/T3ReviewDiffView.swift").read_text() source = source.replace("import ExpoModulesCore\n", "") +# Count the actual lookup's row visits only in the compiled test copy. +needle = """ private func resolvedFileId(for row: ReviewDiffNativeRow) -> String { + row.resolvedFileId + }""" +if source.count(needle) != 1: + raise RuntimeError("Header lookup instrumentation seam changed") +source = source.replace(needle, """ private func resolvedFileId(for row: ReviewDiffNativeRow) -> String { + HeaderLookupReads.record(row) + return row.resolvedFileId + }""") stubs = """ public final class AppContext {} open class ExpoView: UIView {