-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix: avoid duplicating inner tag in MemoComponent wrapper tag #7004
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Memo wrappers no longer duplicate the wrapped component's tag when computing the generated component name, so `MemoComponent` instances get stable, collision-free names instead of a doubled tag prefix. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Auto-memoized `@rx.memo` wrapper tags no longer duplicate the wrapped component's tag. `MemoComponent._compute_memo_tag` now skips the `self.tag` segment, which is already embedded in the dynamic subclass `__qualname__` (`MemoComponent_<tag>`), so generated wrapper names read as `Memocomponent_<tag>_<hash>` instead of `Memocomponent_<tag>_<tag>_<hash>`. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -398,6 +398,34 @@ def _validate_component_children(self, children: list[Component]) -> None: | |
| children: The children of the component (ignored). | ||
| """ | ||
|
|
||
| def _compute_memo_tag(self) -> str: | ||
|
LeonxLJX marked this conversation as resolved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This override repeats the whole body of |
||
| """Compute a stable tag name for this memo component. | ||
|
|
||
| Overrides ``Component._compute_memo_tag`` to avoid duplicating the | ||
| wrapped component's tag. For a ``MemoComponent`` the dynamic subclass | ||
| ``__qualname__`` is ``MemoComponent_<tag>`` (see | ||
| :func:`_get_memo_component_class`), which already encodes the inner | ||
| tag. Appending ``self.tag`` again would produce the inner tag twice | ||
| (e.g. ``Memocomponent_card_98ffd1e1_card_98ffd1e1_<hash>``). | ||
|
|
||
| The class identity is still preserved via the qualname prefix, so the | ||
| collision guarantee from the base implementation holds. | ||
|
|
||
| Returns: | ||
| The stable tag name. | ||
| """ | ||
| from reflex_base.components.memoize_helpers import ( | ||
| MemoizationStrategy, | ||
| get_memoization_strategy, | ||
| ) | ||
|
|
||
| comp_hash = self._get_component_hash( | ||
| shallow=get_memoization_strategy(self) == MemoizationStrategy.PASSTHROUGH | ||
| ) | ||
| return format.format_state_name( | ||
| f"{type(self).__qualname__}_{comp_hash}" | ||
| ).capitalize() | ||
|
|
||
| def _post_init(self, **kwargs): | ||
| """Initialize the memo component. | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2593,3 +2593,35 @@ def test_each_memo_wrapper_emits_one_component_module_file() -> None: | |
| "for Plain, one for WithProp, and one snapshot wrapper for the " | ||
| f"LeafComponent boundary. Got: {sorted(ctx.memoize_wrappers)}" | ||
| ) | ||
|
|
||
|
|
||
| def test_memo_wrapper_tag_contains_inner_tag_once() -> None: | ||
| """Auto-memo wrapper tags must not duplicate the wrapped component's tag. | ||
|
|
||
| Regression test for #6955: ``MemoComponent._compute_memo_tag`` previously | ||
| concatenated ``type(self).__qualname__`` (which already embeds the inner | ||
| tag as ``MemoComponent_<inner_tag>``) with ``self.tag`` again, producing | ||
| names like ``Memocomponent_plain_abc123_plain_abc123_<hash>``. The inner | ||
| tag must appear exactly once in the wrapper tag. | ||
| """ | ||
| ctx, _page_ctx = _compile_single_page(lambda: Plain.create(STATE_VAR)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test fails on this branch.
The doubling this PR fixes only occurs when a |
||
|
|
||
| assert len(ctx.memoize_wrappers) == 1, ( | ||
| f"Expected exactly one auto-memo wrapper, got: {sorted(ctx.memoize_wrappers)}" | ||
| ) | ||
| wrapper_tag = next(iter(ctx.memoize_wrappers)) | ||
| # The inner component's tag is "Plain"; after format_state_name it becomes | ||
| # lowercase "plain" in the wrapper tag. Count occurrences of the inner tag | ||
| # segment (case-insensitive, word-bounded to avoid matching hash substrings). | ||
| inner_tag_lower = Plain.tag.lower() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| # Split on underscores and count exact matches of the inner tag | ||
| segments = wrapper_tag.lower().split("_") | ||
| inner_tag_occurrences = segments.count(inner_tag_lower) | ||
| assert inner_tag_occurrences == 1, ( | ||
| f"Inner tag '{Plain.tag}' should appear exactly once in wrapper tag, " | ||
| f"found {inner_tag_occurrences} times. wrapper_tag={wrapper_tag}" | ||
| ) | ||
| # The wrapper tag should still start with the MemoComponent prefix | ||
| assert wrapper_tag.lower().startswith("memocomponent"), ( | ||
| f"Wrapper tag should keep the MemoComponent prefix, got: {wrapper_tag}" | ||
| ) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the reflex-base changelog is materialized, the
6955.bugfix.mdfilename associates this change with PR 6955 instead of the current PR 7004, causing the generated release note to link to the wrong pull request.Context Used: CLAUDE.md (source)
Knowledge Base Used: Release engineering