Fix performance bottleneck when deleting a lot of trees at once #8176
Conversation
Also fix a bug when importing an NML with groups when only groups but no trees exist in an annotation. Also fix a bug where trying to delete a non-existing node (via the API, for example) would delete the whole active tree.
WalkthroughThe pull request introduces several enhancements and bug fixes to the WEBKNOSSOS application. Notable features include the ability to delete multiple trees simultaneously, improved error handling, and modifications to the UI for better user experience. Performance improvements are made for loading images asynchronously and handling multiple tree deletions. Additionally, the changelog has been updated to reflect these changes, and various bugs related to tree management and node handling have been addressed. Changes
Possibly related PRs
Suggested reviewers
Poem
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: 1
🧹 Outside diff range and nitpick comments (7)
CHANGELOG.unreleased.md (1)
44-44: Consider adding performance metrics to the changelog entry.The current entry could be more specific about the performance improvement achieved. Consider revising to include concrete metrics.
-Fix performance bottleneck when deleting a lot of trees at once. [#8176](https://github.com/scalableminds/webknossos/pull/8176) +Improve tree deletion performance, reducing operation time to under 1 second for 10,000+ trees. [#8176](https://github.com/scalableminds/webknossos/pull/8176)🧰 Tools
🪛 LanguageTool
[style] ~44-~44: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...ix performance bottleneck when deleting a lot of trees at once. [#8176](https://github.c...(A_LOT_OF)
frontend/javascripts/oxalis/model/actions/skeletontracing_actions.tsx (2)
347-358: Consider consolidating duplicate documentationThe documentation comment is duplicated from
deleteTreeAction. Consider creating a shared documentation section for both actions or referencing the documentation fromdeleteTreeAction.export const deleteTreesAction = (treeIds: number[], suppressActivatingNextNode: boolean = false) => - // If suppressActivatingNextNode is true, the trees will be deleted without activating - // another node (nor tree). Use this in cases where you want to avoid changing - // the active position (due to the auto-centering). One could also suppress the auto-centering - // behavior, but the semantics of changing the active node might also be confusing to the user - // (e.g., when proofreading). So, it might be clearer to not have an active node in the first - // place. + // See deleteTreeAction for documentation about suppressActivatingNextNode ({ type: "DELETE_TREES", treeIds, suppressActivatingNextNode, }) as const;
578-582: Improve code readability with better indentationThe nested chain of operations could be more readable with proper indentation.
- .getOrElse( - getTree(skeletonTracing, treeId) - .map((tree) => (tree.nodes.size() === 0 ? deleteTreeAction(tree.treeId) : noAction())) - .getOrElse(noAction()), - ); + .getOrElse( + getTree(skeletonTracing, treeId) + .map((tree) => + tree.nodes.size() === 0 + ? deleteTreeAction(tree.treeId) + : noAction() + ) + .getOrElse(noAction()) + );frontend/javascripts/oxalis/model/sagas/skeletontracing_saga.ts (1)
471-473: Consider optimizing centerActiveNode for batch operationsWhen deleting multiple trees at once,
centerActiveNodewill be triggered, which might be unnecessary if none of the deleted trees contained the active node. Consider adding a check to skip centering if the active node's tree wasn't affected.function* centerActiveNode(action: Action): Saga<void> { + // Skip centering for batch tree deletions if active node's tree wasn't affected + if (action.type === "DELETE_TREES") { + const activeNode = getActiveNode( + yield* select((state: OxalisState) => enforceSkeletonTracing(state.tracing)), + ); + if (!activeNode || !action.treeIds.includes(activeNode.treeId)) { + return; + } + } if ("suppressCentering" in action && action.suppressCentering) { return; }frontend/javascripts/oxalis/view/right-border-tabs/skeleton_tab_view.tsx (1)
1031-1034: Add type annotation for the dispatch functionConsider adding a return type annotation to make the function signature more explicit:
- onDeleteTrees(treeIds: number[]) { + onDeleteTrees(treeIds: number[]): void { dispatch(deleteTreesAction(treeIds)); },frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts (1)
891-904: LGTM! Consider enhancing the test coverage.The test case effectively verifies the batch deletion of trees. However, it could be more comprehensive.
Consider adding the following test scenarios:
- Verify the state when attempting to delete non-existent tree IDs
- Test deletion of trees with nodes, comments, and branch points
- Verify the active tree ID after batch deletion
test("SkeletonTracing should delete several trees at once", (t) => { const createTreeAction = SkeletonTracingActions.createTreeAction(); const deleteTreesAction = SkeletonTracingActions.deleteTreesAction([1, 2, 3]); - // create trees and delete them + // Create trees with nodes and metadata, then delete them const newState = ChainReducer<OxalisState, Action>(initialState) .apply(SkeletonTracingReducer, createTreeAction) .apply(SkeletonTracingReducer, createTreeAction) + .apply(SkeletonTracingReducer, createNodeAction) + .apply(SkeletonTracingReducer, createCommentAction("test")) + .apply(SkeletonTracingReducer, createBranchPointAction()) .apply(SkeletonTracingReducer, deleteTreesAction) .unpack(); t.not(newState, initialState); const newSkeletonTracing = enforceSkeletonTracing(newState.tracing); t.deepEqual(_.size(newSkeletonTracing.trees), 0); t.not(newSkeletonTracing.trees, initialSkeletonTracing.trees); + t.is(newSkeletonTracing.activeTreeId, null); + + // Verify deletion of non-existent trees doesn't throw + const invalidDeleteState = SkeletonTracingReducer( + newState, + SkeletonTracingActions.deleteTreesAction([99, 100]) + ); + t.deepEqual(invalidDeleteState, newState); });frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer.ts (1)
903-908: Refactor nested ternary operators for better readabilityThe nested ternary operator in the assignment of
treeIdsreduces code clarity and may make future maintenance more challenging. Consider refactoring this segment to improve readability.Apply this diff to refactor the code:
- const treeIds = - action.type === "DELETE_TREE" - ? getTree(skeletonTracing, action.treeId) // The treeId in a DELETE_TREE action can be undefined which will select the active tree - .map((tree) => [tree.treeId]) - .getOrElse([]) - : action.treeIds; + let treeIds = []; + if (action.type === "DELETE_TREE") { + treeIds = getTree(skeletonTracing, action.treeId) + .map((tree) => [tree.treeId]) + .getOrElse([]); + } else { + treeIds = action.treeIds; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/model/actions/skeletontracing_actions.tsx(7 hunks)frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer.ts(1 hunks)frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts(2 hunks)frontend/javascripts/oxalis/model/sagas/skeletontracing_saga.ts(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/skeleton_tab_view.tsx(4 hunks)frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[style] ~44-~44: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...ix performance bottleneck when deleting a lot of trees at once. [#8176](https://github.c...
(A_LOT_OF)
🔇 Additional comments (12)
CHANGELOG.unreleased.md (1)
45-46: LGTM! Clear and specific bug fix descriptions.
The bug fix entries are well-written, following changelog conventions and clearly describing the issues resolved.
frontend/javascripts/oxalis/model/actions/skeletontracing_actions.tsx (4)
42-42: LGTM: Type definitions are well-structured
The new DeleteTreesAction type is properly integrated into both BatchableUpdateTreeAction and SkeletonTracingAction union types, enabling batch operations which aligns with the PR's performance optimization goals.
Also applies to: 69-73, 101-101
148-148: LGTM: Action type properly added to save-relevant actions
The DELETE_TREES action type is correctly added to SkeletonTracingSaveRelevantActions, ensuring that batch tree deletions are properly tracked for saving.
586-586: LGTM: Type safety improvement and clean UI component updates
The changes improve type safety by making treeId a required parameter in confirmDeletingInitialNode, and the UI component updates are clean and consistent.
Also applies to: 600-600, 602-602, 605-608
347-358: Verify performance improvement with large-scale deletion
While the implementation for batch tree deletion looks correct, it's important to verify the actual performance improvement.
Consider adding performance benchmarks to verify that the batch deletion meets the sub-second performance target for 10,000+ trees mentioned in the PR objectives.
✅ Verification successful
Let me search for more context about the performance improvements.
Based on the search results, I can now generate the final response since I found clear evidence about the performance improvement in the changelog.
Performance improvement for batch tree deletion is verified
The implementation has been verified through the changelog entry which confirms: "Fix performance bottleneck when deleting a lot of trees at once." (#8176). The code changes implement batch deletion capability through deleteTreesAction and optimize the deletion process by:
- Handling multiple tree deletions in a single action
- Avoiding unnecessary node/edge deletion actions when deleting entire trees
- Supporting suppression of node activation to prevent unnecessary UI updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for existing performance tests or benchmarks
rg -g '*.test.*' -g '*.spec.*' "deleteTree" -A 5
Length of output: 13614
Script:
#!/bin/bash
# Look for the PR description or related files
rg -g '!*.test.*' -g '!*.spec.*' "deleteTree" -B 2 -A 2
# Look for any performance related comments or documentation
rg -i "performance|benchmark" -g "*.md" -g "*.txt"
Length of output: 31951
frontend/javascripts/oxalis/model/sagas/skeletontracing_saga.ts (2)
471-471: LGTM: Addition of DELETE_TREES action
The addition of "DELETE_TREES" action to trigger centerActiveNode is appropriate to maintain consistent behavior when deleting multiple trees.
471-473: Verify the impact of batch tree deletions
Let's verify if there are any other places in the codebase that need to handle the "DELETE_TREES" action consistently.
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information about the DELETE_TREES action handling:
DELETE_TREES action is properly handled across the codebase
The action is consistently implemented across all relevant parts of the codebase:
- Defined in
actions/skeletontracing_actions.tsx - Handled in
reducers/skeletontracing_reducer.tsalongside DELETE_TREE with proper logic - Properly watched in
sagas/skeletontracing_saga.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other saga watchers or reducers that might need to handle DELETE_TREES action
# Test: Look for patterns similar to the existing action handling
rg -A 5 '"DELETE_TREE"'
# Test: Look for any existing DELETE_TREES handlers
rg '"DELETE_TREES"'
# Test: Look for tree deletion related patterns in saga files
fd -e ts -e js --exec ast-grep --pattern 'function* $_(action: $_) {
$$$
"DELETE_TREE"
$$$
}'
Length of output: 3429
frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts (2)
564-567: LGTM! Improved group handling logic.
The additional condition for checking existing tree groups ensures proper ID reassignment, fixing the bug with importing NML files containing groups when no trees are present.
639-654: LGTM! Significant performance improvement for bulk tree deletion.
Excellent optimization:
- Changed to accept array of treeIds for batch processing
- Uses lodash's
_.omitfor efficient bulk removal - Smart selection of next active tree based on maximum deleted tree ID
This change directly addresses the performance bottleneck when deleting multiple trees, reducing the deletion time for 10,000+ trees from minutes to under a second.
frontend/javascripts/oxalis/view/right-border-tabs/skeleton_tab_view.tsx (2)
463-466: LGTM: Efficient batch update implementation
The concatenation of actions into a single batch operation is a good approach for handling multiple tree deletions efficiently. This change directly addresses the performance bottleneck mentioned in the PR objectives.
514-514: LGTM: Proper implementation of bulk tree deletion
The implementation correctly handles the deletion of multiple selected trees while maintaining important safeguards like initial node deletion confirmation.
frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer.ts (1)
900-909: New DELETE_TREES action enhances batch deletion capability
The addition of the DELETE_TREES action type effectively allows for batch deletion of trees, aligning with the PR objective of improving performance when deleting a large number of trees. The implementation correctly updates the reducer to handle multiple tree deletions efficiently.
There was a problem hiding this comment.
Thanks a lot for all these awesome fixes 🎉
Import an NML with groups. Delete all but one group and import the NML again. On master, the newly imported group gets the same ID as the one that was not deleted before, leading to all sorts of issues. If only one of the groups gets the blueish selected background, the fix works.
Nice ;fix works 🎉
One unrelated thing I noticed is that under linux (ubuntu + chrome) the tree context menu does not close after clicking on delete or shuffle tree color. I would have expected for the context menu to close at least after the delete action was clicked, because the tree for with the context menu was opened does not exist anymore. Thus, the context menu points to a deleted tree and imo should be closed directly when hitting the deletec menu item.
Do you think you can also take a look at this in this pr or do you want to do this as a follow-up?
Moreover:
Also fix a bug where trying to delete a non-existing node (via the API, for example) would delete the whole active tree (at least that's what it looked like to me when reading the code).
While testing this with
bug_test.nml.txt
(and having tree with id 2 (the blue one) active), I get the following error in the console:
Here is my mini test API script:
window.webknossos.apiReady(3).then(api => {
const activeTreeId = api.tracing.getActiveTreeId();
api.tracing.deleteNode(100, activeTreeId);
});Could you maybe give this a little try to avoid the UnhandledRejection: "Error: Get empty"?
At least, I hope that me trying to reproduce what you meant with was correct
Also fix a bug where trying to delete a non-existing node (via the API, for example) would delete the whole active tree (at least that's what it looked like to me when reading the code).
frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts
Show resolved
Hide resolved
|
Oh an the rest of the deletion fixes worked perfectly. Thanks a lot 🔥 🏎️ |
…hrow an error in getNodeAndTree if a node with nodeId does not exist.
|
Thanks for the review @MichaelBuessemeyer!
Fixed 👍
Thank you for testing this! Looks like the "bug" I fixed couldn't actually be triggered through the api. Still, I think the code is more explicit and correct now, so I'd leave it as-is. I adapted the code to avoid the thrown error in case of a non-existing node id. This is in line with the other accessor functions and in the spirit of the used Maybe pattern I'd argue. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
CHANGELOG.unreleased.md (1)
50-52: Consider refining the changelog entries for better clarity.The changelog entries effectively communicate the fixes. However, consider these minor refinements to improve clarity and precision:
-Fix performance bottleneck when deleting a lot of trees at once. [#8176](https://github.com/scalableminds/webknossos/pull/8176) +Fix performance bottleneck when deleting multiple trees simultaneously, reducing deletion time for 10,000+ trees. [#8176](https://github.com/scalableminds/webknossos/pull/8176)The other two entries are clear and well-documented.
🧰 Tools
🪛 LanguageTool
[style] ~50-~50: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...ix performance bottleneck when deleting a lot of trees at once. [#8176](https://github.c...(A_LOT_OF)
frontend/javascripts/oxalis/model/accessors/skeletontracing_accessor.ts (2)
Line range hint
63-65: Consider optimizing tree lookup operations for better performance.Given the PR's focus on improving performance for bulk tree operations, consider optimizing these utility functions:
findTreeByNodeId: Current implementation iterates through all trees using_.values().find(). For better performance with large tree maps, consider maintaining a nodeId-to-treeId index.getTreesWithType: Uses_.pickBy()which creates a new object. For bulk operations, consider caching or using iterators to avoid object creation.Example optimization for
findTreeByNodeId:+// Add this to the SkeletonTracing type +interface SkeletonTracing { + nodeToTreeIndex: Map<number, number>; // Cache mapping nodeIds to treeIds +} export function findTreeByNodeId(trees: TreeMap, nodeId: number): Tree | undefined { - return _.values(trees).find((tree) => tree.nodes.has(nodeId)); + // Use the index if available, fallback to current implementation + return this.nodeToTreeIndex?.get(nodeId) + ? trees[this.nodeToTreeIndex.get(nodeId)!] + : _.values(trees).find((tree) => tree.nodes.has(nodeId)); }Also applies to: 91-94
Line range hint
196-220: Consider consistent error handling approach.The codebase uses both Maybe monad and null checks for error handling. For better type safety and consistency, consider:
- Using Maybe consistently instead of converting to null values
- Adding JSDoc comments to clarify the error handling strategy
Example refactor for
getNodeAndTreeOrNull:-export function getNodeAndTreeOrNull( +/** + * @deprecated Use getNodeAndTree().toNullable() if null values are needed + */ +export function getNodeAndTreeOrNull( skeletonTracing: SkeletonTracing, nodeId?: number | null | undefined, treeId?: number | null | undefined, ): { tree: Tree | null; node: Node | null; } { return getNodeAndTree(skeletonTracing, nodeId, treeId) .map( ([maybeTree, maybeNode]): { tree: Tree | null; node: Node | null; } => ({ tree: maybeTree, node: maybeNode, }), ) .getOrElse({ tree: null, node: null, }); }frontend/javascripts/libs/diffable_map.ts (1)
57-57: LGTM! Consider adding a test case.The improved error message is more descriptive and helpful for debugging. It's a good practice to include the specific key that wasn't found.
Consider adding a test case to verify the error message format:
it("should throw descriptive error for missing key", () => { const map = new DiffableMap<number, string>(); const key = 42; expect(() => map.getOrThrow(key)).toThrow(`Key '${key}' does not exist in diffable map.`); });frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts (1)
637-655: Excellent performance optimization for bulk tree deletionThe refactored
deleteTreesfunction efficiently handles bulk deletion through:
- Early return for empty array
- Using
_.omitfor efficient bulk removal- Smart active tree selection based on the maximum deleted tree ID
This change directly addresses the PR's performance bottleneck when deleting multiple trees.
Consider adding a performance test to verify the improvement from "multiple minutes" to "under one second" for 10,000+ trees deletion.
frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts (1)
891-904: Consider adding edge case tests for the new deleteTreesAction.To ensure robust error handling, consider adding test cases for:
- Deleting non-existent tree IDs
- Deleting an empty array of trees
- Deleting a mix of existing and non-existing tree IDs
test("SkeletonTracing should handle non-existent trees in deleteTreesAction", (t) => { const deleteTreesAction = SkeletonTracingActions.deleteTreesAction([999, 888]); const newState = SkeletonTracingReducer(initialState, deleteTreesAction); t.is(newState, initialState); }); test("SkeletonTracing should handle empty array in deleteTreesAction", (t) => { const deleteTreesAction = SkeletonTracingActions.deleteTreesAction([]); const newState = SkeletonTracingReducer(initialState, deleteTreesAction); t.is(newState, initialState); }); test("SkeletonTracing should handle mix of existing and non-existing trees", (t) => { const createTreeAction = SkeletonTracingActions.createTreeAction(); const deleteTreesAction = SkeletonTracingActions.deleteTreesAction([1, 999]); const newState = ChainReducer<OxalisState, Action>(initialState) .apply(SkeletonTracingReducer, createTreeAction) .apply(SkeletonTracingReducer, deleteTreesAction) .unpack(); const newSkeletonTracing = enforceSkeletonTracing(newState.tracing); t.deepEqual(_.size(newSkeletonTracing.trees), 0); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/libs/diffable_map.ts(1 hunks)frontend/javascripts/oxalis/model/accessors/skeletontracing_accessor.ts(1 hunks)frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer.ts(2 hunks)frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts(2 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx(4 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx(1 hunks)frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[style] ~50-~50: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...ix performance bottleneck when deleting a lot of trees at once. [#8176](https://github.c...
(A_LOT_OF)
🔇 Additional comments (10)
frontend/javascripts/oxalis/model/accessors/skeletontracing_accessor.ts (1)
178-178: Approve change to handle non-existent nodes gracefully.
The switch from getOrThrow to getNullable is a good improvement that:
- Fixes the bug where deleting a non-existing node would unintentionally delete the entire active tree
- Potentially improves performance by avoiding exception handling overhead during bulk tree deletions
Let's verify the usage of this function in tree deletion operations:
✅ Verification successful
Verified that all deletion-related operations handle non-existent nodes correctly.
All usages of getNodeAndTree in deletion contexts appropriately handle null results, ensuring no unintended side effects from the change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of getNodeAndTree in deletion-related code
# to ensure the change doesn't have unintended side effects
# Search for files containing both getNodeAndTree and delete/remove operations
rg -l 'getNodeAndTree.*delete|delete.*getNodeAndTree|getNodeAndTree.*remove|remove.*getNodeAndTree'
# Search for the actual function calls to understand the context
rg -A 5 'getNodeAndTree\('
Length of output: 12325
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx (2)
143-143: UI improvement: Hide context menu after tree deletion
Good addition to improve the user experience by hiding the context menu immediately after deletion.
141-144: Verify the impact of tree deletion on selected trees
The code deselects all trees before deletion. Let's verify if this is still necessary with the new multi-tree deletion functionality, as it might affect the UX when users want to delete multiple selected trees.
frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts (1)
564-567: LGTM: Enhanced validation for tree groups
The additional check for existing tree groups in the needsReassignedIds condition ensures proper ID reassignment when tree groups are present.
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx (3)
462-465: LGTM: Efficient batching of tree deletion actions
The implementation correctly batches multiple actions together, which helps improve performance when deleting multiple trees at once. This aligns well with the PR's objective of optimizing bulk tree deletions.
513-513: LGTM: Proper handling of bulk tree deletions
The implementation correctly handles multiple tree deletions in a single operation while maintaining proper validation and state cleanup. This is a key improvement for the performance bottleneck mentioned in the PR objectives.
1035-1038: LGTM: Clean implementation of bulk deletion dispatch
The Redux action mapping is implemented cleanly and consistently with proper typing. This completes the bulk deletion feature by providing a single dispatch for multiple tree deletions.
frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer.ts (2)
900-901: LGTM! Efficient handling of both single and multiple tree deletions.
The unified case handling for both DELETE_TREE and DELETE_TREES actions reduces code duplication and aligns with the PR's objective of improving tree deletion performance.
903-908: Verify the handling of undefined treeId in DELETE_TREE action.
The code handles the case where treeId is undefined in a DELETE_TREE action by falling back to the active tree. This is a potential edge case that needs verification.
Run the following script to verify the usage of undefined treeId:
frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts (1)
891-904: LGTM! Well-structured test case for the new multi-tree deletion feature.
The test case effectively verifies the functionality of deleting multiple trees simultaneously, which aligns with the PR's objective of improving performance for bulk tree deletions.
| Store.dispatch(deleteTreeAction(tree.treeId)); | ||
| hideContextMenu(); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider updating to use the new multi-tree deletion action
According to the PR objectives, this PR introduces performance improvements for deleting multiple trees at once. However, this code still uses the single-tree deleteTreeAction. Consider updating to use the new deleteTreesAction for consistency.
- Store.dispatch(deleteTreeAction(tree.treeId));
+ Store.dispatch(deleteTreesAction([tree.treeId]));📝 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.
| Store.dispatch(deleteTreeAction(tree.treeId)); | |
| hideContextMenu(); | |
| }, | |
| Store.dispatch(deleteTreesAction([tree.treeId])); | |
| hideContextMenu(); | |
| }, |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.unreleased.md (1)
19-19: Consider using more precise language.The phrase "a lot of" is informal. Consider rewording to be more specific and professional.
-Fix performance bottleneck when deleting a lot of trees at once. [#8176](https://github.com/scalableminds/webknossos/pull/8176) +Fix performance bottleneck when deleting multiple trees simultaneously. [#8176](https://github.com/scalableminds/webknossos/pull/8176)🧰 Tools
🪛 LanguageTool
[style] ~19-~19: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...ix performance bottleneck when deleting a lot of trees at once. [#8176](https://github.c...(A_LOT_OF)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
CHANGELOG.unreleased.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[style] ~19-~19: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...ix performance bottleneck when deleting a lot of trees at once. [#8176](https://github.c...
(A_LOT_OF)
🔇 Additional comments (1)
CHANGELOG.unreleased.md (1)
19-21: LGTM! Changelog entries accurately reflect the PR changes.
The changelog entries properly document all three improvements mentioned in the PR objectives:
- Performance optimization for tree deletion
- NML import bug fix
- Node deletion API bug fix
🧰 Tools
🪛 LanguageTool
[style] ~19-~19: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...ix performance bottleneck when deleting a lot of trees at once. [#8176](https://github.c...
(A_LOT_OF)
Also fix a bug when importing an NML with groups when only groups but no trees exist in an annotation.
Also fix a bug where trying to delete a non-existing node (via the API, for example) would delete the whole active tree (at least that's what it looked like to me when reading the code).
URL of deployed dev instance (used for testing):
Steps to test:
See this gif for deleting 50,000 trees at once:
Issues:
(Please delete unneeded items, merge only when none are left open)
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Performance Improvements
These updates significantly enhance user experience and system performance.