web: derive DOM insertion index from actual mount parent - #76
Merged
Merged
Conversation
… 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.
visible_child_index counted retained children by kind, but portal children, never-mounted dynamic segments and same-batch dropped nodes all retain entries without a DOM presence. Counting them pushed the insertion index past the container's real child count and apply_batch raised 'DOM child index is out of bounds'. Count only children whose platform node is a DOM child of the target container right now.
Contributor
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
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
Fixes
DOM child index is out of boundsthrown byapply_batchwhen inserting into a container whose retained children include nodes with no DOM presence (portal children, never-mounted dynamic segments, nodes created and dropped inside the same batch).visible_child_indexmapped a schema child index to a DOM index by skipping only kind-pinned children (child_hidden_in_parent: ContextMenu/DropdownMenu/Toast/modal/tooltip). Any other retained child that never materialized a DOM node still counted, so the computed index ran past the container's real child count andinsert_dom_childraisedInvalid_argument, aborting the whole batch.Repro: an
.cp__overlays-style container with ~8 retained children (cmdk, popups, dialogs, toasts, menu dyns) but only 2 real DOM children; mounting a page menu at schema index 7 threw, wedging the app.The fix derives the visible index from DOM truth instead of kind heuristics: a retained child counts only when its
platform_node'sparentElementis the insertion container right now. This subsumeschild_hidden_in_parent(portal-mounted children have a differentparentElement) and also covers never-mounted segments and same-batch dropped nodes —child_hidden_in_parentis removed.visible_child_indexgains a~containerargument resolved viadom_child_containerat the insert/move call sites.Follow-up to #75.
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