Live Collab M2 - Automatically update annotation to newest changes #8648
Conversation
📝 WalkthroughWalkthroughThis change implements real-time, live updates for annotation views in WEBKNOSSOS, allowing users to see annotation changes made by others without reloading the page. It introduces mechanisms for incrementally incorporating server-sent update actions into the client state for both skeleton and volume tracings, updates action/reducer logic, and refactors bounding box type usage throughout the codebase. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
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 (
|
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
| getEdgesForAgglomerateMinCut: vi.fn( | ||
| ( | ||
| _tracingStoreUrl: string, | ||
| _tracingId: string, | ||
| _segmentsInfo: unknown, | ||
| ): Promise<Array<MinCutTargetEdge>> => { | ||
| throw new Error("Not yet mocked."); | ||
| }, | ||
| ), |
There was a problem hiding this comment.
But why mock it then? Or is this still a todo for this pr?
There was a problem hiding this comment.
I changed the content of the function to:
// This simply serves as a preparation so that specs can mock the function
// when needed. Without this stub, it's harder to mock this specific function
// later.
throw new Error("No test has mocked the return value yet here.");
I hope this makes it clearer.
frontend/javascripts/test/reducers/update_action_application/skeleton.spec.ts
Outdated
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/test/reducers/update_action_application/skeleton.spec.ts
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
frontend/javascripts/test/reducers/update_action_application/skeleton.spec.ts (2)
156-215: Complex but comprehensive test coverage.This nested test structure provides thorough coverage by testing all combinations of before/after version indices with and without compaction. While complex, this approach ensures the diff re-application mechanism works correctly across all scenarios.
The complexity concern raised in past reviews is valid, but the comprehensive nature of this test is essential for validating the live collaboration feature's correctness.
278-280: Essential verification of action type coverage.This ensures that each possible action type is included in the testing at least once, providing confidence in comprehensive test coverage.
frontend/javascripts/viewer/model/reducers/update_action_application/skeleton.ts (1)
178-191: Missing active tree/node handling in tree deletion.The
deleteTreecase should also clear the active tree and node if the deleted tree was active, similar to howdeleteNodehandles active node deletion.case "deleteTree": { const { id } = ua.value; const skeleton = enforceSkeletonTracing(state.annotation); const updatedTrees = skeleton.trees.delete(id); + const newActiveTreeId = skeleton.activeTreeId === id ? null : skeleton.activeTreeId; + const newActiveNodeId = skeleton.activeTreeId === id ? null : skeleton.activeNodeId; + return update(state, { annotation: { skeleton: { trees: { $set: updatedTrees }, cachedMaxNodeId: { $set: getMaximumNodeId(updatedTrees) }, + activeTreeId: { $set: newActiveTreeId }, + activeNodeId: { $set: newActiveNodeId }, }, }, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/javascripts/test/fixtures/hybridtracing_server_objects.ts(1 hunks)frontend/javascripts/test/fixtures/skeletontracing_server_objects.ts(2 hunks)frontend/javascripts/test/fixtures/tasktracing_server_objects.ts(3 hunks)frontend/javascripts/test/helpers/apiHelpers.ts(10 hunks)frontend/javascripts/test/reducers/update_action_application/skeleton.spec.ts(1 hunks)frontend/javascripts/viewer/default_state.ts(4 hunks)frontend/javascripts/viewer/model/reducers/skeletontracing_reducer.ts(8 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/reducers/volumetracing_reducer.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/javascripts/viewer/model/reducers/skeletontracing_reducer.ts
- frontend/javascripts/viewer/default_state.ts
- frontend/javascripts/test/fixtures/hybridtracing_server_objects.ts
- frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts
- frontend/javascripts/test/helpers/apiHelpers.ts
🧰 Additional context used
🪛 GitHub Check: frontend-tests
frontend/javascripts/viewer/model/reducers/update_action_application/skeleton.ts
[failure] 350-350:
Property 'name' does not exist on type 'never'.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (14)
frontend/javascripts/test/fixtures/skeletontracing_server_objects.ts (2)
7-7: Good practice: Centralizing view mode constants.The import of
ViewModeValuesaligns with the broader refactoring effort to centralize view mode definitions across the codebase.
209-209: Verify that ViewModeValues contains the expected view modes.The replacement of the hardcoded array with
ViewModeValuesis a good practice for maintainability. Please ensure that the constant contains the same values as the previous hardcoded array["orthogonal", "oblique", "flight"].#!/bin/bash # Description: Verify that ViewModeValues contains the expected view modes # Expected: ViewModeValues should contain ["orthogonal", "oblique", "flight"] # Find the ViewModeValues constant definition ast-grep --pattern 'export const ViewModeValues = $_' # Also search for any ViewModeValues definition or usage rg -A 3 "ViewModeValues"frontend/javascripts/test/fixtures/tasktracing_server_objects.ts (3)
7-7: Consistent import pattern with other fixture files.The import of
ViewModeValuesfollows the same pattern as the skeleton tracing fixture, maintaining consistency across the test fixtures.
92-92: Good refactoring: Centralized constants for task type settings.Replacing the hardcoded array with
ViewModeValuesin the task type settings improves maintainability and consistency.
150-150: Consistent usage of ViewModeValues across all settings.The replacement of the hardcoded array in the top-level settings maintains consistency with the task type settings above and other fixture files.
frontend/javascripts/test/reducers/update_action_application/skeleton.spec.ts (3)
59-82: Excellent type safety approach for action coverage.The
actionNamesHelperdictionary ensures compile-time verification that all applicable skeleton update actions are tested. This is a robust approach that will catch missing test coverage when new action types are added.
247-276: Test logic for active tree deletion is correct.This test properly verifies that both active tree and active node are cleared when the active tree is deleted, which is the expected behavior.
217-245: Let’s inspect how server-sidedeleteNodeupdates are applied and how the reducer handles client-side delete and active-node state:#!/bin/bash set -e echo "----- Server-update deleteNode case in update_action_application/skeleton.ts -----" sed -n '200,400p' frontend/javascripts/viewer/model/reducers/update_action_application/skeleton.ts echo echo "----- Client deleteNodeAction handling in skeletontracing_reducer.ts -----" sed -n '720,820p' frontend/javascripts/viewer/model/reducers/skeletontracing_reducer.ts echo echo "----- setActiveNodeAction handling in skeletontracing_reducer.ts -----" rg -n "setActiveNodeAction" -g "frontend/javascripts/viewer/model/reducers/skeletontracing_reducer.ts" -C 8frontend/javascripts/viewer/model/reducers/update_action_application/skeleton.ts (2)
19-30: Clean function structure for batch action application.The function correctly iterates through actions and applies them sequentially, maintaining immutability throughout the process.
38-57: Proper tree creation logic with unused parameter handling.The destructuring correctly ignores server-specific parameters (
updatedId,actionTracingId) and creates a new tree with the appropriate client-side structure.frontend/javascripts/viewer/model/reducers/update_action_application/volume.ts (4)
17-31: Well-structured function signature and implementation.The function properly accepts the reducer as a parameter and applies actions sequentially. The parameter ordering is consistent and logical.
46-58: Correct handling of segment creation and updates.The destructuring properly separates server-specific fields and transforms
anchorPositiontosomePositionfor client-side compatibility.
92-97: Appropriate handling of user-specific actions.Correctly ignores user-specific update actions (expanded state and visibility) that don't need to be applied for other users in the collaborative environment.
98-107: Clean exhaustive type checking without unreachable code.Unlike the skeleton implementation, this properly handles the default case without unreachable code issues.
frontend/javascripts/viewer/model/reducers/update_action_application/skeleton.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/model/reducers/update_action_application/skeleton.ts
Outdated
Show resolved
Hide resolved
…ng accessor; fix nulling activeTreeId when it the active tree deleted
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
frontend/javascripts/test/reducers/update_action_application/skeleton.spec.ts (2)
156-215: Comprehensive test coverage for update action application.The parameterized test approach with compaction modes and version ranges provides excellent coverage of the update action application mechanism. This ensures the system correctly handles both incremental updates and compacted update sequences.
217-279: Well-designed edge case tests for active item deletion.These tests ensure that the active node and tree selections are properly cleared when the selected items are deleted, preventing invalid application state.
frontend/javascripts/viewer/model/reducers/update_action_application/skeleton.ts (2)
304-304: Fix incorrect active node ID update logic.When deleting a node that is not the active node, the active node ID should remain unchanged, not be set to the deleted node's ID.
Apply this fix:
- const newActiveNodeId = skeleton.activeNodeId === nodeId ? null : nodeId; + const newActiveNodeId = skeleton.activeNodeId === nodeId ? null : skeleton.activeNodeId;
347-355: Remove unreachable code and fix TypeScript error.The code after the switch statement is unreachable and causes TypeScript errors. Move the error handling into the default case.
Apply this fix:
default: { ua satisfies never; + throw new Error(`Unexpected update action: ${(ua as any).name}`); } } - ua satisfies never; - - // Satisfy TS. - throw new Error("Reached unexpected part of function."); }
🧹 Nitpick comments (1)
frontend/javascripts/test/reducers/update_action_application/skeleton.spec.ts (1)
286-298: Consider removing or documenting the debug function.The
_debugLogTreesfunction appears to be unused debug code. Consider either removing it or adding a comment explaining when it should be used for debugging test failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/test/reducers/update_action_application/skeleton.spec.ts(1 hunks)frontend/javascripts/viewer/model/reducers/update_action_application/skeleton.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: frontend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (2)
frontend/javascripts/test/reducers/update_action_application/skeleton.spec.ts (1)
59-82: Excellent test coverage tracking mechanism!Using a TypeScript Record type to ensure all
ApplicableSkeletonUpdateActiontypes are covered is a clever approach. This guarantees compile-time verification that the test suite remains comprehensive as new action types are added.frontend/javascripts/viewer/model/reducers/update_action_application/skeleton.ts (1)
19-30: Well-structured implementation of server update action application.The function properly iterates through actions and maintains state immutability throughout the update process. The separation of concerns with
applySingleActionmakes the code maintainable and testable.
frontend/javascripts/viewer/model/reducers/update_action_application/skeleton.ts
Outdated
Show resolved
Hide resolved
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
| } | ||
| } | ||
| ua satisfies never; | ||
|
|
||
| // Satisfy TS. | ||
| throw new Error("Reached unexpected part of function."); |
There was a problem hiding this comment.
The ua satisfies never; part is now also needed because TS is not smart enough / too pessimistic or so?
There was a problem hiding this comment.
the throw-error part is because TS is not smart enough. satisfies-never is only there because I wanted to be extra sure that TS is really not smart enough ^^ in other words: TS agrees that ua satisfies never. so, the end of this function should not be reachable and TS should know this (from my understanding).
…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.
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)