feat(design-system): mask the pinned chrome in the document-viewer baseline, and add a baseline adopt helper - #1743
Conversation
…seline, and add a baseline adopt helper Two changes that make refreshing a golden after a deliberate design change cheap and readable, which is what the pixel gate needs in order to be read at all. Mask the viewport-pinned chrome (#278). The document-viewer target clips a ~2900px region against a 900px viewport, and it contains a `sm:sticky sm:top-0` header and a `sm:fixed` composer. Playwright stitches an oversized element capture, so both land partway DOWN the image, overlap whatever content sits behind them at that offset, and move whenever content above them changes height -- so an unrelated edit anywhere on the page redrew two bands of the golden and inflated every diff. Masked rather than clipped away, because both are real chrome that belongs in the frame and narrowing the selector would drop the rail panels this target exists to watch. Their own geometry is covered by the phone-chrome contracts in docs/search-chrome-behaviour.md, not by this pixel gate. Fail loudly when a mask matches nothing. A mask selector that matches no element masks nothing, silently -- the golden keeps comparing the region the mask was meant to exclude while the declaration reads as protection that is not there. Renaming a class is enough to cause it. Every declared mask must now resolve to at least one element before the comparison is trusted. That guard is what makes the mask above safe to rely on rather than merely present. Add scripts/adopt-visual-baselines.mjs (npm run design-system:baselines:adopt). Adopting previously meant hand-copying six PNGs and hand-assembling provenance.json with a SHA-256 and pixel dimensions per candidate, the capture commit, the run id and the reviewer attestation. Doing that by hand on every design change is the friction that makes people skip the refresh and leave a red advisory standing. It resolves candidates from both shapes the artifact can take -- visual-candidates/ when a target was awaiting a baseline, and <id>-actual.png when it compared and differed. It refuses a missing or non-numeric run id, a short or unknown capture commit, and a missing --reviewed-by, because that field records a HUMAN review of the images. It never captures screenshots: baselines are platform-scoped and a developer-machine shot lands where ubuntu CI never reads it. Dry run by default. The document-viewer golden is NOT refreshed here -- masking changes its pixels, so it must be re-shot from a CI run that already contains this mask. That is the refresh loop the helper exists for, and it needs this to land first. Verified: tsc 0 errors; lint exit 0; check:design-system-adoption exit 0; docs:check-inventory current at 232 npm scripts; docs:check-links exit 0; format:check clean. Helper exercised against two real artifacts -- it resolves all six candidates from run 31251091603 and correctly refuses run 31254917796, which was all-green and so contains no candidate images -- and all three input guards were confirmed to reject. Dry run wrote nothing. Refs #278, #118
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:4 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:4279b1b5cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
BigSimmo
commented
Aug 8, 2026
@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. The only repository destination is BigSimmo/Database, and the only branch destination is the pull request head branch claude/ds-baseline-workflow at starting commit 4279b1b; never publish fixes to a detached or synthetic work branch and never create a stacked pull request. Use the authenticated GitHub connector to commit each approved fix to BigSimmo/Database:claude/ds-baseline-workflow, then verify that the pull request head contains the published commit before reporting success. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. For a fixed thread, reply with as the first line and as the second line. For a no-code disposition, use followed by . These result markers authorize the workflow to close that exact thread only after it verifies a fixed commit is the pull request head; a local-only commit is not a fix. If publication or verification fails, do not use either result marker, do not claim success, and leave the thread open with the blocker. If human input or new authorization is required, do the same. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation. |
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
BigSimmo
commented
Aug 8, 2026
@cursoragent Unblock the current open PR. Confirm the PR number and GitHub head first from context. If more than one open PR could apply, stop and say which one you would use and why. Fetch and start from the remote tip that matches that GitHub head. If the named branch ref is missing or stale, use the PR head ref. Preserve unrelated local WIP; do not discard dirty work; do not treat a local-ahead commit as the reviewed tip. Do not merge the PR, force-push, rebase, or arm auto-merge. No provider-backed gates without approval. If you cannot push or resolve threads, diagnose and comment only; if inline replies fail, resolve when possible and put blocker dispositions in the summary comment. If auto-merge is already armed, push only for a real blocker and avoid cancelling in-flight required CI unless the push clears that blocker. If the PR is already merged or closed: confirm outcome, post one summary, and stop. Primary job: clear mergeability and required-CI blockers on this tip. Do a full product review only if needed to explain or fix a blocker. Stay light otherwise—no broad verification suites or Playwright unless needed to prove one concrete unblock fix. Snapshot the GitHub head: tip, behind/ahead, mergeable state, merge-tree versus origin/main (real conflict versus behind-but-clean), required checks on that tip including Production UI when selected, and unresolved threads that block merge or required CI. Missing checks while dirty are not green. Tip moved → re-snapshot. Ledger-lookup against that GitHub head under an unblock/fix scope for this PR. If already handled at this head for unblock work, merge-tree clean, required checks green, and no blocking threads: summarize, comment, stop. Unblock once: real conflict → merge origin/main with the smallest correct resolution (prefer main’s shared queues; keep this PR’s notes); behind-but-clean → one sync, late if required CI is in flight; clean → leave. skip-branch-sync and similar labels do not block a manual sync needed to clear a real blocker. No sync thrash. After any sync or push, re-snapshot tip, merge-tree, and required checks before declaring done. Dedupe the ledger if a merge touched it. Fix only what blocks required checks or mergeability on this tip. Ignore advisory red unless it reveals a real required failure. Prefer the smallest safe fix; if a bot/autofix commit caused the break, prefer revert. Prove with the smallest targeted check only. Format and commit before push. Push only blocker-fix commits. Append ledger for the final GitHub head under the unblock scope. Never push a ledger-only tip. One PR comment: tip, sync/merge-tree, what blocked and what you fixed or dispositioned, required CI status, residual risks; merge left to me. Stop when merge-tree is clean on the current GitHub tip, no blocking threads remain for merge/required CI, and required checks are green or clearly in progress. |
Taking a look!   |
BigSimmo
commented
Aug 8, 2026
@cursoragent please resolve all failing CI testing and resolve all comments and resolve merge conflicts please. |
Unblocked PR #1743 ( Tip: Fixed
Status
|
When CI changes only some surfaces, passing targets emit no *-actual.png and unchanged images live under tests/__screenshots__/ in the artifact. The adopt helper now resolves candidates from diff output, artifact baselines, or committed goldens, records refresh provenance (including replacedCandidateIds), and validates capture-head AWAITING_BASELINE binding before writing. Also fix git cat-file reachability checks and run mask-selector validation before candidate capture.
There was a problem hiding this comment.
Pull request overview
Stabilizes document-viewer visual baselines and automates adopting Linux CI screenshots with provenance.
Changes:
- Masks pinned document chrome and validates mask selectors.
- Adds a dry-run baseline adoption helper with tests.
- Registers and documents the new npm script.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/ui-visual-baseline.spec.ts | Masks pinned chrome and guards selectors. |
tests/adopt-visual-baselines.test.ts | Tests baseline adoption and provenance. |
scripts/adopt-visual-baselines.mjs | Implements CI artifact adoption. |
package.json | Adds the adoption command. |
docs/scripts-index.md | Updates script counts. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…asks Adopt the masked document-viewer capture from visual-baseline-31267928439 (tip ac1d0dd) via the partial-refresh helper. Clears the expected advisory diff introduced when the sticky header and fixed composer were masked. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
… mask Copilot: retaining screenshots from the artifact without visual-junit proof could bless a stale golden for a target that failed before emitting an actual. Require a passing junit case for every retained id, refuse all-green refreshes, and only treat *-actual.png as a fresh diff. Also mask [data-document-sticky-header] instead of .edge-glass-header so the fail-loud guard cannot pass against the universal search header after a DocumentViewer rename. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Resolve document-viewer baseline/provenance conflicts by taking main's #1741 capture as the interim golden; the masked refresh will be re-adopted from CI on this merged tip. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
…ain sync Re-adopt from visual-baseline-31268982766 on the post-#1741 merge tip so the golden includes the unique data-document-sticky-header mask and the viewer rework from main. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Unblock summary — PR #1743Tip: What blocked
What was fixed
Review threadsAll 5 review threads are resolved. Inline disposition replies were FORBIDDEN for this token; resolutions were applied via Required CIMergeability is clean ( Residual
|
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
) * ci(visual): stop running the visual baseline job on pull requests Owner decision, 2026-08-09: the pixel gate taxes exactly the work it exists to support. During a redesign the honest result is red on every UI pull request, and the baselines cannot be refreshed until a run has produced the new images, so that red is unavoidable rather than actionable. It was never merge-blocking -- `continue-on-error: true`, and absent from `pr-required`, whose needs list is [changes, static-pr, safety, coverage, build, container-images, ui-critical-fast, ui-critical, db-reset-verify]. But a red check nobody can act on mid-iteration is friction whether or not it gates, and it generated a steady stream of failure notifications on PR #1743 while the only correct response was "yes, that is the change". Drift is still caught. The job keeps running on pushes to main and release branches, on the weekly schedule, and on demand via workflow_dispatch. That moves the signal to where a refresh is cheap -- after the change has landed, against a run whose artifact already contains the new images -- instead of interrupting the change. One command refreshes it: npm run design-system:baselines:adopt -- --from <artifact> --run-id <id> --head <sha> --reviewed-by "<name>" --write Nothing else changes: the job, its targets, its provenance contract and its advisory status are untouched, and no required check is added or removed. Verified: npm run check:github-actions passed; ci-cache-safety 18 passed, 11 skipped; no test pins this job to pull_request; format clean. Refs #118 * issues: record the owner decision that visual-baseline stays off pull requests * fix(ci): keep visual-baseline off merge_group as well as PRs Allowlist push/schedule/workflow_dispatch so merge-queue UI churn does not re-run the advisory pixel gate, and document the schedule in testing.md. --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Summary
document-viewerbaseline (#278). That target clips a ~2900px region against a 900px viewport and contains asm:sticky sm:top-0header and asm:fixedcomposer. Playwright stitches an oversized element capture, so both land partway down the image, overlap whatever content sits behind them at that offset, and move whenever content above them changes height — an unrelated edit anywhere on the page redrew two bands of the golden and inflated every diff.scripts/adopt-visual-baselines.mjs(npm run design-system:baselines:adopt) so refreshing a golden is one command instead of six hand-copied PNGs plus a hand-assembledprovenance.json.Masked rather than clipped away: both elements are real chrome that belongs in the frame, and narrowing the selector would drop the rail panels this target exists to watch. A mask is a hole in the gate, so it is limited to those two — their own geometry is covered by the phone-chrome contracts in
docs/search-chrome-behaviour.md, not by this pixel gate. The match guard is what makes the mask safe to rely on rather than merely present; renaming a class would otherwise silently reopen the hole.The helper resolves candidates from both shapes the artifact can take —
visual-candidates/when a target was awaiting a baseline, and<id>-actual.pngwhen it compared and differed. Handling only the first is what would have made it useless for the refresh case it exists to serve. It refuses a missing or non-numeric run id, a short or unknown capture commit, and a missing--reviewed-by, because that field records a human review of the images and a baseline of a broken render silently blesses the break. It never captures screenshots: baselines are platform-scoped, and a developer-machine shot lands where ubuntu CI never reads it. Dry run by default.The
document-viewergolden is deliberately NOT refreshed in this PR. Masking changes its pixels, so it must be re-shot from a CI run that already contains the mask. ExpectVisual baselines (advisory)to go red on this PR for that one target — that is the change working, and the job is advisory and outsidepr-required, so it cannot block. The refresh is a follow-up using the helper this PR adds.Verification
node ./node_modules/typescript/bin/tsc -p tsconfig.typecheck.json --noEmit— exit 0, measured error count0.npm run lint— exit 0 under--max-warnings 0.npm run check:design-system-adoption— exit 0.npm run docs:check-inventory—Docs inventory current: 219 script files, 232 npm scripts.npm run docs:check-links— exit 0.npm run format:check—All matched files use Prettier code style!visual-baseline-31251091603, and correctly refusesvisual-baseline-31254917796(the all-green run, which contains no candidate images). All three input guards were confirmed to reject. The dry run wrote nothing.win32/baselines and proves nothing aboutlinux/. This PR's ownVisual baselinesjob is the evidence, and its expected result is one red target as described above.Risk and rollout
git revert. That restores the unmasked target and removes the helper.Refs #278, #118.