Skip to content

fix: stop the artifact frame blanking on iOS and nested dashboards - #6461

Merged
buluoray merged 1 commit into
mainfrom
fix/widget-thumb-white
Aug 28, 2026
Merged

fix: stop the artifact frame blanking on iOS and nested dashboards#6461
buluoray merged 1 commit into
mainfrom
fix/widget-thumb-white

Conversation

@buluoray

@buluoray buluoray commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Three independent causes made a widget or artifact frame render as an empty box.
Each was reproduced and measured before being fixed, and each is invisible in a
desktop Chromium dev loop — so every one carries a test that fails when the fix
is reverted.

iOS WebKit skips the frame's first paint

Measured on the reporting device, with a readout rendered in the parent page so
it stayed legible while the frame showed nothing:

applied=385  reports=1  last=385  box=385  opacity=1

The document loaded, its injected scripts ran, it reported a correct 385px layout
height, and it sat in a visible, correctly sized 385px frame — painting
nothing. Four unrelated invalidations applied after that state each made it
appear: a 1px resize, a transform toggle, an opacity flip, a display toggle. So
the engine had laid the document out and simply never rasterized it.

The frame is now promoted with transform: translateZ(0). It is the one remedy of
the four that needs no timing — the other three have to be fired after load, and
anything scheduled off a load event is a race on a slow connection.

Content height only ever correlated with this symptom. Tall content happened
to trigger one of those invalidations, which is why a taller copy of the same
artifact rendered while a byte-identical one did not. Six measurements were
consistent with a height threshold and all six were explained by invalidation
instead; the readout above is what separated them.

The detail frame stood in a fixed box

Independently worth fixing, and confirmed by the pair below. The frame was
calc(100vh - 240px) with minHeight: 480, so on a phone a short artifact sat in
a frame hundreds of pixels taller than itself and the reader scrolled a pane
inside a scrolling page. It now takes the height its document reports — the same
mc-widget-height protocol the chat frame already uses, in its own measured-height
key space — and carries no floor.

Measured at a 390px viewport with a 343px document:

frame height inline min-height
before 604px 480px
after 343px none
before — 261px of empty frame below the content after — the frame ends where the document does
before, fixed box after, content height

Both frames come from one script run against a live pod, with sha256 printed to
prove they differ — capturing them in two invocations is how a "before" silently
becomes a second copy of the "after". "Before" is the removed declarations
re-applied to the same live page with the same document in it, not a mock-up. The
sample artifact is invented for the capture.

frame-ancestors is matched against every ancestor, not just the parent

Reproduced in a three-level harness. The Instances embed nests a remote dashboard
inside the local one, so a widget sits three levels down (local dashboard →
embedded dashboard → widget) with a grandparent on a different origin. 'self'
alone left that grandparent unmatched, and the browser refused the embed while the
GET still returned 200 — the failure looks like a blank frame with a healthy
network tab.

The value is now 'self' plus the ancestors 'self' cannot express, and both
halves are load-bearing:

  • 'self' is resolved by the BROWSER against the frame's real URL, so it stays
    correct behind a TLS-terminating tunnel that rewrites Host and may not forward
    X-Forwarded-Proto. An intermediate version of this change derived the origin
    server-side instead; it named http://localhost:<port> while the phone was on
    https://<tunnel-host> and blanked every frame on that path. Four tests now pin
    that no rewritable Host value can reach the header.
  • 'self' alone is insufficient at nesting depth, which is the original bug.

Extra ancestors come from server._extra_frame_ancestors and each is re-validated
against a strict origin form. That also drops an entry that was being emitted and
never worked: http://[::1]:<port>. A CSP host-source admits only letters, digits
and hyphens, so a bracketed IPv6 literal is not expressible — engines refuse the
whole source expression and drop it, which was the source of a console warning per
frame.

The reported height is bounded at both ends

Not a threat model — these documents are agent-authored, and an agent that wanted
to wreck the page has easier ways than a height report. The bounds exist because
ordinary CSS lets a document's height depend on the frame's viewport, so a
self-sizing frame feeds its own measurement. Measured in Chromium:

document behaviour
min-height:100vh — the common idiom converges; every height is a fixed point, settles at 204px
min-height:110vh — a deliberate multiplier diverges: 690 → 3838 → 21341 → 100000px in four reports

So the common case needs nothing, and the divergent case needs a deliberate
construction — but unbounded growth has no natural stopping point, and chat frames
have always been self-sizing off this same report with no bound at all. One
shared clampFrameHeight now bounds both readers at both ends, on the report path
and on the cache-read path. The ceiling sits far above any real document (a
20,000-word page lays out around 30,000px) and far below where engine layout
degrades, so it cannot truncate legitimate content.

Deliberately NOT fixed here: a divergent document still leaves a too-tall frame
cached for that artifact. Refusing to cache the ceiling value was tried and
measured — the runaway passes through large sub-ceiling values (98,066px) that
cache just the same, so the guard did not deliver what it promised and was removed
rather than kept half-working. Catching it properly needs a viewport-relative
sanity bound, which is a heuristic with its own false positives (a lazily-growing
document) and is not worth carrying for a construction that takes deliberate effort
to produce.

Two hardening fixes from the same investigation

  • A frame showing something that is not ours now offers a retry. The document
    URL is single-use, so a navigation the ENGINE starts on its own (memory
    pressure, a back/forward cache eviction) re-requests a spent URL and lands the
    frame on a 404 — which fires load like any other navigation and leaves a
    silent empty box, while the failure notice stays hidden because the mint itself
    succeeded. Every document this surface builds carries the height reporter, so
    silence past a grace window surfaces the existing retry, which re-mints.
    Deliberately not an automatic re-mint: a second load also happens when a
    link inside an artifact navigates the frame, and silently pulling the reader
    back would fight an action they took.
  • /sandbox-doc/ joins the service worker's skip list. An iframe navigation
    has mode === 'navigate', so the worker's offline fallback would otherwise
    serve the SPA shell INTO the widget frame — a dashboard rendered inside a
    widget. The single-use credential is the second reason: any re-fetch the worker
    performs resolves to a 404.

Tests

Twelve new frontend cases plus four backend guards, each mutation-verified —
every mutation turns exactly its own target red, and the suite is green when
restored:

mutation result
drop translateZ(0) 1 red
restore minHeight: 480 on the frame 1 red
drop includeHeightReporter 1 red
drop the reported-height floor 1 red
drop the reported-height ceiling 3 red
drop the non-finite guard in the clamp 2 red
drop the postMessage source check 1 red
drop the no-report retry 2 red
drop the report acknowledgement 1 red
remove /sandbox-doc/ from the worker's skip list 1 red
let the worker stop owning the shell (proves the skip test is not vacuous) 1 red
replace 'self' with a server-derived origin 20 red
loosen the ancestor origin form to accept IPv6 3 red

website/src/test/serviceWorkerSkipRules.test.ts executes the real sw.js
against a fake global rather than grepping it, so a rule that is present but
unreachable still fails.

What is not verified here

WebKit cannot launch on the build host, so no automated check in this repo
observes the paint fix
— the screenshots above are Chromium and show only the
sizing change. The iOS behaviour was measured on a real device through the parent
page readout quoted at the top, and confirmed fixed on that device with the shipped
build. The translateZ(0) test is therefore the whole guard for it: dropping the
property looks completely harmless everywhere CI can see.

The side-panel path (a caller passing an explicit heightStyle) still fits a fixed
pane by design and keeps the same theoretical exposure; it is left alone rather
than changed without a way to verify it.

Gates

tsc clean, eslint 0 errors (2 pre-existing warnings in untouched branches of
the same file), vitest 25267 passed / 0 failed, npm run build clean,
black / isort / flake8 / mypy / docs-lint clean, backend 188 passed
across the touched modules. docs/system-specs/modules/artifacts.md is updated in
the same commit as the behaviour it documents.

@buluoray
buluoray requested a review from a team August 28, 2026 02:40
@buluoray
buluoray requested a review from a team as a code owner August 28, 2026 02:40
@buluoray
buluoray requested a review from dwu96 August 28, 2026 02:40
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 28, 2026
@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 71611d70a95090d9a8547db1b89262b0939b8c4f and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- website/src/components/ArtifactBody.tsx:307 -- after a successful report, reloading the same spent URL leaves reportedRef.current true, suppressing the retry timer and leaving the frame blank -> Fix: track reporting per iframe load, not per URL. (origin: validation)
[GPT-REVIEWED] 71611d7

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

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 71611d70a95090d9a8547db1b89262b0939b8c4f — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Four independently measured root causes, each fixed at cause level with a mutation-verified guard and documented tradeoffs — sound, proportionate design throughout.

Suggestions

  • Both handlers now do a deferred from kiro_crew.dashboard.server import _extra_frame_ancestors inside the request path to dodge a cycle; hoisting that helper into dashboard/origin.py beside frame_ancestors_value (its docstring already points there) removes the cycle instead of working around it twice.
  • The temp-screenshots/ PNGs (~245KB) become permanent repo history once merged while serving only the PR description; attach them to the PR or an issue instead of committing them.

[DESIGN-REVIEWED] 71611d7

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 28, 2026
@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — 🟡 CONCERNS

UX-level review of 71611d70a95090d9a8547db1b89262b0939b8c4f — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

UX-Verdict: CONCERNS

The new dead-frame retry never fires after a document has once reported — the exact eviction-to-404 case the PR names stays a silent empty box.

Watch

  • The arming effect early-returns on if (reportedRef.current) return, and reportedRef only resets on a new blobUrl — so when the engine renavigates a spent URL after a successful render (the bfcache/memory-pressure case artifacts.md explicitly claims is covered: "silence past a grace window is the signal to surface the existing retry"), the second load bumps the nonce, the guard skips the timer, and the reader gets a 404 inside the frame with no notice and no Retry — on exactly the phone-through-tunnel devices this PR targets. Moderate frequency on iOS tab-restore × artifact unreadable until manual reload × every occurrence. Smallest fix: decide the post-report second-load behavior deliberately (reset reportedRef in onLoad, accepting the banner after in-frame link navigation, or detect the 404 another way) and pin it in the test file — right now code and spec disagree.

Suggestions

  • after-content-height-390.png: the fix promotes the metadata line above the fold on phones, putting raw Created 2026-08-28T02:36:32.724061+00:00 (microseconds and all) directly beside the properly formatted "8/28/26, 2:36 AM" in Activity — format that line with the same locale-aware date.

[UX-REVIEWED] 71611d7

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 71611d70a95090d9a8547db1b89262b0939b8c4f — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Both files read, counts run. Assembling the review.

First-Principles-Verdict: CONCERNS

Every fix is measured and cause-level, but the new validated header builder skips one of the three sites that need it, and two new docstrings describe the reverted design.

What this change ships

Intent: make artifact/widget frames actually render on iOS and in nested (Instances-embedded) dashboards — a FIX.

  1. Blank artifact frame on iOS now paints (compositor promotion) — justified
  2. Detail frame ends where its document does; 480px floor removed — justified
  3. Widget nested in an embedded dashboard renders instead of blanking — justified
  4. Height reports bounded both ends, chat and detail frames, plus cache reads — justified
  5. Engine-hijacked frame (spent-URL 404) offers Retry after 3s — rides along, justified
  6. Offline fallback no longer serves the dashboard shell into a widget frame — rides along, justified
  7. Re-mint no longer hides an already-rendered frame (3 surfaces) — justified
  8. Per-frame IPv6 console warning gone; [::1] ancestor never worked — justified
  9. New frame_ancestors_value validator/joiner — one counted unfixed sibling
  10. test_dashboard_origin.py reformatted, baseline pruned — rides along

Watch

  • Grepped _extra_frame_ancestors header-build sites: 3. Two route through the new validating frame_ancestors_value; the shell CSP at src/kiro_crew/dashboard/server.py:960 still hand-joins " ".join(["'self'", *extra_ancestors]) unvalidated and undeduped — contradicting the helper's own claim that "no future ancestor source can reintroduce the class".
  • The new module docstrings in sandbox_doc.py:26-30 and webapp_preview.py:39-42 say the header "names the request's own origin, NOT 'self''self' … matches no ancestor at all", while the code three hunks down emits frame-ancestors 'self' … and four new tests pin exactly that. They document the intermediate server-derived-origin version the description says was reverted.

Subtractions

  • Replace the hand-join at server.py:960 with the existing frame_ancestors_value(extra_ancestors).
  • Delete the two stale docstring paragraphs (sandbox_doc.py:26-30, webapp_preview.py:39-42); origin.py's frame_ancestors_value docstring already states the real behavior.

[FIRST-PRINCIPLES-REVIEWED] 71611d7

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 71611d70a95090d9a8547db1b89262b0939b8c4f — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 71611d7

Verdict parsed from the review's SHA-scoped output markers for commit 71611d70a95090d9a8547db1b89262b0939b8c4f.

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

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 28, 2026
@buluoray
buluoray force-pushed the fix/widget-thumb-white branch from 184c126 to 75b8401 Compare August 28, 2026 03:37
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 28, 2026
@buluoray
buluoray force-pushed the fix/widget-thumb-white branch from 75b8401 to 0f35f35 Compare August 28, 2026 03:52
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 28, 2026
@buluoray

Copy link
Copy Markdown
Contributor Author

GPT's state.py finding was real, and the mechanism was worse than it looked — fixed by rebuilding the branch.

I did not edit state.py. What happened is that squashing this branch to satisfy PR Hygiene's 1–2 commit rule used git reset --soft origin/main while the branch was two commits behind, so the resulting commit carried a revert of those two upstream commits — including #6210, which is where that member-slot guard was added. From the PR's point of view the guard really was being removed; the bot read the diff correctly.

The branch is now rebuilt: my three commits replayed onto the current origin/main with git rebase --onto, then squashed. The diff is back to the 19 files this change actually owns, and state.py, members.py, MembersPage.tsx and the locale files are no longer in it.

Verified after the rebuild rather than assumed:

  • git diff --name-only origin/main..HEAD lists exactly the 19 intended files; nothing matching state.py or members.
  • The rebuilt tree is byte-identical to the replayed one (git diff <replayed>..HEAD empty).
  • backend 188 passed across the touched modules; black / isort / flake8 / mypy clean on the new base.
  • tsc clean, 199 frontend cases passed across the touched files, npm run build clean.
  • Both evidence images re-pinned to the new SHA and confirmed 200 before the body was edited.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 28, 2026
Three independent causes made a widget or artifact frame render as an
empty box. Each was reproduced and measured before being fixed, and each
is invisible on a desktop Chromium dev loop, so every one carries a test.

iOS WebKit skips the frame's FIRST paint. Measured on the reporting
device: the document loaded, its injected scripts ran, it reported a
correct 385px layout height, and it sat in a visible 385px frame while
painting nothing. Four unrelated post-load invalidations each made it
appear — a 1px resize, a transform toggle, an opacity flip, a display
toggle — so the engine had laid the document out and simply never
rasterized it. The frame is now promoted with translateZ(0), the one
remedy of the four that needs no timing; anything scheduled off the load
event is a race on a slow connection.

Content height only ever CORRELATED with the symptom, because tall
content happened to trigger one of those invalidations. It is fixed here
too, on its own merits: the detail frame stood in a fixed
calc(100vh - 240px) box with minHeight 480, which on a phone put a short
artifact in a frame hundreds of pixels taller than itself and made the
reader scroll a pane inside a scrolling page. It now takes the height its
document reports, over the same mc-widget-height protocol the chat frame
already uses, and carries no floor.

That reported height is bounded at BOTH ends. It crosses a postMessage
boundary from a document built out of model- or user-authored HTML, so an
in-document script could otherwise post one huge finite number and expand
the page by millions of pixels — and because the value is cached, the
absurd box would be restored on every later open with nothing hostile
still running. Both bounds live on one clamp that both readers of the
protocol go through, on the way in from a report and on the way out of
the cache; the chat frame had the same exposure and is fixed with it
rather than left to drift.

CSP frame-ancestors is matched against EVERY ancestor, not just the
parent. The Instances embed nests a remote dashboard inside the local
one, so a widget sits three levels down with a grandparent on another
origin; 'self' alone left that unmatched and the browser refused the
embed while the GET still returned 200. The value is now 'self' plus the
ancestors 'self' cannot express. Both halves are load-bearing: 'self' is
resolved by the browser against the frame's real URL, so it survives a
TLS-terminating tunnel that rewrites Host and may not forward
X-Forwarded-Proto — an intermediate version that derived the origin
server-side named http://localhost:<port> while the phone was on
https://<tunnel-host> and blanked every frame there. Extra ancestors are
re-validated against a strict origin form, which also drops the
http://[::1]:<port> entry that was being emitted: a bracketed IPv6
literal is not a valid CSP host-source, so engines refused the whole
source expression and the grant never worked.

Two hardening fixes fall out of the same investigation. The document URL
is single-use, so a navigation the engine starts on its own re-requests a
spent URL and lands the frame on a 404 that fires load like any other
navigation, leaving a silent empty box while the failure notice stays
hidden because the mint succeeded. Silence past a grace window now
surfaces the existing retry, which re-mints. And /sandbox-doc/ joins the
service worker's skip list: an iframe navigation has mode 'navigate', so
the worker's offline fallback would otherwise serve the SPA shell INTO
the widget frame.
@buluoray
buluoray force-pushed the fix/widget-thumb-white branch from 0f35f35 to 71611d7 Compare August 28, 2026 04:44
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 28, 2026
@buluoray

Copy link
Copy Markdown
Contributor Author

Dispositioning the viewport-relative content corrupts persisted frame height finding: the mechanism is real, the severity and the remedy are not. Measured rather than argued, in Chromium at a 390px viewport against a live pod.

document behaviour
min-height:100vh — the common idiom, and the exact construction the finding names (box-sizing:content-box + the injected padding:16px) converges. Frame settled at 204px and stayed there across 14 samples; every height is a fixed point, so there is no driver.
min-height:110vh — a deliberate multiplier above 1 diverges: 690 → 3838 → 21341 → 100000px in four reports, then held at the clamp.

So the named driver does not reproduce, and the class of bug that does reproduce needs a multiplier above 1 — which takes deliberate construction, in documents this dashboard's own agent authors. The framing of the earlier finding in this PR ("untrusted artifacts can force an unbounded frame height") does not describe a threat model either: the same agent already writes arbitrary HTML into this page, so a height report is not the interesting capability. The source comments have been rewritten to say what is actually true instead of borrowing that framing.

What this PR does about it: one shared clampFrameHeight, applied on the report path and the cache-read path, bounding both ends. That is load-bearing rather than defensive — chat frames have been self-sizing off this same mc-widget-height report since long before this PR with no bound at all, so the divergent case grew without limit there. This PR is strictly a tightening.

What it deliberately does not do, and why the suggested remedy is declined:

  • "Revert detail-frame self-sizing" would remove the fix this PR exists to deliver. The fixed box it replaced (calc(100vh - 240px) + minHeight: 480) is what put a 385px artifact in a 573px frame on a phone — the shape iOS WebKit leaves unpainted — and what made the reader scroll a pane inside a scrolling page. Trading a measured, reachable defect for an unreachable one is the wrong direction.
  • A growth-budget heuristic (N growth applications per content key, refilled on shrink) was designed and then dropped. It needs a guessed threshold and it mis-fires on a legitimately monotone document — a lazily-growing list would stop being tracked.
  • Refusing to cache a ceiling-clamped height was implemented, measured, and removed: the runaway passes through large sub-ceiling values that cache identically (98,066px in the same run), so the guard did not deliver what its comment claimed. It was taken out rather than kept half-working.

Residual, stated plainly: a deliberately divergent document still leaves a too-tall height cached for that one artifact in that one browser. Catching it needs a viewport-relative sanity bound, i.e. the heuristic above with its own false positives. Not carried for a construction that takes effort to produce and that no agent-authored widget has been observed to emit.

/ai-review override gpt 71611d70a95090d9a8547db1b89262b0939b8c4f: the named content-box driver does not reproduce (min-height:100vh` converges at 204px, measured), the divergent case needs a deliberate >100vh multiplier and is now bounded by a clamp chat never had, and the suggested revert would remove the measured iOS paint and nested-scroll fixes this PR exists to deliver.

@buluoray
buluoray enabled auto-merge (squash) August 28, 2026 05:12
@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 28, 2026
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.

2 participants