Add Split Segments Toolkit and Draw Mode for Skeleton Tool #8434
Conversation
📝 WalkthroughWalkthroughThis update introduces a comprehensive "Split Segments" workflow and toolkit for voxel-wise segment splitting in WEBKNOSSOS. Key changes include new React UI components for toolkits, tool buttons, and split segment controls; a new saga and geometry module to render and manage 3D split boundary surfaces; and integration of these features into the flood fill algorithm to prevent propagation across user-defined surfaces. The codebase refactors tool management to use class-based abstractions, adds toolkit selection to the UI, and updates reducers, sagas, and selectors to support toolkit-driven workflows and tool disabling logic. Extensive documentation and test updates accompany these changes. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
philippotto
changed the title
|
|
||
|  | ||
|
|
||
| ## Impact of "Split Segments" Toolkit |
There was a problem hiding this comment.
The nodes/skeletons that are being placed in this mode, are the persisted? Or will they disappear after the split operation?
There was a problem hiding this comment.
The nodes/skeletons that are being placed in this mode, are the persisted?
yes, it's a normal tree. it is only rendered differently when the split toolkit is active. if the toolkit is deactivated, the tree will look like any other tree.
Co-authored-by: Tom Herold <[email protected]>
Co-authored-by: Tom Herold <[email protected]>
Co-authored-by: Tom Herold <[email protected]>
Co-authored-by: Tom Herold <[email protected]>
Co-authored-by: Tom Herold <[email protected]>
Co-authored-by: Tom Herold <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/proofreading/split_segments_toolkit.md (1)
3-4: Duplicate suggestion: clear split interface description
This sentence effectively conveys how to draw the splitting surface, as previously recommended.
🧹 Nitpick comments (5)
docs/proofreading/split_segments_toolkit.md (5)
6-9: Consider using a numbered list for sequential steps
Since these bullets describe an ordered workflow, switching to a numbered list (1.,2., …) could improve readability.
10-10: Improve readability and correct spelling
This bullet is very long—consider splitting into two sentences. Also replace “proof-reading” with “proofreading” and rephrase one of the “If you want to” clauses to avoid repetition.🧰 Tools
🪛 LanguageTool
[style] ~10-~10: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ml) for large-scale corrections. If you want to correct data for the purpose of ground-...(REP_WANT_TO_VB)
[misspelling] ~10-~10: This word is normally spelled as one.
Context: ...o only do a local correction instead of proof-reading the whole dataset. - Go to the first z ...(EN_COMPOUNDS_PROOF_READING)
12-12: Format tool mode with code ticks
For consistency, wrap UI elements likePenmode in backticks (e.g., activate thePenmode) so they stand out.
14-14: Rephrase to avoid repetition
You’ve already used “If you want to” above. Consider “To adjust the interpolation, add new nodes on a slice…” to vary the phrasing.
22-22: Improve alt text for accessibility
The current alt text is verbose. Consider simplifying to something like:
“Flood-fill bleeding across an incomplete split surface.”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/proofreading/split_segments_toolkit.md(1 hunks)docs/skeleton_annotation/tools.md(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/skeleton_annotation/tools.md
🧰 Additional context used
🪛 LanguageTool
docs/proofreading/split_segments_toolkit.md
[style] ~10-~10: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ml) for large-scale corrections. If you want to correct data for the purpose of ground-...
(REP_WANT_TO_VB)
[misspelling] ~10-~10: This word is normally spelled as one.
Context: ...o only do a local correction instead of proof-reading the whole dataset. - Go to the first z ...
(EN_COMPOUNDS_PROOF_READING)
🔇 Additional comments (6)
docs/proofreading/split_segments_toolkit.md (6)
1-2: Good heading clarity
The title clearly indicates the feature being documented.
11-11: Verify UI terminology
Ensure that “Go to the first z slice of the bounding box” matches the exact wording and capitalization used in the application’s UI.
15-15: Verify tool mode names
Confirm that “3D mode” and “Restrict to Bounding Box” match the exact labels in the UI settings to avoid confusion.
17-19: Approve troubleshooting advice
The troubleshooting steps are clear and provide helpful guidance for common pitfalls.
20-21: Approve configuration troubleshooting
The note about spline-curve configuration and clipping distance is concise and accurate.
29-31: Approve impact section details
The bullet points accurately describe how the toolkit alters rendering and flood-fill behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/proofreading/split_segments_toolkit.md (1)
24-31:⚠️ Potential issueFix product name casing
The application’s name is consistently branded as “WebKnossos,” not “WEBKNOSSOS.” Please update to avoid confusion.Proposed diff:
- By activating that toolkit, WEBKNOSSOS behaves differently in the following way: + By activating that toolkit, WebKnossos behaves differently in the following way:
🧹 Nitpick comments (2)
docs/proofreading/split_segments_toolkit.md (2)
8-16: Refine long sentence and correct spelling
The third sentence in step 1 is very long and uses “proof-reading” (hyphenated), which is non-standard. Consider splitting it for readability and changing “proof-reading” to “proofreading”.Proposed diff:
- 1. Use the bounding box tool to create a box around the merge error that you want to correct. Theoretically, this step is optional, but keep in mind that relabeling large parts of a segment requires a lot of computing resources and is limited by your computer performance. Consider using the [proofreading tool](../proofreading/tools.html) for large-scale corrections. If you want to correct data for the purpose of ground-truth, it is often enough to only do a local correction instead of proof-reading the whole dataset. + 1. Use the bounding box tool to create a box around the merge error you want to correct. This step is optional, but relabeling large regions can be resource-intensive. For large-scale corrections, consider using the [proofreading tool](../proofreading/tools.html). When generating ground-truth data, a local correction within the bounding box is often sufficient instead of proofreading the entire dataset.🧰 Tools
🪛 LanguageTool
[style] ~10-~10: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ml) for large-scale corrections. If you want to correct data for the purpose of ground-...(REP_WANT_TO_VB)
[misspelling] ~10-~10: This word is normally spelled as one.
Context: ...o only do a local correction instead of proof-reading the whole dataset. 2. Go to the first z...(EN_COMPOUNDS_PROOF_READING)
17-23: Enhance formatting for UI elements and consistency
In the Troubleshooting section, GUI labels like “Layers” → “Skeleton” should use backticks for consistency with other docs.Proposed diff:
- - If multiple spline curves appear per slice, this is often due to a configuration issue. Please adapt the clipping distance in the left sidebar ("Layers" -> "Skeleton"). + - If multiple spline curves appear per slice, this is often due to a configuration issue. Please adapt the clipping distance in the left sidebar (`Layers` → `Skeleton`).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/proofreading/split_segments_toolkit.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/proofreading/split_segments_toolkit.md
[style] ~10-~10: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ml) for large-scale corrections. If you want to correct data for the purpose of ground-...
(REP_WANT_TO_VB)
[misspelling] ~10-~10: This word is normally spelled as one.
Context: ...o only do a local correction instead of proof-reading the whole dataset. 2. Go to the first z...
(EN_COMPOUNDS_PROOF_READING)
🔇 Additional comments (1)
docs/proofreading/split_segments_toolkit.md (1)
1-5: Introduction is clear and well-structured
The title and opening paragraph succinctly introduce the Split Segments Toolkit and link to the Toolkits overview. No changes needed here.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
docs/proofreading/split_segments_toolkit.md (4)
4-4: Consolidated phrasing matches past suggestion
This sentence aligns exactly with a previous suggestion to introduce the split interface. No additional changes needed here.
15-15: Highlighting past suggestion for bounding box restriction
This instruction reflects a previous recommendation to remind users to enable both 3D and bounding box modes during the fill step. It’s been applied correctly.
19-19: Reminder: Draw the splitting surface larger
This advice repeats an earlier note to enlarge the surface to prevent floodfill bleed. It’s already included and no further action is needed.
27-27: Product name casing is inconsistent
This line still usesWEBKNOSSOSin all caps, which conflicts with the application’s branding. Please replace withWebKnossos.
🧹 Nitpick comments (4)
docs/proofreading/split_segments_toolkit.md (4)
12-12: Use code formatting for UI labels
UI labels likePenmode are more clearly denoted with backticks than quotes. For consistency:- Consider activating the "Pen" mode within the skeleton tool + Consider activating the `Pen` mode within the skeleton tool
21-21: Link to configuration documentation
When advising users to adapt the clipping distance, consider linking to the relevant settings page for clarity:- If multiple spline curves appear per slice, adapt the [Skeleton clipping distance configuration](../ui/toolbar.md#skeleton-settings).
22-22: Consider adding a figure caption for accessibility
Wrapping the image in a<figure>with a<figcaption>provides additional context and improves accessibility:<figure> <img src="../images/splitting-floodfill-visualization.png" alt="Floodfill bleed visualization"> <figcaption>Figure: Floodfill operation bleeding across an incomplete boundary.</figcaption> </figure>
29-31: Refine phrasing for parallel structure
For a more consistent tone and parallelism, you could adjust the bullets:- Using the spline curves from before, a 3D surface is generated... + A 3D surface is generated from these spline curves... - The flood-fill tool will respect the splitting surface by not crossing it. + The flood-fill tool will not cross the splitting surface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
CHANGELOG.unreleased.md(1 hunks)README.md(2 hunks)docs/getting_started.md(1 hunks)docs/proofreading/split_segments_toolkit.md(1 hunks)frontend/javascripts/oxalis/model_initialization.ts(3 hunks)package.json(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- README.md
- package.json
- docs/getting_started.md
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.unreleased.md
- frontend/javascripts/oxalis/model_initialization.ts
🧰 Additional context used
🪛 LanguageTool
docs/proofreading/split_segments_toolkit.md
[style] ~10-~10: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ml) for large-scale corrections. If you want to correct data for the purpose of ground-...
(REP_WANT_TO_VB)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
🔇 Additional comments (1)
docs/proofreading/split_segments_toolkit.md (1)
8-8: Great use of an ordered list for workflow steps
Switching from bullet points to a numbered list improves clarity and signals the required sequence.
| @@ -0,0 +1,31 @@ | |||
| # Split Segments Toolkit | |||
|
|
|||
| As explained in the [Toolkits](../ui/toolbar.md#Toolkits) section, there is a dedicated toolkit for splitting segments in your volume annotation. | |||
There was a problem hiding this comment.
Broken markdown link anchor
The link to the Toolkits section uses #Toolkits, but Markdown heading anchors are typically all lowercase (#toolkits). This may break the link. Please update accordingly:
- As explained in the [Toolkits](../ui/toolbar.md#Toolkits) section...
+ As explained in the [Toolkits](../ui/toolbar.md#toolkits) section...📝 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.
| As explained in the [Toolkits](../ui/toolbar.md#Toolkits) section, there is a dedicated toolkit for splitting segments in your volume annotation. | |
| As explained in the [Toolkits](../ui/toolbar.md#toolkits) section, there is a dedicated toolkit for splitting segments in your volume annotation. |
There was a problem hiding this comment.
Thanks for going through all my feedback 🙏 I looked at everything relevant again.
- I think this is my problem but during the first spliting attempt I think I missed to strech the plane through all slices that need to be splitted leading to the flood fill "bleeding into the other side" of the plane. Not sure how to avoid this except for undo and refine. But I think this fine. Still watned to mention it.
yeah, this is not ideal, but for the first iteration okay imo.
Ok 👍
- The 3d floodfill restrictions are a little too hard. For each split I tried, the limit was reached and a bbox was created. Maybe increasing the limit a little more would help here 🤔
the limits should be orthogonal to this PR. when using the "restrict to bbox" feature, the limits are already higher and I think that should be enough for now.
But in that case the user would need to explicitly create a new bounding box before flood filling :/. But maybe that's also ok for the first iteration.
- In case the user wants to split of a merged cell and wants to create a new cell it might be convinient to have this as part of the split toolkit. The brush makes it easy via pressing c and direclty indicating that a new cell was created via the color of the brush. But the split toolkit there is no real visual indication for this. The floodfill tool also supports the c shortcut but except for the number of the active segment in the toolbar changing there is no visual indication. Maybe some kind of visual ui change in case of creating a new cell in the flood fill tool would be helpful 🤔. Bbut maybe that's not something for this PR. And maybe creating a new cell is very unsual as usually only a segment of the cell needs to be corrected to be part of a different cell. 🤔 So please just treat this a a thought and not as a must fix / implement or so :)
when the fill tool is active, there is the normal "create new segment id" button in the toolbar with the colored dot-indcator. isn't this enough?
Oh 💡, I was looking for such a button while the flood fill tool wasn't the active tool. So please nvm this.
| const getSuperUserItems = (): CommandWithoutId[] => { | ||
| if (!activeUser?.isSuperUser) { | ||
| return []; | ||
| } | ||
| return [ | ||
| { | ||
| name: "Toggle Action Logging", | ||
| command: () => (WkDevFlags.logActions = !WkDevFlags.logActions), | ||
| color: commandEntryColor, | ||
| }, | ||
| ]; | ||
| }; |
There was a problem hiding this comment.
For me, it is also fine to only have this available via webknossos.DEV, but my argument is that these cool helping tools should somehow be discoverable or documented for us developers. E.g.: I'd never know about the shader editor if you wouldn't have shown it to me. This would go the same for this feature. So the usage of webknossos.DEV seems to me to be dependent on the DEVs knowledge of such an implementation and what I am requesting is a way we can share this knowledge. Or do you think that the devs should look into wk_dev.ts in case they are debugging and maybe find something there? Because that is at least not my current workflow.
| const minZ = Math.min(...zValues); | ||
| const maxZ = Math.max(...zValues); | ||
|
|
||
| const curvesByZ: Record<number, THREE.CatmullRomCurve3> = {}; |
There was a problem hiding this comment.
Neat, thanks a lot. That really helps 🙏
As everything is sorted by z axis. Did you try how well the tool works if trying to split in another viewport besides the xy viewport? Maybe it is possible to generate plausible skeletons in these other viewports without meeting the "at least 3 points per z coordinate requirement" leading to no mesh being generated 🤔?
frontend/javascripts/oxalis/model/sagas/split_boundary_mesh_saga.ts
Outdated
Show resolved
Hide resolved
| @@ -100,6 +100,7 @@ export default function computeSplitBoundaryMeshWithSplines(points: Vector3[]): | |||
|
|
|||
| // Number of points per curve | |||
| const numPoints = 50; | |||
| const numDivisions = numPoints - 1; | |||
There was a problem hiding this comment.
why is this now 49?
There was a problem hiding this comment.
there should be 50 points on the spline. this means that the line will be divided into 49 "sections". if threejs' getPoints function would require the point count, this would not be necessary. however, it requests a division count.
There was a problem hiding this comment.
Ah, ok a little unexpected. Thanks for the clarification 👍 (was unable to find the docs for getPoints in the catmull curve -> it is inherited as I just noticed 🙈)
There was a problem hiding this comment.
yeah, I only saw it because my code editor showed the functions signature (thanks to TS).
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
frontend/javascripts/oxalis/model/sagas/annotation_tool_saga.ts (1)
24-32:⚠️ Potential issueGuard against empty toolkit arrays.
If
Toolkits[activeToolkit]is ever empty, indexing[0]will return undefined. It's safer to add a check before attempting to access the first element.function* ensureActiveToolIsInToolkit() { const activeToolkit = yield* select((state) => state.userConfiguration.activeToolkit); const activeTool = yield* select((state) => state.uiInformation.activeTool); const availableTools = Toolkits[activeToolkit]; + if (!availableTools?.length) { + console.warn("No tools available for the active toolkit. Please verify configuration."); + return; + } if (!availableTools.includes(activeTool)) { yield put(setToolAction(availableTools[0])); } }
🧹 Nitpick comments (10)
frontend/javascripts/oxalis/view/action-bar/tools/toolkit_switcher_view.tsx (1)
53-67: Consider extracting badge styling to constantsThe badge styling contains hardcoded values including a high z-index (1000). For better maintainability, consider extracting these styles to a constants file.
<Badge dot={activeToolkit !== Toolkit.ALL_TOOLS} - style={{ - boxShadow: "none", - background: "red", - zIndex: 1000, - }} + style={TOOLKIT_INDICATOR_BADGE_STYLE} >Then define the constant in
tool_helpers.tsx:export const TOOLKIT_INDICATOR_BADGE_STYLE = { boxShadow: "none", background: "red", zIndex: 1000, };frontend/javascripts/oxalis/view/action-bar/tools/brush_presets.tsx (2)
78-87: Simplify conditional assignment of brush sizesThe current code unnecessarily uses optional chaining after already checking if
presetBrushSizesis null. This can be simplified.let smallBrushSize: number, mediumBrushSize: number, largeBrushSize: number; if (presetBrushSizes == null) { smallBrushSize = defaultBrushSizes.small; mediumBrushSize = defaultBrushSizes.medium; largeBrushSize = defaultBrushSizes.large; } else { - smallBrushSize = presetBrushSizes?.small; - mediumBrushSize = presetBrushSizes?.medium; - largeBrushSize = presetBrushSizes?.large; + smallBrushSize = presetBrushSizes.small; + mediumBrushSize = presetBrushSizes.medium; + largeBrushSize = presetBrushSizes.large; }
254-256: Document calculateMediumBrushSize functionThe
calculateMediumBrushSizefunction uses a specific formula that would benefit from a comment explaining the rationale behind the calculation.function calculateMediumBrushSize(maximumBrushSize: number) { + // Calculate a medium brush size that scales with the maximum size + // but remains reasonable for practical use (roughly 1/10 of the range) return Math.ceil((maximumBrushSize - userSettings.brushSize.minimum) / 10) * 5; }frontend/javascripts/oxalis/view/action-bar/tools/toolbar_view.tsx (3)
115-124: Consider using a more informative icon for the split workflow tagThe current implementation uses an InfoCircleOutlined icon with an orange Tag, which might signal a warning. Since this is informational rather than a warning, consider using a different icon or color.
{isSplitToolkit ? ( <FastTooltip title={`Some tools behave differently because the "Split Segments" toolkit is active. Read more in the documentation.`} > - <Tag style={{ marginLeft: 12 }} color="orange"> - <InfoCircleOutlined style={{ marginRight: 4 }} /> + <Tag style={{ marginLeft: 12 }} color="blue"> + <InfoCircleOutlined style={{ marginRight: 4 }} /> Split Workflow </Tag> </FastTooltip> ) : null}
129-141: Consider breaking down ToolSpecificSettings into smaller componentsThe
ToolSpecificSettingscomponent has many props and conditional rendering logic. Consider breaking it down into smaller, more focused components for better maintainability.For example, you could create separate components like
BrushToolSettings,MeasurementToolSettings, etc., and conditionally render them based on the active tool.
202-219: Extract QuickSelect AI button to a separate componentThe Quick Select AI toggle button implementation is quite complex and would be more maintainable as a separate component.
Consider extracting this to a new component like
QuickSelectAITogglein the volume_specific_ui.tsx file, similar to other tool-specific components.frontend/javascripts/oxalis/view/action-bar/tools/skeleton_specific_ui.tsx (1)
164-180: Consider using a more maintainable approach for icon positioningThe absolute positioning of the plus icon might break if the base styles change. Consider using a more robust approach, such as wrapping both icons in a container with relative positioning.
<ButtonComponent onClick={handleCreateTree} style={{ ...NARROW_BUTTON_STYLE, paddingRight: 5 }} title={`Create a new Tree (C) – ${activeTreeHint}`} > + <div style={{ position: 'relative', display: 'inline-block' }}> <i style={{ opacity: 0.9, transform: "scale(0.9) translate(-2px, -1px)", marginRight: 3, }} className="fas fa-project-diagram" /> <i className="fas fa-plus" style={{ - position: "absolute", - top: 13, - left: 21, + position: "absolute", + bottom: -5, + right: -5, fontSize: 11, }} /> + </div> </ButtonComponent>frontend/javascripts/oxalis/view/action-bar/tools/tool_buttons.tsx (2)
104-108: Consider adding a JSDoc comment to document this helper function.The
getIsVolumeModificationAllowedfunction is used by multiple tool components. Since it encapsulates an important permission check, adding a JSDoc comment would help explain the conditions under which volume modification is allowed.+/** + * Determines if volume modification is allowed based on annotation state. + * Volume modification requires: volume data is present, annotation is not read-only, + * and there is no editable mapping active. + * @param state The current application state + * @returns boolean indicating if volume modification is allowed + */ function getIsVolumeModificationAllowed(state: OxalisState) { const isReadOnly = !state.annotation.restrictions.allowUpdate; const hasVolume = state.annotation?.volumes.length > 0; return hasVolume && !isReadOnly && !hasEditableMapping(state); }
92-100: Consider standardizing icon opacity pattern across all tools.Most tools set icon opacity based on the disabled state, but the pattern varies slightly between components. For instance, the Skeleton tool sets opacity inline, while the Trace tool combines it with other styles using the spread operator.
For consistency, consider standardizing how opacity is applied to icons across all tool components.
// Example of a standardized approach for one of the components <img src="/assets/images/quick-select-tool.svg" alt="Quick Select Icon" style={{ - opacity: disabledInfosForTools[AnnotationTool.QUICK_SELECT.id].isDisabled ? 0.5 : 1, - ...IMG_STYLE_FOR_SPACEY_ICONS, + ...IMG_STYLE_FOR_SPACEY_ICONS, + opacity: disabledInfosForTools[AnnotationTool.QUICK_SELECT.id].isDisabled ? 0.5 : 1, }} />Also applies to: 185-198, 303-312
frontend/javascripts/oxalis/view/action-bar/tools/volume_specific_ui.tsx (1)
313-337: Consider moving NUX state update to a Redux action.The
NuxPopConfirmcomponent directly calls the API and dispatches an action with the result. This mixes API call and state management logic in a UI component. Consider moving this logic to a saga or action creator for better separation of concerns.// In a new file like frontend/javascripts/oxalis/model/actions/nux_actions.ts export const updateNuxStateAction = (nuxKey: keyof APIUser["novelUserExperienceInfos"], value: boolean) => ({ type: "UPDATE_NUX_STATE", nuxKey, value, }); // In a saga file function* handleUpdateNuxState(action) { const { nuxKey, value } = action; const activeUser = yield select(state => state.activeUser); if (!activeUser) return; try { const [newUserSync] = yield call(updateNovelUserExperienceInfos, activeUser, { [nuxKey]: value, }); yield put(setActiveUserAction(newUserSync)); } catch (error) { console.error("Failed to update NUX state:", error); } } // Then in the component <Popconfirm open title="Did you know?" showCancel={false} onConfirm={() => { dispatch(updateNuxStateAction("hasSeenSegmentAnythingWithDepth", true)); }} // ...rest of props >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
frontend/javascripts/libs/order_points_with_mst.ts(1 hunks)frontend/javascripts/oxalis/controller/viewmodes/plane_controller.tsx(13 hunks)frontend/javascripts/oxalis/model/sagas/annotation_tool_saga.ts(5 hunks)frontend/javascripts/oxalis/model/sagas/split_boundary_mesh_saga.ts(1 hunks)frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx(0 hunks)frontend/javascripts/oxalis/view/action-bar/tools/brush_presets.tsx(1 hunks)frontend/javascripts/oxalis/view/action-bar/tools/skeleton_specific_ui.tsx(1 hunks)frontend/javascripts/oxalis/view/action-bar/tools/tool_buttons.tsx(1 hunks)frontend/javascripts/oxalis/view/action-bar/tools/tool_helpers.tsx(1 hunks)frontend/javascripts/oxalis/view/action-bar/tools/toolbar_view.tsx(1 hunks)frontend/javascripts/oxalis/view/action-bar/tools/toolkit_switcher_view.tsx(1 hunks)frontend/javascripts/oxalis/view/action-bar/tools/volume_specific_ui.tsx(1 hunks)frontend/javascripts/oxalis/view/action-bar/view_modes_view.tsx(4 hunks)frontend/javascripts/oxalis/view/action_bar_view.tsx(6 hunks)
💤 Files with no reviewable changes (1)
- frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx
✅ Files skipped from review due to trivial changes (1)
- frontend/javascripts/oxalis/controller/viewmodes/plane_controller.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/javascripts/oxalis/view/action-bar/view_modes_view.tsx
- frontend/javascripts/libs/order_points_with_mst.ts
- frontend/javascripts/oxalis/model/sagas/split_boundary_mesh_saga.ts
- frontend/javascripts/oxalis/view/action_bar_view.tsx
🧰 Additional context used
🧬 Code Graph Analysis (5)
frontend/javascripts/oxalis/view/action-bar/tools/toolkit_switcher_view.tsx (3)
frontend/javascripts/oxalis/store.ts (1)
OxalisState(608-644)frontend/javascripts/oxalis/model/actions/settings_actions.ts (1)
updateUserSettingAction(55-63)frontend/javascripts/oxalis/view/action-bar/tools/tool_helpers.tsx (1)
NARROW_BUTTON_STYLE(8-11)
frontend/javascripts/oxalis/view/action-bar/tools/brush_presets.tsx (9)
frontend/javascripts/oxalis/model/actions/settings_actions.ts (1)
updateUserSettingAction(55-63)frontend/javascripts/oxalis/store.ts (2)
BrushPresets(325-329)OxalisState(608-644)frontend/javascripts/oxalis/constants.ts (1)
Unicode(235-239)frontend/javascripts/oxalis/model/accessors/volumetracing_accessor.ts (1)
getMaximumBrushSize(262-273)frontend/javascripts/types/schemas/user_settings.schema.ts (1)
userSettings(10-152)frontend/javascripts/oxalis/model/accessors/view_mode_accessor.ts (1)
getViewportExtents(58-61)frontend/javascripts/oxalis/model/actions/volumetracing_actions.ts (1)
setMousePositionAction(333-337)frontend/javascripts/components/fast_tooltip.tsx (1)
FastTooltip(54-123)frontend/javascripts/oxalis/view/components/setting_input_views.tsx (1)
LogSliderSetting(147-241)
frontend/javascripts/oxalis/view/action-bar/tools/skeleton_specific_ui.tsx (10)
frontend/javascripts/oxalis/store.ts (1)
OxalisState(608-644)frontend/javascripts/oxalis/model/actions/settings_actions.ts (1)
updateUserSettingAction(55-63)frontend/javascripts/libs/utils.ts (1)
isUserAdminOrManager(496-498)frontend/javascripts/libs/react_helpers.tsx (1)
useIsActiveUserAdminOrManager(132-135)frontend/javascripts/oxalis/model/accessors/volumetracing_accessor.ts (1)
getActiveSegmentationTracing(333-333)frontend/javascripts/oxalis/model/actions/skeletontracing_actions.tsx (2)
setMergerModeEnabledAction(546-550)createTreeAction(301-311)frontend/javascripts/oxalis/view/components/button_component.tsx (1)
ToggleButton(64-67)frontend/javascripts/oxalis/view/action-bar/tools/tool_helpers.tsx (2)
NARROW_BUTTON_STYLE(8-11)IMG_STYLE_FOR_SPACEY_ICONS(12-18)frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx (1)
MaterializeVolumeAnnotationModal(1157-1275)frontend/javascripts/oxalis/model/accessors/skeletontracing_accessor.ts (1)
getActiveTree(82-93)
frontend/javascripts/oxalis/view/action-bar/tools/tool_helpers.tsx (2)
frontend/javascripts/libs/window.ts (1)
document(19-34)frontend/javascripts/components/fast_tooltip.tsx (1)
FastTooltip(54-123)
frontend/javascripts/oxalis/view/action-bar/tools/tool_buttons.tsx (7)
frontend/javascripts/oxalis/model/accessors/tool_accessor.ts (1)
AnnotationToolId(11-11)frontend/javascripts/oxalis/model/accessors/volumetracing_accessor.ts (3)
getRenderableMagForActiveSegmentationTracing(561-563)hasEditableMapping(591-600)hasAgglomerateMapping(797-826)frontend/javascripts/components/fast_tooltip.tsx (1)
FastTooltip(54-123)frontend/javascripts/oxalis/view/action-bar/tools/tool_helpers.tsx (2)
ToolRadioButton(67-92)IMG_STYLE_FOR_SPACEY_ICONS(12-18)frontend/javascripts/oxalis/store.ts (1)
OxalisState(608-644)frontend/javascripts/oxalis/model/accessors/disabled_tool_accessor.ts (1)
getDisabledInfoForTools(366-368)frontend/javascripts/oxalis/model/actions/dataset_actions.ts (1)
ensureLayerMappingsAreLoadedAction(47-51)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (21)
frontend/javascripts/oxalis/view/action-bar/tools/toolkit_switcher_view.tsx (1)
9-26: Well-structured toolkit options definitionThe toolkit options are clearly defined with descriptive labels and corresponding keys from the Toolkit enum. This makes it easy to understand and maintain the available toolkits.
frontend/javascripts/oxalis/view/action-bar/tools/brush_presets.tsx (1)
89-95: Well-implemented brush centeringThe centering of the brush in the viewport when opening the popover is well implemented, calculating the center position based on the active viewport.
frontend/javascripts/oxalis/view/action-bar/tools/toolbar_view.tsx (2)
62-74: Good documentation for tool radio group logicThe
toolToRadioGroupValuefunction is well-documented, clearly explaining the special handling for measurement tools in the radio group.
99-105: Efficient tool button renderingThe code efficiently uses the toolkit configuration to determine which tools to render, and uses a component lookup pattern to render the appropriate button component for each tool.
frontend/javascripts/oxalis/view/action-bar/tools/skeleton_specific_ui.tsx (4)
22-69: Well-implemented conditional rendering based on state and permissionsThe component properly checks multiple conditions (merger mode, editable mappings, user permissions) to determine which buttons to show and which to disable. The tooltips are informative and context-sensitive.
77-90: Good conditional rendering based on active toolkitThe component conditionally renders the skeleton-specific buttons based on whether the Split Segments toolkit is active, which makes sense as some buttons may not be applicable in that mode.
109-116: Well-implemented continuous node creation buttonThe continuous node creation toggle button is straightforward and has a clear tooltip explaining its functionality. This new "draw mode" feature mentioned in the PR objectives is well implemented.
136-147: Nice active tree visualization with color badgeThe
CreateTreeButtoncomponent provides helpful visual feedback by displaying the active tree's color in a badge. The tooltip also provides useful information about the active tree.frontend/javascripts/oxalis/view/action-bar/tools/tool_helpers.tsx (3)
8-18: Good exportable style constants!Having these centralized styles for buttons and icons promotes consistency across the UI.
20-65: Well-designed RadioButtonWithTooltip component with appropriate focus management.This component effectively wraps Ant Design's Radio.Button with a tooltip and handles edge cases well. The blur on click improves the user experience by removing focus indicators after selection, and the tooltip integration is nicely commented.
Note the clever pattern of removing padding from the button itself and adding it to the inner span, ensuring the tooltip appears correctly when hovering anywhere on the button.
67-92: Clean implementation of ToolRadioButton.This component builds nicely on the RadioButtonWithTooltip to provide a consistent format for tool tooltips, combining the name and description (or disabled explanation) in a standard format.
frontend/javascripts/oxalis/view/action-bar/tools/tool_buttons.tsx (4)
38-53: Helpful multi-slice annotation info icon with clear user feedback.This component provides useful visual feedback when users are annotating at low magnification where actions might affect multiple slices. The conditional display based on magnification is a good UX choice.
132-133: Consistent pattern for showing multi-slice annotation info.The pattern of conditionally showing the multi-slice annotation icon when the tool is active is applied consistently across all relevant tools. This provides a clean and consistent UX for the user.
Also applies to: 168-169, 197-198, 230-232, 259-261
344-391: Good preloading pattern in ProofreadTool component.The
onMouseEnterhandler dispatches an action to ensure layer mappings are loaded, which is a nice way to preload data before the user actually clicks the button. This reduces perceived latency when the tool is activated.
19-36:❓ Verification inconclusive
Well-organized mapping of tool IDs to components.
The ToolIdToComponent record provides a clean way to map from tool IDs to their respective React components. This makes the code more maintainable and allows for easy tool registration.
One small note: The Area Measurement tool maps to a null component function. Is this intentional because the UI for this tool is handled elsewhere, or is this a placeholder for future implementation?
🏁 Script executed:
#!/bin/bash # Check where AREA_MEASUREMENT tool might be used rg -A 5 "AREA_MEASUREMENT" --type ts --type tsxLength of output: 75
It looks like ripgrep didn’t recognize the
tsxtype. Let’s search without restricting file types:
🏁 Script executed:
#!/bin/bash # Search for any references to AREA_MEASUREMENT across .ts and .tsx files rg -n "AREA_MEASUREMENT" -g '*.{ts,tsx}'Length of output: 2268
Toolbar Mapping: AREA_MEASUREMENT Returns Null
TheToolIdToComponententry forAnnotationTool.AREA_MEASUREMENT.idis currently a no-op (() => null), which means no button will appear in the action bar for the area-measurement tool. If you intentionally handle the area-measurement UI via the floating controls inplane_controller/tool_controls, this is fine. Otherwise, you may want to supply and register a dedicated button component here to keep toolbar behavior consistent.• File: frontend/javascripts/oxalis/view/action-bar/tools/tool_buttons.tsx
Lines 34–35[AnnotationTool.AREA_MEASUREMENT.id]: () => null,Please confirm whether this placeholder is intentional or if we should add an actual toolbar component for the area-measurement tool.
frontend/javascripts/oxalis/model/sagas/annotation_tool_saga.ts (2)
34-87: Well-designed tool switching mechanism with smart restoration logic.The
switchAwayFromDisabledToolsaga intelligently handles the case when a tool becomes disabled:
- It automatically switches to the MOVE tool as a fallback
- It remembers the previously disabled tool
- It automatically restores the previous tool if it becomes available again
- It clears memory if the user explicitly selects another tool
The debouncing approach with
yield delay(50)is a good compromise to reduce the performance impact of listening to all actions without needing to maintain an exhaustive list of relevant actions.
131-149: Clean orchestration in the main toolSaga.The main saga neatly orchestrates:
- Setting the right initial state with
ensureWkReadyand toolkit setup for view mode- Forking the worker sagas to handle specific responsibilities
- Setting up watchers for relevant state changes
The code follows good saga patterns by separating concerns and making each saga responsible for a specific aspect of tool management.
frontend/javascripts/oxalis/view/action-bar/tools/volume_specific_ui.tsx (4)
97-147: Excellent modifier key handling in OverwriteModeSwitch.The
OverwriteModeSwitchcomponent implements a sophisticated system for toggling between overwrite modes using keyboard modifiers. The code includes detailed comments explaining the various states and toggle logic, which is very helpful for understanding the complex interactions.Note that the component continues to process key events even when not visible, which is essential for consistent behavior with keyboard shortcuts.
221-231: Good useMemo optimization for buttonsRender.The
useCallbackhook with appropriate dependencies ensures that thebuttonsRenderfunction is only recreated whentooltipTitleorisDisabledchanges. This is a good optimization for a function passed to a child component.
382-416: Good toggle button with clear visual and textual feedback.The
FloodFillSettingscomponent provides clear feedback on the current state through both visual styling (opacity and button type) and a descriptive tooltip explaining how the restriction works. This is excellent for usability.
456-491: Nicely organized compact button group for proofreading.The ProofreadingComponents uses Space.Compact to create a visually cohesive set of related buttons. Each button has clear tooltips explaining its function and provides immediate visual feedback on the current state.
| const handleMenuClick: MenuProps["onClick"] = (args) => { | ||
| const toolkit = args.key; | ||
| Store.dispatch(updateUserSettingAction("activeToolkit", toolkit as Toolkit)); | ||
| // @ts-expect-error ts-migrate(2339) FIXME: Property 'blur' does not exist on type 'Element'. | ||
| args.domEvent.target.blur(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix TypeScript error in event handling
There's a TypeScript error in the handleMenuClick function that needs to be addressed.
const handleMenuClick: MenuProps["onClick"] = (args) => {
const toolkit = args.key;
Store.dispatch(updateUserSettingAction("activeToolkit", toolkit as Toolkit));
- // @ts-expect-error ts-migrate(2339) FIXME: Property 'blur' does not exist on type 'Element'.
- args.domEvent.target.blur();
+ (args.domEvent.target as HTMLElement).blur();
};📝 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 handleMenuClick: MenuProps["onClick"] = (args) => { | |
| const toolkit = args.key; | |
| Store.dispatch(updateUserSettingAction("activeToolkit", toolkit as Toolkit)); | |
| // @ts-expect-error ts-migrate(2339) FIXME: Property 'blur' does not exist on type 'Element'. | |
| args.domEvent.target.blur(); | |
| }; | |
| const handleMenuClick: MenuProps["onClick"] = (args) => { | |
| const toolkit = args.key; | |
| Store.dispatch(updateUserSettingAction("activeToolkit", toolkit as Toolkit)); | |
| (args.domEvent.target as HTMLElement).blur(); | |
| }; |
| onOpenChange={(open: boolean) => { | ||
| setIsBrushSizePopoverOpen(open); | ||
| if (open) centerBrushInViewport(); | ||
| else dispatch(setMousePositionAction(null)); | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clean up brush position when component unmounts
The component properly cleans up the mouse position when the popover is closed, but you should also ensure it's cleaned up if the component unmounts while the popover is open.
Add a cleanup function to the component:
useEffect(() => {
if (presetBrushSizes == null) {
handleUpdatePresetBrushSizes(defaultBrushSizes);
}
}, [presetBrushSizes]);
+ useEffect(() => {
+ return () => {
+ // Clean up mouse position on unmount
+ dispatch(setMousePositionAction(null));
+ };
+ }, [dispatch]);| // biome-ignore lint/correctness/useExhaustiveDependencies: Needs investigation whether defaultBrushSizes is needed as dependency. | ||
| useEffect(() => { | ||
| if (presetBrushSizes == null) { | ||
| handleUpdatePresetBrushSizes(defaultBrushSizes); | ||
| } | ||
| }, [presetBrushSizes]); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix useEffect dependency warning
The linter is warning about missing dependencies in the useEffect. This should be investigated and fixed to prevent potential bugs.
The comment indicates defaultBrushSizes might need to be added as a dependency. Let's analyze if it's necessary:
🏁 Script executed:
#!/bin/bash
# Check where defaultBrushSizes is used in the useEffect
rg -A 2 -B 2 "defaultBrushSizes" frontend/javascripts/oxalis/view/action-bar/tools/brush_presets.tsxLength of output: 1135
🏁 Script executed:
#!/bin/bash
# Locate handleUpdatePresetBrushSizes definition and usage context
rg -A 3 -B 3 "handleUpdatePresetBrushSizes" -n frontend/javascripts/oxalis/view/action-bar/tools/brush_presets.tsxLength of output: 1621
Fix useEffect dependency warning
Add defaultBrushSizes to the useEffect dependency array so the hook will re-run whenever its value changes. To fully satisfy the exhaustive-deps rule, you can also memoize defaultBrushSizes (and/or wrap handleUpdatePresetBrushSizes in useCallback) and then remove the biome-ignore comment.
• File: frontend/javascripts/oxalis/view/action-bar/tools/brush_presets.tsx
- Memoize
defaultBrushSizesto stabilize its identity:const defaultBrushSizes = useMemo( () => getDefaultBrushSizes(maximumBrushSize, userSettings.brushSize.minimum), [maximumBrushSize, userSettings.brushSize.minimum] );
- (Optional) Wrap dispatcher in
useCallbackto silence linter on the updater function:const handleUpdatePresetBrushSizes = useCallback( (brushSizes: BrushPresets) => Store.dispatch(updateUserSettingAction("presetBrushSizes", brushSizes)), [] );
- Update the
useEffectto includedefaultBrushSizesin deps and remove the ignore comment:- // biome-ignore lint/correctness/useExhaustiveDependencies: Needs investigation whether defaultBrushSizes is needed as dependency. - useEffect(() => { + useEffect(() => { if (presetBrushSizes == null) { handleUpdatePresetBrushSizes(defaultBrushSizes); } - }, [presetBrushSizes]); + }, [presetBrushSizes, defaultBrushSizes]);
After making these changes, you can delete the // biome-ignore… line.
| value={overwriteMode} | ||
| // @ts-expect-error ts-migrate(2322) FIXME: Type '(event: { target: { value: Overwri... Remove this comment to see the full error message | ||
| onChange={handleSetOverwriteMode} | ||
| style={{ | ||
| marginLeft: 10, | ||
| }} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix TypeScript error in event handler type.
There's a TypeScript error that's being suppressed with a @ts-expect-error comment. Since this is a new file, it would be better to fix the underlying type issue rather than suppressing it.
<Radio.Group
value={overwriteMode}
- // @ts-expect-error ts-migrate(2322) FIXME: Type '(event: { target: { value: Overwri... Remove this comment to see the full error message
- onChange={handleSetOverwriteMode}
+ onChange={handleSetOverwriteMode as unknown as ((e: RadioChangeEvent) => void)}
style={{
marginLeft: 10,
}}
>Alternatively, if you have time to properly fix the type issue, update the handleSetOverwriteMode function signature to match what Radio.Group expects:
-const handleSetOverwriteMode = (event: {
- target: {
- value: OverwriteMode;
- };
-}) => {
+const handleSetOverwriteMode = (event: RadioChangeEvent) => {
Store.dispatch(updateUserSettingAction("overwriteMode", event.target.value as OverwriteMode));
};📝 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.
| value={overwriteMode} | |
| // @ts-expect-error ts-migrate(2322) FIXME: Type '(event: { target: { value: Overwri... Remove this comment to see the full error message | |
| onChange={handleSetOverwriteMode} | |
| style={{ | |
| marginLeft: 10, | |
| }} | |
| > | |
| <Radio.Group | |
| value={overwriteMode} | |
| onChange={handleSetOverwriteMode as unknown as ((e: RadioChangeEvent) => void)} | |
| style={{ | |
| marginLeft: 10, | |
| }} | |
| > |
philippotto
changed the title
This PR:
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)