fix: portable text editor crash #6870

stipsan · 2024-06-07T00:17:35Z

Description

On React 19 the Portable Text Editor sometimes ends up in an infinite render loop when there's no value yet, as demonstrated here: https://test-compiled-studio-b8mfdmi85.sanity.build/test/structure/input-standard;portable-text;pt_allTheBellsAndWhistles;7455ba88-75d5-4a77-94a4-91738fa670c5

This PR fixes it: https://test-compiled-studio-git-fix-portable-text-editor-crash.sanity.build/test/structure/input-standard;portable-text;pt_allTheBellsAndWhistles;7455ba88-75d5-4a77-94a4-91738fa670c5

The PR also contains a lot of other minor fixes related to React Compiler, improvements to how the color scheme state is passed around, adding the React Component stack to the form input error boundary etc.

What to review

There shouldn't be any infinite render recursion loops happening in React 19 anymore for Portable Text Editor, try and break it 🙌

Testing

Beyond manual testing the existing e2e testing suite should be enough.

Notes for release

Fixed React 19 causing Portable Text Editor to crash in some scenarios.

vercel · 2024-06-07T00:17:41Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
page-building-studio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 7, 2024 8:26am
performance-studio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 7, 2024 8:26am
test-compiled-studio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 7, 2024 8:26am
test-next-studio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 7, 2024 8:26am
test-studio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 7, 2024 8:26am

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
studio-workshop	⬜️ Ignored (Inspect)	Visit Preview		Jun 7, 2024 8:26am

github-actions · 2024-06-07T00:20:40Z

No changes to documentation

github-actions · 2024-06-07T00:27:50Z

Component Testing Report Updated Jun 7, 2024 8:31 AM (UTC)

File	Status	Duration	Passed	Skipped
comments/CommentInput.spec.tsx	✅ Passed (Inspect)	37s	15	0
formBuilder/ArrayInput.spec.tsx	✅ Passed (Inspect)	6s	3	0
formBuilder/inputs/PortableText/Annotations.spec.tsx	✅ Passed (Inspect)	27s	6	0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx	✅ Passed (Inspect)	31s	11	7
formBuilder/inputs/PortableText/Decorators.spec.tsx	✅ Passed (Inspect)	14s	6	0
formBuilder/inputs/PortableText/DisableFocusAndUnset.spec.tsx	✅ Passed (Inspect)	8s	3	0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx	✅ Passed (Inspect)	36s	15	0
formBuilder/inputs/PortableText/Input.spec.tsx	✅ Passed (Inspect)	1m 16s	21	0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx	✅ Passed (Inspect)	1m 4s	18	0
formBuilder/inputs/PortableText/PresenceCursors.spec.tsx	✅ Passed (Inspect)	7s	3	9
formBuilder/inputs/PortableText/RangeDecoration.spec.tsx	✅ Passed (Inspect)	21s	9	0
formBuilder/inputs/PortableText/Styles.spec.tsx	✅ Passed (Inspect)	15s	6	0
formBuilder/inputs/PortableText/Toolbar.spec.tsx	✅ Passed (Inspect)	31s	12	0

stipsan · 2024-06-07T07:17:21Z

packages/sanity/src/core/studio/colorScheme.tsx

+  useEffect(() => {
+    console.warn(
+      'useColorScheme() is deprecated, use useColorSchemeValue() or useColorSchemeSetValue() instead',
+    )
+  }, [])


The @deprecated TSDoc haven't been sufficient to discourage using this hook, which is why it makes sense to console.warn in useEffect to push for possible userland to refactor away 🙌

might be nice to have another boolean outside this to prevent it from being logged more than once in cases where it's used in multiple components but more warnings means more annoying which may be more motivating lol

Yeah, I'm in favor of only logging once when it's not actionable. I'm this case the migration is very easy to do and I'd like to encourage it as much as possible 😬

christianhg · 2024-06-07T07:47:30Z

The PR also contains a lot of other minor fixes related to React Compiler, improvements to how the color scheme state is passed around, adding the React Component stack to the form input error boundary etc.

I'm curious, what was the actual culprit and what change fixed the issue?

ricokahler

amazing work. the <Code> change i’m curious about. lmk what u think!

ricokahler · 2024-06-07T07:57:07Z

packages/sanity/src/structure/panes/document/DocumentPane.tsx

@@ -115,6 +115,7 @@ function DocumentPaneInner(props: DocumentPaneProviderProps) {
              t={t}
              i18nKey="panes.document-pane.document-not-found.text"
              values={{id: options.id}}
+              components={{Code: ({children}) => <code>{children}</code>}}


why was this change introduced? i think the simple <code> tag (lowercase) should be supported so ideally the translation strings should be refactored instead but i may be missing something

It crashed and complained about Code not being declared instead of showing the error. Then I saw a commit from @rexxars that fixed the same error but from a different location. Hang on let's see if I can find it.

oh hmm. I was referencing #5114

It's a bit tedious to pass in components like <Code> that just maps to <code>. This PR allows using this predefined list of tags without specifying it directly: <abbr>, <address>, <cite>, <code>, <del>, <em>, <ins>, <kbd>, <q>, <samp>, <strong>, <sub>, <sup>.

the suggestion is to change any translation string to remove <Code> in favor of just <code> instead of adding support for <Code> but maybe we don't have control over those translation strings?

nbd though

Gotcha, I have to admit I'm not too familiar with how our i18n works yet. All I know is that adding this line allowed the 404 to render again, instead of crashing the Structure Tool and propagating to the StudioErrorBoundary that wraps the tool layout :D

ricokahler · 2024-06-07T07:59:35Z

packages/sanity/src/core/studio/colorScheme.tsx

+  useEffect(() => {
+    console.warn(
+      'useColorScheme() is deprecated, use useColorSchemeValue() or useColorSchemeSetValue() instead',
+    )
+  }, [])


might be nice to have another boolean outside this to prevent it from being logged more than once in cases where it's used in multiple components but more warnings means more annoying which may be more motivating lol

ricokahler · 2024-06-07T08:00:55Z

packages/sanity/src/core/studio/colorScheme.tsx

  const setValue = useContext(ColorSchemeSetValueContext)
  if (setValue === null) throw new Error('Could not find `ColorSchemeSetValueContext` context')
  return setValue
 }

 /** @internal */
-export function _useColorSchemeInternalValue(): StudioThemeColorSchemeKey {


do you think this hook needs to be exported? would refactoring it to not be exported be better? it seems like it was prefaced with an _ to prevent external usage but not exposing it does the same job

I'd be happy to do that in a quick follow-up! It's only needed internally so it doesn't need to be available on import 'sanity'

Btw the underscore caused the React Compiler to not see it as a hook, causing hook re-ordering errors, which is why it was changed here 🙌

Good to know 💡

ricokahler · 2024-06-07T08:03:05Z

packages/sanity/src/core/form/studio/FormBuilderInputErrorBoundary.tsx

@@ -38,8 +42,8 @@ export function FormBuilderInputErrorBoundary(
 * when there are no errors.
 * @internal
 */
-function ErrorCard(props: {error: unknown; onRetry: () => void}) {
-  const {error, onRetry} = props
+function ErrorCard(props: {error: unknown; info?: React.ErrorInfo; onRetry: () => void}) {


just double checking but is React.ErrorInfo available in the react 18 types as well?

Yup! We don't use React 19 types yet, and we might not do so before we drop 18 support, depending on wether it'll be possible to use both (framer-motion tried but couldn't get it to work, we'll see once 19 is stable and the new typings are available on @types/reactinstead of the temporarytypes-react@rc`) 🙌

ricokahler · 2024-06-07T08:08:01Z

packages/sanity/src/core/form/studio/FormBuilderInputErrorBoundary.tsx

@@ -22,14 +23,17 @@ export function FormBuilderInputErrorBoundary(
  props: FormBuilderInputErrorBoundaryProps,
 ): JSX.Element {
  const {children} = props
-  const [{error}, setError] = useState<{error: unknown}>({error: null})
-  const handleRetry = useCallback(() => setError({error: null}), [])
+  const [{error, info}, setError] = useState<{error: unknown; info: React.ErrorInfo}>({


stylistic comment: since you made info optional in the ErrorCard, would it be better for info here to be optional as well?

The typings from ErrorCard in @sanity/ui claims that it's always provided, so I follow that assumption here to ensure default values and reset values fulfill that contract.

It's optional in the component as a defensive measure, to ensure that should the component in @sanity/ui somehow return null or undefined then the ErrorCard makes sure to account for it. I don't think it's likely to happen, but given that this component is supposed to handle unhandled runtime exceptions it felt reasonable to follow Murphy's Law ☺️

ricokahler · 2024-06-07T08:09:26Z

packages/sanity/src/core/form/studio/FormBuilderInputErrorBoundary.tsx

+            </Stack>
+          </Box>
+        )}
+        {isDev && componentStack && (


really nice dev quality of life improvement here 💖

packages/sanity/src/core/form/inputs/PortableText/hooks/usePortableTextMembers.tsx

ricokahler · 2024-06-07T08:24:01Z

packages/sanity/src/core/form/inputs/PortableText/hooks/usePortableTextMembers.tsx

nice refactor!

stipsan · 2024-06-07T08:43:16Z

The PR also contains a lot of other minor fixes related to React Compiler, improvements to how the color scheme state is passed around, adding the React Component stack to the form input error boundary etc.

I'm curious, what was the actual culprit and what change fixed the issue?

Responded to this in depth on slack, the TL;DR is that writing to the ref in usePortableTextMemberItemsForProps during every render makes the return value of the hook, which is used in a long trail of hooks and components side effects as a dependency, unstable.
In React 18 it makes PTE slower than it needs to be.
In React 19 Strict Mode it can create infinite render loops and crash, but it's difficult to consistently reproduce as it's caused by race conditions and multiple other side effects happening at the same time.
In React 19 with React Compiler it's causing a very consistent crash since the way parent components are optimized cause certain deps to change on every render, causing side effects that end up scheduling another render, to fire and round and round we go.

stipsan changed the base branch from refactor-use-forwarded-ref to add-input-error-boundary June 7, 2024 00:18

stipsan force-pushed the fix-portable-text-editor-crash branch from 1865e6a to e001835 Compare June 7, 2024 00:19

vercel bot deployed to Preview – performance-studio June 7, 2024 00:20 View deployment

vercel bot deployed to Preview – page-building-studio June 7, 2024 00:20 View deployment

vercel bot deployed to Preview – test-studio June 7, 2024 00:20 View deployment

vercel bot deployed to Preview – test-next-studio June 7, 2024 00:21 View deployment

vercel bot deployed to Preview – test-compiled-studio June 7, 2024 00:21 View deployment

stipsan force-pushed the add-input-error-boundary branch from 88aaa8c to 9e81d00 Compare June 7, 2024 00:34

stipsan added the react-compiler label Jun 7, 2024

stipsan force-pushed the add-input-error-boundary branch from 9e81d00 to 1f64bfb Compare June 7, 2024 00:36

stipsan force-pushed the fix-portable-text-editor-crash branch from e001835 to e6ca596 Compare June 7, 2024 00:40

vercel bot deployed to Preview – page-building-studio June 7, 2024 00:43 View deployment

vercel bot deployed to Preview – performance-studio June 7, 2024 00:43 View deployment

vercel bot deployed to Preview – test-studio June 7, 2024 00:44 View deployment

vercel bot deployed to Preview – test-next-studio June 7, 2024 00:45 View deployment

vercel bot deployed to Preview – test-compiled-studio June 7, 2024 00:47 View deployment

Base automatically changed from add-input-error-boundary to next June 7, 2024 01:40

stipsan marked this pull request as ready for review June 7, 2024 06:45

stipsan requested review from a team as code owners June 7, 2024 06:45

stipsan requested review from a team, rexxars and jordanl17 and removed request for a team June 7, 2024 06:45

stipsan force-pushed the fix-portable-text-editor-crash branch from e6ca596 to 5c4869b Compare June 7, 2024 06:59

vercel bot deployed to Preview – page-building-studio June 7, 2024 06:59 View deployment

stipsan force-pushed the fix-portable-text-editor-crash branch from 5c4869b to 435e4fc Compare June 7, 2024 07:02

vercel bot deployed to Preview – page-building-studio June 7, 2024 07:03 View deployment

vercel bot deployed to Preview – performance-studio June 7, 2024 07:03 View deployment

vercel bot deployed to Preview – test-studio June 7, 2024 07:03 View deployment

vercel bot deployed to Preview – test-compiled-studio June 7, 2024 07:03 View deployment

vercel bot deployed to Preview – test-next-studio June 7, 2024 07:04 View deployment

stipsan enabled auto-merge June 7, 2024 07:14

stipsan commented Jun 7, 2024

View reviewed changes

stipsan force-pushed the fix-portable-text-editor-crash branch from 435e4fc to 62666c8 Compare June 7, 2024 07:22

vercel bot deployed to Preview – page-building-studio June 7, 2024 07:26 View deployment

vercel bot deployed to Preview – performance-studio June 7, 2024 07:26 View deployment

vercel bot deployed to Preview – test-studio June 7, 2024 07:27 View deployment

vercel bot deployed to Preview – test-next-studio June 7, 2024 07:27 View deployment

vercel bot deployed to Preview – test-compiled-studio June 7, 2024 07:28 View deployment

fix: portable text editor crash on React 19

f39f42e

stipsan force-pushed the fix-portable-text-editor-crash branch from 62666c8 to f39f42e Compare June 7, 2024 08:21

vercel bot deployed to Preview – page-building-studio June 7, 2024 08:24 View deployment

vercel bot deployed to Preview – performance-studio June 7, 2024 08:24 View deployment

vercel bot deployed to Preview – test-studio June 7, 2024 08:25 View deployment

ricokahler reviewed Jun 7, 2024

View reviewed changes

vercel bot deployed to Preview – test-compiled-studio June 7, 2024 08:26 View deployment

vercel bot deployed to Preview – test-next-studio June 7, 2024 08:26 View deployment

ricokahler approved these changes Jun 7, 2024

View reviewed changes

stipsan added this pull request to the merge queue Jun 7, 2024

Merged via the queue into next with commit 293e7d1 Jun 7, 2024
42 of 43 checks passed

stipsan deleted the fix-portable-text-editor-crash branch June 7, 2024 10:56

leocabrallce mentioned this pull request Jul 4, 2024

[Snyk] Upgrade sanity from 3.43.0 to 3.46.0 leocabrallce/portfolio-next#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: portable text editor crash #6870

fix: portable text editor crash #6870

stipsan commented Jun 7, 2024

vercel bot commented Jun 7, 2024 •

edited

Loading

github-actions bot commented Jun 7, 2024

github-actions bot commented Jun 7, 2024 •

edited

Loading

stipsan Jun 7, 2024

ricokahler Jun 7, 2024

stipsan Jun 7, 2024

christianhg commented Jun 7, 2024

ricokahler left a comment

ricokahler Jun 7, 2024

stipsan Jun 7, 2024

stipsan Jun 7, 2024

ricokahler Jun 7, 2024 •

edited

Loading

stipsan Jun 7, 2024

ricokahler Jun 7, 2024

ricokahler Jun 7, 2024

stipsan Jun 7, 2024

stipsan Jun 7, 2024

christianhg Jun 7, 2024

ricokahler Jun 7, 2024

stipsan Jun 7, 2024 •

edited

Loading

ricokahler Jun 7, 2024

stipsan Jun 7, 2024

ricokahler Jun 7, 2024

ricokahler Jun 7, 2024

stipsan commented Jun 7, 2024

fix: portable text editor crash #6870

fix: portable text editor crash #6870

Conversation

stipsan commented Jun 7, 2024

Description

What to review

Testing

Notes for release

vercel bot commented Jun 7, 2024 • edited Loading

github-actions bot commented Jun 7, 2024

github-actions bot commented Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianhg commented Jun 7, 2024

ricokahler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricokahler Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stipsan Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stipsan commented Jun 7, 2024

vercel bot commented Jun 7, 2024 •

edited

Loading

github-actions bot commented Jun 7, 2024 •

edited

Loading

ricokahler Jun 7, 2024 •

edited

Loading

stipsan Jun 7, 2024 •

edited

Loading