From af855748eb3ad7df81227d06dd1f51194750755c Mon Sep 17 00:00:00 2001 From: nikospapcom Date: Mon, 17 Feb 2025 11:37:21 +0200 Subject: [PATCH] fix(clerk-react): Avoid re-render UserProfile when we pass `userProfileProps` and `customMenuItems` in `` --- .changeset/sharp-owls-live.md | 5 ++ .../with-dynamic-label-and-custom-pages.tsx | 68 +++++++++++++++++++ .../with-dynamic-labels.tsx | 20 +----- integration/templates/react-vite/src/main.tsx | 5 ++ integration/tests/custom-pages.test.ts | 68 +++++++++++++++++++ .../src/components/ClerkHostRenderer.tsx | 27 +++++++- 6 files changed, 171 insertions(+), 22 deletions(-) create mode 100644 .changeset/sharp-owls-live.md create mode 100644 integration/templates/react-vite/src/custom-user-button/with-dynamic-label-and-custom-pages.tsx diff --git a/.changeset/sharp-owls-live.md b/.changeset/sharp-owls-live.md new file mode 100644 index 00000000000..1820c22db33 --- /dev/null +++ b/.changeset/sharp-owls-live.md @@ -0,0 +1,5 @@ +--- +'@clerk/clerk-react': patch +--- + +Fix an infinity re-render issue in `UserProfileModal` when we pass `userProfileProps` in `` and we have `customMenuItems` and `customPages` diff --git a/integration/templates/react-vite/src/custom-user-button/with-dynamic-label-and-custom-pages.tsx b/integration/templates/react-vite/src/custom-user-button/with-dynamic-label-and-custom-pages.tsx new file mode 100644 index 00000000000..45527039c27 --- /dev/null +++ b/integration/templates/react-vite/src/custom-user-button/with-dynamic-label-and-custom-pages.tsx @@ -0,0 +1,68 @@ +import { UserButton } from '@clerk/clerk-react'; +import { PageContextProvider } from '../PageContext.tsx'; +import React from 'react'; + +export default function Page() { + const [open, setIsOpen] = React.useState(false); + const [theme, setTheme] = React.useState('light'); + const [notifications, setNotifications] = React.useState(false); + const [language, setLanguage] = React.useState('en'); + + return ( + + Loading user button} + userProfileProps={{ appearance: { elements: { modalBackdrop: { zIndex: '100' } } } }} + > + + 🌐} + onClick={() => setIsOpen(!open)} + /> + 🌐} + onClick={() => setTheme(t => (t === 'light' ? 'dark' : 'light'))} + /> + 🌐} + onClick={() => setNotifications(n => !n)} + /> + 🌍} + onClick={() => setLanguage(l => (l === 'en' ? 'es' : 'en'))} + /> + + + 🌐} + /> + + 🌐} + /> + + 🔔} + onClick={() => alert('custom-alert')} + /> + + 🔔} + > +

Notifications page

+ + + + ); +} diff --git a/integration/templates/react-vite/src/custom-user-button/with-dynamic-labels.tsx b/integration/templates/react-vite/src/custom-user-button/with-dynamic-labels.tsx index 2c45cf85968..8ad9aab0dae 100644 --- a/integration/templates/react-vite/src/custom-user-button/with-dynamic-labels.tsx +++ b/integration/templates/react-vite/src/custom-user-button/with-dynamic-labels.tsx @@ -1,25 +1,7 @@ import { UserButton } from '@clerk/clerk-react'; -import { useContext } from 'react'; -import { PageContext, PageContextProvider } from '../PageContext.tsx'; +import { PageContextProvider } from '../PageContext.tsx'; import React from 'react'; -function Page1() { - const { counter, setCounter } = useContext(PageContext); - - return ( - <> -

Page 1

-

Counter: {counter}

- - - ); -} - export default function Page() { const [open, setIsOpen] = React.useState(false); const [theme, setTheme] = React.useState('light'); diff --git a/integration/templates/react-vite/src/main.tsx b/integration/templates/react-vite/src/main.tsx index 20f64bdb9e4..10271eb8a4a 100644 --- a/integration/templates/react-vite/src/main.tsx +++ b/integration/templates/react-vite/src/main.tsx @@ -11,6 +11,7 @@ import UserProfile from './user'; import UserProfileCustom from './custom-user-profile'; import UserButtonCustom from './custom-user-button'; import UserButtonCustomDynamicLabels from './custom-user-button/with-dynamic-labels.tsx'; +import UserButtonCustomDynamicLabelsAndCustomPages from './custom-user-button/with-dynamic-label-and-custom-pages.tsx'; import UserButtonCustomTrigger from './custom-user-button-trigger'; import UserButton from './user-button'; import Waitlist from './waitlist'; @@ -85,6 +86,10 @@ const router = createBrowserRouter([ path: '/custom-user-button-dynamic-labels', element: , }, + { + path: '/custom-user-button-dynamic-labels-and-custom-pages', + element: , + }, { path: '/custom-user-button-trigger', element: , diff --git a/integration/tests/custom-pages.test.ts b/integration/tests/custom-pages.test.ts index 8a87f3efcbb..9efb227c8d9 100644 --- a/integration/tests/custom-pages.test.ts +++ b/integration/tests/custom-pages.test.ts @@ -8,6 +8,7 @@ const CUSTOM_PROFILE_PAGE = '/custom-user-profile'; const CUSTOM_BUTTON_PAGE = '/custom-user-button'; const CUSTOM_BUTTON_TRIGGER_PAGE = '/custom-user-button-trigger'; const CUSTOM_BUTTON_DYNAMIC_LABELS_PAGE = '/custom-user-button-dynamic-labels'; +const CUSTOM_BUTTON_DYNAMIC_LABELS_AND_CUSTOM_PAGES_PAGE = '/custom-user-button-dynamic-labels-and-custom-pages'; async function waitForMountedComponent( component: 'UserButton' | 'UserProfile', @@ -375,5 +376,72 @@ testAgainstRunningApps({ withPattern: ['react.vite.withEmailCodes'] })( await expect(languageButton).toHaveText('🌍Language: EN'); }); }); + + test.describe('User Button with dynamic labels and custom page', () => { + test('click Chat is OFF and ensure that state has been changed', async ({ page, context }) => { + const u = createTestUtils({ app, page, context }); + await u.po.signIn.goTo(); + await u.po.signIn.waitForMounted(); + await u.po.signIn.signInWithEmailAndInstantPassword({ email: fakeUser.email, password: fakeUser.password }); + await u.po.expect.toBeSignedIn(); + + await u.page.goToRelative(CUSTOM_BUTTON_DYNAMIC_LABELS_AND_CUSTOM_PAGES_PAGE); + await u.po.userButton.waitForMounted(); + await u.po.userButton.toggleTrigger(); + await u.po.userButton.waitForPopover(); + + const pagesContainer = u.page.locator('div.cl-userButtonPopoverActions__multiSession').first(); + const buttons = await pagesContainer.locator('button').all(); + + expect(buttons.length).toBe(9); + + const expectedTexts = [ + '🌐Chat is OFF', + '🌐Theme: ☀️ Light', + '🌐Notifications 🔕 OFF', + '🌍Language: EN', + 'Manage account', + 'Sign out', + '🌐Visit Clerk', + '🌐Visit User page', + '🔔Custom Alert', + ]; + + for (let i = 0; i < buttons.length; i++) { + await expect(buttons[i]).toHaveText(expectedTexts[i]); + } + + const chatButton = buttons[0]; + const notificationsButton = buttons[2]; + const languageButton = buttons[3]; + const manageAccountButton = buttons[4]; + + // Test chat toggle + await chatButton.click(); + await u.po.userButton.toggleTrigger(); + await u.po.userButton.waitForPopover(); + await expect(chatButton).toHaveText('🌐Chat is ON'); + await expect(languageButton).toHaveText('🌍Language: EN'); + + await notificationsButton.click(); + await u.po.userButton.toggleTrigger(); + await u.po.userButton.waitForPopover(); + await expect(notificationsButton).toHaveText('🌐Notifications 🔔 ON'); + await expect(chatButton).toHaveText('🌐Chat is ON'); + await expect(languageButton).toHaveText('🌍Language: EN'); + + await manageAccountButton.click(); + await u.po.userProfile.waitForMounted(); + + const userProfilePageButtons = await u.page.locator('button.cl-navbarButton__custom-page-0').all(); + const [notificationsPage] = userProfilePageButtons; + await expect(notificationsPage.locator('div.cl-navbarButtonIcon__custom-page-0')).toHaveText('🔔'); + + await notificationsPage.click(); + + const orderSent = page.locator('h1[data-page="notifications-page"]'); + await orderSent.waitFor({ state: 'attached' }); + }); + }); }, ); diff --git a/packages/react/src/components/ClerkHostRenderer.tsx b/packages/react/src/components/ClerkHostRenderer.tsx index ebaa84add87..3e1304caebc 100644 --- a/packages/react/src/components/ClerkHostRenderer.tsx +++ b/packages/react/src/components/ClerkHostRenderer.tsx @@ -12,6 +12,17 @@ const isMountProps = (props: any): props is MountProps => { const isOpenProps = (props: any): props is OpenProps => { return 'open' in props; }; + +const stripMenuItemIconHandlers = ( + menuItems?: Array<{ + mountIcon?: (el: HTMLDivElement) => void; + unmountIcon?: (el: HTMLDivElement) => void; + [key: string]: any; + }>, +) => { + return menuItems?.map(({ mountIcon, unmountIcon, ...rest }) => rest); +}; + // README: should be a class pure component in order for mount and unmount // lifecycle props to be invoked correctly. Replacing the class component with a // functional component wrapped with a React.memo is not identical to the original @@ -64,14 +75,24 @@ export class ClerkHostRenderer extends React.PureComponent< // Remove children and customPages from props before comparing // children might hold circular references which deepEqual can't handle // and the implementation of customPages relies on props getting new references - const prevProps = without(_prevProps.props, 'customPages', 'children'); - const newProps = without(this.props.props, 'customPages', 'children'); + const prevProps = without(_prevProps.props, 'customPages', 'customMenuItems', 'children'); + const newProps = without(this.props.props, 'customPages', 'customMenuItems', 'children'); // instead, we simply use the length of customPages to determine if it changed or not const customPagesChanged = prevProps.customPages?.length !== newProps.customPages?.length; const customMenuItemsChanged = prevProps.customMenuItems?.length !== newProps.customMenuItems?.length; - if (!isDeeplyEqual(prevProps, newProps) || customPagesChanged || customMenuItemsChanged) { + // Strip out mountIcon and unmountIcon handlers since they're always generated as new function references, + // which would cause unnecessary re-renders in deep equality checks + const prevMenuItemsWithoutHandlers = stripMenuItemIconHandlers(_prevProps.props.customMenuItems); + const newMenuItemsWithoutHandlers = stripMenuItemIconHandlers(this.props.props.customMenuItems); + + if ( + !isDeeplyEqual(prevProps, newProps) || + !isDeeplyEqual(prevMenuItemsWithoutHandlers, newMenuItemsWithoutHandlers) || + customPagesChanged || + customMenuItemsChanged + ) { if (this.rootRef.current) { this.props.updateProps({ node: this.rootRef.current, props: this.props.props }); }