Skip to content

feat(site): migrate agent chat scrolling - #28130

Draft
DanielleMaywood wants to merge 4 commits into
feat/agents-message-reconciliationfrom
feat/agents-message-scroller-declarative
Draft

feat(site): migrate agent chat scrolling#28130
DanielleMaywood wants to merge 4 commits into
feat/agents-message-reconciliationfrom
feat/agents-message-scroller-declarative

Conversation

@DanielleMaywood

@DanielleMaywood DanielleMaywood commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

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 bottom control. The integration uses the upstream component hierarchy without patches, bridges, or application-owned scroll correction.

Depends on #28079.

Implementation notes
  • Use MessageScroller.Provider, Root, Viewport, Content, Item, and Button directly.
  • Keep durable row IDs stable across pagination; the live assistant uses an ephemeral row until its durable message arrives.
  • Load additional history from MessageScroller's start-edge state, including underfilled transcripts and retry after a page error.
  • Remove inverse scrolling, sticky message copies, scroll refs, forced scroll commands, and react-infinite-scroll-component.

Generated by Coder Agents on behalf of @DanielleMaywood.

@DanielleMaywood

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +49 to +52
useEffect(() => {
if (shouldLoadEarlierMessages) {
void onFetchMoreMessages();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +1693 to +1696
const secondPrompt = canvas.getByText("Second prompt");
const offsetInViewport = () =>
secondPrompt.getBoundingClientRect().top -
viewport.getBoundingClientRect().top;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +39 to +40
const isAtHistoryStart =
!canScrollTowardStart && visibleMessageIds.length > 0;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@DanielleMaywood
DanielleMaywood force-pushed the feat/agents-message-scroller-declarative branch from f097c74 to 816ced5 Compare August 13, 2026 16:51
@DanielleMaywood

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +230 to +233
if (previousVisible !== undefined) {
return `read-file-group:after:${previousVisible.message.id}`;
}
return `read-file-group:through:${entry.mergedFrom[entry.mergedFrom.length - 1]}`;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +1139 to +1143
marker.getBoundingClientRect().top -
viewport.getBoundingClientRect().top -
markerOffset,
),
).toBeLessThan(4);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@DanielleMaywood

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +223 to +226
if (entry.mergedFrom === undefined) {
return `message:${entry.message.id}`;
}
return `read-file-group:through:${entry.mergedFrom[entry.mergedFrom.length - 1]}`;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +39 to +45
const isAtHistoryStart =
!canScrollTowardStart && (visibleMessageIds.length > 0 || !hasVisibleRows);
const shouldLoadEarlierMessages =
isAtHistoryStart &&
hasMoreMessages &&
!isFetchingMoreMessages &&
!hasFetchMoreError;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.

1 participant