Cleanup : Remove HistoryAPIProvider and useAppHistory and rely on browser history to prevent navigation loops#16114
Open
NikhilA8606 wants to merge 4 commits intodevelopfrom
Open
Cleanup : Remove HistoryAPIProvider and useAppHistory and rely on browser history to prevent navigation loops#16114NikhilA8606 wants to merge 4 commits intodevelopfrom
NikhilA8606 wants to merge 4 commits intodevelopfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis pull request removes a custom HistoryAPIProvider and useAppHistory hook, replacing them with standard browser history.back() and raviger's navigate function. Multiple components are updated to use history-based navigation, several navigation calls now include replace: true, and BackButton is refactored to handle back navigation directly. Changes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Deploying care-preview with
|
| Latest commit: |
ffd53ad
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://759845a6.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://feat-cleanup-back.care-preview-a7w.pages.dev |
Greptile SummaryThis PR removes the custom
Confidence Score: 3/5
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/components/Common/BackButton.tsx
Outdated
Comment on lines
+3
to
+9
| type BackButtonProps = { | ||
| to?: string; | ||
| fallbackUrl?: string; | ||
| } & React.ComponentProps<typeof Button>; | ||
|
|
||
| children: React.ReactNode; | ||
| variant?: ButtonVariant; | ||
| className?: string; | ||
| size?: ButtonSize; | ||
| disabled?: boolean; | ||
| }; |
There was a problem hiding this comment.
Props not forwarded — data-shortcut-id and aria-label silently dropped
The new BackButtonProps interface only accepts children, variant, className, size, and disabled. Extra attributes are not spread onto the underlying Button, so any caller that passes unknown props will silently lose them at runtime (and TypeScript will raise a type error).
Two existing call sites are already affected by this:
src/CAREUI/misc/PrintPreview.tsx:63passesdata-shortcut-id="go-back"— the shortcut registration will silently not work because the attribute never reaches the DOM.src/pages/Encounters/ReportViewer.tsx:401passesaria-label={t("back")}— the accessibility label will be silently dropped.
The simplest fix is to extend the props interface with React.ComponentPropsWithoutRef<"button"> (or simply spread ...rest through to Button):
Suggested change
| type BackButtonProps = { | |
| to?: string; | |
| fallbackUrl?: string; | |
| } & React.ComponentProps<typeof Button>; | |
| children: React.ReactNode; | |
| variant?: ButtonVariant; | |
| className?: string; | |
| size?: ButtonSize; | |
| disabled?: boolean; | |
| }; | |
| type BackButtonProps = { | |
| children: React.ReactNode; | |
| variant?: ButtonVariant; | |
| className?: string; | |
| size?: ButtonSize; | |
| disabled?: boolean; | |
| } & Omit<React.ComponentPropsWithoutRef<"button">, "onClick">; | |
| export default function BackButton({ | |
| children, | |
| variant = "outline", | |
| className, | |
| size = "default", | |
| disabled = false, | |
| ...rest | |
| }: BackButtonProps) { | |
| return ( | |
| <Button | |
| type="button" | |
| variant={variant} | |
| onClick={() => history.back()} | |
| className={className} | |
| size={size} | |
| disabled={disabled} | |
| {...rest} | |
| > | |
| {children} | |
| </Button> | |
| ); | |
| } |
Comment on lines
415
to
419
| {showHeader && ( | ||
| <div className="flex items-start justify-between flex-col sm:flex-row gap-4 sm:items-center border-b-3 border-double pb-4"> | ||
| <div className="flex gap-3 sm:gap-6 flex-col md:flex-row"> | ||
| <BackButton | ||
| variant="link" | ||
| className="px-0 justify-start" | ||
| fallbackUrl={`/facility/${facilityId}/billing/account/${accountId}`} | ||
| > | ||
| <ChevronLeft /> | ||
| <span>{t("back")}</span> | ||
| </BackButton> | ||
| <div className="h-auto w-px bg-gray-300" aria-hidden="true" /> | ||
| {account?.patient && ( |
There was a problem hiding this comment.
Header back navigation removed without replacement
The previous code rendered a BackButton (with a fallbackUrl pointing to the account page) in the header area of CreateInvoicePage whenever showHeader is true — i.e., when the page is displayed standalone rather than embedded. That button was removed entirely in this PR; the only back navigation now lives in the form's bottom cancel button.
When a user opens the create-invoice page directly (e.g., via a link or bookmark with no prior history), the bottom cancel button still calls history.back(), which could exit the app entirely. At a minimum, the header lost a consistent entry point for navigating back to the account. Consider re-adding a BackButton (or a plain Link) here to restore that affordance.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the custom app-level history tracking (HistoryAPIProvider / useAppHistory) and shifts navigation behavior to rely on browser history and Raviger’s navigate(..., { replace: true }) in several create/edit flows to avoid navigation loops.
Changes:
- Removed
HistoryAPIProviderwiring and deleteduseAppHistory, updating callers to usehistory.back()/navigate(). - Updated many post-create/update navigations to use
replace: true. - Refactored
BackButtonto usehistory.back()and updated multiple screens to use it.
Reviewed changes
Copilot reviewed 52 out of 53 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pages/PublicAppointments/auth/PatientLogin.tsx | Removes useAppHistory usage; adjusts back navigation logic. |
| src/pages/PublicAppointments/Schedule.tsx | Replaces explicit “back” button with BackButton. |
| src/pages/Patient/PatientHome.tsx | Replaces goBack with BackButton for verification failure state. |
| src/pages/Facility/settings/productKnowledge/ProductKnowledgeView.tsx | Removes explicit to from BackButton usage. |
| src/pages/Facility/settings/product/ProductForm.tsx | Uses replace: true on success; uses history.back() on cancel. |
| src/pages/Facility/settings/healthcareService/HealthcareServiceForm.tsx | Uses replace: true on create/update navigations. |
| src/pages/Facility/settings/devices/components/DeviceForm.tsx | Replaces goBack cancel with BackButton. |
| src/pages/Facility/settings/devices/UpdateDevice.tsx | Uses history.back() after successful update. |
| src/pages/Facility/settings/devices/CreateDevice.tsx | Uses history.back() after successful create; removes navigate import. |
| src/pages/Facility/settings/chargeItemDefinitions/UpdateChargeItemDefinition.tsx | Adds replace: true when navigating after success. |
| src/pages/Facility/settings/chargeItemDefinitions/ChargeItemDefinitionForm.tsx | Adds replace: true on success; uses history.back() on cancel. |
| src/pages/Facility/settings/chargeItemDefinitions/ChargeItemDefinitionDetail.tsx | Adds replace: true after delete; removes fallbackUrl from BackButton. |
| src/pages/Facility/settings/activityDefinition/ActivityDefinitionView.tsx | Adds replace: true after delete; removes explicit to/fallbackUrl from BackButton usages. |
| src/pages/Facility/settings/activityDefinition/ActivityDefinitionForm.tsx | Adds replace: true after create/update; replaces cancel button with BackButton. |
| src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx | Replaces goBack button with BackButton. |
| src/pages/Facility/services/pharmacy/MedicationReturnShow.tsx | Removes to={basePath} from BackButton. |
| src/pages/Facility/services/inventory/externalSupply/requestOrder/RequestOrderForm.tsx | Adds replace: true after success; uses history.back() for cancel. |
| src/pages/Facility/services/inventory/externalSupply/deliveryOrder/DeliveryOrderShow.tsx | Replaces BackButton link with Button + navigate(...). |
| src/pages/Facility/services/inventory/externalSupply/deliveryOrder/DeliveryOrderForm.tsx | Adds replace: true after success navigation. |
| src/pages/Facility/services/HealthcareServiceShow.tsx | Removes explicit to from BackButton. |
| src/pages/Facility/queues/ManageQueue.tsx | Removes computed to target for BackButton; trims unused imports. |
| src/pages/Facility/billing/paymentReconciliation/PaymentReconciliationShow.tsx | Replaces “back to payments” button with BackButton. |
| src/pages/Facility/billing/invoice/InvoiceShow.tsx | Removes explicit to target from BackButton. |
| src/pages/Facility/billing/account/utils.ts | Adds billing/account status helper utilities. |
| src/pages/Facility/billing/account/components/PrintChargeItems.tsx | Replaces goBack with BackButton. |
| src/pages/Facility/billing/account/CreateInvoice.tsx | Uses replace: true when navigating to created invoice; replaces cancel behavior with history.back() fallback. |
| src/pages/Facility/FacilityDetailsPage.tsx | Replaces goBack("/facilities") with BackButton. |
| src/pages/Encounters/ReportViewer.tsx | Removes facility/patient props; removes BackButton fallback URL usage. |
| src/pages/Encounters/EncounterShow.tsx | Replaces goBack("/") with navigate("/") on permission error. |
| src/pages/Appointments/AppointmentsPage.tsx | Replaces goBack("/") with navigate("/") on permission error. |
| src/pages/Appointments/AppointmentDetail.tsx | Replaces goBack calls with navigate; adjusts back button usage. |
| src/pages/Admin/TagConfig/TagConfigView.tsx | Replaces goBack button with BackButton. |
| src/hooks/useAppHistory.ts | Deletes the custom app history hook. |
| src/components/ui/button.tsx | Exports ButtonSize type. |
| src/components/ValueSet/ValueSetForm.tsx | Replaces cancel navigation with BackButton. |
| src/components/Users/UserHome.tsx | Replaces goBack("/") with navigate("/") on error. |
| src/components/Users/UserDeleteDialog.tsx | Replaces goBack("/") with navigate("/") after delete success. |
| src/components/Resource/ResourceList.tsx | Minor formatting change (whitespace). |
| src/components/Resource/ResourceForm.tsx | Adds replace: true after create/update; replaces cancel with BackButton. |
| src/components/Questionnaire/QuestionnaireForm.tsx | Removes onCancel prop; uses BackButton for cancel. |
| src/components/Patient/PatientRegistration.tsx | Adds replace: true on navigate; uses history.back() after update. |
| src/components/Patient/PatientDetailsTab/patientUpdates.tsx | Replaces goBack(...) with navigate(...) on permission error. |
| src/components/Patient/PatientDetailsTab/EncounterHistory.tsx | Replaces goBack(...) with navigate(...) on permission error. |
| src/components/Patient/EncounterQuestionnaire.tsx | Removes onCancel usage; drops useAppHistory. |
| src/components/Facility/FacilityDeleteDialog.tsx | Simplifies post-delete navigation to always navigate("/"). |
| src/components/ErrorPages/DefaultErrorPage.tsx | Replaces goBack("/") with navigate("/") before reload. |
| src/components/Common/Drawings/ExcalidrawEditor.tsx | Replaces goBack() with history.back() paths. |
| src/components/Common/BackButton.tsx | Reimplements BackButton to always call history.back() (no link/to/fallback). |
| src/Routers/routes/ConsultationRoutes.tsx | Updates ReportViewer route props after prop removal. |
| src/CAREUI/misc/PrintPreview.tsx | Replaces back Button + goBack() with BackButton. |
| src/App.tsx | Removes HistoryAPIProvider wrapper from the app root. |
| package.json | Bumps raviger dependency version. |
| package-lock.json | Updates lockfile for raviger version bump. |
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+266
to
+268
| <BackButton variant="primary_gradient" className="gap-3 group"> | ||
| {t("go_back")} | ||
| </Button> | ||
| </BackButton> |
Comment on lines
86
to
+100
| @@ -100,14 +94,10 @@ export function FacilityDetailsPage({ id }: Props) { | |||
| return ( | |||
| <div className="container mx-auto px-4 py-8"> | |||
| <div className="flex justify-between items-center pb-4"> | |||
| <Button | |||
| variant="outline" | |||
| className="border border-secondary-400" | |||
| onClick={() => goBack("/facilities")} | |||
| > | |||
| <BackButton variant="outline" className="border border-secondary-400"> | |||
| <CareIcon icon="l-arrow-left" className="size-4 mr-1" /> | |||
| <span className="text-sm underline">{t("back")}</span> | |||
| </Button> | |||
| </BackButton> | |||
Comment on lines
+394
to
397
| <BackButton variant="link" className="px-0 justify-start"> | ||
| <ChevronLeft /> | ||
| <span>{t("back")}</span> | ||
| </BackButton> |
Comment on lines
229
to
+232
| <div className="flex px-2 pb-4 justify-start"> | ||
| <Button | ||
| variant="outline" | ||
| className="border border-secondary-400" | ||
| onClick={() => goBack(`/facility/${facilityId}`)} | ||
| > | ||
| <BackButton> | ||
| <span className="text-sm underline">{t("back")}</span> | ||
| </Button> | ||
| </BackButton> |
src/components/Common/BackButton.tsx
Outdated
Comment on lines
+3
to
+9
| type BackButtonProps = { | ||
| to?: string; | ||
| fallbackUrl?: string; | ||
| } & React.ComponentProps<typeof Button>; | ||
|
|
||
| children: React.ReactNode; | ||
| variant?: ButtonVariant; | ||
| className?: string; | ||
| size?: ButtonSize; | ||
| disabled?: boolean; | ||
| }; |
Comment on lines
+126
to
129
| <BackButton> | ||
| <ArrowLeft /> | ||
| <span>{t("back_to_services")}</span> | ||
| </BackButton> |
Comment on lines
+134
to
137
| <BackButton variant="outline" className="mt-4"> | ||
| <CareIcon icon="l-arrow-left" className="mr-2 size-4" /> | ||
| {t("back_to_list")} | ||
| </BackButton> |
Comment on lines
117
to
143
| if (isError || !product) { | ||
| return ( | ||
| <Page title={t("error")}> | ||
| <div className="container mx-auto max-w-3xl py-8"> | ||
| <Alert variant="destructive"> | ||
| <CareIcon icon="l-exclamation-triangle" className="size-4" /> | ||
| <AlertTitle>{t("error_loading_product_knowledge")}</AlertTitle> | ||
| <AlertDescription> | ||
| {t("product_knowledge_not_found")} | ||
| </AlertDescription> | ||
| </Alert> | ||
| <BackButton> | ||
| <ArrowLeft /> | ||
| {t("back_to_list")} | ||
| </BackButton> | ||
| </div> | ||
| </Page> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <Page title={product.name} hideTitleOnPage={true}> | ||
| <div className="container mx-auto max-w-3xl space-y-6"> | ||
| <BackButton | ||
| to={`/facility/${facilityId}/settings/product_knowledge/categories/${product.category.slug}`} | ||
| > | ||
| <BackButton> | ||
| <ArrowLeft /> | ||
| {t("back")} | ||
| </BackButton> |
Comment on lines
+523
to
+529
| <BackButton | ||
| className="w-full flex items-center relative" | ||
| variant="outline" | ||
| onClick={() => | ||
| goBack(`/facility/${facilityId}/billing/payments`) | ||
| } | ||
| data-shortcut-id="go-back" | ||
| > | ||
| <ArrowLeft className="size-4" /> | ||
| {t("back_to_payments")} | ||
| </Button> | ||
| </BackButton> |
| <div className="container mx-auto max-w-7xl mt-4"> | ||
| <div className="flex gap-2 items-center mb-2"> | ||
| <BackButton size="icon" variant="ghost"> | ||
| <BackButton variant="ghost"> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/Patient/PatientDetailsTab/patientUpdates.tsx (1)
31-35:⚠️ Potential issue | 🟠 MajorUse replace-style navigation to prevent back-loop traps in permission redirects.
On line 31, this permission guard redirects using standard
navigate(), which pushes a new history entry. When users deny access, hitting browser back returns them to the blocked page and immediately redirects again, creating a loop. Pass{ replace: true }as the second argument to replace the current history entry instead:navigate( facilityId ? `/facility/${facilityId}/patient/${patientId}` : `/patient/${patientId}`, { replace: true } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Patient/PatientDetailsTab/patientUpdates.tsx` around lines 31 - 35, The current redirect uses navigate(...) which pushes a new history entry and can create a back-loop on permission denial; update the call to navigate (the one using facilityId and patientId in patientUpdates.tsx) to pass the navigation options object with replace: true as the second argument so the redirect replaces the current history entry instead of pushing a new one.src/pages/Facility/queues/ManageQueue.tsx (1)
125-131:⚠️ Potential issue | 🟠 MajorLabel this icon-only back button.
This renders only
ChevronLeft, so assistive tech gets an unnamed control. Add an accessible name here onceBackButtonforwards button props.As per coding guidelines, "Implement proper ARIA labels and roles in page components".♿ Proposed fix
<BackButton // TODO: move queue index page for practitioner to similar pattern path size="icon" variant="ghost" + aria-label={t("back")} + title={t("back")} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Facility/queues/ManageQueue.tsx` around lines 125 - 131, The icon-only BackButton (rendering ChevronLeft) lacks an accessible name; update the BackButton usage in ManageQueue.tsx to pass an accessible label prop (e.g., aria-label="Go back to queues" or similar) so assistive tech can announce it, and if BackButton does not currently forward button props, modify the BackButton component to forward props to the underlying <button> (or accept an explicit accessibleName/ariaLabel prop) so aria-label is applied; target references: BackButton and ChevronLeft.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/CAREUI/misc/PrintPreview.tsx`:
- Around line 63-67: The BackButton component is swallowing arbitrary DOM
attributes (like data-shortcut-id) instead of forwarding them to the actual
button, breaking shortcut binding; update the BackButton component (BackButton)
to accept and forward remaining props to the rendered button element (e.g.,
extend its props with React.ButtonHTMLAttributes<HTMLButtonElement> or include a
rest parameter like ...props) and spread those props onto the underlying
<button> (or the component that renders the native element) so attributes such
as data-shortcut-id="go-back" reach the DOM.
In `@src/components/Common/BackButton.tsx`:
- Around line 1-27: BackButton drops arbitrary Button props (so aria-label,
data-* attrs, etc. never reach the DOM); change BackButtonProps to extend
React.ComponentProps<typeof Button> (or import ComponentProps) and then in the
BackButton function destructure children plus the props you want to default
(variant = "outline", size = "default", disabled = false) and collect the rest
via ...props, then spread {...props} onto the rendered <Button> so all extra
attributes (aria-*, data-*, event handlers) are forwarded; reference the
BackButton component, BackButtonProps type, and the imported Button to locate
where to apply this change.
In `@src/components/ErrorPages/DefaultErrorPage.tsx`:
- Around line 177-180: The onClick handler in DefaultErrorPage.tsx currently
calls navigate("/") and then window.location.reload(), which is redundant;
change the navigate call to navigate("/", { replace: true }) to avoid pushing a
history entry before the full-page reload (keep the subsequent
window.location.reload() if you still want a hard reload), i.e., update the
onClick lambda that references navigate and window.location.reload accordingly.
In `@src/components/Patient/PatientDetailsTab/EncounterHistory.tsx`:
- Line 57: The redirect unconditionally uses facilityId which may be undefined,
causing URLs like /facility/undefined/patient/:patientId; update the navigation
logic in EncounterHistory (where navigate(...) is called) to check facilityId
from PatientProps and, if present, call
navigate(`/facility/${facilityId}/patient/${patientId}`), otherwise fallback to
the patient-scoped route such as navigate(`/patient/${patientId}`); ensure you
reference the facilityId and patientId variables from the component props and
adjust any types if necessary.
In `@src/components/ui/button.tsx`:
- Line 65: The exported ButtonSize type is duplicating the size literals from
the CVA config and can drift; instead derive it from the buttonVariants
definition by importing VariantProps from "class-variance-authority" and setting
ButtonSize = VariantProps<typeof buttonVariants>["size"] (replace the current
literal union), ensuring any future changes to buttonVariants automatically
update the exported type; update any imports/usages accordingly to use the
derived type.
In `@src/components/Users/UserHome.tsx`:
- Around line 72-74: The component currently calls navigate("/") directly during
render when isError is true; move that redirect into a useEffect inside the
UserHome component so the navigation runs as a side effect (useEffect(() => { if
(!isError) return; navigate("/"); }, [isError, navigate])). Also add an early
return in the render path (e.g., if (isError) return null) to prevent rendering
while the redirect is happening. Ensure you import useEffect from React and
reference the existing navigate and isError variables.
In `@src/pages/Appointments/AppointmentDetail.tsx`:
- Around line 159-167: The permission-guard redirects in AppointmentDetail.tsx
currently call navigate('/') and navigate(`/facility/${facility.id}/overview`)
without replacing history, allowing back-navigation to the restricted route;
update both navigate calls (the one that redirects to `/` and the one that
redirects to `/facility/${facility.id}/overview`) to pass the replace option
(e.g., { replace: true }) so the current history entry is replaced instead of
pushed.
In `@src/pages/Facility/billing/account/CreateInvoice.tsx`:
- Around line 921-927: The Cancel onClick currently calls onCancel or falls back
to history.back(), which is a no-op in a fresh tab; update the handler in the
CreateInvoice component so after if (onCancel) onCancel(); else if
(window.history.length > 1) history.back(); else navigate to a deterministic
fallback route (e.g., push/replace to your invoices/billing page using your
router or window.location.assign to a known URL) so Cancel always returns
somewhere sensible. Reference the onClick handler, onCancel, and history.back()
when making the change.
In `@src/pages/Facility/billing/account/utils.ts`:
- Around line 8-27: Replace the exported arrow-const utilities with named
function declarations: change the const-exported canAddChargeItemsToAccount and
isAccountBillingClosed into exported functions using the function keyword,
preserving their parameter signature (account: AccountRead | AccountBase |
undefined) and the existing logic (checking account falsiness,
AccountStatus.active, and the billing_status includes array). Ensure the
exported names remain identical and update any export style if necessary so
callers continue to import canAddChargeItemsToAccount and isAccountBillingClosed
as before.
- Around line 1-27: The new helpers canAddChargeItemsToAccount and
isAccountBillingClosed are unreferenced causing the "Unimported file" CI
failure; either remove the file or wire these functions into the billing/account
call sites that decide whether to show charge-item controls (e.g., the account
detail/view or charge creation flow). To fix: import canAddChargeItemsToAccount
and/or isAccountBillingClosed into the component or service that renders billing
actions (or into the billing barrel export) and replace inline status checks
with these helpers so they are referenced, or delete the file if you do not
intend to use them yet. Ensure you reference the exact exported names
(canAddChargeItemsToAccount, isAccountBillingClosed) when importing.
In
`@src/pages/Facility/services/inventory/externalSupply/deliveryOrder/DeliveryOrderShow.tsx`:
- Around line 428-446: Replace the icon-only Button that calls
navigate(getInventoryBasePath(...)) with the existing BackButton component so
the control uses browser history back behavior (avoid re-pushing routes and
navigation loops); ensure you remove the navigate(...) call and pass no explicit
"to" prop, and if the BackButton component requires it or you must keep an
icon-only control, add an accessible label (aria-label or visually-hidden text)
to the control to satisfy accessibility for the icon-only back action in
DeliveryOrderShow (replace usages of Button, navigate, and getInventoryBasePath
with BackButton).
In
`@src/pages/Facility/services/inventory/externalSupply/requestOrder/RequestOrderForm.tsx`:
- Line 558: The cancel handler currently calls history.back() directly in
RequestOrderForm which can navigate out of the app or do nothing if the form was
opened in a new tab; replace this with a safe redirect: import navigate and
useRedirect from raviger, get a redirect function (e.g., const redirect =
useRedirect()) or call navigate directly, then update the onClick for the cancel
control to attempt history.back() first in a try/catch and if that fails (or if
document.referrer is empty) call redirect or navigate to the known in‑app route
(the facility inventory list route used elsewhere in RequestOrderForm) so
cancelling always returns the user to the app.
In `@src/pages/Facility/settings/devices/UpdateDevice.tsx`:
- Line 52: Replace the unreliable history.back() call in UpdateDevice with an
explicit navigate to the updated device's detail route: use the existing
deviceId (from props or state in UpdateDevice) and call
navigate(`/devices/${deviceId}`, { replace: true }) so the app behavior matches
patterns used in ProductForm and ActivityDefinitionForm; if CreateDevice needs
similar handling, either navigate to the devices list or refactor DeviceForm to
return the created device data so you can call navigate to
`/devices/${newDeviceId}` with replace: true.
---
Outside diff comments:
In `@src/components/Patient/PatientDetailsTab/patientUpdates.tsx`:
- Around line 31-35: The current redirect uses navigate(...) which pushes a new
history entry and can create a back-loop on permission denial; update the call
to navigate (the one using facilityId and patientId in patientUpdates.tsx) to
pass the navigation options object with replace: true as the second argument so
the redirect replaces the current history entry instead of pushing a new one.
In `@src/pages/Facility/queues/ManageQueue.tsx`:
- Around line 125-131: The icon-only BackButton (rendering ChevronLeft) lacks an
accessible name; update the BackButton usage in ManageQueue.tsx to pass an
accessible label prop (e.g., aria-label="Go back to queues" or similar) so
assistive tech can announce it, and if BackButton does not currently forward
button props, modify the BackButton component to forward props to the underlying
<button> (or accept an explicit accessibleName/ariaLabel prop) so aria-label is
applied; target references: BackButton and ChevronLeft.
🪄 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: 527a27a9-7b3a-479e-acca-b8010bf7e2a8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (52)
package.jsonsrc/App.tsxsrc/CAREUI/misc/PrintPreview.tsxsrc/Routers/routes/ConsultationRoutes.tsxsrc/components/Common/BackButton.tsxsrc/components/Common/Drawings/ExcalidrawEditor.tsxsrc/components/ErrorPages/DefaultErrorPage.tsxsrc/components/Facility/FacilityDeleteDialog.tsxsrc/components/Patient/EncounterQuestionnaire.tsxsrc/components/Patient/PatientDetailsTab/EncounterHistory.tsxsrc/components/Patient/PatientDetailsTab/patientUpdates.tsxsrc/components/Patient/PatientRegistration.tsxsrc/components/Questionnaire/QuestionnaireForm.tsxsrc/components/Resource/ResourceForm.tsxsrc/components/Resource/ResourceList.tsxsrc/components/Users/UserDeleteDialog.tsxsrc/components/Users/UserHome.tsxsrc/components/ValueSet/ValueSetForm.tsxsrc/components/ui/button.tsxsrc/hooks/useAppHistory.tssrc/pages/Admin/TagConfig/TagConfigView.tsxsrc/pages/Appointments/AppointmentDetail.tsxsrc/pages/Appointments/AppointmentsPage.tsxsrc/pages/Encounters/EncounterShow.tsxsrc/pages/Encounters/ReportViewer.tsxsrc/pages/Facility/FacilityDetailsPage.tsxsrc/pages/Facility/billing/account/CreateInvoice.tsxsrc/pages/Facility/billing/account/components/PrintChargeItems.tsxsrc/pages/Facility/billing/account/utils.tssrc/pages/Facility/billing/invoice/InvoiceShow.tsxsrc/pages/Facility/billing/paymentReconciliation/PaymentReconciliationShow.tsxsrc/pages/Facility/queues/ManageQueue.tsxsrc/pages/Facility/services/HealthcareServiceShow.tsxsrc/pages/Facility/services/inventory/externalSupply/deliveryOrder/DeliveryOrderForm.tsxsrc/pages/Facility/services/inventory/externalSupply/deliveryOrder/DeliveryOrderShow.tsxsrc/pages/Facility/services/inventory/externalSupply/requestOrder/RequestOrderForm.tsxsrc/pages/Facility/services/pharmacy/MedicationReturnShow.tsxsrc/pages/Facility/services/serviceRequests/ServiceRequestShow.tsxsrc/pages/Facility/settings/activityDefinition/ActivityDefinitionForm.tsxsrc/pages/Facility/settings/activityDefinition/ActivityDefinitionView.tsxsrc/pages/Facility/settings/chargeItemDefinitions/ChargeItemDefinitionDetail.tsxsrc/pages/Facility/settings/chargeItemDefinitions/ChargeItemDefinitionForm.tsxsrc/pages/Facility/settings/chargeItemDefinitions/UpdateChargeItemDefinition.tsxsrc/pages/Facility/settings/devices/CreateDevice.tsxsrc/pages/Facility/settings/devices/UpdateDevice.tsxsrc/pages/Facility/settings/devices/components/DeviceForm.tsxsrc/pages/Facility/settings/healthcareService/HealthcareServiceForm.tsxsrc/pages/Facility/settings/product/ProductForm.tsxsrc/pages/Facility/settings/productKnowledge/ProductKnowledgeView.tsxsrc/pages/Patient/PatientHome.tsxsrc/pages/PublicAppointments/Schedule.tsxsrc/pages/PublicAppointments/auth/PatientLogin.tsx
💤 Files with no reviewable changes (3)
- src/hooks/useAppHistory.ts
- src/components/Resource/ResourceList.tsx
- src/components/Patient/EncounterQuestionnaire.tsx
Comment on lines
177
to
180
| onClick={() => { | ||
| goBack("/"); | ||
| navigate("/"); | ||
| window.location.reload(); | ||
| }} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/components/ErrorPages/DefaultErrorPage.tsx | head -200Repository: ohcnetwork/care_fe
Length of output: 7533
🏁 Script executed:
rg -A 5 -B 5 "onClick.*navigate.*reload" src/components/ErrorPages/Repository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
rg -A 3 -B 3 "window.location.replace" src/Repository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
rg -B 2 -A 5 "navigate\(" src/components/ErrorPages/ | head -30Repository: ohcnetwork/care_fe
Length of output: 831
🏁 Script executed:
rg "useRedirect|navigate.*replace" src/ -A 2 -B 2 | head -40Repository: ohcnetwork/care_fe
Length of output: 899
🏁 Script executed:
fd -e "ts" -e "tsx" src/ -x grep -l "raviger" | head -5 | xargs -I {} sh -c 'echo "File: {}"; grep -A 2 -B 2 "navigate\(" {} | head -15'Repository: ohcnetwork/care_fe
Length of output: 383
🏁 Script executed:
rg "PAGE_LOAD_ERROR|PAGE_NOT_FOUND" src/ -A 5 -B 5 | head -50Repository: ohcnetwork/care_fe
Length of output: 3185
🏁 Script executed:
rg "window.location.reload" src/ -B 3 -A 1Repository: ohcnetwork/care_fe
Length of output: 996
Simplify recovery navigation to avoid redundant calls.
The navigate("/") call is unnecessary before window.location.reload(). Use navigate("/", { replace: true }) to handle the redirect without triggering an additional SPA navigation that gets replaced by the full page reload.
💡 Suggested fix
onClick={() => {
- navigate("/");
- window.location.reload();
+ navigate("/", { replace: true });
}}📝 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.
Suggested change
| onClick={() => { | |
| goBack("/"); | |
| navigate("/"); | |
| window.location.reload(); | |
| }} | |
| onClick={() => { | |
| navigate("/", { replace: true }); | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ErrorPages/DefaultErrorPage.tsx` around lines 177 - 180, The
onClick handler in DefaultErrorPage.tsx currently calls navigate("/") and then
window.location.reload(), which is redundant; change the navigate call to
navigate("/", { replace: true }) to avoid pushing a history entry before the
full-page reload (keep the subsequent window.location.reload() if you still want
a hard reload), i.e., update the onClick lambda that references navigate and
window.location.reload accordingly.
| if (!canViewPatients) { | ||
| toast.error(t("no_permission_to_view_page")); | ||
| goBack(`/facility/${facilityId}/patient/${patientId}`); | ||
| navigate(`/facility/${facilityId}/patient/${patientId}`); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether PatientProps allows missing facilityId and which patient routes exist.
echo "== PatientProps definition =="
fd "PatientDetailsTab.tsx" src | xargs -I{} rg -n -C3 "interface PatientProps|facilityId\\??|facilityId:" {}
echo
echo "== Patient history route patterns in router files =="
fd ".*Routes\\.tsx$" src/Routers/routes | xargs -I{} rg -n -C2 "/facility/:facilityId/patient/:patientId|/patient/:patientId" {}Repository: ohcnetwork/care_fe
Length of output: 7397
🏁 Script executed:
#!/bin/bash
# Locate and read EncounterHistory.tsx
echo "== Finding EncounterHistory.tsx =="
fd "EncounterHistory.tsx" src/
echo
echo "== Reading EncounterHistory.tsx (full file) =="
fd "EncounterHistory.tsx" src/ | head -1 | xargs catRepository: ohcnetwork/care_fe
Length of output: 5700
🏁 Script executed:
#!/bin/bash
# Search for PatientProps interface definition
echo "== PatientProps definition =="
rg -n "interface PatientProps|type PatientProps" src/components/Patient/PatientDetailsTab/ -A10
echo
echo "== facilityId usage in EncounterHistory context =="
rg -n "facilityId" src/components/Patient/PatientDetailsTab/EncounterHistory.tsx -B2 -A2Repository: ohcnetwork/care_fe
Length of output: 2049
🏁 Script executed:
#!/bin/bash
# Verify the patient-scoped route exists and is used in ClinicalHistoryPage
echo "== Search for /patient/:patientId/history pattern =="
rg "/patient/:patientId/history" src/Routers/routes -B1 -A3
echo
echo "== Verify ClinicalHistoryPage accepts patientId without facilityId =="
rg -n "ClinicalHistoryPage" src/Routers/routes -B2 -A8 | head -30Repository: ohcnetwork/care_fe
Length of output: 4385
Guard unauthorized redirect to handle missing facilityId.
Line 57 unconditionally navigates to a facility-scoped URL, but facilityId is optional in PatientProps. When absent, this produces /facility/undefined/patient/${patientId}. Add a patient-scoped fallback to match the existing route pattern.
💡 Suggested fix
- navigate(`/facility/${facilityId}/patient/${patientId}`);
+ navigate(
+ facilityId
+ ? `/facility/${facilityId}/patient/${patientId}`
+ : `/patient/${patientId}`,
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Patient/PatientDetailsTab/EncounterHistory.tsx` at line 57,
The redirect unconditionally uses facilityId which may be undefined, causing
URLs like /facility/undefined/patient/:patientId; update the navigation logic in
EncounterHistory (where navigate(...) is called) to check facilityId from
PatientProps and, if present, call
navigate(`/facility/${facilityId}/patient/${patientId}`), otherwise fallback to
the patient-scoped route such as navigate(`/patient/${patientId}`); ensure you
reference the facilityId and patientId variables from the component props and
adjust any types if necessary.
| | "alert" | ||
| | "warning" | ||
| | "outline_primary"; | ||
| export type ButtonSize = "default" | "xs" | "sm" | "md" | "lg" | "icon"; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Derive ButtonSize from buttonVariants to avoid future type drift.
Line 65 duplicates size literals already defined in CVA. Prefer deriving the exported type from VariantProps so additions/removals stay in sync automatically.
♻️ Suggested refactor
-export type ButtonSize = "default" | "xs" | "sm" | "md" | "lg" | "icon";
+export type ButtonSize = NonNullable<
+ VariantProps<typeof buttonVariants>["size"]
+>;📝 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.
Suggested change
| export type ButtonSize = "default" | "xs" | "sm" | "md" | "lg" | "icon"; | |
| export type ButtonSize = NonNullable< | |
| VariantProps<typeof buttonVariants>["size"] | |
| >; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/button.tsx` at line 65, The exported ButtonSize type is
duplicating the size literals from the CVA config and can drift; instead derive
it from the buttonVariants definition by importing VariantProps from
"class-variance-authority" and setting ButtonSize = VariantProps<typeof
buttonVariants>["size"] (replace the current literal union), ensuring any future
changes to buttonVariants automatically update the exported type; update any
imports/usages accordingly to use the derived type.
Comment on lines
+8
to
+27
| export const canAddChargeItemsToAccount = ( | ||
| account: AccountRead | AccountBase | undefined, | ||
| ) => { | ||
| if (!account) return false; | ||
| return ( | ||
| account.status === AccountStatus.active && !isAccountBillingClosed(account) | ||
| ); | ||
| }; | ||
|
|
||
| export const isAccountBillingClosed = ( | ||
| account: AccountRead | AccountBase | undefined, | ||
| ) => { | ||
| if (!account) return false; | ||
| return [ | ||
| AccountBillingStatus.closed_baddebt, | ||
| AccountBillingStatus.closed_voided, | ||
| AccountBillingStatus.closed_completed, | ||
| AccountBillingStatus.closed_combined, | ||
| ].includes(account.billing_status); | ||
| }; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use function declarations for these pure utilities.
Please switch from arrow-const exports to function declarations for consistency with repo rules.
♻️ Proposed refactor
-export const canAddChargeItemsToAccount = (
- account: AccountRead | AccountBase | undefined,
-) => {
+export function canAddChargeItemsToAccount(
+ account: AccountRead | AccountBase | undefined,
+): boolean {
if (!account) return false;
return (
account.status === AccountStatus.active && !isAccountBillingClosed(account)
);
-};
+}
-export const isAccountBillingClosed = (
- account: AccountRead | AccountBase | undefined,
-) => {
+export function isAccountBillingClosed(
+ account: AccountRead | AccountBase | undefined,
+): boolean {
if (!account) return false;
return [
AccountBillingStatus.closed_baddebt,
AccountBillingStatus.closed_voided,
AccountBillingStatus.closed_completed,
AccountBillingStatus.closed_combined,
].includes(account.billing_status);
-};
+}As per coding guidelines, “Use the function keyword for pure functions.”
📝 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.
Suggested change
| export const canAddChargeItemsToAccount = ( | |
| account: AccountRead | AccountBase | undefined, | |
| ) => { | |
| if (!account) return false; | |
| return ( | |
| account.status === AccountStatus.active && !isAccountBillingClosed(account) | |
| ); | |
| }; | |
| export const isAccountBillingClosed = ( | |
| account: AccountRead | AccountBase | undefined, | |
| ) => { | |
| if (!account) return false; | |
| return [ | |
| AccountBillingStatus.closed_baddebt, | |
| AccountBillingStatus.closed_voided, | |
| AccountBillingStatus.closed_completed, | |
| AccountBillingStatus.closed_combined, | |
| ].includes(account.billing_status); | |
| }; | |
| export function canAddChargeItemsToAccount( | |
| account: AccountRead | AccountBase | undefined, | |
| ): boolean { | |
| if (!account) return false; | |
| return ( | |
| account.status === AccountStatus.active && !isAccountBillingClosed(account) | |
| ); | |
| } | |
| export function isAccountBillingClosed( | |
| account: AccountRead | AccountBase | undefined, | |
| ): boolean { | |
| if (!account) return false; | |
| return [ | |
| AccountBillingStatus.closed_baddebt, | |
| AccountBillingStatus.closed_voided, | |
| AccountBillingStatus.closed_completed, | |
| AccountBillingStatus.closed_combined, | |
| ].includes(account.billing_status); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Facility/billing/account/utils.ts` around lines 8 - 27, Replace the
exported arrow-const utilities with named function declarations: change the
const-exported canAddChargeItemsToAccount and isAccountBillingClosed into
exported functions using the function keyword, preserving their parameter
signature (account: AccountRead | AccountBase | undefined) and the existing
logic (checking account falsiness, AccountStatus.active, and the billing_status
includes array). Ensure the exported names remain identical and update any
export style if necessary so callers continue to import
canAddChargeItemsToAccount and isAccountBillingClosed as before.
Comment on lines
+428
to
+446
| <Button | ||
| size="icon" | ||
| variant="outline" | ||
| className="shrink-0" | ||
| to={getInventoryBasePath( | ||
| facilityId, | ||
| locationId, | ||
| internal, | ||
| false, | ||
| isRequester, | ||
| "", | ||
| )} | ||
| onClick={() => | ||
| navigate( | ||
| getInventoryBasePath( | ||
| facilityId, | ||
| locationId, | ||
| internal, | ||
| false, | ||
| isRequester, | ||
| "", | ||
| ), | ||
| ) | ||
| } | ||
| > | ||
| <ChevronLeft /> | ||
| </BackButton> | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Back action is re-pushing a route and can reintroduce navigation loops.
Line 432 uses navigate(...) for a back control, which pushes a new entry instead of going back. This can recreate the same loop pattern this PR is trying to eliminate. Also, the icon-only control has no accessible label.
💡 Suggested fix
- <Button
+ <Button
size="icon"
variant="outline"
className="shrink-0"
- onClick={() =>
- navigate(
- getInventoryBasePath(
- facilityId,
- locationId,
- internal,
- false,
- isRequester,
- "",
- ),
- )
- }
+ onClick={() => history.back()}
+ aria-label={t("back")}
>
<ChevronLeft />
</Button>Based on learnings: In the care_fe codebase, using BackButton component without an explicit to prop (relying on browser history navigation) is the expected and preferred behavior, as confirmed by abhimanyurajeesh.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/pages/Facility/services/inventory/externalSupply/deliveryOrder/DeliveryOrderShow.tsx`
around lines 428 - 446, Replace the icon-only Button that calls
navigate(getInventoryBasePath(...)) with the existing BackButton component so
the control uses browser history back behavior (avoid re-pushing routes and
navigation loops); ensure you remove the navigate(...) call and pass no explicit
"to" prop, and if the BackButton component requires it or you must keep an
icon-only control, add an accessible label (aria-label or visually-hidden text)
to the control to satisfy accessibility for the icon-only back action in
DeliveryOrderShow (replace usages of Button, navigate, and getInventoryBasePath
with BackButton).
| : returnPath, | ||
| ) | ||
| } | ||
| onClick={() => history.back()} |
Contributor
There was a problem hiding this comment.
Add a safe fallback for history.back() on cancel.
Line 558 can navigate users out of the app (or do nothing) when the form is opened directly/new tab. Prefer browser-back first, but fallback to the known in-app route.
Suggested patch
- onClick={() => history.back()}
+ onClick={() => {
+ if (window.history.length > 1) {
+ window.history.back();
+ return;
+ }
+ navigate(returnPath, { replace: true });
+ }}As per coding guidelines: Use navigate and useRedirect from raviger for navigation and redirects.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/pages/Facility/services/inventory/externalSupply/requestOrder/RequestOrderForm.tsx`
at line 558, The cancel handler currently calls history.back() directly in
RequestOrderForm which can navigate out of the app or do nothing if the form was
opened in a new tab; replace this with a safe redirect: import navigate and
useRedirect from raviger, get a redirect function (e.g., const redirect =
useRedirect()) or call navigate directly, then update the onClick for the cancel
control to attempt history.back() first in a try/catch and if that fails (or if
document.referrer is empty) call redirect or navigate to the known in‑app route
(the facility inventory list route used elsewhere in RequestOrderForm) so
cancelling always returns the user to the app.
| onSuccess={() => { | ||
| toast.success(t("device_updated")); | ||
| goBack(`/facility/${facilityId}/settings/devices/${device.id}`); | ||
| history.back(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -type f -name "UpdateDevice.tsx" 2>/dev/null | head -5Repository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
git ls-files | grep -i "updatedevice"Repository: ohcnetwork/care_fe
Length of output: 114
🏁 Script executed:
cat -n src/pages/Facility/settings/devices/UpdateDevice.tsx | head -80Repository: ohcnetwork/care_fe
Length of output: 2546
🏁 Script executed:
# Check for imports and the actual onSuccess callback
sed -n '1,60p' src/pages/Facility/settings/devices/UpdateDevice.tsxRepository: ohcnetwork/care_fe
Length of output: 2024
🏁 Script executed:
# Look for similar post-save navigation patterns in the codebase
rg "onSuccess.*history\.back" --type tsx -A 2 -B 2Repository: ohcnetwork/care_fe
Length of output: 89
🏁 Script executed:
# Check how raviger navigation is used in similar device/facility settings pages
rg "navigate.*facilityId.*settings" --type tsx -A 2 -B 2 | head -40Repository: ohcnetwork/care_fe
Length of output: 89
🏁 Script executed:
# Check if there are BackButton usages without explicit `to` prop
rg "BackButton" --type tsx -A 1 -B 1 | grep -A 2 -B 2 "BackButton[^/]" | head -30Repository: ohcnetwork/care_fe
Length of output: 89
🏁 Script executed:
# Try searching for similar onSuccess patterns with correct type specification
rg "onSuccess" --type=ts --type=tsx -A 3 | grep -A 3 "history\.back" | head -30Repository: ohcnetwork/care_fe
Length of output: 89
🏁 Script executed:
# Check how settings pages handle post-save navigation
find src/pages/Facility/settings -name "*.tsx" -exec grep -l "onSuccess" {} \; | head -5Repository: ohcnetwork/care_fe
Length of output: 1817
🏁 Script executed:
# Look at one of these settings pages to see the navigation pattern
find src/pages/Facility/settings -name "*.tsx" -type f | head -3 | xargs grep -A 5 "onSuccess"Repository: ohcnetwork/care_fe
Length of output: 3646
🏁 Script executed:
# Check DeviceForm to understand what happens there
cat -n src/pages/Facility/settings/devices/components/DeviceForm.tsx | head -100Repository: ohcnetwork/care_fe
Length of output: 3932
🏁 Script executed:
# Check if HistoryAPIProvider is actually used and how it works
rg "HistoryAPIProvider" --type=ts --type=tsx -B 2 -A 5Repository: ohcnetwork/care_fe
Length of output: 89
🏁 Script executed:
# Search for navigate usage in similar settings pages with simpler pattern
find src/pages/Facility/settings -name "*.tsx" | xargs grep "navigate(" | head -10Repository: ohcnetwork/care_fe
Length of output: 1195
🏁 Script executed:
# Check how post-save navigation is handled in ActivityDefinitionForm
sed -n '1,150p' src/pages/Facility/settings/activityDefinition/ActivityDefinitionForm.tsx | grep -A 10 "navigate.*settings"Repository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
# Get more context on ActivityDefinitionForm's onSuccess callback
rg "onSuccess.*=>" src/pages/Facility/settings/activityDefinition/ActivityDefinitionForm.tsx -A 15 | head -40Repository: ohcnetwork/care_fe
Length of output: 1456
🏁 Script executed:
# Check if navigate is imported in ActivityDefinitionForm
head -20 src/pages/Facility/settings/activityDefinition/ActivityDefinitionForm.tsxRepository: ohcnetwork/care_fe
Length of output: 676
🏁 Script executed:
# Verify what happens after device update - check if BackButton is used elsewhere and how
grep -r "DeviceForm" src/pages/Facility/settings/devices/ --include="*.tsx" -B 2 -A 2Repository: ohcnetwork/care_fe
Length of output: 2224
🏁 Script executed:
# Check if there's a device list page that would be the "back" destination
find src/pages/Facility/settings/devices -name "*.tsx" | grep -v componentRepository: ohcnetwork/care_fe
Length of output: 333
🏁 Script executed:
# Look at the device list/index page to understand the expected flow
ls -la src/pages/Facility/settings/devices/Repository: ohcnetwork/care_fe
Length of output: 643
🏁 Script executed:
# Check CreateDevice.tsx to see if it also uses history.back()
cat -n src/pages/Facility/settings/devices/CreateDevice.tsxRepository: ohcnetwork/care_fe
Length of output: 1571
🏁 Script executed:
# Check DevicesList to understand the device listing/viewing structure
cat -n src/pages/Facility/settings/devices/DevicesList.tsx | head -60Repository: ohcnetwork/care_fe
Length of output: 2377
🏁 Script executed:
# Check if there are route definitions for devices
grep -r "devices" src/Routers/routes/ --include="*.tsx" -A 2 -B 2 | head -40Repository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
# Verify if HistoryAPIProvider is properly set up in the app
grep -r "HistoryAPIProvider" src/ --include="*.tsx" --include="*.ts" -B 2 -A 2Repository: ohcnetwork/care_fe
Length of output: 441
🏁 Script executed:
# Check if there's a back button or navigation guard that handles this scenario
grep -r "history\.back" src/ --include="*.tsx" --include="*.ts" | wc -lRepository: ohcnetwork/care_fe
Length of output: 64
🏁 Script executed:
# Get a list of all usages of history.back() to see if this is a widespread pattern
grep -r "history\.back" src/pages/ --include="*.tsx"Repository: ohcnetwork/care_fe
Length of output: 730
🏁 Script executed:
# Check DeviceShow to see if it's a typical "back" destination
sed -n '1,50p' src/pages/Facility/settings/devices/DeviceShow.tsxRepository: ohcnetwork/care_fe
Length of output: 1937
🏁 Script executed:
# Look for route definitions to understand the navigation flow
grep -r "UpdateDevice\|CreateDevice\|DeviceShow\|DevicesList" src/Routers/routes/ --include="*.tsx" -B 2 -A 2 | head -50Repository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
# Check if window.history.length is used anywhere else in the codebase
grep -r "window\.history\.length\|history\.length" src/ --include="*.tsx" --include="*.ts"Repository: ohcnetwork/care_fe
Length of output: 321
🏁 Script executed:
# Check FacilityRoutes to understand the device page routes
grep -A 10 -B 2 "Device" src/Routers/routes/FacilityRoutes.tsx | head -50Repository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
# Verify how other similar form pages handle post-success navigation
grep -A 5 "onSuccess.*=>" src/pages/Facility/settings/product/ProductForm.tsxRepository: ohcnetwork/care_fe
Length of output: 1115
🏁 Script executed:
# Check ChargeItemDefinitionForm to see if it uses history.back too
grep -B 5 -A 5 "history\.back" src/pages/Facility/settings/chargeItemDefinitions/ChargeItemDefinitionForm.tsxRepository: ohcnetwork/care_fe
Length of output: 262
🏁 Script executed:
# Check the full context of ProductForm's onSuccess to see the pattern difference
grep -B 10 -A 10 "onSuccess.*product.*=>" src/pages/Facility/settings/product/ProductForm.tsx | grep -A 10 "onSuccess"Repository: ohcnetwork/care_fe
Length of output: 1812
🏁 Script executed:
# Look at ActivityDefinitionForm's complete onSuccess logic
sed -n '60,120p' src/pages/Facility/settings/activityDefinition/ActivityDefinitionForm.tsxRepository: ohcnetwork/care_fe
Length of output: 1950
🏁 Script executed:
# Check if CreateDevice and UpdateDevice should navigate to a detail page
sed -n '50,70p' src/pages/Facility/settings/devices/UpdateDevice.tsxRepository: ohcnetwork/care_fe
Length of output: 572
🏁 Script executed:
# Verify DeviceShow route and typical post-device-update destination
grep -A 20 "function DeviceShow" src/pages/Facility/settings/devices/DeviceShow.tsx | head -25Repository: ohcnetwork/care_fe
Length of output: 826
🏁 Script executed:
# Check if there's a convention for post-create device flow
grep -B 5 -A 10 "onSuccess.*navigate.*device" src/pages/Facility/settings/devices/ -rRepository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
# Verify what happens after device creation - does it need navigation to detail or list?
sed -n '30,45p' src/pages/Facility/settings/devices/CreateDevice.tsxRepository: ohcnetwork/care_fe
Length of output: 341
🏁 Script executed:
# Check if the BackButton learning applies to this scenario by looking at BackButton usage
grep -A 2 "BackButton" src/pages/Facility/billing/account/AccountShow.tsx | head -10Repository: ohcnetwork/care_fe
Length of output: 533
🏁 Script executed:
# Check what the DeviceForm mutation returns to see if we have device ID for navigation
sed -n '100,200p' src/pages/Facility/settings/devices/components/DeviceForm.tsx | grep -A 20 "mutationFn"Repository: ohcnetwork/care_fe
Length of output: 722
🏁 Script executed:
# Check the complete onSuccess handler in DeviceForm
sed -n '130,180p' src/pages/Facility/settings/devices/components/DeviceForm.tsxRepository: ohcnetwork/care_fe
Length of output: 1730
🏁 Script executed:
# Verify the device API response to see if we get the ID back
grep -A 10 "export.*mutateCreateDevice\|export.*mutateUpdateDevice" src/types/device/deviceApi.tsRepository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
# Check if device ID is available in UpdateDevice component
sed -n '20,70p' src/pages/Facility/settings/devices/UpdateDevice.tsxRepository: ohcnetwork/care_fe
Length of output: 1501
🏁 Script executed:
# Check if there's any issue with accessing device.id in the context
sed -n '15,30p' src/pages/Facility/settings/devices/UpdateDevice.tsxRepository: ohcnetwork/care_fe
Length of output: 503
🏁 Script executed:
# Verify CreateDevice doesn't have device ID available
sed -n '10,20p' src/pages/Facility/settings/devices/CreateDevice.tsxRepository: ohcnetwork/care_fe
Length of output: 320
🏁 Script executed:
# Check if DeviceForm passes back the device ID or data
sed -n '160,180p' src/pages/Facility/settings/devices/components/DeviceForm.tsxRepository: ohcnetwork/care_fe
Length of output: 987
🏁 Script executed:
# Look at how ProductForm handles the onSuccess callback - does it get device data?
grep -A 20 "const { mutate: submitForm" src/pages/Facility/settings/product/ProductForm.tsx | head -25Repository: ohcnetwork/care_fe
Length of output: 44
Post-save navigation should navigate explicitly rather than rely solely on browser history.
Line 52 uses history.back() without a fallback route. For better UX consistency with similar form pages in the codebase (e.g., ProductForm, ActivityDefinitionForm), use explicit navigation:
For UpdateDevice, navigate to the device detail page since deviceId is available. For CreateDevice, either navigate to the devices list or refactor DeviceForm to return the created device data so you can navigate to its detail page.
See ProductForm and ActivityDefinitionForm for the preferred pattern of using navigate() with replace: true.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Facility/settings/devices/UpdateDevice.tsx` at line 52, Replace the
unreliable history.back() call in UpdateDevice with an explicit navigate to the
updated device's detail route: use the existing deviceId (from props or state in
UpdateDevice) and call navigate(`/devices/${deviceId}`, { replace: true }) so
the app behavior matches patterns used in ProductForm and
ActivityDefinitionForm; if CreateDevice needs similar handling, either navigate
to the devices list or refactor DeviceForm to return the created device data so
you can call navigate to `/devices/${newDeviceId}` with replace: true.
🎭 Playwright Test ResultsStatus: ❌ Failed
📊 Detailed results are available in the playwright-final-report artifact. Run: #7460 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Common/BackButton.tsx`:
- Around line 19-26: The BackButton component currently spreads {...props} after
setting type="button" on the Button, allowing callers to override the button
type; update BackButton so the explicit type cannot be overridden by either (a)
move the {...props} spread before the explicit props so type="button" takes
precedence on the Button, or (b) remove/omit "type" from BackButtonProps (e.g.
use Omit<BackButtonProps, "type">) so callers cannot pass a type prop; ensure
the component (BackButton) still passes variant, onClick, className, size,
disabled to the Button and that Button.type remains "button".
🪄 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: ae71fc17-757f-48fc-97c1-d71f53cdab7f
📒 Files selected for processing (2)
src/Providers/HistoryAPIProvider.tsxsrc/components/Common/BackButton.tsx
💤 Files with no reviewable changes (1)
- src/Providers/HistoryAPIProvider.tsx
Comment on lines
+19
to
+26
| <Button | ||
| type="button" | ||
| variant={variant} | ||
| onClick={() => history.back()} | ||
| className={className} | ||
| size={size} | ||
| disabled={disabled} | ||
| {...props} |
Contributor
There was a problem hiding this comment.
Prop spread order allows type to be overridden.
The {...props} spread at Line 26 comes after type="button" at Line 20. Since type is not omitted from BackButtonProps, a caller could pass type="submit", which would override type="button" and potentially cause unintended form submissions.
🛡️ Proposed fix: spread props before explicit values or omit `type`
Option 1 - Spread props first so explicit values take precedence:
<Button
+ {...props}
type="button"
variant={variant}
onClick={() => history.back()}
className={className}
size={size}
disabled={disabled}
- {...props}
>Option 2 - Omit type from allowed props:
-} & Omit<React.ComponentProps<"button">, "onClick">;
+} & Omit<React.ComponentProps<"button">, "onClick" | "type">;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Common/BackButton.tsx` around lines 19 - 26, The BackButton
component currently spreads {...props} after setting type="button" on the
Button, allowing callers to override the button type; update BackButton so the
explicit type cannot be overridden by either (a) move the {...props} spread
before the explicit props so type="button" takes precedence on the Button, or
(b) remove/omit "type" from BackButtonProps (e.g. use Omit<BackButtonProps,
"type">) so callers cannot pass a type prop; ensure the component (BackButton)
still passes variant, onClick, className, size, disabled to the Button and that
Button.type remains "button".
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the custom in-app history stack (HistoryAPIProvider / useAppHistory) and shifts navigation behavior to rely on browser history (plus navigate(..., { replace: true }) in several flows) to avoid navigation loops.
Changes:
- Removed
HistoryAPIProvideranduseAppHistory, and refactored many call sites to useBackButton,history.back(), ornavigate. - Updated
BackButtonimplementation to trigger browser back navigation and introduced aButtonSizetype for consistent sizing. - Bumped
ravigerto^5.3.0.
Reviewed changes
Copilot reviewed 52 out of 53 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/App.tsx | Removes History provider wrapper from app composition. |
| src/Providers/HistoryAPIProvider.tsx | Deletes custom history tracking provider. |
| src/hooks/useAppHistory.ts | Deletes custom history hook. |
| src/components/Common/BackButton.tsx | Refactors BackButton to use history.back() and button props. |
| src/components/ui/button.tsx | Exports ButtonSize type to support sizing in wrappers. |
| src/pages/PublicAppointments/auth/PatientLogin.tsx | Replaces goBack usage with navigate(...) for back action. |
| src/pages/PublicAppointments/Schedule.tsx | Replaces manual back button with BackButton. |
| src/pages/Patient/PatientHome.tsx | Replaces goBack button with BackButton. |
| src/pages/Facility/settings/productKnowledge/ProductKnowledgeView.tsx | Removes explicit to on BackButton and relies on back navigation. |
| src/pages/Facility/settings/product/ProductForm.tsx | Uses replace: true on success; uses history.back() on cancel. |
| src/pages/Facility/settings/healthcareService/HealthcareServiceForm.tsx | Uses replace: true after create/update navigations. |
| src/pages/Facility/settings/devices/components/DeviceForm.tsx | Replaces custom back logic with BackButton. |
| src/pages/Facility/settings/devices/UpdateDevice.tsx | Uses history.back() after successful update. |
| src/pages/Facility/settings/devices/CreateDevice.tsx | Uses history.back() after successful create. |
| src/pages/Facility/settings/chargeItemDefinitions/UpdateChargeItemDefinition.tsx | Uses replace: true on success navigation. |
| src/pages/Facility/settings/chargeItemDefinitions/ChargeItemDefinitionForm.tsx | Uses replace: true on success; history.back() on cancel. |
| src/pages/Facility/settings/chargeItemDefinitions/ChargeItemDefinitionDetail.tsx | Uses replace: true on delete navigation; removes BackButton fallback. |
| src/pages/Facility/settings/activityDefinition/ActivityDefinitionView.tsx | Uses replace: true and removes explicit BackButton destinations. |
| src/pages/Facility/settings/activityDefinition/ActivityDefinitionForm.tsx | Uses replace: true and swaps cancel button to BackButton. |
| src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx | Swaps outline back button to BackButton. |
| src/pages/Facility/services/pharmacy/MedicationReturnShow.tsx | Removes to from BackButton (now browser back). |
| src/pages/Facility/services/inventory/externalSupply/requestOrder/RequestOrderForm.tsx | Uses replace: true on success; history.back() on cancel. |
| src/pages/Facility/services/inventory/externalSupply/deliveryOrder/DeliveryOrderShow.tsx | Replaces BackButton with explicit navigate(...) button. |
| src/pages/Facility/services/inventory/externalSupply/deliveryOrder/DeliveryOrderForm.tsx | Uses replace: true on navigation. |
| src/pages/Facility/services/HealthcareServiceShow.tsx | Removes explicit BackButton to destination. |
| src/pages/Facility/queues/ManageQueue.tsx | Removes computed to destination for queue back navigation. |
| src/pages/Facility/billing/paymentReconciliation/PaymentReconciliationShow.tsx | Swaps goBack button to BackButton. |
| src/pages/Facility/billing/invoice/InvoiceShow.tsx | Removes explicit BackButton to destination. |
| src/pages/Facility/billing/account/components/PrintChargeItems.tsx | Swaps goBack to BackButton for “go back” action. |
| src/pages/Facility/billing/account/CreateInvoice.tsx | Uses replace: true on invoice navigation; refactors cancel/back handling. |
| src/pages/Facility/FacilityDetailsPage.tsx | Swaps goBack buttons to BackButton. |
| src/pages/Encounters/ReportViewer.tsx | Removes fallback URL on BackButton; route params reduced. |
| src/Routers/routes/ConsultationRoutes.tsx | Updates ReportViewer route bindings to match new props. |
| src/pages/Encounters/EncounterShow.tsx | Replaces permission-denied goBack with navigate. |
| src/pages/Appointments/AppointmentsPage.tsx | Replaces permission-denied goBack with navigate. |
| src/pages/Appointments/AppointmentDetail.tsx | Adjusts BackButton usage in header. |
| src/pages/Admin/TagConfig/TagConfigView.tsx | Swaps back button to BackButton. |
| src/components/ValueSet/ValueSetForm.tsx | Swaps cancel behavior to BackButton. |
| src/components/Users/UserHome.tsx | Replaces goBack with navigate in error path. |
| src/components/Users/UserDeleteDialog.tsx | Replaces goBack with navigate after delete. |
| src/components/Resource/ResourceList.tsx | Removes stray whitespace line. |
| src/components/Resource/ResourceForm.tsx | Uses replace: true on success; swaps cancel to BackButton. |
| src/components/Questionnaire/QuestionnaireForm.tsx | Removes onCancel prop and uses BackButton for cancel. |
| src/components/Patient/PatientRegistration.tsx | Uses replace: true for query updates; uses history.back() on update success. |
| src/components/Patient/PatientDetailsTab/patientUpdates.tsx | Replaces goBack with navigate on permission failure. |
| src/components/Patient/PatientDetailsTab/EncounterHistory.tsx | Replaces goBack with navigate on permission failure. |
| src/components/Patient/EncounterQuestionnaire.tsx | Removes onCancel wiring after QuestionnaireForm API change. |
| src/components/Facility/FacilityDeleteDialog.tsx | Simplifies post-delete navigation to navigate("/"). |
| src/components/ErrorPages/DefaultErrorPage.tsx | Replaces goBack with navigate("/") before reload. |
| src/components/Common/Drawings/ExcalidrawEditor.tsx | Uses history.back() for cancel/discard flows. |
| src/CAREUI/misc/PrintPreview.tsx | Uses BackButton instead of custom history hook. |
| package.json | Updates raviger dependency to ^5.3.0. |
| package-lock.json | Locks raviger to 5.3.0. |
Comments suppressed due to low confidence (3)
src/pages/Facility/queues/ManageQueue.tsx:131
- Back navigation here used to be a deterministic link back to the queue index (including date/resource query params). After removing the
toprop, this BackButton will justhistory.back(), which can fail for deep-linked/new-tab opens and won’t preserve the intended return URL. Consider passing an explicit destination (or using a BackButton API that supports a fallback/to) so this always returns to the correct queue list.
src/pages/Facility/queues/ManageQueue.tsx:131 - This BackButton is icon-only and currently has no
aria-label, so assistive tech won’t have an accessible name for it. Please addaria-label={t("back")}(or similar).
src/pages/PublicAppointments/auth/PatientLogin.tsx:248 - This back button now uses
navigate(...)(push) instead of going back in history. On flows where the previous page is already in the browser history, pushing a new entry can create a back/forward loop (facility → OTP send → back pushes facility → browser back returns to OTP send). Consider usinghistory.back()/BackButton for true back navigation, ornavigate(..., { replace: true })when you need a deterministic target without growing the history stack.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
100
to
102
| onCancel = () => { | ||
| if (categorySlug) { | ||
| navigate( | ||
| `/facility/${facilityId}/settings/charge_item_definitions/categories/${categorySlug}`, | ||
| ); | ||
| } else { | ||
| navigate(`/facility/${facilityId}/settings/charge_item_definitions`); | ||
| } | ||
| history.back(); | ||
| }, |
Comment on lines
+19
to
+26
| <Button | ||
| type="button" | ||
| variant={variant} | ||
| onClick={() => history.back()} | ||
| className={className} | ||
| size={size} | ||
| disabled={disabled} | ||
| {...props} |
Comment on lines
+20
to
+26
| type="button" | ||
| variant={variant} | ||
| onClick={() => history.back()} | ||
| className={className} | ||
| size={size} | ||
| disabled={disabled} | ||
| {...props} |
Comment on lines
+19
to
+26
| <Button | ||
| type="button" | ||
| variant={variant} | ||
| onClick={() => history.back()} | ||
| className={className} | ||
| size={size} | ||
| disabled={disabled} | ||
| {...props} |
Comment on lines
+333
to
335
| <BackButton size="icon" className="shrink-0"> | ||
| <ChevronLeft /> | ||
| </BackButton> |
Comment on lines
128
to
143
| <BackButton> | ||
| <ArrowLeft /> | ||
| {t("back_to_list")} | ||
| </BackButton> | ||
| </div> | ||
| </Page> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <Page title={product.name} hideTitleOnPage={true}> | ||
| <div className="container mx-auto max-w-3xl space-y-6"> | ||
| <BackButton | ||
| to={`/facility/${facilityId}/settings/product_knowledge/categories/${product.category.slug}`} | ||
| > | ||
| <BackButton> | ||
| <ArrowLeft /> | ||
| {t("back")} | ||
| </BackButton> |
Comment on lines
+384
to
387
| <BackButton> | ||
| <ChevronLeft /> {t("back")} | ||
| </BackButton> | ||
| } |
Comment on lines
146
to
152
| onSuccess = (product: ProductRead) => | ||
| navigate(`/facility/${facilityId}/settings/product/${product.id}`), | ||
| navigate(`/facility/${facilityId}/settings/product/${product.id}`, { | ||
| replace: true, | ||
| }), | ||
| onCancel = () => { | ||
| if (productId) { | ||
| navigate(`/facility/${facilityId}/settings/product/${productId}`); | ||
| } else { | ||
| navigate(`/facility/${facilityId}/settings/product`); | ||
| } | ||
| history.back(); | ||
| }, |
Comment on lines
+523
to
+529
| <BackButton | ||
| className="w-full flex items-center relative" | ||
| variant="outline" | ||
| onClick={() => | ||
| goBack(`/facility/${facilityId}/billing/payments`) | ||
| } | ||
| data-shortcut-id="go-back" | ||
| > | ||
| <ArrowLeft className="size-4" /> | ||
| {t("back_to_payments")} | ||
| </Button> | ||
| </BackButton> |
| <div className="container mx-auto max-w-7xl mt-4"> | ||
| <div className="flex gap-2 items-center mb-2"> | ||
| <BackButton size="icon" variant="ghost"> | ||
| <BackButton variant="ghost"> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Changes
Fixes #16113
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
Refactor