fix(chat): match sub-agent card + composer to main-chat/Studio, freeze on stop - #3644
Conversation
…e on stop Fixes a batch of agentic-chat UI bugs surfaced in the agentic-case-processing example. The `invoke_agent` sub-agent card stays a card, but its insides now render with the same primitives as the main chat. - invoke_agent card: render the shared `Avatar` (image + initial fallback) and plumb child `agentName`/`avatarUrl` through the child stream so the avatar shows while the child is running, not only on completion. - invoke_agent card: replace the hand-rolled `<details>` Instructions/Thought process blocks with the main-chat `Reasoning` disclosure (Studio parity). - invoke_agent card: parse streamed child `inputText` -> `input` so child tool params get the same JSON syntax highlighting as top-level tool cards. - stop: freeze a still-running card + child rows to "Stopped" once the turn stops streaming without a terminal result (no forever-"Running", no perma skill shimmer). New `SkillTool` "stopped" state + `stopped` tool badge. - reasoning: drop the additive `mb-3` on `Reasoning.Root` that fought the flex gap (extra space below "Thought process" / below the avatar). - error banner: match Studio's inline retry banner — amber `warning` alert with a triangle icon and a filled "Try again" button; add real `--alert-*-border` tokens instead of a washed-out color-mix border. - composer: `+` opens the file dialog directly when attach is the only action (no 1-item dropdown); menu item renamed "Add photos & files". - composer: move pending-attachment pills inside the composer card, above the field (Studio), instead of floating as a separate row. - scroll-to-bottom: cleaner floating circle lifted off the transcript. Tests updated to the new anatomy; adds coverage for the stop-freeze. Verified against the agentic-case-processing example running on a local worktree.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (16)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughWalkthroughThe PR propagates child-agent identity through streams, freezes interrupted child tools, extracts streamed tool input, updates composer and error presentation, and adds dedicated alert border tokens. It also adjusts chat spacing, tests, Storybook themes, and API references. ChangesChild-agent stream and rendering
Chat composer and error presentation
Chat visual and alert styling
API reference synchronization
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: 🟡 Moderate · up to The PR is not merge-ready because duplicate declarations in the composition test file prevent the test suite from compiling; remove the duplicates before merging. Sequence Diagram(s)sequenceDiagram
participant AgentConfiguration
participant ChildAgentStream
participant InvokeAgentStream
participant ChildAgentCard
participant ChildTool
AgentConfiguration->>ChildAgentStream: publish identity metadata
ChildAgentStream->>InvokeAgentStream: deliver streamed values
InvokeAgentStream->>ChildAgentCard: provide identity and snapshot data
ChildAgentCard->>ChildTool: render streamed input and status
ChildAgentCard->>ChildTool: freeze active tool as Stopped when streaming ends
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63358a07c7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| default: "bg-[var(--alert-info-bg)] border-[var(--alert-info-border)]", | ||
| warning: "bg-[var(--alert-warning-bg)] border-[var(--alert-warning-border)]", | ||
| error: "bg-[var(--alert-error-bg)] border-[var(--alert-error-border)]", | ||
| success: "bg-[var(--alert-success-bg)] border-[var(--alert-success-border)]", |
There was a problem hiding this comment.
In apps whose CSS is generated from FRAMEWORK_CANDIDATES, these new border-[var(--alert-*-border)] utilities are never emitted because src/server/handlers/dev/framework-candidates.generated.ts:7768-7771 still only safelists the old border-[color-mix(...--alert-*-bg...)] alert border classes. The dev scanner and production static generation both seed framework component CSS from that generated list, so Veryfront alerts lose the intended border unless the host app independently uses the same classes. Regenerate the candidate file with the new alert border utilities.
AGENTS.md reference: AGENTS.md:L13-L14
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. framework-candidates.generated.ts was regenerated (deno run -A scripts/build/prebundle-client-scripts.ts) and now safelists the new utilities:
+ "border-[var(--alert-error-border)]",
+ "border-[var(--alert-info-border)]",
+ "border-[var(--alert-success-border)]",
+ "border-[var(--alert-warning-border)]",
- "border-[color-mix(in_oklch,var(--alert-error-bg),black_10%)]",
Regenerated again on the latest commit, so it also covers the scroll-button change. design-tokens.test.ts now asserts a --alert-*-border token for every fill in both color modes, so a fill added without its border token fails the suite rather than silently falling back to currentColor.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/react/components/chat/chat/components/tool-ui.tsx`:
- Line 416: Update the input resolution near the tool result construction to
prefer parsed non-empty existing.inputText over the empty {} placeholder in
existing.input, while preserving genuinely structured existing.input values. Add
a focused regression test covering the tool-input-start → tool-input-delta →
tool-output-available sequence and verify the streamed JSON appears in the
Parameters block.
In `@src/react/components/chat/chat/composition/chat-composer.test.tsx`:
- Line 343: Add focused tests in the chat composer test suite for attachment
flows: verify the direct picker works when only onAttach is provided, and verify
pending attachments render inside the card and can be removed. Reuse the
existing attachment button/menu selectors and preserve the current combined-menu
coverage.
In `@src/react/components/chat/chat/composition/chat-composer.tsx`:
- Around line 149-153: Update the menu trigger’s aria-label near the c.onAttach
and c.onSelectAttachment handling to use “Add photos & files” whenever
c.onAttach is available, while retaining “Add document” for document-only menus.
In `@src/react/components/chat/chat/composition/error-banner.test.tsx`:
- Line 20: Update the retry-label test to pass a custom value distinct from the
default “Try again,” such as “Retry now,” and assert that custom value in the
rendered HTML. Keep the test focused on the retryLabel override behavior.
In `@src/react/components/ui/alert.tsx`:
- Around line 14-18: Add focused regression coverage in alert.test.tsx for
alertVariants, asserting each default, warning, error, and success variant
renders its matching --alert-{variant}-border class. Keep the existing
foreground assertions and use the test’s established rendering pattern.
In `@storybook/.storybook/preview.css`:
- Around line 56-62: Update the [data-theme="dark"] block in preview.css to
override all four alert border tokens—warning, error, success, and info—with the
corresponding dark-theme values from design-tokens.ts. Keep the existing root
definitions and other dark-theme overrides unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8472debb-fdc9-4545-8316-eb5da3b00fab
📒 Files selected for processing (14)
src/agent/composition/composition.tssrc/chat/invoke-agent-stream.tssrc/react/components/chat/chat/components/empty-state.tsxsrc/react/components/chat/chat/components/reasoning.tsxsrc/react/components/chat/chat/components/skill-tool.tsxsrc/react/components/chat/chat/components/tool-ui.test.tsxsrc/react/components/chat/chat/components/tool-ui.tsxsrc/react/components/chat/chat/composition/chat-composer.test.tsxsrc/react/components/chat/chat/composition/chat-composer.tsxsrc/react/components/chat/chat/composition/error-banner.test.tsxsrc/react/components/chat/chat/composition/error-banner.tsxsrc/react/components/ui/alert.tsxsrc/react/components/ui/design-tokens.tsstorybook/.storybook/preview.css
- Regenerate framework-candidates.generated.ts so the new
`border-[var(--alert-*-border)]` alert utilities are safelisted (fixes the
"framework-candidates is stale" CI typecheck/lint failure and the alert
border tokens actually emit in generated CSS).
- tool-ui: prefer parsed streamed child `inputText` over the `{}` placeholder in
the tool-output-available branch (recovers params for the
start -> delta -> output sequence); add a focused regression test.
- composer: give the combined attach menu trigger the "Add photos & files"
accessible name when file-attach is available (a11y).
- alert: add a regression test asserting each variant's fill + border token pair.
- storybook: add dark-theme alert bg + border token overrides.
- error-banner test: use a distinct custom retry label so it verifies the override.
Splitting the composer's + attachment control into chat-input-attach.tsx keeps chat-composer.tsx under its ban-chat-antipatterns LOC ceiling (338 < 389) after the single-action + attachments-inside-card changes. Public API unchanged: ChatInputAttach is re-exported and still wired as ChatInput.Attach.
Extracting ChatInput.Attach into its own file changed the extracted framework component candidate set; regenerate so framework-candidates.generated.ts is current (fixes the stale-file CI typecheck/lint failure).
Updates veryfront/{agent,chat,ui}.md source line anchors / summaries changed by
this branch (alert doc, error-banner default label, Attach extraction). Fixes
the docs:api-reference:check step of lint:ci.
…its absence Review follow-ups on the sub-agent card work. The card inferred "Stopped" from `!isStreaming && !terminal`. That fires on every terminal path, so a turn killed by a stream error read as a user stop, and a tool paused on a human (`approval-requested` / `approval-responded`) read as stopped while it waited. Resolve the interruption from the chat status instead: still submitted/streaming means the turn only moved on to another message, `error` reports a failure, and an approval-gated tool keeps its own state. A provider that omits `status` falls back to the previous behavior. Also: - Publish the child identity once per run instead of rebuilding it for every streamed chunk. - A stopped skill row reads "Stopped loading skill: X", not a frozen "Loading". - Size the scroll-to-bottom button by padding so a custom icon is not clipped. - Move the attach menu's open state into the branch that renders the menu. - Document the `ErrorBanner.icon` slot move and the `Reasoning.Root` margin removal on the props themselves. Tests: the identity plumbing end to end (composition publish, contract parse and snapshot merge, card header render), the three interruption paths, the direct file-picker branch, pending pills inside the composer card, the stopped skill row, and an alert border token for every fill.
|
Review follow-ups pushed in b513bd6. Stop detection was the real one. Also fixed: identity was rebuilt and re-sent on every streamed chunk (now resolved once per run); a stopped skill row read "Loading skill: X" forever (now "Stopped loading skill: X"); the scroll button clipped a custom Tests for what had none. Breaking changes are now called out in the description - Rendered all seven changed states with the real tokens + storybook Tailwind build and checked the screenshot before saying they look right. Storybook dev is broken in this checkout ( |
`ci (lint)` caught an invalid cast: `ToolExecutionDataEvent` carries an
`unknown` index signature, so asserting on `event as { value: {...} }` does not
typecheck. Parse the published value with `parseInvokeAgentStreamValue`
instead, which also makes the assertion go through the real contract rather
than a shape the test asserts into existence. The snake_case case needed a
typed stream event to parse, so it no longer relies on the bare `data: {}`
default.
Worth noting for the next reader: this one error made `lint:test-typecheck`
split the repository-wide clean check into batches, and four unrelated files
(the RequestInit ones in workflow/transforms/use-chat) then reported failures
that never appear when the whole test surface is checked in one pass. They are
an artifact of the split, not new rot -- the gate is back to "51 grandfathered
files, 0 new" once this typechecks.
2 participants
Fixes a batch of agentic-chat UI bugs surfaced in the
agentic-case-processingexample. Theinvoke_agentsub-agent card stays a card, but its insides now render with the same primitives as the main chat (per design direction).Fixes
Avatar(image + initial fallback); plumb childagentName/avatarUrlthrough the child stream so it shows while running, not only on completionSkillToolstoppedstate +stoppedtool badgeToolCallcard; parse streamed childinputText→inputso params highlight like top-level tools<details>blocks with the main-chatReasoningdisclosure (Studio parity)mb-3onReasoning.Rootthat fought the flex gap (Studio has none)warningalert + triangle icon + filled "Try again" button; add real--alert-*-bordertokens instead of a washed-out color-mix border+opens the file dialog directly when attach is the only action (no 1-item dropdown); menu item renamed "Add photos & files" (ChatGPT parity)Notes on the Stop path
Server-side cancellation is already wired end-to-end (
useChat().stop()-> fetch abort ->run-stream.tsstreamcancel()->sessionManager.cancelRun-> runabortSignal->default-invoke-agent-toolthrowIfChildRunAborted+ forwardedabortSignal-> child fork -> childagent.stream({ abortSignal })). The visible "still Running" defect was purely client-side: already-received tool parts never get a terminal state after the fetch aborts. This PR fixes that client rendering; no server change needed.The client has no explicit "this child was cancelled" event, so the frozen state is inferred - but only where the inference holds.
resolveInterruptionreads the chat lifecycle rather than the mere absence of streaming:submitted/streaming(the turn moved on to another message)ready, tool never resolved (user pressed Stop)error(stream died mid-run)approval-requested/approval-responded(paused on a person)ChatContext/ noMessageContext(standalone card)A hand-built
ChatContextthat omitsstatusfalls back to the message's own streaming flag.Breaking changes (public API)
Small, but they change behavior for existing callers:
ErrorBannericonmoved from the retry button to the alert's leading icon slot. A caller passingiconnow sees it beside the message, not on the button. Put a button glyph inretryLabel.ErrorBannerdefaultretryLabelis "Try again" (was "Retry").+control is labelled "Add photos & files" (was "Add document"). Selectors and e2e specs keying on the old accessible name need updating.Reasoning.Rootno longer carriesmb-3. Both in-repo hosts are flex-gap containers, so they tighten to the intended spacing; a custom non-gap layout should passclassName.Testing
deno check+deno lintclean on all changed files;deno fmtapplied.fmt:check,docs:api-reference:check,lint:chat-composability,lint:chat-ratchets,lint:module-boundariesandlint:dependency-boundariespass.tool-ui,skill-tool,reasoning,error-banner,chat-composer,empty-state,invoke-agent-stream,composition,design-tokens,alert, plus the fullsrc/react/,src/chat/andsrc/agent/react/trees (214 files, 1545 steps, green).compositionpublishes name + avatar on every event (so the header is right while it runs, not only at the end) and honours the deprecatedavatar_url;invoke-agent-stream.test.ts(a new file - the module had no tests) covers identity parse, snapshot merge, and the merge keeping identity once the runtime stops repeating it;tool-uiasserts the streamed name beats the humanized id and the avatar renders mid-run.onAttach-only branch opening the picker with no dropdown, and pending pills rendering inside the composer card and removing.--alert-*-bordertoken for every fill in both color modes.node:async_hooksis externalized for the browser viasrc/react/head-collector.ts, which fails on unmodified stories too. Unrelated to this PR, worth its own issue.)Design source of truth cross-checked against
veryfront-studio(InvokeAgentTool,Reasoning,Alert,AgentAvatar). The light alert border tokens matchveryfront-studio/styles/styles.cssexactly (#F5BA67/#E06E7B/#6FB57C/#ADADAA); Studio has no dark-mode alert overrides, so the dark pair is derived here at 40% of the status color.Scope note
buildInvokeAgentStreamDataEventhas exactly one caller,runAgentAsStreamingTool(theagentAsTool/createInvokeAgentToolcomposition path). The hosteddefault-invoke-agent-toolmirrors child output separately and does not publish these events, so the avatar/name fix lands on the composition path this example uses, not on hosted child cards.Summary by CodeRabbit
New Features
Bug Fixes
Style