Add multi click split tool for proofreading #8824
Conversation
📝 WalkthroughWalkthroughAdds a partition-based multi-split proofreading workflow: new state/flags, actions, reducer, sagas, UI toggle and context-menu options, input handlers for toggling partitions, mesh highlighting for partitions, frontend->backend min-cut API changes, and keyboard docs updates. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
- 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
| const minCutPartitions = state.localSegmentationData[layerName].minCutPartitions; | ||
| const otherPartitionIndex = action.partition === 1 ? 2 : 1; | ||
| if (minCutPartitions.agglomerateId !== action.agglomerateId) { | ||
| // TODOM: Discuss whether to instead ignore the action and show an error toast that the new segment does not belong to the same agglomerate. |
There was a problem hiding this comment.
TODO :D
What do you think about this?
Currently, if a segment of a different agglomerate is toggled, the action simply clears the current selection which might be unwanted. But maybe we could also develop it this way once requested
There was a problem hiding this comment.
Since there is a convenient shortcut to discard the selection already (esc) and discarding it by accident, by clicking on another agglomerate, is not reversible, I would prefer an error toast. The toast should note that another agglomerate was selected and suggest to press esc first if that was deliberate.
| @@ -1726,7 +1828,7 @@ function ContextMenuInner() { | |||
| </FastTooltip> | |||
| ); | |||
|
|
|||
| 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
| @@ -540,6 +542,7 @@ export type ConnectomeData = { | |||
| readonly activeAgglomerateIds: Array<number>; | |||
| readonly skeleton: SkeletonTracing | null | undefined; | |||
| }; | |||
| export type MinCutPartitions = { 1: number[]; 2: number[]; agglomerateId: number }; | |||
There was a problem hiding this comment.
Feel free to argue with this internal data model. It is was just came into my mind and seemed simple :)
There was a problem hiding this comment.
Note to myself that it might be better to type agglomerateId as optional instead of using 0 as the placeholder, but I need to read the remaining code first.
|
@fm3 Could you please take a look at the backend changes once you have some spare time? It shouldn't be much |
There was a problem hiding this comment.
Backend LGTM, just two notes on variable naming and readability.
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.
| partitionOne: List[Long], | ||
| partitionTwo: List[Long], |
There was a problem hiding this comment.
Since we have segmentId1 and segmentId2 in so many places, I’d vote to change these variable names to partition1 and partition2 also, for the benefit of consistency.
| val firstPartitionRootId = -1 | ||
| val secondPartitionRootId = -2 |
There was a problem hiding this comment.
Now we also have the keyword first and second. Here I’d also vote for 1 and 2. Also these lines could use a comment. Maybe move them below the existing comment about the artificial root nodes?
| | ++Escape++ | Clear Partition Selection of Multi Cut Tool | | ||
|
|
||
|
|
||
| #### Orthogonal Viewport Shortcuts: |
There was a problem hiding this comment.
| #### Orthogonal Viewport Shortcuts: | |
| #### Orthogonal Viewport: |
| | ++ctrl++ / ++cmd++ + Left Click | Add Segment to Partition One for Multi Cut | | ||
| | ++ctrl++ / ++cmd++ + ++shift++ + Left Click | Add Segment to Partition Two for Multi Cut | | ||
|
|
||
| #### 3D Viewport Interactions: |
There was a problem hiding this comment.
| #### 3D Viewport Interactions: | |
| #### 3D Viewport: |
| If Multi Cut Tool is activated, the following shortcuts are available: | ||
|
|
||
| | Key Binding | Operation | | ||
| |---------------------------------------------|--------------------------------------------| | ||
| | ++ctrl++ / ++cmd++ + Left Click | Add Segment to Partition One for Multi Cut | | ||
| | ++ctrl++ / ++cmd++ + ++shift++ + Left Click | Add Segment to Partition Two for Multi Cut | |
There was a problem hiding this comment.
I can see why you did it this way, but since this is the same for the orthogonal viewports as well, I'd suggest to move it into the general section of the proofreading shortcuts and also move | ++Enter++ | Perform Multi Cut (if Multi Cut Tool is active) | in there, so it doesn't need the (if Multi Cut Tool is active) note.
|
|
||
| | Key Binding | Operation | | ||
| |---------------------------------|--------------------------------------------------------------| | ||
| | ++ctrl++ / ++cmd++ + Left Click | Activate Super Voxel of Agglomerate for Proofreading Actions | |
There was a problem hiding this comment.
Is this correct? If so we should think about amending the shortcut since it's different to the orthogonal viewports and overlaps with Add Segment to Partition One for Multi Cut now.
Also, this is the only mention of Super Voxel in this section. To be consistent I'd also use Segment instead.
|
|
||
| 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.
I don't understand why the first removed edge is important 🤔 Could you explain that?
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts
Outdated
Show resolved
Hide resolved
fm3
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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.
| @@ -29,6 +29,7 @@ import { HANDLED_ERROR } from "viewer/model_initialization"; | |||
| import { Model } from "viewer/singletons"; | |||
| import type { TraceOrViewCommand, WebknossosState } from "viewer/store"; | |||
| import Store from "viewer/store"; | |||
| import { AnnotationTool } from "./model/accessors/tool_accessor"; | |||
There was a problem hiding this comment.
Fix import path to avoid duplicate module identity.
Use the aliased path to match the rest of the codebase; the relative import can create a second module instance and break equality checks like activeTool === AnnotationTool.PROOFREAD.
Apply this diff:
-import { AnnotationTool } from "./model/accessors/tool_accessor";
+import { AnnotationTool } from "viewer/model/accessors/tool_accessor";📝 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.
| import { AnnotationTool } from "./model/accessors/tool_accessor"; | |
| import { AnnotationTool } from "viewer/model/accessors/tool_accessor"; |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/controller.tsx around line 32, the current
relative import "import { AnnotationTool } from
\"./model/accessors/tool_accessor\";" can create a duplicate module identity;
replace this relative import with the project's aliased module path (e.g. import
{ AnnotationTool } from "@/model/accessors/tool_accessor") to match other files
and ensure strict identity checks like activeTool === AnnotationTool.PROOFREAD
continue to work.
| 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.
| const minCutPartitions = state.localSegmentationData[layerName].minCutPartitions; | ||
| const otherPartitionIndex = action.partition === 1 ? 2 : 1; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid crash when per-layer data is missing.
Add a defensive check before reading minCutPartitions.
Apply this diff:
- switch (action.type) {
+ switch (action.type) {
case "TOGGLE_SEGMENT_IN_PARTITION": {
- const minCutPartitions = state.localSegmentationData[layerName].minCutPartitions;
+ const layerData = state.localSegmentationData[layerName];
+ if (!layerData || !layerData.minCutPartitions) {
+ return state;
+ }
+ const minCutPartitions = layerData.minCutPartitions;📝 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 minCutPartitions = state.localSegmentationData[layerName].minCutPartitions; | |
| const otherPartitionIndex = action.partition === 1 ? 2 : 1; | |
| switch (action.type) { | |
| case "TOGGLE_SEGMENT_IN_PARTITION": { | |
| const layerData = state.localSegmentationData[layerName]; | |
| if (!layerData || !layerData.minCutPartitions) { | |
| return state; | |
| } | |
| const minCutPartitions = layerData.minCutPartitions; | |
| const otherPartitionIndex = action.partition === 1 ? 2 : 1; | |
| // …rest of handler… |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/model/reducers/proofreading_reducer.ts around
lines 14-15, the reducer reads
state.localSegmentationData[layerName].minCutPartitions without checking that
state.localSegmentationData and the per-layer entry exist, which can crash when
per-layer data is missing; add a defensive check that
state.localSegmentationData and state.localSegmentationData[layerName] are
defined before accessing minCutPartitions and if missing either return the
current state (no-op) or initialize a safe default for minCutPartitions, then
proceed to compute otherPartitionIndex only when minCutPartitions is available.
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.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).
| function getMultiCutToolOptions( | ||
| unmappedSegmentId: number, | ||
| mappedSegmentId: number, | ||
| minCutPartitions: MinCutPartitions, | ||
| segmentOrSuperVoxel: string, | ||
| segmentIdLabel: string | number, | ||
| ): MenuItemType[] { | ||
| // Multi split min cut tool options | ||
| const isSegmentInPartitionOne = minCutPartitions[1].includes(unmappedSegmentId); | ||
| const isSegmentInPartitionTwo = minCutPartitions[2].includes(unmappedSegmentId); | ||
| const togglePartitionOneVerb = isSegmentInPartitionOne ? "remove" : "add"; | ||
| const togglePartitionTwoVerb = isSegmentInPartitionTwo ? "remove" : "add"; | ||
| const doBothPartitionsHaveEntries = | ||
| minCutPartitions[1].length > 0 && minCutPartitions[2].length > 0; | ||
| return [ | ||
| { | ||
| key: "mark-as-partition-1", | ||
| onClick: () => | ||
| Store.dispatch(toggleSegmentInPartitionAction(unmappedSegmentId, 1, mappedSegmentId)), | ||
| label: ( | ||
| <> | ||
| {_.capitalize(togglePartitionOneVerb)} {segmentOrSuperVoxel} ({segmentIdLabel}) to | ||
| Partition 1 {shortcutBuilder(["Ctrl", "leftMouse"])} | ||
| </> | ||
| ), | ||
| }, | ||
| { | ||
| key: "mark-as-partition-2", | ||
| onClick: () => | ||
| Store.dispatch(toggleSegmentInPartitionAction(unmappedSegmentId, 2, mappedSegmentId)), | ||
| label: ( | ||
| <> | ||
| {_.capitalize(togglePartitionTwoVerb)} {segmentOrSuperVoxel} ({segmentIdLabel}) to | ||
| Partition 2 {shortcutBuilder(["Shift", "leftMouse"])} | ||
| </> | ||
| ), | ||
| }, | ||
| ...(doBothPartitionsHaveEntries | ||
| ? [ | ||
| { | ||
| key: "min-cut-agglomerate-with-partitions", | ||
| onClick: () => Store.dispatch(minCutPartitionsAction()), | ||
| label: "Split partitions", | ||
| }, | ||
| ] | ||
| : []), | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Shortcut hint for “Partition 2” is wrong (should be Ctrl+Shift+Left Click).
The TD/ortho handlers require Ctrl/Cmd+Shift for partition 2, but the label shows only Shift.
- Partition 2 {shortcutBuilder(["Shift", "leftMouse"])}
+ Partition 2 {shortcutBuilder([CtrlOrCmdKey, "Shift", "leftMouse"])}Optional: For consistency, also use CtrlOrCmdKey for Partition 1:
- Partition 1 {shortcutBuilder(["Ctrl", "leftMouse"])}
+ Partition 1 {shortcutBuilder([CtrlOrCmdKey, "leftMouse"])}📝 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.
| function getMultiCutToolOptions( | |
| unmappedSegmentId: number, | |
| mappedSegmentId: number, | |
| minCutPartitions: MinCutPartitions, | |
| segmentOrSuperVoxel: string, | |
| segmentIdLabel: string | number, | |
| ): MenuItemType[] { | |
| // Multi split min cut tool options | |
| const isSegmentInPartitionOne = minCutPartitions[1].includes(unmappedSegmentId); | |
| const isSegmentInPartitionTwo = minCutPartitions[2].includes(unmappedSegmentId); | |
| const togglePartitionOneVerb = isSegmentInPartitionOne ? "remove" : "add"; | |
| const togglePartitionTwoVerb = isSegmentInPartitionTwo ? "remove" : "add"; | |
| const doBothPartitionsHaveEntries = | |
| minCutPartitions[1].length > 0 && minCutPartitions[2].length > 0; | |
| return [ | |
| { | |
| key: "mark-as-partition-1", | |
| onClick: () => | |
| Store.dispatch(toggleSegmentInPartitionAction(unmappedSegmentId, 1, mappedSegmentId)), | |
| label: ( | |
| <> | |
| {_.capitalize(togglePartitionOneVerb)} {segmentOrSuperVoxel} ({segmentIdLabel}) to | |
| Partition 1 {shortcutBuilder(["Ctrl", "leftMouse"])} | |
| </> | |
| ), | |
| }, | |
| { | |
| key: "mark-as-partition-2", | |
| onClick: () => | |
| Store.dispatch(toggleSegmentInPartitionAction(unmappedSegmentId, 2, mappedSegmentId)), | |
| label: ( | |
| <> | |
| {_.capitalize(togglePartitionTwoVerb)} {segmentOrSuperVoxel} ({segmentIdLabel}) to | |
| Partition 2 {shortcutBuilder(["Shift", "leftMouse"])} | |
| </> | |
| ), | |
| }, | |
| ...(doBothPartitionsHaveEntries | |
| ? [ | |
| { | |
| key: "min-cut-agglomerate-with-partitions", | |
| onClick: () => Store.dispatch(minCutPartitionsAction()), | |
| label: "Split partitions", | |
| }, | |
| ] | |
| : []), | |
| ]; | |
| } | |
| function getMultiCutToolOptions( | |
| unmappedSegmentId: number, | |
| mappedSegmentId: number, | |
| minCutPartitions: MinCutPartitions, | |
| segmentOrSuperVoxel: string, | |
| segmentIdLabel: string | number, | |
| ): MenuItemType[] { | |
| // Multi split min cut tool options | |
| const isSegmentInPartitionOne = minCutPartitions[1].includes(unmappedSegmentId); | |
| const isSegmentInPartitionTwo = minCutPartitions[2].includes(unmappedSegmentId); | |
| const togglePartitionOneVerb = isSegmentInPartitionOne ? "remove" : "add"; | |
| const togglePartitionTwoVerb = isSegmentInPartitionTwo ? "remove" : "add"; | |
| const doBothPartitionsHaveEntries = | |
| minCutPartitions[1].length > 0 && minCutPartitions[2].length > 0; | |
| return [ | |
| { | |
| key: "mark-as-partition-1", | |
| onClick: () => | |
| Store.dispatch(toggleSegmentInPartitionAction(unmappedSegmentId, 1, mappedSegmentId)), | |
| label: ( | |
| <> | |
| {_.capitalize(togglePartitionOneVerb)} {segmentOrSuperVoxel} ({segmentIdLabel}) to | |
| Partition 1 {shortcutBuilder([CtrlOrCmdKey, "leftMouse"])} | |
| </> | |
| ), | |
| }, | |
| { | |
| key: "mark-as-partition-2", | |
| onClick: () => | |
| Store.dispatch(toggleSegmentInPartitionAction(unmappedSegmentId, 2, mappedSegmentId)), | |
| label: ( | |
| <> | |
| {_.capitalize(togglePartitionTwoVerb)} {segmentOrSuperVoxel} ({segmentIdLabel}) to | |
| Partition 2 {shortcutBuilder([CtrlOrCmdKey, "Shift", "leftMouse"])} | |
| </> | |
| ), | |
| }, | |
| ...(doBothPartitionsHaveEntries | |
| ? [ | |
| { | |
| key: "min-cut-agglomerate-with-partitions", | |
| onClick: () => Store.dispatch(minCutPartitionsAction()), | |
| label: "Split partitions", | |
| }, | |
| ] | |
| : []), | |
| ]; | |
| } |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/view/context_menu.tsx around lines 402 to 449,
the shortcut label for Partition 2 is incorrect (shows only Shift); change the
shortcutBuilder arguments so Partition 2 uses Ctrl/Cmd+Shift+Left Click and, for
consistency, Partition 1 uses Ctrl/Cmd+Left Click: update the Partition 1 label
to shortcutBuilder(["CtrlOrCmdKey", "leftMouse"]) and update the Partition 2
label to shortcutBuilder(["CtrlOrCmdKey", "Shift", "leftMouse"]); keep the rest
of the menu logic unchanged.
| const { isMultiSplitActive } = state.userConfiguration; | ||
| const minCutPartitions = state.localSegmentationData[volumeTracing.tracingId].minCutPartitions; | ||
| // The cut and merge operations depend on the active segment. The volume tracing *always* has an activeCellId. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against missing minCutPartitions.
Access to state.localSegmentationData[volumeTracing.tracingId] may be undefined (e.g., before init). Add optional chaining and gate before calling getMultiCutToolOptions.
- const { isMultiSplitActive } = state.userConfiguration;
- const minCutPartitions = state.localSegmentationData[volumeTracing.tracingId].minCutPartitions;
+ const { isMultiSplitActive } = state.userConfiguration;
+ const minCutPartitions =
+ state.localSegmentationData[volumeTracing.tracingId]?.minCutPartitions;
...
- const proofreadingMultiSplitToolActions =
- isProofreadingActive && isMultiSplitActive && maybeUnmappedSegmentId != null
+ const proofreadingMultiSplitToolActions =
+ isProofreadingActive &&
+ isMultiSplitActive &&
+ maybeUnmappedSegmentId != null &&
+ minCutPartitions != null
? getMultiCutToolOptions(
maybeUnmappedSegmentId,
clickedMeshId,
minCutPartitions,
segmentOrSuperVoxel,
segmentIdLabel,
)
: [];Also applies to: 513-523
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/view/context_menu.tsx around lines 478-480 (and
similarly for lines 513-523), access to
state.localSegmentationData[volumeTracing.tracingId] can be undefined; guard
this by using optional chaining when reading minCutPartitions (e.g.
state.localSegmentationData?.[volumeTracing.tracingId]?.minCutPartitions) and
check that the object and minCutPartitions exist before calling
getMultiCutToolOptions — if missing, skip calling the function or provide a safe
default/early return so the code does not throw.
| const segmentIdLabel = | ||
| isProofreadingActive && maybeUnmappedSegmentId != null | ||
| ? `within Segment ${maybeUnmappedSegmentId}` | ||
| : segmentIdAtPosition; | ||
| const segmentOrSuperVoxel = | ||
| isProofreadingActive && maybeUnmappedSegmentId != null ? "Super-Voxel" : "Segment"; | ||
| Store.dispatch(maybeFetchMeshFilesAction(visibleSegmentationLayer, dataset, false)); |
There was a problem hiding this comment.
Wrong “within Segment …” label in no-node context.
You’re showing the unmapped (super-voxel) ID as the enclosing segment. Use segmentIdAtPosition.
- const segmentIdLabel =
- isProofreadingActive && maybeUnmappedSegmentId != null
- ? `within Segment ${maybeUnmappedSegmentId}`
- : segmentIdAtPosition;
+ const segmentIdLabel =
+ isProofreadingActive && maybeUnmappedSegmentId != null
+ ? `within Segment ${segmentIdAtPosition}`
+ : segmentIdAtPosition;And keep passing segmentIdAtPosition as the mapped ID in the call below (already correct).
Also applies to: 1237-1247
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/view/context_menu.tsx around lines 1034 to 1040,
the label uses the unmapped super-voxel ID as the enclosing segment ("within
Segment ...") when proofreading is active; change the label to always use
segmentIdAtPosition for the enclosing segment text while keeping the current
logic that only shows the Super-Voxel wording when proofreading is active (i.e.,
use segmentIdAtPosition in the template string instead of
maybeUnmappedSegmentId), and ensure the call to maybeFetchMeshFilesAction
continues to receive segmentIdAtPosition as the mapped ID (no change to that
call). This same fix should be applied to the similar block at lines 1237-1247.
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.
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)