Skip to content

fix(mobile): bound huge native review diff payloads - #19240

Merged
juliusmarminge merged 6 commits into
pingdotgg:mainfrom
Pipyakas:fix/android-review-diff-oom
Oct 20, 2026
Merged

juliusmarminge merged 6 commits into
pingdotgg:mainfrom
Pipyakas:fix/android-review-diff-oom

Conversation

@Pipyakas

@Pipyakas Pipyakas commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Huge turn diffs currently serialize every row into the native review view. For a synthetic 200,000-line file, that creates a 75 MB JSON payload before Android decodes it on the Java heap.

Limit each file to 2,000 source rows before native mapping and word matching. File headers and totals retain the full counts, and an explicit notice marks the excerpt. Ordinary files remain complete. Saved comments wholly beyond the excerpt are omitted inline; comments overlapping the boundary keep their full range label at the last visible line. Original comment messages remain intact.

Rebuilt from @Pipyakas's #19240, retaining the contributor commits and merging main. The adapter cap was already the smallest model; the takeover fixes comment relocation and strengthens the two existing PR tests.

Scope and approval

This is a partial mitigation for #14649. It differs from the maintainer triage, which proposes a 400-line / 24,000-character gate with a Load diff control. Human approval of this 2,000-row excerpt alternative is still required. This PR adds no load control.

The shared mobile adapter affects Android and iOS thread review and pull request review surfaces. Beyond the excerpt, new inline comments and existing PR review threads cannot attach to native rows. Review-guide snippets remain visible, but opening a deep omitted line in the full diff falls back to the file header. Approval must cover these PR-review limits as well. This is a per-file row bound: individual row length, total file count, comments and the full parsed JavaScript model remain unbounded. It does not establish that every large diff avoids OOM.

Verification

  • PR tests: 2 → 2, rewritten in existing nativeReviewDiffAdapter.test.ts (15 adapter cases). Huge and ordinary file behavior share one test; the other covers visible, overlapping and wholly omitted comments.
  • Before comment fix: 33 passed, 1 failed. After: 34 passed across adapter, model and selection suites. Removing the cap makes the row-bound test fail with 200,001 source rows instead of 2,000.
  • Mobile typecheck, scoped lint, formatting and diff checks passed.
  • Actual main adapter versus this head, with matching synthetic input and the bridge's JSON serialization:
200,000-line file Main This head
Native rows 200,002 2,002
JSON bytes 75,045,048 730,926
Reported additions 200,000 200,000

Payload comparison output. This is headless payload proof, not device memory proof. Android build, on-device heap measurement, opening/scrolling the reporter's huge diff, and matching before/after screenshots or recording remain outstanding. The Device API reports “Agent device access is turned off for this environment”; Java and adb are absent. iOS, macOS, Windows, Electron desktop and remote/relay/tunnel connections were not run. Web/desktop rendering is unchanged.

Two independent full-diff review rounds with Claude Opus and Codex Sol. The saved-comment bug and an overclaim in the notice text were fixed; the final round found no code blockers. Current-head build, lint and test jobs passed. Configured Macroscope checks have not published a result on this head. Human product approval and device proof remain outstanding.

Implementation: Codex gpt-6.1-sol/high in T3 Code; tests: Codex gpt-6-astra/high; adversarial reviews: Claude claude-opus-5-5 and Codex gpt-6-sol.

Every diff row was serialized for the native review view and decoded on the
Java heap, so a 200,000-line file exhausted it. Send at most 2,000 rows per
file followed by a notice; the file header keeps the full counts.

Fixes pingdotgg#14649
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 14, 2026
Comment thread apps/mobile/src/features/review/nativeReviewDiffAdapter.ts
@macroscopeapp

macroscopeapp Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Would Approve

Macroscope's review found this PR approvable — This is a narrowly scoped Android OOM fix that bounds native diff serialization for unusually large files while preserving counts, notices, and existing behavior for ordinary files; tests cover both paths. A remaining Medium finding identifies possible relocation of saved comments beyond the visible excerpt, which represents a correctness risk outside the eligibility assessment.

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.

@coderabbitai

coderabbitai Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Warning

Review limit reached

Only developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing.

Next included review available in 12 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 77889c32-735f-4647-ad20-a9da643b3f7e
📥 Commits

Reviewing files that changed from the base of the PR and between 43e7cd0 and 5225bf2.

📒 Files selected for processing: 2
  • apps/mobile/src/features/review/nativeReviewDiffAdapter.test.ts
  • apps/mobile/src/features/review/nativeReviewDiffAdapter.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: b74f1fad-5b8c-4f7b-8546-c2ef4a6e22ae
📥 Commits

Reviewing files that changed from the base of the PR and between 6002c5c and 43e7cd0.

📒 Files selected for processing: 2
  • apps/mobile/src/features/review/nativeReviewDiffAdapter.test.ts
  • apps/mobile/src/features/review/nativeReviewDiffAdapter.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

📝 Walkthrough

The native review diff adapter now limits each file to 2,000 rendered source rows. When it omits rows, it adds a truncation notice. File headers and change counts continue to use the full file data.

Changes: Native diff size handling

Layer / File(s) Summary
Limit rendered rows and verify counts
apps/mobile/src/features/review/nativeReviewDiffAdapter.ts, apps/mobile/src/features/review/nativeReviewDiffAdapter.test.ts
The adapter processes up to 2,000 source rows per file and appends a notice when it omits rows. Tests cover a 200,000-line diff with full counts and a truncation notice, and a 1,000-line diff with all rows and no notice.

Priority: ⬆️ High

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 43e7c

The row limit is ready to merge after normal checks. Its effect on Android device memory has not yet been measured.

Security architecture risk: 🔵 Low · up to 43e7c

The change reduces memory pressure without adding access or privileges. The main compatibility caveat is that existing comments on omitted lines can appear beside the last displayed line. No new security vulnerability was established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — In the inspected flow, the changed resource exposure is the mobile client's handling of diff content. Per-file source-row expansion is reduced, but parsing, file count, individual row length, and subsequently attached comments are not bounded by this new limit. These remaining dimensions predate the change; no increased exposure was established.

Trust Boundaries and Controls

  • observed — The changed operation selects a prefix of already-parsed rows and adds fixed notice text. It retains the existing row-data interface and native dispatch methods; it introduces no new credential access, execution sink, or authorization decision in the modified production code.

Resilience and Maintainability Implications

  • observed — Repeated preparation and comment edits retain the existing ownership model: aggregate caches are keyed by parsed-diff objects, file preparation by file objects, and comment overlays are refreshed separately. The truncation change does not introduce a shared cross-file cache key or a new persistence or recovery transition.
Pre-merge checks: Passed 4
Check name Status Explanation
Linked Issues check Passed Issue [#14649] requires Android turn-diff review to remain usable without a fatal memory error. The adapter now limits each file to 2,000 source rows before native row serialization. It appends the re…
Out of Scope Changes check Passed The PR changes only native review-diff row preparation and its automated tests. The row cap, truncation notice, preserved counts, and bounded comment targets directly address [#14649]. No unrelated pr…
Title check Passed The title clearly and concisely describes the main change: limiting oversized native review diff payloads on mobile.
Description check Passed The description includes all required sections, explains the problem and implementation, links the related issue, documents verification results, and clearly identifies outstanding approval and device…
Full details: Linked Issues check

Explanation

Issue [#14649] requires Android turn-diff review to remain usable without a fatal memory error. The adapter now limits each file to 2,000 source rows before native row serialization. It appends the required notice for truncated files. The file header keeps full addition and deletion counts. Comment target data uses the same bounded source rows. Tests cover a 200,000-line file, full counts, the notice, and an ordinary 1,000-line file without truncation.

Full details: Out of Scope Changes check

Explanation

The PR changes only native review-diff row preparation and its automated tests. The row cap, truncation notice, preserved counts, and bounded comment targets directly address [#14649]. No unrelated product behavior or unrelated files appear in the reviewed changes.

Full details: Description check

Explanation

The description includes all required sections, explains the problem and implementation, links the related issue, documents verification results, and clearly identifies outstanding approval and device-validation work.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@juliusmarminge

Copy link
Copy Markdown
Member

Taking over this one for maintainer batch worktree-git-03. I will keep the bounded native payload approach, fix saved comments beyond the excerpt, and verify the focused tests. Android device proof and approval of the excerpt behavior remain explicit gates.

@juliusmarminge juliusmarminge changed the title fix(mobile): huge turn diffs no longer run Android out of memory fix(mobile): bound huge native review diff payloads Oct 19, 2026
@juliusmarminge
juliusmarminge merged commit 6fc3c9e into pingdotgg:main Oct 20, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants