Refactor action bar components to React functional components #8754
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit converts several class components in the action bar to functional components, enhancing code readability and maintainability. The following components were refactored: - DatasetPositionView - SaveButton - TracingActionsView - ViewModesView
hotzenklotz
changed the title
📝 WalkthroughWalkthroughSeveral React components in the frontend viewer's action bar were refactored from class-based components using Redux Changes
Suggested reviewers
Poem
✨ Finishing Touches
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: 2
🧹 Nitpick comments (4)
frontend/javascripts/viewer/view/action-bar/view_modes_view.tsx (1)
57-69: Memoize MENU_ITEMS to prevent unnecessary re-renders.The menu items are recreated on every render, which could cause performance issues.
+import { useCallback, useMemo } from "react"; -const MENU_ITEMS: MenuProps["items"] = [ +const MENU_ITEMS: MenuProps["items"] = useMemo(() => [ { key: "1", type: "group", label: "Select View Mode", children: ViewModeValues.map((mode) => ({ label: Utils.capitalize(mode), key: mode, disabled: isDisabled(mode), icon: <span style={{ marginRight: 8 }}>{VIEW_MODE_TO_ICON[mode]}</span>, })), }, -]; +], [allowedModes]);Also update the
isDisabledfunction:-const isDisabled = (mode: ViewMode) => { +const isDisabled = useCallback((mode: ViewMode) => { return !allowedModes.includes(mode); -}; +}, [allowedModes]);frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx (2)
42-60: Memoize event handlers to prevent unnecessary re-renders.The event handlers are recreated on every render, which could impact performance.
+import { useCallback } from "react"; -const copyPositionToClipboard = async () => { +const copyPositionToClipboard = useCallback(async () => { const position = V3.floor(getPosition(flycam)).join(", "); await navigator.clipboard.writeText(position); Toast.success("Position copied to clipboard"); -}; +}, [flycam]); -const copyRotationToClipboard = async () => { +const copyRotationToClipboard = useCallback(async () => { const rotation = V3.round(getRotation(flycam)).join(", "); await navigator.clipboard.writeText(rotation); Toast.success("Rotation copied to clipboard"); -}; +}, [flycam]); -const handleChangePosition = (position: Vector3) => { +const handleChangePosition = useCallback((position: Vector3) => { Store.dispatch(setPositionAction(position)); -}; +}, []); -const handleChangeRotation = (rotation: Vector3) => { +const handleChangeRotation = useCallback((rotation: Vector3) => { Store.dispatch(setRotationAction(rotation)); -}; +}, []);
62-91: Consider memoizing the position bounds checking function.The
isPositionOutOfBoundsfunction contains complex logic and is recreated on every render.-const isPositionOutOfBounds = (position: Vector3) => { +const isPositionOutOfBounds = useCallback((position: Vector3) => { const { min: datasetMin, max: datasetMax } = getDatasetExtentInVoxel(dataset); const isPositionOutOfBounds = (min: Vector3, max: Vector3) => position[0] < min[0] || position[1] < min[1] || position[2] < min[2] || position[0] >= max[0] || position[1] >= max[1] || position[2] >= max[2]; const isOutOfDatasetBounds = isPositionOutOfBounds(datasetMin, datasetMax); let isOutOfTaskBounds = false; if (task?.boundingBox) { const bbox = task.boundingBox; const bboxMax = [ bbox.topLeft[0] + bbox.width, bbox.topLeft[1] + bbox.height, bbox.topLeft[2] + bbox.depth, ]; // @ts-expect-error ts-migrate(2345) FIXME: Argument of type 'number[]' is not assignable to p... Remove this comment to see the full error message isOutOfTaskBounds = isPositionOutOfBounds(bbox.topLeft, bboxMax); } return { isOutOfDatasetBounds, isOutOfTaskBounds, }; -}; +}, [dataset, task]);frontend/javascripts/viewer/view/action-bar/tracing_actions_view.tsx (1)
524-540: Memoize event handlers to optimize performance.Several event handlers are recreated on every render. Consider memoizing them with
useCallback.-const handleSave = async (event?: React.MouseEvent<HTMLButtonElement>) => { +const handleSave = useCallback(async (event?: React.MouseEvent<HTMLButtonElement>) => { if (event != null) { // @ts-expect-error ts-migrate(2339) FIXME: Property 'blur' does not exist on type 'EventTarge... Remove this comment to see the full error message event.target.blur(); } Model.forceSave(); -}; +}, []); -const handleUndo = () => dispatchUndoAsync(Store.dispatch); -const handleRedo = () => dispatchRedoAsync(Store.dispatch); +const handleUndo = useCallback(() => dispatchUndoAsync(Store.dispatch), []); +const handleRedo = useCallback(() => dispatchRedoAsync(Store.dispatch), []); -const handleCopyToAccount = async () => { +const handleCopyToAccount = useCallback(async () => { // duplicates the annotation in the current user account const newAnnotation = await duplicateAnnotation(annotationId, annotationType); location.href = `/annotations/${newAnnotation.id}`; -}; +}, [annotationId, annotationType]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx(3 hunks)frontend/javascripts/viewer/view/action-bar/save_button.tsx(4 hunks)frontend/javascripts/viewer/view/action-bar/tracing_actions_view.tsx(8 hunks)frontend/javascripts/viewer/view/action-bar/view_modes_view.tsx(2 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/action-bar/view_modes_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/action-bar/dataset_position_view.tsx (2)
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: 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.
frontend/javascripts/viewer/view/action-bar/tracing_actions_view.tsx (2)
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.
🧬 Code Graph Analysis (1)
frontend/javascripts/viewer/view/action-bar/save_button.tsx (2)
frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)frontend/javascripts/components/fast_tooltip.tsx (1)
FastTooltip(54-123)
⏰ 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 (3)
frontend/javascripts/viewer/view/action-bar/save_button.tsx (1)
41-85: Well-executed refactor to functional component!The conversion preserves all functionality correctly:
- State management properly converted to hooks
- Polling mechanism correctly implemented with
useEffect- Memoization with
reuseInstanceOnEqualitymaintained- Clean dependency arrays
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx (1)
152-152: Verify the icon change for the rotation copy button.The icon was changed from a rotated pushpin to a reload icon. Please confirm this UI change is intentional.
frontend/javascripts/viewer/view/action-bar/tracing_actions_view.tsx (1)
490-522: Excellent implementation of timeout management with useRef!The timeout management correctly uses
useRefto ensure the timeout can be properly cleared and accessed within the effect. This follows the best practice noted in the retrieved learnings for native event handling.
Comment on lines
36
to
51
| const handleChange: MenuProps["onClick"] = (args) => { | ||
| if (!ViewModeValues.includes(args.key as ViewMode)) { | ||
| return; | ||
| } | ||
| const mode = args.key as ViewMode; | ||
| // If we switch back from any arbitrary mode we stop recording. | ||
| // This prevents that when the user switches back to any arbitrary mode, | ||
| // a new node is instantly created at the screen's center. | ||
| if ( | ||
| constants.MODES_ARBITRARY.includes(this.props.viewMode) && | ||
| mode === constants.MODE_PLANE_TRACING | ||
| ) { | ||
| this.props.onChangeFlightmodeRecording(false); | ||
| if (constants.MODES_ARBITRARY.includes(viewMode) && mode === constants.MODE_PLANE_TRACING) { | ||
| onChangeFlightmodeRecording(false); | ||
| } | ||
|
|
||
| Store.dispatch(setViewModeAction(mode)); | ||
| // @ts-expect-error ts-migrate(2339) FIXME: Property 'blur' does not exist on type 'Element'. | ||
| args.domEvent.target.blur(); | ||
| }; |
There was a problem hiding this comment.
Use consistent dispatch method throughout the component.
The function uses Store.dispatch directly instead of the dispatch hook obtained from useDispatch(). This breaks the pattern established by the refactor.
Apply this diff to fix the inconsistent dispatch usage:
-const handleChange: MenuProps["onClick"] = (args) => {
+const handleChange: MenuProps["onClick"] = useCallback((args) => {
if (!ViewModeValues.includes(args.key as ViewMode)) {
return;
}
const mode = args.key as ViewMode;
// If we switch back from any arbitrary mode we stop recording.
// This prevents that when the user switches back to any arbitrary mode,
// a new node is instantly created at the screen's center.
if (constants.MODES_ARBITRARY.includes(viewMode) && mode === constants.MODE_PLANE_TRACING) {
onChangeFlightmodeRecording(false);
}
- Store.dispatch(setViewModeAction(mode));
+ dispatch(setViewModeAction(mode));
// @ts-expect-error ts-migrate(2339) FIXME: Property 'blur' does not exist on type 'Element'.
args.domEvent.target.blur();
-};
+}, [viewMode, onChangeFlightmodeRecording, dispatch]);📝 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.
Suggested change
| const handleChange: MenuProps["onClick"] = (args) => { | |
| if (!ViewModeValues.includes(args.key as ViewMode)) { | |
| return; | |
| } | |
| const mode = args.key as ViewMode; | |
| // If we switch back from any arbitrary mode we stop recording. | |
| // This prevents that when the user switches back to any arbitrary mode, | |
| // a new node is instantly created at the screen's center. | |
| if ( | |
| constants.MODES_ARBITRARY.includes(this.props.viewMode) && | |
| mode === constants.MODE_PLANE_TRACING | |
| ) { | |
| this.props.onChangeFlightmodeRecording(false); | |
| if (constants.MODES_ARBITRARY.includes(viewMode) && mode === constants.MODE_PLANE_TRACING) { | |
| onChangeFlightmodeRecording(false); | |
| } | |
| Store.dispatch(setViewModeAction(mode)); | |
| // @ts-expect-error ts-migrate(2339) FIXME: Property 'blur' does not exist on type 'Element'. | |
| args.domEvent.target.blur(); | |
| }; | |
| const handleChange: MenuProps["onClick"] = useCallback((args) => { | |
| if (!ViewModeValues.includes(args.key as ViewMode)) { | |
| return; | |
| } | |
| const mode = args.key as ViewMode; | |
| // If we switch back from any arbitrary mode we stop recording. | |
| // This prevents that when the user switches back to any arbitrary mode, | |
| // a new node is instantly created at the screen's center. | |
| if (constants.MODES_ARBITRARY.includes(viewMode) && mode === constants.MODE_PLANE_TRACING) { | |
| onChangeFlightmodeRecording(false); | |
| } | |
| dispatch(setViewModeAction(mode)); | |
| // @ts-expect-error ts-migrate(2339) FIXME: Property 'blur' does not exist on type 'Element'. | |
| args.domEvent.target.blur(); | |
| }, [viewMode, onChangeFlightmodeRecording, dispatch]); |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/view/action-bar/view_modes_view.tsx between lines
36 and 51, replace the direct use of Store.dispatch with the dispatch function
obtained from the useDispatch hook to maintain consistent dispatch usage
throughout the component. Locate where Store.dispatch(setViewModeAction(mode))
is called and change it to dispatch(setViewModeAction(mode)) after ensuring
dispatch is properly initialized via useDispatch at the component level.
frontend/javascripts/viewer/view/action-bar/tracing_actions_view.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/viewer/view/action-bar/tracing_actions_view.tsx (1)
544-544: Consider using hook-selected state for consistency.The handlers are still using
Store.getState()directly instead of hook-selected state, which is inconsistent with the overall refactor approach.For consistency with the functional component pattern, consider adding these state selections at the component level:
+ const annotation = useWkSelector((state) => state.annotation); + const dataset = useWkSelector((state) => state.dataset);Then update the handlers to use the hook-selected state:
- const { annotation: sandboxAnnotation, dataset } = Store.getState(); + const sandboxAnnotation = annotation; // ... and similar for other Store.getState() callsAlso applies to: 561-561
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/viewer/view/action-bar/tracing_actions_view.tsx(8 hunks)frontend/javascripts/viewer/view/action-bar/view_modes_view.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/viewer/view/action-bar/view_modes_view.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 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/action-bar/tracing_actions_view.tsx (3)
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.
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`.
⏰ 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 (5)
frontend/javascripts/viewer/view/action-bar/tracing_actions_view.tsx (5)
45-47: LGTM! Proper imports for functional component refactor.The imports correctly include the necessary hooks (
useWkSelector,useCallback,useEffect,useState) for the functional component implementation.
467-489: Excellent hook-based state selection implementation.All state selections are properly implemented using
useWkSelectorhook instead ofconnectandmapStateToProps, following React hooks best practices.
491-523: Well-implemented state management with hooks.The
useStateanduseEffecthooks properly replace the class component lifecycle methods. The timeout cleanup in the effect's return function correctly prevents memory leaks.
600-668: Excellent useCallback implementation addresses previous review feedback.The
getTracingViewModalsfunction has been properly converted to auseCallbackhook with comprehensive dependencies, and now uses hook-selectedviewModestate instead of direct Store access. This resolves the previous review comment about direct Store.getState() usage.
783-801: Clean functional component return structure.The JSX structure maintains the same UI behavior while properly using the hook-selected state and local handlers. The component follows React functional component best practices.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR converts several class components in the action bar to React functional components.
The following components were refactored:
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)