fix(mobile): sticky diff headers, collapse-on-viewed, discussion scroll - #4849
Conversation
A headless kilo run auto-rejects every permission ask it cannot pre-empt, and the reject kills the round silently with EXITCODE=0 and no sentinel. Role definitions already grant what these agents need; --auto stops the CLI answering no on the agent's behalf.
Pin the current file's header row to the top of the Files-tab diff list via FlashList stickyHeaderIndices, with push-style hand-over to the next file's header. Also disable maintainVisibleContentPosition on this list: every height change above the viewport here is user-initiated in place (file expand, collapse-on-mark), where mVCP's anchor misfire makes the header jump; the only async insertion (gap-context load after scroll-away) trades anchor-hold for a mild self-correcting shift.
Wire a pure collapseOnMarkViewed reducer at both onToggleViewed closures (file-header and file-patch-missing rows). Collapse fires only when toggling TO viewed, keyed on the row's pre-toggle state; un-marking never re-expands, and no-op branches return the same state reference. The file navigator's own mark-viewed toggle stays unwired.
…threads Lift thread expansion state into the Discussion tab keyed by threadId with first-sight seeding (resolve never auto-collapses; also fixes the FlashList recycled-cell expansion leak). A top-clipped collapsed row now settles into full view via await scrollToIndex before it grows, guarded by a generation counter invalidated by user drag, unmount, a threads data change, or a same-thread retap — removing the mVCP anchor misfire that flew the tapped header 300+pt off screen. maintainVisibleContent- Position stays on for this list (mid-list insertion on Load more). Also memoize the threads array in usePrReviewDiscussionThreads so its identity changes only on real data changes; the settle-cancel effect keyed on it previously fired on every render commit. The happy-path list moves to discussion/pr-review-discussion-list.tsx (repo's max-lines fallback), unchanged except location.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe incremental diff since the last review contains only three Files Reviewed (3 files)
Previous Review Summaries (5 snapshots, latest commit abca40e)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit abca40e)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe incremental change (gating the mVCP suppression window to Android) leaves the previously-flagged stale Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous review (commit df4b6c3)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryReviewed the incremental diff since the last review (the new Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 4f13d96)Status: No Issues Found | Recommendation: Merge Executive SummaryReviewed the incremental diff (5 files, 95 lines) since the last review; the only code change flips Files Reviewed (5 files)
Previous review (commit 5fc334a)Status: No Issues Found | Recommendation: Merge Executive SummaryReviewed the incremental diff (5 files, 84 lines) since the last review; the only code change is a fix to the discussion-tab settle scroll ( Files Reviewed (5 files)
Previous review (commit 2c16de9)Status: No Issues Found | Recommendation: Merge Executive SummaryReviewed all 14 changed files across the D1 sticky-header, D2 collapse-on-viewed, and D3 discussion-scroll fixes; the pure helpers and their wiring (settle-generation guarding, first-sight seeding, memoized Files Reviewed (14 files)
Reviewed by claude-sonnet-5 · Input: 24 · Output: 5.7K · Cached: 523.3K Review guidance: REVIEW.md from base branch |
The deferred expand's scrollToIndex(viewPosition: 0) aligned the row to the scroll frame top, ignoring the list's firstItemOffset (~153pt iOS), parking the tapped header behind the nav chrome (E2E flow 4, both platforms). Pass viewOffset: -getFirstItemOffset() so the settle targets the row's own absolute content offset: the row top lands at the visible content top and the scroll moves by exactly the clip amount.
…egion On-device diagnostics (iOS, calibrated ±1.3pt) showed the settle itself was pixel-perfect with viewOffset: -firstItemOffset, but a row parked at the visible top edge loses the maintainVisibleContentPosition anchor to the next row: the expansion then scrolls the list by exactly the expansion height (+339.7pt measured) and the tapped header flies off screen. With viewOffset: +firstItemOffset the settle parks the row fully inside the visible region, a row above stays the anchor, and the post-expand adjustment measures -3pt — the header never leaves the screen (y537 in the diagnostic). No-defer control reproduced the original +340pt flight.
Every measured top-clipped flight across E2E rounds r1-r4 equals the expansion height: the mover is the post-expand native maintainVisibleContentPosition adjustment, not the settle target. The deferred path now disables mVCP for exactly the expand commit (one auto-batched render) and re-enables it 150ms later, so the adjustment cannot fire whatever the anchor-boundary geometry (Android parks nearer the visible edge than iOS). Load-more insertion protection is preserved outside the window; guard semantics unchanged.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
E2E r5 showed the suppression fixes every Android top-clipped flow but blanks the entire discussion list on iOS: removing and re-adding the native mVCP prop loses the anchor state there (deterministic -997949 offset, 4/4, uninstrumented). iOS needs no suppression — its deeper firstItemOffset parks the settled row anchor-safe (r4, 3/3). Suppress only on Android; iOS keeps the r4-proven path byte-identically.
2 participants
Summary
What: three client-side presentation fixes in the mobile PR-review feature
(
apps/mobile/src/components/pr-review/,apps/mobile/src/lib/pr-review/):diff used to scroll its file-header row (path, expand chevron, mark-viewed eye) off screen;
the only way back to the file's controls was scrolling all the way up. The header row now
pins to the top of the list while its file's rows are on screen and hands over to the next
file's header as that file reaches the top — via the installed
@shopify/flash-list@2.0.2stickyHeaderIndices, fed by a new purestickyFileHeaderIndices(items)helper.only the viewed set, leaving the (now uninteresting) expanded diff in place. A new pure
collapseOnMarkViewedreducer is wired at bothonToggleViewedclosures inuseDiffRenderItem. It fires only when toggling TO viewed (keyed on the row's pre-togglestate), never re-expands on un-mark, and returns the same state reference on no-ops. The
file-navigator sheet's own mark-viewed toggle is deliberately not wired.
collapsed resolved thread that was partially clipped at the list's top edge flew the
tapped header 329–644pt off screen (pixel-measured in the repro round): FlashList's
maintainVisibleContentPositionanchors the first fully visible row, so the expansionheight inserted above the anchor became an upward scroll of everything above it. Two-part
fix: thread expansion state is lifted into the tab keyed by
threadId(which also fixes arecycled-cell expansion leak the repro found — a resolved thread rendering expanded
after nothing but scrolling), and a top-clipped row now first settles into full view
(
await scrollToIndex, ≤ the clip amount, animated) and only then grows — guarded by ageneration counter invalidated by user drag, unmount, a threads-data change, or a
same-thread retap.
Why: all three were confirmed reproduced as product failures on the unmodified baseline
(repro gate r0, iOS simulator, sentinel
REPRODUCED.for all three; D3's jump mechanismpixel-measured and probed across clipped/fully-visible/expand/collapse configurations).
How: three pure helpers with colocated vitest coverage plus wiring, no new dependencies,
no backend or contract changes. Slice shapes above; notable decisions a reviewer should know:
maintainVisibleContentPositiondisabled. On this list every height changeabove the viewport is user-initiated and in place (file expand, D2's collapse-on-mark), and
with mVCP on, a top-clipped collapse of a tall diff clamps the negative adjustment at
offset 0 — the list would jump to the top. The accepted residual: a gap-context
("load more lines") load landing after the user scrolled away can shift content mildly
(rare, self-corrects on next scroll; recorded non-goal, not stub-verifiable).
the D3 fix is the deferred-expand guard, not a prop. For a fully-visible tapped thread
nothing moves at all (repro-proven geometry, untouched). For a top-clipped tapped
thread an in-place "no jump" is unachievable (the platform anchor rule is the defect's
mechanism); the designed behavior is a short animated settle of ≤ the clip amount followed
by the thread opening under the user's finger — the tapped header never leaves the screen.
useState(!thread.isResolved)meansresolving a thread does not collapse it; seeding preserves that exactly (a dynamic
?? !isResolveddefault would auto-collapse on resolve and dance on optimistic rollbacks).usePrReviewDiscussionThreadsmemoization:threadswas a fresh array every render,so the settle-cancel effect keyed on it fired on every commit; it is now memoized on
[query.data?.pages], so identity changes only on real data changes (impl-review finding,fixed and re-reviewed).
Verification
Unit coverage (vitest, colocated, 21 new tests): sticky indices across expanded/collapsed
files, empty input, and truncation-banner offset; all three
collapseOnMarkViewedbrancheswith reference identity on the no-ops; thread-expansion defaults by resolution, first-sight
seeding (adds missing, preserves explicit, reference-equal when nothing new, resolve flip
does not change a seeded entry), toggle, explicit-expand identity, and every
shouldDeferExpandtable row (null layout, clipped, exact-at-top boundary NOT clipped,fully visible).
Section-wide checks from
apps/mobile:pnpm format,pnpm typecheck,pnpm lint,pnpm check:unused,pnpm test— all green (280 test files / 2367 tests).files and 6-of-9 resolved threads, five dispatched rounds plus an orchestrator takeover
verification):
re-expands / combined A+B stuck-header mark with scroll-offset hold), 3 (fully-visible
expand zero motion), 5 (recycling-leak scroll pile), 6 (collapse geometry), 7a (retap →
exactly one expand), 8 (navigator scroll-to-file → expand → stuck header) — PASS on
both platforms. Flow 4 (top-clipped expand) FAILED both platforms: the deferred settle
parked the header behind the nav chrome (
viewPosition: 0targets the scroll frame top,ignoring
firstItemOffset≈153pt iOS / ~171px Android). Flow 9 (tablet) environment-limited, recorded with rationale.
mover was mVCP's post-expand native adjustment (+339.67pt = exactly the expansion
height), because a row parked at the visible top edge loses the mVCP anchor to the next
row. Fix:
viewOffset: +firstItemOffsetparks the row fully inside the visible region.collapsed-above geometry but FAIL for top-clipped taps with a large expanded card above
(boundary park → anchor loss → +expansion-height flight, ~1500px).
failure geometries, offsets stable post-expand, header parked, pixel-verified; 7b passes
with the combined adb instrument; load-more insertion protection intact on both
platforms) — but cycling the native mVCP prop blanked the whole list on iOS
(deterministic garbage offset, 4/4, uninstrumented). Fix: gate the suppression to
Android; iOS needs none (its deeper park is anchor-safe, r4).
flow 4 gamma, flow 4 epsilon (expanded gamma above), 7c-f4 zeta (post-load-more,
expanded epsilon above) — all PASS: settle → expand → header parked at y537 fully
below the chrome (a11y + screenshot pixel-scan: 1666 dark samples in the title band),
no blank list, list never leaves the screen.
learning) — the guard's cancel wiring is proven live by traces and by Android 7b PASS.
Visual Changes
Behavioral fixes to existing screens (no new UI surface): the Files-tab file header now
sticks to the top while its file is scrolled (the repro round captured the header's absence
from the accessibility hierarchy 100 lines into a diff; E2E rounds captured it pinned with
push hand-over on both platforms); marking viewed collapses the row; expanding a
top-clipped resolved thread now settles into full view and opens with the header on screen
(r0 measured 329-644pt flights off screen; final-head frames show the header parked fully
below the chrome after the expand).
Reviewer Notes
renderItem(target: "StickyHeader"): when a header is stuck, its expand chevron and mark-viewed eye appeartwice in the accessibility tree (stuck overlay + in-list cell). That is platform
sticky-header convention, not a defect; both copies are live.
pr-review-discussion-tab.tsxcrossed the repo'smax-lines: 300lint cap with the newexpansion unit, so the happy-path list moved to
discussion/pr-review-discussion-list.tsx— unchanged except location (the repo's standardextraction, per the plan's documented fallback).
.kilo_workflow/commits (headless-dispatch--autofix + three E2E learnings)are workflow process files, included per the workflow's learnings rule.
is covered by the files list's mVCP disable — no separate machinery.