fix: Guard against a block styles crash due to null block values #56903
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
dcalhoun
added
[Type] Bug
|
Size Change: 0 B Total Size: 1.71 MB ℹ️ View Unchanged
|
|
Flaky tests detected in d57cf63. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7143982446
|
twstokes
reviewed
Dec 8, 2023
| function BlockStyles( { clientId, url } ) { | ||
| const selector = ( select ) => { | ||
| const { getBlock } = select( blockEditorStore ); | ||
| const { getBlockStyles } = select( blocksStore ); | ||
| const block = getBlock( clientId ); | ||
| return { | ||
| styles: getBlockStyles( block.name ), | ||
| className: block.attributes.className || '', | ||
| styles: getBlockStyles( block?.name ) || EMPTY_ARRAY, |
There was a problem hiding this comment.
Curious - what's the purpose of this idiom in RN over just a [] literal?
There was a problem hiding this comment.
JavaScript does not consider two seemingly equal array literals to be equivalent. I.e., [] === [] returns false.
This becomes important in this particular context as the code in question is a "selector" for querying state in the store. Selectors are a React ecosystem concept that I believe originates from Redux. Selectors provide state to the UI and related logic.
When a selector's return value changes, the UI re-renders for the new value. So, it is a best practice or optimization to avoid returning unnecessary "new" values, which would trigger unnecessary updates to the UI and incur a performance cost.
The Gutenberg project has a development tool that helps avoid introducing unnecessary "new" values from selectors, which helped me ensure I addressed it appropriately. We have a few existing instances to address in the mobile editor.
There was a problem hiding this comment.
Thanks! So the object stays constant, and no change is detected.
In certain scenarios, e.g., when the editor hangs due to poor performance, the `getBlock` value unexpectedly returns `null`. To guard against a crash in this scenario, we now conditionally access attributes on the block. A ideal resolution would be improving editor performance to avoid the scenario where a race condition causes a crash, but the fact remains that `getBlock` _can_ return `null` values and that we should not presume that it will not.
dcalhoun
force-pushed
the
fix/guard-against-block-styles-crash
branch
from
b6da7b2 to
d57cf63
Compare
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.
What?
Guard against
nullvalues returned fromgetBlock.Why?
Fixes wordpress-mobile/gutenberg-mobile#6128. Mitigate
disruptive crashes caused by accessing attributes on a
nullvalue.How?
In certain scenarios, e.g., when the editor hangs due to poor
performance, the
getBlockvalue unexpectedly returnsnull. To guardagainst a crash in this scenario, we now conditionally access attributes
on the block.
A ideal resolution would be improving editor performance to avoid the
scenario where a race condition causes a crash, but the fact remains
that
getBlockcan returnnullvalues and that we should notpresume that it will not.
Additional details are shared in wordpress-mobile/gutenberg-mobile#6128 (comment).
Testing Instructions
The real world scenario where this occurs is difficult to replicate
consistently, as it appears to depend upon a race condition prevalent during
hangs caused by poor app performance. Therefore, a contrived testing approach
is applying the following diff with and without the proposed changes to ensure that
the component correctly handles
nullblock values.Test Diff
Testing Instructions for Keyboard
n/a
Screenshots or screencast
It is worth noting that guarding against the crash results in an unexpected block controls state where the block styles are missing. Hypothetically, the block styles would become visible once the block value is no longer
null. This is unavoidable until the editor performance is addressed and the race condition is avoided.