Remove advanced dataset settings toggle #8954
Conversation
📝 WalkthroughWalkthroughRemoves the JSON-based advanced dataset editor and related sync/validation helpers; switches form types to structured Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (1)
47-49: Update ErrorBoundary message to remove stale “JSON” wording.RetryingErrorBoundary still tells users to “Ensure that the JSON is valid,” which is misleading now that the JSON editor is gone. Please update the copy.
Here’s a minimal change for frontend/javascripts/dashboard/dataset/helper_components.tsx (render message):
message={ <span> An error occurred while processing the configuration. Please check the form inputs. {this.state.error.toString()} </span> }frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (1)
323-325: Remove stale reference to deleted edit‑mode “force update”.The commented line referencing setActiveDataSourceEditMode is dead and confusing. Drop it and tighten the comment.
- // 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); + // Delay validation to next tick so all form fields are mounted before validation setTimeout(afterForceUpdateCallback, 0);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx(0 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx(1 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx(6 hunks)frontend/javascripts/types/validation.ts(0 hunks)
💤 Files with no reviewable changes (2)
- frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx
- frontend/javascripts/types/validation.ts
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (3)
frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx (1)
useDatasetSettingsContext(35-41)frontend/javascripts/admin/account/helpers/settings_title.tsx (1)
SettingsTitle(5-21)frontend/javascripts/dashboard/dataset/helper_components.tsx (1)
RetryingErrorBoundary(61-106)
frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (1)
frontend/javascripts/dashboard/dataset/helper_components.tsx (1)
hasFormError(123-129)
⏰ 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: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (9)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (3)
19-19: Import cleanup looks good.Consolidating to use FormItemWithInfo and RetryingErrorBoundary here aligns with the single‑mode UI.
35-35: Context usage simplified — LGTM.Using only { dataset, form } fits the new provider surface.
27-27: syncValidator exported; legacy JSON validators not referenced; advanced-mode fields remain
- syncValidator exported at frontend/javascripts/types/validation.ts:78
- No references found for validateDatasourceJSON / isDatasourceJSONValid / isValidJSON
- dataSourceJson and syncDataSourceFields still appear in:
- frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx
- frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx
Remove those advanced-mode leftovers or confirm they are intentional.frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (6)
18-18: Type import tidy‑up — LGTM.Keeping MutableAPIDataset for the error-status clone path is appropriate.
31-31: FormData import — LGTM.Matches the narrowed context API.
172-174: Include datasetRotation in “data” validation bucket — LGTM.This ensures rotation errors route to the Data tab.
216-219: Fetching dataSource via getFieldValue — LGTM.Avoids stale-useWatch pitfalls and matches the comment rationale.
236-243: Directly using formValues.dataSource — LGTM.Consistent with the simplified single‑mode flow.
349-361: Provider surface looks consistent with the removal of advanced mode.No leftover mode handlers in the context value — good.
If not already done across the repo, please run the earlier script to confirm no lingering references to:
- dataSourceJson
- activeDataSourceEditMode
- handleDataSourceEditModeChange
- syncDataSourceFields
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/javascripts/dashboard/dataset/helper_components.tsx (1)
109-123: Don't override caller-provided onOk/onCancel — wrap and invoke them.Spreading
optsbefore definingonOk/onCancelcurrently shadows caller callbacks and can silently break side effects; destructure and wrap the original handlers and resolve after calling them.File: frontend/javascripts/dashboard/dataset/helper_components.tsx (lines ~109-123)
-export const confirmAsync = (opts: Record<string, any>): Promise<boolean> => { - return new Promise((resolve) => { - Modal.confirm({ - ...opts, - - onOk() { - resolve(true); - }, - - onCancel() { - resolve(false); - }, - }); - }); -}; +export const confirmAsync = ( + // If importing ModalFuncProps is problematic, keep Record<string, any> but still wrap handlers. + opts: any, +): Promise<boolean> => { + return new Promise((resolve) => { + const { onOk: onOkOrig, onCancel: onCancelOrig, ...rest } = opts || {}; + Modal.confirm({ + ...rest, + onOk: async (...args: any[]) => { + try { + if (typeof onOkOrig === "function") { + await onOkOrig(...args); + } + resolve(true); + } catch (e) { + // Propagate rejection so the modal can remain open per antd behavior + return Promise.reject(e); + } + }, + onCancel: (...args: any[]) => { + try { + if (typeof onCancelOrig === "function") { + onCancelOrig(...args); + } + } finally { + resolve(false); + } + }, + }); + }); +};Optional: add proper typing (verify import path for your antd version):
// Option A (v5 likely): import type { ModalFuncProps } from "antd"; // Option B (v4 style, verify path): // import type { ModalFuncProps } from "antd/lib/modal/confirm"; type ConfirmOpts = Omit<ModalFuncProps, "onOk" | "onCancel"> & Partial<Pick<ModalFuncProps, "onOk" | "onCancel">>;frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (1)
428-437: Incorrect warning message for Bounding Box fieldThe warning text references “manually chosen segment IDs,” which is unrelated to bounding box. Either remove this warning (field is already required) or replace with a bounding-box relevant hint.
-{ - warningOnly: true, - validator: (_rule, value) => - value == null || value === "" - ? Promise.reject( - new Error( - "When left empty, annotating this layer later will only be possible with manually chosen segment IDs.", - ), - ) - : Promise.resolve(), -}, +// Consider removing this warning entirely, or replace with a bbox-specific info Tooltip instead.
🧹 Nitpick comments (7)
frontend/javascripts/admin/rest_api.ts (1)
992-998: Avoid appending a trailing “?” when no query params are setWhen
sharingTokenis null, the URL becomes/api/datasets/${datasetId}?. Build the URL conditionally to keep it clean and cache-friendly.- return Request.receiveJSON(`/api/datasets/${datasetId}?${params}`, options); + const qs = params.toString(); + const url = qs ? `/api/datasets/${datasetId}?${qs}` : `/api/datasets/${datasetId}`; + return Request.receiveJSON(url, options);frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx (3)
47-66: Harden protocol detection: trim input and use case-insensitive checksURLs with leading/trailing spaces or uppercase schemes will currently fail validation. Trim first and use case-insensitive regex for scheme checks.
-const detectProtocol = (url: string): Protocol => { - const cleanUrl = removePrefixes(url, NEUROGLANCER_PREFIXES); - - if (cleanUrl.startsWith("https://") || cleanUrl.startsWith("http://")) { +const detectProtocol = (url: string): Protocol => { + const cleanUrl = removePrefixes(url.trim(), NEUROGLANCER_PREFIXES); + + if (/^https?:\/\//i.test(cleanUrl)) { return "https"; } - if (cleanUrl.startsWith("s3://")) { + if (/^s3:\/\//i.test(cleanUrl)) { return "s3"; } - if (cleanUrl.startsWith("gs://")) { + if (/^gs:\/\//i.test(cleanUrl)) { return "gs"; } - if (cleanUrl.startsWith("file://")) { + if (/^file:\/\//i.test(cleanUrl)) { return "file"; }
309-346: Hide credentials UI for file:// protocol
file://does not need credentials and the current UI suggests otherwise.-{showCredentialsFields && ( +{showCredentialsFields && selectedProtocol !== "file" && ( <> {selectedProtocol === "gs" ? (
193-199: Guard against missing datastore selection with clearer validationThe thrown error is fine, but consider adding a required rule for the datastore field to prevent reaching this branch.
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (2)
26-56: Merging layers: handles duplicates well, but consider stable orderingThe rename-on-duplicate logic is solid. Consider preserving original relative order by keeping existing layers first when merging for a more predictable UX.
- const allLayers = newDatasource.dataLayers.concat(existingDatasource.dataLayers); + const allLayers = existingDatasource.dataLayers.concat(newDatasource.dataLayers);
127-129: Setting team to empty string may be invalid
form.setFieldValue(["dataSource", "id"], { name: defaultDatasetName, team: "" });might violate backend expectations. Consider defaultingteamto the active org/team or omit it if optional.frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (1)
324-325: Remove dead code comment related to advanced edit modeThe commented force-update line references removed advanced-mode state. Safe to drop.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
frontend/javascripts/admin/dataset/composition_wizard/common.ts(1 hunks)frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx(8 hunks)frontend/javascripts/admin/dataset/dataset_add_view.tsx(1 hunks)frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx(5 hunks)frontend/javascripts/admin/rest_api.ts(2 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx(1 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx(4 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx(9 hunks)frontend/javascripts/dashboard/dataset/helper_components.tsx(2 hunks)frontend/javascripts/test/helpers/apiHelpers.ts(0 hunks)frontend/javascripts/viewer/model_initialization.ts(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/javascripts/test/helpers/apiHelpers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T11:34:16.411Z
Learnt from: knollengewaechs
PR: scalableminds/webknossos#8796
File: frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx:68-77
Timestamp: 2025-08-27T11:34:16.411Z
Learning: The webknossos codebase uses `getDefaultLayerViewConfiguration` to ensure that layer configurations always exist in `datasetConfiguration.layers`, making direct access like `datasetConfiguration.layers[colorLayer.name]` safe across multiple files including api_latest.ts and load_histogram_data_saga.ts.
Applied to files:
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx
🧬 Code graph analysis (6)
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (3)
frontend/javascripts/types/schemas/datasource.types.ts (1)
DatasourceConfiguration(80-87)frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx (1)
DatasetSettingsFormData(9-15)frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx (1)
AddRemoteLayer(93-388)
frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx (8)
frontend/javascripts/admin/dataset/composition_wizard/common.ts (1)
FileList(8-8)frontend/javascripts/types/api_types.ts (1)
APIDataStore(153-159)frontend/javascripts/types/schemas/datasource.types.ts (1)
DatasourceConfiguration(80-87)frontend/javascripts/types/globals.d.ts (1)
ArbitraryObject(32-32)frontend/javascripts/libs/read_file.ts (1)
readFileAsText(1-10)frontend/javascripts/libs/format_utils.ts (1)
formatScale(142-161)frontend/javascripts/admin/dataset/remote/google_auth_form_item.tsx (1)
GoogleAuthFormItem(10-60)frontend/javascripts/components/async_clickables.tsx (1)
AsyncButton(64-78)
frontend/javascripts/admin/dataset/composition_wizard/common.ts (1)
frontend/javascripts/admin/rest_api.ts (1)
getDataset(987-998)
frontend/javascripts/viewer/model_initialization.ts (2)
frontend/javascripts/admin/rest_api.ts (1)
getDataset(987-998)frontend/javascripts/admin/api/token.ts (1)
getSharingTokenFromUrlParameters(27-37)
frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (3)
frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx (1)
DatasetSettingsFormData(9-15)frontend/javascripts/admin/rest_api.ts (1)
getDataset(987-998)frontend/javascripts/dashboard/dataset/helper_components.tsx (1)
hasFormError(125-131)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (3)
frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx (1)
useDatasetSettingsContext(38-44)frontend/javascripts/admin/account/helpers/settings_title.tsx (1)
SettingsTitle(5-21)frontend/javascripts/dashboard/dataset/helper_components.tsx (1)
RetryingErrorBoundary(61-107)
⏰ 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 (12)
frontend/javascripts/dashboard/dataset/helper_components.tsx (1)
96-98: Copy update aligns with removal of JSON editing — looks good.Message now refers to “configuration” instead of “JSON,” consistent with the feature removal.
Consider using
this.state.error.messageinstead oftoString()to avoid repeating the “Error:” prefix, or prepend a separator (e.g.,": "). Optional.frontend/javascripts/admin/rest_api.ts (1)
1012-1013: Signature usage LGTM
getDatasetLegacynow calls the updatedgetDatasetsignature correctly.frontend/javascripts/admin/dataset/composition_wizard/common.ts (1)
48-49: Updated callsite LGTMSwitched to
getDataset(id, null, { showErrorToast: false })matches the new signature.frontend/javascripts/viewer/model_initialization.ts (1)
313-314: Updated usage LGTMPassing the sharing token as the second arg to
getDatasetaligns with the new API.frontend/javascripts/admin/dataset/dataset_add_view.tsx (1)
113-113: UI contrast improvement LGTMBackground application on Layout is a nice touch and aligns with the PR intent.
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (2)
1-1: Importing ExportOutlined LGTMMatches the new superuser “view raw API response” button.
47-51: Removing the advanced mode UI is consistent with the PR goalConsolidating to the simple form via an error boundary fits the cleanup.
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (1)
215-231: Propagating preferred voxel size LGTMPassing
datasourceConfig?.scaledown enables nicer warnings on scale mismatch.frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (3)
98-107: Dataset fetch path LGTMUpdated
getDataset(datasetId)usage aligns with the new signature.
172-174: Validation summary keys LGTMCombining
dataSourceanddatasetRotationunder data is consistent with the simplified UI.
328-332: onValuesChange simplification LGTMMinimal change tracking is sufficient after removing advanced/sync logic.
frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx (1)
221-246: Confirm backend support for file:// explorationDocs (docs/data/streaming.md) list supported protocols as HTTPS, Amazon S3 and Google Cloud Storage — no evidence file:// is supported. Before advertising file:// in the UI (frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx), confirm /api/datasets/exploreRemote accepts file:// across environments or remove/guard file://.
| const onSuccessfulExplore = async (url: string, newDataSourceConfig: DatasourceConfiguration) => { | ||
| const datasourceConfig = form.getFieldValue("dataSource"); | ||
| const mergedConfig = mergeNewLayers(datasourceConfig, newDataSourceConfig); | ||
| form.setFieldValue("dataSource", mergedConfig); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Type shape mismatch risk: DatasourceConfiguration vs APIDataSource
onSuccessfulExplore merges a DatasourceConfiguration into dataSource and writes it back to the form, which elsewhere is typed as APIDataSource and later sent to storeRemoteDataset. Ensure the merged object matches the APIDataSource shape expected by the backend (notably id structure).
🏁 Script executed:
#!/bin/bash
# Inspect type definitions and usages to confirm shapes are compatible
rg -n -C2 'type\s+APIDataSource\b|type\s+DatasourceConfiguration\b' frontend/javascripts/types
# Find where storeRemoteDataset is called and ensure the form's dataSource matches APIDataSource
rg -n -C2 'storeRemoteDataset\(|getFieldValue\("dataSource"\)' frontend/javascripts
# Check if APIDataSourceId expects 'owningOrganization' vs 'team'
rg -n -C2 'type\s+APIDataSourceId\b|owningOrganization' frontend/javascripts/typesLength of output: 5626
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- datasource.types.ts (around DatasourceConfiguration) ----"
sed -n '1,240p' frontend/javascripts/types/schemas/datasource.types.ts || true
echo
echo "---- api_types.ts (around APIDataSource / APIDataSourceId) ----"
sed -n '1,320p' frontend/javascripts/types/api_types.ts || true
echo
echo "---- dataset_add_remote_view.tsx (around onSuccessfulExplore & storeRemoteDataset) ----"
sed -n '1,260p' frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx || true
echo
echo "---- search for mergeNewLayers ----"
rg -n --hidden "mergeNewLayers" || true
echo
echo "---- show mergeNewLayers definition(s) ----"
rg -n --hidden "mergeNewLayers" -S --line-number --no-heading -g 'frontend/**' || true
rg -n --hidden "function mergeNewLayers|const mergeNewLayers" -S -g 'frontend/**' || trueLength of output: 21533
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- show storeRemoteDataset in admin/rest_api.ts ----"
sed -n '1200,1280p' frontend/javascripts/admin/rest_api.ts || true
echo
echo "---- locate dataset_settings_context file ----"
rg -n "dataset_settings_context" frontend/javascripts -S || true
echo
echo "---- show DatasetSettingsFormData type (if present) ----"
sed -n '1,320p' frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx || sed -n '1,320p' frontend/javascripts/dashboard/dataset/dataset_settings_context.ts || true
echo
echo "---- search for any conversion helpers between DatasourceConfiguration and APIDataSource ----"
rg -n "DatasourceConfiguration|APIDataSource|dataSourceTo|convert.*DataSource|toAPIDataSource|fromSchema" frontend/javascripts -S || trueLength of output: 8989
Mismatch between schema-derived DatasourceConfiguration and APIDataSource shapes
onSuccessfulExplore writes a DatasourceConfiguration (with dataLayers: { mags, additionalCoordinates, boundingBox }) directly into the form and later passes it to storeRemoteDataset, which expects an APIDataSource (with dataLayers: { resolutions, additionalAxes, boundingBox: BoundingBoxObject, … }). Convert each DataLayer to the API shape (e.g. rename mags→resolutions, additionalCoordinates→additionalAxes, adjust boundingBox format and optional fields) before calling storeRemoteDataset.
🤖 Prompt for AI Agents
In frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx around lines
114–118, the code writes a schema-derived DatasourceConfiguration into the form
but later the flow expects an APIDataSource shape; before saving or passing to
storeRemoteDataset you must map each DataLayer to the API shape: rename mags →
resolutions, additionalCoordinates → additionalAxes, convert boundingBox to the
API BoundingBoxObject format (and omit/normalize empty/optional fields), ensure
other optional fields match API names/types, and then set the transformed object
into the form or pass the transformed APIDataSource to storeRemoteDataset so
types match.
| form: formProp, // In case of Remote Dataset Upload, we start with a prefilled form containing the DS information | ||
| }) => { | ||
| const [form] = Form.useForm<FormData>(formProp); | ||
| const [form] = Form.useForm<DatasetSettingsFormData>(formProp); |
There was a problem hiding this comment.
Form instance initialization bug: useForm does not take an argument
Form.useForm(formProp) is not a valid Ant Design pattern. Create an internal form and fall back to the provided one.
- const [form] = Form.useForm<DatasetSettingsFormData>(formProp);
+ const [internalForm] = Form.useForm<DatasetSettingsFormData>();
+ const form = formProp ?? internalForm;📝 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.
| const [form] = Form.useForm<DatasetSettingsFormData>(formProp); | |
| const [internalForm] = Form.useForm<DatasetSettingsFormData>(); | |
| const form = formProp ?? internalForm; |
🤖 Prompt for AI Agents
In frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx around
line 77, Form.useForm is incorrectly called with formProp; change to create an
internal form with Form.useForm<DatasetSettingsFormData>() and then set the
effective form to use formProp ?? internalForm (i.e. const [internalForm] =
Form.useForm<DatasetSettingsFormData>(); const form = formProp ??
internalForm;), and ensure the Form component and all handlers use this
effective form variable so a provided form is used when passed and an internal
one is used as a fallback.
…ab.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (1)
522-524: Harden optional chaining to avoid runtime errors whendataStoreis missing
dataset?.dataStore.jobsSupportedByAvailableWorkers.includes(...)will throw ifdataStoreis null/undefined. Chain through safely.Apply this diff:
- {dataset?.dataStore.jobsSupportedByAvailableWorkers.includes( - APIJobType.FIND_LARGEST_SEGMENT_ID, - ) ? ( + {dataset?.dataStore?.jobsSupportedByAvailableWorkers?.includes( + APIJobType.FIND_LARGEST_SEGMENT_ID, + ) ? (
🧹 Nitpick comments (2)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (2)
47-51: Decide on keeping the error boundary and remove the TODOIf we keep only the simple form path, the boundary still adds resilience against runtime errors in validators/inputs. Either keep it and drop the TODO or remove both.
124-128: Disable the copy button when no dataset IDUX nit: When creating a dataset, the copy button does nothing. Disable it when
dataset?.idis falsy.Apply this change outside the selected lines (near Lines 139-141):
- <Tooltip title="Copy dataset ID"> - <Button onClick={() => copyDatasetID(dataset?.id)} icon={<CopyOutlined />} /> + <Tooltip title="Copy dataset ID"> + <Button + disabled={!dataset?.id} + onClick={() => copyDatasetID(dataset?.id)} + icon={<CopyOutlined />} + /> </Tooltip>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (3)
frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx (1)
useDatasetSettingsContext(38-44)frontend/javascripts/admin/account/helpers/settings_title.tsx (1)
SettingsTitle(5-21)frontend/javascripts/dashboard/dataset/helper_components.tsx (1)
RetryingErrorBoundary(61-107)
⏰ 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_data_tab.tsx (4)
1-1: LGTM: icon imports
ExportOutlinedis used; no unused imports.
19-19: LGTM: simplified importsRemoving
Hideablealigns with dropping the advanced mode.
27-27: LGTM: validator import
syncValidatoris used consistently across rules.
134-136: Nice: addedrel="noopener noreferrer"Prevents reverse tabnabbing. Good catch.
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx
Outdated
Show resolved
Hide resolved
…ove-advanced-dataset-settings
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (1)
101-109: Incorrect error check causes spurious navigation
getFieldError(...).filter(...)returns an array; comparing tonullwill always be falsey, so navigation runs even when there’s no “already taken” error.Fix:
- const maybeDSNameError = form - .getFieldError(["dataset", "name"]) - .filter((error) => error === messages["dataset.name.already_taken"]); - if (maybeDSNameError == null) return; + const maybeDSNameError = form + .getFieldError(["dataset", "name"]) + .filter((error) => error === messages["dataset.name.already_taken"]); + if (maybeDSNameError.length === 0) return;
🧹 Nitpick comments (8)
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (2)
8-9: Stale comment references removed advanced modeThe inline comment still mentions syncing with “advanced”/JSON. Please update to avoid confusion.
84-84: Type-safety on watched datasource andscale
Form.useWatch(["dataSource"], form)is untyped, and you access.scale. Add a concrete type (APIDataSource or the converted shape) to avoid accidental shape drift.Example:
-const datasourceConfig = Form.useWatch(["dataSource"], form); +const datasourceConfig = Form.useWatch<APIDataSource>(["dataSource"], form);Also applies to: 217-217, 226-231
frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx (6)
36-66: Protocol support vs UI tooltip inconsistentYou accept file:// in detectProtocol but the tooltip lists only HTTPS/S3/GCS. Align the tooltip.
- tooltip="Supported protocols are HTTPS, Amazon S3 and Google Cloud Storage" + tooltip="Supported protocols are HTTPS, Amazon S3, Google Cloud Storage and file://"Also applies to: 281-286
145-157: Avoid side effects in field validatorsCalling
handleFailure()inside the URL validator toggles component error/loading state during typing. Validators should be pure.- } catch (error) { - handleFailure(); - return Promise.reject(error); - } + } catch (error) { + return Promise.reject(error); + }
159-170: Graceful default for single-datastore setupsIf only one uploadable datastore exists and the form field is unset/hidden, exploration fails. Default to the single datastore.
- const datastoreToUse = uploadableDatastores.find( + let datastoreToUse = uploadableDatastores.find( (datastore) => form.getFieldValue("datastoreUrl") === datastore.url, ); - - if (!datastoreToUse) { - throw new Error("Could not find datastore that allows uploading."); - } + if (!datastoreToUse && uploadableDatastores.length === 1) { + datastoreToUse = uploadableDatastores[0]; + } + if (!datastoreToUse) { + throw new Error("Could not find datastore that allows uploading."); + }
239-245: Scale comparison may be too strict
isEqual(preferredVoxelSize, newDataSourceConfig.scale)can fail due to non-essential fields/ordering. Consider comparing onlyfactor(if that’s the contract).- if (preferredVoxelSize?.factor && !isEqual(preferredVoxelSize, newDataSourceConfig.scale)) { + if (preferredVoxelSize?.factor && !isEqual(preferredVoxelSize.factor, newDataSourceConfig.scale?.factor)) {
309-346: Form.Item rules without name are no-opsThese Form.Items lack a
name, sorules/hasFeedbackwon’t validate or show status. Either bind them to form fields or remove the rules and manage validation in local state.Minimal fix (remove rules/hasFeedback):
- <FormItem - label={authLabel.username} - hasFeedback - rules={[{ required: true, message: `${authLabel.username} is required` }]} - validateFirst - > + <FormItem label={authLabel.username}> <Input value={usernameOrAccessKey} onChange={(e) => setUsernameOrAccessKey(e.target.value)} placeholder={`Enter ${authLabel.username.toLowerCase()}`} /> </FormItem> ... - <FormItem - label={authLabel.password} - hasFeedback - rules={[{ required: true, message: `${authLabel.password} is required` }]} - validateFirst - > + <FormItem label={authLabel.password}> <Password value={passwordOrSecretKey} onChange={(e) => setPasswordOrSecretKey(e.target.value)} placeholder={`Enter ${authLabel.password.toLowerCase()}`} /> </FormItem>
375-380: AsyncButton’s loading prop is ignored; button won’t show loading for programmatic exploreAsyncButton overrides
loading, soloading={isExploring}has no effect (auto-explore via defaultUrl won’t show a spinner). Use antd Button here for external loading, or extend AsyncButton to accept an externalLoading prop.Switch to Button:
-import { AsyncButton } from "components/async_clickables"; +import { AsyncButton } from "components/async_clickables"; +import { Button } from "antd"; ... - <AsyncButton + <Button size="large" type="primary" style={{ width: "100%" }} onClick={handleExplore} loading={isExploring} disabled={!datasourceUrl || isExploring} > {defaultUrl == null ? "Add Layer" : "Validate URL and Continue"} - </AsyncButton> + </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx(7 hunks)frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx(6 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx(4 hunks)unreleased_changes/8954.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- unreleased_changes/8954.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T11:34:16.411Z
Learnt from: knollengewaechs
PR: scalableminds/webknossos#8796
File: frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx:68-77
Timestamp: 2025-08-27T11:34:16.411Z
Learning: The webknossos codebase uses `getDefaultLayerViewConfiguration` to ensure that layer configurations always exist in `datasetConfiguration.layers`, making direct access like `datasetConfiguration.layers[colorLayer.name]` safe across multiple files including api_latest.ts and load_histogram_data_saga.ts.
Applied to files:
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx
🧬 Code graph analysis (3)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (3)
frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx (1)
useDatasetSettingsContext(38-44)frontend/javascripts/admin/account/helpers/settings_title.tsx (1)
SettingsTitle(5-21)frontend/javascripts/dashboard/dataset/helper_components.tsx (1)
RetryingErrorBoundary(61-107)
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (3)
frontend/javascripts/types/schemas/datasource.types.ts (1)
DatasourceConfiguration(80-87)frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx (1)
DatasetSettingsFormData(9-15)frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx (1)
AddRemoteLayer(93-388)
frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx (6)
frontend/javascripts/types/api_types.ts (1)
APIDataStore(153-159)frontend/javascripts/types/schemas/datasource.types.ts (1)
DatasourceConfiguration(80-87)frontend/javascripts/types/globals.d.ts (1)
ArbitraryObject(32-32)frontend/javascripts/libs/read_file.ts (1)
readFileAsText(1-10)frontend/javascripts/admin/dataset/remote/google_auth_form_item.tsx (1)
GoogleAuthFormItem(10-60)frontend/javascripts/components/async_clickables.tsx (1)
AsyncButton(64-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
🔇 Additional comments (3)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (2)
35-41: Fix invalidForm.useWatchinvocation
Form.useWatchonly accepts(namePath, form?). Passing{ form, preserve: true }means the hook receives a plain object instead of theFormInstance, so the next call togetInternalHookswill throw at runtime. Please pass the form instance directly.- // TODO: The "preserve" option probably fixes this, e.g. useWatch("dataSource", { form, preserve: true }); - Form.useWatch("dataSource", { form, preserve: true }); + Form.useWatch("dataSource", form);
132-141: Remove the anchor-wrapped ButtonAn anchor containing a Button is invalid HTML and creates focus/a11y issues. Render a single interactive element that opens the JSON view (e.g., via
window.open) so the Tooltip wraps just the Button.- {activeUser?.isSuperUser && dataset ? ( - <Tooltip title="Inspect the full data source JSON response from the server. This is shown to super users only."> - <a - href={`/api/datasets/${dataset?.id}`} - target="_blank" - rel="noopener noreferrer" - > - <Button icon={<ExportOutlined />} /> - </a> - </Tooltip> - ) : null} + {activeUser?.isSuperUser && dataset ? ( + <Tooltip title="Inspect the full data source JSON response from the server. This is shown to super users only."> + <Button + aria-label="Open raw dataset JSON" + icon={<ExportOutlined />} + onClick={() => + window.open(`/api/datasets/${dataset.id}`, "_blank", "noopener,noreferrer") + } + /> + </Tooltip> + ) : null}frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (1)
114-118: Schema/API shape mismatch: convert DatasourceConfiguration → APIDataSource before storingYou merge a schema-derived DatasourceConfiguration into
form.dataSource(typed as APIDataSource) and pass it tostoreRemoteDataset. Field names differ (e.g., mags→resolutions, additionalCoordinates→additionalAxes, id.team vs API id shape), risking runtime failures.Action:
- Introduce a converter
toAPIDataSource(newConfig: DatasourceConfiguration, previous?: APIDataSource)and use it both when setting the form and before callingstoreRemoteDataset.- Ensure
idmatches the API (owningOrganization/equivalent), notteam.I can generate the adapter if you confirm the exact APIDataSource/DataLayer shapes in this branch.
#!/bin/bash # Inspect type shapes to implement a precise converter. rg -n -C2 'type\s+APIDataSource\b|interface\s+APIDataSource\b' frontend/javascripts -S rg -n -C2 'type\s+APIDataSourceId\b|owningOrganization|team\b' frontend/javascripts -S rg -n -C2 'type\s+DatasourceConfiguration\b|type\s+DataLayer\b' frontend/javascripts -SAlso applies to: 128-129, 172-176, 183-186
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (1)
423-433: Incorrect/wasted validator on Bounding box (wrong message, unreachable).
- required: true prevents empty values, so the warningOnly branch for “left empty” never triggers.
- The message mentions segment IDs, which is not related to boundingBox. Likely copy/paste.
Remove the warning-only rule.
rules={[ { required: true, message: "Please define a valid bounding box.", }, { validator: syncValidator( (value: BoundingBoxObject) => value.width !== 0 && value.height !== 0 && value.depth !== 0, "Width, height and depth must not be zero", ), }, - { - warningOnly: true, - validator: (_rule, value) => - value == null || value === "" - ? Promise.reject( - new Error( - "When left empty, annotating this layer later will only be possible with manually chosen segment IDs.", - ), - ) - : Promise.resolve(), - }, ]}frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (1)
160-188: Return type mismatch for getFormValidationSummary (compile-time error).Function is annotated to return Record<"data"|"general"|"defaultConfig", boolean> but returns {} variably. Use Partial<Record<...>>.
- const getFormValidationSummary = useCallback((): Record< - "data" | "general" | "defaultConfig", - boolean - > => { + const getFormValidationSummary = useCallback((): Partial< + Record<"data" | "general" | "defaultConfig", boolean> + > => { // Antd's form.getFieldsError only returns errors for mounted form fields, i.e. from the current tab :anger: const err = form.getFieldsError(); - const formErrors: Record<string, any> = {}; + const formErrors: Partial<Record<"data" | "general" | "defaultConfig", boolean>> = {};frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (1)
96-104: Bug: always navigates due to wrong null checkfilter() never returns null; this condition is always truthy and triggers navigation even when the name isn’t taken.
Apply this diff:
- const maybeDSNameError = form - .getFieldError(["dataset", "name"]) - .filter((error) => error === messages["dataset.name.already_taken"]); - if (maybeDSNameError == null) return; + const dsNameErrors = form.getFieldError(["dataset", "name"]); + const isAlreadyTaken = dsNameErrors.includes(messages["dataset.name.already_taken"]); + if (!isAlreadyTaken) return;
🧹 Nitpick comments (6)
frontend/javascripts/dashboard/dataset/helper_components.tsx (1)
10-12: Remove orphaned JSON edit style export.With the advanced JSON editor gone, this styling constant looks like a leftover. If nothing else imports it anymore, please drop it to keep the helper module lean.
-export const jsonEditStyle = { - fontFamily: 'Monaco, Consolas, "Courier New", monospace', -};frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (2)
124-133: Add accessible label to the icon-only Export button.Icon-only buttons need an aria-label for screen readers. Keep the current href/target/rel.
- <Button + <Button + aria-label="Open raw dataset JSON" href={`/api/datasets/${dataset?.id}`} target="_blank" rel="noopener noreferrer" icon={<ExportOutlined />} />
58-66: Strongly type dataSource prop (avoid any).Use APIDataSource to tighten types and catch schema regressions at compile time.
function SimpleDatasetForm({ dataSource, form, dataset, }: { - dataSource: Record<string, any>; + dataSource: APIDataSource; form: FormInstance; dataset: APIDataset | null | undefined; }) {Add missing import:
// at top-level imports import type { APIDataSource } from "types/api_types";frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (2)
290-302: Select earliest problematic tab: use shift() instead of pop().pop() picks the last key, not the earliest. Use shift() to get the first key in insertion order.
- // Switch to the earliest, problematic tab - switch (Object.keys(validationSummary).pop()) { + // Switch to the earliest, problematic tab + switch (Object.keys(validationSummary).shift()) {
251-258: Harden JSON parsing for defaultConfigurationLayersJson.A malformed JSON string will throw and abort submission without a clear message. Catch and surface a friendly error.
- await updateDatasetDefaultConfiguration( - datasetId, - _.extend({}, datasetDefaultConfiguration, formValues.defaultConfiguration, { - layers: JSON.parse(formValues.defaultConfigurationLayersJson), - }), - ); + let parsedLayers; + try { + parsedLayers = JSON.parse(formValues.defaultConfigurationLayersJson); + } catch { + Toast.error("Default configuration layers contain invalid JSON."); + return; // abort submission + } + await updateDatasetDefaultConfiguration( + datasetId, + _.extend({}, datasetDefaultConfiguration, formValues.defaultConfiguration, { + layers: parsedLayers, + }), + );frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (1)
8-9: Outdated comment“// Sync simple with advanced and get newest datasourceJson” is stale since the advanced JSON editor was removed. Update/remove to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx(8 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx(4 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx(9 hunks)frontend/javascripts/dashboard/dataset/helper_components.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-27T11:34:16.411Z
Learnt from: knollengewaechs
PR: scalableminds/webknossos#8796
File: frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx:68-77
Timestamp: 2025-08-27T11:34:16.411Z
Learning: The webknossos codebase uses `getDefaultLayerViewConfiguration` to ensure that layer configurations always exist in `datasetConfiguration.layers`, making direct access like `datasetConfiguration.layers[colorLayer.name]` safe across multiple files including api_latest.ts and load_histogram_data_saga.ts.
Applied to files:
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx
📚 Learning: 2025-09-26T08:13:33.277Z
Learnt from: philippotto
PR: scalableminds/webknossos#8954
File: frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx:37-41
Timestamp: 2025-09-26T08:13:33.277Z
Learning: Form.useWatch in Ant Design supports the options object format as the second parameter: Form.useWatch(namePath, { form, preserve: true }) where preserve: true is a valid option according to the official documentation.
Applied to files:
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx
🧬 Code graph analysis (3)
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (4)
frontend/javascripts/types/schemas/datasource.types.ts (1)
DatasourceConfiguration(80-87)frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx (1)
DatasetSettingsFormData(9-15)frontend/javascripts/admin/rest_api.ts (2)
isDatasetNameValid(1274-1285)storeRemoteDataset(1253-1271)frontend/javascripts/admin/dataset/remote/add_remote_layer.tsx (1)
AddRemoteLayer(93-388)
frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (3)
frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx (1)
DatasetSettingsFormData(9-15)frontend/javascripts/admin/rest_api.ts (1)
getDataset(987-998)frontend/javascripts/dashboard/dataset/helper_components.tsx (1)
hasFormError(72-78)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (1)
frontend/javascripts/dashboard/dataset/dataset_settings_context.tsx (1)
useDatasetSettingsContext(38-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
🔇 Additional comments (7)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (2)
35-36: useWatch usage is correct (with preserve).Passing the options object with preserve: true is supported and appropriate here.
Based on learnings
116-120: Width calculation accounts for both icon buttons.Looks correct for superusers (two buttons) vs. others (one button).
frontend/javascripts/dashboard/dataset/dataset_settings_provider.tsx (2)
172-173: Good: dataset validation summary includes datasetRotation.Including datasetRotation with dataSource in the “data” tab summary is aligned with the simplified UI.
77-77: Fix Form.useForm misuse (API does not accept an argument).AntD Form.useForm() takes no parameters. Create an internal form and fall back to formProp.
- const [form] = Form.useForm<DatasetSettingsFormData>(formProp); + const [internalForm] = Form.useForm<DatasetSettingsFormData>(); + const form = formProp ?? internalForm;frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (3)
25-51: Layer name collision handling LGTMThe new mergeNewLayers preserves existing names and ensures global uniqueness for new ones. This resolves the duplicate-suffix issue.
109-113: Shape mismatch: DatasourceConfiguration written where APIDataSource is expectedYou set a schema-derived DatasourceConfiguration into form.dataSource, but DatasetSettingsFormData expects APIDataSource and later this is sent to storeRemoteDataset. Convert to the API shape (e.g., mags→resolutions, additionalCoordinates→additionalAxes, boundingBox format) before using it as APIDataSource.
Run to confirm type differences and call sites:
#!/bin/bash # Inspect API vs schema types and references rg -n -C2 'type\s+APIDataSource\b|type\s+APIDataSourceId\b' frontend/javascripts/types rg -n -C2 'type\s+DatasourceConfiguration\b' frontend/javascripts/types rg -n -C3 '\bmags\b|\bresolutions\b|\badditional(Coordinates|Axes)\b|\BboundingBox\b' frontend/javascripts # Where storeRemoteDataset is used with dataSource rg -n -C2 'storeRemoteDataset\(' frontend/javascripts
167-181: Convert dataSource to API shape before POSTstoreRemoteDataset expects APIDataSource. Ensure conversion from DatasourceConfiguration before sending the payload to avoid backend rejection.
Apply this diff:
- const dataSource = form.getFieldValue("dataSource"); + const dataSource = form.getFieldValue("dataSource"); + // Convert schema-derived datasource to API shape before storing + const apiDataSource = convertToApiDataSource(dataSource); ... - const { newDatasetId } = await storeRemoteDataset( + const { newDatasetId } = await storeRemoteDataset( datastoreToUse.name, datasetName, - dataSource, + apiDataSource, targetFolderId, );Add a conversion helper (example location: top of this file or a shared util). Adjust field mapping to your exact types:
// Example stub; implement exact mappings per your types function convertToApiDataSource(ds: any) { if (!ds) return ds; return { ...ds, dataLayers: (ds.dataLayers ?? []).map((l: any) => ({ ...l, resolutions: l.mags ?? l.resolutions, additionalAxes: l.additionalCoordinates ?? l.additionalAxes, // TODO: convert boundingBox to API BoundingBoxObject if shapes differ // Remove schema-only fields if necessary })), }; }Verify exact fields to map:
#!/bin/bash rg -n -C2 'type\s+APIDataSource\b|type\s+DataLayer\b|type\s+DatasourceConfiguration\b' frontend/javascripts/types
| const defaultDatasetName = getDefaultDatasetName(url); | ||
| setDatasourceConfigStr( | ||
| JSON.stringify({ ...dataSourceJson, id: { name: defaultDatasetName, team: "" } }), | ||
| ); | ||
| form.setFieldValue(["dataSource", "id"], { name: defaultDatasetName, team: "" }); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Auto-import flow: dataset.name not set; id.team likely incomplete
Auto-import sets only dataSource.id.name, leaving dataset.name unset (validation may fail) and id.team as empty. Set dataset.name too; verify team/owningOrganization field expected by the API.
Apply this diff:
- form.setFieldValue(["dataSource", "id"], { name: defaultDatasetName, team: "" });
+ form.setFieldValue(["dataSource", "id"], {
+ name: defaultDatasetName,
+ team: activeUser?.organization ?? "",
+ });
+ form.setFieldValue(["dataset", "name"], defaultDatasetName);Please verify whether APIDataSourceId uses team or owningOrganization and adjust accordingly.
🏁 Script executed:
#!/bin/bash
rg -n -C2 'type\s+APIDataSourceId\b' frontend/javascripts/typesLength of output: 342
🏁 Script executed:
#!/bin/bash
rg -n -C5 'MutableAPIDataSourceId' frontend/javascripts/types/api_types.tsLength of output: 810
🏁 Script executed:
#!/bin/bash
rg -n -C3 'export (type|interface) APIDataset' frontend/javascripts/types/api_types.tsLength of output: 698
🏁 Script executed:
#!/bin/bash
file=$(fd dataset_add_remote_view.tsx)
echo "FILE: $file"
sed -n '1,200p' "$file"Length of output: 7415
🏁 Script executed:
#!/bin/bash
# Search for DatasourceConfiguration type definition
rg -n 'DatasourceConfiguration' -C3 -t tsLength of output: 7142
🏁 Script executed:
#!/bin/bash
rg -n 'storeRemoteDataset' -C10 frontend/javascripts/admin/rest_api.tsLength of output: 555
Set id.team to activeUser.organization and populate dataset.name
APIDataSourceId uses owningOrganization/directoryName, so map id.team to activeUser.organization and add dataset.name to avoid validation failures:
- form.setFieldValue(["dataSource", "id"], { name: defaultDatasetName, team: "" });
+ form.setFieldValue(["dataSource", "id"], {
+ name: defaultDatasetName,
+ team: activeUser?.organization ?? "",
+ });
+ form.setFieldValue(["dataset", "name"], defaultDatasetName);📝 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.
| const defaultDatasetName = getDefaultDatasetName(url); | |
| setDatasourceConfigStr( | |
| JSON.stringify({ ...dataSourceJson, id: { name: defaultDatasetName, team: "" } }), | |
| ); | |
| form.setFieldValue(["dataSource", "id"], { name: defaultDatasetName, team: "" }); | |
| const defaultDatasetName = getDefaultDatasetName(url); | |
| form.setFieldValue(["dataSource", "id"], { | |
| name: defaultDatasetName, | |
| team: activeUser?.organization ?? "", | |
| }); | |
| form.setFieldValue(["dataset", "name"], defaultDatasetName); |
🤖 Prompt for AI Agents
In frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx around lines
122-124, the dataSource id.team is being set to an empty string and dataset.name
is not populated which breaks APIDataSourceId validation; change the id.team
assignment to use activeUser.organization (map id.team to
activeUser.organization) and also populate dataset.name with the
defaultDatasetName (set dataset.name to defaultDatasetName) so both the
owningOrganization/directoryName mapping and dataset validation succeed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/viewer/model_initialization.ts (1)
211-216: Optional: cache the sharing token once and thread it through.Minor readability/perf: compute the sharing token once in initialize and pass it to both fetchParallel and getDatasetViewConfiguration to avoid repeated URL param parsing.
Example (optional) change:
@@ Store.dispatch(setControlModeAction(initialCommandType.type)); + const sharingToken = getSharingTokenFromUrlParameters(); @@ - const [apiDataset, initialUserSettings, serverTracings] = await fetchParallel( - annotation, - datasetId, - version, - ); + const [apiDataset, initialUserSettings, serverTracings] = await fetchParallel( + annotation, + datasetId, + version, + sharingToken, + ); @@ - const initialDatasetSettings = await getDatasetViewConfiguration( + const initialDatasetSettings = await getDatasetViewConfiguration( dataset, serverVolumeTracingIds, - getSharingTokenFromUrlParameters(), + sharingToken, ); @@ -async function fetchParallel( +async function fetchParallel( annotation: APIAnnotation | null | undefined, datasetId: string, version: number | undefined | null, -): Promise<[APIDataset, UserConfiguration, Array<ServerTracing>]> { + sharingToken: string | null | undefined, +): Promise<[APIDataset, UserConfiguration, Array<ServerTracing>]> { return Promise.all([ - getDataset(datasetId, getSharingTokenFromUrlParameters()), + getDataset(datasetId, sharingToken), getUserConfiguration(), // Fetch the actual tracing from the datastore, if there is an skeletonAnnotation annotation ? getTracingsForAnnotation(annotation, version) : [], ]); }Also applies to: 224-227, 308-318
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/viewer/model_initialization.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/javascripts/viewer/model_initialization.ts (2)
frontend/javascripts/admin/rest_api.ts (1)
getDataset(987-998)frontend/javascripts/admin/api/token.ts (1)
getSharingTokenFromUrlParameters(27-37)
⏰ 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: backend-tests
- GitHub Check: frontend-tests
🔇 Additional comments (1)
frontend/javascripts/viewer/model_initialization.ts (1)
314-314: Approve signature change. Two-arg invocation matches the updated API; existing three-arg calls intentionally leverage the options parameter and require no further edits.
This PR removes the advanced dataset settings mode (JSON input). PR New reserveUploadToPaths Protocol; Refactor DataLayer Classes; UPath #8844 removed the ability to edit JSON directly anyway - this is the frontend follow-up / clean-up. This affects both the views for dataset settings, as well as, remote dataset upload/import.
PR New reserveUploadToPaths Protocol; Refactor DataLayer Classes; UPath #8844 added a
includePathsGET parameter to/api/datasets. The matching backend changes were never merged tomasterand discarded.Small refinements:
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)