Skip to content

Enforce reader ownership across chat scroll work - #458

Merged
hamzamerzic merged 1 commit into
mobius-os:mainfrom
hamzamerzic:fix/chat-scroll-reader-authority
Aug 1, 2026
Merged

Enforce reader ownership across chat scroll work#458
hamzamerzic merged 1 commit into
mobius-os:mainfrom
hamzamerzic:fix/chat-scroll-reader-authority

Conversation

@hamzamerzic

Copy link
Copy Markdown
Collaborator

Summary

  • make send, reader, and layout transitions obey one explicit scroll-state entry policy
  • preserve the exact physical reader position when a gesture ends inside reserved reply room
  • reject direct and indirect layout work captured before a newer reader gesture
  • keep composer clearance, spacer sizing, terminal settlement, and scroll writes behind the same ownership boundary

Why

This is a focused follow-up to #220, #342, and #430, which established quiet reader settlement, atomic footer geometry, and a stable final-turn range. The residual alignment in #436 preserved those foundations, but it also retained an inherited transition that converted a reader reaching the physical bottom while reservation remained into PIN_USER_MSG.

That transition recreated automatic layout authority from reader movement and could snap the viewport back toward the latest prompt. A timing gate alone also allowed delayed work captured before a later gesture to become eligible again once the gate expired.

This change narrows the state-machine entrances: only Send creates a pin, and only an unreserved real-content-bottom gesture or an already-armed pin consuming its reservation enters follow. A monotonic reader generation permanently rejects stale direct and indirect geometry work.

Testing

  • 134 focused chat scroll and controller contract tests
  • structural test-budget check
  • production frontend and PWA build
  • rendered 426×860 reserved-tail settlement preserved the identical numeric scroll position

@hamzamerzic hamzamerzic added area: ui User interface and interaction design bug Something isn't working labels Aug 1, 2026
@hamzamerzic
hamzamerzic force-pushed the fix/chat-scroll-reader-authority branch from c9412ae to d7f43a5 Compare August 1, 2026 18:00
Co-authored-by: Möbius Agent <mobius-agent@users.noreply.github.com>
@hamzamerzic
hamzamerzic force-pushed the fix/chat-scroll-reader-authority branch from d7f43a5 to a5f9a52 Compare August 1, 2026 18:21
@hamzamerzic

Copy link
Copy Markdown
Collaborator Author

Review synthesis

This review was run twice: once against d7f43a53, then re-run when the head moved. The findings below are stated against the current head, a5f9a525.

Verdict: approve with nits — the direction and the layering are right, and the one substantive issue found has already been fixed on this head.

The core insight is correct and worth having: the reserved-bottom → PIN_USER_MSG transition really was automatic layout authority manufactured out of reader movement, and settling a reader gesture inside reserved room as an exact ANCHOR_AT is the honest representation. Folding the --composer-h write into sizeSpacer is a genuine cause-level fix — composer clearance clamps scrollTop indirectly, so it belongs behind the same ownership boundary as the spacer, and it removes a prop and a callback from ChatView.

Already resolved on this head

The review against d7f43a53 found one major issue: modeForScrollTransition was stated as "may not create authority" but implemented as "may not restore it", so layout:question-viewport-release was rejected and a FOLLOW_BOTTOM/PIN_USER_MSG base captured by freezeQuestionSubmission was silently discarded — leaving the reader stranded on the overlay anchor with _computeSpacerH still sizing from the stale submit-time viewport.

a5f9a525 fixes exactly that, with the identity check rather than a new event-name entry:

const restoresQuestionSubmissionBase =
  event === 'layout:question-viewport-release'
  && previousMode?.kind === 'ANCHOR_AT'
  && Number.isFinite(previousMode.questionSubmitViewportH)
  && previousMode.questionSubmitBaseMode === proposedMode
if (restoresQuestionSubmissionBase) return proposedMode

The accompanying test is the right shape too — it covers the FOLLOW_BOTTOM and PIN_USER_MSG bases and asserts that a structurally-equivalent-but-not-identical mode is still rejected, which is the part that keeps the check meaningful. Nothing further needed there.

[NIT] ARCHITECTURE.md now contradicts itself on this exact case

Where: ARCHITECTURE.md:588 vs :721

Line 588 states, absolutely: "pin ownership is never reconstructed by layout". Line 721 still carries the row "Viewport/keyboard changes after question submission | transient question anchor | pre-submit unanswered-card mode" — and with the fix above, a PIN_USER_MSG base is now restored by a layout event, correctly.

Both can't be read literally at once, and this section declares itself the owner-authoritative contract with the rule that when implementation and contract disagree, the implementation is the bug. Since the implementation is right here, line 588 is the thing to soften — e.g. "never created by layout; a mode captured by question submission may be restored to its owner."

Design notes

  • The generation threading is mostly ceremony. Only three sites genuinely capture a version and commit later: the deferLayoutUntilReaderYields timer, the terminal rAF plan, and the send intent. Elsewhere — layoutOwnsScroll()'s default parameter, runComposerResize, runPaneResize, the ResizeObserver, and reapplyActiveMode (:1453, which reads readerIntentVersionRef.current into authorityVersion and then compares it against readerIntentVersionRef.current) — the comparison is a tautology, and the extra parameter obscures which calls are actually deferred. Passing authorityVersion only where work is genuinely captured would make the three real cases legible, and would remove the standing risk that a new call site forgets the argument and silently degrades to "always current".
  • modeAfterReaderGesture falls back to {kind:'INITIAL'} when anchorModeFromScroll returns null, where the old path left the mode alone. INITIAL re-arms the layout effect's _validateSavedMode restore branch. It is unreachable today — _topmostVisibleMsg only returns null for a transcript with zero rows, which cannot scroll — but holdMode || previousMode would be a safer fallback for free.
  • One suggestion at the class level rather than the instance. The issue above survived a green suite because modeForScrollTransition was only ever tested against hand-written event strings, never against the set of events transitionMode is actually called with. The new test closes this instance; since every one of those event strings is a literal in a single file, a test that feeds each real event a FOLLOW_BOTTOM/PIN_USER_MSG proposal and asserts the expected accept/reject would close the whole class.
  • Worth a product decision, not just a code one: after this change an identical-looking gesture ("I scrolled to the bottom") yields follow or a frozen anchor depending on invisible spacer state, and a reader who reaches the bottom during a reserved turn never follows even once the reservation is consumed. The contract and the rewritten send-rule.spec.mjs:247 both now say that is intended — so it is a decision rather than a defect, but it is a user-visible one, and the PR description reads as internal ownership cleanup.

Notes

  • The second model reviewed this as APPROVE on both heads, finding the implementation consistent with the updated ownership contract, with the caveat that CI should be green first. It did not exercise the guard against the real event set.
  • CI is mid-run on this head; the earlier e2e failure was against an older head and is not necessarily still current.

Synthesized from two independent model reviews (Claude Opus 5, GPT-5.6 Sol), re-run against a5f9a525 after the head moved twice during review, with no shared context between them.

@hamzamerzic
hamzamerzic added this pull request to the merge queue Aug 1, 2026
Merged via the queue into mobius-os:main with commit 7f7752c Aug 1, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: ui User interface and interaction design bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant