Fix navigation loop issue in Billing#16018
Conversation
|
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:
WalkthroughSimplified BackButton usage in the invoice view, removed BackButton from invoice creation and switched to replace navigation for new invoices, and refactored HistoryAPIProvider to handle push/replace/pop actions and adjust the internal history array accordingly. Raviger dependency version bumped. Changes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Deploying care-preview with
|
| Latest commit: |
9305fc7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4b18343a.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://fix-navigation-loop.care-preview-a7w.pages.dev |
There was a problem hiding this comment.
Pull request overview
Fixes a “Back” navigation loop on the Billing Account detail page by making the back destination explicit instead of relying on history.
Changes:
- Set
BackButtondestination onAccountShowto always navigate to the accounts list route.
| return ( | ||
| <div className="space-y-3"> | ||
| <BackButton size="xs"> | ||
| <BackButton size="xs" to={`/facility/${facilityId}/billing/account`}> |
There was a problem hiding this comment.
Hard-coding the BackButton to route drops any query params/filters that were used to reach the accounts list (e.g. /billing/account?patient_filter=... is used elsewhere). This can regress UX by returning to the unfiltered list instead of the prior view; consider deriving the back target from the last non-account-detail entry in app history, or preserving the list search params when available.
| <BackButton size="xs" to={`/facility/${facilityId}/billing/account`}> | |
| <BackButton size="xs"> |
| return ( | ||
| <div className="space-y-3"> | ||
| <BackButton size="xs"> | ||
| <BackButton size="xs" to={`/facility/${facilityId}/billing/account`}> |
There was a problem hiding this comment.
This change fixes a navigation-loop scenario but isn’t covered by the existing Playwright billing specs. Please add an e2e test that reproduces the loop (e.g., switch account tabs then click Back) and asserts Back returns to the accounts list (and, if applicable, preserves list query params).
🎭 Playwright Test ResultsStatus: ❌ Failed
📊 Detailed results are available in the playwright-final-report artifact. Run: #7306 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/Facility/billing/invoice/InvoiceShow.tsx`:
- Line 397: Add a Playwright E2E spec (filename matching **/*.spec.ts) that
exercises the cross-page "from=invoice" navigation created in InvoiceShow (the
link using
`/facility/${facilityId}/billing/account/${invoice.account.id}?from=invoice`):
start on an invoice page, click the link to AccountShow, assert the current URL
contains `from=invoice` and the account id (use page.url().toContain or
expect(page).toHaveURL with toContain), then from AccountShow trigger the "back
to accounts list" action and assert the resulting URL contains the accounts list
path (use `.toContain()` for the query-string tolerant checks). Ensure the spec
uses Playwright's test API and covers the invoice → account → accounts list flow
end-to-end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b5bf0e84-1b89-4576-898f-0457dd5e3dd4
📒 Files selected for processing (2)
src/pages/Facility/billing/account/AccountShow.tsxsrc/pages/Facility/billing/invoice/InvoiceShow.tsx
| variant="link" | ||
| className="px-0 justify-start" | ||
| to={`/facility/${facilityId}/billing/account/${invoice.account.id}`} | ||
| to={`/facility/${facilityId}/billing/account/${invoice.account.id}?from=invoice`} |
There was a problem hiding this comment.
Add a Playwright regression test for this cross-page from=invoice navigation contract.
Line 397 introduces a query-param handshake with AccountShow, but this loop-fix path is still untested in the PR. Please add an E2E covering: invoice back → account page, then account back → accounts list (prefer URL .toContain() checks for query-string-tolerant assertions).
I can draft the Playwright spec for this flow if you want.
Based on learnings: **/*.spec.ts should use Playwright tests, and for dynamic query params URL assertions should prefer .toContain() over strict .toHaveURL().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Facility/billing/invoice/InvoiceShow.tsx` at line 397, Add a
Playwright E2E spec (filename matching **/*.spec.ts) that exercises the
cross-page "from=invoice" navigation created in InvoiceShow (the link using
`/facility/${facilityId}/billing/account/${invoice.account.id}?from=invoice`):
start on an invoice page, click the link to AccountShow, assert the current URL
contains `from=invoice` and the account id (use page.url().toContain or
expect(page).toHaveURL with toContain), then from AccountShow trigger the "back
to accounts list" action and assert the resulting URL contains the accounts list
path (use `.toContain()` for the query-string tolerant checks). Ensure the spec
uses Playwright's test API and covers the invoice → account → accounts list flow
end-to-end.
ohc-project-manager
Bot
added
Work in Progress
and removed
needs testing
needs review
labels
ohc-project-manager
Bot
added
needs testing
needs review
and removed
Work in Progress
labels
| <div className="h-auto w-px bg-gray-300" aria-hidden="true" /> | ||
| {account?.patient && ( | ||
| <> |
There was a problem hiding this comment.
After removing the BackButton, the vertical divider (<div className="h-auto w-px ..." />) becomes the first element in this header row, which will render an orphan separator at the left edge. Either remove the divider when there’s no leading content or replace the BackButton with an alternative control so the layout remains consistent.
| <div className="h-auto w-px bg-gray-300" aria-hidden="true" /> | |
| {account?.patient && ( | |
| <> | |
| {account?.patient && ( | |
| <> | |
| <div className="h-auto w-px bg-gray-300" aria-hidden="true" /> |
| } else { | ||
| onSuccess?.(); | ||
| navigate(invoiceUrl); | ||
| navigate(invoiceUrl, { replace: true }); |
There was a problem hiding this comment.
There are existing Playwright tests covering billing navigation/back-button behavior, but this PR changes history semantics (navigate(..., { replace: true }) and invoice/account navigation) without adding a regression test for the loop described in #16017. Please add a Playwright test that opens an invoice from an account, uses the in-page Back button, and asserts the browser back button does not return to the invoice (i.e., no navigation loop).
|
|
||
| useEffect(() => { | ||
| const original = window.history.replaceState.bind(window.history); //store the original browser replaceState function | ||
| window.history.replaceState = (...args) => { | ||
| isReplaceRef.current = true; | ||
| return original(...args); //call the original browser replaceState function | ||
| }; |
There was a problem hiding this comment.
HistoryAPIProvider monkey-patches window.history.replaceState but never restores it on unmount. In React StrictMode/dev, effects can mount/unmount and remount, and without cleanup you can end up wrapping an already-wrapped function (risking recursion/stack overflow) and leaking the patched global outside the provider’s lifetime. Store the original in a ref, guard against double-patching, and restore replaceState in the effect cleanup.
| useEffect(() => { | |
| const original = window.history.replaceState.bind(window.history); //store the original browser replaceState function | |
| window.history.replaceState = (...args) => { | |
| isReplaceRef.current = true; | |
| return original(...args); //call the original browser replaceState function | |
| }; | |
| const originalReplaceStateRef = useRef<History["replaceState"] | null>(null); | |
| useEffect(() => { | |
| if (typeof window === "undefined" || !window.history) { | |
| return; | |
| } | |
| // Guard against double-patching in case this effect re-runs | |
| if (originalReplaceStateRef.current) { | |
| return; | |
| } | |
| const originalReplaceState = window.history.replaceState; | |
| originalReplaceStateRef.current = originalReplaceState; | |
| window.history.replaceState = function patchedReplaceState( | |
| ...args | |
| ): void { | |
| isReplaceRef.current = true; | |
| return originalReplaceState.apply(this, args); | |
| }; | |
| return () => { | |
| if ( | |
| originalReplaceStateRef.current && | |
| typeof window !== "undefined" && | |
| window.history | |
| ) { | |
| window.history.replaceState = originalReplaceStateRef.current; | |
| originalReplaceStateRef.current = null; | |
| } | |
| }; |
| const newPath = newLocation.fullPath + newLocation.search; | ||
| const isReplace = isReplaceRef.current; | ||
| isReplaceRef.current = false; | ||
|
|
||
| setHistory((history) => { | ||
| if (history.length && newPath === history[0]) | ||
| // Ignore push if navigate to same path (for some weird unknown reasons?) | ||
| if (history.length && newPath === history[0]) { | ||
| return history; | ||
| } | ||
|
|
||
| if (isReplace) { | ||
| return [newPath, ...history.slice(1)]; | ||
| } |
There was a problem hiding this comment.
The isReplaceRef flag is only cleared inside useLocationChange. If window.history.replaceState is called without triggering a route change (or a route change is prevented), the flag will stay true and the next navigation may be incorrectly treated as a replace, corrupting the app’s history stack. Consider resetting the flag immediately after the replace call (e.g., in a microtask) or tracking replace vs push via the router API instead of a global flag.
| to={`/facility/${facilityId}/billing/account/${invoice.account.id}`} | ||
| onClick={() => { | ||
| navigate( | ||
| `/facility/${facilityId}/billing/account/${invoice.account.id}`, |
There was a problem hiding this comment.
The new Back button uses navigate(...) without { replace: true }, which will push the account page onto the history stack and can still reproduce the “Back -> invoice -> Back -> invoice” loop (account -> invoice -> account (push) -> back goes to invoice). Use navigate(target, { replace: true }) or a goBack()/window.history.back()-style navigation that doesn’t add a new entry.
| `/facility/${facilityId}/billing/account/${invoice.account.id}`, | |
| `/facility/${facilityId}/billing/account/${invoice.account.id}`, | |
| { replace: true }, |
| <Button | ||
| variant="link" | ||
| className="px-0 justify-start" | ||
| to={`/facility/${facilityId}/billing/account/${invoice.account.id}`} | ||
| onClick={() => { | ||
| navigate( | ||
| `/facility/${facilityId}/billing/account/${invoice.account.id}`, | ||
| ); | ||
| }} | ||
| > | ||
| <ChevronLeft /> | ||
| <span>{t("back")}</span> | ||
| </BackButton> | ||
| </Button> |
There was a problem hiding this comment.
This change switches the Back control from a link (BackButton renders a Link) to a plain button with onClick navigation. That removes link semantics (open-in-new-tab, copy link address) and may be a regression in accessibility/UX consistency. Consider keeping a link-based control and addressing the loop via { replace: true }/history handling instead.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/pages/Facility/billing/invoice/InvoiceShow.tsx (1)
393-404:⚠️ Potential issue | 🟠 MajorAdd the missing Playwright regression for invoice → account → accounts back flow.
This navigation contract changed here, but there is still no E2E assertion guarding the loop scenario end-to-end.
I can draft the Playwright spec for this flow if you want.
Based on learnings:**/*.spec.tsshould use Playwright tests, and URL assertions should prefer.toContain()when query params may vary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Facility/billing/invoice/InvoiceShow.tsx` around lines 393 - 404, The PR changed the navigation contract in InvoiceShow.tsx (the Button onClick that calls navigate(`/facility/${facilityId}/billing/account/${invoice.account.id}`)), but there's no Playwright E2E test guarding the invoice → account → accounts back flow; add a new Playwright spec (e.g., invoices-account-back.spec.ts under **/*.spec.ts) that: visits an invoice page, clicks the back Button, asserts the URL contains the expected path segments (use expect(page.url()).toContain(...) rather than strict equality to allow query params), navigates through account and then back to accounts to verify the full loop, and include clear selectors for the Button/ChevronLeft used in InvoiceShow.tsx to exercise the exact flow.
🤖 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/pages/Facility/billing/invoice/InvoiceShow.tsx`:
- Around line 397-400: The Back action currently calls
navigate(`/facility/${facilityId}/billing/account/${invoice.account.id}`) which
pushes a new history entry; change the call in the Back handler to use replace
navigation so it replaces the current history entry (i.e., call navigate with
the same target and the options object to set replace: true). Locate the
navigate invocation in InvoiceShow (the Back handler referencing facilityId and
invoice.account.id) and update it to pass the replace flag.
In `@src/Providers/HistoryAPIProvider.tsx`:
- Around line 12-18: The effect in HistoryAPIProvider patches
window.history.replaceState (stored as original) but never restores it, so on
unmount/remount wrappers stack and isReplaceRef behavior breaks; update the
useEffect to return a cleanup that restores window.history.replaceState back to
the saved original (the bound original variable) and also ensure any references
to isReplaceRef remain valid; target the useEffect block that defines original
and assigns window.history.replaceState to revert it on cleanup.
---
Duplicate comments:
In `@src/pages/Facility/billing/invoice/InvoiceShow.tsx`:
- Around line 393-404: The PR changed the navigation contract in InvoiceShow.tsx
(the Button onClick that calls
navigate(`/facility/${facilityId}/billing/account/${invoice.account.id}`)), but
there's no Playwright E2E test guarding the invoice → account → accounts back
flow; add a new Playwright spec (e.g., invoices-account-back.spec.ts under
**/*.spec.ts) that: visits an invoice page, clicks the back Button, asserts the
URL contains the expected path segments (use expect(page.url()).toContain(...)
rather than strict equality to allow query params), navigates through account
and then back to accounts to verify the full loop, and include clear selectors
for the Button/ChevronLeft used in InvoiceShow.tsx to exercise the exact flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7de3fae6-9968-4d75-bad9-6a4f2731a1c7
📒 Files selected for processing (3)
src/Providers/HistoryAPIProvider.tsxsrc/pages/Facility/billing/account/CreateInvoice.tsxsrc/pages/Facility/billing/invoice/InvoiceShow.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Providers/HistoryAPIProvider.tsx (1)
22-45:⚠️ Potential issue | 🟠 MajorAdd regression tests for the new replace/back state machine.
Lines 22-45 change the global navigation-history rules, but this PR still has no specs covering the reported loop. Please add tests for initial load, push, replace, and the “navigate back to
history[1]” path, plus the invoice flow that triggered this fix.Based on learnings:
Test provider state management and updates with comprehensive unit tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/HistoryAPIProvider.tsx` around lines 22 - 45, Add unit tests covering the new replace/back state machine around useLocationChange: write specs that exercise initial load (onInitial:true), a push navigation that prepends a newPath to history, a replace navigation using the isReplaceRef path that replaces history[0] via setHistory, and the “navigate back to history[1]” case where history.slice(1) is returned; also add an end-to-end style test reproducing the invoice flow that previously looped (simulate sequence: initial -> push invoice list -> push invoice detail -> replace/update detail -> navigate back to list) to assert no infinite loop and correct history array mutations. Target the component/provider that holds isReplaceRef and setHistory (HistoryAPIProvider/useLocationChange) and assert state transitions after each navigation step.
♻️ Duplicate comments (1)
src/Providers/HistoryAPIProvider.tsx (1)
14-20:⚠️ Potential issue | 🟠 MajorRestore
window.history.replaceStatein cleanup.Lines 14-20 patch a global browser API but never restore it. If this provider remounts, wrappers stack and stale closures can corrupt
isReplaceRefhandling.🔧 Proposed fix
useEffect(() => { const original = window.history.replaceState.bind(window.history); window.history.replaceState = (...args) => { isReplaceRef.current = true; return original(...args); }; + return () => { + window.history.replaceState = original; + }; }, []);As per coding guidelines:
Implement proper cleanup and memory management in provider components.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/HistoryAPIProvider.tsx` around lines 14 - 20, The useEffect that wraps window.history.replaceState (captured in the local original variable) must restore the original function in its cleanup to avoid stacking wrappers and stale closures; update the effect in HistoryAPIProvider to return a cleanup function that sets window.history.replaceState back to original (guarding if original is defined) and optionally clear or reset isReplaceRef.current to a safe default so remounts behave correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Providers/HistoryAPIProvider.tsx`:
- Around line 22-45: Add unit tests covering the new replace/back state machine
around useLocationChange: write specs that exercise initial load
(onInitial:true), a push navigation that prepends a newPath to history, a
replace navigation using the isReplaceRef path that replaces history[0] via
setHistory, and the “navigate back to history[1]” case where history.slice(1) is
returned; also add an end-to-end style test reproducing the invoice flow that
previously looped (simulate sequence: initial -> push invoice list -> push
invoice detail -> replace/update detail -> navigate back to list) to assert no
infinite loop and correct history array mutations. Target the component/provider
that holds isReplaceRef and setHistory (HistoryAPIProvider/useLocationChange)
and assert state transitions after each navigation step.
---
Duplicate comments:
In `@src/Providers/HistoryAPIProvider.tsx`:
- Around line 14-20: The useEffect that wraps window.history.replaceState
(captured in the local original variable) must restore the original function in
its cleanup to avoid stacking wrappers and stale closures; update the effect in
HistoryAPIProvider to return a cleanup function that sets
window.history.replaceState back to original (guarding if original is defined)
and optionally clear or reset isReplaceRef.current to a safe default so remounts
behave correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bffa869d-257a-45dd-a921-5ea14bed3014
📒 Files selected for processing (1)
src/Providers/HistoryAPIProvider.tsx
|
You should be able to implement this better with the new release of raviger(5.3.0) with kyeotic/raviger#162 |
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| className="px-0 justify-start" | ||
| to={`/facility/${facilityId}/billing/account/${invoice.account.id}`} | ||
| > | ||
| <BackButton variant="link" className="px-0 justify-start"> |
There was a problem hiding this comment.
BackButton no longer provides to or fallbackUrl. If the user lands on this page directly (refresh/new tab/deep link), useAppHistory().history[1] will be undefined and BackButton returns null, leaving no in-app way to navigate back. Consider passing a fallbackUrl (e.g., the account page or invoices list) so the back control is always available.
| <BackButton variant="link" className="px-0 justify-start"> | |
| <BackButton | |
| variant="link" | |
| className="px-0 justify-start" | |
| fallbackUrl={`/facility/${facilityId}/billing/invoices`} | |
| > |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 119: The package.json currently pins "raviger" to a non-existent version
^5.3.0 which breaks installs; update the dependency entry for the raviger
package (the "raviger" key in package.json) to a valid published version such as
"5.2.1" or replace it with a git dependency pointing to the repo/commit that
contains the initiatedBy change (e.g., a git+https URL or commit hash) so npm
install succeeds; ensure package-lock/yarn.lock is regenerated after the change.
In `@src/Providers/HistoryAPIProvider.tsx`:
- Around line 31-37: The branch in HistoryAPIProvider.tsx that checks if (action
=== "push") is redundant because it returns the same value as the default case;
remove the explicit push branch and leave the single return [newPath,
...history] path inside the navigation handler (referencing the action variable
and the newPath and history arrays) so behavior remains identical but code is
simplified.
🪄 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: 5309443d-6935-4e0d-802a-308a3bb758a3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.jsonsrc/Providers/HistoryAPIProvider.tsxsrc/pages/Facility/billing/invoice/InvoiceShow.tsx
| "pigeon-maps": "^0.22.1", | ||
| "qrcode.react": "^4.1.0", | ||
| "raviger": "^5.0.0-2", | ||
| "raviger": "^5.3.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
raviger 5.3.0 useLocationChange initiatedBy
💡 Result:
In raviger, useLocationChange only passes a single argument to your callback: a RavigerLocation object. There is no initiatedBy parameter/property in the hook API. [1]
If you’re trying to tell what caused the navigation (e.g. <Link> / navigate() vs browser back/forward), raviger’s navigate() dispatches a custom popstate event that includes __tag: 'raviger:navigation', which you can use to distinguish raviger-initiated navigations from other popstate sources. [2]
Example:
window.addEventListener('popstate', (e: any) => {
const initiatedByRaviger = e?.__tag === 'raviger:navigation'
// initiatedByRaviger ? "navigate/Link" : "back/forward (or other)"
})Sources: [1] [2]
🏁 Script executed:
fd -t f "HistoryAPIProvider" --exec cat {} \;Repository: ohcnetwork/care_fe
Length of output: 1549
🏁 Script executed:
rg "initiatedBy" -t ts -t tsx -B 2 -A 2Repository: ohcnetwork/care_fe
Length of output: 89
🏁 Script executed:
cat package.json | grep -A 5 -B 5 "raviger"Repository: ohcnetwork/care_fe
Length of output: 395
🌐 Web query:
raviger 5.3.0 npm package
💡 Result:
raviger does not appear to have a 5.3.0 release on npm. The latest published line shown by package mirrors/metadata is 5.2.x (e.g., jsDelivr shows 5.2.1 and UNPKG shows package metadata for 5.2.0). [1] [2]
To install the latest available:
npm i ravigerTo install a specific available version (example):
npm i raviger@5.2.1[1]
Sources: [1] [2]
🌐 Web query:
raviger PR 162 initiatedBy
💡 Result:
In kyeotic/raviger PR #162 (“add initatedBy”), merged March 10, 2026, raviger added an initiatedBy field to the RavigerLocation object produced by useLocationChange so you can tell how navigation happened (programmatic vs back/forward). [1]
initiatedBy?: NavigationInitiator where NavigationInitiator = 'push' | 'replace' | 'pop':
'push':navigate()(useshistory.pushState)'replace':navigate(..., { replace: true })(useshistory.replaceState)'pop': browser back/forward (popstate)undefined: fired on mount orbasePathchange (not triggered by navigation) [2]
🏁 Script executed:
rg "raviger" -t json package.json -A 1 -B 1Repository: ohcnetwork/care_fe
Length of output: 139
🏁 Script executed:
rg "@types/raviger" -t jsonRepository: ohcnetwork/care_fe
Length of output: 44
Raviger 5.3.0 does not exist on NPM yet; package.json specifies a non-existent version.
PR #162 that adds the initiatedBy property was merged March 10, 2026, but the 5.3.0 release has not been published to NPM. The latest available version is 5.2.1. Specifying "raviger": "^5.3.0" in package.json will cause npm install to fail. Either downgrade to an available version (e.g., 5.2.1) and implement an alternative approach for tracking navigation origin, use a git dependency pointing to the latest commit, or wait for the 5.3.0 release to be published.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 119, The package.json currently pins "raviger" to a
non-existent version ^5.3.0 which breaks installs; update the dependency entry
for the raviger package (the "raviger" key in package.json) to a valid published
version such as "5.2.1" or replace it with a git dependency pointing to the
repo/commit that contains the initiatedBy change (e.g., a git+https URL or
commit hash) so npm install succeeds; ensure package-lock/yarn.lock is
regenerated after the change.
There was a problem hiding this comment.
@NikhilA8606, you're right to question this! The Comment image shows the raviger GitHub releases page confirming that v5.3.0 has indeed been tagged as the latest release (13 hours ago).
However, there's an important distinction: a GitHub release tag does not automatically mean the package is published to npm. The npm registry currently lists 5.1.0 as the latest published version of raviger, and the main branch package.json in the raviger repo still shows "version": "5.2.1".
This means:
- ✅ The GitHub release for 5.3.0 exists (as shown in your screenshot).
⚠️ The npm package for 5.3.0 may not have been published yet (npm still shows 5.1.0 as latest).
If 5.3.0 hasn't been pushed to npm, npm install will still fail with "raviger": "^5.3.0" since ^5.3.0 requires at least that exact version to be available on the registry.
I'd recommend verifying by running:
npm view raviger versions --json | tail -5or checking https://www.npmjs.com/package/raviger?activeTab=versions directly. If 5.3.0 is confirmed on npm, then the package.json entry is perfectly valid and my original concern no longer applies. I apologize for the confusion caused by my earlier analysis!
🧠 Learnings used
Learnt from: CR
Repo: ohcnetwork/care_fe PR: 0
File: .github/instructions/pages.instructions.md:0-0
Timestamp: 2025-11-25T13:52:19.758Z
Learning: Applies to src/pages/**/*.{ts,tsx} : Use `raviger` router for navigation (existing dependency)
Learnt from: CR
Repo: ohcnetwork/care_fe PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-25T13:49:43.065Z
Learning: Applies to **/*.{ts,tsx} : Use raviger for routing
Learnt from: CR
Repo: ohcnetwork/care_fe PR: 0
File: .github/instructions/config-files.instructions.md:0-0
Timestamp: 2025-11-25T13:51:07.680Z
Learning: Applies to package.json : Maintain `package.json` as the primary dependency and script configuration
Learnt from: CR
Repo: ohcnetwork/care_fe PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-09T11:25:31.348Z
Learning: Applies to src/pages/**/*.{ts,tsx} : Use raviger for application routing instead of other routing libraries
ohc-project-manager
Bot
added
Changes Required
and removed
needs review
needs testing
labels
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/Providers/HistoryAPIProvider.tsx:34
- This PR changes browser-history semantics for billing flows (HistoryAPIProvider pop/replace handling + invoice creation navigation using
replace). There are Playwright specs for BackButton/history behavior, but there isn’t a regression test covering the reported billing loop (#16017). Adding a Playwright test that reproduces the loop (open invoice from account, click in-app back, then ensure browser back doesn’t return to the invoice) would help prevent regressions.
useLocationChange(
(location) => {
const newPath = location.fullPath + location.search;
const action = location.initiatedBy;
setHistory((history) => {
// instead of pushing, we are popping the history when the same page is navigated to
if (
(history.length > 1 && newPath === history[1]) ||
action === "pop"
) {
return history.slice(1);
}
// replace navigation
if (action === "replace") {
return [newPath, ...history.slice(1)];
}
// normal navigation or push navigation
return [newPath, ...history];
});
},
{ onInitial: true },
);
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Rithvik Nishad <rithvikn2001@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
You can also share your feedback on Copilot code review. Take the survey.
| setHistory((history) => { | ||
| // Pop current path if navigate back to previous path | ||
| if ( | ||
| (history.length > 1 && newPath === history[1]) || | ||
| action === "pop" | ||
| ) { | ||
| return history.slice(1); | ||
| } | ||
|
|
||
| if (action === "replace") { | ||
| return [newPath, ...history.slice(1)]; | ||
| } | ||
|
|
||
| // Otherwise just push the current path | ||
| return [newPath, ...history]; |
There was a problem hiding this comment.
action === "pop" currently always drops the current entry (history.slice(1)), even when the user navigates forward (also a POP). This can desync the app history (e.g., forward navigation can clear the stack) and cause BackButton to disappear or point to the wrong page. Consider handling POP by comparing newPath to history[1] (back) and otherwise treating it like a push (forward), or reconciling by finding newPath within the existing stack before slicing.
| setHistory((history) => { | |
| // Pop current path if navigate back to previous path | |
| if ( | |
| (history.length > 1 && newPath === history[1]) || | |
| action === "pop" | |
| ) { | |
| return history.slice(1); | |
| } | |
| if (action === "replace") { | |
| return [newPath, ...history.slice(1)]; | |
| } | |
| // Otherwise just push the current path | |
| return [newPath, ...history]; | |
| setHistory((currentHistory) => { | |
| const isBackToPrevious = | |
| currentHistory.length > 1 && newPath === currentHistory[1]; | |
| if (action === "pop") { | |
| // Handle browser history POP (back or forward) | |
| if (isBackToPrevious) { | |
| // Back: drop the current entry, reveal previous | |
| return currentHistory.slice(1); | |
| } | |
| // Forward or arbitrary POP: try to reconcile with existing stack | |
| const existingIndex = currentHistory.indexOf(newPath); | |
| if (existingIndex > -1) { | |
| // Move the existing entry to the front, preserving its forward history | |
| return currentHistory.slice(existingIndex); | |
| } | |
| // POP to a path not in our stack: treat as a push | |
| return [newPath, ...currentHistory]; | |
| } | |
| if (action === "replace") { | |
| if (currentHistory.length === 0) { | |
| return [newPath]; | |
| } | |
| return [newPath, ...currentHistory.slice(1)]; | |
| } | |
| // Non-POP navigation that effectively goes back to the previous path | |
| if (isBackToPrevious) { | |
| return currentHistory.slice(1); | |
| } | |
| // Otherwise just push the current path | |
| return [newPath, ...currentHistory]; |
| onSuccess?.(); | ||
| } else { | ||
| onSuccess?.(); | ||
| navigate(invoiceUrl); | ||
| navigate(invoiceUrl, { replace: true }); | ||
| } |
There was a problem hiding this comment.
This change adjusts navigation history semantics (using navigate(..., { replace: true })) to avoid the invoice/account back-loop. Please add/extend a Playwright spec that reproduces #16017 (invoice -> back -> account -> back does not return to the same invoice) and also covers browser back/forward behavior after a replaced navigation, since the HistoryAPIProvider logic is sensitive to POP/REPLACE actions.
Proposed Changes
Fix #16017
Screen.Recording.2026-03-05.at.11.58.10.AM.mov
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Bug Fixes
Refactor