Per-User State in Annotations: Camera, Visibility Toggles #8542
Conversation
📝 WalkthroughWalkthroughThis change introduces per-user state storage for annotation UI elements, including position, zoom, visibility, and expansion states, across backend, frontend, and protocol layers. It refactors update actions, reducers, and sagas to handle user-dependent states, updates protocol buffers and APIs, and incorporates new test cases and utility functions to support and validate this multi-user state management. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. All functional modifications relate directly to the objective of storing and handling user-dependent annotation state. Possibly related PRs
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…State, UpdateTreeVisibility
…otation-user-state
…otation-user-state
…nossos into annotation-user-state
There was a problem hiding this comment.
Ok here is what I noticed during testing. For each of these points I do not know whether BE or FE causes this as I did not further investigation:
[ ][see follow-up issue] When I have an annotation like this:
(Note the two added but empty volume layers) The reuploaded nml yields an annotation where the volume1 layer is active and not the volume layer as in the state of the downloaded annotation. Not sure how much thats related. And maybe that's expected as I did an nml only download. But the reuploaded annotation seems to have the volume1 and volume2 layers.- Moreover, nml reupload does not seem to store the correct active node id or no active node id is stored at all
- And I think the segment group merging is broken: Her I have merged 2 annotations and this is the new owners perspective -> meaning the two user states of the 2 merged annotations and what the result was.
Source 1:
Source 2:
Result:
notable differences: The "from 2" and "from 2 2" groups are empty in the merged annotation And I think the sub segments from groups "from 2" and "from 2 2" are now in "Group 1" and "Group 2". - [Expected because non-owner user states are dismissed on merge] Moreover, the skeleton user state also seems inconsistent: In this case, the user is not the owner of the merged annotation. Source 1 and Source 2 show the user's state of the two annotations which were merged by another user and then the result is this users state of the merged annotation.
Source 1 (Where group 1 and group3 each has multiple inviisble trees as children)
Source 2:
Result:
Here the group 1 from source 1 is not invisible and collapsed and in group2 tree with_001is visible now although it is not in source 1. Moreover, trees from source 2 with_003and_004are no longer invisible in the merged annotation. [ ][can't reproduce, see slack] I managed to get into a strange state of an annotation where nml reupload would make bbox with id 1 invisible though in the source it is visible. The same goes for duplicating. Not sure how to share this annotation state as the nml already has visibility set to false for this bounding box :/
|
Thanks so much for putting so much effort into testing!
I’d be happy to have a look at the two things I could not reproduce again, maybe we can find a way together. |
let's have a chat about this 🤙 I agree that it makes sense to tackle this, but I cannot say whether it's easy enough to do in the same pr. I turned @MichaelBuessemeyer comment into checkboxes. Since I fixed the "active node" issue, I ticked that off. I also crossed out or checked what seems to be a non-issue or already done. please have a second look whether I got everything right. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/test/helpers/apiHelpers.ts (1)
203-205: Dispatch order: reset first, restart sagas second
restartSagaAction()cancels and immediately re-starts the root saga tree.
Triggering it beforeresetStoreAction()means the freshly spawned sagas observe the old store state for at least one event loop tick, then have the ground pulled away when the store is reset.
Flipping the order guarantees that sagas attach to a pristine store from the outset and avoids rare race conditions in tests that listen fortakeLateston initialised state slices.- Store.dispatch(restartSagaAction()); - Store.dispatch(resetStoreAction()); + // Reset the store to its initial state *before* bringing the sagas back online + Store.dispatch(resetStoreAction()); + Store.dispatch(restartSagaAction());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
frontend/javascripts/admin/rest_api.ts(1 hunks)frontend/javascripts/libs/utils.ts(4 hunks)frontend/javascripts/test/backend-snapshot-tests/annotations.e2e.ts(3 hunks)frontend/javascripts/test/fixtures/skeletontracing_server_objects.ts(2 hunks)frontend/javascripts/test/fixtures/tasktracing_server_objects.ts(2 hunks)frontend/javascripts/test/fixtures/volumetracing_server_objects.ts(2 hunks)frontend/javascripts/test/helpers/apiHelpers.ts(2 hunks)frontend/javascripts/test/sagas/saga_integration.spec.ts(3 hunks)frontend/javascripts/test/sagas/save_saga.spec.ts(3 hunks)frontend/javascripts/test/sagas/skeletontracing_saga.spec.ts(24 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts(4 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_1.spec.ts(1 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_2.spec.ts(1 hunks)frontend/javascripts/viewer/model/sagas/save_saga.ts(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_1.spec.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_2.spec.ts
- frontend/javascripts/admin/rest_api.ts
- frontend/javascripts/test/sagas/save_saga.spec.ts
- frontend/javascripts/test/fixtures/tasktracing_server_objects.ts
- frontend/javascripts/test/fixtures/skeletontracing_server_objects.ts
- frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts
- frontend/javascripts/test/fixtures/volumetracing_server_objects.ts
- frontend/javascripts/test/sagas/saga_integration.spec.ts
- frontend/javascripts/viewer/model/sagas/save_saga.ts
- frontend/javascripts/test/backend-snapshot-tests/annotations.e2e.ts
- frontend/javascripts/test/sagas/skeletontracing_saga.spec.ts
- frontend/javascripts/libs/utils.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/test/helpers/apiHelpers.ts (1)
41-41: Confirm availability of newly-imported action
resetStoreActionis now imported fromviewer/model/actions/actions.
Double-check that this symbol is indeed exported by that barrel file; otherwise the test helper will fail to compile at runtime.
No further issues spotted with the revised import list.
There was a problem hiding this comment.
Latest backend commit (e756def) looks good to me. But I haven't tested the fix. Should I?
|
Should be fine, I tested it locally. Thanks! |
|
@MichaelBuessemeyer @daniel-wer both of you left a lgtm. this means we can merge it? :) |
|
One more question: What's the rollback strategy with the new update actions? Should we have an intermediate PR which only introduces the new actions so we can rollback to that or do you think it's fine like it is? |
|
As discussed in Slack, a full rollback would be tedious. Instead, we’ll create backups of both fossildb and postgres content before upgrading and in case of errors we can’t hotfix, we’ll restore that snapshot instead. I did another round of testing and encountered no problems. @MichaelBuessemeyer If there are no other open points, could you hit approve so that we can merge this? |
In shared annotations with multiple authors, some changes are now stored per user. This means that other users won’t see all those changes if their own diverge. This includes the current position and zoom, visibilities of trees, bounding boxes, and segments (as specified with the checkboxes in the lists), as well as which groups are expanded in the lists. The annotation owner’s user state is used as a fallback for users who haven’t explicitly changed these values themselves.
User-Specific state for annotations and tracings
Update actions that now mutate (only) the user state
https://annotationuserstate.webknossos.xyz/
Steps to test:
Note: since new update actions are introduced here, annotations that are edited on this branch cannot be loaded after switching back to master. Refresh your db to be sure everything is clean
TODOs:
Backend
UpdateSegmentGroupVisibilityVolumeActionandUpdateSegmentVisibilityVolumeActionFrontend
Issues: