Warn user when using precomputed meshes after brushing #8218
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several enhancements related to volume tracing, primarily through the addition of a Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
…et data has changed
philippotto
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (12)
frontend/javascripts/oxalis/model/sagas/volumetracing_saga.tsx (1)
Line range hint
607-618: Specify more precise return type instead of usinganyIn the
ensureToolIsAllowedInMagfunction, replaceSaga<any>with a more specific return type to improve type safety and code clarity.Apply this change to specify the return type:
-export function* ensureToolIsAllowedInMag(): Saga<any> { +export function* ensureToolIsAllowedInMag(): Saga<void> {frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/load_mesh_menu_item.tsx (3)
12-14: Consider adding explicit null checksThe warning logic uses nullish coalescing, but it might be clearer to add explicit null checks to handle edge cases more explicitly.
- const showWarning = - volumeTracing?.volumeBucketDataHasChanged ?? - (!volumeTracing?.hasEditableMapping && volumeTracing?.mappingIsLocked); + const showWarning = volumeTracing != null && ( + volumeTracing.volumeBucketDataHasChanged || + (!volumeTracing.hasEditableMapping && volumeTracing.mappingIsLocked) + );
16-33: UI implementation looks good with a minor suggestionThe UI implementation is clean and follows best practices. Consider extracting the warning tooltip text to a constant since it's a user-facing message that might need to be maintained or internationalized in the future.
+const WARNING_MESSAGE = "Warning: The segmentation data has changed since the mesh file was created. The mesh may not match the current data."; export function LoadMeshMenuItemLabel({ currentMeshFile, volumeTracing }: Props) { // ... rest of the code ... {showWarning && ( - <FastTooltip title="Warning: The segmentation data has changed since the mesh file was created. The mesh may not match the current data."> + <FastTooltip title={WARNING_MESSAGE}>
1-34: Overall implementation successfully addresses the PR objectivesThe component effectively implements the warning system for precomputed meshes after brushing, as specified in issue #7805. The implementation:
- Properly handles different states of mesh files and volume tracing
- Provides clear visual feedback with tooltips
- Uses appropriate UI components and styling
This implementation should successfully prevent users from encountering errors when working with precomputed meshes for modified segments.
Consider adding error boundary handling around this component to gracefully handle any potential runtime errors in the warning logic.
webknossos-datastore/proto/VolumeTracing.proto (1)
51-51: LGTM! Consider documenting field relationship.The new field is well-structured and directly supports the PR objective of implementing warnings for modified segments. The field numbering and optional status maintain backward compatibility.
Consider expanding the comment to document the relationship with
mappingIsLocked:- optional bool volumeBucketDataHasChanged = 24; // The volume bucket data has been edited at least once + optional bool volumeBucketDataHasChanged = 24; // The volume bucket data has been edited at least once. When true, mappingIsLocked should also be set to prevent mapping changes.frontend/javascripts/oxalis/model/sagas/quick_select_saga.ts (1)
35-39: Update the comment to reflect the new behavior.The current comment only mentions mapping locks, but the new code does more than that. It checks whether bucket modifications are allowed based on the volume tracing state.
- // As changes to the volume layer will be applied, the potentially existing mapping should be locked to ensure a consistent state. + // Check if bucket modifications are allowed (e.g., segments haven't been brushed) before applying changes to the volume layerfrontend/javascripts/oxalis/model/sagas/saga_helpers.ts (1)
116-146: LGTM! Consider these minor improvements.The implementation effectively handles both mapping locks and bucket modification tracking. A few suggestions for improvement:
- Consider defining the action type as a constant to maintain consistency and prevent typos:
export const SET_VOLUME_BUCKET_DATA_HAS_CHANGED = "SET_VOLUME_BUCKET_DATA_HAS_CHANGED";
- Consider adding JSDoc tags for better documentation:
/** * Requests permission to modify bucket data in volume tracing * @param volumeTracing - The volume tracing to be modified * @returns A saga that resolves to true if modification is allowed, false otherwise */frontend/javascripts/oxalis/model/actions/volumetracing_actions.ts (1)
Based on the evidence gathered, I can now provide the final response:
The
SET_VOLUME_BUCKET_DATA_HAS_CHANGEDaction should be added toVolumeTracingSaveRelevantActions.The
volumeBucketDataHasChangedflag is a persistent state that:
- Is initialized as
falsein the annotation service- Gets persisted in the VolumeTracing proto definition
- Is used by the VolumeTracingService to track bucket data modifications
- Affects UI behavior (e.g., showing warnings in the mesh loading menu)
Since this state needs to persist and is critical for tracking volume data modifications, its corresponding action should be included in the save-relevant actions.
🔗 Analysis chain
Line range hint
108-124: Consider adding the new action to VolumeTracingSaveRelevantActions.If the volume bucket data change state needs to persist, consider adding
"SET_VOLUME_BUCKET_DATA_HAS_CHANGED"to theVolumeTracingSaveRelevantActionsarray.Let's check if other similar state-changing actions are included in the save relevant actions:
Let me gather more information to understand if this state change needs to persist.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check similar actions in VolumeTracingSaveRelevantActions # Find the reducer to see if this state needs to persist fd volumetracing_reducer -e ts -x cat {} \; | grep -A 10 "SET_VOLUME_BUCKET_DATA_HAS_CHANGED"Length of output: 401
Script:
#!/bin/bash # Check how volumeBucketDataHasChanged is used in the codebase rg "volumeBucketDataHasChanged" -B 2 -A 2 # Check if there are any save/persistence related functions that use this flag ast-grep --pattern 'function $FUNC_NAME($_) { $$$ volumeBucketDataHasChanged $$$ }'Length of output: 4854
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts (1)
443-447: Consider adding more test cases for bucket modification scenarios.While the test correctly verifies the basic functionality of
requestBucketModificationInVolumeTracing, consider adding test cases for:
- Bucket modification with different volume tracing states
- Error scenarios
- Multiple consecutive modifications
Example test structure:
test("requestBucketModificationInVolumeTracing should handle multiple modifications", (t) => { const saga = editVolumeLayerAsync(); // Setup initial state // Perform multiple modifications // Verify state changes }); test("requestBucketModificationInVolumeTracing should handle error scenarios", (t) => { const saga = editVolumeLayerAsync(); // Setup error state // Verify error handling });frontend/javascripts/oxalis/model/reducers/volumetracing_reducer.ts (1)
380-385: Consider documenting the volumeBucketDataHasChanged flag's lifecycle.The implementation integrates well with the existing reducer architecture. To improve maintainability, consider adding documentation about:
- When this flag gets reset
- Which components consume this state
- How this interacts with other volume tracing states
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingService.scala (1)
213-213: Consider extracting volume modification tracking into a helper method.The logic for setting
volumeBucketDataHasChangedis duplicated. Consider creating a helper method to ensure consistent behavior across all volume modification points:private def withVolumeModificationTracking(tracing: VolumeTracing): VolumeTracing = { tracing.copy(volumeBucketDataHasChanged = Some(true)) }This would make the code more maintainable and ensure consistent behavior if more volume modification methods need this tracking in the future.
Also applies to: 284-284
frontend/javascripts/oxalis/view/context_menu.tsx (1)
1440-1446: LGTM with a minor suggestion.The hooks are used correctly and the segment index loading logic is properly implemented. Consider destructuring the dataset selector for better readability.
- const dataset = useSelector((state: OxalisState) => state.dataset); + const { dataset } = useSelector((state: OxalisState) => ({ + dataset: state.dataset, + }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
app/models/annotation/AnnotationService.scala(1 hunks)frontend/javascripts/oxalis/model/actions/volumetracing_actions.ts(3 hunks)frontend/javascripts/oxalis/model/reducers/volumetracing_reducer.ts(1 hunks)frontend/javascripts/oxalis/model/sagas/quick_select_saga.ts(2 hunks)frontend/javascripts/oxalis/model/sagas/saga_helpers.ts(1 hunks)frontend/javascripts/oxalis/model/sagas/volume/volume_interpolation_saga.ts(2 hunks)frontend/javascripts/oxalis/model/sagas/volumetracing_saga.tsx(4 hunks)frontend/javascripts/oxalis/store.ts(1 hunks)frontend/javascripts/oxalis/view/context_menu.tsx(4 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/load_mesh_menu_item.tsx(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx(4 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts(2 hunks)webknossos-datastore/proto/VolumeTracing.proto(1 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingService.scala(2 hunks)
🔇 Additional comments (24)
frontend/javascripts/oxalis/model/sagas/volumetracing_saga.tsx (2)
85-85: Import requestBucketModificationInVolumeTracing correctly added
The addition of requestBucketModificationInVolumeTracing to the imports ensures that the function is available for usage in the sagas.
Line range hint 607-618: Verify that ensureToolIsAllowedInMag saga is properly connected
Ensure that the ensureToolIsAllowedInMag saga is correctly registered and connected to the middleware so it listens for zoom-related actions and functions as intended.
Confirm that ensureToolIsAllowedInMag is included in the saga middleware registration:
export default [
editVolumeLayerAsync,
handleDeleteSegmentData,
+ ensureToolIsAllowedInMag,
floodFill,
watchVolumeTracingAsync,
maintainSegmentsMap,frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/load_mesh_menu_item.tsx (1)
1-9: Verify VolumeTracing type structure
The imports and type definitions look good. However, let's verify the structure of the VolumeTracing type to ensure we're handling all edge cases correctly.
✅ Verification successful
Based on the search results, we can now see the VolumeTracing type definition in frontend/javascripts/oxalis/store.ts and its relationship with the server-side type. The type is well-defined and includes all necessary fields for handling volume tracing data. The imports in the reviewed file are correct and the type definitions look appropriate for the functionality.
VolumeTracing type structure is properly defined and used
The VolumeTracing type extends TracingBase and includes volume-specific properties like segments, segmentGroups, and type identifier. The imports in the reviewed file correctly reference this type structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for VolumeTracing type definition
ast-grep --pattern 'type VolumeTracing = {
$$$
}'
# Alternatively, look for interface definition
ast-grep --pattern 'interface VolumeTracing {
$$$
}'
Length of output: 110
Script:
#!/bin/bash
# Let's try different approaches to find VolumeTracing definition
# Search for files containing VolumeTracing
rg "VolumeTracing" -l
# Search for the actual definition with context
rg "VolumeTracing" -A 10
# Look for type exports containing VolumeTracing
ast-grep --pattern 'export type VolumeTracing = $_'
# Look for type aliases containing VolumeTracing
ast-grep --pattern 'type VolumeTracing = $_'
Length of output: 154986
Script:
#!/bin/bash
# Let's search for VolumeTracing type definition in frontend TypeScript files
rg "type VolumeTracing" frontend/ -A 5
# Also search for interface definition in case it's defined that way
rg "interface VolumeTracing" frontend/ -A 5
# Search in api_flow_types since it's imported from there
rg "VolumeTracing" frontend/javascripts/types/api_flow_types.ts -A 10
Length of output: 2581
frontend/javascripts/oxalis/model/sagas/quick_select_saga.ts (2)
18-18: LGTM!
The import change aligns with the new modification permission checking system.
35-39: Verify the implementation of requestBucketModificationInVolumeTracing.
Let's verify that the helper function properly implements the warning logic for precomputed meshes.
✅ Verification successful
Implementation correctly handles precomputed mesh warning logic
The implementation of requestBucketModificationInVolumeTracing properly handles the warning logic for precomputed meshes by:
- Ensuring the mapping state is locked before allowing modifications
- Prompting users for confirmation when locking is needed
- Preventing bucket modifications if users deny the locking request
- Setting
volumeBucketDataHasChangedflag when modifications are allowed
The function is used consistently across volume tracing operations (quick select, interpolation, etc.) to maintain data consistency and prevent unwanted modifications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of requestBucketModificationInVolumeTracing
# Find the implementation
ast-grep --pattern 'export function requestBucketModificationInVolumeTracing($$$) {
$$$
}'
# Find related tests
rg -l "requestBucketModificationInVolumeTracing" --glob "**/*test*"
Length of output: 162
Script:
#!/bin/bash
# Let's try a broader search to find the implementation and related code
# Search for the function name without export keyword
ast-grep --pattern 'function requestBucketModificationInVolumeTracing($$$)'
# Search for any occurrence of the function name in the codebase
rg "requestBucketModificationInVolumeTracing" -A 10
# Search for files containing the function name
fd . -t f -x grep -l "requestBucketModificationInVolumeTracing" {}
Length of output: 13065
Script:
#!/bin/bash
# Let's get more context around the implementation in saga_helpers.ts
rg -A 20 "export function\* requestBucketModificationInVolumeTracing" frontend/javascripts/oxalis/model/sagas/saga_helpers.ts
Length of output: 1298
frontend/javascripts/oxalis/model/sagas/saga_helpers.ts (1)
135-138: Verify comprehensive usage of this guard.
The function serves as a crucial guard against invalid states when using precomputed meshes. To ensure robustness:
- Consider adding more detailed error information when returning false:
return { allowed: false, reason: "User denied mapping lock" };- Let's verify that all bucket modification paths use this guard:
frontend/javascripts/oxalis/model/actions/volumetracing_actions.ts (2)
48-50: LGTM! Type definitions are well-structured.
The new action type is properly defined and integrated into the VolumeTracingAction union type, following the established patterns in the codebase.
Also applies to: 105-105
445-449: LGTM! Action creator implementation is consistent.
The setVolumeBucketDataHasChangedAction follows the established patterns:
- Takes
layerNameparameter to target specific layers - Returns a properly typed action object
- Uses const assertion for type safety
Let's verify the usage of this new action:
✅ Verification successful
The search results show that the action is properly integrated into the codebase:
- It's handled in the
volumetracing_reducer.tswhich updates the state withvolumeBucketDataHasChanged: truefor the specified layer - It's used in
saga_helpers.tsas part of saga workflow - The action type is properly included in the action types union type
Action is properly integrated and used in the codebase
The verification confirms that the action creator is not only well-implemented but also correctly integrated into the reducer and saga workflow, maintaining consistency with the application's state management patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check where this action is dispatched and handled
# Find action usage
echo "Searching for action usage..."
rg "setVolumeBucketDataHasChangedAction|SET_VOLUME_BUCKET_DATA_HAS_CHANGED" -A 3
# Find reducer handling
echo "Searching for reducer handling..."
fd -e ts -e tsx -x ast-grep --pattern 'case "SET_VOLUME_BUCKET_DATA_HAS_CHANGED": {
$$$
}'
Length of output: 2005
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts (1)
43-46: LGTM! Clean import restructuring.
The grouped import of saga helper functions is well-organized and follows TypeScript best practices.
frontend/javascripts/oxalis/model/reducers/volumetracing_reducer.ts (2)
Line range hint 1-24: Verify type definitions for the new state property.
Please ensure that:
- The
VolumeTracingtype includes thevolumeBucketDataHasChangedproperty - The
SET_VOLUME_BUCKET_DATA_HAS_CHANGEDaction is included in theVolumeTracingActiontype
✅ Verification successful
Type definitions are properly implemented
The verification shows that:
- The
VolumeTracingtype infrontend/javascripts/oxalis/store.tsalready includesreadonly volumeBucketDataHasChanged?: boolean - The
volumeBucketDataHasChangedproperty is being used in multiple places across the codebase, indicating proper integration - The reducer is correctly setting this property to
trueinvolumetracing_reducer.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check type definitions
# Expected: Find type definitions for volumeBucketDataHasChanged
# Look for VolumeTracing type definition
ast-grep --pattern 'type VolumeTracing = {
$$$
volumeBucketDataHasChanged?: boolean;
$$$
}'
# Look for action type definition
ast-grep --pattern 'type VolumeTracingAction =
$$$
| { type: "SET_VOLUME_BUCKET_DATA_HAS_CHANGED", $$$}
$$$'
Length of output: 217
Script:
#!/bin/bash
# Let's try a different approach to find these type definitions
# Search for VolumeTracing type definition
rg "type VolumeTracing" -A 20
# Search for VolumeTracingAction type definition
rg "type VolumeTracingAction" -A 20
# Also search for any usage of volumeBucketDataHasChanged
rg "volumeBucketDataHasChanged"
Length of output: 8815
380-384: LGTM! Consider adding error handling for invalid layer names.
The implementation correctly uses the updateVolumeTracing helper to set the volumeBucketDataHasChanged flag. This change aligns with the PR objective of implementing a warning system for modified segments.
Let's verify the integration with the warning system:
frontend/javascripts/oxalis/model/sagas/volume/volume_interpolation_saga.ts (2)
41-41: LGTM: Import change aligns with new bucket modification permission system
The change from ensureMaybeActiveMappingIsLocked to requestBucketModificationInVolumeTracing reflects the shift towards a more comprehensive bucket modification permission system.
439-443: Verify error handling for denied modifications
The permission check is correctly placed before performing interpolation. However, when modification is denied, consider providing user feedback about why the operation was blocked.
Let's verify the user feedback implementation:
Consider adding user feedback when modification is denied:
const isModificationAllowed = yield* call(
requestBucketModificationInVolumeTracing,
volumeTracing,
);
if (!isModificationAllowed) {
+ Toast.warning("Cannot modify segment as it has been changed after precomputed meshes were generated.");
return;
}frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx (4)
43-43: LGTM: Import of specialized component for mesh loading labels
The addition of LoadMeshMenuItemLabel aligns with the PR's objective to provide warnings for precomputed meshes.
83-83: LGTM: Added volume tracing parameter for mesh loading state
The addition of activeVolumeTracing parameter enables the component to access volume tracing state, which is necessary for determining if the volume bucket data has changed.
119-122: LGTM: Clean implementation of mesh loading label component
The LoadMeshMenuItemLabel component is properly implemented with the necessary props to handle mesh loading states and warnings.
Line range hint 228-228: Verify consistent prop usage across components
Let's verify that the activeVolumeTracing prop is consistently used across all related components.
✅ Verification successful
Props type definition is consistent with component usage
Based on the search results, the activeVolumeTracing prop is consistently used across components:
-
In
segment_list_item.tsx, it's properly typed asVolumeTracing | null | undefinedand used for:- Generating mesh menu items
- Controlling segment deletion functionality
- Passing to child components
-
The prop is correctly sourced from the Redux store using
getActiveSegmentationTracingselector in parent components likesegments_view.tsx -
The type matches its usage in related components like
statusbar.tsxwhere it's used for displaying active segment information
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of activeVolumeTracing prop
# Look for components that might need the activeVolumeTracing prop
# Search for components using LoadMeshMenuItemLabel
ast-grep --pattern 'LoadMeshMenuItemLabel $$$'
# Search for other components that might need volume tracing state
rg -l "volumeBucketDataHasChanged|VolumeTracing" --type typescript --type tsx
Length of output: 163
Script:
#!/bin/bash
# Let's try a different approach to find related components and prop usage
# Search for files containing VolumeTracing or volume-related terms
rg -l "VolumeTracing|volumeTracing|activeVolumeTracing"
# Search for the specific prop type definition
ast-grep --pattern 'activeVolumeTracing?: VolumeTracing'
# Search for components that might be using this prop
rg -A 5 "activeVolumeTracing"
Length of output: 26001
frontend/javascripts/oxalis/store.ts (2)
267-267: LGTM! Well-structured type definition.
The new volumeBucketDataHasChanged property is properly defined as an optional readonly boolean, maintaining type safety and backward compatibility.
267-267: Verify the handling of the optional property.
Please ensure that code accessing this property properly handles the undefined case. Let's verify the implementation in dependent files:
✅ Verification successful
Based on the search results, I can see the usage of volumeBucketDataHasChanged in several files. Let me analyze the handling of this optional property:
- In
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/load_mesh_menu_item.tsx, it's properly handled with the nullish coalescing operator:
volumeTracing?.volumeBucketDataHasChanged ?? (!volumeTracing?.hasEditableMapping && volumeTracing?.mappingIsLocked)- In
frontend/javascripts/oxalis/model/reducers/volumetracing_reducer.ts, it's set to a definite value:
volumeBucketDataHasChanged: true,- In the backend (Scala) code, it's handled as an Option type:
volumeTracing.copy(volumeBucketDataHasChanged = Some(true))- In the protobuf definition, it's correctly marked as optional:
optional bool volumeBucketDataHasChanged = 24;Let me verify if there are any other usages of this property that might not handle the undefined case properly:
Let me try one more search to find any potential direct property access:
Based on the complete analysis, I can now provide the final response:
The optional property is properly handled in all its usages.
The property volumeBucketDataHasChanged is:
- Correctly defined as optional with
?in the type definition - Properly accessed with optional chaining (
?.) in the UI component - Set with a definite value (
true) in the reducer - Handled as an
Optiontype in the Scala backend - Defined as optional in the protobuf schema
- Only referenced in a comment in saga_helpers.ts, not actually accessed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of volumeBucketDataHasChanged in the codebase
rg "volumeBucketDataHasChanged" -A 3
# Search for VolumeTracing type usage to identify potential initialization points
ast-grep --pattern 'type $_ = VolumeTracing'
Length of output: 83323
Script:
#!/bin/bash
# Search for direct property access without optional chaining
ast-grep --pattern 'volumeTracing.volumeBucketDataHasChanged'
# Search for any other references we might have missed
rg -l "volumeBucketDataHasChanged"
Length of output: 162417
Script:
#!/bin/bash
# Search for direct property access in TypeScript files
rg "\.volumeBucketDataHasChanged" --type ts -A 2
Length of output: 756
app/models/annotation/AnnotationService.scala (1)
180-181: LGTM! The new parameter aligns with PR objectives.
The addition of volumeBucketDataHasChanged = Some(false) to the VolumeTracing instantiation provides the necessary tracking mechanism for detecting modifications to volume bucket data, which is essential for implementing the warning system for precomputed meshes after brushing.
Let's verify how this parameter is used across the codebase:
✅ Verification successful
Based on the results, I can now generate the final response:
The volumeBucketDataHasChanged parameter is properly integrated across the codebase
The verification shows that the parameter is well-integrated and used appropriately:
- In
VolumeTracingService.scala, it's updated toSome(true)when volume data is modified - In the frontend's
volumetracing_reducer.ts, it's used to track bucket data changes - In
load_mesh_menu_item.tsx, it's used for UI decisions about mesh loading - The parameter is properly defined in the protobuf schema with clear documentation
The default value of Some(false) in AnnotationService.scala is correct as it represents the initial state before any modifications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of volumeBucketDataHasChanged to ensure proper integration
# Search for references to volumeBucketDataHasChanged
rg -A 5 'volumeBucketDataHasChanged'
# Search for VolumeTracing case class definition to verify parameter declaration
ast-grep --pattern 'case class VolumeTracing'
Length of output: 5421
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingService.scala (2)
213-213: LGTM: Correctly tracks bucket data modifications.
The change appropriately sets volumeBucketDataHasChanged after successful bucket updates, which will help implement the warning system for precomputed meshes.
284-284: LGTM: Correctly tracks segment data deletions.
The change appropriately sets volumeBucketDataHasChanged after successful segment deletions, which will help implement the warning system for precomputed meshes.
frontend/javascripts/oxalis/view/context_menu.tsx (2)
134-134: LGTM!
The import statement follows proper conventions and is correctly placed with other imports.
1183-1185: LGTM! Component integration looks good.
The LoadMeshMenuItemLabel component is properly integrated with the required props to implement the mesh warning functionality.
Let's verify the component exists and has the expected props:
✅ Verification successful
Component integration is correctly implemented
The LoadMeshMenuItemLabel component exists in frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/load_mesh_menu_item.tsx and is properly imported and used with the correct props (currentMeshFile and volumeTracing) in both context_menu.tsx and segment_list_item.tsx.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify LoadMeshMenuItemLabel component implementation
# Test: Check if the component exists and accepts the expected props
ast-grep --pattern 'type LoadMeshMenuItemLabelProps = {
currentMeshFile: $_
volumeTracing: $_
}'
Length of output: 102
Script:
#!/bin/bash
# Let's try a broader search for the LoadMeshMenuItemLabel component
ast-grep --pattern 'const LoadMeshMenuItemLabel = ({ $$ }: $_) => $_'
# Also search for any file containing LoadMeshMenuItemLabel
rg "LoadMeshMenuItemLabel" -A 5
Length of output: 3423
|
@MichaelBuessemeyer ping |
There was a problem hiding this comment.
Hej you two, thanks for this PR 🙏
I found one thing:
The frontend constructs its own action object at place instead of using the action constructor, which messes up the typing and due to different naming of actions params the setVolumeBucketDataHasChangedAction does not work anymore. You can find more details in my comments
The backend LGTM :)
frontend/javascripts/oxalis/model/reducers/volumetracing_reducer.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/load_mesh_menu_item.tsx
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/load_mesh_menu_item.tsx
Show resolved
Hide resolved
…ume-bucket-data-has-changed
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/load_mesh_menu_item_label.tsx (1)
19-36: Consider adding aria-label for better accessibility.The UI implementation is clean, but could benefit from accessibility improvements.
- <span style={{ display: "flex", alignItems: "center", gap: "4px" }}> + <span + style={{ display: "flex", alignItems: "center", gap: "4px" }} + aria-label={showWarning ? "Warning: Modified mesh data" : "Load mesh menu item"} + >frontend/javascripts/oxalis/view/context_menu.tsx (1)
1440-1442: Consider addressing the ignored lint warning.The
useExhaustiveDependencieslint rule is being ignored without proper justification. If the intention is to always focus the newest input ref, consider documenting this decision more explicitly in the comment.frontend/javascripts/oxalis/model/sagas/saga_helpers.ts (1)
119-146: LGTM with minor documentation suggestionsThe implementation is well-structured and properly handles the mapping lock workflow. Consider adding:
- Documentation for the return value's meaning (true = modification allowed, false = modification denied)
- A more specific function name like
requestBucketModificationForPrecomputedMeshesto better reflect its purpose in the precomputed meshes warning systemexport function* requestBucketModificationInVolumeTracing( volumeTracing: VolumeTracing, ): Saga<boolean> { /* * Should be called when the modification of bucket data is about to happen. If * the saga returns false, the modification should be cancelled (this happens if * the user is not okay with locking the mapping). + * @returns boolean - true if modification is allowed, false if user denied the mapping lock * * In detail: When the bucket data of a volume tracing is supposed to be mutated, we need to do
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
app/models/annotation/AnnotationService.scala(1 hunks)frontend/javascripts/oxalis/model/reducers/volumetracing_reducer.ts(2 hunks)frontend/javascripts/oxalis/model/sagas/saga_helpers.ts(2 hunks)frontend/javascripts/oxalis/model/sagas/volume/volume_interpolation_saga.ts(2 hunks)frontend/javascripts/oxalis/model/sagas/volumetracing_saga.tsx(4 hunks)frontend/javascripts/oxalis/store.ts(1 hunks)frontend/javascripts/oxalis/view/context_menu.tsx(4 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/load_mesh_menu_item_label.tsx(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx(4 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts(2 hunks)frontend/javascripts/types/api_flow_types.ts(1 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingService.scala(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/javascripts/oxalis/model/reducers/volumetracing_reducer.ts
- frontend/javascripts/oxalis/store.ts
- frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts
- app/models/annotation/AnnotationService.scala
- webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingService.scala
🧰 Additional context used
📓 Learnings (1)
frontend/javascripts/oxalis/model/sagas/volume/volume_interpolation_saga.ts (1)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/volumetracing_saga.tsx:433-434
Timestamp: 2024-11-22T17:19:07.947Z
Learning: In the codebase, certain usages of `segmentationLayer.resolutions` are intentionally retained and should not be changed to `segmentationLayer.mags` during refactoring.
🔇 Additional comments (13)
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/load_mesh_menu_item_label.tsx (1)
6-17: LGTM! Clean implementation of props and warning logic.
The warning logic is well-implemented with:
- Clear type definitions for props
- Proper null checks using nullish coalescing
- Backward compatibility for older annotations
frontend/javascripts/oxalis/model/sagas/volume/volume_interpolation_saga.ts (1)
439-443: LGTM! Clean integration of the new modification check.
The implementation:
- Properly integrates with the new bucket modification request system
- Maintains the existing control flow
- Has clear error handling
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx (3)
43-43: LGTM: Import for warning UI component
The import of LoadMeshMenuItemLabel aligns with the PR objective of implementing a warning system for precomputed meshes.
83-83: LGTM: Added volumeTracing parameter
The addition of activeVolumeTracing parameter enables checking the state of volume data for displaying warnings.
119-122: LGTM: Updated menu item label component
The LoadMeshMenuItemLabel component now receives necessary props to implement the warning system:
- currentMeshFile: For mesh information
- volumeTracing: For checking volume modification state
frontend/javascripts/oxalis/model/sagas/volumetracing_saga.tsx (3)
85-85: LGTM: Import for volume modification tracking
The import of requestBucketModificationInVolumeTracing aligns with the PR objective of tracking volume modifications.
226-230: LGTM: Volume modification check in editVolumeLayerAsync
Proper implementation of volume modification tracking in brush operations.
The error handling suggestion from previous reviews still applies here.
456-460: LGTM: Volume modification check in floodFill
Proper implementation of volume modification tracking in flood fill operations.
The error handling suggestion from previous reviews still applies here.
frontend/javascripts/types/api_flow_types.ts (1)
878-881: LGTM: Well-documented type for volume modification tracking
The addition of volumeBucketDataHasChanged with clear documentation:
- Automatically managed by backend
- No need for explicit UpdateAction
- Type properly integrated into ServerVolumeTracing
frontend/javascripts/oxalis/view/context_menu.tsx (3)
134-134: LGTM: Import statement is correctly placed.
The import statement for LoadMeshMenuItemLabel follows proper import conventions.
1440-1446: LGTM: State management implementation is robust.
The code correctly:
- Uses useSelector to access Redux state
- Ensures segment index loading via useEffect
- Checks segment index availability with proper arguments
1183-1185: LGTM: Good component composition.
The replacement of a static string label with the LoadMeshMenuItemLabel component improves modularity and enables dynamic mesh-related warnings, which aligns with the PR's objective of warning users about precomputed mesh usage after brushing.
frontend/javascripts/oxalis/model/sagas/saga_helpers.ts (1)
12-15: LGTM: Clean import structure
The new action imports are well-organized and properly scoped for the new functionality.
…ume-bucket-data-has-changed
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx (1)
120-123: Consider adding error boundaries for robust error handlingWhile the warning system implementation is solid, consider wrapping the
LoadMeshMenuItemLabelwith an error boundary to gracefully handle any potential runtime errors that might occur during warning display.Example implementation:
+class MeshWarningErrorBoundary extends React.Component { + componentDidCatch(error, errorInfo) { + console.error('Mesh warning error:', error, errorInfo); + } + + render() { + return this.props.children; + } +} // In the menu item label: ( + <MeshWarningErrorBoundary> <LoadMeshMenuItemLabel currentMeshFile={currentMeshFile} volumeTracing={activeVolumeTracing} /> + </MeshWarningErrorBoundary> ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
frontend/javascripts/test/snapshots/public-test/test-bundle/test/backend-snapshot-tests/annotations.e2e.js.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx(4 hunks)frontend/javascripts/test/snapshots/public-test/test-bundle/test/backend-snapshot-tests/annotations.e2e.js.md(2 hunks)
🔇 Additional comments (5)
frontend/javascripts/test/snapshots/public-test/test-bundle/test/backend-snapshot-tests/annotations.e2e.js.md (2)
1321-1321: LGTM: New field added for tracking volume data changes
The addition of volumeBucketDataHasChanged field with a default value of false aligns with the PR objective of tracking whether volume data has been modified through brushing.
1412-1412: LGTM: Consistent implementation in hybrid tracing
The volumeBucketDataHasChanged field is consistently implemented in the hybrid tracing response, maintaining parity with the volume-only tracing response.
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx (3)
43-43: LGTM: Import statement is correctly implemented
The import of LoadMeshMenuItemLabel aligns with the PR objective to implement warnings for precomputed meshes.
Line range hint 84-123: LGTM: Function changes properly implement warning system
The changes to getLoadPrecomputedMeshMenuItem correctly integrate the warning system by:
- Adding the necessary
activeVolumeTracingparameter - Properly passing the required props to
LoadMeshMenuItemLabel
429-429: LGTM: Props updated correctly for warning system
The activeVolumeTracing prop is correctly added with proper TypeScript typing, maintaining type safety for the warning system implementation.
CHANGELOG.unreleased.md
Outdated
| - Renamed "resolution" to "magnification" in more places within the codebase, including local variables. [#8168](https://github.com/scalableminds/webknossos/pull/8168) | ||
| - Reading image files on datastore filesystem is now done asynchronously. [#8126](https://github.com/scalableminds/webknossos/pull/8126) | ||
| - Datasets can now be renamed and can have duplicate names. [#8075](https://github.com/scalableminds/webknossos/pull/8075) | ||
| - Layer names are now allowed to contain `$` as special characters. [#8241](https://github.com/scalableminds/webknossos/pull/8241) |
There was a problem hiding this comment.
Add changelog entry for the precomputed meshes warning feature.
The PR implements a warning system for users when attempting to use precomputed meshes after brushing segments, but this user-facing change is not documented in the changelog. Please add an entry under the "Added" section describing this feature.
Add the following entry:
### Added
- When exploring remote URIs pasted from Neuroglancer, the format prefixes like `precomputed://` are now ignored, so users don't have to remove them. [#8195](https://github.com/scalableminds/webknossos/pull/8195)
- Added the total volume of a dataset to a tooltip in the dataset info tab. [#8229](https://github.com/scalableminds/webknossos/pull/8229)
+- Added a warning indicator when attempting to use precomputed meshes for segments that have been modified through brushing. [#8218](https://github.com/scalableminds/webknossos/pull/8218)📝 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.
| - Layer names are now allowed to contain `$` as special characters. [#8241](https://github.com/scalableminds/webknossos/pull/8241) | |
| ### Added | |
| - When exploring remote URIs pasted from Neuroglancer, the format prefixes like `precomputed://` are now ignored, so users don't have to remove them. [#8195](https://github.com/scalableminds/webknossos/pull/8195) | |
| - Added the total volume of a dataset to a tooltip in the dataset info tab. [#8229](https://github.com/scalableminds/webknossos/pull/8229) | |
| - Added a warning indicator when attempting to use precomputed meshes for segments that have been modified through brushing. [#8218](https://github.com/scalableminds/webknossos/pull/8218) |
3 participants
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)