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 and context. It introduces a new Changes
Estimated code review effort4 (~90 minutes) Possibly related PRs
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 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.
| 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} |
| 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 |
…ctional_dataset_settings
…s/webknossos into functional_dataset_settings
…ctional_dataset_settings
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/javascripts/dashboard/dataset/useBeforeUnload_hook.ts (1)
5-6: Consider addressing TypeScript suppression if possible.The
@ts-ignorecomment indicates a type mismatch between thebeforeUnloadfunction signature and whatuseBlockerexpects. While this might be legitimate due to overloaded signatures, consider if the typing can be improved.Since this was flagged in previous reviews, the TypeScript suppression should be addressed by creating properly typed wrapper functions rather than using
@ts-ignore.
🧹 Nitpick comments (1)
frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (1)
479-552: Memoize the tabs array to avoid unnecessary re-renders.The tabs array is recreated on every render, which could impact performance. Consider memoizing it with
useMemosince it depends on several state values.+ import { useMemo } from "react"; - const tabs = [ + const tabs = useMemo(() => [ { label: <span> Data {formErrors.data ? errorIcon : ""}</span>, key: "data", forceRender: true, children: ( <Hideable hidden={activeTabKey !== "data"}> <DatasetSettingsDataTab key="SimpleAdvancedDataForm" dataset={dataset} form={form} activeDataSourceEditMode={activeDataSourceEditMode} onChange={handleDataSourceEditModeChange} /> </Hideable> ), }, // ... rest of tabs - ]; + ], [ + formErrors.data, + formErrors.general, + formErrors.defaultConfig, + errorIcon, + activeTabKey, + dataset, + form, + activeDataSourceEditMode, + handleDataSourceEditModeChange, + datasetId, + maybeDataSourceId, + isUserAdmin + ]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/javascripts/dashboard/dataset/dataset_settings_delete_tab.tsx(2 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx(10 hunks)frontend/javascripts/dashboard/dataset/useBeforeUnload_hook.ts(1 hunks)frontend/javascripts/messages.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/messages.tsx
- frontend/javascripts/dashboard/dataset/dataset_settings_delete_tab.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 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.
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.
🧬 Code Graph Analysis (1)
frontend/javascripts/dashboard/dataset/useBeforeUnload_hook.ts (1)
frontend/javascripts/admin/dataset/dataset_upload_view.tsx (1)
unblockHistory(265-273)
🪛 Biome (1.9.4)
frontend/javascripts/dashboard/dataset/useBeforeUnload_hook.ts
[error] 43-43: beforeUnload changes on every re-render and should not be used as a hook dependency.
To fix this, wrap the definition of beforeUnload in its own useCallback() hook.
(lint/correctness/useExhaustiveDependencies)
⏰ 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 (4)
frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (4)
377-378: Excellent integration of the useBeforeUnload hook.The integration of the custom
useBeforeUnloadhook is clean and follows React best practices. It properly handles unsaved changes warnings during navigation.
63-86: Well-structured state management conversion.The conversion from class state to multiple
useStatehooks is well-organized and follows React functional component best practices. Each piece of state is properly separated and initialized.
380-387: Acknowledge the intentional dependency omission.The biome ignore comment correctly identifies that removing
datasetfrom the dependency array is intentional to avoid infinite loops. This is a valid pattern when the effect accesses state inside the callback without needing to re-run when that state changes.The pattern of accessing
datasetinside the effect callback while omitting it from dependencies is correct here to prevent unnecessary re-fetches.
266-321: Comprehensive form submission logic with proper memoization.The
submitfunction conversion touseCallbackis well-implemented with all necessary dependencies included. The function properly handles dataset updates, team management, data source changes, and cache invalidation.The callback dependencies are correctly specified and the async logic maintains the original functionality while benefiting from proper memoization.
…ctional_dataset_settings
…ctional_dataset_settings
This PR reworks the appearance of the dataset settings to be similar to
the account & orga settings pages.
- `DatasetSettingView`
- move form state, validation and helpers into a React.Context
(`DatasetSettingsContextProvider`)
- reworked all settings/inputs to use the `SettingsCards` to use the new
UI style
- defined each settings tab as a route. Let React router handle
displaying the correct tab
- refine all tabs as functional style components, i.e. more
`useCallback` etc
- rename "Data" tab to "Data Source" to differentiate it from metadata
and put an emphasis on configuring the origin and layer details. This is
only cosmetic in the page title and menu (and not the underlying data
structures).
- remove deprecated "display name" form field which was left over from
the new Dataset renaming PRs.
- handle synching between "simple" and "advanced" datasource "locally"
within the `<DatasetSettingsDataTab>` and Context provider and stop the
spreading of sync code in other components
- `SettingsCard`
- renamed several props to be more general
- added proper typing to all setting items in the account & organization
views to avoid issues with refactoring/renaming in the future
- Remote Dataset Upload
- split the components from file `dataset_add_remote_view.tsx` into
separate files:
- No changes to `<GoogleAuthFormItem>`
- Removed "simple"/"advanced" datasource view related props from
`<AddRemoteLayer>`
<img width="1597" height="993" alt="Screenshot 2025-07-16 at 15 31 33"
src="https://github.com/user-attachments/assets/e00bc7d1-4a56-472a-b164-0c0fe3936ee5"
/>
### URL of deployed dev instance (used for testing):
- https://datasetsettingscontext.webknossos.xyz/
### Testing
Editing and an existing dataset's settings:
1. Edit the setting of an existing dataset
2. switch between the simple/advanced view and make changes. Verify the
settings were synced between UI and JSON
3. switch between the different settings tabs, make changes and save.
Verify your changes were applied correctly.
Uploading new Remote dataset:
1. Add a new Remote dataset, e.g.
`gs://h01-release/data/20210601/4nm_raw`
2. Verify that the settings form is pre-filled correctly
3. Finish the importing of the remote dataset successfully
### Issues:
- fixes #8737
------
(Please delete unneeded items, merge only when none are left open)
- [x] Added changelog entry (create a `$PR_NUMBER.md` file in
`unreleased_changes` or use `./tools/create-changelog-entry.py`)
- [ ] Added migration guide entry if applicable (edit the same file as
for the changelog)
- [ ] Updated [documentation](../blob/master/docs) if applicable
- [ ] Adapted [wk-libs python
client](https://github.com/scalableminds/webknossos-libs/tree/master/webknossos/webknossos/client)
if relevant API parts change
- [ ] Removed dev-only changes like prints and application.conf edits
- [ ] Considered [common edge
cases](../blob/master/.github/common_edge_cases.md)
- [ ] Needs datastore update after deployment
---------
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
All changes were reviewed and tested as part of PR #8732 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (3)
175-178: Consider adding a TODO comment for the TypeScript issueThe
@ts-ignorecomment addresses a known Ant Design DatePicker type issue. Consider adding a TODO comment to track this for future resolution when Ant Design fixes the type definitions.- // @ts-ignore: The Antd DatePicker component requires a daysjs date object instead of plain number timestamp + // TODO: Remove @ts-ignore when Ant Design DatePicker types are fixed to accept dayjs objects + // @ts-ignore: The Antd DatePicker component requires a dayjs date object instead of plain number timestamp
380-381: Document the force update workaround patternThis creative workaround for forcing form re-validation deserves better documentation for future maintainers.
+ // Force update pattern: Setting state to its current value triggers a re-render + // This ensures all form fields are mounted before validation setActiveDataSourceEditMode((prev) => prev);
148-149: Use optional chaining for cleaner codeThe fallback assignments can be simplified using the nullish coalescing operator.
- onComplete = onComplete ? onComplete : () => navigate("/dashboard"); - onCancel = onCancel ? onCancel : () => navigate("/dashboard"); + onComplete = onComplete ?? (() => navigate("/dashboard")); + onCancel = onCancel ?? (() => navigate("/dashboard"));frontend/javascripts/dashboard/dataset/dataset_settings_viewconfig_tab.tsx (1)
41-42: Clarify the biome-ignore comment to reflect all dependencies.The comment "validate on dataset change" is misleading since the dependency array correctly includes both
availableMappingsPerLayerCacheanddataset. Consider updating the comment to be more accurate.- // biome-ignore lint/correctness/useExhaustiveDependencies: validate on dataset change + // biome-ignore lint/correctness/useExhaustiveDependencies: Intentionally including specific dependencies for validationAlso applies to: 92-92
frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx (1)
173-209: Consider extracting the credential handling logic.The nested async IIFE for handling credentials is complex. Consider extracting this into a separate function for better readability and testability.
async function getExploreParams( showCredentialsFields: boolean, selectedProtocol: string, fileList: FileList, usernameOrAccessKey: string, passwordOrSecretKey: string, datasourceUrl: string, datastoreName: string, preferredVoxelSize: any ) { if (!showCredentialsFields) { return exploreRemoteDataset([datasourceUrl], datastoreName, null, preferredVoxelSize); } if (selectedProtocol === "gs") { const credentials = fileList.length > 0 ? await parseCredentials(fileList[0]?.originFileObj) : null; if (credentials) { return exploreRemoteDataset( [datasourceUrl], datastoreName, { username: "", pass: JSON.stringify(credentials) }, preferredVoxelSize ); } } else if (usernameOrAccessKey && passwordOrSecretKey) { return exploreRemoteDataset( [datasourceUrl], datastoreName, { username: usernameOrAccessKey, pass: passwordOrSecretKey }, preferredVoxelSize ); } return exploreRemoteDataset([datasourceUrl], datastoreName, null, preferredVoxelSize); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
frontend/javascripts/admin/account/account_auth_token_view.tsx(5 hunks)frontend/javascripts/admin/account/account_password_view.tsx(4 hunks)frontend/javascripts/admin/account/account_profile_view.tsx(3 hunks)frontend/javascripts/admin/account/account_settings_view.tsx(1 hunks)frontend/javascripts/admin/account/helpers/settings_card.tsx(1 hunks)frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx(3 hunks)frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx(1 hunks)frontend/javascripts/admin/dataset/remote/google_auth_form_item.tsx(1 hunks)frontend/javascripts/admin/onboarding.tsx(2 hunks)frontend/javascripts/admin/organization/organization_danger_zone_view.tsx(1 hunks)frontend/javascripts/admin/organization/organization_notifications_view.tsx(1 hunks)frontend/javascripts/admin/organization/organization_overview_view.tsx(5 hunks)frontend/javascripts/admin/organization/organization_view.tsx(1 hunks)frontend/javascripts/components/async_clickables.tsx(3 hunks)frontend/javascripts/dashboard/dataset/color_layer_ordering_component.tsx(2 hunks)frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx(1 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx(1 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx(7 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_delete_tab.tsx(2 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_metadata_tab.tsx(1 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx(1 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_sharing_tab.tsx(1 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx(3 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_viewconfig_tab.tsx(3 hunks)frontend/javascripts/dashboard/dataset/helper_components.tsx(2 hunks)frontend/javascripts/dashboard/dataset/useBeforeUnload_hook.ts(1 hunks)frontend/javascripts/messages.tsx(3 hunks)frontend/javascripts/router/route_wrappers.tsx(2 hunks)frontend/javascripts/router/router.tsx(2 hunks)unreleased_changes/8732.md(1 hunks)
🧠 Learnings (8)
📓 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/admin/organization/organization_notifications_view.tsx (1)
Learnt from: MichaelBuessemeyer
PR: #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.
frontend/javascripts/admin/account/account_settings_view.tsx (1)
Learnt from: philippotto
PR: #8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using pathname.split("/").pop() as it fails with trailing slashes. Instead, use pathname.split("/").filter(Boolean) to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
frontend/javascripts/dashboard/dataset/dataset_settings_viewconfig_tab.tsx (2)
Learnt from: dieknolle3333
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.
Learnt from: MichaelBuessemeyer
PR: #8221
File: frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx:584-584
Timestamp: 2024-11-25T14:52:23.692Z
Learning: When evaluation is done, ensure that useAnnotation is always set to true in the form data by using a hidden Form.Item with initialValue={true}.
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (3)
Learnt from: MichaelBuessemeyer
PR: #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: MichaelBuessemeyer
PR: #8221
File: frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx:584-584
Timestamp: 2024-11-25T14:52:23.692Z
Learning: When evaluation is done, ensure that useAnnotation is always set to true in the form data by using a hidden Form.Item with initialValue={true}.
Learnt from: dieknolle3333
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.
frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (1)
Learnt from: MichaelBuessemeyer
PR: #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/dashboard/dataset/dataset_settings_data_tab.tsx (1)
Learnt from: MichaelBuessemeyer
PR: #8221
File: frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx:584-584
Timestamp: 2024-11-25T14:52:23.692Z
Learning: When evaluation is done, ensure that useAnnotation is always set to true in the form data by using a hidden Form.Item with initialValue={true}.
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (2)
Learnt from: dieknolle3333
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.
Learnt from: MichaelBuessemeyer
PR: #8221
File: frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx:584-584
Timestamp: 2024-11-25T14:52:23.692Z
Learning: When evaluation is done, ensure that useAnnotation is always set to true in the form data by using a hidden Form.Item with initialValue={true}.
🧬 Code Graph Analysis (10)
frontend/javascripts/admin/onboarding.tsx (1)
frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (1)
DatasetSettingsProvider(122-436)
frontend/javascripts/admin/organization/organization_notifications_view.tsx (1)
frontend/javascripts/admin/account/helpers/settings_card.tsx (1)
SettingsCard(12-32)
frontend/javascripts/admin/account/account_password_view.tsx (1)
frontend/javascripts/admin/account/helpers/settings_card.tsx (2)
SettingsCardProps(4-10)SettingsCard(12-32)
frontend/javascripts/dashboard/dataset/dataset_settings_metadata_tab.tsx (2)
frontend/javascripts/admin/account/helpers/settings_card.tsx (2)
SettingsCardProps(4-10)SettingsCard(12-32)frontend/javascripts/admin/account/helpers/settings_title.tsx (1)
SettingsTitle(5-21)
frontend/javascripts/admin/account/account_settings_view.tsx (1)
frontend/javascripts/libs/window.ts (1)
location(60-60)
frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx (3)
frontend/javascripts/types/api_types.ts (2)
APIDataSource(152-152)APIDataset(242-245)frontend/javascripts/viewer/store.ts (1)
DatasetConfiguration(263-288)frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)
DatasetRotationAndMirroringSettings(180-184)
frontend/javascripts/admin/dataset/remote/google_auth_form_item.tsx (2)
frontend/javascripts/admin/dataset/composition_wizard/common.ts (1)
FileList(8-8)frontend/javascripts/viewer/constants.ts (1)
Unicode(279-283)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)
frontend/javascripts/types/api_types.ts (1)
APIDataLayer(113-113)
frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (10)
frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx (4)
FormData(9-16)DataSourceEditMode(7-7)DatasetSettingsContextValue(18-32)DatasetSettingsContext(34-36)frontend/javascripts/libs/utils.ts (2)
jsonStringify(180-182)parseMaybe(137-149)frontend/javascripts/types/globals.d.ts (1)
ArbitraryObject(32-32)frontend/javascripts/types/api_types.ts (4)
MutableAPIDataSource(148-151)APIDataSource(152-152)APIDataset(242-245)MutableAPIDataset(238-241)frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)
DatasetRotationAndMirroringSettings(180-184)frontend/javascripts/viewer/store.ts (1)
DatasetConfiguration(263-288)frontend/javascripts/admin/rest_api.ts (7)
readDatasetDatasource(978-984)getDatasetDefaultConfiguration(1081-1083)updateDatasetPartial(1042-1050)updateDatasetDefaultConfiguration(1085-1095)updateDatasetTeams(1312-1320)updateDatasetDatasource(986-1000)sendAnalyticsEvent(113-124)frontend/javascripts/types/schemas/dataset_view_configuration_defaults.ts (1)
enforceValidatedDatasetViewConfiguration(60-108)frontend/javascripts/libs/error_handling.ts (1)
handleGenericError(42-55)frontend/javascripts/dashboard/dataset/helper_components.tsx (1)
hasFormError(123-129)
frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (3)
frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx (1)
useDatasetSettingsContext(38-44)frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)frontend/javascripts/viewer/constants.ts (1)
Unicode(279-283)
✅ Files skipped from review due to trivial changes (7)
- unreleased_changes/8732.md
- frontend/javascripts/dashboard/dataset/helper_components.tsx
- frontend/javascripts/admin/account/account_profile_view.tsx
- frontend/javascripts/admin/organization/organization_view.tsx
- frontend/javascripts/components/async_clickables.tsx
- frontend/javascripts/admin/account/account_auth_token_view.tsx
- frontend/javascripts/admin/organization/organization_overview_view.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/javascripts/messages.tsx
- frontend/javascripts/dashboard/dataset/dataset_settings_delete_tab.tsx
- frontend/javascripts/dashboard/dataset/useBeforeUnload_hook.ts
🧰 Additional context used
🧠 Learnings (8)
📓 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/admin/organization/organization_notifications_view.tsx (1)
Learnt from: MichaelBuessemeyer
PR: #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.
frontend/javascripts/admin/account/account_settings_view.tsx (1)
Learnt from: philippotto
PR: #8672
File: frontend/javascripts/admin/account/account_settings_view.tsx:50-50
Timestamp: 2025-06-25T13:16:26.966Z
Learning: When parsing URL pathnames in React Router applications, avoid using pathname.split("/").pop() as it fails with trailing slashes. Instead, use pathname.split("/").filter(Boolean) to remove empty segments and explicitly find the desired segment by index for more robust URL parsing.
frontend/javascripts/dashboard/dataset/dataset_settings_viewconfig_tab.tsx (2)
Learnt from: dieknolle3333
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.
Learnt from: MichaelBuessemeyer
PR: #8221
File: frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx:584-584
Timestamp: 2024-11-25T14:52:23.692Z
Learning: When evaluation is done, ensure that useAnnotation is always set to true in the form data by using a hidden Form.Item with initialValue={true}.
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (3)
Learnt from: MichaelBuessemeyer
PR: #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: MichaelBuessemeyer
PR: #8221
File: frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx:584-584
Timestamp: 2024-11-25T14:52:23.692Z
Learning: When evaluation is done, ensure that useAnnotation is always set to true in the form data by using a hidden Form.Item with initialValue={true}.
Learnt from: dieknolle3333
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.
frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (1)
Learnt from: MichaelBuessemeyer
PR: #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/dashboard/dataset/dataset_settings_data_tab.tsx (1)
Learnt from: MichaelBuessemeyer
PR: #8221
File: frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx:584-584
Timestamp: 2024-11-25T14:52:23.692Z
Learning: When evaluation is done, ensure that useAnnotation is always set to true in the form data by using a hidden Form.Item with initialValue={true}.
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (2)
Learnt from: dieknolle3333
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.
Learnt from: MichaelBuessemeyer
PR: #8221
File: frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx:584-584
Timestamp: 2024-11-25T14:52:23.692Z
Learning: When evaluation is done, ensure that useAnnotation is always set to true in the form data by using a hidden Form.Item with initialValue={true}.
🧬 Code Graph Analysis (10)
frontend/javascripts/admin/onboarding.tsx (1)
frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (1)
DatasetSettingsProvider(122-436)
frontend/javascripts/admin/organization/organization_notifications_view.tsx (1)
frontend/javascripts/admin/account/helpers/settings_card.tsx (1)
SettingsCard(12-32)
frontend/javascripts/admin/account/account_password_view.tsx (1)
frontend/javascripts/admin/account/helpers/settings_card.tsx (2)
SettingsCardProps(4-10)SettingsCard(12-32)
frontend/javascripts/dashboard/dataset/dataset_settings_metadata_tab.tsx (2)
frontend/javascripts/admin/account/helpers/settings_card.tsx (2)
SettingsCardProps(4-10)SettingsCard(12-32)frontend/javascripts/admin/account/helpers/settings_title.tsx (1)
SettingsTitle(5-21)
frontend/javascripts/admin/account/account_settings_view.tsx (1)
frontend/javascripts/libs/window.ts (1)
location(60-60)
frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx (3)
frontend/javascripts/types/api_types.ts (2)
APIDataSource(152-152)APIDataset(242-245)frontend/javascripts/viewer/store.ts (1)
DatasetConfiguration(263-288)frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)
DatasetRotationAndMirroringSettings(180-184)
frontend/javascripts/admin/dataset/remote/google_auth_form_item.tsx (2)
frontend/javascripts/admin/dataset/composition_wizard/common.ts (1)
FileList(8-8)frontend/javascripts/viewer/constants.ts (1)
Unicode(279-283)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)
frontend/javascripts/types/api_types.ts (1)
APIDataLayer(113-113)
frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (10)
frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx (4)
FormData(9-16)DataSourceEditMode(7-7)DatasetSettingsContextValue(18-32)DatasetSettingsContext(34-36)frontend/javascripts/libs/utils.ts (2)
jsonStringify(180-182)parseMaybe(137-149)frontend/javascripts/types/globals.d.ts (1)
ArbitraryObject(32-32)frontend/javascripts/types/api_types.ts (4)
MutableAPIDataSource(148-151)APIDataSource(152-152)APIDataset(242-245)MutableAPIDataset(238-241)frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)
DatasetRotationAndMirroringSettings(180-184)frontend/javascripts/viewer/store.ts (1)
DatasetConfiguration(263-288)frontend/javascripts/admin/rest_api.ts (7)
readDatasetDatasource(978-984)getDatasetDefaultConfiguration(1081-1083)updateDatasetPartial(1042-1050)updateDatasetDefaultConfiguration(1085-1095)updateDatasetTeams(1312-1320)updateDatasetDatasource(986-1000)sendAnalyticsEvent(113-124)frontend/javascripts/types/schemas/dataset_view_configuration_defaults.ts (1)
enforceValidatedDatasetViewConfiguration(60-108)frontend/javascripts/libs/error_handling.ts (1)
handleGenericError(42-55)frontend/javascripts/dashboard/dataset/helper_components.tsx (1)
hasFormError(123-129)
frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (3)
frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx (1)
useDatasetSettingsContext(38-44)frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)frontend/javascripts/viewer/constants.ts (1)
Unicode(279-283)
⏰ 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 (40)
frontend/javascripts/admin/account/account_settings_view.tsx (1)
47-51: Good improvement in path parsing clarity.The change from
filter(Boolean)tofilter((p) => p.length > 0)makes the intent more explicit by filtering only empty strings rather than all falsy values. This aligns with the established pattern across similar admin views and improves code readability.frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)
45-48: Good enhancement to form state preservation.Adding the
preserve: trueoption toForm.useWatchensures that watched form values are maintained across form resets or unmounts. This is particularly important for nested form fields like["dataSource", "dataLayers"]and aligns with the improved form state management in the dataset settings refactoring.frontend/javascripts/admin/organization/organization_danger_zone_view.tsx (1)
57-57: Consistent prop naming standardization.The change from
descriptiontocontentaligns with the standardizedSettingsCardcomponent interface used throughout the admin UI. This improves consistency and type safety across all SettingsCard usages.frontend/javascripts/admin/organization/organization_notifications_view.tsx (3)
101-102: Consistent SettingsCard prop standardization.The prop names have been updated from
explanationanddescriptiontotooltipandcontentrespectively, aligning with the standardizedSettingsCardcomponent interface used throughout the admin UI.
108-109: Consistent SettingsCard prop standardization.The prop names have been updated from
explanationanddescriptiontotooltipandcontentrespectively, maintaining consistency with the updated SettingsCard interface.
115-116: Consistent SettingsCard prop standardization.The prop names have been updated from
explanationanddescriptiontotooltipandcontentrespectively, completing the consistent standardization across all SettingsCard usages in this file.frontend/javascripts/admin/account/account_password_view.tsx (4)
10-10: Good addition of type safety.Adding the
SettingsCardPropstype import improves type safety for thepassKeyListarray and ensures consistency with the SettingsCard component interface.
158-165: Excellent type safety and prop standardization.The explicit typing of
passKeyListasSettingsCardProps[]and the property rename fromvaluetocontentaligns with the standardized SettingsCard interface, improving both type safety and consistency.
174-174: Consistent SettingsCard prop usage.The prop name change from
descriptiontocontentmaintains consistency with the updated SettingsCard component interface used throughout the admin UI.
192-192: Proper property access after rename.Correctly accessing
item.contentinstead ofitem.valueafter the property rename in thepassKeyListarray structure.frontend/javascripts/router/router.tsx (2)
55-59: LGTM: Clean imports for dataset settings tab componentsThe new imports are well-organized and follow consistent naming patterns for the refactored dataset settings tabs.
278-285: Excellent nested routing structure for dataset settingsThe conversion from a self-closing route to nested routes with child routes is well-implemented:
- Index route properly redirects to the "data" tab as the default
- Each tab has a semantic route path that matches its functionality
- The structure integrates cleanly with React Router's Outlet pattern
- Route organization is consistent and maintainable
This aligns perfectly with the context-based refactoring and enables proper tabbed navigation.
frontend/javascripts/admin/onboarding.tsx (2)
23-23: LGTM: Proper import for context providerThe import follows the established pattern and is correctly placed with other dashboard imports.
531-538: Excellent integration with DatasetSettingsProviderThe wrapping of
DatasetSettingsViewwithDatasetSettingsProvideris properly implemented:
isEditingMode={false}is appropriate for the import scenario during onboardingdatasetIdToImportis correctly passed as thedatasetIdprop- The
onCompleteandonCancelhandlers are properly passed to the providerDatasetSettingsViewno longer receives props directly, consistent with the context-based refactoringThis change aligns perfectly with the broader refactoring to use context for state management.
frontend/javascripts/router/route_wrappers.tsx (2)
9-9: LGTM: Correct import for dataset settings providerThe import is properly placed and follows the established import organization.
103-106: Perfect implementation of provider pattern in route wrapperThe refactoring correctly wraps
DatasetSettingsViewwithDatasetSettingsProvider:
isEditingMode={true}is appropriate for the dataset edit route contextdatasetIdis properly extracted from URL parameters and passed to the providerDatasetSettingsViewno longer receives direct props, consistent with the context-based architecture- The change maintains the existing route structure while enabling the new context-driven state management
This implementation is consistent with the pattern established in the onboarding modal and aligns with the overall refactoring objectives.
frontend/javascripts/dashboard/dataset/dataset_settings_metadata_tab.tsx (3)
1-3: LGTM: Clean imports for refactored UI componentsThe imports are well-organized and bring in the necessary components for the new SettingsCard-based layout.
6-27: Excellent structured approach to metadata form itemsThe
metadataItemsarray is well-designed:
- Clear separation of concerns with each item having title, tooltip, and content
- Informative tooltips that provide helpful context to users
- Proper form field nesting with
["dataset", "sortingKey"]and["dataset", "description"]- Appropriate form components (DatePicker for dates, TextArea for descriptions)
- Publication Date tooltip clearly explains the sorting functionality
This structured approach makes the code maintainable and follows good React patterns.
29-40: Perfect implementation of consistent UI layoutThe layout implementation is excellent:
SettingsTitleprovides consistent section header with description- Responsive grid layout with
gutter={[24, 24]}for proper spacingCol span={12}creates a clean two-column layout- Dynamic rendering of metadata items maintains DRY principles
- Proper key assignment using
item.titlefor React list renderingThe component follows the established SettingsCard pattern and integrates seamlessly with the context-based form management.
frontend/javascripts/admin/dataset/remote/google_auth_form_item.tsx (4)
1-6: LGTM: Well-organized importsThe imports are properly structured and include all necessary dependencies. Good use of type imports for better tree-shaking.
10-18: Excellent TypeScript interface designThe component props are well-typed:
FileListtype from the composition wizard maintains consistencyUploadChangeParamprovides proper typing for the change handler- Optional
showOptionalHintflag enables flexible usage across different contexts
20-36: Great accessibility and user experience in form labelThe label implementation is excellent:
- Uses
Unicode.NonBreakingSpacefor proper spacing- External link to Google Cloud documentation with proper
target="_blank"andrel="noopener noreferrer"for security- Conditional "(Optional)" hint improves user understanding
hasFeedbackprovides visual feedback for form validation
37-58: Perfect drag-and-drop upload implementationThe
Upload.Draggeris well-configured:
beforeUpload={() => false}correctly prevents automatic upload, allowing manual control- Clear visual hierarchy with unlock icon and descriptive text
- Informative hint text explains when the authentication is needed
- Proper styling with appropriate icon size and margins
- User-friendly copy that explains the drag-and-drop functionality
This provides an intuitive user experience for uploading Google Cloud authentication files.
frontend/javascripts/dashboard/dataset/color_layer_ordering_component.tsx (1)
1-78: Well-executed UI simplification!The refactoring successfully removes unnecessary wrapper components while preserving all functionality. The use of
useCallbackhooks with correct dependencies prevents unnecessary re-renders, and the simplified conditional rendering improves code readability.frontend/javascripts/admin/account/helpers/settings_card.tsx (1)
1-32: Clean prop renaming and UI improvements!The prop names are now more intuitive (
contentandtooltipbetter describe their purpose). The switch fromPopovertoTooltipis appropriate for simple explanatory text, and the addedstyleprop provides useful customization flexibility.frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx (1)
1-44: Excellent context implementation with proper TypeScript typing!The context structure follows React best practices with clear type definitions, proper error handling in the custom hook, and well-organized interfaces. This provides a solid foundation for centralized dataset settings management.
frontend/javascripts/dashboard/dataset/dataset_settings_sharing_tab.tsx (1)
1-149: Excellent modernization to functional component!The refactoring successfully converts the component to use hooks and context, with proper memoization of computed values and callbacks. The React Query integration for data fetching is clean, and the UI restructuring with
SettingsCardcomponents improves consistency.frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (1)
122-436: Outstanding provider implementation with comprehensive state management!This provider successfully centralizes all dataset settings logic with proper error handling, form validation, and API integration. The use of React Query for cache invalidation and the custom
useBeforeUnloadhook for unsaved changes are particularly well done.frontend/javascripts/dashboard/dataset/dataset_settings_viewconfig_tab.tsx (2)
22-28: LGTM! Clean separation of concerns.The pattern of checking for null dataset and delegating to a component with explicit props is a good approach for type safety and clear data flow.
159-303: Excellent UI restructuring with consistent card-based layout.The refactoring to use
SettingsCardcomponents with consistent structure improves code maintainability and provides a uniform user experience. The switch from checkboxes toSwitchcomponents for boolean settings is a good modern UI choice.frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (2)
140-142: Good documentation of the dataset name handling distinction.The comment clearly explains why the dataset name is retrieved separately from the form field rather than from datasource.id.name, which helps future maintainers understand this important distinction.
231-233: Proper integration with the new context-based architecture.Wrapping
DatasetSettingsDataTabwithDatasetSettingsProvidercorrectly integrates this component with the new context-based state management system.frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (4)
319-326: Well-documented workaround for form validation.The biome-ignore comment clearly explains why only
dataLayersis included in the dependencies. This is a reasonable workaround for theshouldUpdatelimitation.
183-196: Great UX improvement with read-only dataset ID and copy functionality.The addition of a copy button for the dataset ID improves user experience, and making the field read-only prevents accidental modifications.
634-634: Good simplification of React API usage.Using the imported
cloneElementdirectly instead ofReact.cloneElementis cleaner and follows modern import practices.
48-49: Address the TODO comment about useWatch stale values.The comment suggests using the
preserveoption to fix stale values. This should be implemented to ensure data consistency.- // TODO: The "preserve" option probably fixes this, e.g. useWatch("dataSource", { form, preserve: true }); - Form.useWatch("dataSource", { form, preserve: true }); + Form.useWatch("dataSource", { form, preserve: true });⛔ Skipped due to learnings
Learnt from: MichaelBuessemeyer PR: scalableminds/webknossos#8221 File: frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx:584-584 Timestamp: 2024-11-25T14:52:23.692Z Learning: When evaluation is done, ensure that `useAnnotation` is always set to `true` in the form data by using a hidden `Form.Item` with `initialValue={true}`.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/dataset_settings_view.tsx (3)
35-55: Excellent refactoring to functional component with hooks.The conversion from class component to functional component is well-executed. The use of context for state management and the extraction of the selected tab from the URL path are clean implementations.
131-165: Great UX enhancement with validation error indicators.The menu items showing error icons for tabs with validation errors is an excellent user experience improvement, making it immediately clear which tabs need attention.
197-203: Consider memoizing the navigation callback.While the current implementation works, memoizing the
navigateToTabcallback could prevent unnecessary re-renders of child components if this callback is passed down as a prop in the future.The current
useCallbackimplementation is already optimal with correct dependencies.frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx (1)
25-69: Well-structured helper functions with good edge case handling.The helper functions for ensuring largest segment IDs, merging layers with automatic renaming of duplicates, and parsing credentials are well-implemented with appropriate error handling.
| useEffect(() => { | ||
| if (defaultUrl != null) { | ||
| // only set datasourceUrl in the first render | ||
| if (datasourceUrl == null) { | ||
| form.setFieldValue("url", defaultUrl); | ||
| form.validateFields(["url"]); | ||
| } else { | ||
| handleExplore(); | ||
| } | ||
| } | ||
| }, [defaultUrl, datasourceUrl, form.setFieldValue, form.validateFields]); | ||
|
|
There was a problem hiding this comment.
Fix missing dependencies in useEffect.
The useEffect hook is missing handleExplore in its dependencies, which could lead to stale closures.
- }, [defaultUrl, datasourceUrl, form.setFieldValue, form.validateFields]);
+ }, [defaultUrl, datasourceUrl, form, handleExplore]);However, this would cause an infinite loop since handleExplore is defined inline. Consider wrapping handleExplore in useCallback or moving the logic inside the effect.
📝 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.
| useEffect(() => { | |
| if (defaultUrl != null) { | |
| // only set datasourceUrl in the first render | |
| if (datasourceUrl == null) { | |
| form.setFieldValue("url", defaultUrl); | |
| form.validateFields(["url"]); | |
| } else { | |
| handleExplore(); | |
| } | |
| } | |
| }, [defaultUrl, datasourceUrl, form.setFieldValue, form.validateFields]); | |
| useEffect(() => { | |
| if (defaultUrl != null) { | |
| // only set datasourceUrl in the first render | |
| if (datasourceUrl == null) { | |
| form.setFieldValue("url", defaultUrl); | |
| form.validateFields(["url"]); | |
| } else { | |
| handleExplore(); | |
| } | |
| } | |
| }, [defaultUrl, datasourceUrl, form, handleExplore]); |
🤖 Prompt for AI Agents
In frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx around lines
110 to 121, the useEffect hook is missing handleExplore in its dependency array,
which can cause stale closures. To fix this, either wrap handleExplore in a
useCallback hook to memoize it and then add it to the dependencies, or move the
logic inside handleExplore directly into the useEffect to avoid the infinite
loop caused by inline function definitions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (4)
75-78: Consider adding validation after parsing JSONThe parsed JSON is cast to
ArbitraryObject | nullwithout validation. WhileparseMaybehandles parsing errors, consider validating the parsed structure before using it as a data source.const dataSourceFromAdvancedTab = parseMaybe( form.getFieldValue("dataSourceJson"), ) as ArbitraryObject | null; +// Validate that parsed object has expected structure +if (dataSourceFromAdvancedTab && !dataSourceFromAdvancedTab.dataLayers) { + console.warn("Parsed data source is missing required dataLayers property"); +}
148-149: Memoize default callback functionsThe default callbacks are recreated on every render. Consider using
useMemoto optimize performance.-onComplete = onComplete ? onComplete : () => navigate("/dashboard"); -onCancel = onCancel ? onCancel : () => navigate("/dashboard"); +const memoizedOnComplete = useCallback( + onComplete || (() => navigate("/dashboard")), + [onComplete, navigate] +); +const memoizedOnCancel = useCallback( + onCancel || (() => navigate("/dashboard")), + [onCancel, navigate] +);Then use
memoizedOnCompleteandmemoizedOnCancelthroughout the component.
380-384: Document the force update patternThe force update pattern is clever but non-obvious. Consider adding a more detailed comment explaining the mechanism.
// Force update pattern: Setting state to its current value triggers a re-render // This ensures all form fields are mounted before validation +// This workaround is necessary because form.validateFields() only validates mounted fields, +// and we need to validate fields from all tabs, not just the currently visible one. setActiveDataSourceEditMode((prev) => prev); setTimeout(afterForceUpdateCallback, 0);
215-215: Fix typo in dependency arrayThere's an inconsistency in the dependency array formatting.
-}, [datasetId, form.setFieldsValue, form.validateFields]); +}, [datasetId, form]);Since you're using multiple form methods, it's cleaner to depend on the entire form object.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx(1 hunks)
🧠 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_provider.tsx (1)
Learnt from: MichaelBuessemeyer
PR: #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.
🧰 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_provider.tsx (1)
Learnt from: MichaelBuessemeyer
PR: #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. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (2)
frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (2)
409-413: Well-implemented analytics trackingGood practice to track when dataset settings are opened, with appropriate fallback for dataset name.
273-286: Good defensive programming in validation checkThe
isOnlyDatasourceIncorrectAndNotEditedfunction properly handles JSON parsing errors and validates the specific scenario where only the datasource has errors but hasn't been edited.
| // @ts-ignore: The Antd DatePicker component requires a daysjs date object instead of plain number timestamp | ||
| sortingKey: dayjs(fetchedDataset.sortingKey as any as Dayjs), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace @ts-ignore with proper typing
Instead of using @ts-ignore, consider properly typing the date field.
-// @ts-ignore: The Antd DatePicker component requires a daysjs date object instead of plain number timestamp
-sortingKey: dayjs(fetchedDataset.sortingKey as any as Dayjs),
+sortingKey: dayjs(fetchedDataset.sortingKey),The dayjs constructor accepts timestamps natively, so the cast to Dayjs is unnecessary.
📝 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.
| // @ts-ignore: The Antd DatePicker component requires a daysjs date object instead of plain number timestamp | |
| sortingKey: dayjs(fetchedDataset.sortingKey as any as Dayjs), | |
| sortingKey: dayjs(fetchedDataset.sortingKey), |
🤖 Prompt for AI Agents
In frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx at lines
176-177, remove the @ts-ignore comment and the unnecessary cast to Dayjs when
calling dayjs with fetchedDataset.sortingKey. Instead, ensure the type of
sortingKey is correctly defined as a number or compatible timestamp type so that
dayjs can accept it directly without type errors, eliminating the need for
ignoring TypeScript checks.
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)