Skip to content

fix(chat): auto-scroll transcript to newest message (#155) - #238

Merged
dzianisv merged 2 commits into
mainfrom
fix/auto-scroll-155
Nov 13, 2026
Merged

dzianisv merged 2 commits into
mainfrom
fix/auto-scroll-155

Conversation

@dzianisv

Copy link
Copy Markdown
Owner

Closes #155.

Root cause

The transcript is an inverted FlatList, so "scroll to the newest message" means scrolling to offset 0 — and nothing was observing content growth, so nothing ever scrolled at all. The only scroll-to-newest path was the manual scroll-to-bottom button. maintainVisibleContentPosition (kept: it stops jumps when older pages load via onEndReached) actively holds visible items still when new messages insert at index 0, parking new content just outside the viewport.

Approach

Scoped port of the auto-scroll part of PR #182 (commit c60ebaf), which is unmergeable as a whole (49 files spanning keyboard, SSE, voice-alert and other work):

The follow only fires while the view is within 200px of the newest message, so a user scrolled up reading history is never yanked back down. The scroll-to-bottom button uses the same predicate, so the two cannot disagree about where "bottom" is.

Signature kept at #182's 2-field form (count:length): the only case the 4-field form would catch is a same-length swap of the newest message, negligible for a streaming transcript.

Test evidence

  • npm run check:versions — clean (Version metadata aligned: 0.4.17 (44))
  • npm run typecheck (tsc --noEmit) — clean
  • Full suite npm test — 405 pass / 0 fail (baseline 395 + 10 ported auto-scroll tests; standalone node --test src/lib/auto-scroll.test.ts 10/10)

Test-count delta: +1 test file, +10 cases — the ported auto-scroll tests are AC-mapped cases for genuinely new behavior (recorded exception, context pack on #155 criterion 5).

Still owed before merge

Device proof (acceptance criterion 6): emulator/physical verification that a streaming reply follows and that reading history is never yanked. Unit tests only so far.

Port the auto-scroll policy from PR #182 (commit c60ebaf) in scoped form:
src/lib/auto-scroll.ts + its 10 tests, byte-identical to PR head
1181372, wired into app/session/[id].tsx.

Root cause: the transcript is an inverted FlatList and nothing observed
content growth, so an arriving or streaming reply never followed; the
only scroll-to-newest path was the manual button. maintainVisibleContentPosition
stays (it stops jumps when older pages load), so the fix scrolls
explicitly via the existing scrollToBottom when new content arrives —
but only while the view is within 200px of the newest message, so a
user scrolled up reading history is never yanked. The scroll-to-bottom
button now uses the same predicate, so the two cannot disagree about
where 'bottom' is.

Signature kept at #182's 2-field form (count:length): the only case the
4-field form would catch is a same-length swap of the newest message,
negligible for a streaming transcript.

Gates: npm run check:versions clean (0.4.17/44), tsc --noEmit clean,
full suite 405 pass / 0 fail (395 baseline + 10 ported auto-scroll
tests). Device proof still owed before merge.
@dzianisv

Copy link
Copy Markdown
Owner Author

Stage 4 review verdict: PASS

Checked against head 8c7747a069825fd68df8c54c693d1fe8ea05354c (fix/auto-scroll-155). I ran both passes myself in sequence (no subagents available at this depth): compliance against the acceptance criteria, then a bug hunt over the changed code and its call sites. Diff read directly via gh api (not a summary); gates re-run in the real worktree checkout.

Test-count delta: 395 → 405 pass (+10 cases), 50 → 51 test files (+1). Baseline re-run independently in a temp worktree at 2667ad5c: 395 pass / 0 fail. The +10 is covered by the AC-mapped exception recorded in the context pack on #155 (comment 6856194485): all 10 cases map to ACs 1–3 (follow on arrival, follow while streaming, no-yank, shared button predicate, threshold/signature boundaries). The ported test file follows the repo's established convention — 40 pure-helper src/lib/*.test.ts files already exist on main (e.g. scroll-config.test.ts, keyboard-offset.test.ts), node:test, no mocks, no RN imports — so it is not a new unit/mock-test violation.

Compliance (one criterion at a time)

  1. New message / streaming reply scrolls to offset 0 within 200px — PASS. app/session/[id].tsx:324-351: effect computes transcriptSignature(messageData.length, newest-parts text length) and calls existing scrollToBottom(true) → scrollToOffset({offset: 0}) when shouldAutoScroll says so. Signature changes on count and on text-length growth, so streaming follows. Tests: "scrolls when new content arrives", "follows a streaming reply as it grows", "scrolls on the very first content".
  2. User scrolled up (>200px) is never yanked — PASS. src/lib/auto-scroll.ts:44-57: shouldAutoScroll returns false unless isAtBottom(offsetY) (offsetY <= 200). Test at offset 5000 → false; unchanged-content test guards against fighting a gesture.
  3. Button uses the same predicate/threshold — PASS. handleScroll now uses shouldShowScrollButton(contentOffset.y) instead of the hardcoded > 200. Grep confirms no other literal 200 threshold remains in app//src/ (only unrelated comments). Test asserts shouldShowScrollButton(offset) === !isAtBottom(offset) at 0/50/200/201/900.
  4. maintainVisibleContentPosition stays; onEndReached no jump — PASS. Prop unchanged at [id].tsx:812 ({minIndexForVisible: 0}), onEndReached/handleLoadMore untouched. Diff touches only 3 files, 3 hunks in [id].tsx (import, follow effect, handleScroll).
  5. Typecheck clean + full suite green — PASS, re-run by me. Worktree: tsc --noEmit exit 0; npm test 405 pass / 0 fail. Port verified byte-identical to PR fix: keyboard/composer (#156), auto-scroll (#155), dead SSE (#186), blocked SSE startup (#189), stale transcripts, uncopyable text, axis-perfect table scrolling, 50-session list cap, transcript flash, dark-mode contrast #182 head 1181372f (blob hashes f892bada… / e81d3d07… match on both sides). No keyboard code, SessionInfo.onScrollToTop, or any other fix: keyboard/composer (#156), auto-scroll (#155), dead SSE (#186), blocked SSE startup (#189), stale transcripts, uncopyable text, axis-perfect table scrolling, 50-session list cap, transcript flash, dark-mode contrast #182 part touched.
  6. Device proof — OWED, not a merge block. Per the project's established pattern and the task brief: the owner's phone check on the v0.4.18 build before it ships. The PR body already lists it as owed. Not failed on this ground.
  7. Message cannot be scrolled automatically. #155 closed by this scoped PR — PASS on merge. PR body says Closes #155; fix: keyboard/composer (#156), auto-scroll (#155), dead SSE (#186), blocked SSE startup (#189), stale transcripts, uncopyable text, axis-perfect table scrolling, 50-session list cap, transcript flash, dark-mode contrast #182 stays unmerged (49 files, mergeable: false).

Bug hunt — no validated findings

Checked the call sites: messageData is reversed so messageData[0] is the newest (L222-233); scrollToBottom is a stable useCallback([]) defined before the effect; effect deps [contentSignature, scrollToBottom] are correct; scrollOffsetRef is read only inside the post-commit effect, so its later declaration in source order is not a TDZ issue; part.text?.length ?? 0 handles text-less parts. Two filed notes, neither a fail:

  • scrollEventThrottle={100} (pre-existing) means scrollOffsetRef can be up to ~100ms stale mid-fling, so a chunk arriving in that window could fire one auto-scroll after the user has just passed 200px. Narrow race, throttle not introduced by this PR.
  • The signature counts only text length of the newest message, so growth via non-text parts alone would not trigger a follow until text grows. The 2-field form itself was a recorded decision (comment 6856285744); same-length newest-message swap remains the known accepted gap.

CI at review time: typecheck/test/secret-scan green; build, build-fdroid/iOS Simulator, activation-e2e still in progress. Merge still waits on CI green + the device proof in (6).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Message cannot be scrolled automatically.

1 participant