Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: improve presence menu performance #8039

Open
wants to merge 1 commit into
base: add-million-lint-mode
Choose a base branch
from
Open

fix: improve presence menu performance #8039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/sanity/src/core/studio/components/navbar/presence/PresenceMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ export function PresenceMenu() {
focused={focusedId === item.user.id}
key={item.user.id}
onFocus={handleItemFocus}
presence={item}
locations={item.locations}
user={item.user}
/>
))}

Expand Down
71 changes: 36 additions & 35 deletions packages/sanity/src/core/studio/components/navbar/presence/PresenceMenuItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as PathUtils from '@sanity/util/paths'
import {orderBy} from 'lodash'
import {type ForwardedRef, forwardRef, memo, useCallback, useEffect, useMemo, useState} from 'react'
import {memo, useCallback, useEffect, useState} from 'react'
import {IntentLink} from 'sanity/router'

import {MenuItem} from '../../../../../ui-components'
Expand All @@ -11,19 +11,15 @@ import {type GlobalPresence} from '../../../../store'
interface PresenceListRowProps {
focused: boolean
onFocus: (id: string) => void
presence: GlobalPresence
locations: GlobalPresence['locations']
user: GlobalPresence['user']
}

export const PresenceMenuItem = memo(function PresenceMenuItem(props: PresenceListRowProps) {
const {presence, focused, onFocus} = props
const {locations, user, focused, onFocus} = props
const [menuItemElement, setMenuItemElement] = useState<HTMLElement | null>(null)
const {t} = useTranslation()

const lastActiveLocation = orderBy(presence.locations || [], ['lastActiveAt'], ['desc']).find(
(location) => location.documentId,
)
const hasLink = Boolean(lastActiveLocation?.documentId)

/**
* This is a workaround to keep focus on the selected menu item
* when the list of users in the menu is updated
Expand All @@ -40,42 +36,47 @@ export const PresenceMenuItem = memo(function PresenceMenuItem(props: PresenceLi
}, [menuItemElement, focused])

const handleFocus = useCallback(() => {
onFocus(presence.user.id)
}, [onFocus, presence.user.id])
onFocus(user.id)
}, [onFocus, user.id])

const LinkComponent = useMemo(
() =>
// eslint-disable-next-line @typescript-eslint/no-shadow
forwardRef(function LinkComponent(linkProps, ref: ForwardedRef<HTMLAnchorElement>) {
if (!lastActiveLocation?.path) return null

return (
<IntentLink
{...linkProps}
intent="edit"
params={{
id: lastActiveLocation?.documentId,
path: PathUtils.toString(lastActiveLocation?.path),
}}
ref={ref}
/>
)
}),
[lastActiveLocation],
Comment on lines -46 to -64
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason React Compiler bails out of this component here.
I tried removing the useMemo to leave it to the compiler how to memoize it, and the bug was still there.

Turns out the problem is that defining components inline like this isn't a supported pattern.
By refactoring <MenuItem> to use a stable as prop to IntentLink, and give it props and live it to styled-components to forward props, it works with the compiler as well.

const lastActiveLocation = orderBy(locations || [], ['lastActiveAt'], ['desc']).find(
(location) => location.documentId,
)
const hasLink = Boolean(lastActiveLocation?.documentId)

if (lastActiveLocation) {
return (
<MenuItem
as={IntentLink}
// @ts-expect-error - `intent` is valid when using `IntentLink`
intent="edit"
params={{
id: lastActiveLocation.documentId,
path: PathUtils.toString(lastActiveLocation.path),
}}
// Shared props
data-as="a"
onFocus={handleFocus}
preview={<UserAvatar size={1} user={user} />}
ref={setMenuItemElement}
text={user.displayName}
/>
)
}

return (
<MenuItem
as={lastActiveLocation ? LinkComponent : 'div'}
data-as="a"
as="div"
disabled={!hasLink}
onFocus={handleFocus}
preview={<UserAvatar key={presence.user.id} size={1} user={presence.user} />}
ref={setMenuItemElement}
text={presence.user.displayName}
tooltipProps={
hasLink ? undefined : {content: t('presence.not-in-a-document'), placement: 'left'}
}
// Shared props
data-as="a"
onFocus={handleFocus}
preview={<UserAvatar size={1} user={user} />}
ref={setMenuItemElement}
text={user.displayName}
/>
)
})
Loading