Skip to content

Commit 83f50f6

Browse files
jescalanjacekradko
andauthored
fix(react): Prevent custom page remounts in profile components (#8604)
Co-authored-by: Jacek Radko <jacek@clerk.dev>
1 parent 7eb7253 commit 83f50f6

14 files changed

Lines changed: 666 additions & 51 deletions
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@clerk/react': patch
3+
---
4+
5+
Keep custom pages and menu items mounted when sibling pages are added, removed, or reordered. Portals are now keyed by a stable id rather than their array index, so a surviving page is reconciled as an update instead of being remounted.
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@clerk/react': patch
3+
---
4+
5+
Prevent custom pages in profile components from remounting during parent rerenders.

integration/templates/react-vite/src/custom-user-profile/index.tsx

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,12 @@
11
import { UserProfile } from '@clerk/react';
2-
import { useContext } from 'react';
2+
import { useContext, useState } from 'react';
33
import { PageContext, PageContextProvider } from '../PageContext.tsx';
44

55
function Page1() {
66
const { counter, setCounter } = useContext(PageContext);
7+
// Local state lives INSIDE the portaled custom page. It only resets if the
8+
// page is remounted, so it is our instrument for detecting remounts.
9+
const [localCounter, setLocalCounter] = useState(0);
710

811
return (
912
<>
@@ -15,13 +18,31 @@ function Page1() {
1518
>
1619
Update
1720
</button>
21+
<p data-local-counter={1}>Local counter: {localCounter}</p>
22+
<button
23+
data-local-counter={1}
24+
onClick={() => setLocalCounter(a => a + 1)}
25+
>
26+
Increment local
27+
</button>
1828
</>
1929
);
2030
}
2131

2232
export default function Page() {
33+
// Bumping parent state recreates the <UserProfile> element, forcing the
34+
// profile component (and useCustomPages) to rerender. The custom page content
35+
// must survive this without remounting.
36+
const [parentTick, setParentTick] = useState(0);
37+
2338
return (
2439
<PageContextProvider>
40+
<button
41+
data-testid='rerender-parent'
42+
onClick={() => setParentTick(t => t + 1)}
43+
>
44+
Rerender parent: {parentTick}
45+
</button>
2546
<UserProfile
2647
fallback={<>Loading user profile</>}
2748
path={'/custom-user-profile'}

integration/tests/custom-pages.test.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,37 @@ testAgainstRunningApps({ withPattern: ['react.vite.withEmailCodes'] })(
171171
});
172172
});
173173

174+
test('custom profile page survives a parent rerender without remounting', async ({ page, context }) => {
175+
const u = createTestUtils({ app, page, context });
176+
await u.po.signIn.goTo();
177+
await u.po.signIn.waitForMounted();
178+
await u.po.signIn.signInWithEmailAndInstantPassword({ email: fakeUser.email, password: fakeUser.password });
179+
await u.po.expect.toBeSignedIn();
180+
181+
await u.page.goToRelative(CUSTOM_PROFILE_PAGE);
182+
await u.po.userProfile.waitForMounted();
183+
184+
// Open the custom page (Page 1)
185+
const [profilePage] = await u.page.locator('button.cl-navbarButton__custom-page-0').all();
186+
await profilePage.click();
187+
188+
// Local state lives inside the portaled custom page and starts at 0.
189+
await u.page.waitForSelector('p[data-local-counter="1"]', { state: 'attached' });
190+
await expect(u.page.locator('p[data-local-counter="1"]')).toHaveText('Local counter: 0');
191+
192+
// Mutate the local state to 2.
193+
await u.page.locator('button[data-local-counter="1"]').click();
194+
await u.page.locator('button[data-local-counter="1"]').click();
195+
await expect(u.page.locator('p[data-local-counter="1"]')).toHaveText('Local counter: 2');
196+
197+
// Force a parent rerender: this re-creates the <UserProfile> element and reruns useCustomPages.
198+
await u.page.locator('button[data-testid="rerender-parent"]').click();
199+
await expect(u.page.locator('button[data-testid="rerender-parent"]')).toHaveText('Rerender parent: 1');
200+
201+
// The custom page must NOT remount, so its local state is preserved.
202+
await expect(u.page.locator('p[data-local-counter="1"]')).toHaveText('Local counter: 2');
203+
});
204+
174205
test.describe('User Button with experimental asStandalone and asProvider', () => {
175206
test('items at the specified order', async ({ page, context }) => {
176207
const u = createTestUtils({ app, page, context });

packages/react/src/components/ClerkHostRenderer.tsx

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -75,17 +75,18 @@ export class ClerkHostRenderer extends React.PureComponent<
7575
// Remove children and customPages from props before comparing
7676
// children might hold circular references which deepEqual can't handle
7777
// and the implementation of customPages relies on props getting new references
78-
const prevProps = without(_prevProps.props, 'customPages', 'customMenuItems', 'children');
79-
const newProps = without(this.props.props, 'customPages', 'customMenuItems', 'children');
78+
const prevProps = without(_prevProps.props || {}, 'customPages', 'customMenuItems', 'children');
79+
const newProps = without(this.props.props || {}, 'customPages', 'customMenuItems', 'children');
8080

8181
// instead, we simply use the length of customPages to determine if it changed or not
82-
const customPagesChanged = prevProps.customPages?.length !== newProps.customPages?.length;
83-
const customMenuItemsChanged = prevProps.customMenuItems?.length !== newProps.customMenuItems?.length;
82+
const customPagesChanged = _prevProps.props?.customPages?.length !== this.props.props?.customPages?.length;
83+
const customMenuItemsChanged =
84+
_prevProps.props?.customMenuItems?.length !== this.props.props?.customMenuItems?.length;
8485

8586
// Strip out mountIcon and unmountIcon handlers since they're always generated as new function references,
8687
// which would cause unnecessary re-renders in deep equality checks
87-
const prevMenuItemsWithoutHandlers = stripMenuItemIconHandlers(_prevProps.props.customMenuItems);
88-
const newMenuItemsWithoutHandlers = stripMenuItemIconHandlers(this.props.props.customMenuItems);
88+
const prevMenuItemsWithoutHandlers = stripMenuItemIconHandlers(_prevProps.props?.customMenuItems);
89+
const newMenuItemsWithoutHandlers = stripMenuItemIconHandlers(this.props.props?.customMenuItems);
8990

9091
if (
9192
!isDeeplyEqual(prevProps, newProps) ||
Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
import { render } from '@testing-library/react';
2+
import React from 'react';
3+
import { describe, expect, it, vi } from 'vitest';
4+
5+
import { ClerkHostRenderer } from '../ClerkHostRenderer';
6+
7+
vi.mock('@clerk/shared/object', () => ({
8+
without: (obj: Record<string, unknown>, ...keys: string[]) =>
9+
Object.fromEntries(Object.entries(obj).filter(([key]) => !keys.includes(key))),
10+
}));
11+
12+
vi.mock('@clerk/shared/react', () => ({
13+
isDeeplyEqual: (a: unknown, b: unknown) => JSON.stringify(a) === JSON.stringify(b),
14+
}));
15+
16+
describe('<ClerkHostRenderer />', () => {
17+
it('does not throw when mounted component props are omitted during updates', () => {
18+
const mount = vi.fn();
19+
const unmount = vi.fn();
20+
const updateProps = vi.fn();
21+
22+
const { rerender } = render(
23+
<ClerkHostRenderer
24+
component='UserProfile'
25+
mount={mount}
26+
unmount={unmount}
27+
updateProps={updateProps}
28+
/>,
29+
);
30+
31+
expect(() =>
32+
rerender(
33+
<ClerkHostRenderer
34+
component='OrganizationProfile'
35+
mount={mount}
36+
unmount={unmount}
37+
updateProps={updateProps}
38+
/>,
39+
),
40+
).not.toThrow();
41+
42+
expect(updateProps).not.toHaveBeenCalled();
43+
});
44+
45+
it('updates mounted component props when custom pages are added or removed', () => {
46+
const mount = vi.fn();
47+
const unmount = vi.fn();
48+
const updateProps = vi.fn();
49+
50+
const { rerender } = render(
51+
<ClerkHostRenderer
52+
mount={mount}
53+
props={{ customPages: [{ label: 'General' }] }}
54+
unmount={unmount}
55+
updateProps={updateProps}
56+
/>,
57+
);
58+
59+
rerender(
60+
<ClerkHostRenderer
61+
mount={mount}
62+
props={{ customPages: [{ label: 'General' }, { label: 'Permissions' }] }}
63+
unmount={unmount}
64+
updateProps={updateProps}
65+
/>,
66+
);
67+
68+
expect(updateProps).toHaveBeenCalledTimes(1);
69+
expect(updateProps).toHaveBeenCalledWith({
70+
node: expect.any(HTMLDivElement),
71+
props: { customPages: [{ label: 'General' }, { label: 'Permissions' }] },
72+
});
73+
});
74+
});

packages/react/src/components/uiComponents.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,8 +127,8 @@ type OrganizationSwitcherPropsWithoutCustomPages = Without<
127127
const CustomPortalsRenderer = (props: CustomPortalsRendererProps) => {
128128
return (
129129
<>
130-
{props?.customPagesPortals?.map((portal, index) => createElement(portal, { key: index }))}
131-
{props?.customMenuItemsPortals?.map((portal, index) => createElement(portal, { key: index }))}
130+
{props?.customPagesPortals?.map(({ key, portal }) => createElement(portal, { key }))}
131+
{props?.customMenuItemsPortals?.map(({ key, portal }) => createElement(portal, { key }))}
132132
</>
133133
);
134134
};
Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
import { render, screen } from '@testing-library/react';
2+
import React, { createElement, useEffect, useRef } from 'react';
3+
import { afterEach, describe, expect, it, vi } from 'vitest';
4+
5+
import { OrganizationProfilePage } from '../../components/uiComponents';
6+
import { useOrganizationProfileCustomPages } from '../useCustomPages';
7+
8+
vi.mock('@clerk/shared/utils', () => ({
9+
logErrorInDevMode: vi.fn(),
10+
}));
11+
12+
// Per-page mount/unmount counters. A remount re-runs the mount effect.
13+
const mounts: Record<string, number> = {};
14+
const unmounts: Record<string, number> = {};
15+
16+
// Stable component type, defined once. If it remounts across a rerender it is
17+
// because the portal wrapping it changed identity or render key.
18+
const TrackedContent = ({ id, text }: { id: string; text: string }) => {
19+
useEffect(() => {
20+
mounts[id] = (mounts[id] ?? 0) + 1;
21+
return () => {
22+
unmounts[id] = (unmounts[id] ?? 0) + 1;
23+
};
24+
// eslint-disable-next-line react-hooks/exhaustive-deps -- mount-once instrument; id is stable per instance
25+
}, []);
26+
return <div data-testid={`content-${id}`}>{text}</div>;
27+
};
28+
29+
/**
30+
* Faithfully reproduces the production render path for custom pages:
31+
* - useOrganizationProfileCustomPages parses children into { customPages, customPagesPortals }
32+
* - clerk-js calls customPages[i].mount(node) once per logical page (by identity; here keyed by url)
33+
* - CustomPortalsRenderer renders each portal via createElement(portal, { key }) using the STABLE key
34+
*/
35+
const Harness = ({ children, tick }: { children: React.ReactNode; tick: number }) => {
36+
const { customPages, customPagesPortals } = useOrganizationProfileCustomPages(children);
37+
const hostRef = useRef<HTMLDivElement | null>(null);
38+
const mountedUrls = useRef<Set<string>>(new Set());
39+
40+
useEffect(() => {
41+
customPages.forEach(page => {
42+
if (page.mount && page.url && !mountedUrls.current.has(page.url)) {
43+
mountedUrls.current.add(page.url);
44+
const node = document.createElement('div');
45+
hostRef.current?.appendChild(node);
46+
page.mount(node);
47+
}
48+
});
49+
});
50+
51+
return (
52+
<>
53+
<div
54+
data-tick={tick}
55+
ref={hostRef}
56+
/>
57+
{customPagesPortals.map(({ key, portal }) => createElement(portal, { key }))}
58+
</>
59+
);
60+
};
61+
62+
const makePage = (id: string, label: string, url: string, text: string) => (
63+
<OrganizationProfilePage
64+
key={id}
65+
label={label}
66+
labelIcon={<span>i</span>}
67+
url={url}
68+
>
69+
<TrackedContent
70+
id={id}
71+
text={text}
72+
/>
73+
</OrganizationProfilePage>
74+
);
75+
76+
afterEach(() => {
77+
for (const k of Object.keys(mounts)) {
78+
delete mounts[k];
79+
}
80+
for (const k of Object.keys(unmounts)) {
81+
delete unmounts[k];
82+
}
83+
});
84+
85+
describe('custom pages remount behavior (integration through CustomPortalsRenderer path)', () => {
86+
it('does not remount custom page content when the parent rerenders', async () => {
87+
const { rerender } = render(<Harness tick={0}>{[makePage('p1', 'Page 1', 'page-1', 'first')]}</Harness>);
88+
89+
await screen.findByText('first');
90+
expect(mounts['p1']).toBe(1);
91+
92+
// Parent rerenders for an unrelated reason; the page content prop changes but the
93+
// logical page (key/label/url) is identical.
94+
rerender(<Harness tick={1}>{[makePage('p1', 'Page 1', 'page-1', 'second')]}</Harness>);
95+
96+
await screen.findByText('second');
97+
expect(mounts['p1']).toBe(1);
98+
expect(unmounts['p1'] ?? 0).toBe(0);
99+
});
100+
101+
it('does not remount a surviving custom page when another page is inserted before it', async () => {
102+
const second = makePage('second', 'Second', 'second', 'second-content');
103+
const first = makePage('first', 'First', 'first', 'first-content');
104+
105+
const { rerender } = render(<Harness tick={0}>{[second]}</Harness>);
106+
await screen.findByText('second-content');
107+
expect(mounts['second']).toBe(1);
108+
109+
// Insert a new page BEFORE the existing one.
110+
rerender(<Harness tick={1}>{[first, second]}</Harness>);
111+
await screen.findByText('first-content');
112+
113+
// The surviving page keeps its stable key + portal identity, so React reconciles it as an
114+
// update rather than a remount.
115+
expect(mounts['second']).toBe(1);
116+
expect(unmounts['second'] ?? 0).toBe(0);
117+
// The newly inserted page mounts exactly once.
118+
expect(mounts['first']).toBe(1);
119+
});
120+
});

0 commit comments

Comments
 (0)