Skip to content

Commit 5d95d91

Browse files
ShaneKShaneK
committed
fix(react-router): clean up replaced views with parameterized routes
1 parent 008ff95 commit 5d95d91

File tree

6 files changed

+274
-34
lines changed

6 files changed

+274
-34
lines changed

‎packages/react-router/src/ReactRouter/StackManager.tsx‎

Lines changed: 56 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -209,34 +209,15 @@ export class StackManager extends React.PureComponent<StackManagerProps> {
209209
}
210210

211211
if (routeInfo.routeAction === 'replace') {
212-
const enteringRoutePath = enteringViewItem?.reactElement?.props?.path as string | undefined;
213212
const leavingRoutePath = leavingViewItem?.reactElement?.props?.path as string | undefined;
214213

215-
// Never unmount root path - needed for back navigation
216-
if (leavingRoutePath === '/' || leavingRoutePath === '') {
214+
// Never unmount root path or views without a path - needed for back navigation
215+
if (!leavingRoutePath || leavingRoutePath === '/' || leavingRoutePath === '') {
217216
return false;
218217
}
219218

220-
if (enteringRoutePath && leavingRoutePath) {
221-
const getParentPath = (path: string) => {
222-
const normalized = path.replace(/\/\*$/, '');
223-
const lastSlash = normalized.lastIndexOf('/');
224-
return lastSlash > 0 ? normalized.substring(0, lastSlash) : '/';
225-
};
226-
227-
const enteringParent = getParentPath(enteringRoutePath);
228-
const leavingParent = getParentPath(leavingRoutePath);
229-
230-
// Unmount if routes are siblings or entering is a child of leaving (redirect)
231-
const areSiblings = enteringParent === leavingParent && enteringParent !== '/';
232-
const isChildRedirect =
233-
enteringRoutePath.startsWith(leavingRoutePath) ||
234-
(leavingRoutePath.endsWith('/*') && enteringRoutePath.startsWith(leavingRoutePath.slice(0, -2)));
235-
236-
return areSiblings || isChildRedirect;
237-
}
238-
239-
return false;
219+
// Replace actions unmount the leaving view since it's being replaced in history.
220+
return true;
240221
}
241222

242223
// For non-replace actions, only unmount for back navigation
@@ -460,7 +441,12 @@ export class StackManager extends React.PureComponent<StackManagerProps> {
460441
this.transitionPage(routeInfo, enteringViewItem, leavingViewItem, undefined, false, shouldSkipAnimation);
461442

462443
if (shouldUnmountLeavingViewItem && leavingViewItem && enteringViewItem !== leavingViewItem) {
463-
leavingViewItem.mount = false;
444+
// For non-replace actions (back nav), set mount=false here to hide the view.
445+
// For replace actions, handleLeavingViewUnmount sets mount=false only after
446+
// its container-to-container guard passes, avoiding zombie state.
447+
if (routeInfo.routeAction !== 'replace') {
448+
leavingViewItem.mount = false;
449+
}
464450
this.handleLeavingViewUnmount(routeInfo, enteringViewItem, leavingViewItem);
465451
}
466452

@@ -472,12 +458,18 @@ export class StackManager extends React.PureComponent<StackManagerProps> {
472458
* Handles leaving view unmount for replace actions.
473459
*/
474460
private handleLeavingViewUnmount(routeInfo: RouteInfo, enteringViewItem: ViewItem, leavingViewItem: ViewItem): void {
475-
if (!leavingViewItem.ionPageElement) {
461+
// Only replace actions unmount views; push/pop cache for navigation history
462+
if (routeInfo.routeAction !== 'replace') {
476463
return;
477464
}
478465

479-
// Only replace actions unmount views; push/pop cache for navigation history
480-
if (routeInfo.routeAction !== 'replace') {
466+
if (!leavingViewItem.ionPageElement) {
467+
leavingViewItem.mount = false;
468+
const viewToUnmount = leavingViewItem;
469+
setTimeout(() => {
470+
this.context.unMountViewItem(viewToUnmount);
471+
this.forceUpdate();
472+
}, VIEW_UNMOUNT_DELAY_MS);
481473
return;
482474
}
483475

@@ -497,6 +489,8 @@ export class StackManager extends React.PureComponent<StackManagerProps> {
497489
return;
498490
}
499491

492+
leavingViewItem.mount = false;
493+
500494
const viewToUnmount = leavingViewItem;
501495
setTimeout(() => {
502496
this.context.unMountViewItem(viewToUnmount);
@@ -553,11 +547,25 @@ export class StackManager extends React.PureComponent<StackManagerProps> {
553547

554548
const getParent = (path: string) => {
555549
const normalized = path.replace(/\/\*$/, '');
556-
const lastSlash = normalized.lastIndexOf('/');
557-
return lastSlash > 0 ? normalized.substring(0, lastSlash) : '/';
550+
const segments = normalized.split('/').filter(Boolean);
551+
// Strip trailing parameter segments (e.g., :id) so that
552+
// sibling routes like /items/list/:id and /items/detail/:id
553+
// resolve to the same parent (/items).
554+
while (segments.length > 0 && segments[segments.length - 1].startsWith(':')) {
555+
segments.pop();
556+
}
557+
segments.pop();
558+
return segments.length > 0 ? '/' + segments.join('/') : '/';
558559
};
559560

560-
return getParent(path1) === getParent(path2);
561+
const parent = getParent(path1);
562+
// Exclude root-level routes from sibling detection to avoid unintended
563+
// cleanup of unrelated top-level routes. Also covers single-depth param
564+
// routes (e.g., /items/:id) which resolve to root after param stripping.
565+
if (parent === '/') {
566+
return false;
567+
}
568+
return parent === getParent(path2);
561569
};
562570

563571
for (const viewItem of allViewsInOutlet) {
@@ -568,13 +576,23 @@ export class StackManager extends React.PureComponent<StackManagerProps> {
568576
(leavingViewItem && viewItem.id === leavingViewItem.id) ||
569577
!viewItem.mount ||
570578
!viewRoutePath ||
579+
// Don't clean up container routes when entering a container route
580+
// (e.g., /tabs/* and /settings/* coexist for tab switching)
571581
(viewRoutePath.endsWith('/*') && enteringRoutePath.endsWith('/*'));
572582

573583
if (shouldSkip) {
574584
continue;
575585
}
576586

577-
if (areSiblingRoutes(enteringRoutePath, viewRoutePath)) {
587+
const isOrphanedSpecificRoute = !viewRoutePath.endsWith('/*');
588+
589+
// Clean up sibling non-container routes that are no longer reachable.
590+
let shouldCleanup = false;
591+
if ((isReplaceAction || isPushToContainer) && isOrphanedSpecificRoute) {
592+
shouldCleanup = areSiblingRoutes(enteringRoutePath, viewRoutePath);
593+
}
594+
595+
if (shouldCleanup) {
578596
hideIonPageElement(viewItem.ionPageElement);
579597
viewItem.mount = false;
580598

@@ -667,7 +685,10 @@ export class StackManager extends React.PureComponent<StackManagerProps> {
667685

668686
// Don't unmount if entering and leaving are the same view item
669687
if (shouldUnmountLeavingViewItem && leavingViewItem && enteringViewItem !== leavingViewItem) {
670-
leavingViewItem.mount = false;
688+
if (routeInfo.routeAction !== 'replace') {
689+
leavingViewItem.mount = false;
690+
}
691+
this.handleLeavingViewUnmount(routeInfo, enteringViewItem, leavingViewItem);
671692
}
672693

673694
this.forceUpdate();
@@ -701,8 +722,9 @@ export class StackManager extends React.PureComponent<StackManagerProps> {
701722
this.transitionPage(routeInfo, latestEnteringView, latestLeavingView ?? undefined, undefined, false, shouldSkipAnimation);
702723

703724
if (shouldUnmountLeavingViewItem && latestLeavingView && latestEnteringView !== latestLeavingView) {
704-
latestLeavingView.mount = false;
705-
// Call handleLeavingViewUnmount to ensure the view is properly removed
725+
if (routeInfo.routeAction !== 'replace') {
726+
latestLeavingView.mount = false;
727+
}
706728
this.handleLeavingViewUnmount(routeInfo, latestEnteringView, latestLeavingView);
707729
}
708730

‎packages/react-router/test/base/src/App.tsx‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ import RedirectParams from './pages/redirect-params/RedirectParams';
6060
import MultiStepBack from './pages/multi-step-back/MultiStepBack';
6161
import DirectionNoneBack from './pages/direction-none-back/DirectionNoneBack';
6262
import TabSearchParams from './pages/tab-search-params/TabSearchParams';
63+
import { Step1, Step2, Step3, Step4 } from './pages/replace-params/ReplaceParams';
6364

6465
setupIonicReact();
6566

@@ -109,6 +110,11 @@ const App: React.FC = () => {
109110
<Route path="/multi-step-back/*" element={<MultiStepBack />} />
110111
<Route path="/direction-none-back/*" element={<DirectionNoneBack />} />
111112
<Route path="/tab-search-params/*" element={<TabSearchParams />} />
113+
<Route path="/replace-params/step1" element={<Step1 />} />
114+
<Route path="/replace-params/step2/:id" element={<Step2 />} />
115+
<Route path="/replace-params/step3/:id" element={<Step3 />} />
116+
<Route path="/replace-params/step4/:id" element={<Step4 />} />
117+
<Route path="/replace-params" element={<Navigate to="/replace-params/step1" replace />} />
112118
</IonRouterOutlet>
113119
</IonReactRouter>
114120
</IonApp>

‎packages/react-router/test/base/src/pages/Main.tsx‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,9 @@ const Main: React.FC = () => {
125125
<IonItem routerLink="/tab-search-params">
126126
<IonLabel>Tab Search Params</IonLabel>
127127
</IonItem>
128+
<IonItem routerLink="/replace-params/step1">
129+
<IonLabel>Replace Params</IonLabel>
130+
</IonItem>
128131
</IonList>
129132
</IonContent>
130133
</IonPage>

‎packages/react-router/test/base/src/pages/replace-params/ReplaceParams.tsx‎

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
import {
2+
IonContent,
3+
IonHeader,
4+
IonPage,
5+
IonTitle,
6+
IonToolbar,
7+
IonButton,
8+
IonButtons,
9+
IonBackButton,
10+
} from '@ionic/react';
11+
import React, { useState } from 'react';
12+
import { useNavigate, useParams } from 'react-router-dom';
13+
14+
/**
15+
* Test for issue #25640: Replace navigation with parameterized routes
16+
* should properly clean up leaving views from the DOM.
17+
*
18+
* Flow: step1 → (replace) step2/:id → (replace) step3/:id → (push) step4/:id
19+
* → (back) → step1 → (replace) step2/:id → (replace) step3/:id
20+
*
21+
* On the second visit to step3, it should be a fresh component instance
22+
* with updated params (verified via a unique mount ID).
23+
*/
24+
25+
const Step1: React.FC = () => {
26+
const navigate = useNavigate();
27+
28+
return (
29+
<IonPage data-pageid="replace-params-step1">
30+
<IonHeader>
31+
<IonToolbar>
32+
<IonTitle>Step 1</IonTitle>
33+
</IonToolbar>
34+
</IonHeader>
35+
<IonContent>
36+
<IonButton
37+
id="go-step2-first"
38+
onClick={() => navigate('/replace-params/step2/first', { replace: true })}
39+
>
40+
Replace to Step 2 (first)
41+
</IonButton>
42+
<IonButton
43+
id="go-step2-second"
44+
onClick={() => navigate('/replace-params/step2/second', { replace: true })}
45+
>
46+
Replace to Step 2 (second)
47+
</IonButton>
48+
</IonContent>
49+
</IonPage>
50+
);
51+
};
52+
53+
const Step2: React.FC = () => {
54+
const navigate = useNavigate();
55+
const { id } = useParams<{ id: string }>();
56+
57+
return (
58+
<IonPage data-pageid="replace-params-step2">
59+
<IonHeader>
60+
<IonToolbar>
61+
<IonTitle>Step 2</IonTitle>
62+
</IonToolbar>
63+
</IonHeader>
64+
<IonContent>
65+
<p data-testid="step2-param">{id}</p>
66+
<IonButton
67+
id="go-step3"
68+
onClick={() => navigate(`/replace-params/step3/${id}`, { replace: true })}
69+
>
70+
Replace to Step 3
71+
</IonButton>
72+
</IonContent>
73+
</IonPage>
74+
);
75+
};
76+
77+
const Step3: React.FC = () => {
78+
const navigate = useNavigate();
79+
const { id } = useParams<{ id: string }>();
80+
const [mountId] = useState(() => Math.random().toString(36).slice(2, 8));
81+
82+
return (
83+
<IonPage data-pageid="replace-params-step3">
84+
<IonHeader>
85+
<IonToolbar>
86+
<IonTitle>Step 3</IonTitle>
87+
</IonToolbar>
88+
</IonHeader>
89+
<IonContent>
90+
<p data-testid="step3-param">{id}</p>
91+
<p data-testid="step3-mount-id">{mountId}</p>
92+
<IonButton
93+
id="go-step4"
94+
onClick={() => navigate(`/replace-params/step4/${id}`)}
95+
>
96+
Push to Step 4
97+
</IonButton>
98+
</IonContent>
99+
</IonPage>
100+
);
101+
};
102+
103+
const Step4: React.FC = () => {
104+
const navigate = useNavigate();
105+
const { id } = useParams<{ id: string }>();
106+
107+
return (
108+
<IonPage data-pageid="replace-params-step4">
109+
<IonHeader>
110+
<IonToolbar>
111+
<IonButtons slot="start">
112+
<IonBackButton defaultHref="/replace-params/step1" />
113+
</IonButtons>
114+
<IonTitle>Step 4</IonTitle>
115+
</IonToolbar>
116+
</IonHeader>
117+
<IonContent>
118+
<p data-testid="step4-param">{id}</p>
119+
<IonButton
120+
id="go-to-step1"
121+
onClick={() => navigate('/replace-params/step1')}
122+
>
123+
Go to Step 1
124+
</IonButton>
125+
</IonContent>
126+
</IonPage>
127+
);
128+
};
129+
130+
const ReplaceParams: React.FC = () => null;
131+
export default ReplaceParams;
132+
export { Step1, Step2, Step3, Step4 };

‎packages/react-router/test/base/tests/e2e/playwright/replace-params.spec.ts‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
import { test, expect } from '@playwright/test';
2+
import { ionPageVisible, ionPageDoesNotExist, withTestingMode } from './utils/test-utils';
3+
4+
test.describe('Replace Params', () => {
5+
test('replaced views with params should be unmounted and fresh on revisit', async ({ page }, testInfo) => {
6+
testInfo.annotations.push({
7+
type: 'issue',
8+
description: 'https://github.com/ionic-team/ionic-framework/issues/25640',
9+
});
10+
11+
await page.goto(withTestingMode('/replace-params/step1'));
12+
await ionPageVisible(page, 'replace-params-step1');
13+
14+
await page.locator('#go-step2-first').click();
15+
await ionPageVisible(page, 'replace-params-step2');
16+
await expect(page.locator('[data-testid="step2-param"]')).toHaveText('first');
17+
18+
await page.locator('#go-step3').click();
19+
await ionPageVisible(page, 'replace-params-step3');
20+
await expect(page.locator('[data-testid="step3-param"]')).toHaveText('first');
21+
22+
// Record mount ID to verify we get a fresh component later
23+
const firstMountId = await page.locator('[data-testid="step3-mount-id"]').textContent();
24+
25+
await page.locator('#go-step4').click();
26+
await ionPageVisible(page, 'replace-params-step4');
27+
28+
await page.locator('#go-to-step1').click();
29+
await ionPageVisible(page, 'replace-params-step1');
30+
31+
await page.locator('#go-step2-second').click();
32+
await ionPageVisible(page, 'replace-params-step2');
33+
await expect(page.locator('[data-testid="step2-param"]')).toHaveText('second');
34+
35+
// Wait for previously replaced views to be fully removed from the DOM
36+
await ionPageDoesNotExist(page, 'replace-params-step4');
37+
await ionPageDoesNotExist(page, 'replace-params-step3');
38+
39+
// Critical assertion: step3 should show 'second' params, not stale 'first'
40+
await page.locator('#go-step3').click();
41+
await ionPageVisible(page, 'replace-params-step3');
42+
await expect(page.locator('[data-testid="step3-param"]')).toHaveText('second');
43+
44+
// Verify it's a fresh component instance, not the cached one
45+
const secondMountId = await page.locator('[data-testid="step3-mount-id"]').textContent();
46+
expect(secondMountId).not.toBe(firstMountId);
47+
});
48+
49+
test('simple replace chain should clean up replaced views', async ({ page }, testInfo) => {
50+
testInfo.annotations.push({
51+
type: 'issue',
52+
description: 'https://github.com/ionic-team/ionic-framework/issues/25640',
53+
});
54+
55+
await page.goto(withTestingMode('/replace-params/step1'));
56+
await ionPageVisible(page, 'replace-params-step1');
57+
58+
await page.locator('#go-step2-first').click();
59+
await ionPageVisible(page, 'replace-params-step2');
60+
await ionPageDoesNotExist(page, 'replace-params-step1');
61+
62+
await page.locator('#go-step3').click();
63+
await ionPageVisible(page, 'replace-params-step3');
64+
await ionPageDoesNotExist(page, 'replace-params-step2');
65+
});
66+
});

0 commit comments

Comments
 (0)