Per-User State in Annotations: Camera, Visibility Toggles #8542
Conversation
|
Warning Rate limit exceeded@philippotto has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis change introduces user-specific state management for annotations, skeleton tracings, and volume tracings. It adds Changes
Assessment against linked issues
Possibly related PRs
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…State, UpdateTreeVisibility
…otation-user-state
…otation-user-state
…nossos into annotation-user-state
…otation-user-state
…nossos into annotation-user-state
…nossos into annotation-user-state
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: 22
🔭 Outside diff range comments (4)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala (1)
376-385: 💡 Verification agent🧩 Analysis chain
LGTM! Properly extends duplicate method with user context.
The addition of
ownerIdandrequestingUserIdparameters aligns with the PR's per-user state management objectives. The parameters are cleanly passed through to the service layer.Consider verifying that these user IDs are validated either in this controller or in the
annotationService.duplicateVolumeTracingmethod to ensure they are non-empty and valid.Also applies to: 397-406
🏁 Script executed:
#!/bin/bash # Description: Check if user ID validation exists in the service layer # Search for validation of ownerId and requestingUserId in duplicateVolumeTracing implementations ast-grep --pattern 'def duplicateVolumeTracing($$$) { $$$ }' # Also check for any validation patterns rg -A 10 "duplicateVolumeTracing.*ownerId.*requestingUserId" --type scalaLength of output: 140
🏁 Script executed:
#!/bin/bash set -e echo "1) Locate all definitions of duplicateVolumeTracing in the service layer" rg -n "def duplicateVolumeTracing" -t scala || true echo echo "2) Show 5 lines of context around each duplicateVolumeTracing definition" rg -n -C5 "def duplicateVolumeTracing" -t scala || true echo echo "3) Find all references to annotationService to identify where duplicateVolumeTracing is invoked" rg -n "annotationService" -t scala || trueLength of output: 28364
🏁 Script executed:
#!/bin/bash # Show the full duplicateVolumeTracing implementation in TSAnnotationService sed -n '880,950p' webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/annotation/TSAnnotationService.scalaLength of output: 3338
🏁 Script executed:
#!/bin/bash # Show the duplicate action implementation in VolumeTracingController sed -n '350,420p' webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scalaLength of output: 3649
Add validation for ownerId and requestingUserId
Currently neither the controller nor the service layer checks that these IDs are non-empty or well-formed before proceeding with duplication. Please add explicit validation to fail fast on invalid input.
• In
VolumeTracingController.scala(around theduplicate(...)action):// before calling annotationService.duplicateVolumeTracing(...) _ <- Fox.fromBool(ownerId.nonEmpty) ?~> "ownerId must be non-empty" _ <- Fox.fromBool(requestingUserId.nonEmpty) ?~> "requestingUserId must be non-empty"• In
TSAnnotationService.scala(insideduplicateVolumeTracing):for { _ <- Fox.fromBool(ownerId.nonEmpty) ?~> "ownerId must be non-empty" _ <- Fox.fromBool(requestingUserId.nonEmpty) ?~> "requestingUserId must be non-empty" sourceTracing <- findVolume(…) … } yield newTracingIdEnsuring these checks at the boundary or in the service guarantees that empty or malformed IDs cannot slip through.
app/models/annotation/nml/NmlWriter.scala (1)
50-64: 🛠️ Refactor suggestion
⚠️ Potential issueBreaking API change requires documentation.
The addition of the required
annotationProtoparameter and the change fromuserOpt: Option[User]toannotationOwner: Userconstitutes a breaking change to the public API. Consider:
- Adding ScalaDoc documentation for the new parameters
- Documenting the migration path for existing callers
- Ensuring all callers have been updated
Add documentation:
+/** + * Converts annotation layers to NML stream format + * @param name The output file name + * @param annotationProto The annotation protocol buffer containing user states + * @param annotationOwner The owner of the annotation (required) + * @param requestingUser The user requesting the export (optional, defaults to owner's view) + */ def toNmlStream(name: String, annotationProto: AnnotationProto,app/models/task/TaskCreationService.scala (2)
149-160:⚠️ Potential issueMissing
requestingUserIdon fresh skeleton creationsIn the
Nonebranch you call
tracingStoreClient.saveSkeletonTracing(skeleton, newSkeletonId)
without forwardingownerId/requestingUserId, so newly-created bases will not record the per-user state you introduced. Pass both IDs for consistency.- tracingStoreClient.saveSkeletonTracing(skeleton, newSkeletonId) + tracingStoreClient.saveSkeletonTracing( + skeleton, + newSkeletonId, + ownerId = baseAnnotation._user, + requestingUserId = requestingUserId + )
168-204: 🛠️ Refactor suggestionAnalogous omission for volume tracing creation
The
createVolumeTracingBasepath omits the newrequestingUserIdwhen persisting:tracingStoreClient.saveVolumeTracing(newAnnotationId, newVolumeTracingId, _, magRestrictions = magRestrictions, dataSource = dataSource)Add the parameter so that freshly created volume tracings carry the correct initial user state.
🧹 Nitpick comments (19)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/annotation/AnnotationUpdateActions.scala (1)
3-3: Import the geometry types instead of fully-qualifyingYou already import
AdditionalCoordinate; importingVec3Int/Vec3Doublekeeps the signature concise:import com.scalableminds.util.geometry.{Vec3Int, Vec3Double} case class UpdateCameraAnnotationAction( editPosition: Vec3Int, editRotation: Vec3Double, ... )Purely cosmetic, but improves readability across the file.
Also applies to: 124-128
webknossos-datastore/proto/VolumeTracing.proto (1)
21-30: Consider documenting the parallel array relationship.The
VolumeUserStateProtomessage uses parallel arrays (e.g.,segmentGroupIdswithsegmentGroupExpandedStates). While this is a valid pattern, it requires careful handling to maintain synchronization between related arrays.Consider adding a comment to document this relationship:
message VolumeUserStateProto { required string userId = 1; optional int64 activeSegmentId = 2; // The following pairs of fields are parallel arrays - indices must match: // segmentGroupIds[i] corresponds to segmentGroupExpandedStates[i] repeated int32 segmentGroupIds = 3; repeated bool segmentGroupExpandedStates = 4; // boundingBoxIds[i] corresponds to boundingBoxVisibilities[i] repeated int32 boundingBoxIds = 5; repeated bool boundingBoxVisibilities = 6; // segmentIds[i] corresponds to segmentVisibilities[i] repeated int64 segmentIds = 7; repeated bool segmentVisibilities = 8; }webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/annotation/AnnotationWithTracings.scala (1)
133-159: Consider simplifying the updateCamera implementation.The implementation correctly handles both update and insert cases. However, it could be made more concise by extracting the common user state creation logic:
def updateCamera(a: UpdateCameraAnnotationAction): AnnotationWithTracings = a.actionAuthorId match { case None => this case Some(actionUserId) => - val userStateAlreadyPresent = annotation.userStates.exists(state => actionUserId == state.userId) - if (userStateAlreadyPresent) { - this.copy(annotation = annotation.copy(userStates = annotation.userStates.map { - case userState if actionUserId == userState.userId => - userState.copy( - userId = actionUserId, - editPosition = a.editPosition, - editRotation = a.editRotation, - zoomLevel = a.zoomLevel, - editPositionAdditionalCoordinates = AdditionalCoordinate.toProto(a.editPositionAdditionalCoordinates), - ) - case userState => userState - })) - } else - this.copy( - annotation = annotation.copy(userStates = annotation.userStates :+ AnnotationUserStateProto( - userId = actionUserId, - editPosition = a.editPosition, - editRotation = a.editRotation, - zoomLevel = a.zoomLevel, - editPositionAdditionalCoordinates = AdditionalCoordinate.toProto(a.editPositionAdditionalCoordinates), - ))) + val newUserState = AnnotationUserStateProto( + userId = actionUserId, + editPosition = a.editPosition, + editRotation = a.editRotation, + zoomLevel = a.zoomLevel, + editPositionAdditionalCoordinates = AdditionalCoordinate.toProto(a.editPositionAdditionalCoordinates), + ) + + val updatedUserStates = if (annotation.userStates.exists(_.userId == actionUserId)) { + annotation.userStates.map { + case userState if userState.userId == actionUserId => + userState.copy( + editPosition = a.editPosition, + editRotation = a.editRotation, + zoomLevel = a.zoomLevel, + editPositionAdditionalCoordinates = AdditionalCoordinate.toProto(a.editPositionAdditionalCoordinates), + ) + case userState => userState + } + } else { + annotation.userStates :+ newUserState + } + + this.copy(annotation = annotation.copy(userStates = updatedUserStates)) }However, the current implementation is functionally correct and readable.
frontend/javascripts/test/sagas/save_saga.spec.ts (1)
359-359: Update comment to reflect the new action type.The comment still references "updateTracing update actions" but the code now uses
updateActiveNode.- // two of the updateTracing update actions are removed by compactSaveQueue + // two of the updateActiveNode update actions are removed by compactSaveQueuewebknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/skeleton/updating/SkeletonUpdateActions.scala (1)
449-456: Consider using immutable operations instead of mutable maps.While the current implementation works, using immutable operations would be more idiomatic and safer in Scala.
Example refactor for one of the methods:
- val expandedStateMapMutable: mutable.Map[Int, Boolean] = - existingUserState.treeGroupIds.zip(existingUserState.treeGroupExpandedStates).to(collection.mutable.Map) - groupIds.foreach(expandedStateMapMutable(_) = areExpanded) - val (treeGroupIds, expandedStates) = expandedStateMapMutable.unzip + val expandedStateMap = existingUserState.treeGroupIds.zip(existingUserState.treeGroupExpandedStates).toMap + val updatedMap = expandedStateMap ++ groupIds.map(_ -> areExpanded) + val (treeGroupIds, expandedStates) = updatedMap.unzipAlso applies to: 534-541, 586-593, 669-676
frontend/javascripts/viewer/model/sagas/update_actions.ts (1)
344-367: Consider adding @deprecated JSDoc annotation.While the comment explains this is for legacy reasons, consider adding a proper
@deprecatedJSDoc annotation for better IDE support and documentation.// This action only exists for legacy reasons. Old annotations may have this // action in the action log. Don't use it. +/** + * @deprecated This action only exists for legacy reasons. Use updateActiveNode and updateCameraAnnotation instead. + */ function LEGACY_updateSkeletonTracing(frontend/javascripts/viewer/model_initialization.ts (1)
655-720: Excellent backward compatibility with clear priority hierarchy.The implementation correctly prioritizes user state while maintaining backward compatibility for annotations created before the feature deployment. The comment clearly explains the fallback behavior.
Consider extracting the repeated pattern of checking userState and someTracing into a helper function to reduce code duplication:
function getStateValue<T>( userStateValue: T | undefined, someTracingValue: T | undefined, userState: APIAnnotationUserState | null | undefined, someTracing: ServerTracing | undefined ): T | undefined { if (userState != null) { return userStateValue; } else if (someTracing != null) { return someTracingValue; } return undefined; }app/models/annotation/nml/NmlWriter.scala (1)
115-124: Fix indentation for better readability.The method parameters should be consistently indented.
- parameters <- extractTracingParameters( - skeletonLayers, - volumeLayers, - annotation: Option[Annotation], - organizationId, - wkUrl, - datasetName, - datasetId, - voxelSize - ) + parameters <- extractTracingParameters( + skeletonLayers, + volumeLayers, + annotation, + organizationId, + wkUrl, + datasetName, + datasetId, + voxelSize + )frontend/javascripts/viewer/view/version_entry.tsx (2)
372-377: Consider using a more descriptive icon.The function implementation is correct, but the icon choice could be improved.
updateLargestSegmentId: (action: UpdateLargestSegmentIdVolumeAction): Description => { return { description: `Set largest segment id to ${action.value.largestSegmentId}`, - icon: <EditOutlined />, // todop: better icon? + icon: <NumberOutlined />, }; },
378-393: Improve icon choices for expand/collapse actions.Consider using more intuitive icons for expand/collapse operations.
updateSegmentGroupsExpandedState: ( action: UpdateSegmentGroupsExpandedStateUpdateAction, ): Description => { return { description: `${action.value.areExpanded ? "Expanded" : "Collapsed"} some segment groups.`, - icon: <EditOutlined />, // todop: better icon? + icon: action.value.areExpanded ? <ExpandOutlined /> : <CompressOutlined />, }; }, updateTreeGroupsExpandedState: ( action: UpdateSegmentGroupsExpandedStateUpdateAction, ): Description => { return { description: `${action.value.areExpanded ? "Expanded" : "Collapsed"} some tree groups.`, - icon: <EditOutlined />, // todop: better icon? + icon: action.value.areExpanded ? <ExpandOutlined /> : <CompressOutlined />, }; },Note: You may need to import
ExpandOutlinedandCompressOutlinedfrom@ant-design/icons.webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/skeleton/TreeUtils.scala (1)
80-90: Update comment to accurately describe the mapping behavior.The comment mentions "densifying" but the implementation creates sequential IDs starting from
offset + 1, which is more than just densification.- // We're densifying the tree ids to avoid sparse ids growing too fast + // Create a sequential mapping of tree IDs starting from offset + 1 to avoid sparse IDs private def calculateTreeMapping(trees: Seq[Tree], offset: Int): Map[Int, Int] =webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/TSAnnotationController.scala (1)
192-280: Consider breaking down this complex method for better maintainability.The
mergedFromIdsmethod handles many responsibilities including fetching, adapting, merging, and saving annotations. While the implementation appears correct, consider extracting some of the logic into smaller helper methods for better readability and testability.webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeUpdateActions.scala (2)
11-12: Remove unnecessary blank line.import scala.collection.mutable -
439-450: Consider adding a comment explaining the mutable map usage.While the use of mutable maps here is efficient for updating expanded states, it would benefit from a brief comment explaining the approach.
override def applyOnUserState(tracing: VolumeTracing, actionUserId: String, existingUserStateOpt: Option[VolumeUserStateProto]): VolumeUserStateProto = existingUserStateOpt.map { existingUserState => + // Use mutable map for efficient in-place updates of expanded states val expandedStateMapMutable: mutable.Map[Int, Boolean] = existingUserState.segmentGroupIds.zip(existingUserState.segmentGroupExpandedStates).to(collection.mutable.Map) groupIds.foreach(expandedStateMapMutable(_) = areExpanded)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/AnnotationUserStateUtils.scala (1)
233-259: Consider performance implications for large-scale merging.The merge operations create multiple intermediate collections and maps. For annotations with thousands of segments or trees, consider using more efficient data structures or streaming approaches to reduce memory allocation.
For better performance with large datasets, consider:
- Using mutable collections during the merge process and converting to immutable at the end
- Pre-allocating collection sizes when known
- Using specialized primitive collections for ID mappings
Also applies to: 299-327
frontend/javascripts/viewer/model/sagas/save_saga.ts (2)
356-356: Unnecessary Array.from() wrapperThe
diffSkeletonTracinganddiffVolumeTracingfunctions already return arrays, so wrapping them withArray.from()is redundant.- actions = actions.concat(Array.from(diffSkeletonTracing(prevTracing, tracing))); + actions = actions.concat(diffSkeletonTracing(prevTracing, tracing));- actions = actions.concat(Array.from(diffVolumeTracing(prevTracing, tracing))); + actions = actions.concat(diffVolumeTracing(prevTracing, tracing));Also applies to: 360-360
405-409: Consider narrowing the action listener scopeThe saga listens to
SkeletonTracingSaveRelevantActionswhich includes many skeleton-specific actions that don't affect camera state. This could cause unnecessary diff calculations on every skeleton tracing action.Consider creating a more specific set of actions that actually affect camera/annotation state to improve performance.
app/models/annotation/AnnotationService.scala (1)
536-553: Consider a more explicit approach for the dummy annotationProto.While the dummy
AnnotationProtoworks for compound downloads where user state is not needed, this pattern could be made more explicit to improve code clarity.Consider introducing a companion object method to make the intent clearer:
- // user state is not used in compound download, so the annotationProto can be a dummy one. - annotationProto = AnnotationProto("", 0L, Seq.empty, 0L) + // user state is not used in compound download, so the annotationProto can be a dummy one. + annotationProto = AnnotationProto.emptyForCompoundDownloadAnd add to the AnnotationProto companion object:
object AnnotationProto { def emptyForCompoundDownload: AnnotationProto = AnnotationProto("", 0L, Seq.empty, 0L) }frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (1)
11-11: Prefer tree-shakable lodash import
import _ from "lodash";pulls the whole library. Import only the used helper (e.g.import isEqual from "lodash/isEqual";) or switch tolodash-esto keep bundle size small.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/javascripts/test/backend-snapshot-tests/__snapshots__/annotations.e2e.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (74)
CHANGELOG.unreleased.md(1 hunks)app/controllers/AnnotationController.scala(1 hunks)app/controllers/AnnotationIOController.scala(6 hunks)app/controllers/TaskController.scala(1 hunks)app/models/annotation/AnnotationLayerPrecedence.scala(6 hunks)app/models/annotation/AnnotationMerger.scala(2 hunks)app/models/annotation/AnnotationService.scala(3 hunks)app/models/annotation/WKRemoteTracingStoreClient.scala(6 hunks)app/models/annotation/nml/NmlWriter.scala(7 hunks)app/models/task/TaskCreationService.scala(5 hunks)frontend/javascripts/admin/rest_api.ts(1 hunks)frontend/javascripts/libs/utils.ts(2 hunks)frontend/javascripts/test/backend-snapshot-tests/annotations.e2e.ts(3 hunks)frontend/javascripts/test/fixtures/skeletontracing_server_objects.ts(2 hunks)frontend/javascripts/test/fixtures/tasktracing_server_objects.ts(2 hunks)frontend/javascripts/test/fixtures/volumetracing_server_objects.ts(2 hunks)frontend/javascripts/test/helpers/saveHelpers.ts(1 hunks)frontend/javascripts/test/sagas/compact_toggle_actions.spec.ts(2 hunks)frontend/javascripts/test/sagas/saga_integration.spec.ts(3 hunks)frontend/javascripts/test/sagas/save_saga.spec.ts(3 hunks)frontend/javascripts/test/sagas/skeletontracing_saga.spec.ts(26 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts(4 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_1.spec.ts(4 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_2.spec.ts(4 hunks)frontend/javascripts/types/api_types.ts(3 hunks)frontend/javascripts/viewer/controller/combinations/tool_controls.ts(0 hunks)frontend/javascripts/viewer/model/accessors/annotation_accessor.ts(2 hunks)frontend/javascripts/viewer/model/accessors/tracing_accessor.ts(2 hunks)frontend/javascripts/viewer/model/actions/skeletontracing_actions.tsx(3 hunks)frontend/javascripts/viewer/model/helpers/chunk.ts(1 hunks)frontend/javascripts/viewer/model/helpers/compaction/compact_save_queue.ts(1 hunks)frontend/javascripts/viewer/model/helpers/diff_helpers.ts(1 hunks)frontend/javascripts/viewer/model/reducers/reducer_helpers.ts(9 hunks)frontend/javascripts/viewer/model/reducers/save_reducer.ts(1 hunks)frontend/javascripts/viewer/model/reducers/skeletontracing_reducer.ts(5 hunks)frontend/javascripts/viewer/model/reducers/skeletontracing_reducer_helpers.ts(1 hunks)frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts(6 hunks)frontend/javascripts/viewer/model/sagas/save_saga.ts(7 hunks)frontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/split_boundary_mesh_saga.ts(1 hunks)frontend/javascripts/viewer/model/sagas/undo_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/update_actions.ts(12 hunks)frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx(3 hunks)frontend/javascripts/viewer/model/types/tree_types.ts(1 hunks)frontend/javascripts/viewer/model_initialization.ts(9 hunks)frontend/javascripts/viewer/store.ts(1 hunks)frontend/javascripts/viewer/view/action-bar/tracing_actions_view.tsx(2 hunks)frontend/javascripts/viewer/view/jobs/train_ai_model.tsx(2 hunks)frontend/javascripts/viewer/view/version_entry.tsx(3 hunks)test/backend/AnnotationUserStateTestSuite.scala(1 hunks)test/backend/NMLUnitTestSuite.scala(4 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/SkeletonElementDefaults.scala(2 hunks)webknossos-datastore/proto/Annotation.proto(2 hunks)webknossos-datastore/proto/SkeletonTracing.proto(2 hunks)webknossos-datastore/proto/VolumeTracing.proto(2 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/annotation/AnnotationUpdateActions.scala(3 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/annotation/AnnotationWithTracings.scala(3 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/annotation/TSAnnotationService.scala(10 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/annotation/UpdateActions.scala(8 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/SkeletonTracingController.scala(2 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/TSAnnotationController.scala(7 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala(2 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/AnnotationUserStateUtils.scala(1 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/BoundingBoxMerger.scala(2 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/GroupUtils.scala(2 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingMergeService.scala(2 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/skeleton/SkeletonTracingService.scala(4 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/skeleton/TreeUtils.scala(2 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/skeleton/updating/SkeletonUpdateActionHelper.scala(0 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/skeleton/updating/SkeletonUpdateActions.scala(7 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingDefaults.scala(2 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingService.scala(6 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeUpdateActions.scala(8 hunks)webknossos-tracingstore/conf/com.scalableminds.webknossos.tracingstore.routes(3 hunks)
💤 Files with no reviewable changes (2)
- webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/skeleton/updating/SkeletonUpdateActionHelper.scala
- frontend/javascripts/viewer/controller/combinations/tool_controls.ts
🧰 Additional context used
🪛 Buf (1.54.0)
webknossos-datastore/proto/Annotation.proto
2-2: import "geometry.proto": file does not exist
(COMPILE)
🔇 Additional comments (122)
test/backend/NMLUnitTestSuite.scala (2)
5-6: LGTM! Imports align with the new user state management feature.The added imports support the new
UserandAnnotationProtoparameters required for the per-user state management functionality.Also applies to: 15-15, 20-20, 22-23
44-71: Changes correctly adapt the test to the new user state management API.The modifications appropriately update the test to include the required
AnnotationProtoandUsercontext. The dummy values used are suitable for unit testing the NML serialization/parsing logic.Note that
requestingUseris set toNone, which appears to test the fallback behavior where the annotation owner's state is used.Please ensure that comprehensive tests for the actual per-user state functionality (camera positions, visibility toggles, expanded/collapsed states) are included in other test files, as this unit test only verifies NML parsing with minimal dummy values.
test/backend/AnnotationUserStateTestSuite.scala (4)
1-8: LGTM!The package declaration, imports, and class declaration follow standard conventions for a Play framework test suite.
10-29: LGTM!The test data setup provides good coverage with different user states for testing various scenarios.
31-41: LGTM!The test correctly verifies the rendering of skeleton user state with fallback behavior for sparse user states.
63-102: LGTM!The volume user states merging test is well-structured and correctly verifies ID mapping and group transformation logic.
frontend/javascripts/viewer/model/helpers/chunk.ts (1)
2-2: LGTM! Formatting improvements.The whitespace additions improve code readability without affecting functionality.
Also applies to: 6-6
frontend/javascripts/viewer/model/types/tree_types.ts (1)
80-82: Well-documented backward compatibility support.The addition of the optional
isExpandedproperty with clear documentation about its purpose for legacy groups is excellent. This ensures smooth migration to the new user-specific state management system while maintaining compatibility with existing data.frontend/javascripts/admin/rest_api.ts (1)
745-745: Enhanced debugging information.Adding the tracing type to the console log improves debugging capabilities for protobuf parsing, which is valuable for troubleshooting the new user state functionality.
frontend/javascripts/test/fixtures/volumetracing_server_objects.ts (2)
74-74: Proper test fixture update for user states.Adding the
userStates: []property to theServerVolumeTracingtest fixture correctly aligns with the new user state management functionality introduced in this PR.
139-139: Consistent fixture updates for annotation proto.The addition of
userStates: []to theAPITracingStoreAnnotationfixture maintains consistency with the user state management changes and ensures test compatibility.CHANGELOG.unreleased.md (1)
21-21: Well-written changelog entry!The description clearly explains the new per-user state management feature, including what properties are stored per user and the fallback behavior. This will help users understand the new functionality.
frontend/javascripts/libs/utils.ts (2)
1115-1139: Well-implemented set difference utility!The
diffSetsfunction efficiently compares two sets by iterating through each set once and categorizing elements correctly. The implementation follows good performance practices and the return structure is clear and useful.
1338-1346: Excellent recursive mapping utility!The
mapGroupsDeepfunction correctly implements depth-first traversal by recursively processing children before applying the mapping function to the parent. This is exactly what's needed for transforming hierarchical group structures while preserving the tree structure.frontend/javascripts/viewer/store.ts (1)
114-116: Good use of type aliases for semantic clarity!These type aliases improve code readability by providing segment-specific names for the existing tree group types. Since segment groups and tree groups share the same structure, this approach maintains type safety while making the code more semantically meaningful.
frontend/javascripts/test/fixtures/skeletontracing_server_objects.ts (2)
165-165: Appropriate test fixture update!Adding the
userStatesproperty as an empty array to the test fixture ensures compatibility with the new schema while maintaining focus on testing the core functionality rather than user state specifics.
233-233: Consistent test fixture update!The addition of
userStatesas an empty array maintains consistency with the updatedAPITracingStoreAnnotationtype and ensures the test fixture remains valid.frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_1.spec.ts (1)
16-16: LGTM! Improved test setup and cleanup.The changes enhance test reliability:
- Using
resetStoreAction()provides a more comprehensive store reset- Adding
tearDownPullQueues()ensures proper cleanup of resourcesAlso applies to: 44-44, 63-63
frontend/javascripts/viewer/model/helpers/compaction/compact_save_queue.ts (1)
7-22: Consistent with per-user state management approach.The changes from generic tracing actions to specific user-state actions (
updateActiveNode,updateActiveSegmentId,updateCamera) align well with the PR's objective of tracking individual user modifications separately.frontend/javascripts/test/helpers/saveHelpers.ts (1)
24-33: Test helper updated consistently with save queue changes.The filter now correctly excludes the new user-specific action types, maintaining consistency with the changes in
compact_save_queue.ts.webknossos-datastore/proto/Annotation.proto (2)
18-18: LGTM! Well-structured user state implementation.The
AnnotationUserStateProtomessage and its integration intoAnnotationProtoare properly designed. The field types appropriately capture user-specific view state including position, rotation, zoom level, and additional coordinates.Also applies to: 27-33
2-2:✅ Verification successful
Verify that geometry.proto exists in the build environment.
The static analysis tool indicates that
geometry.protomay not exist. Please ensure this file is present in your protobuf compilation path.Run the following script to verify the existence of the geometry.proto file:
🏁 Script executed:
#!/bin/bash # Description: Check if geometry.proto exists in the repository # Search for geometry.proto file fd -t f "geometry.proto" # Also check for references to the types that should be defined in geometry.proto rg -A 2 -B 2 "Vec3IntProto|Vec3DoubleProto|AdditionalCoordinateProto" --glob "*.proto"Length of output: 7466
Confirmed: geometry.proto import is valid
geometry.protoexists atwebknossos-datastore/proto/geometry.proto- Defines
Vec3IntProto,Vec3DoubleProto,AdditionalCoordinateProto, etc.- The import in
Annotation.protocorrectly references the file in the same directoryNo further changes required.
🧰 Tools
🪛 Buf (1.54.0)
2-2: import "geometry.proto": file does not exist
(COMPILE)
frontend/javascripts/viewer/view/jobs/train_ai_model.tsx (1)
722-722: LGTM! Function signatures updated for user state support.The additional parameters (null/undefined) maintain backward compatibility while enabling the new user state management feature.
Also applies to: 734-734
frontend/javascripts/test/sagas/compact_toggle_actions.spec.ts (1)
2-2: LGTM! Test simplification improves clarity.The removal of the unused
Flycammock and its parameters fromdiffVolumeTracingcalls streamlines the tests appropriately.Also applies to: 184-184
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingDefaults.scala (1)
3-3: LGTM! Clean implementation of empty user state factory method.The
emptyUserStatemethod provides a clear and consistent way to create default user state objects. All fields are properly initialized with empty/None values.Also applies to: 18-27
frontend/javascripts/viewer/model/reducers/save_reducer.ts (1)
33-36: Good defensive programming! Prevents empty transactions.The early return with warning message helps catch potential issues where empty save transactions might be dispatched, avoiding unnecessary state updates and queue modifications.
webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/SkeletonElementDefaults.scala (1)
30-40: LGTM! Clean implementation of the empty user state factory method.The
emptyUserStatemethod follows the established pattern in this file and provides a clean way to initialize user-specific skeleton state with sensible defaults.webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/SkeletonTracingController.scala (1)
119-143: LGTM! Appropriate addition of ownership parameters for duplication.The addition of
ownerIdandrequestingUserIdparameters aligns well with the PR's objective of managing per-user state. These parameters are correctly propagated to the service layer.frontend/javascripts/viewer/model/accessors/tracing_accessor.ts (1)
2-2:✅ Verification successful
Verify that
getSomeServerTracingis no longer used in the codebase.The removal of the
ServerTracingimport andgetSomeServerTracingfunction appears to be part of the refactoring. Please ensure this function is not referenced elsewhere.Also applies to: 31-31
🏁 Script executed:
#!/bin/bash # Description: Search for any remaining references to getSomeServerTracing function # Test: Search for the function usage. Expect: No occurrences found. rg -A 2 "getSomeServerTracing" --type ts --type tsx --type js --type jsxLength of output: 100
🏁 Script executed:
#!/bin/bash # Description: Re-run search for any remaining references to getSomeServerTracing across the entire repo, # without restricting by file type to avoid the “unrecognized file type” error. rg -n "getSomeServerTracing" || grep -R -n "getSomeServerTracing" .Length of output: 64
All references to
getSomeServerTracinghave been removed
A repository‐wide search returned no occurrences ofgetSomeServerTracing, so it’s safe to drop the import and function.frontend/javascripts/test/sagas/skeletontracing_saga.spec.ts (2)
35-44: Nice clean-up – fewer test parametersRefactoring
testDiffingto accept only the annotation pair simplifies every call site and keeps focus on the diff logic. 👍
100-107: Confirm type update foruserStatesinServerSkeletonTracing
serverSkeletonTracingnow includesuserStates: [], but the importedServerSkeletonTracingtype must expose this field (and its nested shape) or the test will fail to compile oncestrictchecks are enabled.Please verify that
types/api_types.ts(or equivalent) was updated; otherwise add the property there.webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/BoundingBoxMerger.scala (3)
8-11: Well-structured enum for tracking bounding box sources.The
BoundingBoxSourceenum provides clear categorization of bounding box origins during the merge process, which is essential for correct ID mapping.
15-15: Good use of type alias and enhanced return type.The
UserBboxIdMaptype alias improves code readability, and returning the ID maps alongside the merged bounding boxes enables proper tracking of ID transformations during the merge process.Also applies to: 31-31
33-54: Robust implementation of bounding box merging with ID tracking.The implementation correctly:
- Handles deprecated single bounding boxes by converting them to named versions
- Ensures uniqueness while preserving source information
- Generates consistent new IDs using index-based assignment
- Creates accurate ID mappings for both sources
The use of
distinctBywithcopy(id = 0)ensures proper deduplication based on content rather than ID.webknossos-datastore/proto/VolumeTracing.proto (1)
72-72: Proper addition of user states field.The
userStatesfield is correctly added as a repeated field with a non-conflicting field number, enabling per-user state management for volume tracings.webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/annotation/AnnotationWithTracings.scala (1)
3-7: Appropriate imports and trait extension for user state functionality.The added imports and
ProtoGeometryImplicitstrait extension provide the necessary types and implicit conversions for handling user-specific camera state.Also applies to: 12-13, 32-33
frontend/javascripts/test/sagas/save_saga.spec.ts (2)
74-87: LGTM! The granular update actions improve state management.The refactoring from monolithic
updateSkeletonTracingto specific update actions (updateActiveNode,updateActiveSegmentId,updateCameraAnnotation) aligns well with the PR's objective of per-user state management. This allows for more precise control over which state updates are compacted and preserved.
295-302: Consistent refactoring to use specific update actions.The change from
updateSkeletonTracingtoupdateActiveNodemaintains the test's original purpose while aligning with the new granular update action pattern.webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingMergeService.scala (2)
40-45: Good practice: Using Seq instead of List for method parameters.The change from
ListtoSeqimproves API flexibility and follows Scala best practices.Seqis more general and allows callers to pass various collection types without conversion.
101-102: Consistent parameter type update.The private method signature is correctly updated to match the public method's parameter type change.
frontend/javascripts/viewer/model/reducers/skeletontracing_reducer_helpers.ts (2)
879-909: Well-implemented user state transformation support.The optional
applyUserStateToTreeFnparameter elegantly enables per-user tree state customization while maintaining backward compatibility. The default identity function ensures existing code continues to work unchanged.
915-923: Consistent parameter propagation to immutable version.The immutable sibling function correctly accepts and passes through the
applyUserStateToTreeFnparameter, maintaining API consistency.frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts (2)
55-59: LGTM! Appropriate initialization of new user state fields.The new fields are initialized with sensible defaults that align with the PR's user state management feature.
73-73: LGTM! Correct usage of null parameters for user state.The
nullparameters correctly represent the absence of existing user state and user ID in the test context.webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/annotation/UpdateActions.scala (3)
25-25: LGTM! Well-designed trait for user state updates.The
UserStateUpdateActiontrait appropriately extendsUpdateActionto support user-specific state modifications.
42-62: LGTM! Comprehensive deserialization support for new user state actions.The pattern matching correctly handles all new user state update actions with proper deserialization.
Also applies to: 79-85, 99-99
140-146: LGTM! Symmetric serialization implementation.The serialization logic properly mirrors the deserialization, ensuring round-trip compatibility for all new user state actions.
Also applies to: 168-173, 194-196, 234-235
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/skeleton/updating/SkeletonUpdateActions.scala (2)
26-45: LGTM! Well-structured trait for user-specific skeleton updates.The
UserStateSkeletonUpdateActiontrait properly combines skeleton and user state update functionality. TheapplyOnmethod correctly handles both creation of new user states and updates to existing ones.
731-756: LGTM! Proper JSON format definitions for new actions.All new action classes have appropriate JSON format implicits defined.
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/skeleton/SkeletonTracingService.scala (4)
22-22: LGTM!The addition of the
AnnotationUserStateUtilstrait is appropriate for handling user-specific state management functionality.
57-84: Good implementation of user state handling for duplication.The method correctly creates a user state for the requesting user using
renderSkeletonUserStateIntoUserState, which should handle the fallback logic to the owner's state as described in the PR objectives. The integration into the duplicated tracing is clean and follows the existing pattern.
99-136: Comprehensive and correct implementation of user state merging.The merge logic properly handles all necessary mappings (node, tree, group, and bounding box) before merging user states. This ensures that user states are correctly updated with remapped IDs during the merge process, maintaining consistency across the merged annotation.
139-139: Helpful clarification comment.Good addition of context explaining why user state mapping is not needed in
remapTooLargeTreeIds. This helps future maintainers understand the scope of this method.frontend/javascripts/viewer/model/helpers/diff_helpers.ts (4)
6-12: Clean recursive implementation for stripping UI state.The function correctly creates shallow copies while preserving the tree structure. Good use of recursion for handling nested groups.
14-30: Efficient recursive state gathering.Good use of a mutable accumulator pattern for performance. The function correctly categorizes all groups by their expansion state while traversing the tree structure.
32-52: Well-structured diff function with good performance characteristics.Excellent optimization with reference equality check. The separation of content changes from state changes is a clean design that allows consumers to handle different types of changes appropriately.
54-94: Consistent implementation for bounding box diffing.Good use of object destructuring to strip properties. The implementation maintains consistency with the group diffing pattern while appropriately handling the simpler, non-nested structure of bounding boxes.
webknossos-tracingstore/conf/com.scalableminds.webknossos.tracingstore.routes (1)
13-15: Consistent addition of user identification parameters.The route changes correctly add
ownerIdandrequestingUserIdparameters to support per-user state management. These align with the controller method signatures and the PR objectives.Note that the
mergedFromIdsroute (line 15) only addsrequestingUserIdwithoutownerId. Please verify this is intentional - perhaps merged annotations determine ownership differently?Also applies to: 26-26, 74-74
frontend/javascripts/viewer/model/sagas/update_actions.ts (12)
24-29: Good refactoring approach with backward compatibility.The renaming of update actions to include the
LEGACY_prefix and introduction of granular actions is a clean approach that maintains backward compatibility while moving towards a more modular architecture.
45-50: LGTM! Consistent naming for user-specific visibility actions.The new action types for user bounding box visibility follow the established naming pattern and support the per-user state feature.
369-380: Clean implementation of granular update action.The
updateActiveNodefunction follows the single responsibility principle by only updating the active node state.
423-425: LGTM! Proper null handling for segment ID.The function correctly handles both number and null values for
largestSegmentId.
450-483: Consistent implementation for user bounding box visibility.Both skeleton and volume tracing visibility update functions follow the same pattern and are properly typed.
595-624: Efficient bulk update for group expansion states.The functions efficiently handle multiple group IDs in a single action, which is good for performance when toggling multiple groups.
665-680: Well-structured camera state update action.The
updateCameraAnnotationfunction properly encapsulates all camera-related state updates in a single action.
24-29: LGTM! Good separation of concerns with legacy actions.The renaming of comprehensive update actions to
LEGACY_prefix and introduction of granular actions (UpdateActiveNodeUpdateAction,UpdateActiveSegmentIdUpdateAction, etc.) aligns well with the PR objective of supporting per-user state management. This approach maintains backward compatibility while enabling more targeted state updates.
344-380: Well-structured refactoring for per-user state management.The separation of the monolithic
updateSkeletonTracinginto focused actions likeupdateActiveNodeis excellent. This granular approach supports the PR's goal of managing user-specific states independently, allowing different users to have their own active nodes without affecting camera positions or other properties.
450-483: Consistent implementation for bounding box visibility controls.The new functions for updating bounding box visibility in both skeleton and volume tracings follow a consistent pattern and enable per-user visibility states as intended by the PR objectives.
595-623: Efficient bulk update approach for group expansion states.The implementation allows updating multiple group expansion states in a single action, which is efficient for UI operations. This supports the per-user state requirement where each user can have their own view of expanded/collapsed groups.
665-680: Clean separation of camera state from tracing state.The dedicated
updateCameraAnnotationfunction properly isolates camera-related updates (position, rotation, zoom) from tracing data, supporting the per-user view state management objective.frontend/javascripts/viewer/model/reducers/reducer_helpers.ts (4)
49-68: Robust implementation with proper fallback handling.The function correctly handles user-specific visibility states with a logical fallback chain: user state → bounding box visibility → default true.
70-88: Clean conversion functions with proper structure.Both conversion functions correctly transform between frontend and server bounding box formats.
239-258: Well-designed higher-order function for tree visibility.The function correctly returns a transformation function that applies user-specific visibility states to trees, maintaining consistency with the group expansion implementation.
212-237:✅ Verification successful
Verify the availability of
mapGroupsDeeputility function.The implementation correctly handles user-specific group expansion states for both skeleton and volume tracings. The use of a lookup object is efficient.
Verify that
Utils.mapGroupsDeepis properly implemented:
🏁 Script executed:
#!/bin/bash # Description: Verify the implementation of mapGroupsDeep utility function # Search for mapGroupsDeep implementation ast-grep --pattern 'export function mapGroupsDeep'Length of output: 52
🏁 Script executed:
#!/bin/bash # Search for any reference or definition of mapGroupsDeep in the codebase rg 'mapGroupsDeep' -nLength of output: 366
mapGroupsDeep utility function confirmed
- Located in
frontend/javascripts/libs/utils.ts(around line 1338), providing the expected recursive mapping behavior.- No further action needed—implementation is present and correct.
frontend/javascripts/viewer/model_initialization.ts (2)
145-145: Proper initialization of user state with null safety.The user state is correctly obtained when annotation proto is available, with appropriate null checking.
Also applies to: 159-162
242-244: Correct placement of zoom level initialization.Setting the zoom level from user state during data layer initialization ensures the view is properly configured before rendering.
app/models/annotation/nml/NmlWriter.scala (3)
44-47: LGTM!The addition of the
AnnotationUserStateUtilstrait to theNmlWriterclass is appropriate for implementing user-specific state rendering functionality.
70-74: Good implementation of user state rendering.The implementation correctly applies user-specific states with a fallback to the annotation owner's state, which aligns with the PR objectives.
431-454: Good improvement to metadata consistency.The change from optional to required
annotationOwnerparameter ensures that the username metadata is always present and accurate, improving NML file consistency.webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/skeleton/TreeUtils.scala (1)
71-90: Good refactoring to improve code clarity.The separation of mapping calculation (
calculateTreeMappings) from mapping application (mergeTrees) improves code clarity and makes the ID remapping logic more explicit and testable.webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/annotation/TSAnnotationService.scala (3)
838-882: LGTM!The
ownerIdandrequestingUserIdparameters are correctly propagated to both volume and skeleton tracing duplication methods based on the layer type.
884-925: LGTM!The volume tracing duplication correctly passes the
ownerIdandrequestingUserIdparameters to the adaptation method, maintaining the proper context for user-specific state management.
942-965: LGTM!The skeleton tracing duplication correctly propagates the
ownerIdandrequestingUserIdparameters, maintaining consistency with the volume tracing implementation.webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/TSAnnotationController.scala (2)
158-173: LGTM!The helper method correctly filters volume layers, fetches the corresponding tracings, and adapts them with user-specific states using the appropriate rendering method.
116-140: LGTM!The
duplicatemethod correctly propagates the newownerIdandrequestingUserIdparameters to the annotation service.frontend/javascripts/types/api_types.ts (2)
606-613: LGTM!The
APIAnnotationUserStatetype properly captures user-specific view state including position, rotation, zoom level, and support for additional coordinates.
908-914: LGTM!The fallback properties in
ServerTracingBaseare well-documented with clear comments indicating they should only be used when theuserStatesarray is empty, ensuring proper backward compatibility.app/models/annotation/WKRemoteTracingStoreClient.scala (3)
103-107: LGTM! Clean implementation of annotation proto retrieval.The new method follows the established pattern for RPC calls with proper token authentication and optional version support.
110-127: Good consistency in adding user context parameters.The addition of
ownerIdandrequestingUserIdparameters across duplication methods is implemented consistently, with all methods following the same pattern of adding these as query parameters.Also applies to: 130-147, 150-172
175-188:✅ Verification successful
Verify the order of ownerIds matches annotationIds.
The method now accepts a list of
ownerIdsthat should correspond to theannotationIdslist. Please ensure that the calling code maintains the correct order alignment between these two lists to avoid mismatched ownership assignments.Run the following script to verify the usage of this method:
🏁 Script executed:
#!/bin/bash # Description: Check how mergeAnnotationsByIds is called to ensure ownerIds and annotationIds are properly aligned # Search for calls to mergeAnnotationsByIds rg -A 10 "mergeAnnotationsByIds\(" --type scalaLength of output: 2391
mergeAnnotationsByIds invocation order verified
Both
annotationIdsandownerIdsare derived from the sameannotationslist in AnnotationMerger.scala, preserving their alignment:
- File: app/models/annotation/AnnotationMerger.scala
Call site:tracingStoreClient.mergeAnnotationsByIds( annotations.map(_.id), annotations.map(_._user), newAnnotationId, toTemporaryStore, requestingUserId )No further changes are needed here.
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeUpdateActions.scala (2)
40-59: Well-designed trait for user-specific update actions.The
UserStateVolumeUpdateActiontrait correctly handles the creation and update of user-specific states, with proper handling of the case where no author ID is present (view-only changes).
425-459:✅ Verification successful
Verify action naming consistency.
The AI summary mentions both
UpdateSegmentGroupsExpandedStateVolumeActionandUpdateSegmentGroupExpandedStateVolumeAction, but only the former (plural "Groups") is defined in the code. Please verify this is intentional and update any references to use the consistent plural form.
🏁 Script executed:
#!/bin/bash # Description: Check for any references to the singular form of the action name # Search for references to the potentially incorrect singular form rg "UpdateSegmentGroupExpandedStateVolumeAction" --type scala # Also check for the correct plural form to compare usage rg "UpdateSegmentGroupsExpandedStateVolumeAction" --type scalaLength of output: 1395
Naming consistency verified – only the plural form is used throughout
All references to this action useUpdateSegmentGroupsExpandedStateVolumeAction; there are no occurrences of the singular form. No changes needed.webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/AnnotationUserStateUtils.scala (2)
14-23: Clean implementation of user state rendering.The method correctly applies user-specific states to annotation layers with proper fallback logic.
25-30: Correct priority order for user state selection.The fallback from requesting user to owner provides the expected behavior for shared annotations.
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/GroupUtils.scala (2)
20-67: Good encapsulation improvements!Making these helper methods private properly hides the internal implementation details while maintaining the public API through the type-specific methods below.
63-67:✅ Verification successful
Verify the removal of the default parameter doesn't break existing code.
The removal of the default parameter
ids: Seq[Int] = Seq[Int]()fromgetAllGroupIdscould potentially break existing code that relied on calling this method without the second parameter.Run the following script to check if any code calls
getAllGroupIdswithout the second parameter:
🏁 Script executed:
#!/bin/bash # Description: Search for getAllGroupIds calls that might be affected by the removal of the default parameter # Test: Search for getAllGroupIds calls. Expect: All calls should have two parameters. rg -A 3 'getAllGroupIds\s*\(' --type scalaLength of output: 1389
No invocations rely on the removed default parameter
A repo-wide search forgetAllGroupIdsonly found its definition and the internal recursive call—both explicitly pass two arguments. As this helper isprivateand has no external usages, removing the defaultidsparameter introduces no breakage.app/controllers/TaskController.scala (1)
67-71:✅ Verification successful
LGTM! User ID properly passed for user-specific state management.
The addition of
request.identity._idaligns with the PR's objective of implementing per-user state management for annotations.Run the following script to verify the method signature update is consistent across the codebase:
🏁 Script executed:
#!/bin/bash # Description: Verify that createTracingsFromBaseAnnotations method signature is updated consistently # Test: Search for the method definition. Expect: The method should accept the user ID parameter. ast-grep --pattern 'def createTracingsFromBaseAnnotations($$$)'Length of output: 65
Let’s locate the method definition to confirm its parameter list:
🏁 Script executed:
#!/bin/bash set -e # Search for the method declaration to verify it includes the user ID parameter rg -n "def createTracingsFromBaseAnnotations" .Length of output: 146
Inspecting the full method signature to confirm the added
userIdparameter:
🏁 Script executed:
#!/bin/bash set -e # Show method signature with surrounding lines rg -n -A 5 -B 2 "def createTracingsFromBaseAnnotations" app/models/task/TaskCreationService.scalaLength of output: 577
LGTM – Service signature updated correctly.
The call in TaskController.scala now passes
request.identity._id, which matches therequestingUserId: ObjectIdparameter in the method definition found in app/models/task/TaskCreationService.scala (lines 67–72). This aligns with the PR’s per-user state management goal.frontend/javascripts/viewer/model/sagas/split_boundary_mesh_saga.ts (1)
111-111:✅ Verification successful
Verify the action type rename is complete across the codebase.
The change from
"SET_TRACING"to"SET_SKELETON_TRACING"looks good, but we should ensure this rename is consistent throughout the codebase.Run the following script to verify the action type rename is complete:
🏁 Script executed:
#!/bin/bash # Description: Verify that SET_TRACING is no longer used and SET_SKELETON_TRACING is properly defined # Test: Search for any remaining usage of SET_TRACING action. Expect: No occurrences. rg '"SET_TRACING"' --type ts --type tsx --type js --type jsx # Test: Verify SET_SKELETON_TRACING is defined and used. Expect: At least one definition and usage. rg '"SET_SKELETON_TRACING"' --type ts --type tsx --type js --type jsx -A 2Length of output: 195
🏁 Script executed:
#!/bin/bash # Retry search using file globs since tsx wasn't recognized as a type # Ensure no remaining usages of SET_TRACING rg '"SET_TRACING"' --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.jsx' # Verify definition and usage of SET_SKELETON_TRACING with context rg '"SET_SKELETON_TRACING"' --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.jsx' -C 2Length of output: 2067
Action rename verified: "SET_TRACING" removed, "SET_SKELETON_TRACING" properly defined and used.
- No occurrences of
"SET_TRACING"remain in .ts/.tsx/.js/.jsx files."SET_SKELETON_TRACING"is defined inviewer/model/actions/skeletontracing_actions.tsx.- It’s referenced in
split_boundary_mesh_saga.ts,save_saga.ts, andskeletontracing_reducer.ts.All usages have been updated accordingly.
frontend/javascripts/viewer/model/sagas/undo_saga.ts (1)
24-25:✅ Verification successful
Action rename looks consistent – double-check for stragglers
The import and dispatch were updated to
setSkeletonTracingAction.
Please run a quick grep to ensure no remainingsetTracingAction(...)usages linger in the codebase to avoid dead code and broken builds.Also applies to: 589-590
🏁 Script executed:
#!/bin/bash rg --no-heading -n "setTracingAction(" || echo "No leftover occurrences."Length of output: 184
🏁 Script executed:
#!/bin/bash # Search for any remaining references to `setTracingAction` rg --no-heading -n 'setTracingAction' || echo "No leftover occurrences of setTracingAction."Length of output: 136
All
setTracingActionreferences replaced
No leftover occurrences ofsetTracingActionfound in the codebase—rename tosetSkeletonTracingActionis complete.frontend/javascripts/viewer/view/action-bar/tracing_actions_view.tsx (1)
58-59: Import/dispatch rename OK
setSkeletonTracingActionreplaces the old name and the payload structure stays untouched — looks good.Also applies to: 568-573
frontend/javascripts/test/backend-snapshot-tests/annotations.e2e.ts (1)
187-219: LGTM! Comprehensive test coverage for user state functionality.The test properly verifies that user-specific states are correctly stored and retrieved:
- Active node ID in skeleton user states
- Camera position, rotation, and zoom level in annotation user states
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_2.spec.ts (1)
16-16: Good test cleanup improvements.The changes enhance test isolation by:
- Using
resetStoreAction()for more comprehensive store reset- Adding
tearDownPullQueues()to properly clean up async operationsThese changes are consistent with the improvements made to
volumetracing_saga_integration_1.spec.ts.Also applies to: 41-41, 60-60
frontend/javascripts/viewer/model/accessors/annotation_accessor.ts (1)
96-119: Well-implemented user state retrieval with proper fallback logic.The
getUserStateForTracingfunction correctly implements the fallback hierarchy as described in the PR objectives:
- First attempts to find the active user's state
- Falls back to the owner's state if not found
- Returns undefined if neither exists
The generic implementation allows reuse across different user state types.
frontend/javascripts/test/sagas/saga_integration.spec.ts (2)
101-101: Good use of numeric separators.Using underscores in numeric literals improves readability.
115-117: Verify the expected chunking behavior change.The test now expects 2 chunks instead of 1 after deleting a node. This appears to be an intentional change, possibly due to additional user state data or modified chunking logic. Please confirm this behavioral change is expected and consider adding a comment explaining why the save queue now produces 2 chunks.
frontend/javascripts/test/fixtures/tasktracing_server_objects.ts (1)
54-56: Test fixture updates align with per-user state management.The addition of
userStatesarrays and other property changes correctly reflect the new data model for storing individual user preferences. The empty arrays are appropriate for test fixtures representing initial states.Also applies to: 196-196
app/models/annotation/AnnotationMerger.scala (1)
39-39: User ID propagation for annotation merging looks good.The addition of
requestingUserIdand user ID mapping properly supports per-user state management during annotation merging. The parameters are consistently propagated through the call chain, enabling the tracing store to handle user-specific states correctly.Also applies to: 49-54, 72-72, 78-81
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingService.scala (3)
527-539: LGTM! Method signature correctly extended for user state support.The addition of
ownerIdandrequestingUserIdparameters aligns with the PR objective of implementing per-user state management for annotations.
545-545: User state handling is correctly implemented.The user state is properly generated using
renderVolumeUserStateIntoUserStateand included in the duplicated tracing, ensuring that user-specific view states are preserved during duplication.Also applies to: 556-557
738-781: Comprehensive user state merging implementation.The merging logic correctly handles:
- Group mapping calculation and segment group merging
- Segment ID remapping from
mergedVolumeStats.labelMaps- User bounding box combination with ID mapping
- User state merging with proper remapping of groups, segments, and bounding boxes
The implementation ensures that user-specific states are properly preserved and remapped during annotation merging.
app/controllers/AnnotationIOController.scala (1)
416-416: Consistent parameter naming improvement.The rename from
issuingUsertorequestingUserbetter reflects the semantic meaning - this represents the user requesting the download operation, making the code more intuitive.Also applies to: 428-429, 446-447, 475-476, 491-492, 529-530, 535-536
frontend/javascripts/viewer/model/reducers/skeletontracing_reducer.ts (2)
60-70: User state integration is well-implemented.The initialization correctly:
- Retrieves user-specific state based on the active user and annotation owner
- Creates a transformation function to apply user state to trees
- Applies user state to tree groups and bounding boxes
- Sets the active node from user state when available
This ensures that each user's view preferences are properly restored when loading the annotation.
Also applies to: 110-111
467-475: Simplified state update improves code clarity.The direct assignment
$set: action.tracingis cleaner than the previous nested update approach and achieves the same result more efficiently.frontend/javascripts/viewer/model/actions/skeletontracing_actions.tsx (1)
62-62: LGTM! Clean refactoring for better naming clarity.The renaming from
SetTracingActiontoSetSkeletonTracingActionmakes the intent more explicit and better differentiates skeleton tracing from other tracing types.Also applies to: 128-128, 530-534
frontend/javascripts/viewer/model/sagas/save_saga.ts (1)
366-390: Well-structured annotation diff logicThe
performDiffAnnotationfunction correctly handles camera state changes and only generates update actions when actual changes are detected.frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts (1)
268-307: Well-implemented user state integrationThe changes properly integrate user-specific state management for volume tracings, including:
- User-specific bounding boxes
- Segment visibility overrides
- Active segment selection
- Proper fallback handling through
getUserStateForTracingThe implementation aligns well with the PR objectives.
Also applies to: 427-431
app/models/annotation/AnnotationService.scala (1)
363-367: LGTM! Proper user state tracking for task assignments.The addition of
annotationBase._useranduser._idparameters correctly implements the user state management feature, allowing the system to track both the original annotation owner and the new assignee.app/models/annotation/AnnotationLayerPrecedence.scala (1)
31-31: Good addition for user-specific bounding box visibility tracking.The new field properly captures per-user bounding box visibility states as a map from user ID to bounding box IDs and their visibility states.
frontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts (2)
604-625: Excellent refactoring for granular tree group state management.The new approach correctly separates content changes from expansion state changes, which is essential for the per-user state management feature. This ensures that one user's expansion/collapse actions don't affect other users' views.
630-648: Well-structured handling of user bounding box states.The implementation properly distinguishes between content changes and visibility state changes, allowing for per-user visibility preferences while maintaining shared bounding box definitions.
frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (1)
469-471: Identity-only short-circuit may miss in-place mutations
if (prevVolumeTracing === volumeTracing) return;assumes immutability.
If any nested field is mutated in place, the reference remains equal and no further diffing occurs, silently dropping required updates. Please ensureVolumeTracingobjects are treated as strictly immutable (e.g. enforced by Redux reducers) or add a cheap hash / version check here.app/models/task/TaskCreationService.scala (1)
72-79: Extra parameter breaks callers
createTracingsFromBaseAnnotations(...)now requiresrequestingUserId.
Please update all call sites (e.g. TaskController, tests) or compilation will fail.
...store/app/com/scalableminds/webknossos/tracingstore/annotation/AnnotationUpdateActions.scala
Show resolved
Hide resolved
...scalableminds/webknossos/tracingstore/tracings/skeleton/updating/SkeletonUpdateActions.scala
Show resolved
Hide resolved
| const segmentVisibilityMap: Record<number, boolean> = userState | ||
| ? Object.fromEntries(_.zip(userState.segmentIds, userState.segmentVisibilities)) | ||
| : {}; |
There was a problem hiding this comment.
Validate array lengths before creating segmentVisibilityMap
The _.zip function will create tuples with undefined values if userState.segmentIds and userState.segmentVisibilities have different lengths. This could lead to unexpected behavior in the visibility map.
- const segmentVisibilityMap: Record<number, boolean> = userState
- ? Object.fromEntries(_.zip(userState.segmentIds, userState.segmentVisibilities))
- : {};
+ const segmentVisibilityMap: Record<number, boolean> = {};
+ if (userState) {
+ if (userState.segmentIds.length !== userState.segmentVisibilities.length) {
+ console.warn("Segment IDs and visibilities arrays have different lengths");
+ }
+ userState.segmentIds.forEach((id, index) => {
+ if (index < userState.segmentVisibilities.length) {
+ segmentVisibilityMap[id] = userState.segmentVisibilities[index];
+ }
+ });
+ }📝 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 segmentVisibilityMap: Record<number, boolean> = userState | |
| ? Object.fromEntries(_.zip(userState.segmentIds, userState.segmentVisibilities)) | |
| : {}; | |
| const segmentVisibilityMap: Record<number, boolean> = {}; | |
| if (userState) { | |
| if (userState.segmentIds.length !== userState.segmentVisibilities.length) { | |
| console.warn("Segment IDs and visibilities arrays have different lengths"); | |
| } | |
| userState.segmentIds.forEach((id, index) => { | |
| if (index < userState.segmentVisibilities.length) { | |
| segmentVisibilityMap[id] = userState.segmentVisibilities[index]; | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts between
lines 284 and 286, the code creates segmentVisibilityMap using _.zip on
userState.segmentIds and userState.segmentVisibilities without validating their
lengths. To fix this, add a check to ensure both arrays have the same length
before calling _.zip, or handle mismatched lengths appropriately to prevent
unexpected undefined values in the resulting map.
| private def adaptSkeletonUserStates( | ||
| userStates: Seq[SkeletonUserStateProto], | ||
| oldPrecedenceLayerProperties: RedundantTracingProperties): Seq[SkeletonUserStateProto] = { | ||
| val adaptedExistingUserStates = userStates.map { userState => | ||
| val userId = userState.userId | ||
| oldPrecedenceLayerProperties.userStateBoundingBoxVisibilities.get(userId) match { | ||
| case None => userState | ||
| case Some((precedenceBboxIds, precedenceBboxVisibilities)) => | ||
| userState.copy(boundingBoxIds = precedenceBboxIds, boundingBoxVisibilities = precedenceBboxVisibilities) | ||
| } | ||
| } | ||
| // We also have to create new user states for the users the old precedence layer has, but the new precedence layer is missing. | ||
| val newUserPrecedenceProperties = oldPrecedenceLayerProperties.userStateBoundingBoxVisibilities.filter(tuple => | ||
| !userStates.exists(_.userId == tuple._1)) | ||
| val newUserStates = newUserPrecedenceProperties.map { | ||
| case (userId: String, (boundingBoxIds: Seq[Int], boundingBoxVisibilities: Seq[Boolean])) => | ||
| SkeletonTracingDefaults | ||
| .emptyUserState(userId) | ||
| .copy( | ||
| boundingBoxIds = boundingBoxIds, | ||
| boundingBoxVisibilities = boundingBoxVisibilities | ||
| ) | ||
| } | ||
| adaptedExistingUserStates ++ newUserStates | ||
| } | ||
|
|
||
| private def adaptVolumeUserStates( | ||
| userStates: Seq[VolumeUserStateProto], | ||
| oldPrecedenceLayerProperties: RedundantTracingProperties): Seq[VolumeUserStateProto] = { | ||
| val adaptedExistingUserStates = userStates.map { userState => | ||
| val userId = userState.userId | ||
| oldPrecedenceLayerProperties.userStateBoundingBoxVisibilities.get(userId) match { | ||
| case None => userState | ||
| case Some((precedenceBboxIds, precedenceBboxVisibilities)) => | ||
| userState.copy(boundingBoxIds = precedenceBboxIds, boundingBoxVisibilities = precedenceBboxVisibilities) | ||
| } | ||
| } | ||
| // We also have to create new user states for the users the old precedence layer has, but the new precedence layer is missing. | ||
| val newUserPrecedenceProperties = oldPrecedenceLayerProperties.userStateBoundingBoxVisibilities.filter(tuple => | ||
| !userStates.exists(_.userId == tuple._1)) | ||
| val newUserStates = newUserPrecedenceProperties.map { | ||
| case (userId: String, (boundingBoxIds: Seq[Int], boundingBoxVisibilities: Seq[Boolean])) => | ||
| VolumeTracingDefaults | ||
| .emptyUserState(userId) | ||
| .copy( | ||
| boundingBoxIds = boundingBoxIds, | ||
| boundingBoxVisibilities = boundingBoxVisibilities | ||
| ) | ||
| } | ||
| adaptedExistingUserStates ++ newUserStates | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to eliminate code duplication.
The adaptSkeletonUserStates and adaptVolumeUserStates functions are nearly identical, differing only in their proto types. This duplication could lead to maintenance issues.
Consider extracting the common logic into a generic helper function:
private def adaptUserStatesGeneric[T](
userStates: Seq[T],
oldPrecedenceLayerProperties: RedundantTracingProperties,
getUserId: T => String,
updateBoundingBoxProperties: (T, Seq[Int], Seq[Boolean]) => T,
createDefaultUserState: String => T
): Seq[T] = {
val adaptedExistingUserStates = userStates.map { userState =>
val userId = getUserId(userState)
oldPrecedenceLayerProperties.userStateBoundingBoxVisibilities.get(userId) match {
case None => userState
case Some((precedenceBboxIds, precedenceBboxVisibilities)) =>
updateBoundingBoxProperties(userState, precedenceBboxIds, precedenceBboxVisibilities)
}
}
val newUserPrecedenceProperties = oldPrecedenceLayerProperties.userStateBoundingBoxVisibilities.filter(tuple =>
!userStates.exists(state => getUserId(state) == tuple._1))
val newUserStates = newUserPrecedenceProperties.map {
case (userId: String, (boundingBoxIds: Seq[Int], boundingBoxVisibilities: Seq[Boolean])) =>
val defaultState = createDefaultUserState(userId)
updateBoundingBoxProperties(defaultState, boundingBoxIds, boundingBoxVisibilities)
}
adaptedExistingUserStates ++ newUserStates
}Then simplify the specific implementations:
private def adaptSkeletonUserStates(
userStates: Seq[SkeletonUserStateProto],
oldPrecedenceLayerProperties: RedundantTracingProperties): Seq[SkeletonUserStateProto] =
adaptUserStatesGeneric(
userStates,
oldPrecedenceLayerProperties,
_.userId,
(state, ids, vis) => state.copy(boundingBoxIds = ids, boundingBoxVisibilities = vis),
SkeletonTracingDefaults.emptyUserState
)
private def adaptVolumeUserStates(
userStates: Seq[VolumeUserStateProto],
oldPrecedenceLayerProperties: RedundantTracingProperties): Seq[VolumeUserStateProto] =
adaptUserStatesGeneric(
userStates,
oldPrecedenceLayerProperties,
_.userId,
(state, ids, vis) => state.copy(boundingBoxIds = ids, boundingBoxVisibilities = vis),
VolumeTracingDefaults.emptyUserState
)🤖 Prompt for AI Agents
In app/models/annotation/AnnotationLayerPrecedence.scala between lines 77 and
127, the functions adaptSkeletonUserStates and adaptVolumeUserStates are nearly
identical, leading to duplicated code. To improve maintainability, extract the
common logic into a generic helper function that takes parameters for user state
type, user ID extraction, state updating, and default state creation. Then
refactor the specific functions to call this generic helper with appropriate
arguments, reducing duplication and simplifying future updates.
| if (prevVolumeTracing.fallbackLayer != null && volumeTracing.fallbackLayer == null) { | ||
| yield removeFallbackLayer(volumeTracing.tracingId); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Only handles fallback-layer removal
You handle the transition “had layer → now none”, but not the inverse (“none → newly added”). If a fallback layer gets (re-)added, the scene will not update until a full reload. Add the complementary branch.
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx around lines
530 to 533, the code only handles removing the fallback layer when transitioning
from having a fallback layer to none, but does not handle the case where a
fallback layer is added. Add an else-if branch to handle the scenario where
fallbackLayer is now non-null while previously it was null, and call a function
to add or update the fallback layer accordingly to ensure the scene updates
correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
app/controllers/AnnotationController.scala (1)
420-428: LGTM! Parameter order issue has been resolved.The parameter order for
duplicateAnnotationhas been corrected to match the expected method signature. The call now properly passes:
annotation._id(annotationId)newAnnotationIdannotation._user(ownerId - original annotation's user)user._id(requestingUserId - user making the duplication request)This addresses the issue flagged in the previous review about incorrect argument ordering.
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/skeleton/updating/SkeletonUpdateActions.scala (2)
546-548: Previous syntax error has been fixed.The trailing comma issue mentioned in past review comments has been resolved. The code now compiles correctly.
590-592: Previous logic error has been fixed.The code now correctly updates
treeIdsandtreeVisibilitiesinstead of the incorrecttreeGroupIdsthat was flagged in previous reviews.webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/skeleton/TreeUtils.scala (1)
31-36: Breaking API change requires caller update.The
mergeTreesmethod signature has changed from 4 to 6 parameters. The caller in SkeletonTracingService.scala needs to be updated to pass the additionalnodeMappingAandgroupMappingAparameters.
🧹 Nitpick comments (3)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/TSAnnotationController.scala (1)
192-281: Consider refactoring for better maintainability.The
mergedFromIdsmethod correctly implements the merging logic and addresses validation concerns from past reviews. However, the method is quite complex (90 lines) and handles multiple responsibilities.Consider extracting some of the logic into smaller, focused methods:
- Volume merging logic (lines 212-246)
- Skeleton merging logic (lines 247-257)
- Final annotation assembly (lines 258-277)
This would improve readability and testability while maintaining the same functionality.
private def mergeVolumeTracings(annotations: Seq[AnnotationProto], ownerIds: Seq[String], requestingUserId: String, newVolumeId: TracingId, newAnnotationId: String, toTemporaryStore: Boolean) (implicit tc: TokenContext): Fox[(Option[VolumeTracing], Long)] = { // Extract volume merging logic here } private def mergeSkeletonTracings(annotations: Seq[AnnotationProto], ownerIds: Seq[String], requestingUserId: String, newSkeletonId: TracingId, targetVersion: Long, toTemporaryStore: Boolean) (implicit tc: TokenContext): Fox[Option[SkeletonTracing]] = { // Extract skeleton merging logic here }webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/AnnotationUserStateUtils.scala (2)
32-137: Consider refactoring this large method for better maintainability.The
renderUserStateForTracingmethod is over 100 lines long with significant duplication between skeleton and volume cases. Consider extracting common logic into helper methods.For example, you could extract common patterns:
private def buildVisibilityMap[T](userState: Option[_], ids: Seq[T], visibilities: Seq[Boolean]): Map[T, Boolean] = userState.map(_ => ids.zip(visibilities).toMap).getOrElse(Map.empty) private def mergeWithFallback[T](requestingMap: Map[Int, T], ownerMap: Map[Int, T], id: Int, default: Option[T]): Option[T] = requestingMap.get(id).orElse(ownerMap.get(id)).orElse(default)This would reduce duplication and make the code more modular.
12-23: Consider adding ScalaDoc comments for public methods.The trait and its public methods would benefit from documentation explaining the user state management behavior, especially the precedence rules when multiple user states exist.
Add documentation like:
/** * Utilities for managing user-specific annotation states. * Handles rendering, merging, and transforming user states with fallback logic. */ trait AnnotationUserStateUtils extends BoundingBoxMerger { /** * Renders user-specific state onto annotation layers. * Uses requesting user's state with fallback to owner's state. * * @param annotationProto The annotation containing user states * @param annotationLayers The layers to render state onto * @param requestingUserId The user requesting the annotation (if any) * @param ownerId The owner of the annotation * @return Annotation layers with user-specific state applied */ protected def renderUserState(...)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/controllers/AnnotationController.scala(1 hunks)test/backend/AnnotationUserStateTestSuite.scala(1 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/TSAnnotationController.scala(7 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/AnnotationUserStateUtils.scala(1 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/skeleton/TreeUtils.scala(2 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/skeleton/updating/SkeletonUpdateActions.scala(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (23)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/skeleton/updating/SkeletonUpdateActions.scala (6)
5-11: LGTM! Proper imports for user state functionality.The imports correctly include the necessary user state protocol buffer classes and skeleton tracing defaults.
26-45: Well-designed user state action trait with proper conditional logic.The
UserStateSkeletonUpdateActiontrait provides a clean abstraction for user-specific skeleton updates. TheapplyOnmethod correctly handles both creating new user states and updating existing ones based on the presence ofactionAuthorId.
428-465: Efficient implementation of tree group expansion state management.The
UpdateTreeGroupsExpandedStateSkeletonActioncorrectly uses mutable maps for efficient updates of expansion states. The logic properly handles both existing and new user states with appropriate fallbacks.
500-520: Simple and correct active node state management.The
UpdateActiveNodeSkeletonActionfollows the established pattern and correctly updates the active node in user state with proper fallback to empty state when needed.
662-693: Consistent user state pattern for bounding box visibility.The
UpdateUserBoundingBoxVisibilitySkeletonActioncorrectly follows the established user state pattern with proper handling of single vs. all bounding boxes and mutable map operations for efficient updates.
731-756: Standard JSON format implementations.The companion objects correctly provide JSON serialization formats for the new action classes following the established pattern.
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/TSAnnotationController.scala (3)
37-41: LGTM! Request validation implemented correctly.The
MergedFromIdsRequestcase class is well-defined with proper JSON serialization support. The validation for equal length sequences (mentioned in past reviews) has been properly implemented in themergedFromIdsmethod at line 199.
118-119: LGTM! Method signature updated appropriately for user state support.The addition of
ownerIdandrequestingUserIdparameters to theduplicatemethod aligns with the PR objectives for user-specific state management. The parameters are correctly passed through to the annotation service.Also applies to: 131-132
158-190: LGTM! Helper methods are well-structured and consistent.Both helper methods follow a consistent pattern:
- Proper filtering by annotation layer type
- Correct variable naming (the previous
volumeLayersOfAnnotationtypo has been fixed)- Appropriate user state rendering logic
- Good error handling with Fox monads
The methods effectively encapsulate the logic for retrieving and adapting tracings with user-specific states.
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/skeleton/TreeUtils.scala (6)
11-11: Good addition of type alias for clarity.The
TreeIdMaptype alias improves code readability and makes the API more self-documenting.
37-47: Implementation correctly applies ID mappings.The logic properly:
- Extracts node IDs for efficient lookups
- Applies tree, node, and group mappings to treesA
- Applies only tree ID mapping to treesB
- Preserves treesB order by concatenating it first
49-61: Comprehensive node mapping application.The method correctly applies node mapping to all node references including nodes, edges, comments, and branch points. The addition of
nodeIdsAparameter enables selective remapping in comment content.
62-69: Excellent improvement for selective node reference remapping.The conditional remapping based on
nodeIdsA.contains(oldId)ensures that only references to nodes from treesA are remapped, preserving references to external nodes. This prevents incorrect remapping of node IDs in comments.
77-89: Well-designed tree ID densification strategy.The approach of creating dense sequential tree IDs prevents ID space explosion over multiple merges. The implementation correctly:
- Creates non-overlapping ID ranges for both tree sets
- Starts treesB IDs from 1 and treesA IDs from (treesB.length + 1)
- Maintains a sorted, deterministic mapping
91-98: Parameter naming consistency maintained.The parameter names are updated to match the new naming convention (treesA/treesB) while preserving the correct offset calculation logic.
test/backend/AnnotationUserStateTestSuite.scala (4)
1-7: LGTM: Clean imports and package structure.The imports are well-organized and include all necessary dependencies for testing annotation user state functionality.
8-30: Well-structured test setup with comprehensive dummy data.The dummy skeleton data provides good test coverage with two different user states having varying levels of completeness, which effectively tests the fallback behavior.
31-62: Test structure issues resolved - comprehensive skeleton user state testing.The test structure issues mentioned in past review comments have been properly addressed. The tests now follow correct ScalaTest conventions with proper nesting and closing braces. The three test scenarios effectively cover:
- userA: Tests fallback behavior when user has sparse state data
- userB (owner): Tests owner's complete state usage
- userC: Tests fallback to owner's state when user has no state
The assertions correctly verify the expected merge/fallback logic for each scenario.
64-103:✅ Verification successful
Verify the ID mapping logic in the volume user state merging test.
The test logic appears correct, but let me verify the expected behavior:
segmentIdMapBmaps segment ID 1 → 2 for tracingBgroupMappingAmaps group ID 1 → 6 for tracingA- Expected result:
segmentIds = Seq(1L, 2L)andsegmentGroupIds = Seq(6, 1)This suggests tracingA keeps original segment ID (1) but gets mapped group ID (6), while tracingB gets mapped segment ID (2) but keeps original group ID (1).
Please confirm this is the intended merging behavior by running the following verification:
🏁 Script executed:
#!/bin/bash # Search for the mergeVolumeUserStates function implementation to understand the expected behavior ast-grep --pattern 'def mergeVolumeUserStates($$$) = { $$$ }'Length of output: 65
Let’s locate the actual implementation to confirm its behavior:
🏁 Script executed:
#!/bin/bash # Find where mergeVolumeUserStates is defined rg -n 'def\s\+mergeVolumeUserStates'Length of output: 38
Let’s inspect the test imports and search the
srctree for the real implementation:
🏁 Script executed:
#!/bin/bash set -e echo "=== Test file imports and package ===" sed -n '1,50p' test/backend/AnnotationUserStateTestSuite.scala echo echo "=== Searching for mergeVolumeUserStates in src ===" rg -n 'mergeVolumeUserStates' srcLength of output: 2437
Let’s search the entire repository for the
mergeVolumeUserStatesdefinition and, if not found, locate and inspect theAnnotationUserStateUtilsfile where it should live:
🏁 Script executed:
#!/bin/bash set -e echo "=== Searching for mergeVolumeUserStates ===" rg -n mergeVolumeUserStates || echo "No matches for mergeVolumeUserStates" echo echo "=== Locating AnnotationUserStateUtils source ===" utils_file=$(rg -l 'trait AnnotationUserStateUtils|object AnnotationUserStateUtils') if [ -n "$utils_file" ]; then echo "Found file: $utils_file" echo echo "=== Inspecting first 200 lines of $utils_file ===" sed -n '1,200p' "$utils_file" else echo "AnnotationUserStateUtils not found in repository" fiLength of output: 13410
No action needed: mergeVolumeUserStates mapping logic is correct.
The test correctly applies
groupMappingAto tracing A’s segment group IDs andsegmentIdMapBto tracing B’s segment IDs, producing
• segmentIds = Seq(1L, 2L)
• segmentVisibilities = Seq(true, false)
• segmentGroupIds = Seq(6, 1)
• segmentGroupExpandedStates = Seq(true, false)All mappings behave as intended; the assertion stands.
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/AnnotationUserStateUtils.scala (4)
156-156: The merge order inconsistency has been fixed.Good job fixing the merge order - both skeleton (line 156) and volume (line 202) implementations now consistently give precedence to the requesting user's visibility settings.
Also applies to: 202-202
274-281: The method name typo has been fixed.The method names are now correctly spelled as
applyIdMappingsOnSkeletonUserStateandapplyIdMappingsOnVolumeUserState.Also applies to: 342-347
283-296: Good implementation of ID mapping with proper filtering.The bounding box ID mapping methods correctly filter out unmapped IDs using
flatMap, which prevents issues with missing mappings. The implementation is clean and handles edge cases well.Also applies to: 349-362
261-272:✅ Verification successful
Verify that duplicate IDs are handled correctly when merging user states.
The merge methods concatenate lists without handling potential duplicates. This could lead to duplicate tree/segment/bbox IDs if both user states contain settings for the same elements.
Run the following script to check if the codebase handles duplicate IDs when converting these lists to maps:
Consider whether you need to deduplicate or use the last-one-wins strategy when merging these lists.
Also applies to: 329-340
🏁 Script executed:
#!/bin/bash # Description: Check how the concatenated lists are used and if duplicates are handled # Search for usage of the merged lists to understand if duplicate handling is needed ast-grep --pattern 'userState.$_Ids.zip(userState.$_)' # Look for any deduplication logic rg -A 5 "treeIds.*zip.*treeVisibilities|segmentIds.*zip.*segmentVisibilities|boundingBoxIds.*zip.*boundingBoxVisibilities"Length of output: 19200
Duplicate ID handling confirmed; no action required
The merged lists are immediately converted into maps via.zip(...).toMap, which automatically deduplicates keys by keeping the last occurrence. Subsequent merges usingownerMap ++ requestingMapalso apply a last-one-wins strategy.
…otation-user-state
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (2)
508-516: Reference equality check is appropriate in this context.The reference equality check
prevVolumeTracing.segments !== volumeTracing.segmentsis actually correct here because the Redux state management ensures immutability. When segments are updated, a newSegmentMapinstance is created, so reference inequality properly indicates changes.Note: This addresses the previous review comment about stale diffs. The memoization with
cachedDiffSegmentListscombined with reference equality is the intended pattern for immutable state.
540-557: Fallback layer handling could be more comprehensive.The current implementation only handles the removal of fallback layers but doesn't handle the case where a fallback layer is added.
Consider adding the complementary case:
if (prevVolumeTracing.fallbackLayer != null && volumeTracing.fallbackLayer == null) { yield removeFallbackLayer(volumeTracing.tracingId); } + if (prevVolumeTracing.fallbackLayer == null && volumeTracing.fallbackLayer != null) { + // Handle fallback layer addition - implementation depends on requirements + // This might require a new update action or scene controller refresh + }
🧹 Nitpick comments (2)
frontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts (2)
701-720: Remove TODO comment and clarify implementation status.The bounding box diffing logic looks correct, but there's a "todop" comment that should be addressed.
Apply this diff to clean up the comment:
- if (boxDiff.didContentChange) { - // todop - yield LEGACY_updateUserBoundingBoxesInSkeletonTracing( + if (boxDiff.didContentChange) { + // Use legacy action for content changes until full migration is complete + yield LEGACY_updateUserBoundingBoxesInSkeletonTracing(
723-729: Clarify the status of commented legacy code.The commented-out
diffBoundingBoxescall should either be removed if it's no longer needed or have a clear comment explaining why it's temporarily disabled.Either remove the commented code or add a clear explanation:
- // todop: integrate this? - // yield* diffBoundingBoxes( - // prevSkeletonTracing.userBoundingBoxes, - // skeletonTracing.userBoundingBoxes, - // skeletonTracing.tracingId, - // AnnotationLayerEnum.Skeleton, - // ); + // Legacy diffBoundingBoxes approach is replaced by the granular diffing above + // Remove this once the new approach is fully validated
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/libs/utils.ts(2 hunks)frontend/javascripts/viewer/model/helpers/compaction/compact_save_queue.ts(1 hunks)frontend/javascripts/viewer/model/reducers/reducer_helpers.ts(8 hunks)frontend/javascripts/viewer/model/reducers/skeletontracing_reducer_helpers.ts(1 hunks)frontend/javascripts/viewer/model/sagas/save_saga.ts(7 hunks)frontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/update_actions.ts(12 hunks)frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx(3 hunks)frontend/javascripts/viewer/view/version_entry.tsx(3 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/annotation/UpdateActions.scala(9 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/skeleton/updating/SkeletonUpdateActions.scala(7 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeUpdateActions.scala(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.unreleased.md
🚧 Files skipped from review as they are similar to previous changes (9)
- frontend/javascripts/libs/utils.ts
- frontend/javascripts/viewer/view/version_entry.tsx
- frontend/javascripts/viewer/model/reducers/skeletontracing_reducer_helpers.ts
- frontend/javascripts/viewer/model/reducers/reducer_helpers.ts
- webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/annotation/UpdateActions.scala
- frontend/javascripts/viewer/model/helpers/compaction/compact_save_queue.ts
- webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeUpdateActions.scala
- frontend/javascripts/viewer/model/sagas/save_saga.ts
- webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/skeleton/updating/SkeletonUpdateActions.scala
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (3)
frontend/javascripts/viewer/store.ts (1)
VolumeTracing(191-212)frontend/javascripts/viewer/model/sagas/update_actions.ts (8)
updateActiveSegmentId(455-463)updateLargestSegmentId(451-453)LEGACY_updateUserBoundingBoxesInVolumeTracing(480-493)updateUserBoundingBoxVisibilityInVolumeTracing(520-529)updateSegmentGroups(719-727)updateSegmentGroupsExpandedState(729-742)removeFallbackLayer(791-798)updateMappingName(830-845)frontend/javascripts/viewer/model/helpers/diff_helpers.ts (2)
diffUserBoundingBoxes(74-94)diffGroups(32-52)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (12)
frontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts (3)
69-69: LGTM: Import changes align with the refactoring approach.The imports correctly introduce the new granular update actions and mark legacy functions appropriately. The addition of specific update actions like
updateActiveNodeandupdateTreeGroupsExpandedStatesupports the fine-grained update approach.Also applies to: 76-76, 78-78, 82-82
675-695: LGTM: Well-structured group diffing logic.The new logic properly separates content changes from expansion state changes, which is essential for per-user state management. The approach of checking
didContentChangefor server updates while handling expanded state separately is correct.
697-699: LGTM: Clean active node update logic.The direct comparison and specific update action is much cleaner than the previous broad update approach.
frontend/javascripts/viewer/model/sagas/update_actions.ts (6)
28-32: LGTM: Consistent type definitions for new granular actions.The new export types properly reflect the granular update approach and maintain consistency across skeleton and volume tracing actions.
Also applies to: 75-82, 88-88
117-123: LGTM: Union type updates maintain backward compatibility.The union types correctly include the new granular actions while preserving legacy action types for backward compatibility.
Also applies to: 142-142, 145-145, 147-147
371-408: LGTM: Proper legacy marking and clean implementation.The legacy function is correctly marked and not exported, while the new
updateActiveNodefunction has a clean, focused implementation that only handles the active node state.
427-463: LGTM: Consistent granular action pattern.The new
updateActiveSegmentIdandupdateLargestSegmentIdfunctions follow the same pattern as the skeleton tracing actions, providing a consistent API across tracing types.
729-757: LGTM: Well-designed expanded state actions.The
updateSegmentGroupsExpandedStateandupdateTreeGroupsExpandedStatefunctions properly support batch updates of expansion states, which is efficient for user state management.
799-814: LGTM: Clean camera annotation action.The
updateCameraAnnotationfunction properly separates camera state from tracing data, which aligns with the goal of separating user-specific UI state from annotation content.frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (3)
467-479: LGTM: Clean granular property updates.The new approach of checking individual properties and dispatching specific update actions is much cleaner and supports per-user state management effectively.
481-506: LGTM: Consistent bounding box diffing pattern.The bounding box diffing logic follows the same pattern as the skeleton tracing saga, providing consistency across tracing types. The use of the
diffUserBoundingBoxeshelper function is appropriate.
518-538: LGTM: Well-structured group diffing implementation.The group diffing logic properly separates content changes from expansion state changes, using the helper function consistently with the skeleton tracing approach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/viewer/model/helpers/diff_helpers.ts (1)
18-24: Consider adding return type annotation for consistency.While the implementation is correct, adding an explicit return type would improve consistency with other functions in the codebase.
-function stripIsExpanded(groups: TreeGroup[]): TreeGroup[] { +function stripIsExpanded(groups: TreeGroup[]): Omit<TreeGroup, "isExpanded">[] {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/javascripts/test/sagas/bounding_box_saving.spec.ts(4 hunks)frontend/javascripts/viewer/model/helpers/diff_helpers.ts(1 hunks)frontend/javascripts/viewer/model/reducers/reducer_helpers.ts(7 hunks)frontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/update_actions.ts(15 hunks)frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx(3 hunks)frontend/javascripts/viewer/store.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/javascripts/viewer/store.ts
- frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx
- frontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
frontend/javascripts/test/sagas/bounding_box_saving.spec.ts (1)
frontend/javascripts/viewer/model/helpers/diff_helpers.ts (1)
diffBoundingBoxes(186-201)
frontend/javascripts/viewer/model/reducers/reducer_helpers.ts (4)
frontend/javascripts/types/api_types.ts (4)
ServerBoundingBox(838-843)UserBoundingBoxFromServer(844-850)SkeletonUserState(916-931)VolumeUserState(946-961)frontend/javascripts/viewer/constants.ts (1)
BoundingBoxType(41-44)frontend/javascripts/viewer/store.ts (3)
UserBoundingBox(110-112)UserBoundingBoxWithOptIsVisible(113-115)UserBoundingBoxToServer(91-97)frontend/javascripts/viewer/model/types/tree_types.ts (2)
TreeGroup(85-87)Tree(61-75)
⏰ 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 (7)
frontend/javascripts/test/sagas/bounding_box_saving.spec.ts (1)
3-3: LGTM! Test correctly updated for new diffing behavior.The test has been properly updated to:
- Use the new
diffBoundingBoxeshelper from the diff_helpers module- Handle the increased number of diff actions (from 4 to 5) due to separate visibility updates
- Assert the correct structure for the new visibility update actions
Also applies to: 67-119
frontend/javascripts/viewer/model/reducers/reducer_helpers.ts (2)
46-65: Well-implemented user state integration for bounding boxes.The function correctly:
- Creates an id-to-visibility mapping from the user state arrays using lodash zip
- Falls back through
idToVisible[id] ?? isVisible ?? trueto ensure a sensible default- Handles undefined user state gracefully
223-248: Robust implementation of group expansion state handling.Good use of type guards to distinguish between skeleton and volume user states. The recursive mapping with
Utils.mapGroupsDeepensures all nested groups receive their user-specific expansion states.frontend/javascripts/viewer/model/helpers/diff_helpers.ts (1)
44-201: Excellent implementation of diffing utilities.The diffing logic is well-structured with:
- Early return optimizations for reference equality
- Proper separation between content and visibility changes
- Clear error messages for debugging
- Efficient use of generator functions
frontend/javascripts/viewer/model/sagas/update_actions.ts (3)
35-36: Good practice: Legacy actions clearly marked.Renaming legacy actions with the
LEGACY_prefix and adding deprecation comments helps prevent their use in new code while maintaining backward compatibility.Also applies to: 123-124, 378-401, 433-455
403-469: Well-designed granular update actions.The new update actions for active nodes, segments, group expansion states, and camera position provide better granularity and align well with the per-user state feature. This will improve performance by reducing unnecessary updates.
Also applies to: 735-820
500-524:❓ Verification inconclusive
Verify callers are updated for the parameter type change.
The parameter type for bounding box functions changed from
UserBoundingBoxtoUserBoundingBoxWithOptIsVisible. This is a good change as it makes theisVisibleproperty optional, but ensure all callers have been updated.
🏁 Script executed:
#!/bin/bash # Description: Check for calls to addUserBoundingBox functions to ensure they handle the optional isVisible property # Search for calls to these functions echo "Searching for addUserBoundingBoxInSkeletonTracing calls:" rg -A 3 "addUserBoundingBoxInSkeletonTracing\(" --type ts --type tsx echo -e "\nSearching for addUserBoundingBoxInVolumeTracing calls:" rg -A 3 "addUserBoundingBoxInVolumeTracing\(" --type ts --type tsxLength of output: 443
I’ll broaden the search to include all
.tsand.tsxfiles without relying on predefined types:
🏁 Script executed:
#!/bin/bash # Re-run search across .ts and .tsx globs echo "Searching for addUserBoundingBoxInSkeletonTracing calls:" rg -A 3 "addUserBoundingBoxInSkeletonTracing\(" -g '*.ts' -g '*.tsx' echo -e "\nSearching for addUserBoundingBoxInVolumeTracing calls:" rg -A 3 "addUserBoundingBoxInVolumeTracing\(" -g '*.ts' -g '*.tsx'Length of output: 1113
Verify callers updated for optional
isVisibleI didn’t find any calls to
addUserBoundingBoxInSkeletonTracingoraddUserBoundingBoxInVolumeTracingoutside their own definitions. Please review all usages—especially where these actions are dispatched or handled—to ensure they correctly pass or guard the now‐optionalisVisibleproperty:• Check every
yield put(addUserBoundingBoxInSkeletonTracing(...))andyield put(addUserBoundingBoxInVolumeTracing(...))in your sagas.
• Confirm any reducers or middleware that readaction.value.boundingBox.isVisiblehandleundefined.
• If you match these actions by name or string in other parts of the code, make sure those references are updated.
| @@ -1107,6 +1107,32 @@ export function diffObjects<K extends string | number | symbol, V, Dict extends | |||
| return _.fromPairs(_.differenceWith(_.toPairs(b), _.toPairs(a), _.isEqual)) as Partial<Dict>; | |||
| } | |||
|
|
|||
| export function diffSets<T>(setA: Set<T>, setB: Set<T>) { | |||
There was a problem hiding this comment.
Might be premature optimization, but as far as I can see only bWithoutA is ever used from the result of this function. That could be more easily (and probably faster) computed using the set's difference method: setB.difference(setA).
| @@ -123,8 +112,8 @@ describe("Saga Integration Tests", () => { | |||
| const skeletonSaveQueue = Store.getState().save.queue; | |||
|
|
|||
| // There should only be one chunk | |||
There was a problem hiding this comment.
The comment is no longer accurate. Please mention what the first save queue entry is
| const updateSkeletonTracingOnlyBatches = updateActionsBatches.filter( | ||
| (batch) => batch.actions.length === 1 && batch.actions[0].name === "updateSkeletonTracing", | ||
| (batch) => batch.actions.length === 1 && batch.actions[0].name === "updateActiveNode", | ||
| ); | ||
| const updateVolumeTracingOnlyBatches = updateActionsBatches.filter( | ||
| (batch) => batch.actions.length === 1 && batch.actions[0].name === "updateVolumeTracing", | ||
| (batch) => batch.actions.length === 1 && batch.actions[0].name === "updateActiveSegmentId", |
There was a problem hiding this comment.
The names for the two variables should be adapted
| export type APITracingStoreAnnotation = { | ||
| readonly description: string; | ||
| readonly version: number; | ||
| readonly earliestAccessibleVersion: number; | ||
| readonly annotationLayers: APITracingStoreAnnotationLayer[]; | ||
| readonly userStates: APIAnnotationUserState[]; |
There was a problem hiding this comment.
It's a bit unexpected to me that all user states are accessible to each viewer of the annotation. I would have expected the server to either select the state of the active user and only return that or the data model to be different so that that happens "automatically" (because the user state is saved at the user instead of the annotation).
At a minimum the current solution exposes who has viewed an annotation (together with the user-specific properties). Whether this is problematic at the moment is debatable but I fear that more "sensitive" data might slip in there in the future. I might be overly cautious though, so I'm interested in your opinion :)
| ? userState.segmentGroupExpandedStates | ||
| : userState.treeGroupExpandedStates; | ||
|
|
||
| const segmentGroupToExpanded: Record<number, boolean> = Object.fromEntries( |
There was a problem hiding this comment.
I'd rename to groupIdToExpanded since both segments and tree groups are supported
| ) { | ||
| if (prevBoundingBoxes === currentBoundingBoxes) return; | ||
|
|
||
| yield* diffBoundingBoxesContents(prevBoundingBoxes, currentBoundingBoxes, tracingId, tracingType); |
There was a problem hiding this comment.
| yield* diffBoundingBoxesContents(prevBoundingBoxes, currentBoundingBoxes, tracingId, tracingType); | |
| yield* diffBoundingBoxContents(prevBoundingBoxes, currentBoundingBoxes, tracingId, tracingType); |
| return { visible, invisible }; | ||
| } | ||
|
|
||
| function* diffBoundingBoxesContents( |
There was a problem hiding this comment.
| function* diffBoundingBoxesContents( | |
| function* diffBoundingBoxContents( |
| }, | ||
| updateCamera: (_action: AsServerAction<UpdateCameraAnnotationAction>): Description => { | ||
| return { | ||
| description: "Updated the camera position", |
There was a problem hiding this comment.
Could actually be one of editPosition, editRotation, zoomLevel, editPositionAdditionalCoordinates. Maybe simply "Adjusted the camera"?
| action: AsServerAction<UpdateSegmentGroupsExpandedStateUpdateAction>, | ||
| ): Description => { | ||
| return { | ||
| description: `${action.value.areExpanded ? "Expanded" : "Collapsed"} some tree groups..`, |
There was a problem hiding this comment.
| description: `${action.value.areExpanded ? "Expanded" : "Collapsed"} some tree groups..`, | |
| description: `${action.value.areExpanded ? "Expanded" : "Collapsed"} some tree groups.`, |
| }; | ||
| }, | ||
| updateTreeGroupsExpandedState: ( | ||
| action: AsServerAction<UpdateSegmentGroupsExpandedStateUpdateAction>, |
There was a problem hiding this comment.
| action: AsServerAction<UpdateSegmentGroupsExpandedStateUpdateAction>, | |
| action: AsServerAction<UpdateTreeGroupsExpandedStateSkeletonAction>, |
Needs to be imported as well.
In shared annotations with multiple authors, some changes are now stored per user. This means that other users won’t see all those changes if their own diverge. This includes the current position and zoom, visibilities of trees, bounding boxes, and segments (as specified with the checkboxes in the lists), as well as which groups are expanded in the lists. The annotation owner’s user state is used as a fallback for users who haven’t explicitly changed these values themselves.
User-Specific state for annotations and tracings
Update actions that now mutate (only) the user state
Steps to test:
Note: since new update actions are introduced here, annotations that are edited on this branch cannot be loaded after switching back to master. Refresh your db to be sure everything is clean
TODOs:
Backend
UpdateSegmentGroupVisibilityVolumeActionandUpdateSegmentVisibilityVolumeActionFrontend
Issues: