feat(mockups): restyle the shared filter sheet, and give the segment bar a job - #1846
Conversation
…bar a job Rounds one and two settled the information architecture. This is craft, drawn on the formulation sheet because it stresses the layout hardest: two groups, sixteen options, and the longest title in the app. The segment bar now carries SCOPE, not a verb. Round two used it for "narrow these / start a new search", which is a mode set once and rarely changed — a poor use of the most valuable strip in the sheet. Here it is "These results 2 | All mechanisms 12" with live counts on both segments. That is a decision the reader makes constantly and which nothing in the product answers today: filtering two results by twelve domains is close to pointless, and the only route to the full set is clearing the query and losing it. It also makes the empty state recoverable — the commit button becomes "Show N in all mechanisms" rather than a dead end. Three defects the study documents, all verifiable in source: - Biological, Social and Cultural are offered as domain chips and match zero of the twelve mechanisms. Counts expose this on sight. - formulationDomainGroups already exists in src/lib/formulation.ts, with four themes each carrying a written description, and the sheet ignores it in favour of one flat ragged wrap of twelve chips. - formulationSearchPresets.slice(0, 4) of five leaves "If it is not perfect" unreachable from the filter. Three styles over one shared set of craft fixes: A refined clinical, B themed cards, C dense list. Counts here are intrinsic (how many mechanisms carry that domain) rather than the services study's "total if I added this". Domains are a single OR group, so the union contract reports the unchanged total for an empty domain — Cultural would read 7, indistinguishable from a full one. The commit button remains what predicts the outcome. The desktop specimen's min-width is gated at sm: unconditional, it propagated out of its scroll box into the grid track and scrolled the page 45px at 320px, the blocking narrow breakpoint. Measured, not assumed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
Written into AGENTS.md by next dev via node_modules/next/dist/server/lib/generate-agent-files.js, verified present. Left uncommitted it regenerates on every dev-server start and leaves a permanently dirty tree; committed here on its own rather than buried inside an unrelated design change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR adds a formulation filter-sheet restyle study with live scope and domain filtering, three responsive presentation styles, empty-state recovery, route integration, documentation, production-search validation, and review ledger updates. ChangesFilter sheet restyle mockup
Review ledger maintenance
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant FilterSheetRestyleMockupPage
participant FilterSheetRestyleMockupsPage
participant StyleSheetRenderer
Browser->>FilterSheetRestyleMockupPage: Open filter-sheet restyle route
FilterSheetRestyleMockupPage->>FilterSheetRestyleMockupsPage: Render mockup page
FilterSheetRestyleMockupsPage->>StyleSheetRenderer: Pass scope and selected domains
StyleSheetRenderer-->>FilterSheetRestyleMockupsPage: Render counts, styles, previews, and empty states
FilterSheetRestyleMockupsPage-->>Browser: Display responsive mockup study
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. Comment |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
![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: 2f69b2346e
ℹ️ 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".
|
@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/filter-popup-design-mockups-x6sbjv at starting commit 1fa0113; 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/filter-popup-design-mockups-x6sbjv, 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. 🎉 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". |
Regenerated by the production build run for this PR's bundle-budget check. Written by next dev via node_modules/next/dist/server/lib/generate-agent-files.js; left uncommitted it re-appears on every build and leaves a dirty tree. Its own commit rather than folded into the SegmentedControl change, so it can be dropped independently. Note this block is not yet on main — PR #1846 also carries it, so whichever lands first makes the other a no-op. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #9889 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
…esign-mockups-x6sbjv
…ps-x6sbjv' into claude/filter-popup-design-mockups-x6sbjv
Another agent synced this branch from main as b34be0c while I was doing the same. Their conflict resolution dropped two append-only rows that were present on the branch at 442b709 and are not on main: 2f69b23... claude/filter-popup-design-mockups-x6sbjv — the PR #1846 record e34a617... 1846 — the full-PR-diff review record Both restored verbatim in their original position rather than re-appended, so the records keep their own SHAs and evidence rather than becoming new same-day rows. AGENTS.md: the ledger is append-only and syncs must not rewrite existing rows. Four other rows appeared in the diff as moved rather than lost — the union driver reorders on merge — and were verified present in main, their sync and mine before concluding anything about them. ledger:dedupe reports 865 unique rows, the guard passes, and my own redundant main merge was folded into their tip rather than stacked as a second sync. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
…ps-x6sbjv' into claude/filter-popup-design-mockups-x6sbjv
![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
🧹 Nitpick comments (2)
src/components/filter-sheet-restyle-mockups.tsx (1)
147-167: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConfirm the proportion-bar denominator is intended to ignore scope.
useDomainCountscomputesperDomainfrom the scoped pool, butDomainRowdivides byMECHANISMS.length(12) at line 455. In thecurrentscope the pool holds two mechanisms, so every bar renders at most 17% and the relative weight the comment describes is lost. If the bar must show weight inside the scope, pass the scope pool size and divide by it.Also applies to: 447-457
🤖 Prompt for 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. In `@src/components/filter-sheet-restyle-mockups.tsx` around lines 147 - 167, Update useDomainCounts and DomainRow so the proportion-bar denominator matches the intended scope: expose the scoped pool size from useDomainCounts and use it instead of MECHANISMS.length when rendering each domain bar. Preserve safe handling for an empty scope pool to avoid division by zero.tests/filter-sheet-restyle-mockups.test.ts (1)
11-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the literal mechanism names.
filterSheetRestyleCurrentSubsetis defined from the same search expression, so the equality assertion restates its implementation. For"Worry", assert["Reassurance seeking", "Worry"]after sorting both values. Retain the length comparison.🤖 Prompt for 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. In `@tests/filter-sheet-restyle-mockups.test.ts` around lines 11 - 15, Replace the self-referential equality assertion for filterSheetRestyleCurrentSubset with an assertion against the literal mechanism names ["Reassurance seeking", "Worry"], sorting both the expected values and the subset before comparison. Retain the existing length and formulationMechanisms comparison assertions.Source: Path instructions
🤖 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 `@mockups/README.md`:
- Around line 133-135: Update the study description consistently in the README,
the component header comment, and the page lede to match the actual specimen
data: four domain themes, twelve domains, twelve mechanisms, four presets, and
thirteen domain chips. Remove the inaccurate “two groups, sixteen options”
wording without changing the underlying study data or layout.
In `@src/components/filter-sheet-restyle-mockups.tsx`:
- Around line 309-313: Update the compact phone scope segment sizing in the
segment button className to use the existing min-h-tap utility instead of
min-h-11, while preserving min-h-9 for non-compact segments and the surrounding
styling.
- Around line 354-392: Update DomainChip to generate the empty-description
element id with React’s useId rather than deriving it solely from domain, and
use that unique id consistently in both aria-describedby and the sr-only span.
Keep the existing empty-state behavior unchanged.
---
Nitpick comments:
In `@src/components/filter-sheet-restyle-mockups.tsx`:
- Around line 147-167: Update useDomainCounts and DomainRow so the
proportion-bar denominator matches the intended scope: expose the scoped pool
size from useDomainCounts and use it instead of MECHANISMS.length when rendering
each domain bar. Preserve safe handling for an empty scope pool to avoid
division by zero.
In `@tests/filter-sheet-restyle-mockups.test.ts`:
- Around line 11-15: Replace the self-referential equality assertion for
filterSheetRestyleCurrentSubset with an assertion against the literal mechanism
names ["Reassurance seeking", "Worry"], sorting both the expected values and the
subset before comparison. Retain the existing length and formulationMechanisms
comparison assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b90d0085-bb76-485f-b95d-5a6e0250fde6
📒 Files selected for processing (7)
docs/branch-review-ledger.mddocs/site-map.mdmockups/README.mdsrc/app/mockups/filter-sheet-restyle/page.tsxsrc/app/mockups/mockups-layout-client.tsxsrc/components/filter-sheet-restyle-mockups.tsxtests/filter-sheet-restyle-mockups.test.ts
|
Addressed the three current CodeRabbit review findings at
Focused validation passed in the repair run: Prettier, focused ESLint, |
|
Final review disposition on head
Exact-head CI is fully green: CI, build and bundle budget, static guards, lint, typecheck, unit coverage, safety/config, critical UI, advisory UI, all three production-UI partitions, Lighthouse, SAST, and secret scanning. |
2 participants
Summary
Adds
/mockups/filter-sheet-restyle— three style directions for the shared filter sheet, drawn on the formulation specimen because that is the one that stresses the layout hardest: two groups, sixteen options, and the longest title in the app. Rounds one and two (PR #1828) settled the information architecture; this round is craft.These results 2 | All mechanisms 12, with live counts on both segments. That is a decision the reader makes constantly and which nothing in the product answers today: filtering two results by twelve domains is close to pointless, and the only current route to the full set is clearing the query and losing it. It also makes the empty state recoverable — the commit button becomes "Show N in all mechanisms" instead of a dead end.mockups-layout-client.tsx, records the study inmockups/README.md, regeneratesdocs/site-map.md.Three defects the study documents, all verifiable in source
formulationDomainGroupsalready exists insrc/lib/formulation.ts— four themes, each with a written description — and the sheet ignores it, rendering one flat ragged wrap of twelve chips. The structure was authored and then never used.formulationSearchPresets.slice(0, 4)of five leaves "If it is not perfect" unreachable from the filter.A deliberate departure from the services study
Per-option counts here are intrinsic (how many mechanisms carry that domain), not the "total if I added this" contract PR #1828 settled on. Domains are a single OR group, so the union contract reports the unchanged total for an empty domain — Cultural would read
7, indistinguishable from a full one, hiding the single most useful thing these counts have to say. The commit button remains what predicts the outcome. This is called out in the study and inmockups/README.md.Design scratch only — no production route, component or behaviour changes.
Verification
npm run verify:pr-local— fully green, no failuresThe
tests/pr-handoff-stop.test.tsfailure reported on PR #1828 no longer occurs:af8e717on main removed thechmod 0o555assertion that could not hold when the suite runs as uid 0.npm run check:bundle-budget— afterrm -rf .next && npm run build, since a cached.nextmakes this report a false pass.Worth watching rather than acting on: three filter studies have taken the mockups bucket from 267.5 to 295.8 KiB (+10.6% of a 25% tolerance). Retiring the earlier two once a direction is chosen would return most of it.
320px is clean, and that was measured rather than assumed. An unconditional
min-w-[46rem]on the desktop specimen propagated out of its scroll box into the grid track and scrolled the whole page 45px at the blocking narrow breakpoint. Bisected section-by-section, confirmed by zeroing the min-width, and fixed by gating it atsm. All three filter mockup routes now report0pxdocument overflow at 320px.UI verification not run in full:
npm run verify:uiwas not run because this adds only a design-scratch route that 404s in production and touches no production component or shared chrome. Proof was taken against the route directly — rendered atnpm run ensure's URL and screenshotted at desktop and phone.Risk and rollout
src/app/mockups/**404s viamockupsEnabled()(src/lib/env.ts) and is blocked again at the proxy;robots.tsdisallows it; eslint forbids production source importing*-mockups. The only shared file touched ismockups-layout-client.tsx, one additional pathname branch.Notes
The second commit (
chore(agents)) commits thenextjs-agent-rulesblock thatnext devwrites intoAGENTS.mdvianode_modules/next/dist/server/lib/generate-agent-files.js— verified present, and the marker is already in git history. Left uncommitted it regenerates on every dev-server start and leaves a permanently dirty tree. It is a separate commit rather than buried inside a design change, so it can be dropped independently if you would rather it stayed out.Before any of this ships: Biological, Social and Cultural should be fixed, not merely labelled — either tag mechanisms into those domains or stop offering them, because shipping a permanent "0" is a worse admission than shipping nothing. Scope also needs a home in the URL, or a shared link silently reverts to the current-results scope and the recipient sees a different set from the sender.
No Clinical Governance Preflight or
RAG impact:line:classifyPullRequestFilesreturnsclinicalRisk: falseandoperationalRisk: false, and this touches no RAG-ranking surface.Generated by Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests