feat: add app_wraps to VarData for Var-driven provider injection#6447
feat: add app_wraps to VarData for Var-driven provider injection#6447FarhanAliRaza wants to merge 12 commits into
Conversation
Lets Vars declare app-level wrapper components in their VarData so the compiler can mount providers (state context, event loop, upload, etc.) based on what's actually used, instead of relying on hardcoded chains or special-case logic. State/event-loop providers now ride along on VarData.from_state and the events-hook helper, and UploadFilesProvider is mounted when selected_files/upload_file is referenced — even without an Upload component on the page. Layout renders the assembled chain so AppWrap reduces to hooks + children.
Merging this PR will not alter performance
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes |
Greptile SummaryThis PR replaces the hardcoded All P2 findings — a missing type annotation on an internal helper and a tag-only equality semantics note — are non-blocking. Confidence Score: 4/5Safe to merge; all findings are P2 style/latent concerns with no current-code defects. Only P2 issues found: an
Important Files Changed
Sequence DiagramsequenceDiagram
participant V as Var/VarData
participant C as Component
participant CP as DefaultCollectorPlugin
participant MP as MemoizeStatefulPlugin
participant R as _resolve_app_wrap_components
participant T as app_root_template
V->>V: VarData(app_wraps=[(priority, Provider)])
C->>CP: _get_vars() / _get_hooks_internal()
CP->>CP: collect_var_app_wraps_for_component()
CP->>CP: _ingest_var_data_app_wraps() → page_app_wrap_components
MP->>CP: collect_var_app_wraps_in_subtree() (snapshot boundary)
CP->>R: page_app_wrap_components
R->>R: merge Component._get_app_wrap_components()
R->>R: fixpoint: collect app_wraps from wrapper subtrees
R->>T: ordered wrap chain (priority desc)
T->>T: Layout renders StateProvider > EventLoopProvider > ... > AppWrap(hooks + children)
Reviews (1): Last reviewed commit: "feat: add app_wraps to VarData for Var-d..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
EventLoopProvider now populates a module-level addEvents in $/utils/context, so JSX literals constructed outside the React-tree hoist path (e.g. ErrorBoundary.onError) can dispatch events without useContext(EventLoopContext) being lexically in scope. State and event-loop providers ride along on event-invocation VarData.app_wraps (and via _get_event_app_wraps) so they still mount in the app root. connectErrors stays on useContext since it drives re-renders; AppWrap is now a Fragment rendering the chain in its body.
| # ``addEvents`` no longer hoists a hook here (it's reached via the | ||
| # module-level import in ``Imports.EVENTS``), so a no-arg | ||
| # ``on_click=State.ping`` only shows up as ``event_triggers`` — without | ||
| # this check the boundary skips memoization and the callback leaks. | ||
| if component.event_triggers: | ||
| return True |
There was a problem hiding this comment.
i think it's okay to allow the callback to leak into the main page here, because it's not something that will cause a re-render since addEvents is now a statically imported function.
# Conflicts: # packages/reflex-base/src/reflex_base/compiler/templates.py # reflex/compiler/compiler.py # reflex/compiler/plugins/memoize.py # tests/units/compiler/test_memoize_plugin.py # tests/units/test_app.py # tests/units/test_event.py
Per review: a no-arg handler (on_click=State.ping) surfaces only through event_triggers and reaches addEvents via a module-level import, not a hoisted hook. The inline callback carries no reactive data and never drives a re-render, so wrapping it in a memo gains nothing. Drop the event_triggers fast-path from the reactive-data check and let such callbacks render in the page module. Handlers that reference state still surface through the per-Var scan and memoize as before.
…dren App._app_root deep-copies app-wrap components and rebinds their children to assemble the provider chain. __copy__ already drops the render-path caches for this reason, but copy.deepcopy preserved them, so a component rendered during page compilation (e.g. a State/EventLoop/UploadFiles provider injected via VarData.app_wraps) returned its stale, childless _cached_render_result after children were appended. The page content (children/Outlet) was silently dropped and the page rendered blank. Add a __deepcopy__ that mirrors __copy__: the clone is for compile-time mutation, so render caches are not carried over. Extract the shared cache attr list to _COMPILE_CACHE_ATTRS. Regression test fails before the fix.
The page collector already pulls _get_hooks_internal/_get_added_hooks for page-hook aggregation; pass those dicts into the Var app-wrap scan instead of re-fetching, so each component avoids a second (uncached) _get_added_hooks and _get_event_app_wraps per compile. Event-trigger providers now surface solely through the Var scan, dropping the event_triggers special-case in the subclass-override app-wrap path.
…port
The mixed-dispatch path (_dispatch_mixed_event_var, used by
on_click=lambda: rx.cond(..., function_var, event_spec)) still emitted the
legacy hooks={Hooks.EVENTS: None} hook — const [addEvents, connectErrors]
= useContext(EventLoopContext) — but Imports.EVENTS no longer imports
EventLoopContext. The rendered component threw "ReferenceError:
EventLoopContext is not defined", crashing the page and failing the
test_event_chain_click integration tests.
Match the other dispatch sites: reach addEvents through the module-level
import and ride the state/event-loop providers on app_wraps via
get_event_app_wraps(). Drop the now-unused Hooks import. Regression test
covers the mixed-dispatch VarData.
Construct StateProvider/EventLoopProvider once and reuse them instead of rebuilding per state Var. This is now safe because consumers deep-copy before rebinding children and the render-cache no longer survives deepcopy, so the shared markers are never mutated; it lets the page-tree deepcopy and render hash memoize them. VarData.add_state now routes through get_event_app_wraps() for the single source of truth.
masenf
left a comment
masenf
left a comment
There was a problem hiding this comment.
haven't gotten through all of the review yet. initial comments
Extract the per-(priority, tag) app-wrap merge rule into a shared insert_app_wraps primitive used by both VarData.merge and the compiler's page-wide collection, so the dedup logic lives in one place. Two different wrappers claiming the same slot now raise instead of silently keeping the first. Hash app_wraps as a frozenset so merge order no longer affects VarData identity (a + b == b + a).
| _var_data=VarData( | ||
| imports=Imports.EVENTS, | ||
| hooks={Hooks.EVENTS: None}, | ||
| app_wraps=get_event_app_wraps(), |
There was a problem hiding this comment.
if we're putting the app wraps on the event chains themselves, do we still need Component._get_event_app_wraps? they should be picked up because they're on the event vars i would think
There was a problem hiding this comment.
At walk time the EventChain is not converted to Var yet. So we can not read it. Maybe if we added some way to store on EventChain, then maybe we can delete it.
There was a problem hiding this comment.
Sounds good, let's get a backlog ticket capturing some of that context, but we don't have to prioritize it.
masenf
left a comment
masenf
left a comment
There was a problem hiding this comment.
Working well for me. I'm thinking let's cut a 0.9.4 before we bring this in, but it's ready to merge.
2 participants
Lets Vars declare app-level wrapper components in their VarData so the compiler can mount providers (state context, event loop, upload, etc.) based on what's actually used, instead of relying on hardcoded chains or special-case logic. State/event-loop providers now ride along on VarData.from_state and the events-hook helper, and UploadFilesProvider is mounted when selected_files/upload_file is referenced — even without an Upload component on the page. Layout renders the assembled chain so AppWrap reduces to hooks + children.
All Submissions:
Type of change
Please delete options that are not relevant.
New feature (non-breaking change which adds functionality)
This change requires a documentation update
New Feature Submission:
Changes To Core Features:
fixes ENG-9152