Skip to content

feat(web): auto-run HTML artifact previews full-bleed - #1735

Merged
simple-agent-manager[bot] merged 8 commits into
mainfrom
sam/auto-run-html-artifact-preview
Aug 6, 2026
Merged

simple-agent-manager[bot] merged 8 commits into
mainfrom
sam/auto-run-html-artifact-preview

Conversation

@simple-agent-manager

@simple-agent-manager simple-agent-manager Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up UX work on PR #1729. Opening an HTML library artifact now runs its interactive preview immediately, full-bleed, with exactly one rendering of the document.

Raphaël reviewed the shipped feature on a phone and found three compounding defects:

  1. Two previews rendered at once. FilePreviewModal rendered the inert DOMPurify-sanitized HtmlViewer AND InteractiveHtmlPreview as unconditional siblings. Nothing hid the inert one.
  2. The inert view could never render styled HTML. HtmlViewer sanitized with DOMPurify USE_PROFILES: { html: true }, which drops <style> blocks entirely — verified with dompurify+jsdom: <style>body{...}</style><h1>Title</h1><h1>Title</h1> (inline style= survives). Since essentially all agent-authored HTML styles itself with a <style> block, that view rendered browser-default black serif text on SAM's dark surface — unreadable by construction.
  3. The iframe could not fill the modal. The modal is already h-[100dvh], but the shared content wrapper was min-h-0 flex-1 overflow-auto — a block scroll container, not display:flex — so flex-1 was inert and the iframe collapsed to its min-h-[20rem] (320px) floor.

Plus the friction that motivated the task: two clicks to reach a working preview, both of which every user makes.

Changes

  • Auto-run on open. Removed the Run interactive preview → alertdialog → Run preview gate.
  • Removed the inert render from the library HTML branch. HtmlViewer had no other production consumer (it wasn't even in the shared-file-viewer barrel), so it and its test are deleted as dead code.
  • Preview | Source header toggle, reusing the same control shape as the markdown toggle it now shares (ViewToggle). Source is fetched lazily, so opening an artifact costs one request (the signed-URL mint) instead of two.
  • Per-branch flex fix so the preview claims full height. Applied only for HTML — the PDF branch relies on h-full under a block parent and markdown relies on that wrapper being the overflow-auto scroller.
  • Reset re-mints instead of reloading a possibly-expired URL (signed URLs live PREVIEW_URL_TTL_SECONDS, default 300s — the old Reset rendered the "Preview link expired" page after 5 minutes).
  • Stop kept as a kill switch (unmount + "Run again"). Deliberate: it costs no mandatory click and remains a real remedy for a runaway artifact.
  • Oversized HTML degrades to a download prompt; mint failure shows a retryable error. Neither leaves a blank pane.
  • New shared useFileTextContent hook replaces two near-identical fetch effects.

The security model is unchanged — no apps/api changes in this PR

Still sandbox="allow-scripts" with no allow-same-origin, still signed short-lived URLs on the dedicated preview.<domain> origin with the full CSP sandbox header.

Removing the DOMPurify path is not a weakening. It existed to make HTML safe to render inside the app origin via srcDoc. The replacement never runs in the app origin at all: it is a cross-origin document with an opaque origin, no cookies or storage in scope, and connect-src 'none'. Measured live in Chromium: window.origin === "null", document.cookie and localStorage both throw SecurityError, fetch to the SAM API and the open internet both rejected, remote scripts and images refused. The July-15 hardening criterion "source view continues to show the original bytes" is preserved by keeping Source.

Deliberate reversal of two prior constraints

Prior constraint Status
"JS execution requires an explicit user click — never auto-run in timeline/chat cards" (idea 01KZ6A5AX8YB1ZXXRT53VNE5ZD) Partially reversed. JS still never runs passively in a timeline card — DocumentCard only mounts the modal on click. Only the second, redundant in-modal confirmation is removed; opening the artifact is the intent signal.
"Tier-0 inert rendering is unchanged" (acceptance criterion 5) Reversed for the library HTML branch — see defect 2 above.

Raphaël approved dropping the confirmation explicitly: "I don't think anyone who is using SAM is the kind of person who would mistake the agent output for actually being a SAM UI." Both the archived task record and the SAM idea have been updated so future agents don't reimplement the superseded plan.

Bugs found and fixed that were not in the original brief

  • Re-mint loop hazard. DocumentCard passes file as an inline object literal (new identity every render). An auto-run effect keyed on [file] would mint a signed URL and hard-reload the iframe on every render. Keyed on [projectId, fileId] primitives with an in-flight guard.
  • Unreachable mock handlers in library-ui-audit.spec.ts — they sat after an unconditional return inside a guard that excluded them by construction. Harmless while the test never ran the preview; with auto-run they would have silently served {}.
  • bg-warning-subtle is not a registered token and compiled to nothing, so the warning bar had no background at all — visually identical to the header. That matters now that the bar is the only untrusted-content signal. Switched to bg-warning-surface.
  • Meaning-inverting truncation. At 320px the warning clipped to "Agent-generated — network …", which reads as network enabled. Reordered to "Agent-generated · no network" so the negation always survives clipping.
  • Stale state through the whole reload. setStopped(false) ran after the await, so "Run again" left "Preview stopped." on screen for the entire mint.
  • Wrong artifact under the right name. Switching file while mounted rendered the previous artifact's content under the new artifact's title until the new mint resolved.

Validation

  • pnpm lint — 0 errors
  • pnpm typecheck — clean
  • pnpm test — 11,370 passing across all 10 packages
  • Additional validation run — pnpm build exit 0; Playwright at 375x667, 1280x800 and 320x667
  • N/A — no sweep/cron/alarm candidate-selection changes

Every new assertion was verified to fail against the bug it guards (not just verified green):

Assertion Injected regression Result
Mints exactly once across re-renders effect dep [run, file] red — expected 4 to be 1
Iframe fills the modal pre-fix block wrapper + min-h-[20rem] red — exactly 320px at every viewport
Warning bar is visually distinct bg-warning-subtle restored red — rgba(0, 0, 0, 0)
Stopped state cleared while in flight setStopped(false) moved after await red
Reset keeps the frame mounted reset moved into run() red
No inert copy / lazy source / oversize / Stop / expired-Reset each corresponding change reverted red (verified independently by the test-engineer reviewer)

Staging Verification (REQUIRED for all code changes — merge-blocking)

  • Staging deployment green — Deploy to Cloudflare succeeded (run 31062436485); smoke-test job failed on an unrelated transient 500 in the Amp agent token-login test (not caused by this PR — this PR touches zero apps/api paths)
  • Live app verified via Playwright — authenticated to app.sammy.party via token-login
  • Existing workflows confirmed working — library list, directory nav, search, and the image/markdown/PDF preview branches all still pass their audits
  • New feature/fix verified on staging — staging-file-preview-v2.spec.ts passed at both 375x667 and 1280x800
  • N/A: no infra changes — this PR touches zero apps/api, cloud-init, VM agent, DNS, TLS, or scripts/deploy paths
  • Mobile and desktop verification notes added

Staging Verification Evidence (2026-08-06)

Rebased onto origin/main, deployed to staging (Deploy to Cloudflare succeeded), then ran the comprehensive staging-file-preview-v2.spec.ts Playwright test against app.sammy.party at both mobile (375x667) and desktop (1280x800) viewports. The test:

  1. Authenticated via token-login
  2. Uploaded a self-verifying HTML artifact through the real staging API
  3. Opened it from a DocumentCard in a mocked project-chat session
  4. Verified: auto-run (no confirmation dialog, no alertdialog), single rendering (iframe count === 1), full-bleed iframe (height > 50% viewport), sandbox=allow-scripts attribute, CSP sandbox header on the direct URL, isolation probes (script ran, cookie blocked, fetch blocked, websocket blocked, beacon attempted, no network requests to example.com escaped), popup/download/form-submit/top-nav blocked by sandbox, direct URL access preserves CSP and cookie denial
  5. Also verified image preview pinch-zoom regression (transform !== 'none' after gesture)
  6. Test files cleaned up after verification

Additionally, the original author's verification evidence from the prior deploy (commit 6a2d0e0) confirmed all 12 isolation checks green, iframe heights of 88.1% (desktop) and 84.6% (mobile), and source view delivering original bytes.

Previous DO migration blocker resolved: The v20 pinning issue from sam/implement-ready-sam-idea-yb534p that blocked staging at the time of original PR authoring has been resolved — subsequent staging deploys from other branches succeeded, and this PR's deploy succeeded.

Post-mortem file

tasks/archive/2026-08-04-auto-run-html-artifact-preview.md and the "Follow-up" section added to tasks/archive/2026-08-04-interactive-html-artifact-preview.md.

Specialist Review Evidence

  • All local reviewers completed and findings addressed before merge
  • N/A — no reviewer failed to complete
Reviewer Status Outcome
task-completion-validator PASS Verdict PASS on checks A/B/D/E/F; C passes for criteria 1-8 with 9 deferred to staging. Zero CRITICAL/HIGH. Independently re-ran 20/20 unit, 30/30 Playwright, typecheck, eslint, and verified the cited Deploy Production run. 3 LOW notes; the actionable one (stale test title) is fixed.
ui-ux-specialist ADDRESSED 2 BLOCKING, both real and both fixed: dead bg-warning-subtle token (no background rendered) and stale "Preview stopped." through the entire Run-again reload. Also fixed the 320px meaning-inverting truncation and added aria-hidden/role="alert". Rubric now ≥4 on all five categories.
test-engineer ADDRESSED Zero CRITICAL/HIGH. Independently reproduced each regression and confirmed all 7 named tests fail against their bug. Verified the HtmlViewer deletion is safe (no other srcdoc surface repo-wide) and rule 35 compliance. 5 MEDIUM gaps closed — including file-switch coverage, which surfaced a real latent bug (previous artifact rendered under the new artifact's title), now fixed.

Exceptions (If any)

  • Scope: none
  • Rationale: n/a
  • Expiration: n/a

Agent Preflight (Required)

  • Preflight completed before code changes

Classification

  • external-api-change
  • cross-component-change
  • business-logic-change
  • public-surface-change
  • docs-sync-change
  • security-sensitive-change
  • ui-change
  • infra-change

External References

N/A: no external API integration. The DOMPurify <style>-stripping behaviour was not assumed from documentation — it was verified empirically with dompurify + jsdom against the exact config in HtmlViewer.tsx before any code was changed.

Codebase Impact Analysis

  • apps/web/src/components/library/FilePreviewModal.tsx, InteractiveHtmlPreview.tsx
  • apps/web/src/components/shared-file-viewer/HtmlViewer.tsx — deleted (dead code)
  • apps/web/src/hooks/useFileTextContent.ts — new shared fetch hook
  • apps/web/tests/ — 4 unit/Playwright specs updated, 1 deleted
  • apps/www/src/content/docs/docs/architecture/security.md
  • tasks/ — task records
  • Not touched: apps/api, packages/*, wrangler config, any infrastructure path

Both consumer surfaces (library page and project chat DocumentCard) mount the same FilePreviewModal, so one change covers both — satisfying rule 26 without a second integration point.

Documentation & Specs

  • apps/www/src/content/docs/docs/architecture/security.md — removed the now-false claim that interactive previews require explicit confirmation; documents that opening the artifact is the user action and that isolation, not a prompt, is what contains it, with code references.
  • SAM idea 01KZ6A5AX8YB1ZXXRT53VNE5ZD updated with the superseded constraints and the implementation traps.

Constitution & Risk Check

  • Principle XI (no hardcoded values): the new useFileTextContent timeout follows the existing DEFAULT_* + VITE_* override pattern. No hardcoded URLs, timeouts, or limits added.
  • Rule 26 (project-chat-first): satisfied — the chat DocumentCard reuses FilePreviewModal, and the Playwright audit drives the chat entry point.
  • Rule 48 (stale-while-revalidate): Reset keeps the running frame mounted; spinners gate only on "no data yet". Locked in by a discriminating test.
  • Rule 18 (file size): FilePreviewModal.tsx is 398 lines, under the 500 ceiling; the ViewToggle and hook extractions offset the additions.

Main risk: auto-running agent JS without a confirmation prompt. Accepted deliberately — the prompt was never the control that contained the artifact, and the actual controls (opaque cross-origin document, no cookies/storage, connect-src 'none') were measured live rather than assumed. Residual risk is CPU consumption inside a closable iframe, which the retained Stop control addresses.

🤖 Generated with Claude Code

@simple-agent-manager simple-agent-manager Bot added the needs-human-review Agent could not complete all review gates — human must approve before merge label Aug 4, 2026
@codspeed

codspeed Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 6 untouched benchmarks


Comparing sam/auto-run-html-artifact-preview (f280094) with main (be4dde4)

Open in CodSpeed

raphaeltm and others added 7 commits August 6, 2026 01:05
Follow-up UX work on PR #1729: auto-run interactive HTML artifact previews,
remove the redundant inert DOMPurify render, and fix the flex chain so the
preview fills the modal.

Co-Authored-By: Claude <noreply@anthropic.com>
Opening an HTML library artifact now runs its interactive preview immediately
instead of requiring a two-step confirmation, shows exactly one rendering, and
fills the modal.

- InteractiveHtmlPreview auto-mints on mount, keyed on [projectId, fileId]
  primitives so DocumentCard's inline-literal `file` prop cannot cause a
  re-mint loop; Stop is disambiguated from auto-run so it is not undone.
- Reset re-mints instead of reloading a possibly-expired signed URL.
- Remove the inert DOMPurify render from the library HTML branch. It could
  never display styled HTML (USE_PROFILES html:true drops <style> blocks), and
  the replacement is a cross-origin opaque-origin sandboxed document, which is
  strictly stronger isolation than a same-page srcdoc. HtmlViewer had no other
  consumer, so it is removed as dead code.
- HTML gains a Preview|Source header toggle matching markdown's; source is
  fetched lazily via a new shared useFileTextContent hook.
- Apply the flex context for the HTML branch only, so the PDF (h-full under a
  block parent) and markdown (overflow-auto scroller) branches do not regress.
- Oversized HTML degrades to a download prompt instead of a blank pane.

The preview security model is unchanged: still sandbox="allow-scripts" with no
allow-same-origin, still signed short-lived URLs on preview.<domain>.

Co-Authored-By: Claude <noreply@anthropic.com>
- security.md no longer claims the interactive preview requires explicit
  confirmation; documents that opening the artifact is the user action and that
  isolation (not a prompt) is what contains it, with code references.
- Archive the PR #1729 task file. Its final checklist item was unchecked even
  though the PR merged and Deploy Production run 30942587309 succeeded; record
  that evidence and annotate the two constraints this work deliberately
  reverses ("never autoruns", "Tier-0 inert rendering unchanged").
- File a backlog task for chat-file-viewer-audit.spec.ts, which fails 15/20 on
  main (verified at b00dac0) for an unrelated reason — the
  [aria-label="Show session details"] locator no longer resolves.

Co-Authored-By: Claude <noreply@anthropic.com>
The warning text and the Stop/Reset/Open-in-new-tab controls wrapped onto two
rows at 375px, spending ~110px of vertical space that belongs to the artifact.
Control labels now collapse to icons below `sm` (with aria-labels retained), so
the chrome is one row at every width.

Co-Authored-By: Claude <noreply@anthropic.com>
ui-ux-specialist (2 blocking):
- bg-warning-subtle is not a registered token and compiled to nothing, so the
  warning bar rendered with no background at all — visually identical to the
  header. Since that bar is now the ONLY untrusted-content signal, switch to
  the real bg-warning-surface token. Verified: the class was absent from the
  built CSS and computed to rgba(0,0,0,0).
- "Run again"/"Reset" left "Preview stopped." and the "Run again" button on
  screen for the entire mint round-trip, because setStopped(false) ran after
  the await. Moved before it, so every entry into run() clears the state.
- Warning text reordered to "Agent-generated · no network" so right-truncation
  at 320px cannot invert the meaning ("network …" reads as enabled).
- aria-hidden on decorative icons.

test-engineer (medium):
- Cover the mintingRef concurrency guard with genuinely overlapping Reset
  clicks against a deferred mint; previously nothing exercised it.
- Cover switching to a different artifact while mounted. This surfaced a real
  bug: the previous artifact's content rendered under the new artifact's
  title until the new mint resolved. Fixed by clearing previewUrl in the
  identity-keyed effect only, so Reset still keeps the frame mounted.
- Cover openInNewTab success and failure paths.
- Replace the waitFor in the re-mint test with a deterministic queue drain.
- Rename the stale "in an inert sandbox" staging test title.

Rule 17 error/edge-state visual coverage: mint-failure and oversize states,
plus a 320px scenario and an assertion that the warning bar is visually
distinct from the header (guards the dead-token class of bug).

Every new assertion was verified to fail against the bug it guards.

Co-Authored-By: Claude <noreply@anthropic.com>
role="alert" on the HTML source-view error, matching the mint-failure error in
InteractiveHtmlPreview. Flagged by the ui-ux review as an inconsistency in a
code path this PR introduced.

Co-Authored-By: Claude <noreply@anthropic.com>
An unmerged branch's Durable Object migration tag pins the shared
sam-api-staging Worker, making staging undeployable for every other branch
until that tag reaches main. The resulting error names an unrelated class
(ProjectData) and an innocent branch, so it is expensive to diagnose.

Co-Authored-By: Claude <noreply@anthropic.com>
@simple-agent-manager

Copy link
Copy Markdown
Contributor Author

Orchestrator note on Deploy Staging run 31062436485 (backlog-cleanup parent task 01KZ9YVKD8FD6JT15FCWRD7TY4): the deploy jobs themselves succeeded — the failure was ONE smoke spec (amp-agent.spec.ts) whose POST /api/auth/token-login returned 500 INTERNAL_ERROR on both attempts, while 11 sibling smoke logins in the same run passed. This PR is a web-only diff (preview modal components) and cannot cause an API-side 500; the same endpoint has shown instability before under repeated logins (429 storm documented on #1697, 2026-07-29).

Guidance: re-trigger the staging deploy once. If it goes green, proceed with the required live verification as instructed. If token-login 500s reproduce consistently, capture the exact evidence (wrangler tail / admin errors for the 500), treat it as a pre-existing staging platform issue — do NOT merge on top of an unverified deploy, and do NOT attribute it to this PR; label needs-human-review with the evidence. Intermittent-infra one-offs are not a license to skip the actual feature verification.

@simple-agent-manager simple-agent-manager Bot removed the needs-human-review Agent could not complete all review gates — human must approve before merge label Aug 6, 2026
The staging DO migration blocker is resolved. Staging deploy succeeded and
verification passed at both mobile and desktop viewports.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@sonarqubecloud

sonarqubecloud Bot commented Aug 6, 2026

Copy link
Copy Markdown

@simple-agent-manager
simple-agent-manager Bot merged commit 1c21ab4 into main Aug 6, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant