python panel state stability enhancements #5390
Conversation
WalkthroughThis pull request introduces enhancements to state management across multiple components and files. The changes primarily focus on adding a new Changes
Sequence DiagramsequenceDiagram
participant MainSpace
participant CustomPanel
participant PanelConfig
participant Operators
MainSpace->>CustomPanel: Initialize with reset_state
CustomPanel->>PanelConfig: Configure reset behavior
PanelConfig->>Operators: Register panel with reset option
Operators-->>CustomPanel: Trigger panel load/reset
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
imanjra
force-pushed
the
operator-state-im-x1
branch
from
ddf5839 to
4471ba1
Compare
imanjra
force-pushed
the
operator-state-im-x1
branch
from
4471ba1 to
1551756
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
fiftyone/operators/panel.py (1)
36-36: Enhance reset_state parameter documentation.While the documentation is clear, it could be more descriptive about when to use this parameter and its implications.
Consider expanding the documentation:
- reset_state (False): whether to reset the state of the panel prior to on load + reset_state (False): whether to reset the panel's state before the on_load callback is triggered. + Use this when you need to ensure the panel starts with a clean state, regardless of any + previously saved state.fiftyone/operators/operations.py (1)
375-376: Enhance reset_state parameter documentation.The documentation could be more descriptive about the parameter's behavior and use cases.
Consider expanding the documentation:
- reset_state (False): whether to reset the state of the panel prior to on load + reset_state (False): whether to reset the panel's state before the on_load callback is triggered. + Use this when you need to ensure the panel starts with a clean state, regardless of any + previously saved state. This is particularly useful when the panel's behavior depends on + starting from a known state.app/packages/core/src/components/MainSpace/MainSpace.tsx (2)
25-33: Consider adding TypeScript interface for useUnboundState parameters.Adding a type definition would improve code maintainability and make the hook's requirements more explicit.
interface UnboundStateProps { spaces: SpaceTree; sessionSpaces: SpaceNodeJSON; panelsState: PanelState; sessionPanelsState: PanelState; updateSpaces: (spaces: SpaceNodeJSON) => void; setPanelsState: (state: PanelState) => void; setSessionSpaces: (spaces: SpaceNodeJSON, state: PanelState) => void; }
Line range hint
53-72: Consider simplifying update conditions.The complex update conditions could be extracted into named functions for better readability and maintainability.
const hasSpacesChanged = (current: SpaceTree, session: SpaceNodeJSON, previous: SpaceNodeJSON) => !current.equals(session) && !current.equals(previous); const hasPanelsChanged = (current: PanelState, session: PanelState, previous: PanelState) => !isEqual(session, current) && !isEqual(current, previous);app/packages/core/src/plugins/SchemaIO/components/DynamicIO.tsx (2)
86-95: Consider addressing the technical debt indicated by the TODO comment.The initialization logic could benefit from a proper refactor as indicated by the TODO comment at the top of the function.
Would you like me to help create a GitHub issue to track this technical debt and propose a refactoring plan?
82-99: Consider optimizing effect dependencies.The use of object dependencies (
unboundState,props) in effect dependencies could cause unnecessary re-renders. Consider memoizing these objects or extracting only the needed primitive values.const memoizedProps = useMemo(() => ({ data, path, root_id, otherProps }), [data, path, root_id, otherProps]); // Use memoizedProps in effect dependencies instead of props
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/packages/core/src/components/MainSpace/MainSpace.tsx(3 hunks)app/packages/core/src/plugins/SchemaIO/components/DynamicIO.tsx(2 hunks)app/packages/operators/src/CustomPanel.tsx(3 hunks)fiftyone/operators/operations.py(3 hunks)fiftyone/operators/panel.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/core/src/plugins/SchemaIO/components/DynamicIO.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/components/MainSpace/MainSpace.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/operators/src/CustomPanel.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: lint / eslint
- GitHub Check: build
🔇 Additional comments (11)
app/packages/operators/src/CustomPanel.tsx (3)
7-7: LGTM! Required imports added.The new imports are correctly placed and necessary for the panel state management enhancements.
Also applies to: 19-19
100-100: LGTM! Default value updates disabled.Setting
updatableDefaultValueto false ensures that default values remain stable during state resets.
134-134: LGTM! Reset state parameter added.The
reset_stateparameter addition aligns with the backend changes inPanelConfigandOperationsclasses.fiftyone/operators/panel.py (2)
53-53: LGTM! Reset state parameter properly initialized.The parameter is correctly added to the constructor with a sensible default value and properly assigned to the instance.
Also applies to: 71-71
90-90: LGTM! Reset state properly serialized.The
reset_statefield is correctly included in the JSON serialization.fiftyone/operators/operations.py (2)
327-327: LGTM! Reset state parameter added to method signature.The parameter addition is consistent with the changes in
PanelConfigclass.
404-404: LGTM! Reset state properly included in params.The
reset_stateparameter is correctly added to the params dictionary for the trigger call.app/packages/core/src/components/MainSpace/MainSpace.tsx (2)
Line range hint
1-22: LGTM! Type safety improvements are well implemented.The explicit typing of
SpaceTree | SpaceNodeJSONenhances type safety and code maintainability.
37-51: Verify state update order between effects.The two effects updating local state based on session state could potentially race. Consider combining them into a single effect or adding a mechanism to ensure proper ordering.
// Example combined effect: useEffect(() => { const { spaces, panelsState, updateSpaces, setPanelsState } = unboundState; // Update spaces first if (!spaces.equals(sessionSpaces)) { updateSpaces(sessionSpaces); } // Then update panels if (size(sessionPanelsState) && !isEqual(sessionPanelsState, panelsState)) { setPanelsState(sessionPanelsState); } }, [unboundState, sessionSpaces, sessionPanelsState]);app/packages/core/src/plugins/SchemaIO/components/DynamicIO.tsx (2)
Line range hint
1-74: LGTM! Import changes align with state management improvements.The explicit imports and hook additions improve code clarity.
75-75: Simplification of initialization condition is an improvement.Removing the dependency on
isEditableViewmakes the logic clearer and more maintainable.
| const [state, setState] = usePanelState(); | ||
| const [localState] = usePanelState(null, null, true); | ||
| const hasState = !isNullish(state?.state); | ||
| const isLoaded = localState?.loaded; | ||
| const isReset = useRef(false); | ||
|
|
||
| if (!isLoaded && hasState && reset_state) { | ||
| if (!isReset.current) { | ||
| setState({}); | ||
| isReset.current = true; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve state management implementation.
The current implementation has several areas for improvement:
- The state check and reset logic could race if multiple renders occur quickly
- The
localStatehook with null values could be simplified - The
isResetref could be replaced with state for better predictability
Consider this implementation:
- const [state, setState] = usePanelState();
- const [localState] = usePanelState(null, null, true);
- const hasState = !isNullish(state?.state);
- const isLoaded = localState?.loaded;
- const isReset = useRef(false);
-
- if (!isLoaded && hasState && reset_state) {
- if (!isReset.current) {
- setState({});
- isReset.current = true;
- }
- return null;
- }
+ const [state, setState] = usePanelState();
+ const [isReset, setIsReset] = useState(false);
+
+ useEffect(() => {
+ if (reset_state && !isReset && !isNullish(state?.state)) {
+ setState({});
+ setIsReset(true);
+ }
+ }, [reset_state, state?.state, isReset]);
+
+ if (reset_state && !isReset) {
+ return null;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [state, setState] = usePanelState(); | |
| const [localState] = usePanelState(null, null, true); | |
| const hasState = !isNullish(state?.state); | |
| const isLoaded = localState?.loaded; | |
| const isReset = useRef(false); | |
| if (!isLoaded && hasState && reset_state) { | |
| if (!isReset.current) { | |
| setState({}); | |
| isReset.current = true; | |
| } | |
| return null; | |
| } | |
| const [state, setState] = usePanelState(); | |
| const [isReset, setIsReset] = useState(false); | |
| useEffect(() => { | |
| if (reset_state && !isReset && !isNullish(state?.state)) { | |
| setState({}); | |
| setIsReset(true); | |
| } | |
| }, [reset_state, state?.state, isReset]); | |
| if (reset_state && !isReset) { | |
| return null; | |
| } |
| const { data, path, root_id } = props || {}; | ||
| const { data, path, root_id, otherProps = {} } = props || {}; | ||
| const { updatableDefaultValue = true } = otherProps; | ||
| const updateToDefault = updatableDefaultValue ? true : isNullish(data); | ||
| if ( |
There was a problem hiding this comment.
This logic is getting out of hand!
| @@ -324,6 +324,7 @@ def register_panel( | |||
| on_change_query_performance=None, | |||
| allow_duplicates=False, | |||
| priority=None, | |||
| reset_state=False, | |||
There was a problem hiding this comment.
I think this name is confusing.
There was a problem hiding this comment.
Agree on this, since this is supposed to be about persisting state on refresh, I think more descriptive is better here - something like persist_state_on_refresh or refresh_saving etc.
What changes are proposed in this pull request?
(Please fill in changes proposed in this fix)
How is this patch tested? If it is not, please explain why.
(Details)
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Release Notes
New Features
reset_stateoption for panels, allowing more granular control over panel state managementImprovements
Technical Updates
These changes provide more robust and configurable state management for panels and components in the application.