Skip to content

Commit

Permalink
fix: don't read/write ref during render (#8077)
Browse files Browse the repository at this point in the history
  • Loading branch information
stipsan authored Dec 18, 2024
1 parent 260faec commit b1f183d
Show file tree
Hide file tree
Showing 40 changed files with 308 additions and 322 deletions.
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"check:deps": "pnpm --recursive --parallel exec depcheck",
"check:format": "prettier . --check",
"check:lint": "turbo run lint --continue -- --quiet",
"check:react-compiler": "eslint --cache --no-inline-config --no-eslintrc --ext .cjs,.mjs,.js,.jsx,.ts,.tsx --parser @typescript-eslint/parser --plugin react-compiler --rule 'react-compiler/react-compiler: [warn]' --ignore-path .eslintignore.react-compiler --max-warnings 76 .",
"check:react-compiler": "eslint --cache --no-inline-config --no-eslintrc --ext .cjs,.mjs,.js,.jsx,.ts,.tsx --parser @typescript-eslint/parser --plugin react-compiler --rule 'react-compiler/react-compiler: [warn]' --ignore-path .eslintignore.react-compiler --max-warnings 28 .",
"report:react-compiler-bailout": "eslint --cache --no-inline-config --no-eslintrc --ext .cjs,.mjs,.js,.jsx,.ts,.tsx --parser @typescript-eslint/parser --plugin react-compiler --rule 'react-compiler/react-compiler: [error,{__unstable_donotuse_reportAllBailouts:true}]' --ignore-path .eslintignore.react-compiler -f ./scripts/reactCompilerBailouts.cjs . || true",
"check:test": "run-s test -- --silent",
"check:types": "tsc && turbo run check:types --filter='./packages/*' --filter='./packages/@sanity/*'",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {useRef, useState} from 'react'
import React, {useState} from 'react'
import {
Card,
Container,
Expand Down Expand Up @@ -32,8 +32,8 @@ export const GetStartedTutorial = () => {
)

const {sanity} = useTheme()
const rootElement = useRef(null)
const rect = useElementSize(rootElement.current)
const [rootElement, setRootElement] = useState<HTMLDivElement | null>(null)
const rect = useElementSize(rootElement)
const width = rect?.content?.width
const isSmallScreen = width ? width < sanity.media[1] : false
const isProdEnv = process.env.NODE_ENV !== 'development'
Expand All @@ -48,7 +48,7 @@ export const GetStartedTutorial = () => {
}

return (
<div ref={rootElement}>
<div ref={setRootElement}>
<Card tone="primary" padding={isSmallScreen ? 3 : 5} paddingBottom={isSmallScreen ? 4 : 6}>
<Flex justify={isSmallScreen ? 'space-between' : 'flex-end'} align="center">
{isSmallScreen && (
Expand Down
1 change: 1 addition & 0 deletions packages/sanity/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@
"tar-fs": "^2.1.1",
"tar-stream": "^3.1.7",
"use-device-pixel-ratio": "^1.1.0",
"use-effect-event": "^1.0.2",
"use-hot-module-reload": "^2.0.0",
"use-sync-external-store": "^1.2.0",
"vite": "^5.4.11",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ export const CommentsListItem = memo(function CommentsListItem(props: CommentsLi
const {t} = useTranslation(commentsLocaleNamespace)
const [value, setValue] = useState<CommentMessage>(EMPTY_ARRAY)
const [collapsed, setCollapsed] = useState<boolean>(true)
const didExpand = useRef<boolean>(false)
const [didExpand, setDidExpand] = useState(false)
const replyInputRef = useRef<CommentInputHandle>(null)

const {isTopLayer} = useLayer()
Expand Down Expand Up @@ -238,7 +238,7 @@ export const CommentsListItem = memo(function CommentsListItem(props: CommentsLi
const handleExpand = useCallback((e: MouseEvent<HTMLButtonElement>) => {
e.stopPropagation()
setCollapsed(false)
didExpand.current = true
setDidExpand(true)
}, [])

const splicedReplies = useMemo(() => {
Expand All @@ -258,10 +258,10 @@ export const CommentsListItem = memo(function CommentsListItem(props: CommentsLi
}, [replies?.length])

useEffect(() => {
if (replies.length > MAX_COLLAPSED_REPLIES && !didExpand.current) {
if (replies.length > MAX_COLLAPSED_REPLIES && !didExpand) {
setCollapsed(true)
}
}, [replies])
}, [didExpand, replies])

const renderedReplies = useMemo(
() =>
Expand Down Expand Up @@ -356,7 +356,7 @@ export const CommentsListItem = memo(function CommentsListItem(props: CommentsLi
/>
</Stack>

{showCollapseButton && !didExpand.current && (
{showCollapseButton && !didExpand && (
<Flex gap={1} paddingY={1} sizing="border">
<SpacerAvatar />

Expand Down Expand Up @@ -397,3 +397,4 @@ export const CommentsListItem = memo(function CommentsListItem(props: CommentsLi
</StyledThreadCard>
)
})
CommentsListItem.displayName = 'Memo(CommentsListItem)'
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {useClickOutsideEvent} from '@sanity/ui'
// eslint-disable-next-line camelcase
import {getTheme_v2} from '@sanity/ui/theme'
import {isEqual} from 'lodash'
import {type KeyboardEvent, useCallback, useEffect, useMemo, useRef} from 'react'
import {type KeyboardEvent, useCallback, useEffect, useMemo, useRef, useState} from 'react'
import {css, styled} from 'styled-components'

import {Popover, type PopoverProps} from '../../../../../ui-components'
Expand Down Expand Up @@ -85,8 +85,8 @@ export function Editable(props: EditableProps) {
renderBlock,
} = props
const popoverRef = useRef<HTMLDivElement | null>(null)
const rootElementRef = useRef<HTMLDivElement | null>(null)
const editableRef = useRef<HTMLDivElement | null>(null)
const [rootElement, setRootElement] = useState<HTMLDivElement | null>(null)
const [inputElement, setInputElement] = useState<HTMLDivElement | null>(null)
const mentionsMenuRef = useRef<MentionsMenuHandle | null>(null)

const selection = usePortableTextEditorSelection()
Expand All @@ -104,7 +104,7 @@ export function Editable(props: EditableProps) {

const cursorElement = useCursorElement({
disabled: !mentionsMenuOpen,
rootElement: rootElementRef.current,
rootElement: rootElement,
})

const renderPlaceholder = useCallback(
Expand Down Expand Up @@ -195,7 +195,7 @@ export function Editable(props: EditableProps) {

const popoverContent = (
<MentionsMenu
inputElement={editableRef.current}
inputElement={inputElement}
loading={mentionOptions.loading}
onSelect={insertMention}
options={mentionOptions.data || EMPTY_ARRAY}
Expand All @@ -204,7 +204,7 @@ export function Editable(props: EditableProps) {
)

return (
<div ref={rootElementRef}>
<div ref={setRootElement}>
<StyledPopover
arrow={false}
constrainSize
Expand All @@ -216,15 +216,14 @@ export function Editable(props: EditableProps) {
ref={popoverRef}
referenceElement={cursorElement}
/>

<PortableTextEditable
data-testid="comment-input-editable"
data-ui="EditableElement"
onBeforeInput={onBeforeInput}
onBlur={onBlur}
onFocus={onFocus}
onKeyDown={handleKeyDown}
ref={editableRef}
ref={setInputElement}
renderBlock={renderBlock}
renderChild={renderChild}
renderPlaceholder={renderPlaceholder}
Expand All @@ -235,3 +234,4 @@ export function Editable(props: EditableProps) {
</div>
)
}
Editable.displayName = 'Editable'
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {
Flex,
Text,
} from '@sanity/ui'
import {memo, useCallback, useMemo, useRef} from 'react'
import {memo, useCallback, useMemo, useState} from 'react'

import {Tooltip, TooltipDelayGroupProvider} from '../../../../ui-components'
import {COMMENT_REACTION_EMOJIS, COMMENT_REACTION_OPTIONS} from '../../constants'
Expand Down Expand Up @@ -110,16 +110,22 @@ export const CommentReactionsBar = memo(function CommentReactionsBar(
return grouped.filter(([name]) => COMMENT_REACTION_EMOJIS[name])
}, [reactions])

// An array of the initial order of the reactions. This is used to sort the reactions.
// E.g. [':+1:', ':heart:']
const sortOrder = useRef<string[]>(Object.keys(Object.fromEntries(groupedReactions)))

const [sortedGroupedReactions, setSortedGroupedReactions] = useState(() => ({
// An array of the initial order of the reactions. This is used to sort the reactions.
// E.g. [':+1:', ':heart:']
sortOrder: Object.keys(Object.fromEntries(groupedReactions)),
// We cache the groupedReactions to know when we should update the sortedReactions, ensuring we don't run into an infinite render loop.
groupedReactions: [] as typeof groupedReactions,
sortedReactions: [] as typeof groupedReactions,
}))
// Sort the reactions based on the initial order to make sure that the reactions
// are not jumping around when new reactions are added.
const sortedReactions = useMemo(() => {
const sorted = groupedReactions.sort(([nameA], [nameB]) => {
const indexA = sortOrder.current.indexOf(nameA)
const indexB = sortOrder.current.indexOf(nameB)
let {sortedReactions} = sortedGroupedReactions
if (sortedGroupedReactions.groupedReactions !== groupedReactions) {
const {sortOrder} = sortedGroupedReactions
sortedReactions = groupedReactions.sort(([nameA], [nameB]) => {
const indexA = sortOrder.indexOf(nameA)
const indexB = sortOrder.indexOf(nameB)

if (indexA === -1) {
return 1
Expand All @@ -132,10 +138,12 @@ export const CommentReactionsBar = memo(function CommentReactionsBar(
return indexA - indexB
})

sortOrder.current = sorted.map(([name]) => name)

return sorted
}, [groupedReactions])
setSortedGroupedReactions({
groupedReactions,
sortOrder: sortedReactions.map(([name]) => name),
sortedReactions,
})
}

return (
<Flex align="center" gap={1} wrap="wrap">
Expand Down Expand Up @@ -190,3 +198,4 @@ export const CommentReactionsBar = memo(function CommentReactionsBar(
</Flex>
)
})
CommentReactionsBar.displayName = 'Memo(CommentReactionsBar)'
6 changes: 3 additions & 3 deletions packages/sanity/src/core/comments/helpers.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import {isPortableTextSpan, isPortableTextTextBlock} from '@sanity/types'
import {isEqual} from 'lodash'
import {useMemo, useRef} from 'react'
import {useMemo, useState} from 'react'

import {type CommentContext, type CommentDocument, type CommentMessage} from './types'

export function useCommentHasChanged(message: CommentMessage): boolean {
const prevMessage = useRef<CommentMessage>(message)
const [prevMessage] = useState<CommentMessage>(message)

return useMemo(() => !isEqual(prevMessage.current, message), [message])
return useMemo(() => !isEqual(prevMessage, message), [prevMessage, message])
}

export function hasCommentMessageValue(value: CommentMessage): boolean {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {type RangeDecoration, type RangeDecorationOnMovedDetails} from '@portabletext/editor'
import {type PortableTextBlock} from '@sanity/types'
import {memo, useCallback, useEffect, useRef} from 'react'
import {memo, useCallback, useEffect, useRef, useState} from 'react'

import {CommentInlineHighlightSpan} from '../../components'
import {applyInlineCommentIdAttr} from '../../hooks'
Expand Down Expand Up @@ -32,8 +32,8 @@ const CommentRangeDecoration = memo(function CommentRangeDecoration(
threadId,
} = props
const decoratorRef = useRef<HTMLSpanElement | null>(null)
const isNestedRef = useRef<boolean>(false)
const parentCommentId = useRef<string | null>(null)
const [isNested, setIsNested] = useState(false)
const [parentCommentId, setParentCommentId] = useState<string | null>(null)

useEffect(() => {
// Get the previous and next sibling of the decorator element
Expand All @@ -42,7 +42,7 @@ const CommentRangeDecoration = memo(function CommentRangeDecoration(

// If there is no previous or next sibling, then the decorator is not nested
if (!prevEl || !nextEl) {
isNestedRef.current = false
setIsNested(false)
return
}

Expand All @@ -56,9 +56,9 @@ const CommentRangeDecoration = memo(function CommentRangeDecoration(
const isEqual = prevId === nextId

const isNestedDecorator = Boolean(prevId && nextId && isEqual)
parentCommentId.current = isNestedDecorator ? prevId : null
setParentCommentId(isNestedDecorator ? prevId : null)

isNestedRef.current = isNestedDecorator
setIsNested(isNestedDecorator)
}, [])

const handleMouseEnter = useCallback(() => onHoverStart(commentId), [commentId, onHoverStart])
Expand All @@ -67,15 +67,15 @@ const CommentRangeDecoration = memo(function CommentRangeDecoration(

const hovered =
currentHoveredCommentId === commentId ||
(currentHoveredCommentId === parentCommentId.current && isNestedRef.current)
(currentHoveredCommentId === parentCommentId && isNested)

const selected = selectedThreadId === threadId

return (
<CommentInlineHighlightSpan
isAdded
isHovered={hovered || selected}
isNested={isNestedRef.current}
isNested={isNested}
onClick={handleClick}
onMouseEnter={handleMouseEnter}
onMouseLeave={handleMouseLeave}
Expand All @@ -86,6 +86,7 @@ const CommentRangeDecoration = memo(function CommentRangeDecoration(
</CommentInlineHighlightSpan>
)
})
CommentRangeDecoration.displayName = 'Memo(CommentRangeDecoration)'

interface BuildRangeDecorationsProps {
comments: CommentDocument[]
Expand Down
6 changes: 2 additions & 4 deletions packages/sanity/src/core/field/conditional-property/utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {
type CurrentUser,
} from '@sanity/types'
import {omit} from 'lodash'
import {useMemo, useRef} from 'react'
import {useMemo} from 'react'

import {isRecord} from '../../util'

Expand All @@ -23,8 +23,6 @@ export function useCheckCondition(
): boolean {
const {currentUser, document, parent, value} = context

const didWarn = useRef(false)

return useMemo(() => {
let isTrueIsh = false

Expand All @@ -46,7 +44,7 @@ export function useCheckCondition(
return false
}

if (isThenable(isTrueIsh) && !didWarn.current) {
if (isThenable(isTrueIsh)) {
console.warn(
`The \`${checkPropertyName}\` option is either a promise or a promise returning function. Async callbacks for \`${checkPropertyName}\` option is not currently supported.`,
)
Expand Down
9 changes: 6 additions & 3 deletions packages/sanity/src/core/form/contexts/GetFormValue.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {type Path} from '@sanity/types'
import {type ReactNode, useCallback, useContext, useRef} from 'react'
import {type ReactNode, useCallback, useContext, useEffect, useRef} from 'react'
import {GetFormValueContext} from 'sanity/_singletons'

import {getValueAtPath} from '../../field'
Expand All @@ -10,18 +10,21 @@ import {type FormDocumentValue} from '../types'
* @internal
* @hidden
*/
export function GetFormValueProvider(props: {
export const GetFormValueProvider = function GetFormValueProvider(props: {
value: FormDocumentValue | undefined
children: ReactNode
}) {
const valueRef = useRef(props.value)
valueRef.current = props.value
useEffect(() => {
valueRef.current = props.value
}, [props.value])

const getValue = useCallback((path: Path) => getValueAtPath(valueRef.current, path), [valueRef])
return (
<GetFormValueContext.Provider value={getValue}>{props.children}</GetFormValueContext.Provider>
)
}
GetFormValueProvider.displayName = 'GetFormValueProvider'

/**
* React hook that returns a function that can be called to look up the value from the current document at the given path.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,8 @@ describe('useDidUpdate', () => {
wrapper: StrictMode,
})

// The first time, didUpdate should be called. StrictMode runs hooks twice https://dev/reference/react/StrictMode#strictmode
expect(didUpdate).toHaveBeenCalledTimes(2)
// StrictMode runs hooks twice https://dev/reference/react/StrictMode#strictmode didUpdate should still be called once
expect(didUpdate).toHaveBeenCalledTimes(1)

rerender({value: currentValue})

Expand All @@ -94,7 +94,7 @@ describe('useDidUpdate', () => {
wrapper: StrictMode,
})

// The first time, didUpdate should be called. StrictMode runs hooks twice https://dev/reference/react/StrictMode#strictmode
// StrictMode runs hooks twice https://dev/reference/react/StrictMode#strictmode didUpdate should still be called once
expect(didUpdate).toHaveBeenCalledTimes(2)
expect(compare).toHaveBeenCalledTimes(2)

Expand All @@ -114,8 +114,8 @@ describe('useDidUpdate', () => {
wrapper: StrictMode,
})

// The first time, didUpdate should be called. StrictMode runs hooks twice https://dev/reference/react/StrictMode#strictmode
expect(didUpdate).toHaveBeenCalledTimes(2)
// StrictMode runs hooks twice https://dev/reference/react/StrictMode#strictmode didUpdate should still be called once
expect(didUpdate).toHaveBeenCalledTimes(1)

didUpdate.mockClear()

Expand Down
Loading

0 comments on commit b1f183d

Please sign in to comment.