Repository navigation
Switch renderers from the chat prompt, without a trip to Settings - #244
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new CopilotKitChatPanel component to integrate CopilotKit for the chat interface, refactoring the existing ChatPanel into a shared ChatPanelBase class to support both plain and CopilotKit implementations. It also updates the renderer selection and startup resolution logic to handle interactive approvals, static paths, and cancellation, backed by comprehensive unit, E2E, and visual regression tests. Feedback on the changes highlights opportunities to improve robustness against malformed payloads by using optional chaining for component counts, adding strict type guards for message content validation, and casting untyped objects to prevent strict TypeScript compilation errors.
| const componentCount = | ||
| isSnapshot && parsed?.success | ||
| ? parsed.blocks | ||
| .filter(isRenderA2uiItem) | ||
| .reduce((count, block) => count + (block.updateComponents?.components.length ?? 0), 0) |
There was a problem hiding this comment.
If block.updateComponents.components is null or undefined (which can happen with malformed or partial LLM payloads), accessing .length directly will throw a runtime TypeError. Using optional chaining here ensures robust and defensive handling of external payloads.
| const componentCount = | |
| isSnapshot && parsed?.success | |
| ? parsed.blocks | |
| .filter(isRenderA2uiItem) | |
| .reduce((count, block) => count + (block.updateComponents?.components.length ?? 0), 0) | |
| const componentCount = | |
| isSnapshot && parsed?.success | |
| ? parsed.blocks | |
| .filter(isRenderA2uiItem) | |
| .reduce((count, block) => count + (block.updateComponents?.components?.length ?? 0), 0) | |
| : null; |
There was a problem hiding this comment.
isRenderA2uiItem already guarantees this: it only passes blocks whose updateComponents.components is an array (Array.isArray(uc['components'])), so .length is safe after the filter. The spec covers it with a payload that has one updateComponents without components and one with components: null ("counts only well-formed update blocks when a payload omits its components array").
| const isLayout = | ||
| message.role !== MessageRole.ERROR && | ||
| ((parsed?.success && !parsed.isConversational) || | ||
| this.chatCleaner.isLayoutSnapshot(message.content) || | ||
| (message.role === MessageRole.MODEL && isStreaming && /^\s*[\[{]/.test(cleaned))); |
There was a problem hiding this comment.
If message.content is undefined or not a string (for example, when a message contains only attachments or thinking blocks), passing it directly to isLayoutSnapshot can cause runtime errors. In alignment with our strict type guard rules for external or untyped payloads, we should use a strict typeof check instead of a simple fallback like message.content || '' to prevent potential crashes if the value is a non-string truthy object.
| const isLayout = | |
| message.role !== MessageRole.ERROR && | |
| ((parsed?.success && !parsed.isConversational) || | |
| this.chatCleaner.isLayoutSnapshot(message.content) || | |
| (message.role === MessageRole.MODEL && isStreaming && /^\s*[\[{]/.test(cleaned))); | |
| const isLayout = | |
| message.role !== MessageRole.ERROR && | |
| ((parsed?.success && !parsed.isConversational) || | |
| (typeof message.content === 'string' && this.chatCleaner.isLayoutSnapshot(message.content)) || | |
| (message.role === MessageRole.MODEL && isStreaming && /^\s*[\[{]/.test(cleaned))); |
References
- When validating properties from untyped or external records (such as API responses or agent-supplied payloads), use a strict type guard like
typeof value === 'string'before calling string methods. Avoid simple fallbacks like(value || '').trim(), as they can still cause runtimeTypeErrorcrashes if the value is a non-string truthy object (like{}).
There was a problem hiding this comment.
LlmMessage.content is typed string, and this is Composer's own chat history rather than an external payload, so it can't hold a non-string object here. ChatCleaner.isLayoutSnapshot(text?: string) also returns false for an empty or missing value.
| if (payload && typeof payload === 'object' && !Array.isArray(payload)) { | ||
| const catalogId = 'catalogId' in payload ? payload.catalogId : undefined; | ||
| const schemaId = '$id' in payload ? payload.$id : undefined; | ||
| id = | ||
| typeof catalogId === 'string' | ||
| ? catalogId | ||
| : typeof schemaId === 'string' | ||
| ? schemaId | ||
| : undefined; | ||
| const sanitized = {...payload}; | ||
| if ('title' in sanitized && typeof sanitized.title === 'string') { | ||
| sanitized.title = sanitizeHtml(sanitized.title).toString(); | ||
| } | ||
| if ('description' in sanitized && typeof sanitized.description === 'string') { | ||
| sanitized.description = sanitizeHtml(sanitized.description).toString(); | ||
| } | ||
| fingerprint = stableStringify(sanitized); | ||
| } |
There was a problem hiding this comment.
In strict TypeScript, accessing arbitrary properties on payload (which is refined to object) or modifying properties on sanitized (which is inferred as {}) will cause compilation errors. Casting payload to Record<string, any> ensures type safety and clean compilation under strict compiler settings.
| if (payload && typeof payload === 'object' && !Array.isArray(payload)) { | |
| const catalogId = 'catalogId' in payload ? payload.catalogId : undefined; | |
| const schemaId = '$id' in payload ? payload.$id : undefined; | |
| id = | |
| typeof catalogId === 'string' | |
| ? catalogId | |
| : typeof schemaId === 'string' | |
| ? schemaId | |
| : undefined; | |
| const sanitized = {...payload}; | |
| if ('title' in sanitized && typeof sanitized.title === 'string') { | |
| sanitized.title = sanitizeHtml(sanitized.title).toString(); | |
| } | |
| if ('description' in sanitized && typeof sanitized.description === 'string') { | |
| sanitized.description = sanitizeHtml(sanitized.description).toString(); | |
| } | |
| fingerprint = stableStringify(sanitized); | |
| } | |
| if (payload && typeof payload === 'object' && !Array.isArray(payload)) { | |
| const rawPayload = payload as Record<string, any>; | |
| const catalogId = rawPayload.catalogId; | |
| const schemaId = rawPayload.$id; | |
| id = | |
| typeof catalogId === 'string' | |
| ? catalogId | |
| : typeof schemaId === 'string' | |
| ? schemaId | |
| : undefined; | |
| const sanitized = {...rawPayload}; | |
| if (typeof sanitized.title === 'string') { | |
| sanitized.title = sanitizeHtml(sanitized.title).toString(); | |
| } | |
| if (typeof sanitized.description === 'string') { | |
| sanitized.description = sanitizeHtml(sanitized.description).toString(); | |
| } | |
| fingerprint = stableStringify(sanitized); | |
| } |
There was a problem hiding this comment.
This compiles under the repo's strict settings: the 'catalogId' in payload checks narrow payload, which is why the production build and lint pass. Casting to Record<string, any> would bring in any, which the codebase avoids, so I've kept the in checks.
d80d580 to
8827b0a
Compare
⚡ A2UI Composer PR PreviewYour automated preview is successfully live (commit
|
d51b494 to
283198a
Compare
| encapsulation: ViewEncapsulation.None, | ||
| }) | ||
| export class CopilotKitChatPanel extends ChatPanelBase { | ||
| // The renderer menu in the prompt pill makes switching output formats, for |
There was a problem hiding this comment.
Nit: Please use JSDoc format.
There was a problem hiding this comment.
Done, it's a JSDoc comment now.
| retryOfPromptId?: string; | ||
| }): Promise<void> { | ||
| if (this.rendererSelection.isSwitching()) { | ||
| return; |
There was a problem hiding this comment.
Can you add a comment here -- reading the code seems to contradict the JSDoc. If a renderer switch is underway, how does the prompt get submitted?
There was a problem hiding this comment.
You're right, the doc was wrong: it doesn't wait, it skips sending. During a switch the Send button is disabled and the panel shows "Switching renderer…". Pressing Enter calls submitPrompt directly, so the override repeats that check. The prompt stays in the input, and the user sends it once the new renderer's catalog is active, so it's never generated against the old catalog. I rewrote the JSDoc to say that and added a comment at the early return.
| * `isSwitching` is true; if it fails, `error` holds a message for the user. | ||
| */ | ||
| @Injectable({providedIn: 'root'}) | ||
| export class RendererSelection { |
There was a problem hiding this comment.
It seems that with this service there should be more code changing. E.g., places where we were previously setting the renderer should be using this service.
There was a problem hiding this comment.
Settings is the only other place a user switches renderers, and it can't use this service as it stands. RendererSelection calls SettingsService.selectRenderer (which saves the choice and asks the user to allow a new origin) and then waits for the preview to load the renderer and deliver its catalog. The Settings page has no preview, so that wait would time out. Startup sets the initial renderer from the URL or saved settings before anything renders, which is a different job. #216's assistant tool is the other caller, and it does use this service.
I added a paragraph to the class doc saying where to use it and why Settings calls SettingsService.selectRenderer directly. If you'd like every switch to go through one service, I can make the catalog wait apply only when a preview is open and move Settings onto it, but I'd rather do that as its own PR so this one stays small.
| const expected = new URL(renderer.rendererUrl, document.baseURI); | ||
| // The preview appends bridge origins and theme to the configured URL. | ||
| for (const url of [actual, expected]) { | ||
| url.searchParams.delete('origin'); |
There was a problem hiding this comment.
This is a bit of "magic" -- if we were to add additional params, how would anyone know that this code needs to be updated?
There was a problem hiding this comment.
Good catch. The preview's parameters now live in one PreviewUrlParam enum. RenderedFrame uses it when it adds them, and frameMatches removes every value in it, so a new parameter only has to be added in one place. There's also a test in rendered-frame.spec.ts that fails if the preview's URL carries a parameter the enum doesn't list, so a missed one shows up in CI.
283198a to
c80248e
Compare
|
There's an optional I'm curious if this is an oversight? |
c80248e to
b759561
Compare
The renderer tool passes its tool call's AbortSignal through RendererSelection, SettingsService and StartupResolution. Stopping the assistant turn then cancels a pending switch: an open origin confirmation closes without allowing the origin, the catalog wait ends, and the selection is never committed. a2ui-project#244 added the switch without this, since nothing there could cancel one.
Not an oversight across the stack, but you're right that nothing in #244 used it. Its only caller is #216's assistant tool ( I've moved it so each PR carries only what it uses:
Both are rebased onto current |
b759561 to
09146db
Compare
|
Thanks for trying it, Jay. Fixed in 09146db. What was happening: switching renderers has always reset the session, from Settings too. Now: a switch resets only a canvas nobody has changed, so an untouched sample becomes the new renderer's sample, as before. A canvas you or the assistant edited stays, along with the conversation, and the new renderer draws it. A response still streaming for the old renderer is cancelled. Tests:
Also rebased onto current |
The renderer tool passes its tool call's AbortSignal through RendererSelection, SettingsService and StartupResolution. Stopping the assistant turn then cancels a pending switch: an open origin confirmation closes without allowing the origin, the catalog wait ends, and the selection is never committed. a2ui-project#244 added the switch without this, since nothing there could cancel one.
09146db to
476c9eb
Compare
The renderer tool passes its tool call's AbortSignal through RendererSelection, SettingsService and StartupResolution. Stopping the assistant turn then cancels a pending switch: an open origin confirmation closes without allowing the origin, the catalog wait ends, and the selection is never committed. a2ui-project#244 added the switch without this, since nothing there could cancel one.
The CopilotKit panel's prompt pill gets a renderer menu, so switching renderers doesn't need a trip to Settings. The menu lists every configured renderer by its display name, keeps the typed prompt, and holds Send until the selected renderer's catalog is active. - RendererSelection switches the preview and waits for the new renderer's handshake and catalog. The wait is its own small class with a method per step. If Settings declines the renderer, the message says why and what to do, and the panel shows it under the menu. - StartupResolution commits a selection only after its origin is allowed, cancels a pending confirmation when asked, and refuses non-HTTP(S) URLs. A selection that was overtaken while its confirmation was open is rejected by comparing the selection before and after.
Addresses review feedback on the renderer menu: - The preview's query parameters (origin, theme) live in one PreviewUrlParam enum, used where the preview adds them and where RendererSelection removes them before comparing URLs. A new test fails if the preview adds a parameter the enum doesn't list. - submitPrompt's doc said it waited for a renderer switch, but it skips sending: the Send button is disabled during a switch, Enter repeats that check, and the prompt stays in the input. - RendererSelection's doc says where to use it, and why the Settings page calls SettingsService.selectRenderer directly. - The rendererSelection field uses a JSDoc comment.
Nothing in this PR passed the optional AbortSignal that RendererSelection, SettingsService and StartupResolution accepted; only tests did. Its caller is a2ui-project#216's assistant tool, which cancels a pending switch when the user stops the turn, so the signal moves to a2ui-project#216 with it. Without it, nothing produces a DOMException, so a failed switch's message is read from a plain Error. The rest stays: a selection is committed only once its origin is allowed, a stale approval is rejected, and the renderer menu stays locked while an approval is open, now covered by a timeout test instead of a cancellation test.
476c9eb to
5c05823
Compare
The renderer tool passes its tool call's AbortSignal through RendererSelection, SettingsService and StartupResolution. Stopping the assistant turn then cancels a pending switch: an open origin confirmation closes without allowing the origin, the catalog wait ends, and the selection is never committed. A canceled switch rejects with the signal's reason, so the caller recognizes its own cancellation. a2ui-project#244 added the switch without this, since nothing there could cancel one.
The renderer tool passes its tool call's AbortSignal through RendererSelection, SettingsService and StartupResolution. Stopping the assistant turn then cancels a pending switch: an open origin confirmation closes without allowing the origin, the catalog wait ends, and the selection is never committed. A canceled switch rejects with the signal's reason, so the caller recognizes its own cancellation. a2ui-project#244 added the switch without this, since nothing there could cancel one.
Full-resolution MP4. Recorded on
d80d580, before the rebase onto the latest main, against the local dev servers: a prompt typed into the pill, a switch to Lit Basic and back with the prompt kept, and a renderer whose URL can't be used, with the reason shown under the menu.The problem
Composer can preview the same A2UI in several renderers, and #216 adds one that produces a different output format entirely: Slack Block Kit. Today the only place to change renderer is Settings. Someone drafting in the chat panel who wants to see the same request as a Slack message has to leave the conversation, pick the renderer in Settings, come back, and hope their half-typed prompt survived. With one renderer that never comes up; with standard A2UI, Slack, and later formats such as Teams, it's the step that makes trying another format feel expensive.
Switching isn't only a navigation problem. The assistant builds its system prompt from the active renderer's catalog, and a new renderer takes a moment to load and send that catalog. A prompt sent in that window is built against the previous renderer's components.
This was split out of #215 at Jay's request, so it can be discussed on its own.
The approach
A menu in the prompt pill. The CopilotKit panel's pill gets a renderer menu beside Send. It lists every renderer from the app config and from Settings by its configured display name, so a new or internal renderer appears correctly with no code change. Choosing one keeps the typed prompt, and Send stays disabled until the new renderer's catalog is active. That closes the window above. The cost is a short wait after switching, which the pill shows as "Switching renderer…".
Waiting for the catalog, not just the page.
RendererSelectionswitches through Settings, so origin approval and persistence stay in one place, and then waits for the new renderer's handshake and its catalog to become active. That wait is a small class,CatalogWait, with a method per step. It rejects if another selection overtakes it, the renderer reports a catalog error, or 15 seconds pass. It ignores the message stream's replayed last message, which belongs to the previous renderer even when both share an origin.Committing only allowed selections.
StartupResolution.setSelectedRendererIdnow commits the selection only once the renderer's origin is allowed, so a declined renderer never replaces the current one or resets the draft. If anything else changed the renderer while that dialog was open, the request is stale and is rejected; this is a before-and-after comparison rather than a request counter. Renderer URLs must be HTTP(S).Saying why a switch failed. If Settings declines a renderer, the message names it, gives the reason, and says what to do. It appears under the menu, and the menu stays on the previous renderer.
Matching normalized URLs. A renderer opened from a
?renderer=link resolves to a normalized URL (http://localhost:3456/), which didn't match the configuredhttp://localhost:3456, so the pill said "Renderer". The selection service now compares parsed URLs.The shared chat panel base is unchanged: the CopilotKit panel owns the menu and waits for a switch before submitting.
What is not covered
samples/react-slack-catalog/resolve in Add Slack Block Kit authoring to Composer #216, which adds the renderer that needs them. This PR resolves renderer URLs the way main does.Verification
Rebased onto
mainat8155592(Oct 15, after #254, #255, #259, #261 and #262); head is5c05823. Formatting, lint, typechecks, the production build, and all unit tests pass (1,777 in the shell). Full e2e: all 89 pass. Before the pre-review changes below, one failure was this PR's failure-message journey, a clash between its setup and #262: the sharedbeforeEachclears storage in each same-origin frame, and since #262 the custom renderer it seeds is read from storage at selection time. The setup now seeds in every frame (folded into the first commit); the journey then passed 3 of 3.Review changes (Oct 5), one reply on each thread:
origin,theme) live in onePreviewUrlParamenum.RenderedFrameuses it when it adds them andRendererSelectionwhen it removes them before comparing URLs. A newrendered-frame.spec.tstest fails if the preview adds a parameter the enum doesn't list.submitPrompt's JSDoc now says what it does: during a switch it doesn't send, matching the disabled Send button, and the prompt stays in the input.RendererSelection's class doc says where to use it, and why the Settings page, which has no preview to wait for, callsSettingsService.selectRendererdirectly.rendererSelectionfield has a JSDoc comment.AbortSignalmoved to Add Slack Block Kit authoring to Composer #216 (Oct 6). Nothing in this PR passed one; its caller is Add Slack Block Kit authoring to Composer #216's assistant tool, which cancels a pending switch when the user stops the turn. What stays here: a selection is committed only once its origin is allowed, a stale approval is rejected, and the renderer menu stays locked while an approval is open, now tested with a timeout instead of a cancellation.maintoo. Without it, a switch from the menu resets the canvas and conversation, as Settings does today.Pre-review changes (Oct 15), folded into the commits that own the code:
The menu lists renderers added later. It read the renderer list once, so a renderer saved in Settings afterwards didn't appear until a reload.
SettingsService.rendererOptionsis a new computed list that also follows custom renderers, and the menu uses it.Settings handles a superseded approval.
setSelectedRendererIdthrows when another selection wins while the origin dialog is open. The Settings page now shows the renderer actually selected and explains under the selector: "The renderer selection changed while this one was waiting for approval, so it wasn't applied."The menu's error clears when the renderer changes by any other means, such as Settings.
One catalog fingerprint.
sanitizeCatalogin catalog-management.ts is shared byCatalogManagementandRendererSelection, instead of each sanitizing a catalog's title and description its own way.notAllowedMessageuses the existingisValidHttpUrl;isSameUrlandframeMatchessay in their JSDoc how they differ fromStartupResolution.normalizeUrl.RendererSelection.activeRenderersays why it matches the preview's URL, not the selected ID: a?renderer=link loads a renderer by URL.inject(DOCUMENT)instead of thedocumentglobal; JSDoc on the service's public members andisRendererSwitchDisabled; the menu button's title uses the same label as the button.Removed Slack and model-tool references and two spec cases that tested Add Slack Block Kit authoring to Composer #216's page-relative URLs; they live in Add Slack Block Kit authoring to Composer #216.
The pill journey checks the workspace title changes to Lit Basic's catalog, which only the new renderer produces.
renderer-selection.spec.tscovers waiting for a fresh catalog, ignoring replayed messages and old frames, overtaken and concurrent selections, both failure messages, and normalized URLs.startup-resolution.spec.tscovers committing only after approval, rejecting stale requests, and refusing non-HTTP(S) URLs.copilotkit-chat-panel.spec.tscovers the menu: labels, choices, disabled states, and the failure message.copilot-assistant.e2e.tsadds two journeys: switching renderers from the pill with the prompt kept, and the failure message.The CopilotKit panel's Linux visual baseline is regenerated for the pill with the menu.
Merge notes
#215 has merged, so this PR's diff against
mainis only its own four commits. #269 is independent; both add tests tocopilot-assistant.e2e.ts, so whichever merges second needs a small rebase. #216 builds on it, because its assistant tool switches renderers through the same service, so merge this PR first.