Live Collab M2 - Automatically update annotation to newest changes #8648
Conversation
📝 WalkthroughWalkthroughThis change introduces live update functionality for annotation views, allowing read-only users to see real-time changes made by others without reloading the page. It accomplishes this by implementing new update action application logic, batch update reducers, and sagas for synchronizing annotation and tracing state. The update also includes extensive refactoring, type improvements, and test enhancements. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
philippotto
force-pushed
the
live-m2
branch
2 times, most recently
from
0ff6084 to
1badc1d
Compare
philippotto
force-pushed
the
live-m2
branch
2 times, most recently
from
1240cda to
4699f30
Compare
…; integrate bbox code for volume tracing, too
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
frontend/javascripts/viewer/model/reducers/update_action_application/skeleton.ts (2)
305-305: Critical bug in active node handling logic.The logic incorrectly sets
newActiveNodeIdto the deleted node's ID when the deleted node is not the active one.Apply this fix:
- const newActiveNodeId = skeleton.activeNodeId === nodeId ? null : nodeId; + const newActiveNodeId = skeleton.activeNodeId === nodeId ? null : skeleton.activeNodeId;
352-355: Remove unreachable code causing TypeScript errors.The code after the switch statement is unreachable and causes TypeScript compilation issues.
Remove the unreachable code:
} - ua satisfies never; - - // Satisfy TS. - throw new Error("Reached unexpected part of function.");frontend/javascripts/viewer/model/sagas/save_saga.ts (1)
521-524: Restore production polling intervals before merge.The polling intervals are still set to aggressive 1-second values for debugging, which will cause excessive server load in production.
-// todop: restore to 10, 60, 30 ? -const VERSION_POLL_INTERVAL_COLLAB = 1 * 1000; -const VERSION_POLL_INTERVAL_READ_ONLY = 1 * 1000; -const VERSION_POLL_INTERVAL_SINGLE_EDITOR = 1 * 1000; +const VERSION_POLL_INTERVAL_COLLAB = 10 * 1000; +const VERSION_POLL_INTERVAL_READ_ONLY = 60 * 1000; +const VERSION_POLL_INTERVAL_SINGLE_EDITOR = 30 * 1000;
🧹 Nitpick comments (2)
frontend/javascripts/viewer/model/reducers/update_action_application/skeleton.ts (1)
39-40: Document unused legacy properties.Good practice to explicitly document why
updatedIdandactionTracingIdare extracted but not used.The comment on line 39 effectively explains this for
updatedId, but consider making it more prominent:- // updatedId is part of the updateAction format but was never really used. + // updatedId and actionTracingId are part of the updateAction format but are unused here + // (kept for backwards compatibility with server update action format)frontend/javascripts/viewer/model/sagas/save_saga.ts (1)
871-871: Consider structured logging for unsupported actions.The console.log for unsupported actions could be enhanced for better debugging.
- console.log("Cannot apply action", action.name); + console.warn("Cannot apply update action during incremental sync:", action.name);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_1.spec.ts(1 hunks)frontend/javascripts/viewer/api/api_latest.ts(7 hunks)frontend/javascripts/viewer/model/bucket_data_handling/data_cube.ts(6 hunks)frontend/javascripts/viewer/model/reducers/update_action_application/skeleton.ts(1 hunks)frontend/javascripts/viewer/model/sagas/proofread_saga.ts(11 hunks)frontend/javascripts/viewer/model/sagas/save_saga.ts(11 hunks)frontend/javascripts/viewer/view/version_list.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/javascripts/viewer/model/bucket_data_handling/data_cube.ts
- frontend/javascripts/viewer/model/sagas/proofread_saga.ts
- frontend/javascripts/viewer/api/api_latest.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (5)
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_1.spec.ts (1)
92-92: LGTM! Method rename improves semantic clarity.The rename from
collectAllBuckets()toremoveAllBuckets()better describes the actual operation being performed and aligns with the broader refactoring effort across the codebase.frontend/javascripts/viewer/view/version_list.tsx (1)
83-83: LGTM! Consistent method rename improves semantic clarity.The rename from
collectAllBuckets()toremoveAllBuckets()is consistent with the broader refactoring effort and better describes the bucket clearing operation performed during version preview.frontend/javascripts/viewer/model/reducers/update_action_application/skeleton.ts (1)
166-230: Well-implemented tree component movement logic.The
moveTreeComponentcase correctly handles the complex operation of moving nodes and edges between trees, including proper separation of moved vs remaining elements and safe cloning of data structures.frontend/javascripts/viewer/model/sagas/save_saga.ts (2)
686-884: Well-architected incremental update mechanism.The
tryToIncorporateActionsfunction is well-structured with:
- Clear separation of user-specific vs shared state updates
- Proper delegation to appropriate reducers and sagas
- Comprehensive action type coverage
- Effective finalization pattern for bucket/mapping updates
The error handling gracefully falls back to user notification when incorporation fails.
596-617: Robust error handling for update action incorporation.The try-catch around
tryToIncorporateActionsproperly handles failures and falls back to user notification, preventing crashes while maintaining user experience.
|
@MichaelBuessemeyer thank you for your review! I think I addressed most of it (I already marked several comments as resolved but please have a look at the remaining ones if they can also be resolved in your opinion). |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
frontend/javascripts/viewer/model/sagas/save_saga.ts (2)
521-524: Temporary debugging intervals already addressed in past reviews.The aggressive 1-second polling intervals with TODO comment have been previously flagged and acknowledged by the team. This is intentional for testing the live collaboration functionality.
680-681: Remove debug toast and TODO comment.The debug error toast and TODO comment should be addressed before production.
🧹 Nitpick comments (1)
frontend/javascripts/viewer/model/sagas/save_saga.ts (1)
686-884: Consider refactoring the largetryToIncorporateActionsfunction for better maintainability.The
tryToIncorporateActionsfunction is quite large (~200 lines) and handles multiple responsibilities through a complex switch statement. This makes it harder to maintain, test, and reason about.Consider refactoring by:
- Extracting action type handlers into separate functions (e.g.,
handleSkeletonActions,handleVolumeActions,handleProofreadingActions)- Using a mapping object to route actions to their handlers
- Moving the finalization logic to a separate concern
Example refactor structure:
+const actionHandlers = { + skeleton: handleSkeletonActions, + volume: handleVolumeActions, + proofreading: handleProofreadingActions, + ignored: handleIgnoredActions, +}; + +function* handleSkeletonActions(action, finalizers) { + // Handle skeleton-specific actions +} + export function* tryToIncorporateActions(newerActions) { // ... setup code ... for (const actionBatch of newerActions) { for (const action of actionBatch.value) { - switch (action.name) { - // ... 100+ lines of cases ... - } + const handler = getActionHandler(action.name); + yield* handler(action, finalizers); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/javascripts/viewer/model/sagas/save_saga.ts(11 hunks)frontend/javascripts/viewer/model_initialization.ts(8 hunks)frontend/javascripts/viewer/store.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/viewer/store.ts
- frontend/javascripts/viewer/model_initialization.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
🔇 Additional comments (1)
frontend/javascripts/viewer/model/sagas/save_saga.ts (1)
508-510: LGTM - Appropriate parameter addition for compaction.The addition of
prevTracingparameter tocompactUpdateActionsis consistent with the function signature changes and enables better compaction logic by comparing previous and current states.
There was a problem hiding this comment.
Ok, here is my testing result. Sorry, I found some bugs and for some of them I cannot clearly say how to reproduce them.
- Sadly, I managed to get into a error case in an annotation with already a skeleton and drawn segments visible. The error occured when I selected a new cell, drew in the YZ viewport a bit larger area and then hit save. After that the private tab getting the updates changed to: "You need to reload". Not sure how easy it is to reproduce this. The error was:
- I also got into this state when starting to do some node moving, activation and such. Afterwards, I did some flood fills which the server action applying does not seem to handle correctly currently.
- Applying skeleton group update apparently also has bugs:
This is the original state
This the state of the annotation which was long opened: One can see that the groups were not created and trees were lost / are no longer rendered.
And here the state of the annotation which was loaded after Groups "Gourp 1" "Blue" and "Schmu" already existed. This shows that the groups wich existed before the annotation was opened are ok, but once a new group is created, this is not synced.
- Proofreading looked good. I did not manage to break anything there
- Volume interactions with an agglomerate mapping active also seems to work
frontend/javascripts/viewer/model/actions/skeletontracing_actions_with_effects.tsx
Show resolved
Hide resolved
frontend/javascripts/test/reducers/update_action_application/skeleton.spec.ts
Show resolved
Hide resolved
frontend/javascripts/test/reducers/update_action_application/skeleton.spec.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/model/reducers/update_action_application/skeleton.ts
Show resolved
Hide resolved
frontend/javascripts/viewer/model/reducers/update_action_application/skeleton.ts
Show resolved
Hide resolved
…eActionLog; remove the former and rely on the latter
Oops, there was a race condition when the frontend requested the newestVersion (number) from the backend and afterwards it fetched all newer versions (now, there might be more versions than before). I fixed this in my latest commit.
I'll have a look. |
|
I fixed the tree group bug, too. It was a "permission problem" because the annotation was opened in read-only mode and some check prevented the update actions from being applied. I adapted the tests to reflect the read-only aspect and fixed them. Please have a look at my latest commits 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/javascripts/test/helpers/utils.ts (1)
19-29: Consider adding JSDoc documentation.The helper function could benefit from JSDoc comments explaining its purpose and parameters, especially since it manipulates nested state properties.
+/** + * Updates the allowUpdate flag in the annotation restrictions of a WebknossosState. + * @param state The state to update + * @param value The new value for the allowUpdate flag + * @returns A new state with the updated allowUpdate flag + */ function overrideAllowUpdateInState(state: WebknossosState, value: boolean) {frontend/javascripts/test/reducers/update_action_application/skeleton.spec.ts (2)
157-218: Well-structured parameterized test but consider debugging complexity.The nested parameterized test structure is comprehensive but may be challenging to debug if failures occur. The logic correctly tests update action application with and without compaction across different state version ranges.
Consider adding more descriptive test names or helper functions to make debugging easier if this test fails:
- test.each(afterVersionIndices)("To v=%i", (afterVersionIndex: number) => { + test.each(afterVersionIndices)("To v=%i (%i actions)", (afterVersionIndex: number) => { + const actionCount = afterVersionIndex - beforeVersionIndex + 1;
294-306: Remove unused debug function.This debug logging function appears to be leftover development code and should be removed to keep the test file clean.
-function _debugLogTrees(prefix: string, state: WebknossosState) { - const size = state.annotation.skeleton!.trees.getOrThrow(1).nodes.size(); - console.log("logTrees. size", size); - for (const tree of state.annotation.skeleton!.trees.values()) { - console.log( - `${prefix}. tree.id=${tree.treeId}.`, - "edges: ", - Array.from(tree.edges.values().map((edge) => `${edge.source}-${edge.target}`)).join(", "), - "nodes: ", - Array.from(tree.nodes.values().map((n) => n.id)).join(", "), - ); - } -}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/javascripts/test/helpers/utils.ts(1 hunks)frontend/javascripts/test/reducers/update_action_application/skeleton.spec.ts(1 hunks)frontend/javascripts/test/reducers/update_action_application/volume.spec.ts(1 hunks)frontend/javascripts/viewer/model/reducers/skeletontracing_reducer.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/test/reducers/update_action_application/volume.spec.ts
- frontend/javascripts/viewer/model/reducers/skeletontracing_reducer.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/javascripts/test/helpers/utils.ts (1)
frontend/javascripts/viewer/store.ts (1)
WebknossosState(568-588)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
🔇 Additional comments (6)
frontend/javascripts/test/helpers/utils.ts (1)
4-17: LGTM! Well-designed utility function for testing.The implementation correctly handles the read-only state transformation pattern and uses appropriate immutable update patterns with the
immutability-helperlibrary.frontend/javascripts/test/reducers/update_action_application/skeleton.spec.ts (5)
32-51: LGTM! Proper test state initialization.The initial state setup correctly configures the annotation restrictions and adds necessary data layers for testing.
60-83: Excellent type-safe test coverage mechanism!This approach using a TypeScript record ensures that all
ApplicableSkeletonUpdateActiontypes are covered in tests. If new action types are added, TypeScript will enforce adding them to this dictionary.
98-140: Comprehensive test action sequence covers complex scenarios.The sequence effectively tests various skeleton operations including node/tree creation, deletion, merging, renaming, and user bounding box operations. This provides thorough coverage of the update action application logic.
206-212: Good use of the transformStateAsReadOnly utility.The usage correctly simulates read-only state during update action application, which aligns with the live collaboration feature requirements.
288-291: Excellent test coverage validation.The afterAll check ensures all action types have been exercised during testing, providing confidence in the test suite's completeness.
This PR is stacked on live-m2. This PR merely moves around saga files and splits the save saga into two modules (save queue filling and draining). ### URL of deployed dev instance (used for testing): - https://___.webknossos.xyz ### Steps to test: - CI should be enough --------- Co-authored-by: Michael Büßemeyer <39529669+MichaelBuessemeyer@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
frontend/javascripts/viewer/model/helpers/compaction/compact_update_actions.ts (1)
133-144: Correctly implemented node change detection.The addition of
updateNodeactions for changed nodes ensures that property changes are captured when nodes are moved between trees. The reference equality check is appropriate given Redux's immutability principles.frontend/javascripts/viewer/model/actions/skeletontracing_actions.tsx (1)
573-579: Server-driven update action correctly implemented.The new action enables bulk application of skeleton update actions from the server, supporting the live collaboration feature.
🧹 Nitpick comments (2)
frontend/javascripts/viewer/model/sagas/saving/save_saga_constants.ts (1)
3-4: Fix typo and consider reverting temporary changeTwo issues to address:
- Typo: "todop" should be "todo"
- The throttle time was reduced from 30s to 5s - ensure this temporary change is reverted before production deployment
-// todop: restore to 30s +// TODO: restore to 30sPlease confirm whether the reduced throttle time (5s instead of 30s) should remain for this deployment or be reverted to the original 30s value.
frontend/javascripts/viewer/model/sagas/volume/mapping_saga.ts (1)
444-519: Consider adding JSDoc to clarify HDF5-specific nature.The removal of the
mappingTypeparameter and hardcoding to "HDF5" is appropriate since this function is specifically for HDF5 mappings. Consider adding JSDoc documentation to make this explicit:+/** + * Updates the local HDF5 mapping by fetching agglomerate information for new segments. + * This function is specifically for HDF5 mappings and should not be used for JSON mappings. + */ export function* updateLocalHdf5Mapping(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (53)
frontend/javascripts/test/backend-snapshot-tests/annotations.e2e.ts(1 hunks)frontend/javascripts/test/helpers/saveHelpers.ts(2 hunks)frontend/javascripts/test/reducers/save_reducer.spec.ts(1 hunks)frontend/javascripts/test/reducers/update_action_application/skeleton.spec.ts(1 hunks)frontend/javascripts/test/reducers/update_action_application/volume.spec.ts(1 hunks)frontend/javascripts/test/sagas/bounding_box_saving.spec.ts(1 hunks)frontend/javascripts/test/sagas/compact_toggle_actions.spec.ts(3 hunks)frontend/javascripts/test/sagas/proofreading.spec.ts(1 hunks)frontend/javascripts/test/sagas/saga_integration.spec.ts(2 hunks)frontend/javascripts/test/sagas/save_saga.spec.ts(1 hunks)frontend/javascripts/test/sagas/skeletontracing_saga.spec.ts(16 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_remote_bucket_updates.spec.ts(1 hunks)frontend/javascripts/types/api_types.ts(8 hunks)frontend/javascripts/viewer/geometries/skeleton.ts(1 hunks)frontend/javascripts/viewer/merger_mode.ts(1 hunks)frontend/javascripts/viewer/model/actions/save_actions.ts(1 hunks)frontend/javascripts/viewer/model/actions/skeletontracing_actions.tsx(7 hunks)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts(4 hunks)frontend/javascripts/viewer/model/bucket_data_handling/pushqueue.ts(1 hunks)frontend/javascripts/viewer/model/bucket_data_handling/wkstore_adapter.ts(1 hunks)frontend/javascripts/viewer/model/helpers/compaction/compact_save_queue.ts(1 hunks)frontend/javascripts/viewer/model/helpers/compaction/compact_toggle_actions.ts(1 hunks)frontend/javascripts/viewer/model/helpers/compaction/compact_update_actions.ts(5 hunks)frontend/javascripts/viewer/model/helpers/diff_helpers.ts(1 hunks)frontend/javascripts/viewer/model/reducers/reducer_helpers.ts(6 hunks)frontend/javascripts/viewer/model/reducers/save_reducer.ts(1 hunks)frontend/javascripts/viewer/model/reducers/update_action_application/bounding_box.ts(1 hunks)frontend/javascripts/viewer/model/reducers/update_action_application/skeleton.ts(1 hunks)frontend/javascripts/viewer/model/reducers/update_action_application/volume.ts(1 hunks)frontend/javascripts/viewer/model/sagas/annotation_saga.tsx(2 hunks)frontend/javascripts/viewer/model/sagas/root_saga.ts(1 hunks)frontend/javascripts/viewer/model/sagas/save_saga.ts(0 hunks)frontend/javascripts/viewer/model/sagas/saving/save_queue_draining.ts(1 hunks)frontend/javascripts/viewer/model/sagas/saving/save_queue_filling.ts(1 hunks)frontend/javascripts/viewer/model/sagas/saving/save_saga.ts(1 hunks)frontend/javascripts/viewer/model/sagas/saving/save_saga_constants.ts(1 hunks)frontend/javascripts/viewer/model/sagas/settings_saga.ts(1 hunks)frontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/undo_saga.ts(1 hunks)frontend/javascripts/viewer/model/sagas/volume/mapping_saga.ts(5 hunks)frontend/javascripts/viewer/model/sagas/volume/min_cut_saga.ts(3 hunks)frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts(12 hunks)frontend/javascripts/viewer/model/sagas/volume/quick_select/quick_select_heuristic_saga.ts(3 hunks)frontend/javascripts/viewer/model/sagas/volume/quick_select/quick_select_ml_saga.ts(1 hunks)frontend/javascripts/viewer/model/sagas/volume/quick_select/quick_select_saga.ts(1 hunks)frontend/javascripts/viewer/model/sagas/volume/update_actions.ts(8 hunks)frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx(2 hunks)frontend/javascripts/viewer/store.ts(7 hunks)frontend/javascripts/viewer/view/context_menu.tsx(5 hunks)frontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsx(1 hunks)frontend/javascripts/viewer/view/left-border-tabs/modals/add_volume_layer_modal.tsx(1 hunks)frontend/javascripts/viewer/view/version_entry.tsx(3 hunks)frontend/javascripts/viewer/view/version_list.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- frontend/javascripts/viewer/model/sagas/save_saga.ts
✅ Files skipped from review due to trivial changes (22)
- frontend/javascripts/viewer/model/helpers/compaction/compact_save_queue.ts
- frontend/javascripts/viewer/model/bucket_data_handling/wkstore_adapter.ts
- frontend/javascripts/viewer/model/reducers/save_reducer.ts
- frontend/javascripts/viewer/model/sagas/undo_saga.ts
- frontend/javascripts/viewer/model/bucket_data_handling/pushqueue.ts
- frontend/javascripts/viewer/model/actions/save_actions.ts
- frontend/javascripts/test/reducers/save_reducer.spec.ts
- frontend/javascripts/viewer/model/helpers/diff_helpers.ts
- frontend/javascripts/viewer/view/left-border-tabs/modals/add_volume_layer_modal.tsx
- frontend/javascripts/viewer/model/sagas/settings_saga.ts
- frontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsx
- frontend/javascripts/viewer/model/sagas/volume/quick_select/quick_select_ml_saga.ts
- frontend/javascripts/viewer/merger_mode.ts
- frontend/javascripts/test/sagas/bounding_box_saving.spec.ts
- frontend/javascripts/test/backend-snapshot-tests/annotations.e2e.ts
- frontend/javascripts/viewer/model/helpers/compaction/compact_toggle_actions.ts
- frontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts
- frontend/javascripts/viewer/model/sagas/volume/quick_select/quick_select_saga.ts
- frontend/javascripts/test/sagas/save_saga.spec.ts
- frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx
- frontend/javascripts/viewer/model/sagas/annotation_saga.tsx
- frontend/javascripts/viewer/model/sagas/volume/quick_select/quick_select_heuristic_saga.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- frontend/javascripts/test/sagas/compact_toggle_actions.spec.ts
- frontend/javascripts/test/helpers/saveHelpers.ts
- frontend/javascripts/viewer/view/version_entry.tsx
- frontend/javascripts/test/sagas/volumetracing/volumetracing_remote_bucket_updates.spec.ts
- frontend/javascripts/viewer/view/version_list.tsx
- frontend/javascripts/viewer/model/reducers/update_action_application/volume.ts
- frontend/javascripts/test/reducers/update_action_application/skeleton.spec.ts
- frontend/javascripts/viewer/model/actions/volumetracing_actions.ts
- frontend/javascripts/test/sagas/skeletontracing_saga.spec.ts
- frontend/javascripts/viewer/model/reducers/update_action_application/bounding_box.ts
- frontend/javascripts/test/reducers/update_action_application/volume.spec.ts
- frontend/javascripts/test/sagas/proofreading.spec.ts
- frontend/javascripts/viewer/view/context_menu.tsx
- frontend/javascripts/types/api_types.ts
- frontend/javascripts/viewer/store.ts
- frontend/javascripts/viewer/model/reducers/reducer_helpers.ts
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
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.
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/test/sagas/saga_integration.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/sagas/saving/save_saga_constants.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/root_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`.
frontend/javascripts/viewer/geometries/skeleton.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: 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.
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/volume/min_cut_saga.ts (5)
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: 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.
Learnt from: knollengewaechs
PR: scalableminds/webknossos#8266
File: frontend/javascripts/components/mag_selection.tsx:51-60
Timestamp: 2024-12-16T15:48:37.065Z
Learning: In the `MagSelection` component (`frontend/javascripts/components/mag_selection.tsx`), the selected magnification `value` will always be present in `allMags` because only existing values can be selected by the user.
frontend/javascripts/viewer/model/helpers/compaction/compact_update_actions.ts (3)
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/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: 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/reducers/update_action_application/skeleton.ts (3)
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.
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: 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/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#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.
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/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`.
frontend/javascripts/viewer/model/sagas/saving/save_queue_filling.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/actions/skeletontracing_actions.tsx (3)
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.
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`.
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: 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.
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`.
🧬 Code Graph Analysis (4)
frontend/javascripts/viewer/model/sagas/volume/min_cut_saga.ts (2)
frontend/javascripts/types/bounding_box.ts (1)
BoundingBoxMinMaxType(3-6)frontend/javascripts/viewer/model/types/tree_types.ts (1)
Node(32-32)
frontend/javascripts/viewer/model/sagas/volume/mapping_saga.ts (4)
frontend/javascripts/types/api_types.ts (1)
APIMapping(61-67)frontend/javascripts/test/puppeteer/dataset_rendering_helpers.ts (1)
process(21-21)frontend/javascripts/viewer/model.ts (1)
getLayerByName(270-276)frontend/javascripts/viewer/model/actions/settings_actions.ts (2)
setMappingAction(205-227)finishMappingInitializationAction(181-185)
frontend/javascripts/viewer/model/actions/skeletontracing_actions.tsx (2)
frontend/javascripts/viewer/model/actions/annotation_actions.ts (1)
AddNewUserBoundingBox(60-60)frontend/javascripts/viewer/model/sagas/volume/update_actions.ts (1)
ApplicableSkeletonUpdateAction(109-124)
frontend/javascripts/viewer/model/sagas/volume/update_actions.ts (4)
frontend/javascripts/viewer/model/types/tree_types.ts (1)
Node(32-32)frontend/javascripts/viewer/geometries/skeleton.ts (1)
createNode(554-581)frontend/javascripts/viewer/store.ts (1)
BoundingBoxObject(85-85)frontend/javascripts/viewer/constants.ts (1)
Vector3(13-13)
⏰ 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 (29)
frontend/javascripts/viewer/geometries/skeleton.ts (1)
14-14: LGTM: Import path updated for module reorganizationThe import path update correctly reflects the reorganization of update actions into the
volumesubdirectory.frontend/javascripts/test/sagas/saga_integration.spec.ts (1)
6-6: LGTM: Import paths updated for module reorganizationBoth import path updates correctly reflect the reorganization:
- Save saga constants moved to
saving/subdirectory- Update actions moved to
volume/subdirectoryAlso applies to: 22-22
frontend/javascripts/viewer/model/sagas/root_saga.ts (1)
13-13: LGTM: Import paths updated for module reorganizationAll import path updates correctly reflect the systematic reorganization:
- Save-related functionality moved to
saving/subdirectory- Volume-related sagas moved to
volume/subdirectory- Error highlighting function moved to specialized
save_queue_drainingmoduleAlso applies to: 18-19, 27-27
frontend/javascripts/viewer/model/sagas/volume/min_cut_saga.ts (2)
10-11: LGTM: Import consolidation and type standardizationThe import changes improve organization by consolidating types and standardizing the bounding box type to
BoundingBoxMinMaxType.Also applies to: 21-21
174-177: LGTM: Function signature updated with appropriate type changeThe function signature update correctly:
- Changes from
BoundingBoxTypetoBoundingBoxMinMaxTypefor type standardization- Improves readability with multi-line formatting
- Maintains the same functionality - the internal logic remains unchanged
frontend/javascripts/viewer/model/sagas/volume/mapping_saga.ts (2)
72-81: LGTM!Good refactoring to use relative imports and optimize throttle delay for test environments.
230-267: Excellent refactoring with clear documentation!The extraction of
startInterruptibleUpdateMappingand the detailed comments explaining the custom throttling approach (avoiding parallel executions) make the code much more maintainable.frontend/javascripts/viewer/model/sagas/saving/save_queue_filling.ts (1)
1-191: Well-structured save queue filling implementation!The separation between annotation and tracing saving logic is clean, with proper handling of concurrent actions through buffered channels and appropriate compaction of update actions.
frontend/javascripts/viewer/model/sagas/saving/save_saga.ts (2)
36-39: Address the TODO before merging.The polling intervals are currently all set to 1 second with a TODO comment. These should be restored to appropriate production values (10s, 60s, 30s) or the TODO should be resolved.
187-385: Comprehensive update action incorporation!The implementation properly handles all relevant action types with clear separation between user-specific state (ignored) and shared state. The fallback behavior for unsupported actions appropriately triggers a page reload request.
frontend/javascripts/viewer/model/reducers/update_action_application/skeleton.ts (1)
1-356: Well-implemented skeleton update action application!The reducer correctly handles all skeleton-related update actions with proper state immutability and error handling. The exhaustive switch statement ensures type safety.
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (2)
1245-1369: Good extraction of mapping update logic into reusable functions!The new helper functions
getSegmentIdsThatMapToAgglomerate,updateMappingWithMerge, andremoveAgglomerateFromActiveMappingimprove code modularity and properly handle both number and BigInt types.
1301-1301: Sensible API improvements!The parameter order change in
mergeAgglomeratesInMapping(source before target) follows conventional ordering patterns, and the more specific toast message improves user feedback.Also applies to: 1115-1115
frontend/javascripts/viewer/model/helpers/compaction/compact_update_actions.ts (5)
11-12: LGTM! Module reorganization is well-structured.The import path change from general
update_actionsto volume-specificupdate_actionsaligns with the broader refactoring to separate skeleton and volume update actions.
20-38: Good defensive programming with early return check.The addition of
prevTracingparameter and early return for non-skeleton tracings improves the function's robustness. This prevents unnecessary processing for volume tracings.
85-91: Type safety improvement with explicit type annotation.Good addition of explicit typing for
groupedMovedNodesAndEdges. This makes the complex nested type structure clearer and improves type safety.
98-104: Cleaner implementation using filter with type guard.Good refactoring to use
filterwith a type predicate instead of mapping. This is more idiomatic TypeScript and provides better type narrowing.
115-127: Good refactoring to eliminate code duplication.Extracting
moveActioninto a variable eliminates duplication and improves maintainability.frontend/javascripts/viewer/model/sagas/saving/save_queue_draining.ts (4)
39-102: Well-designed save queue processing with appropriate throttling.The implementation correctly handles both force-push and auto-save scenarios with different batching strategies. The periodic
delay(0)to prevent blocking is a good practice for long-running sagas.
198-222: Creative handling of 409 conflicts with page reload.The approach of sleeping for a year after showing the conflict error is unusual but justified given that the page will reload. This prevents cascading failures while allowing other sagas to complete gracefully.
233-267: Efficient bucket marking with proper memoization.Good use of
memoizeOneto cache layer and mag info lookups. The dirtyCount decrement logic correctly tracks bucket save state.
305-317: Correct version number assignment logic.The function properly increments versions only at transaction group boundaries (when
transactionGroupIndex === 0), ensuring consistent versioning across related updates.frontend/javascripts/viewer/model/actions/skeletontracing_actions.tsx (2)
7-9: Good architectural separation of actions from effects.Moving user interaction logic with modal confirmations to a separate effects module (
skeletontracing_actions_with_effects.tsx) properly separates pure actions from side effects. This follows Redux best practices.Also applies to: 567-567
241-241: Necessary type change to support nullable active node.Allowing
nullfornodeIdenables explicitly deactivating nodes, which is important for operations like tree deletion where maintaining an active node would be confusing.frontend/javascripts/viewer/model/sagas/volume/update_actions.ts (5)
109-136: Well-organized type definitions for applicable update actions.The new type aliases clearly separate skeleton and volume update actions, improving type safety when applying server-sent updates.
26-26: Clear deprecation marking with LEGACY prefix.Good practice to prefix deprecated functions with
LEGACY_while maintaining backwards compatibility.Also applies to: 316-329
376-401: Improved type safety in node action creators.The refactoring to explicitly construct typed values (
CreateActionNode,UpdateActionNode) with proper destructuring improves code clarity and type safety.
603-646: Clean refactoring of bounding box update helper.The helper now focuses solely on data transformation, with callers responsible for wrapping in appropriate action objects. This reduces code duplication and improves maintainability.
753-766: Good defensive programming with runtime validation.The null check for
base64Datawith a clear error message helps catch issues early. The comment effectively explains the type discrepancy between frontend and server usage.
|
But please mind the TODOs before merging |
From changelog:
Implementation details:
Limitations:
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)