Repository navigation
fix: render hooks in declaration order instead of by HookPosition - #7586
Conversation
Hooks are kept in an ordered tuple inside VarData, and every hook's dependencies are merged before it. The INTERNAL / PRE_TRIGGER / POST_TRIGGER buckets reordered that sequence. VarData.merge also could not keep two positions, and _get_added_hooks overwrote a hook's position with None, so a hook could render before a state declaration it reads. _render_hooks now joins the hooks in the order they were declared and merged. VarData drops the position field; the position argument is deprecated and ignored. The framework's own position uses (ref, mount, var and event-loop hooks, window_event_listener, clipboard) are removed. Closes reflex-dev#7584
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
All reported issues were addressed across 13 files
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
[High impact] The PR appears safe to merge; the latest documentation matches the current hook collection code. SummaryThe PR renders hooks in declaration order and deprecates
Reviews (3) · Last reviewed commit: "docs: describe declaration-order hooks i..." · Reviewed by Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b4463216f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Merging this PR will regress 1 benchmark
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | test_state_access[write_own] |
126.1 ms | 132.3 ms | -4.69% |
| ⚡ | test_compile_all_artifacts[_repeated_stateful_page] |
467.7 ms | 446.4 ms | +4.79% |
| ⚡ | test_compile_page[_repeated_stateful_page] |
543.8 ms | 522.2 ms | +4.15% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing FarhanAliRaza:farhan/hook-declaration-order (fdce21f) with main (5ae5dcd)
Document that a hook reading state must be a Var that carries it.
masenf
left a comment
There was a problem hiding this comment.
looks good after fixing outstanding reviewbot comments
The guide told wrapper authors to set VarData.position, which is now deprecated and ignored. Document declaration order and VarData(hooks=...) for a hook that reads another hook's declaration instead, and mark the components-core fragment as a bugfix with its user-visible result. Claude-Session: https://claude.ai/code/session_01SyBSxfLKyRDRQrKJqcg3Wh
masenf
left a comment
There was a problem hiding this comment.
reflex-components-core already depends on in-tree version of reflex-base, so no bump was required.
Closes #7584.
Problem
VarDatakeeps hooks in an ordered tuple, and it merges each hook's dependencies before the hook._sort_hooksthen split the hooks into INTERNAL, PRE_TRIGGER and POST_TRIGGER buckets, which broke that order. Two parts of the code lose a hook's position:VarData.mergecannot keep two different positions._get_added_hookswritescode[sub_hook] = None, which overwrites the INTERNAL position that the component's own prop hooks gave the same hook.So a hook can render before a declaration it reads. This happens on main with one component: a prop with a derived hook, plus an
add_hooksVar that reads the same state:Fix
_render_hooksjoins the hooks in the order they were declared and merged._sort_hooksand the unusedmemoslot are removed.VarDatano longer has apositionfield.VarData(position=...)still works, but it shows a deprecation warning and has no effect.Hooks.HookPositionstays, and its docstring marks it as deprecated.window_event_listenerandclipboard.Tests
test_hooks_render_after_the_state_hook_they_read: the case above. It fails on main.test_hooks_render_after_their_dependencies: random hook graphs (25 seeds), read through props and throughadd_hooksacross a random component tree. Each hook must render after the hooks it reads. 11 of the 25 seeds fail on main.test_var_data_position_is_deprecated_and_ignored: the argument warns, and hooks with different positions now merge in order instead of raising.Validation outside the unit tests
I compiled each app on main and on this branch, ran ESLint
no-use-before-defineon every generated module, and compared each component's hook sequence:The three reorders do not change behaviour: the state declaration still comes before every reader, and a
useEffectmoves before someuseRefdeclarations. The same ESLint check finds the error in #7583's head that was reported there, so the check can detect this bug.Prod builds of playground (20 routes) and the fixture load with no page or console errors. On the fixture,
window_event_listener(resize),clipboard(paste) and form submit work.The enterprise dnd and combobox code passes
position=and now gets the deprecation warning. Its compiled output does not change.Follow-ups
useStateContextfirst.position=arguments.