You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Non-blocking residuals from the fresh-context review of PR #2753, which committed Taskdeck's first visual-regression baseline set. None blocked that merge. Each was verified against source by the reviewer; where something is unverified it says so.
1. Variable-length test identities spend the pixel-diff budget for nothing
tests/e2e/support/authSession.ts builds a username with Math.floor(Math.random() * 1_000_000), which renders 1–6 digits. .td-topbar__user is hidden with visibility: hidden, which keeps the element's box, and that box sits in .td-topbar__right, a content-sized flex container anchored right (ShellTopbar.vue). So a shorter username shifts the "System Live" chip horizontally in every full-page baseline. The same applies to Playwright mask rectangles, which are sized to the element box — settings-profile.png and board-toolbar.png.
By the reviewer's arithmetic the worst case stays inside maxDiffPixelRatio: 0.005 (~4,600 px of a 1280×720 frame), so this is tolerance erosion, not a guaranteed failure. It is filed rather than fixed because authSession.ts is shared with the whole e2e suite and changing it belongs in its own change.
Suggested fix, one line: String(Math.floor(Math.random() * 1_000_000)).padStart(6, '0'), so every generated username is a fixed width.
2. Nothing in the visual suite covers toast rendering any more
PR #2753 added [data-toast-id] { visibility: hidden !important; } to hideDynamicContent, and error toasts were already excluded by the pre-existing [role="alert"][aria-live="assertive"] rule. So no baseline captures a toast in any state, and a regression that breaks the Paper toast card grid (grid-template-columns: 44px 1fr auto) is invisible to this lane.
The hide rule should not be weakened — it is correct, and waiting for toasts to clear is not viable: toastStore.error(message, duration = 0) means error toasts never auto-remove, and page.clock.install() in the calendar spec freezes the removal timer entirely. The right follow-up is a dedicated spec that seeds one toast with a fixed message and captures it deliberately, opting out of the hide rule for that shot only.
3. Two silent-failure seams in the bootstrap path
.github/workflows/reusable-visual-regression.yml:
The Upload generated baselines step is gated only on steps.baselines.outputs.exist == 'false', which implies success(). If the bootstrap run exits non-zero for any reason the artifact is silently not produced, and the documented step ("the job runs --update-snapshots and uploads the result") simply does not happen — with no diagnostic. Suggested: if: always() && steps.baselines.outputs.exist == 'false'.
maxFailures: 5 applies to the bootstrap run too. Unverified hypothesis:if Playwright counts newly written snapshots as failures under --update-snapshots, a full reset would regenerate only 5 of 22. The fact that all 22 PNGs were produced in one bootstrap run on test: commit reviewed visual-regression baselines #2753 is decent evidence that it does not, but it was not proven. To settle it: delete __screenshots__/, run CI=true npx playwright test --config playwright.visual.config.ts --update-snapshots, and check the exit code and PNG count.
Note this touches .github/**, which is R4 control plane — it needs the control-plane lane, not a drive-by.
4. Baseline drift accumulates silently because the job is label-gated
The 22 baselines are viewport-scoped full-page shots of mostly empty states plus three element shots. They assert chrome and layout rather than product logic, which is the correct shape for a VR baseline — but it means any change to shared chrome (sidebar, topbar, design tokens) invalidates most of the 22 at once.
Because visual-regression only runs on PRs labelled testing or visual (ci-extended.yml), that drift is invisible until someone next applies the label, and then lands on them in full.
Cheap mitigation worth considering: a workflow_dispatch input (e.g. update_baselines) that forces the --update-snapshots path without requiring the committed set to be deleted first, so regeneration does not need the draft-PR dance.
5. Optional hardening, one line each
playwright.visual.config.ts pins viewport, colour scheme and reduced motion but not locale / timezoneId. Adding locale: 'en-US', timezoneId: 'UTC' would close a whole class of runner-environment drift.
.gitattributes has * text=auto and no *.png binary. Git's NUL-byte detection handles PNGs correctly, so there is no defect here — belt-and-braces only.
Explicitly refuted during that review, recorded so they are not re-raised
maxFailures: 5 is not the wrong trade-off. The repo's primary Playwright config uses a stricter maxFailures: 3 in CI, the visual lane is advisory and label-gated, and its step budget is tight (12-minute step, 15-minute globalTimeout, workers: 1, backend + Vite boot inside it).
The [data-toast-id] rule is correct.data-toast-id is set unconditionally by both ToastContainer.vue and PaperToastContainer.vue, both stacks are position: fixed with pointer-events: none so hiding shifts no layout, and visibility: hidden is the safer choice over display: none.
Determinism holds elsewhere. All 16 specs were read: the only view that renders a date pins the clock, modal timestamps carry data-testid="timestamp" and are block-level, presence is seeded synchronously on mount rather than racing SignalR, and sidebar badges only render at count > 0.
Non-blocking residuals from the fresh-context review of PR #2753, which committed Taskdeck's first visual-regression baseline set. None blocked that merge. Each was verified against source by the reviewer; where something is unverified it says so.
1. Variable-length test identities spend the pixel-diff budget for nothing
tests/e2e/support/authSession.tsbuilds a username withMath.floor(Math.random() * 1_000_000), which renders 1–6 digits..td-topbar__useris hidden withvisibility: hidden, which keeps the element's box, and that box sits in.td-topbar__right, a content-sized flex container anchored right (ShellTopbar.vue). So a shorter username shifts the "System Live" chip horizontally in every full-page baseline. The same applies to Playwrightmaskrectangles, which are sized to the element box —settings-profile.pngandboard-toolbar.png.By the reviewer's arithmetic the worst case stays inside
maxDiffPixelRatio: 0.005(~4,600 px of a 1280×720 frame), so this is tolerance erosion, not a guaranteed failure. It is filed rather than fixed becauseauthSession.tsis shared with the whole e2e suite and changing it belongs in its own change.Suggested fix, one line:
String(Math.floor(Math.random() * 1_000_000)).padStart(6, '0'), so every generated username is a fixed width.2. Nothing in the visual suite covers toast rendering any more
PR #2753 added
[data-toast-id] { visibility: hidden !important; }tohideDynamicContent, and error toasts were already excluded by the pre-existing[role="alert"][aria-live="assertive"]rule. So no baseline captures a toast in any state, and a regression that breaks the Paper toast card grid (grid-template-columns: 44px 1fr auto) is invisible to this lane.The hide rule should not be weakened — it is correct, and waiting for toasts to clear is not viable:
toastStore.error(message, duration = 0)means error toasts never auto-remove, andpage.clock.install()in the calendar spec freezes the removal timer entirely. The right follow-up is a dedicated spec that seeds one toast with a fixed message and captures it deliberately, opting out of the hide rule for that shot only.3. Two silent-failure seams in the bootstrap path
.github/workflows/reusable-visual-regression.yml:Upload generated baselinesstep is gated only onsteps.baselines.outputs.exist == 'false', which impliessuccess(). If the bootstrap run exits non-zero for any reason the artifact is silently not produced, and the documented step ("the job runs--update-snapshotsand uploads the result") simply does not happen — with no diagnostic. Suggested:if: always() && steps.baselines.outputs.exist == 'false'.maxFailures: 5applies to the bootstrap run too. Unverified hypothesis: if Playwright counts newly written snapshots as failures under--update-snapshots, a full reset would regenerate only 5 of 22. The fact that all 22 PNGs were produced in one bootstrap run on test: commit reviewed visual-regression baselines #2753 is decent evidence that it does not, but it was not proven. To settle it: delete__screenshots__/, runCI=true npx playwright test --config playwright.visual.config.ts --update-snapshots, and check the exit code and PNG count.Note this touches
.github/**, which is R4 control plane — it needs the control-plane lane, not a drive-by.4. Baseline drift accumulates silently because the job is label-gated
The 22 baselines are viewport-scoped full-page shots of mostly empty states plus three element shots. They assert chrome and layout rather than product logic, which is the correct shape for a VR baseline — but it means any change to shared chrome (sidebar, topbar, design tokens) invalidates most of the 22 at once.
Because
visual-regressiononly runs on PRs labelledtestingorvisual(ci-extended.yml), that drift is invisible until someone next applies the label, and then lands on them in full.Cheap mitigation worth considering: a
workflow_dispatchinput (e.g.update_baselines) that forces the--update-snapshotspath without requiring the committed set to be deleted first, so regeneration does not need the draft-PR dance.5. Optional hardening, one line each
playwright.visual.config.tspins viewport, colour scheme and reduced motion but notlocale/timezoneId. Addinglocale: 'en-US', timezoneId: 'UTC'would close a whole class of runner-environment drift..gitattributeshas* text=autoand no*.png binary. Git's NUL-byte detection handles PNGs correctly, so there is no defect here — belt-and-braces only.Explicitly refuted during that review, recorded so they are not re-raised
maxFailures: 5is not the wrong trade-off. The repo's primary Playwright config uses a strictermaxFailures: 3in CI, the visual lane is advisory and label-gated, and its step budget is tight (12-minute step, 15-minuteglobalTimeout,workers: 1, backend + Vite boot inside it).[data-toast-id]rule is correct.data-toast-idis set unconditionally by bothToastContainer.vueandPaperToastContainer.vue, both stacks areposition: fixedwithpointer-events: noneso hiding shifts no layout, andvisibility: hiddenis the safer choice overdisplay: none.data-testid="timestamp"and are block-level, presence is seeded synchronously on mount rather than racing SignalR, and sidebar badges only render at count > 0.Refs #2753, #1363