Skip to content

Reject browser storage defaults produced by a default_factory - #7582

Open
masenf wants to merge 7 commits into
mainfrom
claude/reject-factory-storage-defaults-1uthtm
Open

masenf wants to merge 7 commits into
mainfrom
claude/reject-factory-storage-defaults-1uthtm

Conversation

@masenf

@masenf masenf commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

Requested by Masen · project thread

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

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

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?

What

Before: a str-annotated var whose default_factory returned a LocalStorage was never detected as browser storage. It compiled as an ordinary var and each session silently held a storage instance as a plain str. A storage-annotated var with a factory was accepted, and compile_client_storage called the factory to read the storage name and options.

After: both declarations fail at compile time with a VarTypeError naming the var and showing the supported form (theme: str = rx.LocalStorage("", name='theme')). Browser storage must be declared with a storage value as the default or with a storage type annotation. The docs say so in Browser Storage and in Changing Defaults.

This rejects rather than warns: a factory-produced storage value cannot be classified when the app compiles, so there is no working fallback to warn about, and nothing in the codebase deprecates a declaration that never worked.

How

_compile_client_storage_field raises when a storage-annotated field has no default (a declared factory, or one set later with set_default(default_factory=...)), and when a field whose annotation accepts a str (str, str | None, Any, object) has a factory that returns a ClientStorageBase. That second check calls the factory once at compile for str-compatible factory fields only; factories for other types are never called. The special case that read storage options from a factory's product is removed, along with the unit and Playwright tests that exercised factory-declared storage.

Test plan

  • tests/units/compiler/test_compiler_utils.py: rejection for each storage type via annotation, via set_default(default_factory=...), and for str / str | None factories returning storage; a plain str factory and a list factory compile as ordinary vars with the str factory called once.
  • uv run pytest tests/units/compiler tests/units/test_state.py tests/units/utils/test_telemetry_accounting.py: 638 passed.
  • uv run pytest tests/integration/tests_playwright/test_hydration_storage.py: 32 passed.
  • ruff check, ruff format --check, pyright reflex tests: clean.

Closes #7498

🤖 Generated with Claude Code

https://claude.ai/code/session_014uPh1ADyPWucA2MM6zpW9T


Generated by Claude Code

View guided diff Turn on auto-fix

A browser storage var is classified at compile time from its default
value or its annotation. A default_factory only reveals what it produces
when called, so a str-annotated var whose factory returns a LocalStorage
was never compiled as storage and silently held a storage instance as an
ordinary str (#7498), while a storage-annotated var's factory was called
during compile_client_storage just to read its options.

compile_client_storage now raises VarTypeError in both cases: a storage
annotated field with a factory (declared, or set later through
set_default), and a str/Any-compatible field whose factory returns a
storage value. The factory of a str-compatible field is called once at
compile for the check; factories for other types are never called. The
special case reading options from a factory's product is removed, and
the tests that exercised factory-declared storage go with it.

Closes #7498

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014uPh1ADyPWucA2MM6zpW9T
@masenf masenf self-assigned this Oct 16, 2026
_accepts_str used issubclass(str, type_) directly, which raises for a
TypedDict annotation (and other types that refuse class checks), so
compiling an app with a TypedDict factory field failed. Use
safe_issubclass, which treats such types as not accepting a str.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014uPh1ADyPWucA2MM6zpW9T
@greptile-apps

greptile-apps Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no new actionable issue was found.

Summary

Browser storage declared through a default_factory now fails compilation with a message showing the supported declaration.

  • Browser storage factories now fail during app compilation.

Reviews (5) · Last reviewed commit: "Merge main into browser storage factory ..." · Reviewed by Greptile

Comment thread reflex/compiler/utils.py
Comment thread reflex/compiler/utils.py Outdated
The factory check only concerns frontend vars, since a backend var is
never browser storage, and a backend factory may depend on runtime-only
state such as an environment variable. A union annotation now counts as
holding a str when any member is a str or a str subclass, so a storage
type inside an Optional is checked too.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014uPh1ADyPWucA2MM6zpW9T
@codspeed

codspeed Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Merging this PR will degrade performance by 5.64%

1 warning

⚠️ 16 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

❌ 1 regressed benchmark
✅ 147 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
❌ test_state_access[read_cached_computed] 47.1 ms 49.9 ms -5.64%

Tip

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


Comparing claude/reject-factory-storage-defaults-1uthtm (20d0251) with main (a9b6613)

Open in CodSpeed

masenf commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator Author

CodSpeed Performance Analysis is red on this head, flagging test_state_access[write_own] at -4.7%. On the previous head it flagged test_state_access[read_own] instead, and both runs carry CodSpeed's "Different runtime environments detected" warning. The PR only changes compile_client_storage, a compile-time path that tests/benchmarks/test_state_access.py never calls, so the regression is measurement noise rather than this change. Nothing to port; leaving it as is.


Generated by Claude Code

@masenf masenf added bug Something isn't working documentation Improvements or additions to documentation breaking Public API facing change considered for minor release bump pre-v1.0 labels Oct 19, 2026 — with ChatGPT Codex Connector
@masenf
masenf marked this pull request as ready for review October 22, 2026 23:49
@masenf
masenf requested review from a team and Alek99 as code owners October 22, 2026 23:49
@masenf masenf added the on deck PRs lined up to review / merge next label Oct 22, 2026
Comment thread reflex/compiler/utils.py
A Literal of string values accepts a str subclass at runtime, so a
factory on such a field is checked for a storage value like a str field.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014uPh1ADyPWucA2MM6zpW9T

@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.

Review completed against the latest diff

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread docs/vars/base_vars.md Outdated
Comment thread docs/api-reference/browser_storage.md Outdated
Comment thread reflex/compiler/utils.py
Comment thread reflex/compiler/utils.py
claude and others added 2 commits October 22, 2026 23:57
An Annotated[str, ...] field is a str field, so its factory is checked
for a storage value too. The docs now say which annotations the check
covers instead of claiming it applies to every annotation.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014uPh1ADyPWucA2MM6zpW9T

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Public API facing change considered for minor release bump pre-v1.0 bug Something isn't working documentation Improvements or additions to documentation on deck PRs lined up to review / merge next

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A str-annotated field whose default_factory produces browser storage is never treated as browser storage

2 participants