feat(site): migrate agent chat scrolling - #28130
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d1d36b188
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| useEffect(() => { | ||
| if (shouldLoadEarlierMessages) { | ||
| void onFetchMoreMessages(); | ||
| } |
There was a problem hiding this comment.
Move history loading out of the effect
This invokes the react-query paging function from useEffect whenever derived scroller state changes, coupling server requests to effect replay and dependency identity; for example, a replay can call fetchNextPage again before the isFetchingMoreMessages prop reflects the first request. Trigger pagination from the scroller's start-edge event or another explicit query control instead of chaining the fetch from an effect.
AGENTS.md reference: site/AGENTS.md:L22-L23
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not fixing. The race described here cannot occur: the effect only re-runs when the scroller store values change, those stores publish only from DOM events and rAF callbacks (never synchronously from the fetch call), and isFetchingMoreMessages flips on the very next render. Even a hypothetical second invocation is absorbed by React Query, which dedupes an in-flight fetchNextPage.
The suggested alternative does not exist: @shadcn/react@0.3.0 exposes no start-edge event. Its only event surface is the raw Viewport onScroll DOM handler, which cannot express the underflow case (a transcript shorter than the viewport sits at the start edge without any scroll event firing), so the effect would be needed regardless. Syncing the query cache from the scroller state is the integration point this API shape forces, and the comment on the effect now documents that.
Generated by Coder Agents on behalf of @DanielleMaywood.
| const secondPrompt = canvas.getByText("Second prompt"); | ||
| const offsetInViewport = () => | ||
| secondPrompt.getBoundingClientRect().top - | ||
| viewport.getBoundingClientRect().top; |
There was a problem hiding this comment.
Replace the geometry-based scrolling assertion
This story decides success using exact getBoundingClientRect() offsets and a 160-pixel threshold, so font loading, viewport dimensions, or browser layout differences can fail the test even when prompt navigation works correctly. FE10 requires tests to query and assert observable semantic behavior; expose the scroller's visible-message state or spy on the navigation operation instead of asserting DOM geometry.
AGENTS.md reference: site/AGENTS.md:L26-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f097c74 by removing the geometry assertion. The story still verifies the chevrons render, enable and disable at the transcript ends, and that clicking one fires without error; the scroll itself is MessageScroller behavior covered by the library's own browser tests. The suggested alternatives were all weaker: spying on scrollToMessage would assert on a mock, and the library exposes no semantic marker for "row is at the top of the viewport".
Generated by Coder Agents on behalf of @DanielleMaywood.
| const isAtHistoryStart = | ||
| !canScrollTowardStart && visibleMessageIds.length > 0; |
There was a problem hiding this comment.
Continue paging when the rendered page is empty
When the newest API page contains only entries filtered by buildDisplayMessages, such as metadata-only, provider tool-result-only, or empty non-user messages, no MessageScroller.Item is measured and visibleMessageIds remains empty. Even if hasMoreMessages is true, this condition permanently suppresses onFetchMoreMessages, leaving a blank transcript while older visible messages remain unreachable; distinguish measurement readiness from an empty rendered page and keep fetching filtered-empty pages.
AGENTS.md reference: site/AGENTS.md:L16-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f097c74. ConversationTimeline now reports whether the loaded messages produced any rendered rows (onVisibleRowsChange, reported during render, no new effect), and the paging guard is now !canScrollTowardStart && (visibleMessageIds.length > 0 || !hasVisibleRows). A page that filters down to zero rows keeps paging instead of leaving the transcript blank with older history unreachable.
Generated by Coder Agents on behalf of @DanielleMaywood.
| for (const [index, entry] of displayMessages.entries()) { | ||
| const { message } = entry; | ||
| let key: string; | ||
| const key = getChatMessageRenderKey(message); |
There was a problem hiding this comment.
Keep merged read-file row IDs stable across pagination
When a 50-message boundary splits a consecutive read_file assistant group, buildDisplayMessages initially represents the visible group using its first loaded message, then switches to the newly prepended group's earlier first message after the next page arrives. Deriving the item key from that changing message ID removes the old first item instead of producing a clean prepend, defeating MessageScroller's reading-position preservation and potentially jumping the historical transcript; use an identity stable when older group members are added.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f097c74. Merged read_file groups no longer key off a group member. They carry their member IDs (mergedFrom) and the row key comes from the visible entry before the group (read-file-group:after:<id>), which is stable under both prepending (older members join the group) and live appending (new members arrive mid-stream). A group at the very start of loaded history has no preceding entry and falls back to its newest member's ID; that key remounts once if the next older page extends the group, which is the only residual instability and is not avoidable from the loaded entries alone. A key-stability test covering the prepend-split case was added in timelineRows.test.ts.
Generated by Coder Agents on behalf of @DanielleMaywood.
f097c74 to
816ced5
Compare
816ced5 to
8c4b929
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c4b929654
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (previousVisible !== undefined) { | ||
| return `read-file-group:after:${previousVisible.message.id}`; | ||
| } | ||
| return `read-file-group:through:${entry.mergedFrom[entry.mergedFrom.length - 1]}`; |
There was a problem hiding this comment.
Keep start-of-page merged row keys stable
When the initial page begins inside a consecutive read_file group, the fallback keys that row as through:<newest member>, but fetching the preceding page can reveal an ordinary visible row and switch the same group to after:<preceding row>. The fresh evidence is that the current fallback still takes this unstable branch whenever the predecessor has not been loaded yet. MessageScroller then sees a removal and insertion rather than a clean prepend, which can lose the user's reading position; use an identity that does not depend on whether the preceding page is loaded.
AGENTS.md reference: site/AGENTS.md:L16-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8d0badb, and simplified in the process: merged groups now always key off the newest member (read-file-group:through:<id>), which pagination never changes for an existing group, so the key no longer depends on whether the preceding page has loaded. The after: branch is gone entirely. It is never worse on live appends either: a mid-turn append changes the newest member under both schemes, and that remount happens at the live bottom where the row content is changing anyway.
Generated by Coder Agents on behalf of @DanielleMaywood.
| const [reportedVisibleRows, setReportedVisibleRows] = useState<boolean>(); | ||
| if (hasVisibleRows !== reportedVisibleRows) { | ||
| setReportedVisibleRows(hasVisibleRows); | ||
| onVisibleRowsChange?.(hasVisibleRows); |
There was a problem hiding this comment.
Stop updating the parent during timeline render
When an empty or fully filtered transcript first renders, hasVisibleRows differs from the local sentinel and this callback invokes the parent's setHasVisibleRows while ConversationTimeline is still rendering. React reports this as a cross-component render-phase update and schedules cascading renders on each visible-to-empty transition; derive the visibility in the parent or report it outside the child render path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8d0badb. Confirmed against the React 19 source that the parent setter invoked during the child render is a cross-component render-phase update. The callback chain is gone: ChatPageTimeline now derives hasVisibleRows directly with buildDisplayMessages(parsedMessages).length > 0, and the prop, sentinel state, and render-phase block in ConversationTimeline are deleted.
Generated by Coder Agents on behalf of @DanielleMaywood.
| marker.getBoundingClientRect().top - | ||
| viewport.getBoundingClientRect().top - | ||
| markerOffset, | ||
| ), | ||
| ).toBeLessThan(4); |
There was a problem hiding this comment.
Replace the exact scroll geometry assertion
In this newly added pagination story, the assertion requires the marker's bounding-rectangle offset to remain within four pixels. Font loading, viewport sizing, and browser layout rounding can violate that threshold even when MessageScroller correctly preserves the reading position, making CI flaky; assert an observable scroller behavior or navigation operation instead of DOM geometry.
AGENTS.md reference: site/AGENTS.md:L26-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Keeping this one. It is the only regression guard for the behavior this PR exists to fix (scroll position preservation across prepends). The environment is pinned: headless Chromium, bundled preloaded fonts, fixed-height container, stable scrollbar gutter. The library's own restore epsilon is 0.5px and its failure modes are row-height-scale (hundreds of px), so there is no plausible marginal-failure path that lands between 4px and a real regression. Every faithful oracle for "the scroll yanked" is a geometry assertion; the semantic alternatives either do not exist in the library's API or are strictly weaker (a yank to a nearby historical anchor keeps the marker visible, so visibility assertions miss it).
Generated by Coder Agents on behalf of @DanielleMaywood.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d0badbcb8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (entry.mergedFrom === undefined) { | ||
| return `message:${entry.message.id}`; | ||
| } | ||
| return `read-file-group:through:${entry.mergedFrom[entry.mergedFrom.length - 1]}`; |
There was a problem hiding this comment.
Keep singleton read-file keys stable when groups expand
When an initial page starts with exactly one visible read_file assistant entry, it has no mergedFrom and is keyed as message:<id>; loading the preceding page can reveal an adjacent older read, turning the same row into a group keyed read-file-group:through:<id>. MessageScroller then processes a removal and insertion rather than a clean prepend, which can lose the user's preserved reading position. Fresh evidence beyond the previously resolved key findings is the current single-entry early return, while the added regression test starts with an already merged two-member group.
AGENTS.md reference: site/AGENTS.md:L16-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1663956. Singleton read_file runs now carry mergedFrom too, so they key as read-file-group:through:<id> from the start and the key survives a prepend that extends the run. Nothing else reads mergedFrom, and a synthetic singleton renders identically through the existing single-tool branch. The regression test gained a singleton variant.
Generated by Coder Agents on behalf of @DanielleMaywood.
| const isAtHistoryStart = | ||
| !canScrollTowardStart && (visibleMessageIds.length > 0 || !hasVisibleRows); | ||
| const shouldLoadEarlierMessages = | ||
| isAtHistoryStart && | ||
| hasMoreMessages && | ||
| !isFetchingMoreMessages && | ||
| !hasFetchMoreError; |
There was a problem hiding this comment.
Wait for initial store hydration before paging history
When opening a chat whose first API page reports more history, the chat store is still empty during the first child render because useChatStore hydrates REST messages in the parent's passive effect. The child's paging effect runs first with hasVisibleRows=false, so this condition deterministically fetches page two even when the initial 50-message page would overflow and the user is still at the bottom. This is distinct from the previously rebutted effect-replay race: the fresh evidence is the initial parent/child passive-effect ordering, so pagination should be gated on completed store hydration.
AGENTS.md reference: site/AGENTS.md:L22-L23
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1663956. The escape hatch now distinguishes "store not yet hydrated" from "loaded page fully filtered": the parent passes hasFilteredOutRows = parsedMessages.length > 0 && buildDisplayMessages(parsedMessages).length === 0. Pre-hydration (raw empty) no longer triggers an eager fetch; a fully filtered page (raw non-empty, zero visible rows) still keeps paging.
Generated by Coder Agents on behalf of @DanielleMaywood.
| <Button | ||
| variant="outline" | ||
| size="icon" | ||
| className="absolute bottom-4 left-1/2 z-10 -translate-x-1/2 rounded-full bg-surface-primary shadow-md transition-all duration-200 data-[active=false]:translate-y-2 data-[active=false]:opacity-0" |
There was a problem hiding this comment.
Remove the hidden scroll button from accessibility navigation
When the transcript is already at the bottom, this control is hidden only with opacity-0 and a translation. The rendered button has no corresponding inactive tabIndex, aria-hidden, or conditional removal, so keyboard or screen-reader users can still encounter an invisible “Scroll to bottom” control; remove it from the tab order and accessibility tree while inactive.
AGENTS.md reference: site/AGENTS.md:L199-L204
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not fixing: the library already handles this. MessageScrollerButton sets inert and tabIndex={-1} when there is nothing to scroll toward (upstream components.tsx, asserted by upstream's own tests, and documented: "it sets inert, uses tabIndex={-1}, and exposes data-active=\"false\" so inactive scroll controls do not create extra focus stops"). The inert attribute removes the button from both the tab order and the accessibility tree, and our Button spreads all props onto the element. The opacity/translation classes are only the visual half of the pattern; upstream's own docs example styles the inactive state the same way (inert:opacity-0).
Generated by Coder Agents on behalf of @DanielleMaywood.
Replace the Agents chat's inverse scroll container and sticky user-message overlays with the stock MessageScroller from
@shadcn/react@0.3.0.Transcript rows now render as direct MessageScroller Items with stable server-backed identities. Only the latest active user turn is a scroll anchor, older history preserves the reading position when prepended, and the package owns follow mode, prompt navigation, and the
Scroll to bottomcontrol. The integration uses the upstream component hierarchy without patches, bridges, or application-owned scroll correction.Depends on #28079.
Implementation notes
MessageScroller.Provider,Root,Viewport,Content,Item, andButtondirectly.react-infinite-scroll-component.Generated by Coder Agents on behalf of @DanielleMaywood.