From 5ca1f6c4204f117dcbbbbef3dfb2b29994ce9f1e Mon Sep 17 00:00:00 2001 From: Maarten Breddels Date: Mon, 28 Sep 2026 17:54:35 +0200 Subject: [PATCH 1/3] Fuzz keyed children that move out of a replaced container Review of the previous fuzz test found a case it cannot reach: a child with an explicit key that moves to a sibling container while the container it leaves is replaced by one of another type. The random trees now have a Mover component that does this with an element made once (use_memo), so it is the same object in every render. On master, 2 of the 12 seeds fail: the fast renderer has a closed widget in the tree. Over 300 seeds, about 1 in 5 fail, some with a KeyError in the fast renderer. Fixed in the next commit. Co-Authored-By: Claude Opus 5.5 (1M context) --- reacton/fuzz_test.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/reacton/fuzz_test.py b/reacton/fuzz_test.py index e8d1feb..8e6312b 100644 --- a/reacton/fuzz_test.py +++ b/reacton/fuzz_test.py @@ -69,6 +69,20 @@ def set_value(value): return w.Label(value=f"caught {exception}") return w.HBox(children=[Thrower(id * 3 + 2), Leaf(id * 3 + 1)]) + @react.component + def Mover(id): + # an element made once (the same object in every render) with an explicit key, + # that moves between two sibling containers whose types flip + state, set_state = react.use_state(0) + registry[id] = set_state + child = react.use_memo(lambda: Leaf(id * 3 + 2).key(f"moved {id}"), []) + kind = h(id, state) % 8 + First = w.HBox if kind & 2 else w.VBox + Second = w.HBox if kind & 4 else w.VBox + if kind & 1: + return w.VBox(children=[First(children=[child]).key("first"), Second(children=[]).key("second")]) + return w.VBox(children=[First(children=[]).key("first"), Second(children=[child]).key("second")]) + @react.component def Node(id, depth): state, set_state = react.use_state(0) @@ -98,6 +112,8 @@ def cleanup(): child = Wrapper(child_id) elif r < 0.7: child = Catcher(child_id) + elif r < 0.8: + child = Mover(child_id) else: child = Leaf(child_id) if rnd.random() < 0.3: From 5ed17a8dcc8c84611ed7e1ffd6e46209959fc179 Mon Sep 17 00:00:00 2001 From: Maarten Breddels Date: Mon, 28 Sep 2026 17:54:43 +0200 Subject: [PATCH 2/3] Keep keyed children that move out of a replaced container When reconciliation replaces a widget by one of another type (a VBox becomes an HBox), it first removes the old widget with all its children, by key. An explicit key is the same in any container of a component, so a child with an explicit key that moved to a sibling container in the same render was removed too: - if the sibling was reconciled first, the moved child's widget was closed while it was in the tree (both renderers); - if it came later, the fast renderer raised a KeyError, since it had kept the child's subtree as it was. While a replaced element is removed, its children with an explicit key that the new tree still uses are now left alone, and reconciliation updates them where the new tree has them. A keyed child that stays under the new container keeps its widget instead of getting a new one (its state was kept already). The fuzz test: on master 2 of the 12 seeds fail, and about 1 in 5 of 300; with this fix 1000 of 1000 pass. Co-Authored-By: Claude Opus 5.5 (1M context) --- reacton/core.py | 35 +++++++++++++++++++++++++++++++---- reacton/core_test.py | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/reacton/core.py b/reacton/core.py index 40c0eeb..110bea4 100644 --- a/reacton/core.py +++ b/reacton/core.py @@ -1306,6 +1306,27 @@ def __call__(self): class _RenderContext: context: Optional[ComponentContext] = None + # the element that an element of another type replaces, and its context, while + # reconciliation removes it (see _remove_replaced) + _replaced: Optional[Tuple[ComponentContext, Element]] = None + + def _remove_replaced(self, el_prev: Element, key: str, parent_key: str): + """Remove the element at key, which an element of another type replaces.""" + assert self.context is not None + replaced = self._replaced + self._replaced = (self.context, el_prev) + try: + self._remove_element(el_prev, key, parent_key=parent_key) + finally: + self._replaced = replaced + + def _keep_keyed_child(self, context: ComponentContext, el: Element, key: str) -> bool: + # An explicit key is the same in any container of a component, so a child with an + # explicit key that the new tree still uses (it moved out of the replaced element, or + # stays under the new one) is not removed with the replaced element: reconciliation + # updates it where the new tree has it. + replaced = self._replaced + return replaced is not None and context is replaced[0] and el is not replaced[1] and el._key is not None and key in context.used_keys def __init__(self, element: Element, container: widgets.Widget = None, children_trait="children", handle_error: bool = True, initial_state=None): self.element = element @@ -2094,7 +2115,7 @@ def _reconsolidate(self, el: Element, default_key: str, parent_key: str): try: if isinstance(el.component, ComponentFunction): if el_prev and isinstance(el_prev.component, ComponentWidget): - self._remove_element(el_prev, default_key=key, parent_key=parent_key) + self._remove_replaced(el_prev, key, parent_key=parent_key) new_parent_key = join_key(parent_key, key) try: # TODO: test suite passes when this block if commented out @@ -2271,7 +2292,7 @@ def reconsolidate_children(): else: assert el_prev is not None, "widget_previous is not None, but el_prev is" logger.debug("Replacing widget: %r → %r %r", el_prev, el, key) - self._remove_element(el_prev, key, parent_key=parent_key) + self._remove_replaced(el_prev, key, parent_key=parent_key) kwargs = reconsolidate_children() widget = None if not context.exceptions_children: @@ -2367,6 +2388,9 @@ def _remove_element(self, el: Element, default_key: str, parent_key): context = self.context logger.debug("Remove: (%s, %s) %r", parent_key, key, el) + if self._keep_keyed_child(context, el, key): + return + if el.is_shared: if el not in self._shared_elements: return @@ -2769,7 +2793,7 @@ def _reconsolidate(self, el: Element, default_key: str, parent_key: str): if el_prev and isinstance(el_prev.component, ComponentWidget): # a widget element was replaced by a component element at this key - self._remove_element(el_prev, default_key=key, parent_key=parent_key) + self._remove_replaced(el_prev, key, parent_key=parent_key) new_parent_key = join_key(parent_key, key) try: if el.is_shared and (el.args or el.kwargs): @@ -2877,7 +2901,7 @@ def _reconsolidate(self, el: Element, default_key: str, parent_key: str): else: assert el_prev is not None, "widget_previous is not None, but el_prev is" # a different widget type at the same key: replace - self._remove_element(el_prev, key, parent_key=parent_key) + self._remove_replaced(el_prev, key, parent_key=parent_key) kwargs = self._visit_children_values(el.kwargs, key, parent_key, self._reconsolidate) widget = None if not context.exceptions_children: @@ -3041,6 +3065,9 @@ def _remove_element(self, el: Element, default_key: str, parent_key): context = self.context assert context is not None + if self._keep_keyed_child(context, el, key): + return + if el.is_shared: if el not in self._shared_elements: # another use of this element keeps it alive, or it was already removed diff --git a/reacton/core_test.py b/reacton/core_test.py index 581253c..25d2a10 100644 --- a/reacton/core_test.py +++ b/reacton/core_test.py @@ -4187,3 +4187,41 @@ def App(): "new run", ] rc.close() + + +@pytest.mark.parametrize("from_first", [True, False]) +def test_keyed_child_moves_out_of_replaced_container(from_first): + # an element with an explicit key (the same object in every render) moves to a sibling + # container, while the container it leaves is replaced by one of another type: the + # removal of the old container must not remove (or close) the moved child + set_moved = lambda x: None # noqa + + @react.component + def Child(): + return w.Button(description="child") + + child = Child().key("x") + + @react.component + def Test(): + nonlocal set_moved + moved, set_moved = react.use_state(False) + source = [child] if not moved else [] + target = [child] if moved else [] + Source = w.HBox if moved else w.VBox + containers = [Source(children=source).key("source"), w.VBox(children=target).key("target")] + if not from_first: + containers = containers[::-1] + return w.VBox(children=containers) + + box, rc = react.render(Test(), handle_error=False) + button = rc.find(widgets.Button).widget + set_moved(True) + buttons = rc.find(widgets.Button) + assert len(buttons) == 1 + assert buttons.widget.comm is not None + assert rc.find(widgets.HBox).widget.children == () + set_moved(False) + assert rc.find(widgets.Button).widget.comm is not None + assert button.comm is not None + rc.close() From 821f7a3bee040982e38f0cafdb1637bb9adfb51b Mon Sep 17 00:00:00 2001 From: Maarten Breddels Date: Mon, 28 Sep 2026 18:14:40 +0200 Subject: [PATCH 3/3] Leave shared keyed children to the old removal Review found that keeping a keyed child of a replaced container also kept shared elements (.shared()), whose bookkeeping needs the removal: the element stayed in _shared_elements, so reconciliation raised "Element not reconsolidated", and old _shared_widgets entries piled up. Shared elements now keep the old behavior. Co-Authored-By: Claude Opus 5.5 (1M context) --- reacton/core.py | 7 +++++-- reacton/core_test.py | 20 ++++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/reacton/core.py b/reacton/core.py index 110bea4..76adc3b 100644 --- a/reacton/core.py +++ b/reacton/core.py @@ -1324,9 +1324,12 @@ def _keep_keyed_child(self, context: ComponentContext, el: Element, key: str) -> # An explicit key is the same in any container of a component, so a child with an # explicit key that the new tree still uses (it moved out of the replaced element, or # stays under the new one) is not removed with the replaced element: reconciliation - # updates it where the new tree has it. + # updates it where the new tree has it. Shared elements keep the old behavior: their + # bookkeeping (_shared_elements, _shared_widgets) needs the removal. replaced = self._replaced - return replaced is not None and context is replaced[0] and el is not replaced[1] and el._key is not None and key in context.used_keys + return ( + replaced is not None and context is replaced[0] and el is not replaced[1] and el._key is not None and not el.is_shared and key in context.used_keys + ) def __init__(self, element: Element, container: widgets.Widget = None, children_trait="children", handle_error: bool = True, initial_state=None): self.element = element diff --git a/reacton/core_test.py b/reacton/core_test.py index 25d2a10..fa2955c 100644 --- a/reacton/core_test.py +++ b/reacton/core_test.py @@ -4225,3 +4225,23 @@ def Test(): assert rc.find(widgets.Button).widget.comm is not None assert button.comm is not None rc.close() + + +def test_shared_keyed_child_in_replaced_container(): + # a shared element keeps the old behavior: it is removed with the replaced container + # and made again, and no old shared mappings stay behind + set_flip = lambda x: None # noqa + + @react.component + def Test(): + nonlocal set_flip + flip, set_flip = react.use_state(False) + child = w.Button(description="shared").key("x").shared() + return w.VBox(children=[(w.HBox if flip else w.VBox)(children=[child])]) + + box, rc = react.render(Test(), handle_error=False) + for flip in [True, False, True]: + set_flip(flip) + assert rc.find(widgets.Button).widget.comm is not None + assert len(rc._shared_widgets) == 1 + rc.close()