feat: Add function to skip serialization.#6137
feat: Add function to skip serialization.#6137abulvenz wants to merge 1 commit intoreflex-dev:mainfrom
Conversation
Merging this PR will not alter performance
Comparing |
|
Locally the test passes. |
Greptile SummaryThis PR adds a
Confidence Score: 1/5
Important Files Changed
Flowchartflowchart TD
A["get_delta() called"] --> B{"__skip_serialization()?"}
B -->|True| C["Return empty delta {}"]
B -->|False| D["_mark_dirty_computed_vars()"]
D --> E["Compute subdelta for dirty vars"]
E --> F["Recursively get substate deltas"]
F --> G["Return full delta"]
H["dict() called"] --> I{"initial?"}
I -->|Yes| J["Proceed normally"]
I -->|No| K{"__skip_serialization()?"}
K -->|True| L["Return empty dict {}"]
K -->|False| J
style C fill:#f99,stroke:#333
style L fill:#f99,stroke:#333
style B fill:#ff9,stroke:#333
style K fill:#ff9,stroke:#333
Last reviewed commit: 3bcac1c |
| def __skip_serialization(self) -> bool: | ||
| return False |
There was a problem hiding this comment.
Name mangling prevents subclass override
The double-underscore prefix (__skip_serialization) triggers Python's name mangling. Inside BaseState, the call self.__skip_serialization() is compiled as self._BaseState__skip_serialization(), meaning it will always call BaseState's version (which returns False), regardless of any override in a subclass.
Since the stated purpose is for subclasses to override this method (e.g., for permissions/roles on states), this makes the feature unusable. Use a single leading underscore instead:
| def __skip_serialization(self) -> bool: | |
| return False | |
| def _skip_serialization(self) -> bool: | |
| return False |
And update the two call sites accordingly (self._skip_serialization()).
| if self.__skip_serialization(): | ||
| return delta |
There was a problem hiding this comment.
Early return also silently skips all substate deltas
When __skip_serialization() returns True, this early return prevents the recursive substate delta collection at line 2098-2099. This means if a parent state skips serialization, all of its substates are also silently skipped, even if those substates do not skip serialization themselves.
This may be intentional, but it's a significant behavioral side-effect that should be documented. If substates should still be serialized independently, the skip should only guard the subdelta block (lines 2083-2094), not the entire method.
| def __skip_serialization(self) -> bool: | ||
| return False |
There was a problem hiding this comment.
Missing docstring on new method
This method has no docstring, which is inconsistent with the rest of BaseState where all methods have docstrings. Adding one would clarify the intended purpose and how subclasses should use it.
| def __skip_serialization(self) -> bool: | |
| return False | |
| def __skip_serialization(self) -> bool: | |
| """Whether to skip serialization for this state. | |
| Override in a subclass to skip sending state updates to the frontend. | |
| Returns: | |
| True if serialization should be skipped, False otherwise. | |
| """ | |
| return False |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
masenf
left a comment
There was a problem hiding this comment.
to bring this feature, we need some unit test cases added
This is needed for permissions and roles on states.