Repository navigation
fix: capture client-state setter expressions in event scope - #7692
Draft
DevChiniwala wants to merge 1 commit into
Draft
DevChiniwala wants to merge 1 commit into
DevChiniwala wants to merge 1 commit into
Conversation
Do not turn a value expression into a JavaScript function parameter. Optional chaining and property access are not identifiers, and shadowing the enclosing event can also read the wrong value. Bind explicit values in a zero-argument closure while leaving the no-value setter unchanged. Execute 18 generated callback cases, including 14 failing regressions on unchanged source. Cover raw and typed event arguments and direct setter bindings in the existing browser app in development and production. All 164 affected tests, 14 browser cases and nine presubmit hooks pass on the refreshed main branch. The full unit suite remains in progress. Fixes reflex-dev#5741.
Contributor
[Medium impact] The PR appears safe to merge. Summary
No actionable issues were found. Tests were inspected, not run during this review. Reviews (1) · Last reviewed commit: "fix: capture client-state setter express..." · Reviewed by Greptile |
Author
|
Final validation for
The PR description includes these limits, and this remains a draft. Could a maintainer approve the fork workflows and advise on the backend-reload gate? Related readiness work is already tracked in #7416. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5741.
ClientStateVar.set_value()derives a JavaScript parameter name from its value. A custom event argument such as_event?.clientXtherefore produces invalid JavaScript; other property expressions can shadow the enclosing event and read the wrong value.Capture an explicit value in a zero-argument closure instead. The enclosing event callback supplies its scope, and
.setwithout an explicit value keeps accepting the trigger's argument. Setter imports and initialization hooks are retained.Validation on
mainat6c3713012522:call_functionandrun_script..setbindings and the existing shared-state behavior.Full Linux suite: 15,281 passed, 64 skipped, two backend-reload failures. The backend-refusal/recovery test also fails on a clean worktree at the same base. The reload-hold test fails in the full suite but passes in both unchanged and patched isolated controls. The changed client-state tests pass in the full suite. These backend tests and implementation are unchanged by this PR; related readiness work is already open in #7416.
Keeping this draft pending backend-gate guidance and approval of the fork workflows. No complete upstream or local full-suite pass is claimed.
All submissions