Skip to content

Fix type validation for browser storage annotations - #7603

Merged
masenf merged 2 commits into
reflex-dev:mainfrom
sansynx:fix/storage-annotation-hydration
Oct 23, 2026
Merged

masenf merged 2 commits into
reflex-dev:mainfrom
sansynx:fix/storage-annotation-hydration

Conversation

@sansynx

@sansynx sansynx commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Changes To Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Changes

Hydration supplies plain strings for browser-storage vars, but fields annotated with Cookie, LocalStorage, or SessionStorage currently log type errors for those values. Accept str when initializing these fields, including storage-containing unions, while preserving their annotations, defaults, persistence options, and checks for other invalid values. Assignment requires no additional runtime checks or storage-wrapper allocations.

Add unit coverage for all three storage types, Field wrappers, optional and union annotations, empty strings, subsequent writes, dirty tracking, unchanged storage options, and invalid integer values. Extend the existing browser-storage app to exercise hydration, event writes, persistence, and reloads.

Closes #7499.

Validation

Tested on Windows with Python 3.12.10. The 465-test run covers 9aaed22c318cbfb1d71f94b4fc9a839f5fddbe9d; the final head, f9b47d3a6fb65ac535b4d2629328af3402c0b207, only strengthens the browser test's error-log assertion. Production source and unit tests are byte-identical between these revisions. The final browser check and repeated applicable hooks cover that follow-up.

  • Before the corresponding fixes, 12 direct/wrapped storage cases and four optional/union cases failed with the reported type errors; the two plain-string controls passed.
  • uv run --python 3.12 --locked pytest tests/units/test_state.py tests/units/reflex_base/event/processor/test_base_state_processor.py tests/units/compiler/test_compiler_utils.py -q: 465 passed. The run emitted Pydantic deprecation and collection warnings.
  • The final-head audit reran uv run --python 3.12 --locked pytest tests/units/test_state.py -k storage_annotations_accept_browser_strings -q --tb=short: 18 passed, with the existing TestMixin collection warning. All applicable hooks on the four changed files passed again.
  • UV_PYTHON=3.12 uv run --python 3.12 --locked pre-commit run --files reflex/state.py tests/units/test_state.py tests/integration/tests_playwright/test_hydration_storage.py news/+storage-annotation-hydration.bugfix.md: all applicable hooks passed, including Ruff, codespell, stub generation, Pyright, and ty. The environment-variable prefix shown here uses POSIX shell syntax.
  • An earlier dedicated browser regression run passed in development and production (2 passed), before extending the fix to optional/union annotations.
  • After the review feedback, the browser test captures reflex_base.vars.base records directly and asserts that no errors occurred during hydration, event writes, and reloads. uv run --python 3.12 --locked pytest 'tests/integration/tests_playwright/test_hydration_storage.py::test_storage_annotations_hydrate_and_persist[AppHarness-chromium]' -q -o faulthandler_timeout=120: 1 passed on the final test. All applicable pre-commit hooks passed again for the changed browser test.
  • The development/production dedicated run after the optional/union fix, before the log-assertion follow-up: uv run --python 3.12 --locked pytest tests/integration/tests_playwright/test_hydration_storage.py -k storage_annotations_hydrate_and_persist -q -o faulthandler_timeout=120 reported 1 passed, 1 setup error. The development browser regression passed; production setup failed during React Router prerendering with Prerender: Request failed for /: The operation was aborted due to timeout. The process was interrupted after its error report. The diagnostic flag dumps stacks; it does not raise a test or prerender timeout. Final production behavior remains unverified.
  • A comparison run restored only reflex/state.py to upstream 75a08e179390bd88826b4ed0bb3912d60facf7cc while keeping the same regression app and environment. The production-only node failed during setup with the same React Router prerender timeout (1 setup error, 409 seconds). The source was restored to the PR head afterward. This comparison does not validate production behavior; it shows the timeout also occurs with the original affected implementation.
  • The expanded browser-suite run reported a failure and then stalled; it was interrupted. A fail-fast diagnostic rerun passed the previously failing case and the new regression, then stalled during the transition to the production fixture. Its stack dump shows AppHarness.stop() waiting in backend_thread.join() while the Uvicorn event loop remains running. The initial failure's cause is unresolved. The full browser suite and full unit suite are not claimed as passing.
  • A final production-only diagnostic with Node.js 24.19.0, UTF-8 console output, and no competing builds also failed during the same React Router prerender step (1 setup error, 144.12 seconds). The process was interrupted after its error report because the setup failure left the backend thread running. No production timeout was raised, and production browser behavior remains unverified.
  • GitHub Actions workflows for this head report action_required, with no jobs run yet; remote CI is unverified.

An independent automated review identified the optional/union gap, which was reproduced and fixed; the follow-up review found no remaining code issues. A separate source review of the final patch found no actionable correctness or security finding; it did not execute a full repository security audit. Production browser and expanded-suite validation remain incomplete as described above.

Implemented with assistance from OpenAI Codex. No human review or manual testing is claimed.

@greptile-apps

greptile-apps Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The reviewed changes appear safe to merge, with no outstanding findings.

Summary

Accepts browser strings for fields annotated with Cookie, LocalStorage, or SessionStorage, including storage-containing unions.

  • Storage-annotated state fields accept browser strings.

Reviews (2) · Last reviewed commit: "Assert browser storage hydration emits n..." · Reviewed by Greptile

Comment thread tests/integration/tests_playwright/test_hydration_storage.py
@sansynx
sansynx marked this pull request as ready for review October 19, 2026 16:40
@sansynx
sansynx requested a review from a team as a code owner October 19, 2026 16:40

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 4 files

View guided diff | Re-trigger cubic

@masenf masenf added the bug Something isn't working label Oct 19, 2026 — with ChatGPT Codex Connector
@masenf masenf added the on deck PRs lined up to review / merge next label Oct 22, 2026
@codspeed

codspeed Bot commented Oct 23, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 4.08%

2 warnings

⚠️ 15 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

⚠️ Base and head ran on different runner types

Some benchmarks with significant performance changes were compared across different runner types, so the changes might not come from the code. Run base and head on the same runner type, or use CodSpeed Macro Runners for more stable results.

Learn more about Macro Runners

Open the report in CodSpeed to investigate

❌ 2 regressed benchmarks
✅ 146 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
❌ test_proxy_list_mutation 9.6 ms 10 ms -4.11%
❌ test_route_argument_extraction 115.2 µs 120 µs -4.04%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing sansynx:fix/storage-annotation-hydration (f9b47d3) with main (75a08e1)

Open in CodSpeed

@masenf
masenf merged commit 814eb35 into reflex-dev:main Oct 23, 2026
146 of 147 checks passed
benedikt-bartscher added a commit to benedikt-bartscher/reflex that referenced this pull request Oct 25, 2026
Upstream's browser-storage type fix (reflex-dev#7603) widened the field's plain types
where _init_var also bound the field's Var. Here _bind_var does the binding,
so the widening stays in _init_var, which runs once per field, and the Var
binding is left to _bind_var.
benedikt-bartscher added a commit to benedikt-bartscher/reflex that referenced this pull request Oct 25, 2026
Upstream's browser-storage type fix (reflex-dev#7603) widened the field's plain types
where _init_var also bound the field's Var. Here _bind_var does the binding,
so the widening stays in _init_var, which runs once per field, and the Var
binding is left to _bind_var.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working on deck PRs lined up to review / merge next

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hydrating a var annotated with a browser storage type logs a type error from Field.__set__

2 participants