Repository navigation
feat(conversation): add Conversation and its scroll anchoring - #114
Conversation
Conversation owns the scroll container so every chat surface scrolls the same way. Scroll policy is a prop: settle lands a sent message at the top and never moves while text streams; stick follows the end while the reader is there. Layout shifts never scroll, and only the reader's own input breaks a follow. The anchor algorithm and settle geometry are pure functions, and an anchor correction also applies mid-glide so a glide lands where it meant to. useConversation() shares an item registry, scrollToItem, scrollToEnd, captureAnchor/restoreAnchor, the scroll container and a per-frame scroll snapshot that tells the reader's scrolls from the engine's. The context value keeps its identity across scrolls.
|
Superseded by the comment below. |
This comment has been minimized.
This comment has been minimized.
React 18 ignores a cleanup returned from a ref and calls the ref with null, so an unmounted item stayed registered and scroll anchoring measured a detached node.
4bfccbc to
7a39481
Compare
TheGreatAxios
left a comment
There was a problem hiding this comment.
Automated multi-agent review: independent review + second-agent validation; only two-agent-confirmed items listed. The scroll engine itself is strong — one contract-level blocker found and verified by executing the real engine code in a harness.
Blocking (VERIFIED, executed harness against the branch engine)
- A scrollbar drag while the view is following is classified as a layout shift and actively reversed: the engine writes
endTop()on every drag step (src/ui/conversation.tsx:458-495,:509-512;src/lib/conversation-scroll.ts:245-259). Scrollbar drags dispatch no page pointer events in Chromium (compositor-consumed) or Firefox, sopointerHeldnever latches, no wheel/key/touch fires, andclassifyScrollreturns"layout"— the reader cannot scroll away with the scrollbar in either policy; instickthe follow is never broken by it at all. Suggested shape: default-deny — an unexplained scroll away from the end, outside the resize window (now - resizedAt <= 100), is the reader's. Regression test: scroll with no preceding input event while following, assert no write back toendTop.
Should-fix (VERIFIED, executed harness)
- Scroll events can never cancel a glide: reader-classified momentum mid-glide is recorded as the reader's in the snapshot yet the glide overwrites it and lands at its target (contradicting
glide's own doc); past the 150 ms quiet window, momentum instead classifies as layout because the early-return precedesextendGesture(conversation.tsx:474vs:468/476-483/533). Settle the precedence once. reservedsurvives remount while the fresh spacer has height 0; when the recomputed reserve equals the stale value,updateReserveearly-returns and the spacer is never re-synced —scrollToEnd()lands ~reserve short whileatBottomreports true (conversation.tsx:345-365). Resetreservedon attach.
Minor
ConversationScrollButtonunmounts whenatBottomflips true; a focused keyboard reader falls to<body>— return focus to the log in the button's unmount path when it held focus (conversation.tsx:905/919).- File-for-later: non-scroll keydowns open the gesture window and can misattribute layout scrolls (
:505-507); reserve collapses to 0 with no hand-over when the settled item unregisters;sentIdseed contract,setPolicymid-life switch andConversationEmptyStatelack harness/test coverage.
|
Thanks for the thorough review. On the blocking item (a scrollbar drag while following is classified as a layout shift and reversed), we couldn't reproduce it in any engine:
Pressing the scrollbar does dispatch |
|
Follow-up to the comment above: the scrollbar case doesn't reproduce (now also checked in WebKit with a custom scrollbar:
That contradicts the engine's own comment that find-in-page and focus are kept. We'll fix it along the lines you suggested: treat an unexplained scroll away from the end, outside the post-resize window, as the reader's, with a regression test for both cases. Thanks for flagging it. |
|
Thanks for the review. The scrollbar-drag case is covered in the two comments above. Here's what happened to the rest. Fixed1. A scroll that comes from the page, not the reader, got snapped back to the end (this is the real part of the blocking item)
2. Scroll events could never cancel a glide (should-fix)
3. After a remount,
4. The "Scroll to latest" button dropped focus to the page when it disappeared (minor)
5. The file-for-later items, done now:
Commit: |
TheGreatAxios
left a comment
There was a problem hiding this comment.
Review · Request changes
Adds Conversation and the scroll engine: settle pins a sent message, stick follows the end, and only the reader's own input should break a follow.
Findings
-
src/ui/conversation.tsx:535— A key or touch on a control inside an item cancels a glide.onKeyDowncallsopenGesturefor Space, arrows, Page, Home, and End on anything that is not an input (isEditableat:198does not treat a button as editable).onTouchStart(:553) does not look at the target at all, and setstouchHeld, so the next scroll is the reader's.onPointerDown(:539) already ignores a press that is not on the log itself. Descendants should get the same check: a button's Space or a tap inside a message is not a scroll, and it currently freezes settle or "scroll to latest" mid-glide. -
src/ui/conversation.tsx:517— A focus or find-in-page scroll within 100ms of a resize is snapped back to the end while following. The comment says a scroll that leaves the end is kept and breaks the follow. The branch does the opposite:afterResizewritesendTop()and returns beforefollowingcan become false. Streaming resizes the log for the whole reply, so that window stays open. Only treat the resize's own clamp as disposable; a scroll that lands away from the end should stick and clearfollowing.
Any scroll within 100ms of a resize was taken for the resize's clamp. Streaming resizes on every chunk, so while a reply wrote, a focus or find-in-page scroll was undone: written back to the end while following, or restored to the old anchor by the next chunk after a send. With the browser's anchoring off, a resize moves the view only by clamping it to the last reachable position, so only a scroll that lands there is skipped now. Any other is kept and breaks a follow. Space on a button or similar control inside the log activates it, not scrolls, yet it cancelled a glide in flight. Its other scroll keys and touch still do, since they really scroll the log. scrollToItem takes an onEnd callback that reports whether the glide arrived, so a caller can tell a jump cut short from one that landed.
The test slept in real time and read the wall clock, so on a loaded machine a gap between two scrolls could outlast the gesture's quiet window and end the reader's gesture before the cap. It now moves performance.now itself, 100ms per scroll, and checks every scroll on each side of the two-second cap.
|
Thanks for the review. The second point is fixed, and so is part of the first. The rest of the first is kept on purpose, for the reasons below. 1. A key or a tap on a control inside a message stopped a glide
2. A focus or find-in-page scroll right after a resize was undone
Also in this PR
Commits: |
ConversationContent, ConversationItem and ConversationScrollButton read ref from their props, which only React 19 passes to a function component. Under React 18, inside the peer range, a host ref on any of them stayed null and React warned that ref is not a prop. They are forwardRef components now, so the ref reaches the scroll container, the item and the button under both versions.
When ConversationContent mounted again inside the same Conversation, attach wrote the end only while the view followed and otherwise kept the new container's scrollTop of 0. After any send under settle, or once the reader left the end under stick, a remount (a key change, a panel that unmounts its log) opened at the oldest loaded message. attach now restores the held anchor, which the new items answer to under the same ids, and still writes the end while following.
scrollToItem dropped the follow before the glide found there was no such item, and starting that glide cancelled any glide in flight. A call for an id outside the mounted window moved nothing and reported onEnd(false), yet under stick a streaming reply then ran off the bottom of the view, and a jump already under way stopped short. scrollToItem now reports onEnd(false) for an unmounted id and returns before touching the follow or the glide in flight.
ConversationEmptyState centres with flex-1 and justify-center, but inside ConversationContent its parent is the content column, which is only as tall as its content. With no free space to grow into, the empty state sat at the top of the log. The column now fills the view while it holds an empty state, so the empty state centres in it.
Without a label, ConversationDivider was a div with role separator around a decorative span. Its rule is an hr now, styled as before, so the separator is a real element and the div carries no role. The labelled form is unchanged: its label is read in place and its rules stay decoration.
560dc7a to
137ada2
Compare
Round 3 fixesA fresh review pass over the stack turned up the points below; each is fixed in a new commit at the end of this branch, and the stack is rebased onto it.
|
scrollToItem kept calling onEnd(true) after the item unmounted and the view never moved. End that glide as not arrived.
The probe assigns the API from a child, so a null check narrowed it to never under tsc.
Nothing triage asked to change still fails, so this request no longer stands.
8fb7318 to
167a6a4
Compare
Superseded by the completed stacked review; blocking fixes were addressed and the rebased stack is being landed.
Conversation owns the scroll container so every chat surface scrolls the same way. Scroll policy is a prop: settle lands a sent message at the top and never moves while text streams; stick follows the end while the reader is there. Layout shifts never scroll, and only the reader's own input breaks a follow.
The anchor algorithm and settle geometry are pure functions, and an anchor correction also applies mid-glide so a glide lands where it meant to. useConversation() shares an item registry, scrollToItem, scrollToEnd, captureAnchor/restoreAnchor, the scroll container and a per-frame scroll snapshot that tells the reader's scrolls from the engine's. The context value keeps its identity across scrolls.
Stacked on #113: review only this commit. Part of #94.
Notes for review
settleis built from the spec text (the reference design only sticks to the bottom); not yet compared side by side with other assistant chat UIs.itemId(anidprop would shadow the HTML id).atBottomupdates at most once per frame.Size
Over the 3,000-line triage limit (3,279 lines in 9 files). About 1,300 lines are the scroll engine and its components; the rest is unit tests (about 1,100), browser specs (about 570) and stories. The engine is one unit, so it isn't split.
This PR has two commits: the component, and a follow-up fix that releases a
ConversationItemwhen React 18 detaches it (React 18 ignores a cleanup returned from a ref).Verification
bun run typecheckandbun run linton its own (the stack is checked commit by commit).bun run check,bun run build,bun run dep-guard,bun run contrast-testandbun run test:e2e(Playwright specs and the Storybook axe sweep in light and dark) pass.Closes #99