Add multi click split tool for proofreading #8824
Conversation
📝 WalkthroughWalkthroughAdds a proofreading multi-split workflow: UI toggle and context-menu options, store/actions/reducer/sagas for partitioned selections and partition-aware min-cut, input handlers and keyboard bindings to toggle partitions, partition-aware mesh highlighting/rendering, backend min-cut API changes, docs and localization updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
- it was always cleared as the current agglomerate id was never stored to the store state leading to an always auto clear - fix logic disabling multi select short cuts when multi select is disabled in ortho views
…e to partitions not being rendered in viewports and thus not being part of the local mapping
There was a problem hiding this comment.
Mainly formatting
frontend/javascripts/viewer/model/reducers/proofreading_reducer.ts
Outdated
Show resolved
Hide resolved
| ); | ||
|
|
||
| const areSegmentStatisticsAvailable = wasSegmentOrMeshClicked && isSegmentIndexAvailable; | ||
| const areSegmentStatisticsAvailable = false && wasSegmentOrMeshClicked && isSegmentIndexAvailable; |
There was a problem hiding this comment.
@fm3 Do we consider fixing this segment stats oom issue? I think we do not have an issue for this yet. At least I could not find any while searching for it
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (1)
1466-1511: Fix BigInt compatibility insplitAgglomerateInMapping.Ensure added entries match the existing mapping’s key/value types.
function* splitAgglomerateInMapping( activeMapping: ActiveMappingInfo, sourceAgglomerateId: number, volumeTracingId: string, additionalSegmentsToRequest: number[] = [], ) { @@ - const splitMapping = new Map( + const splitMapping = new Map( Array.from(activeMapping.mapping as NumberLikeMap, ([segmentId, agglomerateId]) => { // @ts-ignore get() is expected to accept the type that segmentId has. const mappedId = mappingAfterSplit.get(segmentId); if (mappedId != null) { return [segmentId, mappedId]; } return [segmentId, agglomerateId]; }), ); - // Add potentially missing entries of segment in additionalSegmentsToRequest to the new map. - for (const unmappedId of additionalSegmentsToRequest) { - // @ts-ignore get() is expected to accept the type that unmappedId has. - const mappedId = mappingAfterSplit.get(unmappedId); - if (mappedId) { - splitMapping.set(unmappedId, mappedId); - } - } + // Add potentially missing entries while preserving map key/value types. + const isNumberKeyed = isNumberMap(activeMapping.mapping); + const adaptKey = isNumberKeyed ? (n: number) => n : (n: number) => BigInt(n); + const adaptVal = isNumberKeyed + ? (v: number | bigint) => Number(v) + : (v: number | bigint) => BigInt(v as number); + for (const unmappedId of additionalSegmentsToRequest) { + // @ts-ignore get() is expected to accept the type that unmappedId has. + const mappedId = mappingAfterSplit.get(unmappedId); + if (mappedId != null) { + // @ts-ignore set() accepts the appropriate typed key/value. + splitMapping.set(adaptKey(unmappedId), adaptVal(mappedId)); + } + } return splitMapping as Mapping; }
♻️ Duplicate comments (8)
docs/ui/keyboard_shortcuts.md (1)
151-151: Use “Segment” consistently (avoid “Super Voxel”).Earlier feedback asked for consistent terminology. Replace “Super Voxel” with “Segment”.
-| ++ctrl++ / ++cmd++ + Left Click | Activate Super Voxel of Agglomerate for Proofreading Actions | +| ++ctrl++ / ++cmd++ + Left Click | Activate Segment of Agglomerate for Proofreading Actions |frontend/javascripts/viewer/view/action-bar/tools/volume_specific_ui.tsx (1)
494-503: Align tooltip copy with feature scope and add ARIA label.Mention both orthogonal and 3D viewports (as supported) and add an accessible label.
- <ToggleButton + <ToggleButton active={isMultiSplitActive} - title={`${isMultiSplitActive ? "Disable" : "Enable"} multi splitting. When enabled, two partitions can be selected in the 3D viewport to split more accurately.`} + title={`${isMultiSplitActive ? "Disable" : "Enable"} Multi Cut mode. When enabled, you can select two partitions in the orthogonal or 3D viewports for more accurate splitting.`} style={NARROW_BUTTON_STYLE} onClick={() => handleToggleIsMultiSplitActive(!isMultiSplitActive)} + aria-label="Toggle Multi Cut mode" > <ScissorOutlined /> </ToggleButton>frontend/javascripts/viewer/view/context_menu.tsx (1)
507-512: Terminology consistency: “Super-Voxel” vs “Supervoxel”.Prior code/reviews questioned the term. If the product wording elsewhere uses “Supervoxel”, align here.
frontend/javascripts/viewer/controller/viewmodes/plane_controller.tsx (1)
80-98: Update error message to include proofreading controls.Message still says “between skeleton and volume controller” though proofreading is part of the check.
- `There are unsolved conflicts between skeleton and volume controller: ${conflictingHandlers.join( + `There are unsolved conflicts between skeleton, volume, and proofreading controllers: ${conflictingHandlers.join( ", ", )}`,frontend/javascripts/viewer/controller/segment_mesh_controller.ts (2)
666-667: Simplify partition iteration using const assertion.The iteration can be simplified as suggested in the past review comment.
- for (const partitionNumber of [1, 2]) { - const partition = partitionNumber as 1 | 2; + for (const partition of [1, 2] as const) {
654-690: Consider renaming for consistency withhighlightActiveUnmappedSegmentId.The function name could be more consistent with the existing naming pattern as suggested in the past review.
- updateMinCutPartitionHighlighting = (minCutPartitions: MinCutPartitions | null) => { + updateActiveMinCutPartitionHighlighting = (minCutPartitions: MinCutPartitions | null) => {Or alternatively:
- updateMinCutPartitionHighlighting = (minCutPartitions: MinCutPartitions | null) => { + updatePartitionHighlighting = (minCutPartitions: MinCutPartitions | null) => {webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala (2)
465-466: Use consistent naming for partition roots.Consider using
partition1RootIdandpartition2RootIdfor consistency with the partition naming used elsewhere.- val firstPartitionRootId = -1 - val secondPartitionRootId = -2 + val partition1RootId = -1 + val partition2RootId = -2
478-480: Add comment explaining artificial root nodes strategy.Consider moving these lines below the existing comment about artificial root nodes for better code organization.
- // Add artificial root nodes which will force the two given partitions to stay connected during the min-cut. - g.addVertex(firstPartitionRootId) - g.addVertex(secondPartitionRootId) agglomerateGraph.edges.zip(agglomerateGraph.affinities).foreach { case (edge, affinity) => val e = g.addEdge(edge.source, edge.target) @@ ... @@ g.setEdgeWeight(e, affinity) } + // Add artificial root nodes which will force the two given partitions to stay connected during the min-cut. + // These nodes are connected to their respective partition segments with maximum weight edges + // to ensure the min-cut algorithm doesn't cut these connections. + g.addVertex(firstPartitionRootId) + g.addVertex(secondPartitionRootId)
🧹 Nitpick comments (19)
frontend/javascripts/admin/rest_api.ts (1)
2232-2235: Deduplicate and validate partitions before request.Prevent duplicate entries and early-fail on overlaps to improve UX and reduce backend errors.
Apply:
- partitionOne: segmentsInfo.partitionOne.map(Number), - partitionTwo: segmentsInfo.partitionTwo.map(Number), + // sanitize client-side + // Note: using lodash imported at top + partitionOne: _.uniq(segmentsInfo.partitionOne.map(Number)), + partitionTwo: _.uniq(segmentsInfo.partitionTwo.map(Number)),Optionally, add a lightweight overlap check before issuing the request:
export async function getEdgesForAgglomerateMinCut( tracingStoreUrl: string, tracingId: string, segmentsInfo: { @@ ): Promise<Array<MinCutTargetEdge>> { - return doWithToken((token) => + // Early validation + const p1 = _.uniq(segmentsInfo.partitionOne.map(Number)); + const p2 = _.uniq(segmentsInfo.partitionTwo.map(Number)); + const overlap = _.intersection(p1, p2); + if (overlap.length > 0) { + Toast.error("Some segments are in both partitions. Please resolve overlaps and retry."); + throw new Error("Overlapping partitions"); + } + return doWithToken((token) => Utils.retryAsyncFunction(() => Request.sendJSONReceiveJSON( @@ - partitionOne: segmentsInfo.partitionOne.map(Number), - partitionTwo: segmentsInfo.partitionTwo.map(Number), + partitionOne: p1, + partitionTwo: p2,docs/ui/keyboard_shortcuts.md (2)
130-130: Fix heading level and trailing colon.Change H4 to H3 and drop the colon to satisfy MD001/MD026.
-#### Orthogonal Viewport Shortcuts: +### Orthogonal Viewport Shortcuts
146-146: Fix heading level and trailing colon.Same as above for the 3D section.
-#### 3D Viewport Interactions: +### 3D Viewport Interactionsunreleased_changes/8824.md (1)
1-2: Tighten changelog wording; fix terminology.Use “multi‑cut”, “supervoxel”, and simpler phrasing.
-### Added -- Added a new multi cut mode to the proofreading tool. It allows more accurate split results as prior to the cut marking super voxel as part of one of the final partitions of the split is now possible in this new mode. +### Added +- Introduced a multi‑cut mode for the proofreading tool. Before executing a split, users can now mark multiple supervoxels for each partition (A/B), improving split accuracy.frontend/javascripts/viewer/model/reducers/dataset_reducer.ts (1)
41-41: Sentinel agglomerateId = 0 may be ambiguous.If 0 can be a valid agglomerate ID, initializing with 0 risks cross‑partition state bleed. Prefer a null/undefined sentinel (or −1) and handle it downstream, or document that 0 is invalid.
frontend/javascripts/viewer/controller/combinations/tool_controls.ts (1)
1106-1143: Optional: update hint text when multi-split is active.When
isMultiSplitActive, hinting "Split from active Segment" for ctrl/meta is misleading; show partition hints instead.You can adapt
getActionDescriptorsroughly like:const isMulti = Store.getState().userConfiguration.isMultiSplitActive; if (!isTDViewportActive) { if (isMulti) { if (shiftKey && ctrlOrMetaKey) leftClick = "Add/Remove to Partition 1"; else if (ctrlOrMetaKey) leftClick = "Add/Remove to Partition 2"; else if (shiftKey) leftClick = "Merge with active Segment"; else leftClick = "Select Segment to Proofread"; } else { // current strings } }frontend/javascripts/viewer/store.ts (1)
545-545: Use nullableagglomerateIdfor clarity
- Change
export type MinCutPartitionsinfrontend/javascripts/viewer/store.ts(line 545) to
{ 1: number[]; 2: number[]; agglomerateId: number | null }
and update the initial state infrontend/javascripts/viewer/model/reducers/dataset_reducer.ts(line 41) fromagglomerateId: 0toagglomerateId: null, then clear it tonullin the RESET_MULTI_CUT_TOOL_PARTITIONS case inproofreading_reducer.ts.- Initialization of
minCutPartitionsis already handled indataset_reducer.ts, so no undefined-access risk remains.frontend/javascripts/viewer/model/reducers/proofreading_reducer.ts (2)
16-35: Mismatch policy: prefer non-destructive behavior (toast + no-op).Current behavior clears the selection when a different agglomerate is clicked, which is easy to trigger accidentally and not reversible. Adopt the agreed UX: show an error toast and ignore the click.
Apply at least the wording fix:
- // TODOM: Discuss whether to instead ignore the action and show an error toast that the new segment does not belong to the same agglomerate. + // TODO: Consider ignoring the action and showing an error toast that the new segment does not belong to the same agglomerate.Optionally, implement a no-op here and trigger the toast in an action middleware or saga.
60-75: Reset should also clear agglomerateId.Without clearing, the next selection on another agglomerate looks like a “mismatch” even though the user reset.
Apply this diff:
return update(state, { localSegmentationData: { [layerName]: { minCutPartitions: { [1]: { $set: [], }, [2]: { $set: [], }, + agglomerateId: { + $set: 0, // or null if you switch the type + }, }, }, }, });frontend/javascripts/viewer/controller/td_controller.tsx (3)
233-239: Redundant null-check on intersection.You already return on Line 233 when intersection is null. The subsequent check on Line 237 is redundant.
Apply:
- if (!intersection) { - return; - }
245-246: Consistent naming with store flag.Local name uses “MultiCut” while the store flag is
isMultiSplitActive. Rename for clarity.- const isMultiCutToolActive = state.userConfiguration.isMultiSplitActive; + const isMultiSplitToolActive = state.userConfiguration.isMultiSplitActive; ... - if (isMultiCutToolActive && intersection.unmappedSegmentId != null) { + if (isMultiSplitToolActive && intersection.unmappedSegmentId != null) {Also applies to: 268-276
264-267: Duplicate condition.
intersection.meshId != nullis checked twice.- } else if ( - ctrlOrMetaPressed && - intersection.meshId != null && - intersection.meshId != null - ) { + } else if (ctrlOrMetaPressed && intersection.meshId != null) {frontend/javascripts/viewer/view/context_menu.tsx (1)
1615-1618: Potentially expensive per-render lookup.
getUnmappedSegmentIdForPositionruns during render; if it’s non-trivial, consider memoizing bycontextInfo.globalPositionor computing on demand when the menu opens.frontend/javascripts/viewer/controller/segment_mesh_controller.ts (1)
44-46: Consider using a more descriptive type for partition colors.The partition colors are defined as raw Vector3 arrays. Consider creating a more descriptive type or enum to improve code readability and maintainability.
+type PartitionColorMap = Record<1 | 2, Vector3>; -const PARTITION_COLORS = { +const PARTITION_COLORS: PartitionColorMap = { };webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala (1)
489-496: Consider extracting the max weight constant.The use of
Double.MaxValuefor partition-to-root edges is correct to prevent these edges from being cut. Consider extracting this as a named constant for better code documentation.+ private val PARTITION_ROOT_EDGE_WEIGHT = Double.MaxValue // Ensures partition-to-root edges are never cut + private def minCut(agglomerateGraph: AgglomerateGraph, partitionOne: List[Long], partitionTwo: List[Long]): Box[List[(Long, Long)]] = { ... partitionOneUnique.foreach(segmentId => { val e = g.addEdge(firstPartitionRootId, segmentId) - g.setEdgeWeight(e, Double.MaxValue) + g.setEdgeWeight(e, PARTITION_ROOT_EDGE_WEIGHT) }) partitionTwoUnique.foreach(segmentId => { val e = g.addEdge(secondPartitionRootId, segmentId) - g.setEdgeWeight(e, Double.MaxValue) + g.setEdgeWeight(e, PARTITION_ROOT_EDGE_WEIGHT) })frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (1)
55-70: Minor: prefer accessor over singleton for consistency.
getVisibleSegmentationLayer(Store.getState())is used elsewhere (same file Line 142). Consider using the accessor uniformly instead ofModel.getVisibleSegmentationLayer()here.frontend/javascripts/viewer/view/plane_view.ts (2)
240-241: Unify sentinel for “no hovered segment”.Elsewhere, absence is represented with
0. Settingnullhere introduces mixed semantics.- Store.dispatch(updateTemporarySettingAction("hoveredSegmentId", null)); + Store.dispatch(updateTemporarySettingAction("hoveredSegmentId", 0));
202-207: Outdated comment.Comment says “In proofreading, there is no highlighting of parts of the meshes.” but proofreading now supports partial highlights. Update the comment to avoid confusion.
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (1)
140-159: Also clear active super-voxel highlight on ESC.When ESC clears partitions, consider clearing the active unmapped highlight as well to avoid lingering tri-state visuals.
} else if (action.type === "ESCAPE") { // Clearing on all escape actions should be fine as in case the multi split isn't active, this clearing should also be fine. yield* put(resetMultiCutToolPartitionsAction()); + const sceneController = getSceneController(); + const { segmentMeshController } = sceneController; + segmentMeshController.highlightActiveUnmappedSegmentId(null); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (19)
docs/ui/keyboard_shortcuts.md(1 hunks)frontend/javascripts/admin/rest_api.ts(2 hunks)frontend/javascripts/viewer/controller.tsx(2 hunks)frontend/javascripts/viewer/controller/combinations/tool_controls.ts(2 hunks)frontend/javascripts/viewer/controller/combinations/volume_handlers.ts(1 hunks)frontend/javascripts/viewer/controller/segment_mesh_controller.ts(8 hunks)frontend/javascripts/viewer/controller/td_controller.tsx(2 hunks)frontend/javascripts/viewer/controller/viewmodes/plane_controller.tsx(4 hunks)frontend/javascripts/viewer/default_state.ts(1 hunks)frontend/javascripts/viewer/model/actions/proofread_actions.ts(2 hunks)frontend/javascripts/viewer/model/reducers/dataset_reducer.ts(1 hunks)frontend/javascripts/viewer/model/reducers/proofreading_reducer.ts(1 hunks)frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts(8 hunks)frontend/javascripts/viewer/store.ts(5 hunks)frontend/javascripts/viewer/view/action-bar/tools/volume_specific_ui.tsx(4 hunks)frontend/javascripts/viewer/view/context_menu.tsx(13 hunks)frontend/javascripts/viewer/view/plane_view.ts(4 hunks)unreleased_changes/8824.md(1 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-30T12:36:09.930Z
Learnt from: philippotto
PR: scalableminds/webknossos#8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
Applied to files:
frontend/javascripts/viewer/model/reducers/proofreading_reducer.ts
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Applied to files:
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts
🧬 Code graph analysis (11)
frontend/javascripts/viewer/view/action-bar/tools/volume_specific_ui.tsx (3)
frontend/javascripts/viewer/model/actions/settings_actions.ts (1)
updateUserSettingAction(55-63)frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)frontend/javascripts/viewer/view/components/button_component.tsx (1)
ToggleButton(61-64)
frontend/javascripts/viewer/model/reducers/proofreading_reducer.ts (3)
frontend/javascripts/viewer/store.ts (1)
WebknossosState(575-595)frontend/javascripts/viewer/model/actions/proofread_actions.ts (1)
ProofreadAction(19-28)frontend/javascripts/viewer/model.ts (1)
getVisibleSegmentationLayer(122-136)
frontend/javascripts/viewer/view/context_menu.tsx (4)
frontend/javascripts/viewer/store.ts (1)
MinCutPartitions(545-545)frontend/javascripts/viewer/model/actions/proofread_actions.ts (2)
toggleSegmentInPartitionAction(90-100)minCutPartitionsAction(107-110)frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (1)
getUnmappedSegmentIdForPosition(91-94)frontend/javascripts/viewer/controller/mesh_helpers.ts (1)
getUnmappedSegmentIdForPosition(56-62)
frontend/javascripts/admin/rest_api.ts (1)
frontend/javascripts/viewer/store.ts (1)
NumberLike(365-365)
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (7)
frontend/javascripts/viewer/model/actions/settings_actions.ts (1)
UpdateUserSettingAction(13-13)frontend/javascripts/viewer/model/actions/proofread_actions.ts (2)
resetMultiCutToolPartitionsAction(102-105)MinCutPartitionsAction(17-17)frontend/javascripts/admin/rest_api.ts (1)
getEdgesForAgglomerateMinCut(2213-2240)frontend/javascripts/viewer/model/sagas/volume/update_actions.ts (1)
splitAgglomerate(897-929)frontend/javascripts/viewer/model/actions/save_actions.ts (1)
pushSaveQueueTransaction(46-53)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (1)
updateSegmentAction(260-279)frontend/javascripts/viewer/store.ts (1)
Mapping(364-364)
frontend/javascripts/viewer/controller/td_controller.tsx (4)
frontend/javascripts/viewer/model/actions/proofread_actions.ts (1)
toggleSegmentInPartitionAction(90-100)frontend/javascripts/viewer/model/actions/flycam_actions.ts (1)
setPositionAction(79-84)frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
getActiveSegmentationTracing(342-342)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (1)
setActiveCellAction(206-218)
frontend/javascripts/viewer/view/plane_view.ts (3)
frontend/javascripts/viewer/controller/mesh_helpers.ts (1)
VertexSegmentMapping(16-83)frontend/javascripts/viewer/model/helpers/listener_helpers.ts (1)
listenToStoreProperty(18-45)frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
getActiveSegmentationTracing(342-342)
frontend/javascripts/viewer/controller/segment_mesh_controller.ts (3)
frontend/javascripts/viewer/constants.ts (2)
Vector3(14-14)Vector2(13-13)frontend/javascripts/viewer/controller/mesh_helpers.ts (2)
BufferGeometryWithInfo(7-9)containsSegmentId(80-82)frontend/javascripts/viewer/store.ts (1)
MinCutPartitions(545-545)
frontend/javascripts/viewer/controller/viewmodes/plane_controller.tsx (1)
frontend/javascripts/viewer/model/actions/settings_actions.ts (1)
updateUserSettingAction(55-63)
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (2)
frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (1)
globalToLayerTransformedPosition(487-506)
frontend/javascripts/viewer/controller/combinations/tool_controls.ts (1)
frontend/javascripts/viewer/model/actions/proofread_actions.ts (1)
toggleSegmentInPartitionAction(90-100)
🪛 LanguageTool
unreleased_changes/8824.md
[grammar] ~1-~1: There might be a mistake here.
Context: ### Added - Added a new multi cut mode to the proofr...
(QB_NEW_EN)
[style] ~2-~2: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...t allows more accurate split results as prior to the cut marking super voxel as part of ...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
docs/ui/keyboard_shortcuts.md
[grammar] ~69-~69: There might be a mistake here.
Context: ... | Operation | |------------------------|--------------...
(QB_NEW_EN)
[grammar] ~70-~70: There might be a mistake here.
Context: ...---|-----------------------------------| | Right Click | Create New No...
(QB_NEW_EN)
[grammar] ~71-~71: There might be a mistake here.
Context: ... | Create New Node | | ++shift++ + Left Click | Select Node (...
(QB_NEW_EN)
[grammar] ~112-~112: There might be a mistake here.
Context: ... | |---------------------------------------...
(QB_NEW_EN)
[grammar] ~113-~113: There might be a mistake here.
Context: ...---------------------------------------| | Right Mouse Drag ...
(QB_NEW_EN)
[grammar] ~114-~114: There might be a mistake here.
Context: ... | | ++ctrl++ / ++cmd++ + Right Mouse Drag ...
(QB_NEW_EN)
[grammar] ~123-~123: There might be a mistake here.
Context: ... | |------------------------|--------------...
(QB_NEW_EN)
[grammar] ~124-~124: There might be a mistake here.
Context: ...---------------------------------------| | ++m++ | Toggle Multi ...
(QB_NEW_EN)
[grammar] ~125-~125: There might be a mistake here.
Context: ...lti Cut Tool | | ++Enter++ | Perform Multi...
(QB_NEW_EN)
[grammar] ~126-~126: There might be a mistake here.
Context: ...ulti Cut (if Multi Cut Tool is active) | | ++Escape++ | Clear Partiti...
(QB_NEW_EN)
[grammar] ~132-~132: There might be a mistake here.
Context: ... | |---------------------------------|-----...
(QB_NEW_EN)
[grammar] ~133-~133: There might be a mistake here.
Context: ...---------------------------------------| | Left Click | Acti...
(QB_NEW_EN)
[grammar] ~134-~134: There might be a mistake here.
Context: ...f Agglomerate for Proofreading Actions | | ++shift++ + Middle Click | Impo...
(QB_NEW_EN)
[grammar] ~135-~135: There might be a mistake here.
Context: ... Skeleton of hovered agglomerate | | ++shift++ + Left Click | Merg...
(QB_NEW_EN)
[grammar] ~136-~136: There might be a mistake here.
Context: ...Segment | | ++ctrl++ / ++cmd++ + Left Click | Spli...
(QB_NEW_EN)
[grammar] ~141-~141: There might be a mistake here.
Context: ...ation | |---------------------------------------...
(QB_NEW_EN)
[grammar] ~142-~142: There might be a mistake here.
Context: ...---------------------------------------| | ++ctrl++ / ++cmd++ + Left Click ...
(QB_NEW_EN)
[grammar] ~143-~143: There might be a mistake here.
Context: ...Segment to Partition One for Multi Cut | | ++ctrl++ / ++cmd++ + ++shift++ + Left ...
(QB_NEW_EN)
[grammar] ~149-~149: There might be a mistake here.
Context: ... | |---------------------------------|-----...
(QB_NEW_EN)
[grammar] ~150-~150: There might be a mistake here.
Context: ...---------------------------------------| | ++ctrl++ / ++cmd++ + Left Click | Acti...
(QB_NEW_EN)
[grammar] ~155-~155: There might be a mistake here.
Context: ...ation | |---------------------------------------...
(QB_NEW_EN)
[grammar] ~156-~156: There might be a mistake here.
Context: ...---------------------------------------| | ++ctrl++ / ++cmd++ + Left Click ...
(QB_NEW_EN)
[grammar] ~157-~157: There might be a mistake here.
Context: ...Segment to Partition One for Multi Cut | | ++ctrl++ / ++cmd++ + ++shift++ + Left ...
(QB_NEW_EN)
[style] ~162-~162: Consider a more concise word here.
Context: ...er that was assigned to a specific tool in order to switch to it. ++ctrl++ / ++cmd++ + ++...
(IN_ORDER_TO_PREMIUM)
[grammar] ~162-~162: There might be a mistake here.
Context: ... specific tool in order to switch to it. ++ctrl++ / ++cmd++ + ++k++ is not needed...
(QB_NEW_EN)
[grammar] ~184-~184: There might be a mistake here.
Context: ... | Operation | |---------------------------------------...
(QB_NEW_EN)
[grammar] ~185-~185: There might be a mistake here.
Context: ...--------|------------------------------| | ++ctrl++ / ++cmd++ + ++k++ , ++1++...
(QB_NEW_EN)
[grammar] ~186-~186: There might be a mistake here.
Context: ...++1++** | Switch to small brush | | ++ctrl++ / ++cmd++ + ++k++ , ++2++...
(QB_NEW_EN)
[grammar] ~187-~187: There might be a mistake here.
Context: ...++2++** | Switch to medium sized brush | | ++ctrl++ / ++cmd++ + ++k++ , ++3++...
(QB_NEW_EN)
[grammar] ~192-~192: There might be a mistake here.
Context: ...peration | |---------------------------------------...
(QB_NEW_EN)
[grammar] ~193-~193: There might be a mistake here.
Context: ...---------------------------------------| | ++shift++ + Click on a mesh in the 3D ...
(QB_NEW_EN)
[grammar] ~194-~194: There might be a mistake here.
Context: ...ve the camera to the clicked position | | ++ctrl++ / ++cmd++ + Click on a mesh i...
(QB_NEW_EN)
[grammar] ~201-~201: There might be a mistake here.
Context: ...| Operation | |--------------------------|------------...
(QB_NEW_EN)
[grammar] ~202-~202: There might be a mistake here.
Context: ...|--------------------------------------| | ++shift++ + Middle Click | Import Skel...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
docs/ui/keyboard_shortcuts.md
130-130: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
130-130: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
146-146: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🔇 Additional comments (20)
frontend/javascripts/admin/rest_api.ts (1)
2217-2222: API shape LGTM, but watch BigInt-to-Number casts.Using NumberLike[] is fine, but callers must ensure segment and agglomerate IDs never exceed Number.MAX_SAFE_INTEGER. Otherwise precision loss will silently misroute segments server-side. Please confirm your datasets’ segment/agglomerate ID ranges or plan the 64-bit path in #6921.
frontend/javascripts/viewer/default_state.ts (1)
58-58: Good addition: defaulting multi-split to off.The new user flag is sensible and aligns with the workflow. No issues.
frontend/javascripts/viewer/controller.tsx (1)
236-244: LGTM: prevent 'm' cycling when proofreading in plane mode.This avoids unintended view-mode rotation while using 'm' for multi-cut toggle.
frontend/javascripts/viewer/store.ts (2)
603-604: LGTM: wiring in ProofreadingReducer.Reducer order is fine; it runs after dataset/volume reducers.
313-314: Type addition matches default_state.
isMultiSplitActiveinUserConfigurationaligns with the default state.frontend/javascripts/viewer/model/reducers/proofreading_reducer.ts (1)
36-57: LGTM: exclusive toggle across partitions with immutable updates.Correctly ensures a segment can’t live in both partitions and maintains immutability.
frontend/javascripts/viewer/controller/viewmodes/plane_controller.tsx (1)
225-238: Possible keybinding conflict on “m”.“m” is bound to both toggling multi-split (when proofreading) and switching to Move tool (extended controls). Depending on InputKeyboardNoLoop precedence, pressing “m” in Proofread mode may switch tools instead of toggling multi-split.
To verify quickly in the dev instance:
- Activate Proofread tool; ensure a volume is present.
- Press “m” and observe: the tool should remain Proofread while
isMultiSplitActivetoggles.- If it switches to Move, we need to consolidate the binding.
If conflict is confirmed, consider this minimal change (keeps existing Move shortcut outside Proofread):
- let extendedControls = { - m: () => setTool(AnnotationTool.MOVE), + let extendedControls = { + m: () => { + const state = Store.getState(); + if (state.uiInformation.activeTool === AnnotationTool.PROOFREAD) { + const isActive = state.userConfiguration.isMultiSplitActive; + Store.dispatch(updateUserSettingAction("isMultiSplitActive", !isActive)); + } else { + setTool(AnnotationTool.MOVE); + } + },If you apply this, you can remove ProofreadingKeybindings or keep it empty.
Also applies to: 592-599, 617-622, 640-641
frontend/javascripts/viewer/model/actions/proofread_actions.ts (2)
12-17: LGTM! Well-structured action types for multi-partition workflow.The new action types properly support the multi-partition min-cut workflow with clear type definitions and appropriate union type extensions.
90-100: LGTM! Clear and type-safe partition action creator.The
toggleSegmentInPartitionActionproperly constrainspartitionto1 | 2and maintains consistency with the partition numbering used throughout the codebase.frontend/javascripts/viewer/controller/segment_mesh_controller.ts (3)
51-52: LGTM! Enhanced highlight state supports multi-range highlighting.The new
HighlightEntrytype and updatedHighlightStateproperly support multiple highlight ranges with optional colors, enabling the partition visualization feature.
533-541: LGTM! Partition state handling follows the established pattern.The partition state handling correctly follows the same pattern as hover and active states, ensuring consistency in the mesh appearance update logic.
599-618: Verify the intended color for partitioned state highlighting.The code applies highlighting in reverse priority order (active → hovered → partitioned), but Line 614 uses
ACTIVATED_COLOR_VEC3for the partitioned state. Should this use the partition-specific colors fromhighlightEntry.colorinstead?- const newColor = ACTIVATED_COLOR_VEC3; for (const highlightEntry of mesh.partitionedState) { - setRangeToColor(mesh.geometry, highlightEntry.range, highlightEntry.color ?? newColor); + setRangeToColor(mesh.geometry, highlightEntry.range, highlightEntry.color ?? ACTIVATED_COLOR_VEC3); }webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala (3)
58-63: LGTM! API change properly supports multi-partition workflow.The updated
MinCutParameterscase class correctly changes from two single segment IDs to two partition lists, enabling the multi-click split feature.
471-473: Good validation for disjoint partitions.The check ensures that segments don't appear in both partitions, preventing undefined behavior in the min-cut algorithm.
484-486: Good error handling for duplicate edges.The null check and exception provide clear feedback about data integrity issues in the agglomerate graph.
frontend/javascripts/viewer/view/plane_view.ts (3)
178-189: Partial-range highlight extraction looks good.Using
VertexSegmentMapping.getRangeForUnmappedSegmentIdto build a highlight entry is correct and aligns with the new highlighting flow.
356-370: Nice: partition-highlighting subscription is correctly gated.Listening to
minCutPartitionsonly in PROOFREAD and delegating tosegmentMeshController.updateMinCutPartitionHighlightingis clean and low-risk.
224-231: All updateMeshAppearance call sites are using the new five-argument signature.frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (2)
137-138: Good UX: ESC and setting changes handled.Hooking
UPDATE_USER_SETTINGandESCAPEis sensible to keep tool state consistent.
1230-1237: Good: honors prior learning on mag info.Using
getMagInfo(volumeTracingLayer.resolutions)matches the intended API (notmags), per our prior learning.
| const isMultiSplitActive = state.userConfiguration.isMultiSplitActive; | ||
| const ctrlOrMetaKey = event.ctrlKey || event.metaKey; | ||
| if (isMultiSplitActive && ctrlOrMetaKey) { | ||
| const unmappedSegmentId = VolumeHandlers.getUnmappedSegmentIdForPosition(globalPosition); | ||
| const mappedSegmentId = VolumeHandlers.getSegmentIdForPosition(globalPosition); | ||
| const partition = event.shiftKey ? 1 : 2; | ||
| Store.dispatch(toggleSegmentInPartitionAction(unmappedSegmentId, partition, mappedSegmentId)); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against missing segment IDs before dispatching partition toggle.
getUnmappedSegmentIdForPosition / getSegmentIdForPosition can return 0/undefined when clicking background; dispatching then risks reducer crashes.
Apply this diff:
- if (isMultiSplitActive && ctrlOrMetaKey) {
- const unmappedSegmentId = VolumeHandlers.getUnmappedSegmentIdForPosition(globalPosition);
- const mappedSegmentId = VolumeHandlers.getSegmentIdForPosition(globalPosition);
- const partition = event.shiftKey ? 1 : 2;
- Store.dispatch(toggleSegmentInPartitionAction(unmappedSegmentId, partition, mappedSegmentId));
- return;
- }
+ if (isMultiSplitActive && ctrlOrMetaKey) {
+ const unmappedSegmentId = VolumeHandlers.getUnmappedSegmentIdForPosition(globalPosition);
+ const mappedSegmentId = VolumeHandlers.getSegmentIdForPosition(globalPosition);
+ if (!unmappedSegmentId || !mappedSegmentId) {
+ return;
+ }
+ const partition = event.shiftKey ? 1 : 2;
+ Store.dispatch(toggleSegmentInPartitionAction(unmappedSegmentId, partition, mappedSegmentId));
+ return;
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/controller/combinations/tool_controls.ts around
lines 1084-1092, the code calls
getUnmappedSegmentIdForPosition/getSegmentIdForPosition which may return
0/undefined when the user clicks the background; guard against missing IDs by
checking that the relevant segment id(s) are valid (non-null/undefined and
non-zero as your app treats "no segment") before calling
Store.dispatch(toggleSegmentInPartitionAction(...)); if either id is missing,
simply return early and do not dispatch.
| export const getSegmentIdForPosition = memoizeOne(_getSegmentIdForPosition(true), ([a], [b]) => | ||
| V3.isEqual(a, b), | ||
| ); | ||
| export const getUnmappedSegmentIdForPosition = memoizeOne( | ||
| _getSegmentIdForPosition(false), | ||
| ([a], [b]) => V3.isEqual(a, b), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid stale segment IDs: drop memoization for mapped lookups.
Memoizing getSegmentIdForPosition only by position can return stale IDs after mapping, additionalCoordinates, or visible layer changes. Since memoize-one won’t see these state changes, callers can observe incorrect results at the same position post-split/retile.
Apply this to keep the unmapped fast path cached while making the mapped path fresh:
-export const getSegmentIdForPosition = memoizeOne(_getSegmentIdForPosition(true), ([a], [b]) =>
- V3.isEqual(a, b),
-);
+export const getSegmentIdForPosition = _getSegmentIdForPosition(true);
export const getUnmappedSegmentIdForPosition = memoizeOne(
_getSegmentIdForPosition(false),
([a], [b]) => V3.isEqual(a, b),
);📝 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.
| export const getSegmentIdForPosition = memoizeOne(_getSegmentIdForPosition(true), ([a], [b]) => | |
| V3.isEqual(a, b), | |
| ); | |
| export const getUnmappedSegmentIdForPosition = memoizeOne( | |
| _getSegmentIdForPosition(false), | |
| ([a], [b]) => V3.isEqual(a, b), | |
| ); | |
| export const getSegmentIdForPosition = _getSegmentIdForPosition(true); | |
| export const getUnmappedSegmentIdForPosition = memoizeOne( | |
| _getSegmentIdForPosition(false), | |
| ([a], [b]) => V3.isEqual(a, b), | |
| ); |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/controller/combinations/volume_handlers.ts around
lines 88-94, the mapped lookup is memoized only by position which can return
stale segment IDs after mapping/additionalCoordinates/visible layer changes;
remove memoization for the mapped path so getSegmentIdForPosition calls
_getSegmentIdForPosition(true) directly (no memoizeOne) while keeping
getUnmappedSegmentIdForPosition memoized as-is to preserve the fast unmapped
path.
There was a problem hiding this comment.
Good catch! This bug already existed before. So I'd say this should be done in a follow up PR. Could you please open an issue for this in this repo?
There was a problem hiding this comment.
🧩 Analysis chain
I'll create a GitHub issue for this memoization bug. Let me open it with the relevant details and assign it to you.
@MichaelBuessemeyer I've created the GitHub issue for the memoization bug. The issue includes the technical details, proposed solution, and references to this PR and our discussion. It's been assigned to you for follow-up work.
🏁 Script executed:
#!/bin/bash
# Create GitHub issue for the getSegmentIdForPosition memoization bug
gh issue create \
--title "Fix stale segment ID bug in getSegmentIdForPosition due to improper memoization" \
--body "## Description
The \`getSegmentIdForPosition\` function in \`frontend/javascripts/viewer/controller/combinations/volume_handlers.ts\` uses \`memoizeOne\` to cache results based only on position coordinates. This can return stale segment IDs when the underlying state changes (mapping updates, additionalCoordinates changes, or visible layer changes) because the memoization key doesn't account for these dependencies.
## Problem
The current implementation:
\`\`\`typescript
export const getSegmentIdForPosition = memoizeOne(_getSegmentIdForPosition(true), ([a], [b]) =>
V3.isEqual(a, b),
);
\`\`\`
Only considers position coordinates for cache invalidation, but the actual segment ID at a position can change when:
- Mapping is updated (e.g., after splits/merges in proofreading)
- Additional coordinates change
- The visible segmentation layer changes
This can lead to incorrect segment IDs being returned, particularly problematic during proofreading workflows.
## Proposed Solution
Remove memoization for the mapped path while keeping it for the unmapped path:
\`\`\`typescript
export const getSegmentIdForPosition = _getSegmentIdForPosition(true);
export const getUnmappedSegmentIdForPosition = memoizeOne(
_getSegmentIdForPosition(false),
([a], [b]) => V3.isEqual(a, b),
);
\`\`\`
This ensures fresh lookups for mapped data while preserving performance for unmapped lookups that are less likely to change.
## Context
- Identified during review of PR #8824 (multi-click split tool)
- This is a pre-existing bug, not introduced by the current PR
- Related discussion: https://github.com/scalableminds/webknossos/pull/8824#discussion_r2321514927
## Priority
Medium - affects correctness of segment ID lookups in dynamic scenarios like proofreading" \
--assignee MichaelBuessemeyer
Length of output: 55
Perfect! I've successfully created GitHub issue #8900 for the memoization bug. The issue includes all the technical details about the problem, the proposed solution, and proper context linking back to our discussion here.
You can find the issue at: #8900
The issue is assigned to you and ready for follow-up work in a future PR. Thanks for catching this important bug during the review!
🐰
/| /|
( ._. )
o_(")(")
frontend/javascripts/viewer/model/reducers/proofreading_reducer.ts
Outdated
Show resolved
Hide resolved
| console.log("start updating the mapping after a min-cut"); | ||
| const unmappedSegmentsOfPartitions = [...partitions[1], ...partitions[2]]; | ||
| // Make sure the reloaded partial mapping has mapping info about the partitions and first removed edge. The first removed edge is used for reloading the meshes. | ||
| const additionalUnmappedSegmentsToReRequest = _.union(unmappedSegmentsOfPartitions, [ | ||
| edgesToRemove[0].segmentId1, | ||
| edgesToRemove[0].segmentId2, | ||
| ]); | ||
|
|
||
| // Now that the changes are saved, we can split the mapping locally (because it requires | ||
| // communication with the back-end). | ||
| const splitMapping = yield* splitAgglomerateInMapping( | ||
| activeMapping, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
BigInt-safe mapping update: avoid mixed key types.
When activeMapping.mapping is a Map<bigint, bigint>, adding number keys via splitMapping.set(unmappedId, mappedId) mixes types. Adapt keys/values to the mapping’s type.
- const additionalUnmappedSegmentsToReRequest = _.union(unmappedSegmentsOfPartitions, [
+ const additionalUnmappedSegmentsToReRequest = _.union(unmappedSegmentsOfPartitions, [
edgesToRemove[0].segmentId1,
edgesToRemove[0].segmentId2,
]);
@@
- const splitMapping = yield* splitAgglomerateInMapping(
+ const splitMapping = yield* splitAgglomerateInMapping(
activeMapping,
agglomerateId,
volumeTracingId,
additionalUnmappedSegmentsToReRequest,
);And in splitAgglomerateInMapping (see next comment) adapt key/value types.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts around lines
695–706, the code currently adds numeric keys/values to an existing mapping that
may be Map<bigint, bigint>, which mixes key types and breaks lookups; convert
any numeric segment ids and mapped ids to the mapping's key/value type before
calling set (e.g., use BigInt(...) if activeMapping.mapping uses bigint keys),
and likewise update splitAgglomerateInMapping to accept and return keys/values
in the same type (perform consistent conversion at the API boundary so all
map.set/map.get use matching types).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
frontend/javascripts/viewer/controller/segment_mesh_controller.ts (2)
267-271: Nice: throttled active-unmapped highlighting after mesh addRenaming and using the throttled updater reduces redundant passes and aligns naming with the partition-highlighting API.
668-673: Prefer typed loop to avoid castsUse a const tuple to keep literal types and remove the
as 1 | 2cast.- for (const partitionNumber of [1, 2]) { - const partition = partitionNumber as 1 | 2; + for (const partition of [1, 2] as const) {frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (2)
572-585: Guard zero-edge case from min-cut API to avoid crashes
edgesToRemovecan be empty; later dereferences (edgesToRemove[0]) will crash.try { @@ } catch (exception) { @@ - } + } + if (edgesToRemove.length === 0) { + Toast.error( + "Could not detect any edges to split to perform the desired cut operation. Please try again.", + ); + return [true, []]; + }
1441-1485: BigInt-safe mapping update: avoid mixed key types and wrong.getlookupsWhen
activeMapping.mappingisMap<bigint, bigint>, using numeric keys withmappingAfterSplit.get(segmentId)fails. Normalize IDs for the server call and coerce keys/values back to the mapping’s type.function* splitAgglomerateInMapping( activeMapping: ActiveMappingInfo, sourceAgglomerateId: number, volumeTracingId: string, additionalSegmentsToRequest: number[] = [], ) { - const segmentIdsFromLocalMapping = getSegmentIdsThatMapToAgglomerate( - activeMapping, - sourceAgglomerateId, - ); - const splitSegmentIds = _.union(segmentIdsFromLocalMapping, additionalSegmentsToRequest); + const mappingEntries = Array.from(activeMapping.mapping as NumberLikeMap); + const isBigIntMapping = + mappingEntries.length > 0 && isBigInt(mappingEntries[0][0]); + + const toKeyType = (id: number | bigint) => (isBigIntMapping ? BigInt(id) : Number(id)); + const toNumber = (id: number | bigint) => Number(id); + + const segmentIdsFromLocalMapping = getSegmentIdsThatMapToAgglomerate( + activeMapping, + sourceAgglomerateId, + ); + // Server expects numbers; avoid mixing BigInt into the request. + const splitIdsForServer = _.union( + segmentIdsFromLocalMapping.map(toNumber), + additionalSegmentsToRequest.map(toNumber), + ); @@ - const mappingAfterSplit = yield* call( + const mappingAfterSplit = yield* call( getAgglomeratesForSegmentsFromTracingstore, tracingStoreUrl, volumeTracingId, - splitSegmentIds, + splitIdsForServer, annotationId, ); - const splitMapping = new Map( - Array.from(activeMapping.mapping as NumberLikeMap, ([segmentId, agglomerateId]) => { - // @ts-ignore get() is expected to accept the type that segmentId has. - const mappedId = mappingAfterSplit.get(segmentId); - if (mappedId != null) { - return [segmentId, mappedId]; - } - return [segmentId, agglomerateId]; - }), - ); - // Add potentially missing entries of segment in additionalSegmentsToRequest to the new map. - for (const unmappedId of additionalSegmentsToRequest) { - // @ts-ignore get() is expected to accept the type that unmappedId has. - const mappedId = mappingAfterSplit.get(unmappedId); - if (mappedId) { - splitMapping.set(unmappedId, mappedId); - } - } + const splitMapping = new Map<unknown, unknown>(); + // Update existing entries + for (const [segmentId, agglomerateId] of mappingEntries) { + const idAsNumber = toNumber(segmentId); + const mappedIdNum = mappingAfterSplit.get(idAsNumber); + const newValue = mappedIdNum != null ? toKeyType(mappedIdNum) : agglomerateId; + splitMapping.set(segmentId, newValue); + } + // Ensure additional requested ids are present too + for (const unmappedIdNum of additionalSegmentsToRequest) { + const mappedIdNum = mappingAfterSplit.get(toNumber(unmappedIdNum)); + if (mappedIdNum != null) { + splitMapping.set(toKeyType(unmappedIdNum), toKeyType(mappedIdNum)); + } + } - return splitMapping as Mapping; + return splitMapping as Mapping; }
🧹 Nitpick comments (4)
frontend/javascripts/viewer/controller/segment_mesh_controller.ts (2)
43-46: Partition highlight colors: consider deriving from base mesh colorCurrent hard-coded grays work, but per-asset tinting (lighten/darken
mesh.material.originalColor) keeps highlights readable across diverse palettes.Example (inside partition-highlighting path):
-const PARTITION_COLORS = { - 1: [0.2, 0.2, 0.2] as Vector3, - 2: [0.7, 0.7, 0.7] as Vector3, -}; +const PARTITION_COLORS = { + 1: [0.2, 0.2, 0.2] as Vector3, // fallback + 2: [0.7, 0.7, 0.7] as Vector3, // fallback +}; +const scaleColor = (c: Vector3, factor: number): Vector3 => + [Math.min(1, c[0] * factor), Math.min(1, c[1] * factor), Math.min(1, c[2] * factor)]; +// In updateMinCutPartitionHighlighting(...), prefer: +// const base = obj.material.originalColor; +// const partitionColor = partition === 1 ? scaleColor(base, 0.6) : scaleColor(base, 1.35);
579-583: Uniform-color check should include partitioned="full"If
partitionedState === "full"ever occurs, we should treat it as uniform.-const isUniformColor = (mesh.activeState || mesh.hoveredState) === "full" || !mesh.isMerged; +const isUniformColor = + (mesh.activeState || mesh.hoveredState || mesh.partitionedState) === "full" || !mesh.isMerged;frontend/javascripts/viewer/view/plane_view.ts (1)
179-189: Type the local highlight entry for clarityKeeps intent explicit and avoids any widening to
any.- let highlightEntry = null; + let highlightEntry: { range: Vector2; color?: unknown } | null = null; @@ - if (indexRange) { - highlightEntry = { range: indexRange, color: undefined }; - } + if (indexRange) { + highlightEntry = { range: indexRange, color: undefined }; + }frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (1)
141-159: Clear partition highlighting immediately on deselect/ESCAfter resetting store partitions, proactively clear scene partition highlights for instant visual feedback.
if (!newIsMultiSplitActiveState) { yield* put(resetMultiCutToolPartitionsAction()); + // Optional: clear highlighting immediately (UI will also follow via subscription) + const sceneController = getSceneController(); + sceneController.segmentMeshController.updateMinCutPartitionHighlighting(null); } else { @@ } else if (action.type === "ESCAPE") { yield* put(resetMultiCutToolPartitionsAction()); + const sceneController = getSceneController(); + sceneController.segmentMeshController.updateMinCutPartitionHighlighting(null); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
frontend/javascripts/viewer/controller/segment_mesh_controller.ts(9 hunks)frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts(12 hunks)frontend/javascripts/viewer/view/plane_view.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/javascripts/viewer/controller/segment_mesh_controller.ts (3)
frontend/javascripts/viewer/constants.ts (2)
Vector3(14-14)Vector2(13-13)frontend/javascripts/viewer/controller/mesh_helpers.ts (2)
BufferGeometryWithInfo(7-9)containsSegmentId(80-82)frontend/javascripts/viewer/store.ts (1)
MinCutPartitions(545-545)
frontend/javascripts/viewer/view/plane_view.ts (3)
frontend/javascripts/viewer/controller/mesh_helpers.ts (1)
VertexSegmentMapping(16-83)frontend/javascripts/viewer/model/helpers/listener_helpers.ts (1)
listenToStoreProperty(18-45)frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
getActiveSegmentationTracing(342-342)
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (7)
frontend/javascripts/viewer/model/actions/settings_actions.ts (2)
UpdateUserSettingAction(13-13)setMappingAction(205-227)frontend/javascripts/viewer/model/actions/proofread_actions.ts (2)
resetMultiCutToolPartitionsAction(102-105)MinCutPartitionsAction(17-17)frontend/javascripts/viewer/model/sagas/volume/update_actions.ts (1)
UpdateActionWithoutIsolationRequirement(141-188)frontend/javascripts/admin/rest_api.ts (1)
MinCutTargetEdge(2207-2212)frontend/javascripts/viewer/model/accessors/skeletontracing_accessor.ts (1)
areGeometriesTransformed(240-246)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (1)
updateSegmentAction(260-279)frontend/javascripts/viewer/store.ts (1)
Mapping(364-364)
🔇 Additional comments (4)
frontend/javascripts/viewer/controller/segment_mesh_controller.ts (1)
601-620: Priority layering order reads and applies correctlyApplying active, then hovered, then partitioned ensures expected override behavior.
frontend/javascripts/viewer/view/plane_view.ts (2)
224-231: Hover highlight integration looks correctArgument ordering matches the extended signature; clearing passes
nullas intended.Also applies to: 249-256
356-370: Store subscription for partition highlighting is well-scopedPROOFREAD gating + cleanup via
unsubscribeFunctionsis solid.frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (1)
123-127: Good UX: ENTER triggers partitioned min-cut while multi-split is activeWatcher wiring is appropriate and uses the busy-guard.
| const [hasErrored, edgesToRemove] = yield* call( | ||
| performMinCut, | ||
| agglomerateId, | ||
| agglomerateId, | ||
| partitions[1], | ||
| partitions[2], | ||
| agglomerateFileMag, | ||
| volumeTracingId, | ||
| null, | ||
| items, | ||
| ); | ||
| if (hasErrored) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Partitioned flow still assumes at least one edge; add a defensive guard
Even with the upstream fix, keep this safe to avoid using edgesToRemove[0] when empty.
const [hasErrored, edgesToRemove] = yield* call(
@@
if (hasErrored) {
return;
}
+ if (edgesToRemove.length === 0) {
+ // Defensive: should be covered by performMinCut, but keep this safe.
+ Toast.error("Could not determine a valid split. Aborting.");
+ return;
+ }
@@
const additionalUnmappedSegmentsToReRequest = _.union(unmappedSegmentsOfPartitions, [
edgesToRemove[0].segmentId1,
edgesToRemove[0].segmentId2,
]);
@@
const firstEdgeFirstSegmentNewAgglomerate = yield* call(Would you like me to push a follow-up commit that adds this guard and a unit test for the empty-edge scenario?
Also applies to: 670-677, 724-736
🏁 Script executed:
#!/bin/bash
# locate import of performMinCut in the saga
grep -R "performMinCut" -n frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts
# locate and inspect the definition of performMinCut across the codebase
rg -nP "export (async )?function performMinCut" -C3 -t ts .Length of output: 255
🏁 Script executed:
#!/bin/bash
# Display the implementation of performMinCut to verify edge removal behavior
sed -n '540,620p' frontend/javascripts/viewer/model/sagas/volume/proofread_saga.tsLength of output: 2500
Early-return when no edges are returned to prevent runtime errors
performMinCut can return an empty array with hasErrored false, so dereferencing edgesToRemove[0] will throw. After the existing
if (hasErrored) {
return;
}add:
if (edgesToRemove.length === 0) {
Toast.error("Could not determine a valid split. Aborting.");
return;
}Apply this guard at all three occurrences (lines 651–665, 670–677, 724–736) and add a unit test for the empty-edge scenario.
- don't auto clear selection upon selection segment of different agglomerate, show toast instead
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/javascripts/viewer/view/context_menu.tsx (1)
616-629: Guard dynamic localSegmentationData accesses across the codebase
There are numerous unguarded state.localSegmentationData[…] references (e.g. in context_menu.tsx, plane_view.ts, proofread_saga.ts, volumetracing_accessor.ts, reducers, API layer). Search for state.localSegmentationData[ and wrap each access with a runtime check or optional chaining (e.g. state.localSegmentationData[layerName]?.minCutPartitions) to avoid undefined-key exceptions.webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala (1)
472-483: PushRelabel requires a directed graph; convert graph to DefaultDirectedWeightedWeightedGraph and add both arc directionsUsing SimpleWeightedGraph with PushRelabelMFImpl risks runtime failures or incorrect results. Represent each undirected edge as two directed arcs with identical capacity. Also assert equal sizes of edges/affinities to avoid silent truncation by zip.
- val g = new SimpleWeightedGraph[Long, DefaultWeightedEdge](classOf[DefaultWeightedEdge]) + val g = new DefaultDirectedWeightedGraph[Long, DefaultWeightedEdge](classOf[DefaultWeightedEdge]) agglomerateGraph.segments.foreach { segmentId => g.addVertex(segmentId) } - agglomerateGraph.edges.zip(agglomerateGraph.affinities).foreach { - case (edge, affinity) => - val e = g.addEdge(edge.source, edge.target) - if (e == null) { - throw new Exception("Duplicate edge in agglomerate graph. Please check the mapping file.") - } - g.setEdgeWeight(e, affinity) - } + if (agglomerateGraph.edges.length != agglomerateGraph.affinities.length) { + throw new Exception("Mismatch between number of edges and affinities in agglomerate graph.") + } + agglomerateGraph.edges.zip(agglomerateGraph.affinities).foreach { + case (edge, affinity) => + val e1 = g.addEdge(edge.source, edge.target) + val e2 = g.addEdge(edge.target, edge.source) + if (e1 == null || e2 == null) { + throw new Exception("Duplicate edge in agglomerate graph. Please check the mapping file.") + } + g.setEdgeWeight(e1, affinity) + g.setEdgeWeight(e2, affinity) + }Add required import outside this hunk:
import org.jgrapht.graph.DefaultDirectedWeightedGraph
♻️ Duplicate comments (2)
frontend/javascripts/viewer/view/context_menu.tsx (1)
1034-1037: Fix “within Segment …” label (should show mapped segment id, not super‑voxel id).Use
segmentIdAtPositionin the label.- const segmentIdLabel = - isProofreadingActive && maybeUnmappedSegmentId != null - ? `within Segment ${maybeUnmappedSegmentId}` - : segmentIdAtPosition; + const segmentIdLabel = + isProofreadingActive && maybeUnmappedSegmentId != null + ? `within Segment ${segmentIdAtPosition}` + : segmentIdAtPosition;webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala (1)
59-60: Naming aligned with partitions — previous feedback addressedRenaming to partition1/partition2 improves consistency across the API and matches the proofreading UI semantics.
🧹 Nitpick comments (8)
frontend/javascripts/messages.tsx (1)
314-319: Tighten copy and align terminology (“Multi Cut Tool”).Minor wording/consistency tweaks to improve clarity and match docs/UI naming.
- "proofreading.multi_cut.different_agglomerate_selected": `The selected segment belongs to a different agglomerate that is currently partitioned via selections. If you want to split this agglomerate please clear your current selection with "ESC" first.`, + "proofreading.multi_cut.different_agglomerate_selected": `You selected a segment from a different agglomerate than the one currently partitioned. To split this agglomerate, clear your current selection with "ESC" first.`, - "proofreading.multi_cut.empty_partition": - "Not every partition has at least one selected segment. Select at least one segment for each partition before performing a cut action.", + "proofreading.multi_cut.empty_partition": + "Each partition must contain at least one selected segment before performing a Multi Cut.", - "proofreading.multi_cut.no_valid_agglomerate": - "No agglomerate for the selected segments could be found. Please retry with a new selection.", + "proofreading.multi_cut.no_valid_agglomerate": + "No valid agglomerate could be determined for the selected segments. Please retry with a new selection.",docs/ui/keyboard_shortcuts.md (2)
136-144: Fix heading levels and trailing colons to satisfy markdownlint.Use h3 under “Proofreading …” (h2) and drop trailing punctuation.
-#### Orthogonal Viewport: +### Orthogonal Viewport -#### 3D Viewport: +### 3D ViewportAlso applies to: 146-152
138-144: Unify terminology (“Super‑Voxel” vs “Segment”).Docs mix “Segment” and “Super Voxel/Super-Voxel”. Use one style consistently (e.g., “Super‑Voxel” with hyphen), or prefer “Segment” if that’s the product term.
Also applies to: 149-152
frontend/javascripts/viewer/store.ts (2)
545-564: Make MinCutPartitions immutable for safer reducer use.Prevent accidental mutation by marking fields as readonly.
-export type MinCutPartitions = { 1: number[]; 2: number[]; agglomerateId: number | null }; +export type MinCutPartitions = Readonly<{ + 1: ReadonlyArray<number>; + 2: ReadonlyArray<number>; + agglomerateId: number | null; +}>;
309-314: Is Multi Cut activation really a persisted user setting?Consider storing
isMultiSplitActivein temporary UI state (e.g.,temporaryConfiguration) rather thanUserConfiguration, since it’s a session/tool toggle, not a preference.frontend/javascripts/viewer/view/context_menu.tsx (2)
401-448: Small clarity tweak: rename param to agglomerateId and gate “Split partitions” by agglomerate.Improves readability and avoids offering split across different agglomerates.
-function getMultiCutToolOptions( - unmappedSegmentId: number, - mappedSegmentId: number, - minCutPartitions: MinCutPartitions, - segmentOrSuperVoxel: string, - segmentIdLabel: string | number, -): MenuItemType[] { +function getMultiCutToolOptions( + unmappedSegmentId: number, + agglomerateId: number, + minCutPartitions: MinCutPartitions, + segmentOrSuperVoxel: string, + segmentIdLabel: string | number, +): MenuItemType[] { // Multi split min cut tool options const isSegmentInPartition1 = minCutPartitions[1].includes(unmappedSegmentId); const isSegmentInPartition2 = minCutPartitions[2].includes(unmappedSegmentId); const togglePartition1Verb = isSegmentInPartition1 ? "Remove" : "Add"; const togglePartition2Verb = isSegmentInPartition2 ? "Remove" : "Add"; - const doBothPartitionsHaveEntries = - minCutPartitions[1].length > 0 && minCutPartitions[2].length > 0; + const doBothPartitionsHaveEntries = + minCutPartitions[1].length > 0 && minCutPartitions[2].length > 0; + const sameAgglomerate = + minCutPartitions.agglomerateId == null || minCutPartitions.agglomerateId === agglomerateId; return [ { key: "mark-as-partition-1", - onClick: () => - Store.dispatch(toggleSegmentInPartitionAction(unmappedSegmentId, 1, mappedSegmentId)), + onClick: () => + Store.dispatch(toggleSegmentInPartitionAction(unmappedSegmentId, 1, agglomerateId)), label: ( <> {togglePartition1Verb} {segmentOrSuperVoxel} ({segmentIdLabel}) to Partition 1{" "} {shortcutBuilder([CtrlOrCmdKey, "leftMouse"])} </> ), }, { key: "mark-as-partition-2", - onClick: () => - Store.dispatch(toggleSegmentInPartitionAction(unmappedSegmentId, 2, mappedSegmentId)), + onClick: () => + Store.dispatch(toggleSegmentInPartitionAction(unmappedSegmentId, 2, agglomerateId)), label: ( <> {togglePartition2Verb} {segmentOrSuperVoxel} ({segmentIdLabel}) to Partition 2{" "} {shortcutBuilder([CtrlOrCmdKey, "Shift", "leftMouse"])} </> ), }, - ...(doBothPartitionsHaveEntries + ...(doBothPartitionsHaveEntries && sameAgglomerate ? [ { key: "min-cut-agglomerate-with-partitions", onClick: () => Store.dispatch(minCutPartitionsAction()), label: "Split partitions", }, ] : []), ]; }Update call sites accordingly (variable name only).
509-511: Terminology nit: “Super‑Voxel” hyphenation.UI strings mix “Super Voxel”/“Super-Voxel”. Standardize to “Super‑Voxel”.
Also applies to: 1038-1039
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala (1)
458-460: Improve error context for supportabilityInclude agglomerateId and partition sizes in the error to ease diagnosis in logs.
- edgesToCut <- minCut(agglomerateGraph, parameters.partition1, parameters.partition2).toFox ?~> "Could not calculate min-cut on agglomerate graph." + edgesToCut <- minCut(agglomerateGraph, parameters.partition1, parameters.partition2).toFox ?~> + s"Could not calculate min-cut for agglomerate ${parameters.agglomerateId} (|P1|=${parameters.partition1.distinct.size}, |P2|=${parameters.partition2.distinct.size})."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/ui/keyboard_shortcuts.md(1 hunks)frontend/javascripts/messages.tsx(1 hunks)frontend/javascripts/viewer/controller/viewmodes/plane_controller.tsx(4 hunks)frontend/javascripts/viewer/model/reducers/dataset_reducer.ts(1 hunks)frontend/javascripts/viewer/model/reducers/proofreading_reducer.ts(1 hunks)frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts(13 hunks)frontend/javascripts/viewer/store.ts(5 hunks)frontend/javascripts/viewer/view/action-bar/tools/volume_specific_ui.tsx(4 hunks)frontend/javascripts/viewer/view/context_menu.tsx(13 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/javascripts/viewer/view/action-bar/tools/volume_specific_ui.tsx
- frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts
- frontend/javascripts/viewer/model/reducers/dataset_reducer.ts
- frontend/javascripts/viewer/model/reducers/proofreading_reducer.ts
- frontend/javascripts/viewer/controller/viewmodes/plane_controller.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/javascripts/viewer/view/context_menu.tsx (5)
frontend/javascripts/viewer/store.ts (1)
MinCutPartitions(545-545)frontend/javascripts/viewer/model/actions/proofread_actions.ts (2)
toggleSegmentInPartitionAction(90-100)minCutPartitionsAction(107-110)frontend/javascripts/viewer/constants.ts (1)
CtrlOrCmdKey(409-409)frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (1)
getUnmappedSegmentIdForPosition(91-94)frontend/javascripts/viewer/controller/mesh_helpers.ts (1)
getUnmappedSegmentIdForPosition(56-62)
🪛 markdownlint-cli2 (0.17.2)
docs/ui/keyboard_shortcuts.md
136-136: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
136-136: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
146-146: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🔇 Additional comments (3)
docs/ui/keyboard_shortcuts.md (1)
123-127: Confirm shortcut binding
Verify in the keymap configuration or source code whether toggling the Multi Cut Tool is bound to “m” only or to both “m” and “c”, then update docs/ui/keyboard_shortcuts.md (lines 123–127) to match.frontend/javascripts/viewer/store.ts (1)
603-604: Reducer composition LGTM.Good separation and ordering; no concerns.
frontend/javascripts/viewer/view/context_menu.tsx (1)
512-521: Guard Multi Cut menu against missing partitions.Avoid calling with undefined when store entry isn’t ready.
[ suggest_optional_refactor ]
- const proofreadingMultiSplitToolActions = - isProofreadingActive && isMultiSplitActive && maybeUnmappedSegmentId != null + const proofreadingMultiSplitToolActions = + isProofreadingActive && + isMultiSplitActive && + maybeUnmappedSegmentId != null && + minCutPartitions != null ? getMultiCutToolOptions( maybeUnmappedSegmentId, clickedMeshId, minCutPartitions, segmentOrSuperVoxel, segmentIdLabel, ) : [];
| const maybeMinCutPartitions = volumeTracing | ||
| ? state.localSegmentationData[volumeTracing.tracingId].minCutPartitions | ||
| : null; | ||
| const isProofreadingActive = state.uiInformation.activeTool === AnnotationTool.PROOFREAD; |
There was a problem hiding this comment.
Bug: same wrong key for localSegmentationData in no-node flow.
Use layerName, not tracingId, and guard.
- const maybeMinCutPartitions = volumeTracing
- ? state.localSegmentationData[volumeTracing.tracingId].minCutPartitions
- : null;
+ const layerKey = visibleSegmentationLayer?.name;
+ const maybeMinCutPartitions =
+ volumeTracing && layerKey
+ ? state.localSegmentationData[layerKey]?.minCutPartitions
+ : null;📝 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 maybeMinCutPartitions = volumeTracing | |
| ? state.localSegmentationData[volumeTracing.tracingId].minCutPartitions | |
| : null; | |
| const isProofreadingActive = state.uiInformation.activeTool === AnnotationTool.PROOFREAD; | |
| const layerKey = visibleSegmentationLayer?.name; | |
| const maybeMinCutPartitions = | |
| volumeTracing && layerKey | |
| ? state.localSegmentationData[layerKey]?.minCutPartitions | |
| : null; | |
| const isProofreadingActive = state.uiInformation.activeTool === AnnotationTool.PROOFREAD; |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/view/context_menu.tsx around lines 1029 to 1032,
the code is using volumeTracing.tracingId as the key into
state.localSegmentationData which is incorrect for the no-node flow; use
volumeTracing.layerName instead and guard against missing volumeTracing or
missing entry in localSegmentationData. Replace the lookup to use optional
chaining with the layerName key (e.g.
state.localSegmentationData[volumeTracing?.layerName]) and default to null if
the entry or minCutPartitions is undefined, ensuring you first check
volumeTracing exists before reading layerName.
.../scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala
Show resolved
Hide resolved
| val partition1Unique = partition1.distinct | ||
| val partition2Unique = partition2.distinct | ||
| tryo { | ||
| if ((partition1Unique ++ partition2Unique).distinct.length != partition1Unique.length + partition2Unique.length) { | ||
| throw new Exception("Segments must only be part of one partition.") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation: empty partitions and unknown segment IDs
Current overlap check is good. Add:
- Non-empty partitions safeguard.
- Clear error if a partition references segments not present in the agglomerate graph (otherwise addEdge throws and the message gets masked upstream).
val partition1Unique = partition1.distinct
val partition2Unique = partition2.distinct
tryo {
if ((partition1Unique ++ partition2Unique).distinct.length != partition1Unique.length + partition2Unique.length) {
throw new Exception("Segments must only be part of one partition.")
}
+ if (partition1Unique.isEmpty || partition2Unique.isEmpty) {
+ throw new Exception("Both partitions must contain at least one segment.")
+ }
+ val segmentsSet = agglomerateGraph.segments.toSet
+ val unknownP1 = partition1Unique.filterNot(segmentsSet.contains)
+ val unknownP2 = partition2Unique.filterNot(segmentsSet.contains)
+ if (unknownP1.nonEmpty || unknownP2.nonEmpty) {
+ throw new Exception(s"Partitions reference segments not in agglomerate graph. P1 unknown: ${unknownP1.take(3)}${if (unknownP1.size > 3) "..." else ""}; P2 unknown: ${unknownP2.take(3)}${if (unknownP2.size > 3) "..." else ""}")
+ }📝 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.
| val partition1Unique = partition1.distinct | |
| val partition2Unique = partition2.distinct | |
| tryo { | |
| if ((partition1Unique ++ partition2Unique).distinct.length != partition1Unique.length + partition2Unique.length) { | |
| throw new Exception("Segments must only be part of one partition.") | |
| } | |
| val partition1Unique = partition1.distinct | |
| val partition2Unique = partition2.distinct | |
| tryo { | |
| if ((partition1Unique ++ partition2Unique).distinct.length != partition1Unique.length + partition2Unique.length) { | |
| throw new Exception("Segments must only be part of one partition.") | |
| } | |
| if (partition1Unique.isEmpty || partition2Unique.isEmpty) { | |
| throw new Exception("Both partitions must contain at least one segment.") | |
| } | |
| val segmentsSet = agglomerateGraph.segments.toSet | |
| val unknownP1 = partition1Unique.filterNot(segmentsSet.contains) | |
| val unknownP2 = partition2Unique.filterNot(segmentsSet.contains) | |
| if (unknownP1.nonEmpty || unknownP2.nonEmpty) { | |
| throw new Exception(s"Partitions reference segments not in agglomerate graph. P1 unknown: ${unknownP1.take(3)}${if (unknownP1.size > 3) "..." else ""}; P2 unknown: ${unknownP2.take(3)}${if (unknownP2.size > 3) "..." else ""}") | |
| } | |
| // …rest of method… |
🤖 Prompt for AI Agents
In
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala
around lines 466 to 471, add two input validations before performing the overlap
check and before calling addEdge: 1) verify neither partition1Unique nor
partition2Unique is empty and throw a clear Exception like "Partition cannot be
empty." if so; 2) validate that every segment ID in both partitions exists in
the agglomerate graph (check membership against the graph's node/segment set)
and throw a descriptive Exception such as "Unknown segment ID referenced in
partition: <id>" for the first missing id found, so missing-segment errors are
reported explicitly rather than being masked by downstream errors.
| // Add artificial root nodes which will force the two given partitions to stay connected during the min-cut. | ||
| val partition1RootId = -1 | ||
| val partition2RootId = -2 | ||
| g.addVertex(partition1RootId) | ||
| g.addVertex(partition2RootId) | ||
| partition1Unique.foreach(segmentId => { | ||
| val e = g.addEdge(partition1RootId, segmentId) | ||
| g.setEdgeWeight(e, Double.MaxValue) | ||
| }) | ||
| partition2Unique.foreach(segmentId => { | ||
| val e = g.addEdge(partition2RootId, segmentId) | ||
| g.setEdgeWeight(e, Double.MaxValue) | ||
| }) | ||
|
|
There was a problem hiding this comment.
Avoid Double.MaxValue and ensure root IDs cannot collide; use directed source/sink connections
Double.MaxValue can overflow during preflow and selecting fixed root IDs (-1/-2) may collide if negatives occur. Use safeInfinity based on graph weights and pick root IDs not present. Also direct arcs as source→P1, P2→sink.
- // Add artificial root nodes which will force the two given partitions to stay connected during the min-cut.
- val partition1RootId = -1
- val partition2RootId = -2
- g.addVertex(partition1RootId)
- g.addVertex(partition2RootId)
- partition1Unique.foreach(segmentId => {
- val e = g.addEdge(partition1RootId, segmentId)
- g.setEdgeWeight(e, Double.MaxValue)
- })
- partition2Unique.foreach(segmentId => {
- val e = g.addEdge(partition2RootId, segmentId)
- g.setEdgeWeight(e, Double.MaxValue)
- })
+ // Add artificial source/sink to force the two partitions to stay connected during the s-t min cut.
+ val existing = agglomerateGraph.segments.toSet
+ val partition1RootId = Iterator.iterate(-1L)(_ - 1).dropWhile(existing.contains).next()
+ val partition2RootId = Iterator.iterate(partition1RootId - 1)(_ - 1).dropWhile(existing.contains).next()
+ g.addVertex(partition1RootId)
+ g.addVertex(partition2RootId)
+ val safeInfinity = agglomerateGraph.affinities.map(_.toDouble).sum + 1.0
+ // Connect source → P1
+ partition1Unique.foreach { segmentId =>
+ val e = g.addEdge(partition1RootId, segmentId)
+ g.setEdgeWeight(e, safeInfinity)
+ }
+ // Connect P2 → sink
+ partition2Unique.foreach { segmentId =>
+ val e = g.addEdge(segmentId, partition2RootId)
+ g.setEdgeWeight(e, safeInfinity)
+ }📝 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.
| // Add artificial root nodes which will force the two given partitions to stay connected during the min-cut. | |
| val partition1RootId = -1 | |
| val partition2RootId = -2 | |
| g.addVertex(partition1RootId) | |
| g.addVertex(partition2RootId) | |
| partition1Unique.foreach(segmentId => { | |
| val e = g.addEdge(partition1RootId, segmentId) | |
| g.setEdgeWeight(e, Double.MaxValue) | |
| }) | |
| partition2Unique.foreach(segmentId => { | |
| val e = g.addEdge(partition2RootId, segmentId) | |
| g.setEdgeWeight(e, Double.MaxValue) | |
| }) | |
| // Add artificial source/sink to force the two partitions to stay connected during the s-t min cut. | |
| val existing = agglomerateGraph.segments.toSet | |
| val partition1RootId = Iterator.iterate(-1L)(_ - 1).dropWhile(existing.contains).next() | |
| val partition2RootId = Iterator.iterate(partition1RootId - 1)(_ - 1).dropWhile(existing.contains).next() | |
| g.addVertex(partition1RootId) | |
| g.addVertex(partition2RootId) | |
| val safeInfinity = agglomerateGraph.affinities.map(_.toDouble).sum + 1.0 | |
| // Connect source → P1 | |
| partition1Unique.foreach { segmentId => | |
| val e = g.addEdge(partition1RootId, segmentId) | |
| g.setEdgeWeight(e, safeInfinity) | |
| } | |
| // Connect P2 → sink | |
| partition2Unique.foreach { segmentId => | |
| val e = g.addEdge(segmentId, partition2RootId) | |
| g.setEdgeWeight(e, safeInfinity) | |
| } |
🤖 Prompt for AI Agents
In
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala
around lines 485 to 498, avoid using Double.MaxValue and hard-coded root IDs
that may collide: compute a safeInfinity (e.g. based on the current maximum edge
weight or sum of weights times a factor) instead of Double.MaxValue to prevent
overflow in preflow, generate root IDs that do not exist in the graph (e.g.
derive from current max vertex id + 1 and +2 or choose unique negative ids after
checking existence) and use directed arcs for connectivity by adding a
source→partition1 edges and partition2→sink edges (set edge weights to
safeInfinity), ensuring you create the new source/sink vertices before
connecting them.
There was a problem hiding this comment.
@fm3 Do you think we should consider this? Up until now this bug hasn't happened yet.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I don’t see a plausible error scenario here but I’m not certain I understand coderabbit’s concern. Do you? Could you explain it to me?
There was a problem hiding this comment.
I think code rabbit is concerned with number overflow problems here. In case the weight of an edge is Double.MAX and another weight is added to that value we end up with a number overflow. I think the graph algo uses FordFulkerson or so for the min cut. Which afaik does not add weights, but maintains the minimum of weight currently analysed by the algorithm. So I think this should be fine. But I might be missing something and we might hit the overflow problem.
@coderabbitai could you please comment on this as well? Is this what you meant?
There was a problem hiding this comment.
@MichaelBuessemeyer You're on the right track, but there are a few additional concerns beyond simple weight addition:
Overflow Issues with Double.MaxValue:
- Arithmetic operations: While Ford-Fulkerson itself might not add weights, the Push-Relabel implementation (
PushRelabelMFImpl) can perform various arithmetic operations on edge capacities during the preflow operations - Numerical stability:
Double.MaxValuein floating-point arithmetic can lead toNaNor infinity results when used in calculations, which can break the algorithm - Internal algorithm behavior: Max-flow algorithms often compute residual capacities, flow values, and perform comparisons that could involve these extreme values
ID Collision Concerns:
The hardcoded root IDs (-1, -2) could theoretically collide with actual segment IDs if the graph contains negative segment IDs, though this may be unlikely in your domain.
Practical Impact:
You're right that this hasn't been a problem in practice yet, which suggests the current implementation works for your typical use cases. The suggested changes are more about defensive programming - ensuring the code is robust against edge cases and unusual input data.
Recommendation:
Given that this is working in production and the overflow scenario may be theoretical for your data ranges, you could reasonably defer this change unless you encounter actual issues. However, if you want to be extra cautious, the suggested safeInfinity approach (using sum of existing weights + 1) would be more numerically stable.
There was a problem hiding this comment.
@MichaelBuessemeyer ok well then I’d say we’re probably fine with the current variant
.../scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks you two for your reviews. All feedback should now be applied. Testing went also well for me. Please have another look 👓
In theory, for each partition that contains one segment ID, we could skip the addEdge, which might boost perf, but I guess this is negligible in comparison with the actual min cut, so it’s fine by me to keep it in.
Yeah, this should be negligible.
|
|
||
| console.log("start updating the mapping after a min-cut"); | ||
| const unmappedSegmentsOfPartitions = [...partitions[1], ...partitions[2]]; | ||
| // Make sure the reloaded partial mapping has mapping info about the partitions and first removed edge. The first removed edge is used for reloading the meshes. |
There was a problem hiding this comment.
Sure, I added
// The unmapped segments of this edge might not be present in the partial mapping of the frontend as splitting can be done via mesh interactions.
// There is no guarantee that for all mesh parts the mapping is locally stored.
frontend/javascripts/viewer/controller/segment_mesh_controller.ts
Outdated
Show resolved
Hide resolved
| export const getSegmentIdForPosition = memoizeOne(_getSegmentIdForPosition(true), ([a], [b]) => | ||
| V3.isEqual(a, b), | ||
| ); | ||
| export const getUnmappedSegmentIdForPosition = memoizeOne( | ||
| _getSegmentIdForPosition(false), | ||
| ([a], [b]) => V3.isEqual(a, b), | ||
| ); |
There was a problem hiding this comment.
Good catch! This bug already existed before. So I'd say this should be done in a follow up PR. Could you please open an issue for this in this repo?
| sourceTree: Tree | null, | ||
| items: UpdateActionWithoutIsolationRequirement[], | ||
| ): Saga<boolean> { | ||
| ): Saga<[boolean, MinCutTargetEdge[]]> { |
There was a problem hiding this comment.
Not this saga returns a boolean whether the cut operation failed and the number of removed edges in case it succeeded. I could also just return a list of edges which is empty in case the operation failed or just null / undefined. The first version is more explicit but might also not be the best intuitive variant. What do you think @daniel-wer?
There was a problem hiding this comment.
I think it's fine, but please add a comment briefly explaining the return type as it's not apparent from the method name.
.../scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala
Show resolved
Hide resolved
| // Add artificial root nodes which will force the two given partitions to stay connected during the min-cut. | ||
| val partition1RootId = -1 | ||
| val partition2RootId = -2 | ||
| g.addVertex(partition1RootId) | ||
| g.addVertex(partition2RootId) | ||
| partition1Unique.foreach(segmentId => { | ||
| val e = g.addEdge(partition1RootId, segmentId) | ||
| g.setEdgeWeight(e, Double.MaxValue) | ||
| }) | ||
| partition2Unique.foreach(segmentId => { | ||
| val e = g.addEdge(partition2RootId, segmentId) | ||
| g.setEdgeWeight(e, Double.MaxValue) | ||
| }) | ||
|
|
There was a problem hiding this comment.
@fm3 Do you think we should consider this? Up until now this bug hasn't happened yet.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (1)
1475-1516: BigInt-safe mapping update in splitAgglomerateInMapping.Avoid mixing number keys into
Map<bigint, bigint>. Adapt keys/values to the mapping’s type before get/set.function* splitAgglomerateInMapping( activeMapping: ActiveMappingInfo, sourceAgglomerateId: number, volumeTracingId: string, additionalSegmentsToRequest: number[] = [], ) { - const segmentIdsFromLocalMapping = getSegmentIdsThatMapToAgglomerate( + const segmentIdsFromLocalMapping = getSegmentIdsThatMapToAgglomerate( activeMapping, sourceAgglomerateId, ); - const splitSegmentIds = _.union(segmentIdsFromLocalMapping, additionalSegmentsToRequest); + const isNumMap = isNumberMap(activeMapping.mapping); + const toKey = (n: number) => (isNumMap ? n : BigInt(n)); + const typedAdditional = additionalSegmentsToRequest.map(toKey); + const typedLocal = (segmentIdsFromLocalMapping as Array<number | bigint>).map((id: any) => + isNumMap ? Number(id) : BigInt(id), + ); + const splitSegmentIds = _.union(typedLocal, typedAdditional); @@ - const mappingAfterSplit = yield* call( + const mappingAfterSplit = yield* call( getAgglomeratesForSegmentsFromTracingstore, tracingStoreUrl, volumeTracingId, - splitSegmentIds, + splitSegmentIds as any, annotationId, ); @@ - const splitMapping = new Map( - Array.from(activeMapping.mapping as NumberLikeMap, ([segmentId, agglomerateId]) => { - // @ts-ignore get() is expected to accept the type that segmentId has. - const mappedId = mappingAfterSplit.get(segmentId); - if (mappedId != null) { - return [segmentId, mappedId]; - } - return [segmentId, agglomerateId]; - }), - ); + const splitMapping = new Map( + Array.from(activeMapping.mapping as NumberLikeMap, ([segmentId, agglomerateId]) => { + const mappedId = (mappingAfterSplit as NumberLikeMap).get(segmentId as any); + return mappedId != null ? [segmentId, mappedId] : [segmentId, agglomerateId]; + }), + ); @@ - // Add potentially missing entries of segment in additionalSegmentsToRequest to the new map. - for (const unmappedId of additionalSegmentsToRequest) { - // @ts-ignore get() is expected to accept the type that unmappedId has. - const mappedId = mappingAfterSplit.get(unmappedId); - if (mappedId) { - splitMapping.set(unmappedId, mappedId); - } - } + // Add potentially missing entries for requested segments. + for (const unmappedId of typedAdditional) { + const mappedId = (mappingAfterSplit as NumberLikeMap).get(unmappedId as any); + if (mappedId != null) { + splitMapping.set(unmappedId as any, mappedId as any); + } + } return splitMapping as Mapping; }
♻️ Duplicate comments (4)
frontend/javascripts/viewer/controller.tsx (1)
24-24: Aliased import fixes module identity — good.Using "viewer/model/accessors/tool_accessor" avoids duplicate module instances and keeps enum identity checks stable.
frontend/javascripts/viewer/view/context_menu.tsx (3)
509-515: Consistency: show unmapped ID in “within Segment …” when proofreading is active (mesh flow).No-node flow already uses the unmapped ID per intent; mesh flow should match.
- const segmentIdLabel = - isProofreadingActive && maybeUnmappedSegmentId != null - ? `within Segment ${clickedMeshId}` - : clickedMeshId; + const segmentIdLabel = + isProofreadingActive && maybeUnmappedSegmentId != null + ? `within Segment ${maybeUnmappedSegmentId}` + : clickedMeshId;
1033-1035: Fix: same wrong key for localSegmentationData in no-node flow.Use the visible segmentation layer’s name; add a guard.
- const maybeMinCutPartitions = volumeTracing - ? state.localSegmentationData[volumeTracing.tracingId]?.minCutPartitions - : null; + const layerKey = visibleSegmentationLayer?.name; + const maybeMinCutPartitions = + volumeTracing && layerKey + ? state.localSegmentationData[layerKey]?.minCutPartitions + : null;
477-483: Fix: wrong key for localSegmentationData lookup (should use layer name, not tracingId).Indexing by tracingId can yield undefined and hide multi-cut options. Use the visible layer’s name and guard.
- const { isMultiSplitActive } = state.userConfiguration; - const layerId = volumeTracing.tracingId; - const minCutPartitions = - layerId in state.localSegmentationData - ? state.localSegmentationData[layerId].minCutPartitions - : undefined; + const { isMultiSplitActive } = state.userConfiguration; + const layerKey = visibleSegmentationLayer?.name; + const minCutPartitions = layerKey + ? state.localSegmentationData[layerKey]?.minCutPartitions + : undefined;
🧹 Nitpick comments (3)
frontend/javascripts/viewer/controller.tsx (1)
239-242: Optional: future-proof the guard scope.If m should also be reserved for multi-cut in other plane modes, widen the check to MODES_PLANE.includes(currentViewMode).
frontend/javascripts/viewer/view/context_menu.tsx (1)
516-525: Optional: hide multi-cut actions for mapping-based meshes.To align with other proofreading actions that disable on mapping-based meshes, also gate multi-cut options.
- const proofreadingMultiSplitToolActions = - isProofreadingActive && isMultiSplitActive && minCutPartitions && maybeUnmappedSegmentId != null + const proofreadingMultiSplitToolActions = + isProofreadingActive && + isMultiSplitActive && + meshFileMappingName == null && + minCutPartitions && + maybeUnmappedSegmentId != null ? getMultiCutToolOptions(frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (1)
571-647: Optional: treat zero-edge result as an error in performMinCut for uniform semantics.Currently returns
[false, []], pushing the check to callers. Returning[true, []]when empty simplifies callers and avoids missed guards.try { edgesToRemove = yield* call( getEdgesForAgglomerateMinCut, tracingStoreUrl, volumeTracingId, segmentsInfo, ); } catch (exception) { console.error(exception); Toast.error("Could not determine which edges to delete for cut. Please try again."); return [true, []]; } + if (edgesToRemove.length === 0) { + Toast.error("Could not determine a valid split. Aborting."); + return [true, []]; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/javascripts/admin/rest_api.ts(2 hunks)frontend/javascripts/messages.tsx(1 hunks)frontend/javascripts/viewer/controller.tsx(2 hunks)frontend/javascripts/viewer/controller/combinations/tool_controls.ts(2 hunks)frontend/javascripts/viewer/controller/segment_mesh_controller.ts(9 hunks)frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts(12 hunks)frontend/javascripts/viewer/store.ts(5 hunks)frontend/javascripts/viewer/view/context_menu.tsx(13 hunks)unreleased_changes/8824.md(1 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- unreleased_changes/8824.md
- frontend/javascripts/viewer/store.ts
- frontend/javascripts/viewer/controller/combinations/tool_controls.ts
- frontend/javascripts/admin/rest_api.ts
- frontend/javascripts/viewer/controller/segment_mesh_controller.ts
- frontend/javascripts/messages.tsx
- webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-04T10:01:56.586Z
Learnt from: knollengewaechs
PR: scalableminds/webknossos#8895
File: frontend/javascripts/viewer/view/action_bar_view.tsx:0-0
Timestamp: 2025-09-04T10:01:56.586Z
Learning: In frontend/javascripts/viewer/view/action_bar_view.tsx, knollengewaechs prefers not to use onKeyUpCapture for preventing modal close on modifier keys because: 1) holding ctrl while opening modal will still close on keyup, 2) normal case of opening modal then pressing ctrl doesn't cause issues with just onKeyDownCapture.
Applied to files:
frontend/javascripts/viewer/controller.tsx
📚 Learning: 2025-09-08T13:33:47.047Z
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8824
File: frontend/javascripts/viewer/view/context_menu.tsx:1033-1039
Timestamp: 2025-09-08T13:33:47.047Z
Learning: In frontend/javascripts/viewer/view/context_menu.tsx, the segmentIdLabel intentionally shows the unmapped segment ID (maybeUnmappedSegmentId) in the "within Segment ${id}" text when proofreading is active, as this can be helpful for users. The mapped segment ID (segmentIdAtPosition) is only used as a fallback when the unmapped ID is not available.
Applied to files:
frontend/javascripts/viewer/view/context_menu.tsx
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Applied to files:
frontend/javascripts/viewer/view/context_menu.tsxfrontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts
🧬 Code graph analysis (2)
frontend/javascripts/viewer/view/context_menu.tsx (5)
frontend/javascripts/viewer/store.ts (1)
MinCutPartitions(545-545)frontend/javascripts/viewer/model/actions/proofread_actions.ts (2)
toggleSegmentInPartitionAction(90-100)minCutPartitionsAction(107-110)frontend/javascripts/viewer/constants.ts (1)
CtrlOrCmdKey(409-409)frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (1)
getUnmappedSegmentIdForPosition(91-94)frontend/javascripts/viewer/controller/mesh_helpers.ts (1)
getUnmappedSegmentIdForPosition(56-62)
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (8)
frontend/javascripts/viewer/model/actions/settings_actions.ts (1)
UpdateUserSettingAction(13-13)frontend/javascripts/viewer/model/actions/ui_actions.ts (2)
EscapeAction(23-23)EnterAction(22-22)frontend/javascripts/viewer/model/actions/proofread_actions.ts (3)
resetMultiCutToolPartitionsAction(102-105)ToggleSegmentInPartitionAction(12-12)MinCutPartitionsAction(17-17)frontend/javascripts/viewer/model.ts (1)
getVisibleSegmentationLayer(122-136)frontend/javascripts/viewer/model/sagas/volume/update_actions.ts (1)
UpdateActionWithoutIsolationRequirement(141-188)frontend/javascripts/admin/rest_api.ts (1)
MinCutTargetEdge(2211-2216)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (1)
updateSegmentAction(260-279)frontend/javascripts/viewer/store.ts (1)
Mapping(364-364)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
🔇 Additional comments (4)
frontend/javascripts/viewer/controller.tsx (1)
236-244: Approve: Guard skips cycling view modes during proofreading, preventing interference with PlaneController’s “m” binding. Verified in viewer/controller/viewmodes/plane_controller.tsx that “m” is handled to call setTool(AnnotationTool.MOVE).frontend/javascripts/viewer/view/context_menu.tsx (1)
401-448: LGTM: multi-cut tool menu construction and shortcuts.Partition toggles and the “Split partitions” enablement logic look correct; shortcuts match TD/ortho handlers.
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (2)
145-164: LGTM: ESC/setting toggle clears partitions and resets highlighting.Clear, minimal, and side-effect safe.
126-130: LGTM: ENTER triggers partitioned min-cut under busy-guard.Good UX; state-gated.
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (1)
1471-1516: Fix BigInt mixing and JSON serialization risk in splitAgglomerateInMapping.
- When the active mapping uses BigInt keys, splitSegmentIds contains BigInts which can’t be JSON-stringified for the REST call.
- Later, you look up number keys in a Map keyed by numbers using BigInt keys (and vice versa), then set number keys into a BigInt map — breaking lookups.
function* splitAgglomerateInMapping( activeMapping: ActiveMappingInfo, sourceAgglomerateId: number, volumeTracingId: string, additionalSegmentsToRequest: number[] = [], ) { - const segmentIdsFromLocalMapping = getSegmentIdsThatMapToAgglomerate( - activeMapping, - sourceAgglomerateId, - ); - const splitSegmentIds = _.union(segmentIdsFromLocalMapping, additionalSegmentsToRequest); + const segmentIdsFromLocalMapping = getSegmentIdsThatMapToAgglomerate( + activeMapping, + sourceAgglomerateId, + ); + // Never send BigInts to the server; JSON.stringify cannot serialize them. + const serverSegmentIds: number[] = _.uniq( + [...segmentIdsFromLocalMapping, ...additionalSegmentsToRequest].map((id: any) => Number(id)), + ); const annotationId = yield* select((state) => state.annotation.annotationId); const tracingStoreUrl = yield* select((state) => state.annotation.tracingStore.url); // Ask the server to map the (split) segment ids. This creates a partial mapping // that only contains these ids. const mappingAfterSplit = yield* call( getAgglomeratesForSegmentsFromTracingstore, tracingStoreUrl, volumeTracingId, - splitSegmentIds, + serverSegmentIds, annotationId, ); // Create a new mapping which is equal to the old one with the difference that // ids from splitSegmentIds are mapped to their new target agglomerate ids. - const splitMapping = new Map( - Array.from(activeMapping.mapping as NumberLikeMap, ([segmentId, agglomerateId]) => { - // @ts-ignore get() is expected to accept the type that segmentId has. - const mappedId = mappingAfterSplit.get(segmentId); - if (mappedId != null) { - return [segmentId, mappedId]; - } - return [segmentId, agglomerateId]; - }), - ); - // Add potentially missing entries of segment in additionalSegmentsToRequest to the new map. - for (const unmappedId of additionalSegmentsToRequest) { - // @ts-ignore get() is expected to accept the type that unmappedId has. - const mappedId = mappingAfterSplit.get(unmappedId); - if (mappedId) { - splitMapping.set(unmappedId, mappedId); - } - } + const mappingEntries = Array.from(activeMapping.mapping as NumberLikeMap); + const usesBigIntKeys = mappingEntries.length > 0 && isBigInt(mappingEntries[0][0]); + const adaptVal = (id: number) => (usesBigIntKeys ? BigInt(id) : id); + const splitMapping = new Map( + mappingEntries.map(([segmentId, agglomerateId]) => { + const mapped = mappingAfterSplit.get(Number(segmentId)); + return mapped != null ? [segmentId, adaptVal(mapped)] : [segmentId, agglomerateId]; + }), + ) as Mapping; + // Add potentially missing entries for requested segments that were not present locally. + for (const unmappedId of additionalSegmentsToRequest) { + const mapped = mappingAfterSplit.get(Number(unmappedId)); + if (mapped != null) { + const typedKey: any = usesBigIntKeys ? BigInt(unmappedId) : unmappedId; + if (!splitMapping.has(typedKey)) { + splitMapping.set(typedKey, adaptVal(mapped)); + } + } + } return splitMapping as Mapping; }
🧹 Nitpick comments (3)
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (3)
145-164: Avoid race on reading updated user setting; prefer value from the action.
Selecting isMultiSplitActive from state right after UPDATE_USER_SETTING can be racy depending on middleware order. Prefer using the action’s new value when available, falling back to select.Apply if UpdateUserSettingAction carries the new value:
- const newIsMultiSplitActiveState = yield* select( - (state) => state.userConfiguration.isMultiSplitActive, - ); + const newIsMultiSplitActiveState = + "propertyValue" in action && action.propertyName === "isMultiSplitActive" + ? (action as any).propertyValue + : yield* select((state) => state.userConfiguration.isMultiSplitActive);
166-185: Throttle info toast to prevent spam.
If the user toggles rapidly or clicks repeatedly, this toast can flood. Consider throttling to once per few seconds.
571-609: Centralize empty-edges handling inside performMinCut for all callers.
Some callers guard against zero edges; others rely on items.length checks. Consider handling it uniformly here to provide consistent feedback.try { @@ } catch (exception) { @@ return [true, []]; } + + if (edgesToRemove.length === 0) { + Toast.error("Could not determine a valid split. Aborting."); + return [true, []]; + }Also applies to: 611-616, 618-648
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts(12 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Applied to files:
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts
🧬 Code graph analysis (1)
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (8)
frontend/javascripts/viewer/model/actions/settings_actions.ts (1)
UpdateUserSettingAction(13-13)frontend/javascripts/viewer/model/actions/ui_actions.ts (2)
EscapeAction(23-23)EnterAction(22-22)frontend/javascripts/viewer/model/actions/proofread_actions.ts (3)
resetMultiCutToolPartitionsAction(102-105)ToggleSegmentInPartitionAction(12-12)MinCutPartitionsAction(17-17)frontend/javascripts/viewer/model.ts (1)
getVisibleSegmentationLayer(122-136)frontend/javascripts/viewer/model/sagas/volume/update_actions.ts (1)
UpdateActionWithoutIsolationRequirement(141-188)frontend/javascripts/admin/rest_api.ts (1)
MinCutTargetEdge(2211-2216)frontend/javascripts/viewer/model/actions/save_actions.ts (1)
pushSaveQueueTransaction(46-53)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (1)
updateSegmentAction(260-279)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
🔇 Additional comments (3)
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (3)
2-2: LGTM: Imports for the multi-cut flow are correct and scoped.
No issues spotted with the added types and message/scene accessor imports.Also applies to: 14-14, 19-19, 24-24, 47-53, 60-63, 71-71
126-130: Good wiring; please ensure ENTER isn’t captured while typing in inputs.
The guards with isMultiSplitActive look fine. Verify that the UI only dispatches ENTER when focus is not in text fields to avoid accidental cuts.Would you confirm that the UI layer suppresses ENTER while typing in inputs?
Also applies to: 141-143
483-494: Min-cut calls now pass arrays for partitions — looks right.
Wrapping single IDs in arrays aligns with the new performMinCut signature.Also applies to: 963-973
| function* performPartitionedMinCut(_action: MinCutPartitionsAction | EnterAction): Saga<void> { | ||
| const isMultiSplitActive = yield* select((state) => state.userConfiguration.isMultiSplitActive); | ||
| if (!isMultiSplitActive) { | ||
| return; | ||
| } | ||
|
|
||
| const preparation = yield* call(prepareSplitOrMerge, false); | ||
| if (!preparation) { | ||
| return; | ||
| } | ||
| const partitions = yield* select( | ||
| (state) => state.localSegmentationData[preparation.volumeTracing.tracingId].minCutPartitions, | ||
| ); | ||
| const agglomerateId = partitions.agglomerateId; | ||
| if (partitions[1].length <= 0 || partitions[2].length <= 0) { | ||
| console.error(messages["proofreading.multi_cut.empty_partition"]); | ||
| Toast.error(messages["proofreading.multi_cut.empty_partition"]); | ||
| return; | ||
| } | ||
| if (agglomerateId == null) { |
There was a problem hiding this comment.
Guard missing partitions and de-dup IDs before server calls.
- Accessing partitions without a null check can crash when state is not initialized.
- Use a union to avoid duplicate segment IDs.
const partitions = yield* select(
(state) => state.localSegmentationData[preparation.volumeTracing.tracingId].minCutPartitions,
);
+ if (
+ !partitions ||
+ !Array.isArray(partitions[1]) ||
+ !Array.isArray(partitions[2])
+ ) {
+ console.error(messages["proofreading.multi_cut.split_failed"]);
+ Toast.error(messages["proofreading.multi_cut.split_failed"]);
+ return;
+ }
@@
- const unmappedSegmentsOfPartitions = [...partitions[1], ...partitions[2]];
+ const unmappedSegmentsOfPartitions = _.union(partitions[1], partitions[2]);Also applies to: 680-695, 701-709
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts around lines
650-669 (also apply same fixes at 680-695 and 701-709): guard against partitions
being undefined/null before indexing (use optional chaining or an explicit null
check on partitions and partitions[1]/partitions[2] and return with the existing
error/Toast if missing), move/ensure the agglomerateId null check occurs after
the partitions guard, and before making any server calls deduplicate segment ID
arrays (e.g., convert to a Set or use a union helper) so the server never
receives duplicate IDs (also ensure you dedupe any combination of agglomerateId
and partition IDs before sending).
There was a problem hiding this comment.
Very nice! My testing also went well 🎉 I like the Enter and Escape shortcuts.
I'm wondering whether it would make sense to show the multi-cut shortcuts in the bottom bar instead of the usual shortcuts once the multi click cut tool is activated. However, I don't know how involved that is and it's not that important for the first iteration.
Also, I'm pretty sure one can partition agglomerates in such a way that multiple cuts are necessary to solve that (imagine a chain of segments in partitions 1 - 2 - 1 - 2 - 1 ...). During my testing it looked like that didn't work and only one cut was made, but maybe that's also not important for the first iteration. We might want to detect these "unsolvable" cuts and warn the user though.
frontend/javascripts/messages.tsx
Outdated
| "The annotation was successfully unlocked. Reloading this annotation ...", | ||
| "annotation.lock.success": | ||
| "The annotation was successfully locked. Reloading this annotation ...", | ||
| "proofreading.multi_cut.different_agglomerate_selected": `The selected segment belongs to a different agglomerate that is currently partitioned via selections. If you want to split this agglomerate please clear your current selection with "ESC" first.`, |
There was a problem hiding this comment.
| "proofreading.multi_cut.different_agglomerate_selected": `The selected segment belongs to a different agglomerate that is currently partitioned via selections. If you want to split this agglomerate please clear your current selection with "ESC" first.`, | |
| "proofreading.multi_cut.different_agglomerate_selected": `The selected segment belongs to a different agglomerate than the one that is currently partitioned via selections. If you want to split this other agglomerate please clear your current selection with "ESC" first.`, |
| sourceTree: Tree | null, | ||
| items: UpdateActionWithoutIsolationRequirement[], | ||
| ): Saga<boolean> { | ||
| ): Saga<[boolean, MinCutTargetEdge[]]> { |
There was a problem hiding this comment.
I think it's fine, but please add a comment briefly explaining the return type as it's not apparent from the method name.
🤔 Maybe make this a follow-up once this turns into an actual problem? Because
Is implemented now, but it doesn't really help with the discoverability, as the special shortcuts hints are only shown when |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (1)
1478-1516: BigInt/number mixing in mapping updates and API calls can break lookups and JSON serialization. Normalize types.
- splitSegmentIds may contain BigInt from the local mapping; sending BigInt to the REST API will fail JSON.stringify.
- mappingAfterSplit likely uses number keys; calling get() with BigInt misses entries; values set in splitMapping can mix number/bigint.
Fix by normalizing request IDs to number and adapting mapped values to the active mapping’s value type. Remove the @ts-ignore.
function* splitAgglomerateInMapping( activeMapping: ActiveMappingInfo, sourceAgglomerateId: number, volumeTracingId: string, additionalSegmentsToRequest: number[] = [], ) { - const segmentIdsFromLocalMapping = getSegmentIdsThatMapToAgglomerate( + const segmentIdsFromLocalMapping = getSegmentIdsThatMapToAgglomerate( activeMapping, sourceAgglomerateId, ); - const splitSegmentIds = _.union(segmentIdsFromLocalMapping, additionalSegmentsToRequest); + const splitSegmentIds = _.union(segmentIdsFromLocalMapping, additionalSegmentsToRequest); + // Normalize request payload to numbers to satisfy REST API / JSON + const splitSegmentIdsNumber: number[] = splitSegmentIds.map((id: any) => Number(id)); + // Determine mapping’s key/value type to adapt mapped ids accordingly + const mappingEntries = Array.from(activeMapping.mapping as NumberLikeMap); + const mappingUsesNumber = mappingEntries.length === 0 || !isBigInt(mappingEntries[0][0]); + const adaptKey = (id: number) => (mappingUsesNumber ? id : BigInt(id)); + const adaptValue = (id: number) => (mappingUsesNumber ? id : (BigInt(id) as any)); @@ - const mappingAfterSplit = yield* call( + const mappingAfterSplit = yield* call( getAgglomeratesForSegmentsFromTracingstore, tracingStoreUrl, volumeTracingId, - splitSegmentIds, + splitSegmentIdsNumber, annotationId, ); @@ - const splitMapping = new Map( - Array.from(activeMapping.mapping as NumberLikeMap, ([segmentId, agglomerateId]) => { - // @ts-ignore get() is expected to accept the type that segmentId has. - const mappedId = mappingAfterSplit.get(segmentId); - if (mappedId != null) { - return [segmentId, mappedId]; - } - return [segmentId, agglomerateId]; - }), - ); + const splitMapping = new Map( + Array.from(activeMapping.mapping as NumberLikeMap, ([segmentId, agglomerateId]) => { + const mappedIdNum = mappingAfterSplit.get(Number(segmentId)); + if (mappedIdNum != null) { + return [segmentId, adaptValue(mappedIdNum)]; + } + return [segmentId, agglomerateId]; + }), + ); @@ - for (const unmappedId of additionalSegmentsToRequest) { - // @ts-ignore get() is expected to accept the type that unmappedId has. - const mappedId = mappingAfterSplit.get(unmappedId); - if (mappedId) { - splitMapping.set(unmappedId, mappedId); - } - } + for (const unmappedId of additionalSegmentsToRequest) { + const mappedIdNum = mappingAfterSplit.get(Number(unmappedId)); + if (mappedIdNum != null) { + splitMapping.set(adaptKey(unmappedId), adaptValue(mappedIdNum)); + } + } return splitMapping as Mapping; }
♻️ Duplicate comments (1)
frontend/javascripts/viewer/controller/combinations/tool_controls.ts (1)
1087-1092: Guard against undefined IDs before dispatch.
Background clicks can yield 0/undefined; only checking=== 0missesundefinedand risks reducer crashes. Replace with a falsy check.Apply this diff:
- const unmappedSegmentId = VolumeHandlers.getUnmappedSegmentIdForPosition(globalPosition); - const mappedSegmentId = VolumeHandlers.getSegmentIdForPosition(globalPosition); - if (unmappedSegmentId === 0 || mappedSegmentId === 0) { + const unmappedSegmentId = VolumeHandlers.getUnmappedSegmentIdForPosition(globalPosition); + const mappedSegmentId = VolumeHandlers.getSegmentIdForPosition(globalPosition); + if (!unmappedSegmentId || !mappedSegmentId) { // No valid ids were found, ignore action. return; }
🧹 Nitpick comments (5)
frontend/javascripts/viewer/controller/combinations/tool_controls.ts (2)
1122-1128: Don’t show multi-split hints in 3D (TD) where clicks can’t map to 3D.
You early-return on TD clicks, but the TD viewport hints can still be overwritten to “Add to Partition …”, which is misleading.Apply this diff:
- if (isMultiSplitActive) { + if (isMultiSplitActive && !isTDViewportActive) { if (shiftKey && ctrlOrMetaKey) { multiSplitOverwrites.leftClick = "Add to Partition 2"; } else if (ctrlOrMetaKey) { multiSplitOverwrites.leftClick = "Add to Partition 1"; } }Also applies to: 1139-1144, 1156-1158
1088-1088: Nit: clarify naming — this is an agglomerate ID.
Renaming improves readability and matches the action’s third parameter name.Apply this diff:
- const mappedSegmentId = VolumeHandlers.getSegmentIdForPosition(globalPosition); + const agglomerateId = VolumeHandlers.getSegmentIdForPosition(globalPosition); @@ - Store.dispatch(toggleSegmentInPartitionAction(unmappedSegmentId, partition, mappedSegmentId)); + Store.dispatch(toggleSegmentInPartitionAction(unmappedSegmentId, partition, agglomerateId));Also applies to: 1094-1094
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (3)
145-164: Defensive null-check before touching scene controller.Add a guard to avoid rare NPEs if the scene controller isn’t ready yet.
- const sceneController = getSceneController(); - const { segmentMeshController } = sceneController; - segmentMeshController.updateActiveUnmappedSegmentIdHighlighting(null); + const sceneController = getSceneController(); + const segmentMeshController = sceneController?.segmentMeshController; + if (segmentMeshController) { + segmentMeshController.updateActiveUnmappedSegmentIdHighlighting(null); + }
619-646: Avoid partial side-effects: pre-validate all edges before dispatching DeleteEdge.If getDeleteEdgeActionForEdgePositions fails midway, earlier deleteEdgeAction calls will already have mutated the skeleton, while the save queue is skipped. Pre-compute and only dispatch after all edges validate.
- for (const edge of edgesToRemove) { - if (sourceTree) { - const result = getDeleteEdgeActionForEdgePositions(sourceTree, edge); - if (result == null) { - return [true, []]; - } - const { firstNodeId, secondNodeId } = result; - yield* put(deleteEdgeAction(firstNodeId, secondNodeId, Date.now(), "PROOFREADING")); - } - ... - } + if (sourceTree) { + const deletions: Array<{ firstNodeId: number; secondNodeId: number }> = []; + for (const edge of edgesToRemove) { + const result = getDeleteEdgeActionForEdgePositions(sourceTree, edge); + if (result == null) { + return [true, []]; + } + deletions.push(result); + } + for (const { firstNodeId, secondNodeId } of deletions) { + yield* put(deleteEdgeAction(firstNodeId, secondNodeId, Date.now(), "PROOFREADING")); + } + } + for (const edge of edgesToRemove) { + // keep splitAgglomerate enqueueing as-is + ... + }
702-710: Minor: dedupe earlier to avoid oversized request lists.You already union with the first removed edge; using union for the two partitions too keeps arrays minimal.
- const unmappedSegmentsOfPartitions = [...partitions[1], ...partitions[2]]; + const unmappedSegmentsOfPartitions = _.union(partition1, partition2);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/javascripts/messages.tsx(1 hunks)frontend/javascripts/viewer/controller/combinations/tool_controls.ts(4 hunks)frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/messages.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Applied to files:
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts
📚 Learning: 2025-09-08T13:33:47.097Z
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8824
File: frontend/javascripts/viewer/view/context_menu.tsx:1033-1039
Timestamp: 2025-09-08T13:33:47.097Z
Learning: In frontend/javascripts/viewer/view/context_menu.tsx, the segmentIdLabel intentionally shows the unmapped segment ID (maybeUnmappedSegmentId) in the "within Segment ${id}" text when proofreading is active, as this can be helpful for users. The mapped segment ID (segmentIdAtPosition) is only used as a fallback when the unmapped ID is not available.
Applied to files:
frontend/javascripts/viewer/controller/combinations/tool_controls.ts
🧬 Code graph analysis (2)
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (9)
frontend/javascripts/viewer/model/actions/settings_actions.ts (1)
UpdateUserSettingAction(13-13)frontend/javascripts/viewer/model/actions/ui_actions.ts (2)
EscapeAction(23-23)EnterAction(22-22)frontend/javascripts/viewer/model/actions/proofread_actions.ts (3)
resetMultiCutToolPartitionsAction(102-105)ToggleSegmentInPartitionAction(12-12)MinCutPartitionsAction(17-17)frontend/javascripts/viewer/model.ts (1)
getVisibleSegmentationLayer(122-136)frontend/javascripts/viewer/model/sagas/volume/update_actions.ts (1)
UpdateActionWithoutIsolationRequirement(141-188)frontend/javascripts/admin/rest_api.ts (1)
MinCutTargetEdge(2211-2216)frontend/javascripts/viewer/model/actions/save_actions.ts (1)
pushSaveQueueTransaction(46-53)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (1)
updateSegmentAction(260-279)frontend/javascripts/viewer/store.ts (1)
Mapping(364-364)
frontend/javascripts/viewer/controller/combinations/tool_controls.ts (2)
frontend/javascripts/viewer/model/actions/proofread_actions.ts (1)
toggleSegmentInPartitionAction(90-100)frontend/javascripts/viewer/store.ts (1)
UserConfiguration(309-353)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: frontend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (6)
frontend/javascripts/viewer/controller/combinations/tool_controls.ts (1)
47-48: LGTM: action import is correct.
Matches the new multi-split flow usage.frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (5)
126-130: Good event wiring for partitioned min-cut.Hooking both MIN_CUT_PARTITIONS and ENTER into a single saga keeps behavior consistent.
141-143: Appropriate listeners for deactivation/escape.Clear separation of responsibilities; pairing UPDATE_USER_SETTING and ESCAPE makes sense.
166-185: Non-blocking notice on cross-agglomerate selection.Info toast is fine for UX; logic correctly short-circuits when layer or partitions are missing.
483-489: Array-based performMinCut invocation is correct.Adapting to the new API by passing singleton arrays looks good and keeps compatibility.
730-752: Name preservation looks good.Applying the source agglomerate name to the split-off target is a nice touch; using Number() matches updateSegmentAction’s expectations.
frontend/javascripts/viewer/controller/combinations/tool_controls.ts
Outdated
Show resolved
Hide resolved
| function* performPartitionedMinCut(_action: MinCutPartitionsAction | EnterAction): Saga<void> { | ||
| const isMultiSplitActive = yield* select((state) => state.userConfiguration.isMultiSplitActive); | ||
| if (!isMultiSplitActive) { | ||
| return; | ||
| } | ||
|
|
||
| const preparation = yield* call(prepareSplitOrMerge, false); | ||
| if (!preparation) { | ||
| return; | ||
| } | ||
| const partitions = yield* select( | ||
| (state) => state.localSegmentationData[preparation.volumeTracing.tracingId].minCutPartitions, | ||
| ); | ||
| const agglomerateId = partitions.agglomerateId; | ||
| if (partitions[1].length <= 0 || partitions[2].length <= 0) { | ||
| console.error(messages["proofreading.multi_cut.empty_partition"]); | ||
| Toast.error(messages["proofreading.multi_cut.empty_partition"]); | ||
| return; | ||
| } | ||
| if (agglomerateId == null) { | ||
| console.error(messages["proofreading.multi_cut.no_valid_agglomerate"]); |
There was a problem hiding this comment.
Guard missing partitions and dedupe IDs before server calls; use locals thereafter.
Without a null/shape check, accessing partitions[1]/[2] can throw. Also, dedupe before requests and reuse the local arrays throughout to avoid races with RESET action.
- const partitions = yield* select(
- (state) => state.localSegmentationData[preparation.volumeTracing.tracingId].minCutPartitions,
- );
- const agglomerateId = partitions.agglomerateId;
- if (partitions[1].length <= 0 || partitions[2].length <= 0) {
+ const layerName = preparation.volumeTracing.tracingId;
+ const layerData = yield* select((state) => state.localSegmentationData[layerName]);
+ const partitions = layerData?.minCutPartitions;
+ if (
+ !partitions ||
+ !Array.isArray(partitions[1]) ||
+ !Array.isArray(partitions[2])
+ ) {
+ console.error(messages["proofreading.multi_cut.split_failed"]);
+ Toast.error(messages["proofreading.multi_cut.split_failed"]);
+ return;
+ }
+ const agglomerateId = partitions.agglomerateId;
+ const partition1: number[] = _.uniq(partitions[1]);
+ const partition2: number[] = _.uniq(partitions[2]);
+ if (partition1.length === 0 || partition2.length === 0) {
console.error(messages["proofreading.multi_cut.empty_partition"]);
Toast.error(messages["proofreading.multi_cut.empty_partition"]);
return;
}
@@
- const [hasErrored, edgesToRemove] = yield* call(
+ const [hasErrored, edgesToRemove] = yield* call(
performMinCut,
agglomerateId,
agglomerateId,
- partitions[1],
- partitions[2],
+ partition1,
+ partition2,
agglomerateFileMag,
volumeTracingId,
null,
items,
);
@@
- const unmappedSegmentsOfPartitions = [...partitions[1], ...partitions[2]];
+ const unmappedSegmentsOfPartitions = _.union(partition1, partition2);
@@
- const newAgglomerateIdFromPartition1 = yield* call(
+ const newAgglomerateIdFromPartition1 = yield* call(
preparation.mapSegmentId,
- partitions[1][0],
+ partition1[0],
newMapping,
);
- const newAgglomerateIdFromPartition2 = yield* call(
+ const newAgglomerateIdFromPartition2 = yield* call(
preparation.mapSegmentId,
- partitions[2][0],
+ partition2[0],
newMapping,
);Also applies to: 681-739
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts around lines
651 to 671 (and apply same changes to 681-739), the saga accesses partitions[1]
and partitions[2] without guarding for null/undefined or unexpected shapes and
performs server calls with arrays that may be mutated by concurrent RESET
actions; fix this by first selecting and validating that partitions and
partitions[1]/partitions[2] are arrays with length > 0, then create local copies
of those arrays (e.g., slice or spread) and deduplicate their IDs before any
server calls, and use those local, deduped arrays for all subsequent logic and
requests to avoid races.
This is a simple example for visualization purposes. Depending on the agglomerate graph I think it can more easily happen that multiple cuts are necessary (because the meshes don't show the agglomerate edges), but I also think that we should only address this once we see it becoming an issue.
Ok let's leave it then. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
frontend/javascripts/viewer/controller/combinations/tool_controls.ts (2)
1093-1095: Partition mapping now aligns with UI hints.
Shift+Ctrl → Partition 2 and Ctrl → Partition 1 is consistent with the displayed hints/documentation.
1087-1092: Guard against undefined/null segment IDs before dispatch.
getUnmappedSegmentIdForPosition/getSegmentIdForPosition may yield 0 or undefined when clicking background; only checking === 0 risks dispatching invalid IDs.Apply this diff:
- if (unmappedSegmentId === 0 || mappedSegmentId === 0) { + if ( + unmappedSegmentId == null || + mappedSegmentId == null || + unmappedSegmentId === 0 || + mappedSegmentId === 0 + ) { // No valid ids were found, ignore action. return; }
🧹 Nitpick comments (2)
frontend/javascripts/viewer/controller/combinations/tool_controls.ts (2)
1112-1159: Avoid showing multi-split hints in 3D (TD) viewport where the action is disabled.
onLeftClick returns early for TDView, but getActionDescriptors can still surface “Add to Partition …” in TD view, which is misleading. Gate the overwrite by viewport.Apply this diff:
- if (isMultiSplitActive) { + if (isMultiSplitActive && !isTDViewportActive) { if (shiftKey && ctrlOrMetaKey) { multiSplitOverwrites.leftClick = "Add to Partition 2"; } else if (ctrlOrMetaKey) { multiSplitOverwrites.leftClick = "Add to Partition 1"; } }This keeps TD hints accurate while preserving ortho behavior. The spread order that follows already ensures the overwrite takes precedence where applicable.
1130-1130: Nit: fix comment wording.
“TD-viewport viewport” → “TD viewport”.Apply this diff:
- // --- TD-viewport viewport ----------------------------------------------------- + // --- TD viewport --------------------------------------------------------------
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/viewer/controller/combinations/tool_controls.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T13:33:47.097Z
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8824
File: frontend/javascripts/viewer/view/context_menu.tsx:1033-1039
Timestamp: 2025-09-08T13:33:47.097Z
Learning: In frontend/javascripts/viewer/view/context_menu.tsx, the segmentIdLabel intentionally shows the unmapped segment ID (maybeUnmappedSegmentId) in the "within Segment ${id}" text when proofreading is active, as this can be helpful for users. The mapped segment ID (segmentIdAtPosition) is only used as a fallback when the unmapped ID is not available.
Applied to files:
frontend/javascripts/viewer/controller/combinations/tool_controls.ts
🧬 Code graph analysis (1)
frontend/javascripts/viewer/controller/combinations/tool_controls.ts (2)
frontend/javascripts/viewer/model/actions/proofread_actions.ts (1)
toggleSegmentInPartitionAction(90-100)frontend/javascripts/viewer/store.ts (1)
UserConfiguration(309-353)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (2)
frontend/javascripts/viewer/controller/combinations/tool_controls.ts (2)
47-47: Add action import — LGTM.
Import of toggleSegmentInPartitionAction is correct and scoped appropriately.
1084-1086: Correct gating for multi-split interaction.
Early branching on isMultiSplitActive and Ctrl/Meta is appropriate and avoids interfering with regular proofreading clicks.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/javascripts/viewer/view/plane_view.ts (1)
201-207: Fix stale partial highlight outside PROOFREAD (parent-only reuse condition).When not in PROOFREAD, the early return reuses the old hit if the parent group matches. With partial highlighting enabled, moving between different unmapped segments inside the same parent leaves the old partial highlight in place. Gate reuse on “same range” instead.
@@ - } else { - // In proofreading, there is no highlighting of parts of the meshes. - // If the parent group is identical, we can reuse the old hit object. - if (hitObject?.parent === oldRaycasterHit?.node.parent) { - return oldRaycasterHit; - } - } + } else { + // Outside of proofreading, only reuse if the highlighted range (if any) did not change. + if (hitObject?.parent === oldRaycasterHit?.node.parent) { + const newRange = highlightEntry?.range ?? null; + const oldRange = oldRaycasterHit?.indexRange ?? null; + const sameRange = + (newRange == null && oldRange == null) || + (newRange != null && + oldRange != null && + newRange[0] === oldRange[0] && + newRange[1] === oldRange[1]); + if (sameRange) { + return oldRaycasterHit; + } + } + }Also applies to: 224-231
🧹 Nitpick comments (2)
frontend/javascripts/viewer/view/plane_view.ts (2)
178-189: Type the highlightEntry and avoid passingcolor: undefined.Add a small type alias and annotate
highlightEntryto prevent it from becomingany. Also, omit thecolorfield when you want the default.@@ type RaycasterHit = { node: MeshSceneNode; indexRange: Vector2 | null; unmappedSegmentId: number | null; point: Vector3; } | null; +// Local alias to keep highlight entries typed in this file. +type MeshHighlightEntry = { range: Vector2; color?: any }; @@ - let highlightEntry = null; + let highlightEntry: MeshHighlightEntry | null = null; @@ - if (indexRange) { - highlightEntry = { range: indexRange, color: undefined }; - } + if (indexRange) { + highlightEntry = { range: indexRange }; + }Also applies to: 30-36
216-216: Optional: carry highlightEntry in RaycasterHit for clarity.You’re deriving
indexRangefromhighlightEntry. Consider storinghighlightEntryitself onRaycasterHitto avoid future drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/viewer/view/plane_view.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/javascripts/viewer/view/plane_view.ts (3)
frontend/javascripts/viewer/controller/mesh_helpers.ts (1)
VertexSegmentMapping(16-83)frontend/javascripts/viewer/model/helpers/listener_helpers.ts (1)
listenToStoreProperty(18-45)frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
getActiveSegmentationTracing(342-342)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
🔇 Additional comments (3)
frontend/javascripts/viewer/view/plane_view.ts (3)
249-255: Clear highlight vianulllooks correct.Matches the new API contract to remove prior highlight state on mouse-out.
357-360: Rename applied; ensure no callers still usehighlightActiveUnmappedSegmentId.Looks good here. Ran
rg -nP --type=ts '\bhighlightActiveUnmappedSegmentId\s*\('— no matches. Re-run a broader search across .ts/.tsx/.js/.jsx and generated/build outputs (e.g.rg -nP '\bhighlightActiveUnmappedSegmentId\s*\(' -S -uu --hidden) to confirm.
224-231: All updateMeshAppearance call sites use the new 5-arg signature.Inspected repository: calls in frontend/javascripts/viewer/view/plane_view.ts and frontend/javascripts/viewer/controller/segment_mesh_controller.ts pass a fifth argument (array, "full", or null); no call passes indexRange directly.
| this.unsubscribeFunctions.push( | ||
| listenToStoreProperty( | ||
| (storeState) => { | ||
| const segmentationTracing = getActiveSegmentationTracing(storeState); | ||
| if (segmentationTracing == null) { | ||
| return null; | ||
| } | ||
| return storeState.uiInformation.activeTool === AnnotationTool.PROOFREAD | ||
| ? storeState.localSegmentationData[segmentationTracing.tracingId].minCutPartitions | ||
| : null; | ||
| }, | ||
| (minCutPartitions) => | ||
| segmentMeshController.updateMinCutPartitionHighlighting(minCutPartitions), | ||
| true, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Guard minCutPartitions selector to avoid runtime errors when data is missing.
Directly indexing localSegmentationData[tracingId] can throw if the entry isn’t present yet. Use optional chaining and coalesce to null.
@@
- (storeState) => {
- const segmentationTracing = getActiveSegmentationTracing(storeState);
- if (segmentationTracing == null) {
- return null;
- }
- return storeState.uiInformation.activeTool === AnnotationTool.PROOFREAD
- ? storeState.localSegmentationData[segmentationTracing.tracingId].minCutPartitions
- : null;
- },
+ (storeState) => {
+ const segmentationTracing = getActiveSegmentationTracing(storeState);
+ if (
+ storeState.uiInformation.activeTool !== AnnotationTool.PROOFREAD ||
+ segmentationTracing == null
+ ) {
+ return null;
+ }
+ return (
+ storeState.localSegmentationData?.[segmentationTracing.tracingId]?.minCutPartitions ??
+ null
+ );
+ },📝 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.
| this.unsubscribeFunctions.push( | |
| listenToStoreProperty( | |
| (storeState) => { | |
| const segmentationTracing = getActiveSegmentationTracing(storeState); | |
| if (segmentationTracing == null) { | |
| return null; | |
| } | |
| return storeState.uiInformation.activeTool === AnnotationTool.PROOFREAD | |
| ? storeState.localSegmentationData[segmentationTracing.tracingId].minCutPartitions | |
| : null; | |
| }, | |
| (minCutPartitions) => | |
| segmentMeshController.updateMinCutPartitionHighlighting(minCutPartitions), | |
| true, | |
| ), | |
| ); | |
| this.unsubscribeFunctions.push( | |
| listenToStoreProperty( | |
| (storeState) => { | |
| const segmentationTracing = getActiveSegmentationTracing(storeState); | |
| if ( | |
| storeState.uiInformation.activeTool !== AnnotationTool.PROOFREAD || | |
| segmentationTracing == null | |
| ) { | |
| return null; | |
| } | |
| return ( | |
| storeState.localSegmentationData?.[segmentationTracing.tracingId]?.minCutPartitions ?? | |
| null | |
| ); | |
| }, | |
| (minCutPartitions) => | |
| segmentMeshController.updateMinCutPartitionHighlighting(minCutPartitions), | |
| true, | |
| ), | |
| ); |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/view/plane_view.ts around lines 361 to 376, the
selector unguards access to
storeState.localSegmentationData[segmentationTracing.tracingId] which can be
undefined and cause a runtime error; change the selector so it uses optional
chaining and a null coalesce when accessing minCutPartitions (e.g. access
localSegmentationData?.[tracingId]?.minCutPartitions) and return null if any
intermediate value is missing, then pass that possibly-null value to
updateMinCutPartitionHighlighting as before.
3 participants
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
enterupon partition select should do the cut (context menu interaction is not needed)m/c)enterand cancel selection viaEscapeIssues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)