Dataset Settings: Refactor as React functional components #8719
Conversation
…nts to functional components
📝 WalkthroughWalkthroughThis update refactors several dataset settings components in the frontend from class-based or higher-order-component (HOC) patterns to modern React functional components using hooks. It introduces a custom Changes
Possibly related PRs
Suggested labels
Poem
📜 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 (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 Review! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@hotzenklotz I'll review this PR for you! Let me gather information about the changes first. 🏁 Script executed: Length of output: 2349 Let me examine the actual diff content to see the refactoring changes: 🏁 Script executed: Length of output: 704 Let me get the full diff content to examine the refactoring: 🏁 Script executed: Length of output: 46894 @hotzenklotz I've reviewed this comprehensive refactoring of dataset settings components from class components to functional components. This is excellent modernization work! Here's my detailed analysis: 🎯 Overall Assessment: Excellent RefactoringThis is a well-executed conversion that properly modernizes the React components using current best practices. The code is cleaner, more readable, and follows modern React patterns. ✅ What's Done WellDatasetSettingsView Component
HOC Replacement
Custom Hooks Usage
🔍 Potential Areas for Attention1. useEffect Dependencies// In DatasetSettingsView - this looks correct but worth double-checking
useEffect(() => {
fetchData();
sendAnalyticsEvent("open_dataset_settings", {
datasetName: dataset ? dataset.name : "Not found dataset",
});
}, [fetchData]); // dataset dependency removed to avoid infinite loopThe comment indicates 2. Form Reference PatternThe component now uses 3. Force Update Replacement// In handleSubmit - clever solution for force update in functional components
setActiveDataSourceEditMode((prev) => prev); // Force re-render
setTimeout(afterForceUpdateCallback, 0);This is a creative solution to replace 💡 Minor Suggestions1. Consider Extracting Custom HookThe beforeunload logic in // Could create useBeforeUnload hook
const useBeforeUnload = (hasUnsavedChanges: boolean, message: string) => {
// beforeunload logic here
};2. Memoization OpportunityConsider memoizing the tabs array in const tabs = useMemo(() => [
// tab definitions
], [activeTabKey, formErrors, dataset, /* other dependencies */]);📋 Code Quality Highlights✅ Proper Hook Rules: All hooks are called at the top level and conditionally 🚀 Technical Debt ReductionThis refactoring significantly reduces technical debt by:
✅ ChangelogThe changelog entry has a small typo: "Reactored" should be "Refactored": - Reactored dataset settings and child tabs as React functional components.
+ Refactored dataset settings and child tabs as React functional components.Overall: This is exemplary refactoring work that modernizes the codebase while maintaining functionality. Great job! 🎉 The code looks so much cleaner now! This refactoring really showcases modern React best practices. |
…" in dataset configuration prompt.
…ctional_dataset_settings
…ctional_dataset_settings
![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 (4)
frontend/javascripts/dashboard/dataset/useBeforeUnload_hook.ts (1)
34-37: Consider simplifying the timeout logic.The current approach temporarily disables the
window.onbeforeunloadhandler and re-enables it after 500ms. This adds complexity and potential race conditions.Consider a simpler approach that avoids the timeout mechanism:
- window.onbeforeunload = null; - blockTimeoutIdRef.current = window.setTimeout(() => { - // @ts-ignore - window.onbeforeunload = beforeUnload; - }, 500); + // Return the message directly without manipulating window.onbeforeunloadThe timeout appears to be working around duplicate prompts, but this might be better handled by checking the navigation state or using a flag to track whether a prompt is already showing.
frontend/javascripts/dashboard/dataset/team_selection_component.tsx (1)
81-81: Simplify the disabled prop.The ternary operator is redundant here.
- disabled={disabled ? disabled : false} + disabled={disabled}frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (2)
375-380: Document the force update workaround for clarity.The pattern of using
setActiveDataSourceEditMode((prev) => prev)withsetTimeoutto force a re-render is clever but non-obvious. Consider adding a more detailed comment.// Need to force update of the SimpleAdvancedDataForm as removing a layer in the advanced tab does not update // the form items in the simple tab (only the values are updated). The form items automatically update once // the simple tab renders, but this is not the case when the user directly submits the changes. - // In functional components, we can trigger a re-render by updating state - setActiveDataSourceEditMode((prev) => prev); // Force re-render + // In functional components, we can trigger a re-render by updating state. + // This setState call with the same value forces React to re-render the component, + // which is necessary to update the form items before validation. + setActiveDataSourceEditMode((prev) => prev); setTimeout(afterForceUpdateCallback, 0);
503-576: Consider memoizing the tabs array for performance.The tabs array is recreated on every render. Since it contains complex JSX and conditional logic, memoizing it could improve performance.
Add at the top of the component:
import { useMemo } from "react";Then wrap the tabs array:
- const tabs = [ + const tabs = useMemo(() => [ { label: <span> Data {formErrors.data ? errorIcon : ""}</span>, key: "data", // ... rest of tabs }, // ... other tabs - ]; + ], [formErrors, errorIcon, activeTabKey, dataset, form, activeDataSourceEditMode, + handleDataSourceEditModeChange, datasetId, isUserAdmin, maybeDataSourceId]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/javascripts/dashboard/dataset/dataset_settings_delete_tab.tsx(2 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_sharing_tab.tsx(1 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx(10 hunks)frontend/javascripts/dashboard/dataset/team_selection_component.tsx(1 hunks)frontend/javascripts/dashboard/dataset/useBeforeUnload_hook.ts(1 hunks)frontend/javascripts/messages.tsx(1 hunks)unreleased_changes/8719.md(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 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/messages.tsx (1)
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`.
frontend/javascripts/dashboard/dataset/dataset_settings_sharing_tab.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/dashboard/dataset/useBeforeUnload_hook.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/dashboard/dataset/team_selection_component.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.
frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (1)
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.
🧬 Code Graph Analysis (1)
frontend/javascripts/dashboard/dataset/useBeforeUnload_hook.ts (1)
frontend/javascripts/admin/dataset/dataset_upload_view.tsx (1)
unblockHistory(269-280)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (11)
frontend/javascripts/dashboard/dataset/useBeforeUnload_hook.ts (1)
31-31: Clarify theaction === undefinedcondition.The condition
action === undefinedseems unusual since history actions should typically be defined (PUSH, REPLACE, POP). This might indicate an edge case or potential bug.Run this script to check how the history.block callback is used elsewhere in the codebase:
#!/bin/bash # Search for other uses of history.block to understand expected action values rg -A 5 -B 2 'history\.block' # Also search for beforeUnload patterns rg -A 10 'beforeUnload.*action.*undefined'frontend/javascripts/messages.tsx (1)
336-336: LGTM! Improved user experience with clearer button text.Changing "Cancel" to "Ok" better reflects the action of discarding unsaved changes, making the user interface more intuitive.
unreleased_changes/8719.md (1)
1-2: LGTM! Clear and accurate changelog entry.The changelog entry accurately summarizes the refactoring work performed in this PR.
frontend/javascripts/dashboard/dataset/dataset_settings_delete_tab.tsx (1)
4-4: Excellent refactoring to modern React patterns!The conversion from class/HOC patterns to functional components with hooks significantly improves code readability and maintainability:
useFetchhook simplifies dataset fetching and state managementuseHistoryhook eliminates the need forwithRouterHOC- Functional component syntax is more concise and easier to understand
Also applies to: 7-8, 15-20
frontend/javascripts/dashboard/dataset/dataset_settings_sharing_tab.tsx (1)
9-9: Excellent modernization removing HOC dependencies!The refactoring successfully eliminates HOC patterns in favor of direct hook usage:
useWkSelectorhook provides direct Redux store access, removing the need forconnectHOC- Functional component syntax improves readability
- Core functionality and UI remain unchanged
This aligns well with modern React best practices and the overall modernization effort in this PR.
Also applies to: 25-27
frontend/javascripts/dashboard/dataset/team_selection_component.tsx (3)
10-17: LGTM! Good naming convention for component props.The rename from
PropstoTeamSelectionComponentPropsfollows React TypeScript best practices for more descriptive type names.
27-39: Excellent conversion of lifecycle methods to hooks!The state initialization and effect hooks are properly implemented:
useEffectwith[value]dependency correctly syncs state with prop changesuseEffectOnlyOnceappropriately replacescomponentDidMountfor initial data fetch
55-57: Good memoization of getAllTeams function!Proper use of
useCallbackwith correct dependencies prevents unnecessary re-renders in child components.frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (3)
65-73: Excellent modernization to functional component with hooks!Great job replacing HOCs with hooks:
useWkSelectorfor Redux state accessuseQueryClientfor React Query cache managementForm.useForm()for modern Ant Design form handling
405-411: Good handling of the infinite loop prevention!The intentional exclusion of
datasetfrom the dependency array with clear documentation is the correct approach here. The effect correctly accesses the latestdatasetvalue when needed without causing re-execution loops.
65-620: Outstanding refactoring work!This is an exemplary conversion from class to functional component:
- ✅ Proper use of all React hooks with correct dependencies
- ✅ Clean state management with multiple
useStatecalls- ✅ Appropriate memoization with
useCallback- ✅ Modern form handling with
Form.useForm()- ✅ Excellent integration of custom hooks like
useBeforeUnload- ✅ Maintains all original functionality while improving code quality
The code is now more maintainable and aligns with modern React best practices.
| // @ts-ignore | ||
| window.onbeforeunload = beforeUnload; |
There was a problem hiding this comment.
Remove TypeScript suppressions and fix type issues.
The @ts-ignore comments indicate unresolved TypeScript issues that should be addressed rather than suppressed. These suppressions can hide real type safety problems.
For line 36, the issue is likely that beforeUnload expects specific types but window.onbeforeunload has a different signature. Consider creating a properly typed wrapper function:
- // @ts-ignore
- window.onbeforeunload = beforeUnload;
+ window.onbeforeunload = (event: BeforeUnloadEvent) => {
+ if (hasUnsavedChanges) {
+ event.preventDefault();
+ return message;
+ }
+ };For line 45, create a separate function for the window beforeunload handler instead of reusing the history block callback.
Also applies to: 45-46
🤖 Prompt for AI Agents
In frontend/javascripts/dashboard/dataset/useBeforeUnload_hook.ts around lines
35-36 and 45-46, remove the @ts-ignore comments by addressing the type mismatch
between the beforeUnload function and window.onbeforeunload. Create a properly
typed wrapper function that matches the expected signature of
window.onbeforeunload and assign that wrapper to window.onbeforeunload.
Additionally, for lines 45-46, define a separate function for the window
beforeunload handler instead of reusing the history block callback to ensure
correct typing and separation of concerns.
| import { useCallback, useEffect, useRef } from "react"; | ||
| import { useHistory } from "react-router-dom"; | ||
|
|
||
| const useBeforeUnload = (hasUnsavedChanges: boolean, message: string) => { |
There was a problem hiding this comment.
the old code had some comments (which are needed in my opinion because the code is not straightforward):
// Only show the prompt if this is a proper beforeUnload event from the browser
// or the pathname changed
// This check has to be done because history.block triggers this function even if only the url hash changed
...
// clear the event handler otherwise it would be called twice. Once from history.block once from the beforeunload event
...
// restore the event handler in case a user chose to stay on the page
please restore them (or some adapted version of it).
| if (this.props.onChange) { | ||
| this.props.onChange(Array.isArray(selectedTeamIdsOrId) ? selectedTeams : selectedTeams[0]); | ||
| const allTeams = getAllTeams(); | ||
| const selected = _.compact(selectedTeamIds.map((id) => allTeams.find((t) => t.id === id))); |
There was a problem hiding this comment.
I find selectedTeams clearer 🤔
| onChange={onSelectTeams} | ||
| value={selectedTeams.map((t) => t.id)} | ||
| filterOption | ||
| disabled={disabled ? disabled : false} |
There was a problem hiding this comment.
| disabled={disabled ? disabled : false} | |
| disabled={disabled ?? false} |
| setPossibleTeams(possibleTeams); | ||
| setIsFetchingData(false); | ||
| if (afterFetchedTeams) { | ||
| afterFetchedTeams(possibleTeams); | ||
| } | ||
| } catch (_exception) { | ||
| console.error("Could not load teams."); |
There was a problem hiding this comment.
it's independent of the functional refactoring, but I'd use Toast.error here and setIsFetchingData should also be called with false to avoid an infinite loading state.
| const unblockRef = useRef<UnregisterCallback | null>(null); | ||
| const blockTimeoutIdRef = useRef<number | null>(null); | ||
|
|
||
| const unblockHistory = useCallback(() => { |
There was a problem hiding this comment.
I think, this can be deleted completely. the refs are only set to null (and never anything else) and the unblock stuff should happen by the new useBeforeUnload hook once the component is unmounted (which should happen on cancel).
| // Need to force update of the SimpleAdvancedDataForm as removing a layer in the advanced tab does not update | ||
| // the form items in the simple tab (only the values are updated). The form items automatically update once | ||
| // the simple tab renders, but this is not the case when the user directly submits the changes. | ||
| this.forceUpdate(afterForceUpdateCallback); | ||
| }; | ||
|
|
||
| submit = async (formValues: FormData) => { | ||
| const { dataset, datasetDefaultConfiguration } = this.state; | ||
| const datasetChangeValues = { ...formValues.dataset }; | ||
|
|
||
| if (datasetChangeValues.sortingKey != null) { | ||
| datasetChangeValues.sortingKey = datasetChangeValues.sortingKey.valueOf(); | ||
| } | ||
|
|
||
| const teamIds = formValues.dataset.allowedTeams.map((t) => t.id); | ||
| await updateDatasetPartial(this.props.datasetId, datasetChangeValues); | ||
|
|
||
| if (datasetDefaultConfiguration != null) { | ||
| await updateDatasetDefaultConfiguration( | ||
| this.props.datasetId, | ||
| _.extend({}, datasetDefaultConfiguration, formValues.defaultConfiguration, { | ||
| layers: JSON.parse(formValues.defaultConfigurationLayersJson), | ||
| }), | ||
| ); | ||
| } | ||
|
|
||
| await updateDatasetTeams(this.props.datasetId, teamIds); | ||
| const dataSource = JSON.parse(formValues.dataSourceJson); | ||
|
|
||
| if (dataset != null && this.didDatasourceChange(dataSource)) { | ||
| if (this.didDatasourceIdChange(dataSource)) { | ||
| Toast.warning(messages["dataset.settings.updated_datasource_id_warning"]); | ||
| } | ||
| await updateDatasetDatasource(dataset.directoryName, dataset.dataStore.url, dataSource); | ||
| this.setState({ | ||
| savedDataSourceOnServer: dataSource, | ||
| }); | ||
| } | ||
|
|
||
| const verb = this.props.isEditingMode ? "updated" : "imported"; | ||
| Toast.success(`Successfully ${verb} ${dataset?.name || this.props.datasetId}.`); | ||
| this.setState({ | ||
| hasUnsavedChanges: false, | ||
| // In functional components, we can trigger a re-render by updating state | ||
| setActiveDataSourceEditMode((prev) => prev); // Force re-render | ||
| setTimeout(afterForceUpdateCallback, 0); |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (1)
357-362: The force update pattern won't trigger a re-renderSetting state to the same value (
setActiveDataSourceEditMode((prev) => prev)) doesn't trigger a re-render in React functional components. This won't achieve the intended forced update of SimpleAdvancedDataForm.Consider using a proper force update pattern:
+ const [updateKey, setUpdateKey] = useState(0); const handleSubmit = useCallback(() => { // Ensure that all form fields are in sync syncDataSourceFields(form, activeDataSourceEditMode === "simple" ? "advanced" : "simple"); const afterForceUpdateCallback = () => { // Trigger validation manually... setTimeout( () => form .validateFields() .then((formValues) => submit(formValues)) .catch((errorInfo) => handleValidationFailed(errorInfo)), 0, ); }; - // In functional components, we can trigger a re-render by updating state - setActiveDataSourceEditMode((prev) => prev); // Force re-render + // Force re-render by incrementing a key + setUpdateKey((prev) => prev + 1); setTimeout(afterForceUpdateCallback, 0); - }, [form, activeDataSourceEditMode, submit, handleValidationFailed]); + }, [form, activeDataSourceEditMode, submit, handleValidationFailed]);Then pass
updateKeyas a key prop to the component that needs to re-render:<DatasetSettingsDataTab - key="SimpleAdvancedDataForm" + key={`SimpleAdvancedDataForm-${updateKey}`} dataset={dataset} form={form} activeDataSourceEditMode={activeDataSourceEditMode} onChange={handleDataSourceEditModeChange} />
🧹 Nitpick comments (1)
frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (1)
480-553: Consider memoizing the tabs array for performanceThe tabs array is recreated on every render. Since it contains complex JSX structures and conditional logic, memoizing it would improve performance.
Wrap the tabs array construction in
useMemo:- const tabs = [ + const tabs = useMemo(() => { + const tabItems = [ { label: <span> Data {formErrors.data ? errorIcon : ""}</span>, key: "data", // ... rest of tabs }, ]; if (isUserAdmin && features().allowDeleteDatasets) - tabs.push({ + tabItems.push({ label: <span> Delete Dataset </span>, key: "deleteDataset", // ... rest of tab config }); + + return tabItems; + }, [formErrors, activeTabKey, dataset, form, activeDataSourceEditMode, + handleDataSourceEditModeChange, datasetId, maybeDataSourceId, isUserAdmin]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx(10 hunks)frontend/javascripts/dashboard/dataset/team_selection_component.tsx(1 hunks)frontend/javascripts/dashboard/dataset/useBeforeUnload_hook.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/dashboard/dataset/useBeforeUnload_hook.ts
- frontend/javascripts/dashboard/dataset/team_selection_component.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/dashboard/dataset/dataset_settings_view.tsx (1)
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.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (6)
frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (6)
1-42: LGTM! Clean refactoring of importsThe imports have been properly updated to support the functional component refactoring, including the addition of React hooks and the custom
useBeforeUnloadhook.
64-86: Excellent conversion to functional component with hooksThe state management has been properly migrated from class state to individual
useStatehooks, and the form management uses the modernForm.useForm()pattern. The Redux state access viauseWkSelectoris also the correct approach.
88-184: Well-structured data fetching with proper error handlingThe
fetchDatacallback is correctly implemented with:
- Appropriate dependency array
[datasetId, form]- Proper loading state management
- Comprehensive error handling
- Correct form field initialization sequence
186-265: Validation logic properly migrated to callbacksAll validation and state-checking callbacks are correctly wrapped in
useCallbackwith appropriate dependency arrays. The logic has been preserved from the original implementation while adapting to the functional component pattern.
378-388: Proper lifecycle management with hooksGood use of the custom
useBeforeUnloadhook for handling unsaved changes. TheuseEffectcorrectly excludesdatasetfrom dependencies with a clear comment explaining the reasoning to prevent infinite loops.
555-597: Excellent modernization of the componentThe refactoring from class-based to functional component is comprehensive and well-executed:
- Clean JSX structure with proper form handling
- Correct usage of all React hooks
- Maintained all original functionality
- Improved code readability and maintainability
Great work on this modernization effort! 🎉
…ctional_dataset_settings
…ctional_dataset_settings
|
@philippotto Thanks for starting a review here. A lot of this code has been used and further modified in the full PR for the dataset settings. So, some of these changes might be outdated already and not worth digging to deep. See ##8732 |
In preparation of restyling this component in a similar fashion as the account and organization settings, I first refactored several datasets settings & tab components as React functional components to use modern hook APIs etc.
I move the beforeUnload into it's own hook. This might be related to issue #5977
URL of deployed dev instance (used for testing):
Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)