RTC: Fix inner template multiple insertion#79021
Conversation
…lock insert updates to avoid syncing inner block template insertion separately
|
Size Change: +504 B (+0.01%) Total Size: 7.61 MB 📦 View Changed
|
|
Flaky tests detected in 9d1b414. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/28530629278
|
…Sync reducer, as we can rely on insertion signals alone
| updateBlockAttributes( clientId, newAttributes ); | ||
| replaceInnerBlocks( clientId, newInnerBlocks ); | ||
| if ( shouldReselectBlock ) { | ||
| selectBlock( clientId ); |
There was a problem hiding this comment.
This code was added to useMigrateOnLoad() because style-variation.spec.js was failing after the changes in this PR.
The problem is that the test inserts list content via an old attributes value, which triggered the above useMigrateOnLoad() to migrate the block to inner blocks. After this PR the quote block was no longer automatically selected after the migration due to timing changes.
Previously in trunk:
- Block is inserted with legacy
attributes.value. - Parent block is currently selected.
- Template sync is scheduled for later.
useMigrateOnLoad()runs and converts the legacy attribute into real inner blocks.- When template sync runs asynchronously and checks inner blocks, they are no longer empty, so it does not run the default inner block insertion and does not change selection.
- Parent selection survives, even though we inserted inner blocks.
Previously, parent selection was preserved as a (probably) accidental result of the delayed template sync. The legacy migration populated the inner blocks before template sync ran, so template sync skipped the default inner-block insertion and never moved selection into the template child. Normally, you'd expect selection to move to the inner block, but here it was preserved and the test could click the "Plain" button on the parent quote block.
In the new logic, we run inner block template syncing first, which switches selection to the inserted child, before migrating the legacy value next. Migration uses replaceInnerBlocks() which doesn't keep selection, and the selection on the inner child is lost. The same applies for core/list's migration code, so I added the same fix in there.
I don't think this really matters in any real scenario. useMigrateOnLoad() is designed to run on initial editor load of a quote block with legacy attributes, where selection won't be set and doesn't matter. It really only applies to the style-variation.spec.js test where the parent block is assumed to still be selected after migration of a newly-created block with legacy attributes. We could also solve the issue by changing style-variation.spec.js to avoid inserting a legacy version, or actively manually select the quote block before trying to click the "Plain" button.
There was a problem hiding this comment.
I wish we could fix this differently. This used to work because the migration ran before the template sync. The migration converted the legacy attributes to inner blocks, and then the template sync found that there are already inner blocks, and that it doesn't need to insert the template.
The migration ran first because the template sync was deferred with queueMicrotask. Now when the microtask is gone the template sync run first. Because the template sync is in a child component (UncontrolledInnerBlocks) and the migration is in a parent component (quote's Edit). And effects run bottom-up, from children to parents.
Because the template sync run first, it modifies the selection: it selects the quote block's new paragraph child. That's why we need special code to fix the selection, i.e., move it back to the parent.
The template sync is unwanted. If we found a way to run the migration effect first again, we'd avoid the arbitrary fixes. But that means moving the useMigrateOnLoad to a child component -- not sure how to do that elegantly.
There was a problem hiding this comment.
I'm solving this as part of #80100. There the useMigrateOnLoad is moved to an InnerBlocks child, ensuring the right order: migration first, template sync second.
|
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. |
|
Hello @ellatrix, I'd really appreciate a review on this PR. We've been seeing a problem with inner templates getting inserted multiple times in a WebSockets RTC environment, and the core is a |
|
Isn't there a conceptual issue here at the "sync layer" where any side effects of block insertion ( |
Good question, and yes, I think you're right that this is a general class of problem. This is a broad issue with Gutenberg that needs to be addressed probably in several places for real-time collaboration compatibility. Anywhere we process a block (e.g. a Although this isn't true in all cases of deferred effects. I think we need two properties in order for this problem to appear:
So the scope is narrowed a bit to where an is operation is non-atomic and non-convergent. This PR fixes non-atomicness for template insertion on the originator side, but generally any non-atomic non-convergent operations are incompatible with collaboration. Some broader ways to try to solve this generally:
The PR here is narrowly targeted to templates, but we'll probably need to add originator fixes for other non-sync-compatible patterns. That said, I don't see a way to generally fix this issue without a major change to Gutenberg. Fortunately, many deferred operations work just fine due to convergence, so we largely need to focus on those that indiscriminately append data after a delay like inner templates. |
|
What I'm saying is that the template insertion is not a user action, the block object shouldn't change, it's just a reflection of a "default state" of a block. Which means, we shouldn't have to update the blocks state to render these template, now it's true that the block editor can only render block if they are in store, so maybe there should be a way to mark some blocks as "default" or something in the block-editor store which means, they get inserted into the store, but not as part of the |
This idea makes sense to me. This is also applicable in other areas like undo, where we have to be careful to make block tree changes that don't actually affect the stack like a user action. I'll try some things and get back to you soon. |
|
@youknowriad What do you think about the save flow right after inserting a block with a template? e.g. I insert a |
|
It would be good to get rid of inner block list "setting" syncing entirely. Which parts actually need to be dynamic and can't be moved to block.json or a simple non-render function? |
Indeed a good point, and that would probably break a lot of assumptions we've made over time. I guess my suggestion is not a good solution. I still don't understand the proposed solution though. I think if we consider "core/list" without template has a "half formed block", we shouldn't be able to insert it at all like that. So is there a solution where the "template" is resolved before the block is added to the store. I guess the main issue here is that today |
It's possible, but I don't think it will really help. In core blocks, the template passed to But what if any third party block declares the Nested settings are also almost static. But exceptions are possible. For example, the I think the root cause of the issue is somewhere else. The block editor does many "automated" edits as React side-effects. There are attribute edits, like these we discussed in #78989 ( We should somehow detect that both Does Yjs/RTC have some mechanism how to connect these two edits together? In some sense, they are not done by the two human collaborators, but by a third one, a machine agent. Is there a way to model this? The automated side-effect edits are ubiquitous and useful, I doubt that we'll be able to fully eliminate them, although it's probably possible in theory. We should rather teach the RTC engine to treat them correctly. |
The solution here was for me to deprecate the API and consider a plan for them, but before doing there, I wanted to understand how we're using this ourselves.
I agree that this is also a "root cause", but IMO both are issues. The reason the core/list insertion without template is an issue is specifically the fact that if that block is inserted and the effect doesn't run, the frontend is broken. (the save issue Alex mentions)
My guess is it's the clientId and two different clientIds means two different blocks. |
| // immediately after insertion through useInnerBlockTemplateSync. | ||
| return ( | ||
| getBlockType( block.name )?.allowedBlocks !== undefined || | ||
| getBlockSupport( block.name, 'allowedBlocks' ) !== undefined |
There was a problem hiding this comment.
This is a very unreliable condition. The allowedBlocks setting/attribute is not related to the template passed to useInnerBlocksProps. Many blocks have one but not the other.
| // settings before the template replacement to preserve that ordering. | ||
| // Example: If you remove flush here, ctrl + click to insert quote | ||
| // block won't close the inserter. | ||
| flushPendingNestedSettingsUpdates( registry ); |
There was a problem hiding this comment.
We could do this flushing more precisely. We don't want to flush all the pending updates for all blocks, but just for the clientId of the current block. Remove that one entry from pendingSettingsUpdates and leave the other entries alone.
I think this is a nice patch that's worth merging separately. Flushing is more elegant than deferring the entire template sync with queueMicrotask. Even if we never merged the rest of the PR, this part is worth doing.
I think there's two things here:
Even without RTC, these settings have caused performance an memo problems so it would be good to fix it "properly". |
| const shouldDeferBlockSync = | ||
| ( ( action.type === 'INSERT_BLOCKS' && | ||
| action.meta?.__unstableShouldDeferBlockSync ) || | ||
| action.type === 'REPLACE_BLOCKS' || |
There was a problem hiding this comment.
Is it expected that we are "deferring block sync" for all REPLACE_BLOCKS actions?
|
I've been poking Claude about the |
Another "framework gap" that would be worth looking at is the |
These functions don't have a |
There are also |
I think I'm starting to understand the When syncing This PR essentially makes this fully synchronous loop a little less synchronous. The actions that are marked as Now let me offer an idea that is both very radical and very simple: what if we deferred all actions in the sync loop? There's no targeted When the store subscription sees an incoming We would have to carefully handle updates with different persistent/non-persistent flags, with different undo/history regimes. And maybe also update the |
Yes for me the main issue of the proposal, is that it's not clear why we defer "inserter" actions and not others, there's no real logic here. So if you propose that we defer everything, I'd be more onboard with this. That said, I think a long time ago we had things like that, but maybe more specific to RichText and reverted it. We need to ensure that there are no undesired side effects but I wouldn't mind that change. My guess though is that there are situations where we navigate between entities, undo/redo, site editor navigation that might need to be considered carefully |
|
Appreciate all of the discussion here, thank you all! I agree with some of the concerns above where we may be skipping a defer in a situation where it makes sense ( The original intent behind some of the defer scoping was to reduce the number of changes we deferred, but it seems like that logic makes things more confusing, and will still probably miss cases.
This sounds like a better solution. I like that it would naturally coalesce microtasks and some effects so we don't need to spot-fix and carefully time when we switch between synchronous and asynchronous tasks in the editor. I'm happy to give this a shot! Good callout on when we have conflicting flags, I think we'd just want to flush incompatible sets separately. |
|
I have three PRs that together migrate the all template settings to static arrays on the block type object (no function or fn args). For post content and cover, we had some hacks that I removed. (Post content block should be using the appender when there's empty content, just like the post editor. Cover has a weird setup where a template should not be immediately inserted, it should only be added after submitting the placeholder form, so it's not really a template.) |
|
@ellatrix Awesome! Do you think we should still proceed with the defer strategy? I think this solution is great, we can statically determine if a block has a template and insert it synchronously. Of course this doesn't address third-party blocks that still use Your fix is much more ideal but not backwards compatible. What do you think? Are you trying to avoid the defer fix? |
|
I think third parties are not really an issue. There's 61 hits and most of them are static/constant. 1 is "dynamic" but not really, it's coming from an attribute that is set server side for some reason, and another 1 is a similar situation to the cover block. So we should 100% keep this API clean and static only. https://veloria.dev/search/89cf04eb-b18a-4312-8572-1a79a22091c2 A lot of them also have 0 installs. We could deprecate, disable RTC when the old approach is used (or something like that, open to ideas), and contact the ones with the highest install counts to update and be compatible (only 6 are 4k+). |
|
I'm surprised by the results in your search, I expected more usage! An occasional custom block with this property still might be present, but we'd have a difficult time detecting or disabling RTC until the block was already inserted due to the dynamic reasons you discussed above. But that should be rare and an easy diagnosis. I think if template insertion is actually a rarely-used feature as it appears, and we can completely cover core, then that's a better plan. Thank you for putting this together. |
|
If there is a custom block that does truly need them to be dynamic, it wouldn't have any way to work with RTC properly so that's where you're saying we might need to disable RTC or something? It might be a small enough use case that it isn't worth worrying about, but it would be nice if we were able to provide an alternate for those seemingly rare use cases. |
|
There's alternatives, you can still insert the blocks yourself like I'm doing with the cover block. Plenty of APIs around that. |
|
But like I said, there's not really any (public) custom cases out there, it's all practically static. Maybe there's private plugins that do it, but we have a pretty good sample size from core + public plugins. |
|
Even if all plugins use only static templates, it only means that they can easily migrate to the new API. But the duplicate insert issue will still be there. It will only happen much less often because it now affects just a few minor plugins, not an essential Core block like List or Quote. We still have many blocks that, for example, use I think the proper fix is still worth doing, although it will be much less urgent now. |
|
It's up to you all. I think |
This is a human-written PR description.
Follow-up from #78696, which addressed the same problem by having peers track which blocks were remotely synced. This worked but involved a large amount of bookkeeping and plumbing from the
syncpackage, throughcore-data, and deeply intoblock-editor. This PR uses a different strategy, deferring block updates on the originator to send both the outer block and inner block template at the same time.There's a fair amount of test code inflating LOC numbers, but a more manageable
+227 / -84changed outside of test code.What?
With RTC over a WebSocket connection, inserting a
core/listin the presence of other peers causes multiplecore/list-iteminsertions:list-item-duplication.mov
Running on
trunkwith WebSockets usingnpm run rtc:ws:slowThis scales with the number of connected peers. If a document has 5 connected users, 5 list items will be inserted.
In this PR, we address the issue by changing the sender to sync the outer
core/listblock and inner template insertion as a single logical operation:list-duplication-pr-fix.mov
Running on
fix/rtc-inner-template-sync2with WebSockets usingnpm run rtc:ws:slowWhy?
The primary cause is that
useInnerBlockTemplateSync(), which is in charge of inserting block templates, runs asynchronously after an outer block is inserted. In the reproduction above, all peers receive thecore/listouter block in one update, mount it, and then run the same inner template insertion logic independently.There's another snag that makes detecting this behavior difficult to predict. Blocks that use inner templates like
core/listonly apply inner block logic after mountingEdit. There is no definitive way to know that a block will insert template blocks later because template insertion happens after block mounting.These result in the duplication problem. User A adds a block. The block is added to the editor and post entity and synced with peers. After the block is mounted, the original user and each peer uses
useInnerBlockTemplateSync()to apply a template. An asynchronous amount of time later, every peer adds a separate template to the block.How?
In short, remove the async delay from
useInnerBlockTemplateSync(), defer outer block insertion operations until the next React event loop, and combine follow-up synchronous inner block insertions into a single logical operation. A bit more about each of those:Remove the async delay from
useInnerBlockTemplateSync(). As mentioned in a comment on trunk, the reasonuseInnerBlockTemplateSync()is queued into a microtask is auseNestedSettingsUpdate()timing dependency. Add aflushPendingNestedSettingsUpdates()instead to synchronously apply pending settings updates instead of relying on asynchronous ordering.Defer outer block insertion operations until the next React event loop. Blocks that we should defer have a
__unstableShouldDeferBlockSyncflag, which is activated as a heuristic for empty blocks that have a definedallowedBlockslist. The rest of the logic happens inuseBlockSync(). When we detect a "should defer" block, store that change indeferredBlockSyncRef, and schedule it to fire on the start of the next React event loop via auseEffect().Combine follow-up synchronous inner block insertions into a single logical operation. In step 2, we slightly deferred the outer block insertion. In the meantime, if we detect another change from the now-synchronous inner block insertion, overwrite the deferred change with new block content which contains the inner template. Finally, the
useEffect()fires on the next event loop andflushDeferredBlockSync()sends the block to core data and other users, complete with template. If there was no follow-up insertion, the block is still synced here too.There was also a small change to legacy quote and list migrations that I'll comment on in code below.
Testing Instructions
npm run rtc:ws:slow. Note we're using slow mode for a 30ms delay, which reflects real-world delays and makes race condition reproduction reliable.IMPORTANT: Use two separate browsers, not just two tabs, as
queueMicrotask()seems to avoid race conditions between two tabs in the same browser.core/listblock via/listor the sidebar.core/listand ensure only one inner template is added.In
trunk, steps 4 & 6 will result in two list items as per the reproduction at the top.Also try:
to run the same steps in an automated test.
Use of AI Tools
Code written with Codex and a lot of iteration.