Explicit state and event id minification#6100
Conversation
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Merging this PR will improve performance by 13.48%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
benedikt-bartscher
changed the title
| has_substates = len(substates) > 0 | ||
|
|
||
| if handlers: | ||
| console.log(f"{child_prefix}|-- Event Handlers:") |
There was a problem hiding this comment.
Very tiny optical nit, cause sometimes EventHandlers is the last sub item.
It might work like this, didn't test it.
| console.log(f"{child_prefix}|-- Event Handlers:") | |
| console.log(f"{child_prefix}{'|' if has_substates else '`'}-- Event Handlers:") |
|
TODO: best-effort-mode
a similar thing can be implemented for event handler id's edit: not that easy, as state names are baked into to many things (events, vars etc). we would need to defer init_subclass and possibly parts of @rx.var and @rx.event edit2: i have found a better approach with a minify.json which makes this obsolete |
|
Open for discussion: Should reflex reserve the first 5/10 state id's for internal states? Current reflex uses 0-3, but that might change in the future. edit: with sibling uniqueness we would just need to reserve the state id's on the first rx.State subclass level. edit2: i have found a better approach with a minify.json which makes this obsolete |
|
TODO: think about sibling uniqueness for states. currently using global uniqueness edit: adjusted in 6b4c5fa |
35ad1c6 to
1647c1e
Compare
|
Idea: store ids in a minify.json file
|
When a parent and child state have the same minified name, substate resolution can fail because the leading segment is stripped incorrectly. This change adds a flag to skip stripping only on the initial recursive call, ensuring correct resolution even in name collision scenarios.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…stay reserved per sibling group
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af4219fac8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if output_json: | ||
| import json | ||
|
|
||
| console.log(json.dumps(tree_data, indent=2)) |
There was a problem hiding this comment.
When users run reflex minify list --json, this still goes through console.log, which uses Rich log output and honors the current log level, so stdout can be prefixed or suppressed instead of being parseable JSON. minify lookup --json uses click.echo; this branch should do the same so scripts can reliably consume it.
Useful? React with 👍 / 👎.
| errors: list[str] = [] | ||
| warnings: list[str] = [] | ||
|
|
||
| all_states = collect_all_states(root_state) |
There was a problem hiding this comment.
When minify.json intentionally contains only user-owned entries, this unfiltered state list makes reflex minify validate report missing framework states and handlers even though MinifyNameResolver never minifies framework modules and runtime falls back to their original names. Filter the same framework-owned states here before computing missing state/event entries so valid user-only configs do not fail validation.
Useful? React with 👍 / 👎.
| for i, part in enumerate(parts): | ||
| # Find the state whose minified id matches ``part``: the root state | ||
| # for the first segment, otherwise a child of the previous match. | ||
| candidates = [current] if i == 0 else current.get_substates() | ||
| found = next( | ||
| (c for c in candidates if path_to_id.get(get_state_full_path(c)) == part), | ||
| None, | ||
| ) | ||
| if found is None: | ||
| console.error( | ||
| f"No state found for minified segment '{part}' in path '{minified_path}'" | ||
| ) | ||
| raise SystemExit(1) | ||
|
|
There was a problem hiding this comment.
minify_lookup always fails for any input
For i == 0, candidates = [current] where current = State (the framework root). path_to_id.get(get_state_full_path(State)) is always None because State is in _FRAMEWORK_STATE_MODULES and is never given a minified ID. The comparison None == part is False for any non-empty input, so found is always None and every lookup immediately exits with an error.
For the PR's documented reflex minify lookup "a.bU" example, "a" is the ID of a direct child of State (a user root state). The first-segment candidates should be State.get_substates(), not [State] itself — matching the same pattern used for subsequent segments.
7 participants
Summary
Add state and event name minification with CLI management tools.
minify.jsonconfig file to map state/event names to short IDs (e.g.,myapp.state.AppState→"a")reflex minifyCLI command group for managing minificationCLI Commands
based on #6098