Add patient information factsheet vertical slice (home, search, detail)#751
Add patient information factsheet vertical slice (home, search, detail)#751BigSimmo wants to merge 5 commits into
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughAdds a Clinical KB patient information library with sample factsheet data, browse and search routes, dynamic detail pages, shared navigation, metadata, sharing and printing actions, sitemap entries, and data tests. ChangesClinical factsheets
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Visitor
participant FactsheetsHomePage
participant FactsheetsSearchPage
participant factsheets
Visitor->>FactsheetsHomePage: Submit search query
FactsheetsHomePage->>FactsheetsSearchPage: Navigate with q parameter
FactsheetsSearchPage->>factsheets: Filter factsheets
factsheets-->>FactsheetsSearchPage: Matching records
FactsheetsSearchPage-->>Visitor: Render results or empty state
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
CI triageCI failed on this PR. Automated classification of the 3 failed job(s):
Compared with main CI run #2920 (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: 2
🤖 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/factsheets/page.tsx`:
- Around line 5-8: Update the metadata description in the factsheets page’s
Metadata declaration to describe the content as sample layouts only, removing
the claim that it contains approved local resources and clarifying that it is
not clinical guidance.
In `@src/components/factsheets/factsheets-search-page.tsx`:
- Around line 27-33: Update the loading effect keyed by submittedQuery so
clearing the query explicitly calls setLoading(false) before returning. Preserve
the existing timer-based reset and cleanup behavior for non-empty queries,
ensuring the skeleton cannot remain visible after “Clear search”.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: 5f1b1fc4-1a90-49a5-a2d6-742bf0e69bb5
📒 Files selected for processing (11)
docs/site-map.mdsrc/app/factsheets/[slug]/page.tsxsrc/app/factsheets/layout.tsxsrc/app/factsheets/page.tsxsrc/app/factsheets/search/page.tsxsrc/components/factsheets/factsheet-detail-page.tsxsrc/components/factsheets/factsheet-shell.tsxsrc/components/factsheets/factsheets-data.tssrc/components/factsheets/factsheets-home-page.tsxsrc/components/factsheets/factsheets-search-page.tsxtests/factsheets-data.test.ts
| export const metadata: Metadata = { | ||
| title: "Patient Information Sheets | Clinical KB", | ||
| description: "Browse patient information sheet layouts and approved local resources.", | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not describe sample content as approved resources.
Line 7 claims the route contains “approved local resources,” while the rendered page explicitly says it contains sample layouts only. Metadata can appear outside the app, so make this description clearly sample-only and not clinical guidance.
🤖 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/factsheets/page.tsx` around lines 5 - 8, Update the metadata
description in the factsheets page’s Metadata declaration to describe the
content as sample layouts only, removing the claim that it contains approved
local resources and clarifying that it is not clinical guidance.
| useEffect(() => setInput(submittedQuery), [submittedQuery]); | ||
| useEffect(() => { | ||
| if (!submittedQuery) return; | ||
| setLoading(true); | ||
| const timeout = window.setTimeout(() => setLoading(false), 180); | ||
| return () => window.clearTimeout(timeout); | ||
| }, [submittedQuery]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' src/components/factsheets/factsheets-search-page.tsxRepository: BigSimmo/Database
Length of output: 6669
🏁 Script executed:
ast-grep outline src/components/factsheets/factsheets-search-page.tsx --view expandedRepository: BigSimmo/Database
Length of output: 253
🏁 Script executed:
rg -n "loading|submittedQuery|setLoading|useEffect" src/components/factsheets/factsheets-search-page.tsxRepository: BigSimmo/Database
Length of output: 1011
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('src/components/factsheets/factsheets-search-page.tsx')
print(p.read_text())
PYRepository: BigSimmo/Database
Length of output: 6670
🏁 Script executed:
sed -n '1,220p' src/components/factsheets/factsheets-search-page.tsx | nl -baRepository: BigSimmo/Database
Length of output: 195
Clear search should reset loading.
When submittedQuery becomes empty, the cleanup clears the pending timer and this effect returns without calling setLoading(false), so the skeleton can stay visible after clicking “Clear search”. Reset loading on the empty-query path or derive the busy state from navigation instead.
🧰 Tools
🪛 ESLint
[error] 27-27: Error: Calling setState synchronously within an effect can trigger cascading renders
Effects are intended to synchronize state between React and external systems such as manually updating the DOM, state management libraries, or other platform APIs. In general, the body of an effect should do one or both of the following:
- Update external systems with the latest state from React.
- Subscribe for updates from some external system, calling setState in a callback function when external state changes.
Calling setState synchronously within an effect body causes cascading renders that can hurt performance, and is not recommended. (https://react.dev/learn/you-might-not-need-an-effect).
/home/jailuser/git/src/components/factsheets/factsheets-search-page.tsx:27:19
25 | const [loading, setLoading] = useState(false);
26 |
27 | useEffect(() => setInput(submittedQuery), [submittedQuery]);
| ^^^^^^^^ Avoid calling setState() directly within an effect
28 | useEffect(() => {
29 | if (!submittedQuery) return;
30 | setLoading(true);
(react-hooks/set-state-in-effect)
[error] 30-30: Error: Calling setState synchronously within an effect can trigger cascading renders
Effects are intended to synchronize state between React and external systems such as manually updating the DOM, state management libraries, or other platform APIs. In general, the body of an effect should do one or both of the following:
- Update external systems with the latest state from React.
- Subscribe for updates from some external system, calling setState in a callback function when external state changes.
Calling setState synchronously within an effect body causes cascading renders that can hurt performance, and is not recommended. (https://react.dev/learn/you-might-not-need-an-effect).
/home/jailuser/git/src/components/factsheets/factsheets-search-page.tsx:30:5
28 | useEffect(() => {
29 | if (!submittedQuery) return;
30 | setLoading(true);
| ^^^^^^^^^^ Avoid calling setState() directly within an effect
31 | const timeout = window.setTimeout(() => setLoading(false), 180);
32 | return () => window.clearTimeout(timeout);
33 | }, [submittedQuery]);
(react-hooks/set-state-in-effect)
🤖 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/factsheets/factsheets-search-page.tsx` around lines 27 - 33,
Update the loading effect keyed by submittedQuery so clearing the query
explicitly calls setLoading(false) before returning. Preserve the existing
timer-based reset and cleanup behavior for non-empty queries, ensuring the
skeleton cannot remain visible after “Clear search”.
Source: Linters/SAST tools
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
…erate docs/site-map.md
Head branch was pushed to by a user without write access
- factsheets-search-page: replace synchronous setState-in-effect with the 'adjusting state during rendering' pattern for input sync; remove the artificial loading state (search is synchronous over in-memory data). Fixes two @eslint-react/hooks-extra lint errors. - generate-site-map: register /factsheets, /factsheets/[slug], and /factsheets/search descriptions in routeDescriptions so the generator emits proper descriptions instead of 'Route discovered from app directory'. - docs/site-map.md: correct entry order so factsheets/* sorts before favourites (alphabetical localeCompare). Fixes tests/site-map.test.ts.
…tient-information-pages
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/generate-site-map.ts (1)
169-186: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover template-literal redirects in the sitemap extractor.
src/app/auth/callback/route.tsusesNextResponse.redirect(\${origin}...`)`, which this matcher still misses, so it lands in the app-route-handler list instead of redirects. Add a regression case for this redirect shape.🤖 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 `@scripts/generate-site-map.ts` around lines 169 - 186, Update extractRedirectTarget to recognize NextResponse.redirect calls whose destination is a template literal beginning with a dynamic origin or URL prefix, including the shape used by src/app/auth/callback/route.ts, and return an appropriate stable redirect target. Add a regression case covering this template-literal redirect so it is classified as a redirect rather than an app-route handler.
🤖 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.
Outside diff comments:
In `@scripts/generate-site-map.ts`:
- Around line 169-186: Update extractRedirectTarget to recognize
NextResponse.redirect calls whose destination is a template literal beginning
with a dynamic origin or URL prefix, including the shape used by
src/app/auth/callback/route.ts, and return an appropriate stable redirect
target. Add a regression case covering this template-literal redirect so it is
classified as a redirect rather than an app-route handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0dfb9bd4-c87b-4a54-8dd6-08c83ecc38de
📒 Files selected for processing (2)
docs/site-map.mdscripts/generate-site-map.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/site-map.md
Motivation
Description
src/components/factsheets/factsheets-data.tsand a small lookup helperfindFactsheetthat the routes consume.src/app/factsheets/page.tsx(Home),src/app/factsheets/search/page.tsx(Search), andsrc/app/factsheets/[slug]/page.tsx(Detail), with a layoutsrc/app/factsheets/layout.tsxthat reuses aFactsheetShell(src/components/factsheets/factsheet-shell.tsx).FactsheetsHomePage(src/components/factsheets/factsheets-home-page.tsx),FactsheetsSearchPage(src/components/factsheets/factsheets-search-page.tsx), andFactsheetDetailPage(src/components/factsheets/factsheet-detail-page.tsx) using existing primitives (ui-primitives) and theSheetoverlay for accessible share interactions.tests/factsheets-data.test.tsand updatedocs/site-map.mdto list the new routes; all sample records are explicitly labelledSample contentand the UI contains prominent copy stating these are not clinical guidance.Testing
git diff --checkto validate whitespace/merge markers and found no issues. (success)npx --yes prettier@3.6.2 --check ...against the added factsheet files, which passed for the new files though unrelated docs in the repo had existing format warnings. (new files OK)npm run workflow:lifecycle -- --phase start,npm run workflow:flightplan -- --write-evidence, andnpm run workflow:design-sweepto generate the planned verification steps and confirm the change is covered by the repo's UI/design-sweep plan. (planning/sweep output produced)npm run verify:pr-local -- --dry-run --files ...which printed the verification plan for this change. (dry-run succeeded)npm run testfailed because the project dependencies (e.g.vitest) are not installed here, andnpm run ensure/npm run verify:uicould not start the app because the local Node runtime is20.20.2while the repo requires Node24.x. (could not run runtime/browser tests)tests/factsheets-data.test.ts; runningnpm run test:focused -- --files tests/factsheets-data.test.tswas refused by the repo scripts because test/fixture paths changed and the full suite is required. (test not executed here)If requested, the next steps are to: run
npm run ensureand theverify:uimatrix on a Node 24 environment to visually validate mobile/desktop/dark-mode/keyboard/reduced-motion/forced-colors states and to run the full test suite (npm run test) to confirm all automated checks pass.Codex Task
Summary by CodeRabbit
/factsheets,/factsheets/search, and/factsheets/[slug].