mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(site/src/pages/AgentsPage): keep agent chat sticky truncation in sync as the transcript grows (#26714)
## Problem In `/agents`, the sticky user-message truncation sometimes does not update as new content arrives. While pinned to the bottom with the transcript overflowing, several messages (or a streaming response) can land and the sticky bubble keeps a stale clip height, overflowing and overlapping the content below it. It only snaps back once you scroll manually. ## Root cause `StickyUserMessage` recomputes its clip height (`--clip-h`) and push-up `top` in an `update()` driven by three triggers: a scroll listener, a window-resize listener, and a `ResizeObserver` meant to catch the transcript growing. The observer watched `scroller.firstElementChild`, but in `ChatScrollContainer` the scroller's first child is the `flex-1 basis-0` spacer that pins content to the bottom, not the content wrapper. That spacer collapses to `0px` the moment the transcript overflows (exactly when truncation engages) and then never resizes again, so the observer goes silent. The other triggers do not cover this case either: in `flex-col-reverse` the `scrollTop` stays at `0` while pinned to the bottom, so no scroll event fires as content grows. The result is a stale `--clip-h` until the next manual scroll. ## Fix - Observe the real content wrapper instead of the collapsing spacer. The wrapper is tagged with `data-chat-scroll-content` (it contains both the committed timeline and the streaming live tail), and the sticky code resolves it via `sentinel.closest(...)`, falling back to the previous node only if the marker is absent. - Recompute the scroller geometry (`scrollerTop`/`scrollerHeight`) inside `update()` on every tick instead of caching it at effect setup, so the clip and push-up math cannot drift when the scroller moves or resizes without a window resize (for example the composer growing). This also removes the now-redundant `onResize` handler. No change to the sticky visuals or the rAF throttling. ## Testing - New story `StickyUserMessageClipUpdatesWhilePinned` grows the transcript while pinned (no scroll dispatched) and asserts the clip tracks the new geometry, plus structural guards that the observed node is the content marker and not the `aria-hidden` spacer. - Verified as a true regression guard: with the fix reverted the new story fails; with the fix it passes. The existing `StickyUserMessagePinsOnScroll` is unaffected. - `biome check`, `tsc -p .`, React Compiler check, emdash check, and `vitest --project=storybook` for this stories file all pass (48/48). <details> <summary>Decision log</summary> - Considered centralizing the per-message scroll/resize/observer wiring into a single coordinator in `ConversationTimeline` (it already centralizes sentinels) to cut N observers/listeners down to one. Deferred as a follow-up to keep this PR a surgical, low-risk fix; this change alone resolves the staleness. - Chose a semantic `data-chat-scroll-content` marker over reusing the `chat-timeline-wrapper` test id so runtime behavior does not depend on a test-only attribute. The marker sits on the wrapper that contains both the timeline and the live tail, so streaming growth is observed too. - Kept the `scroller.firstElementChild` fallback so other `ConversationTimeline` consumers and stories without the marker keep working. </details> --- Filed via Coder Agents on behalf of @kylecarbs.
This commit is contained in:
Kyle Carberry
@@ -1380,6 +1380,133 @@ export const StickyUserMessagePinsOnScroll: Story = {
|
||||
},
|
||||
};
|
||||
|
||||
// Tall user messages interleaved with verbose assistant replies. The height
|
||||
// gives the sticky clip room to shrink as the transcript grows, and the
|
||||
// volume overflows the 600px scroll decorator.
|
||||
const buildTallStickyConversation = (count: number): TypesGen.ChatMessage[] => {
|
||||
const messages: TypesGen.ChatMessage[] = [];
|
||||
for (let i = 1; i <= count; i++) {
|
||||
const role: TypesGen.ChatMessageRole = i % 2 === 1 ? "user" : "assistant";
|
||||
const text =
|
||||
role === "user"
|
||||
? Array.from(
|
||||
{ length: 6 },
|
||||
(_, line) =>
|
||||
`Question ${Math.ceil(i / 2)} paragraph ${line + 1}: keep this user message tall enough to clip.`,
|
||||
).join("\n\n")
|
||||
: `Detailed answer ${Math.floor(i / 2)}. `.repeat(12);
|
||||
messages.push(buildMessage(i, role, text));
|
||||
}
|
||||
return messages;
|
||||
};
|
||||
|
||||
const stickyClipUpdateStore = buildStoreWithMessages(
|
||||
buildTallStickyConversation(30),
|
||||
);
|
||||
|
||||
/**
|
||||
* Regression guard: the sticky truncation must stay in sync as the
|
||||
* transcript grows while the user is pinned to the bottom.
|
||||
*
|
||||
* The clip height is recomputed by a scroll handler, a window-resize
|
||||
* handler, and a ResizeObserver on the transcript. The observer used to
|
||||
* watch `scroller.firstElementChild`, which is the aria-hidden flex spacer
|
||||
* that pins content to the bottom. That spacer collapses to 0px once the
|
||||
* transcript overflows and then stops emitting resize callbacks, so several
|
||||
* messages arriving while pinned left the clip stale until the next manual
|
||||
* scroll and the bubble overflowed. The fix observes the real content
|
||||
* wrapper tagged with `data-chat-scroll-content`.
|
||||
*
|
||||
* This story grows the transcript while pinned and asserts the clip tracks
|
||||
* the new geometry without any scroll event.
|
||||
*/
|
||||
export const StickyUserMessageClipUpdatesWhilePinned: Story = {
|
||||
parameters: { chromatic: { disableSnapshot: true } },
|
||||
decorators: scrollStoryDecorators,
|
||||
render: () => <StoryAgentChatPageView store={stickyClipUpdateStore} />,
|
||||
play: async ({ canvasElement }) => {
|
||||
stickyClipUpdateStore.replaceMessages(buildTallStickyConversation(30));
|
||||
stickyClipUpdateStore.setChatStatus("completed");
|
||||
const canvas = within(canvasElement);
|
||||
const scrollContainer = canvas.getByTestId("scroll-container");
|
||||
|
||||
await waitForScrollOverflow(scrollContainer);
|
||||
|
||||
// The observed transcript node must be the real content wrapper, not
|
||||
// the aria-hidden flex spacer that collapses to 0px on overflow.
|
||||
const contentMarker = scrollContainer.querySelector(
|
||||
"[data-chat-scroll-content]",
|
||||
);
|
||||
expect(contentMarker).not.toBeNull();
|
||||
const spacer = scrollContainer.firstElementChild;
|
||||
expect(spacer).not.toBe(contentMarker);
|
||||
expect(spacer?.getAttribute("aria-hidden")).toBe("true");
|
||||
|
||||
// Every sticky sentinel lives inside the observed content node, so a
|
||||
// resize of that node reflects transcript growth.
|
||||
const sentinels = scrollContainer.querySelectorAll("[data-user-sentinel]");
|
||||
expect(sentinels.length).toBeGreaterThan(0);
|
||||
for (const sentinel of sentinels) {
|
||||
expect(contentMarker?.contains(sentinel)).toBe(true);
|
||||
}
|
||||
|
||||
// At scrollTop 0 the newest message is pinned to the bottom. The most
|
||||
// recent user message whose sentinel sits just above the top edge is
|
||||
// the bubble pinned at the top and actively clipped.
|
||||
const scrollerRect = scrollContainer.getBoundingClientRect();
|
||||
const pinnedSentinel = Array.from(sentinels)
|
||||
.reverse()
|
||||
.find(
|
||||
(sentinel) =>
|
||||
sentinel.getBoundingClientRect().top < scrollerRect.top - 4,
|
||||
) as HTMLElement | undefined;
|
||||
expect(pinnedSentinel).toBeDefined();
|
||||
if (!pinnedSentinel) {
|
||||
return;
|
||||
}
|
||||
const pinnedContainer = pinnedSentinel.nextElementSibling as HTMLElement;
|
||||
|
||||
const MIN_CLIP_HEIGHT = 72;
|
||||
const readClip = () =>
|
||||
Number.parseFloat(pinnedContainer.style.getPropertyValue("--clip-h")) ||
|
||||
0;
|
||||
const measureScrolledPast = () =>
|
||||
scrollContainer.getBoundingClientRect().top -
|
||||
pinnedSentinel.getBoundingClientRect().top;
|
||||
const expectedClip = () =>
|
||||
Math.max(
|
||||
pinnedContainer.offsetHeight - measureScrolledPast(),
|
||||
MIN_CLIP_HEIGHT,
|
||||
);
|
||||
|
||||
const scrolledPastBefore = measureScrolledPast();
|
||||
expect(scrolledPastBefore).toBeGreaterThan(4);
|
||||
// Stay in the clipping regime (not a near-full-height bubble).
|
||||
expect(pinnedContainer.offsetHeight).toBeLessThanOrEqual(
|
||||
scrollContainer.clientHeight * 0.75,
|
||||
);
|
||||
expect(scrollContainer.scrollTop).toBe(0);
|
||||
|
||||
// Grow the transcript at the newest end. While pinned, scrollTop stays
|
||||
// at 0 so no scroll event fires; only the content ResizeObserver can
|
||||
// drive the recompute.
|
||||
stickyClipUpdateStore.replaceMessages([
|
||||
...getStoreMessages(stickyClipUpdateStore),
|
||||
buildMessage(31, "assistant", "Freshly streamed reply. ".repeat(80)),
|
||||
buildMessage(32, "assistant", "More freshly streamed reply. ".repeat(80)),
|
||||
]);
|
||||
|
||||
// The pinned bubble is now further above the top edge. Its clip must
|
||||
// follow the new geometry. Before the fix it stayed stale (matching
|
||||
// the pre-growth scrolledPast) until a manual scroll.
|
||||
await waitFor(() => {
|
||||
expect(scrollContainer.scrollTop).toBe(0);
|
||||
expect(measureScrolledPast()).toBeGreaterThan(scrolledPastBefore + 10);
|
||||
expect(Math.abs(readClip() - expectedClip())).toBeLessThanOrEqual(2);
|
||||
});
|
||||
},
|
||||
};
|
||||
|
||||
/**
|
||||
* Selecting the Terminal tab in the sidebar must move keyboard focus into
|
||||
* the terminal so typing goes there, not the chat input.
|
||||
|
||||
@@ -894,7 +894,7 @@ export const AgentChatPageView: FC<AgentChatPageViewProps> = ({
|
||||
onFetchMoreMessages={onFetchMoreMessages}
|
||||
messageCount={messageCount}
|
||||
>
|
||||
<div className="px-4">
|
||||
<div className="px-4" data-chat-scroll-content>
|
||||
<ChatPageTimeline
|
||||
store={store}
|
||||
persistedError={persistedError}
|
||||
|
||||
@@ -843,10 +843,13 @@ const StickyUserMessage = memo<{
|
||||
const MIN_HEIGHT = 72;
|
||||
const STICKY_TOP = 8;
|
||||
|
||||
let scrollerTop = scroller.getBoundingClientRect().top;
|
||||
let scrollerHeight = scroller.clientHeight;
|
||||
|
||||
const update = () => {
|
||||
// Read the scroller geometry on each tick. Caching it goes
|
||||
// stale when the scroller moves or resizes without a window
|
||||
// resize (for example the composer growing), which skews the
|
||||
// clip height and push-up math.
|
||||
const scrollerTop = scroller.getBoundingClientRect().top;
|
||||
const scrollerHeight = scroller.clientHeight;
|
||||
const fullHeight = container.offsetHeight;
|
||||
|
||||
// Skip sticky behavior for messages that take up
|
||||
@@ -904,12 +907,6 @@ const StickyUserMessage = memo<{
|
||||
};
|
||||
updateFnRef.current = update;
|
||||
|
||||
const onResize = () => {
|
||||
scrollerTop = scroller.getBoundingClientRect().top;
|
||||
scrollerHeight = scroller.clientHeight;
|
||||
update();
|
||||
};
|
||||
|
||||
// Throttle to one update per animation frame so we don't
|
||||
// do redundant work on high-refresh-rate displays.
|
||||
let rafId: number | null = null;
|
||||
@@ -921,12 +918,21 @@ const StickyUserMessage = memo<{
|
||||
});
|
||||
};
|
||||
|
||||
// Re-run the visual update when the scrollable content height
|
||||
// changes (e.g. streaming responses growing the transcript).
|
||||
// In flex-col-reverse, scrollTop stays at 0 when pinned to
|
||||
// bottom so no scroll event fires — but the content wrapper
|
||||
// resizes and this observer catches that.
|
||||
const contentEl = scroller.firstElementChild as HTMLElement | null;
|
||||
// Re-run the visual update when the transcript height changes,
|
||||
// for example a streaming response or several messages arriving
|
||||
// at once. In flex-col-reverse the scrollTop stays at 0 while
|
||||
// pinned to the bottom, so no scroll event fires; observing the
|
||||
// content wrapper catches that growth instead.
|
||||
//
|
||||
// The scroller's firstElementChild is the flex spacer that pins
|
||||
// content to the bottom. It collapses to 0px once the transcript
|
||||
// overflows and then stops emitting resize callbacks, which is
|
||||
// exactly when truncation is active, so observe the real content
|
||||
// node (an ancestor of the sentinel) and fall back to the spacer
|
||||
// only when the marker is absent.
|
||||
const contentEl =
|
||||
sentinel.closest<HTMLElement>("[data-chat-scroll-content]") ??
|
||||
(scroller.firstElementChild as HTMLElement | null);
|
||||
let contentRafId: number | null = null;
|
||||
const contentObserver = contentEl
|
||||
? new ResizeObserver(() => {
|
||||
@@ -940,7 +946,7 @@ const StickyUserMessage = memo<{
|
||||
contentObserver?.observe(contentEl!);
|
||||
|
||||
scroller.addEventListener("scroll", onScroll, { passive: true });
|
||||
window.addEventListener("resize", onResize);
|
||||
window.addEventListener("resize", update);
|
||||
update();
|
||||
// Set immediately — both --clip-h and --overlay-ready are
|
||||
// applied before the browser paints since we're in a
|
||||
@@ -948,7 +954,7 @@ const StickyUserMessage = memo<{
|
||||
container.style.setProperty("--overlay-ready", "1");
|
||||
return () => {
|
||||
scroller.removeEventListener("scroll", onScroll);
|
||||
window.removeEventListener("resize", onResize);
|
||||
window.removeEventListener("resize", update);
|
||||
contentObserver?.disconnect();
|
||||
container.style.removeProperty("--overlay-ready");
|
||||
if (rafId !== null) cancelAnimationFrame(rafId);
|
||||
|
||||
Reference in New Issue
Block a user