Stop keeping stale controlled blocks after reset#76591
Merged
Conversation
jsnajdr
added
[Type] Bug
Closed
6 tasks
|
Size Change: 0 B Total Size: 7.66 MB ℹ️ View Unchanged
|
|
Flaky tests detected in fa003c1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/23339932064
|
Member
Author
|
@youknowriad A question related to this PR: what was the idea behind selection and block state restoration in #75371 and friends? When navigating from one editor page to another, you'll see a lot of The approach to fix this bug will depend on the answer. If we want to keep the |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
talldan
reviewed
Mar 18, 2026
Comment on lines
+2514
to
+2538
| // Reset blocks completely, simulates root useBlockSync cleanup. | ||
| state = blocks( state, { | ||
| type: 'RESET_BLOCKS', | ||
| blocks: [], | ||
| } ); | ||
|
|
||
| // Unset controlled inner blocks, simulates inner useBlockSync cleanup. | ||
| state = blocks( state, { | ||
| type: 'SET_HAS_CONTROLLED_INNER_BLOCKS', | ||
| clientId: 'chicken', | ||
| hasControlledInnerBlocks: false, | ||
| } ); | ||
|
|
||
| // Initialize content again, simulates useBlockSync after navigation. | ||
| state = blocks( state, { | ||
| type: 'RESET_BLOCKS', | ||
| blocks: [ chickenBlock ], | ||
| } ); | ||
|
|
||
| // Set controlled inner blocks again. | ||
| state = blocks( state, { | ||
| type: 'SET_HAS_CONTROLLED_INNER_BLOCKS', | ||
| clientId: 'chicken', | ||
| hasControlledInnerBlocks: true, | ||
| } ); |
Contributor
There was a problem hiding this comment.
It'd be great to clarify exactly what type of user interaction is being simulated by the test, and maybe which branch of useBlockSync this is.
Is it what's described in #76310, where chicken would be Post content, and as the user navigates from one page to another the controlled inner blocks of Post content are supposed to update to represent different content?
Member
Author
There was a problem hiding this comment.
I updated the test with better names. The block IDs are now template and content, and the block types are core/post-content and core/paragraph. Together with the comments, it should be now very clear what are we doing: simulating the two nested useBlockSync hooks, one syncing the root template entity, the other syncing the nested content entity.
At the end, after a few resets caused by a page navigation, the tree is currently in inconsistent state and the test fails. It's exactly the condition that useInnerBlockTemplateSync would encounter, and fail to apply the content template.
jsnajdr
changed the title
Member
Author
|
So, I fixed this by modifying the After this, all unit tests succeed, the @youknowriad said that the controlled blocks are supposed to survive a |
jsnajdr
added
the
Backport to WP 7.0 Beta/RC
tyxla
reviewed
Mar 18, 2026
| state?.controlledInnerBlocks ?? {}; | ||
|
|
||
| const newState = { | ||
| ...state, |
Member
There was a problem hiding this comment.
Should we still keep this just in case? It's totally fine to rebuild the state from scratch, but I just want to be sure we're not missing something
Member
Author
There was a problem hiding this comment.
To ensure we're not forgetting some piece of state, not even in the future, I switched to another approach. Let the inner reducer create the new state, by initializing an empty state with INSERT_BLOCKS:
reducer( undefined, { type: 'INSERT_BLOCKS', rootClientId: '', blocks } )That's exactly what the previous code was doing. Everything is much simpler now.
Member
There was a problem hiding this comment.
I really like how this simplified the logic. Great refactoring, @jsnajdr!
| if ( ! treeEntry ) { | ||
| return; | ||
| } | ||
| newState.tree.set( blockId, treeEntry ); |
Member
There was a problem hiding this comment.
Are references to these treeEntry old state objects fine? Should we create copies just in case?
Member
Author
There was a problem hiding this comment.
Yes, we're copying entries in the controlled subtrees that are guaranteed to be not touched. We can copy them. All the byClientId, attributes, parents and tree records.
Other reducers also do this. Typically they copy all old entries to a new map with newState.foo = new Map( oldState.foo ) and then modify the map.
| newState.controlledInnerBlocks | ||
| ) ) { | ||
| if ( ! preservedControlledInnerBlocks[ clientId ] ) { | ||
| if ( ! newState.controlledInnerBlocks[ clientId ] ) { |
Member
There was a problem hiding this comment.
Is this necessary? We have newState.controlledInnerBlocks[ clientId ] = true; above.
Member
Author
There was a problem hiding this comment.
It's not, the first loop (copying the non-tree records) already verified this condition. I removed the already verified conditions from the second (tree) loop.
Member
There was a problem hiding this comment.
Don't we clear to {} again after this change? Should we update the comment?
Member
Author
There was a problem hiding this comment.
Yes, I updated the comment 🙂
jsnajdr
force-pushed
the
fix/stale-controlled-blocks-after-reset
branch
from
9b882f8 to
d40677f
Compare
Member
Author
Thanks for the review @tyxla, I addressed all feedback. Now let's if there is a brave soul that approves this, including the 7.0 backport 🙂 |
Member
Author
|
Now, after rebase, there are plenty of e2e failures in |
jsnajdr
force-pushed
the
fix/stale-controlled-blocks-after-reset
branch
from
d40677f to
437168f
Compare
jsnajdr
force-pushed
the
fix/stale-controlled-blocks-after-reset
branch
from
437168f to
fa003c1
Compare
Mamaduka
approved these changes
Mar 20, 2026
Member
Mamaduka
left a comment
Mamaduka
left a comment
There was a problem hiding this comment.
I see that all tests are passing now. I did some general testing and core review, changes make sense and simplify things a lot.
I don't think there are any obvious blockers here.
Member
Author
Yes, there were same failures in |
github-actions
bot
removed
the
Backport to WP 7.0 Beta/RC
gutenbergplugin
pushed a commit
that referenced
this pull request
Mar 20, 2026
* Stop keeping stale controlled blocks after reset (test) * Improve the test code to make clear what is it simulating * withBlockReset: don't copy over stale tree entries * Address feedback
github-actions
bot
added
the
Backported to WP Core
|
I just cherry-picked this PR to the wp/7.0 branch to get it included in the next release: c7c7759 |
pento
pushed a commit
to WordPress/wordpress-develop
that referenced
this pull request
Mar 20, 2026
This updates the pinned hash from the `gutenberg` from `487a096a9782ba6110a7686d7b4b2d0c55ed1b06` to `2ee7ede6be6d4e55d5c7047394c5c4e0ea8d521d`. The following changes are included: - RTC: Backport race condition fix (WordPress/gutenberg#76649) - Fix navigation block rendering unit test (WordPress/gutenberg#76685) - Hide Additional CSS controls when block is inside contentOnly editing mode (WordPress/gutenberg#76512) - RTC: Increase polling intervals, increase polling on primary room only (WordPress/gutenberg#76704) - Navigation: Avoid List View changing position when navigation block saves (WordPress/gutenberg#76659) - Fix navigation block unit test and e2e test (WordPress/gutenberg#76692) - Fix locked content when switching to a different template without exiting 'Edit pattern' (WordPress/gutenberg#76710) - Metabox: Fix checkbox style in sidebar (WordPress/gutenberg#76718) - Stop keeping stale controlled blocks after reset (WordPress/gutenberg#76591) - Gate client-side media processing as plugin-only (WordPress/gutenberg#76700) A full list of changes can be found on GitHub: https://github.com/WordPress/gutenberg/compare/487a096a9782ba6110a7686d7b4b2d0c55ed1b06…2ee7ede6be6d4e55d5c7047394c5c4e0ea8d521d. Log created with: git log --reverse --format="- %s" 487a096a9782ba6110a7686d7b4b2d0c55ed1b06..2ee7ede6be6d4e55d5c7047394c5c4e0ea8d521d | sed 's|#\([0-9][0-9]*\)|https://github.com/WordPress/gutenberg/pull/\1|g; /github\.com\/WordPress\/gutenberg\/pull/!d' | pbcopy See #64595. git-svn-id: https://develop.svn.wordpress.org/trunk@62076 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith
pushed a commit
to markjaquith/WordPress
that referenced
this pull request
Mar 20, 2026
This updates the pinned hash from the `gutenberg` from `487a096a9782ba6110a7686d7b4b2d0c55ed1b06` to `2ee7ede6be6d4e55d5c7047394c5c4e0ea8d521d`. The following changes are included: - RTC: Backport race condition fix (WordPress/gutenberg#76649) - Fix navigation block rendering unit test (WordPress/gutenberg#76685) - Hide Additional CSS controls when block is inside contentOnly editing mode (WordPress/gutenberg#76512) - RTC: Increase polling intervals, increase polling on primary room only (WordPress/gutenberg#76704) - Navigation: Avoid List View changing position when navigation block saves (WordPress/gutenberg#76659) - Fix navigation block unit test and e2e test (WordPress/gutenberg#76692) - Fix locked content when switching to a different template without exiting 'Edit pattern' (WordPress/gutenberg#76710) - Metabox: Fix checkbox style in sidebar (WordPress/gutenberg#76718) - Stop keeping stale controlled blocks after reset (WordPress/gutenberg#76591) - Gate client-side media processing as plugin-only (WordPress/gutenberg#76700) A full list of changes can be found on GitHub: https://github.com/WordPress/gutenberg/compare/487a096a9782ba6110a7686d7b4b2d0c55ed1b06…2ee7ede6be6d4e55d5c7047394c5c4e0ea8d521d. Log created with: git log --reverse --format="- %s" 487a096a9782ba6110a7686d7b4b2d0c55ed1b06..2ee7ede6be6d4e55d5c7047394c5c4e0ea8d521d | sed 's|#\([0-9][0-9]*\)|https://github.com/WordPress/gutenberg/pull/\1|g; /github\.com\/WordPress\/gutenberg\/pull/!d' | pbcopy See #64595. Built from https://develop.svn.wordpress.org/trunk@62076 git-svn-id: http://core.svn.wordpress.org/trunk@61358 1a063a9b-81f0-0310-95a4-ce76da25c4cd
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
In this PR I managed to isolate a bug described in #76310 (comment) (the second one). Sometimes the
core/block-editorstate ends up with block tree whereorderandparentssay that there are no child blocks, buttreecontains child blocks.At this moment the PR has only a failing unit test that isolates the bug.
The key events and problems are:
resetBlocks( [] )action leaves staletreerecords behind, although they are no longer relevant. Namely there is thecontrolled||chickentree entry although thechickenblock is completely gone from the tree.setHasControlledInnerBlocks( 'chicken', false )afterwards, the action fails (inwithResetControlledBlocks) to reset the inner blocks to empty array. BecauseinnerBlockOrderis undefined. Then the stalecontrolled||chickenrecord survives.I'm not sure about the fix yet, but the most suspicious place is
withBlockResetwhere it handlesRESET_BLOCKSand copies the entire oldstate.tree. We shouldn't do that, we should cherry-pick only the records that are still relevant.fyi @youknowriad @talldan