Fix(differentials): route "Compare selected" directly to resolved presentation workflow#882
Fix(differentials): route "Compare selected" directly to resolved presentation workflow#882BigSimmo wants to merge 3 commits into
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughCentralized differentials URL construction in shared helpers, including query and selected-ID handling, then updated dashboard navigation links and fallback actions to use those helpers. Added tests covering route encoding and selected comparison URLs. ChangesDifferentials navigation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1c5c1eca8a
ℹ️ 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".
| @@ -0,0 +1,20 @@ | |||
| import { getPresentationWorkflowSelectionForDiagnosisIds } from "@/lib/differentials"; | |||
There was a problem hiding this comment.
When DifferentialsHome value-imports this helper from the client component, this import follows @/lib/differentials → differential-fixtures → data/differentials-snapshot.json, so simply opening the differentials dashboard can pull the full ~1.2 MB generated catalogue into the browser bundle. That regresses the repo's fixture-free client boundary for the dashboard; the smallest proof is a production build plus npm run check:bundle-budget, or extending tests/client-performance-boundaries.test.ts to catch this transitive import. Keep this lookup server-side or move only the small diagnosis-to-workflow mapping needed for hrefs into a client-safe module.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Replaced the compare href helper’s transitive
@/lib/differentialsimport with a client-safe workflow lookup module, so dashboard navigation no longer pulls the generated differential catalogue into the client graph. src/lib/differentials-navigation.tsL1-L4 - Added a compact diagnosis-to-presentation workflow mapping plus selection helper for client-side compare URL construction, keeping it independent of
differential-fixturesand the snapshot JSON. src/lib/differentials-navigation-workflows.tsL1-L60 - Extended the fixture-free client boundary test to assert that differential navigation does not import
@/lib/differentials,differential-fixtures,differentials-snapshot, orloadDifferentialSnapshot. tests/client-performance-boundaries.test.tsL18-L25 - Committed the scoped fix on the current branch:
dd4c1f6 fix(differentials): keep compare navigation client-safe. - Created PR metadata with title:
Fix(differentials): keep compare navigation client-safe.
Testing
⚠️ npm run test -- tests/differentials-navigation.test.ts tests/client-performance-boundaries.test.ts— initially could not run because dependencies were not installed andvitestwas missing fromnode_modules.⚠️ npm ci— initially failed under Nodev20.20.2because the repo requires Node24.x.- ✅
export PATH="$HOME/.nvm/versions/node/v24.15.0/bin:$PATH"; npm ci - ✅
export PATH="$HOME/.nvm/versions/node/v24.15.0/bin:$PATH"; npm run test -- tests/differentials-navigation.test.ts tests/client-performance-boundaries.test.ts - ✅
export PATH="$HOME/.nvm/versions/node/v24.15.0/bin:$PATH"; npm run typecheck - ✅
export PATH="$HOME/.nvm/versions/node/v24.15.0/bin:$PATH"; npm run lint - ✅
git diff --check
Worktree
- Current branch:
work. - Worktree is clean after commit.
|
@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: 1c5c1eca8a
ℹ️ 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".
| @@ -0,0 +1,20 @@ | |||
| import { getPresentationWorkflowSelectionForDiagnosisIds } from "@/lib/differentials"; | |||
There was a problem hiding this comment.
When the /differentials client component imports differentials-navigation, this value import pulls in @/lib/differentials, which imports differential-fixtures and evaluates differentialRecords = catalog().diagnoses; that loads data/differentials-snapshot.json into the client graph (~1.2 MB raw) just to build a compare href. This bypasses the existing fixture-free client boundary because the dashboard no longer imports @/lib/differentials directly but still reaches it through this helper, so the differentials home page pays a large bundle/parse cost for every user. Split out a tiny client-safe diagnosis→workflow map or keep workflow resolution on the server/redirect path instead of importing the full catalog here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Replaced the client navigation helper’s transitive
@/lib/differentialsvalue import with a client-safe static workflow candidate map, preserving direct presentation href resolution without pulling the generated catalogue into the client graph. src/lib/differentials-navigation.tsL1-L45 - Added
src/lib/differential-presentation-workflow-map.tsas the small browser-safe diagnosis-to-presentation index used by compare navigation. src/lib/differential-presentation-workflow-map.tsL1-L10 - Extended the fixture-free client performance boundary test to assert both the navigation helper and workflow map avoid
@/lib/differentials, fixtures, and snapshot imports. tests/client-performance-boundaries.test.tsL18-L32 - Kept the focused navigation test proving selected compare hrefs still route directly to presentation pages and preserve query/IDs without
0.0.0.0. tests/differentials-navigation.test.tsL12-L21 - Committed the fix on the current branch:
500028a Fix client-safe differentials navigation. - Created PR metadata with
make_prtitled:Fix client-safe differentials compare navigation.
Testing
- ❌
npm run test -- tests/differentials-navigation.test.ts tests/client-performance-boundaries.test.ts— failed before dependencies were installed: missingnode_modules/vitest/vitest.mjs. - ❌
npm ci— failed under Nodev20.20.2because this repo requires Node24.x/ npm11.x. - ✅
source ~/.nvm/nvm.sh && nvm use 24.15.0 >/dev/null && npm ci - ✅
source ~/.nvm/nvm.sh && nvm use 24.15.0 >/dev/null && npm run test -- tests/differentials-navigation.test.ts tests/client-performance-boundaries.test.ts && npm run typecheck
Status
- Current branch:
work. - Worktree is clean after commit.
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #3374 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/lib/differentials-navigation.ts`:
- Around line 13-18: Update differentialSelectedCompareHref to normalize
selectedIds once before calling getPresentationWorkflowSelectionForDiagnosisIds,
then pass the normalized IDs to differentialRouteWithQuery when workflow
resolution is undefined while preserving selection?.diagnosisIds for resolved
workflows. Add a regression test covering an unmapped or whitespace-padded ID
and verifying the fallback URL retains the selected diagnosis IDs.
🪄 Autofix (Beta)
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 Plus
Run ID: b15afa7f-2309-4799-9920-2621a2e6cc7d
📒 Files selected for processing (3)
src/components/clinical-dashboard/differentials-home.tsxsrc/lib/differentials-navigation.tstests/differentials-navigation.test.ts
| export function differentialSelectedCompareHref(query: string, selectedIds: Iterable<string>) { | ||
| const selection = getPresentationWorkflowSelectionForDiagnosisIds(selectedIds); | ||
| return differentialRouteWithQuery( | ||
| `/differentials/presentations/${selection?.workflow.id ?? "acute-confusion-encephalopathy"}`, | ||
| query, | ||
| selection?.diagnosisIds, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve selected IDs on the fallback route.
If workflow resolution returns undefined, this code routes to acute-confusion-encephalopathy but passes undefined for selectedIds, so the generated URL omits the user’s selected diagnoses. Normalize the iterable once before resolution and fall back to those IDs when no workflow selection exists. Add a regression test for an unmapped or whitespace-padded ID.
Proposed fix
export function differentialSelectedCompareHref(query: string, selectedIds: Iterable<string>) {
- const selection = getPresentationWorkflowSelectionForDiagnosisIds(selectedIds);
+ const normalizedIds = Array.from(selectedIds, (id) => id.trim()).filter(Boolean);
+ const selection = getPresentationWorkflowSelectionForDiagnosisIds(normalizedIds);
return differentialRouteWithQuery(
`/differentials/presentations/${selection?.workflow.id ?? "acute-confusion-encephalopathy"}`,
query,
- selection?.diagnosisIds,
+ selection?.diagnosisIds ?? normalizedIds,
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function differentialSelectedCompareHref(query: string, selectedIds: Iterable<string>) { | |
| const selection = getPresentationWorkflowSelectionForDiagnosisIds(selectedIds); | |
| return differentialRouteWithQuery( | |
| `/differentials/presentations/${selection?.workflow.id ?? "acute-confusion-encephalopathy"}`, | |
| query, | |
| selection?.diagnosisIds, | |
| export function differentialSelectedCompareHref(query: string, selectedIds: Iterable<string>) { | |
| const normalizedIds = Array.from(selectedIds, (id) => id.trim()).filter(Boolean); | |
| const selection = getPresentationWorkflowSelectionForDiagnosisIds(normalizedIds); | |
| return differentialRouteWithQuery( | |
| `/differentials/presentations/${selection?.workflow.id ?? "acute-confusion-encephalopathy"}`, | |
| query, | |
| selection?.diagnosisIds ?? normalizedIds, | |
| ); | |
| } |
🤖 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-navigation.ts` around lines 13 - 18, Update
differentialSelectedCompareHref to normalize selectedIds once before calling
getPresentationWorkflowSelectionForDiagnosisIds, then pass the normalized IDs to
differentialRouteWithQuery when workflow resolution is undefined while
preserving selection?.diagnosisIds for resolved workflows. Add a regression test
covering an unmapped or whitespace-padded ID and verifying the fallback URL
retains the selected diagnosis IDs.
|
Closing as superseded by #885 (Fix(differentials): route Compare selected to resolved presentation workflow). Preferring the newer Cursor PR with green CI. |
1 participant
Motivation
0.0.0.0) and produced a restricted viewport; the goal is to route selected comparisons directly to the resolved presentation workflow URL and preserve selected IDs.Description
src/lib/differentials-navigation.tswithdifferentialRouteWithQuery()anddifferentialSelectedCompareHref()to build same-origin relative route hrefs and resolve the presentation workflow for chosen diagnosis IDs.routeWithQueryusage insrc/components/clinical-dashboard/differentials-home.tsxwith the new helpers for mobile and desktop "Compare selected" CTAs and other catalogue navigation links.routeWithQueryhelper and wire CTAs to the presentation page directly so the app no longer relies on the redirect endpoint that could embed a bad origin.tests/differentials-navigation.test.tswhich asserts that the new hrefs target a presentation page, include the query and IDs, and do not contain0.0.0.0.Testing
npm cito populate node modules and it completed successfully.npm run test -- tests/differentials-navigation.test.tsand the new test suite passed (2 tests, 0 failures).npm run typecheckand it passed with no errors.npm run lintand it completed without errors.npm run ensureto verify the dev UI, but the local Next/Turbopack dev server did not become ready due to font-module resolution / Google font fetch failures in this environment, so end-to-end browser verification was not completed; no persistent dev server was left running.Codex Task
Summary by CodeRabbit
Bug Fixes
Tests