fix: resolve design and UX review findings#471
Conversation
…w-integration # Conflicts: # docs/branch-review-ledger.md
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
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:
📝 WalkthroughWalkthroughThe changes add validated URL persistence for clinical query context, propagate that context into document and differentials search requests, update responsive design tokens and component styling, and extend verification and regression tests. ChangesSearch Context and UI Updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant GlobalMockupSearchShell
participant ClinicalDashboard
participant MasterDocumentSearch
participant DocumentSearchAPI
User->>GlobalMockupSearchShell: Select mode, query, or scope filters
GlobalMockupSearchShell->>ClinicalDashboard: Navigate with serialized context
ClinicalDashboard->>ClinicalDashboard: Synchronize URL state
MasterDocumentSearch->>MasterDocumentSearch: Build request body from URL context
MasterDocumentSearch->>DocumentSearchAPI: POST document search request
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: cd23d67d53
ℹ️ 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".
…w-integration # Conflicts: # docs/branch-review-ledger.md
|
Resolved the phone mode-home composer overlap in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?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: ea6c0d759e
ℹ️ 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".
|
Fixed the scoped document-search contract in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b170616156
ℹ️ 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".
|
Fixed the routed-context stale-result path in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f70e04593
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/search-navigation-context.test.ts (1)
29-47: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression case for the slice-before-filter ordering bug.
This test places the valid
sourceStatusesentry ("current") before the invalid one, and well within the 4-item limit, so it doesn't exercise the ordering bug flagged insrc/lib/search-navigation-context.ts(uniqueTrimmed slices before the allowed-value filter runs). Consider adding a case with more invalid entries than the limit, followed by a valid one, to lock in the fix.🤖 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/search-navigation-context.test.ts` around lines 29 - 47, Add a regression test for readSearchNavigationContext covering more than four invalid sourceStatuses values followed by a valid "current" value, and assert that the valid status is retained. Ensure the case specifically verifies filtering allowed values occurs before the uniqueTrimmed four-item 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/lib/search-navigation-context.ts`:
- Around line 98-119: Validation currently occurs after limit truncation,
allowing invalid values to consume the limit and hide valid entries. Update
uniqueTrimmed and the callers allowedValues, appendValues, and importBatchIds
handling so trimming, emptiness, length, allowed-value, and UUID checks happen
before applying slice(0, limit), while preserving deduplication and output
behavior.
---
Nitpick comments:
In `@tests/search-navigation-context.test.ts`:
- Around line 29-47: Add a regression test for readSearchNavigationContext
covering more than four invalid sourceStatuses values followed by a valid
"current" value, and assert that the valid status is retained. Ensure the case
specifically verifies filtering allowed values occurs before the uniqueTrimmed
four-item limit.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8fabed09-cc89-455b-b74f-b65c6a71a706
📒 Files selected for processing (24)
docs/branch-review-ledger.mdpackage.jsonscripts/check-type-scale.mjssrc/app/globals.csssrc/components/ClinicalDashboard.tsxsrc/components/clinical-dashboard/account-setup-dialog.tsxsrc/components/clinical-dashboard/clinical-output-helpers.tsxsrc/components/clinical-dashboard/evidence-panels.tsxsrc/components/clinical-dashboard/global-mockup-search-shell.tsxsrc/components/clinical-dashboard/master-search-header.tsxsrc/components/clinical-dashboard/medication-prescribing-workspace.tsxsrc/components/clinical-dashboard/medication-record-page.tsxsrc/components/clinical-dashboard/settings-dialog.tsxsrc/components/master-document-flow-mockups.tsxsrc/components/mode-home-template.tsxsrc/components/source-overlay-redesign-mockups.tsxsrc/components/ui-primitives.tsxsrc/lib/app-modes.tssrc/lib/document-flow-routes.tssrc/lib/search-navigation-context.tstests/app-modes.test.tstests/clinical-dashboard-merge-artifacts.test.tstests/document-flow-routes.test.tstests/search-navigation-context.test.ts
…w-integration # Conflicts: # docs/branch-review-ledger.md
|
Resolved the remaining routed-context review items in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64a8fb9f7b
ℹ️ 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".
|
Fixed standalone differentials scope propagation in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d34f8c628
ℹ️ 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".
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/components/differentials/differentials-home-page.tsx`:
- Around line 31-57: Prevent stale requests in runSearch from updating state
after a newer routed search starts. Track the latest request with an
incrementing identifier or AbortController, and before committing
documentMatches or loading state, verify the response belongs to the current
request; update the effect/callback cleanup as needed so older searches cannot
overwrite newer URL-context results.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fe72bffd-29a4-4770-8863-7babcdddb078
📒 Files selected for processing (8)
docs/branch-review-ledger.mdsrc/components/ClinicalDashboard.tsxsrc/components/clinical-dashboard/master-search-header.tsxsrc/components/differentials/differentials-home-page.tsxsrc/lib/differentials-search-request.tssrc/lib/search-navigation-context.tstests/differentials-search-request.test.tstests/search-navigation-context.test.ts
✅ Files skipped from review due to trivial changes (2)
- docs/branch-review-ledger.md
- src/components/clinical-dashboard/master-search-header.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/search-navigation-context.ts
- tests/search-navigation-context.test.ts
- src/components/ClinicalDashboard.tsx
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 `@tests/ui-smoke.spec.ts`:
- Around line 1492-1506: Update the answer request tracking in the test around
mockDemoApi and onAnswerRequest to capture each request body, not only the
query. Preserve the existing query assertion, then add an assertion for the
second request verifying its filters.sourceStatuses value is exactly
["outdated"] after the scoped URL rerun.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e6b576ad-3550-4391-b435-7776e190b462
📒 Files selected for processing (2)
src/components/ClinicalDashboard.tsxtests/ui-smoke.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ClinicalDashboard.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/ui-smoke.spec.ts (1)
1882-1918: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the second
/api/searchrequest actually carries the routedsourceStatusesscopeThe mock keys its response purely on
requestCountordinal (Line 1890), never inspecting the actual request body/params. The test proves stale-response ordering is handled correctly, but it can't catch a regression where the differentials search request fails to forwardscope.sourceStatuses=outdatedinto the/api/searchpayload — two requests would still fire and render distinct mocked source counts regardless of what was actually requested. A reviewer already flagged the identical gap on a related test elsewhere in this file (scoped answer rerun not assertingfilters.sourceStatusesin the request body).🧪 Suggested addition to capture and assert the request body
let requestCount = 0; + const requestBodies: unknown[] = []; await page.route(/\/api\/search$/, async (route) => { requestCount += 1; const currentRequest = requestCount; + requestBodies.push(route.request().postDataJSON()); if (currentRequest === 1) await new Promise((resolve) => setTimeout(resolve, 500));await expect.poll(() => requestCount).toBe(2); + expect(requestBodies[1]).toMatchObject({ filters: { sourceStatuses: ["outdated"] } }); const sourceStatus = page.getByRole("heading", { name: "Source status" }).locator("..");🤖 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-smoke.spec.ts` around lines 1882 - 1918, Update the test using the `/api/search` route handler and test body assertions so the second request is validated for the routed `sourceStatuses` scope. Capture or parse each request’s payload, assert the second request includes `scope.sourceStatuses=outdated` (matching the API’s actual payload shape), and retain the existing stale-response ordering assertions; do not rely solely on requestCount to select mock responses.
🤖 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.
Nitpick comments:
In `@tests/ui-smoke.spec.ts`:
- Around line 1882-1918: Update the test using the `/api/search` route handler
and test body assertions so the second request is validated for the routed
`sourceStatuses` scope. Capture or parse each request’s payload, assert the
second request includes `scope.sourceStatuses=outdated` (matching the API’s
actual payload shape), and retain the existing stale-response ordering
assertions; do not rely solely on requestCount to select mock responses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ea653f2b-159f-459f-bfbb-1f589c460806
📒 Files selected for processing (2)
src/components/differentials/differentials-home-page.tsxtests/ui-smoke.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/differentials/differentials-home-page.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc7581a680
ℹ️ 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".
| @@ -67,7 +77,15 @@ export function DifferentialsHomePage({ query = "", autoRunSearch = false }: Dif | |||
| documentMatches={documentMatches} | |||
| desktopComposerSlotId={modeHomeDesktopComposerSlotId} | |||
| onRunSearch={(nextQuery) => { | |||
There was a problem hiding this comment.
When a standalone /differentials page has routed queryMode or scope.* filters, this only wraps the explicit onRunSearch path; DifferentialsHome also uses handleSuggestedSearch for the Recent work/library-match pills, and without an onSuggestedSearch prop it falls back to router.push(appModeHomeHref("differentials", { query, run: true, focus: true })), dropping the active filters before the next auto-run. A focused check would open /differentials?scope.sourceStatuses=current, click a suggested pill, and assert the resulting URL/API body still includes that scope.
Useful? React with 👍 / 👎.
1 participant
Summary
Why
The open Codex design-system and UX review tasks confirmed five issue groups: an undefined shadow token, lost search context during navigation, undersized interactive targets, excess composer whitespace, and arbitrary type-scale drift.
Areas touched
Design tokens and global CSS, dashboard/search navigation, mode/document/medication routes, shared UI primitives, dialogs and headers, type-scale guard/tests, and the branch review ledger.
Verification
npm run check:type-scale— passed.verify:cheapstages passed through runtime, action pins, sitemap, type-scale, and lint.git diff --check— passed.Checks not run
Local typecheck stopped because this older worktree's dependency install predates the
@next/bundle-analyzerpackage now imported by current main. No live provider/API check was run. Hosted CI's clean install is the authoritative typecheck/build and UI verification.Risk and governance
This changes navigation context and responsive UI behavior but does not alter database schema, clinical generation, private document authorization, credentials, or production data. Query context is encoded locally in existing route parameters, and regression tests cover the new navigation helpers.