fix(preview): report a target that is this gateway instead of a blank frame

[kyleseaman]

Problem

The Web Preview panel showed an unexplained blank rectangle. Reported from a session whose Browser tab held http://127.0.0.1:6776/api/hooks/agent — the POST-only inbound webhook endpoint, which is not a page at all.

The panel's only health signal is a no-cors liveness probe:

try { await fetch(url, { mode: 'no-cors', cache: 'no-store', signal: ctrl.signal }) ; fails = 0 }
catch { fails += 1; if (fails >= 2) setUnreachable(true) }

That catches a dead server. It cannot catch a server that answers but cannot be displayed — a no-cors fetch to a 405, or to a page the browser refuses to frame, resolves. So unreachable stays false, the iframe stays mounted, and the user gets a black box with no message. The panel already has this shape of state for mixed content ("detect it so we can explain … instead of rendering a silently-blank frame"); the reachable-but-unframeable case had no equivalent.

Why it matters

The reachable target that always lands here is the dashboard's own gateway, and the panel offers it to the user unprompted. detectPreviewUrl heuristically scoops any loopback URL out of assistant prose, so whenever the agent discusses webhooks, the dashboard or the gateway, the panel raises a "Load preview" card for the gateway's own URL. One click, blank box, no explanation. Nothing breaks and no data is at risk, but the panel reads as broken.

Fix (symptom → root cause → change)

The gateway answers every request with X-Frame-Options: SAMEORIGIN and CSP frame-ancestors 'self', and isolatePreviewHost deliberately swaps a same-host loopback target onto the other alias (localhost ↔ 127.0.0.1) for cookie isolation. So a self-target is a catch-22: keeping the host identical to satisfy 'self' is exactly the cookie leak the swap exists to prevent, and swapping it guarantees the frame is cross-origin and refused. It can never render, and no probe can discover that.

It is, however, knowable client-side without any probe, which is what this change uses:

isDashboardOrigin(url, dashboardOrigin?) compares effective ports (so :80 and an absent port agree) and requires both sides to be loopback. Both conditions carry weight — ports because the host strings legitimately differ for one and the same server after the isolation swap, and both-loopback because over a tunnel or a LAN address a loopback target is the user's own machine and must still frame normally.

Wired at the two entry points:

• A typed or restored URL renders an explanatory state naming the target, with an open-in-browser escape hatch, instead of a blank iframe. The liveness probe is skipped for it (it would pass and mean nothing).
• setSessionPreviewPending refuses such a URL outright, so the chat-detected mention can no longer raise a card whose only outcome is a blank panel.

isolateFrameTarget narrows the cookie-isolation swap to targets the panel will actually frame. The swap protects a framed server; a self-origin target is never framed, so swapping it only misreports what the user typed — someone entering 127.0.0.1:6776 was told localhost:6776 "is the dashboard's own server" at the exact moment the panel was explaining itself.

A general frameability check would need a server-side fetch to read the target's headers. That is deliberately not here: it means adding an outbound-fetch endpoint to the gateway for a display affordance. The broader reachable-but-unframeable class is tracked in #2134, with Design Review's load-timeout suggestion recorded there.

Tests

• isDashboardOrigin unit coverage: cross-alias same port, different port, default-port equivalence, non-loopback target, remote dashboard with a loopback target, unparseable input
• the panel reports the state and mounts no iframe for a self-target
• an ordinary dev server on another port still frames (guards the port comparison against over-matching)
• a chat-fed self-target raises no card and returns null
• the self-origin state shows the URL as entered, asserted on all three surfaces (URL bar, code block, open-in-browser href), plus a guard that an ordinary dev server is still cookie-isolated

Revert-verified on the current base: removing the isolateFrameTarget exemption fails 1 test; disabling the detection fails 3.

Manual verification

Captured against the real stylesheet and theme tokens via an isolated harness that mounts the real WebPreviewPanel — the target URL is built from the harness page's own origin, so isDashboardOrigin fires exactly as in production rather than being stubbed.

Note the URL bar and the code block both read 127.0.0.1:6809 — the host the harness page was served from, not swapped to localhost. That is the isolateFrameTarget fix visible: before it, this state contradicted itself by naming an alias the user never typed.

Control: an ordinary dev server on another port still frames normally

Test environment change

happy-dom defaults the test document to localhost:3000, which is one of the panel's own dev-server quick-picks — so "the dashboard" and "a dev server" were indistinguishable in tests and five existing cases began exercising the new state. The test URL now matches production (localhost:6776), which separates them and leaves every existing case meaning what it did before.

i18n

Two new keys, translated across all 11 locales with en-XA regenerated. Appended within their group so no catalog reorders (3 changed lines per file). The body is a complete sentence rather than a fragment wrapped around the URL, so no target language is pinned to English clause order.

This branch was rebased across 229 commits of main. That surfaced two things worth flagging for review: main had deleted four webPreviewPanel keys (the browse-mirror "Let the agent act" strings) and no longer references them, so the catalogs were resolved as main's file plus only the two new keys rather than a union merge that would have resurrected dead keys; and main added a Korean locale during the gap, which never conflicted but needed both keys to satisfy catalog parity.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of 696bc7ab965f5fa81f59a79c7bcb7236a9debdd4 — updated in place on each push; does not block merge.

Design is sound. Let me verify nothing unrelated is smuggled in and the fix's edge cases are honestly handled.

The diff is fully accounted for: the isDashboardOrigin/isolateFrameTarget detection, the self-origin render state, the probe skip, the setSessionPreviewPending rejection, two i18n keys across locales, the vite test-URL change, and four committed screenshots (the gitignore convention at .gitignore:89 explicitly treats temp-screenshots/ as committed deliverables). No scope creep.

Design gate: the problem is real (unprompted "Load preview" card for the gateway's own URL → blank box). The fix detects the condition client-side without a probe, which is the correct layer — a probe cannot observe frame refusal. The narrower ::1-same-port edge case is documented and degrades to the same explanatory state plus an escape hatch, so it never silently swallows a page. The broader reachable-but-unframeable class is honestly deferred to #2134 rather than over-built here, and the deliberate rejection of a server-side header-fetch endpoint is the proportionate call for a single-user local tool.

Design-Verdict: PASS

Correctly-scoped, root-cause client-side detection of the un-framable self-target; broader unframeable class honestly deferred rather than over-engineered.

[DESIGN-REVIEWED] 696bc7a

[github-actions]

UX Review (Fable 5) — ✅ PASS

Advisory UX-level review of 696bc7ab965f5fa81f59a79c7bcb7236a9debdd4 — updated in place on each push; does not block merge.

UX-Verdict: PASS

A former silent dead-end now explains itself in the user's terms — cause, the URL as typed, and a working escape hatch — matching the panel's sibling states.

Suggestions

• Align the escape-hatch label with the sibling mixed-content state's Open {url} pattern — the same panel now uses two labels ("Open in browser" vs "Open http://…") for the identical open-externally action; pick one form.

[UX-REVIEWED] 696bc7a

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 696bc7ab965f5fa81f59a79c7bcb7236a9debdd4 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 696bc7a

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 696bc7ab965f5fa81f59a79c7bcb7236a9debdd4: <one-sentence reason>

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 696bc7ab965f5fa81f59a79c7bcb7236a9debdd4 — this comment is updated in place on each push.

Review details

No blocking findings.

FINDING — temp-screenshots/preview-self-origin/*.png — Four binary PNGs under a temp-screenshots/ dir (manual-verification artifacts) are committed into the repo tree, permanently bloating git history with throwaway files → Fix: drop these four files from the PR (git rm).

[OPUS-REVIEWED] 696bc7a

Verdict parsed from the review's SHA-scoped output markers for commit 696bc7ab965f5fa81f59a79c7bcb7236a9debdd4.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 696bc7ab965f5fa81f59a79c7bcb7236a9debdd4: <one-sentence reason>

[kyleseaman]

Both advisory suggestions read and addressed (e2bb012d5).

Design Review — documented, as suggested. isDashboardOrigin's doc comment now names the ::1-on-the-gateway's-port case explicitly, including why the degradation is acceptable, so the next reader does not have to re-derive it:

Same port on loopback therefore means the same server, with one exception: a dev server bound to a DIFFERENT loopback interface (::1) on the gateway's own port is a distinct listener that this reports as self. That target degrades to the explanatory state plus its open-in-browser link rather than to a blank frame, so the misread costs a click and never silently swallows a page.

Worth noting the case is narrower still in practice: isolatePreviewHost canonicalises [::1] to 127.0.0.1 before this runs, so reaching it takes a server bound to ::1 and the gateway on the same port — and the outcome is a click, not a lost page. Agreed that no behaviour change is warranted.

UX Review — agreed, and deliberately left for the follow-up you suggested. The two escape-hatch shapes should converge on the new one (URL in the code block, clean button label). Not folding it in here: aligning mixed content means retiring open + page_mixed_content_open_it_in_a_new_tab_instead and re-translating across ten locales for a state this change does not touch, which would bury a small fix in unrelated churn. The inconsistency predates this PR rather than being introduced by it, so shipping is not making anything worse — but it is a real wart and it should not sit indefinitely.

[kyleseaman]

UX Review — suggestion taken, and it was a real defect (d41c6f7cd).

You caught something I had actually seen and mis-read as reassuring: my own verification screenshots showed the URL bar reading localhost:6809 while the harness page was on 127.0.0.1:6809, and I logged that as evidence the check survives the host swap. It is also exactly the contradiction you describe — the panel asserting "This URL is the dashboard's own server" over a URL the user never typed.

Fixed at the root rather than at the display site. The host swap exists to keep the dashboard's host-scoped cookie out of a framed server; a self-origin target is never framed, so the swap buys no safety there and only misreports the target:

/** Isolate a host only for a target the panel will actually FRAME. */
function isolateFrameTarget(url: string): string {
  return isDashboardOrigin(url) ? url : isolatePreviewHost(url)
}

Applied at the three sites that feed what the user sees — commit(), the mount-restore of the persisted URL, and setSessionPreviewUrl — so the URL bar, the <code> block, and the open-in-browser href all now show what was entered, not just the one you pointed at. setSessionPreviewPending is untouched because it already rejects a self-origin URL before isolation, so it can never reach that state.

Pinned by a test that types the dashboard's own host (the only input that triggers the swap) and asserts all three surfaces carry the entered URL, plus a guard test that an ordinary dev server on another port is still cookie-isolated — so the exemption cannot widen.

Worth recording how nearly this shipped: my first version of that test typed 127.0.0.1 while the test DOM runs on localhost, so the hosts differed, the swap never fired, and the test passed with the fix reverted. Revert-verification is what caught it; retargeting the test at the dashboard's own host made it fail without the fix and pass with it.

Design Review — no reply needed this round: your prior ::1 same-port note is documented in isDashboardOrigin's doc comment and this head's verdict carries no Suggestions.

[kyleseaman]

Design Review — accepted and deferred, filed as #2134.

You're right that the broader class is still open: any reachable-but-frame-refusing target (a dev server sending X-Frame-Options: DENY, or a restrictive frame-ancestors) still lands on a silent blank frame. This PR only closes the one instance that is fully knowable client-side — a target on the dashboard's own port, which can only ever be this gateway.

The load-timeout idea is a better mechanism than the one I ruled out. My PR body says a general frameability check "would need a server-side fetch of the target to read its headers, and I did not want to add an outbound-fetch endpoint for a display affordance" — an iframe load-timeout sidesteps that entirely, with no new server surface. I hadn't considered it.

Deferring rather than folding it in, for the reason you gave and one more: it needs real design work, not a few lines. Chromium fires load on a blocked frame (it loads an error document), so "load never fired" and "load fired but nothing painted" are different tells and the browsers differ — that wants measuring, not assuming. The budget also has to tolerate a cold Vite start, and a false "can't display" on a merely-slow server is worse than today's blank, so it has to be recoverable. Landing that speculatively inside a PR whose whole point is one deterministic case would trade a known-good fix for an unmeasured heuristic.

The issue carries the mechanism, the three hazards above, and the explicit non-goal of the server-side probe, so the next person starts where this thread ended rather than re-deriving it.

[kyleseaman]

UX Review — same concern, now properly tracked: #2589

You raised this on the first head and I answered "agreed, follow-up" — but I never filed the follow-up, so the only record was a chat reply on a superseded SHA. Re-raising it on this head is fair, and an untracked deferral is what invited it. The issue now carries the direction, the reasoning, and the scope note.

Disposition unchanged, accepted-and-deferred, for the same reason as before: converging the two labels means retiring open and page_mixed_content_open_it_in_a_new_tab_instead and re-translating across all 11 locales for the mixed-content state, which this PR does not otherwise touch. Folding that in would put unrelated catalog churn on top of a one-state fix.

On which direction to converge, the issue records your earlier preference rather than my choice: move mixed content to the self-origin shape (URL in a <code> block, verb-only button label), not the reverse. Two supporting reasons are written up there — Open {url} pins every target language to English clause order, which is the sentence-fragment pattern the i18n conventions discourage, and a long URL inside a button label is a layout hazard that the code block avoids.

Worth being explicit that the inconsistency is not introduced by this PR: mixed content already read Open {url} on main. This change adds a second state that does it better rather than making an existing state worse. That is a weak defence of leaving it, which is why it is filed rather than merely argued.