Adding duplicate Template Part removes both from page, can trigger clearing inner blocks in dirty state.

[Addison-Stavlo]

Describe the bug
This seems to be a recent regression. Adding the same template part to a page twice will cause both to be removed from the page. Navigating through the page content (up and down arrows) may then also trigger the same bug noted in #22638.

To reproduce
Steps to reproduce the behavior:

1. Go to the editor (page editor shown below) with full site editing experiment enabled.
2. Add a template part.
3. Add the same template part again.
4. See that both template parts are removed from the page. Scroll (up and down arrows) through page content to see some 'funky' behavior and additionally trigger dirty state of the template part to have cleared all of its content (second part likely related to Removing a Template Part dirties the entity state with inner content removed. #22638) .

Expected behavior
Expected both template parts to remain (previous behavior). Or if we are iterating to prevent that, at least 1 should remain and a notification should occur.

Screenshots

Since this is a recent regression, perhaps @epiqueras, @youknowriad, @noahtallen, @vindl, @ockham, or others may have an idea of where this could originate from?

[noahtallen]

This is probably also related to the blockSync. I think it has to do with how when you insert a block for the first time, it would not have any inner blocks (probably causing the blockSync hook to update the template part to also not have any blocks). This is because the template part sets its blocks to the block editor asynchronously after it gets a chance to fetch them, so they might not be there the first time the block renders. Could possibly be resolved by adding a loading state until the inner blocks are available?

Definitely a tricky edge case.

[epiqueras]

The issue is that withReplaceInnerBlocks removes blocks from everywhere in the block tree and only adds them back to the block the action was called for.

This is easy to fix, but then we get an infinite update loop because one of the template parts replacing its inner blocks busts the cache for the other template part and vice versa.

[noahtallen]

one of the template parts replacing its inner blocks busts the cache for the other template part and vice versa

would the equality check possibly prevent this from happening? I don't think it will ever dispatch a change in or out if the block list is equal by reference. I guess the reference would probably change if the cache is busted 🤔

[epiqueras]

Yeah, getBlocks() changes when the cache is busted.

We need to restore the same caches in withReplaceInnerBlocks for replaced blocks that have not changed.

[chrisvanpatten]

Looks like this is also the source of one of the issues we're seeing on #23601, trying to get reusable blocks working with controlled InnerBlocks.

[epiqueras]

What if we make parent blocks with the same children share the same client ID? It makes sense since they are essentially the same block, and their children already share client IDs. This would solve the issue and allow us to do things like render selection indicators in both blocks.

[annezazu]

@epiqueras this came up today in the core-editor chat. Where does this issue stand in terms of being addressed? I know you've been working hard on #23661 (which I need to test :D ).

[ZebulanStanphill]

For the record, I just checked: this issue is still present in master. (Changes from #24012 have not affected this bug, in case anyone was wondering.)

[epiqueras]

We need more input on whether #22639 (comment) is an acceptable solution.

@youknowriad @noahtallen

[noahtallen]

Ah sorry for never responding to that :)

What if we make parent blocks with the same children share the same client ID? It makes sense since they are essentially the same block, and their children already share client IDs. This would solve the issue and allow us to do things like render selection indicators in both blocks.

I think this is an interesting solution. On the one hand, I really like the idea of each of those blocks actually being the same thing. On the other hand, I think that might be a difficult change to make. For example, a lot of stuff in the block-editor store relies on the block ID, and particularly the uniqueness of the block ID.

My concern is that if we have a post with this block in two places, if we then remove one of those blocks by dispatching a clientId, wouldn't we remove both? I think there might be a lot of other side effects in a similar vein.

I feel like there should be a way to solve it without having to make that much of a change to how the block-editor store relies on the uniqueness of IDs. 🤔

[epiqueras]

I think the selectors would just target the first block with that ID, which could be fine.

[noahtallen]

That might set us up for different bugs like "removing the second of the template part blocks which are the same deletes the first one but not the second one" and maybe others 🤔

Admittedly, it's less destructive

[epiqueras]

Agreed

I guess #22639 (comment) is the way then?

[Addison-Stavlo]

Im not quite sure to be honest. If they share the same client ID then will selecting one select both? Will selecting the second select the first instead? Would removing the second remove the first instead?

It is better than them both disappearing but if it causing issues like this it doesn't seem like a permanent solution. 🤔 Does it make sense to be able to have 2 of the exact same template parts in the same context, other than a mistake made by the user in editing? Or as a temporary state where they want to put in a second copy of one to then 'create a new variation' of?

Would it make sense to restrict template parts from having more than 1 instance in any given context? Should the second just render a warning similar to "this template part is already in use here" and give some sort of prompt to turn that second instance into a new cpt as a variation of the first?

[ZebulanStanphill]

The controlled InnerBlocks API will also be used by reusable blocks in the future, and I'd definitely like to still be able to insert multiple of the same reusable block in posts.

Why don't we just add/use something like clientId that is the same for blocks referencing the same content?

I'm not certain how this all works technically, but it seems like the issue is that duplicate template parts both try to update each other at the same time, resulting in the content of both becoming invalid and getting erased? Why not just lock all the other instances of a template part while the user is editing one?

[epiqueras]

@noahtallen Making unselected template parts show a preview would work too. But I think it's nice to see selections mirrored. It shows you are editing multiple instances.

[noahtallen]

🤔 Interesting. I might investigate a solution for this today.

Why don't we just add/use something like clientId that is the same for blocks referencing the same content?

This is an interesting idea, but my intuition is that it would be a big change and hard to maintain

Why not just lock all the other instances of a template part while the user is editing one?

That unfortunately won't fix the root of the issue, which is:

withReplaceInnerBlocks removes blocks from everywhere in the block tree and only adds them back to the block the action was called for.

It's problematic because all of the child blocks for each of the duplicated template parts share the same clientId, even though the template part block containing them does not. the children share the same clientId because the template parts share the same entity record data source. Theoretically, you can still update each template part block separately -- e.g:

- template part block with attributes { id: 1 }
- template part block with attributes { id: 1 }

As you can see, you could still update the attributes separately such that they point at different template parts. This is why it is helpful for them to continue having separate clientIds, because they are actually separate instances, even though they happen to share the same data source.

At the root, it's an issue with how the store is maintained internally, so I don't think it can be fixed at the block level

Making unselected template parts show a preview would work too

It sounds promising, so I'll try it out and see what happens :)

But I think it's nice to see selections mirrored. It shows you are editing multiple instances

I definitely agree here. Is the issue that block preview uses a separate data store?

[epiqueras]

Block previews don't show selections, but perhaps the double selections would be confusing anyway.

[noahtallen]

diff --git a/packages/block-editor/src/components/inner-blocks/index.js b/packages/block-editor/src/components/inner-blocks/index.js
index 668c6c5e89..5a767fbadd 100644
--- a/packages/block-editor/src/components/inner-blocks/index.js
+++ b/packages/block-editor/src/components/inner-blocks/index.js
@@ -27,6 +27,7 @@ import BlockList from '../block-list';
 import { BlockContextProvider } from '../block-context';
 import { useBlockEditContext } from '../block-edit/context';
 import useBlockSync from '../provider/use-block-sync';
+import BlockPreview from '../block-preview';
 
 /**
  * InnerBlocks is a component which allows a single block to have multiple blocks
@@ -126,8 +127,22 @@ function UncontrolledInnerBlocks( props ) {
  * @param {Object} props The component props.
  */
 function ControlledInnerBlocks( props ) {
+       const { clientId, value } = props;
+       const isSelected = useSelect( ( select ) => {
+               const { isBlockSelected, hasSelectedInnerBlock } = select(
+                       'core/block-editor'
+               );
+               return (
+                       isBlockSelected( clientId ) ||
+                       hasSelectedInnerBlock( clientId, true )
+               );
+       } );
        useBlockSync( props );
-       return <UncontrolledInnerBlocks { ...props } />;
+       return isSelected ? (
+               <UncontrolledInnerBlocks { ...props } />
+       ) : (
+               <BlockPreview blocks={ value } />
+       );
 }

Using this diff to test it out ^^

There are some issues:

1. It doesn't fix the issue. :/ I'm not sure exactly why, but even when showing BlockPreview, inserting a duplicate clears the editor.
2. I'm worried that hasSelectedInnerBlock is going to return true even if the block is selected under the other duplicated template part.
3. The styles in the editor are different using BlockPreview vs UncontrolledInnerBlocks. (e.g. background color is different)
4. Performance seems a bit rough

[epiqueras]

Then I think we can't get around modifying the cache so both template parts share it.

[noahtallen]

Will investigate that next 👀

I wonder if we can use the fact that they have the same data source (e.g. value or onChange are equal by reference)

[epiqueras]

#22639 (comment) also needs to be handled.

[noahtallen]

#22639 (comment) also needs to be handled.

I'm not sure how easy this will be to handle. a lot of the client Ids are denormalized into different parts of the state, so stuff like byClientId or attributes is just going to remove each key passed (i.e. all the blocks in the template part). did you have an idea about how to get around that?

[noahtallen]

Why don't we just add/use something like clientId that is the same for blocks referencing the same content?

This is actually sounding more interesting to me now. I.e. a "fake" clientId which is the parent of any blocks inside the innerBlockController. That "fake" client ID is the same if the data source is the same. Then all block operations under that client ID get updated correctly.

For example, we currently remove all blocks associated with the inner block, and then add them back only to the inserted blocks. (because there is no shared client ID)

[noahtallen]

Other things are weird in this scenario too..... like the parents state is childId: parentId. But that assumes those children IDs are unique, which isn't the case for duplicated controlled inner blocks

[noahtallen]

Even if we just show a placeholder, it would still work on the front end. So it wouldn't be... totally broken 😁

I'm just not sure how to solve the problem. Any way I think about it, I wind up back at "the block editor store can't handle duplicate clientIds" (whether that is children or parents). And that seems like a really tough problem to solve considering how foundational the uniqueness of clientIds is to the inner workings of the store

[noahtallen]

Hm. I wonder if the one duplicated block could just pretend to be the existing one. So that it renders the same children, but in the store it wouldn't duplicate stuff.

[noahtallen]

You know, I think the preview didn't work because it was still using blockSync.

[noahtallen]

Yeah. it's fixed when we avoid blockSync as well. Still really weird around UX & block selection. I wonder if it makes sense to:

• show a block preview all the time for any duplicates.
• to make an edit to the duplicate, you are directed to the first one on the page.

[noahtallen]

By "fixed" I mean that it does't delete off the page immediately, but there still appear to be some race conditions

[epiqueras]

The issue is that withReplaceInnerBlocks removes blocks from everywhere in the block tree and only adds them back to the block the action was called for.

We need to add the blocks back in everywhere they were removed.

[ZebulanStanphill]

to make an edit to the duplicate, you are directed to the first one on the page.

That sounds like it would make for a jarring user experience. Ideally, the editable instance should be whichever one you have selected.

[noahtallen]

Ideally, the editable instance should be whichever one you have selected.

I agree ideally. The problem that would theoretically solve is that only one of the template parts is actually stored in the block-editor store (since the previews are separate). Therefore, there are no issues with duplicate clientIds in the store.

In my testing locally, I think there are some problems that would have to be solved for previews:

1. It's not really possible to tell which template part block is the parent of one of inner blocks. (since the parent/child state has a hard requirement on the clientIds being unique.
2. I think there are still race conditions around selecting into a block. Obviously for it to work, when you select into one of the duplicates, the other duplicate's blocks would have to be removed from block-editor (and it shows the preview). They have to be removed because of the requirements around unique clientIds

since the children inner blocks come from the same parsed entity data source, the client IDs are always the same when pulled from that data source

[epiqueras]

I think we should just do #22639 (comment) and see where that gets us. That will solve this ticket. We can refine the UX with things like the preview later.

[noahtallen]

I'll look into that more.

[noahtallen]

current status lmao 😁

[noahtallen]

It is actually working fairly well, other than selection state not updating properly. see #24180.

[noahtallen]

The challenge with cache at the moment is that getBlocks cache is actually just the aggregate of the children caches. So there isn't really a "cache to restore" at that point in time, unless the cache value for each of the children needs to be restored. But if we did that, then getBocks for the original controller would also not update

[noahtallen]

Some ideas I have:

1. Provide a way to mark which block controller should be in charge of updating the blocks, and then only call the update functions for that specific one.
2. Use incoming/outgoing changes at a module-level. I.e. if the onChange/value references are the same, then the incoming/outgoing changes are also the same.

[epiqueras]

2 sounds safer.

[noahtallen]

For an in-depth explanation of why this bug happens, see this comment: #24180 (comment)