Skip to content

Conversation

@benedikt-bartscher
Copy link
Contributor

@benedikt-bartscher benedikt-bartscher commented Oct 19, 2025

Now I remember what "multiple issues" meant in #5629

@codspeed-hq
Copy link

codspeed-hq bot commented Oct 19, 2025

CodSpeed Performance Report

Merging #5909 will create unknown performance changes

Comparing benedikt-bartscher:fix-rx-field-annotated-backend-vars-in-mixin-states (ee35403) with main (fb19fb4)

Summary

⚠️ No benchmarks were detected in both the base of the PR and the PR.
Please ensure that your benchmarks are correctly instrumented with CodSpeed.

Check out the benchmarks creation guide

⏩ 8 skipped1

Footnotes

  1. 8 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@benedikt-bartscher benedikt-bartscher marked this pull request as ready for review October 19, 2025 15:52
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Overview

Summary

Fixed backend variable initialization for mixin states when using rx.Field annotations by consolidating collection logic to properly unwrap Field objects.

  • Previously, backend vars from mixins with rx.Field were processed in a separate loop that used copy.deepcopy(), which incorrectly stored the Field object itself instead of calling default_value() to extract the actual default value
  • Now backend vars from all mixins are collected upfront in the same comprehension that handles the main class, ensuring Field objects are properly unwrapped via value.default_value()
  • Also fixes is_backend_base_variable() to receive mixin_cls instead of cls, which correctly handles name-mangled private attributes specific to each mixin class
  • Removes 3 lines of redundant mixin backend var processing that was causing the bug

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The fix consolidates backend var initialization logic and eliminates a clear bug where Field objects were incorrectly deep-copied instead of unwrapped. The change follows the existing pattern used for the main class and aligns with how computed vars from mixins are already handled. No edge cases or breaking changes identified.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
reflex/state.py 5/5 Fixes proper initialization of rx.Field annotated backend vars in mixin states by collecting them upfront with correct Field unwrapping instead of deep copying them later

Sequence Diagram

sequenceDiagram
    participant C as Class.__init_subclass__
    participant M as Mixin Classes
    participant BV as Backend Vars Collection
    participant F as Field Objects

    C->>M: Get _mixins()
    Note over C,M: Returns list of mixin classes
    
    C->>BV: Collect backend vars from mixins + cls
    loop For each mixin_cls in [*mixins, cls]
        BV->>M: Get __dict__.items()
        BV->>BV: Check is_backend_base_variable(name, mixin_cls)
        alt Value is Field object
            BV->>F: value.default_value()
            F-->>BV: Return unwrapped default
        else Value is not Field
            BV->>BV: Use value directly
        end
    end
    
    C->>BV: Update with annotated backend vars
    loop For each mixin_cls in [*mixins, cls]
        BV->>M: Get _get_type_hints().items()
        BV->>BV: Check if not in new_backend_vars
        BV->>C: cls._get_var_default(name, annotation)
        C-->>BV: Return default value
    end
    
    C->>C: Merge inherited + new backend vars
    Note over C: cls.backend_vars = {**inherited, **new}
Loading

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@masenf masenf merged commit 40309e1 into reflex-dev:main Oct 22, 2025
46 of 47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants