Skip to content

perf(mobile): prepare word diffs for visible rows - #9699

Open
t3dotgg wants to merge 4 commits into
mainfrom
t3code/perf-mobile-visible-word-diffs
Open

t3dotgg wants to merge 4 commits into
mainfrom
t3code/perf-mobile-visible-word-diffs

Conversation

@t3dotgg

@t3dotgg t3dotgg commented Sep 4, 2026 •

Copy link
Copy Markdown
Member

Native release hold

Hold this PR for a compatible native release. It changes the iOS and Android review module.

Mobile prepared word highlights for every changed line before showing a large diff. Prepare source rows first, then compute word ranges for visible replacement pairs. Indexing, skipped rows, and word matching yield during long requests. Keep word work independent of syntax highlighting and send ranges through the existing native setter. Android reports source row indexes after file collapse.

Mark rows complete only after their patch is sent to native. Measure scroll distance from the last requested range so slow scrolls keep requesting ranges, and deliver comment-card word ranges through the same patch path. Retain the last visible range when highlights reset in the same thread and section.

Validation:

  • 49 focused tests, mobile typecheck, and targeted lint passed. Headless React tests cover replaced dispatch frames and scrolled resets.
  • 130 source cases preserve text and coordinates. After visiting all rows, word ranges match the eager implementation.
  • On the original 25-file audit fixture, initial row preparation fell from 294.75 ms to 25.81 ms. The first visible word request took 17.64 ms including yields. Initial row JSON fell from 9.46 MB to 7.33 MB.
  • On 300,008-row fixtures, maximum synchronous work fell from 29.43 ms to 4.24 ms for mostly collapsed rows and from 85.53 ms to 4.05 ms for non-pair rows. Yielding increased the collapsed fixture's total elapsed time.

Measurements used Node 24.20.0. The original fixture used two warmups and six samples. The larger scan checks used one sample each. They exclude native decode and drawing. No local native build, browser, or device testing. Native release and device verification remain required. Existing line-length and coverage rules stay intact.

Refs #9661.

Created with GPT-6 Astra (preview) in Codex.


Note

Medium Risk
Touches iOS/Android review rendering and async patch ordering; incorrect stale-patch handling could show wrong or missing word highlights, but no auth or data-path changes.

Overview
Defers word-level diff highlighting so large reviews no longer compute every deletion/addition pair up front. Row preparation drops embedded wordDiffRanges; a new computeVisibleNativeReviewWordDiffRanges path indexes pairs once (with yield/cancel budgets), matches only the visible window (plus overscan), respects collapsed files, and caches completed pairs.

Streams ranges to native separately from syntax tokens. useNativeReviewDiffHighlighting runs word work in its own effect, sends wordDiffRangesByRowId through the existing token patch bridge (wordDiffRangesPatchJson / setTokensPatchJson), and marks rows complete only after native dispatch via onWordDiffRangesPatchSent. Highlight resets clear native word state but keep the last scroll range within the same thread/section (contentResetKey vs tokensResetKey).

iOS and Android review modules merge patched ranges into drawing state, fall back to row JSON ranges, and report source row indexes in visible-range debug events so JS range requests stay aligned when files are collapsed. Tests add react-test-renderer coverage for delayed patches and scroll/reset behavior.

Reviewed by Cursor Bugbot for commit 166f5fa. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Compute word diffs asynchronously for visible rows only in mobile review diff

  • Moves word-diff pairing and range computation out of synchronous row preparation into a new async module nativeReviewWordDiffs.ts that processes only visible rows plus a bounded overscan, yields to the event loop, and cancels obsolete work
  • Sends computed word ranges to native views (iOS and Android) as row-ID keyed patches through the existing setTokensPatchJson bridge command, with a delivery acknowledgement callback that excludes already-delivered row IDs from future requests
  • Native drawing on both platforms now prefers cached patched ranges over embedded row ranges, falls back to the original embedded ranges when no patch exists, and clears word-range state on token or content resets
  • Adds sourceIndex to parsed DiffRow models so visible-range debug events report original payload row indexes rather than laid-out indexes
  • Risk: prepareFileRows in nativeReviewDiffAdapter.ts no longer mutates rows with computed word ranges; any out-of-tree caller relying on DiffRow containing pre-computed word ranges at construction time will not find them
📊 Macroscope summarized 0c9015e. 11 files reviewed, 1 issue evaluated, 0 issues filtered, 1 comment posted

🗂️ Filtered Issues

Summary by CodeRabbit

  • New Features

    • Added word-level highlighting for review diffs, including review comment cards.
    • Highlights are calculated progressively for visible content to support smoother scrolling and large diffs.
    • Highlight updates are delivered incrementally as additional rows become visible.
  • Bug Fixes

    • Preserved word highlights across delayed updates, resets, and content changes.
    • Corrected visible-range reporting when filtered or collapsed rows are present.
    • Avoided unnecessary token resets when only line numbers change.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). 📱 Native Change Changes the native fingerprint; merging blocks production OTAs until a new store build ships. labels Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 13.5 KiB 13.5 KiB +25 B (+0.2%) 15.1 KiB ✅
Codex Thread snapshot wire 7.1 KiB 7.1 KiB +2 B (+0.0%) 7.3 KiB ✅
Codex Live turn WebSocket wire 6.4 KiB 6.5 KiB +23 B (+0.3%) 7.8 KiB ✅
Codex Live turn WebSocket decoded 56.2 KiB 56.3 KiB +44 B (+0.1%) 66.4 KiB ✅
Codex Live turn messages 9 10 +1 (+11.1%) 21 ✅
Claude Total thread wire 13.5 KiB 13.5 KiB −2 B (−0.0%) 15.1 KiB ✅
Claude Thread snapshot wire 7.1 KiB 7.1 KiB +4 B (+0.1%) 7.3 KiB ✅
Claude Live turn WebSocket wire 6.4 KiB 6.4 KiB −6 B (−0.1%) 7.8 KiB ✅
Claude Live turn WebSocket decoded 57.0 KiB 57.0 KiB 0 B (0.0%) 66.4 KiB ✅
Claude Live turn messages 9 9 0 (0.0%) 21 ✅

Baseline: e74c668 · PR result: 55afb76 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 113.9 KiB
  • Claude decoded thread snapshot: 114.6 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 0184829. Configure here.

Comment thread apps/mobile/src/features/review/useNativeReviewDiffHighlighting.ts
@macroscopeapp

macroscopeapp Bot commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This XL PR substantially rewires the production mobile diff-rendering pipeline across JavaScript, iOS, and Android, including asynchronous patch ordering, caching, resets, and native release compatibility. An unresolved Medium-severity finding also identifies a concrete path to stale word-diff highlights.

Not approved because:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

@t3dotgg t3dotgg mentioned this pull request Sep 4, 2026
27 of 66 tasks
@github-actions github-actions Bot added size:XL 500-999 changed lines (additions + deletions). and removed size:L 100-499 changed lines (additions + deletions). labels Sep 4, 2026
function buildCommentRowsResetKey(rows: ReadonlyArray<NativeReviewDiffRow>): string {
let hash = 5381;
for (const row of rows) {
const text = `${row.id}\n${row.content ?? ""}\n`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium review/useNativeReviewCommentWordDiffs.ts:18

buildCommentRowsResetKey returns the same key when rows retain their IDs and text but change kind or change, so a delete/add pair becoming context rows keeps the old native word-diff highlights. computeVisibleNativeReviewWordDiffRanges then returns pairCount === 0, and line 49 leaves the stale patch intact because no reset is triggered. Include kind and change in the reset-key input so native ranges are cleared when pairing semantics change.

Suggested change
const text = `${row.id}\n${row.content ?? ""}\n`;
const text = `${row.id}
${row.kind}
${row.change}
${row.content ?? ""}
`;
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/review/useNativeReviewCommentWordDiffs.ts around line 18:

`buildCommentRowsResetKey` returns the same key when rows retain their IDs and text but change `kind` or `change`, so a delete/add pair becoming context rows keeps the old native word-diff highlights. `computeVisibleNativeReviewWordDiffRanges` then returns `pairCount === 0`, and line 49 leaves the stale patch intact because no reset is triggered. Include `kind` and `change` in the reset-key input so native ranges are cleared when pairing semantics change.

@coderabbitai

coderabbitai Bot commented Sep 12, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 45a140d3-298a-4c37-bf1d-5dfa821c14ef

📥 Commits

Reviewing files that changed from the base of the PR and between 0c9015e and 55afb76.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (4)
  • apps/mobile/package.json
  • apps/mobile/src/features/review/ReviewSheet.tsx
  • apps/mobile/src/features/review/nativeReviewDiffAdapter.test.ts
  • apps/mobile/src/features/review/nativeReviewDiffAdapter.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The PR moves native word-diff computation to visible-range processing. React hooks send row-keyed patches to Android and iOS views, which cache and render the ranges. Visible-range reporting and tests are updated.

Changes

Native word-diff highlighting

Layer / File(s) Summary
Visible word-diff computation
apps/mobile/src/features/review/nativeReviewWordDiffs.ts, apps/mobile/src/features/review/nativeReviewDiffAdapter.ts, apps/mobile/src/features/review/nativeReviewDiffAdapter.test.ts
Word-diff calculation uses visible ranges, caching, batching, cancellation, collapsed-file handling, and range limits.
Highlighting hook orchestration
apps/mobile/src/features/review/useNativeReviewDiffHighlighting.ts, apps/mobile/src/features/review/useNativeReviewDiffBridge.ts, apps/mobile/src/features/review/useNativeReviewCommentWordDiffs.ts
Hooks compute ranges, reset them by content identity, and expose serialized patches with delivery callbacks.
React surface and review integration
apps/mobile/src/features/diffs/nativeReviewDiffSurface.ts, apps/mobile/src/features/review/ReviewSheet.tsx, apps/mobile/src/features/review/ReviewCommentCard.tsx
The native surface sends word-diff patches through setTokensPatchJson. Review sheets and comment cards provide patch data and reset keys.
Android and iOS range caching
apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/*, apps/mobile/modules/t3-review-diff/ios/T3ReviewDiffView.swift
Native views parse, merge, clear, and render row-keyed word-diff ranges. Android visible-range events use source indices.
Asynchronous highlighting validation
apps/mobile/src/features/diffs/nativeReviewDiffSurface.test.ts, apps/mobile/src/features/review/nativeReviewDiffAdapter.test.ts, apps/mobile/package.json
Tests cover delayed delivery, resets, scroll throttling, comment patches, cancellation, cache behavior, and React test-renderer support.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~60 minutes

Change: Refactor

Suggested reviewers: juliusmarminge, chrisdeeming

Sequence Diagram(s)

sequenceDiagram
  participant ReviewSheet
  participant useNativeReviewDiffHighlighting
  participant computeVisibleNativeReviewWordDiffRanges
  participant NativeReviewDiffView
  participant ReviewDiffContentView
  ReviewSheet->>useNativeReviewDiffHighlighting: provide visible range and collapsed files
  useNativeReviewDiffHighlighting->>computeVisibleNativeReviewWordDiffRanges: compute visible row ranges
  computeVisibleNativeReviewWordDiffRanges-->>useNativeReviewDiffHighlighting: return row-keyed ranges
  useNativeReviewDiffHighlighting->>NativeReviewDiffView: send wordDiffRangesPatchJson
  NativeReviewDiffView->>ReviewDiffContentView: merge word-diff ranges
  ReviewDiffContentView-->>NativeReviewDiffView: render cached ranges
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.36% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 44 functions across 13 files. (1 skipped:… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: preparing word diffs only for visible rows in the mobile review diff.
Description check ✅ Passed The description provides detailed change scope, motivation, validation results, limitations, and release requirements. It does not use all template headings or include the checklist, but the required …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 11.36% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 44 functions across 13 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

t3dotgg and others added 4 commits September 18, 2026 17:58
…cards

Scroll distance was measured against the previous viewport event, so a
one-row-at-a-time Android drag never crossed the 20 row threshold and
stopped requesting word ranges past the initial overscan. Measure it
against the last requested range instead.

Review comment cards in the thread feed still sent only rows, so they
lost word highlights once the adapter stopped embedding ranges. Compute
their ranges with the same deferred path and deliver them as a keyed
patch.
@juliusmarminge
juliusmarminge force-pushed the t3code/perf-mobile-visible-word-diffs branch from 0c9015e to 55afb76 Compare September 19, 2026 01:00
@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Sep 30, 2026 — with ChatGPT Codex Connector

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews 📱 Native Change Changes the native fingerprint; merging blocks production OTAs until a new store build ships. size:XL 500-999 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants