Store position in layer space when double-clicking segment

[knollengewaechs]

URL of deployed dev instance (used for testing):

• https://___.webknossos.xyz

Steps to test:

• brush or register a segment
• toggle coordinate system so that the volume layer has active transforms
• double click the segment in the data viewport (this stores the clicked position in the segment)
• click the segment in the sidebar list --> jumps to the incorrect position

Issues:

• fixes Store segment positions in layer space #9012
• follow-up for Fix loading ad-hoc meshes for transformed layers #8992

---

(Please delete unneeded items, merge only when none are left open)

• Added changelog entry (create a $PR_NUMBER.md file in unreleased_changes or use ./tools/create-changelog-entry.py)
• Added migration guide entry if applicable (edit the same file as for the changelog)
• Updated documentation if applicable
• Adapted wk-libs python client if relevant API parts change
• Removed dev-only changes like prints and application.conf edits
• Considered common edge cases
• Needs datastore update after deployment

[coderabbitai]

📝 Walkthrough

Walkthrough

Selection and context-menu flows now compute and store cell/segment positions in a visible segmentation layer's local coordinate space when available, falling back to global coordinates otherwise.

Changes

Cohort / File(s)	Summary
Layer-space selection handlers frontend/javascripts/viewer/controller/combinations/volume_handlers.ts, frontend/javascripts/viewer/view/context_menu.tsx	Dispatch of setActiveCellAction now uses the position transformed into the visible segmentation layer's local coordinate space (positionInLayerSpace) when available, otherwise falls back to the global position.
Changelog unreleased_changes/9017.md	Added entry documenting that registering/double-clicking a segment stores its position in the segment's layer coordinate system.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• Fix loading ad-hoc meshes for transformed layers #8992 — Similar edits switching click/context handlers to pass layer-space transformed positions with a global fallback.

Suggested reviewers

• philippotto
• MichaelBuessemeyer

Poem

🐰 Hopping through transforms, I chart each place,

Positions tucked safely in their layer space.
Clicks land true no matter what the view,
A burrowed fix — precise and new!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title "Store position in layer space when double-clicking segment" directly and accurately summarizes the primary change in the pull request. According to the raw summary, both modified files now compute and dispatch cell positions using layer-space coordinates instead of global positions. The title is specific, concise, and immediately conveys the main objective to a reviewer scanning the history, without unnecessary noise or vagueness.
Linked Issues Check	✅ Passed	The code changes directly address the requirements from linked issue #9012. The modifications in volume_handlers.ts and context_menu.tsx now compute cell positions in layer space when a visible segmentation layer exists and use these transformed positions (with fallback to global position) when dispatching the setActiveCellAction, replacing the previous behavior of always using global coordinates. This implementation aligns with the objective to ensure segment positions are stored in layer space and remain correct across coordinate system changes, fixing the bug where stored positions became incorrect when switching coordinate systems.
Out of Scope Changes Check	✅ Passed	All code changes in this pull request are directly in scope for addressing issue #9012. The modifications to volume_handlers.ts and context_menu.tsx focus exclusively on transforming positions to layer space before dispatching them, and the changelog entry (unreleased_changes/9017.md) documents this fix. No tangential changes, refactoring, or unrelated modifications are apparent in the changeset—all modifications are tightly focused on the single objective of storing segment positions in layer space.
Description Check	✅ Passed	The pull request description is clearly related to the changeset and provides relevant context including testing steps, reproduction instructions, issue references (#9012 and #8992), and confirmation that a changelog entry was added. While the description includes some unchecked checklist items and placeholder information, the core content meaningfully describes the purpose and testing approach for the changes made.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-segment-position-double-click

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 62461e2 and 338769e.

📒 Files selected for processing (1)

• unreleased_changes/9017.md (1 hunks)

⏰ 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: backend-tests
• GitHub Check: build-smoketest-push

🔇 Additional comments (1)

unreleased_changes/9017.md (1)

1-2: LGTM!

The changelog entry clearly documents the fix and aligns well with the PR objectives. The spelling has been corrected from the previous review feedback.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (1)

141-154: LGTM! Correctly stores position in layer space.

The implementation correctly transforms the global position to layer space when a visible segmentation layer exists, falling back to the global position otherwise. This ensures segment positions remain correct across coordinate system changes, addressing the issue described in #9012.

Optional: Consider extracting common logic to reduce duplication.

Similar position transformation logic appears in both this file (lines 141-150) and context_menu.tsx (lines 1059-1067). Consider extracting a helper function:

function getPositionForSegmentActivation(
  globalPos: Vector3,
  visibleSegmentationLayer: APIDataLayer | null | undefined,
  state: WebknossosState,
): Vector3 {
  if (visibleSegmentationLayer != null) {
    return globalToLayerTransformedPosition(
      globalPos,
      visibleSegmentationLayer.name,
      "segmentation",
      state,
    );
  }
  return globalPos;
}

Then use it in both locations:

-  const positionInLayerSpace =
-    visibleSegmentationLayer != null
-      ? globalToLayerTransformedPosition(
-          globalPos,
-          visibleSegmentationLayer.name,
-          "segmentation",
-          Store.getState(),
-        )
-      : null;
-
-  Store.dispatch(
-    setActiveCellAction(segmentId, positionInLayerSpace || globalPos, additionalCoordinates),
-  );
+  const positionForActivation = getPositionForSegmentActivation(
+    globalPos,
+    visibleSegmentationLayer,
+    Store.getState(),
+  );
+  Store.dispatch(
+    setActiveCellAction(segmentId, positionForActivation, additionalCoordinates),
+  );

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 155e22a and 7b615c1.

📒 Files selected for processing (2)

• frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (1 hunks)
• frontend/javascripts/viewer/view/context_menu.tsx (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

frontend/javascripts/viewer/view/context_menu.tsx (1)

frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (1)

• setActiveCellAction (206-218)

frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (2)

frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (1)

• globalToLayerTransformedPosition (487-506)

frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (1)

• setActiveCellAction (206-218)

🔇 Additional comments (2)

frontend/javascripts/viewer/view/context_menu.tsx (2)

1059-1067: LGTM! Consistent layer space transformation.

The position transformation logic correctly mirrors the implementation in volume_handlers.ts, ensuring consistent behavior across different code paths for segment activation.

---

1433-1437: LGTM! Correctly applies layer space position.

The context menu action properly uses the transformed position with appropriate fallback, ensuring segment positions are stored in layer space across all activation paths.

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7b615c1 and 62461e2.

📒 Files selected for processing (1)

• unreleased_changes/9017.md (1 hunks)

🧰 Additional context used

🪛 LanguageTool

unreleased_changes/9017.md

[grammar] ~2-~2: Ensure spelling is correct
Context: ...r coordinate system, preserving correct behvaiour when layer transforms are in use.

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

⏰ 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). (3)

• GitHub Check: build-smoketest-push
• GitHub Check: frontend-tests
• GitHub Check: backend-tests

[MichaelBuessemeyer]

very nice, looks fixed to me 🎉

Thanks for fixing this so fast 🏎️

The comment below is only regarding a potential follow up

[MichaelBuessemeyer]

I was a little confused by the plain string "segmentation" here at first. @philippotto Do we maybe want to refactor this into an enum, so we can write something like LayerType.Segmentation or so? This would have made this upon first read easier to understand / not stumble upon.

=> But this refacoring shouldn't be done here.

[philippotto]

sorry, this comment slipped through in my mail box. yes, sounds like a good idea. we should be careful about the naming though. I think, it's often called "category".

[MichaelBuessemeyer]

ok I opened a short issue fore this: #9035