RTC: Fix Block Hooks auto-inserted blocks duplication on collaboration#76895
RTC: Fix Block Hooks auto-inserted blocks duplication on collaboration#76895ingeniumed wants to merge 3 commits intotrunkfrom
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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. |
|
Size Change: +85 B (0%) Total Size: 7.73 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in a687c2b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/23723508339
|
chriszarate
left a comment
There was a problem hiding this comment.
Left a suggestion. We probably want to consult with @ockham who helped build Block Hooks. Perhaps there is an escape hatch we don't know about.
Regardless, I think this should target 7.1.
| const recordToSave = { | ||
| ...editedRecord, | ||
| meta: { ...meta }, | ||
| }; |
There was a problem hiding this comment.
Shallow-cloning in the resolver and mutating via function call feels like the wrong fix.
At the very least, we can move this logic to prePersistPostType in entities.js. There, the record is already cloned in order to amend it with additional edits. I can imagine something like a addPersistedCrdtDocToPostRecord function that is exported from crdt.ts and called from prePersistPostType.
However, what nags is that the root cause of this issue is something different: We are calling saveEntityRecord in the persistCrdtDoc record handler, which results in saving the entire record, not just fields that have changed.
User-initiated saves, on the other hand, are conducted via saveEditedEntityRecord which saves only the "dirty" fields. Therefore _wp_ignored_hooked_blocks would never be among them.
Most likely, the best fix is to persist CRDT documents outside of post meta (i.e., in the sync data table) where it does not need to interfere with the save data.
Short of that, moving this logic to entities.js is probably better.
Noting that I didn't add the backport label for this reason, and was going to leave it unmerged until it was safe to do so. This isn't for 7.0. I've also kept @ockham as a reviewer to get their take on this. |
|
Thanks for the ping. I think I see what's going on. Block Hooks operate almost 100% on the server side, so it'd be good to find a solution that works there. I'll try to think of something. I'll follow up with a short explanation of what's happening. |
|
Block Hooks are run upon e.g. For example, a paragraph block hooked <!-- wp:heading {"metadata":{"ignoredHookedBlocks":["core/paragraph"]}} -->
<h2 class="wp-block-heading">Heading</h2>
<!-- /wp:heading -->This tells Block Hooks to "ignore" that instance of the Heading block, i.e. to skip insertion of the hooked paragraph block. This doesn't work at the "edges" of some entities. For example, a paragraph block can be hooked as There's one more difference to the "regular" case (where Note that there doesn't seem to be a problem with the "regular" case (which is probably because RTC syncs the block attributes between editors). Maybe what's missing is a way to sync the relevant post meta 🤔 I'll think a bit more about it. |
| // Post meta keys that are server-managed and must not be sent by the client | ||
| // during saves. Sending stale client values for these keys overwrites the | ||
| // server's calculations (e.g. update_ignored_hooked_blocks_postmeta). | ||
| const serverManagedPostMetaKeys = new Set< string >( [ | ||
| '_wp_ignored_hooked_blocks', | ||
| ] ); | ||
|
|
||
| /** | ||
| * Remove server-managed meta keys from a record's meta object in place. | ||
| * These keys are calculated by the server during save and must not be | ||
| * overwritten by possibly stale client values. | ||
| * | ||
| * @param {Object} record An entity record with an optional `meta` property. | ||
| */ | ||
| export function stripServerManagedMeta( | ||
| record: Record< string, unknown > | ||
| ): void { | ||
| const meta = record.meta; | ||
| if ( ! meta || typeof meta !== 'object' ) { | ||
| return; | ||
| } | ||
| serverManagedPostMetaKeys.forEach( ( metaKey ) => { | ||
| delete ( meta as Record< string, unknown > )[ metaKey ]; | ||
| } ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Without knowing too much about RTC, this seems backwards -- the _wp_ignored_hooked_blocks post meta is added precisely to prevent duplication of hooked blocks (see).
|
So looking at the video (and reproducing locally), we're only ever saving that post once. At the time, the Block Hooks plugin isn't enabled yet, so the Once the plugin is enabled and we reload the editor, Block Hooks see that Then:
|
|
Here's a tentative fix: WordPress/wordpress-develop#11410. This makes sure that the |
|
@ockham - thanks for explaining the issue and for putting a fix in core for this. That does fix the problem! Given that it fixes the problem, I'll go ahead and close this PR and mark your PR as the fix for the issue. |
What?
Closes #76176
Fixes Block Hooks API auto-inserted blocks being duplicated on page refresh or when a collaborator joins an RTC session.
Why?
When the CRDT persistence save fires, it sends the full edited record including
_wp_ignored_hooked_blockspost meta back to the server. The server recalculates this meta on every save viaupdate_ignored_hooked_blocks_postmeta, but the stale client value overwrites the server's calculation, causing hooked blocks to be re-inserted on each subsequent load.How?
Strip server-managed meta before CRDT saves: Added a
stripServerManagedMetautility that removes_wp_ignored_hooked_blocksfrom the record's meta beforesaveEntityRecordis called during CRDT persistence. The record is shallow-cloned first to avoid mutating the cached selector result.Remove the collaboration workaround in block hooks e2e tests: The tests previously disabled collaboration entirely for block hooks tests with a comment noting "a fundamental incompatibility with RTC." Now that the root cause is fixed, that isn't required anymore.
Testing Instructions
core/headingblock and acore/paragraphblock.packages/e2e-test/plugins).Testing Instructions for Keyboard
N/A - no UI changes.
Use of AI Tools
Claude Code (Claude Opus 4.6) was used to assist with cherry-picking commits, reviewing code, and creating this PR.