Repository navigation
refactor(site/src/pages/AgentsPage/components/ChatElements/tools): add ToolCall.Scrollable - #30648
Conversation
…d ToolCall.Scrollable Moves the tool-call ScrollArea boilerplate (focusable labelled region, thin scrollbar, follow-to-end for logs) into ToolCall.Scrollable. Callers keep their own box styling.
…ate refs from follow effect
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 519619360f
ℹ️ 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".
|
|
||
| it("follows new content while the reader is at the end", () => { | ||
| renderLog(); | ||
| expect(log().scrollTop).toBe(200); |
There was a problem hiding this comment.
Replace DOM-geometry assertions with visual coverage
Both new cases make scrollTop the asserted outcome while mocking the element's geometry, so they verify the implementation in jsdom rather than an allowed non-visual behavior. The frontend testing contract assigns rendered layout to Storybook/Pixel and prohibits implementation-detail assertions; move this scroll-follow coverage to visual coverage, or assert a callback or state outcome instead.
AGENTS.md reference: site/AGENTS.md:L73-L75
Useful? React with 👍 / 👎.
Adds
ToolCall.Scrollableand moves the 13 tool-callScrollAreablocks in the Agents chat onto it. The component owns the shared behavior: a labelled, keyboard-focusable scroll region, the thin tool-call scrollbar, and an opt-infollowEndthat keeps the newest content in view while the reader is at the end. It is otherwise unstyled, so each consumer keeps its own border, margin, height cap, and padding (unchanged frommain).followEndreplaces the hand-rolledviewportRef+useLayoutEffect(followLogEnd, [logs])copies inWorkspaceBuildLogSectionandWorkspaceAgentLogSection. It watches the content with aResizeObserverinstead of a data dependency, so it also catches growth from children that rerender on their own (smoothly streamed text), which is what the ACPwait_agenttranscript in #30561 needs. One visible side effect: onmainthe build log can settle 1px short of the end when content grows after mount withoutlogschanging; it now lands exactly at the end, so Pixel may flag the start/stop workspace stories by 1px.Refs #30561
Decision log
ScrollAreausages migrated.TerminalOutputis left alone: its border lives on an outer wrapper with the command header, and enabling follow-to-end forexecute/process_outputis a behavior change worth deciding separately. Non-toolScrollAreausers in AgentsPage (sidebar, search, debug panel, settings,Response) are out of scope; the shared#/components/ScrollAreais unchanged.max-h-64plustallandpaddedprops. Dropped because consumers differ (diffs need 80vh,EditFilesToolrelies on parentspace-y, ACP wantsmax-h-96and its own card), so callers passclassName/viewportClassNamelike they did withScrollArea. The scrollbar width stays in the component because every consumer used the same value.ResizeObserveron a content wrapper (only rendered whenfollowEndis set) driving the existingfollowLogEndhelper, rather than a dependency prop, so streamed children that update internally are followed. Growth while the reader is at the end scrolls; after the reader scrolls up, their place is kept.ReasoningDisclosurepreview uses a nativeoverflow-y-autobox that unconditionally snaps to the bottom; it could usefollowEnd. The ACPwait_agentcard in feat(site): support ACP subagents on the frontend #30561 could adoptToolCall.ScrollablewithfollowEnd.ToolCall.test.tsxfor follow and keep-place (confirmed the follow test fails with the observer callback disabled). Before/after Storybook screenshots of 162 tool stories showed no differences beyond shimmer/background noise and the 1px build-log end position described above.