[WIP] Live Collab M3 - Proofreading (without segments list) #8723
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
📝 WalkthroughWalkthroughThis change introduces a mutex mechanism for annotation editing, enabling exclusive editing rights per user. It adds backend APIs for acquiring and releasing mutexes, updates the frontend to manage and display mutex state, and integrates Redux-Saga logic for continuous mutex acquisition and release. Related types and reducers are updated accordingly. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested labels
Suggested reviewers
Poem
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
philippotto
changed the title
fm3
changed the title
| case _ => | ||
| Fox.successful(()) | ||
| } | ||
| } yield () |
There was a problem hiding this comment.
since we’re not using any info from the findOne other than whether to delete, we could merge the two into one query and just DELETE WHERE annotationId = a AND userId = u
There was a problem hiding this comment.
I changed that now in 3f66d4e – note that I could not test end-to-end since the frontend isn’t ready. Let me know if this doesn’t work as expected :)
…essary because of unified annotation versioning)
mutex was acquired This reverts commit e5b1c95.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
app/models/annotation/AnnotationMutexService.scala (1)
71-80: Consider optimizing the mutex release operation.The current approach uses separate
findOneanddeleteOneoperations. As noted in a previous review, this could be optimized into a single query that deletes only if the user owns the mutex.Consider replacing the current implementation with a single query:
-def release(annotationId: ObjectId, userId: ObjectId): Fox[Unit] = - for { - mutex <- annotationMutexDAO.findOne(annotationId).shiftBox - _ <- mutex match { - case Full(mutex) if mutex.userId == userId => - annotationMutexDAO.deleteOne(annotationId).map(_ => ()) - case _ => - Fox.successful(()) - } - } yield () +def release(annotationId: ObjectId, userId: ObjectId): Fox[Unit] = + annotationMutexDAO.deleteOneByUserAndAnnotation(annotationId, userId).map(_ => ())And add to the DAO:
def deleteOneByUserAndAnnotation(annotationId: ObjectId, userId: ObjectId): Fox[Int] = run(q"DELETE FROM webknossos.annotation_mutexes WHERE _annotation = $annotationId AND _user = $userId".asUpdate)This would be more efficient and eliminate the race condition between the find and delete operations.
🧹 Nitpick comments (3)
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (1)
361-361: Address unused variable warnings for preparatory mutex integration.The
annotationIdvariables are retrieved from state but not used because the mutex calls are commented out as part of the WIP development. To address the static analysis warnings while maintaining the preparatory code, consider prefixing the variables with underscore:-const annotationId = yield* select((state) => state.annotation.annotationId); +const _annotationId = yield* select((state) => state.annotation.annotationId);This clearly indicates the variables are intentionally unused in the current state while being prepared for future integration.
Also applies to: 445-446, 710-710, 796-797, 902-902, 957-958, 1142-1149
frontend/javascripts/viewer/model/sagas/saving/save_mutex_saga.tsx (2)
29-29: Use import type for type-only imports.The static analysis correctly identifies that
EnsureMaySaveNowActionis only used as a type.-import { EnsureMaySaveNowAction } from "viewer/model/actions/save_actions"; +import type { EnsureMaySaveNowAction } from "viewer/model/actions/save_actions";
93-106: Clarify the mutex acquisition flow for late-mutex approach.The conditional logic based on
DISABLE_EAGER_MUTEX_ACQUISITIONimplements different mutex acquisition strategies, but the flow could be clearer.Consider adding comments to explain:
- When
DISABLE_EAGER_MUTEX_ACQUISITIONis true, mutex is acquired only when saving (late acquisition)- When false, mutex is acquired eagerly when the annotation is opened
- The race condition between
tryAcquireMutexContinuouslyandDONE_SAVINGensures mutex is released after save completesThis aligns with the PR objective of implementing a "late-mutex approach" as an opt-in feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
app/controllers/AnnotationController.scala(1 hunks)app/models/annotation/AnnotationMutexService.scala(2 hunks)conf/webknossos.latest.routes(1 hunks)frontend/javascripts/admin/rest_api.ts(1 hunks)frontend/javascripts/test/sagas/annotation_saga.spec.ts(1 hunks)frontend/javascripts/viewer/api/wk_dev.ts(2 hunks)frontend/javascripts/viewer/default_state.ts(1 hunks)frontend/javascripts/viewer/model/actions/annotation_actions.ts(3 hunks)frontend/javascripts/viewer/model/actions/save_actions.ts(3 hunks)frontend/javascripts/viewer/model/reducers/annotation_reducer.ts(1 hunks)frontend/javascripts/viewer/model/reducers/reducer_helpers.ts(1 hunks)frontend/javascripts/viewer/model/sagas/annotation_saga.tsx(2 hunks)frontend/javascripts/viewer/model/sagas/saving/save_mutex_saga.tsx(1 hunks)frontend/javascripts/viewer/model/sagas/saving/save_queue_draining.ts(5 hunks)frontend/javascripts/viewer/model/sagas/saving/save_saga.ts(1 hunks)frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts(7 hunks)frontend/javascripts/viewer/store.ts(1 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala(1 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingUpdateActions.scala(2 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: conf/evolutions/126-credit-transactions.sql:89-130
Timestamp: 2025-01-27T15:01:17.868Z
Learning: Team prefers to discuss concurrency handling approaches (like transaction isolation) during the review phase rather than as individual suggestions.
Learnt from: philippotto
PR: scalableminds/webknossos#8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
frontend/javascripts/test/sagas/annotation_saga.spec.ts (1)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
frontend/javascripts/viewer/model/reducers/annotation_reducer.ts (1)
Learnt from: knollengewaechs
PR: scalableminds/webknossos#8492
File: frontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts:671-674
Timestamp: 2025-05-15T19:44:16.110Z
Learning: Visibility updates for user bounding boxes are intentionally handled separately from other property updates, with dedicated update actions for visibility changes.
frontend/javascripts/viewer/model/sagas/saving/save_saga.ts (1)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
app/models/annotation/AnnotationMutexService.scala (3)
Learnt from: frcroth
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/MeshFileService.scala:170-173
Timestamp: 2025-04-23T08:51:57.756Z
Learning: In the webknossos codebase, classes extending `FoxImplicits` have access to an implicit conversion from `Option[A]` to `Fox[A]`, where `None` is converted to an empty Fox that fails gracefully in for-comprehensions.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransactionService.scala:0-0
Timestamp: 2025-01-27T12:06:42.865Z
Learning: In Scala's for-comprehension with Fox (Future-like type), the `<-` operator ensures sequential execution. If any step fails, the entire chain short-circuits and returns early, preventing subsequent operations from executing. This makes it safe to perform validation checks before database operations.
Learnt from: frcroth
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/NeuroglancerPrecomputedMeshFileService.scala:161-166
Timestamp: 2025-04-28T14:18:04.368Z
Learning: In Scala for-comprehensions with the Fox error handling monad, `Fox.fromBool()` expressions should use the `<-` binding operator instead of the `=` assignment operator to properly propagate error conditions. Using `=` will cause validation failures to be silently ignored.
frontend/javascripts/viewer/model/sagas/saving/save_mutex_saga.tsx (1)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (1)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala (6)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/volumetracing_saga.tsx:433-434
Timestamp: 2024-11-22T17:19:07.947Z
Learning: In the codebase, certain usages of `segmentationLayer.resolutions` are intentionally retained and should not be changed to `segmentationLayer.mags` during refactoring.
Learnt from: frcroth
PR: scalableminds/webknossos#8609
File: app/models/dataset/Dataset.scala:753-775
Timestamp: 2025-05-12T13:07:29.637Z
Learning: In the `updateMags` method of DatasetMagsDAO (Scala), the code handles different dataset types distinctly:
1. Non-WKW datasets have `magsOpt` populated and use the first branch which includes axisOrder, channelIndex, and credentialId.
2. WKW datasets will have `wkwResolutionsOpt` populated and use the second branch which includes cubeLength.
3. The final branch is a fallback for legacy data.
This ensures appropriate fields are populated for each dataset type.
Learnt from: philippotto
PR: scalableminds/webknossos#8602
File: frontend/javascripts/oxalis/model/volumetracing/volume_annotation_sampling.ts:365-366
Timestamp: 2025-05-07T06:17:32.810Z
Learning: The parameter in applyVoxelMap was renamed from `sliceCount` to `sliceOffset` to better reflect its purpose, but this doesn't affect existing call sites since JavaScript/TypeScript function calls are position-based.
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts:125-125
Timestamp: 2024-11-22T17:17:39.914Z
Learning: In the `createNode` function within `skeletontracing_reducer_helpers.ts`, the property `resolution` is intentionally kept even when the parameter is renamed to `mag`.
Learnt from: frcroth
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/MeshFileService.scala:170-173
Timestamp: 2025-04-23T08:51:57.756Z
Learning: In the webknossos codebase, classes extending `FoxImplicits` have access to an implicit conversion from `Option[A]` to `Fox[A]`, where `None` is converted to an empty Fox that fails gracefully in for-comprehensions.
frontend/javascripts/viewer/model/sagas/saving/save_queue_draining.ts (1)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
frontend/javascripts/viewer/model/sagas/annotation_saga.tsx (1)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingUpdateActions.scala (1)
Learnt from: frcroth
PR: scalableminds/webknossos#8609
File: app/models/dataset/Dataset.scala:753-775
Timestamp: 2025-05-12T13:07:29.637Z
Learning: In the `updateMags` method of DatasetMagsDAO (Scala), the code handles different dataset types distinctly:
1. Non-WKW datasets have `magsOpt` populated and use the first branch which includes axisOrder, channelIndex, and credentialId.
2. WKW datasets will have `wkwResolutionsOpt` populated and use the second branch which includes cubeLength.
3. The final branch is a fallback for legacy data.
This ensures appropriate fields are populated for each dataset type.
🧬 Code Graph Analysis (2)
app/models/annotation/AnnotationMutexService.scala (2)
util/src/main/scala/com/scalableminds/util/tools/Fox.scala (2)
shiftBox(296-296)successful(53-56)app/utils/sql/SqlInterpolation.scala (2)
q(19-38)asUpdate(73-73)
frontend/javascripts/viewer/model/sagas/saving/save_queue_draining.ts (2)
frontend/javascripts/viewer/model/actions/save_actions.ts (2)
dispatchEnsureMaySaveNowAsync(146-151)doneSavingAction(153-156)frontend/javascripts/libs/window.ts (2)
alert(18-18)location(60-60)
🪛 Biome (1.9.4)
frontend/javascripts/viewer/model/sagas/saving/save_saga.ts
[error] 55-55: Unexpected constant condition.
(lint/correctness/noConstantCondition)
frontend/javascripts/viewer/model/sagas/saving/save_mutex_saga.tsx
[error] 29-29: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts
[error] 361-361: This variable is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
Unsafe fix: If this is intentional, prepend annotationId with an underscore.
(lint/correctness/noUnusedVariables)
[error] 710-710: This variable is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
Unsafe fix: If this is intentional, prepend annotationId with an underscore.
(lint/correctness/noUnusedVariables)
[error] 902-902: This variable is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
Unsafe fix: If this is intentional, prepend annotationId with an underscore.
(lint/correctness/noUnusedVariables)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (19)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingUpdateActions.scala (2)
14-14: Well-designed backward compatibility approach.The conversion of
agglomerateIdandmagfields toOptiontypes is a clean approach for supporting legacy update actions while maintaining type safety. The comment provides clear context for why these fields are now optional.Also applies to: 19-19
39-40: Consistent optional field implementation.The same backward compatibility pattern is correctly applied to
MergeAgglomerateUpdateActionwith bothagglomerateId1andagglomerateId2fields becoming optional, maintaining consistency with theSplitAgglomerateUpdateActionchanges.Also applies to: 45-45
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala (2)
188-188: Proper method signature alignment with update actions.The parameter change from
mag: Vec3InttomagOpt: Option[Vec3Int]correctly aligns with the optional fields in the update actions, maintaining consistency across the codebase.Also applies to: 191-191
196-196: Effective validation for optional parameters.The validation logic properly handles the optional
magOptparameter by converting it to a Fox with a descriptive error message when required. This ensures that the method fails gracefully when mandatory data is missing while supporting the backward compatibility requirements.Also applies to: 199-199
frontend/javascripts/test/sagas/annotation_saga.spec.ts (1)
15-15: LGTM: Import path correctly updated for refactored saga.The import path update aligns with the refactoring where
acquireAnnotationMutexMaybewas moved to a dedicated mutex saga module, improving code organization.frontend/javascripts/viewer/store.ts (1)
149-149: LGTM: Mutex state flag properly added to Annotation type.The
isMutexAcquiredboolean property is correctly defined as readonly, maintaining Redux immutability principles and providing clear state tracking for mutex management.frontend/javascripts/viewer/default_state.ts (1)
181-181: LGTM: Appropriate default value for mutex state.Initializing
isMutexAcquiredtofalseis the correct default since annotations start without mutex acquisition, maintaining consistency with the expected initial state.conf/webknossos.latest.routes (1)
158-158: LGTM: Well-structured REST endpoint for mutex release.The DELETE route for
/annotations/:id/mutexcorrectly complements the existing POST route for mutex acquisition, following REST conventions and maintaining consistent route structure.frontend/javascripts/viewer/model/reducers/reducer_helpers.ts (1)
159-159: LGTM: Consistent mutex flag initialization in conversion function.Adding
isMutexAcquired: falseensures that server-to-frontend annotation conversions maintain consistent object structure with the default state initialization.frontend/javascripts/viewer/model/reducers/annotation_reducer.ts (1)
140-145: LGTM! Clean implementation of the mutex state reducer.The new action case follows the established patterns in the file and correctly uses the
updateKeyhelper function to maintain immutability. The implementation is consistent with other reducer cases in the file.frontend/javascripts/admin/rest_api.ts (1)
732-736: LGTM! Clean implementation of the mutex release API.The function follows the established patterns in the file and correctly uses the HTTP DELETE method for releasing the mutex. The implementation is consistent with the existing
acquireAnnotationMutexfunction and integrates well with the broader mutex management feature.frontend/javascripts/viewer/api/wk_dev.ts (1)
14-14: Good debugging setup for development.Enabling these debug flags will help with development and troubleshooting of the new mutex functionality. The enhanced logging and version display should provide valuable insights during the WIP development phase.
Also applies to: 31-31
app/controllers/AnnotationController.scala (1)
457-463: Clean implementation of the mutex release endpoint.The controller method follows established patterns in the codebase, uses appropriate error handling with Fox, and includes performance monitoring via
logTime. The implementation is straightforward and correctly delegates to the service layer.frontend/javascripts/viewer/model/actions/annotation_actions.ts (1)
55-55: Well-implemented action creator for mutex state management.The new
setIsMutexAcquiredActionfollows established patterns in the codebase, is properly typed, and correctly integrated into the action type union. The implementation is clean and straightforward.Also applies to: 91-91, 181-185
frontend/javascripts/viewer/model/sagas/annotation_saga.tsx (1)
2-2: Clean refactoring of mutex logic!The relocation of mutex acquisition logic to a dedicated module improves code organization and separation of concerns.
Also applies to: 9-9, 14-17, 34-34
frontend/javascripts/viewer/model/actions/save_actions.ts (1)
29-30: Well-structured save flow control actions!The new actions and async dispatch helper follow the established patterns in the codebase. The Deferred pattern for callback synchronization is consistent with existing implementations.
Also applies to: 43-45, 140-156
frontend/javascripts/viewer/model/sagas/saving/save_queue_draining.ts (2)
68-70: Good integration of save flow control!The addition of
dispatchEnsureMaySaveNowAsyncanddoneSavingActionproperly coordinates save operations with mutex state.Also applies to: 102-103
206-210: Verify the simplified 409 conflict handling behavior.The removal of the flag and indefinite sleep simplifies the code, but may change behavior:
- Previously, only one alert was shown due to the flag
- The indefinite sleep prevented further save attempts
- Now the error is thrown immediately after reload
Since multiple retries could trigger multiple alerts before the page reloads, consider if this behavior change is intentional. The immediate throw after reload seems correct as the page will be refreshed anyway.
frontend/javascripts/viewer/model/sagas/saving/save_mutex_saga.tsx (1)
104-104: Add missing yield operator.*The
calleffect requiresyield*to properly execute the saga.- if (doneSaving) { - yield call(releaseMutex); - } + if (doneSaving) { + yield* call(releaseMutex); + }Likely an incorrect or invalid review comment.
frontend/javascripts/viewer/model/sagas/saving/save_mutex_saga.tsx
Outdated
Show resolved
Hide resolved
…nymore; also invalidate complete mapping when incorporating merge and split for now
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
frontend/javascripts/viewer/model/sagas/saving/save_mutex_saga.tsx (3)
39-40: Fix TODO typo and make the flag configurable.
177-188: Consider using a finally block as mentioned in the comment.
225-235: Fix the releaseMutex implementation to actually release the mutex.frontend/javascripts/viewer/model/sagas/saving/save_saga.ts (1)
59-60: Fix the disabled early return condition and unclear comment.
🧹 Nitpick comments (1)
frontend/javascripts/viewer/model/sagas/volume/mapping_saga.ts (1)
158-205: Well-structured debugging feature with clear documentation.The addition of
DEBUG__RELOAD_HDF5_MAPPINGaction handler and supporting functions provides a useful debugging capability. The code is well-documented with clear comments explaining the debugging purpose.One minor issue: The error message on line 200 mentions "splitAgglomerate" but should be more generic since this is a general HDF5 reloading function:
- throw new Error("Could not apply splitAgglomerate because no active mapping was found."); + throw new Error("Could not reload HDF5 mapping because no active mapping was found.");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/javascripts/viewer/api/wk_dev.ts(1 hunks)frontend/javascripts/viewer/model/actions/save_actions.ts(3 hunks)frontend/javascripts/viewer/model/sagas/saving/save_mutex_saga.tsx(1 hunks)frontend/javascripts/viewer/model/sagas/saving/save_queue_draining.ts(5 hunks)frontend/javascripts/viewer/model/sagas/saving/save_saga.ts(7 hunks)frontend/javascripts/viewer/model/sagas/volume/mapping_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts(6 hunks)frontend/javascripts/viewer/model/sagas/volume/update_actions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/javascripts/viewer/api/wk_dev.ts
- frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts
- frontend/javascripts/viewer/model/sagas/saving/save_queue_draining.ts
- frontend/javascripts/viewer/model/actions/save_actions.ts
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: conf/evolutions/126-credit-transactions.sql:89-130
Timestamp: 2025-01-27T15:01:17.868Z
Learning: Team prefers to discuss concurrency handling approaches (like transaction isolation) during the review phase rather than as individual suggestions.
Learnt from: philippotto
PR: scalableminds/webknossos#8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
frontend/javascripts/viewer/model/sagas/saving/save_mutex_saga.tsx (2)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:29-35
Timestamp: 2025-07-09T07:30:57.149Z
Learning: In the webknossos project, error handling and user notifications are handled at the API function level (e.g., `removeWebAuthnKey`, `fetchPasskeys`) rather than in the UI components, so additional try-catch blocks in the UI layer are unnecessary.
frontend/javascripts/viewer/model/sagas/volume/update_actions.ts (4)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Learnt from: philippotto
PR: scalableminds/webknossos#8602
File: frontend/javascripts/oxalis/model/volumetracing/volume_annotation_sampling.ts:365-366
Timestamp: 2025-05-07T06:17:32.810Z
Learning: The parameter in applyVoxelMap was renamed from `sliceCount` to `sliceOffset` to better reflect its purpose, but this doesn't affect existing call sites since JavaScript/TypeScript function calls are position-based.
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts:125-125
Timestamp: 2024-11-22T17:17:39.914Z
Learning: In the `createNode` function within `skeletontracing_reducer_helpers.ts`, the property `resolution` is intentionally kept even when the parameter is renamed to `mag`.
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts:568-585
Timestamp: 2024-11-22T17:18:43.411Z
Learning: In the file `frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts`, the uniform name `allResolutions` should remain unchanged to maintain consistency with the shader code.
frontend/javascripts/viewer/model/sagas/volume/mapping_saga.ts (2)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Learnt from: philippotto
PR: scalableminds/webknossos#8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
frontend/javascripts/viewer/model/sagas/saving/save_saga.ts (2)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Learnt from: philippotto
PR: scalableminds/webknossos#8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
🧬 Code Graph Analysis (3)
frontend/javascripts/viewer/model/sagas/saving/save_mutex_saga.tsx (1)
frontend/javascripts/libs/window.ts (1)
location(60-60)
frontend/javascripts/viewer/model/sagas/volume/update_actions.ts (2)
frontend/javascripts/viewer/constants.ts (1)
Vector3(13-13)frontend/javascripts/viewer/store.ts (1)
NumberLike(362-362)
frontend/javascripts/viewer/model/sagas/volume/mapping_saga.ts (3)
frontend/javascripts/viewer/model.ts (2)
getSegmentationLayers(93-98)getLayerByName(270-276)frontend/javascripts/viewer/store.ts (1)
ActiveMappingInfo(366-374)frontend/javascripts/viewer/model/actions/settings_actions.ts (1)
setMappingAction(205-227)
🪛 Biome (1.9.4)
frontend/javascripts/viewer/model/sagas/saving/save_saga.ts
[error] 60-60: Unexpected constant condition.
(lint/correctness/noConstantCondition)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (2)
frontend/javascripts/viewer/model/sagas/volume/update_actions.ts (1)
897-955: Good approach to API evolution with backward compatibility!The refactoring to make
agglomerateId,mag,agglomerateId1, andagglomerateId2optional fields clearly maintains backward compatibility while simplifying the current API. The comments explaining that these fields are "unused in back-end but may exist in older update actions" provide excellent documentation for future maintainers.frontend/javascripts/viewer/model/sagas/volume/mapping_saga.ts (1)
166-174: Clean implementation of clearActiveMapping.The function properly creates a new empty Map and dispatches the appropriate action to clear the mapping state. This will be useful for the deferred mapping updates in save_saga.ts.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
…ecific update action properties from some applied update actions
…ctions in preocgress as the are already blocking - Fix using updated agglomerate ids in proofreading saga after saving and thus syncing with the backend - Add more mesh interaction tests - merge and split now also have a test variation where an update is injected to test intervening users when doing mesh proofreading
…freading saga functions
There was a problem hiding this comment.
whew, I looked at everything except for the tests. kudos for pushing this PR!
it's a lot of code and parts of it are quite complex. overall, I think we should make this very tidy and at least add more docstrings to explain the gritty details. otherwise, we will be quite lost when some time has passed and we look at the code again.
| | DoneSavingAction | ||
| | SetIsMutexAcquiredAction | ||
| | SetUserHoldingMutexAction | ||
| | PrepareRebasingAction |
There was a problem hiding this comment.
is it on purpose that prepare-rebase is listed here and finished-rebase is not? if so, please explain in a code comment.
There was a problem hiding this comment.
Well good catch. It is actually not needed here as only the save_queue_filling.ts saga listens to this action. Thus, it is not needed in the type used for the save_reducer.ts. But could also be added here for completion sake. I'll also add a comment
| switch (action.type) { | ||
| case "INITIALIZE_ANNOTATION": { | ||
| return updateAnnotation(state, { | ||
| // Initialize state needed for rebasing newest server updates. |
There was a problem hiding this comment.
I feel like this comment could go a bit more into detail to explain what this means.
| const adaptToType = | ||
| upToDateMapping && isNumberMap(upToDateMapping) | ||
| ? (el: number) => el | ||
| : (el: number) => BigInt(el); |
There was a problem hiding this comment.
I've seen this multiple times now. I think we should extract it into a getAdaptToTypeFn that returns the actual helper function.
There was a problem hiding this comment.
I extracted the adaptToType into a helper function. Should I also extract
let upToDateAgglomerateId1 = (mappingSyncedWithBackend as NumberLikeMap).get(
adaptToType(segmentId1),
);
let upToDateAgglomerateId2 = (mappingSyncedWithBackend as NumberLikeMap).get(
adaptToType(segmentId2),
);
into another helper? This pattern is quite often used in this file and `proofreading_saga.ts? Maybe a mapping accessor 👀 #9001?
- Add support for reapplying user specific volume update actions - and misc
| | DoneSavingAction | ||
| | SetIsMutexAcquiredAction | ||
| | SetUserHoldingMutexAction | ||
| | PrepareRebasingAction |
There was a problem hiding this comment.
Well good catch. It is actually not needed here as only the save_queue_filling.ts saga listens to this action. Thus, it is not needed in the type used for the save_reducer.ts. But could also be added here for completion sake. I'll also add a comment
| type: "FINISHED_REBASING", | ||
| }) as const; | ||
|
|
||
| export const updateMappingRebaseInformationAction = (volumeLayerIdToUpdate: string) => |
There was a problem hiding this comment.
good point 👍. I went for snapshotMappingDataForNextRebaseAction to emphasize that only the mapping data is snapshotted and the whole rebase state.
| if (state.save.queue.length !== action.newSaveQueue.length) { | ||
| return state; | ||
| } |
There was a problem hiding this comment.
actually this should be a hard error, as this would mean an update was missed to update in the save queue by the rebasing mechanism. This should never occur if rebasing is done properly.
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts
Outdated
Show resolved
Hide resolved
| targetAgglomerateId, | ||
| ); | ||
| if (mergedMapping === activeMapping.mapping) { | ||
| /* TODOM: in case setMappingAction is called with the same mapping |
There was a problem hiding this comment.
Yeah lets talk about this ☎️. cannot really follow what the original issue / problem is and you seemed to already understand this :)
| const controlMode = useWkSelector((state) => state.temporaryConfiguration.controlMode); | ||
| const isViewMode = controlMode === ControlModeEnum.VIEW; | ||
| const isReadOnly = useWkSelector((state) => !state.annotation.restrictions.allowUpdate); | ||
| // TODOM: Not sure whether this change is entirely correct, but this should be the same behaviour as before. |
There was a problem hiding this comment.
Yeah open TODO: I need to test in some scenarios where it logic here is still correct. Previously
const isReadOnly = useWkSelector((state) => !state.annotation.restrictions.allowUpdate);
was used. In the past this was changed if the user was not allowed to edit. Now this is handled by state.annotation.isUpdatingCurrentlyAllowed. And this todo should remind me to test this change properly :)
SO I'll keep this open
| if (action.isUnsyncedWithServer) { | ||
| return state; | ||
| } |
There was a problem hiding this comment.
Was a hacky / bad solution to avoid updating the mapping data snapshot used for rebasings. isUnsyncedWithServer was true in case of a optimistic merge.
Now this is refactored in a better way :D
The SET_MAPPING action is no longer used to snapshot the mapping for rebasing. Instead, the SNAPSHOT_MAPPING_DATA_FOR_NEXT_REBASE_ACTION action should be triggered after each mapping update that does not include user-specific modifications caused by optimistic updates of proofreading actions (which is merge only I guess).
This makes it more explicit and hopefully less confusing, as no isUnsyncedWithServer is needed. Instead, the snapshotMappingDataForNextRebaseAction action can be deliberately left away when updating a mapping, and this change is not yet in sync with the backend.
The only downside I can see in this approach is that one needs to remember to also trigger snapshotMappingDataForNextRebaseAction after updating the mapping in case the changes is in sync with whats on the server :)
| while (hasMutex) { | ||
| const { canEdit } = yield* call(acquireAnnotationMutex, annotationId); | ||
| if (!canEdit) { | ||
| // TODOM: Think of a better way to handle this. This should usually never happen only if a client disconnects for a longer time while saving. |
There was a problem hiding this comment.
Yeah you are right, this needs better save guarding than the current version 👍. Thanks for pointing this out 🙏
WK "freezes"
Do you mean I should set it to a busy state? During saving the ui is only busy when pushing proofreading update actions. Else not. We could use this here to ensure that the user does not interfere in such an error case
some time passes and eventually the mutex is acquired and everything is fine
For this the code pushing the actual updates needs also guarding as while being stuck in sending updates in multiple requests or while rebasing the connection to the server could be lost. Thus, the update requests could fail and the user no longer has the mutex. so maybe guarding the sendSaveRequestToServer calls with a check whether the current user has the mutex would be a good thing 🤔?
| // Therefore, we also print the error in the test context. | ||
| console.error("Error while trying to acquire mutex:", error); | ||
| } | ||
| // todop: I think this needs to happen in a finally block? |
There was a problem hiding this comment.
This is a comment left by you. I don't know why this would be moved to the finally block. Because if everything wents well and the user acquires the mutex, the finally block would still be executed meaning
const wasCanceled = yield* cancelled();
console.log("wasCanceled", wasCanceled);
if (!wasCanceled) {
console.error("Error while trying to acquire mutex.", error);
yield* put(setUserHoldingMutexAction(undefined));
yield* put(setIsUpdatingAnnotationCurrentlyAllowedAction(false));
if (yield* call(getDoesHaveMutex)) {
yield* put(setIsMutexAcquiredAction(false));
}
}
would be executed and the store state would always in every loop iteration be reset to "I don't have the mutex". And this should be wrong imo.
3 participants
This PR enables experimental (opt-in) collaborative proofreading (ignoring segment lists and meshes). Multiple users can split and merge agglomerates "in parallel". Synchronization happens as follows:
¹ the incorporation of the update actions is not trivial, because the frontend maintains a partial mapping from segment ids to agglomerate ids. the update actions only encode which segment ids were split or merged. simply mapping the affected segment ids to their (old or new?) agglomerate ids and refreshing all segment ids that map to these agglomerate ids might not be enough, because the mapping is partial and might have changed in between.
therefore, a first iteration simply reloads all segment ids. when the rest of the pr works, we can re-think this.
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
buffers.sliding<EnsureHasNewestVersionAction>(1))state.save.needsMutexboolean which the sagas act on)WkDevFlags.liveCollab: The others may edit feature works just as expected.othersMayEdit = truetest that after the test the user does not still have the mutex.annotation_saga.spec.ts. Tests seem to not have enoughno such vertex in graphtoast if a user tries to split an agglomerate that was already split. see [slack]ensureNewestVersionwould never terminate and other users will wait forever when trying to save. if this code version is never used in prod, it might not matter, though.. -> is fine for now, as this will only be available via the feature flag.
[ ] make the new late-mutex approach opt-in (beta flag in sharing modal?)[ ] discuss:IMO this only makes sense starting with milestone 4[ ] disable all other tools if activated[ ] disable segment list editing?WkDevFlags.liveCollabupdateMappingWithMergeinitialAllowUpdateKnown Bugs:
othersMayEditis turned on due to rebase mechanism not supporting updateMappingName action.Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)