Skip to content

Test: reproduce command removal and replacement affecting stolen nodes - #212

Closed
nguyenyou wants to merge 1 commit into
raquo:masterfrom
nguyenyou:codex/command-stolen-node-reproducer
Closed

nguyenyou wants to merge 1 commit into
raquo:masterfrom
nguyenyou:codex/command-stolen-node-reproducer

Conversation

@nguyenyou

Copy link
Copy Markdown
Contributor

Adds reproducers for children.command <-- targeting a node that another inserter has taken.

Current behavior:

  • When the destination has a different parent, Remove and Replace leave the stolen node untouched.
  • When both inserters share a parent, Remove deletes the stolen node and Replace replaces it in the destination’s region.

Expected behavior:
Both commands should leave nodes outside their own region untouched, consistent with the existing cross-parent tests in ChildrenCommandReceiverSpec.

Tests only; no implementation changes. Against master, the cross-parent reference passes and four same-parent cases fail.

Run:
sbt 'testOnly *ChildrenCommandStolenNodeSpec'

Add a cross-parent reference and four same-parent reproducers for commands affecting nodes taken by another inserter.

Co-Authored-By: Codex GPT-6 Astra <codex@openai.com>
@nguyenyou
nguyenyou requested a review from raquo as a code owner September 23, 2026 07:09
@raquo

raquo commented Sep 23, 2026

Copy link
Copy Markdown
Owner

Oof. That's a tricky one.

children.command is intended as a more limited, but maximum-performance API, for cases where you can't, or more often, don't want to build a list of elements that you could pass to children <--.

To make Remove and Replace reject calls on nodes stolen into a sibling inserter, we'd need to add an O(N) search through the DOM nodes when those commands are fired. This gives me pause, specifically in this API.

The behaviour is indeed inconsistent, but I think a user hitting this issue is actually quite unlikely, even aside from the steal-into-same-parent-sibling precondition. To run Remove / Replace commands, you need a reference to the element being replaced. But, the whole point of children.command references is that you don't keep such references in a central location. So, you're most likely to have that reference in the element in question, i.e. the element might have a "delete" button which would Remove it from the list. But then, if we call this action from the element itself, chances are it's actually going to do what we expect. Not elegant by any means, but still.

Also, note that there's one more issue with stealing elements from children.command – the latter will never update its contentMap to remove the stolen element – and there's no easy solution to that with the current architecture. This is orthogonal to the issue in question, just pointing out that we can't make children.command support stealing perfectly even if we fix this issue.

All of the above considered, I'm inclined to keep the inconsistent behaviour as-is for now.

Long term, I think the solution may be to upgrade the overall inserters architecture so that each inserter remembers its parent inserter, and when it's stolen, does the handoff gracefully, informing its past parent so that it can remove it. But, that's a non-trivial change to the architecture – we need to mind performance, memory use and links, and also need to reconcile this idea with how raw elements keep track of their parents, including for subscription lifetime purposes.

Another idea I considered for simplifying the tracking of nodes and their inserters is to write more data (e.g. links to current inserter and/or to current laminar element) into the raw JS DOM elements. This too could potentially simplify some code, including obviating the need for contentMap in most inserters (except for children <-- I guess).

Either way, this re-architecture is definitely out of scope for v18. Let's keep this PR open, treating it as an issue. I suspect that we may see more such edge cases that need a similar solution – if such do pop up, maybe I'll create an actual issue to collect them.

@raquo raquo added the needs design The solution is not clear, or I am not very happy with it label Sep 23, 2026
@nguyenyou nguyenyou closed this by deleting the head repository Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs design The solution is not clear, or I am not very happy with it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants