Repository navigation
Fix local client state across sibling components - #7442
harsh21234i wants to merge 17 commits into
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
[Medium impact] The change affects page compilation for pages that use local client state. The PR needs one more fix before merging: default styles can still split shared local state. Findings
SummaryKeeps sibling components that share local client state in the same React scope.
Reviews (16) · Last reviewed commit: "fix: preserve empty component style scop..." · Reviewed by Greptile |
Merging this PR will degrade performance by 5.5%2 warnings
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing |
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
…42-conflict # Conflicts: # .github/workflows/examples.yml
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
View guided diff | Re-trigger cubic
masenf
left a comment
There was a problem hiding this comment.
i'm not too keen on patching memo plugin so deeply to support the old experimental client_state.
the new client state already uses scoped access on the javascript side and just works transparently without all this compiler machinery.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
|
Thanks, I understand the concern. I’ve simplified the prepass based on the review feedback, but this still adds compiler logic for the legacy experimental API. Since the newer client-state API already handles scoped access, would you prefer we keep a compatibility fix for this existing behavior, or close/defer this issue and direct users to the newer API? |
| for data in var_data: | ||
| local_scope_keys.update(data.shared_scope_keys) | ||
| local_scope_keys.update( | ||
| _component_style_scope_keys(component, style_scope_keys) | ||
| ) |
There was a problem hiding this comment.
Local state added by add_style() is missing from this scan. For example, subclass AutoScroll, return {"color": local.value} from add_style(), and place it beside a local.value reader and setter. The root scan runs before the child's default style is applied and only accounts for App.style. The child can then get its own memo component and state, so clicking the outside setter leaves its color unchanged.
Include default-style state users before computing the shared-scope decisions.
Fixes #7426, where calling a local client-state setter from a sibling component could raise a
ReferenceErrorand leave the displayed value unchanged.The compiler now detects when local client state is shared across components and keeps those components in a compatible React scope. Added a compiler regression test and a Playwright test that verifies the sibling setter updates the rendered value. Included changelog entries for both affected packages.
Testing: Targeted unit and Playwright tests, Ruff, Pyright, and
git diff --checkpass locally.