Bugfix: clean treeselectionview type and fix a initial state check grid samples issue #5053
Conversation
WalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)app/packages/core/src/plugins/SchemaIO/components/TreeSelectionView.tsx (1)Pattern 🔇 Additional comments (2)app/packages/core/src/plugins/SchemaIO/components/TreeSelectionView.tsx (2)
A previous review comment already flagged the unused unboundState issue. Also applies to: 74-75
Critical: Fix duplicate initialization and effect dependencies The useEffect has several critical issues:
Apply this fix to resolve the issues: - useEffect(() => {
- const sampleIds = view?.data.flatMap(([parentId, children]) => {
- return children.map((childId) =>
- typeof childId === "string" ? childId : childId[0]
- );
- });
- onChange(path, sampleIds);
- // eslint-disable-next-line react-hooks/exhaustive-deps
- }, []);
+ // Move initialization to a single location in the early return
+ if (data === undefined && view?.data) {
+ const sampleIds = view.data.flatMap(([parentId, children]) => {
+ return children.map((childId) =>
+ typeof childId === "string" ? childId : childId[0]
+ );
+ });
+ onChange(path, sampleIds);
+ return null;
+ }This change:
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
|
| @@ -70,6 +71,13 @@ export default function TreeSelectionView(props: ViewPropsType) { | |||
| const [checkedState, setCheckedState] = | |||
| React.useState<CheckedState>(initialCheckedState); | |||
|
|
|||
| const unboundState = useUnboundState(checkedState); | |||
|
|
|||
| // useEffect(() => { | |||
There was a problem hiding this comment.
If you are confident this useEffect is no longer needed, you can probably just delete it @lanzhenw - it looks like it is just a console.log() anyway
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
fiftyone/operators/types.py (1)
613-618: Update the docstring to include**kwargsparameterThe docstring for the
treemethod does not document the**kwargsparameter. Including**kwargsin the docstring will provide clarity on the additional arguments that can be passed to the method.Apply this diff to update the docstring:
def tree(self, name, **kwargs): """Defines a tree property on the object. Args: name: the name of the property + **kwargs: additional keyword arguments for the tree view and property Returns: a :class:`Tree` """
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/core/src/plugins/SchemaIO/components/TreeSelectionView.tsx(3 hunks)fiftyone/operators/types.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/core/src/plugins/SchemaIO/components/TreeSelectionView.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.
🔇 Additional comments (1)
fiftyone/operators/types.py (1)
613-623:
Ensure all calls to tree method are updated to match the new signature
The tree method signature has changed from accepting *items to only **kwargs. This modification may break existing code where tree is called with positional arguments. Please verify that all usages of tree in the codebase have been updated to match the new signature.
Run the following script to identify any calls to tree that may need updating:
| const unboundState = useUnboundState(checkedState); | ||
|
|
||
| // useEffect(() => { | ||
| // console.log('unboundState useEffect', unboundState) | ||
| // // no need to call onChange here, because the change comes from python side | ||
| // }, [data, unboundState]) | ||
|
|
There was a problem hiding this comment.
Remove unused unboundState or document its purpose
The unboundState is created but never used in the component. The commented useEffect suggests it's related to Python-side synchronization, but its purpose isn't clear. This could lead to unnecessary re-renders and memory allocation.
Either:
- Remove the unused state:
- const unboundState = useUnboundState(checkedState);
-
- // useEffect(() => {
- // console.log('unboundState useEffect', unboundState)
- // // no need to call onChange here, because the change comes from python side
- // }, [data, unboundState])- Or document its purpose and implement the intended functionality.
📝 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 unboundState = useUnboundState(checkedState); | |
| // useEffect(() => { | |
| // console.log('unboundState useEffect', unboundState) | |
| // // no need to call onChange here, because the change comes from python side | |
| // }, [data, unboundState]) |
| useEffect(() => { | ||
| const sampleIds = view?.data.flatMap(([parentId, children]) => { | ||
| return children.map((childId) => | ||
| typeof childId === "string" ? childId : childId[0] | ||
| ); | ||
| }); | ||
| onChange(path, sampleIds); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
Refactor duplicate initialization logic and fix effect dependencies
The useEffect duplicates the initialization logic from the early return block and has several issues:
- Potential race condition with the early return logic
- Missing dependencies in the effect's dependency array
- Unconditional state update on mount
Consider refactoring to:
- useEffect(() => {
- const sampleIds = view?.data.flatMap(([parentId, children]) => {
- return children.map((childId) =>
- typeof childId === "string" ? childId : childId[0]
- );
- });
- onChange(path, sampleIds);
- // eslint-disable-next-line react-hooks/exhaustive-deps
- }, []);
+ useEffect(() => {
+ if (data === undefined && view?.data) {
+ const sampleIds = view.data.flatMap(([parentId, children]) => {
+ return children.map((childId) =>
+ typeof childId === "string" ? childId : childId[0]
+ );
+ });
+ onChange(path, sampleIds);
+ }
+ }, [data, view?.data, path, onChange]);Committable suggestion skipped: line range outside the PR's diff.
What changes are proposed in this pull request?
clean up and added initial state to select samples from the grid
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
New Features
TreeSelectionViewcomponent with improved checkbox functionality for selecting all items and managing tree node states.Bug Fixes
Objectclass for more flexible parameter handling in thetreemethod.Documentation
treemethod with keyword arguments for better understanding.