Improve jumping between comments and skeletons #8689
Conversation
📝 WalkthroughWalkthroughThis change adds synchronization between the active tree selection and the skeleton tab UI by introducing lifecycle updates, centralized scroll logic, a Redux action for focusing trees, and an event bus with middleware and hooks to propagate and listen for Redux actions, enabling consistent UI updates and navigation between trees and comments. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~40 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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)
✨ Finishing Touches
🧪 Generate unit tests
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 (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/viewer/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
frontend/javascripts/viewer/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx (1)
Learnt from: philippotto
PR: scalableminds/webknossos#8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
🧬 Code Graph Analysis (1)
frontend/javascripts/viewer/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx (1)
frontend/javascripts/viewer/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx (1)
Props(60-73)
🔇 Additional comments (1)
frontend/javascripts/viewer/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx (1)
796-804: Good implementation of tree selection synchronization.The componentDidUpdate method correctly implements the synchronization between the active tree and selected trees. The reference equality check is appropriate given WebKnossos's Redux immutability principles, and the null safety checks prevent potential runtime errors.
frontend/javascripts/viewer/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/javascripts/viewer/model/reducers/skeletontracing_reducer.ts (1)
561-561: Remove console.log statement before production deployment.This appears to be debugging code that should not be committed to the main branch. If logging is needed for production, consider using a proper logging library instead of console.log.
- console.log("Expanding parent groups of tree in reducer");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/javascripts/viewer/model/helpers/event_bus.ts(1 hunks)frontend/javascripts/viewer/model/helpers/event_emitter_middleware.ts(1 hunks)frontend/javascripts/viewer/model/helpers/listener_helpers.ts(2 hunks)frontend/javascripts/viewer/model/reducers/skeletontracing_reducer.ts(1 hunks)frontend/javascripts/viewer/store.ts(2 hunks)frontend/javascripts/viewer/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx(3 hunks)
🧠 Learnings (5)
📓 Common learnings
Learnt from: knollengewaechs
PR: scalableminds/webknossos#8689
File: frontend/javascripts/viewer/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx:796-804
Timestamp: 2025-07-15T13:15:37.856Z
Learning: In the WebKnossos codebase, the activeTreeId in skeleton tracing can become null when the active tree is deleted, during initialization, or when trees are deselected via deselectActiveTreeAction. The constructor of SkeletonTabView already handles this null case by setting selectedTreeIds to an empty array.
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#8321
File: frontend/javascripts/components/slider.tsx:0-0
Timestamp: 2025-02-05T20:16:45.712Z
Learning: When tracking focus state that needs to be accessed by native event listeners (e.g., wheel events), use `useRef` instead of `useState` to ensure the event handlers always have access to the current value. This is because event listeners added via `addEventListener` would capture the initial state value if `useState` were used.
frontend/javascripts/viewer/model/reducers/skeletontracing_reducer.ts (4)
Learnt from: dieknolle3333
PR: #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: #8689
File: frontend/javascripts/viewer/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx:796-804
Timestamp: 2025-07-15T13:15:37.856Z
Learning: In the WebKnossos codebase, the activeTreeId in skeleton tracing can become null when the active tree is deleted, during initialization, or when trees are deselected via deselectActiveTreeAction. The constructor of SkeletonTabView already handles this null case by setting selectedTreeIds to an empty array.
Learnt from: philippotto
PR: #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: #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/store.ts (1)
Learnt from: philippotto
PR: #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/helpers/event_emitter_middleware.ts (1)
Learnt from: philippotto
PR: #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/helpers/listener_helpers.ts (2)
Learnt from: knollengewaechs
PR: #8321
File: frontend/javascripts/components/slider.tsx:0-0
Timestamp: 2025-02-05T20:16:45.712Z
Learning: When tracking focus state that needs to be accessed by native event listeners (e.g., wheel events), use useRef instead of useState to ensure the event handlers always have access to the current value. This is because event listeners added via addEventListener would capture the initial state value if useState were used.
Learnt from: philippotto
PR: #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.
✅ Files skipped from review due to trivial changes (1)
- frontend/javascripts/viewer/model/helpers/event_bus.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/viewer/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: knollengewaechs
PR: scalableminds/webknossos#8689
File: frontend/javascripts/viewer/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx:796-804
Timestamp: 2025-07-15T13:15:37.856Z
Learning: In the WebKnossos codebase, the activeTreeId in skeleton tracing can become null when the active tree is deleted, during initialization, or when trees are deselected via deselectActiveTreeAction. The constructor of SkeletonTabView already handles this null case by setting selectedTreeIds to an empty array.
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#8321
File: frontend/javascripts/components/slider.tsx:0-0
Timestamp: 2025-02-05T20:16:45.712Z
Learning: When tracking focus state that needs to be accessed by native event listeners (e.g., wheel events), use `useRef` instead of `useState` to ensure the event handlers always have access to the current value. This is because event listeners added via `addEventListener` would capture the initial state value if `useState` were used.
frontend/javascripts/viewer/model/reducers/skeletontracing_reducer.ts (4)
Learnt from: dieknolle3333
PR: #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: #8689
File: frontend/javascripts/viewer/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx:796-804
Timestamp: 2025-07-15T13:15:37.856Z
Learning: In the WebKnossos codebase, the activeTreeId in skeleton tracing can become null when the active tree is deleted, during initialization, or when trees are deselected via deselectActiveTreeAction. The constructor of SkeletonTabView already handles this null case by setting selectedTreeIds to an empty array.
Learnt from: philippotto
PR: #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: #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/store.ts (1)
Learnt from: philippotto
PR: #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/helpers/event_emitter_middleware.ts (1)
Learnt from: philippotto
PR: #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/helpers/listener_helpers.ts (2)
Learnt from: knollengewaechs
PR: #8321
File: frontend/javascripts/components/slider.tsx:0-0
Timestamp: 2025-02-05T20:16:45.712Z
Learning: When tracking focus state that needs to be accessed by native event listeners (e.g., wheel events), use useRef instead of useState to ensure the event handlers always have access to the current value. This is because event listeners added via addEventListener would capture the initial state value if useState were used.
Learnt from: philippotto
PR: #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.
⏰ 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: backend-tests
- GitHub Check: frontend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (2)
frontend/javascripts/viewer/store.ts (1)
81-81: Good middleware placement and integration.The eventEmitterMiddleware is correctly placed at the end of the middleware chain, ensuring all other middleware processes actions before events are emitted to listeners. This prevents potential race conditions or ordering issues.
Also applies to: 614-619
frontend/javascripts/viewer/model/helpers/event_emitter_middleware.ts (1)
4-7: Clean and correct Redux middleware implementation.The middleware properly follows Redux middleware patterns, emits events for all actions, and maintains the middleware chain. The implementation is minimal and focused on its single responsibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/viewer/model/actions/skeletontracing_actions.tsx (1)
406-411: Refactor to match established coding pattern.The implementation is functionally correct but inconsistent with the coding style used throughout the file. All other action creators use arrow functions with implicit returns.
Apply this diff to match the established pattern:
-export const focusTreeAction = (tree: Tree) => { - return { - type: "FOCUS_TREE", - tree, - } as const; -}; +export const focusTreeAction = (tree: Tree) => + ({ + type: "FOCUS_TREE", + tree, + }) as const;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/javascripts/viewer/model/actions/skeletontracing_actions.tsx(3 hunks)frontend/javascripts/viewer/view/context_menu.tsx(2 hunks)frontend/javascripts/viewer/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx(3 hunks)
🧠 Learnings (3)
📓 Common learnings
Learnt from: knollengewaechs
PR: scalableminds/webknossos#8689
File: frontend/javascripts/viewer/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx:796-804
Timestamp: 2025-07-15T13:15:37.856Z
Learning: In the WebKnossos codebase, the activeTreeId in skeleton tracing can become null when the active tree is deleted, during initialization, or when trees are deselected via deselectActiveTreeAction. The constructor of SkeletonTabView already handles this null case by setting selectedTreeIds to an empty array.
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/view/context_menu.tsx (1)
Learnt from: knollengewaechs
PR: #8689
File: frontend/javascripts/viewer/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx:796-804
Timestamp: 2025-07-15T13:15:37.856Z
Learning: In the WebKnossos codebase, the activeTreeId in skeleton tracing can become null when the active tree is deleted, during initialization, or when trees are deselected via deselectActiveTreeAction. The constructor of SkeletonTabView already handles this null case by setting selectedTreeIds to an empty array.
frontend/javascripts/viewer/model/actions/skeletontracing_actions.tsx (1)
Learnt from: knollengewaechs
PR: #8689
File: frontend/javascripts/viewer/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx:796-804
Timestamp: 2025-07-15T13:15:37.856Z
Learning: In the WebKnossos codebase, the activeTreeId in skeleton tracing can become null when the active tree is deleted, during initialization, or when trees are deselected via deselectActiveTreeAction. The constructor of SkeletonTabView already handles this null case by setting selectedTreeIds to an empty array.
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/viewer/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: knollengewaechs
PR: scalableminds/webknossos#8689
File: frontend/javascripts/viewer/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx:796-804
Timestamp: 2025-07-15T13:15:37.856Z
Learning: In the WebKnossos codebase, the activeTreeId in skeleton tracing can become null when the active tree is deleted, during initialization, or when trees are deselected via deselectActiveTreeAction. The constructor of SkeletonTabView already handles this null case by setting selectedTreeIds to an empty array.
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/view/context_menu.tsx (1)
Learnt from: knollengewaechs
PR: #8689
File: frontend/javascripts/viewer/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx:796-804
Timestamp: 2025-07-15T13:15:37.856Z
Learning: In the WebKnossos codebase, the activeTreeId in skeleton tracing can become null when the active tree is deleted, during initialization, or when trees are deselected via deselectActiveTreeAction. The constructor of SkeletonTabView already handles this null case by setting selectedTreeIds to an empty array.
frontend/javascripts/viewer/model/actions/skeletontracing_actions.tsx (1)
Learnt from: knollengewaechs
PR: #8689
File: frontend/javascripts/viewer/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx:796-804
Timestamp: 2025-07-15T13:15:37.856Z
Learning: In the WebKnossos codebase, the activeTreeId in skeleton tracing can become null when the active tree is deleted, during initialization, or when trees are deselected via deselectActiveTreeAction. The constructor of SkeletonTabView already handles this null case by setting selectedTreeIds to an empty array.
⏰ 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 (4)
frontend/javascripts/viewer/model/actions/skeletontracing_actions.tsx (2)
30-30: LGTM!The type alias follows the established pattern and is positioned appropriately.
123-123: LGTM!The union type update is correct and the action type is appropriately excluded from
SkeletonTracingSaveRelevantActionssince it's a UI-only action.frontend/javascripts/viewer/view/context_menu.tsx (2)
113-113: LGTM!The import is correctly placed and follows the established pattern.
628-628: LGTM!The dispatch call is logically placed after expanding parent groups, ensuring proper sequence of operations for the "Focus Tree in Skeleton Tab" functionality.
knollengewaechs
changed the title
knollengewaechs
requested review from
MichaelBuessemeyer
and removed request for
philippotto
frontend/javascripts/viewer/model/helpers/event_emitter_middleware.ts
Outdated
Show resolved
Hide resolved
|
Thanks for the quick review! 🙏 Will merge this when everyone is back at their desk :) |
Improve jumping between comments and skeletons by keeping skeleton tab up to date with active tree.
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
-> It wasnt trivial to open another tab so this is for a follow-up
(Please delete unneeded items, merge only when none are left open)