fix(differentials): preserve compare selections across presentations - #1756
Conversation
Stop silently dropping diagnoses that do not share a presentation host. Cross-presentation ticks now open an ad-hoc compare view, search selection syncs into the URL for ModeNav handoff, and MobileTabs keep ids/q. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.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:1 minute 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 (19)
📝 WalkthroughWalkthroughThe PR adds ChangesDifferential workflow selection
Compare route and rendering
Navigation and route coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant DifferentialCompareRoute
participant differentials
participant DifferentialPresentationWorkflowPage
Browser->>DifferentialCompareRoute: Request /differentials/compare with query and ids
DifferentialCompareRoute->>differentials: Resolve diagnosis IDs
differentials-->>DifferentialCompareRoute: Return catalog or ad-hoc workflow
DifferentialCompareRoute->>DifferentialPresentationWorkflowPage: Render selected workflow
DifferentialPresentationWorkflowPage-->>Browser: Display comparison workflow
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Therapy-compass action entries have no href; guard with an in-check so typecheck accepts the builder-target scrape. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Register the new /differentials/compare page in the adoption contract and regenerate the manifest so design-system adoption checks stay green. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Bump the production route coverage pin to 48 and refresh ADOPTION.md generated sections after adding differentials compare. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:d206f66fe4
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
tests/differentials-navigation.test.ts (1)
66-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
idsremoval branch.The new tests cover writing
idsand parsing them. They do not coverdifferentialSelectionIdsSearchwith an empty selection, which takes theparams.delete("ids")branch. That branch controls what happens when the user unticks the last result.Add a case that asserts an empty selection removes
idsand preserves the other parameters.💚 Proposed test
).toBe("?q=Pain&run=1&ids=medical-gi-endocrine-painful-organic-cause%2Cbpsd-as-unmet-need-delirium-pain-mimic"); }); ++ it("removes ids when the compare selection empties", () => {+ expect(differentialSelectionIdsSearch([], "?q=Pain&run=1&ids=anorexia-nervosa")).toBe("?q=Pain&run=1");+ expect(differentialSelectionIdsSearch([], "?ids=anorexia-nervosa")).toBe("");+ });🤖 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/differentials-navigation.test.ts` around lines 66 - 74, Add a test case for differentialSelectionIdsSearch that passes an empty selection with existing query parameters including ids, and assert the result removes ids while preserving the other parameters. Place it alongside the existing differentialIdsFromSearchParams and differentialSelectionIdsSearch coverage.src/app/(search-app)/differentials/presentations/route.ts (1)
15-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFold the reserved-ID guard into the pathname selection.
The guard at Lines 24-27 duplicates the suffix computation at Lines 29-32. Merge the reserved-ID condition into the
pathnameternary. The behavior stays the same and there is one return path.♻️ Proposed simplification
- // Cross-presentation selections keep every valid ID on the ad-hoc compare- // route instead of silently dropping diagnoses that lose the presentation vote.- const pathname =- selection?.kind === "ad-hoc"- ? "/differentials/compare"- : `/differentials/presentations/${selection?.workflow.id ?? "acute-confusion-encephalopathy"}`;-- // Guard against ever advertising the reserved ad-hoc id as a presentation slug.- if (selection?.kind === "presentation" && selection.workflow.id === AD_HOC_DIFFERENTIAL_COMPARE_ID) {- const suffix = params.toString();- return suffix ? `/differentials/compare?${suffix}` : "/differentials/compare";- }-+ // Cross-presentation selections keep every valid ID on the ad-hoc compare+ // route instead of silently dropping diagnoses that lose the presentation vote.+ // The reserved ad-hoc id is never advertised as a presentation slug.+ const usesCompareRoute =+ selection?.kind === "ad-hoc" || selection?.workflow.id === AD_HOC_DIFFERENTIAL_COMPARE_ID;+ const pathname = usesCompareRoute+ ? "/differentials/compare"+ : `/differentials/presentations/${selection?.workflow.id ?? "acute-confusion-encephalopathy"}`;+🤖 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/app/`(search-app)/differentials/presentations/route.ts around lines 15 - 28, Fold the reserved-ID check for AD_HOC_DIFFERENTIAL_COMPARE_ID into the existing pathname selection ternary, alongside the ad-hoc branch, and remove the separate guard return. Preserve the current compare-route behavior and suffix query-string handling while leaving a single return path.src/lib/differentials.ts (1)
142-192: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider capping the ad-hoc candidate count.
buildAdHocPresentationWorkflowaccepts any number of valid IDs. Theidsquery parameter is user-controlled, so a request can select every catalogue slug. The compare page then renders one column per candidate, andsafetySnapshot.summaryconcatenates every record summary into a single paragraph. The result is a very wide table and an unreadable safety block.Apply a maximum candidate count and truncate the summary the same way
safetyTagsis capped at 6.♻️ Proposed cap
+const AD_HOC_MAX_CANDIDATES = 8;+ export function buildAdHocPresentationWorkflow(ids: Iterable<string>): DifferentialPresentationWorkflow | null { - const diagnosisIds = normalizeRequestedDiagnosisIds(ids);+ const diagnosisIds = normalizeRequestedDiagnosisIds(ids).slice(0, AD_HOC_MAX_CANDIDATES); if (!diagnosisIds.length) return null;🤖 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/lib/differentials.ts` around lines 142 - 192, Update buildAdHocPresentationWorkflow to cap the normalized valid records/candidates at a defined maximum before building the comparison and safety snapshot. Ensure selectedCount, totalCount, comparison columns, and concatenated safetySnapshot.summary all use the capped records, and truncate the summary consistently with the six-item safetyTags limit.
🤖 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/`(search-app)/differentials/compare/page.tsx:
- Around line 30-40: Update the compare page flow around
getPresentationWorkflowSelectionForDiagnosisIds so it renders an explicit empty
state with a link to /differentials when selection is null, including when IDs
are missing or all unknown. Only render DifferentialPresentationWorkflowPage
with the resolved workflow and diagnosis IDs when a valid selection exists, and
remove the acuteConfusionPresentationWorkflow fallback for this case.
In `@src/components/clinical-dashboard/differentials-home.tsx`:
- Around line 871-877: Update the selection state flow around comparisonIdsKey
and the useEffect that calls syncDifferentialSelectionIdsToUrl so URL
synchronization occurs only after a user toggle. Track a user-modified flag when
the selection toggle handler changes the selection, and gate the effect on that
flag so the mount-time diagnosisIds.slice(0, 2) auto-seed is never written to
the URL.
- Around line 835-844: Move the URL-based selection initialization out of the
render path in the component containing this diagnosis selection logic. Preserve
the first-render selection consistently between server and client, then apply
valid differentialIdsFromSearchParams values in a client-side effect (or
equivalent server-provided initialization) without changing later query
re-seeding behavior.
In `@src/lib/differentials-navigation.ts`:
- Around line 26-38: Update differentialIdsFromSearchParams to lowercase each
trimmed ID before deduplication and insertion into the returned list, matching
normalizeRequestedDiagnosisIds and the exact-match diagnosisIdSet consumer while
preserving empty-value filtering and order.
---
Nitpick comments:
In `@src/app/`(search-app)/differentials/presentations/route.ts:
- Around line 15-28: Fold the reserved-ID check for
AD_HOC_DIFFERENTIAL_COMPARE_ID into the existing pathname selection ternary,
alongside the ad-hoc branch, and remove the separate guard return. Preserve the
current compare-route behavior and suffix query-string handling while leaving a
single return path.
In `@src/lib/differentials.ts`:
- Around line 142-192: Update buildAdHocPresentationWorkflow to cap the
normalized valid records/candidates at a defined maximum before building the
comparison and safety snapshot. Ensure selectedCount, totalCount, comparison
columns, and concatenated safetySnapshot.summary all use the capped records, and
truncate the summary consistently with the six-item safetyTags limit.
In `@tests/differentials-navigation.test.ts`:
- Around line 66-74: Add a test case for differentialSelectionIdsSearch that
passes an empty selection with existing query parameters including ids, and
assert the result removes ids while preserving the other parameters. Place it
alongside the existing differentialIdsFromSearchParams and
differentialSelectionIdsSearch coverage.
🪄 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: 6e6fdd03-69da-4080-9521-8de412180e2d
📒 Files selected for processing (14)
docs/codebase-index.mddocs/site-map.mdscripts/generate-site-map.tssrc/app/(search-app)/differentials/compare/page.tsxsrc/app/(search-app)/differentials/presentations/route.tssrc/components/clinical-dashboard/differentials-home.tsxsrc/components/differentials/differential-presentation-workflow-page.tsxsrc/lib/differentials-navigation.tssrc/lib/differentials.tssrc/lib/mode-secondary-navigation.tstests/differentials-navigation.test.tstests/differentials.test.tstests/mode-secondary-navigation.test.tstests/route-reachability.test.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Resolve generated docs/adoption/sitemap conflicts by regenerating from source after combining main mode-home routing with the compare route.
Defer URL sync while search is loading, hydrate from mount-captured ids, lowercase id parsing to match the server, and drop the unsupported what-argues-against ad-hoc criterion that was all placeholders.
CI triageCI failed on this PR. Automated classification of the 6 failed job(s):
Compared with main CI run #9371 (cancelled). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
The compare page expands discovered route coverage to 50; keep the adoption contract assertion aligned so Unit coverage can pass. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Resolve adoption-manifest conflict by regenerating with design-system:adoption:update (50 discovered routes including compare). Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
… compare/presentations route conflicts)
App Router rejects page.tsx and route.ts on the same path, which broke Build/static-pr/coverage/ui-critical after the main merge. Keep compare as a page that redirects same-presentation selections and renders ad-hoc cross-presentation comparisons; regenerate site-map and adoption docs.
Summary
Pain)./differentials/comparewith an ad-hoc workflow built from diagnosis sections; same-presentation selections still use the hosting catalogue workflow.idsparam so ModeNav Compare carries the same selection, and MobileTabs Compare keepsq/ids.Verification
npm run test:focused -- --files src/lib/differentials.ts,src/lib/differentials-navigation.ts,src/lib/mode-secondary-navigation.ts,src/app/(search-app)/differentials/presentations/route.ts,src/app/(search-app)/differentials/compare/page.tsx,src/components/clinical-dashboard/differentials-home.tsx,src/components/differentials/differential-presentation-workflow-page.tsx— 255 passednpm run verify:pr-local— Test Files 529 passed (529); Tests 5709 passed | 4 skipped (5713); build listsƒ /differentials/compare; Offline RAG fixture and manifest validation passed (36 golden cases, 23 suites)idssync. Chromium journey can follow if needed.npm run verify:release— not required for this handoffRisk and rollout
/differentials/presentations?ids=redirect remains available.Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
/differentials/compare(presentation deep links still mark Compare current).modeSecondaryNavigationRegistryhrefs as inbound nav builders.Summary by CodeRabbit