Allow toggling visibility of segments and segment groups #8546
Conversation
📝 WalkthroughWalkthroughThis change introduces explicit visibility control for segments in both backend and frontend code. It adds an Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
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 ✨ Finishing Touches
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 (
|
…isibilityVolumeAction
…boxes if selective visibility is active
…ts in context menu of data viewports
|
@fm3 could you already have a look at the nml export part? I think, adding a boolean |
philippotto
changed the title
![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/viewer/model/reducers/volumetracing_reducer.ts (1)
338-342:toggledGroupstill has implicitany– compilation will fail withnoImplicitAnyThis issue has been pointed out in an earlier review.
Add an explicit type to satisfy the compiler:- let toggledGroup; + let toggledGroup: TreeGroup | undefined;
🧹 Nitpick comments (2)
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx (1)
1853-1856: ExpensivedeepFlatFilterruns on every render
checkedKeysis re-computed on every render withdeepFlatFilter, even when neither
groupTreenorshowCheckboxeschanged.
With datasets containing tens-of-thousands of segments this can become a
noticeable bottleneck.Consider memoising the result:
-const checkedKeys = deepFlatFilter(this.state.groupTree, …).map((n) => n.key); +const checkedKeys = React.useMemo( + () => + deepFlatFilter(this.state.groupTree, (node) => node.type === "segment" && node.isChecked).map( + (node) => node.key, + ), + [this.state.groupTree], +);Or move the computation to
getDerivedStateFromProps, similar to the existingsearchableTreeItemList.frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts (1)
368-391: Minor performance nit – avoid cloning when no change is requiredIn
toggleAllSegmentsReduceryou always clone thesegmentsmap even if the
computedshouldBecomeVisibleequals the current state for every segment.A quick early-exit prevents unnecessary allocations:
-const newSegments = segments.clone(); -Array.from(segments.values()).forEach((segment) => { - if (segment.isVisible !== shouldBecomeVisible) { - newSegments.mutableSet(segment.id, { … }); - } -}); - -return updateSegments(…newSegments); +if (!Array.from(segments.values()).some((s) => s.isVisible !== shouldBecomeVisible)) { + return state; // nothing changed +} + +const newSegments = segments.clone(); +segments.forEach((segment) => + newSegments.mutableSet(segment.id, { …segment, isVisible: shouldBecomeVisible }), +); +return updateSegments(…newSegments);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts(3 hunks)frontend/javascripts/viewer/model/actions/volumetracing_action_helpers.ts(1 hunks)frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts(7 hunks)frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx(17 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/viewer/controller/combinations/volume_handlers.ts
- frontend/javascripts/viewer/model/actions/volumetracing_action_helpers.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts (1)
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx (1)
updateSegments(279-289)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (2)
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx (1)
1696-1714: Potential “stale-layer” edge case when dispatching toggle actions
onCheckfetches the visible layer viagetVisibleSegmentationLayer(Store.getState())instead of relying on the prop (this.props.visibleSegmentationLayer).
If the component is rendered while a different layer is visible in the Redux state (e.g., the user quickly switches layers before the click handler is executed), the action might be dispatched against an unexpected layer.-const visibleSegmentationLayer = getVisibleSegmentationLayer(Store.getState()); +const visibleSegmentationLayer = this.props.visibleSegmentationLayer ?? + getVisibleSegmentationLayer(Store.getState());This keeps the handler deterministic and avoids a race between UI state and Redux state.
frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts (1)
331-365: Local-segmentation layers cannot toggle groups
getSegmentUpdateInforeturnssegmentGroups: []for
UPDATE_LOCAL_SEGMENTATION_DATA.
toggleSegmentGroupReducerrelies onsegmentGroupsto findtoggledGroup.
For local (non-tracing) segmentations that support groups, this reducer will
early-return and the visibility toggle silently does nothing.Please confirm whether local segmentations are expected to have groups.
If they are, include the realsegmentGroupsinUPDATE_LOCAL_SEGMENTATION_DATA
or guard the UI against showing group checkboxes in that mode.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/javascripts/types/api_types.ts(2 hunks)frontend/javascripts/viewer/store.ts(6 hunks)frontend/javascripts/viewer/view/context_menu.tsx(14 hunks)frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_list_item.tsx(4 hunks)frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx(17 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_list_item.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/types/api_types.ts
- frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/javascripts/viewer/store.ts (1)
frontend/javascripts/types/api_types.ts (1)
APIMeshFileInfo(946-955)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (3)
frontend/javascripts/viewer/store.ts (2)
242-252:isVisibleshould be backward-compatible or defaulted
Segmentnow requires a non-optionalisVisibleflag. Every place that instantiates aSegment(initial state, reducers, backend deserialisation, tests, etc.) must now provide this flag, otherwise compilation or runtime errors are inevitable.Two safer alternatives:
- readonly isVisible: boolean; + /** If undefined, treat the segment as visible to keep old data valid. */ + readonly isVisible?: boolean;or keep it mandatory but initialise it in one central place (e.g. a
createSegmenthelper and indefault_state.ts). Please verify that all code paths are updated.
279-281: Missing default forhideUnregisteredSegments
VolumeTracing.hideUnregisteredSegmentsis added as a required boolean.default_state.ts(and any migration code for persisted annotations) must initialise it (falseseems sensible). Otherwiseundefinedwill leak into selectors that expect a boolean.frontend/javascripts/viewer/view/context_menu.tsx (1)
1015-1034:toggleSegmentVisibilitydoes not pass the layer name
getUpdateSegmentActionToToggleVisibilityis invoked without the layer name; when multiple segmentation layers are loaded the helper cannot know which layer to update—risking a wrong layer or a no-op. If the helper does not infer the layer internally, pass it explicitly:- const action = getUpdateSegmentActionToToggleVisibility( + const action = getUpdateSegmentActionToToggleVisibility( Store.getState(), clickedSegmentId, globalPosition, additionalCoordinates, + visibleSegmentationLayer?.name, );Please confirm the helper’s signature.
| export type LocalSegmentationData = { | ||
| // For meshes, the string represents additional coordinates, number is the segment ID. | ||
| // The undefined types were added to enforce null checks when using this structure. | ||
| readonly meshes: Record<string, Record<number, MeshInformation> | undefined> | undefined; | ||
| readonly availableMeshFiles: Array<APIMeshFileInfo> | null | undefined; | ||
| readonly currentMeshFile: APIMeshFileInfo | null | undefined; | ||
| // Note that for a volume tracing, this information should be stored | ||
| // in state.annotation.volume.segments, as this is also persisted on the | ||
| // server (i.e., not "local"). | ||
| // The `segments` here should only be used for non-annotation volume | ||
| // layers. | ||
| readonly segments: SegmentMap; | ||
| // Note that segments that are not in the segment tab could be stored as selected. | ||
| // To get only available segments or group, use getSelectedIds() in volumetracing_accessor. | ||
| readonly selectedIds: { segments: number[]; group: number | null }; | ||
| readonly connectomeData: ConnectomeData; | ||
| }; |
There was a problem hiding this comment.
Potential runtime crash when layer entry is missing
localSegmentationData is now a required record entry per layer, yet many selectors use
state.localSegmentationData[visibleSegmentationLayer.name].currentMeshFilewithout optional-chaining. When a layer is opened for the first time (or a dataset is migrated) the entry may be absent, causing Cannot read properties of undefined.
Either guarantee the record is pre-populated for all layers in default_state/reducers, or guard every access:
-? state.localSegmentationData[visibleSegmentationLayer.name].currentMeshFile
+? state.localSegmentationData[visibleSegmentationLayer.name]?.currentMeshFileSame for connectomeData, segments, etc.
Also applies to: 645-647
| const currentMeshFile = useWkSelector((state) => | ||
| visibleSegmentationLayer != null | ||
| ? state.localSegmentationData[visibleSegmentationLayer.name].currentMeshFile | ||
| : null, | ||
| ); | ||
| const currentConnectomeFile = useWkSelector((state) => | ||
| visibleSegmentationLayer != null | ||
| ? state.localSegmentationData[visibleSegmentationLayer.name].connectomeData | ||
| .currentConnectomeFile | ||
| : null, | ||
| ); |
There was a problem hiding this comment.
Selector may throw when localSegmentationData[layer] is undefined
state.localSegmentationData[visibleSegmentationLayer.name].currentMeshFileIf the record for visibleSegmentationLayer.name has not been created yet, the code crashes. Use optional-chaining (same for connectomeData a few lines below):
-? state.localSegmentationData[visibleSegmentationLayer.name].currentMeshFile
+? state.localSegmentationData[visibleSegmentationLayer.name]?.currentMeshFileand ensure reducers always create an empty LocalSegmentationData for new layers.
| const onlyShowSegment = () => { | ||
| if (!visibleSegmentationLayer || globalPosition == null) { | ||
| return; | ||
| } | ||
| const clickedSegmentId = getSegmentIdForPosition(globalPosition); | ||
| if (clickedSegmentId === 0) { | ||
| Toast.info("No segment found at the clicked position"); | ||
| return; | ||
| } | ||
|
|
||
| Store.dispatch(setHideUnregisteredSegmentsAction(true, visibleSegmentationLayer.name)); | ||
| Store.dispatch(toggleAllSegmentsAction(visibleSegmentationLayer.name, false)); | ||
| Store.dispatch( | ||
| updateSegmentAction( | ||
| clickedSegmentId, | ||
| { | ||
| isVisible: true, | ||
| somePosition: globalPosition, | ||
| someAdditionalCoordinates: additionalCoordinates, | ||
| }, | ||
| visibleSegmentationLayer.name, | ||
| undefined, | ||
| true, | ||
| ), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid recomputing the segment id & guard missing layer
onlyShowSegment() calls getSegmentIdForPosition although segmentIdAtPosition has already been computed outside. In addition, if visibleSegmentationLayer is missing the call is skipped, but the function still assumes segmentIdAtPosition exists later.
- const clickedSegmentId = getSegmentIdForPosition(globalPosition);
+ // Re-use the already determined id if available to avoid an extra traversal
+ const clickedSegmentId =
+ segmentIdAtPosition > 0 ? segmentIdAtPosition : getSegmentIdForPosition(globalPosition);Besides saving work, this prevents subtle mismatches when the position changes between invocations.
Committable suggestion skipped: line range outside the PR's diff.
frontend/javascripts/test/screenshots/dtype_int16_segmentation_zoomed_in_selective_segment.png
Show resolved
Hide resolved
frontend/javascripts/viewer/model/actions/volumetracing_action_helpers.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/model/bucket_data_handling/layer_rendering_manager.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/model/bucket_data_handling/layer_rendering_manager.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx
Outdated
Show resolved
Hide resolved
...tore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeUpdateActions.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
frontend/javascripts/types/schemas/dataset_view_configuration.schema.ts (1)
82-82: Improved naming by removing redundant WithoutNull suffix.Good cleanup - this resolves the confusing situation where
defaultDatasetViewConfigurationWithoutNullactually contained a null value fornativelyRenderedLayerName. The consolidation of configurations makes the code clearer.frontend/javascripts/viewer/model/bucket_data_handling/layer_rendering_manager.ts (2)
354-366:⚠️ Potential issueMissing fast-path for "deleteSegment" leads to stale cuckoo entries
When a segment is deleted, we should directly remove its entry from the cuckoo table, but the current implementation lets
deleteSegmentfall through to the general logic which could leave stale entries depending onhideUnregisteredSegments.@@ for (const updateAction of cachedDiffSegmentLists(this.name, prevSegments, newSegments)) { - if ( + if (updateAction.name === "deleteSegment") { + cuckoo.unset(updateAction.value.id); + continue; + } + + if ( updateAction.name === "updateSegment" || updateAction.name === "createSegment" || - updateAction.name === "deleteSegment" || updateAction.name === "updateSegmentVisibility" ) {
425-425: 🛠️ Refactor suggestionColour components should be integers – use
Math.round
CuckooTableVec3#setexpects integer components (0-255), but255 * color[0]yields floating-point numbers that will be truncated implicitly. This could cause subtle off-by-one artefacts.- cuckoo.set(id, [255 * color[0], 255 * color[1], blueChannel]); + cuckoo.set(id, [Math.round(255 * color[0]), Math.round(255 * color[1]), blueChannel]);This should also be applied to line 416 for consistency.
🧹 Nitpick comments (3)
frontend/javascripts/viewer/model/bucket_data_handling/layer_rendering_manager.ts (3)
372-375: Performance improvement: move capacity check outside the loopChecking the cuckoo table capacity inside the update loop for each segment is inefficient. Consider performing this check once before the loop.
const updateToNewSegments = (newSegments: SegmentMap) => { const cuckoo = this.getCustomColorCuckooTable(); const hideUnregisteredSegments = getHideUnregisteredSegmentsForLayer( Store.getState(), this.name, ); + + if (cuckoo.entryCount >= cuckoo.getCriticalCapacity()) { + ignoreCustomColors(); + return; + } + for (const updateAction of cachedDiffSegmentLists(this.name, prevSegments, newSegments)) { if ( updateAction.name === "updateSegment" || updateAction.name === "createSegment" || updateAction.name === "deleteSegment" || updateAction.name === "updateSegmentVisibility" ) { const { id } = updateAction.value; const newSegment = newSegments.getNullable(id); const isVisible = newSegment?.isVisible ?? !hideUnregisteredSegments; const color = newSegment?.color; - if (cuckoo.entryCount >= cuckoo.getCriticalCapacity()) { - ignoreCustomColors(); - return; - }
441-444: Log specific errors in catch blockThe catch block swallows all errors without logging them, making debugging difficult. Consider logging the specific error before ignoring custom colors.
} catch (error) { + console.error("Error updating segment in cuckoo table:", error); ignoreCustomColors(); return; }
452-456: Provide more actionable error messageThe warning message could be more helpful by suggesting what users can do to address the issue.
function showTooManyCustomColorsWarning() { Toast.warning( - "There are too many segments with custom colors/visibilities. Default rendering will be used for now.", + "Too many segments with custom colors/visibilities. Default rendering will be used. Consider reducing the number of custom segments.", ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/javascripts/types/schemas/dataset_view_configuration.schema.ts(1 hunks)frontend/javascripts/viewer/default_state.ts(2 hunks)frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts(1 hunks)frontend/javascripts/viewer/model/actions/volumetracing_action_helpers.ts(1 hunks)frontend/javascripts/viewer/model/bucket_data_handling/layer_rendering_manager.ts(4 hunks)frontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsx(4 hunks)frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx(17 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts
- frontend/javascripts/viewer/model/actions/volumetracing_action_helpers.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
frontend/javascripts/viewer/default_state.ts (1)
frontend/javascripts/types/schemas/dataset_view_configuration.schema.ts (1)
defaultDatasetViewConfiguration(82-92)
frontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsx (1)
frontend/javascripts/types/schemas/dataset_view_configuration.schema.ts (1)
defaultDatasetViewConfiguration(82-92)
⏰ 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 (15)
frontend/javascripts/viewer/default_state.ts (1)
3-3: Updated imports and configuration reference to match renamed constant.The change updates the import and usage of
defaultDatasetViewConfigurationwhich was previously nameddefaultDatasetViewConfigurationWithoutNull. This aligns with the simplification of configuration structures in the schema file.Also applies to: 52-52
frontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsx (2)
58-58: Updated imports to match renamed configuration constant.The change updates imports and references to
defaultDatasetViewConfiguration(previously nameddefaultDatasetViewConfigurationWithoutNull), aligning with changes in the schema file. Also added theModelimport which was missing.Also applies to: 114-114, 937-937
135-135: Replaced selective visibility toggle with new HideUnregisteredSegmentsSwitch component.This introduces a new component for controlling segment visibility that replaces the previous selective segment visibility toggle. The approach looks cleaner and aligns with the PR goals of enhancing segment visibility control.
Also applies to: 944-945
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx (8)
11-11: Added necessary imports for new segment visibility functionality.The imports have been updated to support the new visibility toggle features, benchmark functionality, and improved tree rendering with folder icons.
Also applies to: 34-34, 38-38, 44-44, 51-51, 65-65, 91-91, 99-100
189-190: Added Redux state mapping for segment visibility controls.Added
hideUnregisteredSegmentsstate mapping to access the visibility state, and a newshowCheckboxesproperty to control checkbox visibility based on proofreading mode settings. This effectively integrates the new visibility feature with the Redux state.Also applies to: 217-219
387-388: Added isChecked property to track segment visibility.The segment visibility state is now tracked at the tree node level, making it available for rendering and interactions.
1696-1714: Implemented checkbox toggle handlers for segments and groups.Added an
onCheckhandler that performs three distinct visibility toggles:
- For individual segments by dispatching segment visibility toggle actions
- For the root group by toggling all segments
- For segment groups by toggling visibility for all segments in that group
This provides a comprehensive approach to visibility control that matches the PR requirements.
1716-1823: Extracted and improved titleRender for better tree node display.The
titleRendermethod has been extracted and refactored to:
- Render segment nodes with
SegmentListItem- Render group nodes with folder icons and editable labels
- Add context menus for both node types with appropriate actions
This is a good refactoring that improves code organization and enhances the UI with folder icons for better visual hierarchy.
1925-1928: Added checkbox support to the tree component.The tree now supports checkboxes for toggling visibility with appropriate callbacks and state management. The
showCheckboxesprop controls whether checkboxes are visible based on proofreading mode and user configuration.
406-407:✅ Verification successful
Added benchmark functionality for performance testing.
The addition of benchmark functionality allows programmatic scrolling through segments for performance testing. This is a good approach for ensuring the new visibility features don't negatively impact performance.
However, there's a potential issue in
deepFlatFilter(called in line 1854) that might lead to errors for empty nodes. Ensure that function properly checks for null/undefined children.Also applies to: 441-470
🏁 Script executed:
#!/bin/bash # Check the implementation of deepFlatFilter to ensure it handles null/undefined children rg -A 10 "deepFlatFilter" --type tsLength of output: 12196
Benchmark functionality looks solid; deepFlatFilter is safe
The added benchmark-based scrolling for performance testing is a great way to validate the new visibility features. Regarding the concern in
deepFlatFilter, the implementation already includes anif (node.children)guard before recursing, so it safely skips overnullorundefinedchildren without errors. No changes are needed here.
854-857:✅ Verification successful
Tree node visibility tracking with deepFlatFilter.
The function collects all visible segment nodes to create
checkedKeysfor the tree. Make suredeepFlatFilterproperly handles null or undefined children to avoid runtime errors.
🌐 Web query:
What is the purpose of deepFlatFilter in React tree components?💡 Result:
In React tree components,
deepFlatFilteris typically a utility function designed to recursively traverse a tree structure, apply a filter condition at every node, and flatten the result into a one-dimensional array.The purpose is generally:
- Deep Filtering: It goes through every node, even deeply nested ones, checking if they meet the filter criteria.
- Flattening: Instead of returning a nested filtered tree, it returns a flat list of all nodes that match the filter condition.
- Efficient Searching/Filtering: This is useful when you want to find all nodes in a tree that satisfy some condition, regardless of their level in the hierarchy.
- Simplification for Rendering or Processing: Having a flat array of filtered nodes can simplify rendering logic or other operations where tree-structured data is converted to a list.
If you want, I can provide a sample implementation or explain more on how such a function works in practice. Would you like that?
🏁 Script executed:
#!/bin/bash # Locate all occurrences of deepFlatFilter in the repository rg "deepFlatFilter" -nLength of output: 1330
🏁 Script executed:
#!/bin/bash # Display the implementation of deepFlatFilter to verify how children are handled sed -n '270,330p' frontend/javascripts/viewer/view/right-border-tabs/trees_tab/tree_hierarchy_view_helpers.tsLength of output: 2010
deepFlatFilter safely handles null/undefined children
ThedeepFlatFilterimplementation usesif (node.children)before recursing, so it will skip overnullorundefinedchildren without error. No changes required here.frontend/javascripts/viewer/model/bucket_data_handling/layer_rendering_manager.ts (4)
348-352: Good function extraction for blue channel calculationExcellent extraction of the blue channel calculation logic into a dedicated function. This improves code maintainability and readability.
336-346: Good implementation of visibility togglingThe implementation correctly rebuilds the cuckoo table when the visibility settings change. This ensures consistent rendering behavior across the application.
377-386: Well-documented visibility encoding logicThe comments explaining the visibility encoding mechanism are thorough and helpful for understanding the complex logic.
135-136: Good encapsulation of class membersMaking
colorCuckooTableprivate and adding a dedicated array for unsubscribers improves the class structure and maintainability.
There was a problem hiding this comment.
I found one thing in your latest changes regarding the defaultDatasetViewConfiguration refactoring.
Beside this thanks for implementing. I'll give it re-test and final approval after my defaultDatasetViewConfiguration is discussed / resolved as well as the one backend comment. Then this should be ready to go :D
frontend/javascripts/test/screenshots/dtype_int16_segmentation_zoomed_in_selective_segment.png
Show resolved
Hide resolved
frontend/javascripts/viewer/model/bucket_data_handling/layer_rendering_manager.ts
Outdated
Show resolved
Hide resolved
...tore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeUpdateActions.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CHANGELOG.unreleased.md (2)
17-17: Include segment group visibility in changelog entryThe bullet for PR #8546 currently mentions toggling individual segments but omits the new ability to toggle segment groups. For accuracy and completeness, update the first sentence to reflect both features.
Proposed diff:
- - Added checkboxes to the segments tab that allow to show/hide individual segments. + - Added checkboxes to the segments tab that allow to show/hide individual segments and segment groups.
38-38: Typographical: add comma after "From now on"To improve readability, add a comma after the introductory phrase “From now on.”
Proposed diff:
- - The old "Selective Segment Visibility" feature that allowed to only see the active and the hovered segment was removed. From now on the visibility of segments can be controlled with checkboxes in the segment list and with the "Hide unlisted segments" toggle in the layer settings. [#8546](https://github.com/scalableminds/webknossos/pull/8546) + - The old "Selective Segment Visibility" feature that allowed to only see the active and the hovered segment was removed. From now on, the visibility of segments can be controlled with checkboxes in the segment list and with the "Hide unlisted segments" toggle in the layer settings. [#8546](https://github.com/scalableminds/webknossos/pull/8546)🧰 Tools
🪛 LanguageTool
[typographical] ~38-~38: Consider adding a comma after this introductory phrase.
Context: ...ve and the hovered segment was removed. From now on the visibility of segments can be contr...(FROM_NOW_ON_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.unreleased.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[typographical] ~38-~38: Consider adding a comma after this introductory phrase.
Context: ...ve and the hovered segment was removed. From now on the visibility of segments can be contr...
(FROM_NOW_ON_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
|
could you approve, @MichaelBuessemeyer, or is sth missing? :) @fm3 not sure whether you missed it or whether you decided against incorporating it, but here's an optional, unresolved back-end comment from @MichaelBuessemeyer: #8546 (comment) |
|
Thanks for the reminder, I resolved it now. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
frontend/javascripts/viewer/view/context_menu.tsx (3)
1470-1472:⚠️ Potential issueUse optional chaining to prevent potential runtime errors.
If
visibleSegmentationLayer.nameexists but hasn't been added tostate.localSegmentationDatayet, this code will crash when trying to accesscurrentMeshFile. Use optional chaining for safer access.const currentMeshFile = useWkSelector((state) => visibleSegmentationLayer != null - ? state.localSegmentationData[visibleSegmentationLayer.name].currentMeshFile + ? state.localSegmentationData[visibleSegmentationLayer.name]?.currentMeshFile : null, );
1473-1478:⚠️ Potential issueUse optional chaining for connectomeData access.
Similar to the previous issue with
currentMeshFile, this code doesn't use optional chaining when accessing nested properties oflocalSegmentationData.const currentConnectomeFile = useWkSelector((state) => visibleSegmentationLayer != null - ? state.localSegmentationData[visibleSegmentationLayer.name].connectomeData + ? state.localSegmentationData[visibleSegmentationLayer.name]?.connectomeData .currentConnectomeFile : null, );
989-1014: 🛠️ Refactor suggestionRe-use the already determined segment ID instead of recalculating it.
The
onlyShowSegment()function recalculates the segment ID at the clicked position even thoughsegmentIdAtPositionis already available from the outer scope. This causes unnecessary processing and may lead to inconsistent behavior if the position changes between function calls.const onlyShowSegment = () => { if (!visibleSegmentationLayer || globalPosition == null) { return; } - const clickedSegmentId = getSegmentIdForPosition(globalPosition); + // Re-use the already determined id if available to avoid an extra traversal + const clickedSegmentId = + segmentIdAtPosition > 0 ? segmentIdAtPosition : getSegmentIdForPosition(globalPosition); if (clickedSegmentId === 0) { Toast.info("No segment found at the clicked position"); return; }frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts (1)
348-351:⚠️ Potential issueSpecify the type for
toggledGroupto avoid implicit any.The variable
toggledGroupdoesn't have an explicit type, which causes issues in environments withnoImplicitAny: true.- let toggledGroup; + let toggledGroup: TreeGroup | undefined; forEachGroups(segmentGroups, (group) => { if (group.groupId === groupId) toggledGroup = group; });
🧹 Nitpick comments (3)
CHANGELOG.unreleased.md (1)
46-46: Add a comma after the introductory phrase.From now on, the visibility of segments can be controlled...
-The old "Selective Segment Visibility" feature that allowed to only see the active and the hovered segment was removed. From now on the visibility of segments can be controlled with checkboxes in the segment list and with the "Hide unlisted segments" toggle in the layer settings. [#8546](https://github.com/scalableminds/webknossos/pull/8546) +The old "Selective Segment Visibility" feature that allowed to only see the active and the hovered segment was removed. From now on, the visibility of segments can be controlled with checkboxes in the segment list and with the "Hide unlisted segments" toggle in the layer settings. [#8546](https://github.com/scalableminds/webknossos/pull/8546)🧰 Tools
🪛 LanguageTool
[typographical] ~46-~46: Consider adding a comma after this introductory phrase.
Context: ...ve and the hovered segment was removed. From now on the visibility of segments can be contr...(FROM_NOW_ON_COMMA)
frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts (2)
357-364: Optimize performance for large segment sets.The current implementation iterates through all segments to determine if any segment in affected groups is invisible. For large segment sets, this can be inefficient.
Consider adding an early exit if there are a large number of segments:
const shouldBecomeVisible = targetVisibility != null ? targetVisibility - : Array.from(segments.values()).some( + : (segments.size > 10000 ? false : Array.from(segments.values()).some( (segment) => typeof segment.groupId === "number" && affectedGroupIds.has(segment.groupId) && !segment.isVisible, - ); + ));Or better, try to keep track of visibility state per group to avoid iterations.
394-398: Add a check to skip update if no change is required.For toggle all segments, check first if any segments need updating to avoid unnecessary operations, especially for large segment sets.
+ // Check if any segment needs an update before cloning the map + const needsUpdate = Array.from(segments.values()).some( + (segment) => segment.isVisible !== shouldBecomeVisible + ); + + if (!needsUpdate) { + return state; + } const newSegments = segments.clone(); Array.from(segments.values()).forEach((segment) => { if (segment.isVisible !== shouldBecomeVisible) { newSegments.mutableSet(segment.id, { ...segment, isVisible: shouldBecomeVisible }); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.unreleased.md(2 hunks)frontend/javascripts/types/api_types.ts(2 hunks)frontend/javascripts/viewer/api/wk_dev.ts(3 hunks)frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts(7 hunks)frontend/javascripts/viewer/view/context_menu.tsx(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/types/api_types.ts
- frontend/javascripts/viewer/api/wk_dev.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts (1)
frontend/javascripts/viewer/store.ts (5)
VolumeTracing(260-281)SegmentMap(253-253)TreeGroup(172-174)Segment(242-252)WebknossosState(629-649)
🪛 LanguageTool
CHANGELOG.unreleased.md
[typographical] ~46-~46: Consider adding a comma after this introductory phrase.
Context: ...ve and the hovered segment was removed. From now on the visibility of segments can be contr...
(FROM_NOW_ON_COMMA)
⏰ 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 (4)
frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts (4)
304-304: LGTM: Good default for hideUnregisteredSegments.Setting the default value for
hideUnregisteredSegmentstofalseensures backward compatibility with existing data.
335-375: Approve the toggleSegmentGroupReducer implementation.The implementation of the segment group visibility toggle is well-structured, maintaining immutability patterns. It correctly handles both direct visibility state settings and toggling based on current state, and efficiently applies changes only to relevant segments.
377-401: Approve the toggleAllSegmentsReducer implementation.The implementation correctly handles global visibility toggling for all segments in a layer, with a sensible auto-toggle behavior when no explicit visibility value is provided.
491-527: Approve the new action handlers in VolumeTracingReducer.The redux reducer now properly handles the new visibility-related actions:
TOGGLE_SEGMENT_GROUP,TOGGLE_ALL_SEGMENTS, andSET_HIDE_UNREGISTERED_SEGMENTS. The implementation correctly updates both volume tracing state and local segmentation data as needed.
| const toggleSegmentVisibility = () => { | ||
| if (!visibleSegmentationLayer || globalPosition == null) { | ||
| return; | ||
| } | ||
| const clickedSegmentId = getSegmentIdForPosition(globalPosition); | ||
| if (clickedSegmentId === 0) { | ||
| Toast.info("No segment found at the clicked position"); | ||
| return; | ||
| } | ||
|
|
||
| const action = getUpdateSegmentActionToToggleVisibility( | ||
| Store.getState(), | ||
| clickedSegmentId, | ||
| globalPosition, | ||
| additionalCoordinates, | ||
| ); | ||
| if (action != null) { | ||
| Store.dispatch(action); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Re-use the already determined segment ID in toggleSegmentVisibility.
Similarly to the onlyShowSegment function, this function also unnecessarily recalculates the segment ID.
const toggleSegmentVisibility = () => {
if (!visibleSegmentationLayer || globalPosition == null) {
return;
}
- const clickedSegmentId = getSegmentIdForPosition(globalPosition);
+ // Re-use the already determined id if available to avoid an extra traversal
+ const clickedSegmentId =
+ segmentIdAtPosition > 0 ? segmentIdAtPosition : getSegmentIdForPosition(globalPosition);
if (clickedSegmentId === 0) {
Toast.info("No segment found at the clicked position");
return;
}📝 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.
| const toggleSegmentVisibility = () => { | |
| if (!visibleSegmentationLayer || globalPosition == null) { | |
| return; | |
| } | |
| const clickedSegmentId = getSegmentIdForPosition(globalPosition); | |
| if (clickedSegmentId === 0) { | |
| Toast.info("No segment found at the clicked position"); | |
| return; | |
| } | |
| const action = getUpdateSegmentActionToToggleVisibility( | |
| Store.getState(), | |
| clickedSegmentId, | |
| globalPosition, | |
| additionalCoordinates, | |
| ); | |
| if (action != null) { | |
| Store.dispatch(action); | |
| } | |
| }; | |
| const toggleSegmentVisibility = () => { | |
| if (!visibleSegmentationLayer || globalPosition == null) { | |
| return; | |
| } | |
| // Re-use the already determined id if available to avoid an extra traversal | |
| const clickedSegmentId = | |
| segmentIdAtPosition > 0 ? segmentIdAtPosition : getSegmentIdForPosition(globalPosition); | |
| if (clickedSegmentId === 0) { | |
| Toast.info("No segment found at the clicked position"); | |
| return; | |
| } | |
| const action = getUpdateSegmentActionToToggleVisibility( | |
| Store.getState(), | |
| clickedSegmentId, | |
| globalPosition, | |
| additionalCoordinates, | |
| ); | |
| if (action != null) { | |
| Store.dispatch(action); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/view/context_menu.tsx around lines 1025 to 1044,
the function toggleSegmentVisibility recalculates the segment ID by calling
getSegmentIdForPosition again, which is redundant. Modify the function to accept
the already determined segment ID as a parameter or use the existing variable
holding the segment ID instead of recalculating it, similar to how
onlyShowSegment handles it. This avoids unnecessary computation and keeps the
code consistent.
4 participants
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)