Undo/redo trap in the navigation block

[talldan]

Description

Observed by @getdave in #34533 (review).

Undo doesn't work as smoothly as it could often requiring mores steps than needed to undo.

It's also not possible to 'redo' adding link blocks.

I search for previous reports of this problem, #33911 may be related to this.

Step-by-step reproduction instructions

1. Add x3 links to the Navigation.
2. Click Undo to revert the adding of the links.
3. Click Redo to restore the links.
4. See that you can't do this. There is only 1 level of redos available.

Screenshots, screen recording, code snippet

No response

Environment info

Latest Gutenberg trunk.

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[talldan]

This effect seems to be what causes the extra undo level when adding navigation links:

gutenberg/packages/block-library/src/navigation-link/edit.js

Lines 357 to 358 in f0e66a1

	// Store the colors from context as attributes for rendering
	useEffect( () => setAttributes( { isTopLevelLink } ), [ isTopLevelLink ] );

When undoing the addition of a Page Link block, the following steps happen

1. URL and label attributes removed
2. isTopLevelLinkattribute is removed
3. Link block removed

I debugged this using redux dev tools, but a really easy way to see this is to do the following:

1. Add a navigation block in the post editor
2. Add a Page block and select a page from the URL control
3. Switch to code editor view
4. Click undo, see that the URL and label attributes are removed
5. Click undo, see that the isTopLevelLinkattribute is removed
6. Click undo, see that the Link block is removed.

[talldan]

Out of interest, I commented out the line that sets the isTopLevelLink attribute, and it seems to solve the issue with the redo trap. That code seems to have originated in this PR - #31149.

I don't really know what this code does. The comment above the line says 'Store the colors from context as attributes for rendering', but that doesn't seem to be what the code does. 🤔

[talldan]

I'm think I have an understanding of this now:

1. User clicks undo, then redo
2. This triggers the side effect of modifying the isTopLevelLink attribute.
3. The setAttribute call in step 2 modifies the undo stack so that a 'redo' is no longer possible.

This is in essence the same thing as if a user clicks the undo button a bunch of times and then makes some changes manually, you lose the ability to 'redo', but here the code is causing this.

I now remember fixing the same problem in the gallery block (#26377) using the mysterious __unstableMarkNextChangeAsNotPersistent, which seems to make the next action not end up on the undo stack. I tried that here and it solved the issue - #34565.

[gwwar]

Thanks @talldan for fixing this one!