Fix: Use forwardRef for scrollable virtualized tree #8186
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)CHANGELOG.unreleased.md (1)
The changelog entry accurately describes the fix, provides proper context by referencing the related PR that introduced the issue, and aligns well with the PR objectives. 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx (1)
66-70: Well-typed forwardRef implementation with proper generic support.The type assertion correctly maintains generic type support while using forwardRef. Consider adding a brief JSDoc comment explaining the generic type parameter T for better documentation.
Add documentation above the component:
+/** + * A virtualized tree component that supports scrolling and drag-and-drop. + * @template T - The type of tree node data, must extend BasicDataNode + */ const ScrollableVirtualizedTree = forwardRef(ScrollableVirtualizedTreeInner) as <frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (4)
Line range hint
1-100: Consider splitting SegmentsView into smaller componentsThe component is quite large (~2000 lines) and handles many responsibilities including:
- Tree view management
- Context menu handling
- Mesh operations
- Segment selection
- State management
Consider splitting it into smaller, more focused components to improve maintainability and testability. For example:
- SegmentTreeView
- SegmentContextMenu
- MeshOperations
- SegmentDetails
1904-1910: Improve error handling for ScrollableVirtualizedTreeThe tree component is rendered without explicit error boundaries. Consider adding error handling to gracefully recover from rendering failures and provide meaningful error messages to users.
<ScrollableVirtualizedTree + ErrorBoundary={SegmentTreeErrorBoundary} allowDrop={this.allowDrop} onDrop={this.onDrop} onSelect={this.onSelectTreeItem} className="segments-tree"
Line range hint
1904-1922: Optimize performance with useMemo and useCallbackThe tree component receives several callback props that could be memoized to prevent unnecessary re-renders. Consider using
useMemofor computed values anduseCallbackfor event handlers.+ const memoizedAllowDrop = useCallback(this.allowDrop, []); + const memoizedOnDrop = useCallback(this.onDrop, [visibleSegmentationLayer]); + const memoizedOnSelect = useCallback(this.onSelectTreeItem, []); <ScrollableVirtualizedTree - allowDrop={this.allowDrop} - onDrop={this.onDrop} - onSelect={this.onSelectTreeItem} + allowDrop={memoizedAllowDrop} + onDrop={memoizedOnDrop} + onSelect={memoizedOnSelect}
Line range hint
1904-1922: Enhance type safety for tree data and callbacksThe tree component's props could benefit from stricter typing:
- The
treeDataprop should have a more specific type thanSegmentHierarchyNode[]- Callback functions should have explicit parameter types
- Consider using discriminated unions for the node types
+ type TreeNodeType = 'segment' | 'group'; + interface BaseTreeNode { + type: TreeNodeType; + id: number; + key: string; + } + interface SegmentNode extends BaseTreeNode { + type: 'segment'; + color: Vector3; + } + interface GroupNode extends BaseTreeNode { + type: 'group'; + children: TreeNode[]; + } + type TreeNode = SegmentNode | GroupNode; <ScrollableVirtualizedTree + treeData={this.state.groupTree as TreeNode[]}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx(3 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx(1 hunks)
🔇 Additional comments (4)
frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx (3)
4-4: LGTM: Import changes align with forwardRef implementation.
The addition of forwardRef import is necessary for the component restructuring.
13-16: Well-structured implementation of forwardRef with generics.
The component has been correctly restructured to handle refs while maintaining type safety. The separation of props and ref parameters, along with the explanatory comment about forwardRef's limitations with generics, demonstrates a good understanding of React's type system.
61-61: Correctly forwards ref to AntdTree component.
The ref is now properly forwarded to the underlying AntdTree component, which should resolve the console warning about function components and refs.
✅ Verification successful
Based on the code analysis, I can now generate the final response:
Ref forwarding is correctly implemented and used consistently.
The ScrollableVirtualizedTree component is properly implemented with forwardRef and is used correctly in both SegmentsView and TreeHierarchyView components. The ref is forwarded to the underlying AntdTree component and used appropriately for scrolling functionality in both use cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the component is using forwardRef consistently
# Look for any other instances where this component might be used without proper ref forwarding
# Search for usage of ScrollableVirtualizedTree with ref prop
rg -l "ScrollableVirtualizedTree.*ref="
# Search for potential ref-related warnings in test files
rg -l "ScrollableVirtualizedTree.*warning" "**/*test*"
Length of output: 148
Script:
#!/bin/bash
# Let's try a more comprehensive search strategy
# 1. Find all files containing ScrollableVirtualizedTree
rg -l "ScrollableVirtualizedTree"
# 2. Look for test files specifically
fd "test|spec" -e ts -e tsx -e js -e jsx
# 3. Check the implementation of ScrollableVirtualizedTree for forwardRef
ast-grep --pattern 'forwardRef<$_,$_>($$$)'
# 4. Look for any usage of ScrollableVirtualizedTree with ref
rg "ScrollableVirtualizedTree.*ref"
Length of output: 3963
Script:
#!/bin/bash
# Let's examine the implementation and usage more closely
# 1. Check the implementation of ScrollableVirtualizedTree
cat frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx
# 2. Check how it's used in the two files that import it
cat frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx
cat frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx
Length of output: 93602
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (1)
1907-1907:
Verify the forwardRef implementation of ScrollableVirtualizedTree
The PR objectives mention implementing forwardRef for the ScrollableVirtualizedTree component, but the usage here doesn't show explicit ref forwarding syntax. This could potentially cause the ref warning mentioned in the PR description.
✅ Verification successful
Based on the search results, I can now generate the final response since I found the implementation of ScrollableVirtualizedTree component:
The forwardRef implementation is correctly used in ScrollableVirtualizedTree
The component is properly implemented using forwardRef in frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx:
const ScrollableVirtualizedTree = forwardRef(ScrollableVirtualizedTreeInner)And its usage in segments_view.tsx is correct as the component properly handles ref forwarding internally.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the implementation of ScrollableVirtualizedTree
ast-grep --pattern 'const ScrollableVirtualizedTree = React.forwardRef'
Length of output: 73
Script:
#!/bin/bash
# Let's try to find the ScrollableVirtualizedTree component definition with a broader pattern
ast-grep --pattern 'ScrollableVirtualizedTree = $_'
# Also search for the file containing this component
fd ScrollableVirtualizedTree
# As a backup, let's use ripgrep to find any mentions of this component
rg "ScrollableVirtualizedTree" -A 5
Length of output: 84631
| <ScrollableVirtualizedTree<SegmentHierarchyNode> | ||
| <ScrollableVirtualizedTree |
There was a problem hiding this comment.
Can now be removed as the type casting is results in a "better" / "more understandable" typing to ts -> Explicitly telling the generic type is not needed anymore.
| const ScrollableVirtualizedTree = forwardRef(ScrollableVirtualizedTreeInner) as < | ||
| T extends BasicDataNode, | ||
| >( | ||
| props: TreeProps<T> & { ref?: React.Ref<RcTree> }, | ||
| ) => ReturnType<typeof ScrollableVirtualizedTreeInner>; | ||
|
|
There was a problem hiding this comment.
This way of using forwardRef was suggested by AI overlord as forwardRef does not seem to support generic typing
| @@ -56,9 +58,15 @@ function ScrollableVirtualizedTree<T extends BasicDataNode>( | |||
|
|
|||
| return ( | |||
| <div ref={wrapperRef}> | |||
| <AntdTree {...props} onDragOver={onDragOver} /> | |||
| <AntdTree {...props} onDragOver={onDragOver} ref={ref} /> | |||
There was a problem hiding this comment.
ref is now a separate arg and thus needs to be passed explicitly 🤷♂️
The newly deployed scrollable fix added a wrapper for the antd tree implementation. The wrapper is functional component which needs to be wrapped in a
forwardRefas it received a ref which should be forwarded to the antd tree component. TheforwardRefwas missing and is now added in this pr.URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor