Skip to content

fix(artifacts): stop flagging healthy artifacts as no longer showing - #7918

Merged
iamwhatever merged 1 commit into
mainfrom
fix/artifact-notice-report-race
Sep 2, 2026
Merged

fix(artifacts): stop flagging healthy artifacts as no longer showing#7918
iamwhatever merged 1 commit into
mainfrom
fix/artifact-notice-report-race

Conversation

@NicholasRBowers

@NicholasRBowers NicholasRBowers commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

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 load like 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:

  1. ArtifactBody's onLoad re-arms the DOC_REPORT_GRACE_MS (3s) silence window and discards the current height report on every iframe load — by design, because a renavigation onto a spent URL fires load with 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).
  2. The injected height reporter (HEIGHT_REPORTER_BODY) posts its first measurement as soon as layout settles — before the load event 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 — reset lastSent before the scheduled re-check so it routes through the first-measurement path. Every load event 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.tsx gets 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 from buildSrcdoc output (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 load listener that resets a
"reported" flag paired with a postMessage sender guarded by
Math.abs(next - lastSent) <= EPS.

@NicholasRBowers
NicholasRBowers requested a review from a team September 2, 2026 16:24
@NicholasRBowers
NicholasRBowers requested a review from a team as a code owner September 2, 2026 16:24
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

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

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

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] bcbc3ad

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

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

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

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 load with no reporter), so the reporter — the only side that knows a load occurred with a live document behind it — is the right place to guarantee a post-load report. The lastSent = -1 reset composes cleanly with the shrink/deadband machinery, the animated-widget quietness contract is pinned by a test executing the real extracted script, and both halves of the contract are documented at each end.

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

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of bcbc3adc20ae3795b23610fe561a7ece37e00b7a — 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.

All evidence gathered. The fix is a one-line behavior change (lastSent = -1 before the load re-check) in the injected height reporter, plus comments and a test that executes the real extracted script. I verified the claimed race against the actual code: the pre-fix reporter's load re-check (setTimeout(schedule, 100)) routes into evaluate, where an unchanged height falls into the <= EPS deadband and never posts — while ArtifactBody.tsx:437 resets reportedRef on every load and arms the 3s DOC_REPORT_GRACE_MS window. The parent's discard-at-load is itself load-bearing (spent single-use URL detection, documented in the same file), so the reporter side is the right place. Grep confirms only one post-load-silence detector exists (ArtifactBody), and the three other reporter embedders (chat WidgetFrame.tsx:215, ArtifactThumbs.tsx:53, mochi WidgetFrame.tsx:58) only consume heights, where the extra identical post is a no-op.

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 ships

Intent: stop the artifact page from overlaying a false "no longer showing" warning on healthy artifacts whose images finish after first layout — a FIX.

  1. Artifacts with images/fonts no longer get the false warning on every open — justified (reported defect Artifact detail page falsely flags healthy artifacts as "no longer showing" (height-report/load race) #7915, fixed at the contract cause: reporter re-posts unconditionally after load).
  2. The same reporter re-post also reaches the three height-only embedders (chat widgets, gallery thumbnails, mochi) — same single change; a no-op for them (counted: WidgetFrame.tsx:215, ArtifactThumbs.tsx:53, apps/mochi/.../WidgetFrame.tsx:58 consume height only, setFrameHeight dedups an unchanged value).
  3. Comment-only cross-reference tying DOC_REPORT_GRACE_MS's silence invariant to the reporter's re-report — justified (spec-with-change convention).
  4. New test executing the real extracted reporter script, pinning the load re-report and the unchanged jitter quietness — justified; proven red pre-fix per description.

Sibling count for the root cause (a load observer depending on a deadbanded reporter): grepped DOC_REPORT_GRACE|docSilent|reportedRef — 1 detector exists, 0 unfixed siblings. The parent-side alternative (keep pre-load reports) was considered and is correctly rejected: discard-at-load is the spent-URL detection itself. Shipped diff equals the smallest honest version: one behavioral line, comments, one test.

[FIRST-PRINCIPLES-REVIEWED] bcbc3ad

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] bcbc3ad

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

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

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

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 ## Pattern harvest section to this body, but the hygiene workflow evaluates the event payload frozen at run creation, so a rerun cannot see a body edit — it clears on the next push. Once #7943 lands on main, a rebase push here picks up both.

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
@NicholasRBowers
NicholasRBowers force-pushed the fix/artifact-notice-report-race branch from 5d16519 to bcbc3ad Compare September 2, 2026 18:57
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 2, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

✅ Review-ready at bcbc3adc2: all 65 checks green (PR Readiness passed), GPT/Opus review lanes clean, UX/Design/First-Principles reviews all PASS, 0 unresolved threads. The earlier Dependency Audit and PR Hygiene reds were external (fleet-wide fast-uri advisory, fixed on main by #7936) and cleared with the rebase. Awaiting maintainer merge — merging closes #7915.

@chenmingwei23
chenmingwei23 enabled auto-merge (squash) September 2, 2026 21:07

@chenmingwei23 chenmingwei23 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
iamwhatever merged commit ef802fa into main Sep 2, 2026
65 checks passed
@iamwhatever
iamwhatever deleted the fix/artifact-notice-report-race branch September 2, 2026 21:07

@iamwhatever iamwhatever left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 dwu96 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 2, 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.

Artifact detail page falsely flags healthy artifacts as "no longer showing" (height-report/load race)

4 participants