Expand parent group when new tree node is selected #8585
Conversation
📝 Walkthrough""" WalkthroughThis update introduces a new action and reducer logic to ensure that when a skeleton node is selected in the viewport, its parent group is automatically expanded in the skeleton tab, even if previously collapsed. Related changes are made to action creators and reducers, with minor improvements to group expansion logic in volumetric tracing. Additionally, a new context menu option was added to trigger this expansion manually. Changes
Assessment against linked issues
Suggested labels
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the skeleton annotation view by automatically expanding the parent group when a new tree node is selected, ensuring that the selected node is visible in the skeleton tab.
- Added a useSelector hook to monitor the active node state.
- Introduced a new effect to expand the parent group and scroll the view to the active tree node.
- Updated the changelog to reflect the fix.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx | Added active node detection with useSelector and implemented an effect to expand parent groups and scroll to the active tree node. |
| CHANGELOG.unreleased.md | Updated changelog to document the fix regarding automatic expansion and scrolling behavior. |
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx
Outdated
Show resolved
Hide resolved
| if (expandedGroups == null) return; | ||
| // props.treeGroups are used to get the newly expanded groups but can be outdated, so the union needs to be used here | ||
| setExpandedNodeKeys(Utils.unique([...expandedNodeKeys, ...Array.from(expandedGroups)])); | ||
| setTimeout(() => { |
There was a problem hiding this comment.
[nitpick] Consider defining the timeout duration as a named constant to improve code readability and maintainability.
Copilot uses AI. Check for mistakes.
![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
♻️ Duplicate comments (1)
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx (1)
271-271: Dependency array is intentionally limited for performance reasonsThe comment acknowledges the intentional exclusion of dependencies for performance reasons. This approach is reasonable given that certain props may change frequently and cause excessive re-renders, while activeNode is the primary trigger we care about.
🧹 Nitpick comments (1)
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx (1)
271-290: Implementation for expanding parent groups looks goodThis useEffect hook implements the core functionality to expand parent groups when a node is selected. The implementation correctly:
- Checks if there's an active tree
- Uses the additionallyExpandGroup helper to determine which groups need expanding
- Updates expanded groups while preserving existing expansions
- Scrolls to the active node after allowing time for DOM updates
Consider extracting the timeout value (300ms) as a named constant at the top of the file for better maintainability:
+const SCROLL_TO_NODE_DELAY_MS = 300; function TreeHierarchyView(props: Props) { // existing code... // Inside the useEffect setTimeout(() => { if (treeRef.current && props.activeTreeId) treeRef.current.scrollTo({ key: getNodeKey(GroupTypeEnum.TREE, props.activeTreeId), align: "auto", }); - }, 300); + }, SCROLL_TO_NODE_DELAY_MS);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx (3)
frontend/javascripts/oxalis/store.ts (1)
OxalisState(608-644)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view_helpers.ts (2)
additionallyExpandGroup(320-338)getNodeKey(298-300)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx (1)
setExpandedGroups(506-508)
🔇 Additional comments (2)
CHANGELOG.unreleased.md (1)
20-20: Changelog entry accurately describes the fixThe entry clearly describes the new behavior that's being implemented: automatically expanding parent groups and scrolling to skeleton nodes when they're selected.
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx (1)
17-18: New imports and Redux selector support expanded tree functionalityThe import additions for OxalisState type, additionallyExpandGroup helper function, and useSelector hook from react-redux are necessary to support the new functionality.
Also applies to: 22-23, 33-34, 74-75
|
@philippotto we were talking about the corresponding functionality for segments. their parent groups are not expanded upon registering, but upon "focus segment in list". let me know if you want me to adjust that to the skeletons! |
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx
Outdated
Show resolved
Hide resolved
I think, the way you did it in this PR is good :) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx (2)
271-289: Consider adding cleanup for the timeout and using a named constant.The effect creates a timeout but doesn't clean it up with a return function. Also, the 300ms delay value would be more maintainable as a named constant.
Add a cleanup function and define the timeout as a constant:
+ const SCROLL_TO_NODE_DELAY_MS = 300; // biome-ignore lint/correctness/useExhaustiveDependencies: The other dependencies change too often, they were omitted due to performance reasons useEffect(() => { // maybe expand group of the active tree if (props.activeTreeId == null) return; const sourceNode = props.trees[props.activeTreeId]; if (sourceNode.groupId == null) return; // tree is a direct child of the root group which is always expanded const expandedGroups = additionallyExpandGroup(props.treeGroups, sourceNode.groupId, (id) => getNodeKey(GroupTypeEnum.GROUP, id), ); if (expandedGroups == null) return; setExpandedGroups(expandedGroups); + const timeoutId = setTimeout(() => { if (treeRef.current && props.activeTreeId) treeRef.current.scrollTo({ key: getNodeKey(GroupTypeEnum.TREE, props.activeTreeId), align: "auto", }); - }, 300); + }, SCROLL_TO_NODE_DELAY_MS); + return () => clearTimeout(timeoutId); }, [activeNode]);
271-271: Improve dependency comment to explain potential risks.The current comment acknowledges ignoring dependencies but doesn't explain the specific risks or reasoning.
- // biome-ignore lint/correctness/useExhaustiveDependencies: The other dependencies change too often, they were omitted due to performance reasons + // biome-ignore lint/correctness/useExhaustiveDependencies: Dependencies like props.activeTreeId, props.trees, and props.treeGroups were intentionally omitted for performance reasons, as they change frequently. This optimization assumes that all relevant tree data changes will trigger an activeNode change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.unreleased.md
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx (2)
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view_helpers.ts (2)
additionallyExpandGroup(320-338)getNodeKey(298-300)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx (1)
setExpandedGroups(506-508)
🪛 GitHub Check: frontend-tests
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx
[failure] 17-17:
Module '"oxalis/store"' has no exported member 'OxalisState'. Did you mean to use 'import OxalisState from "oxalis/store"' instead?
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (1)
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx (1)
74-74: Good implementation of the expand-on-select feature.The new Redux selector and useEffect hook correctly implement the behavior of expanding parent groups when a node is selected. The logic to determine which groups need to be expanded and the scrolling behavior work as intended.
Also applies to: 271-289
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx
Outdated
Show resolved
Hide resolved
oh thanks for finding this! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx (1)
271-289: Consider consolidating scrolling logic and defining a constant for timeout duration.This effect properly expands parent groups when the active tree changes, implementing the core functionality requested in the PR. However, there are a few improvement opportunities:
- There's potential duplication between this scrolling logic and the one at lines 100-114
- The timeout duration (300ms) should be defined as a named constant for better maintainability
Consider refactoring to consolidate the two scrolling effects and define a constant for the timeout:
+const SCROLL_TIMEOUT_SHORT = 50; +const SCROLL_TIMEOUT_LONG = 300; // biome-ignore lint/correctness/useExhaustiveDependencies: The other dependencies change too often, they were omitted due to performance reasons useEffect(() => { // maybe expand group of the active tree if (activeTreeId == null) return; const activeTree = props.trees[activeTreeId]; if (activeTree.groupId == null) return; // tree is a direct child of the root group which is always expanded const expandedGroups = additionallyExpandGroup(props.treeGroups, activeTree.groupId, (id) => getNodeKey(GroupTypeEnum.GROUP, id), ); if (expandedGroups == null) return; setExpandedGroups(expandedGroups); setTimeout(() => { if (treeRef.current && activeTreeId) treeRef.current.scrollTo({ key: getNodeKey(GroupTypeEnum.TREE, activeTreeId), align: "auto", }); - }, 300); + }, SCROLL_TIMEOUT_LONG); }, [activeTreeId]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx (2)
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view_helpers.ts (2)
getNodeKey(298-300)additionallyExpandGroup(320-338)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx (1)
setExpandedGroups(506-508)
🪛 Biome (1.9.4)
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx
[error] 4-4: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (4)
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx (4)
74-75: LGTM! Destructuring props improves readability.Good practice to destructure
activeTreeIdfrom props to avoid repetitive prop access throughout the component.
102-114: LGTM! Updated effect to use local activeTreeId variable.The code now uses the destructured
activeTreeIdvariable, which is consistent with the other changes in this PR. The dependency array also correctly includesactiveTreeIdandprops.onSingleSelectTree.
168-172: LGTM! Updated multi-selection logic for consistency.The SHIFT+click range selection now consistently uses the local
activeTreeIdvariable.
271-271:✅ Verification successful
Verify that the dependency array simplification doesn't cause stale closure issues.
The biome-ignore comment suggests that other dependencies are omitted for performance reasons. Ensure that this doesn't lead to stale closures or other unexpected behavior, especially for
props.treesandprops.treeGroupswhich are used inside the effect.
🏁 Script executed:
#!/bin/bash # Check where props.trees and props.treeGroups are updated in the codebase echo "Searching for updates to props.trees..." rg -A 5 "setUITreeData|setTrees|updateTrees" --type typescript echo "Searching for updates to props.treeGroups..." rg -A 5 "setTreeGroups|updateTreeGroups" --type typescript # Check how often these props might change echo "Checking frequency of tree/group updates in action creators..." rg -A 2 "export function .*Tree.*Action|export function .*Group.*Action" --type typescriptLength of output: 646
I’ll broaden the search to include both
.tsand.tsxfiles and look for direct prop usage as well as update functions.
🏁 Script executed:
#!/bin/bash echo "Searching for updates to props.trees (setUITreeData|setTrees|updateTrees)..." rg -A5 'setUITreeData|setTrees|updateTrees' -g '*.{ts,tsx}' echo echo "Searching for updates to props.treeGroups (setTreeGroups|updateTreeGroups)..." rg -A5 'setTreeGroups|updateTreeGroups' -g '*.{ts,tsx}' echo echo "Finding all usages of props.trees and props.treeGroups in components..." rg -n 'props\.trees' -g '*.{ts,tsx}' rg -n 'props\.treeGroups' -g '*.{ts,tsx}' echo echo "Checking where TreeHierarchyView is rendered/passed props..." rg -C3 'TreeHierarchyView' -g '*.{ts,tsx}' echo echo "Looking for reducer keys or selectors for trees/treeGroups..." rg -A3 'trees\s*:' -g '*.ts' rg -A3 'treeGroups\s*:' -g '*.ts'Length of output: 67143
Dependencies are exhaustive for props and stable imports
All values used inside the effect—props.trees, props.sortBy, props.treeGroups—and the helper functions (createGroupToTreesMap,deepFlatFilter) are included in the dependency array or are static imports. No other local variables or callback props are captured, so there’s no risk of stale closures here.
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx
Outdated
Show resolved
Hide resolved
|
@philippotto I built a context menu entry like we discussed (https://scm.slack.com/archives/C5AKLAV0B/p1746610489924599)- let me know what you think 😊 |
frontend/javascripts/viewer/model/reducers/skeletontracing_reducer.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/viewer/view/context_menu.tsx (1)
623-631: Well-positioned menu item for focusing skeleton trees.The new context menu option "Focus Tree in Skeleton Tab" is appropriately conditioned to only appear when the active tree matches the clicked node's tree. This implements the requested functionality to expand parent groups in the skeleton annotation tree when a node is selected.
This enhancement addresses the core issue described in PR #8305 and aligns with the requirements to make navigation within nested skeleton groups easier.
Consider adding a keyboard shortcut for this action to improve accessibility, similar to other context menu operations that have shortcut indicators.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/viewer/view/context_menu.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.unreleased.md
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/javascripts/viewer/view/context_menu.tsx (1)
frontend/javascripts/viewer/model/actions/skeletontracing_actions.tsx (1)
expandCorrespondingTreeGroupAction(396-400)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (1)
frontend/javascripts/viewer/view/context_menu.tsx (1)
113-113: Appropriate action import for tree focus functionality.The import of
expandCorrespondingTreeGroupActioncorrectly introduces the new action required for the parent group expansion feature.
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)