Skip to content

Commit 095e687

Browse files
authored
Do not memoize parent component of Foreach (#6454)
* Do not memoize parent component of Foreach Avoid issue where broader-than-needed SNAPSHOT memoization was occuring when these components should have been memoized separately. * add test case test_foreach_parent_does_not_absorb_sibling_into_snapshot
1 parent f2a9517 commit 095e687

4 files changed

Lines changed: 82 additions & 34 deletions

File tree

AGENTS.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ docs/ # documentation site (separate workspace member)
4646
- No block comments (`# --- Section ---`, `# ============`). Plain inline comments only.
4747
- Be cautious creating new public APIs — they must be documented and supported long-term.
4848
- Google-style docstrings on all functions: one-line summary, optional detail sentence(s), then Args/Returns (or Yields)/Raises.
49+
- Prefer imports at the top of the module in isort order. Only use inline imports when necessary to avoid circular dependencies.
4950

5051
## Testing
5152

packages/reflex-base/src/reflex_base/components/memoize_helpers.py

Lines changed: 1 addition & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -214,26 +214,6 @@ def _is_structural_memoization_child(component: Component) -> bool:
214214
return bool(isinstance(component, Foreach))
215215

216216

217-
def _has_memoization_snapshot_child(component: Component) -> bool:
218-
"""Whether ``component`` has a structural child that needs a memo snapshot.
219-
220-
Component-valued ``Foreach`` is a structural form, not an ordinary user
221-
child. It must render its body in the memo module instead of flowing
222-
through as a normal ``children`` payload.
223-
224-
Args:
225-
component: The component whose direct children should be inspected.
226-
227-
Returns:
228-
True when a direct child requires the parent memo wrapper to render a
229-
captured snapshot.
230-
"""
231-
return any(
232-
isinstance(child, Component) and _is_structural_memoization_child(child)
233-
for child in component.children
234-
)
235-
236-
237217
def passthrough_children_var(
238218
children: list[BaseComponent],
239219
) -> ArrayVar[list[BaseComponent]] | None:
@@ -275,11 +255,7 @@ def get_memoization_strategy(component: Component) -> MemoizationStrategy:
275255
Returns:
276256
The strategy to use when generating a memo wrapper.
277257
"""
278-
if (
279-
is_snapshot_boundary(component)
280-
or _is_structural_memoization_child(component)
281-
or _has_memoization_snapshot_child(component)
282-
):
258+
if is_snapshot_boundary(component) or _is_structural_memoization_child(component):
283259
return MemoizationStrategy.SNAPSHOT
284260

285261
return MemoizationStrategy.PASSTHROUGH

reflex/compiler/plugins/memoize.py

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
)
3131
from reflex_base.components.memoize_helpers import (
3232
MemoizationStrategy,
33+
_is_structural_memoization_child,
3334
fix_event_triggers_for_memo,
3435
get_memoization_strategy,
3536
is_snapshot_boundary,
@@ -188,8 +189,16 @@ def _should_memoize(component: Component) -> bool:
188189
):
189190
return True
190191
# Cond and Match render conditional branch JSX from their own props rather
191-
# than from a tag, so they have no `tag` but still must be considered.
192-
if component.tag is None and not isinstance(component, (Cond, Match)):
192+
# than from a tag; structural memoization children (e.g. ``Foreach``)
193+
# render via their own structural form. All have no ``tag`` but still
194+
# must be considered for memoization — the structural-child case in
195+
# particular owns its whole subtree as a snapshot, so if it does not
196+
# wrap here, its descendants leak to the page module.
197+
if (
198+
component.tag is None
199+
and not isinstance(component, (Cond, Match))
200+
and not _is_structural_memoization_child(component)
201+
):
193202
return False
194203
if component._memoization_mode.disposition == MemoizationDisposition.ALWAYS:
195204
return True

tests/units/compiler/test_memoize_plugin.py

Lines changed: 69 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,18 +8,20 @@
88
from typing import Any, cast
99

1010
import pytest
11-
from reflex_base.components.component import Component, field
11+
from reflex_base.components.component import Component
12+
from reflex_base.components.component import field as component_field
1213
from reflex_base.components.memoize_helpers import (
1314
MemoizationStrategy,
1415
get_memoization_strategy,
1516
)
1617
from reflex_base.constants.compiler import MemoizationDisposition, MemoizationMode
1718
from reflex_base.plugins import CompileContext, CompilerHooks, PageContext
1819
from reflex_base.vars import VarData
19-
from reflex_base.vars.base import LiteralVar, Var
20+
from reflex_base.vars.base import Field, LiteralVar, Var, field
2021
from reflex_components_core.base.bare import Bare
2122
from reflex_components_core.base.fragment import Fragment
2223
from reflex_components_core.base.link import RawLink, ScriptTag
24+
from reflex_components_core.core.foreach import Foreach
2325
from reflex_components_core.el.elements.forms import BaseInput, Textarea
2426
from reflex_components_core.el.elements.inline import Br, Wbr
2527
from reflex_components_core.el.elements.media import (
@@ -37,13 +39,15 @@
3739
from reflex_components_core.el.elements.scripts import Noscript, Script
3840
from reflex_components_core.el.elements.tables import Col
3941
from reflex_components_core.el.elements.typography import Hr
42+
from reflex_components_radix.themes.layout.box import Box
4043

4144
import reflex as rx
4245
import reflex.compiler.plugins.memoize as memoize_plugin
4346
from reflex.compiler.plugins import DefaultCollectorPlugin, default_page_plugins
4447
from reflex.compiler.plugins.memoize import MemoizeStatefulPlugin, _should_memoize
4548
from reflex.experimental.memo import (
4649
ExperimentalMemoComponent,
50+
ExperimentalMemoComponentDefinition,
4751
create_passthrough_component_memo,
4852
)
4953
from reflex.state import BaseState
@@ -62,7 +66,7 @@ class WithProp(Component):
6266
tag = "WithProp"
6367
library = "with-prop-lib"
6468

65-
label: Var[str] = field(default=LiteralVar.create(""))
69+
label: Var[str] = component_field(default=LiteralVar.create(""))
6670

6771

6872
class LeafComponent(Component):
@@ -72,9 +76,9 @@ class LeafComponent(Component):
7276

7377

7478
class SpecialFormMemoState(BaseState):
75-
items: list[str] = ["a"]
76-
flag: bool = True
77-
value: str = "a"
79+
items: Field[list[str]] = field(default_factory=lambda: ["a"])
80+
flag: Field[bool] = field(default=True)
81+
value: Field[str] = field(default="a")
7882

7983

8084
@dataclasses.dataclass(slots=True)
@@ -251,9 +255,61 @@ def special_child() -> Component:
251255
assert body_marker not in page_output
252256

253257

258+
def test_foreach_parent_does_not_absorb_sibling_into_snapshot() -> None:
259+
"""Foreach owns its own snapshot while the parent stays passthrough.
260+
261+
Regression for the foreach-parent memoization fix: a parent component
262+
holding a Foreach used to be promoted to SNAPSHOT, absorbing any sibling
263+
reactive content into the same wide memo body. The parent should now render
264+
on the page side, with Foreach and any reactive sibling each getting their
265+
own independent wrapper.
266+
"""
267+
ctx, _page_ctx = _compile_single_page(
268+
lambda: rx.box(
269+
Bare.create(SpecialFormMemoState.items.length()),
270+
rx.foreach(
271+
SpecialFormMemoState.items,
272+
lambda item: rx.text(item),
273+
),
274+
)
275+
)
276+
277+
wrapped_definitions = [
278+
definition
279+
for definition in ctx.auto_memo_components.values()
280+
if isinstance(definition, ExperimentalMemoComponentDefinition)
281+
]
282+
wrapped_types = {type(definition.component) for definition in wrapped_definitions}
283+
284+
assert len(wrapped_definitions) == 2
285+
assert Box not in wrapped_types
286+
287+
foreach_definition = next(
288+
definition
289+
for definition in wrapped_definitions
290+
if isinstance(definition.component, Foreach)
291+
)
292+
assert (
293+
get_memoization_strategy(foreach_definition.component)
294+
is MemoizationStrategy.SNAPSHOT
295+
)
296+
297+
bare_definition = next(
298+
definition
299+
for definition in wrapped_definitions
300+
if isinstance(definition.component, Bare)
301+
)
302+
assert (
303+
get_memoization_strategy(bare_definition.component)
304+
is MemoizationStrategy.PASSTHROUGH
305+
)
306+
assert bare_definition is not foreach_definition
307+
308+
254309
def test_common_memoization_snapshot_helper_classifies_snapshot_cases() -> None:
255310
"""The shared memoization strategy classifies structural render forms."""
256311
from reflex_components_core.core.cond import Cond
312+
from reflex_components_core.core.foreach import Foreach
257313
from reflex_components_core.core.match import Match
258314
from reflex_components_core.el.elements.forms import Form, Input
259315

@@ -280,7 +336,13 @@ def test_common_memoization_snapshot_helper_classifies_snapshot_cases() -> None:
280336
),
281337
)
282338

283-
assert get_memoization_strategy(foreach_parent) is MemoizationStrategy.SNAPSHOT
339+
# A parent with a structural-memoization child (Foreach) is itself
340+
# PASSTHROUGH — the snapshotting is owned by the structural child, which
341+
# captures its whole subtree.
342+
assert get_memoization_strategy(foreach_parent) is MemoizationStrategy.PASSTHROUGH
343+
foreach_child = foreach_parent.children[0]
344+
assert isinstance(foreach_child, Foreach)
345+
assert get_memoization_strategy(foreach_child) is MemoizationStrategy.SNAPSHOT
284346
assert get_memoization_strategy(cond_fragment) is MemoizationStrategy.PASSTHROUGH
285347
# Cond and Match now use passthrough so branch JSX renders on the page side
286348
# and the memo body just selects via children[i] indexing.

0 commit comments

Comments
 (0)