fix(e2e): make visual-regression editor captures deterministic#2192
Conversation
Blur the active element before capturing so a hydration focus race on the TipTap toolbar cannot leave a button highlighted in some runs and not others, which produced spurious diffs on the content-editor and content-new screens. Also decide visual drift from git status of the snapshots directory rather than from a non-zero Playwright exit. A flaky render can fail the first-pass diff yet regenerate byte-identical baselines under --update-snapshots; the old candidate-emptiness check always saw the committed baselines and flagged drift regardless. Now a clean tree after regeneration is treated as flake or infra failure and keeps the check green with a warning.
|
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | f1aa36f | Jul 22 2026, 09:45 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | f1aa36f | Jul 22 2026, 09:45 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-do | f1aa36f | Jul 22 2026, 09:45 AM |
github-actions
Bot
added
area/ci
size/S
review/needs-review
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the right fix for a real problem. The visual suite's editor captures were nondeterministic because of a focus race during hydration, and the measure job was mislabeling flaky/infra exits as drift because cp -r always produced a non-empty candidates dir. Blurring the active element before capture is a pragmatic, low-cost way to stabilize the toolbar state, and switching the drift decision from exit code to git status --porcelain correctly distinguishes changed/new baselines from byte-identical regeneration.
What I checked:
- The diff, the full
visual.ymlworkflow, and the relatedvisual-report.yml/visual-apply.ymlworkflows. - The full
e2e/tests/visual-regression.spec.tsspec and the.gitignorerules for snapshot files. - AGENTS.md conventions that could apply to this e2e/CI-only change (changesets, comments, query counts, localization, RTL, SQL, authorization). No violations there; no published package is touched, so no changeset is needed.
Headline concern: the implementation intentionally treats any non-zero visual-suite exit with a git-clean snapshots tree as flake or infra. That is correct for the documented flaky-render case, but it also swallows genuine test/setup failures that prevent screenshots from being generated at all (e.g., a JS error or navigation timeout on one screen). In those cases the regeneration also leaves the tree clean, so the measure job exits green and the report workflow clears/op-noops the sticky comment, leaving no artifact that a maintainer can inspect. Because the visual job is explicitly advisory this is a trade-off, not a hard blocker, but it's worth adding a non-drift artifact path that preserves the failure signal.
The spec change itself looks correct: blur() is called only on a non-body HTMLElement, and it runs after the freeze CSS and before waiting for fonts, so the capture reflects a consistent unfocused state.
| # or infra and keep the check green with a warning. | ||
| if [ -z "$(git status --porcelain -- "$SNAP_DIR")" ]; then | ||
| echo "false" > visual-out/has-drift | ||
| echo "has_drift=false" >> "$GITHUB_OUTPUT" | ||
| echo "::warning::Visual suite failed without producing candidate baselines (likely an infra failure, not a UI diff)." | ||
| else | ||
| echo "true" > visual-out/has-drift | ||
| echo "has_drift=true" >> "$GITHUB_OUTPUT" | ||
| echo "Visual drift detected — candidates and diff images staged for the Report workflow." | ||
| echo "::warning::Visual suite exited non-zero but regenerated baselines are identical to the committed ones (flaky render or infra failure, not a UI diff)." |
There was a problem hiding this comment.
[suggestion] The git-clean branch now treats every non-zero exit without baseline changes as flake/infra and exits 0. That correctly handles the documented flaky-render case, but it also makes the measure job green for genuine failures that prevent screenshots from being generated in the first place (e.g., a JS error or navigation timeout on a screen, which also leaves the snapshots tree unchanged). The ::warning:: is visible in the job log, but the report workflow will delete or skip the sticky comment, so there's no durable signal for maintainers.
Consider preserving test-results/ (logs, traces, or at least the Playwright output) in the artifact on this path, so a failure that isn't drift can still be investigated without re-running the suite.
| # or infra and keep the check green with a warning. | |
| if [ -z "$(git status --porcelain -- "$SNAP_DIR")" ]; then | |
| echo "false" > visual-out/has-drift | |
| echo "has_drift=false" >> "$GITHUB_OUTPUT" | |
| echo "::warning::Visual suite failed without producing candidate baselines (likely an infra failure, not a UI diff)." | |
| else | |
| echo "true" > visual-out/has-drift | |
| echo "has_drift=true" >> "$GITHUB_OUTPUT" | |
| echo "Visual drift detected — candidates and diff images staged for the Report workflow." | |
| echo "::warning::Visual suite exited non-zero but regenerated baselines are identical to the committed ones (flaky render or infra failure, not a UI diff)." | |
| if [ -z "$(git status --porcelain -- "$SNAP_DIR")" ]; then | |
| echo "false" > visual-out/has-drift | |
| echo "has_drift=false" >> "$GITHUB_OUTPUT" | |
| echo "::warning::Visual suite exited non-zero but regenerated baselines are identical to the committed ones (flaky render or infra failure, not a UI diff)." | |
| # Keep failure artifacts around even though we aren't reporting drift, | |
| # so setup/screenshot-prevention failures remain inspectable. | |
| cp -r test-results visual-out/test-results 2>/dev/null || true | |
| exit 0 | |
| fi |
github-actions
Bot
added
review/awaiting-author
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce flakiness in the admin visual-regression pipeline by making editor screenshots deterministic and by changing the CI “drift” decision logic to rely on actual snapshot file changes rather than a Playwright exit code.
Changes:
- Updates the visual-regression spec to blur the active element as part of the stabilization sequence to avoid focus-dependent toolbar rendering differences.
- Updates the
Visual Regressionmeasure workflow to detect drift viagit status --porcelainon the snapshots directory after--update-snapshots, treating a clean tree as flake/infra instead of drift.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| e2e/tests/visual-regression.spec.ts | Adjusts stabilization steps to reduce focus-state nondeterminism before screenshots. |
| .github/workflows/visual.yml | Changes drift detection to be based on actual snapshot diffs (git status) rather than non-zero test exit codes. |
Comments suppressed due to low confidence (1)
e2e/tests/visual-regression.spec.ts:128
- The focus blur happens before awaiting
document.fonts.ready, which leaves a window where the editor can re-focus itself (e.g. a post-hydration effect) while fonts are still loading. That defeats the determinism goal because the screenshot can still capture a focused/active toolbar state on slow font loads.
Move the blur to after fonts have settled (right before the screenshot), or blur both before and after the font wait to close the race.
// Drop focus before capturing. The editor pages mount a TipTap toolbar
// whose buttons reflect the editor's focus/active state; whether the editor
// grabs focus during hydration is a race, so a run can capture a focused
// (highlighted) toolbar button or an unfocused one. Blurring makes the
// capture depend on the rendered page, not on who won the focus race.
await admin.page.evaluate(() => {
const active = document.activeElement;
if (active instanceof HTMLElement && active !== document.body) {
active.blur();
}
});
// Await font loading without returning the FontFaceSet that
// document.fonts.ready fulfils with -- Playwright cannot serialize it.
await admin.page.evaluate(async () => {
await document.fonts.ready;
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
What does this PR do?
Makes the visual-regression suite deterministic on the editor screens and stops the measure job from flagging drift on flaky/infra failures.
Two independent fixes:
Spec: blur before capture.
content-editorandcontent-newmount a TipTap toolbar whose buttons reflect the editor's focus/active state. Whether the editor grabs focus during hydration is a race, so a run could capture a focused (highlighted) toolbar button or an unfocused one, producing spurious diffs.stabilize()now blurs the active element before awaiting fonts, so the capture depends on the rendered page rather than who won the focus race.Workflow: decide drift from
git status, not the exit code. The measure job regenerates baselines with--update-snapshotsafter a failing diff, then decided "drift" from whether the candidates directory was non-empty. Butcp -ralways copies the committed baselines (which always exist), so that check flagged drift on any non-zero exit, including a flaky render that regenerated byte-identical baselines. The decision is nowgit status --porcelainon the snapshots dir: a modified/untracked PNG is real drift (including the bootstrap case, where a new screen has no committed baseline yet); a clean tree after regeneration is treated as flake or infra failure, keeps the check green, and emits a warning.Root cause was confirmed from PR #2185's measure artifact (run 29865573115, head 86d30b1): the run-1
actualhad a highlighted toolbar button while the--update-snapshotscandidate was byte-identical to the committed baseline.Follow-up to #2147 (suite + CI) and #2165 (baselines).
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure
Screenshots / test output
The determinism fix cannot be meaningfully validated on macOS (Linux baselines differ), so verification happens on this PR's own measure runs. Residual fallback if flakiness persists on those two screens: mask the editor toolbar region for
content-editor/content-new.