Fix Specifiers and Formulation separation#672
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a standalone Specifiers workspace with catalog-backed search, detail, builder, compare, and map routes. Separates Specifiers and Formulation universal-search domains, updates navigation and sitemap generation, and adds unit and Playwright coverage. ChangesSpecifiers mode
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant SpecifiersRoute
participant SpecifiersHomePage
participant searchSpecifiers
Browser->>SpecifiersRoute: Request /specifiers?q=...&run=1
SpecifiersRoute->>SpecifiersHomePage: Pass query and autoRunSearch
SpecifiersHomePage->>searchSpecifiers: Search catalog
searchSpecifiers-->>SpecifiersHomePage: Return ranked records
SpecifiersHomePage-->>Browser: Render results and detail links
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 888d4a93a9
ℹ️ 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. 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. After fixing or dispositioning a thread, reply in that thread with as the first line, followed by a concise summary; that marker authorizes the workflow to close that exact thread. If human input or new authorization is required, do not use the marker and leave the thread open with the blocker. 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 888d4a93a9
ℹ️ 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".
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Heuristic only — a main-side or flake label is a starting point, not a verdict. |
![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 (3)
src/components/clinical-dashboard/master-search-header.tsx (1)
434-453: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNested ternary keeps growing with each new mode.
actionMenuSetIdis now a 10-branch nested ternary. Functionally correct, but a lookup map keyed bysearchModewould scale better as more modes are added.♻️ Suggested lookup-based rewrite
- const actionMenuSetId: ModeActionSetId = - searchMode === "prescribing" - ? "prescribing" - : searchMode === "forms" - ? "forms" - : searchMode === "services" - ? "services" - : searchMode === "documents" - ? "documents" - : searchMode === "favourites" - ? "favourites" - : searchMode === "differentials" - ? "differentials" - : searchMode === "specifiers" - ? "specifiers" - : searchMode === "formulation" - ? "formulation" - : searchMode === "tools" - ? "tools" - : "answer"; + const actionMenuSetIdByMode: Partial<Record<AppModeId, ModeActionSetId>> = { + prescribing: "prescribing", + forms: "forms", + services: "services", + documents: "documents", + favourites: "favourites", + differentials: "differentials", + specifiers: "specifiers", + formulation: "formulation", + tools: "tools", + }; + const actionMenuSetId: ModeActionSetId = actionMenuSetIdByMode[searchMode] ?? "answer";🤖 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/clinical-dashboard/master-search-header.tsx` around lines 434 - 453, Replace the nested ternary assigned to actionMenuSetId with a lookup map keyed by searchMode, preserving the existing mappings for prescribing, forms, services, documents, favourites, differentials, specifiers, formulation, and tools, with "answer" as the fallback. Keep the result typed as ModeActionSetId and update the map when adding future modes.src/components/clinical-dashboard/global-search-shell.tsx (1)
123-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate mode-exclusion list risks future drift.
shouldRenderDashboardSearchis computed identically inGlobalSearchShellClient(Lines 123-131) andGlobalStandaloneSearchShellClient(Lines 244-252). Adding "specifiers" required editing both copies in this PR — a future mode addition could easily miss one, silently breaking that mode's standalone-search routing in only one entry point.♻️ Suggested extraction
+const dashboardExcludedSearchModes: readonly AppModeId[] = [ + "services", + "forms", + "favourites", + "differentials", + "specifiers", + "formulation", +]; + +function shouldRenderDashboardSearchFor( + hasSubmittedModeSearch: boolean, + resolvedSearchMode: AppModeId, + isDocumentSearchMockupRoute: boolean, +) { + return ( + hasSubmittedModeSearch && + !dashboardExcludedSearchModes.includes(resolvedSearchMode) && + !isDocumentSearchMockupRoute + ); +}Then call
shouldRenderDashboardSearchFor(...)from both components.Also applies to: 244-252
🤖 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/clinical-dashboard/global-search-shell.tsx` around lines 123 - 131, Extract the shared mode-exclusion logic from GlobalSearchShellClient and GlobalStandaloneSearchShellClient into a single shouldRenderDashboardSearchFor(...) helper. Have both components call this helper with their existing search state and route values, preserving the current exclusions and document-search mockup behavior so future modes require only one update.tests/ui-specifiers.spec.ts (1)
1-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate Playwright test-harness helpers across specs.
tests/ui-specifiers.spec.tscopy-pastesblockExternalRequests,gotoApp,expectNoHorizontalOverflow, andexpectNoBlockingAxeViolationsverbatim fromtests/ui-formulation.spec.ts, giving the same root cause (no shared test-utils module) at both sites.
tests/ui-specifiers.spec.ts#L1-L49: import the four helpers from a shared module instead of redefining them here.tests/ui-formulation.spec.ts#L1-L49: extract these four helpers into a sharedtests/utils/...module and import them here too.♻️ Suggested extraction
// tests/utils/playwright-ui-helpers.ts export async function blockExternalRequests(page: Page) { /* ... */ } export async function gotoApp(page: Page, path: string) { /* ... */ } export async function expectNoHorizontalOverflow(page: Page) { /* ... */ } export async function expectNoBlockingAxeViolations(page: Page, testInfo: TestInfo) { /* ... */ }Then in both spec files:
-async function blockExternalRequests(page: Page) { ... } -async function gotoApp(page: Page, path: string) { ... } -async function expectNoHorizontalOverflow(page: Page) { ... } -async function expectNoBlockingAxeViolations(page: Page, testInfo: TestInfo) { ... } +import { blockExternalRequests, gotoApp, expectNoHorizontalOverflow, expectNoBlockingAxeViolations } from "./utils/playwright-ui-helpers";🤖 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/ui-specifiers.spec.ts` around lines 1 - 49, Extract blockExternalRequests, gotoApp, expectNoHorizontalOverflow, and expectNoBlockingAxeViolations from tests/ui-formulation.spec.ts lines 1-49 into a shared tests/utils module, exporting them with the required Playwright types. Remove the duplicate definitions from tests/ui-formulation.spec.ts lines 1-49 and tests/ui-specifiers.spec.ts lines 1-49, and import the helpers from the shared module in both specs.
🤖 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 `@src/app/specifiers/`[slug]/page.tsx:
- Around line 1-16: Improve specifier metadata and filtering safety: in
src/app/specifiers/[slug]/page.tsx lines 1-16, add generateMetadata using the
matched record’s name and summary; in src/app/specifiers/compare/page.tsx lines
1-14, add metadata reflecting compared specifiers; in
src/app/specifiers/map/page.tsx lines 1-11, add static map metadata. In
src/lib/specifiers.ts lines 635-676, introduce and reuse a shared
SpecifierApplicability union for record appliesTo values and both applicability
maps. In src/components/specifiers/specifier-map-page.tsx lines 19-22,
synchronize selection when initialSlug changes, or key the component
accordingly.
In `@src/components/specifiers/specifier-builder-page.tsx`:
- Around line 45-52: Update SpecifierBuilderPage so initialSpecifiers are not
filtered using the hard-coded diagnosisPresets[0] value; determine the intended
diagnosis from the deep link before filtering, or preserve incompatible
selections until the user chooses a compatible diagnosis. Add the smallest
targeted regression test proving a non-MDD deep link retains its requested
specifier, then run the narrowest relevant validation.
In `@src/components/specifiers/specifiers-home-page.tsx`:
- Around line 194-196: Update the user-facing copy in the specifiers home page,
including the explanatory paragraph near search results and the first-result
label around the corresponding result markup, to describe ranking as text
relevance and use “Top match” instead of clinical “fit.” Add or update the
smallest targeted test to assert the revised label, then run the narrowest
relevant validation.
---
Nitpick comments:
In `@src/components/clinical-dashboard/global-search-shell.tsx`:
- Around line 123-131: Extract the shared mode-exclusion logic from
GlobalSearchShellClient and GlobalStandaloneSearchShellClient into a single
shouldRenderDashboardSearchFor(...) helper. Have both components call this
helper with their existing search state and route values, preserving the current
exclusions and document-search mockup behavior so future modes require only one
update.
In `@src/components/clinical-dashboard/master-search-header.tsx`:
- Around line 434-453: Replace the nested ternary assigned to actionMenuSetId
with a lookup map keyed by searchMode, preserving the existing mappings for
prescribing, forms, services, documents, favourites, differentials, specifiers,
formulation, and tools, with "answer" as the fallback. Keep the result typed as
ModeActionSetId and update the map when adding future modes.
In `@tests/ui-specifiers.spec.ts`:
- Around line 1-49: Extract blockExternalRequests, gotoApp,
expectNoHorizontalOverflow, and expectNoBlockingAxeViolations from
tests/ui-formulation.spec.ts lines 1-49 into a shared tests/utils module,
exporting them with the required Playwright types. Remove the duplicate
definitions from tests/ui-formulation.spec.ts lines 1-49 and
tests/ui-specifiers.spec.ts lines 1-49, and import the helpers from the shared
module in both specs.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d17f7f70-876f-43ba-bf7e-4f4054db80de
📒 Files selected for processing (34)
docs/branch-review-ledger.mddocs/site-map.mdplaywright.config.tsscripts/generate-site-map.tssrc/app/page.tsxsrc/app/specifiers/[[...path]]/page.tsxsrc/app/specifiers/[slug]/page.tsxsrc/app/specifiers/builder/page.tsxsrc/app/specifiers/compare/page.tsxsrc/app/specifiers/layout.tsxsrc/app/specifiers/map/page.tsxsrc/app/specifiers/page.tsxsrc/components/clinical-dashboard/ClinicalSidebar.tsxsrc/components/clinical-dashboard/global-search-shell.tsxsrc/components/clinical-dashboard/master-search-header.tsxsrc/components/clinical-dashboard/mode-action-popup.tsxsrc/components/clinical-dashboard/universal-search-command-surface.tsxsrc/components/specifiers/specifier-builder-page.tsxsrc/components/specifiers/specifier-compare-page.tsxsrc/components/specifiers/specifier-map-page.tsxsrc/components/specifiers/specifier-record-page.tsxsrc/components/specifiers/specifier-ui.tsxsrc/components/specifiers/specifiers-home-page.tsxsrc/lib/app-mode-icons.tssrc/lib/app-modes.tssrc/lib/specifiers.tssrc/lib/universal-search-domains.tssrc/lib/universal-search-mode-context.tssrc/lib/universal-search.tstests/app-modes.test.tstests/specifiers.test.tstests/ui-formulation.spec.tstests/ui-specifiers.spec.tstests/universal-search.test.ts
💤 Files with no reviewable changes (1)
- src/app/specifiers/[[...path]]/page.tsx
|
@copilot resolve the merge conflicts on this branch. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Cannot run autofix: This PR has merge conflicts. Please resolve the conflicts with the base branch and try again. Alternatively, use |
Head branch was pushed to by a user without write access
Resolved the branch merge conflicts and pushed the fix in commit |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 8 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 8 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Head branch was pushed to by a user without write access
…ration-current' into codex/formulation-specifier-separation-current # Conflicts: # src/app/specifiers/[slug]/page.tsx # src/app/specifiers/compare/page.tsx # src/components/clinical-dashboard/universal-search-command-surface.tsx # src/components/specifiers/specifier-builder-page.tsx # src/components/specifiers/specifiers-home-page.tsx # src/lib/search-command-surface.ts
…ration-current' into codex/formulation-specifier-separation-current # Conflicts: # src/components/specifiers/specifier-builder-page.tsx # src/components/specifiers/specifier-map-page.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37ae49f158
ℹ️ 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".
|
@copilot resolve the merge conflicts in this pull request |
Head branch was pushed to by a user without write access
…or conflicted files)
…cifier-separation-current # Conflicts: # src/app/specifiers/[slug]/page.tsx # src/app/specifiers/compare/page.tsx # src/app/specifiers/map/page.tsx # src/components/clinical-dashboard/universal-search-command-surface.tsx # src/components/specifiers/specifier-builder-page.tsx # src/components/specifiers/specifier-map-page.tsx # tests/ui-smoke.spec.ts # tests/ui-specifiers.spec.ts
…ration-current' into codex/formulation-specifier-separation-current # Conflicts: # src/app/specifiers/[slug]/page.tsx # src/app/specifiers/compare/page.tsx # src/app/specifiers/map/page.tsx # src/components/clinical-dashboard/universal-search-command-surface.tsx # src/components/specifiers/specifier-builder-page.tsx # src/components/specifiers/specifier-map-page.tsx # tests/ui-specifiers.spec.ts
|
Closing as superseded by #674 ("Complete Specifiers/Formulation separation (review of #672)"), which is already merged into Everything this PR set out to add is already present on
Because #674's canonical (Fails the With no net contribution over Generated by Claude Code |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
…cifier-separation-current
Summary
Verification
No live Supabase or OpenAI checks were run.
Summary by CodeRabbit
/specifiers/[slug]), builder, compare, and map flows.