Refactor TreeMap to DiffableMap #8626
Conversation
…isibilityVolumeAction
…boxes if selective visibility is active
…ts in context menu of data viewports
…ng is active; fix screenshot tests
the opacity is a bit different now)
![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 comments (1)
frontend/javascripts/viewer/model/types/tree_types.ts (1)
96-126: 🛠️ Refactor suggestionRemove commented-out code
There's a large block of commented-out type definitions at the end of the file that duplicates the actual definitions. This should be removed to avoid confusion.
-// export type MutableCommentType = { -// content: string; -// nodeId: number; -// }; -// export type CommentType = Readonly<MutableCommentType>; -// export type MutableEdge = { -// source: number; -// target: number; -// }; -// export type Edge = Readonly<MutableEdge>; -// export type MutableNode = { -// id: number; -// untransformedPosition: Vector3; -// additionalCoordinates: AdditionalCoordinate[] | null; -// rotation: Vector3; -// bitDepth: number; -// viewport: number; -// mag: number; -// radius: number; -// timestamp: number; -// interpolation: boolean; -// }; -// export type Node = Readonly<MutableNode>; -// export type MutableBranchPoint = { -// timestamp: number; -// nodeId: number; -// }; -// export type BranchPoint = Readonly<MutableBranchPoint>; -// export type MutableNodeMap = DiffableMap<number, MutableNode>; -// export type NodeMap = DiffableMap<number, Node>;
🧹 Nitpick comments (7)
frontend/javascripts/viewer/model/helpers/iterator_utils.ts (4)
1-3: Module comment could be more preciseThe comment describes "IteratorsObjects" but this term isn't standard. Consider clarifying whether you mean the objects returned by iterator methods (like
Map.prototype.values()) or a custom interface you're implementing.-// This module contains helpers methods for working with IteratorsObjects -// i.e. Map.keys(), Set.values(), etc. +// This module contains helper methods for working with Iterator objects +// returned by collection methods like Map.keys(), Set.values(), etc.
47-58: Optimize selector function creation outside the reduction loopThe
valueSelectorfunction is being recreated for each item in themaxByreduction. Move it outside the iterator reduction for better performance.export function maxBy<T extends { [key in K]: unknown }, K extends string>( iterator: IteratorObject<T>, selector: ((value: T) => number) | K, ): T | undefined { const first = iterator.next(); if (first.done) return undefined; const valueSelector = typeof selector === "string" ? (value: T) => value[selector] : selector; return iterator.reduce((result: T, entry: T) => { - const entryValue = valueSelector(entry); - const resultValue = valueSelector(result); + const entryValue = valueSelector(entry); + const resultValue = valueSelector(result); if (entryValue > resultValue) { return entry; } return result; }, first.value); }
74-85: Optimize selector function creation outside the reduction loop (in minBy too)Same issue as in
maxBy- thevalueSelectorfunction is being recreated for each iteration in theminByreduction.export function minBy<T extends { [key in K]: unknown }, K extends string>( iterator: IteratorObject<T>, selector: ((value: T) => number) | K, ): T | undefined { const first = iterator.next(); if (first.done) return undefined; const valueSelector = typeof selector === "string" ? (value: T) => value[selector] : selector; return iterator.reduce((result: T, entry: T) => { - const entryValue = valueSelector(entry); - const resultValue = valueSelector(result); + const entryValue = valueSelector(entry); + const resultValue = valueSelector(result); if (entryValue < resultValue) { return entry; } return result; }, first.value); }
29-31: Consider handling non-numeric values in sum functionThe sum function assumes all values in the iterator are numbers, but doesn't validate this. Consider adding type guards or error handling for non-numeric values.
export function sum<T extends number>(iterator: IteratorObject<T>): number { - return iterator.reduce((sum, value) => sum + value, 0); + return iterator.reduce((sum, value) => { + if (typeof value !== 'number') { + console.warn('Non-numeric value found in sum operation'); + return sum; + } + return sum + value; + }, 0); }frontend/javascripts/viewer/model/types/tree_types.ts (3)
43-44: Add comprehensive JSDoc for MutableTree typeThis is a central type in the tree system that deserves detailed documentation. Adding JSDoc would help developers understand its purpose and usage.
-// When changing MutableTree, remember to also update Tree +/** + * Represents a mutable tree structure containing nodes, edges, and metadata. + * + * @remarks + * When changing MutableTree, remember to also update the corresponding readonly Tree type. + * This type supports operations that modify the tree structure. + */ export type MutableTree = {
60-61: Add comprehensive JSDoc for readonly Tree typeLike MutableTree, this type would benefit from detailed documentation describing its purpose and immutability guarantees.
-// When changing Tree, remember to also update MutableTree +/** + * Represents an immutable (readonly) tree structure containing nodes, edges, and metadata. + * + * @remarks + * When changing Tree, remember to also update the corresponding MutableTree type. + * This type enforces immutability for all tree properties. + */ export type Tree = {
93-94: Add JSDoc documentation for TreeMap classesThe
MutableTreeMapandTreeMapclasses are central to this refactoring effort but lack documentation explaining their purpose and usage.+/** + * A mutable map collection for trees, supporting efficient diffing and updates. + * Extends DiffableMap to provide optimized tree operations. + */ export class MutableTreeMap extends DiffableMap<number, MutableTree> {} +/** + * An immutable map collection for trees, supporting efficient diffing. + * Extends DiffableMap to provide optimized tree operations with readonly guarantees. + */ export class TreeMap extends DiffableMap<number, Tree> {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (45)
frontend/javascripts/libs/diffable_map.ts(14 hunks)frontend/javascripts/test/fixtures/hybridtracing_object.ts(4 hunks)frontend/javascripts/test/geometries/skeleton.spec.ts(10 hunks)frontend/javascripts/test/libs/diffable_map.spec.ts(6 hunks)frontend/javascripts/test/libs/iterator_utils.spec.ts(1 hunks)frontend/javascripts/test/libs/nml.spec.ts(24 hunks)frontend/javascripts/test/model/edge_collection.spec.ts(1 hunks)frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts(65 hunks)frontend/javascripts/test/sagas/compact_toggle_actions.spec.ts(8 hunks)frontend/javascripts/test/sagas/skeletontracing_saga.spec.ts(14 hunks)frontend/javascripts/types/api_types.ts(2 hunks)frontend/javascripts/viewer/api/api_latest.ts(7 hunks)frontend/javascripts/viewer/controller/combinations/skeleton_handlers.ts(1 hunks)frontend/javascripts/viewer/geometries/skeleton.ts(9 hunks)frontend/javascripts/viewer/model/accessors/skeletontracing_accessor.ts(8 hunks)frontend/javascripts/viewer/model/accessors/tracing_accessor.ts(1 hunks)frontend/javascripts/viewer/model/actions/connectome_actions.ts(1 hunks)frontend/javascripts/viewer/model/actions/proofread_actions.ts(1 hunks)frontend/javascripts/viewer/model/actions/skeletontracing_actions.tsx(1 hunks)frontend/javascripts/viewer/model/edge_collection.ts(5 hunks)frontend/javascripts/viewer/model/helpers/compaction/compact_toggle_actions.ts(5 hunks)frontend/javascripts/viewer/model/helpers/iterator_utils.ts(1 hunks)frontend/javascripts/viewer/model/helpers/nml_helpers.ts(26 hunks)frontend/javascripts/viewer/model/reducers/connectome_reducer.ts(6 hunks)frontend/javascripts/viewer/model/reducers/reducer_helpers.ts(1 hunks)frontend/javascripts/viewer/model/reducers/skeletontracing_reducer.ts(32 hunks)frontend/javascripts/viewer/model/reducers/skeletontracing_reducer_helpers.ts(22 hunks)frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts(1 hunks)frontend/javascripts/viewer/model/sagas/min_cut_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/proofread_saga.ts(1 hunks)frontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts(5 hunks)frontend/javascripts/viewer/model/sagas/update_actions.ts(1 hunks)frontend/javascripts/viewer/model/types/tree_types.ts(1 hunks)frontend/javascripts/viewer/store.ts(2 hunks)frontend/javascripts/viewer/view/action-bar/merge_modal_view.tsx(1 hunks)frontend/javascripts/viewer/view/context_menu.tsx(3 hunks)frontend/javascripts/viewer/view/right-border-tabs/abstract_tree_renderer.ts(1 hunks)frontend/javascripts/viewer/view/right-border-tabs/comment_tab/comment.tsx(1 hunks)frontend/javascripts/viewer/view/right-border-tabs/comment_tab/comment_tab_view.tsx(4 hunks)frontend/javascripts/viewer/view/right-border-tabs/connectome_tab/connectome_view.tsx(4 hunks)frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx(1 hunks)frontend/javascripts/viewer/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx(3 hunks)frontend/javascripts/viewer/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx(2 hunks)frontend/javascripts/viewer/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx(5 hunks)frontend/javascripts/viewer/view/right-border-tabs/trees_tab/tree_hierarchy_view_helpers.ts(2 hunks)
✅ Files skipped from review due to trivial changes (12)
- frontend/javascripts/viewer/view/right-border-tabs/comment_tab/comment.tsx
- frontend/javascripts/viewer/model/actions/connectome_actions.ts
- frontend/javascripts/test/model/edge_collection.spec.ts
- frontend/javascripts/viewer/controller/combinations/skeleton_handlers.ts
- frontend/javascripts/viewer/model/sagas/proofread_saga.ts
- frontend/javascripts/viewer/model/actions/proofread_actions.ts
- frontend/javascripts/viewer/model/actions/skeletontracing_actions.tsx
- frontend/javascripts/viewer/view/right-border-tabs/abstract_tree_renderer.ts
- frontend/javascripts/viewer/model/reducers/reducer_helpers.ts
- frontend/javascripts/viewer/model/sagas/update_actions.ts
- frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts
- frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx
🚧 Files skipped from review as they are similar to previous changes (28)
- frontend/javascripts/test/sagas/compact_toggle_actions.spec.ts
- frontend/javascripts/viewer/model/sagas/min_cut_saga.ts
- frontend/javascripts/viewer/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx
- frontend/javascripts/viewer/view/right-border-tabs/connectome_tab/connectome_view.tsx
- frontend/javascripts/viewer/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx
- frontend/javascripts/viewer/view/context_menu.tsx
- frontend/javascripts/types/api_types.ts
- frontend/javascripts/test/libs/iterator_utils.spec.ts
- frontend/javascripts/viewer/model/helpers/compaction/compact_toggle_actions.ts
- frontend/javascripts/viewer/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx
- frontend/javascripts/viewer/view/right-border-tabs/trees_tab/tree_hierarchy_view_helpers.ts
- frontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts
- frontend/javascripts/test/fixtures/hybridtracing_object.ts
- frontend/javascripts/test/libs/nml.spec.ts
- frontend/javascripts/viewer/geometries/skeleton.ts
- frontend/javascripts/viewer/view/right-border-tabs/comment_tab/comment_tab_view.tsx
- frontend/javascripts/viewer/model/reducers/connectome_reducer.ts
- frontend/javascripts/test/geometries/skeleton.spec.ts
- frontend/javascripts/viewer/model/edge_collection.ts
- frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts
- frontend/javascripts/viewer/api/api_latest.ts
- frontend/javascripts/viewer/store.ts
- frontend/javascripts/test/sagas/skeletontracing_saga.spec.ts
- frontend/javascripts/viewer/model/reducers/skeletontracing_reducer.ts
- frontend/javascripts/viewer/model/reducers/skeletontracing_reducer_helpers.ts
- frontend/javascripts/libs/diffable_map.ts
- frontend/javascripts/viewer/model/accessors/skeletontracing_accessor.ts
- frontend/javascripts/viewer/model/helpers/nml_helpers.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
frontend/javascripts/test/libs/diffable_map.spec.ts (1)
frontend/javascripts/libs/diffable_map.ts (5)
map(302-312)diffDiffableMaps(412-504)entries(319-323)keys(341-345)values(330-334)
frontend/javascripts/viewer/model/types/tree_types.ts (3)
frontend/javascripts/viewer/constants.ts (2)
Vector3(13-13)TreeType(224-224)frontend/javascripts/types/api_types.ts (2)
AdditionalCoordinate(28-28)MetadataEntryProto(867-876)frontend/javascripts/viewer/model/edge_collection.ts (1)
EdgeCollection(8-158)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
🔇 Additional comments (12)
frontend/javascripts/viewer/model/accessors/tracing_accessor.ts (1)
6-6: LGTM: Clean import reorganization.The change separates the
UserBoundingBoxtype import into its own line while maintaining the same import source. This is a non-functional change that aligns with the broader refactoring effort described in the PR objectives.frontend/javascripts/test/libs/diffable_map.spec.ts (10)
55-74: Good addition of tests for mutableDelete functionality.This test case thoroughly verifies both successful deletion of existing keys and the behavior when attempting to delete non-existent keys. It ensures that the mutableDelete method properly updates the map's state.
202-203: Good type annotation addition.Adding the explicit type annotation
as [number, any]improves type safety and helps prevent potential type inference issues in the mapping operation.
225-236: Great test coverage for null value handling.This test properly verifies that the DiffableMap can store and retrieve null values correctly, and appropriately differentiates between null values and undefined/missing keys.
238-265: Good testing of iteration methods.These tests thoroughly verify the behavior of all three iteration methods (entries, keys, values), ensuring they correctly expose the map's contents.
267-279: Good test for map transformation.This test verifies that the map method correctly applies the provided transformation function to each value in the map and returns the expected array of results.
281-294: Good test for toObject conversion.This test ensures that the toObject method correctly converts the DiffableMap to a plain JavaScript object with the expected key-value pairs.
296-308: Good edge case testing for delete operation.This test verifies that deleting a non-existent key is handled gracefully by returning the original map instance without modification, which is an important optimization.
310-341: Good performance testing for batch sizes.These two tests verify that the DiffableMap correctly handles both very large and very small batch sizes, which is important for performance optimization and memory management.
343-363: Good edge case testing for empty map diffing.These tests ensure that diffing operations involving empty maps produce the expected results, which is an important edge case to handle.
365-392: Excellent test for the new merge functionality.This test thoroughly verifies the behavior of the static merge method, ensuring it correctly combines two maps with proper precedence for overlapping keys. The assertions check all important aspects including the handling of unique keys, overlapping keys with different values, and ensuring the result is a new instance.
frontend/javascripts/viewer/view/action-bar/merge_modal_view.tsx (1)
22-22: LGTM: Import refactoring for better modularityThe import path change moves tree types from generic store to a dedicated tree types module, which improves code organization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/viewer/model/helpers/nml_helpers.ts (1)
784-813: Consider a more immutable approach for clarityThe current pattern of cloning and then mutating the cloned map might be confusing. Consider a more immutable approach by constructing a new map directly with the updated trees.
export function wrapInNewGroup( originalTrees: MutableTreeMap, _originalTreeGroups: TreeGroup[] | null | undefined, wrappingGroupName: string, ): [MutableTreeMap, TreeGroup[]] { const originalTreeGroups = _originalTreeGroups || []; const unusedGroupId = getMaximumGroupId(originalTreeGroups) + 1; - const trees = originalTrees.clone(); - - originalTrees.values().forEach((tree) => - trees.mutableSet(tree.treeId, { - ...tree, - // Give parentless trees the new treeGroup as parent - groupId: tree.groupId != null ? tree.groupId : unusedGroupId, - }), - ); + const trees = new MutableTreeMap( + originalTrees.values().map(tree => [ + tree.treeId, + { + ...tree, + // Give parentless trees the new treeGroup as parent + groupId: tree.groupId != null ? tree.groupId : unusedGroupId, + } + ]) + ); const treeGroups = [ // Create a new tree group which holds the old ones { name: wrappingGroupName, groupId: unusedGroupId, children: originalTreeGroups, }, ]; return [trees, treeGroups]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/javascripts/viewer/model/helpers/nml_helpers.ts(26 hunks)frontend/javascripts/viewer/model/reducers/skeletontracing_reducer.ts(32 hunks)frontend/javascripts/viewer/model/reducers/skeletontracing_reducer_helpers.ts(22 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/viewer/model/reducers/skeletontracing_reducer.ts
- frontend/javascripts/viewer/model/reducers/skeletontracing_reducer_helpers.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (5)
frontend/javascripts/viewer/model/helpers/nml_helpers.ts (5)
143-146: Great refactoring to use DiffableMap's native methods!The change from using direct property access or lodash utilities to DiffableMap's
.values().filter().toArray()pattern is a clean improvement that aligns with the overall refactoring goal.
1185-1201: Properly handling mutation during iterationGood approach to materialize the trees before iterating and mutating the collection. This prevents potential issues with modifying a collection while iterating over it.
1134-1134: Good replacement of lodash dependencyUsing the imported
minutility function fromiterator_utilsinstead of lodash's_.minreduces external dependencies and aligns with the codebase's move toward native JS methods and custom utilities.
879-880: Using the appropriate data structure for treesThis is a key part of the refactoring, changing from a plain JavaScript object to a
MutableTreeMapfor storing trees, which enables consistent access patterns and better type safety throughout the codebase.
367-392: Well-structured refactoring of tree serializationThe refactored
serializeTreesfunction cleanly handles the new data structures with a functional approach usingflatMap. This improves readability and consistency.
…knossos into treemap_to_diffablemap
frontend/javascripts/viewer/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx
Outdated
Show resolved
Hide resolved
…lection to prevent errors
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/javascripts/test/helpers/chainReducer.ts (1)
2-2: Consider improving type definitions for better clarity.The
actionGettersparameter type could be more explicit about the function signature to improve readability and type safety.- return (state: S, actionGetters: Array<A | ((arg0: S) => A)>) => { + return (state: S, actionGetters: Array<A | ((currentState: S) => A)>) => {This makes the parameter name more descriptive and consistent with its usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/javascripts/test/helpers/chainReducer.ts(1 hunks)frontend/javascripts/test/libs/chainReducer.spec.ts(2 hunks)frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts(48 hunks)frontend/javascripts/test/sagas/skeletontracing_saga.spec.ts(29 hunks)frontend/javascripts/viewer/model/reducers/skeletontracing_reducer.ts(32 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts
- frontend/javascripts/test/sagas/skeletontracing_saga.spec.ts
- frontend/javascripts/viewer/model/reducers/skeletontracing_reducer.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/javascripts/test/libs/chainReducer.spec.ts (1)
frontend/javascripts/test/helpers/chainReducer.ts (1)
chainReduce(1-10)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: frontend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (4)
frontend/javascripts/test/libs/chainReducer.spec.ts (4)
1-1: Import updated correctly for the new functional API.The import has been properly updated to use the named export
chainReducefunction instead of the previous class-based approach.
15-15: Correct usage of the new functional API.The test correctly demonstrates the new functional approach with an empty actions array, which should return the initial state unchanged.
21-21: Proper implementation of the increment reducer test.The test correctly uses the new API to apply multiple null actions to the IncrementReducer, expecting 4 increments from the initial state of 0.
27-27: Accurate test for the sum reducer with action values.The test properly validates that the SumReducer correctly processes the action values [2, 3, 4] with initial state 1, resulting in 10 (1 + 2 + 3 + 4).
This PR refactors the
TreeMaptype from a simple JS object to aDiffableMaptype. That should make skeleton handling more similar to Segments. Further, it it could provided benefits when diffing skeletons during saving.Similar to regular Arrays, JS
MapandSetdata types now supportfilter,map,reduceetc. See Iterator Helpers on MDNURL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Release a new frontend API version?Issues:
(Please delete unneeded items, merge only when none are left open)