-
Notifications
You must be signed in to change notification settings - Fork 29
Remove advanced dataset settings toggle #8954
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
c91743c
e0db028
c31c0a7
f0b01b6
850b62c
8845d32
412545f
936e08f
cdbeecf
c36cb93
36d75b6
2cb49e6
ead5207
4067eef
936b0dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,10 +2,10 @@ import { CardContainer, DatastoreFormItem } from "admin/dataset/dataset_componen | |||||||||||||||||||||||
| import { isDatasetNameValid, storeRemoteDataset } from "admin/rest_api"; | ||||||||||||||||||||||||
| import { Button, Col, Divider, Form, type FormInstance, List, Modal, Row } from "antd"; | ||||||||||||||||||||||||
| import BrainSpinner from "components/brain_spinner"; | ||||||||||||||||||||||||
| import type { DatasetSettingsFormData } from "dashboard/dataset/dataset_settings_context"; | ||||||||||||||||||||||||
| import DatasetSettingsDataTab from "dashboard/dataset/dataset_settings_data_tab"; | ||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||
| DatasetSettingsProvider, // Sync simple with advanced and get newest datasourceJson | ||||||||||||||||||||||||
| syncDataSourceFields, | ||||||||||||||||||||||||
| } from "dashboard/dataset/dataset_settings_provider"; | ||||||||||||||||||||||||
| import { FormItemWithInfo, Hideable } from "dashboard/dataset/helper_components"; | ||||||||||||||||||||||||
| import FolderSelection from "dashboard/folders/folder_selection"; | ||||||||||||||||||||||||
|
|
@@ -16,11 +16,45 @@ import messages from "messages"; | |||||||||||||||||||||||
| import { useEffect, useState } from "react"; | ||||||||||||||||||||||||
| import { useNavigate } from "react-router-dom"; | ||||||||||||||||||||||||
| import type { APIDataStore } from "types/api_types"; | ||||||||||||||||||||||||
| import type { DataLayer, DatasourceConfiguration } from "types/schemas/datasource.types"; | ||||||||||||||||||||||||
| import { dataPrivacyInfo } from "./dataset_upload_view"; | ||||||||||||||||||||||||
| import { AddRemoteLayer } from "./remote/add_remote_layer"; | ||||||||||||||||||||||||
| import { groupBy } from "lodash"; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| const FormItem = Form.Item; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| const mergeNewLayers = ( | ||||||||||||||||||||||||
| existingDatasource: DatasourceConfiguration | null, | ||||||||||||||||||||||||
| newDatasource: DatasourceConfiguration, | ||||||||||||||||||||||||
| ): DatasourceConfiguration => { | ||||||||||||||||||||||||
| if (existingDatasource?.dataLayers == null) { | ||||||||||||||||||||||||
| return newDatasource; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| const allLayers = newDatasource.dataLayers.concat(existingDatasource.dataLayers); | ||||||||||||||||||||||||
| const groupedLayers: Record<string, DataLayer[]> = groupBy( | ||||||||||||||||||||||||
| allLayers, | ||||||||||||||||||||||||
| (layer: DataLayer) => layer.name, | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| const uniqueLayers: DataLayer[] = []; | ||||||||||||||||||||||||
| for (const [name, layerGroup] of Utils.entries(groupedLayers)) { | ||||||||||||||||||||||||
| if (layerGroup.length === 1) { | ||||||||||||||||||||||||
| uniqueLayers.push(layerGroup[0]); | ||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||
| layerGroup.forEach((layer, idx) => { | ||||||||||||||||||||||||
| const layerName = idx === 0 ? name : `${name}_${idx + 1}`; | ||||||||||||||||||||||||
| uniqueLayers.push({ ...layer, name: layerName }); | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||
| ...existingDatasource, | ||||||||||||||||||||||||
| dataLayers: uniqueLayers, | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| type Props = { | ||||||||||||||||||||||||
| onAdded: ( | ||||||||||||||||||||||||
| uploadedDatasetId: string, | ||||||||||||||||||||||||
|
|
@@ -44,10 +78,10 @@ function DatasetAddRemoteView(props: Props) { | |||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| const [showAddLayerModal, setShowAddLayerModal] = useState(false); | ||||||||||||||||||||||||
| const [showLoadingOverlay, setShowLoadingOverlay] = useState(defaultDatasetUrl != null); | ||||||||||||||||||||||||
| const [form] = Form.useForm(); | ||||||||||||||||||||||||
| const [form] = Form.useForm<DatasetSettingsFormData & { datastoreUrl?: string }>(); | ||||||||||||||||||||||||
| const [targetFolderId, setTargetFolderId] = useState<string | null>(null); | ||||||||||||||||||||||||
| const isDatasourceConfigStrFalsy = Form.useWatch("dataSourceJson", form) == null; | ||||||||||||||||||||||||
| const maybeDataLayers = Form.useWatch(["dataSource", "dataLayers"], form); | ||||||||||||||||||||||||
| const datasourceConfig = Form.useWatch(["dataSource"], form); | ||||||||||||||||||||||||
| const navigate = useNavigate(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||||||
|
|
@@ -66,7 +100,7 @@ function DatasetAddRemoteView(props: Props) { | |||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| const maybeOpenExistingDataset = () => { | ||||||||||||||||||||||||
| const maybeDSNameError = form | ||||||||||||||||||||||||
| .getFieldError("datasetName") | ||||||||||||||||||||||||
| .getFieldError(["dataset", "name"]) | ||||||||||||||||||||||||
| .filter((error) => error === messages["dataset.name.already_taken"]); | ||||||||||||||||||||||||
| if (maybeDSNameError == null) return; | ||||||||||||||||||||||||
| navigate( | ||||||||||||||||||||||||
|
|
@@ -77,18 +111,22 @@ function DatasetAddRemoteView(props: Props) { | |||||||||||||||||||||||
| const hasFormAnyErrors = (form: FormInstance) => | ||||||||||||||||||||||||
| form.getFieldsError().filter(({ errors }) => errors.length).length > 0; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| const onSuccesfulExplore = async (url: string) => { | ||||||||||||||||||||||||
| const dataSourceJsonString = form.getFieldValue("dataSourceJson"); | ||||||||||||||||||||||||
| const onSuccessfulExplore = async (url: string, newDataSourceConfig: DatasourceConfiguration) => { | ||||||||||||||||||||||||
| const datasourceConfig = form.getFieldValue("dataSource"); | ||||||||||||||||||||||||
| const mergedConfig = mergeNewLayers(datasourceConfig, newDataSourceConfig); | ||||||||||||||||||||||||
| form.setFieldValue("dataSource", mergedConfig); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
Comment on lines
+109
to
+113
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainType shape mismatch risk: DatasourceConfiguration vs APIDataSource
🏁 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 🤖 Prompt for AI Agents |
||||||||||||||||||||||||
| if (defaultDatasetUrl == null) { | ||||||||||||||||||||||||
| setShowLoadingOverlay(false); | ||||||||||||||||||||||||
| setShowAddLayerModal(false); | ||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (!showLoadingOverlay) setShowLoadingOverlay(true); // show overlay again, e.g. after credentials were passed | ||||||||||||||||||||||||
| const dataSourceJson = JSON.parse(dataSourceJsonString); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| const defaultDatasetName = getDefaultDatasetName(url); | ||||||||||||||||||||||||
| setDatasourceConfigStr( | ||||||||||||||||||||||||
| JSON.stringify({ ...dataSourceJson, id: { name: defaultDatasetName, team: "" } }), | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
| form.setFieldValue(["dataSource", "id"], { name: defaultDatasetName, team: "" }); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
Comment on lines
88
to
+124
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainAuto-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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||
| await form.validateFields(); | ||||||||||||||||||||||||
| } catch (_e) { | ||||||||||||||||||||||||
|
|
@@ -98,19 +136,13 @@ function DatasetAddRemoteView(props: Props) { | |||||||||||||||||||||||
| return; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (!hasFormAnyErrors(form)) { | ||||||||||||||||||||||||
| handleStoreDataset(); | ||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||
| setShowLoadingOverlay(false); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| const setDatasourceConfigStr = (dataSourceJson: string) => { | ||||||||||||||||||||||||
| form.setFieldsValue({ dataSourceJson }); | ||||||||||||||||||||||||
| // Since this function sets the JSON string, we have to update the | ||||||||||||||||||||||||
| // data which is rendered by the "simple" page. | ||||||||||||||||||||||||
| syncDataSourceFields(form, "simple", true); | ||||||||||||||||||||||||
| form.validateFields(); | ||||||||||||||||||||||||
| setShowAddLayerModal(false); | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| async function handleStoreDataset() { | ||||||||||||||||||||||||
|
|
@@ -139,18 +171,18 @@ function DatasetAddRemoteView(props: Props) { | |||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // The dataset name is not synced with the datasource.id.name in the advanced settings: See DatasetSettingsDataTab. | ||||||||||||||||||||||||
| const datasetName = form.getFieldValue(["dataset", "name"]); | ||||||||||||||||||||||||
| const dataSourceJsonStr = form.getFieldValue("dataSourceJson"); | ||||||||||||||||||||||||
| if (dataSourceJsonStr && activeUser) { | ||||||||||||||||||||||||
| const dataSource = form.getFieldValue("dataSource"); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (dataSource && activeUser) { | ||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||
| const nameValidationResult = await isDatasetNameValid(datasetName); | ||||||||||||||||||||||||
| if (nameValidationResult) { | ||||||||||||||||||||||||
| throw new Error(nameValidationResult); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| const dataSourceJson = JSON.parse(dataSourceJsonStr); | ||||||||||||||||||||||||
| const { newDatasetId } = await storeRemoteDataset( | ||||||||||||||||||||||||
| datastoreToUse.name, | ||||||||||||||||||||||||
| datasetName, | ||||||||||||||||||||||||
| dataSourceJson, | ||||||||||||||||||||||||
| dataSource, | ||||||||||||||||||||||||
| targetFolderId, | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
| onAdded(newDatasetId, datasetName); | ||||||||||||||||||||||||
|
|
@@ -182,20 +214,20 @@ function DatasetAddRemoteView(props: Props) { | |||||||||||||||||||||||
| > | ||||||||||||||||||||||||
| <AddRemoteLayer | ||||||||||||||||||||||||
| form={form} | ||||||||||||||||||||||||
| preferredVoxelSize={datasourceConfig?.scale} | ||||||||||||||||||||||||
| uploadableDatastores={uploadableDatastores} | ||||||||||||||||||||||||
| setDatasourceConfigStr={setDatasourceConfigStr} | ||||||||||||||||||||||||
| onSuccess={() => setShowAddLayerModal(false)} | ||||||||||||||||||||||||
| onSuccess={onSuccessfulExplore} | ||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||
| </Modal> | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| {hideDatasetUI && ( | ||||||||||||||||||||||||
| <AddRemoteLayer | ||||||||||||||||||||||||
| form={form} | ||||||||||||||||||||||||
| preferredVoxelSize={datasourceConfig?.scale} | ||||||||||||||||||||||||
| uploadableDatastores={uploadableDatastores} | ||||||||||||||||||||||||
| setDatasourceConfigStr={setDatasourceConfigStr} | ||||||||||||||||||||||||
| defaultUrl={defaultDatasetUrl} | ||||||||||||||||||||||||
| onError={() => setShowLoadingOverlay(false)} | ||||||||||||||||||||||||
| onSuccess={(defaultDatasetUrl: string) => onSuccesfulExplore(defaultDatasetUrl)} | ||||||||||||||||||||||||
| onSuccess={onSuccessfulExplore} | ||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||
| )} | ||||||||||||||||||||||||
| <Hideable hidden={hideDatasetUI}> | ||||||||||||||||||||||||
|
|
@@ -249,7 +281,6 @@ function DatasetAddRemoteView(props: Props) { | |||||||||||||||||||||||
| type="default" | ||||||||||||||||||||||||
| style={{ width: "100%" }} | ||||||||||||||||||||||||
| onClick={() => { | ||||||||||||||||||||||||
| setDatasourceConfigStr(""); | ||||||||||||||||||||||||
| form.resetFields(); | ||||||||||||||||||||||||
| }} | ||||||||||||||||||||||||
| > | ||||||||||||||||||||||||
|
|
@@ -266,7 +297,6 @@ function DatasetAddRemoteView(props: Props) { | |||||||||||||||||||||||
| style={{ width: "100%" }} | ||||||||||||||||||||||||
| onClick={handleStoreDataset} | ||||||||||||||||||||||||
| disabled={ | ||||||||||||||||||||||||
| isDatasourceConfigStrFalsy || | ||||||||||||||||||||||||
| !!form.getFieldsError().filter(({ errors }) => errors.length).length | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| > | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.