Repository navigation
Target the right element when a set_focus/set_value id is built from a Var - #7904
FarhanAliRaza wants to merge 2 commits into
Conversation
…a Var
rx.set_focus, rx.blur_focus and rx.set_value named a ref by formatting the
id at compile time, with the Var marker of an f-string dropped. An id such
as f"row-{item}" inside rx.foreach therefore named one ref for every row
("ref_row_item_rx_state_"), which the last rendered row had registered:
each row's button focused or filled the last row's input.
An id built from Vars is only known when the event runs, so it is now sent
as it evaluates, and the frontend looks the element up by that id when no
ref is registered under the name. A literal id still names its registered
ref, as before. The three events also accept a plain Var id.
Fixes #7838
Claude-Session: https://claude.ai/code/session_01LwJUCTCgM7D4Vj6H4dRWss
[Medium impact] The PR appears safe to merge. SummaryFixes focus, blur, and value changes for element ids built from a
The previous collision finding is fixed. FarhanAliRaza explicitly leaves backend-returned events with computed plain-string ids out of scope to avoid changing every literal-id payload. Reviews (3) · Last reviewed commit: "Send an id built from a Var apart from t..." · Reviewed by Greptile |
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/reflex-frontend/src/utils/state.js">
<violation number="1" location="packages/reflex-frontend/src/utils/state.js:97">
P2: `resolveRef` treats inherited `Object.prototype` names as registered refs, so dynamic ids such as `"toString"` never reach `getElementById` and their focus/value operations target the inherited function instead of the row. Check only own registry keys with `Object.hasOwn(refs, ref)`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| * itself when it is not a string. | ||
| */ | ||
| const resolveRef = (ref) => { | ||
| if (ref in refs) { |
There was a problem hiding this comment.
P2: resolveRef treats inherited Object.prototype names as registered refs, so dynamic ids such as "toString" never reach getElementById and their focus/value operations target the inherited function instead of the row. Check only own registry keys with Object.hasOwn(refs, ref).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/reflex-frontend/src/utils/state.js, line 97:
<comment>`resolveRef` treats inherited `Object.prototype` names as registered refs, so dynamic ids such as `"toString"` never reach `getElementById` and their focus/value operations target the inherited function instead of the row. Check only own registry keys with `Object.hasOwn(refs, ref)`.</comment>
<file context>
@@ -85,6 +85,23 @@ export const useRegisteredRef = (name) => {
+ * itself when it is not a string.
+ */
+const resolveRef = (ref) => {
+ if (ref in refs) {
+ return refs[ref];
+ }
</file context>
| if (ref in refs) { | |
| if (Object.hasOwn(refs, ref)) { |
There was a problem hiding this comment.
Fixed in 96c0897 by a different route than Object.hasOwn. An id built from a Var is now sent as id and looked up with document.getElementById only, so it never reaches the refs lookup; state.test.js covers an element whose id is toString.
The ref in refs check that remains is the expression from main. It only sees names produced by format_ref, which always start with ref_, so an inherited name cannot match it, and I left it as it was rather than widen the diff.
Generated by Claude Code
Merging this PR will not alter performance1 warning
Comparing Footnotes
|
rx.set_focus, rx.blur_focus and rx.set_value sent an id built from a Var in the same `ref` field as the name a literal id registers its ref under, and the frontend tried the registry first. A row id that evaluated to "ref_alpha" therefore acted on the input with the literal id "alpha", and one spelled like a name every object inherits, such as "toString", found that instead of the element. The id is now sent as `id` and looked up in the document only; `ref` names a registered ref only, resolved as it was before. The frontend test imports `refs` rather than reading the global. Claude-Session: https://claude.ai/code/session_01LwJUCTCgM7D4Vj6H4dRWss
All Submissions:
Type of change
Changes To Core Features:
Problem
Clicking "focus" or "fill" on the first row acted on the last row's input.
rx.set_focus,rx.blur_focusandrx.set_valueformat the id into a ref name at compile time and drop the Var marker of the f-string, so every row sentref: "ref_rf_item_rx_state_", a name the last rendered row had registered. Reproduced onmain.Fix
An id built from Vars is only known when the event runs, so it cannot name a ref chosen at compile time.
reflex_base/event/__init__.py): for an id that is aVaror an f-string holding one, the three events send the id as it evaluates, in its own payload field (id: ("rf_"+item_rx_state_)). A literal id is unchanged and still sends its registered ref name (ref: "ref_input1").utils/state.js):_set_focus,_blur_focusand_set_valueresolverefthrough therefsregistry exactly as onmain, and resolveidwithdocument.getElementByIdonly. The two never cross, so a row id that happens to evaluate toref_alpha, or to a name every object inherits such astoString, cannot be taken for a registered ref.This is a different route from the one #7838 sketches (registering each row's ref under a key derived from its evaluated id). That needs a hook whose argument reads the loop variable, which is only safe where the hook is guaranteed to compile inside the loop; it is not in an
@rx.memobody (#7836) and not for a component the per-item pass leaves unwrapped. Looking the element up by id needs no hook and works in every one of those places.rx.scroll_toalready resolves its target withdocument.getElementById.As a side effect the three events now accept a plain
Varid (rx.set_focus(State.target_id)), which raised aTypeErrorbefore.Not changed
Formcollects for such controls, so I left it alone.rx.set_focus(f"rf_{self.selected}")passes a plain string, which is formatted toref_rf_alphaas before and still finds nothing for an input whose id came from a Var. Supporting that needs the raw id in the payload for literal ids too, which changes the payload of every existing use, so I kept it out of this fix. Happy to follow up if you want it.Tests
tests/units/test_event.py: the exact event the three functions compile to for an f-string id and for a plainVarid.packages/reflex-frontend/tests/state.test.js:reffinds a registered ref by name,idfinds an element by its id (including ids spelledref_namedandtoString), and a missing target still logs the "No element found" error. Fails without thestate.jschange.tests/integration/tests_playwright/test_server_side_event.py: the existing app gets anrx.foreachwhose rows fill and focus their own input. Without the fix the first row's input stays empty.Ran locally:
tests/units(minusreflex_bench, whose browser test cannot start Chromium in my sandbox),npm run checkinpackages/reflex-frontend(lint, type-check, vitest), the Playwright modulestest_server_side_eventandtest_input, plusruff check,ruff formatandpyrighton the changed files.closes #7838
https://claude.ai/code/session_01LwJUCTCgM7D4Vj6H4dRWss