fix(event): handle mixed rx.cond event/function returns in event lambdas#6354
Conversation
Greptile SummaryThis PR fixes
Confidence Score: 5/5Safe to merge; the fix is well-scoped and all previously-raised review concerns have been addressed. The union-origin guard prevents over-broad matching, the JS dispatcher is structurally correct (confirmed by unit test assertions on rendered output), the leading-comma fix in format_args_function_operation is a mechanical improvement with direct test coverage, and no existing behaviour is altered for the non-union code paths. The integration test in tests/integration/test_event_chain.py contains a minor poll condition that may resolve on intermediate state mid-typing, but this does not affect the correctness of the feature itself. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["call_event_fn(fn, arg_spec)"] --> B["Invoke fn with parsed args"]
B --> C["Normalize out to list"]
C --> D["For each e in out"]
D --> E{isinstance EventHandler?}
E -- yes --> F["call_event_handler"]
F --> G
E -- no --> G{isinstance EventChain?}
G -- yes --> H["Var.create(e)"]
H --> I
G -- no --> I{"Var, not EventVar/FunctionVar\nUnion origin\ntypehint_issubclass EventSpec|Callable?"}
I -- yes --> J["_dispatch_mixed_event_var(e)"]
J --> K
I -- no --> K{"EventSpec, FunctionVar, or EventVar?"}
K -- yes --> L["events.append(e)"]
K -- no --> M["raise EventHandlerValueError"]
subgraph "_dispatch_mixed_event_var emits"
N["const __event_or_fn = cond_expr"]
O{"typeof === 'function'?"}
N --> O
O -- true --> P["__event_or_fn(...args)"]
O -- false --> Q["addEvents([__event_or_fn], args, {})"]
end
J -.->|generates| N
Reviews (2): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| f'if (typeof {event_like_name} === "function") {{' | ||
| f"return {event_like_name}(...args);" | ||
| "}" | ||
| f"return {CompileVars.ADD_EVENTS}([{event_like_name}], args, ({{ }}));" |
There was a problem hiding this comment.
event_actions not propagated to conditional backend events
_dispatch_mixed_event_var hard-codes ({ }) (an empty object) as the event_actions argument to addEvents. If this dispatcher ends up in an EventChain carrying chain-level event_actions (e.g. preventDefault: True), those actions will be silently dropped for any conditional branch that resolves to a backend event. The existing EventVar→invocation.call(...) path threads those actions correctly, but the new FunctionVar wrapper bypasses that path. This is consistent with pre-existing FunctionVar behavior (not a regression), but worth noting for future callers.
There was a problem hiding this comment.
bit out of the scope of this PR imo
| f'if (typeof {event_like_name} === "function") {{' | ||
| f"return {event_like_name}(...args);" | ||
| "}" | ||
| f"return {CompileVars.ADD_EVENTS}([{event_like_name}], args, ({{ }}));" |
There was a problem hiding this comment.
({{ }}) produces ({ }) — a parenthesised empty object with two interior spaces — in the emitted JavaScript. This is valid but inconsistent with idiomatic {}. Consider {{}} instead.
| f"return {CompileVars.ADD_EVENTS}([{event_like_name}], args, ({{ }}));" | |
| f"return {CompileVars.ADD_EVENTS}([{event_like_name}], args, ({{}}));" |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
@masenf plz review when you get a chance 🙏🙏🙏 |
masenf
left a comment
masenf
left a comment
There was a problem hiding this comment.
since this behavior is very dependent on how the generated code actually works, we need to see an integration test (or extend an existing integration test) that exercises this behavior end to end.
| return Var( | ||
| _js_expr=( | ||
| "(...args) => {" | ||
| f"const {event_like_name} = {event_like_var!s};" | ||
| f'if (typeof {event_like_name} === "function") {{' | ||
| f"return {event_like_name}(...args);" | ||
| "}" | ||
| f"return {CompileVars.ADD_EVENTS}([{event_like_name}], args, ({{}}));" | ||
| "}" | ||
| ), | ||
| _var_type=Callable, | ||
| _var_data=VarData.merge( | ||
| event_like_var._get_all_var_data(), | ||
| VarData( | ||
| imports=Imports.EVENTS, | ||
| hooks={Hooks.EVENTS: None}, | ||
| ), | ||
| ), | ||
| ).to(FunctionVar) |
There was a problem hiding this comment.
prefer to use ArgsFunctionOperations.create helper with rest and explicit_return arguments.
using a structured Var is helpful because it allows the compiler to optimize the form down the road without changing user-facing code and it tends to be less brittle w.r.t composition, carry var data, etc.
Build the runtime dispatcher with ArgsFunctionOperation.create + a ternary_operation Var instead of a hand-rolled JS string so the form composes naturally and var data propagates through the helper. Extends the EventChain integration test app with a mixed-cond button that exercises both branches end-to-end: the FunctionVar branch writes a DOM data-attribute via a frontend-only function, and the EventSpec branch dispatches a backend handler.
When args_names is empty and rest is set, the formatter produced "(, ...args) => ..." — invalid JS. Build the parameter list as a list joined with ", " instead so an empty positional list does not introduce a stray separator.
|
@greptile-apps re-review |
|
oh shoot sorry @masenf for being a little late on the fixes! Lemme go ahead and update my other branches to clear up some of the backlog |
2 participants
All Submissions:
github.com/reflex-dev/reflex/blob/main/CONTRIBUTING.md) file?
Requests for the desired
changed?
Type of change
New Feature Submission:
Changes To Core Features:
like us to include them?
Description
This fixes event-lambda handling for
rx.cond(...)expressions that returnmixed event-like values (frontend function branch vs backend event branch).
Before this change,
call_event_fnrejected this pattern because thelambda return was a
Var(CustomVarOperation) rather than one of:EventSpecEventHandlerEventVarFunctionVarThat made valid mixed conditional handlers fail during event-chain
creation.
handles issue #6204