web: tolerate nodes created and dropped inside one patch batch - #75
Merged
Merged
Conversation
Contributor
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
… for dropped nodes - drop_node/emit_dropped_subtree: when a node's create op is still in pending_ops, prune every queued op mentioning it and skip the drop op instead of emitting an unreplayable create+drop pair (the backend commits the batch before DOM replay, so post-commit lookups for the node always miss and the Invalid_argument wedges the generation). - dispatch: return early when the event's node is no longer live — listeners can fire after their node was dropped. - web apply: include the failing op JSON and label the roving-focus passes in Invalid_argument messages for diagnosability.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The web DOM applier crashed with
unknown DOM nodewhen a patch batch created a node and dropped it again inside the same batch (a keyed remount that replaces a subtree mid-flush). Ops are replayed against the retained store's final state, so a node absent at batch end had noplatform_nodeto resolve:CreateNode/CreateExtension→Nodes.dom_nodethrew on the missing store record.InsertChild/RemoveChild/MoveChild→dom_node/dom_node_beforethrew for the transient node.SetProp→invalid_arg "unknown DOM node"(inconsistent withRemoveProp, which already tolerated misses).Changes
known_node(current store ∪previous_nodes) and skipInsertChild/RemoveChild/MoveChildwhose parent or child is unresolvable — the element was either never created or is being discarded with its batch.CreateNode/CreateExtensionset-id now resolve the record viaStore.nodeand skip when absent.insert_child_domresolves parent/child viadom_node_beforeand takesprevious_nodes; a child missing from the store still gets inserted (a laterRemoveChildin the same stream detaches it).SetPropon a missing node is now a no-op, matchingRemoveProp.Reproduced in Logseq's e2e (Meta+k opening cmdk while the editor mounted → batch contained
create-extension → … → dropfor the same ids →Invalid_argumentaborted the flush and the palette never opened). After this change the batch applies cleanly.Link to Devin session: https://app.devin.ai/sessions/5d6b198dceb54bb2b8fa02c8a412f45c
Open in Devin Desktop: https://app.devin.ai/desktop/session/5d6b198dceb54bb2b8fa02c8a412f45c?variant=devin
Requested by: @tiensonqin