Add button to select all trees and all segments that match a search #8123
Conversation
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx
Outdated
Show resolved
Hide resolved
| ); | ||
| let selectedKeys = props.selectedTreeIds.map((treeId) => getNodeKey(GroupTypeEnum.TREE, treeId)); | ||
|
|
||
| if (props.activeGroupId) selectedKeys = [getNodeKey(GroupTypeEnum.GROUP, props.activeGroupId)]; |
There was a problem hiding this comment.
Is there a reason why you remove the selected keys here in case there is an active group?
There was a problem hiding this comment.
yes! it is because when selecting multiple elements with ctrl, you cannot select skeletons and skeleton groups at the same time. next to that, there is no multi group selection.
Its not like there is a distiction between active and selected group like for the segments, right?
There was a problem hiding this comment.
There is even this modal which says "You have ${numberOfSelectedTrees} selected Trees. Do you really want to select this group? This will deselect all selected trees."
I thought about adding this here to in this specific case, but it seems hard to realize without very much changing the current architecture:
- the logic that the selected key is a group is within tree_hierarchy_view (for the skeletons tab)
- we would need it in
AdvancedSearchPopoverthough, because if the user selects "no" within the modal, we dont want to change the selection. As there is no history to the selection, this needs to happen in the search popover module. - the components dont know each other, they are both placed in the
SkeletonView
Adding to this, the user pressed the arrow to select the next tree / treegroup, so its hopefully not surprising behaviour. So I personally dont see the need to restructure the code for this to work.
What do you think?
There was a problem hiding this comment.
Oh right, I forgot about this limitation.
There is even this modal which says "You have ${numberOfSelectedTrees} selected Trees. Do you really want to select this group? This will deselect all selected trees."
Thanks for the clarification 🙏
So you kinda fixed the code a little here, correct? It behaves "more correctly" than before. I guess the code before was not broken because when a group was selected the code took care of deselecting all trees? Beside that, in regard to your clarification your changes are much nicer than the previous version ✨
So I personally dont see the need to restructure the code for this to work.
What do you think?
Sure, fine by me. If we get the feedback that this should be included in the "search multi selection" then we could still develop this as a follow up :)
There was a problem hiding this comment.
Yes, I think I removed inconsistent behaviour regarding the multiselection via ctrl vs the selection in the search. Previously if you had selected skeletons and selected a group via the search, nothing happened to the skeleton selection. I think this wasnt too bad but it doesnt make much sense either. you cannot do any actions with your heterogenous selection anyway, such as drag and drop or actions via a context menu
There was a problem hiding this comment.
Thanks for this pr 🎉 Makes moving and organizing segments / trees likely much easier for our users 💯
I noticed a few things / had questions regarding your changes. Please find them below.
Moreover, I kinda stumbled upon the X/Y indicator of the search popover which shows 0/Y even using the new selecting all matches. To me it looks like 0/Y say that there is no active selection although there is. But I also could not come up with a good visual indication that all matches are currently selected. In case you can come up with something, I am glad so see / hear your idea :D
frontend/javascripts/oxalis/view/right-border-tabs/tree_hierarchy_view.tsx
Outdated
Show resolved
Hide resolved
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe changes in this pull request introduce several enhancements to the WEBKNOSSOS application, including new features for managing annotations and segment selections, improvements in UI terminology, and performance optimizations. Notable updates include the addition of metadata functionality, a summary row in time tracking, and a new button for selecting all matching segments. The control flow has been refined in various components to improve user interactions, and several bugs have been addressed, particularly around dataset uploads and segment management. Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 2
🧹 Outside diff range and nitpick comments (5)
frontend/javascripts/oxalis/view/right-border-tabs/advanced_search_popover.tsx (1)
206-220: Consider improving the button implementation.A few suggestions to enhance clarity and type safety:
- Make the tooltip more specific about what can be selected
- Handle the undefined onClick case more explicitly
- Remove the need for non-null assertion
Consider this implementation:
- <Tooltip title="Select all matches (except groups)"> + <Tooltip title="Select all matching trees and segments"> <ButtonComponent style={{ width: 40, }} - onClick={ - this.props.onSelectAllMatches != null - ? () => this.props.onSelectAllMatches!(availableOptionsToSelectAllMatches) - : undefined - } + onClick={() => this.props.onSelectAllMatches?.(availableOptionsToSelectAllMatches)} disabled={isSelectAllMatchesDisabled} > <CheckSquareOutlined /> </ButtonComponent> </Tooltip>frontend/javascripts/oxalis/view/right-border-tabs/skeleton_tab_view.tsx (2)
Line range hint
671-685: LGTM! Consider adding error logging.The implementation correctly handles both tree and group expansions. However, consider adding error logging when
skeletonTracingis undefined to help with debugging.maybeExpandParentGroups = (selectedElement: TreeOrTreeGroup) => { const { skeletonTracing } = this.props; if (!skeletonTracing) { + console.warn('maybeExpandParentGroups called without skeletonTracing'); return; }
696-702: Consider improving type safety for matching trees.While the implementation works, the method name suggests it handles trees, but the parameter type includes groups. Consider either:
- Filtering out non-tree elements, or
- Renaming the method to better reflect its capability to handle both trees and groups
- handleSelectAllMatchingTrees = (matchingTrees: TreeOrTreeGroup[]) => { + handleSelectAllMatchingTrees = (matchingElements: TreeOrTreeGroup[]) => { + const matchingTrees = matchingElements.filter( + (element): element is TreeOrTreeGroup & { type: typeof GroupTypeEnum.TREE } => + element.type === GroupTypeEnum.TREE + ); const treeIds = matchingTrees.map((tree) => { this.maybeExpandParentGroups(tree); return tree.id; }); this.setState({ selectedTreeIds: treeIds }); };frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (2)
1611-1629: Consider using a constant for the timeout value.The timeout value of 50ms is hardcoded. Consider extracting it to a named constant at the class/file level for better maintainability.
+ const SCROLL_DELAY_MS = 50; handleSearchSelect = (selectedElement: SegmentHierarchyNode) => { this.maybeExpandParentGroup(selectedElement); setTimeout(() => { if (this.tree.current) this.tree.current.scrollTo({ key: selectedElement.key }); - }, 50); + }, SCROLL_DELAY_MS);
1632-1646: Consider adding error handling for empty matches.The method assumes
allMatchesarray is non-empty when accessing index 0. Consider adding a guard clause.handleSelectAllMatchingSegments = (allMatches: SegmentHierarchyNode[]) => { if (this.props.visibleSegmentationLayer == null) return; + if (allMatches.length === 0) return; const allMatchingSegmentIds = allMatches.map((match) => { this.maybeExpandParentGroup(match); return match.id; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- CHANGELOG.unreleased.md (1 hunks)
- frontend/javascripts/oxalis/view/right-border-tabs/advanced_search_popover.tsx (5 hunks)
- frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (3 hunks)
- frontend/javascripts/oxalis/view/right-border-tabs/skeleton_tab_view.tsx (3 hunks)
- frontend/javascripts/oxalis/view/right-border-tabs/tree_hierarchy_view.tsx (3 hunks)
🔇 Additional comments (12)
CHANGELOG.unreleased.md (1)
19-19: LGTM! Clear and accurate changelog entry.The changelog entry accurately describes the new feature, correctly references the PR, and is placed in the appropriate section.
frontend/javascripts/oxalis/view/right-border-tabs/advanced_search_popover.tsx (4)
14-14: LGTM: Props type extension is well-defined.The new optional callback prop
onSelectAllMatchesis properly typed and aligns with the feature requirements.
116-116: LGTM: Button state logic is well-implemented.The disabled state correctly handles edge cases where selecting all matches wouldn't be meaningful (< 2 matches).
190-190: LGTM: Navigation button states are consistently handled.The disabled state is now correctly based on whether there are any results to navigate through.
Also applies to: 201-201
113-115: Verify type safety of the result type check.The filter assumes that
result.typewill be either "Tree" or "segment". Consider using TypeScript string literals to ensure type safety.✅ Verification successful
Let me try different commands to gather the necessary information.
Based on the search results, I can now generate the final response.
Type safety for result.type is properly enforced
The type checks are consistent with the codebase's type system:
- "Tree" is defined as an enum value in
tree_hierarchy_view_helpers.ts:TREE = "Tree"- "segment" type is defined in multiple interfaces across the codebase (e.g., in
segments_view_helper.tsxandsynapse_tree.tsx)- These types are consistently used throughout the codebase for type discrimination
The filter's type assumptions are valid and match the established type system in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for type definitions that define these specific string literals ast-grep --pattern 'type $_ = "Tree" | "segment"' # Search for other usages of these type strings to ensure consistency rg '"Tree"|"segment"' -t typescriptLength of output: 124
Script:
#!/bin/bash # Search for type definitions and usages with proper file extensions rg '"Tree"|"segment"' --type-add 'ts:*.{ts,tsx}' --type ts -A 2 # Look for type definitions or interfaces that might define these values ast-grep --pattern 'type $name = $_' ast-grep --pattern 'interface $name { type: $_ }' # Search for imports that might bring in these types rg 'import.*Tree.*from' --type-add 'ts:*.{ts,tsx}' --type tsLength of output: 14808
frontend/javascripts/oxalis/view/right-border-tabs/tree_hierarchy_view.tsx (3)
Line range hint
191-211: LGTM! Good improvement to the group selection handling.The function signature change simplifies the implementation while maintaining the same functionality. The confirmation modal is a good UX improvement to prevent accidental deselection of multiple trees.
299-299: LGTM! Clean update to group selection handling.The change correctly passes the group ID to the new
onSelectGroupNodesignature.
260-260:⚠️ Potential issueFix unwanted auto-scroll behavior.
The direct scroll call in the component body causes unwanted scrolling on every render. This can lead to poor UX, especially when using context menus on trees far from the selected item.
Move this into a useEffect hook to only trigger on selectedKeys changes:
-treeRef.current?.scrollTo({ key: selectedKeys[0], align: "auto" }); +useEffect(() => { + if (selectedKeys.length > 0) { + treeRef.current?.scrollTo({ key: selectedKeys[0], align: "auto" }); + } +}, [selectedKeys]);frontend/javascripts/oxalis/view/right-border-tabs/skeleton_tab_view.tsx (2)
687-695: LGTM! Clean and focused implementation.The method follows a clear sequence of operations and correctly handles both tree and group selections.
879-879: LGTM! Clean prop addition.The new prop correctly integrates the "select all matches" functionality with the search popover.
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (2)
Line range hint
1593-1609: LGTM! Clean extraction of parent group expansion logic.The extracted method
maybeExpandParentGroupis well-implemented with proper null checks and clear logic for handling both segment and group cases.
1865-1865: LGTM! Clean integration of new functionality.The
onSelectAllMatchesprop is properly connected to the new handler method.
frontend/javascripts/oxalis/view/right-border-tabs/tree_hierarchy_view.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx
Outdated
Show resolved
Hide resolved
Hm, very good point! I feel like the easiest would be to display nothing at all? and change the color of the button. I built something that felt good, let me know what you think! |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
frontend/javascripts/oxalis/view/right-border-tabs/advanced_search_popover.tsx (2)
Line range hint
13-26: Consider adding JSDoc comments for the new prop and state field.While the types are well-defined, adding documentation would improve maintainability:
type Props<S> = { data: S[]; searchKey: keyof S | ((item: S) => string); onSelect: (arg0: S) => void; + /** Callback triggered when all matching items are selected. Excludes group items. */ onSelectAllMatches?: (arg0: S[]) => void; children: React.ReactNode; provideShortcut?: boolean; targetId: string; }; type State = { isVisible: boolean; searchQuery: string; currentPosition: number | null | undefined; + /** Tracks whether all matching items are currently selected */ areAllMatchesSelected: boolean; };
187-193: Consider improving the counter display logic.The current implementation hides the counter when all matches are selected, which might be confusing to users:
addonAfter={ <div style={{ minWidth: 25 }}> - {!areAllMatchesSelected - ? `${currentPosition + 1}/${numberOfAvailableOptions}` - : ""} + {`${areAllMatchesSelected ? 'All' : currentPosition + 1}/${numberOfAvailableOptions}`} </div> }frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (1)
1613-1633: Consider adding error handling for the tree reference.The method assumes the tree.current will be available after the timeout, but this might not always be true if the component unmounts.
handleSearchSelect = (selectedElement: SegmentHierarchyNode) => { this.maybeExpandParentGroup(selectedElement); // As parent groups might still need to expand, we need to wait for this to finish. setTimeout(() => { - if (this.tree.current) this.tree.current.scrollTo({ key: selectedElement.key }); + if (this.tree?.current?.scrollTo) { + this.tree.current.scrollTo({ key: selectedElement.key }); + } }, SCROLL_DELAY_MS);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- CHANGELOG.unreleased.md (1 hunks)
- frontend/javascripts/oxalis/view/right-border-tabs/advanced_search_popover.tsx (8 hunks)
- frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (4 hunks)
- frontend/javascripts/oxalis/view/right-border-tabs/tree_hierarchy_view.tsx (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.unreleased.md
🔇 Additional comments (6)
frontend/javascripts/oxalis/view/right-border-tabs/advanced_search_popover.tsx (2)
2-2: LGTM: Import and constant additions are appropriate.The new imports and constant definition align well with the new select-all functionality.
Also applies to: 10-11
36-36: Verify state management for edge cases.The areAllMatchesSelected state is properly reset when:
- Selecting next/previous options
- Changing the search query
However, consider what happens when:
- All matches are selected
- The search query changes to yield no results
- The search query changes back to yield results again
Should areAllMatchesSelected remain false in this case?
Also applies to: 77-77, 184-184
✅ Verification successful
Based on the search results, I can now provide a conclusive response about the state management of
areAllMatchesSelected:State management for
areAllMatchesSelectedis correctly implementedThe code properly handles all edge cases for
areAllMatchesSelected:
- It's initialized as
falsein the initial state- It's reset to
falsewhen:
- Selecting next/previous options via
selectNextOptionWithOffset- Changing the search query in the input field
- Starting a new search (currentPosition becomes null)
- It's only set to
truewhen explicitly selecting all matches via the dedicated button- The UI correctly reflects the state by showing/hiding the position counter based on
areAllMatchesSelectedThe concern about search query changes yielding no results and then results again is handled correctly since the state is properly reset to
falseon any search query change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other components that might depend on areAllMatchesSelected state rg -l "areAllMatchesSelected" --type tsxLength of output: 68
Script:
#!/bin/bash # Search for areAllMatchesSelected using ripgrep without file type restriction rg "areAllMatchesSelected" -A 3 -B 3Length of output: 6458
Script:
#!/bin/bash # Find TypeScript/React files that might contain the state fd "\.(tsx?|jsx?)$" | xargs rg "areAllMatchesSelected"Length of output: 1274
Script:
#!/bin/bash # Search for state management patterns around areAllMatchesSelected ast-grep --pattern 'setState({ areAllMatchesSelected: $_ })'Length of output: 62
Script:
#!/bin/bash # Look for the component's state initialization ast-grep --pattern 'interface State { $$$ areAllMatchesSelected: $_ $$$ }'Length of output: 82
frontend/javascripts/oxalis/view/right-border-tabs/tree_hierarchy_view.tsx (1)
Line range hint
191-211: LGTM: Function signature simplificationThe change to accept
groupIddirectly instead of aTreeNodeobject simplifies the interface while maintaining the same functionality. The modal warning for multiple tree selections provides good user experience.Also applies to: 302-302
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (3)
140-141: LGTM! Good practice extracting the delay value into a constant.The extraction of the scroll delay value into a named constant improves maintainability and consistency across the codebase.
Line range hint
1595-1611: LGTM! Well-structured helper method for expanding parent groups.The method effectively handles both segment and group cases, and properly integrates with the tree expansion logic.
1869-1869: LGTM! Good integration of the new select all matches functionality.The onSelectAllMatches prop is properly connected to the new handleSelectAllMatchingSegments method.
frontend/javascripts/oxalis/view/right-border-tabs/advanced_search_popover.tsx
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/right-border-tabs/advanced_search_popover.tsx
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/right-border-tabs/tree_hierarchy_view.tsx
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx
Show resolved
Hide resolved
I think that's a good idea 🎉. Maybe this could be improved a little more by not having an empty x/y but simply replacing the text with "all" or something like that? What do you think? To me the empty space seems kinds "out of place / unused / useless"
|
| @@ -7,6 +7,8 @@ import Shortcut from "libs/shortcut_component"; | |||
| import DomVisibilityObserver from "oxalis/view/components/dom_visibility_observer"; | |||
| import { mod } from "libs/utils"; | |||
|
|
|||
| const PRIMARY_COLOR = "var(--ant-color-primary)"; | |||
There was a problem hiding this comment.
Uh nice 🎉, this made me come up with the following possible refactoring: #8150
There was a problem hiding this comment.
I LOVE THE IDEA, THANK YOU 🙌
frontend/javascripts/oxalis/view/right-border-tabs/advanced_search_popover.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/right-border-tabs/advanced_search_popover.tsx
Outdated
Show resolved
Hide resolved
I think both are fine, I'll go with yours! This is how it looks once all are selected with the count field in ant-color-text-disabled |
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/advanced_search_popover.tsx (1)
121-121: Review the disabling condition for the "Select All Matches" buttonThe condition
availableOptionsToSelectAllMatches.length < 2disables the button when there is only one match. Consider allowing the button to be enabled even if there's a single match to enhance user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- frontend/javascripts/oxalis/view/right-border-tabs/advanced_search_popover.tsx (8 hunks)
🔇 Additional comments (7)
frontend/javascripts/oxalis/view/right-border-tabs/advanced_search_popover.tsx (7)
10-10: Good use of constant for primary colorDefining
PRIMARY_COLORas a constant enhances maintainability and ensures consistent use of primary color throughout the component.
16-16: Enhancing flexibility withonSelectAllMatchespropAdding the optional
onSelectAllMatchesprop allows the component to handle bulk selection, improving its versatility.
26-26: Appropriate addition ofareAllMatchesSelectedto stateIncluding
areAllMatchesSelectedin the component's state effectively tracks whether all matches are selected.
77-77: Resetting selection state upon navigationResetting
areAllMatchesSelectedtofalsewhen navigating through options ensures accurate representation of the selection state.
184-184: Resetting selection state on search query changeProperly resetting
areAllMatchesSelectedtofalsewhen the search query changes prevents inconsistencies in selection status.
197-199: Clear display of selection statusDisplaying "all" when all matches are selected provides clear feedback to the user. Good implementation.
226-246: Well-implemented "Select All Matches" buttonThe addition of the "Select All Matches" button with conditional styling enhances functionality and improves user interaction.
|
found bug that if i select a group, the skeletons are not selected. and fixed it. |
|
@MichaelBuessemeyer do you want to rereview my lastest changes? |
URL of deployed dev instance (used for testing):
skeleton tab:
segments tab:
Steps to test:
Go to segments and skeleton tab and search for something that yields multiple results
click button "select all matches"
all matching leaves should be selected and scrolled to
clicking arrow up or down afterwards should still work
the selected segments and skeletons should behave appropriately, so e.g. dragging and dropping them and multiple-segment-actions should work as usual
try to search for a term that yields groups and leaves as a result. only multiple leaves can be selected
if only groups are the result, the button should be disabled
parent groups of results should be expanded after hitting the new "select all matches" button
a little unrelated: if a search only yields one result, you can now use the arrow buttons to select it
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements