[ENG-831] feat: add Patient Medications and Diagnostic Reports pages with navigation links - #16612
[ENG-831] feat: add Patient Medications and Diagnostic Reports pages with navigation links#16612abhimanyurajeesh wants to merge 25 commits into
Conversation
|
github-actions
Bot
added
Type Changes
|
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:
WalkthroughChangesPatient portal and booking experience
Possibly related PRs
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 |
Deploying care-preview with
|
| Latest commit: |
90fd169
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://100439f2.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://eng-831.care-preview-a7w.pages.dev |
![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
🤖 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/pages/Patient/PatientMedications.tsx`:
- Around line 39-46: Update the Collapsible state and detail queries in
src/pages/Patient/PatientMedications.tsx#L39-L46 and
src/pages/Patient/PatientDiagnosticReports.tsx#L40-L47: track each card’s isOpen
state, pass it to the corresponding Collapsible, and set the
getPrescription/getDiagnosticReport query enabled condition to isOpen && !!token
so details are fetched only after expansion.
- Around line 30-36: Replace the inline props type in PrescriptionCard with a
named PrescriptionCardProps interface in
src/pages/Patient/PatientMedications.tsx:30-36, and use that interface in the
component signature. Apply the same change to DiagnosticReportCard by extracting
and using a DiagnosticReportCardProps interface in
src/pages/Patient/PatientDiagnosticReports.tsx:31-37.
- Around line 39-90: Handle query errors explicitly in the affected render
paths: in src/pages/Patient/PatientMedications.tsx lines 39-90, show a
user-friendly detail error instead of an empty MedicationsTable; in lines
108-144, show a list error instead of “no prescriptions.” In
src/pages/Patient/PatientDiagnosticReports.tsx lines 40-92, show a detail error
instead of “no observations”; in lines 110-148, show a list error instead of “no
reports.” Use each query’s error state alongside loading and data checks, while
preserving existing empty-state behavior for successful empty responses.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: deac220b-a84e-4284-a6f1-1f67e1529aac
📒 Files selected for processing (6)
public/locale/en.jsonsrc/Routers/PatientRouter.tsxsrc/components/ui/sidebar/patient-nav.tsxsrc/pages/Patient/PatientDiagnosticReports.tsxsrc/pages/Patient/PatientMedications.tsxsrc/types/emr/patientPortal/patientPortalApi.ts
There was a problem hiding this comment.
Pull request overview
Adds patient-portal pages for viewing medications (prescriptions) and diagnostic reports, wires them into the PatientRouter, and exposes navigation links in the patient sidebar.
Changes:
- Added OTP patient-portal API routes for listing/retrieving prescriptions and diagnostic reports.
- Introduced new patient pages to render prescriptions/medications and diagnostic reports with collapsible card layouts.
- Added patient sidebar navigation entries and an English i18n label for “Diagnostic Reports”.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/emr/patientPortal/patientPortalApi.ts | Adds OTP API route definitions for prescriptions and diagnostic reports. |
| src/Routers/PatientRouter.tsx | Registers new patient-portal routes for medications and diagnostic reports. |
| src/pages/Patient/PatientMedications.tsx | New patient-portal page rendering prescription list + per-prescription medication details. |
| src/pages/Patient/PatientDiagnosticReports.tsx | New patient-portal page rendering diagnostic report list + results display. |
| src/components/ui/sidebar/patient-nav.tsx | Adds sidebar links to the new patient-portal pages. |
| public/locale/en.json | Adds diagnostic_reports translation key. |
| const { data: detail, isLoading } = useQuery({ | ||
| queryKey: ["portal-prescription", prescription.id], | ||
| queryFn: query(patientPortalApi.getPrescription, { | ||
| pathParams: { id: prescription.id }, | ||
| headers: { Authorization: `Bearer ${token}` }, | ||
| }), | ||
| enabled: !!token, | ||
| }); |
| function DiagnosticReportCard({ | ||
| report, | ||
| token, | ||
| }: { | ||
| report: DiagnosticReportRead; | ||
| token?: string; | ||
| }) { | ||
| const { t } = useTranslation(); | ||
|
|
||
| const { data: detail, isLoading } = useQuery({ | ||
| queryKey: ["portal-diagnostic-report", report.id], | ||
| queryFn: query(patientPortalApi.getDiagnosticReport, { | ||
| pathParams: { id: report.id }, | ||
| headers: { Authorization: `Bearer ${token}` }, | ||
| }), | ||
| enabled: !!token, | ||
| }); |
🎭 Playwright Test ResultsStatus: ✅ Passed
📊 Detailed results are available in the playwright-final-report artifact. Run: #10602 |
Entire-Checkpoint: bc6a6d40b35a
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 40 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (5)
.env:12
- This PR changes the default
REACT_CARE_API_URLin.envtohttps://develop-api.ohc.network, which is unrelated to the patient portal feature and conflicts with the README’s documented fallback (https://careapi.ohc.network). This will change the default backend used in local/dev setups for anyone pulling the branch.
# Care API URL without the /api prefix
REACT_CARE_API_URL=https://develop-api.ohc.network
src/Routers/PublicRouter.tsx:67
- Comment typo: "deep-linking int" looks truncated/unfinished, which makes the intent unclear for future readers.
// A signed-out visitor deep-linking int
src/pages/PublicAppointments/auth/PatientLogin.tsx:64
- This introduces a new patient OTP login UX and routing entrypoint (
/patient/login) but there’s no Playwright coverage for the patient login flow (send OTP → verify OTP → profile selection) or for the new patient portal navigation. Given existing Playwright coverage for staff login (tests/auth/login.spec.ts), adding at least a basic patient sign-in/navigation spec would help prevent regressions.
src/pages/PublicAppointments/PatientRegistration.tsx:54 isGeoOrganizationCompleteduplicates the geo-organization depth/leaf validation logic that already exists insrc/components/Patient/PatientRegistration.tsx(isGeoOrganizationValid). Keeping two copies increases the risk of the public booking registration and staff registration drifting in behavior when the rules change.
src/components/Auth/Login.tsx:77- The
mode=patientquery param is still read/persisted, but the/loginscreen no longer renders patient login. Visiting/login?mode=patientcurrently shows the staff login without redirecting, which breaks existing deep links/bookmarks and also stores an unsupported preference in localStorage.
// Remember the last login mode
useEffect(() => {
localStorage.setItem(LocalStorageKeys.loginPreference, mode);
}, [mode]);
There was a problem hiding this comment.
Actionable comments posted: 30
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Providers/PatientUserProvider.tsx (1)
43-53: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not render a failed patient lookup as an empty profile list.
After a query error,
isLoadingbecomes false anduserData?.results ?? []becomes[];SelectProfilethen tells the user no profiles are linked and offers profile creation. Expose and render a patient-query error state separately from a legitimate empty result.🤖 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/Providers/PatientUserProvider.tsx` around lines 43 - 53, Expose the error state from the useQuery call in PatientUserProvider and pass it through the provider’s rendering flow separately from patients. Update the SelectProfile conditional so failed patient lookups render an error state rather than treating userData?.results ?? [] as a legitimate empty profile list or offering profile creation.
🤖 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 `@public/locale/en.json`:
- Around line 33-41: Move the newly introduced blood-group keys and later
patient-related keys in the English locale to the end of the locale object,
preserving their existing key names and translations. Keep all pre-existing
entries in their current order and ensure the new keys are appended rather than
inserted among existing translations.
In `@src/components/Patient/CancelAppointmentButton.tsx`:
- Around line 70-91: Replace the fixed format call on the `start` timestamp in
`CancelAppointmentButton` with locale-aware date/time formatting using the
active `i18n.language`, preserving medium date and short time styles before
passing `datetime` to the cancellation confirmation translation.
In `@src/components/Patient/PatientAppShell.tsx`:
- Around line 25-40: Update the TABS label handling and its usage in
PatientAppShell so translation calls use statically detectable literal keys
instead of passing tab.label to t(). Preserve each tab’s existing translation
key by using explicit t("home"), t("visits"), t("records"), and t("profile")
call sites or equivalent per-tab label accessors.
- Around line 245-252: Update the confirmation notice rendered by
PatientAppShell around switchedTo to use an appropriate ARIA live-region role,
such as status, so assistive technologies announce the “now showing” message
when the profile switch takes effect. Preserve the existing conditional
rendering and auto-dismiss behavior.
In `@src/components/Patient/PatientProfileCard.tsx`:
- Around line 14-16: Update the JSDoc for PatientProfileCard in
src/components/Patient/PatientProfileCard.tsx:14-16 to document its patient
profile-selection medical use case and selection semantics for assistive
technology. Also update the component documentation in
src/components/Patient/CancelAppointmentButton.tsx:30 to describe appointment
cancellation and the confirmation dialog’s accessibility behavior.
- Around line 21-31: Update the blood-group translation logic in the patient
display formatter to avoid constructing the t() key dynamically. Use a static
BloodGroupChoices-to-translation-key map or explicit branches so every
BLOOD_GROUP__* key is discoverable, while preserving the existing exclusion of
unknown or missing blood groups.
In `@src/hooks/usePatientOtpLogin.ts`:
- Around line 80-117: Update the error extraction in the verifyOtpRequest
mutation’s onError handler to read validation errors from the thrown response’s
established error-data properties: use error.errors for the manually thrown OTP
response and error.data for HTTP failures from mutate(), while retaining
rawMessage as the fallback. Remove the cause-based lookup and preserve the
existing OTP-specific selection and "invalid_otp" default behavior.
In `@src/hooks/usePatientPortalData.ts`:
- Around line 42-46: Update the appointment query key in usePatientPortalData to
use only the phoneNumber, since getAppointments returns the same phone-scoped
payload for every profile. Keep selectedPatient-based filtering in the existing
useMemo logic so profile switches reuse cached data without refetching.
- Around line 78-89: Expose isError from the useQuery result in the prescription
hook alongside isLoading, while retaining silent: true to suppress global
toasts. Apply the same change to usePatientDiagnosticReports so both hooks
return their query error state for pages to render retry/error states instead of
treating failures as empty data.
In `@src/pages/Patient/DiagnosticReportDetail.tsx`:
- Around line 92-113: Replace the observation results’ unassociated div/span
structure in the observations.map block with a semantic table, using table,
thead, tbody, tr, th, and td elements while preserving the existing grid styling
and displayed Test/Result/Reference content. Associate each data cell with its
corresponding column header and retain the current flagged, reference,
interpretation, and observation.id behavior.
In `@src/pages/Patient/index.tsx`:
- Around line 47-64: Update the Patient page’s QUICK_ACTIONS label resolution so
each translation key is exposed through explicit static t() calls rather than
dynamic t(action.key). Define the resolved labels or a labelKey mapping inside
the component with literal calls for book_appointment, prescriptions,
diagnostic_reports, and visits, then use those values when rendering each quick
action.
In `@src/pages/Patient/PatientRecords.tsx`:
- Around line 194-196: In the shownReports mapping, replace the
readyReports.includes(report) identity check with a direct status check using
the exported READY_REPORT_STATUSES symbol. Update isReady in the relevant
component flow so readiness is derived from report.status without relying on
shared object references or scanning readyReports.
In `@src/pages/Patient/PrescriptionDetail.tsx`:
- Around line 47-51: Update src/pages/Patient/PrescriptionDetail.tsx around the
useQuery call and loading branch to destructure isError, keeping the skeleton
only while loading and rendering a not-found/retry message once the query
settles without prescription; apply the same change to
src/pages/Patient/DiagnosticReportDetail.tsx around its report query and
rendering branch, using isError and the settled-without-report state.
- Around line 18-25: Extract the duplicated MetaField component into a shared
component with an optional value-size or className prop, preserving the default
styling. In src/pages/Patient/PrescriptionDetail.tsx#L18-L25 and
src/pages/Patient/VisitSummary.tsx#L32-L39, remove the local declarations and
import the shared component; in
src/pages/Patient/DiagnosticReportDetail.tsx#L28-L35, do the same and pass the
smaller value styling through the prop.
In `@src/pages/Patient/records/reportUtils.ts`:
- Around line 140-143: Update observationValueLabel so the fallback and final
placeholder decision use nullish checks rather than truthiness, preserving
numeric 0 as a displayed value while still returning "-" for null or undefined
results.
- Around line 61-67: Update the reference_range matching predicate in the
surrounding report utility to require at least one parseable bound before
accepting a range. Preserve the existing inclusive min/max comparisons and
undefined-bound behavior for ranges where either min or max is valid, while
ensuring entries with both bounds undefined do not match any value.
In `@src/pages/PublicAppointments/BookFacility.tsx`:
- Line 22: Centralize the booking wizard constants beside BookingStepLayout.tsx
and export TOTAL_STEPS, SLOT_STEP, and REASON_STEP from that shared module. In
src/pages/PublicAppointments/BookFacility.tsx lines 22-22 and
src/pages/PublicAppointments/BookPractitioner.tsx lines 16-16, remove the local
TOTAL_STEPS declarations and import the shared constant; in
src/pages/PublicAppointments/Schedule.tsx lines 43-45, move all three local
constants to the shared module and import them.
- Around line 28-39: Update districtOrganization to begin traversal at
patient?.geo_organization rather than its parent, and walk ancestors until the
level_cache === 1 district is found. Return that district-level organization,
while preserving undefined when no qualifying organization exists.
In `@src/pages/PublicAppointments/BookingStepLayout.tsx`:
- Around line 63-69: Add an accessible name to the progressbar element in
BookingStepLayout by providing an aria-label that describes the tracked booking
step; optionally add aria-valuetext conveying the current step and total steps
(for example, “Step 3 of 5”) while preserving the existing aria-valuenow,
aria-valuemin, and aria-valuemax attributes.
In `@src/pages/PublicAppointments/BookPractitioner.tsx`:
- Line 52: Update the back action in BookPractitioner to navigate to the
facility-list step route that renders BookFacility instead of
/nearby_facilities. Also update Schedule’s back action to return to
/patient/book/${facilityId} instead of goBack("/facility/${facilityId}"),
preserving the wizard’s step-by-step flow at both affected sites:
src/pages/PublicAppointments/BookPractitioner.tsx:52-52 and
src/pages/PublicAppointments/Schedule.tsx:326-326.
In `@src/pages/PublicAppointments/PatientRegistration.tsx`:
- Around line 373-382: Align the age validation boundary used by the schema’s
superRefine with the UI condition in the PatientRegistration age display: update
the schema to reject age 0 as well as negative values, matching the existing
enteredAge check for <= 0.
- Around line 172-177: Align token handling in PatientRegistration with the
patient context contract: update the createPatient mutation and the other
tokenData.token access near the submit flow to consistently either require a
guaranteed patientUserContext and remove optional access, or guard both token
reads when the context may be absent. Preserve the existing selected-patient
behavior while ensuring no optional tokenData is dereferenced unconditionally.
- Around line 56-64: Replace the PatientRegistrationProps object type alias with
an interface while preserving the existing optional facilityId and staffId
properties and their documentation.
- Around line 476-478: Update the submit Button in PatientRegistration so it
remains disabled through the entire patient registration and appointment-booking
flow, not just while isCreatingPatient is true. Include the booking-in-progress
state used by createAppointment in the disabled condition, preserving the
existing register_patient label and preventing a second submission while the
form remains mounted.
In `@src/pages/PublicAppointments/Schedule.tsx`:
- Around line 333-337: The hardcoded English date/time format must be replaced
with the repository’s localized formatter across all appointment timestamp
displays. Update Schedule.tsx lines 333-337, 240-242, and 417, plus Success.tsx
lines 159-161 and 104, using the active locale so weekday/month names and
12/24-hour conventions are localized; preserve the existing fallback behavior.
- Around line 185-215: Update the rescheduling flow around cancelAppointment’s
onSuccess and createAppointment so creation failures are handled explicitly:
show a distinct error explaining that the original appointment was cancelled and
the user must choose another slot, then keep the user on the slot-selection step
instead of advancing. Add the necessary onError handling for the create
operation while preserving normal cancellation and creation behavior.
In `@src/pages/PublicAppointments/Success.tsx`:
- Around line 70-93: Handle the missing-auth case explicitly in the component
before the appointment lookup/not-found rendering: when tokenData?.token is
absent, render the established patient login prompt or redirect behavior instead
of “appointment not found.” Keep the existing query and appointment rendering
unchanged for authenticated users.
- Around line 113-123: Update the clipboard fallback in handleShare to guard
navigator.clipboard availability and catch writeText failures, showing an
appropriate translated error toast instead of allowing an unhandled rejection.
Reuse an existing translation key if available; otherwise add the required error
key to public/locale/en.json and use it in the failure path.
In `@src/Routers/PatientRouter.tsx`:
- Around line 114-117: Update the appointmentPages branch in the PatientRouter
route rendering logic to wrap appointmentPages with the same ErrorBoundary used
by the pages branch, while preserving the existing PatientUserProvider wrapper
and fallback configuration.
In `@src/Routers/PublicRouter.tsx`:
- Around line 65-75: Update the isPatientPath check in PublicRouter to match
only the exact "/patient" path or paths beginning with "/patient/"; keep
excluding "/patient/login" and preserve the existing redirect behavior for valid
patient deep links.
---
Outside diff comments:
In `@src/Providers/PatientUserProvider.tsx`:
- Around line 43-53: Expose the error state from the useQuery call in
PatientUserProvider and pass it through the provider’s rendering flow separately
from patients. Update the SelectProfile conditional so failed patient lookups
render an error state rather than treating userData?.results ?? [] as a
legitimate empty profile list or offering profile creation.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 59613ba8-df4a-48a2-9e2d-ab420cd86177
📒 Files selected for processing (38)
.envcare.config.tspublic/locale/en.jsonsrc/Providers/AuthUserProvider.tsxsrc/Providers/PatientUserProvider.tsxsrc/Routers/PatientRouter.tsxsrc/Routers/PublicRouter.tsxsrc/common/constants.tsxsrc/components/Auth/Login.tsxsrc/components/Patient/CancelAppointmentButton.tsxsrc/components/Patient/PatientAppShell.tsxsrc/components/Patient/PatientProfileCard.tsxsrc/components/Patient/PatientSwitcherSheet.tsxsrc/hooks/usePatientOtpLogin.tssrc/hooks/usePatientPortalData.tssrc/pages/Landing/LandingPage.tsxsrc/pages/Organization/components/GovtOrganizationSelector.tsxsrc/pages/Patient/DiagnosticReportDetail.tsxsrc/pages/Patient/PatientProfileSettings.tsxsrc/pages/Patient/PatientRecords.tsxsrc/pages/Patient/PatientVisits.tsxsrc/pages/Patient/PrescriptionDetail.tsxsrc/pages/Patient/SelectProfile.tsxsrc/pages/Patient/VisitSummary.tsxsrc/pages/Patient/components/AppointmentDialog.tsxsrc/pages/Patient/index.tsxsrc/pages/Patient/records/reportUtils.tssrc/pages/PublicAppointments/BookFacility.tsxsrc/pages/PublicAppointments/BookPractitioner.tsxsrc/pages/PublicAppointments/BookingStepLayout.tsxsrc/pages/PublicAppointments/PatientRegistration.tsxsrc/pages/PublicAppointments/PatientSelect.tsxsrc/pages/PublicAppointments/Schedule.tsxsrc/pages/PublicAppointments/Success.tsxsrc/pages/PublicAppointments/auth/PatientAuthLayout.tsxsrc/pages/PublicAppointments/auth/PatientLogin.tsxsrc/types/otp/otp.tssrc/vite-env.d.ts
💤 Files with no reviewable changes (2)
- src/pages/PublicAppointments/PatientSelect.tsx
- src/pages/Patient/components/AppointmentDialog.tsx
| const start = dayjs(appointment.token_slot.start_datetime); | ||
|
|
||
| return ( | ||
| <> | ||
| <Button | ||
| variant="outline" | ||
| size={size} | ||
| className={className} | ||
| onClick={() => setOpen(true)} | ||
| > | ||
| {t("cancel_appointment")} | ||
| </Button> | ||
|
|
||
| <AlertDialog open={open} onOpenChange={setOpen}> | ||
| <AlertDialogContent className="max-w-[440px]"> | ||
| <AlertDialogHeader> | ||
| <AlertDialogTitle>{t("cancel_appointment")}</AlertDialogTitle> | ||
| <AlertDialogDescription> | ||
| {t("patient_visits__cancel_confirmation", { | ||
| name: formatScheduleResourceName(appointment), | ||
| datetime: start.format("ddd, D MMM YYYY · h:mm A"), | ||
| })} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use locale-aware formatting for the appointment timestamp.
The fixed Day.js format hard-codes ordering and a 12-hour clock. Format the value with the active locale, such as Intl.DateTimeFormat(i18n.language, { dateStyle: "medium", timeStyle: "short" }).
As per coding guidelines, “localized medical timestamps must use localized date/time formatting.”
🤖 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/Patient/CancelAppointmentButton.tsx` around lines 70 - 91,
Replace the fixed format call on the `start` timestamp in
`CancelAppointmentButton` with locale-aware date/time formatting using the
active `i18n.language`, preserving medium date and short time styles before
passing `datetime` to the cancellation confirmation translation.
Source: Coding guidelines
| const TABS = [ | ||
| { key: "home", href: "/patient/home", icon: Home, label: "home" }, | ||
| { | ||
| key: "visits", | ||
| href: "/patient/visits", | ||
| icon: CalendarDays, | ||
| label: "visits", | ||
| }, | ||
| { | ||
| key: "records", | ||
| href: "/patient/records", | ||
| icon: FolderClosed, | ||
| label: "records", | ||
| }, | ||
| { key: "profile", href: "/patient/profile", icon: User, label: "profile" }, | ||
| ] as const; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
i18n keys passed via variable aren't statically detectable.
t(tab.label) resolves keys from TABS at runtime, so the repo's remove-unused-i18n.js cleanup can't see home/visits/records/profile and may prune them. Store the full call site statically, e.g. put a label: () => t("home")-style accessor or inline literal t() calls per tab.
Based on learnings: to ensure the remove-unused-i18n.js cleanup script detects i18n keys, avoid passing computed keys directly to t(); prefer explicit literal calls or a precomputed shared key.
Also applies to: 193-193
🤖 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/Patient/PatientAppShell.tsx` around lines 25 - 40, Update the
TABS label handling and its usage in PatientAppShell so translation calls use
statically detectable literal keys instead of passing tab.label to t(). Preserve
each tab’s existing translation key by using explicit t("home"), t("visits"),
t("records"), and t("profile") call sites or equivalent per-tab label accessors.
Source: Learnings
| /** | ||
| * `34 yrs · Female · O+`, dropping the blood group when it is not recorded. | ||
| */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Document medical use and accessibility considerations for both patient UI components.
src/components/Patient/PatientProfileCard.tsx#L14-L16: document its profile-selection use case and selection semantics for assistive technology.src/components/Patient/CancelAppointmentButton.tsx#L30-L30: document its appointment-cancellation use case and confirmation-dialog accessibility behavior.
As per coding guidelines, “Document components with their medical use cases and accessibility considerations.”
📍 Affects 2 files
src/components/Patient/PatientProfileCard.tsx#L14-L16(this comment)src/components/Patient/CancelAppointmentButton.tsx#L30-L30
🤖 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/Patient/PatientProfileCard.tsx` around lines 14 - 16, Update
the JSDoc for PatientProfileCard in
src/components/Patient/PatientProfileCard.tsx:14-16 to document its patient
profile-selection medical use case and selection semantics for assistive
technology. Also update the component documentation in
src/components/Patient/CancelAppointmentButton.tsx:30 to describe appointment
cancellation and the confirmation dialog’s accessibility behavior.
Source: Coding guidelines
| ? dayjs(selectedSlot.start_datetime).format( | ||
| "ddd D MMM · h:mm A", | ||
| ) | ||
| : "-"} | ||
| </span> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Appointment timestamps use hardcoded English dayjs patterns. "ddd D MMM · h:mm A" bypasses locale-aware weekday/month names and 12/24-hour conventions, so these clinical timestamps stay English under every non-English locale. Use the repo's localized date/time formatting helper (or dayjs with the active i18n locale loaded).
src/pages/PublicAppointments/Schedule.tsx#L333-L337: format the selected-slot summary via the localized formatter (same for the reason-step footer at Lines 240-242 and the slot time at Line 417).src/pages/PublicAppointments/Success.tsx#L159-L161: format the appointment date/time via the localized formatter (same for the shared text at Line 104).
As per coding guidelines, "localized medical timestamps must use localized date/time formatting".
📍 Affects 2 files
src/pages/PublicAppointments/Schedule.tsx#L333-L337(this comment)src/pages/PublicAppointments/Success.tsx#L159-L161
🤖 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/pages/PublicAppointments/Schedule.tsx` around lines 333 - 337, The
hardcoded English date/time format must be replaced with the repository’s
localized formatter across all appointment timestamp displays. Update
Schedule.tsx lines 333-337, 240-242, and 417, plus Success.tsx lines 159-161 and
104, using the active locale so weekday/month names and 12/24-hour conventions
are localized; preserve the existing fallback behavior.
Source: Coding guidelines
| if (navigator.share) { | ||
| try { | ||
| await navigator.share({ title: summaryTitle, text }); | ||
| } catch { | ||
| // Dismissing the native share sheet is not an error worth surfacing. | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| await navigator.clipboard.writeText(text); | ||
| toast.success(t("copied_to_clipboard")); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Clipboard fallback can throw unhandled.
navigator.clipboard is undefined in non-secure contexts and writeText rejects when permission is denied; handleShare is an unguarded async click handler, so the failure surfaces as an unhandled rejection with no user feedback.
🛡️ Proposed fix
- await navigator.clipboard.writeText(text);
- toast.success(t("copied_to_clipboard"));
+ try {
+ await navigator.clipboard.writeText(text);
+ toast.success(t("copied_to_clipboard"));
+ } catch {
+ toast.error(t("copy_to_clipboard_failed"));
+ }Confirm the error key exists in public/locale/en.json (add it if not).
📝 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.
| if (navigator.share) { | |
| try { | |
| await navigator.share({ title: summaryTitle, text }); | |
| } catch { | |
| // Dismissing the native share sheet is not an error worth surfacing. | |
| } | |
| return; | |
| } | |
| await navigator.clipboard.writeText(text); | |
| toast.success(t("copied_to_clipboard")); | |
| if (navigator.share) { | |
| try { | |
| await navigator.share({ title: summaryTitle, text }); | |
| } catch { | |
| // Dismissing the native share sheet is not an error worth surfacing. | |
| } | |
| return; | |
| } | |
| try { | |
| await navigator.clipboard.writeText(text); | |
| toast.success(t("copied_to_clipboard")); | |
| } catch { | |
| toast.error(t("copy_to_clipboard_failed")); | |
| } |
🤖 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/pages/PublicAppointments/Success.tsx` around lines 113 - 123, Update the
clipboard fallback in handleShare to guard navigator.clipboard availability and
catch writeText failures, showing an appropriate translated error toast instead
of allowing an unhandled rejection. Reuse an existing translation key if
available; otherwise add the required error key to public/locale/en.json and use
it in the failure path.
| const path = usePath(); | ||
|
|
||
| // A signed-out visitor deep-linking int | ||
| const isPatientPath = | ||
| !!path && path.startsWith("/patient") && path !== "/patient/login"; | ||
|
|
||
| return ( | ||
| <> | ||
| <BrowserWarning /> | ||
| {routeResult || <Login />} | ||
| {routeResult || | ||
| (isPatientPath ? <Redirect to="/patient/login" /> : <Login />)} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
startsWith("/patient") can false-positive match unrelated paths.
Without a trailing slash, any unmatched path beginning with the literal string "/patient" (e.g. /patients, /patient-registration) is treated as a patient deep link and redirected to /patient/login, even if it isn't actually under the /patient/... namespace.
🐛 Proposed fix
- const isPatientPath =
- !!path && path.startsWith("/patient") && path !== "/patient/login";
+ const isPatientPath =
+ !!path &&
+ (path === "/patient" || path.startsWith("/patient/")) &&
+ path !== "/patient/login";#!/bin/bash
rg -n '"/patient' src/Routers -g '*.tsx' -g '*.ts'🤖 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/Routers/PublicRouter.tsx` around lines 65 - 75, Update the isPatientPath
check in PublicRouter to match only the exact "/patient" path or paths beginning
with "/patient/"; keep excluding "/patient/login" and preserve the existing
redirect behavior for valid patient deep links.
… a scale Replaced 90 arbitrary font sizes (53 of them sub-pixel: 12.5px, 13.5px, 10.5px, 14.5px, 11.5px, 9.5px) with Tailwind scale classes. A single screen rendered 8-11 distinct sizes with no ratio between them. Scale is now 10 / xs(12) / sm(14) / base(16) / lg(18) plus three display sizes. Max visual change is 1.5px. Entire-Checkpoint: ee2aa8184c79
The patient portal is mobile-first but several primary controls were well under the 44px tap minimum: 'See all' at 20px (below even the WCAG 2.2 AA 24px floor), Records/Visits header tabs at 34px, filter chips at 33px, the patient switcher chip at 42px, and the logo home link at 28px. All now min-h-11. Filter chips lose their vertical padding in favour of the min-height so the pill keeps its proportions. Entire-Checkpoint: fd291f30c9fd
View Details, Reschedule and Cancel Appointment used Button size="sm" (h-8, 32px). These are the primary actions on the upcoming appointment card, so they get a min-h-11 floor. Same for the cancel dialog's confirm/dismiss pair. Entire-Checkpoint: 007b8d19f32c
Entire-Checkpoint: c316d7900a98
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 57 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/pages/PublicAppointments/PatientRegistration.tsx:193
reasoncomes from query params and can beundefinedwhen the URL has noreasonparameter; calling.trim()will throw and break the post-registration appointment creation. Use optional chaining and default to an empty string instead (also removes the redundant?? ""aftertrim()).
src/pages/PublicAppointments/auth/PatientAuthLayout.tsx:63- Clickable
<img>elements are not keyboard-accessible and don’t announce as an interactive control to assistive tech. Wrap the logo in a<button>(or a<Link>) so it’s focusable and has proper semantics.
src/pages/PublicAppointments/Success.tsx:84 navigator.clipboard.writeTextcan fail (permissions, insecure context, unsupported browser) and currently any failure will result in an unhandled rejection and no user feedback. Handle errors and show a fallback toast.
.env:12- This PR is focused on patient portal pages/navigation, but it also changes the committed default
REACT_CARE_API_URL. Since.envis tracked (and not gitignored), this alters the default API target for anyone pulling the branch and can unintentionally affect builds/deployments. If this isn’t intentional, revert the value or move it to a non-committed env file (e.g..env.local).
REACT_CARE_API_URL=https://develop-api.ohc.network
src/pages/PublicAppointments/auth/PatientLogin.tsx:51
- This introduces a new end-to-end patient OTP login flow (consent → send OTP → verify → profile selection) and new booking/records/visits pages, but there are no Playwright specs covering
/patient/loginor the patient portal routes. Adding at least one Playwright test for successful sign-in + profile select and a smoke test for records/visits navigation would help prevent regressions.
src/Routers/PatientRouter.tsx:117 BrowserWarningis rendered for dashboard routes but not forAppointmentRoutes(booking / registration pages). This makes unsupported-browser handling inconsistent and can hide the warning on some patient flows. Render<BrowserWarning />in theappointmentPagesbranch as well.
<PatientUserProvider>
<ErrorBoundary fallback={<ErrorPage forError="PAGE_LOAD_ERROR" />}>
{appointmentPages}
</ErrorBoundary>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Grumpy Review 🔥
The architecture here is actually reasonable — extracting usePatientOtpLogin, building out a proper PatientAppShell with a shared switcher, and wiring up the localStorage-backed patient selection properly. Begrudgingly: it's a decent refactor.
That said, there are issues you need to address:
- The
.envAPI URL change is the most alarming thing in this PR. Fix it before merge. path.startsWithtab detection — works today, breaks silently later. Add the trailing slash guard.BookingStepLayoutdouble space — run Prettier.extractOtpErrorMessagetype-casting — not wrong enough to block, but it's the kind of code that bites you six months from now.PatientUserProviderdouble navigation — not a hard bug but worth tidying up.
This is also a draft PR, so you presumably know it's not ready. When you are ready: write those Playwright tests mentioned in the checklist. "Add specs" being unchecked on a feature this size is not an acceptable final state.
Generated by Grumpy PR Reviewer for issue #16612 · 71.1 AIC · ⌖ 6.22 AIC · ⊞ 6.3K
| import mutate from "@/Utils/request/mutate"; | ||
|
|
||
| const extractOtpErrorMessage = (error: unknown, fallback: string) => { | ||
| const cause = (error as { cause?: unknown })?.cause; |
There was a problem hiding this comment.
This whole function is a tower of as casts on unknown. It works, barely, but it's fragile — if the backend error shape changes, this silently falls through and you get the generic fallback with no indication why. At least use typeof x === "object" && x !== null type guards so TypeScript can actually help you here.
|
|
||
| # Care API URL without the /api prefix | ||
| REACT_CARE_API_URL=https://careapi.ohc.network | ||
| REACT_CARE_API_URL=https://develop-api.ohc.network |
There was a problem hiding this comment.
🚨 You're committing develop-api.ohc.network as the default API URL. That was careapi.ohc.network before. Anyone who clones this repo now gets pointed at the dev API by default. This is almost certainly a "works on my machine" leftover. Revert this before it bites someone in production.
| const tabBar = ( | ||
| <> | ||
| {TABS.map((tab) => { | ||
| const isActive = path?.startsWith(tab.href); |
There was a problem hiding this comment.
path?.startsWith("/patient/home") is a trap. If someone ever adds a route like /patient/homepage or /patient/home-visit, the Home tab will light up incorrectly. Use path === tab.href || path?.startsWith(tab.href + "/") — or at minimum add a trailing-slash guard for the home route specifically.
| <div className=" xl:w-2/3 xl:mx-auto">{headerExtra}</div> | ||
| </header> | ||
|
|
||
| <div className="flex flex-1 flex-col xl:w-2/3 xl:mx-auto"> |
There was a problem hiding this comment.
Double space in the className: "flex flex-1 flex-col xl:w-2/3 xl:mx-auto". Harmless, but embarrassing. Prettier should have caught this.
| // unauthenticated (or signed-out) visitor to the patient login rather than | ||
| // the public landing page. | ||
| useEffect(() => { | ||
| if (!tokenData) { |
There was a problem hiding this comment.
You have navigate("/patient/login") inside a useEffect AND if (!tokenData) { return null } at line 100. The effect fires asynchronously so there's a render gap where this component returns null and the navigate hasn't resolved yet — flickers are possible. Pick one: either return null and navigate synchronously in the render body (before hooks, which is a no-no) or use only the effect. The effect is fine; just make sure the return null below doesn't cause a blank flash while raviger processes the navigation.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 56 out of 59 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/pages/PublicAppointments/PatientRegistration.tsx:193
reasoncomes from query params and can be undefined; callingreason.trim()will throw at runtime and break the booking flow after patient creation. Guard it before trimming.
src/pages/PublicAppointments/Success.tsx:85- If
navigator.shareis unavailable,navigator.clipboard.writeTextcan throw (e.g. insecure context, permission denied, unsupported API), which would create an unhandled rejection and no user feedback. Wrap clipboard access in try/catch and surface a generic error toast on failure.
src/Providers/PatientUserProvider.tsx:95 - This redirect triggers whenever
patients.length === 0and the query is no longer loading. If the patients query fails (network/API error),userDatastays undefined andpatientsbecomes[], so the user will be redirected to the profile picker as if they had no linked profiles. Gate this redirect on having receiveduserDatasuccessfully.
if (
tokenData &&
!isLoading &&
patients.length === 0 &&
!isProfileSetupPath
.env:12
- This PR changes the default
REACT_CARE_API_URLin the committed.env. That alters the default backend target for anyone running the repo without a custom.env.local, and it isn’t mentioned in the PR description. If this isn’t intentional, revert it (or move it to a documented sample env file).
REACT_CARE_API_URL=https://develop-api.ohc.network
There was a problem hiding this comment.
Grumpy but fair assessment
The patient portal redesign is architecturally solid — the PatientUserProvider refactor with proper localStorage persistence, the usePortalInfiniteList abstraction for server-paged data, and separating data-fetching into usePatientPortalData.ts are all good decisions.
But I found 5 things that need fixing before this ships:
.envAPI URL — committeddevelop-api.ohc.networkas the default. Revert immediately.usePatientAppointments— all appointments fetched for the phone number then client-side filtered by patient ID. The queryKey also doesn't includeselectedPatient?.id, meaning cached data from one patient bleeds into another. Fix the queryKey and preferably server-filter.DiagnosticReportDetail— noisErrorhandling. Perpetual skeleton on failure.PrescriptionDetail— noisLoadingorisErrordestructured fromuseQuery. Flashes empty state while loading; silent failure on error.PrescritionListtypo — it exists upstream, yes. You don't have to fix it everywhere today, but don't add more references to a misspelled type name.
The rest of it is... fine. Good, even. The SelectProfile auto-skip for single-patient accounts is a nice touch.
Generated by Grumpy PR Reviewer for issue #16612 · 91 AIC · ⌖ 6.27 AIC · ⊞ 6.3K
|
|
||
| # Care API URL without the /api prefix | ||
| REACT_CARE_API_URL=https://careapi.ohc.network | ||
| REACT_CARE_API_URL=https://develop-api.ohc.network |
There was a problem hiding this comment.
🔥 Seriously? You're committing a change to the default API URL pointing at develop-api.ohc.network in a feature PR? This will redirect every developer who clones this repo away from the production API. Revert this to careapi.ohc.network. Dev-only URL overrides belong in .env.local, not in the committed .env.
|
|
||
| return useMemo(() => { | ||
| const appointments = (data?.results ?? []) | ||
| .filter((appointment) => appointment.patient.id === selectedPatient?.id) |
There was a problem hiding this comment.
The queryKey is ["appointment", phoneNumber], but you filter the results by selectedPatient?.id client-side. You're fetching every appointment for the entire phone number and filtering in JS. Worse, the cache key doesn't include the patient ID — when a user switches profiles, React Query serves the same stale list until a refetch. Either pass patient as a query param to the server (like you correctly do for prescriptions and reports), or at minimum include selectedPatient?.id in the queryKey so stale data isn't served for the wrong patient.
| const { t } = useTranslation(); | ||
| const { tokenData } = usePatientContext(); | ||
|
|
||
| const { data: report, isLoading } = useQuery({ |
There was a problem hiding this comment.
You destructure isLoading from useQuery but never destructure isError. When this request fails (404, 500, network error — take your pick), the user is stuck staring at skeletons forever. Add isError to the destructuring and render an error state. Medical reports that silently fail to load are a usability nightmare.
| const { t } = useTranslation(); | ||
| const { tokenData } = usePatientContext(); | ||
|
|
||
| const { data: prescription } = useQuery({ |
There was a problem hiding this comment.
You only destructure { data: prescription } — no isLoading, no isError. While the page fetches, it renders with empty medications and falls through to the empty state. The user sees "no medications found" for a second before the data loads. And if the request errors out? Same empty state, no indication that something went wrong. Destructure isLoading and isError and handle them properly.
| import { DiagnosticReportRead } from "@/types/emr/diagnosticReport/diagnosticReport"; | ||
| import { | ||
| PrescriptionRead, | ||
| PrescritionList, |
There was a problem hiding this comment.
PrescritionList — missing the 'c' in Prescription. Yes, this typo originates in prescription.ts (line 26), but this is your chance to fix it. You're creating a new API file right now. An exported interface named PrescritionList that's already used in 3+ places is technical debt that compounds with every new reference you add here.
ohc-project-manager
Bot
added
needs testing
needs review
and removed
Work in Progress
labels
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 56 out of 59 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/pages/PublicAppointments/PatientRegistration.tsx:193
reasoncomes fromuseQueryParams()and can beundefined; callingreason.trim()will throw at runtime, breaking patient registration after a successful create. Use optional chaining (and only trim when present).
src/Utils/observationRange.ts:85formatRangeBoundstreats empty strings as present bounds (e.g.min === ""renders as"> "), but earlier in this filetoNumber()explicitly treats""as missing. If the API sends""for absent bounds (as the comment suggests), this will render incorrect range text.
const hasMin = min !== null && min !== undefined;
const hasMax = max !== null && max !== undefined;
src/pages/Patient/PatientVisits.tsx:127
- The History tab’s empty-state copy still uses the Upcoming-tab wording/key. You already added
patient_visits__no_historyto i18n; use it here so the UI matches the selected tab.
.env:13 - The committed
.envnow pointsREACT_CARE_API_URLtodevelop-api.ohc.network, but bothREADME.mdand.example.envdocumenthttps://careapi.ohc.networkas the default fallback. This mismatch is likely to confuse local development and onboarding; prefer keeping.envaligned with docs (and use.env.localfor developer-specific overrides).
# Care API URL without the /api prefix
REACT_CARE_API_URL=https://develop-api.ohc.network
REACT_SBOM_BASE_URL=https://sbom.ohc.network
There was a problem hiding this comment.
Review summary
The patient portal redesign is architecturally sound — the move from a sidebar-driven layout to a mobile-first app shell, consolidating sign-out into AuthUserProvider, and the new usePatientPortalData hook are all steps in the right direction. That said, there are a handful of issues that need addressing before this merges:
Must fix:
.envAPI URL —REACT_CARE_API_URLpoints todevelop-api.ohc.network. That's the wrong default for a default config file that ships in the repo.- QR code encodes raw patient UUID — Use the booking reference or token number instead; the internal DB ID shouldn't be baked into a shareable QR code.
PatientShellContextis never provided — The context is created and a value object is assembled, but<PatientShellContext.Provider>is missing from the JSX. Consumers will silently get the default (no-op) value.
Should fix:
usePatientOtpLoginerror strings — The hook sets error state with bare i18n keys ("phone_number_validation_error","invalid_otp", etc.) without translating them. Verify consumers callt()on these, or translate at the source.- Auth guard in
PatientUserProvider— Navigating on everytokenData === nullrender, before the auth context has finished hydrating, risks a redirect loop. Add a loading guard. - Client-side appointment filtering — Fetching all appointments then filtering by patient ID in JS wastes bandwidth on multi-patient accounts. Pass the patient ID to the API.
Grumpy but not hopeless — the bones are good, fix the sharp edges.
Generated by Grumpy PR Reviewer for issue #16612 · 120.7 AIC · ⌖ 6.38 AIC · ⊞ 6.3K
Comments that could not be inline-anchored
.env:12
Oh, fantastic. The default REACT_CARE_API_URL has been quietly swapped from the production API (careapi.ohc.network) to the dev API (develop-api.ohc.network). Anyone cloning this repo — or any CI build that sources .env — now points at the dev backend by default. That's not a configuration, that's a footgun. Revert this or at least add a screaming comment explaining why develop is the new default.
src/components/Patient/AppointmentTokenPass.tsx:98
You're encoding the patient's internal UUID directly into a QR code that's rendered on-screen and potentially screenshotted/shared. That UUID can be used to correlate the patient across API calls. If the QR is meant to be scanned at a counter, use the booking reference ID (appointment.id.slice(0, 8)) or the token number — something that's already shown in the same card. Don't give the raw database ID to scanners who may not need it.
src/hooks/usePatientOtpLogin.ts:116
setPhoneError("phone_number_validation_error") — you're passing a bare i18n key as the error string, but is anyone calling t() on it before rendering? If the error is rendered directly as {phoneError}, users see the raw key. Check that every consumer of phoneError and otpError passes them through t(). If not, either translate here or document the contract. Same applies to the hardcoded "send_otp_error" and "invalid_otp" strings below.
src/Providers/PatientUserProvider.tsx:78
Navigating inside a useEffect that fires whenever tokenData changes is asking for trouble. If PatientUserProvider is mounted before the auth context has fully initialised (e.g. during the very first render), tokenData will be null and you'll immediately redirect to /patient/login — which likely triggers a re-render, which re-fires this effect. At minimum, wait until the auth context has finished loading before deciding to redirect. Add a loading guard here.
src/hooks/usePatientPortalData.ts:70
You're fetching every appointment for the phone number and then filtering by selectedPatient?.id on the client. When a phone has multiple linked patients with many appointments, that's a lot of data transferred to silently throw most of it away. Pass the patient ID as a query parameter to the API so the server does the filtering — your bandwidth bill and your users on slow mobile connections will thank you.
src/components/Patient/PatientAppShell.tsx:1152
PatientShellContext is created here but I don't see it exported or provided anywhere in the tree below. PatientAppShell creates the context value but never wraps children in a <PatientShellContext.Provider>, so any descendant calling useContext(PatientShellContext) will always get the default { openSwitcher: () => {}, canSwitch: false }. Either export and provide it, or remove the context entirely and pass props directly.
There was a problem hiding this comment.
Grumpy ReviewTM
This is a substantial feature — new portal pages, hooks, routing, shell component. The architecture is generally sound. But there are a handful of real problems worth fixing before merge:
Must fix:
.envAPI URL — you committed a dev-API URL as the new default. This breaks any environment that sources this file expecting to hit production. Revert it.PrescriptionDetailhas no error state — permanent skeletons on query failure.DiagnosticReportDetailhas the same issue (it does destructureisLoadingbut no error handling).PatientShellContextis dead code — context is provided but never consumed. Either wire it up with an exported hook or remove it.
Should fix:
- All three new files import
dayjsdirectly instead of from"@/Utils/dayjs". This is an inconsistency that will bite someone when they add a plugin-dependent call. queryKey: unknown[]inusePortalInfiniteListis lazy typing in a file that otherwise has good type coverage.- The
usePatientAppointmentsclient-side filter vs cache-key design needs a comment explaining the intent, otherwise the next reader will "fix" it and break the patient-switcher performance.
The first name extraction (name.trim().split(/\s+/)[0]) is duplicated between PatientRecords.tsx and PatientAppShell.tsx's abbreviateName. Minor, but worth a shared utility.
Otherwise: the routing redirects for legacy paths, the infinite scroll integration, and the overall component structure are... fine. Begrudgingly fine.
Generated by Grumpy PR Reviewer for issue #16612 · 73.2 AIC · ⌖ 6.38 AIC · ⊞ 6.3K
|
|
||
| # Care API URL without the /api prefix | ||
| REACT_CARE_API_URL=https://careapi.ohc.network | ||
| REACT_CARE_API_URL=https://develop-api.ohc.network |
There was a problem hiding this comment.
Oh fantastic, you committed the dev API URL to the shared .env. The old value pointed at the production API; this now silently redirects anyone who pulls this branch—or any CI job that sources .env—to the dev backend. Either leave .env alone or add a comment making it clear this is a local-dev-only override. Changing the default in the committed file is how you break everyone else's environment.
| @@ -0,0 +1,236 @@ | |||
| import { useInfiniteQuery, useQuery } from "@tanstack/react-query"; | |||
| import dayjs from "dayjs"; | |||
There was a problem hiding this comment.
The entire project uses import dayjs from "@/Utils/dayjs" — the plugin-extended instance that has relativeTime and other plugins pre-loaded. Bare import dayjs from "dayjs" skips that setup. Right now .isAfter() doesn't need a plugin, but the moment someone calls .fromNow() or similar in this file, it silently returns Invalid Date. Use the project's shared instance.
| queryParams, | ||
| enabled, | ||
| }: { | ||
| queryKey: unknown[]; |
There was a problem hiding this comment.
queryKey: unknown[] is too loose. You're basically telling TypeScript "don't help me here." The factory function already knows the shape — use a typed tuple or at least readonly (string | number | undefined)[]. unknown[] defeats the purpose of having TypeScript.
| const { token, phoneNumber, headers } = useAuthHeaders(); | ||
|
|
||
| const { data, isLoading } = useQuery({ | ||
| queryKey: ["appointment", phoneNumber], |
There was a problem hiding this comment.
queryKey is ["appointment", phoneNumber] but the results are filtered by selectedPatient?.id. If the user switches profiles, the query key doesn't change, so TanStack Query returns cached data instantly and filters it client-side — fine. But this means the whole appointment list for the phone number is always fetched regardless of which patient is selected. At least document this intent; right now it looks like a missing cache-key dependency.
| const SWITCH_NOTICE_MS = 3000; | ||
|
|
||
| /** Lets descendant screens open the shell's shared patient switcher sheet. */ | ||
| const PatientShellContext = createContext<{ |
There was a problem hiding this comment.
PatientShellContext is created and provided but never consumed anywhere — no exported hook, no useContext call outside this file. The context value (openSwitcher, canSwitch) is dead code. Either export a usePatientShell() hook so descendant screens can actually open the switcher, or delete the context and just wire things up via props/callbacks directly. As written it does nothing.
| @@ -0,0 +1,130 @@ | |||
| import { useQuery } from "@tanstack/react-query"; | |||
| import dayjs from "dayjs"; | |||
There was a problem hiding this comment.
Same bare dayjs import problem as in usePatientPortalData.ts — use "@/Utils/dayjs". Also: import dayjs from "dayjs" here (line 2) vs import { useTranslation } in line 3 violates the project's import ordering convention (3rd-party alphabetical, then internal). Run npm run format before committing.
| const { t } = useTranslation(); | ||
| const { tokenData } = usePatientContext(); | ||
|
|
||
| const { data: prescription } = useQuery({ |
There was a problem hiding this comment.
useQuery is called without destructuring isLoading. When the query is loading or has errored, prescription is undefined, so the skeleton renders forever on a network error. The user stares at spinning bones with no way to retry. Destructure isLoading and isError, render an error state, and add retry: 1 or similar. Medical data pages need proper error handling.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 57 out of 60 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/pages/PublicAppointments/PatientRegistration.tsx:193
reasoncomes from query params and can be undefined; callingreason.trim()will throw and block patient registration. Use optional chaining (and the nullish fallback becomes meaningful).
src/Utils/observationRange.ts:89formatRangeBoundstreats empty-string bounds ("") as present, which can render placeholders like"> "/"< ". The module already treats""as absent intoNumber, so the formatter should normalize empty strings too.
const hasMin = min !== null && min !== undefined;
const hasMax = max !== null && max !== undefined;
if (hasMin && hasMax) return `${min} - ${max}`;
if (hasMin) return `> ${min}`;
if (hasMax) return `< ${max}`;
src/Routers/PatientRouter.tsx:64
<Redirect>from raviger doesn’t support aqueryprop, so these redirects will droptab=...and always land on the default records tab. Include the query string in thetoURL instead.
"/patient/medications": () => (
<Redirect to="/patient/records" query={{ tab: "prescriptions" }} />
),
"/patient/diagnostic_reports": () => (
<Redirect to="/patient/records" query={{ tab: "reports" }} />
src/pages/PublicAppointments/auth/PatientAuthLayout.tsx:37
- An
<img>withonClickis not keyboard-accessible and won’t be announced as an interactive control. Wrap the logo in a<button>/link so it’s focusable and has an accessible name.
src/components/Common/LoginHeader.tsx:50 - The dropdown trigger
<button>has no accessible name (its contents are icons marked/treated as decorative), so screen readers will announce it poorly. Add anaria-label(or a visually-hidden text label).
<button
type="button"
className="flex items-center gap-2 rounded-full border border-gray-200 bg-gray-50 py-1.5 pl-1.5 pr-3 hover:border-gray-300"
>
.env:12
- Changing the committed default
REACT_CARE_API_URLtodevelop-apiwill silently change the backend used by anyone relying on this repo’s.env. If this was only needed for a local/testing setup, it’s safer to keep the previous default (or move the override to.env.local, which shouldn’t be committed).
# Care API URL without the /api prefix
REACT_CARE_API_URL=https://develop-api.ohc.network
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 56 out of 59 changed files in this pull request and generated no new comments.
Suppressed comments (7)
src/pages/PublicAppointments/PatientRegistration.tsx:193
reasoncomes from query params and can be undefined; callingreason.trim()will throw at runtime during patient creation. Use a null-safe trim so booking without a reason (or without the param) doesn’t crash the flow.
src/pages/PublicAppointments/auth/PatientAuthLayout.tsx:38- The logo is made clickable via
onClickon an<img>, which isn’t keyboard-accessible and doesn’t expose an interactive role to assistive tech. Wrap it in a<button>(or<Link>) so it’s reachable and operable via keyboard.
src/components/Patient/PatientSwitcherSheet.tsx:50 max-w-120is not part of Tailwind’s default scale and isn’t added in tailwind.config.js, so the sheet width constraint will be ignored. Use an arbitrary value (or an existing max-w token) to ensure the layout matches the intended 480px column.
className="mx-auto max-h-[85dvh] max-w-120 overflow-y-auto rounded-t-3xl px-5 pb-6 pt-3 [&>button:first-of-type]:hidden"
src/components/Patient/VisitCard.tsx:86
w-13is not a default Tailwind width token and isn’t defined in tailwind.config.js, so the date tile width won’t apply. Use an arbitrary width (or an existing spacing token) to keep the layout consistent.
<div className="w-13 shrink-0 rounded-xl border border-primary-200 bg-white py-1.5 text-center">
src/pages/Patient/SelectProfile.tsx:51
h-19is not a default Tailwind height token and isn’t defined in tailwind.config.js, so these skeleton rows won’t have the intended height. Consider using an arbitrary height (or a standard token) to match the design.
src/components/Patient/PrescriptionRow.tsx:62size-4.25is not a default Tailwind size token and isn’t defined in tailwind.config.js, so the chevron size won’t apply. Use an arbitrarysize-[…]value or a standard size token.
className="size-4.25 shrink-0 text-gray-600"
src/pages/Patient/VisitSummary.tsx:231
h-15.5is not a default Tailwind spacing token and isn’t defined in tailwind.config.js, so the sentinel placeholder height will be ignored. Use an arbitrary height (or a standard token) so the list doesn’t jump when fetching more rows.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 56 out of 59 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/pages/PublicAppointments/PatientRegistration.tsx:193
reasoncomes from query params and can be undefined; callingreason.trim()will throw and prevent appointment creation. Use optional chaining (or a fallback string) before trimming.
src/pages/Patient/PatientVisits.tsx:128- The empty state for the History tab uses the upcoming-appointments description key, which is misleading. There is already a dedicated
patient_visits__no_historystring in en.json.
src/pages/PublicAppointments/auth/PatientAuthLayout.tsx:37 - The logo is implemented as a clickable
<img>, which is not keyboard-accessible and doesn’t convey button/link semantics to assistive tech. Wrap it in a<button>(or a link) and keep the image purely presentational inside.
There was a problem hiding this comment.
Grumpy Review: Patient Portal Redesign
Big feature, generally clean. The pattern of centralising all portal data fetching in usePatientPortalData.ts is the right call, and the useMemo for selectedPatient resolution in PatientUserProvider with the localStorage persistence is done correctly.
That said, four things need addressing before this ships:
- Appointment pagination (
usePatientPortalData.tsL97) — the appointments hook doesn't paginate. The prescriptions and reports hooks do. This is a latent data-completeness bug. - Client-side appointment filtering (L103) — inconsistent with the server-side patient scoping everywhere else. Fix or document why it's intentional here.
PrescritionListtypo (patientPortalApi.tsL11) — propagating a misspelling. Fix it at the source.- Fragile Tailwind DOM hack (
PatientSwitcherSheet.tsxL56) —[&>button:first-of-type]:hiddenwill break silently on any shadcn upgrade. - Non-localized date formats (
DiagnosticReportDetail.tsxL73/78) — hard-coded"DD MMM, h:mm A"in a multi-locale healthcare app is a bad look.
The rest is... fine, I guess. The infinite-scroll abstraction is reusable, the profile-selection redirect logic is solid, and removing AppointmentDialog.tsx in favour of the new flows is the right call.
Generated by Grumpy PR Reviewer for issue #16612 · 56.6 AIC · ⌖ 7.54 AIC · ⊞ 6.3K
| export default { | ||
| listPrescriptions: { | ||
| path: "/api/v1/otp/medication_prescription/", | ||
| method: HttpMethod.GET, |
There was a problem hiding this comment.
Typo in the imported type name. PrescritionList is missing a 'p' — it should be PrescriptionList. This pre-existing typo in the source type is now spreading to every consumer of this API file. Fix it at the source before it metastasizes further.
| <MetaField | ||
| label={t("reported")} | ||
| value={dayjs(report.created_date).format("DD MMM, h:mm A")} | ||
| /> |
There was a problem hiding this comment.
Hard-coded date format, not localized. "DD MMM, h:mm A" is baked in on lines 73 and 78. This is a patient-facing healthcare app deployed in multiple locales — dates should be formatted with dayjs locale-aware helpers or at minimum routed through a shared utility. Same issue on line 78.
| aria-hidden | ||
| className="mx-auto mb-4 block h-1 w-10 rounded-full bg-gray-300" | ||
| /> | ||
| <SheetHeader className="space-y-1 text-left"> |
There was a problem hiding this comment.
[&>button:first-of-type]:hidden is a fragile DOM-structure hack. You're reaching into shadcn's SheetContent internals to hide the default close button. If the component library ever changes its internal structure (it has before), this silently stops working and the close button reappears. Use a SheetClose without a trigger, or pass hideClose / a similar controlled prop to your SheetContent wrapper instead.
|
|
||
| return useMemo(() => { | ||
| const appointments = (data?.results ?? []) | ||
| .filter((appointment) => appointment.patient.id === selectedPatient?.id) |
There was a problem hiding this comment.
No pagination here. getAppointments almost certainly returns a paginated response, but you're using a plain useQuery with no limit/offset handling. If a patient has more than whatever the default page size is, history and upcoming counts will be silently wrong. The prescriptions and diagnostic reports correctly use usePortalInfiniteList — appointments deserve the same treatment.
| new Date(a.token_slot.start_datetime).getTime() - | ||
| new Date(b.token_slot.start_datetime).getTime(), | ||
| ); | ||
|
|
There was a problem hiding this comment.
Client-side patient filtering is inconsistent with the rest of the portal. Prescriptions and diagnostic reports pass patient: selectedPatient?.id as a server-side query param. Appointments fetch all patients' data for the phone number and filter client-side. That's fine for the Success page (intentional), but for the Visits page you're shipping extra data over the wire and doing unnecessary work on the client. Pass patient_id (or whatever the backend accepts) as a query param here if the API supports it.
Proposed Changes
Fixes ENG-831
Screenshoot
Mobile
if multiple users
Desktop
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit