test(design): prove the accent rail renders, not just that its class is present (#094) - #1415
test(design): prove the accent rail renders, not just that its class is present (#094)#1415BigSimmo wants to merge 21 commits into
Conversation
…is present Closes the #94 gap: design-system gates asserted structure, not rendered effect. PR #1316 shipped an inert accent rail — `.search-band` sits in the cascade against a `border-[color:var(--border)]` utility on the same element, and Tailwind's utilities layer outranks `@layer components` regardless of specificity — while `toHaveClass("search-band")` passed the whole time. Two gates, both verified to go RED on that exact regression rather than merely to pass today: - `check-design-system-contract.mjs` gains UNLAYERED_EFFECT_SELECTORS and an at-rule walker. Injecting `.search-band` into `@layer components` reports "must stay UNLAYERED — it is inside @layer …". The first draft failed on the same injection for the WRONG reason ("base rule is missing", because the regex anchored at line start and never found the indented rule), so the unlayered assertion never ran; the selector now tolerates indentation. - `ui-smoke` asserts computed style at the already-proven /documents/search state: border-top-width 2px, border-top-color equal to --clinical-accent resolved through a probe element (getPropertyValue returns the specified `var(--primary-500)`, which is not comparable), the rail distinct from its own side borders in colour and width, 3px thickness under forced-colors emulation, and every control with a 44px computed min-height rendering at least 44px with none on an inline box. Against the injected regression it reports Expected "2px", Received "1px". Tap targets are selected by computed min-height, not class substring: a class-name match tests the class rather than the effect, which is the very mistake this item exists to correct. Also records ledger #116 (a deregistered worktree silently redirects git at the shared primary checkout — nearly destroyed another session's uncommitted work today) and #117 (issues:next-id collides between concurrent agents; three collisions in 24 hours, resolved only by the union merge driver plus manual renumbering). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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: 53 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 selected for processing (3)
📝 WalkthroughWalkthroughThe pull request adds CSS contract enforcement for the unlayered ChangesLedger updates
Search UI contract coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 228ffc8583
ℹ️ 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".
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Three findings, all on the gates themselves rather than the product.
P2 — nested layers escaped the static check. `enclosingAtRule` returned only
the innermost at-rule, so `@layer components { @media (…) { .search-band { … } } }`
reported `@media` and passed while the rule still lost to Tailwind's utilities
layer. Now `enclosingAtRules` returns the full ancestry and any `@layer` in it
fails, with the whole chain in the message. Verified: the nested injection goes
red where it previously passed.
P2 — the tap audit truncated before validating. `measured.slice(0, 20)` ran
before the inline/undersized filters, so an offender past the twentieth match
was discarded and the test could go green on a rendered defect. Every match is
now validated; only the diagnostic lists cap (at 10).
P1 — the 44px filter was brittle. Not resolved by swapping in 48: --spacing-tap
is 2.75rem and the 48px sheet control is a documented exception, so hard-coding
either number encodes one cohort. The floor is now derived from --spacing-tap
via a probe element, every control at or above it is held to its OWN declared
min-height, and the token itself is asserted to resolve ≥44px. Correct at 44,
at 48, and if the token moves.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…of' into claude/design-computed-style-proof
The open-items table was restored by hand after a #1415 conflict merge wiped it, which left it off Prettier's formatting. verify:pr-local's format:changed gate caught it. Table integrity re-verified: 59 open rows, my two rows intact as #123 and #124, no duplicate IDs, ledger guard passing at 1287 records. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Bundled from draft PR #1414. Confirms the maturity PR was ledger-only: no tests or packages were added; recommendations remain deferred. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
|
Bundled from draft #1414: appended the Review conclusion carried over: #1397 was ledger-only by design — no tests/packages added; account-route Vitest, Draft #1414 is closed. |
…of' into claude/design-computed-style-proof
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #6785 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
|
#codex fix failing CI please. |
…d-style-proof # Conflicts: # docs/outstanding-issues.md
|
@cursoragent fix failing merge conflict |
|
Merge conflict fixed and pushed. Cause: Resolution (kept both, in ID order):
Result:
  |
Resolve the content conflict in docs/outstanding-issues.md by retaining main's #120–#122 (phone-chrome exit, Playwright image lag, CircleCI) alongside this branch's #123–#124 (worktree redirect, next-id race). Co-Authored-By: Cursor Agent <cursoragent@cursor.com> Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/branch-review-ledger.md`:
- Line 1325: Update the ledger entry for claude/design-computed-style-proof to
replace the stale issue references `#116` and `#117` with `#123` for worktree
redirection and `#124` for concurrent ID collisions, preserving the surrounding
record and traceability details.
In `@docs/outstanding-issues.md`:
- Line 160: Update the `#124` issue title to match the evidence in its supporting
detail by replacing the narrower “same hour” claim with “when agents work
concurrently.” Keep the documented collision examples and all other row content
unchanged.
In `@scripts/check-design-system-contract.mjs`:
- Around line 241-259: Update the UNLAYERED_EFFECT_SELECTORS validation loop to
inspect every occurrence of each selector, rather than relying on the first
match from pattern.exec. Assert that each occurrence has no enclosing `@layer`
rule, while preserving the existing missing-rule assertion and diagnostic
context from enclosingAtRules.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a68b8854-3cd4-409c-a600-a0ca1047510a
📒 Files selected for processing (4)
docs/branch-review-ledger.mddocs/outstanding-issues.mdscripts/check-design-system-contract.mjstests/ui-smoke.spec.ts
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 3 unresolved review comments. A stacked PR containing fixes has been created.
Time taken: |
…of' into claude/design-computed-style-proof
…d-style-proof # Conflicts: # docs/outstanding-issues.md
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Superseded by #1404 at |
Pull request was closed
2 participants
Summary
Closes ledger #94 — the design-system gates asserted structure, not rendered effect.
PR #1316 shipped an inert accent rail.
.search-banddeclaresborder-top: 2px solid var(--clinical-accent), but the same element also carries aborder-[color:var(--border)]utility, and Tailwind's utilities layer outranks@layer componentsregardless of selector specificity. Move that rule into a layer and the rail silently renders as a 1px neutral border — whiletoHaveClass("search-band")passes the entire time. Class presence is not effect.Two gates, both verified to go red on that exact regression rather than merely to pass against today's code:
Static —
scripts/check-design-system-contract.mjsUNLAYERED_EFFECT_SELECTORSplus an at-rule walker that reports the enclosing@layer/@mediafor a selector whose declared effect must beat a utility. Injecting.search-bandinto@layer componentsnow fails with ".search-band must stay UNLAYERED — it is inside @layer …".Worth recording: the first draft also failed on that injection, but for the wrong reason — "base rule is missing", because the regex anchored at line start and never found the indented rule, so the unlayered assertion never executed. It would have been correct by accident, and a cosmetic re-indent would have reported a deleted rule. The selector now tolerates indentation. That near-miss is the same class of defect this item exists to fix.
Rendered —
tests/ui-smoke.spec.tsComputed-style assertions at the already-proven
/documents/searchstate:border-top-widthis2px, not the 1px utility borderborder-top-colorequals--clinical-accentresolved through a probe element —getPropertyValuereturns the specifiedvar(--primary-500), which is not comparable to a computedrgb()forcedColors: activethe rail survives as 3px thickness, since--clinical-accentresolves toLinkTextand hue cannot carry the distinction. Previously asserted only by text-searchingglobals.cssinlinebox where CSS ignoresmin-heightoutrightAgainst the injected regression this reports
Expected "2px", Received "1px".Tap targets are selected by computed
min-height, not class substring. A[class*='min-h-tap']match tests the class rather than the effect — exactly the mistake #94 is about — and also catches breakpoint variants that are inert at the measured width.RAG impact: no retrieval behaviour change — a test, a static check, and two ledger rows; nothing under
src/lib/rag/**, clinical-search, ranking-config, the eval harness or the golden fixture is touched. No production source file changes.Verification
npm run verify:pr-local—Test Files 432 passed (432),Tests 4460 passed | 3 skipped.search-band must stay UNLAYERED …, exit 1; reverted cleanExpected "2px", Received "1px", exit 1; reverted cleancheck:design-system-contract— passed, 611 production filesThree earlier runs of the new test failed, all on my own test bugs rather than product defects: the answer-composer helper used on the documents route (its
"Generate source-backed answer"precondition cannot hold there), tap targets selected by class name, and a non-diagnostic assertion that reported only16and sent me guessing. The assertion now names the offending element.Risk and rollout
Test and tooling only — no production source changes. Adds one Chromium case (~19s). Revert is a clean single-commit revert.
Clinical Governance Preflight
No ingestion, answer generation, search/ranking, document access, privacy or clinical output path is touched.
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
Also records two issues verified this session:
git worktree listwhile its directory persisted without a checkout, so everygitcall from that cwd resolved upward to the shared primary checkout, which held another session's 11 modified and 2 untracked files. Agit reset --hardwas issued from there; the primary reflog shows it did not land, but the same sequence would have destroyed that work. The only hint wasgit statusprinting../../../src/....issues:next-idcollided three times in 24 hours across PRs feat(design): port Clinical Sky token system, elevation ladder and radius grid #1375 and fix(ui): repair dead text-4xs classes and six hydration races from #1375 #1391 (#096/#097→#098/#099→#108/#109→ renumbered#110/#111). Correct outcomes came from the union merge driver plus manual renumbering, i.e. hand repair absorbing a race.#40 (visual-regression baselines) remains the larger half of this problem and is deliberately not in scope: baselines freeze the current appearance as the reference, so they need a human sign-off on the Clinical Sky look first. It should land before the #38/#39/#16 refactors, which it would protect.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation