Skip to content

chore(frontend): bump A2A, AI SDK v6, streamdown v2; re-sync AI Elements#2389

Merged
malinskibeniamin merged 38 commits intomasterfrom
chore/bump-a2a-mcp-ai-elements
Apr 20, 2026
Merged

chore(frontend): bump A2A, AI SDK v6, streamdown v2; re-sync AI Elements#2389
malinskibeniamin merged 38 commits intomasterfrom
chore/bump-a2a-mcp-ai-elements

Conversation

@malinskibeniamin
Copy link
Copy Markdown
Contributor

@malinskibeniamin malinskibeniamin commented Apr 19, 2026

Summary

Upgrade the AI Agents chat toolchain to current majors and pull in the
vercel/ai-elements features the UI needs (MCP tool-approval flow,
conversation markdown export), preserving all Redpanda customisations.

Versions

Package Before After Kind
@a2a-js/sdk ^0.3.10 ^0.3.13 patch
ai ^5.0.101 ^6.0.168 major
streamdown ^1.4.0 ^2.5.0 major

@modelcontextprotocol/sdk is intentionally not bumped here — it
ships in a separate PR.

Why this is safer than it looks

  • ai v6. Our custom A2aChatLanguageModel implements LanguageModelV2.
    v6 keeps the V2 types exported and accepts them via the public
    LanguageModel alias (V3 | V2), so no adapter rewrite is needed.
    The one shape change we hit is LanguageModelUsage requiring
    inputTokenDetails / outputTokenDetails; useContextUsage was
    updated accordingly.
  • streamdown v2. Purely internal remark/katex plumbing changes;
    our response.tsx consumer is source-compatible.
  • ai-elements. Every upstream file was diffed against ours and
    deltas applied surgically. Redpanda-specific customisations
    (theming, token-count avatars, Badge variants, artifact metadata)
    are preserved verbatim.

What we adopted from upstream ai-elements

  • ConversationDownload + messagesToMarkdown helper
  • DynamicToolUIPart support and three new tool states:
    approval-requested, approval-responded, output-denied
  • Context provider memoisation and zero-token guards
  • Module-level cached motion components in shimmer

What we deferred (follow-up PRs)

  • Full prompt-input.tsx merge (heavy divergence, needs its own focused pass)
  • loader.tsx → shadcn/ui Spinner migration (upstream deprecation)
  • Vendoring ChainOfThought, Reasoning, Attachments,
    PromptInputActionAddScreenshot (not wired into the agents UI yet)

Risk and verification

  • Typecheck, unit, and integration suites all green; 19 new tests added
    for the adopted behavior (tool, conversation, use-context-usage).
  • No new lint errors introduced on touched files.
  • No call sites broken by v6 — adapter compiles unchanged; only the
    usage hook needed the new sub-object fields.

Screenshots

Captured via vitest browser mode (Chromium, headless, viewport 1440×900,
reduced-motion). Source tests live under
frontend/src/components/ai-elements/*.browser.test.tsx.

Tool states

State Screenshot
input-streaming input-streaming
input-available input-available
output-available output-available
output-error output-error
approval-requested (new) approval-requested
approval-responded (new) approval-responded
output-denied (new) output-denied
DynamicToolUIPart with toolName override (new) dynamic-toolname

ConversationDownload

State Screenshot
Idle conversation-download-idle
Hover conversation-download-hover

Context hover-card

State Screenshot
Zero tokens (guards hide sub-rows) context-zero
Populated usage with sub-objects context-populated

Shimmer

State Screenshot
Loading (frozen frame under reduced-motion) shimmer-loading

Test plan

  • bun install && bun start, open an AI Agents chat.
  • Trigger a tool call; confirm Tool card renders with Redpanda badge
    variants, duration, and toolCallId.
  • Trigger an MCP tool call requiring approval; confirm the three
    new states render (Awaiting Approval → Responded / Denied).
  • Open the Context hover-card; percentages and per-section costs
    still display, zero-token rows hidden.
  • Verify streaming text, artifact rendering, and task transitions.
  • If wired in, confirm ConversationDownload produces a well-formed
    markdown file.

🤖 Generated with Claude Code

@malinskibeniamin malinskibeniamin self-assigned this Apr 19, 2026
@malinskibeniamin malinskibeniamin requested review from a team, Mateoc, SpicyPete, jvorcak and yougotashovel and removed request for a team April 19, 2026 19:04
@malinskibeniamin malinskibeniamin added ai-jam AI Jam April 2026 related work frontend ui/ux team/ux labels Apr 19, 2026
@malinskibeniamin malinskibeniamin changed the title chore(frontend): bump A2A, MCP, AI SDK v6, streamdown v2; re-sync AI Elements chore(frontend): bump A2A, AI SDK v6, streamdown v2; re-sync AI Elements Apr 19, 2026
malinskibeniamin added a commit that referenced this pull request Apr 19, 2026
Captures 13 PNG screenshots of the adopted AI-Elements variants —
Tool card states (incl. the three newly adopted MCP approval flow
states and DynamicToolUIPart), ConversationDownload, Context
hover-card guards, and the Shimmer loading frame — rendered via
vitest browser mode and committed under docs/pr-screenshots/ for
reference in the PR body.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@malinskibeniamin
Copy link
Copy Markdown
Contributor Author

@claude please review this PR. Specifically focus on:

  • A2A adapter v6 compatibility — is the implementation correct given v6's LanguageModel = V3 | V2 alias, and are there any edge cases in doStream that might regress
  • The vendored ai-elements re-sync — did I preserve all Redpanda customisations correctly, any upstream features I missed adopting that would benefit the agents UI
  • useContextUsage shape — correctly populating both new inputTokenDetails/outputTokenDetails and deprecated top-level fields for the Context component that still reads legacy
  • Test coverage — are there critical-path behaviors still untested that we should cover in this PR vs follow-up

Flag any concerns even if minor. Don't worry about pre-existing lint noise.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 19, 2026

Claude finished @malinskibeniamin's task in 3m 16s —— View job


PR Review

Overall this is a well-executed bump. The v6 compatibility path is sound and the ai-elements re-sync is surgical. A few findings below, ordered by severity.


1. Bug — CopyButton title uses toolName instead of displayName (minor)

frontend/src/components/ai-elements/tool.tsx, line ~193

<CopyButton
  content={textToCopy}           // ← correctly uses displayName
 
    ? `Copy: ${toolName} (${toolCallId})`   // ← uses toolName (undefined for static tools)
    : `Copy: ${toolName}`}
/>

For static-typed tools (type="tool-get-weather", etc.) toolName is typed never and is undefined at runtime, so the tooltip renders "Copy: undefined" or "Copy: undefined (id-123)". The adjacent textToCopy already uses displayName correctly. The fix is a one-liner:

```tsx ? Copy: ${displayName} (${toolCallId}) : `Copy: ${displayName}`}


[Fix this →](https://claude.ai/code?q=In%20frontend%2Fsrc%2Fcomponents%2Fai-elements%2Ftool.tsx%2C%20on%20the%20CopyButton%20title%20prop%20inside%20ToolHeader%2C%20replace%20%60toolName%60%20with%20%60displayName%60%20so%20static%20tools%20don%27t%20render%20%22Copy%3A%20undefined%22.%20The%20line%20reads%3A%20title%3D%7BtoolCallId%20%3F%20%60Copy%3A%20%24%7BtoolName%7D%20(%24%7BtoolCallId%7D)%60%20%3A%20%60Copy%3A%20%24%7BtoolName%7D%60%7D%20and%20should%20use%20displayName%20instead.%20Branch%3A%20chore%2Fbump-a2a-mcp-ai-elements&repo=redpanda-data/console)

---

### 2. Pre-existing bug surfaced by this bump — non-streaming fallback sends two requests

**`frontend/src/components/ai-elements/../a2a/a2a-chat-language-model.tsx`, `doStream` ~line 290**

```ts
if (!clientCard.capabilities.streaming) {
  const nonStreamingResponse = await client.sendMessage(streamParams);  // ← request 1
  if ("result" in nonStreamingResponse) { simulatedStream = ...; }
  if ("error" in nonStreamingResponse) { /* FIXME: error */ }            // ← silent swallow
}

const response = client.sendMessageStream(streamParams);                  // ← request 2, always
return {
  stream: (simulatedStream || convertAsyncIteratorToReadableStream(response)).pipeThrough(...)
};

Two issues:

  • sendMessageStream is called unconditionally: even when simulatedStream is non-null and preferred, a live SSE request fires against the agent. The message is sent twice.
  • The error branch is silently ignored; simulatedStream stays null and the code falls through to the streaming path — sending a second message after the first already failed.

Not introduced in this PR, but the v6 bump is a good moment to fix (or at minimum throw in the error branch). Fix this →


3. Minor — ToolOutput error container class is a dead ternary

tool.tsx ~line 277

className={cn("rounded-md", errorText ? "bg-muted/50" : "bg-muted/50")}

Both branches are identical — the ternary is dead. Either collapse to a constant or apply a different class for error state (e.g. bg-destructive/10).


4. A2A adapter v6 compatibility — assessment

The core claim holds. A2aChatLanguageModel implements LanguageModelV2 from @ai-sdk/provider, which is unchanged in the v6 public package bump. The removal of activeTextIds / text-end in the flush handler is correcttext-end is no longer a valid LanguageModelV2StreamPart member in v6. The specificationVersion = 'v2' stays valid because LanguageModel = V3 | V2.

One asymmetry worth noting: doGenerate sets X-Redpanda-Stream-Tokens: true but doStream does not. If this is intentional (e.g. the streaming path doesn't use token headers), a quick comment would help future readers.

The provider-level usage shape ({ inputTokens, outputTokens, totalTokens }) in doGenerate / doStream is from @ai-sdk/provider V2 types, which don't require the new inputTokenDetails/outputTokenDetails — so those two methods are fine without the v6 sub-objects.


5. useContextUsage shape — assessment

The dual-write strategy is correct: legacy fields (reasoningTokens, cachedInputTokens) and new v6 sub-objects are both populated. The Context component reads the legacy fields today so the display is stable.

One question worth verifying: totalTokens = cumulativeInputTokens + cumulativeOutputTokens. If cumulativeReasoningTokens is not already included in cumulativeOutputTokens (i.e. tracked separately by the streaming handler), the total would undercount when a reasoning model is used. If it is included, the current formula is correct. Worth a quick sanity-check against how latestUsage is assembled in use-message-streaming.ts.


6. Test coverage — what's critical-path vs follow-up

Cover in this PR (high confidence in the untested surface):

  • doStream non-streaming fallback: the simulatedStream path + the error branch currently has a FIXME. Even a unit test that stubs client.getAgentCard + client.sendMessage and asserts the stream emits a finish part would catch the double-send regression.
  • Static-tool CopyButton title: add an assertion that the title attribute on the copy button matches the tool name, not "undefined".

Acceptable as follow-up:

  • doGenerate end-to-end (network-boundary, reasonable to defer)
  • ConversationDownload click-to-download DOM behavior (URL.createObjectURL mocking is noise-heavy)
  • ContextReasoningUsage / ContextCacheUsage zero-guard behavior (low-risk guard; simple visual regression)

Summary

Solid upgrade. The v6 story is correct, the ai-elements re-sync is careful, and the token-usage dual-write is a clean forward-compat pattern. The CopyButton title bug (#1) is the only production-visible defect; the double-send bug (#2) pre-existed but is worth addressing. Everything else is polish.

@malinskibeniamin
Copy link
Copy Markdown
Contributor Author

Fix screenshot paths. They don't render properly

malinskibeniamin added a commit that referenced this pull request Apr 19, 2026
Static tools passed `type="tool-foo-bar"` with no `toolName` prop, so the
copy button title rendered "Copy: undefined". Use `displayName` instead,
which resolves to `title ?? derivedName` — already the visible label.

Also fix the lint regressions introduced by the previous refactor:
formatter drift in event-handlers.ts and a dangling `continue` left
after removing the text-delta branch in use-message-streaming.ts.

Caught by @claude PR review on #2389.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
malinskibeniamin added a commit that referenced this pull request Apr 20, 2026
…eline

PostCSS' strict @import-first rule was rejecting globals.css under the
vitest browser-mode pipeline because @source came before @import. Moving
@source after the @imports satisfies PostCSS while still letting tailwind
v4 scan the streamdown bundle for utility classes. Verified the rsbuild
production build is unaffected.

Browser-setup now imports globals.css so vitest-browser screenshots
render with the real theme + tailwind utilities instead of browser
defaults. Unblocks showcase PNGs for PR #2389.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@malinskibeniamin
Copy link
Copy Markdown
Contributor Author

@claude review

@claude
Copy link
Copy Markdown

claude Bot commented Apr 20, 2026

Claude finished @malinskibeniamin's task in 2m 8s —— View job


PR Review

  • Gather context and read key changed files
  • Review A2A adapter v6 compatibility
  • Review vendored ai-elements re-sync
  • Review useContextUsage shape and dual-write strategy
  • Review test coverage

A2A Adapter v6 Compatibility

The specificationVersion = 'v2' + LanguageModelV2 implementation is correct. v6's LanguageModel = V3 | V2 alias accepts this without an adapter rewrite. The a2a-stream-mapper.ts extraction is clean — pure reducer with no side effects, well-typed state. The removal of text-end / activeTextIds bookkeeping from the flush handler is correct for v6 (those part types don't exist in LanguageModelV2StreamPart).

Bug — non-streaming fallback sends two requests (a2a-chat-language-model.tsx line 312)

This is the same pre-existing bug called out in the previous review, still unfixed:

// line 293-309
if (!clientCard.capabilities.streaming) {
  const nonStreamingResponse = await client.sendMessage(streamParams); // request 1
  if ("result" in nonStreamingResponse) {
    simulatedStream = new ReadableStream(...)
  }
  if ("error" in nonStreamingResponse) {
    // FIXME: error  ← silent swallow, falls through
  }
}

// line 312 — fires unconditionally
const response = client.sendMessageStream(streamParams); // request 2, always

Two problems remain:

  • sendMessageStream fires even when simulatedStream is set — message sent twice.
  • Error branch is swallowed; simulatedStream stays null and falls through to the streaming path, sending a second request after the first already failed.

The fix is two lines: move sendMessageStream into an else branch, and throw in the error branch instead of silently ignoring. Fix this →

Minor — X-Redpanda-Stream-Tokens header asymmetry

doGenerate sets X-Redpanda-Stream-Tokens: true (line 201); doStream does not. If this is intentional (e.g. the streaming endpoint ignores it), a brief comment would prevent future readers from "fixing" it.


Vendored ai-elements Re-sync

CopyButton title bug from prior review: confirmed fixed. Line 193 correctly uses displayName for both the toolCallId present/absent branches — no "Copy: undefined" regression.

Dead ternary in ToolOutput (tool.tsx line 277)

className={cn("rounded-md", errorText ? "bg-muted/50" : "bg-muted/50")}

Both branches are identical — the ternary is dead. Likely intended bg-destructive/10 (or similar) for the error state. Fix this →

ConversationDownload positioning assumption (conversation.tsx line 153)

The button renders with absolute top-4 right-4. This implicitly requires its parent to be position: relative. There's nothing enforcing this on the consumer side — a caller who forgets could see the button escape to a distant ancestor. Worth either noting in JSDoc or switching to a relative-positioned wrapper. Minor.

messagesToMarkdown drops non-text parts silently (conversation.tsx lines 102–106)

getMessageText filters to type === "text" parts only. Tool inputs, images, and data parts are silently dropped from the export. For tool-heavy conversations (the primary use case here) the markdown export would be incomplete. This should at minimum be noted as a known limitation in the JSDoc, or tool parts could be serialized as fenced JSON blocks. Fix this →

Shimmer: Module-level motionComponentCache and motion.create caching is correct — avoids creating motion instances during render. memo wrapper is appropriate.

New tool states (approval-requested, approval-responded, output-denied): All three have Redpanda badge variants (warning-inverted, info-inverted, destructive-inverted respectively). Badge and icon choices are sensible. ✓


useContextUsage Shape

Dual-write strategy is correct. Legacy top-level fields (reasoningTokens, cachedInputTokens) are populated for ContextReasoningUsage/ContextCacheUsage which still read them. v6 sub-objects (inputTokenDetails, outputTokenDetails) are also populated for forward compat. The useMemo dependency array is complete and the stable-reference test confirms it.

totalTokens formula (use-context-usage.ts line 42):

totalTokens: usage.cumulativeInputTokens + usage.cumulativeOutputTokens,

In the AI SDK v6, reasoning tokens are a sub-category of output tokens — outputTokenDetails.reasoningTokens is included in outputTokens, so this formula is correct assuming cumulativeOutputTokens already includes reasoning. The existing test asserts totalTokens === 150 for input=100, output=50 which confirms the expected interpretation. If cumulativeReasoningTokens were ever tracked as additional (not included in output), this would undercount — worth a comment to lock in the invariant. Low risk but worth the comment.


Test Coverage

Strong coverage in this PR:

  • a2a-stream-mapper.test.ts — all TaskState branches, isFirstChunk guard, includeRawChunks, finalizeStream. Well structured.
  • a2a-chat-language-model.test.tsmapFinishReason and getResponseMetadata exhaustively covered.
  • use-context-usage.test.tsx — legacy fields, v6 sub-objects, memoisation stability, and recompute-on-change all covered.

Gaps worth addressing in this PR:

  1. doStream non-streaming fallback path — the simulatedStream path and the FIXME error branch have zero test coverage. At minimum, a test that stubs getAgentCard to return { capabilities: { streaming: false } } and sendMessage to return a result would catch the double-send regression.

  2. messagesToMarkdown with non-text parts — a test asserting what happens with a message containing only tool parts would document the current behavior and prevent silent regressions.

Acceptable as follow-up: doGenerate end-to-end, ConversationDownload DOM behavior, ContextReasoningUsage / ContextCacheUsage zero-guard visual regression.


Summary

The v6 compatibility story is solid, the ai-elements re-sync is surgical and the useContextUsage dual-write is a clean forward-compat pattern. The CopyButton title fix from the prior review is confirmed applied. The main concerns remaining are the pre-existing double-send bug in the non-streaming fallback (still unfixed, worth a targeted fix before merge) and the silent tool-part drop in messagesToMarkdown. The dead ternary is cosmetic.

malinskibeniamin added a commit that referenced this pull request Apr 20, 2026
malinskibeniamin added a commit that referenced this pull request Apr 20, 2026
…itting

The 13 showcase PNGs used in PR #2389's description now live on the
orphan branch `pr-screenshots-2389` and are referenced via raw URLs, so
the main merge history stays free of binary assets. The browser tests
still regenerate PNGs locally under `frontend/docs/pr-screenshots/`;
that directory is gitignored so re-runs do not re-commit them.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
malinskibeniamin and others added 6 commits April 20, 2026 15:12
Align Redpanda Console with the current stable versions of the AI
toolchain. All four packages are dependencies of the AI Agents chat
feature:

- @a2a-js/sdk       0.3.10 → 0.3.13 (patch: stream/task fixes, no API change)
- @modelcontextprotocol/sdk 1.26.0 → 1.29.0 (minor additive)
- ai               5.0.101 → 6.0.168 (new major)
- streamdown        1.4.0 → 2.5.0  (new major)

ai v6 ships a new @ai-sdk/provider v3 package that introduces
LanguageModelV3, but keeps the v2 types as a backwards-compat export and
accepts either in the public LanguageModel alias, so our custom
A2aChatLanguageModel (which implements LanguageModelV2 from
@ai-sdk/provider) continues to work without modification.

streamdown v2 only changes internals (remark-cjk/katex plumbing) and is
source-compatible with how response.tsx consumes it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ai v6 reshaped LanguageModelUsage: inputTokenDetails and
outputTokenDetails are now required sub-objects, and the pre-v6
top-level reasoningTokens/cachedInputTokens fields are deprecated but
still present. Our useContextUsage hook returned only the legacy fields,
which tsgo flagged as missing required members once the alias pulled in
the v6 shape.

Populate both the new sub-objects and the legacy fields so the Context
component (which still reads from the deprecated top-level fields)
continues to work today and any future upstream re-sync that switches
to the new fields sees consistent values.

Add a unit test covering the mapping, useMemo stability, and recompute
behaviour.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ments Tool

Re-sync the Tool component with vercel/ai-elements upstream to pick up
support for the ai v6 tool-approval flow. ai v6 introduced three new
ToolUIPart states used by the MCP approval loop:

- approval-requested  → "Awaiting Approval" (shield/alert)
- approval-responded  → "Responded"         (shield/check)
- output-denied       → "Denied"            (X)

It also added DynamicToolUIPart for provider-resolved (e.g. MCP) tools,
where the tool name isn't encoded in the part type and must be passed
via a toolName prop. ToolHeader now accepts a discriminated union:
either a static `tool-*` type with no toolName, or `dynamic-tool` with
toolName.

Redpanda-specific customisations are preserved: our own Badge variants
(success-inverted / info-inverted / destructive-inverted /
neutral-inverted / warning-inverted), the durationMs display, the
toolCallId copy button, and the deepParseJson-aware ToolInput /
ToolOutput behaviour.

Covered by new tests in tool.test.tsx for all seven states plus the
dynamic-tool naming, title override, duration formatting, and
toolCallId rendering.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pull the upstream ConversationDownload component and messagesToMarkdown
helper from vercel/ai-elements into our vendored conversation.tsx. The
AI Agents chat feature will use these to give users a one-click export
of an agent conversation as Markdown.

Kept all Redpanda customisations: import path to
components/redpanda-ui/components/button, overflow-y-auto (intentional
over upstream's overflow-y-hidden so the content scrolls), simpler
ConversationContent classes, no dark:bg-background override on the
scroll button (our Button variant handles theming), and the
Conversation.displayName.

Covered by a unit test of messagesToMarkdown across role capitalisation,
multi-message joining, multi-part text concatenation, non-text parts,
custom formatters, and the empty case.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pick up three small upstream improvements from vercel/ai-elements while
preserving every Redpanda customisation (import paths, Redpanda Button
variants, intentional class overrides):

- shimmer.tsx: cache motion components in a module-level Map so that
  `motion.create(element)` is never called during render. Stops churning
  component identity across every message tick.
- context.tsx: memoise the React context value so consumers only
  re-render when a usage field actually changes, and guard
  ContextInputUsage / ContextOutputUsage against a zero-token state so
  the HoverCard never shows "Input —" lines before a response has
  arrived.
- image.tsx: prefix the unused destructured `uint8Array` arg with an
  underscore, matching the upstream biome convention.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@modelcontextprotocol/sdk upgrade will ship in its own PR so this one
stays scoped to the AI toolchain (A2A + ai v6 + streamdown v2 +
ai-elements re-sync).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
malinskibeniamin and others added 3 commits April 20, 2026 15:13
Move the JSON-RPC error parser and its five regex constants out of
use-message-streaming.ts into chat/utils/parse-a2a-error.ts. The hook
imports from the new module; the existing describe('parseA2AError', ...)
suite in use-message-streaming.test.ts updates its import path only.
Function signature and behavior are unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Dedicated test.each suite targeting the extracted util directly. Covers
the SDK's two serialized JSON-RPC error shapes (SSE-wrapped and
streaming), invalid Data: JSON, empty/missing code fallbacks, non-Error
input types, and prefix-stripping behavior. The existing hook-level
describe('parseA2AError') in use-message-streaming.test.ts stays put
since it exercises the same util through the full streamMessage path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…itting

The 13 showcase PNGs used in PR #2389's description now live on the
orphan branch `pr-screenshots-2389` and are referenced via raw URLs, so
the main merge history stays free of binary assets. The browser tests
still regenerate PNGs locally under `frontend/docs/pr-screenshots/`;
that directory is gitignored so re-runs do not re-commit them.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@malinskibeniamin malinskibeniamin force-pushed the chore/bump-a2a-mcp-ai-elements branch from 994fa1e to 34b9465 Compare April 20, 2026 13:16
Comment thread frontend/package.json Outdated
Comment thread .gitignore Outdated
Comment thread frontend/src/components/ai-elements/context.browser.test.tsx Outdated
Comment thread frontend/src/components/ai-elements/context.tsx Outdated
Comment thread frontend/src/components/ai-elements/conversation.browser.test.tsx
Comment thread frontend/src/components/ai-elements/image.tsx
Comment thread frontend/src/components/ai-elements/tool.browser.test.tsx Outdated
@malinskibeniamin
Copy link
Copy Markdown
Contributor Author

@claude review again

@claude
Copy link
Copy Markdown

claude Bot commented Apr 20, 2026

Claude finished @malinskibeniamin's task in 3m 9s —— View job


PR Review

  • Read key changed files
  • Address author's inline review comments
  • Re-check previously flagged bugs (CopyButton title, dead ternary, double-send)
  • Verify new items: .gitignore, copyright years, NaN check, image.tsx change, error handling
  • Post final review

Response to Inline Review Comments

.gitignore line 27 — "Not needed. Remove this"
Agreed. The entry frontend/docs/pr-screenshots/ was added in this PR with a comment explaining screenshots are hosted on an orphan branch. But since they're never committed to the working tree, the gitignore is redundant and should go. Fix this →


Copyright years — "Use 2026 copyright everywhere"
All new files added in this PR carry Copyright 2025. The existing repo convention (see ai-agent-list-page.browser.test.tsx which uses Copyright 2026) is to use the current year. Every new file in this PR needs a year bump. Affected:

  • ai-elements/context.browser.test.tsx, context.test.tsx
  • ai-elements/conversation.browser.test.tsx, conversation.test.tsx
  • ai-elements/shimmer.browser.test.tsx, shimmer.test.tsx
  • ai-elements/tool.browser.test.tsx, tool.test.tsx
  • ai-elements/response.test.tsx
  • a2a/a2a-stream-mapper.ts, a2a-stream-mapper.test.ts
  • a2a/a2a-chat-language-model.test.ts
  • a2a/chat/utils/parse-a2a-error.ts, parse-a2a-error.test.ts
  • a2a/chat/hooks/use-context-usage.test.tsx

Fix this →


context.tsx line 156 — "Number.isNaN() or similar for completeness"
Current guard: maxTokens > 0 ? usedTokens / maxTokens : 0. This handles zero but not NaN — if either value is NaN (e.g. the usage hook returns undefined after a partial stream), the ratio would still be NaN and slip past the guard. Safer:

const usedPercent = Number.isFinite(maxTokens) && maxTokens > 0
  ? usedTokens / maxTokens
  : 0;

Fix this →


conversation.browser.test.tsx line 77 — "Use ADP UI equivalent naming convention"
The new tests define a local const shot = (name: string) => page.getByTestId(...).screenshot(...). The established pattern in this repo is toMatchScreenshot (see remote-mcp-inspector-tab.browser.test.tsx line 166). These serve different purposes — toMatchScreenshot is visual regression; the shot helper here intentionally writes to a path for PR documentation screenshots. The author is right that the local helper name shot is non-standard. Options: rename to captureScreenshot for clarity, or extract a shared utility in browser-test-utils.tsx. Given the helper is consistent across all four new browser test files, a shared utility would be the right fix.


image.tsx line 11 — "Why no longer using uint8 array?"
The change is correct and safe. The before (spreading all props) would have passed uint8Array as an attribute to the DOM <img> element, causing a React "Unknown prop" warning. The after destructures it as uint8Array: _uint8Array (underscore-prefixed to silence lint) to remove it from ...props. The image still renders via src={data:${mediaType};base64,${base64}}uint8Array was never used for rendering. This is an upstream ai-elements fix picked up correctly. No functional regression.


a2a-chat-language-model.test.ts line 26 — "Backend compatibility / defensive checks"
The @a2a-js/sdk bump is 0.3.10 → 0.3.13 (patch). The test file covers mapFinishReason and getResponseMetadata which depend on the A2A wire event shapes (kind, status.state, taskId, etc.). The patch bump should not break these. One gap: the test file doesn't cover the contextId injection at line 281 (message.contextId = options.providerOptions?.a2a?.contextId). If a caller passes a non-string contextId (or the field name changes in 0.3.13), this cast would silently corrupt the message. A defensive typeof ... === 'string' check and a test asserting contextId is only set when present would harden this.


use-context-usage.test.tsx line 49 — "tokenlens package improvements"
The current hook manually maps raw cumulative counts to LanguageModelUsage fields. If the project uses or plans to use tokenlens (a token-budget visualization utility), the integration point would be at the useContextUsage return shape — not here. The current dual-write strategy is already the right approach; tokenlens would consume the output of this hook, not replace it. No immediate change needed here, but wiring tokenlens in a follow-up would be cleaner if there's an existing package in the workspace.


parse-a2a-error.test.ts line 109 — "MCP error code mapping"
This is a valuable follow-up item. parseA2AError currently handles A2A/JSON-RPC error codes but not MCP protocol errors. MCP uses its own error codes (e.g. -32601 Method Not Found, -32602 Invalid Params, JSON-RPC 2.0 standard codes). A parseMCPError function that maps these to user-readable messages (similar to how ConnectRPC codes are mapped) would complete the error surface. The current parseA2AError would need to be extended or a parallel parseMCPError added. This is appropriately a follow-up given MCP SDK changes are deferred to a separate PR.


Still-Unfixed Issues from Previous Reviews

1. Double-send bug — non-streaming fallback (a2a-chat-language-model.tsx line 312)

This was flagged in both prior reviews and remains unaddressed:

// line 312 — fires unconditionally even when simulatedStream is set
const response = client.sendMessageStream(streamParams);

When clientCard.capabilities.streaming === false, sendMessage is called (request 1) to build simulatedStream, then sendMessageStream fires anyway (request 2), sending the message twice. The "error" branch at line 306 is also still a silent swallow — simulatedStream stays null and falls through to the streaming path after the first request already errored.

Fix: move const response = client.sendMessageStream(streamParams) into an else branch; throw in the error case. Fix this →

2. Dead ternary in ToolOutput (tool.tsx line 277)

className={cn("rounded-md", errorText ? "bg-muted/50" : "bg-muted/50")}

Both branches are identical — error and success output containers look the same. Likely intended bg-destructive/10 for the error branch. Fix this →


New Observation

messagesToMarkdown silently drops tool parts (conversation.tsx line 102)

getMessageText filters to type === "text" only. In a tool-heavy agents conversation the exported markdown would contain only the prose, stripping every tool invocation and result. Given that this is the primary use-case (agents conversations), the export would be misleading. Minimum fix: add a JSDoc noting the limitation. Better: serialize tool parts as fenced JSON blocks.


Summary

CopyButton title fix is confirmed applied. The v6 adapter and useContextUsage dual-write remain sound. The main items to address before merge: copyright year sweep (all new files), .gitignore entry removal, Number.isFinite guard in context, and the still-present double-send + dead ternary bugs from prior reviews. The shot naming convention, messagesToMarkdown tool-part gap, and MCP error mapping are reasonable follow-ups.

@malinskibeniamin
Copy link
Copy Markdown
Contributor Author

@claude review

@claude
Copy link
Copy Markdown

claude Bot commented Apr 20, 2026

Claude finished @malinskibeniamin's task in 2m 39s —— View job


PR Review

  • Read key changed files and new commits
  • Re-check previously flagged bugs (double-send, dead ternary)
  • Review new commits since last review (safeUsedPercent, isKnownA2AEvent, MCP error codes, captureScreenshotFrame)
  • Assess remaining concerns and overall merge readiness

What's been resolved since the last review

All inline review comments are addressed:

  • .gitignore entry removed (09624c5)
  • ✅ Copyright year swept to 2026 across all 18 added files (3dfe7e0)
  • safeUsedPercent helper guards against NaN, ±Infinity, and negatives in all three context.tsx divide sites (fc0e4e9)
  • captureScreenshotFrame shared utility added to browser-test-utils.tsx, all four browser test files now use it (8b3498c)
  • isKnownA2AEvent type guard + UnsupportedFunctionalityError thrown for unrecognised stream event kinds (3a84337) — good: silent-drop manifests as a stalled chat, this surfaces a diagnostic error instead
  • parseA2AError extended with human-readable title + actionable hint for every JSON-RPC 2.0, A2A-extension, and MCP error code (fcae41e) — the code table is complete and the A2AErrorBlock wiring is clean

Still-unfixed — both flagged in all three prior reviews

1. Double-send in non-streaming fallback (a2a-chat-language-model.tsx lines 291–316)

if (!clientCard.capabilities.streaming) {
  const nonStreamingResponse = await client.sendMessage(streamParams); // request 1
  if ("result" in nonStreamingResponse) { simulatedStream = new ReadableStream(...); }
  if ("error" in nonStreamingResponse) {
    // FIXME: error  ← still silently swallowed
  }
}

// line 312 — fires unconditionally regardless of simulatedStream
const response = client.sendMessageStream(streamParams); // request 2, always

Both problems remain:

  • A non-streaming-capable agent receives the message twice (once via sendMessage, once via sendMessageStream).
  • A sendMessage error is swallowed and execution falls through to the streaming path, sending a second request after the first already errored.

Two-line fix: move sendMessageStream into an else branch; throw in the error case. Fix this →

2. Dead ternary in ToolOutput (tool.tsx line 277)

<div className={cn("rounded-md", errorText ? "bg-muted/50" : "bg-muted/50")}>

Both branches are identical — error and success containers look the same. Intended bg-destructive/10 (or similar) for the error branch. Fix this →


New minor observation

JSON_RPC_DATA_REGEX doesn't handle nested objects (parse-a2a-error.ts line 32)

export const JSON_RPC_DATA_REGEX = /Data:\s*(\{[^}]*\})/i;

[^}]* stops at the first }, so nested data like {"field":{"reason":"expired"}} would be truncated to {"field": and JSON.parse would throw — the catch silently returns undefined. The test suite only covers flat payloads. Non-blocking since data is informational, but worth noting.


Architecture note (resolved concern)

a2aEventToV2StreamParts emitting only metadata parts (no text-delta) is correct by design. use-message-streaming.ts passes includeRawChunks: true and routes all content through the raw chunk path — text and artifacts come from status-update.message.parts / artifact-update, not AI SDK text-delta parts. The comment on line 330 of use-message-streaming.ts documents this invariant.


Summary

Strong improvement round. Every inline review comment is addressed and the new additions (isKnownA2AEvent, parseA2AError enrichment, safeUsedPercent, captureScreenshotFrame) are well-executed. The only remaining blockers are the two issues above — the double-send bug has been flagged in every review and is the only production-visible correctness issue; the dead ternary is cosmetic. Both are small, targeted fixes.

malinskibeniamin and others added 5 commits April 20, 2026 19:15
A non-streaming agent previously received the message twice — once via
the sendMessage branch (whose result populated simulatedStream) and
again unconditionally via sendMessageStream. The FIXME error branch
also silently fell through instead of surfacing the error.

Move sendMessageStream into an else branch so the two transport paths
are mutually exclusive, and throw on sendMessage error so failures
propagate instead of being shadowed by the subsequent stream.

Flagged by @claude PR reviewer on #2389.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The ternary on the result container had bg-muted/50 in both branches,
so errors looked identical to successful results. Switch the error
branch to bg-destructive/10 and add a regression test.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The [^}]* fragment stopped at the first closing brace so nested data
got truncated and JSON.parse silently threw, losing the structured
payload. Anchor to end-of-string with the /s flag and add a
nested-object test case.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ort selection

Pull the streaming-vs-blocking decision out of doStream into a pure
function that takes a structural A2ATransport. Lets us cover the
double-dispatch fix with deterministic unit tests (4 cases: streaming
path, blocking success, blocking error, undefined capability).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- hoist regex literal to module scope for useTopLevelRegex
- rename inline generator to reuse across assertions for useAwait
- apply numeric separators via biome --write

No behavior change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
malinskibeniamin and others added 2 commits April 20, 2026 20:35
The renderHook-based tests in remote-mcp.test.tsx and the streaming
progress UI tests in remote-mcp-inspector-tab.test.tsx triggered
component state updates outside of act() when mutateAsync settled or
when onprogress callbacks were fired directly, producing React warnings
in test stderr. Wrap the triggering awaits in act(async () => ...) to
flush them deterministically. No behavior change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@malinskibeniamin malinskibeniamin merged commit 2581fb3 into master Apr 20, 2026
17 checks passed
@malinskibeniamin malinskibeniamin deleted the chore/bump-a2a-mcp-ai-elements branch April 20, 2026 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-jam AI Jam April 2026 related work frontend team/ux ui/ux

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants