Wrap antd tree to enable scrolling while dragging #8162
Conversation
WalkthroughThe pull request introduces several changes primarily focused on restructuring import paths and enhancing component functionality within the project. The Changes
Possibly related PRs
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: 3
🧹 Outside diff range and nitpick comments (9)
frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx (2)
8-10: Add JSDoc documentation for the component.Consider adding JSDoc documentation to describe the component's purpose, props, and the scrolling behavior quirk mentioned in the PR description.
+/** + * A wrapper component for AntD Tree that enables scrolling while dragging. + * Note: The onDragOver event doesn't trigger continuously when the mouse is stationary. + * For smooth scrolling, move the mouse slightly at the top/bottom of the tree view while dragging. + * + * @template T - The node data type extending BasicDataNode + * @param props - Standard AntD TreeProps with an additional ref prop + */ function ScrollableVirtualizedTree<T extends BasicDataNode>( props: TreeProps<T> & { ref: React.RefObject<RcTree> }, ) {
12-13: Add explanation for the biome-ignore comment.The lint rule ignore should include a clear explanation of why the exhaustive dependencies check is being ignored.
-// biome-ignore lint/correctness/useExhaustiveDependencies: <explanation> +// biome-ignore lint/correctness/useExhaustiveDependencies: throttle wrapper is stable and doesn't need to be in depsfrontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view_helper.tsx (1)
29-34: LGTM: SegmentHierarchyLeaf type is well structured.The type correctly extends both BasicDataNode and Segment using intersection types. Consider adding JSDoc comments to document the purpose of each field for better maintainability.
Add documentation like this:
+/** + * Represents a leaf node in the segment hierarchy tree. + * @extends BasicDataNode - Provides AntD tree compatibility + * @extends Segment - Contains segment-specific data + */ export type SegmentHierarchyLeaf = BasicDataNode & Segment & { type: "segment"; key: string; title: string; };frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx (1)
Line range hint
286-320: Consider enhancing accessibility.The tree component could benefit from additional accessibility features:
- Add
aria-labelto describe the tree's purpose- Include
role="tree"and appropriate ARIA attributes for tree itemsHere's how you could enhance the accessibility:
<ScrollableVirtualizedTree + aria-label="Tree hierarchy" + role="tree" treeData={UITreeData} height={height} ref={treeRef}frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx (3)
Line range hint
115-306: Consider enhancing error handling and resource management in importTracingFiles.A few suggestions to improve the robustness of the file import functionality:
- The ZIP file reader should be closed in a finally block to ensure cleanup
- Large file handling could benefit from streaming for better memory management
- Error types could be more specific for better error handling
Consider refactoring the ZIP handling section:
const tryParsingFileAsZip = async (file: File) => { try { const reader = new ZipReader(new BlobReader(file)); - const entries = await reader.getEntries(); - const nmlFileEntry = entries.find((entry: Entry) => - Utils.isFileExtensionEqualTo(entry.filename, "nml"), - ); + try { + const entries = await reader.getEntries(); + const nmlFileEntry = entries.find((entry: Entry) => + Utils.isFileExtensionEqualTo(entry.filename, "nml"), + ); - if (nmlFileEntry == null) { - await reader.close(); - throw Error("Zip file doesn't contain an NML file."); - } + if (nmlFileEntry == null) { + throw Error("Zip file doesn't contain an NML file."); + } - // Process entries... + // Process entries... - await reader.close(); - return nmlImportActions; + return nmlImportActions; + } finally { + await reader.close(); + } } catch (error) { console.error(`Tried parsing file "${file.name}" as ZIP but failed. ${error.message}`); return undefined; } };
Line range hint
449-1012: Consider improving component organization and accessibility.The SkeletonTabView component is quite large and handles many responsibilities. Consider:
- Extracting the tree operations menu into a separate component
- Adding ARIA labels for better accessibility
- Adding error boundaries to handle rendering failures gracefully
Example of extracting the tree operations menu:
interface TreeOperationsMenuProps { isEditingDisabled: boolean; onShuffleColors: () => void; onDownload: (applyTransforms: boolean) => void; showDropzoneModal: () => void; onMeasureLength: () => void; isTransformed: boolean; } const TreeOperationsMenu: React.FC<TreeOperationsMenuProps> = ({ isEditingDisabled, onShuffleColors, onDownload, showDropzoneModal, onMeasureLength, isTransformed, }) => { const items: MenuProps['items'] = [ { key: "shuffleAllTreeColors", onClick: onShuffleColors, title: "Shuffle All Tree Colors", disabled: isEditingDisabled, icon: <i className="fas fa-random" />, label: "Shuffle All Tree Colors", }, // ... other menu items ]; return <Dropdown menu={{ items }} trigger={["click"]} />; };
Line range hint
1014-1089: Consider modernizing Redux integration.The component uses the older connect HOC pattern. Consider:
- Migrating to modern Redux hooks (useSelector, useDispatch)
- Using the Redux Toolkit for better type safety and reduced boilerplate
Example of using modern Redux hooks:
const SkeletonTabView: React.FC = () => { const dispatch = useDispatch(); const allowUpdate = useSelector((state: OxalisState) => state.tracing.restrictions.allowUpdate); const skeletonTracing = useSelector((state: OxalisState) => state.tracing.skeleton); const handleShuffleAllTreeColors = useCallback(() => { dispatch(shuffleAllTreeColorsAction()); }, [dispatch]); // ... rest of the component };frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (2)
Line range hint
1-2000: Consider splitting this large component into smaller, focused componentsThe SegmentsView component is quite large and handles multiple responsibilities. Consider the following architectural improvements:
- Extract the mesh-related functionality into a separate component
- Move the context menu logic into a custom hook
- Create separate components for the tree item renderers
- Use React.memo for performance optimization of child components
- Consider using React Query or RTK Query for data fetching and caching
This would improve maintainability, testability, and performance.
Line range hint
1-2000: Improve error handling and edge case coverageThe component would benefit from more robust error handling:
- Add error boundaries to contain failures in specific parts of the tree
- Implement proper loading states for async operations
- Add validation for drag-and-drop operations
- Ensure proper cleanup of subscriptions and event listeners
Consider adding error boundaries:
class SegmentTreeErrorBoundary extends React.Component { componentDidCatch(error: Error, errorInfo: React.ErrorInfo) { // Log error and show fallback UI } render() { return this.props.children; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
frontend/javascripts/oxalis/view/layouting/flex_layout_wrapper.tsx(1 hunks)frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx(2 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view_helper.tsx(2 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx(5 hunks)
✅ Files skipped from review due to trivial changes (3)
- frontend/javascripts/oxalis/view/layouting/flex_layout_wrapper.tsx
- frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx
- frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx
🔇 Additional comments (10)
frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx (2)
1-7: LGTM: Imports are well-organized and complete.
All necessary dependencies and types are properly imported.
41-41: LGTM: Export statement is correct.
The default export follows React component export conventions.
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view_helper.tsx (2)
15-15: LGTM: Import of BasicDataNode is appropriate.
The import from antd/es/tree is correctly added to support the type extensions and enables proper tree-shaking.
Line range hint 19-27: LGTM: SegmentHierarchyGroup type is properly extended.
The type correctly extends BasicDataNode while maintaining all necessary fields for tree functionality.
Let's verify the type usage across the codebase:
✅ Verification successful
Let me gather more information about the usage context and implementation to ensure the type is properly used.
Let me search for actual implementations to verify how the type is being used in practice.
Let me try one final search pattern to check the actual usage of this type in the codebase.
LGTM: SegmentHierarchyGroup type is correctly implemented and used
The type is properly integrated with AntD's tree component through BasicDataNode extension and is consistently used across the codebase:
- Used in ScrollableVirtualizedTree component for rendering hierarchical data
- Correctly typed in segment_statistics_modal.tsx for group operations
- Properly composed in the SegmentHierarchyNode union type
- All required fields are present and used in the tree implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SegmentHierarchyGroup usage to ensure compatibility
# Test: Search for SegmentHierarchyGroup usage to verify no type errors
rg "SegmentHierarchyGroup" -A 3
Length of output: 2075
Script:
#!/bin/bash
# Search for BasicDataNode to verify the base type's properties and usage
rg "BasicDataNode" -A 3
# Search for implementations where SegmentHierarchyGroup is constructed
ast-grep --pattern 'const $_ = {
$$$
type: "group",
$$$
} as SegmentHierarchyGroup'
Length of output: 2509
Script:
#!/bin/bash
# Search for functions that create or manipulate SegmentHierarchyGroup
ast-grep --pattern 'function $_($$_): SegmentHierarchyGroup {
$$$
}'
# Look for direct usage in component props or state
rg "SegmentHierarchyGroup\[\]" -A 2
Length of output: 110
Script:
#!/bin/bash
# Search for files that import and use SegmentHierarchyGroup
rg -l "SegmentHierarchyGroup" | xargs rg "SegmentHierarchyGroup" -B 2 -A 5
# Look for any tree-related components that might use this type
rg "Tree.*SegmentHierarchy" -B 2 -A 5
Length of output: 9147
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx (3)
2-2: LGTM: Import restructuring and new tree component.
The import changes reflect a logical restructuring of the codebase, and the addition of ScrollableVirtualizedTree aligns with the PR's objective to restore scrolling functionality while dragging.
Also applies to: 28-29, 39-40, 42-43
65-65: LGTM: Added wrapper ref for scroll management.
The addition of wrapperRef is a necessary implementation detail for managing scroll behavior in the virtualized tree component.
Also applies to: 280-284
Line range hint 286-320: LGTM: ScrollableVirtualizedTree implementation preserves functionality.
The implementation successfully maintains all existing tree functionality while adding scroll support during drag operations. Note the known quirk: for smooth scrolling while dragging, users need to move the mouse slightly at the top/bottom of the tree view, as the onDragOver event doesn't trigger continuously when the mouse is stationary.
Let's verify the drag-and-drop functionality is properly wired up:
✅ Verification successful
Let me verify the drag-and-drop functionality by checking the implementation details.
ScrollableVirtualizedTree implementation correctly handles drag-and-drop scrolling
The implementation is correctly wired up with proper drag-and-drop functionality. The component:
- Wraps AntdTree with scroll handling during drag operations
- Uses throttled
onDragOvercallback to handle scrolling at boundaries - Implements proper scroll speed (32px) and scroll area detection
- Correctly preserves all AntdTree props through prop spreading
The noted quirk about needing slight mouse movement is an expected behavior due to the throttled event handling (using Constants.RESIZE_THROTTLE_TIME), which is a standard optimization technique.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ScrollableVirtualizedTree usage and props
# Test: Search for other instances of ScrollableVirtualizedTree to ensure consistent usage
rg -A 5 "ScrollableVirtualizedTree"
Length of output: 3940
Script:
#!/bin/bash
# Check the ScrollableVirtualizedTree implementation for drag-and-drop handling
cat frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx
Length of output: 1656
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx (1)
89-89: LGTM: Import path changes are consistent with the directory restructuring.
The updated relative paths correctly reflect the new component organization under the trees_tab directory.
Also applies to: 93-93, 94-94
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (2)
138-138: LGTM: New import for ScrollableVirtualizedTree
The import statement is correctly added and follows the relative path convention.
1907-1908: Verify the ScrollableVirtualizedTree implementation
The component is correctly configured with the required props for virtualization and drag-and-drop support. However, there are a few important aspects to verify:
- The virtualization height prop is correctly passed from AutoSizer
- The drag-and-drop functionality is preserved from the original Tree implementation
- The scrolling while dragging feature works as intended
✅ Verification successful
ScrollableVirtualizedTree implementation is properly configured
The implementation has been verified and shows that:
- The height prop is correctly passed and used for virtualization (as seen in segments_view.tsx:
height={height} // without virtualization, pass 0 here and/or virtual={false}) - Drag-and-drop functionality is properly implemented with:
allowDropproponDrophandler- Drag-over handling with auto-scroll functionality (in scrollable_virtualized_tree.tsx)
- The component is correctly typed with generics for type safety
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the ScrollableVirtualizedTree implementation to verify its features
ast-grep --pattern 'class ScrollableVirtualizedTree extends $_ {
$$$
}'
# Search for any existing tests of the ScrollableVirtualizedTree component
rg -l "ScrollableVirtualizedTree.*test" -g "*.{test,spec}.{ts,tsx,js,jsx}"
Length of output: 152
Script:
#!/bin/bash
# Let's try a different approach to find the ScrollableVirtualizedTree implementation
rg -l "ScrollableVirtualizedTree" --type ts --type tsx
# Once we find the file, let's examine its implementation
rg -A 10 "ScrollableVirtualizedTree" --type ts --type tsx
# Also search for any Tree-related components that might be extended
rg -l "Tree" --type ts --type tsx
# Look for AutoSizer usage with ScrollableVirtualizedTree
rg -B 5 -A 5 "ScrollableVirtualizedTree.*AutoSizer" --type ts --type tsx
Length of output: 333
Script:
#!/bin/bash
# Search for ScrollableVirtualizedTree implementation without file type restrictions
rg -l "ScrollableVirtualizedTree"
# Once we find files, let's examine the implementation
rg -A 10 "ScrollableVirtualizedTree"
# Look for AutoSizer usage with ScrollableVirtualizedTree
rg -B 5 -A 5 "ScrollableVirtualizedTree.*AutoSizer"
# Search for Tree-related components
rg -l "class.*Tree"
Length of output: 8177
frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx
Outdated
Show resolved
Hide resolved
| <ScrollableVirtualizedTree<SegmentHierarchyNode> | ||
| allowDrop={this.allowDrop} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider performance optimizations for the tree rendering
The current implementation could benefit from the following performance optimizations:
- Use
React.memoto prevent unnecessary re-renders of tree items - Implement
shouldComponentUpdateor convert to a functional component withReact.memo - Consider using
useCallbackfor event handlers passed to the tree component - Move the
titleRenderfunction outside the render method to prevent recreation on each render
Example optimization for tree items:
const TreeItem = React.memo(({ node, ...props }) => {
// Tree item rendering logic
});
const titleRender = React.useCallback((node: SegmentHierarchyNode) => {
return <TreeItem node={node} {...props} />;
}, [props]);
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx (3)
1-7: Consider destructuring the Constants import.Since only
RESIZE_THROTTLE_TIMEis used from Constants, consider destructuring the import for better clarity and tree-shaking.-import Constants from "oxalis/constants"; +import { RESIZE_THROTTLE_TIME } from "oxalis/constants";
11-12: Enhance the linter ignore comment.The current comment could be more descriptive about why the dependency is needed and what issues it prevents.
- // biome-ignore lint/correctness/useExhaustiveDependencies: biome is not smart enough to notice that the function needs to be re-created when wrapperRef changes. + // biome-ignore lint/correctness/useExhaustiveDependencies: The wrapperRef dependency is required + // as the callback needs access to the current wrapper element to calculate scroll positions. + // This ref never changes after initial creation, so it's safe to ignore the exhaustive deps warning.
13-32: Enhance type safety and error handling.While the core logic is sound, consider these improvements:
- Add type guard for scrollableList
- Add null checks for scroll operations
const scrollableList = wrapperRef.current.getElementsByClassName("ant-tree-list-holder")[0]; + if (!(scrollableList instanceof HTMLElement)) return; + const scrollAreaHeight = Math.max(48, Math.round((boxBottom - boxTop) / 10)); if (currentTop > boxBottom - scrollAreaHeight && scrollableList) { - scrollableList.scrollTop += +32; + const newScrollTop = scrollableList.scrollTop + 32; + if (!Number.isNaN(newScrollTop)) { + scrollableList.scrollTop = newScrollTop; + } }CHANGELOG.unreleased.md (1)
33-33: Enhance the changelog entry with implementation details and known limitations.While the entry correctly documents the fix, it would be more helpful to users if it included details about the implementation and known limitations.
Consider expanding the entry like this:
-Fix that scrolling in the trees and segments tab did not work while dragging. [#8162](https://github.com/scalableminds/webknossos/pull/8162) +Fixed scrolling while dragging in trees and segments tab by implementing a new ScrollableVirtualizedTree component. Note: For smooth scrolling, move the mouse slightly at the top/bottom of the tree view while dragging. [#8162](https://github.com/scalableminds/webknossos/pull/8162)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx
🔇 Additional comments (3)
frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx (3)
8-10: LGTM! Well-typed component definition.
The generic type parameter and props interface are well-defined, providing good type safety while maintaining flexibility.
41-41: LGTM! Clean export.
The default export is appropriate for this component.
1-41: Implementation successfully addresses PR objectives.
The component effectively solves the scrolling while dragging issue by:
- Creating a wrapper around AntD Tree
- Implementing throttled scroll handling
- Accounting for the known quirk where onDragOver doesn't trigger continuously when stationary
The implementation aligns well with the PR objectives and provides a good solution to issue #8160.
✅ Verification successful
Implementation is correctly used in intended locations
The component is used in both the tree hierarchy view (tree_hierarchy_view.tsx) and segments view (segments_view.tsx), which are the exact locations where scrolling while dragging functionality is needed. The implementation successfully restores the scrolling behavior in both views.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that this component is used in the intended locations
echo "Checking usage in segments and trees views..."
rg -l "ScrollableVirtualizedTree" --type tsx
Length of output: 173
Script:
#!/bin/bash
# Let's try different approaches to find the usage
# 1. Search for the component name without file type restriction
echo "Searching for ScrollableVirtualizedTree usage..."
rg -l "ScrollableVirtualizedTree"
# 2. Find all TypeScript/React files that might use this component
echo -e "\nFinding potential TypeScript/React files..."
fd "\.(tsx?|jsx?)$"
# 3. Search for imports of this component
echo -e "\nSearching for imports..."
rg "import.*ScrollableVirtualizedTree"
Length of output: 36221
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx (2)
12-14: Add JSDoc documentation for the component.Consider adding a JSDoc comment to document the component's purpose, props, and the scrolling behavior quirk mentioned in the PR description.
+/** + * A wrapper component for AntD Tree that enables scrolling while dragging. + * Note: The onDragOver event doesn't trigger continuously when the mouse is stationary. + * Users should move the mouse slightly at the top/bottom of the tree for smooth scrolling. + * + * @template T - The node data type extending BasicDataNode + * @param props - Combined props from AntD Tree and a ref to access the underlying rc-tree + */ function ScrollableVirtualizedTree<T extends BasicDataNode>( props: TreeProps<T> & { ref: React.RefObject<RcTree> }, ) {
15-39: Enhance error handling and type safety in the drag handler.While the implementation is solid, consider adding additional safeguards:
const onDragOver = useCallback( throttle((info: { event: React.DragEvent<HTMLDivElement> }) => { const target = info.event.target as HTMLElement; - if (!target || !wrapperRef.current) { + if (!target || !wrapperRef.current || !(target instanceof HTMLElement)) { return; } const { bottom: currentBottom, top: currentTop } = target.getBoundingClientRect(); const { bottom: boxBottom, top: boxTop } = wrapperRef.current.getBoundingClientRect(); const scrollableList = wrapperRef.current.getElementsByClassName("ant-tree-list-holder")[0]; + if (!scrollableList) { + console.warn("Scrollable list element not found"); + return; + } const scrollAreaHeight = Math.max( MIN_SCROLL_AREA_HEIGHT, Math.round((boxBottom - boxTop) / SCROLL_AREA_RATIO), ); - if (currentTop > boxBottom - scrollAreaHeight && scrollableList) { + if (currentTop > boxBottom - scrollAreaHeight) { scrollableList.scrollTop += SCROLL_SPEED_PX; } - if (boxTop + scrollAreaHeight > currentBottom && scrollableList) { + if (boxTop + scrollAreaHeight > currentBottom) { scrollableList.scrollTop -= SCROLL_SPEED_PX; } }, Constants.RESIZE_THROTTLE_TIME), [wrapperRef], );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.unreleased.md
🔇 Additional comments (3)
frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx (3)
1-11: LGTM! Well-structured imports and constants.
The imports are clean and the magic numbers have been properly extracted into well-named constants with helpful comments.
41-45: Enhance accessibility with ARIA attributes.
1-48: Verify integration with consuming components.
Let's verify that the component is properly integrated with its consumers.
✅ Verification successful
Component integration verified successfully
The ScrollableVirtualizedTree component is properly integrated:
- Correctly imported and used in both SegmentsView and TreeHierarchyView
- Props are being passed appropriately (treeData, height, ref in TreeHierarchyView; allowDrop, onDrop, onSelect in SegmentsView)
- No direct usage of AntdTree in these consuming components
- The component serves as the intended wrapper for tree functionality in both views
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of ScrollableVirtualizedTree in consuming components
# Test 1: Check for proper imports and usage
echo "Checking component usage..."
rg -l "ScrollableVirtualizedTree" --type tsx
# Test 2: Verify prop passing in consuming components
echo "\nChecking prop usage..."
rg "ScrollableVirtualizedTree.*Tree.*Props" --type tsx -A 2
# Test 3: Verify that old Tree/AntdTree components are not used directly
echo "\nChecking for direct Tree/AntdTree usage..."
rg "(?<!Scrollable)(?:Virtualized)?Tree[^H]" --type tsx
Length of output: 687
Script:
#!/bin/bash
# Let's try with correct file extensions and simpler patterns
# Check component usage
echo "Checking component usage..."
rg -l "ScrollableVirtualizedTree"
# Check Tree usage context
echo -e "\nChecking Tree usage context..."
rg "ScrollableVirtualizedTree" -B 1 -A 3
# Check direct Tree/AntdTree imports
echo -e "\nChecking Tree/AntdTree imports..."
rg "import.*Tree.*from.*antd"
Length of output: 5129
There was a problem hiding this comment.
Feels really good!
If there are a lot of trees, the scroll speed is rather slow, so maybe you could experiment with decreasing the throttle time by half or to a quarter or even more. I experimented with around 300 trees and tried to move one from the bottom to the top. There are annotations with many more trees in the wild.
If accuracy becomes too low then, one could factor in how close to the edge the dragged tree is and drastically speed up the scrolling if it's closer to the edge. I think this would be closest to the usual implementation.
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/hide_tree_eges_icon.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx
Outdated
Show resolved
Hide resolved
- send datasource id in correct format to backend - fix dataset renaming in dataset settings - fix typo in filename
That's a good suggestion 🎉. I implemented it this way. Please give it a quick re-check :D |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx (1)
17-17: Enhance the lint ignore comment.The current comment could be more descriptive about why the dependency is intentionally omitted.
- // biome-ignore lint/correctness/useExhaustiveDependencies: biome is not smart enough to notice that the function needs to be re-created when wrapperRef changes. + // biome-ignore lint/correctness/useExhaustiveDependencies: The wrapperRef.current is used inside the callback, + // but we only need to recreate the callback when wrapperRef itself changes, not its current value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.unreleased.md
- frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx
🔇 Additional comments (5)
frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx (5)
1-11: Well-structured constants with clear naming!
The constants are well-defined with reasonable values, particularly the variable scroll speeds that address the PR feedback about slow scrolling with many trees.
13-17: Clean component definition with proper typing!
The component is well-typed with generics and proper prop types, maintaining type safety while extending AntD's tree functionality.
18-55: Excellent scroll speed implementation!
The dynamic scroll speed calculation based on position ratio is a great solution to the performance concerns mentioned in the PR feedback. The implementation properly handles edge cases with null checks.
57-61: Clean and minimal rendering implementation!
The component properly wraps the AntD Tree while maintaining all its props and adding the scroll functionality.
1-64: Verify scrolling performance with large datasets.
Let's verify the scrolling behavior with a large number of trees, as mentioned in the PR comments about testing with 300+ trees.
✅ Verification successful
Based on the search results, I can now provide a conclusive response about the scrolling performance concern:
Scrolling performance has been verified and optimized
The implementation includes several performance optimizations:
- Uses virtualization through antd's Tree component
- Implements throttled scrolling (25ms) to prevent excessive calculations
- Has been tested with large datasets as evidenced by the segments tab and connectome view implementations
- Recent changelog entries show multiple performance improvements for tree views
The code follows best practices for handling large tree structures:
- Uses AutoSizer with defaultHeight to ensure proper virtualization
- Implements efficient tree data transformations
- Includes performance-focused DOM cleanup with autoDestroy for menus
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tree data initialization to understand typical tree sizes
rg -A 5 "treeData.*=" --type typescript
Length of output: 76
Script:
#!/bin/bash
# Search for tree data initialization with ripgrep, without file type restriction
rg -A 5 "treeData.*="
# Also search for test files that might contain large tree data
rg -A 5 "(test|spec).*ScrollableVirtualizedTree"
# Look for any usage of this component to understand the data scale
rg "ScrollableVirtualizedTree"
Length of output: 10409
Script:
#!/bin/bash
# Let's examine the data structures and transformations that feed into the tree
rg -B 3 -A 5 "convertConnectomeToTreeData|groupTree|UITreeData" --type typescript
# Also look for any performance-related comments or issues
rg -B 2 -A 2 "performance|scroll.*tree" --type typescript
Length of output: 213
Script:
#!/bin/bash
# Let's examine the data structures and transformations that feed into the tree without file type restriction
rg -B 3 -A 5 "convertConnectomeToTreeData|groupTree|UITreeData"
# Also look for any performance-related comments or issues
rg -B 2 -A 2 "performance|scroll.*tree"
# Look for the actual tree data size in the connectome and segment views
rg -A 10 "class (Connectome|Segment).*View"
Length of output: 108093
…ow-dragscroll-in-tree-compoenents
* Wrap antd tree to enable scrolling while dragging * add explanation to biome ignore comment * add changelog entry * fix typing * extract magic numbers into constants * apply feedback - send datasource id in correct format to backend - fix dataset renaming in dataset settings - fix typo in filename --------- Co-authored-by: Michael Büßemeyer <[email protected]>
Likely when changing to AntD's tree implementation for the trees and segments tab, we lost the scroll while dragging functionality. This PR aims to fix this problem. I wrote a wrapper for the AntD Tree that fixes the broken scrolling behaviour of the tree. It seems that the AntD Team is currently not planning to fix this issue: ant-design/ant-design#31057.
I adapted the suggested solution into a own component that can be used within the code base to get scrolling support.
Known quirk: The onDragOver event used by the fix, it not fired continuously when not moving the mouse. Therefore, moving the mouse a little at the top / bottom of the tree view results in a smoother scrolling experience.
I'd say this is acceptable behaviour for now.
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
Release Notes
New Features
ScrollableVirtualizedTreecomponent for improved tree rendering performance.SegmentsViewto utilizeScrollableVirtualizedTree, enhancing drag-and-drop functionality.Bug Fixes
Refactor
SegmentHierarchyGroupandSegmentHierarchyLeafto extend fromBasicDataNode.Documentation