refactor(chat-core): dispatch ChatPage's transcript rows through the renderer registry

[CrysisDeu]

Problem / Motivation

ChatPage.renderMessage was a ~200-line if (m.role === …) chain deciding which component draws each transcript row — a second, hand-maintained copy of the decision app-sdk/messageRenderers already makes for SideChat, ChatPane and ChatEmbed. The two had to be kept in step by hand and by a parity test, and the defect class that bought that test — mcp_oauth wired in the registry and rendered as raw text in the main chat — lived in the gap between them.

Why it matters

RFC chat-core extraction P5-a: the first cut of the ChatPage slim-down, render-dispatch layer only. With this, every chat surface resolves rows through the one registry mechanism (resolveRenderer over mergeRenderers), so an SDK-default role registered once renders on every surface by construction and the parity test stops being the only thing standing between "registered" and "renders in the main chat". What this PR does not yet unify: the dashboard's rich rows still live in two host lists — ChatPage's new one and pages/chat/transcriptRenderers.tsx's createTranscriptRenderers (ChatPane's). They now share ids (tool, file, nudge, recovery_inject, thinking_block, error, workflow_completion, subagent_completion) so they can converge by override, and that convergence — spreading the factory into ChatPage's list — is the next P5 cut, deliberately not bundled here so this PR stays a pure dispatch move. It also gives the later cuts (turn grouping, composer band) a page whose row dispatch is already data, not code.

What changed (motivation → approach → change)

Dispatch (ChatPage.tsx). renderMessage is now resolveRenderer(m, chatPageRenderers).render(m, ctx), with chatPageRenderers = mergeRenderers([...]) memoized on the same dependency list the old callback carried (UI-state deps deliberately stay in it so settled turns re-render on a behaviour change and the changed identity breaks through memo(TurnBlock)). The page's chrome rides as host entries:

• Entries that override a registry default by id (the shared row, plus page chrome): tool (WorkflowRunCard / SubagentRunCard / ToolCallLine with disclosure state, MCP app panel hand-off, trailing-group animation), file, nudge (Loop button gated on the row's own loop), stop_event, error (Continue gated on continuable && interrupted && lastErrorIdx), notice, mcp_oauth (the connectionsUiOn gate), subagent_completion (session/folder/panel hand-offs).
• Page-only entries, each documented in the parity test's PAGE_ONLY_ENTRY_IDS: thinking_block (ThinkingBlock with page disclosure; same id as transcriptRenderers), recovery_inject (RecoveryCard, resolveInjectCard decides — shared with the registry's inject row; same id as transcriptRenderers), permission (undrawn; grouped), workflow_completion, hidden_invisible_assistant, and bubble — the user / inject / assistant row with fork, pin, footer, regenerate, variants and search-scope chrome (one entry for three roles, so it keeps its own id rather than overriding user + assistant + inject; the three defaults stay in the merged list but are never reached).
• The page also overrides undrawn with a narrower role set than the SDK's (REASONING_ROLES + queued): system / done are left unclaimed on purpose so they keep the if-chain's fall-through (the bubble) rather than taking the SDK's null — this store never carries them, but the safety net is the one the old code had.

Entry order is the if-chain's precedence order (thinking_block → tool → file → nudge → stop_event → recovery_inject → error → notice → permission → undrawn → mcp_oauth → workflow completion → sub-agent completion → hidden invisible assistant → bubble). Roles none of these claim fall to the remaining registry defaults (tool_lifecycle for raw wire shapes the store normalizes away; the never-reached user/assistant/inject); a role nobody claims renders as the bubble by reference — the memo returns { renderers, fallback: bubble } and renderMessage does (entry ?? bubbleRenderer).render(...) — which is what the if-chain's fall-through did, so an unknown role stays visible. (Round 1 indexed the merged list's tail for this, which is the SDK undrawn default and would have hidden an unregistered role; all four lanes caught it, and the parity test now pins the by-reference fallback.) The per-row MessageRenderContext carries the page's index/messages/running/key/onFileOpen/hideCardOwnedOAuth; wrapper/row are minimal keyed wrappers only a non-overridden default could reach.

No row's output changes. Every branch body moved verbatim into its entry (const i = ctx.index; const key = ctx.key at the top); the unparseable-file case still falls to the bubble by calling it explicitly.

Parity contract (chatRolesParity.contract.test.ts), narrowed. It now asserts (1) ChatPage imports mergeRenderers/resolveRenderer and its renderer block contains no if (m.role === …) or switch (m.role) dispatch; (2) the unclaimed-role fallback is the bubble by reference and the page's undrawn leaves system/done unclaimed; (3) every host entry id either overrides a default or is in the documented page-only list, and that list carries no stale ids; (4) roles the page's remaining chrome logic names (footer rule, queue rail, last-error lookup, permission grouping) are registry-claimed or chrome-allowlisted — the allowlist shrinks from {queued, permission, streaming} to {permission} (the registry claims the other two); (5) no stale allowlist entry. The old "registry-only roles are orphaned in ChatPage" assertion is gone: the page consumes the registry, so an SDK-default role is covered by construction. The chrome check still keys on the .role === '…' extractor, as before; the structural assertion now also rejects a switch, so a dispatch cannot come back in either spelling.

Four source-shape tests re-pinned to the equivalent entry instead of the old branch text: ChatPage.mcpOAuth, NoticeCard (registry wiring), RecoveryCard (card entry ordered before the bubble), invisibleText (skip entry ordered before the bubble).

Not in this PR. No component, style, layout or behaviour change; no change to app-sdk/messageRenderers itself; the turn grouper, virtualizer and composer band are untouched (later P5 cuts). The merge of ChatPage's host list with createTranscriptRenderers is the next cut (its header comment now says so instead of "ChatPage draws its rows from a local role chain").

Tests

• chatRolesParity.contract.test.ts (rewritten, 5): registry-driven dispatch (no if-chain, no switch); bubble fallback by reference + undrawn leaves system/done unclaimed; host-entry ids override-or-documented (+ no stale docs); remaining chrome literals claimed-or-allowlisted; no stale allowlist.
• ChatPage.mcpOAuth, NoticeCard, RecoveryCard, invisibleText: re-pinned to entry shapes.
• Compatibility evidence — unmodified: all other ChatPage* suites (63 files, 578 tests) pass as-is, plus the 15 non-ChatPage suites that read ChatPage.tsx as source and messageRenderers.test.ts / AppSdkMessageRenderersCov80 (366 tests across 18 files).

Local gates: tsc -b, eslint on changed files, check-i18n-strings (0 added). The full vitest suite runs in CI.

Manual verification

N/A — a pure dispatch refactor with verbatim branch bodies and deterministic coverage; no rendered output changes.

Why no screenshot: every entry's body is the old branch's body moved verbatim; the components, classes, keys and props each row receives are unchanged, so there is no pixel to show.

Related Issues

• RFC chat-core extraction (docs(rfc): record the chat-core extraction RFC and its progress #8690 commits the RFC), P5-a. P1 (feat: chat-core phase 1 — role-parity contract + side panel native composer #5128) introduced the registry and the parity test this PR narrows.

Checklist

• Single commit, conventional title
• Contract test narrowed, not deleted; shape tests re-pinned
• No user-visible strings added
• Render dispatch layer only

Contribution License Agreement

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 7e823e02c5bff871a874869efb28b117a24d4275 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 7e823e0

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 7e823e02c5bff871a874869efb28b117a24d4275: <one-sentence reason>

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 7e823e02c5bff871a874869efb28b117a24d4275 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

A real double-dispatch defect class, closed at the mechanism level with verbatim-moved bodies, pinned fallback semantics, and an explicitly staged convergence path — sound and proportionate.

Watch

• Interim state has two dashboard host lists sharing renderer ids with different bodies (ChatPage's inline list vs createTranscriptRenderers); the shared-id contract only pays off if the next P5 cut lands — if it stalls, id collision without convergence is a drift trap worse than the old if-chain.
• The registry has no "decline and continue" mechanism, so the file entry hard-wires its fall-through by calling bubble.render(m, ctx) directly; expect this pattern to recur when the host lists merge, at which point it belongs in the registry API rather than in per-entry closures.

[DESIGN-REVIEWED] 7e823e0

[github-actions]

UX Review (Fable 5) — ✅ PASS

UX-level review of 7e823e02c5bff871a874869efb28b117a24d4275 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

This PR is a pure internal refactor: ChatPage's transcript row dispatch moves from an inline if-chain to the shared renderer registry, with each row type carried over verbatim (same components, same precedence order, same fall-through-to-bubble behavior for unclaimed roles). I verified:

• No user-facing strings are added or changed — all diff text is code comments and test assertions.
• Every row renderer (ToolCallLine, NudgeCard, ErrorCard, NoticeCard, RecoveryCard, workflow/subagent completion cards, MCP OAuth banner, bubble) keeps its exact props and gating logic, including the error card's Continue affordance and the nudge card's Loop button.
• The rewritten parity contract tests pin that an unclaimed role still renders as a visible bubble rather than vanishing — the one drift that could have produced a user-visible regression (silent row loss).
• No screenshots, no new surfaces, no layout/copy/flow changes.

UX-Verdict: PASS

Zero rendered-pixel change: every transcript row keeps its exact component, chrome, and fall-through behavior, and the parity tests pin unclaimed roles to stay visible.

[UX-REVIEWED] 7e823e0

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 7e823e02c5bff871a874869efb28b117a24d4275 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I've read the contract, the intent file, the full patch, and the surrounding repository (app-sdk/messageRenderers.tsx, pages/chat/transcriptRenderers.tsx, and the consumers of both). Here is the review.

First-Principles-Verdict: PASS

Deletes ChatPage's hand-maintained copy of the registry's dispatch decision, closing the mcp_oauth defect class at its cause; every item is declared.

What this change ships

Intent: make the main chat resolve transcript rows through the one registry every other surface uses, so a role registered once renders everywhere. This is a FIX (of a defect class, previously held shut only by a parity test).

1. ChatPage rows dispatch through the shared registry; the ~200-line if-chain is deleted — justified, cause-level
2. An SDK-default role registered once now renders in the main chat by construction — justified
3. A role nobody claims still renders as a visible bubble (fallback by reference) — justified
4. system/done keep the old bubble fall-through via a narrower undrawn override — justified
5. Page chrome rides as host entries reusing default ids, bodies verbatim — justified
6. Six page-only entry ids (bubble, thinking_block, …) documented in the parity test — declared
7. Parity test drops the orphaned-registry-role and fails-closed-extractor assertions — justified by construction
8. Chrome allowlist shrinks from 3 roles to permission — justified
9. Four source-shape tests re-pinned to entry shapes — rides along, required by item 1
10. Row bodies remain duplicated against createTranscriptRenderers (8 shared ids counted: tool, file, nudge, recovery_inject, thinking_block, error, workflow_completion, subagent_completion) — declared, deferred

On item 10 I verified the deferral is honest rather than a dodge: the page entries carry chrome (toolDisclosureKey state, trailing-group animation, session/panel hand-offs) that TranscriptRendererOptions cannot yet accept, so spreading the factory now is genuinely larger than this change — and the duplication count is unchanged from before the PR (if-chain vs factory), so nothing new is carried. I checked mergeRenderers ordering: both shape-matched defaults (stop_event, subagent_completion) are overridden, so host-entry order is the if-chain's precedence, as claimed. No undeclared item, no zero-consumer surface (nothing new is exported), no existing mechanism bypassed — the existing mechanism is what the change adopts.

[FIRST-PRINCIPLES-REVIEWED] 7e823e0

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 7e823e02c5bff871a874869efb28b117a24d4275 — this comment is updated in place on each push.

Review details

I've independently traced the refactor. Let me verify my analysis of the merge ordering is complete by confirming the one remaining risk: whether any kept default could shadow a page entry.

The kept (non-overridden) defaults are user, assistant, tool_lifecycle, inject — all role-keyed, none shape-matched, so mergeRenderers places them strictly after the page entries. bubble (page entry) claims user/assistant/streaming/inject and sits ahead of those defaults, so it wins for every conversational role, exactly as the if-chain's fall-through did. resolveRenderer returns undefined only for genuinely unclaimed roles (system/done), which then route to bubbleRenderer via (entry ?? bubbleRenderer) — matching the original fall-through.

I verified: REASONING_ROLES === ['thinking'] equals isReasoningRole's set; the undrawn entry's ['thinking','queued'] reproduces the two null arms; thinking_block's '*'+hasReasoningContent can only match thinking (so it never front-runs another role); the huge useMemo dep array covers every closed-over variable across all entries; and renderMessage's useCallback deps (chatPageRenderers, bubbleRenderer, slotRunning, handleFileOpen, connectionsUiOn) cover everything it reads outside refs. No reordering diverges on a concrete input, no key is dropped, and no AUTOSDE blocking rule is touched.

The single candidate ("No candidates") died at discovery, and my falsification finds no new grounded defect.

No findings.

[OPUS-REVIEWED] 7e823e0

Verdict parsed from the review's SHA-scoped output markers for commit 7e823e02c5bff871a874869efb28b117a24d4275.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 7e823e02c5bff871a874869efb28b117a24d4275: <one-sentence reason>

[CrysisDeu]

GPT — BLOCKING: system messages are silently hidden / Design, UX, First Principles — the unclaimed-role fallback resolves to the SDK undrawn default, not the bubble (e5e753943 → fixed in 8e10fda14)

Accepted — all four lanes, one bug, and the description and test comment both asserted the opposite of what shipped. mergeRenderers puts the kept role-keyed defaults after the host entries, so the merged tail was undrawn (() => null). Fixed by reference, not position: the memo returns { renderers, fallback: bubble } and renderMessage does (entry ?? bubbleRenderer).render(m, ctx). For system / done specifically, the page now overrides undrawn with a narrower role set (REASONING_ROLES + queued) so those two lifecycle markers stay unclaimed and take the bubble fallback exactly as the if-chain's fall-through did, instead of the SDK's null. The parity test gains an assertion that pins both (fallback-by-reference present, tail-indexing absent, undrawn roles exclude system/done); description corrected.

[CrysisDeu]

First Principles — Watch: "renders everywhere by construction" holds only for the SDK defaults; the dashboard's rich rows still live in two host lists (createTranscriptRenderers vs ChatPage's), and transcriptRenderers.tsx's header is now false; the chrome-literal extractor lost its fail-closed companion. Subtractions: reuse recovery_inject; defer-and-shrink workflow_completion/subagent_completion by spreading the factory (e5e753943 → 8e10fda14)

• Two host lists: accepted as stated. The description now says the gap is moved, not closed — the registry mechanism is unified, the dashboard's rich rows are not yet — and names the next cut (spread createTranscriptRenderers(...) into ChatPage's list). transcriptRenderers.tsx's header is rewritten to say ChatPage dispatches through the registry with its own list and which ids the two lists now share.
• Ids: inject_recovery → recovery_inject, and reasoning → thinking_block, matching the factory so the lists converge by override; PAGE_ONLY_ENTRY_IDS and the shape tests follow.
• Completion entries: deferred with the merge above, as you frame it — the factory's TranscriptRendererOptions already parameterizes the hand-offs, so that cut replaces both entries at once rather than this PR re-plumbing them alone.
• Extractor: the deleted assertion was the "registry-only roles are orphaned in ChatPage" check, which the registry-driven dispatch makes tautological rather than fail-closed; noted in the description. The structural assertion now rejects a switch (m.role) as well as an if, so a dispatch cannot return in either spelling. The chrome check keys on the .role === '…' literal, as it did before this PR.

[chenmingwei23]

Tier 1 auto-approve: refactor (7 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean (Semgrep + CodeQL both success, 0 annotations, 0 alerts), security checklist all-NO, AI reviewers green. Category: pure render-dispatch refactor moving ChatPage transcript-row branches verbatim into the renderer registry, no behaviour change.

[iamwhatever]

Tier 1 auto-approve: refactor (7 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: code-move only -- ChatPage's ~200-line if (m.role === ...) render chain is replaced by resolveRenderer over mergeRenderers, with each branch body moved verbatim into a host entry; no component, prop, style or output change, and the parity contract test is narrowed rather than deleted.