fix: avoid duplicating inner tag in MemoComponent wrapper tag - #7004
fix: avoid duplicating inner tag in MemoComponent wrapper tag#7004LeonxLJX wants to merge 3 commits into
Conversation
Override _compute_memo_tag on MemoComponent to skip the self.tag segment, which is already embedded in the dynamic subclass __qualname__ (MemoComponent_<tag>). Previously the wrapper tag contained the inner tag twice (e.g. Memocomponent_card_98ffd1e1_card_98ffd1e1_<hash>). Adds a regression test asserting the inner tag appears exactly once. Closes reflex-dev#6955
Greptile SummaryThis PR removes the duplicated inner tag from generated memo-wrapper names and adds regression coverage. It also adds root and reflex-base release notes, although the package fragment is currently associated with the wrong pull-request number.
Confidence Score: 4/5The code change appears sound, but the reflex-base news fragment must be renamed so the generated release note references the correct pull request before merging. The package fragment is named 6955.bugfix.md even though this is PR 7004, causing the release record to associate the fix with the wrong pull request. Files Needing Attention: packages/reflex-base/news/6955.bugfix.md
|
| Filename | Overview |
|---|---|
| packages/reflex-base/src/reflex_base/components/memo.py | Overrides memo tag generation to remove the redundant self.tag segment while preserving the component hash and class-qualified prefix. |
| tests/units/compiler/test_memoize_plugin.py | Adds focused regression coverage asserting that generated memo-wrapper tags contain the wrapped tag exactly once. |
| packages/reflex-base/news/6955.bugfix.md | Adds the previously missing package release note, but its filename associates this PR's change with PR 6955 instead of PR 7004. |
| news/7004.bugfix.md | Adds the root-package release note using the current pull-request number. |
Reviews (3): Last reviewed commit: "chore: add reflex-base news fragment for..." | Re-trigger Greptile
Adds the required package news fragment so changelog validation passes for the user-facing memo-wrapper tag fix.
|
Added the missing Root cause. Fix. Override If you'd prefer the fragment wording tuned (e.g. mention the failing case explicitly), happy to adjust. |
| @@ -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>`. | |||
There was a problem hiding this comment.
When the reflex-base changelog is materialized, the 6955.bugfix.md filename 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
|
Bumping for visibility. Greptile flagged a P1 on the |
Merging this PR will not alter performance
Comparing Footnotes
|
| 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)) |
There was a problem hiding this comment.
This test fails on this branch.
STATE_VAR is passed as a child, not a prop. Component.create wraps it in a Bare, so the tree is Plain(Bare(STATE_VAR)) and the state hooks ride on the Bare. The auto-memoize pass wraps the Bare. That wrapper is an ordinary Component, not a MemoComponent, so MemoComponent._compute_memo_tag never runs and the inner tag Plain never appears in the wrapper tag.
AssertionError: Inner tag 'Plain' should appear exactly once in wrapper tag,
found 0 times. wrapper_tag=Bare_comp_bba9dad596f8328a01eabe6aaa98e665
assert 0 == 1
The doubling this PR fixes only occurs when a @rx.memo component receives a state Var as a prop. In that case the export name on main is Memocomponent_statefulcard_14db30e6_statefulcard_14db30e6_dbfbf34c..., and with this fix it is Memocomponent_statefulcard_14db30e6_dbfbf34c....
| # 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() |
There was a problem hiding this comment.
Component.tag is str | None, so the type check gate fails here.
uv run pyright tests/units/compiler/test_memoize_plugin.py
test_memoize_plugin.py:2616:33 - error: "lower" is not a known attribute of "None" (reportOptionalMemberAccess)
| children: The children of the component (ignored). | ||
| """ | ||
|
|
||
| def _compute_memo_tag(self) -> str: |
There was a problem hiding this comment.
This override repeats the whole body of Component._compute_memo_tag. Only one segment of the f-string differs. The imports, the strategy lookup, the hash call and the format_state_name(...).capitalize() now exist in two places, so the hashing policy can drift between them.
Override compute_memo_tag on MemoComponent to skip the self.tag segment, which is already embedded in the dynamic subclass qualname (MemoComponent). Previously the wrapper tag contained the inner tag twice (e.g. Memocomponent_card_98ffd1e1_card_98ffd1e1_).
Adds a regression test asserting the inner tag appears exactly once.
Closes #6955
All Submissions:
Type of change
Please delete options that are not relevant.
New Feature Submission:
Changes To Core Features:
After these steps, you're ready to open a pull request.