fix(artifacts): stop flagging healthy artifacts as no longer showing - #7918
Conversation
UX Review (Fable 5) — ✅ PASSUX-level review of UX-Verdict: PASS No user-facing surface changes: the fix removes a false "artifact no longer showing" flag on healthy widgets — purely a UX improvement, with a test pinning it. [UX-REVIEWED] bcbc3ad |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — ✅ PASSDesign-level review of Design review complete. The fix targets the correct half of the cross-frame contract: the parent must discard pre-load reports (a spent-URL 404 fires Design-Verdict: PASS Root-cause fix on the correct side of the frame boundary — the reporter, the only party that can prove "load happened with our document" — with the contract pinned by an executed-script test. [DESIGN-REVIEWED] bcbc3ad |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All evidence gathered. The fix is a one-line behavior change ( First-Principles-Verdict: PASS A one-line cause-level fix — the reporter's quietness contract is amended exactly where it starved the parent's only loss signal, nothing rides along. What this change shipsIntent: stop the artifact page from overlaying a false "no longer showing" warning on healthy artifacts whose images finish after first layout — a FIX.
Sibling count for the root cause (a load observer depending on a deadbanded reporter): grepped [FIRST-PRINCIPLES-REVIEWED] bcbc3ad |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
|
Status note: the two red required checks are not this PR's diff. Dependency Audit is a fleet-wide breakage (fresh fast-uri GHSA advisories against main's electron lockfile) — fix open at #7943 / tracked in #7925. PR Hygiene was fixed by adding the |
The artifact page arms a 3s silence window on EVERY iframe load and discards any height report received before it (an engine renavigation onto a spent single-use url fires load with no reporter behind it, so post-load silence is the only signal). But the height reporter posts its first measurement as soon as layout settles - before load whenever images or fonts are still fetching - and then goes deliberately quiet: its load-time re-check measures the same height and is swallowed by the 2px deadband. The parent wipes the pre-load report at load, no re-report ever comes, and a healthy rendering document is flagged 'This artifact is no longer showing' 3 seconds after every open and re-mint. Fix in the reporter, where the invariant belongs: on window load, re-report unconditionally (reset lastSent before the scheduled re-check), so every load event the parent can observe is followed by a report. The real detection case is preserved - a 404 on a spent url carries no reporter and stays correctly silent - and the quietness contract is untouched everywhere else. The regression test executes the actual reporter script extracted from buildSrcdoc output (not a copy) in a stub document, and was proven red against the pre-fix reporter. Closes #7915
5d16519 to
bcbc3ad
Compare
|
✅ Review-ready at |
chenmingwei23
left a comment
There was a problem hiding this comment.
Tier 1 auto-approve: fix (3 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean (Semgrep + CodeQL success, empty PR-scoped alerts), security checklist all-NO, AI reviewers green. Category: fix - re-report widget iframe height unconditionally after load so a healthy artifact is not flagged as no longer showing.
iamwhatever
left a comment
There was a problem hiding this comment.
Tier 1 auto-approve: fix (3 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: widget height reporter re-posts unconditionally after window load so a pre-load measurement is not the last report, which stopped healthy artifact frames from being flagged as no longer showing; behaviour change is confined to report cadence inside the existing sandboxed iframe, with no change to sandbox attributes or CSP.
dwu96
left a comment
There was a problem hiding this comment.
Tier 1 auto-approve: fix (3 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: fix with a clear root cause -- a widget whose layout settles before its images finish posts its first height measurement before window load, the reporter's deadband then swallowed the load-time re-check, and ArtifactBody (which re-arms its silence window on every load) flagged a healthy document as no longer showing; the fix resets lastSent so every observable load is followed by a report, plus a test executing the real extracted reporter script.
Problem / Motivation
The artifact detail page repeatedly overlays a healthy, fully rendered artifact with the cause-neutral notice "This artifact is no longer showing" (and its Show artifact re-mint action) — while the user is looking straight at the rendered content. It reappears on every open and every re-mint (theme change, refetch), so for affected artifacts the page is effectively always wearing a false warning banner.
Why it matters
The notice exists to be the reader's only signal that the sandboxed frame silently lost its document (an engine renavigation onto a spent single-use URL lands on a 404 that fires
loadlike any other navigation). A detector that cries wolf on every artifact with images trains users to ignore it, which defeats the one recovery affordance the surface has — and it makes the artifact library feel broken.What changed (motivation → approach → change)
Symptom: the notice fires ~3 seconds after every load of an artifact whose layout settles before its subresources finish (any widget with images/fonts).
Root cause — a race between two deliberate behaviors:
ArtifactBody'sonLoadre-arms theDOC_REPORT_GRACE_MS(3s) silence window and discards the current height report on every iframeload— by design, because a renavigation onto a spent URL firesloadwith no reporter behind it, and post-load silence is the only observable signal (fix: stop the artifact frame blanking on iOS and nested dashboards #6461).HEIGHT_REPORTER_BODY) posts its first measurement as soon as layout settles — before theloadevent whenever images or fonts are still fetching — and is then deliberately quiet: its own load-time re-check measures the same height and is swallowed by the 2px deadband.Net: the parent wipes the pre-load report at
load, the deadband guarantees no re-report ever arrives, and a healthy document is flagged as "no longer showing".Change: fix in the reporter, where the quietness contract lives. On window
load, re-report unconditionally — resetlastSentbefore the scheduled re-check so it routes through the first-measurement path. Everyloadevent the parent can observe is now followed by a report. The real detection case is preserved (a 404 on a spent URL carries no reporter and stays correctly silent), and quietness is untouched everywhere else (the animated-widget lock-and-stop behavior is unchanged).ArtifactBody.tsxgets a comment-only update pointing its silence-window invariant at the reporter's re-report as the other half of the contract.Tests
New
website/src/test/widgetSrcdocReporter.test.tsx— executes the actual reporter script extracted frombuildSrcdocoutput (not a copy) inside a stub document:posts an initial measurement on execution— baseline contract.re-posts after window load even when the height is unchanged— the regression case; proven red against the pre-fix reporter (deadband swallowed the load re-check) and green with the fix.stays quiet on jitter within the deadband outside of load— pins that the fix does not weaken the quietness contract that keeps animated widgets from feeding a per-frame resize loop back to the parent.Existing neighboring suites (
widgetSrcdoc.test.ts,ArtifactBody.iframeBlob.test.tsx,ArtifactsPage.test.tsx,widgetHeights.test.ts) all pass: 84/84.Manual verification
Root cause and fix traced against the live repro on the reporting user's dashboard (artifact with avatar images flagged on every open). The false positive requires subresources that outlive first layout plus real browser load-event timing, which the executed-reporter unit test captures deterministically at the contract boundary.
Screenshots / video
Why no screenshot: no styled surface changes — the notice component, its copy, and all layout are byte-identical; the diff changes only when the injected reporter script posts (plus comments and a test), so the "after" frame is indistinguishable from a healthy artifact page.
Closes #7915
Pattern harvest
Rule candidate: review-prompt pattern — when a frame-load observer treats
post-load silence as a failure signal, any in-frame reporter it depends on must
re-post unconditionally on
load; a reporter that dedups by last-sent value(deadband/ratchet) silently starves the observer whenever its first report
races ahead of the load event. Grep shape: a
loadlistener that resets a"reported" flag paired with a postMessage sender guarded by
Math.abs(next - lastSent) <= EPS.