Refactor left side bar components to React functional API #8755
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis update refactors three React components—Histogram, DatasetSettings, and MappingSettingsView—from class-based components with Redux Changes
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 (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…actor-left-sidebar
![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
🔭 Outside diff range comments (2)
frontend/javascripts/viewer/view/left-border-tabs/histogram_view.tsx (1)
86-385: Use Antd Slider’s onAfterChange instead of onChangeCompleteAntd’s Slider API doesn’t recognize
onChangeComplete; it usesonAfterChange. Update the histogram view so your callback fires correctly.• In
frontend/javascripts/viewer/view/left-border-tabs/histogram_view.tsx(around line 298)
Replace- onChangeComplete={onThresholdChange} + onAfterChange={onThresholdChange}(and remove the obsolete
onChangeCompleteprop)frontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsx (1)
952-964: Verify Slider Wrapper ImplementationsI inspected the full implementation of the two wrapper components:
- Both
NumberSliderSettingandLogSliderSettingrender an Ant DesignSliderbut only wire up the slider’sonAfterChangehandler to their internal change methods (_onChange/onChangeInput), leavingonChangeunused. This means dragging the slider won’t fire updates until you release the thumb.Actionable fix:
• In both wrappers (frontend/javascripts/viewer/view/components/setting_input_views.tsx), bind the slider’sonChangeprop to the internal handler instead of (or in addition to)onAfterChange.Example diff for
LogSliderSetting:- return ( - <Slider - min={this.props.min} - max={this.props.max} - step={this.props.step} - defaultValue={this.props.defaultValue} - onAfterChange={this.onChangeInput} - disabled={this.props.disabled} - // ... - /> - ); + return ( + <Slider + min={this.props.min} + max={this.props.max} + step={this.props.step} + defaultValue={this.props.defaultValue} + onChange={this.onChangeInput} // fire continuously as thumb moves + onAfterChange={this.onChangeInput} // keep release behavior if needed + disabled={this.props.disabled} + // ... + /> + );Repeat the same adjustment in
NumberSliderSetting. This ensures the UI reacts immediately when dragging.
🧹 Nitpick comments (3)
frontend/javascripts/viewer/view/left-border-tabs/histogram_view.tsx (1)
205-207: Consider optimizing canvas updatesThe
useEffectwithout a dependency array will runupdateCanvason every render. While this works, it could be optimized by specifying the actual dependencies that should trigger a canvas update.Consider adding dependencies to avoid unnecessary canvas redraws:
useEffect(() => { updateCanvas(); - }); + }, [updateCanvas, intensityRangeMin, intensityRangeMax]);frontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsx (2)
337-443: Consider breaking down this large componentWhile the refactoring from class to functional component is well done, this component is quite large (1600+ lines). Consider extracting some of the logic into custom hooks or smaller components for better maintainability.
Consider extracting:
- Layer management logic into a custom hook (e.g.,
useLayerManagement)- Volume tracing logic into
useVolumeTracing- The various button/menu item generators into a separate utility file
This would make the component more maintainable and easier to test.
962-963: Consider replacing _.partial with arrow functions for clarityWhile
_.partialworks, using arrow functions would be more idiomatic in modern React and clearer to read.Replace:
- onChange={_.partial(onChangeLayer, layerName, "gammaCorrectionValue")} + onChange={(value) => onChangeLayer(layerName, "gammaCorrectionValue", value)}This pattern appears multiple times throughout the component and could be updated for consistency and readability.
Also applies to: 977-977, 1029-1029, 1110-1110, 1350-1351, 1359-1360, 1369-1369, 1376-1377, 1381-1381, 1387-1387
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/javascripts/viewer/view/left-border-tabs/histogram_view.tsx(4 hunks)frontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsx(42 hunks)frontend/javascripts/viewer/view/left-border-tabs/mapping_settings_view.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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/left-border-tabs/histogram_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.
frontend/javascripts/viewer/view/left-border-tabs/mapping_settings_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.
frontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsx (8)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#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`.
Learnt from: knollengewaechs
PR: scalableminds/webknossos#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.
Learnt from: dieknolle3333
PR: scalableminds/webknossos#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: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/volumetracing_saga.tsx:433-434
Timestamp: 2024-11-22T17:19:07.947Z
Learning: In the codebase, certain usages of `segmentationLayer.resolutions` are intentionally retained and should not be changed to `segmentationLayer.mags` during refactoring.
Learnt from: frcroth
PR: scalableminds/webknossos#8609
File: app/models/dataset/Dataset.scala:753-775
Timestamp: 2025-05-12T13:07:29.637Z
Learning: In the `updateMags` method of DatasetMagsDAO (Scala), the code handles different dataset types distinctly:
1. Non-WKW datasets have `magsOpt` populated and use the first branch which includes axisOrder, channelIndex, and credentialId.
2. WKW datasets will have `wkwResolutionsOpt` populated and use the second branch which includes cubeLength.
3. The final branch is a fallback for legacy data.
This ensures appropriate fields are populated for each dataset type.
Learnt from: frcroth
PR: scalableminds/webknossos#8609
File: conf/evolutions/133-datasource-properties-in-db.sql:8-16
Timestamp: 2025-05-12T14:15:05.259Z
Learning: The database schema in WEBKNOSSOS has separate tables for dataset layers (`dataset_layers`) and magnifications (`dataset_mags`). The `dataFormat` field is stored in the layers table while magnification-specific fields like `cubeLength` (specific to WKW format) are stored in the mags table.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8485
File: frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts:384-392
Timestamp: 2025-04-01T09:45:17.527Z
Learning: The function `isRotationAndMirrorMaybeOnly` in the dataset_layer_transformation_accessor.ts is intentionally designed to allow mirroring transformations (negative scale values). It uses the length comparison (`scale.length() === NON_SCALED_VECTOR.length()`) rather than component equality to permit mirrored axes while ensuring the overall scale magnitude remains the same.
Learnt from: dieknolle3333
PR: scalableminds/webknossos#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.
🔇 Additional comments (3)
frontend/javascripts/viewer/view/left-border-tabs/mapping_settings_view.tsx (2)
47-87: Clean refactoring to functional componentThe conversion from class component to functional component is well executed. The hooks are used appropriately:
useWkSelectorcorrectly replaces mapStateToPropsuseCallbackhas the correct dependenciesuseEffectproperly replaces the lifecycle methods
88-224: Component logic well preservedThe event handlers and rendering logic have been successfully adapted to the functional component pattern while preserving the original behavior. The use of local variables instead of props/state references is clean and readable.
frontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsx (1)
1569-1641: Successful refactoring of complex componentThe conversion of this large component from class-based to functional is well executed. All lifecycle methods have been properly replaced with hooks, and the Redux integration has been successfully updated to use hooks.
| onChange={(value) => { | ||
| // @ts-expect-error ts-migrate(2345) FIXME: Argument of type 'number' is not assignable to par... Remove this comment to see the full error message | ||
| value = Number.parseFloat(value); | ||
|
|
||
| if (value <= maxRange) { | ||
| setCurrentMin(value); | ||
| updateMinimumDebounced(value, layerName); | ||
| } | ||
| }} |
There was a problem hiding this comment.
Remove type error comments and handle types properly
Instead of suppressing TypeScript errors, handle the InputNumber value types correctly.
Fix the type handling:
onChange={(value) => {
- // @ts-expect-error ts-migrate(2345) FIXME: Argument of type 'number' is not assignable to par... Remove this comment to see the full error message
- value = Number.parseFloat(value);
-
- if (value <= maxRange) {
- setCurrentMin(value);
- updateMinimumDebounced(value, layerName);
+ if (value !== null && value !== undefined) {
+ const numValue = typeof value === 'string' ? Number.parseFloat(value) : value;
+ if (!isNaN(numValue) && numValue <= maxRange) {
+ setCurrentMin(numValue);
+ updateMinimumDebounced(numValue, layerName);
+ }
}
}}Apply the same fix to the max value handler at lines 359-367.
Also applies to: 359-367
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/view/left-border-tabs/histogram_view.tsx around
lines 326 to 334 and 359 to 367, remove the @ts-expect-error comment and
properly handle the InputNumber value types by explicitly typing the event
parameter or value as a number or string, then safely converting it to a number
if needed. Ensure the onChange handlers correctly process the input without type
errors by refining the type definitions or adding appropriate type guards before
calling setCurrentMin or setCurrentMax and their respective update functions.
| const updateMinimumDebounced = useCallback( | ||
| debounce((value, layerName) => onChangeLayer(layerName, "min", value), 500), | ||
| [], | ||
| ); | ||
|
|
||
| updateMaximumDebounced = _.debounce( | ||
| (value, layerName) => this.props.onChangeLayer(layerName, "max", value), | ||
| 500, | ||
| const updateMaximumDebounced = useCallback( | ||
| debounce((value, layerName) => onChangeLayer(layerName, "max", value), 500), | ||
| [], | ||
| ); |
There was a problem hiding this comment.
Fix stale closure issue in debounced callbacks
The debounced callbacks have empty dependency arrays but reference onChangeLayer, which could lead to stale closures if onChangeLayer changes.
Update the dependency arrays:
const updateMinimumDebounced = useCallback(
- debounce((value, layerName) => onChangeLayer(layerName, "min", value), 500),
- [],
+ debounce((value, layerName) => onChangeLayer(layerName, "min", value), 500),
+ [onChangeLayer],
);
const updateMaximumDebounced = useCallback(
- debounce((value, layerName) => onChangeLayer(layerName, "max", value), 500),
- [],
+ debounce((value, layerName) => onChangeLayer(layerName, "max", value), 500),
+ [onChangeLayer],
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const updateMinimumDebounced = useCallback( | |
| debounce((value, layerName) => onChangeLayer(layerName, "min", value), 500), | |
| [], | |
| ); | |
| updateMaximumDebounced = _.debounce( | |
| (value, layerName) => this.props.onChangeLayer(layerName, "max", value), | |
| 500, | |
| const updateMaximumDebounced = useCallback( | |
| debounce((value, layerName) => onChangeLayer(layerName, "max", value), 500), | |
| [], | |
| ); | |
| const updateMinimumDebounced = useCallback( | |
| debounce((value, layerName) => onChangeLayer(layerName, "min", value), 500), | |
| [onChangeLayer], | |
| ); | |
| const updateMaximumDebounced = useCallback( | |
| debounce((value, layerName) => onChangeLayer(layerName, "max", value), 500), | |
| [onChangeLayer], | |
| ); |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/view/left-border-tabs/histogram_view.tsx around
lines 244 to 252, the debounced callbacks useCallback hooks have empty
dependency arrays but reference the onChangeLayer function, which can cause
stale closures if onChangeLayer changes. Fix this by adding onChangeLayer to the
dependency arrays of both useCallback hooks to ensure the latest version is used
in the debounced functions.
…ional style with hooks.
This PR converts several class components in the left side-bar of the annotation UI to React functional components.
The following components were refactored:
TODO
Issues:
Contributes to #8747
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)