BUGFIX: Reflect discard of node move changes correctly in the UI

[grebaldi]

fixes: #3184

NOTE: This PR needs to be tested in combination with its companion PR over at neos-development-collection: neos/neos-development-collection#4291

Buckle up for collisions with #3569

The overall problem

This one's a bit complex and it stretches over two repositories. The basic scenario is:

1. You move some (document) node(s) to a position below a different parent node (so, just sorting them within their own hierarchical level doesn't count)
2. You discard those changes
3. Chaos ensues

#3184 describes the issues that happen only partially. During my investigation I found several problems that I need to disect piece by piece.

(I) The UI does not (entirely) recognize that nodes have been moved to a position below a different parent node

The problem

Peek.2023-05-21.15-14.-.3184.what.happens.on.discard.mp4

In the video you can see that after the two document nodes have been moved, the change is not made visible by the usual orange change indicator on the left hand side of the tree nodes. Also, on the attempt to discard the current set of changes, an error occurs that reads:

Call to a member function isRemoved() on null - Check logs for details

Both phenomena are related, because - as it turns out - the UpdateWorkspaceInfo feedback object, that is supposed to inform the UI about pending changes, delivers the wrong node context paths for the nodes that have just been moved (the context paths are still the old ones).

Because the context paths are now out of sync, the UI is unable to associate the pending change with the respective tree node. It then also uses the stale workspace information as payload to the discard command, which leads to the above error at the following line:

neos-ui/Classes/Controller/BackendServiceController.php

Line 272 in 5c52e08

if ($node->isRemoved() === true) {

How come the context paths are incorrect after the nodes have been moved?

After some investigation I found that the Neos\ContentRepository\Domain\Factory\NodeFactory class memoizes stale data - as opposed to e.g. ContentContext which gets its in-memory cache flushed when nodes were moved:

https://github.com/neos/neos-development-collection/blob/0e28ef208111f5b1576df9078bb9abfedf12500c/Neos.ContentRepository/Classes/Domain/Factory/NodeFactory.php#L81

When the PublishingService is asked for unpublished nodes via getUnpublishedNodes, it receives cache hits within NodeFactory->createFromNodeData for the nodes that have just been moved - thus the old context paths.

EDIT: Not at all true 😅! I investigated further after the functional tests over at neos-development-collection failed. The actual reason is as follows:

The Neos UI API uses a slightly different content context configuration than getUnpublishedNodes. So, NodeFactory actually keeps track of two variants of the moved nodes. The ones that getUnpublishedNodes receives are not the ones that the move operation has been performed on.

The solution

I modified the way the content context for node change operations is build. It is now ensured that the content context sees everything by setting the invisibleContentShown, removedContentShown and inaccessibleContentShown flags all to true.

Related Commit(s): f300e93

(II) Discarded move changes are not properly reflected in the document tree

The problem

Problem (I) can be circumvented by hard-reloading the UI (after that, the workspace info will be correct again). But, there's still some strangeness going on...

Peek.2023-05-21.15-15.-.3184.what.happens.on.discard.after.reload.mp4

In the video you can see that the tree actually reflects the discarded changes correctly for a brief moment there. It then quickly jumps to a broken state in which the nodes that should be at their original positions just disappear.

This problem persists even if you hard-reload the UI after discarding:

Peek.2023-05-23.17-37.-.3184.what.happens.on.hard.reload.mp4

Now, if you use the reload button of the document tree to manually reload the tree, the nodes reappear:

Peek.2023-05-23.17-38.-.3184.what.happens.on.tree.reload.mp4

(But also: If you hard-reload the UI again, the nodes will once again flash briefly and then disappear)

How does this happen?

In those videos, the parent document that originally contained the moved nodes is focused and open in the guest frame. After discard, it should contain those nodes again. The UI reloads the guest frame, but the document is now rendered with stale node metadata. After the guest frame finished loading, the stale metadata (which still thinks the nodes have been moved elsewhere) overwrites the node data in the UI redux store.

This is why the correct state shows up for a brief moment. It gets overwritten after a short delay when the guest frame is loaded. (Also: The nodes do not disappear if you focus a different document and hard-reload the UI).

Looking at the cache configuration for the node metadata:

neos-ui/Resources/Private/Fusion/Prototypes/Page.fusion

Lines 26 to 46 in 5c52e08

	// We need to ensure the JS backend information is always up to date, especially
	// when child nodes change. Otherwise errors like the following might happen:
	// - create a new child (document) node
	// - again visit the parent node
	// - the parent node still has the stale "children" infos (without the newly created child)
	// - thus, the document tree will have the newly created node REMOVED again (visually).
	//
	// as a fix, we ensure that the JS backend information creates an own cache entry, which is flushed
	// whenever children are modified.
	@cache {
	mode = 'cached'
	entryIdentifier {
	jsBackendInfo = 'javascriptBackendInformation'
	documentNode = ${documentNode}
	inBackend = ${documentNode.context.inBackend}
	}
	entryTags {
	1 = ${Neos.Caching.nodeTag(documentNode)}
	2 = ${documentNode.context.inBackend ? Neos.Caching.descendantOfTag(documentNode) : null}
	}
	}

... one should actually assume that the data shouldn't be stale (Neos.Caching.descendantOfTag(documentNode) should do the trick). It turns out though, that the Neos\Neos\Fusion\Cache\ContentCacheFlusher class - in case of discard - will only flush tags related to a node's current workspace. We actually need to have all tags flushed in the base workspace as well to cover the DescendantOf_*-tag of the original parent node.

The solution

The solution for this problem is implemented over here: neos/neos-development-collection#4291

(III) Discarding a node move while having a moved document node open in the guest frame results in an error page

The problem

Peek.2023-05-22.14-30.-.3184.Discard.Page.Move.While.On.Page.mp4

The video shows that when you're currently editing a moved document and then discard the move change, the guest frame reloads and shows a misleading fusion error. This is because the guest frame tries to render a document node that doesn't exist anymore.

A similar situation would be a document that has just been created. If you stay on that document and then discard it, the UI behaves correctly and redirects you to the next-higher document:

Peek.2023-05-23.17-46.-.3184.discard.new.node.mp4

Here, the problem lies within the discardAction of the Neos\Neos\Ui\Controller\BackendServiceController which does not recognize discarded move changes and thus misses to inform the UI that it needs to remove the nodes at their former positions and re-insert them at their original positions.

The solution

I modified the body of the discardAction method of the BackendServiceController. It now explicitly detects node move changes and adds the corresponding RemoveNode and UpdateNodeInfo feedbacks.

Related Commit(s): c4bad62

All solutions combined

Here's what it looks like when the PRs in neos-ui and neos-development-collection are combined:

Peek.2023-05-23.17-48.-.3184.all.changes.combined.mp4

[crydotsnake]

Hello @grebaldi !

In general, your changes does fix the issue you described here.

But afterwards some of the created nodes disapears everytime i click on them. I'm not really sure, if this has something to do with your changes. But if i switch back too the 7.3 branch, the behavior is gone:

Maybe it is a caching issue or something?

Havent we discussed about this issue a couple of months ago? @Sebobo 🤔

[grebaldi]

@crydotsnake Have you tested this in combination with neos/neos-development-collection#4291?

[crydotsnake]

@crydotsnake Have you tested this in combination with neos/neos-development-collection#4291?

No, i didnt. I have totally overread that 🤦🏽 will try it again..

[grebaldi]

No, i didnt. I have totally overread that 🤦🏽 will try it again..

Yeah, I probably should have pointed that one out more clearly 😅 Sorry for the trouble! I'll put up a note at the top of the PR description.

[crydotsnake]

No, i didnt. I have totally overread that 🤦🏽 will try it again..

Yeah, I probably should have pointed that one out more clearly 😅 Sorry for the trouble! I'll put up a note at the top of the PR description.

No! you described everything as good as possible! 💙 was definitely not your fault.

[crydotsnake]

Tested it again, and in combination with neos/neos-development-collection#4291 everything works as expected!

[markusguenther]

Impressive pull request! Appreciate the thorough explanations and the inclusion of screencasts. I've validated that it functions as intended. Unfortunately, the Neos pull request is currently facing some blockers. Would it be beneficial to enhance the existing drag-and-drop (DND) end-to-end test with these scenarios? Although it would only pass once the Neos pull request is merged, it would still allow us to cover these cases in advance.

[grebaldi]

Hi @markusguenther,

Would it be beneficial to enhance the existing drag-and-drop (DND) end-to-end test with these scenarios?

Oh yes, definitely. I'll look into that.

I can use the patch-technique I've used in #3569 to get a green PR and make it easier to reproduce the E2E results locally. We only need to remind ourselves to remove the patch before merge :)

EDIT:

Unfortunately, the Neos pull request is currently facing some blockers.

What are the blockers?

[markusguenther]

What are the blockers?

Maybe blocker was a wrong word ... meant the performance concerns of @Sebobo regarding the context creation.

[markusguenther]

Thank you for your pull request. I appreciate your hard work on this fix.

After discussing this with @mhsdesign, we have decided to postpone merging this change. The main concern is that merging a change this late in the development cycle of 7.3 could potentially introduce regressions.

We would like to thank you for your understanding. Meanwhile, please feel free to continue working on this fix, and we will target Neos-UI 8.3 then.

Thank you again for your contribution 💙

[kitsunet]

yikes with the assignment in the elseif

[kitsunet]

What a mess, thanks for all the research! This fix looks sound to me!

[neos-bot]

🎥 End-to-End Test Recordings

• Recording 1
• Recording 2

These videos demonstrate the end-to-end tests for the changes in this pull request.

[grebaldi]

UPDATE:

I've rebased this branch on 8.3 and added 3 E2E test cases (see: cca5757) that will fail on 8.3 and turn green with this PR.

To make those test cases run in CircleCI, I've temporarily added a patch to the test distribution, so that neos/neos-development-collection#4291 is incorporated during the run (see: 302da1d). That commit must be reverted before this PR can be merged.

To ease verification, here's a summary of the test cases plus before/after videos:

Scenario #1: Moving nodes and then discarding that change does not lead to an error

This corresponds to (I) The UI does not (entirely) recognize that nodes have been moved to a position below a different parent node.

The test takes two document nodes, moves them into another node, discards all changes and asserts that no error message has appeared.

Test run on 8.3

1.mp4

Test run on bugfix/3184/document-move-discard (incl. patch)

1.mp4

Scenario #2: Moved nodes do not just disappear after discarding the move change

This corresponds to (II) Discarded move changes are not properly reflected in the document tree and requires the patch for neos/neos-development-collection#4291.

Like Scenario #1, this test takes two document nodes and moves them into another node. Afterwards it focueses the old parent, reloads the UI and discards all changes. It then asserts that the two moved documents have not disappeared.

Test run on 8.3

1.mp4

Test run on bugfix/3184/document-move-discard (incl. patch)

1.mp4

Scenario #3: Discarding a move change while being on a moved node does not lead to an error in the guest frame

This corresponds to (III) Discarding a node move while having a moved document node open in the guest frame results in an error page.

Like the other two scenarios, this test takes two document nodes and moves them into another node. It then reloads the UI, focuses one of the moved nodes and discards all changes. It then asserts that no error is shown in the guest frame and that the now focused node is the next-higher document node (in reference to the previously focused node).

Test run on 8.3

1.mp4

Test run on bugfix/3184/document-move-discard (incl. patch)

1.mp4

[mhsdesign]

thank you so much for adding the tests. I just ran them on 9.0 to check if we need any change there.
The first two scenarios pass but the third has at the end a wrong assertion, or 9.0 has a bug:

Instead of C being focused MultiA is:

[mhsdesign]

And this seems to be okay from looking at it, as having C highlighted makes no sense as well and in your video its a bit glitchy :D

So i think we can just change the assertion when up-merging.

[mhsdesign]

Wow thank you so much for adding the tests and taking care ❤️

The detail you put in your comments is more than astonishing 🤯 Thanks a lot!

[Sebobo]

Should we merge this fix asap?

[mhsdesign]

@Sebobo and me wondered how much this change is dependant on neos/neos-development-collection#4291 and what are the implications if this is merged and release first, or the other way around.

FIX #3184: Discarded node move changes are reflected correctly in the document tree

Description	plain 8.3	only Neos fix	only Ui fix	both fixes	error
Scenario 1: Moving nodes and then discarding that change does not lead to an error	❌	❌	✅	✅	Call to a member function isRemoved() on null
Scenario 2: Moved nodes do not just disappear after discarding the move change	❌	✅	❌	✅	Document nodes disappeared in tree
Scenario 3: Discarding a move change while being on a moved node does not lead to an error in the guest frame	❌	❌	✅	✅	Sorry, the page you requested was not fount

... but the rest of the ui continues to function