Skip to content

Conversation

hotzenklotz
Copy link
Member

@hotzenklotz hotzenklotz commented Jun 30, 2025

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>
Screenshot 2025-07-16 at 15 31 33

URL of deployed dev instance (used for testing):

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:


(Please delete unneeded items, merge only when none are left open)

  • 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 if applicable
  • Adapted wk-libs python client if relevant API parts change
  • Removed dev-only changes like prints and application.conf edits
  • Considered common edge cases
  • Needs datastore update after deployment

@hotzenklotz hotzenklotz self-assigned this Jun 30, 2025
Copy link
Contributor

coderabbitai bot commented Jun 30, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough

Walkthrough

This change refactors the dataset settings UI, introducing a new context provider and modularizing settings tabs into separate components. It unifies prop naming for SettingsCard, restructures routing for nested tabs, and updates the layout for improved accessibility and maintainability. The metadata tab is reworked, but the display name input remains.

Changes

File(s) Change Summary
frontend/javascripts/admin/account/account_auth_token_view.tsx, .../account_password_view.tsx, .../account_profile_view.tsx, .../organization/organization_danger_zone_view.tsx, .../organization/organization_notifications_view.tsx, .../organization/organization_overview_view.tsx Unified SettingsCard prop names (descriptioncontent, explanationtooltip), added explicit typing.
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx Removed internal AddRemoteLayer logic; now delegates to new imported component, simplified state/validation.
frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx Added new AddRemoteLayer component for remote dataset integration, with protocol/auth/merge logic.
frontend/javascripts/admin/dataset/remote/google_auth_form_item.tsx Added GoogleAuthFormItem component for Google Cloud credentials upload.
frontend/javascripts/admin/onboarding.tsx, .../router/route_wrappers.tsx Wrapped DatasetSettingsView in new DatasetSettingsProvider for context-driven state management.
frontend/javascripts/components/async_clickables.tsx Added JSDoc documentation to exported types and components.
frontend/javascripts/dashboard/dataset/color_layer_ordering_component.tsx Refactored to memoize handlers and simplified UI, removed tooltip from panel header.
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx Updated Form.useWatch usage for field watching.
frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx Added context and hook for dataset settings state management.
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx Refactored to use context, restructured UI with SettingsCard, improved form sync logic.
frontend/javascripts/dashboard/dataset/dataset_settings_delete_tab.tsx Refactored to use context, improved layout, removed prop-based dataset fetching.
frontend/javascripts/dashboard/dataset/dataset_settings_metadata_tab.tsx Refactored to use SettingsCard layout, but display name input remains.
frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx Added DatasetSettingsProvider for context-driven logic, data fetching, and form handling.
frontend/javascripts/dashboard/dataset/dataset_settings_sharing_tab.tsx Refactored to use context, unified sharing/permissions UI with SettingsCard.
frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx Refactored to context-driven, menu-based navigation, removed internal state/logic.
frontend/javascripts/dashboard/dataset/dataset_settings_viewconfig_tab.tsx Refactored to use context, modularized settings into SettingsCard layout.
frontend/javascripts/dashboard/dataset/helper_components.tsx Optimized imports (direct lodash sum, simplified React import).
frontend/javascripts/dashboard/dataset/useBeforeUnload_hook.ts Refactored beforeUnload to useCallback, adjusted navigation blocking logic.
frontend/javascripts/messages.tsx Updated label and tooltip for segmentationPatternOpacity.
frontend/javascripts/router/router.tsx Changed dataset settings route to nested structure for tab navigation.
unreleased_changes/8732.md Changelog entry for improved dataset settings UI accessibility.

Assessment against linked issues

Objective (Issue #) Addressed Explanation
Remove Display Name option from dataset settings metadata tab (#8737) The "Display Name" input remains present in the metadata tab; it is not removed as required.
Move description input if metadata tab is removed (#8737) The metadata tab still exists with the description input; it is unclear if/where it will be moved.

Assessment against linked issues: Out-of-scope changes

Code Change (file_path) Explanation
Addition of new AddRemoteLayer and GoogleAuthFormItem components (admin/dataset/remote/add_remote_layer.tsx, admin/dataset/remote/google_auth_form_item.tsx) These components are unrelated to the removal of the display name from the metadata tab.
Refactoring of async clickables with JSDoc (components/async_clickables.tsx) Documentation updates are unrelated to the metadata tab or display name removal.
Updates to color layer ordering, before-unload logic, and segmentation opacity label (various files) These changes are not connected to the display name or metadata tab objectives.

Possibly related PRs

Suggested labels

refactoring, usability

Suggested reviewers

  • philippotto

Poem

In the warren of code, with carrots in paw,
A rabbit refactors—oh, what it saw!
Cards got new labels, tabs nested with care,
Settings now context-driven, logic to share.
Though the display name lingers, more hops to go—
This bunny brings order, with UI in tow!
🥕✨


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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@hotzenklotz hotzenklotz changed the base branch from master to functional_dataset_settings June 30, 2025 15:47
@hotzenklotz hotzenklotz mentioned this pull request Jul 2, 2025
11 tasks
@MichaelBuessemeyer
Copy link
Contributor

MichaelBuessemeyer commented Jul 4, 2025

  • Did not check the code, but here is a reminder to re-add the blocking mechanism in case of unsaved changes.

@hotzenklotz hotzenklotz mentioned this pull request Jul 14, 2025
8 tasks
hotzenklotz added a commit that referenced this pull request Jul 15, 2025
This PR updates React Router (RR) from v5 to v6 following their [upgrade
guide](https://reactrouter.com/6.30.1/upgrading/v5). There is already
react-router v7 but let's go step by step. v7 also seems to have the
same API as v6 so we might update soon in a follow up PR (and double
check compatibility with React v18/19).

Noteworthy changes and high level concepts:
- RR v6 offer various ways of setting up a router
([Docs](https://reactrouter.com/6.30.1/routers/picking-a-router)):
  - Declarative: Using JSX `<Route`> elements like we did so far
- Data API: Create routes as TS code. This is the recommend method and
what I used for the refactor.
- Ideally the routes are constant and live outside a react component to
avoid constant recreation of the router/routes
- While you are meant to define routes as code, there is a
"bridge"/helper, [`createRoutesFromElements`](
https://reactrouter.com/6.30.1/utils/create-routes-from-elements), that
translates JSX `<Route>` objects into code. I used that to keep the diff
and changes somewhat manageable. We can switch to a purely code based
definition in the future.
- File-Base/Framework: You can define routes through your hierarchy on
the file system. (not used)
- RR v6 `<Routes>` do not have a `render={() => ...}` prop anymore that
would run any function. That makes it impossible to do any sort of
conditional logic inside the router. I moved all these methods into new
React wrapper components.
- RR v6 uses new hooks: `useNavigation`, `useParams`(for accessing route
parameters, e.g. `/tasks/:taskId`)
- RR v6 allows [nested
routing](https://reactrouter.com/6.30.1/start/overview#nested-routes). A
common parent page (with nice layout) can be used to render/insert the
child views with an `<Outlet />` component. I used it for the orga &
account pages for rendering the various tabs into a comment parent.
Also, the router's root element uses an `<Outlet>`.
- RR v6 does not support class-based React component anymore. The
`withRouter` HOC was removed. I build a new `withRouter` HOC as a
workaround for our remaining React class components.
- RR v6 removed an imperative API for blocking the navigation from one
page to another. It was replace with a hook
[`useBlocker`](https://reactrouter.com/6.30.1/hooks/use-blocker).
Unfortunately, we mostly use the blocker in class based components.
There is not easy workaround - so I had to remove some of the blocking
stuff. (Dataset Setting are currently being refactored as functional
components and will be able to use that once more. #8732 )


### URL of deployed dev instance (used for testing):
- https://___.webknossos.xyz

### Steps to test:
My testing process:
- Click through all the routes/views and make sure they still work
- Make sure that routes with parameters still work, e.g. all the task,
projects, task types stuff with ids and prefilled forms
- Test a legacy route

### TODOs:
- [x] Fix: navigation blockers
- [ ] Decide: Should we rename the `SecuredRoute` component? It is not a
`<Route>` any more... maybe `AuthRequired` or `RequireAuth` or
something.
- [x] Fix: `features` type definition. It needs too load earlier.
- [x] Fix: dashboard header having too many items <img width="1650"
alt="Screenshot 2025-07-03 at 09 37 11"
src="https://github.com/user-attachments/assets/9bba76e3-2b3a-4bad-8be0-798685d0e4ac"
/>


### Issues:
- fixes #

------
(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>
Co-authored-by: Michael Büßemeyer <[email protected]>
@hotzenklotz hotzenklotz marked this pull request as ready for review July 16, 2025 13:00
@hotzenklotz
Copy link
Member Author

@coderabbitai review

Copy link
Contributor

coderabbitai bot commented Jul 16, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Nitpick comments (13)
frontend/javascripts/components/async_clickables.tsx (3)

22-27: Consider documenting error-propagation behaviour

useLoadingClickHandler will re-throw any error from originalOnClick, which is the correct default. A brief @throws note would make this explicit and help downstream consumers know they must handle rejections themselves.


80-88: Potential prop leakage to <svg> / DOM in AsyncIconButton

React.cloneElement(isLoading ? <LoadingOutlined /> : props.icon, { ...props, onClick }) forwards all button props (e.g. type, block, danger, etc.) to whatever element props.icon ultimately renders.
For Ant Design icons this is usually safe, but if a consumer passes a custom icon that renders an <svg> or <span>, React will emit “unknown prop” warnings in the console.

A defensive fix:

-  return React.cloneElement(isLoading ? <LoadingOutlined /> : props.icon, { ...props, onClick });
+  const { icon, hideContentWhenLoading, title, ...rest } = props; // pick only relevant
+  return React.cloneElement(isLoading ? <LoadingOutlined /> : icon, { ...rest, onClick });

Keeps the public API unchanged while avoiding accidental prop bleed-through.


94-97: Tiny wording tweak

JSDoc currently says “It displays a loading indicator before the link text while the onClick promise is pending.” – strictly speaking, the indicator is prepended only when isLoading is true; otherwise the original props.icon (which may be undefined) is shown.
A micro-edit such as “ Prepends a loading icon while the promise is pending” would make this perfectly accurate.

frontend/javascripts/dashboard/dataset/helper_components.tsx (1)

5-7: Import sum from the path import to avoid bundling all of Lodash.

Importing from "lodash" still drags the whole library (unless your build does full tree-shaking). Prefer:

-import { sum } from "lodash";
+import sum from "lodash/sum";

This keeps the intended optimization regardless of bundler.

Also applies to: 128-129

frontend/javascripts/admin/organization/organization_notifications_view.tsx (1)

100-117: Cards updated correctly; minor UX polish possible.

If ownerEmail hasn’t loaded yet the card shows an empty line. Consider rendering a placeholder (e.g. “—”) or a spinner while ownerEmail is being fetched.

frontend/javascripts/dashboard/dataset/useBeforeUnload_hook.ts (1)

11-11: Consider more specific path matching for dataset settings.

The current logic bypasses blocking for all paths starting with "/datasets", which may be too broad. Consider using a more specific path like "/datasets/:datasetNameAndId/edit" to ensure blocking only occurs for dataset settings navigation, not for other dataset-related pages.

-if (hasUnsavedChanges && !location.pathname.startsWith("/datasets")) {
+if (hasUnsavedChanges && !location.pathname.match(/^\/datasets\/[^\/]+\/edit/)) {
frontend/javascripts/admin/organization/organization_overview_view.tsx (1)

122-173: Memoize orgaStats to avoid needless re-renders

orgaStats is rebuilt on every render, causing all SettingsCard children to re-mount even when nothing changed.
Wrapping the array construction in useMemo keyed by the values it depends on (organization, activeUsersCount, etc.) will prevent unnecessary reconciliations and improve render performance.

frontend/javascripts/admin/dataset/remote/google_auth_form_item.tsx (2)

8-10: Reuse the existing FileList type instead of redefining it

A FileList alias already exists in frontend/javascripts/admin/dataset/composition_wizard/common.ts.
Importing that avoids duplication and keeps type definitions in one place.


38-43: Restrict accepted file types

Consider adding an accept=".json" (or the exact extension of GCS key files) to Upload.Dragger to prevent accidental uploads of unsupported files and reduce user error.

-<Upload.Dragger
+<Upload.Dragger
+  accept=".json"
frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx (1)

22-30: Consider extracting the edit-mode union to a dedicated type

"simple" | "advanced" appears twice (activeDataSourceEditMode and its handler).
Exporting a type DataSourceEditMode = "simple" | "advanced" increases consistency and eases future refactors.

frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (2)

128-130: Fix the type issue instead of using @ts-ignore

The comment explains that Antd DatePicker requires a dayjs object. Consider properly typing the form data to accept dayjs objects.

Define a proper type for the form that matches Antd's expectations:

-      // @ts-ignore: The Antd DatePicker component requires a daysjs date object instead of plain number timestamp
-      sortingKey: dayjs(fetchedDataset.sortingKey),
+      sortingKey: dayjs(fetchedDataset.sortingKey) as any as Dayjs,

Or better yet, update the FormData type to properly handle dayjs objects for the sortingKey field.


134-136: Address the type mismatch properly

Using @ts-ignore masks a potential type safety issue between APIDataSource and MutableAPIDataset.

Consider fixing the type mismatch by ensuring the dataSource field in FormData accepts the correct type:

-      // @ts-ignore Missmatch between APIDataSource and MutableAPIDataset
-      dataSource,
+      dataSource: dataSource as any, // TODO: Fix FormData type to accept APIDataSource

Or update the FormData type definition to properly handle the APIDataSource type.

frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx (1)

128-151: Consider extracting URL validation logic

The URL validation logic with prefix removal could be extracted to a separate utility function for better testability and reusability.

Would you like me to help extract this validation logic into a separate utility function?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0b927e2 and 2c9e711.

📒 Files selected for processing (28)
  • 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/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/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 (8 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)
🧰 Additional context used
🧠 Learnings (10)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (5)
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8485
File: frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts:384-392
Timestamp: 2025-04-01T09:45:17.527Z
Learning: The function `isRotationAndMirrorMaybeOnly` in the dataset_layer_transformation_accessor.ts is intentionally designed to allow mirroring transformations (negative scale values). It uses the length comparison (`scale.length() === NON_SCALED_VECTOR.length()`) rather than component equality to permit mirrored axes while ensuring the overall scale magnitude remains the same.
Learnt from: 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: dieknolle3333
PR: scalableminds/webknossos#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.
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8229
File: frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts:348-354
Timestamp: 2024-11-25T14:38:49.345Z
Learning: For the `getDatasetExtentAsProduct` function in `dataset_accessor.ts`, input validation for negative or zero dimensions is not necessary.
frontend/javascripts/dashboard/dataset/helper_components.tsx (3)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8229
File: frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts:348-354
Timestamp: 2024-11-25T14:38:49.345Z
Learning: For the `getDatasetExtentAsProduct` function in `dataset_accessor.ts`, input validation for negative or zero dimensions is not necessary.
Learnt from: knollengewaechs
PR: scalableminds/webknossos#8266
File: frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx:403-403
Timestamp: 2024-12-16T15:51:37.612Z
Learning: In Ant Design's Form component, when working with `Form.List`, use `form.setFieldValue` to update a single field in the list.
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}`.
frontend/javascripts/admin/organization/organization_notifications_view.tsx (1)
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8221
File: CHANGELOG.unreleased.md:14-14
Timestamp: 2024-11-25T14:52:23.201Z
Learning: For the `webknossos` project, changelog entries in `CHANGELOG.unreleased.md` should be concise and avoid unnecessary detailed explanations.
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/admin/account/account_profile_view.tsx (1)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#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/messages.tsx (2)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#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.
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_viewconfig_tab.tsx (2)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8229
File: frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts:348-354
Timestamp: 2024-11-25T14:38:49.345Z
Learning: For the `getDatasetExtentAsProduct` function in `dataset_accessor.ts`, input validation for negative or zero dimensions is not necessary.
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}`.
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (2)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8229
File: frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts:348-354
Timestamp: 2024-11-25T14:38:49.345Z
Learning: For the `getDatasetExtentAsProduct` function in `dataset_accessor.ts`, input validation for negative or zero dimensions is not necessary.
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}`.
frontend/javascripts/dashboard/dataset/dataset_settings_provider.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/dataset_settings_data_tab.tsx (1)
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}`.
🧬 Code Graph Analysis (14)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)
frontend/javascripts/types/api_types.ts (1)
  • APIDataLayer (113-113)
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_auth_token_view.tsx (1)
frontend/javascripts/admin/account/helpers/settings_card.tsx (2)
  • SettingsCardProps (4-10)
  • 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/admin/onboarding.tsx (1)
frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (1)
  • DatasetSettingsProvider (74-387)
frontend/javascripts/dashboard/dataset/useBeforeUnload_hook.ts (1)
frontend/javascripts/libs/window.ts (1)
  • location (60-60)
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_profile_view.tsx (1)
frontend/javascripts/admin/account/helpers/settings_card.tsx (2)
  • SettingsCardProps (4-10)
  • SettingsCard (12-32)
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/admin/organization/organization_overview_view.tsx (1)
frontend/javascripts/admin/account/helpers/settings_card.tsx (1)
  • SettingsCardProps (4-10)
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/dashboard/dataset/dataset_settings_sharing_tab.tsx (8)
frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx (1)
  • useDatasetSettingsContext (36-42)
frontend/javascripts/libs/react_hooks.ts (1)
  • useWkSelector (292-294)
frontend/javascripts/libs/utils.ts (2)
  • isUserAdminOrDatasetManager (484-486)
  • isUserAdminOrTeamManager (476-478)
frontend/javascripts/admin/rest_api.ts (2)
  • getDatasetSharingToken (1379-1388)
  • revokeDatasetSharingToken (1390-1394)
frontend/javascripts/admin/account/helpers/settings_card.tsx (2)
  • SettingsCardProps (4-10)
  • SettingsCard (12-32)
frontend/javascripts/components/pricing_enforcers.tsx (1)
  • PricingEnforcedBlur (178-240)
frontend/javascripts/components/async_clickables.tsx (1)
  • AsyncButton (64-78)
frontend/javascripts/admin/account/helpers/settings_title.tsx (1)
  • SettingsTitle (5-21)
frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (2)
frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx (1)
  • useDatasetSettingsContext (36-42)
frontend/javascripts/libs/react_hooks.ts (1)
  • useWkSelector (292-294)
frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx (8)
frontend/javascripts/types/schemas/datasource.types.ts (1)
  • DatasourceConfiguration (85-92)
frontend/javascripts/types/globals.d.ts (1)
  • ArbitraryObject (32-32)
frontend/javascripts/libs/read_file.ts (1)
  • readFileAsText (1-10)
frontend/javascripts/types/api_types.ts (1)
  • APIDataStore (153-159)
frontend/javascripts/libs/utils.ts (1)
  • jsonStringify (180-182)
frontend/javascripts/admin/dataset/remote/google_auth_form_item.tsx (1)
  • GoogleAuthFormItem (11-61)
frontend/javascripts/viewer/view/action-bar/download_modal_view.tsx (1)
  • Hint (188-200)
frontend/javascripts/components/async_clickables.tsx (1)
  • AsyncButton (64-78)
🪛 GitHub Check: frontend-tests
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx

[failure] 231-231:
Property 'datasetId' is missing in type '{ children: Element; form: FormInstance; isEditingMode: false; }' but required in type 'DatasetSettingsProviderProps'.

🪛 GitHub Check: backend-tests
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx

[failure] 231-231:
Property 'datasetId' is missing in type '{ children: Element; form: FormInstance; isEditingMode: false; }' but required in type 'DatasetSettingsProviderProps'.

🪛 GitHub Actions: CI Pipeline
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx

[error] 231-231: TypeScript error TS2741: Property 'datasetId' is missing in type '{ children: Element; form: FormInstance; isEditingMode: false; }' but required in type 'DatasetSettingsProviderProps'.

🔇 Additional comments (36)
frontend/javascripts/components/async_clickables.tsx (1)

7-19: Documentation uplift looks great

The added JSDoc for AsyncButtonProps is concise and covers the new hideContentWhenLoading flag clearly.

unreleased_changes/8732.md (1)

1-3: Changelog is fine – entry is concise and sits under the correct “Changed” heading.

frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)

45-48: Good migration to the options-object form of useWatch.

• Keeps existing behaviour and opts-in to preserve, which prevents value loss when fields unmount – perfect for tab switches.
• Passing an undefined form is safe (AntD handles it).
No further action.

frontend/javascripts/admin/organization/organization_danger_zone_view.tsx (1)

57-66: Prop rename matches new SettingsCard API – all good.

frontend/javascripts/admin/account/account_password_view.tsx (3)

10-10: LGTM: Proper type import added

The import of SettingsCardProps type enables better type safety for the settings card configurations.


158-165: LGTM: Improved typing and prop naming consistency

The explicit typing as SettingsCardProps[] and the prop renaming from value to content align with the refactored SettingsCard component API. This improves type safety and maintains consistency across the codebase.


174-174: LGTM: Consistent prop usage

The usage of content prop instead of description correctly matches the updated SettingsCard component interface as shown in the helper component.

Also applies to: 192-192

frontend/javascripts/admin/onboarding.tsx (2)

23-23: LGTM: Proper provider import

The import of DatasetSettingsProvider enables the centralized state management pattern for dataset settings.


531-538: LGTM: Correct provider pattern implementation

The wrapping of DatasetSettingsView in DatasetSettingsProvider with appropriate props (isEditingMode, datasetId, onComplete, onCancel) follows the correct pattern for centralizing dataset settings state management. This aligns with the broader refactoring effort mentioned in the PR objectives.

frontend/javascripts/admin/account/account_auth_token_view.tsx (3)

7-7: LGTM: Proper type import added

The import of SettingsCardProps type enables better type safety for the API items configuration.


44-83: LGTM: Consistent prop renaming and improved typing

The changes correctly implement the SettingsCard API updates:

  • Explicit typing as SettingsCardProps[] improves type safety
  • Prop renaming from value to content and explanation to tooltip aligns with the refactored component interface
  • All array elements follow the new naming convention consistently

95-95: LGTM: Correct prop usage in component

The SettingsCard usage with content and tooltip props correctly matches the updated component interface.

frontend/javascripts/messages.tsx (2)

43-43: LGTM: Improved label clarity

The change from "Pattern Opacity" to "Segmentation Pattern Opacity" makes it clearer what type of pattern this setting controls, improving user understanding.


54-55: LGTM: Helpful tooltip addition

The new tooltip description clearly explains what the segmentationPatternOpacity setting controls, improving user guidance and accessibility.

frontend/javascripts/router/route_wrappers.tsx (2)

9-9: LGTM: Proper provider import

The import of DatasetSettingsProvider enables the centralized state management pattern for dataset settings in the routing context.


103-106: LGTM: Consistent provider pattern implementation

The wrapping of DatasetSettingsView in DatasetSettingsProvider with appropriate props (isEditingMode set to true, datasetId) follows the same pattern as in the onboarding component. This maintains consistency in how dataset settings are managed across different parts of the application.

frontend/javascripts/admin/account/account_profile_view.tsx (3)

12-12: LGTM! Proper type import and usage.

The explicit import of SettingsCardProps type improves type safety and makes the dependency on the settings card module clear.


59-87: LGTM! Consistent prop renaming aligns with SettingsCard API.

The refactoring from value to content and explanation to tooltip improves semantic clarity and aligns with the updated SettingsCard component API. The explicit typing as SettingsCardProps[] enhances type safety.


98-98: LGTM! Component usage updated correctly.

The SettingsCard component usage correctly reflects the new prop names (content and tooltip) consistent with the refactored API.

frontend/javascripts/router/router.tsx (2)

54-58: LGTM! Proper imports for nested route components.

The import of individual tab components supports the new nested routing architecture, improving modularity by allowing each tab to be rendered independently.


277-284: LGTM! Well-structured nested routing for dataset settings.

The conversion from a single route to nested routes with individual tab components is a good architectural improvement. The index route redirect to "data" ensures a sensible default tab selection. This structure supports the new context-based dataset settings architecture effectively.

frontend/javascripts/dashboard/dataset/dataset_settings_delete_tab.tsx (3)

13-14: LGTM! Proper context usage simplifies component interface.

The refactoring to use useDatasetSettingsContext instead of props aligns with the new context-based architecture and simplifies the component interface.


19-52: LGTM! Well-optimized delete handler with proper dependencies.

The conversion to useCallback with correct dependencies (dataset, navigate, queryClient) optimizes performance by preventing unnecessary re-renders. The core deletion logic, including confirmation, error handling, and cache invalidation, remains robust.


56-76: LGTM! Consistent UI layout with SettingsCard pattern.

The refactoring to use SettingsTitle, Row, Col, and SettingsCard components creates a consistent layout pattern that aligns with other settings tabs. The structured approach improves maintainability and accessibility.

frontend/javascripts/dashboard/dataset/useBeforeUnload_hook.ts (1)

5-25: LGTM! Proper useCallback conversion with correct dependencies.

The conversion to useCallback with dependencies on hasUnsavedChanges and message is appropriate for performance optimization and prevents unnecessary re-renders.

frontend/javascripts/dashboard/dataset/dataset_settings_metadata_tab.tsx (2)

6-37: LGTM! Well-structured metadata items array with proper typing.

The metadataItems array with explicit SettingsCardProps[] typing creates a clean, maintainable structure. Each item properly defines title, tooltip, and form content, making the component more modular and consistent with the settings card pattern.


39-50: LGTM! Consistent layout structure with proper spacing.

The use of SettingsTitle, Row, and Col components creates a consistent layout pattern that aligns with other settings tabs. The mapping over metadataItems maintains DRY principles while providing a clean, accessible grid structure.

frontend/javascripts/admin/account/helpers/settings_card.tsx (1)

14-30: Component looks good

Prop rename and Tooltip swap are clean; no issues found.

frontend/javascripts/dashboard/dataset/dataset_settings_viewconfig_tab.tsx (1)

22-348: Well-executed refactoring to use context and SettingsCard UI

The component has been successfully refactored to:

  • Use useDatasetSettingsContext for state management
  • Adopt the new SettingsCard-based UI pattern
  • Properly handle dataset availability with conditional rendering

The changes align perfectly with the broader architectural improvements in the dataset settings feature.

frontend/javascripts/dashboard/dataset/dataset_settings_sharing_tab.tsx (1)

19-149: Excellent refactoring with improved data fetching

The component has been successfully modernized with:

  • React Query for efficient sharing token management
  • Proper memoization of computed values
  • Clean UI using SettingsCard components
  • Built-in copy functionality via Typography.Paragraph

The refactoring significantly improves code maintainability and performance.

frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (1)

35-242: Excellent UI refactoring with improved navigation

The component has been successfully transformed to:

  • Use sidebar menu navigation instead of tabs
  • Delegate all state management to the context provider
  • Properly integrate with React Router for nested routes
  • Show validation errors directly in menu items

The refactoring significantly improves user experience with better navigation and clearer visual feedback.

frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx (2)

36-69: Well-implemented layer merging logic

The function correctly handles merging layers and resolving naming conflicts by appending numeric suffixes.


110-120: No stale‐closure risk in useEffect
The effect only fires when datasourceUrl goes from null to defaultUrl, and at that moment React schedules the callback with a fresh closure of handleExplore (including the current datasourceUrl, showCredentialsFields, fileList, etc.). After the initial auto-explore, any changes to credentials or file list are picked up by manual calls to the latest handleExplore (via the button’s onClick), so there’s no staleness or race here. Wrapping in useCallback and adding it to deps would either re-run the effect every render or pull in many state values just to satisfy eslint, without improving correctness. You can leave the effect as-is.

Likely an incorrect or invalid review comment.

frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (3)

86-91: Good addition of rotation field synchronization

The addition of rotation field synchronization when copying from advanced to simple mode ensures data consistency across edit modes.


100-100: Good fix for stale values with preserve option

Adding preserve: true to Form.useWatch prevents stale values when the form updates, which is a good improvement for form stability.


471-475: Improved label clarity

Changing "Element Class" to "Data Type" with the additional info about dtype improves clarity for users who may be more familiar with the term "data type".

Comment on lines +38 to +57
const onSortEnd = useCallback(
(event: DragEndEvent) => {
const { active, over } = event;

if (active && over && colorLayerNames) {
const oldIndex = colorLayerNames.indexOf(active.id as string);
const newIndex = colorLayerNames.indexOf(over.id as string);
if (active && over && colorLayerNames) {
const oldIndex = colorLayerNames.indexOf(active.id as string);
const newIndex = colorLayerNames.indexOf(over.id as string);

document.body.classList.remove("is-dragging");
document.body.classList.remove("is-dragging");

if (oldIndex !== newIndex && onChange) {
const movedElement = colorLayerNames[oldIndex];
const newColorLayerNames = colorLayerNames.filter((_, index) => index !== oldIndex);
newColorLayerNames.splice(newIndex, 0, movedElement);
onChange(newColorLayerNames);
if (oldIndex !== newIndex && onChange) {
const movedElement = colorLayerNames[oldIndex];
const newColorLayerNames = colorLayerNames.filter((_, index) => index !== oldIndex);
newColorLayerNames.splice(newIndex, 0, movedElement);
onChange(newColorLayerNames);
}
}
}
};
},
[colorLayerNames, onChange],
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

is-dragging class may stick on cancel/outside drop

document.body.classList.remove("is-dragging") is executed only when active && over && colorLayerNames are truthy.
If the user drops outside a valid target (over === null) the class remains, leaving the page in a “dragging” visual state.

Move the removal outside the conditional (or into the finally of a try/finally block) so it always fires.

-  if (active && over && colorLayerNames) {
+  if (active && over && colorLayerNames) {
     ...
-    document.body.classList.remove("is-dragging");
-    if (oldIndex !== newIndex && onChange) {
+    if (oldIndex !== newIndex && onChange) {
       ...
     }
   }
+  document.body.classList.remove("is-dragging");
🤖 Prompt for AI Agents
In frontend/javascripts/dashboard/dataset/color_layer_ordering_component.tsx
around lines 38 to 57, the removal of the "is-dragging" class from document.body
is currently inside a conditional that only runs if active, over, and
colorLayerNames are truthy. This causes the class to remain if the drop happens
outside a valid target (over is null). To fix this, move the
document.body.classList.remove("is-dragging") call outside and before the
conditional so it always executes regardless of the drop target.

Comment on lines 231 to 233
<DatasetSettingsProvider form={form} isEditingMode={false}>
<DatasetSettingsDataTab />
</DatasetSettingsProvider>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix missing required datasetId prop

The DatasetSettingsProvider requires a datasetId prop, but it's not provided here. Since this is for creating a new dataset, there's no ID yet.

Consider one of these solutions:

Option 1 (Recommended): Make datasetId optional in DatasetSettingsProvider for creation mode:

// In dataset_settings_provider.tsx
type DatasetSettingsProviderProps = {
  children: React.ReactNode;
-  datasetId: string;
+  datasetId?: string;
  isEditingMode: boolean;
  // ...
}

Option 2: Pass a temporary identifier:

-            <DatasetSettingsProvider form={form} isEditingMode={false}>
+            <DatasetSettingsProvider form={form} isEditingMode={false} datasetId="new">

The provider should skip data fetching when isEditingMode is false and no valid datasetId is provided.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<DatasetSettingsProvider form={form} isEditingMode={false}>
<DatasetSettingsDataTab />
</DatasetSettingsProvider>
<DatasetSettingsProvider form={form} isEditingMode={false} datasetId="new">
<DatasetSettingsDataTab />
</DatasetSettingsProvider>
🧰 Tools
🪛 GitHub Check: frontend-tests

[failure] 231-231:
Property 'datasetId' is missing in type '{ children: Element; form: FormInstance; isEditingMode: false; }' but required in type 'DatasetSettingsProviderProps'.

🪛 GitHub Check: backend-tests

[failure] 231-231:
Property 'datasetId' is missing in type '{ children: Element; form: FormInstance; isEditingMode: false; }' but required in type 'DatasetSettingsProviderProps'.

🪛 GitHub Actions: CI Pipeline

[error] 231-231: TypeScript error TS2741: Property 'datasetId' is missing in type '{ children: Element; form: FormInstance; isEditingMode: false; }' but required in type 'DatasetSettingsProviderProps'.

🤖 Prompt for AI Agents
In frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx around lines
231 to 233, the DatasetSettingsProvider is missing the required datasetId prop.
Since this is creation mode and no datasetId exists yet, update the
DatasetSettingsProvider component to make the datasetId prop optional. Modify
its implementation so that when isEditingMode is false and datasetId is not
provided or invalid, it skips any data fetching logic. This allows the provider
to work correctly without a datasetId during new dataset creation.

Comment on lines +175 to +176
// @ts-ignore
const preferredVoxelSize = Utils.parseMaybe(datasourceConfigStr)?.scale;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove @ts-ignore and add proper typing

Using @ts-ignore hides potential type issues. The code should properly type the parsed result.

-      // @ts-ignore
-      const preferredVoxelSize = Utils.parseMaybe(datasourceConfigStr)?.scale;
+      const parsedDatasource = Utils.parseMaybe(datasourceConfigStr) as DatasourceConfiguration | null;
+      const preferredVoxelSize = parsedDatasource?.scale;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// @ts-ignore
const preferredVoxelSize = Utils.parseMaybe(datasourceConfigStr)?.scale;
const parsedDatasource = Utils.parseMaybe(datasourceConfigStr) as DatasourceConfiguration | null;
const preferredVoxelSize = parsedDatasource?.scale;
🤖 Prompt for AI Agents
In frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx around lines
175 to 176, remove the @ts-ignore comment and instead define proper TypeScript
types for the result of Utils.parseMaybe. Update the code to explicitly type the
parsed object so that accessing the scale property is type-safe and does not
require ignoring TypeScript errors.

if (onError) onError();
};

async function handleExplore() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Wrap async function in useCallback

The handleExplore function should be wrapped in useCallback to prevent unnecessary re-renders and ensure stable references when used in effects and event handlers.

-  async function handleExplore() {
+  const handleExplore = useCallback(async () => {

Add the closing bracket and dependencies after line 250:

-  }
+  }, [datasourceUrl, form, uploadableDatastores, showCredentialsFields, selectedProtocol, fileList, usernameOrAccessKey, passwordOrSecretKey, setDatasourceConfigStr, onSuccess, onError]);

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx at line 157,
the async function handleExplore should be wrapped in React's useCallback hook
to avoid unnecessary re-renders and maintain stable function references.
Refactor handleExplore by wrapping its definition with useCallback, adding the
appropriate dependency array after the function body, and ensure the closing
bracket for useCallback is added after line 250.

onError?: () => void;
defaultUrl?: string | null | undefined;
}) {
const isDatasourceConfigStrFalsy = Form.useWatch("dataSourceJson", form) != null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Misleading variable name

The variable name isDatasourceConfigStrFalsy is misleading as it actually checks if the value is NOT null (truthy). Consider renaming to hasDataSourceJson or isDataSourceJsonPresent for clarity.

-  const isDatasourceConfigStrFalsy = Form.useWatch("dataSourceJson", form) != null;
+  const hasDataSourceJson = Form.useWatch("dataSourceJson", form) != null;

Also update line 360:

-              type={isDatasourceConfigStrFalsy ? "primary" : "default"}
+              type={hasDataSourceJson ? "primary" : "default"}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const isDatasourceConfigStrFalsy = Form.useWatch("dataSourceJson", form) != null;
- const isDatasourceConfigStrFalsy = Form.useWatch("dataSourceJson", form) != null;
+ const hasDataSourceJson = Form.useWatch("dataSourceJson", form) != null;
- type={isDatasourceConfigStrFalsy ? "primary" : "default"}
+ type={hasDataSourceJson ? "primary" : "default"}
🤖 Prompt for AI Agents
In frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx at line 101,
the variable name isDatasourceConfigStrFalsy is misleading because it checks for
a non-null value, indicating truthiness. Rename this variable to
hasDataSourceJson or isDataSourceJsonPresent to better reflect its purpose.
Also, update any references to this variable, including the usage at line 360,
to use the new name for consistency and clarity.


// biome-ignore lint/correctness/useExhaustiveDependencies: Always revalidate in case the user changes the data layers in the form.
React.useEffect(() => {
useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Missing dependency in useEffect

The form dependency should be included in the useEffect dependency array to ensure the effect runs when the form instance changes.

-  }, [dataLayers]);
+  }, [dataLayers, form]);

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx at line
371, the useEffect hook is missing the 'form' dependency in its dependency
array. Add 'form' to the dependency array of this useEffect to ensure the effect
re-runs whenever the form instance changes.

hotzenklotz and others added 3 commits July 16, 2025 15:42
…er.tsx

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for beautifying the setting. I left some comments and did some rudimentary testing.

Testing:

  • Not sure whether this pr cause this, but I can no longer configure the color layer order in the view configuration tab for a dataset with two color layers.

UI Feedback:

  • Imo the title of the configuration items shouldn't be gray in light mode. Because this is usually the color indicating an item is disabled / not available. But in the settings this is used for basic information like here:
image I was initially thinking that e.g. in the View configuration tab the items were all disabled and not configurable due to the gray title. Imo this should be changed to have the default black color we usually use or something else.

Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got one mini question left

"data" | "general" | "defaultConfig",
boolean
> => {
// Antd's form.getFieldsError only returns errors for mounted form fields, i.e. from the current tab :anger:
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer Jul 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is this now an actual problem?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. There is no good way around this with antd forms. As I learned, errors rely in the form items rule prop for validation. When an item is not mounted anymore, antd's form just gives up.

In other words, to properly fix this state and validation rules should be discoupled from UI/react components. Perhaps, this could be a good usecase for Tanstack Forms. See #8457

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind you, the backend is also doing validation and will block invalid states.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this is not a problem and thus can be merged 🤔?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and no. Technically it is problem. If you making changes in all 4 tabs including errors this will not catch it. In reality people tend to only make changes in one tab and then it's fine. I don't think it is a blocker and would merge it as is but we should be aware of it.

@MichaelBuessemeyer
Copy link
Contributor

Note https://scm.slack.com/archives/C5AKLAV0B/p1753181376289659 regarding

UI Feedback:
Imo the title of the configuration items shouldn't be gray in light mode. Because this is usually the color indicating an item is disabled / not available. But in the settings this is used for basic information like here:
image
I was initially thinking that e.g. in the View configuration tab the items were all disabled and not configurable due to the gray title. Imo this should be changed to have the default black color we usually use or something else.

@hotzenklotz hotzenklotz merged commit 701c8e2 into functional_dataset_settings Jul 22, 2025
5 checks passed
@hotzenklotz hotzenklotz deleted the dataset_settings_context branch July 22, 2025 12:27
hotzenklotz added a commit that referenced this pull request Jul 24, 2025
Update TanStack Query / `react-query` from version v4 to v5.

I followed the official migration guide:
https://tanstack.com/query/latest/docs/framework/react/guides/migrating-to-v5

Main changes:
- `useQuery` hooks now always take all query settings as an object, e.g.
`useQuery(["dataets"], fetchMyDatasets, {...otherOptions})` -->
`useQuery({queryKey: ["dataets"], queryFn: fetchMyDatasets,
...otherOptions)`
- `isLoading` has been renamed to `isPending`
- the package `@tanstack/query-sync-storage-persister` has been marked
as `deprecated` and was replaced with
`@tanstack/query-ASYNC-storage-persister`.
[Docs](https://tanstack.com/query/v5/docs/framework/react/plugins/createSyncStoragePersister#deprecated)

### TODOs:
- [x] Blocked by #8732: `<DatasetSettingView>` imports `defaultContext`
which is not available anymore. Should be fixed by converting to to a
functional component in PR #8732

### Testing
- Check that the dashboard loads correctly and fetches all information
for a selected dataset (right info panel)
- Check that Zarr-Link can be created/fetched correctly

### Issues:
- fixes Nada

------
(Please delete unneeded items, merge only when none are left open)
- [ ] 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
@coderabbitai coderabbitai bot mentioned this pull request Aug 28, 2025
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants