Repository navigation
fix(web): render Codex visualizations inline - #7220
marijnbent wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
UI consistency review found three issues in the new inline-visualization UI: the frame's presentation depends on remote CDN assets that the desktop renderer's CSP blocks, the loading placeholder does not follow the repo's loading-state pattern, and the wide-mode width decision is split across two owners in a way that widens the message text column.
Posted via Macroscope — UI Consistency
| const VISUALIZATION_STYLES_URL = | ||
| "https://cdn.jsdelivr.net/gh/openai/codex@9ded177ce7c1c0bd2047f902936c177612ab3434/codex-rs/tui/src/inline_visualization/assets/visualize.css"; |
There was a problem hiding this comment.
All of the frame's styling comes from remote CDNs (cdn.jsdelivr.net here, unpkg.com for lucide on line 55), but a srcdoc iframe inherits the embedder's CSP in addition to its own <meta> policy. In the desktop renderer every app-scheme response gets default-src 'self'; style-src 'self' 'unsafe-inline'; script-src 'self' 'unsafe-inline' 'wasm-unsafe-eval' … (apps/desktop/src/electron/ElectronProtocol.ts, makeDesktopContentSecurityPolicy), so the stylesheet and the lucide script are blocked and the visualization renders unstyled/iconless on desktop — while docs/user/providers-codex.md in this PR states desktop shows these visualizations.
Suggested smallest fix: ship visualize.css (and lucide, if it is required) as a local asset served by the app instead of a pinned third-party CDN URL, so the frame's baseline presentation does not depend on origins the desktop CSP forbids. If remote hosts must stay, the desktop policy has to allow them for frames and the docs' desktop claim needs to be verified in a packaged build.
Posted via Macroscope — UI Consistency
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review This PR introduces a new feature (inline Codex visualization rendering) with substantial complexity across web, server, and desktop. Multiple unresolved comments identify potential CSP issues on desktop and UI/copy bugs that warrant human review before merging. You can customize Macroscope's approvability policy. Learn more. |
1c35642 to
88295b3
Compare
There was a problem hiding this comment.
Effect service conventions: two spots drop the underlying failure when wrapping real errors into AssetVisualizationUnavailableError. Sibling branches in the same code paths keep cause (e.g. AssetWorkspaceAssetInspectionError, AssetWorkspaceContextResolutionError), so the visualization path should do the same.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Two findings in apps/web/src/components/chat/InlineVisualization.tsx. The previously flagged desktop CSP issue is re-anchored because inlineVisualizationDocument.ts was folded back into this component, and the new fixed-height frame regresses row geometry relative to the removed height bridge. The earlier notes on the max-w-5xl row wrapper and the loading placeholder treatment still apply as posted.
Posted via Macroscope — UI Consistency
88295b3 to
660f676
Compare
There was a problem hiding this comment.
One minor convention nit on the new visualization asset branch in apps/server/src/ws.ts. The rest of the change (optional cause on AssetVisualizationUnavailableError, the reused inspectError mapper in AssetAccess.ts, subpath namespace Effect imports, no new runtimes or service-instance injection) matches the repository's Effect conventions.
Posted via Macroscope — Effect Service Conventions
AfterReal Codex visualize output rendered inline after this fix. 
Latest verification on 3164bfedb: 24 timeline tests, web type checking, targeted lint, and the React Compiler production build passed. Earlier review verification also passed 14 AssetAccess tests, 6 desktop protocol tests, and server and desktop type checks. The integrated Chrome attack check confirmed that a self-navigation attempt stayed on the thread and sent no request. |
There was a problem hiding this comment.
Reviewed the web UI changes for layout/theme ownership. Three findings on the new inline visualization surface and the timeline row wrapper it changed.
Posted via Macroscope — UI Consistency
660f676 to
aa5d1d5
Compare
There was a problem hiding this comment.
Two geometry/theming issues in InlineVisualization.tsx. Details inline.
Posted via Macroscope — UI Consistency
aa5d1d5 to
7eef150
Compare
There was a problem hiding this comment.
One finding: the new host-token bridge in InlineVisualization only reacts to light/dark, so switching between palettes of the same appearance leaves the frame's interior on the previous theme while its wrapper repaints immediately.
Posted via Macroscope — UI Consistency
7eef150 to
711cc30
Compare
github-actions
Bot
added
size:XL
There was a problem hiding this comment.
One finding: the new nested blob: frame in InlineVisualization cannot load under the desktop app's inherited CSP, so inline visualizations render as an empty card there.
Posted via Macroscope — UI Consistency
| return `<!doctype html><html><head> | ||
| <meta charset="utf-8"><meta http-equiv="Content-Security-Policy" content="${FRAME_HOST_CSP}"> | ||
| <style>html,body,iframe{width:100%;height:100%;margin:0;border:0}</style></head><body> | ||
| <iframe title="${escapeAttribute(title)}" sandbox="allow-scripts"></iframe> | ||
| <script>(()=>{const frame=document.querySelector("iframe");const url=URL.createObjectURL(new Blob([${serializeScriptString(visualizationDocument)}],{type:"text/html"}));frame.addEventListener("load",()=>URL.revokeObjectURL(url),{once:true});addEventListener("message",event=>{if(event.source===frame.contentWindow&&event.data?.type==="${HEIGHT_MESSAGE}"&&typeof event.data.height==="number")parent.postMessage(event.data,"*")});frame.src=url})()</script> |
There was a problem hiding this comment.
The nested blob: frame can't load in the desktop app. A srcdoc document inherits the embedder's CSP in addition to its own <meta> policy, and the Electron protocol handler stamps frame-src 'self' https://challenges.cloudflare.com onto every app-scheme response (apps/desktop/src/electron/ElectronProtocol.ts:93), so the host frame's frame-src blob: cannot widen the inherited policy — frame.src = url is refused and the card paints as an empty 384px bg-background block: the outer frame itself loads fine, so neither failed nor the asset._tag === "Failure" branch shows the "Visualization unavailable." state. This is the same inherited-policy mechanism as the earlier CDN issue, now hit by the host/blob hop rather than by remote assets.
Smallest fix: allow blob: in the desktop frame-src directive (alongside the existing worker-src 'self' blob:) while keeping the frame's own restrictive <meta> CSP, or serve the wrapper document from the asset route so it carries its own response CSP instead of inheriting the app's. Also worth confirming in the real app that the object URL loads at all: the host frame is sandbox="allow-scripts" without allow-same-origin, so its blob URL is minted on an opaque origin (blob:null/<uuid>), which browsers can also refuse to navigate to.
Posted via Macroscope — UI Consistency
711cc30 to
c7f9015
Compare
There was a problem hiding this comment.
One convention finding on the new visualization branch in AssetAccess.ts. Everything else (error definitions with structured attributes and preserved cause, environment-based dependency acquisition in readVisualizationFile/visualizationRevision, namespace subpath imports, and the inline error mapping in ws.ts) matches the conventions.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
One new finding on the timeline wrapper; see the inline comment.
Note: the nested blob: frame CSP problem in the desktop renderer flagged on an earlier commit still stands (apps/desktop/src/electron/ElectronProtocol.ts:93 still sends frame-src 'self' https://challenges.cloudflare.com, which a srcdoc host frame inherits and cannot widen). Not re-posted to avoid duplication.
Posted via Macroscope — UI Consistency
073a9c3 to
5d68216
Compare
There was a problem hiding this comment.
Two findings on the changed web UI code. (The [container-type:inline-size] stacking-context regression on MessagesTimeline.tsx:582 flagged in a previous review is still present and unaddressed — not re-posted here.)
Posted via Macroscope — UI Consistency
5d68216 to
77b33ee
Compare
There was a problem hiding this comment.
One finding on the new wide visualization geometry: it expands into the side gutter the timeline minimap explicitly reserves. Details inline.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
wide mode expands to the full container minus the scroller padding + scrollbar lanes (3.25rem), which is exactly the side gutter the timeline minimap relies on being empty: resolveTimelineMinimapHitStripWidth caps the strip to (viewportWidth - 48rem)/2 precisely so it "never extends past the gutter into the content column" (MessagesTimeline.logic.ts:102-122), and the rail is absolute inset-y-0 left-0 z-40 w-18 (MessagesTimeline.tsx:790) painting above the list.
For container widths in roughly 864–1128px the persistent-gutter rail is always visible and the wide frame's left edge lands at ~26px: the dots (left-3, up to w-6 → 12–36px) paint on top of the frame's border/rounded corner, and the pointer-events-auto hit strip (12–52px) swallows hover/click over the chart's left edge — hovering pops the w-80 message preview over the visualization, clicking jumps to a message instead of reaching the iframe.
Suggest keeping the wide frame out of the reserved rail, e.g. by also subtracting the strip width per side (or reusing the same measured gutter the minimap computes):
| ? "relative left-1/2 w-[min(64rem,calc(100cqw-3.25rem))] -translate-x-1/2" | |
| ? "relative left-1/2 w-[min(64rem,calc(100cqw-9.75rem))] -translate-x-1/2" |
Posted via Macroscope — UI Consistency
|
just keeps on going |
77b33ee to
3164bfe
Compare
There was a problem hiding this comment.
One finding on the wide visualization mode's width calculation. Everything else in the changed web UI looks consistent with the shared primitives and Tailwind ownership rules.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
wide mode can render narrower than the default mode. The default branch is w-full max-w-3xl, i.e. the full content column (100cqw minus the scroller's px-3/sm:px-5 and the two scrollbar lanes ≈ 100cqw - 3.25rem, capped at 48rem), while wide subtracts 9.75rem for the minimap gutters on both sides. Whenever the timeline container is under ~51rem — a split layout with the diff panel open, a narrow window, or the sidebar expanded — 100cqw - 9.75rem is ~104px smaller than the column a non-wide visualization gets, so a chart the model marked wide shrinks below its neighbours and pulls in from the column edges instead of bleeding out.
Suggest flooring the wide width at the normal column width so it can only ever grow:
| ? "relative left-1/2 w-[min(64rem,calc(100cqw-9.75rem))] -translate-x-1/2" | |
| ? "relative left-1/2 w-[min(64rem,max(100%,calc(100cqw-9.75rem)))] -translate-x-1/2" |
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3164bfedb45b6a6d65fbeed37a77ba765a5ca4b1. Configure here.
| threadRef={ctx.threadRef} | ||
| /> | ||
| ) : null, | ||
| )} |
There was a problem hiding this comment.
Copy keeps hidden visualize markers
Medium Severity
Assistant rendering now splits Codex visualize markers into inline previews, but copy still uses raw row.message.text, so clipboard output includes marker payloads users no longer see. The timeline minimap preview builds from the same raw text, so private-use or replacement-character marker junk can also appear there.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3164bfedb45b6a6d65fbeed37a77ba765a5ca4b1. Configure here.
3164bfe to
b97103d
Compare
b97103d to
6a0738b
Compare
|
thanks for working on inline visualizations. we are closing this implementation because the feature needs a durable artifact model rather than continued dependence on provider-generated temporary files.
this introduces executable HTML hosting, a new asset type, nested iframes, theme bridging, and wider chat layouts. those pieces serve the feature; file count alone is not the objection. the persistence, identity, and lifecycle behavior need to be designed together before this becomes a supported artifact type. the sandboxing and file checks show care. this decision is not a claim of a demonstrated sandbox escape or careless security work. a new pr is welcome with durable message-bound artifacts, preserved interactive state, and a clear cross-client behavior. if you disagree with this assessment, please link back to this pr and explain how those concerns are addressed. closed at the request of @StiensWout. |
1 participant
Fixes #7191
Problem
Codex visualize references appeared as raw marker text, so users had to find and open the generated HTML file themselves.
Fix
Proof
Before:
After: see the proof comment below for a real Codex-generated visualization rendered inline.
Verification
git diff --checkpassed.visualizeresult.Implemented with GPT-5.6-Sol through Codex in T3 Code.
Note
Render Codex visualizations inline in assistant messages
splitInlineVisualizations(new shared module) and rendered as sandboxed iframes via the newInlineVisualizationcomponent, replacing raw marker text.ProjectionSnapshotQuery.hasAssistantVisualizationReferenceis added to confirm a path is referenced by an assistant message before a signed URL is issued, preventing unauthorized access.blob:inframe-srcso blob-URL iframes can load in Electron.Macroscope summarized 3164bfe.
Note
Medium Risk
Untrusted HTML is executed in the browser inside sandboxed iframes with layered CSP and server-side path gating, but any sandbox or CSP gap would be client-side code execution.
Overview
Assistant messages can now show Codex
visualizemarkers as embedded previews instead of raw marker text. Shared parsing splits message text into markdown and visualization parts (standalone markers only; fenced code stays markdown).The web
InlineVisualizationcomponent requests a signedvisualizationasset URL, fetches the HTML fragment, and runs it in a nested sandboxedblob:iframe with strict CSP, auto height, and host theme tokens. Wide mode can extend past the normal chat column; timeline layout uses container queries and overflow tweaks.On the server, asset access adds a
visualizationresource: bounded temp.htmlfiles (size cap, canonical path, no links outside allowed tmp roots), SHA-256 revision on the token, and resolution as inert text with sandbox response headers—not as a direct file download.assetsCreateUrlonly issues visualization URLs whenhasAssistantVisualizationReferencefinds that path in an assistant message for the thread (batched projection scan). Desktop CSPframe-srcnow allowsblob:for the nested iframe host.Reviewed by Cursor Bugbot for commit 3164bfedb45b6a6d65fbeed37a77ba765a5ca4b1. Bugbot is set up for automated code reviews on this repo. Configure here.