Skip to content

Commit 9d52671

Browse files
Avoid redundant anchor lookups in Fragment.anchor() and unkeyed v-for
- anchor(): a membership check followed by .index() scanned the parent's children twice; use a single .index() in a try/except, for both children and slot_contents. - Unkeyed ListFragment updates mounted every appended fragment with a freshly computed self.anchor(), which walks the tree per item even though the anchor (the first element after the list) cannot change while appending items from the list's own subtree. Compute it lazily once per update run, turning growth by n items from O(n^2) anchor work into O(n). Same hoist in ListFragment.mount(). Adds a regression test asserting that growing an unkeyed v-for with a trailing sibling inserts elements in order, each anchored before that sibling. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 8a15408 commit 9d52671

2 files changed

Lines changed: 76 additions & 10 deletions

File tree

collagraph/fragment.py

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -145,22 +145,31 @@ def anchor(self) -> Any | None:
145145
assert parent is not None
146146

147147
# Check if this fragment is in parent's children
148-
if self in parent.children:
148+
try:
149149
idx = parent.children.index(self)
150+
except ValueError:
151+
idx = None
152+
153+
if idx is not None:
150154
length = len(parent.children) - 1
151155
while 0 <= idx < length:
152156
idx += 1
153157
if element := parent.children[idx].first():
154158
return element
155159
# Fragment might be slot content - check parent's slot_contents if it's
156160
# a ComponentFragment
157-
elif isinstance(parent, ComponentFragment) and self in parent.slot_contents:
158-
idx = parent.slot_contents.index(self)
159-
length = len(parent.slot_contents) - 1
160-
while 0 <= idx < length:
161-
idx += 1
162-
if element := parent.slot_contents[idx].first():
163-
return element
161+
elif isinstance(parent, ComponentFragment):
162+
try:
163+
idx = parent.slot_contents.index(self)
164+
except ValueError:
165+
idx = None
166+
167+
if idx is not None:
168+
length = len(parent.slot_contents) - 1
169+
while 0 <= idx < length:
170+
idx += 1
171+
if element := parent.slot_contents[idx].first():
172+
return element
164173

165174
# No sibling anchor found at this level. If the parent doesn't have
166175
# its own element (e.g., ComponentFragment, ControlFlowFragment), climb
@@ -706,11 +715,18 @@ def update_children(self):
706715
fragment.unmount()
707716
self.values.pop(index)
708717

718+
# The anchor is loop-invariant: it is the first element
719+
# *after* this ListFragment, and the loop below only
720+
# appends items from the list's own subtree before it.
721+
# Computed lazily so update-only runs don't pay for it.
722+
anchor = _UNSET
709723
for i, item in enumerate(items):
710724
if i < len(self.children):
711725
# Update the content for existing values
712726
self.values[i]["context"] = item
713727
else:
728+
if anchor is _UNSET:
729+
anchor = self.anchor()
714730
# Create a new fragment + context
715731
context = reactive({"context": item})
716732
self.values.append(context)
@@ -719,15 +735,18 @@ def update_children(self):
719735
)
720736
self.children.append(fragment)
721737
fragment.parent = self
722-
fragment.mount(target, anchor=self.anchor())
738+
fragment.mount(target, anchor=anchor)
723739

724740
# Then we add a watch_effect for the children
725741
# which adds/removes/updates all the child fragments
726742
self._watchers["list"] = watch_effect(update_children)
727743

744+
anchor = _UNSET
728745
for child in self.children:
729746
if not child.element:
730-
child.mount(target, anchor=self.anchor())
747+
if anchor is _UNSET:
748+
anchor = self.anchor()
749+
child.mount(target, anchor=anchor)
731750

732751
self._mounted = True
733752

tests/test_directive_for.py

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -675,3 +675,50 @@ class Items(cg.Component):
675675
first_item, second_item = container["children"]
676676
assert first_item["attrs"]["value"] == "a"
677677
assert second_item["attrs"]["value"] == "b"
678+
679+
680+
def test_for_unkeyed_grow_insert_order(parse_source):
681+
"""
682+
Growing an unkeyed v-for in one reactive update must insert the new
683+
elements in order, each anchored before the element that follows the
684+
list (regression test for hoisting the loop-invariant anchor out of
685+
the append loop).
686+
"""
687+
from tests.conftest import CustomElement, TrackingRenderer
688+
689+
App, _ = parse_source(
690+
"""
691+
<template>
692+
<node v-for="item in items" :content="item" />
693+
<footer content="footer" />
694+
</template>
695+
<script>
696+
import collagraph as cg
697+
class App(cg.Component):
698+
pass
699+
</script>
700+
"""
701+
)
702+
703+
renderer = TrackingRenderer()
704+
gui = Collagraph(renderer, event_loop_type=EventLoopType.SYNC)
705+
container = CustomElement(type="root")
706+
state = reactive({"items": []})
707+
gui.render(App, container, state)
708+
709+
renderer.reset_counters()
710+
state["items"] = ["a", "b", "c"]
711+
712+
inserts = [op for op in renderer.operations if op[0] == "insert"]
713+
assert inserts == [
714+
("insert", "a", "before footer"),
715+
("insert", "b", "before footer"),
716+
("insert", "c", "before footer"),
717+
]
718+
assert [child.type for child in container.children] == [
719+
"node",
720+
"node",
721+
"node",
722+
"footer",
723+
]
724+
assert [child.content for child in container.children[:3]] == ["a", "b", "c"]

0 commit comments

Comments
 (0)