Move more globals to RegistrationContext - #6382
Conversation
contextual management of Config, decorated pages, and memo components
Merging this PR will improve performance by 11.71%
Performance Changes
Tip Curious why this is faster? Comment Comparing Footnotes
|
Greptile SummaryThe PR moves app, config, decorated-page, and bundled-library globals into
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported config-loading race is addressed by holding the same reentrant lock across the complete
|
| Filename | Overview |
|---|---|
| packages/reflex-base/src/reflex_base/config.py | Stores cached configuration on the active context and serializes fresh config imports, dependency eviction, and complete sys.path restoration with a reentrant lock. |
| packages/reflex-base/src/reflex_base/registry.py | Extends RegistrationContext with app, config, decorated-page, and bundled-library state plus an independent fork() operation. |
| reflex/app.py | Registers each App with its active context, reloads context-local configuration, and consumes context-scoped decorated pages. |
| reflex/page.py | Replaces the app-name-keyed global decorated-page registry with registration on the active context. |
| reflex/testing.py | Uses RegistrationContext.fork() to isolate harness instances while preserving framework registrations. |
| tests/units/reflex_base/test_registry.py | Adds coverage for context-local config, app, page, and bundled-library behavior, including cross-project config dependency isolation. |
Reviews (9): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile
Addresses Greptile P2 review comment on #6382. The previous get_config guarded sys.path clear/restore with a module-level RLock; this restores the same protection around _load_config so concurrent calls (e.g. from multiple AppHarness instances or reload_config on background threads) cannot leave sys.path in a corrupt state.
|
@greptile-apps re-review |
Accessing these properties raises a ReflexRuntimeError if the value was None at the time it is accessed. This cleans up LBYL None checking wherever these value are accessed. `RegistrationContext.fork` now does NOT copy the Config, allowing both Config and App to be loaded/reloaded in a forked registration context.
Otherwise the second load causes the RegistrationContext to raise an exception, because _app is already set from the initial load.
|
@greptile-apps re-review |
Reconcile the RegistrationContext globals migration with upstream's memo promotion (rx.memo / MEMOS) and the MemoizeStatefulPlugin compiler plugin: - Keep context-based bundled_libraries, decorated_pages, _app, and _config; reimplement reset_bundled_libraries() on top of the context and adopt upstream's slimmed default bundled library list. - Drop tag_to_stateful_component, custom_components, and memo_definitions from RegistrationContext: the legacy StatefulComponent/CustomComponent machinery they cached was removed upstream. Moving the new global MEMOS registry into the context is left as follow-up work. - Point reflex_base.components.memo at the context for bundled library lookups and rewrite compiler_utils._app_style() to read the context's registered app instead of get_and_validate_app(). - Update upstream tests for the context-cached config (reload_config(), clean_registration_context) and remove tests of the deleted 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.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
Review completed against the latest diff
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
test_register_plugin_routes_rejects_stale_dynamic_arg_type_from_prior_app creates a second App, which a single context forbids — fork the context (as an app reload does) for the second instance. test_compile_registers_plugin_routes mocks _get_config, which the context-cached config masks — use clean_registration_context like the other config-mocking compile tests.
FarhanAliRaza
left a comment
There was a problem hiding this comment.
I tested this against existing apps in examples/, a live backend smoke test, the full unit suite, and custom same-process multi-project/concurrency harnesses. Standard builds and tests pass, but the four inline cases below still violate the PR's app/config isolation goal. Posting as a comment review so maintainers can triage severity and intended scope.
- _apply_decorated_pages reads the App's captured registration context instead of the ambient one, so compilation stays tied to the app's own @rx.page registrations. - get_config serializes the check/load/set sequence under _load_config_lock (double-checked) so threads sharing a context load rxconfig exactly once. - _get_config evicts project-local modules imported by rxconfig.py before the next load, so rxconfig dependencies (e.g. shared_settings.py) don't leak across projects or survive a reload with stale values. - The single-App error now recommends only .fork(), since a bare RegistrationContext() loses framework state registrations and breaks event resolution. - Use Dirs.CONTEXTS_PATH/STATE_PATH constants in the default bundled libraries; tidy news fragment wording.
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Conflicts resolved: - reflex/state.py: keep the RegistrationContext-based app lookup in FrontendEventExceptionState while adopting main's rich-markup escaping of the incoming error text. - tests/units/test_app.py: take main's renamed test and flipped assertions (deprecated App.theme wins over an explicit RadixThemesPlugin) while keeping the clean_registration_context fixture and its docstring. - tests/units/test_testing.py: keep both imports. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JqrZtsNCMmFbQzWg1AcjXE
RegistrationContext (#6382) multi-app isolation verified: fork() isolation (11 checks), AppHarness twice per process sequentially and simultaneously with Chromium E2E, bundle_library dev/prod byte-identical to 0.9.8. Found: breaking removals raise bare TypeError/AttributeError without pointing at replacements; second bare rx.App() per process now raises ReflexRuntimeError (undocumented change vs 0.9.8); reflex.page.DECORATED_PAGES removed undocumented; reflex.testing imports uvicorn/psutil the wheel does not declare. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EMjBXPozsNeQNSBZecNH8x
* Restore `dynamic.bundled_libraries` as a deprecated shim #6382 moved the module-level `bundled_libraries` list out of `reflex_base.components.dynamic` and onto `RegistrationContext`, which breaks every published reflex-enterprise (0.8.0 through 0.9.4a4 all read `dynamic.bundled_libraries` in `LiteralLambdaVar._validate_and_extend_return_expr`) when paired with reflex-base 0.9.9. Add a module-level `__getattr__` that resolves `bundled_libraries` (and `DEFAULT_BUNDLED_LIBRARIES`) against the active `RegistrationContext` and emits a deprecation warning pointing at the new API. Because it resolves on each attribute access, callers see the live list for whatever context is current rather than an import-time snapshot. reflex-enterprise reaches the module through `reflex.components.dynamic`, whose star re-export cannot see a module-level `__getattr__`, so that shim module gets a `__getattr__` delegating any missed name to `reflex_base.components.dynamic`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RU2VRJPPA3MWrjCGL15sy * Move bundled-libraries shim news fragment to the root news dir The fragment lands in the `reflex` changelog rather than reflex-base's, so name the `reflex.components.dynamic` path users import from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RU2VRJPPA3MWrjCGL15sy --------- Co-authored-by: Claude <noreply@anthropic.com>
…6985) * Restore deprecated DECORATED_PAGES and get_config(reload=True) shims 0.9.9a1 pre-release testing (FINDING-023, FINDING-008) found two 0.9.8 public names that #6382 removed outright, breaking downstream code with confusing errors: - `from reflex.page import DECORATED_PAGES` raised "ImportError: cannot import name 'DECORATED_PAGES' from 'PageNamespace' (unknown location)" (breaks the published reflex-enterprise flow demo at import). Because the page namespace class replaces the module in sys.modules, a plain module __getattr__ is never consulted, so the shim lives in a PageNamespaceMeta.__getattr__ that emits console.deprecate (0.9.9 -> 1.0) and returns a defaultdict mapping the app name to the active RegistrationContext's decorated_pages list, matching the 0.9.8 shape. - `get_config(reload=True)` raised a bare TypeError. The reload keyword is restored as a deprecated alias that warns and delegates to reload_config(). Also document two approved #6382/#6593 behavior changes in the changelog: a second bare rx.App() in one process now raises ReflexRuntimeError (use a fresh RegistrationContext/fork() for multiple apps), and supersedes-based on_load cancellation on navigation also cancels on_load handlers that are background tasks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EMjBXPozsNeQNSBZecNH8x * Hold _load_config_lock during `reload_config()` avoid racing threads caching different copies of the reloaded config Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
contextual management of App, Config, decorated pages, and memo components