Skip to content

chat-adapters: DOM-returning render path + 4-adapter migration (#1100)#1106

Merged
joelteply merged 4 commits into
CambrianTech:canaryfrom
RebelTechPro:feat/adapter-dom-text
May 13, 2026
Merged

chat-adapters: DOM-returning render path + 4-adapter migration (#1100)#1106
joelteply merged 4 commits into
CambrianTech:canaryfrom
RebelTechPro:feat/adapter-dom-text

Conversation

@RebelTechPro
Copy link
Copy Markdown
Collaborator

@RebelTechPro RebelTechPro commented May 13, 2026

Summary

  • Introduces the new adapter contract (renderMessageElement(): HTMLElement | null) on AbstractMessageAdapter + MessageAdapter interface — opt-in, falls back to the legacy renderMessage() + innerHTML path when an adapter hasn't migrated.
  • Migrates all four shipping adapters: TextMessageAdapter, ImageMessageAdapter, URLCardAdapter, ToolOutputAdapter. Every chat content type that hits the live transcript now does so via DOM construction, not innerHTML. The TODO at the ChatWidget call site is removed.

Decisions made along the way

  • Backward-compatible contract: new method is optional. Default returns null, call site falls back to the legacy string path. No big-bang migration — each adapter is its own commit / its own decision.
  • Sub-renderer scope for ToolOutputAdapter: that adapter has 14 per-tool sub-renderers (Write/Read/Edit/Verify/Git/Search/Tree/Diff + 4 shell variants + Default). Migrating each one to return DOM nodes would have blown up the PR. Instead the scaffold is built via DOM and sub-renderer output is adopted via a detached <template>. Sub-renderers already escape user input through their internal escapeHtml() so the security boundary is preserved.
  • Detached parser lifecycle: the parser templates (Text + Tool adapters) are lazily created on first use, not at module load — keeps these modules importable in non-browser contexts (tests, SSR, type-check).
  • Side fix in ChatWidget: the no-adapter fallback was `<p>${message.content?.text}</p>` injected via innerHTML — small XSS hole for unknown content types. Replaced with a <p> + textContent.

Commit-by-commit

  1. chat-adapters: add DOM-returning render path, migrate TextMessageAdapter — contract change on AbstractMessageAdapter + MessageAdapter interface. New protected helper createAdapterWrapper(). Text adapter overrides; markdown→tool-use→syntax-highlight→file-path-linkify pipeline unchanged, only the destination of the parsed HTML moves to a detached template.
  2. chat-adapters: migrate ImageMessageAdapter to DOM-returning render — every interpolated attribute (img.src, img.alt, dataset.imageId, dataset.mediaId, dataset.url, dataset.filename) moves to property/dataset assignment. Caption (user-controlled) moves to .textContent. Action buttons gain aria-label mirroring title (same fix shape as the user-list a11y PR).
  3. fix(chat): lazily create text adapter parser — moves the text adapter's parser from module-level to function-local so the module remains importable in non-browser contexts.
  4. chat-adapters: migrate URLCardAdapter to DOM-returning render — concrete XSS fix: the string path interpolated ${additionalText} (user message text) directly into element-content position. Every interpolated value now travels via DOM property / dataset / textContent.
  5. chat-adapters: migrate ToolOutputAdapter to DOM-returning render — scaffold via DOM, sub-renderer output via detached template, parser lazily created (consistent with the text adapter fix). data-tool-name / data-message-id via dataset, status icon via textContent, inline tool images via property assignment.

Why DOM over string

  1. Assigning innerHTML on a live element destroys any Lit-managed reactive children — adapter renders blow away signal-bound state in sibling rows.
  2. The XSS surface narrows: user-controlled text is destined for .textContent paths, not concatenated into HTML strings or attribute positions.
  3. Performance: <template>.innerHTML parses once on a detached node, then cloneNode(true) produces a fragment to adopt — no live-tree re-parse.

Structural identity preserved

Every adapter migration preserves:

  • All existing class names (selectors used by handleContentLoading(), updateCardWithMetadata(), CSS, and the event delegator)
  • All data-action attributes (so MessageEventDelegator still finds its handlers)
  • Static action handlers (handleCopy / handleOpenInTab / handleCardClick / etc.) read dataset.* via closest() — works unchanged

Test plan

  • npm run build:ts → green (verified locally after each commit)
  • Text-adapter visual parity — plain text, markdown (heading + list + code fence + inline code), stack trace (collapsible), file path (clickable), <tool_use> block.
  • Image-adapter visual parity — single image, multiple images (grid), image with caption, image-load failure (error overlay + retry).
  • URL-card visual parity — message with one URL + extra text, message with one URL alone, https / http (lock/unlock glyph).
  • Tool-output visual parity — code/write, code/read, code/verify (pass/fail), git/commit, code/shell/execute (stdout/stderr/running). Each renderer's compact + expanded view.
  • All data-action buttons still wire: open in fullscreen, download, retry, AI describe, copy tool output, open in tab, URL summarize, URL retry preview.
  • No console errors during scroll-back over old messages.

⚠️ Not visually validated by me locally — TS compile is the only local check.

Out of scope (follow-ups, not blockers)

  • Lint rule to prevent new innerHTML = on live transcript elements
  • Refactoring ToolOutputAdapter sub-renderers to return DOM nodes directly (current path: scaffold via DOM, sub-renderer strings adopted via detached template)

joelteply and others added 2 commits May 13, 2026 11:35
First step of CambrianTech#1100. Establishes the new adapter contract and proves
it against the simplest, highest-traffic adapter. Behavior-preserving
for all unmigrated adapters — they continue down the existing
renderMessage()+innerHTML path.

Contract change (AbstractMessageAdapter / MessageAdapter):
  - New optional method `renderMessageElement(): HTMLElement | null`.
    Default returns null = "fall back to the legacy string path."
    Adapters that override it return a fully-built wrapper element.
  - New helper `createAdapterWrapper()` for subclasses — produces the
    standard `message-content-adapter` host div with correct classes
    and data-content-type attribute, via DOM APIs (not by concatenating
    class names into a template string).

TextMessageAdapter migration:
  - Overrides `renderMessageElement()`. Builds wrapper with the helper,
    runs the existing renderContent() pipeline (markdown → tool-use
    restoration → syntax highlighting → file-path linkify), and adopts
    the result via a module-level detached `<template>` element so the
    live message-content slot never sees an innerHTML assignment.
  - Sanitization model is unchanged: user text still goes through
    escapeHtmlInPlainText() before marked.parse(), tool-use parameters
    still pass through escapeHtml(), code blocks still go through
    hljs.highlight(). The only change is where the HTML is parsed —
    on a detached node, not on the live transcript.

ChatWidget call site (the previously-flagged TODO):
  - Prefers the DOM path: adapter.renderMessageElement?.() →
    contentDiv.appendChild(node). Falls back to the legacy
    innerHTML path for adapters that haven't migrated.
  - Tiny side-fix: the no-adapter fallback was a string-concatenated
    <p>${text}</p> innerHTML — replaced with a textContent-set <p>.

Out of scope (future PRs in CambrianTech#1100): ImageMessageAdapter, URLCardAdapter,
ToolOutputAdapter, and the rest. Each migrates independently — call
site already handles both paths.

Not visually validated locally — TS compile is green; depends on a live
deployment to confirm rendering parity. Test plan in the PR description.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Second step on CambrianTech#1100, on top of the TextMessageAdapter migration.
Behavior-preserving (same class names, same data attributes, same
structure that handleContentLoading() and the event delegator depend on),
with a concrete security win: every interpolated attribute that the
string path placed inside `"..."` now travels via DOM property /
dataset assignment, and the caption — originating from user message
text — goes through `.textContent` instead of `${caption}` in an
element-content position.

Specifically:
  - img.src / img.alt set as properties (no attribute-quote escapes
    needed; the browser handles it)
  - container.dataset.imageId / .mediaId via DOM API
  - download button: dataset.url / dataset.filename via DOM API
  - retry button: dataset.url via DOM API
  - caption: textContent, not innerHTML
  - emoji button labels: textContent, not template literal
  - action buttons gained aria-label (mirrors the title attribute,
    which screen readers don't reliably surface — same fix shape as
    the user-list buttons in the a11y PR)

Structural identity preserved:
  - `.image-container`, `.image-loading-placeholder`, `.message-image`,
    `.image-error`, `.image-actions`, `.action-button` all kept
  - data-action attributes unchanged so MessageEventDelegator finds
    the same handlers
  - handleContentLoading() still finds `.message-image`,
    `.image-loading-placeholder`, `.image-error` via querySelector

The legacy renderContent() string path is untouched — `renderMessage()`
still returns the string-built version for any caller that hasn't
adopted renderMessageElement yet.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@RebelTechPro RebelTechPro changed the title chat-adapters: DOM-returning render path + TextMessageAdapter migration (step 1 of #1100) chat-adapters: DOM-returning render path + Text/Image migration (#1100) May 13, 2026
@joelteply joelteply changed the base branch from main to canary May 13, 2026 16:53
@joelteply joelteply marked this pull request as ready for review May 13, 2026 17:17
@joelteply joelteply merged commit a37b034 into CambrianTech:canary May 13, 2026
2 of 3 checks passed
@RebelTechPro RebelTechPro changed the title chat-adapters: DOM-returning render path + Text/Image migration (#1100) chat-adapters: DOM-returning render path + 4-adapter migration (#1100) May 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants