Refactored several React class components as functional components #8750
Conversation
📝 WalkthroughWalkthroughSeveral React components were refactored from class-based implementations to functional components using React hooks. Lifecycle methods and state management were replaced by hooks such as Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (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)
✨ 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: 3
🧹 Nitpick comments (6)
frontend/javascripts/components/loop.ts (1)
9-19: LGTM: Correct useEffect implementation, but consider callback stability.The
useEffecthook correctly manages the interval lifecycle. However, includingonTickin the dependency array could cause unnecessary effect re-runs if the parent component recreates this function on every render.Consider using
useCallbackin the parent component for theonTickprop to prevent unnecessary interval resets.frontend/javascripts/libs/shortcut_component.ts (1)
11-28: LGTM: Correct useEffect implementation with proper cleanup.The
useEffecthook correctly manages the keyboard instance lifecycle with proper cleanup. The dependency array includes all relevant props to ensure the effect re-runs when needed.Similar to the Loop component, consider using
useCallbackfor theonTriggerprop in parent components to prevent unnecessary keyboard instance recreations.frontend/javascripts/components/select_experience_domain.tsx (1)
18-83: LGTM: Correct hooks implementation with proper state and effect management.The refactoring correctly uses
useStatefor state management anduseEffectfor data fetching. The logic maintains the same behavior as the original class component.Consider optimizing the
getUnusedDomainsfunction withuseCallbackto prevent unnecessary recreations on every render:+import { useCallback } from "react"; - const getUnusedDomains = (): ExperienceDomainList => { + const getUnusedDomains = useCallback((): ExperienceDomainList => { return domains.filter((domain) => !alreadyUsedDomains.includes(domain)); - }; + }, [domains, alreadyUsedDomains]);frontend/javascripts/components/fixed_expandable_table.tsx (1)
18-24: Improve type safety in getAllRowIds functionThe function uses
anytypes which reduces type safety. Consider using generics to maintain type information.-const getAllRowIds = ( - dataSource: readonly any[] | undefined, - rowKey: string | number | symbol | GetRowKey<any> | undefined, -) => { - const canUseRowKey = typeof rowKey === "string"; - return dataSource != null && canUseRowKey ? dataSource.map((row) => row[rowKey]) : []; -}; +const getAllRowIds = <RecordType = any>( + dataSource: readonly RecordType[] | undefined, + rowKey: string | number | symbol | GetRowKey<RecordType> | undefined, +): React.Key[] => { + const canUseRowKey = typeof rowKey === "string"; + return dataSource != null && canUseRowKey ? dataSource.map((row) => row[rowKey]) : []; +};frontend/javascripts/components/text_with_description.tsx (2)
59-59: Remove unnecessary type assertionThe type assertion can be avoided by using proper TypeScript discriminated union handling.
- <EditableTextLabel {...(editableProps as EditableTextLabelProp)} /> + <EditableTextLabel {...(isEditable ? props : {} as EditableTextLabelProp)} />Or better yet, restructure the component to avoid the need for type assertions entirely.
67-73: Eliminate repetitive type assertionsThe repeated
(props as NonEditableProps)assertions can be simplified by extracting the non-editable props once.+ {(() => { + if (isEditable) { + return <EditableTextLabel {...props} />; + } + const nonEditableProps = props as NonEditableProps; + return ( <span style={{ margin: "0 10px", display: "inline-block", }} > - {(props as NonEditableProps).markdown ? ( + {nonEditableProps.markdown ? ( <span> - <Markdown>{(props as NonEditableProps).value}</Markdown> + <Markdown>{nonEditableProps.value}</Markdown> </span> ) : ( - (props as NonEditableProps).value + nonEditableProps.value )} </span> + ); + })()}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/javascripts/components/disable_generic_dnd.ts(1 hunks)frontend/javascripts/components/fixed_expandable_table.tsx(2 hunks)frontend/javascripts/components/highlightable_row.tsx(1 hunks)frontend/javascripts/components/loop.ts(1 hunks)frontend/javascripts/components/select_experience_domain.tsx(2 hunks)frontend/javascripts/components/text_with_description.tsx(2 hunks)frontend/javascripts/libs/shortcut_component.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8221
File: CHANGELOG.unreleased.md:14-14
Timestamp: 2024-11-25T14:52:23.201Z
Learning: For the `webknossos` project, changelog entries in `CHANGELOG.unreleased.md` should be concise and avoid unnecessary detailed explanations.
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/libs/shortcut_component.ts (1)
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/components/disable_generic_dnd.ts (1)
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/components/highlightable_row.tsx (1)
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/components/select_experience_domain.tsx (1)
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.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (11)
frontend/javascripts/components/disable_generic_dnd.ts (2)
4-6: LGTM: Good practice moving the event handler outside the component.Moving the
preventDefaultfunction outside the component is optimal since it doesn't depend on props or state, preventing unnecessary recreations.
8-20: LGTM: Correct useEffect implementation for event listener management.The
useEffecthook properly replaces the lifecycle methods, correctly adding event listeners on mount and removing them on unmount.frontend/javascripts/components/highlightable_row.tsx (1)
14-28: LGTM: Correct useState implementation maintaining original behavior.The
useStatehook correctly replaces the class state, properly initializingpersistedShouldHighlightwith the initialshouldHighlightprop value. This maintains the same behavior as the original class component where the highlight state was persisted from the initial render.frontend/javascripts/components/select_experience_domain.tsx (1)
85-88: LGTM: Correct default props assignment for functional component.The default props are correctly assigned after the functional component definition, maintaining the same default behavior as the original class component.
frontend/javascripts/components/fixed_expandable_table.tsx (6)
3-3: LGTM: Proper hook import addedThe addition of
useEffectto the React import is correct for the functional component refactoring.
16-16: Good performance optimization with EMPTY_ARRAY constantUsing a constant empty array prevents unnecessary re-renders by maintaining referential equality.
26-33: LGTM: Clean functional component signatureThe destructuring of props with proper TypeScript generics is well implemented.
34-34: LGTM: Proper useState initializationUsing the EMPTY_ARRAY constant is a good optimization for state initialization.
36-39: LGTM: Correct useEffect replacement for componentDidUpdateThe useEffect properly replaces the lifecycle method to reset expanded rows when dataSource changes. The biome-ignore comment is appropriate since
setExpandedRowsis stable and doesn't need to be in the dependency array.
69-81: LGTM: Proper Table component renderingThe Table component is correctly rendered with all necessary props passed through, maintaining the same behavior as the class component.
frontend/javascripts/components/text_with_description.tsx (1)
31-78: LGTM: Functional component logic preserves original behaviorThe refactored component maintains the same rendering logic and behavior as the original class component, with proper conditional rendering and styling.
| const TextWithDescription: React.FC<Props> = (props) => { | ||
| const { isEditable, description, ...editableProps } = props; | ||
| const hasDescription = description !== ""; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve discriminated union type handling
The current destructuring doesn't work well with discriminated unions. Consider using a type guard or conditional logic to handle the different prop types more safely.
-const TextWithDescription: React.FC<Props> = (props) => {
- const { isEditable, description, ...editableProps } = props;
- const hasDescription = description !== "";
+const TextWithDescription: React.FC<Props> = (props) => {
+ const { isEditable, description } = props;
+ const hasDescription = description !== "";Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/javascripts/components/text_with_description.tsx around lines 19 to
21, the destructuring of props does not safely handle discriminated union types.
Refactor the code to use a type guard or conditional checks to differentiate
between the union types before accessing their specific properties. This will
ensure type safety and prevent runtime errors by handling each variant of the
props appropriately.
| @@ -1,6 +1,6 @@ | |||
| import { Popover, Tooltip } from "antd"; | |||
| import Markdown from "libs/markdown_adapter"; | |||
| import * as React from "react"; | |||
| import type React from "react"; | |||
There was a problem hiding this comment.
Fix React import for runtime usage
The type-only import will cause a runtime error since React.FC<Props> requires React at runtime.
-import type React from "react";
+import React from "react";📝 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.
| import type React from "react"; | |
| -import type React from "react"; | |
| +import React from "react"; |
🤖 Prompt for AI Agents
In frontend/javascripts/components/text_with_description.tsx at line 3, the
import of React is currently type-only, which causes runtime errors because
React.FC<Props> needs React at runtime. Change the import from a type-only
import to a regular import so that React is available during runtime.
| allowCreation, | ||
| alreadyUsedDomains = [], | ||
| }) => { | ||
| const [domains, setDomains] = React.useState<ExperienceDomainList>([]); |
There was a problem hiding this comment.
In this file I would also remove the React. in front of the hooks
| const fetchData = async () => { | ||
| const existingDomains = await getExistingExperienceDomains(); | ||
| setDomains(existingDomains); | ||
| }; | ||
| void fetchData(); | ||
| }, []); |
There was a problem hiding this comment.
maybe use useGuardedFetch instead?
There was a problem hiding this comment.
I think we should converged on a standard method for fetching. A while back Philipp introduced TanStack Query and that seems an excellent, and well documented solution by the broader React community. I think I will use that everywhere going forward.
| SelectExperienceDomain.defaultProps = { | ||
| alreadyUsedDomains: [], | ||
| allowCreation: false, | ||
| }; |
There was a problem hiding this comment.
this cna be removed. According to the props allowCreation is required and the optional alreadyUsedDomains already has a default value
Co-authored-by: MichaelBuessemeyer <39529669+MichaelBuessemeyer@users.noreply.github.com>
Co-authored-by: MichaelBuessemeyer <39529669+MichaelBuessemeyer@users.noreply.github.com>
…factor_components
|
@MichaelBuessemeyer thank your for the review I addressed all your feedback and made the appropriate changes. Ready to launch. |
|
@MichaelBuessemeyer Ping |
In this PR I converted several class components to functional components using modern React hooks. Most of the components are located in the general
frontend/javascripts/componentsdirectory.Steps to test:
Check that the view are still showing
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)