diff --git a/.changeset/fix-org-profile-custom-page-remounts.md b/.changeset/fix-org-profile-custom-page-remounts.md new file mode 100644 index 00000000000..07a51925d95 --- /dev/null +++ b/.changeset/fix-org-profile-custom-page-remounts.md @@ -0,0 +1,5 @@ +--- +'@clerk/react': patch +--- + +Prevent custom pages in profile components from remounting during parent rerenders. diff --git a/packages/react/src/components/ClerkHostRenderer.tsx b/packages/react/src/components/ClerkHostRenderer.tsx index 3e1304caebc..886d7057a8d 100644 --- a/packages/react/src/components/ClerkHostRenderer.tsx +++ b/packages/react/src/components/ClerkHostRenderer.tsx @@ -75,17 +75,18 @@ 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', 'customMenuItems', 'children'); - const newProps = without(this.props.props, 'customPages', 'customMenuItems', '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; + const customPagesChanged = _prevProps.props?.customPages?.length !== this.props.props?.customPages?.length; + const customMenuItemsChanged = + _prevProps.props?.customMenuItems?.length !== this.props.props?.customMenuItems?.length; // 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); + const prevMenuItemsWithoutHandlers = stripMenuItemIconHandlers(_prevProps.props?.customMenuItems); + const newMenuItemsWithoutHandlers = stripMenuItemIconHandlers(this.props.props?.customMenuItems); if ( !isDeeplyEqual(prevProps, newProps) || diff --git a/packages/react/src/components/__tests__/ClerkHostRenderer.test.tsx b/packages/react/src/components/__tests__/ClerkHostRenderer.test.tsx new file mode 100644 index 00000000000..02f06a08237 --- /dev/null +++ b/packages/react/src/components/__tests__/ClerkHostRenderer.test.tsx @@ -0,0 +1,74 @@ +import { render } from '@testing-library/react'; +import React from 'react'; +import { describe, expect, it, vi } from 'vitest'; + +import { ClerkHostRenderer } from '../ClerkHostRenderer'; + +vi.mock('@clerk/shared/object', () => ({ + without: (obj: Record, ...keys: string[]) => + Object.fromEntries(Object.entries(obj).filter(([key]) => !keys.includes(key))), +})); + +vi.mock('@clerk/shared/react', () => ({ + isDeeplyEqual: (a: unknown, b: unknown) => JSON.stringify(a) === JSON.stringify(b), +})); + +describe('', () => { + it('does not throw when mounted component props are omitted during updates', () => { + const mount = vi.fn(); + const unmount = vi.fn(); + const updateProps = vi.fn(); + + const { rerender } = render( + , + ); + + expect(() => + rerender( + , + ), + ).not.toThrow(); + + expect(updateProps).not.toHaveBeenCalled(); + }); + + it('updates mounted component props when custom pages are added or removed', () => { + const mount = vi.fn(); + const unmount = vi.fn(); + const updateProps = vi.fn(); + + const { rerender } = render( + , + ); + + rerender( + , + ); + + expect(updateProps).toHaveBeenCalledTimes(1); + expect(updateProps).toHaveBeenCalledWith({ + node: expect.any(HTMLDivElement), + props: { customPages: [{ label: 'General' }, { label: 'Permissions' }] }, + }); + }); +}); diff --git a/packages/react/src/utils/__tests__/useCustomElementPortal.test.tsx b/packages/react/src/utils/__tests__/useCustomElementPortal.test.tsx new file mode 100644 index 00000000000..4e0bf2504c7 --- /dev/null +++ b/packages/react/src/utils/__tests__/useCustomElementPortal.test.tsx @@ -0,0 +1,115 @@ +import { render, screen } from '@testing-library/react'; +import React, { useEffect } from 'react'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import { useCustomElementPortal } from '../useCustomElementPortal'; + +describe('useCustomElementPortal', () => { + let portalRoot: HTMLDivElement; + + afterEach(() => { + portalRoot?.remove(); + }); + + it('does not remount portal content when the parent rerenders', async () => { + const mountTracker = vi.fn(); + const unmountTracker = vi.fn(); + + const CustomContent = ({ label }: { label: string }) => { + useEffect(() => { + mountTracker(); + return unmountTracker; + }, []); + + return
{label}
; + }; + + const TestComponent = ({ label }: { label: string }) => { + const [{ mount, portal: Portal, unmount }] = useCustomElementPortal([ + { component: , id: 0 }, + ]); + + useEffect(() => { + mount(portalRoot); + return unmount; + }, [mount, unmount]); + + return ; + }; + + portalRoot = document.createElement('div'); + document.body.appendChild(portalRoot); + + const { rerender } = render(); + + await screen.findByText('first render'); + expect(mountTracker).toHaveBeenCalledTimes(1); + + rerender(); + + await screen.findByText('second render'); + expect(mountTracker).toHaveBeenCalledTimes(1); + expect(unmountTracker).not.toHaveBeenCalled(); + + expect(portalRoot.textContent).toBe('second render'); + }); + + it('renders falsy ReactNode values after mounting', async () => { + const TestComponent = () => { + const [{ mount, portal: Portal, unmount }] = useCustomElementPortal([{ component: 0, id: 0 }]); + + useEffect(() => { + mount(portalRoot); + return unmount; + }, [mount, unmount]); + + return ; + }; + + portalRoot = document.createElement('div'); + document.body.appendChild(portalRoot); + + render(); + + await screen.findByText('0'); + expect(portalRoot.textContent).toBe('0'); + }); + + it('keeps string and number ids in separate portal caches', async () => { + const TestComponent = () => { + const [ + { mount: mountNumber, portal: NumberPortal, unmount: unmountNumber }, + { mount: mountString, portal: StringPortal, unmount: unmountString }, + ] = useCustomElementPortal([ + { component:
number id
, id: 1 }, + { component:
string id
, id: '1' }, + ]); + + useEffect(() => { + mountNumber(portalRoot); + mountString(portalRoot); + + return () => { + unmountNumber(); + unmountString(); + }; + }, [mountNumber, mountString, unmountNumber, unmountString]); + + return ( + <> + + + + ); + }; + + portalRoot = document.createElement('div'); + document.body.appendChild(portalRoot); + + render(); + + await screen.findByText('number id'); + await screen.findByText('string id'); + expect(portalRoot.textContent).toBe('number idstring id'); + }); +}); diff --git a/packages/react/src/utils/__tests__/useCustomMenuItems.test.tsx b/packages/react/src/utils/__tests__/useCustomMenuItems.test.tsx index b748226b250..ba6821158b2 100644 --- a/packages/react/src/utils/__tests__/useCustomMenuItems.test.tsx +++ b/packages/react/src/utils/__tests__/useCustomMenuItems.test.tsx @@ -135,4 +135,64 @@ describe('useUserButtonCustomMenuItems', () => { expect(mockOnClick).toHaveBeenCalledTimes(1); } }); + + it('uses separate portals for duplicate non-keyed menu items', () => { + const children = ( + + First icon} + open='/duplicate' + /> + Second icon} + open='/duplicate' + /> + + ); + + const { result } = renderHook(() => useUserButtonCustomMenuItems(children)); + + expect(result.current.customMenuItems).toHaveLength(2); + expect(result.current.customMenuItemsPortals).toHaveLength(2); + expect(result.current.customMenuItemsPortals[0]).not.toBe(result.current.customMenuItemsPortals[1]); + }); + + it('keeps portal identity with the logical menu item when inserting before it', () => { + const firstItem = ( + First icon} + onClick={() => {}} + /> + ); + const secondItem = ( + Second icon} + onClick={() => {}} + /> + ); + const makeChildren = (includeFirstItem: boolean) => ( + {includeFirstItem ? [firstItem, secondItem] : [secondItem]} + ); + + const { result, rerender } = renderHook( + ({ includeFirstItem }) => useUserButtonCustomMenuItems(makeChildren(includeFirstItem)), + { + initialProps: { includeFirstItem: false }, + }, + ); + + const secondItemIconPortal = result.current.customMenuItemsPortals[0]; + + rerender({ includeFirstItem: true }); + + expect(result.current.customMenuItems).toHaveLength(2); + expect(result.current.customMenuItemsPortals[0]).not.toBe(secondItemIconPortal); + expect(result.current.customMenuItemsPortals[1]).toBe(secondItemIconPortal); + }); }); diff --git a/packages/react/src/utils/__tests__/useCustomPages.test.tsx b/packages/react/src/utils/__tests__/useCustomPages.test.tsx new file mode 100644 index 00000000000..afc7cfc9d23 --- /dev/null +++ b/packages/react/src/utils/__tests__/useCustomPages.test.tsx @@ -0,0 +1,84 @@ +import { renderHook } from '@testing-library/react'; +import React from 'react'; +import { describe, expect, it, vi } from 'vitest'; + +import { OrganizationProfilePage } from '../../components/uiComponents'; +import { useOrganizationProfileCustomPages } from '../useCustomPages'; + +vi.mock('@clerk/shared', () => ({ + logErrorInDevMode: vi.fn(), +})); + +describe('useOrganizationProfileCustomPages', () => { + it('uses separate portals for duplicate non-keyed custom pages', () => { + const children = [ + React.createElement( + OrganizationProfilePage, + { + label: 'Duplicate', + labelIcon:
First icon
, + url: 'duplicate', + }, +
First content
, + ), + React.createElement( + OrganizationProfilePage, + { + label: 'Duplicate', + labelIcon:
Second icon
, + url: 'duplicate', + }, +
Second content
, + ), + ]; + + const { result } = renderHook(() => useOrganizationProfileCustomPages(children)); + + expect(result.current.customPages).toHaveLength(2); + expect(result.current.customPagesPortals).toHaveLength(4); + expect(result.current.customPagesPortals[0]).not.toBe(result.current.customPagesPortals[2]); + expect(result.current.customPagesPortals[1]).not.toBe(result.current.customPagesPortals[3]); + }); + + it('keeps portal identity with the logical custom page when inserting before it', () => { + const firstPage = ( + First icon} + url='first' + > +
First content
+ + ); + const secondPage = ( + Second icon} + url='second' + > +
Second content
+ + ); + const makeChildren = (includeFirstPage: boolean) => (includeFirstPage ? [firstPage, secondPage] : [secondPage]); + + const { result, rerender } = renderHook( + ({ includeFirstPage }) => useOrganizationProfileCustomPages(makeChildren(includeFirstPage)), + { + initialProps: { includeFirstPage: false }, + }, + ); + + const secondPageContentPortal = result.current.customPagesPortals[0]; + const secondPageIconPortal = result.current.customPagesPortals[1]; + + rerender({ includeFirstPage: true }); + + expect(result.current.customPages).toHaveLength(2); + expect(result.current.customPagesPortals[0]).not.toBe(secondPageContentPortal); + expect(result.current.customPagesPortals[1]).not.toBe(secondPageIconPortal); + expect(result.current.customPagesPortals[2]).toBe(secondPageContentPortal); + expect(result.current.customPagesPortals[3]).toBe(secondPageIconPortal); + }); +}); diff --git a/packages/react/src/utils/useCustomElementPortal.tsx b/packages/react/src/utils/useCustomElementPortal.tsx index c0bf7e39b23..0ac9d20b9a3 100644 --- a/packages/react/src/utils/useCustomElementPortal.tsx +++ b/packages/react/src/utils/useCustomElementPortal.tsx @@ -1,36 +1,66 @@ import type React from 'react'; -import { useState } from 'react'; +import { useRef, useState } from 'react'; import { createPortal } from 'react-dom'; export type UseCustomElementPortalParams = { component: React.ReactNode; - id: number; + id: string | number; }; export type UseCustomElementPortalReturn = { - portal: () => React.JSX.Element; + portal: React.ComponentType; mount: (node: Element) => void; unmount: () => void; - id: number; + id: string | number; }; +type PortalKey = string | number; + // This function takes a component as prop, and returns functions that mount and unmount // the given component into a given node export const useCustomElementPortal = (elements: UseCustomElementPortalParams[]) => { - const [nodeMap, setNodeMap] = useState>(new Map()); - - return elements.map(el => ({ - id: el.id, - mount: (node: Element) => setNodeMap(prev => new Map(prev).set(String(el.id), node)), - unmount: () => - setNodeMap(prev => { - const newMap = new Map(prev); - newMap.set(String(el.id), null); - return newMap; - }), - portal: () => { - const node = nodeMap.get(String(el.id)); - return node ? createPortal(el.component, node) : null; - }, - })); + const [nodeMap, setNodeMap] = useState>(new Map()); + const nodeMapRef = useRef(nodeMap); + const elementsRef = useRef>(new Map()); + const portalsRef = useRef>(new Map()); + + nodeMapRef.current = nodeMap; + elementsRef.current = new Map(elements.map(el => [el.id, el.component])); + + const elementIds = new Set(elements.map(el => el.id)); + portalsRef.current.forEach((_, id) => { + if (!elementIds.has(id)) { + portalsRef.current.delete(id); + } + }); + + return elements.map(el => { + const id = el.id; + const existingPortal = portalsRef.current.get(id); + + if (existingPortal) { + return existingPortal; + } + + const portal: React.ComponentType = () => { + const node = nodeMapRef.current.get(id); + const component = elementsRef.current.get(id); + return node ? createPortal(component, node) : null; + }; + + const customElementPortal = { + id: el.id, + mount: (node: Element) => setNodeMap(prev => new Map(prev).set(id, node)), + unmount: () => + setNodeMap(prev => { + const newMap = new Map(prev); + newMap.set(id, null); + return newMap; + }), + portal, + }; + + portalsRef.current.set(id, customElementPortal); + return customElementPortal; + }); }; diff --git a/packages/react/src/utils/useCustomMenuItems.tsx b/packages/react/src/utils/useCustomMenuItems.tsx index 59385ea423d..62cdc84a02a 100644 --- a/packages/react/src/utils/useCustomMenuItems.tsx +++ b/packages/react/src/utils/useCustomMenuItems.tsx @@ -43,7 +43,7 @@ type UseCustomMenuItemsParams = { allowForAnyChildren?: boolean; }; -type CustomMenuItemType = UserButtonActionProps | UserButtonLinkProps; +type CustomMenuItemType = (UserButtonActionProps | UserButtonLinkProps) & { portalId?: string }; const useCustomMenuItems = ({ children, @@ -58,6 +58,7 @@ const useCustomMenuItems = ({ const validChildren: CustomMenuItemType[] = []; const customMenuItems: CustomMenuItem[] = []; const customMenuItemsPortals: React.ComponentType[] = []; + const portalIdCounts = new Map(); React.Children.forEach(children, child => { if ( @@ -89,6 +90,7 @@ const useCustomMenuItems = ({ } const { props } = child as ReactElement; + const childKey = (child as ReactElement).key; const { label, labelIcon, href, onClick, open } = props; @@ -106,11 +108,13 @@ const useCustomMenuItems = ({ validChildren.push({ ...baseItem, onClick, + portalId: getCustomMenuItemPortalId('action', props, childKey, portalIdCounts), }); } else if (open !== undefined) { validChildren.push({ ...baseItem, open: open.startsWith('/') ? open : `/${open}`, + portalId: getCustomMenuItemPortalId('action', props, childKey, portalIdCounts), }); } else { // Handle the case where neither onClick nor open is defined @@ -125,7 +129,12 @@ const useCustomMenuItems = ({ if (isThatComponent(child, MenuLinkComponent)) { if (isExternalLink(props)) { - validChildren.push({ label, labelIcon, href }); + validChildren.push({ + label, + labelIcon, + href, + portalId: getCustomMenuItemPortalId('link', props, childKey, portalIdCounts), + }); } else { logErrorInDevMode(userButtonMenuItemLinkWrongProps); return; @@ -138,10 +147,10 @@ const useCustomMenuItems = ({ const customLinkLabelIcons: UseCustomElementPortalParams[] = []; validChildren.forEach((mi, index) => { if (isCustomMenuItem(mi)) { - customMenuItemLabelIcons.push({ component: mi.labelIcon, id: index }); + customMenuItemLabelIcons.push({ component: mi.labelIcon, id: mi.portalId || index }); } if (isExternalLink(mi)) { - customLinkLabelIcons.push({ component: mi.labelIcon, id: index }); + customLinkLabelIcons.push({ component: mi.labelIcon, id: mi.portalId || index }); } }); @@ -159,7 +168,7 @@ const useCustomMenuItems = ({ portal: iconPortal, mount: mountIcon, unmount: unmountIcon, - } = customMenuItemLabelIconsPortals.find(p => p.id === index) as UseCustomElementPortalReturn; + } = customMenuItemLabelIconsPortals.find(p => p.id === (mi.portalId || index)) as UseCustomElementPortalReturn; const menuItem: CustomMenuItem = { label: mi.label, mountIcon, @@ -179,7 +188,7 @@ const useCustomMenuItems = ({ portal: iconPortal, mount: mountIcon, unmount: unmountIcon, - } = customLinkLabelIconsPortals.find(p => p.id === index) as UseCustomElementPortalReturn; + } = customLinkLabelIconsPortals.find(p => p.id === (mi.portalId || index)) as UseCustomElementPortalReturn; customMenuItems.push({ label: mi.label, href: mi.href, @@ -193,6 +202,23 @@ const useCustomMenuItems = ({ return { customMenuItems, customMenuItemsPortals }; }; +const getCustomMenuItemPortalId = ( + type: 'action' | 'link', + props: Pick & { href?: string; open?: string }, + key: React.Key | null, + portalIdCounts: Map, +) => { + if (key != null) { + return `${type}:key:${key}`; + } + + const target = props.href || props.open || ''; + const baseId = `${type}:${props.label}:${target}`; + const occurrence = portalIdCounts.get(baseId) ?? 0; + portalIdCounts.set(baseId, occurrence + 1); + return `${baseId}:${occurrence}`; +}; + const isReorderItem = (childProps: any, validItems: string[]): boolean => { const { children, label, onClick, labelIcon } = childProps; return !children && !onClick && !labelIcon && validItems.some(v => v === label); diff --git a/packages/react/src/utils/useCustomPages.tsx b/packages/react/src/utils/useCustomPages.tsx index 6a271188616..e0a153338d6 100644 --- a/packages/react/src/utils/useCustomPages.tsx +++ b/packages/react/src/utils/useCustomPages.tsx @@ -64,7 +64,7 @@ type UseCustomPagesOptions = { allowForAnyChildren: boolean; }; -type CustomPageWithIdType = UserProfilePageProps & { children?: React.ReactNode }; +type CustomPageWithIdType = UserProfilePageProps & { children?: React.ReactNode; portalId?: string }; /** * Exclude any children that is used for identifying Custom Pages or Custom Items. @@ -104,6 +104,7 @@ const useCustomPages = (params: UseCustomPagesParams, options?: UseCustomPagesOp const { children, LinkComponent, PageComponent, MenuItemsComponent, reorderItemsLabels, componentName } = params; const { allowForAnyChildren = false } = options || {}; const validChildren: CustomPageWithIdType[] = []; + const portalIdCounts = new Map(); React.Children.forEach(children, child => { if ( @@ -121,13 +122,21 @@ const useCustomPages = (params: UseCustomPagesParams, options?: UseCustomPagesOp const { children, label, url, labelIcon } = props; + const childKey = (child as ReactElement).key; + if (isThatComponent(child, PageComponent)) { if (isReorderItem(props, reorderItemsLabels)) { // This is a reordering item validChildren.push({ label }); } else if (isCustomPage(props)) { // this is a custom page - validChildren.push({ label, labelIcon, children, url }); + validChildren.push({ + label, + labelIcon, + children, + url, + portalId: getCustomPagePortalId('page', props, childKey, portalIdCounts), + }); } else { logErrorInDevMode(customPageWrongProps(componentName)); return; @@ -137,7 +146,12 @@ const useCustomPages = (params: UseCustomPagesParams, options?: UseCustomPagesOp if (isThatComponent(child, LinkComponent)) { if (isExternalLink(props)) { // This is an external link - validChildren.push({ label, labelIcon, url }); + validChildren.push({ + label, + labelIcon, + url, + portalId: getCustomPagePortalId('link', props, childKey, portalIdCounts), + }); } else { logErrorInDevMode(customLinkWrongProps(componentName)); return; @@ -151,12 +165,12 @@ const useCustomPages = (params: UseCustomPagesParams, options?: UseCustomPagesOp validChildren.forEach((cp, index) => { if (isCustomPage(cp)) { - customPageContents.push({ component: cp.children, id: index }); - customPageLabelIcons.push({ component: cp.labelIcon, id: index }); + customPageContents.push({ component: cp.children, id: cp.portalId || index }); + customPageLabelIcons.push({ component: cp.labelIcon, id: cp.portalId || index }); return; } if (isExternalLink(cp)) { - customLinkLabelIcons.push({ component: cp.labelIcon, id: index }); + customLinkLabelIcons.push({ component: cp.labelIcon, id: cp.portalId || index }); } }); @@ -177,12 +191,12 @@ const useCustomPages = (params: UseCustomPagesParams, options?: UseCustomPagesOp portal: contentPortal, mount, unmount, - } = customPageContentsPortals.find(p => p.id === index) as UseCustomElementPortalReturn; + } = customPageContentsPortals.find(p => p.id === (cp.portalId || index)) as UseCustomElementPortalReturn; const { portal: labelPortal, mount: mountIcon, unmount: unmountIcon, - } = customPageLabelIconsPortals.find(p => p.id === index) as UseCustomElementPortalReturn; + } = customPageLabelIconsPortals.find(p => p.id === (cp.portalId || index)) as UseCustomElementPortalReturn; customPages.push({ label: cp.label, url: cp.url, mount, unmount, mountIcon, unmountIcon }); customPagesPortals.push(contentPortal); customPagesPortals.push(labelPortal); @@ -193,7 +207,7 @@ const useCustomPages = (params: UseCustomPagesParams, options?: UseCustomPagesOp portal: labelPortal, mount: mountIcon, unmount: unmountIcon, - } = customLinkLabelIconsPortals.find(p => p.id === index) as UseCustomElementPortalReturn; + } = customLinkLabelIconsPortals.find(p => p.id === (cp.portalId || index)) as UseCustomElementPortalReturn; customPages.push({ label: cp.label, url: cp.url, mountIcon, unmountIcon }); customPagesPortals.push(labelPortal); return; @@ -203,6 +217,22 @@ const useCustomPages = (params: UseCustomPagesParams, options?: UseCustomPagesOp return { customPages, customPagesPortals }; }; +const getCustomPagePortalId = ( + type: 'page' | 'link', + props: Pick, + key: React.Key | null, + portalIdCounts: Map, +) => { + if (key != null) { + return `${type}:key:${key}`; + } + + const baseId = `${type}:${props.label}:${props.url}`; + const occurrence = portalIdCounts.get(baseId) ?? 0; + portalIdCounts.set(baseId, occurrence + 1); + return `${baseId}:${occurrence}`; +}; + const isReorderItem = (childProps: any, validItems: string[]): boolean => { const { children, label, url, labelIcon } = childProps; return !children && !url && !labelIcon && validItems.some(v => v === label);