Skip to content

fix(visual-baseline): close a real capture race in the search results ribbon - #2562

Merged
BigSimmo merged 3 commits into
mainfrom
claude/visual-baseline-lxry7z
Sep 2, 2026
Merged

fix(visual-baseline): close a real capture race in the search results ribbon#2562
BigSimmo merged 3 commits into
mainfrom
claude/visual-baseline-lxry7z

Conversation

@BigSimmo

@BigSimmo BigSimmo commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Summary

  • Goal was to commit a visual baseline (docs/design-system/adoption-manifest.json's visualBaselineStatus/remoteStatus). Two findings changed the shape of this PR — both explained below — so what actually ships here is a nondeterminism fix and a documented handoff, not a committed baseline.
  • The container this work ran in has no working browser. The preinstalled Chromium is revision 1194; the installed playwright-core (1.62.1) expects revision 1234. Downloading the correct revision failed: cdn.playwright.dev returned 403 from this session's egress proxy — confirmed via the proxy's own status endpoint as an organization policy denial, which its README says explicitly not to retry or route around. The repo's own scripts/check-playwright-browser-revision.mjs already detects and fails closed on exactly this condition (tracked as #255). No local capture was possible.
  • Separately, local capture was never going to be a valid path to "committed" here anyway. docs/design-system/adoption-manifest.json's visualBaselinePolicy requires the committed baseline's provenance to be source.kind: "hosted-ci-artifact" from the real GitHub Actions visual-baseline job on ubuntu-24.04, plus review.status: "approved" by a named human reviewer (reviewerType: "human") — enforced by validateLinuxVisualBaselineSet in scripts/generate-design-system-adoption.mjs. The six candidate PNGs already committed at tests/__screenshots__/linux/ are still review.status: "pending" / reviewerType: "automated" from an August 18 CI run — that's the actual reason visualBaselineStatus still reads not-committed, and only a human looking at real images can change that.
  • So this PR does the part that doesn't need a browser: a five-agent audit of the existing tests/ui-visual-baseline.spec.ts suite (5 conceptual capture targets, 6 test entries) for every class of capture nondeterminism (time, data ordering, animation, fonts/async, environment), a real fix for the one live bug found, and a written record of what's covered and what's deliberately still open. Full findings: docs/design-system/GATES.md §6a.
  • The fix: search-results-band/search-results-band-phone could capture the results ribbon mid-fetch (loading spinner, no filter/sort controls) or settled (real match count, controls) depending on CI scheduling jitter, because the suite's settle() helper never waited for the ribbon's backing /api/registry/records fetch to resolve. Added waitForRibbonSettled, which polls the ribbon's own data-status attribute until it clears "loading"/"refetching" before capture.
  • Next step, for a human, not automatable: once this PR's hosted visual-baseline CI job produces a fresh candidate artifact, someone needs to actually look at the six images and run npm run design-system:baselines:adopt -- --from <artifact-dir> --run-id <id> --head <sha> --reviewed-by "<name>" --write, then flip docs/design-system/adoption-contract.json's baseline status and regenerate the manifest.

Verification

  • npm run check:design-system-contract — passed, GATES.md figures block current, adoption/design-sync contracts both check clean
  • npx tsc --noEmit — no errors on the changed spec file
  • npx eslint tests/ui-visual-baseline.spec.ts — clean
  • npx vitest run tests/check-docs-links.test.ts — 7/7 passed
  • npm run format — run and committed
  • Verification not run: npm run verify:pr-local, npm run verify:ui, npm run test:e2e:visualno working Chromium in this container (revision drift #255 + blocked download host, see above). This is the one thing I could not prove locally; the hosted visual-baseline CI job on this PR is the first real test of the fix.
  • Verification not run: npm run verify:release — provider-backed, not run without explicit approval.

Risk and rollout

  • Risk: low. Test-infrastructure-only change (one Playwright spec file, one doc). No production/app code touched.
  • Rollback: revert the commit; the suite returns to its prior (already-passing-in-CI, just occasionally racy on this one target) state.
  • Provider or production effects: None.
  • RAG impact: none — this PR does not touch src/lib/rag/, retrieval RPCs, ranking, or golden fixtures.

Adversarial checks (ran against this diff before opening the PR)

  1. Run the check three times, diff pixels — could not run at all; no browser in this container (see above). Reporting this as unproven rather than claiming determinism I haven't demonstrated.
  2. Three realistic regressions this baseline would NOT catch, verified against the actual component tree by an independent reviewer agent (not just asserted):
    • src/components/clinical-dashboard/master-search-header.tsx — rendered as a sibling before #main-content on every shell route, so a visual break in the shared search header/composer is invisible to all six test entries on every route.
    • ServiceCard in src/components/services/services-navigator-page.tsx (rendered inside data-testid="service-search-results", a sibling below the ribbon) — the search-results-band targets clip to [data-testid="search-query-ribbon"] only, so the actual result cards are never captured.
    • The masked <header data-document-sticky-header> in src/components/DocumentViewer.tsx (~line 1166) — deliberately painted over before comparison (to avoid an unrelated fix: audit P0 RAG cache, synopsis parity, and safety hardening #278-style diff), so a regression inside it (back link, breadcrumb, section-index controls) is invisible by design.
  3. Independent adversarial code review (frontend-ui-reviewer agent, read the real component tree rather than trusting this PR's summary): confirmed waitForRibbonSettled's logic is sound (the data-status attribute is a faithful, closed six-value proxy for fetch state), traced why it cannot hang for the full 20s timeout on every run (demo mode has no Supabase key configured, so auth resolves to "unconfigured" synchronously rather than "loading", and the registry API short-circuits to a synchronous demo fixture), and found no other correctness issue in the diff. Same agent independently produced the three blind-spot findings above.

Notes

  • docs/design-system/GATES.md §6a has the full per-nondeterminism-class findings (what was checked, what's already covered and why, what's genuinely fixed, what's left open for a future pass — a missing global clock pin, and an unproven /api/setup-status race with a degraded-mode banner).
  • Branch was 14 commits behind origin/main at session start; merged before this work (brings in, among other things, an unrelated already-landed fix for a previously-recorded pinned-clock defect — confirmed unrelated to this suite, different component tree, not reachable from any of the 5 capture targets).

🤖 Generated with Claude Code

https://claude.ai/code/session_01BstBbK4vj1YmEpqqFSjcHv


Generated by Claude Code


Note

Low Risk
Test-only Playwright harness and design-system documentation; no production or app runtime changes.

Overview
Addresses flaky search-results-band / search-results-band-phone captures where the suite could screenshot the query ribbon while registry data was still loading (spinner, no filter/sort row) instead of the settled UI.

Adds waitForRibbonSettled in tests/ui-visual-baseline.spec.ts, wired as prepare on both search-band targets. It polls [data-testid="search-query-ribbon"] until data-status is no longer loading or refetching, instead of relying on settle() visibility alone.

Documents the five-target nondeterminism audit in docs/design-system/GATES.md §6a: what was fixed, what was already mitigated, and known gaps (no global clock pin, unproven /api/setup-status race). Does not commit new baseline PNGs; hosted CI plus human adoption remains the path to committed.

Reviewed by Cursor Bugbot for commit ecc7a1e. Configure here.

… ribbon

A five-agent nondeterminism audit of the five visual-baseline capture targets
(dashboard-shell, dashboard-shell-phone, search-results-band(-phone),
document-viewer, therapy-compass-home) found one live, unmitigated race:
SearchResultsHeaderBand is unconditionally in the DOM before its backing
/api/registry/records fetch resolves, so settle()'s visibility check could
pass while the ribbon still showed its loading spinner instead of the
settled match count and controls. Fixed by waiting for the ribbon's own
data-status attribute to clear "loading"/"refetching" before capture.

Every other class audited (time, data ordering, animation, fonts/async,
environment) was confirmed already neutralized by the suite's existing
settle() contract or structurally unreachable from these five targets'
default render state — see the new docs/design-system/GATES.md §6a for the
per-class findings and what is deliberately left open.

This container has no working browser (Chromium revision drift: the
preinstalled binary is revision 1194, the installed playwright-core expects
1234, and the org egress policy blocks downloading a replacement), so this
fix is offline-typechecked and lint-clean only, not run against the suite
locally. The hosted visual-baseline CI job is the first real proof.
@supabase

supabase Bot commented Sep 2, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: 45f479a1-3f9a-4ac9-be7e-2bfec32af0f5


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@BigSimmo
BigSimmo marked this pull request as ready for review September 2, 2026 10:17
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 2, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-02T10:30:28.887442Z a4a6f06 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@cursor

cursor Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_713be5d5-7afa-4e15-86d9-ae0048aec95f)

@BigSimmo
BigSimmo marked this pull request as draft September 2, 2026 10:17
@BigSimmo
BigSimmo marked this pull request as ready for review September 2, 2026 10:18
@BigSimmo
BigSimmo enabled auto-merge (squash) September 2, 2026 10:18
@BigSimmo
BigSimmo merged commit f74715b into main Sep 2, 2026
29 checks passed
@BigSimmo
BigSimmo deleted the claude/visual-baseline-lxry7z branch September 2, 2026 10:30
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.

2 participants