fix(mobile): simplify PR review file scrolling

[iscekic]

What

Simplifies the mobile PR-review Files list while fixing duplicate paths and scrolling stalls.

• Deletes four obsolete scroll-state files and roughly 285 lines.
• Dedupes infinite-query pages once at the query layer, so Files and the navigator share the same unique paths.
• Keeps FlashList cells memoized while pagination state changes, then replaces the park/retry scroll machine with one built-in scrollToIndex jump.
• Adds committed GitHub-stub fixtures for a 120-file PR and a cross-page duplicate path.

Why

The list had repeated page-flattening code and a scroll subsystem that could not meaningfully park a navigator target. During pagination, both the item objects and renderItem callback changed, making FlashList re-render mounted diff cells while the user dragged.

How

• flattenFilePages is now the shared first-occurrence-wins source of truth.
• File rows and pagination rows are memoized separately; retry/load-all callbacks are stable.
• Navigator jumps expand only files with renderable diffs and use scrollToIndex directly.
• maintainVisibleContentPosition remains disabled because height changes above the viewport previously caused jumps; it is only the recorded escalation if E2E proves necessary.
• No per-file row memo: one page-arrival rebuild is deliberately left as the lower-cost event, with an evidence-based follow-up path.

Verification

• pnpm run format:changed
• pnpm --dir apps/mobile format
• pnpm --dir apps/mobile typecheck
• pnpm --dir apps/mobile lint
• pnpm --dir apps/mobile check:unused
• pnpm --dir apps/mobile test — 294 files, 2,567 tests
• git diff --check
• iOS E2E S0–S7: pending

Visual Changes

Pending iOS E2E screenshots for S2, S3, and S6.

Reviewers

Requested from repository history: @jeanduplessis and @St0rmz1.

---

Section final report (archived from scratch before cleanup)

BLOCKED

Blocker

Bot E2E could not reach the required verifier dispatch. The iOS bundle started three times and the Hermetic PR-review GitHub stub started successfully, but the required dev-only githubApps.devSeedUserGithubToken seed mutation could not be authenticated through the local fake-login flow.

• First setup: callback request returned HTTP 400.
• First recovery: UI-path fake login produced no usable cookie; seed request returned HTTP 401.
• Second recovery: direct callback at the stack host created a session, but the mutation request returned HTTP 400.
• The E2E scenario list is recorded in e2e-scenarios.md; no verifier was dispatched without the required seeded token.

This blocks the required VERIFICATION PASSED. sentinel and S0–S7 screenshots. The runbook requires a valid seeded GitHub token before PR-review entry flows can be verified. Remedy: repair or provide the sanctioned local fake-login/tRPC seed path, then re-run the iOS E2E bundle and all S0–S7.

PR

• fix(mobile): simplify PR review file scrolling #4956
• Branch: pr-review-files-scroll-dd1e
• Commits:
  • ebf9fb3df fix(mobile): simplify PR review file scrolling
  • b23a2ffdd test(mobile): add PR review file fixtures
  • 11eb1b7dc fix(mobile): align PR fixture metadata
• Assigned to iscekic; reviewers requested: jeanduplessis, St0rmz1.

Implemented and verified

• Query-layer duplicate-path flattening, stable FlashList file rows/render callback, simplified navigator scroll hook, and four obsolete state-machine files deleted.
• Committed 120-file and cross-page-duplicate stub fixtures.
• Local checks passed before the final metadata repair:
  • pnpm run format:changed
  • pnpm --dir apps/mobile format
  • pnpm --dir apps/mobile typecheck
  • pnpm --dir apps/mobile lint
  • pnpm --dir apps/mobile check:unused
  • pnpm --dir apps/mobile test — 294 files, 2,567 tests
  • git diff --check
• Fresh cumulative implementation review: No findings.

Kilobot triage

• Fixed the concrete fixture-count finding and committed 11eb1b7dc; its thread is resolved.
• Rejected the absent navigator target fallback: recorded plan §9 accepts re-tap over restoring the deleted park/retry machine; thread resolved.
• Rejected removal of test-only buildItems: it remains the tested composition contract; thread resolved.

Resources and temporary edits

• All three iOS bundle attempts were stopped; recorders stopped, simulator claims released, stacks stopped, stubs killed, and E2E slots freed.
• GITHUB_API_BASE_URL was removed from worktree .env.local.
• Temporary-edit patch saved as github-stub-env.patch in this scratch directory; no temporary edit remains in the product tree.
• No generated fixture remains.

Takeovers

• Manually captured Slice A review diff after slice-diff.sh could not process its staged deleted paths; the reviewer cleared the actual diff.
• Manually corrected the initial fixture repair’s accidental existing-fixture count churn before committing; only generated fixture metadata changed.

Safest next action

Restore a sanctioned authenticated call to githubApps.devSeedUserGithubToken, then run a fresh iOS E2E round using this scratch directory’s e2e-scenarios.md before continuing the PR completion gate.

---

Reviewer note: This PR temporarily contained changes from #4960 to unblock e2e. Those changes are removed. #4960 lands the e2e harness.

[kilo-code-bot]

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

The new head commit (a395f287, chore(e2e): revert apps/mobile/e2e to base) is a history rewrite with a byte-identical tree to the previously reviewed commit, so there is no new code to review and no unresolved findings remain.

Verification of the incremental change

• git diff 5ea408b9..a395f287 is empty and both commits resolve to the same tree 10ea00b4, so the force-push changed only commit identity (message/parent metadata), not content.
• Net PR scope is still the 13 files under apps/mobile/src/components/pr-review/diff/** and apps/mobile/src/lib/pr-review/diff/**; apps/mobile/e2e/** remains reverted to main, with the e2e harness landing separately in refactor(mobile-e2e): simplified runbook, scripted stub setup, load-tolerant flows #4960 per the PR description.
• Existing inline threads reconciled against live GitHub state: the two remaining line comments (use-pr-diff-list-scroll.ts:40, pr-diff-list-builder.ts:269) are SUGGESTION-level and were explicitly rejected by the author with recorded plan rationale; the apps/mobile/e2e/github-api-stub/server.mjs thread is now outdated (line: null) because that file left the PR.

Assumption: the iOS E2E S0–S7 evidence listed as pending in the PR description is validated outside this review; no runtime verification was performed in read-only mode.

Files Reviewed (0 files changed since last review)

• No files changed between 5ea408b9 and a395f287 (identical tree)

Previous Review Summaries (3 snapshots, latest commit 5ea408b)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 5ea408b)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The incremental commit (5ea408b9, chore(e2e): revert apps/mobile/e2e to base) is a clean revert of the E2E stub/doc changes, verified byte-identical to the base tree, and introduces no issues.

Verification of the incremental change

• git diff b9ebddce..5ea408b9 -- apps/mobile/e2e is empty, so apps/mobile/e2e/AGENTS.md and apps/mobile/e2e/github-api-stub/server.mjs now match main exactly; the reverted restPull hardcoded totals (changed_files: 2, additions: 10, deletions: 2) are base code again and stay consistent with the base two-file stubFiles() fixture.
• The AGENTS.md pinned-surface line no longer advertises kilo-stub/files-many#4 / kilo-stub/files-dupe#5, matching the removed fixtures — no stale documentation left behind.
• Repo-wide search at HEAD finds no remaining references to files-many, files-dupe, filesFixture, manyFiles, dupeFiles, or MULTI_HUNK_PATCH, so the removal leaves no dangling consumers.
• Remaining PR scope is now limited to apps/mobile/src/lib/pr-review/diff/** (13 files), unchanged since the previous review.

Assumption: the E2E harness for these fixtures lands separately in #4960, as stated in the PR description.

Previously reported findings

• apps/mobile/e2e/github-api-stub/server.mjs fixture counts — no longer applicable; the whole fixture block was reverted out of this PR (the inline thread is now outdated on GitHub).
• use-pr-diff-list-scroll.ts:40 and pr-diff-list-builder.ts:269 — files unchanged in this increment and addressed by author replies; not carried forward.

Files Reviewed (2 files)

• apps/mobile/e2e/AGENTS.md
• apps/mobile/e2e/github-api-stub/server.mjs

Previous review (commit 11eb1b7)

Status: No Issues Found | Recommendation: Merge

Executive Summary

Incremental review of the single new commit (11eb1b7d, fixture-metadata alignment in the GitHub API stub) found no issues; the previously flagged fixture count mismatch is now resolved.

Verification of the incremental change

• genPatch(n) emits 6 + lines and 1 - line, so prFile(..., 6, 1, genPatch(i)) in both manyFiles and dupeFiles now matches the patch body exactly.
• MULTI_HUNK_PATCH has three hunks of 6 additions / 1 deletion each, so file-010.ts at 18, 3 is correct.
• file-060.ts keeps 0, 0 with patch: null, which is the intended null-patch case.
• REST pull totals are derived (changed_files: fx.files.length, summed additions/deletions), so the overview stays consistent with the per-file numbers without further edits; stubFiles() still sums to additions: 10 / deletions: 2 as its comment states.
• Remaining edits ((n) => -> n =>, wrapped filesFixture(...) call) are formatter-only and behavior neutral.
• The hunk-header line counts in genPatch are approximate, but parse-patch.ts only reads oldStart/newStart for line numbering and echoes the counts back into the header string, matching the pre-existing Alpha/Beta fixtures — no functional impact.

Assumption: no automated assertion pins the previous 4/1, 3/1, or 20/15 values; no consumer of these fixtures outside the stub server references them.

Previously reported findings

• apps/mobile/e2e/github-api-stub/server.mjs fixture counts - fixed in this commit.
• use-pr-diff-list-scroll.ts:40 (silent target-absent no-op) and pr-diff-list-builder.ts:269 (test-only buildItems) - unchanged files in this increment, addressed by author replies; not carried forward.

Files Reviewed (1 file)

• apps/mobile/e2e/github-api-stub/server.mjs

Previous review (commit b23a2ff)

Status: 3 Issues Found | Recommendation: Address before merge

Executive Summary

No correctness defects found in the scroll/dedupe refactor; the three findings are a silent no-op fallback for unresolvable navigator jumps, a now production-unused buildItems that most builder tests still target, and E2E fixture counts that disagree with the generated patch bodies.

Overview

Severity	Count
CRITICAL	0
WARNING	0
SUGGESTION	3

Issue Details (click to expand)

SUGGESTION

File	Line	Issue
apps/mobile/src/lib/pr-review/diff/use-pr-diff-list-scroll.ts	40	Navigator request with an absent target key is dropped silently; the removed park/retry had no replacement fallback
apps/mobile/src/lib/pr-review/diff/pr-diff-list-builder.ts	269	buildItems is now test-only; three builder suites exercise a wrapper the app no longer runs
apps/mobile/e2e/github-api-stub/server.mjs	173	Declared additions/deletions (4/1, 3/1, 20/15) do not match genPatch/MULTI_HUNK_PATCH content

Verified as correct

• flattenFilePages dedupe is first-occurrence-wins and shared by the list and navigator; both read the same query key so the counts stay consistent.
• The paginationItem memo's suppressed dep list covers every field buildPaginationItem reads (isLoading, isFetchingNextPage, hasNextPage, laterPageError via query.isError + files.length, fetchToCompletionRunning, fetchToCompletionLoaded, totalFiles via changedFiles); the pagination-only dummies passed to buildFileItems are all unread by that builder.
• useFetchToCompletion's ref-held query keeps run stable without closure staleness, and the loop still exits on the fetchNextPage() result rather than a captured flag.
• The navigator subscription is created and torn down in the same effect, so removing the pending-scroll state introduces no listener leak.
• Stub fixtures line up with FILES_PAGE_SIZE = 50: files-many#4 is 3 pages (50/50/20) and files-dupe#5 puts the duplicate path first on page 2; restPull's derived changed_files: 2 / additions: 10 / deletions: 2 still matches the existing stubFiles() fixtures.

Assumption: the removed one-shot scroll-to-top (onContentSizeChange) is safe because FlashList only mounts once the first page exists, per the file's own cold-vs-warm note. That is a runtime-layout behavior the pending iOS E2E (S0–S7) should confirm.

Files Reviewed (15 files)

• apps/mobile/e2e/AGENTS.md - 0 issues
• apps/mobile/e2e/github-api-stub/server.mjs - 1 issue
• apps/mobile/src/components/pr-review/diff/pr-diff-file-list-render.tsx - 0 issues
• apps/mobile/src/components/pr-review/diff/pr-diff-file-list.tsx - 0 issues
• apps/mobile/src/components/pr-review/diff/pr-diff-file-navigator.tsx - 0 issues
• apps/mobile/src/lib/pr-review/diff/dedupe-file-pages.ts - 0 issues
• apps/mobile/src/lib/pr-review/diff/dedupe-file-pages.test.ts - 0 issues
• apps/mobile/src/lib/pr-review/diff/pr-diff-list-builder.ts - 1 issue
• apps/mobile/src/lib/pr-review/diff/pr-diff-list-builder.test.ts - 0 issues
• apps/mobile/src/lib/pr-review/diff/pr-review-file-list-state.ts - 0 issues
• apps/mobile/src/lib/pr-review/diff/use-pr-diff-list-scroll.ts - 1 issue
• apps/mobile/src/lib/pr-review/diff/initial-top-scroll.ts (deleted) - 0 issues
• apps/mobile/src/lib/pr-review/diff/initial-top-scroll.test.ts (deleted) - 0 issues
• apps/mobile/src/lib/pr-review/diff/pending-scroll-request.ts (deleted) - 0 issues
• apps/mobile/src/lib/pr-review/diff/pending-scroll-request.test.ts (deleted) - 0 issues

Fix these issues in Kilo Cloud

---

_{Reviewed by claude-opus-5 · Input: 28 · Output: 5.4K · Cached: 654.5K}

_{Review guidance: REVIEW.md from base branch main}