Render HTML artifacts in the guest pool - #94
Conversation
📝 WalkthroughWalkthroughHTML artifacts now render through task-scoped pooled webview guests. The change adds target contracts, lifecycle management, navigation controls, overlay coordination, isolated storage, and Studio integration. ChangesArtifact target contracts and isolation
Workspace lifecycle and RPC coordination
Shared guest controls and overlay coordination
Studio artifact preview integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant HtmlArtifactPreview
participant ArtifactPreviewRPC
participant Workspace
participant BrowserGuest
User->>HtmlArtifactPreview: Open HTML artifact
HtmlArtifactPreview->>ArtifactPreviewRPC: Request task preview target
ArtifactPreviewRPC->>Workspace: Register target and acquire presence
Workspace->>BrowserGuest: Create or reuse artifact guest
HtmlArtifactPreview->>BrowserGuest: Attach slot and navigate entry URL
BrowserGuest-->>HtmlArtifactPreview: Navigation and load state
HtmlArtifactPreview-->>User: Render preview controls and content
User->>HtmlArtifactPreview: Close or change tab
HtmlArtifactPreview->>ArtifactPreviewRPC: Release presence
ArtifactPreviewRPC->>Workspace: Start grace-period lifecycle
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
HTML file artifacts rendered in a sandboxed, opaque-origin `<iframe>` while the
agent loaded the identical asset URL in a `<webview>` guest. Everything
origin-scoped therefore behaved differently between the surface the agent
screenshots to check its own work and the surface the user reads: localStorage,
cookies, and same-origin fetch of a sibling file. Agent-authored HTML that
persists a selection, or reads its own data.json, could pass the agent's check
and fail in front of the user.
Both now render as a real origin through the same pool, which also gives the
preview real back/forward/reload and makes escape-to-root a loadURL from any
depth rather than a remount of `src`. Both reload nonces are gone.
The artifact guest is a second target kind, `${taskId}/artifact`, with its own
RPC, its own lifetime machine (a presence lease plus a 30s grace period, without
task-browser's agent-idle clock or agent-browser daemon fan-out), and its own
storage profile. It is excluded from listTargets and refused by the CDP bridge,
so the agent's target list and CDP surface are unchanged.
Net security tightening rather than the loss the finding assumed: the iframe's
allow list granted camera, microphone, geolocation, clipboard and USB, and its
sandbox granted popups; the guest session denies every permission and every
window open. What is new is storage, scoped to a per-task asset origin in a
profile separate from the agent's browsing profile.
Also fixes a pre-existing bug this would otherwise inherit: a body-mounted guest
kept painting through an app-wide modal's overlay, since opening a dialog is not
a tab switch and nothing else told the slot it was covered.
The artifact panel and the expand modal are two mounted previews of a single pooled guest, and three review findings were the same mistake: something singular -- the current page, the Cmd+F opener, the presence lease -- was driven from a host's own props, which cannot say which host owns it. - Expanding threw the reader back to the entry page, because the modal's mount ran the same navigate-on-mount effect the panel does. A host mounting over a guest another preview is already showing now adopts the page on screen; later runs navigate as before, so go-home still works from the modal. - Cmd+F died after closing the modal. The find opener is a single slot: the modal claimed it, cleared it on unmount, and the panel never re-registered. useBrowserFind takes the same `covered` signal the slot does. - Every backgrounded tab pinned a webContents, since presence was leased on mount rather than on being the foreground tab, so the machine never left Observed. Gated on isActiveTab like the session browser's lease. The open effect is gated too, or a reaped background tab would re-create the guest and feed the reaper every grace period. Separately, the preview's overflow menu had no window-blur dismiss, so clicking into the guest left it stuck open over the page. The browser panel had already solved this; the logic moves to a shared useGuestMenuState. The new toolbar's icon-only buttons carried tooltips but no accessible name, which icon-button-accessible-name.test.ts flags: a tooltip is portalled and never reaches assistive clients or the scripts that drive Studio by name.
7cda8ab to
e7c50a6
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Both come from the same place as the last round: a single guest with more than one thing acting on it, where the timing of who acts first was assumed rather than established. Switching to a different HTML file showed the previous file's page. The mounted-host count that decides whether to adopt a live page or navigate is read while rendering the incoming host, and React runs every cleanup after that, so the outgoing host was still counted and the new file looked like a second viewer of the old one. Keying the count by file as well as target makes a different file a different key, which is the only pair that should ever share a page. A lease arriving while the guest was being closed was dropped, because Stopping handled no presence events. The machine reached Stopped, the parent forgot it, and nothing re-leased -- so the panel's open effect rebuilt a guest that was reaped again every grace period for as long as the user kept looking at it. Stopping now keeps counting and returns to Observed if someone is still watching once the close settles. A forced reap is exempt: trashing a task is final, and a lease landing in the same tick must not revive a preview for a task on its way out.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Being raised above the expand modal's overlay was treated as immunity from every overlay, so opening Settings over an expanded HTML file left the page painted on top of the dialog. The two are independent slots -- the file viewer runs off taskFileViewerAtom and settings off studioModalAtom -- so a menu accelerator reaches one straight through the other, and an app-wide dialog draws at z-50 while the raised guest sits at 51. Split the overlay signal into its two sources so a host can ignore the one it lives inside and still park for the one above it.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The profile was one shared directory, on the reasoning that the distinct asset origins already isolate tasks from each other. That holds for localStorage and IndexedDB, which are keyed by origin, but cookies are scoped by domain: a page served from assets.<a>.localhost can set one for localhost, and every preview sharing a jar would then send and read it. Both ends are untrusted agent-authored HTML, so that is a channel between two tasks -- and a new one, since the opaque-origin iframe had no storage at all. A directory per task closes it. It stays beside the workspace's other private state rather than moving into the task folder, so a Chromium profile never lands somewhere the user browses or exports; trash-task removes it with the task, best-effort, after the guest that owned it is already reaped. The docs asserted the origin-only reasoning, so they are corrected rather than just updated.
| // The artifact preview's storage profile lives outside the task folder | ||
| // (a Chromium profile has no business in a directory the user browses), | ||
| // so trashing the task does not take it. Best-effort: a profile left | ||
| // behind is dead weight, not a correctness problem, and it must not | ||
| // fail the deletion the user asked for. The guest that owned it is | ||
| // already reaped by the time we get here. | ||
| const previewProfile = getArtifactPreviewSessionDir(taskId); | ||
| if (await pathExists(previewProfile)) { | ||
| await rmrf(previewProfile).catch(() => { | ||
| // Nothing to do: the task itself is gone. | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟡 Deleting a task can leave its preview's browser data behind on disk
The preview's stored browser data is deleted (rmrf(previewProfile) at packages/workspace/src/lib/trash-task.ts:80) without ever waiting for the preview window that owns it to be shut down, so the deletion can fail and leave the data behind forever.
Impact: Deleting a task can silently leave a folder of that task's preview browser data on disk, which nothing ever cleans up.
Nothing awaits artifactPreview.stopped, unlike the taskBrowser reap
trashTask blocks on browserReaped (packages/workspace/src/lib/trash-task.ts:35-44), which is resolved only by taskBrowser.stopped (packages/workspace/src/machines/workspace/index.ts handleTaskBrowserStopped). The artifact preview is sent forceReap in prepareToTrashTask but is deliberately excluded from matchingTaskIds, so no resolver waits on artifactPreview.stopped. The reap is asynchronous (Stopping invokes closeTargetLogic, then the guest's destroyed/detach chain runs), so by the time rmrf executes the Chromium session at artifact-preview-session/<taskId> may still be open. On Windows in particular an open profile holds file locks and fs.rm throws; the .catch(() => {}) swallows it and nothing retries. The inline comment "The guest that owned it is already reaped by the time we get here" is therefore not guaranteed by the code.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Mirror the guest's URL into the address bar. Held in local state (rather | ||
| // than read straight off the hook) because the user types into the same box. | ||
| useEffect(() => { | ||
| if (!editingUrlRef.current) { | ||
| setDraftUrl(guest.url); | ||
| } | ||
| }, [guest.url]); |
There was a problem hiding this comment.
🟡 Address bar can keep half-typed text after the page reloads to the same address
The address bar is only refreshed when the page's address changes value (useEffect(..., [guest.url]) at apps/studio/src/client/components/task/browser-panel.tsx:116-120), so a reload or a navigation back to the identical address leaves whatever the user half-typed sitting in the bar.
Impact: After typing in the address bar without pressing Enter and then reloading, the bar keeps showing the abandoned text instead of the page's real address.
Event-driven sync replaced by a value-diffed effect
The previous implementation called sync() directly from every did-navigate / did-navigate-in-page handler, which unconditionally wrote setDraftUrl(webview.getURL()). The refactor moves the guest URL into useGuestNavigation's url state (apps/studio/src/client/hooks/use-guest-navigation.ts:69-71) and mirrors it via an effect keyed on guest.url. When a navigation resolves to the same URL, setUrl(next) is a no-op for React, guest.url never changes identity, and the mirroring effect does not re-run — so draftUrl, which the user mutated via onChange, is never restored. Focus/blur alone does not reset it either (onBlur only clears editingUrlRef).
Prompt for agents
apps/studio/src/client/components/task/browser-panel.tsx now mirrors the guest URL into the address bar with an effect keyed on `guest.url`. Because useGuestNavigation stores the URL in state, a navigation that resolves to the identical URL (a reload, a re-submit of the same address) does not change that state, so the effect never re-runs and any text the user typed but never submitted stays in the box. The old code re-synced on every did-navigate event regardless of whether the URL changed. Consider exposing a navigation counter/epoch from useGuestNavigation (bumped on every did-navigate/did-fail-load) and keying the mirror effect on that as well, or having the panel reset draftUrl on blur.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/workspace/src/machines/workspace/index.ts (1)
716-736: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftFence artifact opens during task disposal.
createArtifactTargetpublishes a manager entry before its attach promise resolves (apps/studio/src/electron-main/browser-view/manager.ts:394-403), butartifactPreview.openregisters it only after that await (packages/workspace/src/rpc/routes/artifact-preview.ts:31-44).prepareToTrashTaskonly reaps actor refs already inartifactPreviewRefs(packages/workspace/src/machines/workspace/index.ts:716-736). A pending open can therefore register a new actor after disposal starts, whiletrashTaskremoves the task directory and profile (packages/workspace/src/lib/trash-task.ts:70-83).
packages/workspace/src/machines/workspace/index.ts#L716-L736: Track in-flight artifact operations and wait for artifact shutdown before cleanup proceeds.packages/workspace/src/rpc/routes/artifact-preview.ts#L31-L44: Coordinate target creation with task disposal and close a target when disposal wins the race.packages/workspace/src/machines/workspace/index.ts#L267-L291: Reject late registration for a disposing or deleted task instead of spawning a new actor.packages/workspace/src/lib/trash-task.ts#L72-L83: Remove the profile only after the artifact disposal barrier resolves.Add a regression test that starts an open, begins disposal before attach settles, then settles the open and verifies that no target or profile remains.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workspace/src/machines/workspace/index.ts` around lines 716 - 736, Fence artifact opens during task disposal: in packages/workspace/src/machines/workspace/index.ts:716-736, track in-flight artifact operations and wait for their shutdown before cleanup; in packages/workspace/src/rpc/routes/artifact-preview.ts:31-44, coordinate target creation with disposal and close the target if disposal wins; in packages/workspace/src/machines/workspace/index.ts:267-291, reject late registration for disposing or deleted tasks; in packages/workspace/src/lib/trash-task.ts:72-83, remove the profile only after the artifact disposal barrier resolves. Add a regression test covering an open whose attach settles after disposal begins, verifying no target or profile remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/studio/src/client/components/html-artifact-preview.tsx`:
- Around line 136-156: Update the error-rendering branch alongside the artifact
preview mutation to add an in-place retry control that invokes openPreview with
the current taskId. Preserve the existing error message and use the mutation’s
openFailed state to show the control only when target creation fails.
- Around line 360-366: Update the retry callback in the guest.loadError
rendering to call guest.navigateTo with the failed URL from guest.loadError,
falling back to entryUrl when that URL is unavailable. Preserve the existing
GuestLoadErrorNotice wiring and retry behavior.
- Around line 203-219: When the modal closes (active transitions from true to
false), the guest has navigated to a different file but the panel state remains
unchanged, causing a desynchronization. Add restoration logic in the useEffect
to sync the panel file state or restore the guest navigation back to entryUrl
when active becomes false. This could be done by navigating the guest back to
entryUrl before the early return when !active, or by syncing
artifactPanel.filePath and artifactPanel.modifiedAt to reflect the panel's
original file context instead of the modal's file.
In `@apps/studio/src/client/components/task/browser-panel.tsx`:
- Around line 103-112: Read useIsGuestCovered() once into a shared covered value
in the browser panel, then pass that value to both useBrowserFind and
useBrowserSlot. Update the useBrowserFind invocation so its coverage dependency
changes alongside the slot, allowing the shared opener to be restored after
overlay unmounts.
In `@apps/studio/src/client/components/task/file-viewer-modal.tsx`:
- Around line 22-26: Raise the carousel controls rendered by the file viewer
modal above GUEST_Z_INDEX so they remain clickable when the body-mounted HTML
guest is visible. Update the controls near DialogPrimitive.Content and its
previous/next buttons to use a body-level or guest-aware layer above 51, while
preserving their existing positioning and behavior.
In `@apps/studio/src/client/hooks/use-guest-navigation.ts`:
- Around line 43-49: Update the state and rebind logic in useGuestNavigation so
loadError is scoped to the current targetId rather than surviving across
sessions. Clear the error when targetId changes and when the target becomes
inactive, ensuring useBrowserSlot and TaskBrowserPanel cannot reuse the previous
session’s parked state or URL. Add a regression test covering a failed session
followed by switching to a new session.
In `@packages/workspace/src/machines/artifact-preview.ts`:
- Around line 178-205: The `Stopping` state at
packages/workspace/src/machines/artifact-preview.ts line 178-205 does not handle
registerTarget events in its on: object (currently showing only acquirePresence
and releasePresence), causing incoming target registrations to be silently
dropped during teardown. Add registerTarget handling to the on: object in the
Stopping state to ensure targets registered during teardown are either closed or
transferred before the machine reaches Stopped. Apply the same registerTarget
event handling to the grace-period teardown path at
packages/workspace/src/machines/artifact-preview.ts line 124-145 to preserve
consistency. Finally, add a regression test at
packages/workspace/src/machines/artifact-preview.test.ts line 216-280 that
transitions to Stopping, sends a registerTarget event before closeTargetLogic
settles, and asserts that the new target is closed or rejected rather than left
dangling.
---
Outside diff comments:
In `@packages/workspace/src/machines/workspace/index.ts`:
- Around line 716-736: Fence artifact opens during task disposal: in
packages/workspace/src/machines/workspace/index.ts:716-736, track in-flight
artifact operations and wait for their shutdown before cleanup; in
packages/workspace/src/rpc/routes/artifact-preview.ts:31-44, coordinate target
creation with disposal and close the target if disposal wins; in
packages/workspace/src/machines/workspace/index.ts:267-291, reject late
registration for disposing or deleted tasks; in
packages/workspace/src/lib/trash-task.ts:72-83, remove the profile only after
the artifact disposal barrier resolves. Add a regression test covering an open
whose attach settles after disposal begins, verifying no target or profile
remains.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 38e7d042-7390-473c-b084-4330cb6e8b1d
📒 Files selected for processing (36)
apps/studio/src/client/atoms/studio-modal.tsapps/studio/src/client/components/file-viewer.tsxapps/studio/src/client/components/html-artifact-preview.tsxapps/studio/src/client/components/sandboxed-html-iframe.tsxapps/studio/src/client/components/task/browser-panel.tsxapps/studio/src/client/components/task/file-viewer-modal.tsxapps/studio/src/client/components/task/guest-load-error.tsxapps/studio/src/client/components/task/view.tsxapps/studio/src/client/hooks/use-browser-find.tsapps/studio/src/client/hooks/use-browser-slot.tsapps/studio/src/client/hooks/use-guest-covered.tsapps/studio/src/client/hooks/use-guest-menu-state.tsapps/studio/src/client/hooks/use-guest-navigation.tsapps/studio/src/client/lib/browser-pool.tsapps/studio/src/electron-main/browser-view/debug-snapshot.tsapps/studio/src/electron-main/browser-view/entry.tsapps/studio/src/electron-main/browser-view/manager.tsdocs/architecture/in-app-browser.mddocs/findings/html-artifact-iframe-navigation.mddocs/plans/active/html-artifacts-in-the-guest-pool.mdpackages/workspace/src/client.tspackages/workspace/src/constants.tspackages/workspace/src/electron.tspackages/workspace/src/lib/task-dir-utils.tspackages/workspace/src/lib/trash-task.tspackages/workspace/src/logic/server/routes/cdp-bridge.tspackages/workspace/src/machines/artifact-preview.test.tspackages/workspace/src/machines/artifact-preview.tspackages/workspace/src/machines/task-browser.test.tspackages/workspace/src/machines/workspace/index.tspackages/workspace/src/machines/workspace/types.tspackages/workspace/src/rpc/index.tspackages/workspace/src/rpc/routes/artifact-preview.tspackages/workspace/src/test/helpers/mock-task-config.tspackages/workspace/src/test/setup.tspackages/workspace/src/types.ts
💤 Files with no reviewable changes (1)
- apps/studio/src/client/components/sandboxed-html-iframe.tsx
| useEffect(() => { | ||
| if (!active) { | ||
| return; | ||
| } | ||
| const isAdoptingLivePage = | ||
| !hasNavigatedRef.current && | ||
| isSecondaryHost && | ||
| Boolean(guest.currentUrl()); | ||
| hasNavigatedRef.current = true; | ||
| if (!isAdoptingLivePage) { | ||
| guest.navigateTo(entryUrl); | ||
| } | ||
| // `guest` is rebuilt every render; navigating is keyed on the file, on the | ||
| // guest becoming available, and on the go-home gesture, not on that | ||
| // identity. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [active, entryUrl, goHomeNonce, isSecondaryHost, targetId]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the state writer and both viewer call paths before inspecting behavior.
ast-grep outline apps/studio/src/client/atoms/task-file-viewer.ts --items all
ast-grep outline apps/studio/src/client/components/task/view.tsx --match TaskView --view expanded
ast-grep outline apps/studio/src/client/components/task/file-viewer-modal.tsx --match TaskFileViewerModal --view expanded
# Expect either route synchronization or explicit guest restoration on modal close.
rg -n -C 8 '\b(setTaskFileViewerIndexAtom|openFileViewerAtom|artifactPanel|FileViewer)\b' \
apps/studio/src/client/atoms/task-file-viewer.ts \
apps/studio/src/client/components/task/view.tsx \
apps/studio/src/client/components/task/file-viewer-modal.tsx \
apps/studio/src/client/components/html-artifact-preview.tsxRepository: instrument-org/instrument
Length of output: 20664
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect modal navigation/closing behavior and the panel route derivation around target isolation.
sed -n '40,90p;150,195p' apps/studio/src/client/components/task/file-viewer-modal.tsx
sed -n '80,230p' apps/studio/src/client/components/task/view.tsx
rg -n -C 5 '(artifactPanel:|searchState|artifactPanel|currentFile|handleFileSelect|FileViewer|setArtifactHomeNonce|openFileViewerAtom|setTaskFileViewerIndexAtom|currentFile)' \
apps/studio/src/client/components/task/file-viewer-modal.tsx \
apps/studio/src/client/components/task/view.tsx \
apps/studio/src/client/atoms/task-file-viewer.tsRepository: instrument-org/instrument
Length of output: 33333
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect HTML artifact preview host behavior and FileViewer ownership paths.
sed -n '1,240p' apps/studio/src/client/components/html-artifact-preview.tsx
sed -n '340,390p' apps/studio/src/client/components/html-artifact-preview.tsx
rg -n -C 4 '\b(guest\.navigateTo|isSecondaryHost|currentUrl|onCurrentUrl|openFileViewer|closeFileViewer|onExpand|onClose)\b' \
apps/studio/src/client/components/html-artifact-preview.tsx \
apps/studio/src/client/components/file-viewer.tsx \
apps/studio/src/client/components/task/view.tsxRepository: instrument-org/instrument
Length of output: 29837
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the useGuestNavigation source and related guest leasing/overlay cleanup to resolve the ownership handoff.
git ls-files | rg 'use-guest-navigation|browser-target|guest|slots|workspace/artifactPreview'
rg -n -C 5 'function useGuestNavigation|useEffect\(\\(\\) => \\{(?!\\n(?:\\s*\\/\\*|\\/\\/|return|active))\\s*\\[[^\\]]*guest.*useGuestNavigation' \
apps/studio/src/client/hooks \
apps/studio/src/client/components \
apps/studio/src/client/atoms \
apps/studio/src/client/rpc \
apps/backend srcRepository: instrument-org/instrument
Length of output: 889
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect guest navigation state and slot cleanup behavior.
sed -n '1,240p' apps/studio/src/client/hooks/use-guest-navigation.ts
sed -n '1,220p' apps/studio/src/client/hooks/use-browser-slot.ts
sed -n '1,220p' apps/studio/src/client/hooks/use-guest-covered.ts
# Search for guest ownership/cleanup paths that may clear the modal file host.
rg -n -C 5 'active=|targetId|useBrowserSlot|useGuestNavigation|navigateTo\(|currentUrl\(\)\?|isSecondaryHost|fileViewerModalOpen|closeFileViewerAtom|mountedHosts' \
apps/studio/src/client/hooks \
apps/studio/src/client/components/html-artifact-preview.tsx \
apps/studio/src/client/components/task/view.tsx \
apps/studio/src/client/atoms/task-file-viewer.tsRepository: instrument-org/instrument
Length of output: 50381
Restore the panel’s file URL after modal file navigation.
setTaskFileViewerIndexAtom only changes modal state and onBeforeLoad closes the modal, so A → modal B → close can leave the task route still pointing at A while the shared guest has navigated to B. Sync artifactPanel.filePath/artifactPanel.modifiedAt when modal navigation changes files, or restore the panel host to its entry URL when the modal closes. Add a regression for A → modal B → close.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/studio/src/client/components/html-artifact-preview.tsx` around lines 203
- 219, When the modal closes (active transitions from true to false), the guest
has navigated to a different file but the panel state remains unchanged, causing
a desynchronization. Add restoration logic in the useEffect to sync the panel
file state or restore the guest navigation back to entryUrl when active becomes
false. This could be done by navigating the guest back to entryUrl before the
early return when !active, or by syncing artifactPanel.filePath and
artifactPanel.modifiedAt to reflect the panel's original file context instead of
the modal's file.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Two things an agent-written report can ordinarily do worked on the sandboxed iframe by inheriting the host page's behavior, and the guest's stricter defaults silently took both away. A `target="_blank"` link used to bubble to the main window's window-open handler, which sends it to the OS browser. The artifact guest denied every open, so such links became inert -- and with no address bar, the user had no other way to follow one. It still opens no child window of its own, but hands http(s) opens to openExternal. The decision is a predicate so the policy module stays free of Electron runtime imports and testable; the manager does the opening, and loads open-external on demand because its exception capture reaches the preferences and app-state stores, which breaks node tests of unrelated modules that transitively import the manager. Downloads were cancelled outright: the guest session refuses anything without an agent-authorized save path, which is right for an agent-driven guest and wrong for a preview. The iframe carried `allow-downloads`, so a "Download CSV" button in a report used to produce a save dialog. An artifact guest now leaves the item to Electron, which prompts.
…guest The browser panel had the same find bug the preview did: it parks its guest under an overlay but kept claiming the single Cmd+F opener slot, so the overlay's host took it, cleared it on unmount, and the panel never re-registered. Both hooks now read one coverage value. A load failure was stored without the guest it happened on. The browser panel swaps targetId in place when the session changes, with no remount, so the error survived into the next guest -- parking it behind a notice naming the previous session's URL. Stamped and filtered on read, so no failed page is briefly shown as fine on the way through. A target registered while teardown was already running was dropped. The close captured the id it was given on entry, so that guest was one nothing would ever close. Recorded and closed on a second pass, or kept if a lease means it is now the live one. The panel and the modal can also disagree about which file is on screen: the carousel moves the shared guest while the panel behind still owns the route naming a different one. A host becoming visible again re-asserts its own file, while leaving a sub-page inside that file alone -- which is what keeps following a link and collapsing the modal from resetting the page. Carousel arrows cannot be lifted over the guest, since their z-50 content is a stacking context the body-mounted guest sits outside. The guest is inset past them instead, and only when they exist. Also: "Try again" on a failed load retries that URL rather than the artifact's entry page, and a failed open now offers a retry, since the open effect fires once per state by design and will not retry itself.
A guest that goes away takes the page with it and its replacement arrives on about:blank, but the host still believed the file was already in place -- so backgrounding a tab past the grace period, or a crash in untrusted artifact HTML, left a blank preview with no error and no way back except Home. The host now forgets what was pointed where when its guest goes, and treats the next one as somewhere to navigate. Verified by crashing a guest under a mounted host. The preview's own menu and tooltips portal to the body at z-50, which a raised guest sits above, so inside the expand modal they opened underneath the page they belong to. They cannot be lifted from within their host -- that content is a stacking context the guest is not in -- so they are raised alongside it. Coming forward in a second tab did not re-assert that tab's file either, so two tabs on one task could show a file the route did not name. The destruction watcher this machine's `targetDestroyedExternally` handler depends on was never wired, so the handler and its test described something production could not reach. Wired the way taskBrowser does it, and the test now fires it through the subscription rather than by hand. Also: the chat's image/diagram preview is a third full-window dialog a guest can be showing behind, so it joins the ones a host parks for; and trash-task's comment claimed the preview guest was already reaped when it gets there, which is not something anything waits for.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/studio/src/electron-main/browser-view/manager.ts (1)
400-407: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWait for the attachment handshake before reuse.
bindGuestsetsentry.webContentsbefore it resolvesentry.attachatapps/studio/src/electron-main/browser-view/manager.ts:153-154andapps/studio/src/electron-main/browser-view/manager.ts:279-285. A concurrentcreateTargetcall returns at Line 404 before the guest has a RenderFrame and debugger setup. A caller can then send CDP commands during the state that Lines 275-282 identify as able to hang.Always await
waitForAttach(existing)for an existing entry. Add a regression test that issues two creates afterdid-attach-webviewand beforedid-finish-load.Proposed fix
const existing = entries.get(targetId); if (existing) { - // Idempotent: a target id owns at most one guest. - // Already bound -> reuse it; mount still in flight -> wait on it. - if (existing.webContents && !existing.webContents.isDestroyed()) { - return Promise.resolve({ targetId }); - } return waitForAttach(existing).then(() => ({ targetId })); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/studio/src/electron-main/browser-view/manager.ts` around lines 400 - 407, Update the existing-entry branch in bindGuest so it always awaits waitForAttach(existing) before returning, including when existing.webContents is already set and alive; remove the immediate Promise.resolve reuse path. Add a regression test covering two create requests issued after did-attach-webview but before did-finish-load, verifying both wait for attachment completion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/studio/src/client/components/task/file-viewer-modal.tsx`:
- Around line 194-199: Update FileViewer and its header menu flow to accept and
forward a z-index value of GUEST_Z_INDEX + 1, reusing the existing floatingStyle
pattern. Apply that value to both the main DropdownMenuContent and nested
submenu content so menu rows render above the raised guest while preserving
existing behavior otherwise.
In `@apps/studio/src/electron-main/browser-view/window-open-policy.test.ts`:
- Line 100: Remove the duplicated closing `satisfies { name: string; overrides:
Partial<HandlerDetails> }[]` expressions at the ends of both test-case
definitions in the `it.each` blocks. Keep one valid closing expression per block
so the test file parses correctly.
In `@packages/workspace/src/machines/artifact-preview.ts`:
- Around line 61-63: Include the destroyed targetId when sending
targetDestroyedExternally from the onTargetDestroyed callback, then update the
corresponding handler around the targetDestroyedExternally transition to clear
context.targetId and stop only when the event ID matches the current target. Add
a regression test covering target B being registered during target A teardown,
A’s destruction callback firing, and B remaining adopted when presence exists.
---
Outside diff comments:
In `@apps/studio/src/electron-main/browser-view/manager.ts`:
- Around line 400-407: Update the existing-entry branch in bindGuest so it
always awaits waitForAttach(existing) before returning, including when
existing.webContents is already set and alive; remove the immediate
Promise.resolve reuse path. Add a regression test covering two create requests
issued after did-attach-webview but before did-finish-load, verifying both wait
for attachment completion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d28a928e-3de0-4e12-afda-31fa2e83c68c
📒 Files selected for processing (17)
apps/studio/src/client/components/file-viewer.tsxapps/studio/src/client/components/html-artifact-preview.tsxapps/studio/src/client/components/task/browser-panel.tsxapps/studio/src/client/components/task/file-viewer-modal.tsxapps/studio/src/client/hooks/use-guest-covered.tsapps/studio/src/client/hooks/use-guest-navigation.tsapps/studio/src/electron-main/browser-view/downloads.tsapps/studio/src/electron-main/browser-view/manager.tsapps/studio/src/electron-main/browser-view/window-open-policy.test.tsapps/studio/src/electron-main/browser-view/window-open-policy.tsdocs/architecture/in-app-browser.mddocs/findings/html-artifact-iframe-navigation.mddocs/plans/active/html-artifacts-in-the-guest-pool.mdpackages/workspace/src/constants.tspackages/workspace/src/lib/trash-task.tspackages/workspace/src/machines/artifact-preview.test.tspackages/workspace/src/machines/artifact-preview.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/workspace/src/constants.ts
- docs/architecture/in-app-browser.md
- docs/findings/html-artifact-iframe-navigation.md
- apps/studio/src/client/components/task/browser-panel.tsx
- apps/studio/src/client/components/file-viewer.tsx
- packages/workspace/src/machines/artifact-preview.test.ts
- apps/studio/src/client/hooks/use-guest-navigation.ts
- apps/studio/src/client/hooks/use-guest-covered.ts
- docs/plans/active/html-artifacts-in-the-guest-pool.md
- packages/workspace/src/lib/trash-task.ts
| <FileViewer | ||
| file={currentFile} | ||
| guestSideGutter={hasMultipleFiles ? GUEST_SIDE_GUTTER : 0} | ||
| guestZIndex={GUEST_Z_INDEX} | ||
| onClose={collapseViewer} | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Raise the FileViewer header menu above the guest.
apps/studio/src/client/components/html-artifact-preview.tsx:275-281 establishes that a raised guest renders above normal z-50 portals. Line 197 enables that mode, but the FileViewer header menu at apps/studio/src/client/components/file-viewer.tsx:466-706 has no elevated style. Its shared primitive uses z-50 at apps/studio/src/client/components/ui/dropdown-menu.tsx:40-64.
When menu rows overlap the HTML guest, the guest receives the clicks. Forward GUEST_Z_INDEX + 1 through FileViewer to its DropdownMenuContent and nested submenu content. Reuse the existing floatingStyle pattern.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/studio/src/client/components/task/file-viewer-modal.tsx` around lines
194 - 199, Update FileViewer and its header menu flow to accept and forward a
z-index value of GUEST_Z_INDEX + 1, reusing the existing floatingStyle pattern.
Apply that value to both the main DropdownMenuContent and nested submenu content
so menu rows render above the raised guest while preserving existing behavior
otherwise.
| { name: "http new tab", overrides: { url: "http://example.test/docs" } }, | ||
| { name: "background tab", overrides: { disposition: "background-tab" } }, | ||
| { name: "new-window popup", overrides: { disposition: "new-window" } }, | ||
| ] satisfies { name: string; overrides: Partial<HandlerDetails> }[])( |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicated satisfies clauses.
Line 100 repeats the closing expression from Line 99. Line 111 repeats the closing expression from Line 110. Each second expression is outside the it.each call and makes apps/studio/src/electron-main/browser-view/window-open-policy.test.ts fail to parse.
Also applies to: 111-111
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/studio/src/electron-main/browser-view/window-open-policy.test.ts` at
line 100, Remove the duplicated closing `satisfies { name: string; overrides:
Partial<HandlerDetails> }[]` expressions at the ends of both test-case
definitions in the `it.each` blocks. Keep one valid closing expression per block
so the test file parses correctly.
| input.browser.onTargetDestroyed(input.targetId, () => { | ||
| sendBack({ type: "targetDestroyedExternally" }); | ||
| }), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Ignore destruction events from non-current targets.
packages/workspace/src/machines/artifact-preview.ts:61-63 sends targetDestroyedExternally without the watched targetId. packages/workspace/src/machines/artifact-preview.ts:175-178 then clears context.targetId for every such event.
If target A is closing and late registration installs target B, the destruction callback for A can clear B. The next Stopping pass receives null, so it does not close or retain B. This can leave a blank observed preview or orphan the guest.
Include the destroyed targetId in the event. Only clear and stop when it equals context.targetId. Add a regression test that registers B during A’s teardown, fires A’s destruction callback, and verifies that B remains adopted when presence exists.
As per coding guidelines, “Flag deterministic tool, prompt, message-assembly, or state-management defects that make agent turns fail, hang, or silently drop state.”
Also applies to: 175-178
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/workspace/src/machines/artifact-preview.ts` around lines 61 - 63,
Include the destroyed targetId when sending targetDestroyedExternally from the
onTargetDestroyed callback, then update the corresponding handler around the
targetDestroyedExternally transition to clear context.targetId and stop only
when the event ID matches the current target. Add a regression test covering
target B being registered during target A teardown, A’s destruction callback
firing, and B remaining adopted when presence exists.
Source: Coding guidelines
| export function shouldOpenArtifactLinkExternally( | ||
| details: HandlerDetails, | ||
| ): boolean { | ||
| try { | ||
| const { protocol } = new URL(details.url); | ||
| return protocol === "http:" || protocol === "https:"; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟨 Agent-authored HTML preview can silently launch the user's real browser via window.open
The artifact-preview guest hands every http(s) window-open request to the OS browser (shouldOpenArtifactLinkExternally at apps/studio/src/electron-main/browser-view/window-open-policy.ts:50-59, invoked from apps/studio/src/electron-main/browser-view/manager.ts:170-183), and the check ignores details.disposition entirely — the added tests assert new-window popups are handed off too. Agent-authored HTML is untrusted content; a script in a previewed report can call window.open("https://attacker.example/...") repeatedly with no user gesture requirement enforced here and open arbitrary pages, with arbitrary URL-encoded data, in the user's default browser (where their real cookies live).
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Superseded by #97, which does the same thing in ~1,100 lines instead of ~2,600. Two decisions here made this bigger than the change needed to be, and #97 drops both:
The three pre-existing browser-panel fixes found here (guests painting through app-wide modals, Cmd+F dying after an overlay closes, load errors surviving a session switch) carry over to #97. Closing rather than merging. The analysis is preserved in |
1 participant
HTML file artifacts rendered in a sandboxed, opaque-origin
<iframe>while the agent loaded the identical asset URL in a<webview>guest. So everything origin-scoped behaved differently between the surface the agent screenshots to check its own work and the surface the human reads:localStorage, cookies, IndexedDB, same-originfetchof a sibling file. Agent-authored HTML that persists a filter selection, or reads its owndata.json, is exactly the class of thing that passed the agent's check and failed in front of the user.Both surfaces now render as a real origin through the same pool. Back/forward chrome is the visible payoff and the smaller half.
Plan:
docs/plans/active/html-artifacts-in-the-guest-pool.md. Resolves the open findinghtml-artifact-iframe-navigation.What changed
BrowserTargetIdgains${taskId}/artifactalongside${taskId}/${sessionId}.artifactis a fixed sentinel; session ids areses_-prefixed ULIDs, so no collision.decodeBrowserTargetIdnow returns a discriminated{kind}.loadURL, the way a browser tab does it. The panel and the expand modal share the one guest.workspace.artifactPreviewRPC,machines/artifact-preview.ts(presence lease + 30s grace), and a separate storage profile. Deliberately nottask-browser.ts, whose session key, 1-hour agent-idle clock andagent-browser close --sessionfan-out all describe something else.listTargetsskips artifact targets and the CDP bridge refuses them.loadURL(entryUrl)from any depth; true reload of a navigated-to sub-page works for the first time.browser-panel.tsxsplits: the target-generic parts becomeuse-guest-navigation+ a shared load-error notice, used by both surfaces.Security: a tightening, not a loss
The finding assumed moving off the opaque sandbox was a one-way loss. Measured against what the iframe actually granted, it isn't:
assets.<taskId>.localhost)allowThe iframe's
allowlist granted camera, microphone, geolocation, clipboard, display-capture, MIDI, payment and USB. What is genuinely new is storage, scoped to a per-task asset origin holding nothing but that task's own files.Also fixes a pre-existing bug on
mainA body-mounted guest kept painting through an app-wide modal's overlay — opening a dialog is not a tab switch, so nothing told the slot it was covered. Open Settings over a live browser panel today and you can see it.
Verification
Driven against a running Studio with an artifact page written to exercise the differences. Confirmed: real origin;
localStorageworks and survives navigation;fetch("data.json")succeeds; zero iframes; artifact storage lands only inartifact-preview-sessionand is absent frombrowser-session;/json?id=omits the artifact target and its CDP upgrade is refused while a session-shaped id is still accepted; in-page link → toolbar tracks it → escape-to-root returns home; guest paints above the modal overlay; close → reap after grace → reopen creates a fresh guest;viewMode: "raw"unaffected.The item flagged as most likely to break did not. Radix does set
pointer-events: noneon the body, butshowOverSlotalready setspointer-events: autoon the container, so the guest stays hit-testable under the dialog. Nomodal={false}workaround and no non-modalContent, so the focus trap and outside-dismiss are kept.Two defects found this way and fixed, neither visible from reading:
Still unverified, listed in the plan: app zoom at 0.5x/2x (mechanism unchanged and shared with the browser panel; bounds invariant confirmed at 1x), range requests, and a physical OS-level click (host CDP input does not route into a
<webview>at all — a control with no modal open failed identically — so this was established by hit testing).pnpm check-and-test:cigreen, 22/22.Summary by CodeRabbit
Release Notes
New Features
Improvements