Repository navigation
feat(conversation): add useHistoryWindow and ConversationEarlier - #122
Conversation
useHistoryWindow keeps a window of the latest items, adds earlier ones as the reader nears the top, freezes while the reader has scrolled away and trims back once they return to the end. It works with history the host pages in asynchronously, and the item in view stays put within a pixel as earlier items load. ConversationEarlier marks the top with a loading state and a retry when a page brings nothing. The doc comment records why this windows rather than virtualizes, and its ceiling.
628cf5e to
a6ac8b7
Compare
a6ac8b7 to
a613233
Compare
TheGreatAxios
left a comment
There was a problem hiding this comment.
Automated multi-agent review: independent review + second-agent validation; only confirmed items listed. The hook itself is well-built and the windowing-ceiling doc is real; one verified race below.
Verified (executed harness running the real hook + real engine)
- The render-phase trim reads the rAF-throttled context
atBottom: an append committing between the reader's scroll-up and the engine's per-frame pass removes the first shown item while the reader is scrolled up — violating the plan's hard constraint "never removes items while the reader is scrolled up" (src/hooks/use-history-window.ts:156-162vssrc/ui/conversation.tsx:389-399; a deterministic ordering harness reproduces it, and a frame-first control run proves nothing is removed when ordering is safe). Fix shape: gate the trim on a fresh position signal — trim in thesubscribeScrolllistener whensnapshot.distanceFromEnd <= threshold, or read the last snapshot's distance from a ref in the render-phase trim. The suggested regression test currently fails on this code.
Minor
e2e/history-window.spec.ts:3-51duplicatese2e/conversation.spec.ts:3-44's harness helpers line-for-line — extract a sharede2e/helper before the next PR copies it.- A sync throw from
onLoadEarlieris settled then rethrown out ofreachStart(), which runs inside the marker's IntersectionObserver callback → uncaught exception in an observer callback (use-history-window.ts:246-253). Document, or route the rethrow through settled state only. - nits: the hook harness differs from the plan's
renderHookprescription without a stated deviation (add a PR-body note);conversation-earlier.tsx:92-101hand-writes the ref-merge idiom.
a613233 to
91acbf0
Compare
91acbf0 to
255751e
Compare
255751e to
4dfa1be
Compare
|
Thanks for the review. Here's what happened to each point. Fixed1. A new message could remove the oldest one while the reader was scrolled up (the verified race)
2. The two e2e specs had duplicate helpers (minor)
3. A loader that throws crashed inside the observer (minor)
Not changed, and why
Commit: |
TheGreatAxios
left a comment
There was a problem hiding this comment.
Review · Request changes
Adds useHistoryWindow and ConversationEarlier. The window holds its tail while a scroll has not been read yet, except when the trim anchor disappears in the same update that grows the list.
Findings
src/hooks/use-history-window.ts:72— A missingthroughid slides the window during render, before the scroll pass can freeze it. Whenstart.kind === "latest"andstart.throughis gone,startIndexreturnsitems.length - windowSize. The layout effect at:204-211only skips a latersetState; it does not undo that fallback, and the next snapshot freezes the slid start. With 300 items, a window of 120 trimmed throughr299, and a scroll that has moved but not yet been read, replacingr299and appendingr300in one update dropsr180. The pinned-start path already keeps the first surviving id of the previous window (:78-80).throughshould do the same when it is missing, instead of jumping to the new tail.
4dfa1be to
202d6df
Compare
The window remembers the last item it was trimmed through. When that id vanished in the same update as an append (an optimistic id swapped for the server's, plus the reply), the window slid by one during render. If the reader had just scrolled up, the next frame froze the slid start and the window's first item was gone. A missing trim id now falls back as a missing pin does: to the first item of the last window that still exists. At the bottom, the next commit trims back, as it does after a plain append.
202d6df to
e66abb5
Compare
|
Thanks for the review. It's fixed. 1. A missing
Commit: |
Only a scroll the reader made froze the window's start. A move the page made, such as a glide to an item, focus or find in page, left the start following the latest items, so the next appended item slid the window and unmounted the item the view had just been moved to. The window now freezes on any scroll snapshot that moved the view towards the top and ended away from the end. Content growing below a view that stays put leaves scrollTop unchanged, so the window still slides then.
The window start's fallback, used when the item it is kept by is gone, read the previous window from a ref during render. The ref was written by a layout effect, so it went stale while Activity hid the host: a start grown while hidden whose item was then deleted fell back to the window from before the host was hidden. The last slice now lives in the hook's state, recorded during render as atBottom already is, and the scroll listener reads the window's first item in the handler rather than in a state updater.
The onLoadEarlier doc said the hook never reports a failure. A loader that throws synchronously is reported through reportError, so that it does not escape the marker's observer callback; only a rejection is left to the host.
ConversationEarlier read ref from its props, which React 19 passes but React 18 strips, so a ref never reached the marker on React 18, the lower end of the peer range. It is now wrapped in forwardRef.
e66abb5 to
78de4e3
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.
|
78de4e3 to
d1672ad
Compare
Superseded by the completed stacked review; blocking fixes were addressed and the rebased stack is being landed.
|
Triage: In progress Triage is waiting for CI to finish before it decides. This comment shows the verdict once it has. |
useHistoryWindow keeps a window of the latest items, adds earlier ones as the reader nears the top, freezes while the reader has scrolled away and trims back once they return to the end. It works with history the host pages in asynchronously, and the item in view stays put within a pixel as earlier items load. ConversationEarlier marks the top with a loading state and a retry when a page brings nothing. The doc comment records why this windows rather than virtualizes, and its ceiling.
Stacked on #121: review only this commit. Part of #94.
Notes for review
stalledonly applies while the window starts at the first held item (the only place the host is asked); a failed or empty page never blocks revealing items already held above the window.renderHook(src/test/render-hook.tsx), becauserenderHookhas no wrapper option and the hook has to run inside a<Conversation>next to a mountedConversationContent. They still go only through the hook's public API.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 #100