[RNMobile] Add optional chaining to reusableBlock.title and reusableBlock.content
#54792
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit adds safety checks for the `title` property within the "getUnsyncedPatterns" function. By using optional chaining, the code now gracefully handles scenarios where `title` might be undefined, potentially preventing a TypeError from being thrown.
SiobhyB
added
Mobile App - i.e. Android or iOS
|
Size Change: +2.14 kB (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
SiobhyB
added
the
Mobile App - Automation
SiobhyB
changed the title
reusableBlock.title within getUnsyncedPatterns selectorreusableBlock.title and reusableBlock.content
|
Flaky tests detected in 3e6923b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6323998899
|
fluiddot
reviewed
Sep 26, 2023
| @@ -1986,7 +1986,7 @@ export const getInserterItems = createSelector( | |||
| isDisabled: false, | |||
| utility: 1, // Deprecated. | |||
| frecency, | |||
| content: reusableBlock.content.raw, | |||
| content: reusableBlock.content?.raw, | |||
There was a problem hiding this comment.
Per the symbolicated stack trace:
gutenberg/packages/block-editor/src/store/selectors.js:1988:buildReusableBlockInserterItem
gutenberg/packages/block-editor/src/store/selectors.js:1998:createSelector$argument_0
gutenberg/node_modules/rememo/rememo.cjs:270:callSelector
gutenberg/packages/data/src/redux-store/index.js:237:boundSelector
gutenberg/packages/block-editor/src/components/inserter/menu.native.js:70:useSelect$argument_0
Seems this is the line that produces the exception.
There was a problem hiding this comment.
@fluiddot, is that stack trace from a recent crash? I've been looking at a few symbolicated stack traces from recent crashes (from after 23.0, which included #53721) but they've all been a bit more vague than what you're seeing. For example:
This error is located at:
in C
in y
in R
in WithPreferredColorScheme(R)
in Unkn..., stack:
/workdir/gutenberg/packages/block-editor/src/store/actions.js:933:<anonymous>
/workdir/gutenberg/packages/block-editor/src/store/actions.js:860:registry.batch$argument_0
/workdir/gutenberg/node_modules/rememo/rememo.cjs:157:getCache
/workdir/gutenberg/packages/data/src/redux-store/index.js:233:bindSelector
null:null:null
/workdir/gutenberg/packages/data/src/components/use-select/index.js:151:updateValue
/workdir/gutenberg/packages/data/src/registry.js:293:registerStoreInstance$argument_1
/workdir/gutenberg/packages/data/src/registry.js:125:__unstableMarkListeningStores
/workdir/gutenberg/packages/data/src/components/use-select/index.js:39:Store
/workdir/gutenberg/packages/data/src/components/use-select/index.js:17:<global>
/workdir/gutenberg/packages/data/src/components/use-select/index.js:106:updateStores
/workdir/gutenberg/packages/data/src/components/use-select/index.js:196:useMappingSelect
/workdir/gutenberg/packages/block-editor/src/components/inner-blocks/use-nested-settings-update.js:64:useSelect$argument_0
There was a problem hiding this comment.
Yes, I used the stack trace from this Sentry event and picking a recent event for version 23.2.2 (GBM: 1.103.3):
anonymous@1:3088153
anonymous@1:3087780
o@1:1722534
i@1:1617316
anonymous@1:3129196
anonymous@1:1665759
__unstableMarkListeningStores@1:1612035
anonymous@1:1612238
w@1:1665550
anonymous@1:1665458
b@1:1666351
anonymous@1:1666493
C@1:3128556
Or@1:381186
Ma@1:421763
zi@1:408435
Ri@1:408337
Pi@1:408221
ki@1:405721
xt@1:370846
Ce@1:422136
Ne@1:363989
Me@1:364262
receiveTouches@1:416972
value@1:165707
anonymous@1:164207
value@1:165153
value@1:164165
⬇️
gutenberg/packages/block-editor/src/store/selectors.js:1988:buildReusableBlockInserterItem
gutenberg/packages/block-editor/src/store/selectors.js:1998:createSelector$argument_0
gutenberg/node_modules/rememo/rememo.cjs:270:callSelector
gutenberg/packages/data/src/redux-store/index.js:237:boundSelector
gutenberg/packages/block-editor/src/components/inserter/menu.native.js:70:useSelect$argument_0
gutenberg/packages/data/src/components/use-select/index.js:134:registry.__unstableMarkListeningStores$argument_0
gutenberg/packages/data/src/registry.js:123:__unstableMarkListeningStores
gutenberg/packages/data/src/registry.js:204:<anonymous>
gutenberg/packages/data/src/components/use-select/index.js:133:updateValue
gutenberg/packages/data/src/components/use-select/index.js:180:<anonymous>
gutenberg/packages/data/src/components/use-select/index.js:198:useMappingSelect
gutenberg/packages/data/src/components/use-select/index.js:287:useSelect
gutenberg/packages/block-editor/src/components/inserter/menu.native.js:53:InserterMenu
gutenberg/node_modules/react-native/Libraries/Renderer/implementations/ReactNativeRenderer-prod.js:3525:renderWithHooks
gutenberg/node_modules/react-native/Libraries/Renderer/implementations/ReactNativeRenderer-prod.js:7743:beginWork$1
gutenberg/node_modules/react-native/Libraries/Renderer/implementations/ReactNativeRenderer-prod.js:7304:performUnitOfWork
gutenberg/node_modules/react-native/Libraries/Renderer/implementations/ReactNativeRenderer-prod.js:7297:workLoopSync
gutenberg/node_modules/react-native/Libraries/Renderer/implementations/ReactNativeRenderer-prod.js:7279:renderRootSync
gutenberg/node_modules/react-native/Libraries/Renderer/implementations/ReactNativeRenderer-prod.js:6975:performSyncWorkOnRoot
gutenberg/node_modules/react-native/Libraries/Renderer/implementations/ReactNativeRenderer-prod.js:2145:flushSyncCallbacks
gutenberg/node_modules/react-native/Libraries/Renderer/implementations/ReactNativeRenderer-prod.js:8462:batchedUpdatesImpl
gutenberg/node_modules/react-native/Libraries/Renderer/implementations/ReactNativeRenderer-prod.js:1106:batchedUpdates
gutenberg/node_modules/react-native/Libraries/Renderer/implementations/ReactNativeRenderer-prod.js:1137:_receiveRootNodeIDEvent
gutenberg/node_modules/react-native/Libraries/Renderer/implementations/ReactNativeRenderer-prod.js:1209:ReactNativePrivateInterface.RCTEventEmitter.register$argument_0.receiveTouches
gutenberg/node_modules/react-native/Libraries/BatchedBridge/MessageQueue.js:427:__callFunction
gutenberg/node_modules/react-native/Libraries/BatchedBridge/MessageQueue.js:113:__guard$argument_0
gutenberg/node_modules/react-native/Libraries/BatchedBridge/MessageQueue.js:368:__guard
gutenberg/node_modules/react-native/Libraries/BatchedBridge/MessageQueue.js:112:callFunctionReturnFlushedQueue
There was a problem hiding this comment.
That's interesting, thanks for confirming! I wonder why there's such a variation.
There was a problem hiding this comment.
I wonder why there's such a variation.
It might be related to the Sentry event, I noticed that we have different ones pointing to similar errors:
TypeError: Cannot read property 'raw' of undefined(only affecting newer version, 23.0 and above)TypeError: undefined is not an object (evaluating 'n.title.raw')(only affecting past versions 22.8 and below)
@SiobhyB I'm wondering which Sentry event you used to symbolicate the stack trace.
There was a problem hiding this comment.
That's interesting, in that case, and as you pointed out, seems the Sentry event might be grouping exceptions caused by different locations 😬. I presume this fix will solve most of the exceptions we are getting, so let's see after releasing this fix if we still get the event in newer versions 🤞 .
fluiddot
reviewed
Sep 26, 2023
There was a problem hiding this comment.
This workaround most likely prevents the crash. However, without being able to reproduce the error, I'm a bit concerned that this change would cover potential issues we haven't managed to identify related to synced patterns. I tried to reproduce it using different approaches but so far I haven't experienced the crash. Not sure in which case a synced pattern could return its content as undefined 🤔. That said, in the spirit of reducing the crash rates, probably this is the best option we have.
If we follow this approach, synced patterns (i.e. reusable blocks) that have no content should probably filtered out from the inserter menu. WDYT?
As per the following feedback, we're going to experiment with no optional chaining for the `getUserPatterns` selector: #54792 (comment)
fluiddot
approved these changes
Sep 27, 2023
There was a problem hiding this comment.
If we follow this approach, synced patterns (i.e. reusable blocks) that have no content should probably filtered out from the inserter menu. WDYT?
In the spirit of merging this fix before the next GBM release cut, I'm approving the PR. We could tackle the topic I shared in a separate PR if needed.
|
Thank you, @fluiddot! I share your concerns about potentially covering other issues, but agree that this may be the best approach given the difficulty reproducing. I'll go ahead to merge to get this into the next release and follow up on this question separately:
|
SiobhyB
deleted the
rnmobile/try/optional-chaining-reusableblock-call
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Potential fix for wordpress-mobile/gutenberg-mobile#5496 and wordpress-mobile/WordPress-iOS#21083
reusableBlock.titleandreusableBlock.contentwordpress-mobile/gutenberg-mobile#6228What?
This PR adds safety checks for the
titleandcontentproperties within thebuildReusableBlockInserterItemandgetUserPatternsfunctions.Why?
We've received multiple reports of Gutenberg Mobile crashing with the following error:
Even after digging deeply into user reports and available logs, attempts to reproduce the crash have been unsuccessful. The main clues we have to go on are as follows:
block-editor/src/store/selectors.jsorblock-editor/src/store/actions.js.rawin those files is withinblock-editor/src/store/selectors.js.rawingetUserPatternscould be at fault.reusableBlock.title#53721. Though, it's worth noting we're not clear if the crash was resolved or simply "replaced" with this newer crash.With this PR, we're making another educated guess at the root cause of the crash in an attempt to fix it.
How?
Similar to #53721, I'm proposing optional chaining be added in the three remaining places where
rawappears without optional chaining in theblock-editor/src/store/selectors.jsfile. Based on the code's logic, we can assume thatreusableBlockwill always be defined, so the chaining is added alongsidetitleandcontent.By using optional chaining, the code should gracefully handle scenarios where
titleorcontentmight be undefined, potentially preventing a TypeError from being thrown.Testing Instructions
As the crash isn't easily reproducible, there isn't a straight forward way to ensure this PR fixes it. Instead, the logic of making these change should be verified.
In both the app and on the web, we should also ensure that there are no regressions related to both synced and unsynced patterns, especially those with no titles: