Skip to content

feat(ui): host theming and dark mode fixes#355

Merged
wonderwhy-er merged 2 commits intomainfrom
feat/mcp-app-dc-integration
Feb 24, 2026
Merged

feat(ui): host theming and dark mode fixes#355
wonderwhy-er merged 2 commits intomainfrom
feat/mcp-app-dc-integration

Conversation

@edgarsskore
Copy link
Collaborator

@edgarsskore edgarsskore commented Feb 23, 2026

User description

SDK (App, applyDocumentTheme, applyHostStyleVariables, applyHostFonts). Remove HTML sanitization to allow JS/CDN execution in sandboxed preview iframes. Add light-dark() CSS fallbacks for cross-host theme compatibility. Remove content duplication from structuredContent (widget reads text from MCP content array).

Summary by CodeRabbit

  • New Features

    • Host-framed rendering mode for tighter, chrome-free embedding.
    • Host context integration to let hosts control theme, initial expansion, and UI flags.
  • Improvements

    • Improved light/dark theming with updated color tokens and global background handling.
    • Refined image preview sizing and layout for better fit and scaling.
    • Simplified HTML preview handling for more consistent sandboxed rendering.

CodeAnt-AI Description

Host-themed interactive HTML previews and single-source text for file preview widget

What Changed

  • HTML previews run inside a nested sandboxed iframe that allows scripts and external CDNs, so interactive previews and embedded assets load in the preview.
  • Previews now adopt host theme variables and fonts; CSS includes light/dark fallbacks so contrast stays correct when a host doesn't provide colors.
  • File text is no longer duplicated in structured metadata: the widget reads textual content from the MCP content array, while structuredContent carries only file metadata.
  • Hosts can control initial expansion and remove the widget's inner header via hostContext flags (initiallyExpanded, hideSummaryRow); when host hides the summary row, only the file content is shown.
  • Widget persists preview state across refresh when available and shows clear status messages when no preview is available or host connection fails.

Impact

✅ Interactive previews load scripts and CDN assets
✅ Host theme and fonts reflected in previews
✅ Clearer preview visibility when host frames or disconnects

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

SDK (App, applyDocumentTheme, applyHostStyleVariables, applyHostFonts). Remove HTML
sanitization to allow JS/CDN execution in sandboxed preview iframes. Add light-dark()
CSS fallbacks for cross-host theme compatibility. Remove content duplication from
structuredContent (widget reads text from MCP content array).

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@codeant-ai
Copy link
Contributor

codeant-ai bot commented Feb 23, 2026

CodeAnt AI is reviewing your PR.


Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 23, 2026

📝 Walkthrough

Walkthrough

This PR replaces legacy RPC/host-lifecycle and theme-adaptation wiring with an App-based model context protocol, adjusts file-preview structured payloads (removes embedded content, returns image bytes/mimeType), updates UI bootstrap and rendering to use a RenderPayload, and introduces theme-aware CSS tokens and host-framed styling.

Changes

Cohort / File(s) Summary
Dependencies
package.json
Added dev dependency @modelcontextprotocol/ext-apps@^1.0.1.
Types
src/types.ts
Removed content: string from FilePreviewStructuredContent; payload content now passed via RenderPayload where needed.
Filesystem handlers
src/handlers/filesystem-handlers.ts
Removed _meta from ServerResult branches; image branch now returns imageData and mimeType (no content); fileType assignment unified via resolvePreviewFileType; PDF uses 'unsupported' as const.
UI bootstrap & renderer
src/ui/file-preview/src/app.ts
Replaced RPC/host-lifecycle with App-based model context protocol; introduced RenderPayload = FilePreviewStructuredContent & { content: string }; refactored render pipeline, host-context application, widget state persistence, and event-driven lifecycle handlers.
HTML renderer
src/ui/file-preview/src/components/html-renderer.ts
Removed per-call sanitize/CSP options and HtmlRenderOptions; renderHtmlPreview now takes (content, mode) and uses a simplified sandboxed iframe approach.
Removed host IPC modules
src/ui/shared/rpc-client.ts, src/ui/shared/host-lifecycle.ts, src/ui/shared/theme-adaptation.ts
Deleted legacy window RPC client, host-lifecycle helper, and theme-adaptation module — associated types, helpers, and exports removed.
Styles — base
src/ui/styles/base.css
Replaced many token fallbacks with light-dark() pairs; added html background rule and expanded theme-aware defaults for panel/text/border/status tokens.
Styles — file preview
src/ui/styles/apps/file-preview.css
Switched syntax-highlight tokens to light-dark() variants; added .host-framed mode to strip inner chrome and defer framing to host; adjusted image sizing and notice/filename styles.

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant App as App Instance
    participant Host as Host Context
    participant Server as Server Tool (handleReadFile)
    participant Renderer as File Preview Renderer

    User->>App: Initialize / connect
    App->>Host: app.connectPath() / getHostContext()
    Host-->>App: hostContext (theme, flags, widgetState)
    App->>App: applyHostCtx(hostContext)
    
    User->>Server: request file preview
    Server-->>App: structuredContent (no content)
    App->>App: buildRenderPayload(structuredContent + content)
    App->>Renderer: render(RenderPayload)
    Renderer-->>User: display preview (image/text/html) with host styling
    
    User->>Renderer: interact (copy, toggle, expand)
    Renderer->>App: app.updateModelContext(widgetState)
    App->>Host: persist widgetState
Loading
sequenceDiagram
    participant Old as RPC/Lifecycle Model (legacy)
    participant New as App/Context Model (current)

    Note over Old: window.postMessage RPC client\nhost-lifecycle handshake\ntheme-adaptation applies CSS vars
    Note over New: App.connectPath()\ncentral host-context injection\nrender via RenderPayload\nwidget-state persistence
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

Suggested labels

size:XL

Suggested reviewers

  • wonderwhy-er
  • serg33v

Poem

🐰 Hopped in code with tiny paws,

Old RPC unwound its claws,
App-context blossoms, light and dark,
Previews hum with cleaner spark,
Host-framed petals, neatly sown.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 4.35% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat(ui): host theming and dark mode fixes' directly corresponds to the main changes: introduction of host-driven theming, light-dark() CSS fallbacks, and integration of host context and theme adaptation throughout the UI components.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/mcp-app-dc-integration

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codeant-ai codeant-ai bot added the size:XXL This PR changes 1000+ lines, ignoring generated files label Feb 23, 2026
@codeant-ai
Copy link
Contributor

codeant-ai bot commented Feb 23, 2026

Nitpicks 🔍

🔒 No security issues identified
⚡ Recommended areas for review

  • Sandbox risk
    The PR removed HTML sanitization and embeds raw HTML (including scripts/CDNs) into a sandboxed iframe using the "allow-same-origin" + "allow-scripts" flags. That combination gives in-iframe scripts elevated capabilities (same-origin privileges) and increases the blast radius if untrusted content is ever rendered. Verify the threat model for all hosts and consider hardening iframe policy or reintroducing selective sanitization/CSP restrictions.

  • Possible Bug
    The handler computes resolvedFilePath for absolute-path behavior but calls readFile(parsed.path, options). This can cause inconsistencies (relative vs absolute reads), broken "Open in folder" behavior, or different file results on hosts. Validate whether readFile should be given resolvedFilePath.

  • Widget state / race
    The new App-based bootstrap restores widget state and applies host context during connect. There's some complexity around initialStateResolved and timeouts — ensure there are no race conditions between hostContext application, cached widgetState restoration, and incoming tool results (ontoolresult). Verify that the host-provided initiallyExpanded/hideSummaryRow flags are applied before the first render and that duplicate renders are avoided.

  • Host context shape
    The initialization response handling assumes the host's initialize response contains hostContext at the top-level as response.hostContext. Some hosts may return nested shapes (e.g. { result: { hostContext } }) or different keys. This may silently skip host context application; consider more robust extraction and validation to avoid missed theme/flags.

  • Host-framed content clipping
    The host-framed rules strip chrome but set .panel-content-wrapper { max-height: none; overflow: hidden; }. Hiding overflow may clip content provided by the host/frame (images, HTML previews). Verify the intended scrolling/overflow behavior when the host supplies an outer frame and ensure content isn't unintentionally cut off.

@codeant-ai
Copy link
Contributor

codeant-ai bot commented Feb 23, 2026

CodeAnt AI finished reviewing your PR.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (3)
src/ui/file-preview/src/app.ts (2)

819-840: Unnecessary double cast on line 821.

extractToolText accepts unknown, so the result as unknown as Record<string, unknown> double cast is redundant. Pass result directly.

Simplify
-        const message = extractToolText(result as unknown as Record<string, unknown>);
+        const message = extractToolText(result);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/ui/file-preview/src/app.ts` around lines 819 - 840, The code currently
double-casts result when calling extractToolText inside app.ontoolresult; remove
the unnecessary cast and pass result directly to extractToolText (i.e., replace
extractToolText(result as unknown as Record<string, unknown>) with
extractToolText(result)), leaving the surrounding logic using
extractRenderPayload, initialStateResolved, renderLoadingState,
resolveInitialState, renderAndSync and renderStatusState unchanged; if
TypeScript complains, narrow or assert the type at the call site with a single
appropriate cast only.

61-73: extractRenderPayload falls back to empty string content — verify this is safe for all renderers.

Line 71 defaults text to '' when neither the top-level nor the structuredContent carry a content array. An empty-string content for fileType: 'html' or 'markdown' would produce a blank preview without any user-facing indication, since the 'unsupported' notice path at line 221 only triggers for that specific fileType.

Consider whether an empty content payload (e.g. from a corrupted widget-state restore) should result in a notice or fallback rather than a silent blank panel.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/ui/file-preview/src/app.ts` around lines 61 - 73, The current
extractRenderPayload returns an empty string payload which can cause silent
blank previews; update extractRenderPayload to detect when the resolved meta
(from isPreviewStructuredContent) has fileType 'html' or 'markdown' (and/or
other text-based types) and the extracted text is empty (extractToolText(value)
and extractToolText(value.structuredContent) both falsy), and instead of
returning buildRenderPayload(meta, '') return undefined (or trigger the existing
unsupported/notice path) so renderers receive a clear fallback; keep the
existing parsing (isObject, isPreviewStructuredContent) and only change the
branch after computing meta and text to check meta.fileType and text before
calling buildRenderPayload.
src/ui/styles/apps/file-preview.css (1)

87-100: overflow: hidden in host-framed mode may clip long content if the host doesn't provide scrolling.

Line 98 sets overflow: hidden on .panel-content-wrapper in host-framed mode. If the host's outer frame doesn't supply its own scroll container, long file previews will be silently clipped.

If the host is guaranteed to handle scrolling, this is fine — but worth a brief comment to that effect.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/ui/styles/apps/file-preview.css` around lines 87 - 100, The rule
.tool-shell.host-framed .panel-content-wrapper currently sets overflow: hidden
which can clip long previews if the host doesn't provide scrolling; update the
style for .panel-content-wrapper in host-framed mode to ensure content scrolls
(e.g., use overflow: auto or remove the overflow rule) and add a short inline
comment clarifying that host-framed mode requires the host to provide the outer
scroll container if you keep overflow: hidden; reference .tool-shell.host-framed
and .panel-content-wrapper when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/ui/file-preview/src/app.ts`:
- Around line 719-734: The applyHostCtx function currently casts ctx to any and
directly accesses nested properties which can throw if intermediate values are
null/undefined; replace the broad cast with a safe runtime check that ctx is a
plain object (and not null) before accessing nested fields, then guard access to
styles, styles.css, and styles.css.fonts (e.g., use typeof/object checks or
optional chaining and type checks) before calling applyHostStyleVariables,
applyHostFonts or applyDocumentTheme, and keep the existing boolean checks for
initiallyExpanded and hideSummaryRow but ensure they only run after confirming
extra is an object; reference applyHostCtx, applyDocumentTheme,
applyHostStyleVariables, applyHostFonts, and the
extra/initiallyExpanded/hideSummaryRow checks when making these changes.
- Around line 847-872: The fallback 8s timer can race with an incoming tool
result and produce a stale "Preview unavailable" message; store the timeout id
returned by the setTimeout in a variable (e.g., fallbackTimer) in the scope
created during app.connect(), then call clearTimeout(fallbackTimer) from the
ontoolresult handler (the code path that eventually calls resolveInitialState
when a payload arrives) before invoking resolveInitialState; also clear the
timer when you resolve from cachedPayload to ensure the fallback is cancelled in
all successful-resolution paths.

In `@src/ui/file-preview/src/components/html-renderer.ts`:
- Around line 28-32: The sandbox on the iframe created by
renderSandboxedHtmlFrame is insecure because it uses both "allow-scripts" and
"allow-same-origin" with srcdoc (frameDocument), which lets the embedded page
escape the sandbox; remove "allow-same-origin" from the sandbox attribute in
renderSandboxedHtmlFrame (keep allow-scripts if you need script execution) or,
if same-origin access is truly required, stop using srcdoc and instead serve the
preview from a different origin or a blob/URL that isolates origin; update any
code that relies on same-origin behavior (e.g., postMessage usage or style
inheritance) accordingly and ensure escapeHtml/resolveThemeFrameStyles are still
applied.

---

Nitpick comments:
In `@src/ui/file-preview/src/app.ts`:
- Around line 819-840: The code currently double-casts result when calling
extractToolText inside app.ontoolresult; remove the unnecessary cast and pass
result directly to extractToolText (i.e., replace extractToolText(result as
unknown as Record<string, unknown>) with extractToolText(result)), leaving the
surrounding logic using extractRenderPayload, initialStateResolved,
renderLoadingState, resolveInitialState, renderAndSync and renderStatusState
unchanged; if TypeScript complains, narrow or assert the type at the call site
with a single appropriate cast only.
- Around line 61-73: The current extractRenderPayload returns an empty string
payload which can cause silent blank previews; update extractRenderPayload to
detect when the resolved meta (from isPreviewStructuredContent) has fileType
'html' or 'markdown' (and/or other text-based types) and the extracted text is
empty (extractToolText(value) and extractToolText(value.structuredContent) both
falsy), and instead of returning buildRenderPayload(meta, '') return undefined
(or trigger the existing unsupported/notice path) so renderers receive a clear
fallback; keep the existing parsing (isObject, isPreviewStructuredContent) and
only change the branch after computing meta and text to check meta.fileType and
text before calling buildRenderPayload.

In `@src/ui/styles/apps/file-preview.css`:
- Around line 87-100: The rule .tool-shell.host-framed .panel-content-wrapper
currently sets overflow: hidden which can clip long previews if the host doesn't
provide scrolling; update the style for .panel-content-wrapper in host-framed
mode to ensure content scrolls (e.g., use overflow: auto or remove the overflow
rule) and add a short inline comment clarifying that host-framed mode requires
the host to provide the outer scroll container if you keep overflow: hidden;
reference .tool-shell.host-framed and .panel-content-wrapper when making this
change.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b606511 and 6e63ac7.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (8)
  • package.json
  • src/handlers/filesystem-handlers.ts
  • src/types.ts
  • src/ui/file-preview/src/app.ts
  • src/ui/file-preview/src/components/html-renderer.ts
  • src/ui/shared/host-lifecycle.ts
  • src/ui/styles/apps/file-preview.css
  • src/ui/styles/base.css
💤 Files with no reviewable changes (1)
  • src/types.ts

Comment on lines +719 to +734
function applyHostCtx(ctx: unknown): void {
const c = ctx as any;
if (c?.theme === 'light' || c?.theme === 'dark') applyDocumentTheme(c.theme);
if (c?.styles?.variables) applyHostStyleVariables(c.styles.variables);
if (c?.styles?.css?.fonts) applyHostFonts(c.styles.css.fonts);

// Hosts may pass additional flags via hostContext (forward-compatible index signature)
const extra = c;
if (typeof extra.initiallyExpanded === 'boolean') {
isExpanded = extra.initiallyExpanded;
}
if (typeof extra.hideSummaryRow === 'boolean') {
hideSummaryRow = extra.hideSummaryRow;
if (hideSummaryRow) isExpanded = true;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Broad as any cast on host context; consider lightweight runtime guards.

applyHostCtx casts the entire context to any (line 720) then accesses nested paths (c.styles.variables, c.styles.css.fonts) without null checks on intermediate objects. If the host sends { styles: null } or { styles: { css: null } }, this will throw a TypeError at runtime.

Proposed defensive access
 function applyHostCtx(ctx: unknown): void {
-    const c = ctx as any;
-    if (c?.theme === 'light' || c?.theme === 'dark') applyDocumentTheme(c.theme);
-    if (c?.styles?.variables) applyHostStyleVariables(c.styles.variables);
-    if (c?.styles?.css?.fonts) applyHostFonts(c.styles.css.fonts);
+    if (!isObject(ctx)) return;
+    const theme = ctx.theme;
+    if (theme === 'light' || theme === 'dark') applyDocumentTheme(theme);
+    const styles = isObject(ctx.styles) ? ctx.styles : undefined;
+    if (styles?.variables) applyHostStyleVariables(styles.variables as Record<string, string>);
+    const css = isObject(styles?.css) ? styles.css : undefined;
+    if (typeof css?.fonts === 'string') applyHostFonts(css.fonts);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/ui/file-preview/src/app.ts` around lines 719 - 734, The applyHostCtx
function currently casts ctx to any and directly accesses nested properties
which can throw if intermediate values are null/undefined; replace the broad
cast with a safe runtime check that ctx is a plain object (and not null) before
accessing nested fields, then guard access to styles, styles.css, and
styles.css.fonts (e.g., use typeof/object checks or optional chaining and type
checks) before calling applyHostStyleVariables, applyHostFonts or
applyDocumentTheme, and keep the existing boolean checks for initiallyExpanded
and hideSummaryRow but ensure they only run after confirming extra is an object;
reference applyHostCtx, applyDocumentTheme, applyHostStyleVariables,
applyHostFonts, and the extra/initiallyExpanded/hideSummaryRow checks when
making these changes.

Comment on lines +847 to 872
app.connect().then(() => {
// Apply initial host context received during the initialization handshake
const ctx = app.getHostContext();
if (ctx) {
applyHostCtx(ctx);
}
}, 8000);

window.addEventListener('message', (event) => {
try {
if (rpcClient.handleMessageEvent(event)) {
return;
}
if (!isTrustedParentMessageSource(event.source, window.parent)) {
return;
}
if (!isObject(event.data)) {
return;
}
themeAdapter.applyFromData(event.data);

if (event.data.method === 'ui/notifications/tool-result') {
const params = event.data.params;
const candidate = isObject(params) && isObject(params.result) ? params.result : params;
const payload = extractStructuredContent(candidate);
const message = extractToolTextFromEvent(event.data) ?? extractToolText(candidate);
if (!initialStateResolved) {
if (payload) {
renderLoadingState(container);
onRender?.();
window.setTimeout(() => resolveInitialState(payload), 120);
return;
}
if (message) {
resolveInitialState(undefined, message);
}
return;
}
if (payload) {
renderAndSync(payload);
} else if (message) {
renderStatusState(container, message);
onRender?.();
}
return;
// Try to restore from persisted widget state (survives refresh on some hosts)
const cachedPayload = widgetState.read();
if (cachedPayload) {
window.setTimeout(() => resolveInitialState(cachedPayload), 50);
}

const payload = extractStructuredContent(event.data);
if (payload) {
// Fallback: if no tool data arrives, show a helpful status message
window.setTimeout(() => {
if (!initialStateResolved) {
resolveInitialState(payload);
return;
resolveInitialState(
undefined,
'Preview unavailable after page refresh. Switch threads or re-run the tool.'
);
}
renderAndSync(payload);
}
} catch {
renderStatusState(container, 'Preview failed to render.');
onRender?.();
}
}, 8000);
}).catch(() => {
renderStatusState(container, 'Failed to connect to host.');
onRender?.();
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

8-second fallback timeout is reasonable but could show a stale message if a tool result arrives just after.

The setTimeout at line 861 fires resolveInitialState(undefined, ...) after 8 seconds. If a tool result arrives at, say, 7.9 seconds and triggers the 120ms delay (line 826), the 8-second timeout fires first and resolves with the "Preview unavailable" message. The actual payload resolution at 8.02s is then blocked by the initialStateResolved guard.

This is a narrow race, but the user would see "Preview unavailable after page refresh" even though data was available. Consider clearing the fallback timeout when ontoolresult receives a payload.

Proposed fix: cancel fallback timer on tool result
+    let fallbackTimer: ReturnType<typeof setTimeout> | undefined;
+
     app.ontoolresult = (result) => {
         const payload = extractRenderPayload(result);
-        const message = extractToolText(result as unknown as Record<string, unknown>);
+        const message = extractToolText(result);
         if (!initialStateResolved) {
             if (payload) {
+                if (fallbackTimer) { clearTimeout(fallbackTimer); fallbackTimer = undefined; }
                 renderLoadingState(container);
                 onRender?.();
                 window.setTimeout(() => resolveInitialState(payload), 120);
                 return;
             }

And at line 861:

-        window.setTimeout(() => {
+        fallbackTimer = window.setTimeout(() => {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/ui/file-preview/src/app.ts` around lines 847 - 872, The fallback 8s timer
can race with an incoming tool result and produce a stale "Preview unavailable"
message; store the timeout id returned by the setTimeout in a variable (e.g.,
fallbackTimer) in the scope created during app.connect(), then call
clearTimeout(fallbackTimer) from the ontoolresult handler (the code path that
eventually calls resolveInitialState when a payload arrives) before invoking
resolveInitialState; also clear the timer when you resolve from cachedPayload to
ensure the fallback is cancelled in all successful-resolution paths.

@wonderwhy-er
Copy link
Owner

🟡 Race condition: 8-second fallback timeout vs tool result

CodeRabbit caught a subtle bug: if a tool result arrives at ~7.9s and triggers the 120ms render delay, the 8s fallback fires first and shows "Preview unavailable," and then the actual payload is blocked by the initialStateResolved guard. The fix is simple — store the timeout ID and clearTimeout it in ontoolresult and from the cached-state restore path.

@wonderwhy-er
Copy link
Owner

🟡 Dead code left behind: host-lifecycle.ts, rpc-client.ts, theme-adaptation.ts

After this PR, app.ts no longer imports host-lifecycle, rpc-client, or theme-adaptation — the App SDK replaces all three. But the files are still in the repo and host-lifecycle.ts was even updated in this PR (the onHostContext callback addition). Since nothing imports these shared modules anymore (only host-lifecycle.ts internally imports rpc-client), they are now dead code. Consider removing them in a follow-up or in this PR to avoid confusion.

@wonderwhy-er
Copy link
Owner

ℹ️ The host-lifecycle.ts change (adding onHostContext callback) is pure dead code in this PR — it was likely developed alongside the old approach before switching to the App SDK. It's harmless but adds to the "dead code" story.

…red modules

Remove allow-same-origin from the HTML preview iframe sandbox to prevent
embedded content from escaping the sandbox via same-origin access. Delete
host-lifecycle.ts, rpc-client.ts, and theme-adaptation.ts which became
dead code after the App SDK migration.

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@codeant-ai
Copy link
Contributor

codeant-ai bot commented Feb 23, 2026

CodeAnt AI is running Incremental review


Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

@codeant-ai codeant-ai bot added size:XXL This PR changes 1000+ lines, ignoring generated files and removed size:XXL This PR changes 1000+ lines, ignoring generated files labels Feb 23, 2026
@codeant-ai
Copy link
Contributor

codeant-ai bot commented Feb 23, 2026

CodeAnt AI Incremental review completed.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/ui/file-preview/src/components/html-renderer.ts (1)

41-50: try/catch around renderSandboxedHtmlFrame is dead code.

renderSandboxedHtmlFrame only performs synchronous string concatenation and a window.getComputedStyle call (which cannot throw for a live document). The catch branch — swallowing the error and falling back to source view — will never execute in practice, making this a misleading safety net.

♻️ Proposed simplification
-    try {
-        return {
-            html: `<div class="panel-content html-content">${renderSandboxedHtmlFrame(content)}</div>`
-        };
-    } catch {
-        return {
-            html: `<div class="panel-content source-content">${renderCodeViewer(content, 'html')}</div>`,
-            notice: 'HTML renderer failed. Showing source instead.'
-        };
-    }
+    return {
+        html: `<div class="panel-content html-content">${renderSandboxedHtmlFrame(content)}</div>`
+    };
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/ui/file-preview/src/components/html-renderer.ts` around lines 41 - 50,
The try/catch around renderSandboxedHtmlFrame is unnecessary because
renderSandboxedHtmlFrame is synchronous and won't throw; remove the try/catch
and always return the HTML panel using renderSandboxedHtmlFrame(content) in
html-renderer.ts (remove the catch branch that returns renderCodeViewer and the
notice). Keep the single return that produces { html: `<div class="panel-content
html-content">${renderSandboxedHtmlFrame(content)}</div>` } and delete the dead
fallback code and unused notice string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/ui/file-preview/src/components/html-renderer.ts`:
- Around line 2-7: Update the HTML preview renderer to remove the unsafe
permission: drop "allow-popups" from the iframe sandbox attribute used by the
HtmlRenderer component (the nested sandboxed iframe) so untrusted HTML/scripts
cannot open external popup windows; also update the top-of-file header comment
that currently asserts "sandbox isolation prevents any escape" to reflect that
popups can allow navigation and that "allow-popups" was removed to mitigate this
phishing/vector risk.

In `@src/ui/styles/apps/file-preview.css`:
- Around line 87-99: The .panel-content-wrapper in host-framed mode currently
sets "overflow: hidden" which silently clips content when the host frame is
smaller; update the .tool-shell.host-framed .panel-content-wrapper rule (and
related host-framed rules) to remove or change overflow: hidden to overflow:
visible (or omit the overflow property entirely) so the widget doesn't
permanently hide overflowed content and leaves scroll/clipping behavior to the
host container; ensure the change is applied to the CSS selector
.tool-shell.host-framed .panel-content-wrapper in this file.

---

Nitpick comments:
In `@src/ui/file-preview/src/components/html-renderer.ts`:
- Around line 41-50: The try/catch around renderSandboxedHtmlFrame is
unnecessary because renderSandboxedHtmlFrame is synchronous and won't throw;
remove the try/catch and always return the HTML panel using
renderSandboxedHtmlFrame(content) in html-renderer.ts (remove the catch branch
that returns renderCodeViewer and the notice). Keep the single return that
produces { html: `<div class="panel-content
html-content">${renderSandboxedHtmlFrame(content)}</div>` } and delete the dead
fallback code and unused notice string.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6e63ac7 and ab85ea9.

📒 Files selected for processing (5)
  • src/ui/file-preview/src/components/html-renderer.ts
  • src/ui/shared/host-lifecycle.ts
  • src/ui/shared/rpc-client.ts
  • src/ui/shared/theme-adaptation.ts
  • src/ui/styles/apps/file-preview.css
💤 Files with no reviewable changes (3)
  • src/ui/shared/host-lifecycle.ts
  • src/ui/shared/rpc-client.ts
  • src/ui/shared/theme-adaptation.ts

Comment on lines +2 to +7
* HTML preview renderer with display mode control. It handles rendered HTML versus
* source text display and ensures fallback behavior is predictable.
*
* The rendered preview runs inside a nested sandboxed iframe, which is itself inside
* the MCP app's sandboxed iframe chain. Scripts and external resources (CDNs) are
* allowed since the sandbox isolation prevents any escape.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Comment is inaccurate: allow-popups does permit escape to external URLs.

Line 7 states "sandbox isolation prevents any escape," but the sandbox includes allow-popups, which allows content to call window.open() and navigate users to arbitrary external URLs (the popup window itself is also sandboxed by default, but the navigation target is fully trusted by the browser). For user-provided HTML with untrusted scripts or CDN-hosted payloads, this is a meaningful phishing vector.

Since no interactive feature in a read-only file preview requires popups, consider dropping allow-popups:

🐛 Proposed fix
-    return `<iframe class="html-rendered-frame" title="Rendered HTML preview" sandbox="allow-scripts allow-forms allow-popups" referrerpolicy="no-referrer" srcdoc="${escapeHtml(frameDocument)}"></iframe>`;
+    return `<iframe class="html-rendered-frame" title="Rendered HTML preview" sandbox="allow-scripts allow-forms" referrerpolicy="no-referrer" srcdoc="${escapeHtml(frameDocument)}"></iframe>`;

And update the header comment accordingly:

- * The rendered preview runs inside a nested sandboxed iframe, which is itself inside
- * the MCP app's sandboxed iframe chain. Scripts and external resources (CDNs) are
- * allowed since the sandbox isolation prevents any escape.
+ * The rendered preview runs inside a nested sandboxed iframe, which is itself inside
+ * the MCP app's sandboxed iframe chain. Scripts and external resources (CDNs) are
+ * allowed; allow-same-origin is intentionally omitted to prevent sandbox escape.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/ui/file-preview/src/components/html-renderer.ts` around lines 2 - 7,
Update the HTML preview renderer to remove the unsafe permission: drop
"allow-popups" from the iframe sandbox attribute used by the HtmlRenderer
component (the nested sandboxed iframe) so untrusted HTML/scripts cannot open
external popup windows; also update the top-of-file header comment that
currently asserts "sandbox isolation prevents any escape" to reflect that popups
can allow navigation and that "allow-popups" was removed to mitigate this
phishing/vector risk.

Comment on lines +87 to +99
/* When the host hides the summary row (hideSummaryRow: true), it means the host
provides its own outer frame/card. Strip inner chrome so only content remains. */
.tool-shell.host-framed .compact-row { display: none; }
.tool-shell.host-framed .panel {
margin-top: 0;
border: none;
border-radius: 0;
background: transparent;
}
.tool-shell.host-framed .panel-topbar { display: none; }
.tool-shell.host-framed .panel-footer { display: none; }
.tool-shell.host-framed .panel-content-wrapper { max-height: none; overflow: hidden; }
.tool-shell.host-framed .image-content { background: transparent; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

overflow: hidden on .panel-content-wrapper in host-framed mode silently clips tall content.

Line 98 sets max-height: none; overflow: hidden on the wrapper. The intent is clear — the host provides its own scroll container — but if the host's outer frame is shorter than the widget content, everything beyond the frame boundary is permanently hidden without a scrollbar. Consider whether overflow: visible is safer as a default, delegating clipping control fully to the host container.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/ui/styles/apps/file-preview.css` around lines 87 - 99, The
.panel-content-wrapper in host-framed mode currently sets "overflow: hidden"
which silently clips content when the host frame is smaller; update the
.tool-shell.host-framed .panel-content-wrapper rule (and related host-framed
rules) to remove or change overflow: hidden to overflow: visible (or omit the
overflow property entirely) so the widget doesn't permanently hide overflowed
content and leaves scroll/clipping behavior to the host container; ensure the
change is applied to the CSS selector .tool-shell.host-framed
.panel-content-wrapper in this file.

@wonderwhy-er wonderwhy-er merged commit bd51f2f into main Feb 24, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL This PR changes 1000+ lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants