Add explicit units to annotation settings #9127
Conversation
📝 WalkthroughWalkthroughUI and formatting updates to make unit display dataset-aware: removed hardcoded movement unit, added dataset-derived short units in labels and toasts, improved voxel/metric formatting (ThinSpace, "Vx", large-number abbreviations), adjusted tooltip rendering contract, and updated related docs/changelog. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2024-11-22T17:18:43.411ZApplied to files:
📚 Learning: 2025-08-27T11:34:16.411ZApplied to files:
🧬 Code graph analysis (1)frontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsx (2)
⏰ 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)
🔇 Additional comments (3)
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. Comment |
![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
🧹 Nitpick comments (1)
frontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsx (1)
1177-1299: Consider guarding against missing or unmapped scale units when building labelsUsing
LongUnitToShortUnitMap[dataset.dataSource.scale.unit]and then always appending(${unit})to the “Node Radius”, “(Min.) Particle Size”, and non‑arbitrary “Clipping Distance” labels works as long as:
dataset.dataSource.scale.unitis always defined, and- every possible value is present in
LongUnitToShortUnitMap.If either assumption is violated (older datasets without units, new/unknown units, or typos), the UI would render e.g.
Node Radius (undefined)which is a small but visible glitch.You could make this a bit more defensive by allowing the unit suffix to be optional, while preserving the intended behavior when the mapping succeeds:
- const unit = LongUnitToShortUnitMap[dataset.dataSource.scale.unit]; + const unit = + dataset.dataSource.scale.unit != null + ? LongUnitToShortUnitMap[dataset.dataSource.scale.unit] + : undefined; ... - <LogSliderSetting - label={`Node Radius (${unit})`} + <LogSliderSetting + label={unit ? `Node Radius (${unit})` : "Node Radius"} ... - <NumberSliderSetting - label={ - (userConfiguration.overrideNodeRadius - ? settings.particleSize - : `Min. ${settings.particleSize}`) + ` (${unit})` - } + <NumberSliderSetting + label={ + (userConfiguration.overrideNodeRadius + ? settings.particleSize + : `Min. ${settings.particleSize}`) + (unit ? ` (${unit})` : "") + } ... - <LogSliderSetting - label={settings.clippingDistance + ` (${unit})`} + <LogSliderSetting + label={settings.clippingDistance + (unit ? ` (${unit})` : "")}This keeps the new explicit units where available, but avoids surfacing
"undefined"in edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/javascripts/messages.tsx(1 hunks)frontend/javascripts/viewer/model/sagas/settings_saga.ts(2 hunks)frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx(1 hunks)frontend/javascripts/viewer/view/left-border-tabs/controls_and_rendering_settings_tab.tsx(3 hunks)frontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsx(6 hunks)frontend/javascripts/viewer/view/statusbar.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-11-22T17:18:43.411Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 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.
Applied to files:
frontend/javascripts/messages.tsxfrontend/javascripts/viewer/view/left-border-tabs/controls_and_rendering_settings_tab.tsxfrontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsx
📚 Learning: 2025-09-08T13:33:47.097Z
Learnt from: MichaelBuessemeyer
Repo: scalableminds/webknossos PR: 8824
File: frontend/javascripts/viewer/view/context_menu.tsx:1033-1039
Timestamp: 2025-09-08T13:33:47.097Z
Learning: In frontend/javascripts/viewer/view/context_menu.tsx, the segmentIdLabel intentionally shows the unmapped segment ID (maybeUnmappedSegmentId) in the "within Segment ${id}" text when proofreading is active, as this can be helpful for users. The mapped segment ID (segmentIdAtPosition) is only used as a fallback when the unmapped ID is not available.
Applied to files:
frontend/javascripts/viewer/view/statusbar.tsx
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos 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`.
Applied to files:
frontend/javascripts/viewer/model/sagas/settings_saga.ts
📚 Learning: 2025-08-27T11:34:16.411Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8796
File: frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx:68-77
Timestamp: 2025-08-27T11:34:16.411Z
Learning: The webknossos codebase uses `getDefaultLayerViewConfiguration` to ensure that layer configurations always exist in `datasetConfiguration.layers`, making direct access like `datasetConfiguration.layers[colorLayer.name]` safe across multiple files including api_latest.ts and load_histogram_data_saga.ts.
Applied to files:
frontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsx
🧬 Code graph analysis (3)
frontend/javascripts/viewer/view/left-border-tabs/controls_and_rendering_settings_tab.tsx (1)
frontend/javascripts/viewer/constants.ts (1)
LongUnitToShortUnitMap(470-497)
frontend/javascripts/viewer/model/sagas/settings_saga.ts (1)
frontend/javascripts/viewer/constants.ts (1)
LongUnitToShortUnitMap(470-497)
frontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsx (2)
frontend/javascripts/viewer/constants.ts (1)
LongUnitToShortUnitMap(470-497)frontend/javascripts/messages.tsx (1)
settings(16-52)
🔇 Additional comments (5)
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx (1)
116-116: LGTM!The added tooltip clarifies that position values are displayed in voxels, which aligns with the PR objective of adding explicit units to the UI.
frontend/javascripts/viewer/view/statusbar.tsx (1)
580-586: LGTM!The tooltip clarifies that the displayed position coordinates are in voxels, consistent with the similar change in
dataset_position_view.tsx.frontend/javascripts/viewer/view/left-border-tabs/controls_and_rendering_settings_tab.tsx (1)
241-242: LGTM!The dynamic unit derivation from the dataset scale is correctly implemented. Both move value slider variants now display the appropriate unit based on the dataset configuration.
frontend/javascripts/viewer/model/sagas/settings_saga.ts (1)
111-114: LGTM!The toast message now correctly displays the move value with its unit derived from the dataset scale, providing consistent feedback to users when they change this setting.
frontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsx (1)
65-70: Imports for control/transform/unit constants are consistent and usedThe extended import of
ControlModeEnum,IdentityTransform,LongUnitToShortUnitMap, andMappingStatusEnumfromviewer/constantsmatches actual usage in this file (view-mode checks, transform comparison, unit mapping, and mapping status checks). No issues here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unreleased_changes/9127.md (1)
1-2: Consider expanding the changelog entry for completeness.The entry captures the main changes (node radius, particle size) but could be more comprehensive. Based on the PR summary, this also includes clipping distance and updates to movement speed display across multiple UI components. The current phrasing "such as" works, but explicitly mentioning these broader changes would better communicate the scope.
Suggested enhancement:
-### Added -- Added explicit units to some layer and annotation settings, such as the node radius or the particle size. +### Added +- Added explicit units to layer and annotation settings (node radius, particle size, clipping distance) and movement speed display across viewer UI components.This more precisely describes the scope and impact of the changes without becoming overly verbose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
unreleased_changes/9127.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: build-smoketest-push
- GitHub Check: backend-tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/ui/layers.md (1)
57-57: Consider clarifying that unit display is now dynamic.The hardcoded unit "(nm/s)" has been appropriately removed from the label to support dataset-specific scaling. However, the documentation doesn't clarify that units are now displayed dynamically alongside values in the UI based on the dataset's configured scale unit. Adding a brief note—either here or in an introductory section—would help readers understand where and how units are displayed, especially for users familiar with the previous hardcoded-unit approach.
For example, you could append: "The unit for this value is displayed dynamically in the UI and corresponds to your dataset's configured scale unit."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/ui/layers.md(1 hunks)frontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsx
⏰ 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: backend-tests
- GitHub Check: frontend-tests
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/action-bar/dataset_position_view.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx (3)
frontend/javascripts/viewer/constants.ts (2)
Vector3(15-15)LongUnitToShortUnitMap(470-497)frontend/javascripts/viewer/model/scaleinfo.ts (1)
convertVoxelSizeToUnit(53-58)frontend/javascripts/libs/format_utils.ts (1)
formatNumberToLength(219-233)
⏰ 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: frontend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (1)
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx (1)
124-155: Tooltip integration aroundVector3Inputlooks good; verify event forwarding and nested tooltip behaviorThe controlled
TooltipwrappingVector3InputwithonMouseEnter/onMouseLeaveon the input is a reasonable approach and keeps computation oftitlegated onisTooltipOpen. Nested tooltips with the outerFastTooltipalso seem acceptable for this UI.Two small things to verify:
- Ensure
Vector3Inputforwards mouse events to its root DOM element so the tooltip reliably opens/closes on hover.- Confirm there are no UX quirks from nesting
FastTooltipand antdTooltip(e.g., overlapping z-order or flicker) in the deployed viewer; if there are, you might consider usingFastTooltipfor both or aligning placements.Otherwise, this wiring looks solid and maintains existing behavior while adding the metric-position affordance.
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/javascripts/viewer/view/distance_measurement_tooltip.tsx (1)
25-37: Distance copy UX looks good; consider minor accessibility polishThe copy behavior and ordering (Vx first, metric second) look correct and consistent with the new unit handling. To make the copy icon more accessible and easier to click, consider wrapping
CopyOutlinedin a<button>(or addingrole="button"andtabIndex={0}plus anaria-label) so keyboard users can trigger it as well.Also applies to: 132-133
frontend/javascripts/libs/format_utils.ts (1)
142-161: Unit string and high-voxel formatting are coherent; consider consolidating helpers laterThe updated use of
ThinSpace,MultiplicationSymbol,"Vx", and${newUnit}³/VxinformatScaleandformatLengthAsVxis consistent with the new unit conventions. The newformatVoxelsForHighNumberscorrectly reuses the same thresholds asformatVoxelswhile improving spacing and omitting the KVx step, which fits coordinate-style usage.If you find more call sites needing the “high numbers” style, it might be worth refactoring
formatVoxels/formatVoxelsForHighNumbersto share the core scaling logic to avoid future divergence, but that’s purely optional.Also applies to: 400-406, 536-556
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx (2)
43-44: Position tooltip math and unit handling look correct; maybe rename state for clarityThe new tooltip content correctly:
- Uses
voxelSizefromstate.dataset.dataSource.scale.- Formats voxel coordinates with
formatVoxelsForHighNumbers(giving values like123 Vx).- Converts to the dataset’s metric unit via
convertVoxelSizeToUnit+LongUnitToShortUnitMapand formats withformatNumberToLength, so the metric row shows explicit units (e.g.12.3 µm).This matches the PR goal of explicit, dataset-aware units.
Small readability nit:
getPositionStringsis a boolean but reads like a function name. Renaming it to something likeisPositionPopoverOpen(and updating the setter accordingly) would make the intent of the state much clearer.Also applies to: 86-99, 100-136
154-166: Popover wiring around Vector3Input is sound; consider minor UX refinementsWrapping
Vector3InputinPopoverwithonOpenChange={setGetPositionStrings}and conditionalcontentensures the tooltip is only computed/rendered when opened, and it will stay up-to-date as the flycam position changes.Optionally, you could:
- Use a more explicit empty content (
nullor a small placeholder) instead of<></>to document intent.- Consider reusing
copyToClipboadfor the main pushpin button as well for consistent copy feedback, though this is non-blocking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/javascripts/libs/format_utils.ts(3 hunks)frontend/javascripts/viewer/model/accessors/dataset_accessor.ts(3 hunks)frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx(4 hunks)frontend/javascripts/viewer/view/distance_measurement_tooltip.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos 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`.
Applied to files:
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsxfrontend/javascripts/viewer/model/accessors/dataset_accessor.ts
📚 Learning: 2024-11-22T17:18:43.411Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 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.
Applied to files:
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx
📚 Learning: 2024-11-25T14:38:49.345Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8229
File: frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts:348-354
Timestamp: 2024-11-25T14:38:49.345Z
Learning: For the `getDatasetExtentAsProduct` function in `dataset_accessor.ts`, input validation for negative or zero dimensions is not necessary.
Applied to files:
frontend/javascripts/viewer/model/accessors/dataset_accessor.ts
📚 Learning: 2024-12-16T15:48:37.065Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 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.
Applied to files:
frontend/javascripts/viewer/model/accessors/dataset_accessor.ts
📚 Learning: 2024-11-22T17:17:39.914Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos 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`.
Applied to files:
frontend/javascripts/viewer/model/accessors/dataset_accessor.ts
📚 Learning: 2025-05-07T06:17:32.810Z
Learnt from: philippotto
Repo: scalableminds/webknossos PR: 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.
Applied to files:
frontend/javascripts/libs/format_utils.ts
🧬 Code graph analysis (1)
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx (6)
frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)frontend/javascripts/viewer/constants.ts (1)
LongUnitToShortUnitMap(470-497)frontend/javascripts/libs/format_utils.ts (2)
formatVoxelsForHighNumbers(536-556)formatNumberToLength(219-233)frontend/javascripts/viewer/model/scaleinfo.ts (1)
convertVoxelSizeToUnit(53-58)frontend/javascripts/admin/voxelytics/utils.ts (1)
copyToClipboad(62-65)frontend/javascripts/libs/vector_input.tsx (1)
Vector3Input(191-195)
⏰ 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: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (1)
frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (1)
19-20: Voxel extent formatting change to ThinSpace + “Vx” looks consistentUsing
Unicode.ThinSpaceand theVxunit ingetDatasetExtentAsStringaligns nicely with the new voxel formatting utilities and improves visual consistency (width×height×depth Vx). No functional issues spotted.Also applies to: 34-35, 300-303
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx (1)
107-117: Consider labeling the two rows in the tooltip for readabilityRight now the tooltip shows two bare triples; adding lightweight labels (e.g. “Voxels:” for the first row and
${shortUnit}:or the long unit name for the second) would make it immediately clear which row is voxel indices vs. metric coordinates, especially for less experienced users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos 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`.
Applied to files:
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx
📚 Learning: 2024-11-22T17:18:43.411Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 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.
Applied to files:
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx
🧬 Code graph analysis (1)
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx (6)
frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)frontend/javascripts/viewer/constants.ts (1)
LongUnitToShortUnitMap(470-497)frontend/javascripts/libs/format_utils.ts (2)
formatVoxelsForHighNumbers(536-556)formatNumberToLength(219-233)frontend/javascripts/viewer/model/scaleinfo.ts (1)
convertVoxelSizeToUnit(53-58)frontend/javascripts/components/fast_tooltip.tsx (1)
FastTooltip(54-123)frontend/javascripts/libs/vector_input.tsx (1)
Vector3Input(191-195)
⏰ 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: frontend-tests
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (1)
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx (1)
42-43: Tooltip computation, unit handling, and hook usage look solid
- Selecting
voxelSizefromstate.dataset.dataSource.scaleand derivingshortUnitviaLongUnitToShortUnitMapaligns with the dataset’s configured scale.- Using
convertVoxelSizeToUnit(voxelSize, shortUnit)and thencoord * voxelSizeInMetricUnit[index]yields coordinates in the dataset’s metric unit, andformatVoxelsForHighNumbers/formatNumberToLengthprovide consistent string representations.getPositionTooltipContentis correctly wrapped inuseCallbackwithpositionandvoxelSizein the dependency array, and its use asdynamicRendererforFastTooltipcleanly augmentsVector3Inputwithout altering existing behavior.Also applies to: 98-120, 138-146
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx (1)
113-126: Consider adding row labels for clarity.The tooltip displays two rows without explicit labels. Users may not immediately understand that the first row shows voxel coordinates and the second shows metric coordinates.
Consider adding labels:
return ( <div> + <div style={{ fontWeight: 'bold', marginBottom: 4 }}>Position</div> <Row justify="space-between" gutter={16} wrap={false}> <Col span={8}>{positionInVxStrings[0]},</Col> <Col span={8}>{positionInVxStrings[1]},</Col> <Col span={8}>{positionInVxStrings[2]}</Col> </Row> <Row justify="space-between" gutter={16} wrap={false}> <Col span={8}>{positionInMetricStrings[0]},</Col> <Col span={8}>{positionInMetricStrings[1]},</Col> <Col span={8}>{positionInMetricStrings[2]}</Col> </Row> </div> );Alternatively, if the formatted strings already include unit suffixes (e.g., "100 Vx", "1.5 nm"), the current display may be sufficient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/components/fast_tooltip.tsx(2 hunks)frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-05-07T06:17:32.810Z
Learnt from: philippotto
Repo: scalableminds/webknossos PR: 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.
Applied to files:
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos 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`.
Applied to files:
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx
📚 Learning: 2024-11-22T17:18:43.411Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 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.
Applied to files:
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx
⏰ 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 (6)
frontend/javascripts/components/fast_tooltip.tsx (1)
52-52: LGTM! Type signature broadening is appropriate.The updated type allows dynamic renderers to return null when tooltip content shouldn't be displayed. This is safe and backward-compatible, and the consuming code at line 148 already handles null returns correctly.
Also applies to: 79-79
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx (5)
2-2: LGTM! Imports support the new tooltip functionality.All added imports are utilized in the tooltip rendering logic and formatting operations.
Also applies to: 4-4, 11-12, 16-16
44-44: LGTM! Ref pattern avoids stale closures.Using
maybeErrorMessageRefto persist the error state for the dynamic tooltip callback is appropriate and avoids stale closure issues.Also applies to: 98-98
127-127: LGTM! Callback dependencies are correct.The
[voxelSize]dependency array is appropriate. UsingStore.getState()inside the callback ensures fresh position data when the tooltip opens, rather than capturing stale values from the render.
145-153: LGTM! FastTooltip integration is well-structured.The
dynamicRendererpattern correctly provides fresh position data with both voxel and metric representations when the tooltip opens. This effectively supports the PR objective of displaying explicit units.
104-104: The code is type-safe and does not require a guard.voxelSize.unitis typed asUnitLongenum, andLongUnitToShortUnitMapis defined asRecord<UnitLong, UnitShort>with complete coverage of all 26 enum values. TypeScript ensures that all validUnitLongvalues have corresponding entries in the map, making undefined access impossible at runtime. The pattern is used consistently throughout the codebase without guards.Likely an incorrect or invalid review comment.
|
given the latest changes, I tested moving again (on l4_sample) |
|
@philippotto I think removing and rerequesting your feedback again didnt work. do you mind having a look at this PR? in my view its ready for review |
philippotto
left a comment
philippotto
left a comment
There was a problem hiding this comment.
great, looks good 👍 thanks for the overall cleanup, too :)
URL of deployed dev instance (used for testing):
Steps to test:
context
discussion about tooltip in navbar: https://scm.slack.com/archives/C5AKLAV0B/p1764771532133509
Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)