feat(factsheets): add per-category counts; correct the eviction claim (PR D) - #1888
Conversation
… (PR D)
The earlier rollout notes said factsheets' category presets replace
the query and must be evicted to the composer, the way formulation's
Pattern group was. That was wrong: searchHref(query, category)
preserves q and adds category, so factsheets never discarded the
search and its category chips were never a navigate-kind violation.
docs/filter-contract.md corrected to say so.
Kept the desktop rail as real <Link> elements rather than converging
it to SegmentedControl: /factsheets/search is a genuine Next.js
Server Component route (category read from searchParams, filtered
server-side), unlike every other lens mode's client-state-only rail,
so a real anchor is the correct primitive and SegmentedControl cannot
render one. Recorded as a documented exception in docs/filter-contract.md
§2, the same way §4 documents the scope-segment exclusions.
Added per-query counts (the same "how many would I have if I picked
this" predicate as the rest of the contract, not a static catalogue
total) to both the desktop rail and the phone sheet via one shared
options array. Updated the one existing Playwright assertion whose
accessible name changed as a result ("All" -> "All (1)").
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K1kfPCeWWSrbprFz56UeDD
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K1kfPCeWWSrbprFz56UeDD
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 42 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 (2)
Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K1kfPCeWWSrbprFz56UeDD
|
@claude This PR is currently |
|
Hi @BigSimmo — thanks for the @claude mention. Open this in Claude Code on the web to continue here: https://claude.ai/code Context: Automated backlink from the Claude backlink workflow. Claude Code can pick up the mention and push a scoped fix to the branch. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
Resolve PR #1888 against current main by retaining the newer factsheets filter implementation, contract, tests, and shared ledgers. Migrate the branch-added legacy review row and record the fresh adversarial review as immutable ledger entries.
Final review-and-fix summaryFinal state
Review outcome and conflict dispositions
Adversarial review and comments
Decisive verification
Exact-head required CIFor
Advisory checks and residual risk
The requested closing statement — “The PR was not merged. Final merge is left to the user.” — is no longer factually applicable. I did not manually merge or close the PR, but GitHub's pre-existing auto-merge merged it automatically after the required checks passed. |
|
@claude Please main-sync this PR from exact head The current PR delta appears to contain only immutable branch-review record files because the substantive Factsheets filter-count work has already converged elsewhere. Preserve current Run branch-review ledger integrity/dedupe/write-discipline and the smallest docs/format check. Report the new head, resulting changed-file delta, and exact outcomes. |
|
Hi @BigSimmo — thanks for the @claude mention. Open this in Claude Code on the web to continue here: https://claude.ai/code Context: Automated backlink from the Claude backlink workflow. Claude Code can pick up the mention and push a scoped fix to the branch. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
… drop an assertion I got wrong The sort comment claimed a total order that the real corpus contradicts: 21 records share a date, ref and head, because one branch can be reviewed twice at one commit under different scopes. Scope is now a fourth key, and the comment states what actually carries determinism — a filename-sorted input and a stable sort — rather than claiming the comparator does it alone. Task 12's React key was heading for the same trap, identifying a row by fields that are only incidentally unique. It now carries the index. The corpus test gains power over scope and outcome, which it previously never referenced at all. Removed: an assertion I specified that refs never contain a space. The implementer refused it with evidence and was right — 106 of the 454 refs legitimately contain spaces, in forms like 'PR #1888 (claude/...)'. I verified that against the corpus before accepting the refusal. The reasoning is recorded in the plan so the idea is not retried. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Summary
Patterngroup was evicted in PR feat(filters): make formulation a real facet surface #1858. That was false —searchHref(query, category)(src/components/factsheets/factsheets-search-page.tsx) preservesqand addscategory. Factsheets never discarded the search; its category chips were never anavigate-kind contract violation, at either breakpoint.docs/filter-contract.md§1 corrected to say so plainly.<Link>elements ontoSegmentedControl, matching the four lens modes from PR feat(filters): adopt the lens contract across the four one-of-N modes #1857? Recommendation: no — keep the<Link>rail. Reasoning, recorded in full indocs/filter-contract.md§2:/factsheets/searchis a genuine Next.js Server Component route —categoryis read fromsearchParamsand filtered server-side before the client ever mounts (src/app/(search-app)/factsheets/search/page.tsx) — unlike every other lens mode, which filters an already-fetched client-side result set withuseState. A real<a href>is the correct primitive for a server-driven route change (native prefetch, right-click/open-in-new-tab, works with zero JS), andSegmentedControlcannot render one. The back-button argument from the kickoff prompt turned out to be a wash (both<Link>androuter.pushpush a history entry either way), but the hyperlink-semantics argument holds. This is recorded as a documented, narrow exception — not a precedent for any other mode to keep a bespoke rail.option.hint) to both breakpoints from one sharedcategoryOptionsarray — the same "how many would I have if I picked this" predicate as the rest of the contract (§3), computed via the existingfilterFactsheets(query, category)against the current query, not a static catalogue total (categoryCount, used on the factsheets home page's browse pills, answers a different question and was deliberately not reused here)./issues #309's density decision does not bind here (confirmed: it doesn't, and is resolved anyway per the note below).Verification
npm run verify:pr-local— full run, all green:check:runtime, check:installed-lock-parity, format:changed, sitemap:check, docs:check-index, docs:check-inventory, docs:check-scripts, docs:check-links, check:branch-review-ledger, check:outstanding-issues, lint, typecheck, test, build, check:rag:fixtures.npm run check:bundle-budgetafter a cleanrm -rf .next && npm run build— production 1305.0 KiB gzip (baseline 1279.1 KiB, within the 10% tolerance), mockups 294.5 KiB gzip (baseline 267.5 KiB, within the 25% tolerance).npm run verify:uicannot run in this environment (/issues #255— Playwright wants chromium-1234, only chromium-1194 installed). Proved UI directly: a Playwright script from the repo root withexecutablePath: "/opt/pw-browsers/chromium-1194/chrome-linux/chrome"against a real dev server — desktop rail renders a real<a href>with the correctaria-label="Medications (1)"forq=sertraline, clicking it preservesqand addscategory, the phone sheet shows the same counts and preservesqon selection, 0px horizontal overflow at 1440/800/390/320. 11/11 checks passed.ui-formulation.spec.ts/ui-specifiers.spec.ts): found one real hit —tests/ui-smoke.spec.ts's existing factsheet test assertedgetByRole("radio", { name: "All" }), which now reads"All (1)"for theq=sertralinequery it uses. Updated and re-ran against a real production build vianode scripts/run-playwright.mjs tests/ui-smoke.spec.ts --project=chromium --grep "factsheet search keeps query and category filters"with the same executable override — passes.npx vitest runafter mergingorigin/mainin:Test Files 564 passed (564)/Tests 6159 passed | 4 skipped (6163).Risk and rollout
classifyPullRequestFilesreturnsclinicalRisk: false,operationalRisk: false,ragRanking: falsefor this diff.Notes
/issues #170(the rollout-tracking row) updated vianpm run issues:updateto correct its own prior "factsheets: evict category presets" claim and record current state across PR C (open, feat(services): adopt the filter contract with real facets (PR C) #1882) and this PR.origin/mainbefore PR C merged, so its copy ofdocs/filter-contract.mddoes not yet reflect PR C's services-specific edits (the density-tier §5 rewrite) or PR C's own correction to the services half of thenavigate-kind paragraph in §1 — only this PR's factsheets-specific correction. Those will reconcile normally at merge time since PR C and this PR touch different paragraphs of the same file.Generated by Claude Code