fix(filters): resolve PR #1998 merge conflicts and required CI failures - #2025
Merged
Conversation
…ntract after main merge
…ests The filter overhaul rewrote these assertions to expect the tools hero composer visible at every width, but never changed the production composer-placement logic (ClinicalDashboard.tsx, global-search-shell.tsx) that deliberately keeps /tools on the phone footer dock per the documented contract in docs/search-chrome-behaviour.md. Restore the phone/desktop branching so the tests match shipped behaviour.
…nder the ClinicalDashboard line budget PR #1998 added a new inline callback (handleDocumentFiltersApply) that pushed ClinicalDashboard.tsx to 4146 lines, past the 4140-line no-growth budget check:maintainability-budgets enforces. Extract it into use-apply-filters.ts following the same pattern already used for useScopeFilterRelax — a hook module beside the component instead of inline growth. Pure extraction, no behaviour change.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 57 minutes Limit details: You’ve used all 1 included review currently available under your plan. You completed 94 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour. 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 (61)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
BigSimmo
enabled auto-merge
August 17, 2026 09:35
Contributor
CI triageCI failed on this PR. Automated classification of the 4 failed job(s):
Compared with main CI run #11281 (cancelled). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
…arget and layout-transition ratchets check:design-system-contract regressed on this PR's head: - favourites-command-library-page.tsx's new 'Recently used' toggle used bare min-h-10 (40px) on every viewport instead of the file's own min-h-tap ... sm:min-h-10 pattern, dropping below the 48px phone tap floor. - result-filter-control.tsx's new coverage progress bar animated width directly (transition-[width]), which is a real layout-thrash risk the ratchet exists to catch; switched to the scaleX(...) + origin-left pattern already used by ui/progress.tsx and DocumentManagerPanel.tsx. Both are genuine fixes to new code from #1998, not baseline bumps.
BigSimmo
disabled auto-merge
August 17, 2026 10:16
BigSimmo
enabled auto-merge
August 17, 2026 10:16
7 tasks
BigSimmo
added a commit
that referenced
this pull request
Sep 5, 2026
…cal gates match CI (#2621) * verify:pr-local: mirror every static-heavy static-pr gate in the heavy plan (M24) Defect: the heavy-scope plan of scripts/verify-pr-local.mjs selected only lint, typecheck and test, while CI's static-pr job additionally runs the migration-role, function-grant and owner-scope guards plus the design-system, generated-artifact, docs and ledger gates for the same scope. A migration using a non-postgres role, a SECURITY DEFINER function left open to PUBLIC, or an API handler reading an owner-scoped table without an owner filter passed the documented "PR mirror" green locally and reddened only after push. Trigger: `npm run verify:pr-local` on any PR touching supabase/migrations/** or src/app/api/** (both classify static_heavy). Fix: a `staticHeavyGuards` list in CI step order, selected for static_heavy scope ahead of lint/typecheck/test; the docs/ledger steps CI runs for heavy scope now ride docsScripts for docs_changed || static_heavy_changed, and check:mockups joins that list to match static-pr. The assertPlan self-test (check:verification-plan) is updated in the same change and pins the three tenancy/database guards by name. Test: tests/ci-cache-safety.test.ts "mirrors every static-heavy static-pr step in the verify:pr-local heavy plan (M24)" parses the static-pr job and fails on any static_heavy-gated `npm run` step missing from the local heavy plan (red on the previous script with 25 missing gates), and checks the guards stay out of the docs-only plan. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t * ci: run the medication interaction index drift gate in static-pr (M30) Defect: `npm run check:medication-interactions` rebuilt the index from the snapshot and diffed it, but ran only inside verify:pr-local — no CI job invoked it — and the only unit test compared data/medication-interaction- index.json against its own sourceRowCount, never against data/medications-snapshot.json. A snapshot or lexicon edit merged without re-running `npm run medications:interactions` (the bare-PR route permits exactly that) shipped a stale index: a renamed drug missing from names/bySlug, an added row that never alerts, or a severity token rendered with the old tone on the reverse path, with every required check green. Trigger: any PR editing data/medications-snapshot.json or the interaction lexicon without regenerating the index, pushed from an environment that skipped verify:pr-local. Fix: a "Medication interaction index drift" step in the static-pr job, under the same static_heavy_changed condition as its lexicon-report sibling. Tests: tests/check-medication-interactions-index.test.ts compares the committed index to the committed snapshot drug for drug and row for row (names, row counts, row keys, verbatim note text, sourceRowCount) and proves the failure shape against a mutated record (renamed drug, edited row, added row). tests/ci-cache-safety.test.ts pins the new static-pr step (red before the workflow change). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t * hazard-controls: require proofs to name their control, run the register gate on PRs (M33) Defect: scripts/check-clinical-hazard-controls.mjs proved only that each listed control symbol appeared somewhere in the control paths and that each listed test file existed with a tests/*.test.ts name. It never checked that any cited test references the control, so a proof test emptied of its case, or a symbol renamed and re-added as a comment, kept CLINICAL_HAZARD_CONTROLS_PASS printing. The gate also ran only inside the provider-backed governance:release chain, so a control rename or the register's reviewExpiresAt lapsing produced no pull-request signal. Trigger: any PR renaming one of the 13 control symbols or deleting the relevant it(...) case; or the calendar reaching the manifest's reviewExpiresAt. Fix: for every controlled/partial hazard, at least one listed test must name a control symbol or import a control path module, reported as "<id>: no listed test references a control symbol or imports a control path". The gate joins the static-pr job (docs or static-heavy scope, which already checks out with fetch-depth 0) and verify:pr-local's docs/heavy plan. On a shallow clone the script prints CLINICAL_HAZARD_CONTROLS_SHALLOW_CLONE and skips only the reviewedCommit ancestry checks instead of reporting every reviewed commit as missing; every file, symbol, test-reference and date check still runs (verified against a depth-one clone of this branch). The existing date, commit and role checks are unchanged. Tests: tests/clinical-hazard-controls.test.ts "requires at least one listed test to reference a control symbol or control path" (red before the change) and tests/ci-cache-safety.test.ts pins the static-pr step and its full-history checkout. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t * hazard-controls: escape regex metacharacters in control symbols (L22) Defect: the symbol-presence check in scripts/check-clinical-hazard-controls.mjs escaped symbols with /[.*+?^${}()|[\\]\\]/g, which JavaScript parses as the class [.*+?^${}()|[\\] followed by a literal "\]", so no metacharacter was ever escaped. A dotted symbol matched as a wildcard (fail-open: reported present when absent) and a symbol carrying "(" or "[" threw or never matched. Trigger: any control symbol that is not a plain identifier. Today's 13 are plain identifiers, so the defect was latent. Fix: the repository's standard escape class /[.*+?^${}()|[\]\\]/g, in the one symbolPattern helper both the control-path and test-reference checks use. Test: tests/clinical-hazard-controls.test.ts "escapes regex metacharacters in control symbols instead of treating them as wildcards" — a runtime-assembled "foo.bar" must not match the file's "fooxbar" sentinel (red before the fix), and a "sentinel(" symbol must neither throw nor be reported missing. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t * owner-scope guard: recognise owner filters, not the owner_id substring (L47) Defect: scripts/check-owner-scope-api.mjs treated a handler as owner-scoped if the literal substring "owner_id" appeared anywhere in its body. A projection such as .select("id,owner_id,title"), a comment, or an unrelated object key satisfied it, so the two most likely tenancy regressions — a handler that returns owner_id without filtering on it, and one that only mentions it in prose — merged green while the gate reported "60 files clean". Trigger: a future src/app/api handler that reads an owner-scoped table and names owner_id without filtering on it. Fix: SCOPE_TOKENS are now filter shapes — .eq/.neq/.is/.in/.not/.match/ .filter("owner_id" or "documents.owner_id"), PostgREST owner_id.eq./.is./.in. strings, an owner_id: write payload key, the documents!inner(owner_id) join projection — plus the existing word-bounded helper names. The guard stays green on the current tree (60 files, 26 tables; the setup-status and clinical-quality reads remain covered by the documented allowlist). Child tables without an owner_id column and src/lib callers of createAdminClient are unchanged and reported for an owner decision. Test: the script's --self-test now fails a select-list-only handler with an owner_id comment (red before the change) and passes every filter shape the codebase uses (.is, .in, .or("owner_id.eq."), documents.owner_id, the inner join, an insert payload, single-quoted .eq). tests/owner-scope-guard.test.ts still passes unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t * tests: restore the 320px low-confidence AccessibleTable mockup journey (M31) Defect: PR #2006 (54585e9, 2026-08-16) added the /mockups/accessible- table-browser-fixture route and a Playwright journey proving that a low-confidence clinical table keeps "Not recorded" and its verify-against- source warning fully legible at 320px; it closed /issues #237. Merge commit ab7c513 (PR #2025, one day later) dropped the journey from tests/ui-tools.spec.ts while no non-merge commit ever removed it, so no PR diff showed the loss and nothing has run it since. Trigger: any change to AccessibleTable's low-confidence / missing-value rendering at 320px. Fix: the journey is restored verbatim from 54585e9 into its own file, tests/ui-accessible-table-mockup.spec.ts (tagged @mockup, so it runs in the advisory chromium-mockups project against the surviving fixture route), and `accessible-table-mockup` joins mockupSpecPattern and the top-level testMatch in playwright.config.ts. check:ci-scope's mockup-spec parity, the shard parity gate and the browser-test-plan self-test all pass with the new arm. docs/agents/test-deletion-guard.md records the merge-commit blind spot of check:diff-integrity. mockups/README.md is outside this change's scope; its 2026-09-02 row still says no committed test navigates to the fixture. The pre-commit hook regenerated docs/design-system/adoption-manifest.json (the new pin test now references AccessibleTable); that output is included. Test: tests/playwright-pr-shards.test.ts "collects the 320px AccessibleTable mockup journey in the advisory mockup project (M31)" pins the spec file, its @mockup tag, the fixture route it navigates to, and its collection by testMatch and mockupSpecPattern but not the production matcher (red before this change: the spec was missing). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t * playwright: collect the orphaned ui-tools-show-all spec and guard both directions (M32) Defect: tests/ui-tools-show-all.spec.ts landed with PR #2008 (0b95d06, 2026-08-16) but its basename was in neither the top-level testMatch nor productionSpecPattern, mockupSpecPattern or seededSpecPattern in playwright.config.ts, so no project ever collected it — not test:e2e, not verify:ui, not the PR shards, not the release matrix. Both existing parity guards checked one direction only (what the matcher selects is sharded; named specs are collected), so a never-selected file was invisible to them. Trigger: any Playwright run; the phone launcher journey (/?mode=tools "Show all" link, 48px tap target, keyboard navigation to /tools) had zero browser evidence for 17 days while appearing to be tested. Fix: `tools-show-all` joins testMatch and productionSpecPattern in playwright.config.ts and the mirrored productionSpecFilePattern in scripts/playwright-pr-shards.mjs, and the spec is assigned to PR shard 2 (3.0s estimate on the shard with the smallest full total; the balance guards still hold at 1.5s/3.0s spread). No spec was deleted, moved or skipped. Tests: tests/playwright-pr-shards.test.ts "collects every on-disk spec in the top-level testMatch and at least one project (M32)" asserts the missing direction against the files on disk (red before the change, naming exactly tests/ui-tools-show-all.spec.ts), and "collects the phone launcher Show-all journey as a sharded production spec (M32)" pins the file by name. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t * gates: repair two couplings the main merge broke (M24, M32) The merge itself was clean on these two files, which is exactly why they broke: each side added something the other side's guard counts. - scripts/playwright-pr-shards.mjs: main and this branch both added a shard row for tests/ui-tools-show-all.spec.ts, so the file listed it twice and `check:playwright-pr-shards` reported "duplicated". Kept main's shard-1 row (zero timing, the lightest measured shard) and folded this branch's M32 rationale into its comment. The spec is still sharded exactly once; the by-name pin in tests/playwright-pr-shards.test.ts asserts membership, not which shard. - scripts/verify-pr-local.mjs: main added a `Design drift ratchet` step to static-pr under static_heavy_changed, which the M24 mirror test in tests/ci-cache-safety.test.ts requires the local heavy plan to carry. Added check:design-drift-ratchet to staticHeavyGuards in CI step order (after check:icon-scale). The assertPlan expectations spread that list, so they move with it. check:playwright-pr-shards: "PR UI shard parity OK: 36 production specs across 3 groups." check:verification-plan: "PR-local verification plan self-test passed." Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
codex/filter-system-overhaul) forward, merged cleanly onto currentmain, with the specific fixes needed to satisfy the requiredpr-requiredCI aggregate:mainindocs/design-system/COMPONENTS.md,docs/design-system/adoption-manifest.json, andsrc/components/ui/sheet.tsx(kept theheaderRef/headerHidden/data-sheet-headerheader-collapse attributes that landed onmainvia feat(guide): pin guide chrome controls to shared scroll-hide behavior #2007 after feat(filters): implement clinical result filter overhaul #1998 branched).npm run design-system:design-sync:update,generate-design-system-adoption.mjs --write) socheck:design-sync-contractand thedesign-sync-contract/design-sync-visual-exportsunit tests pass again — they were failing withdtsPropsFor must be generated from source public Props typesbecause the merge left the generated artifacts stale relative tosheet.tsx.tests/ui-tools.spec.ts: feat(filters): implement clinical result filter overhaul #1998's first commit rewrote several assertions to expect the/toolshero composer visible at every viewport width, but never changed the production composer-placement logic inClinicalDashboard.tsx/global-search-shell.tsx, which deliberately keeps/toolson the phone footer dock per the documented contract indocs/search-chrome-behaviour.md("Tools directory... Compact bottom dock on phones; in-flow hero composer fromsm"). Restored the phone/desktop branching in the 4 affected tests to match the shipped, documented behaviour instead of changing production code on a protected search-chrome surface.handleDocumentFiltersApplycallback out ofClinicalDashboard.tsxintosrc/components/clinical-dashboard/use-apply-filters.ts(mirroring the existinguseScopeFilterRelaxpattern) because feat(filters): implement clinical result filter overhaul #1998's addition pushed the component to 4146 lines, past the 4140-line no-growth budgetcheck:maintainability-budgetsenforces. Pure extraction, no behaviour change.mainafter this branch started (Add outstanding-issues row-staleness fingerprint checks #2009, a URL-regex assertion robustness fix in the same test file).Verification
npm run verify:pr-local— see below (ran the constituent gates directly: full unit suite, typecheck, lint, design-sync/adoption/maintainability-budget checks, targeted + fullui-tools.spec.tsChromium Playwright run, and a cleanrm -rf .next && npm run build)npm run verify:ui— rantests/ui-tools.spec.ts(95/95 passed) against a clean production build with--project=chromium, including the 4 tests that were failing in feat(filters): implement clinical result filter overhaul #1998's CI (Production UI (3))npx vitest run— 635 files / 6771 tests passed, 4 skippednpx tsc --noEmit -p tsconfig.json— cleannpx eslinton all changed files — cleannode scripts/check-design-sync-contract.mjs— passes (54 components, 7 guidelines)node scripts/generate-design-system-adoption.mjs --check— passes (54 components, 59 roots)node scripts/check-maintainability-budgets.mjs— passes (ClinicalDashboard.tsx: 4136/4140 lines)npm run format— clean, nothing to reformatRisk and rollout
ClinicalDashboard.tsxextraction, which is a pure refactor (same logic, moved to a hook module) — verified by the unchangeddocument-filter-panel.dom.test.tsx/medication-prescribing-workspace.dom.test.tsxsuites and typecheck/lint. Thesheet.tsxconflict resolution keeps both features (existing header-collapse frommain+ this PR's filter-sheet changes) with no logic removed on either side.Clinical Governance Preflight
Notes
main, plus the fixes above needed to pass the requiredpr-requiredchecks (static-pr,coverage,production-ui) that were failing on feat(filters): implement clinical result filter overhaul #1998's head. feat(filters): implement clinical result filter overhaul #1998's own branch was left untouched (per this session's branch policy) — recommend closing feat(filters): implement clinical result filter overhaul #1998 once this PR merges, to avoid duplicate/conflicting history.Advisory UI's one quarantined-lane failure (tests/ui-tools-search-mode-mockup.spec.ts— a@mockup"2 showing" filter-summary text assertion) is unrelated and non-blocking (not part ofpr-required); left as-is since it's outside this fix's required scope.Generated by Claude Code