This is an automated email from the ASF dual-hosted git repository.
rebenitez1802 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new c016883970d fix(dataset): preserve pending metric and column edits
(#45506)
c016883970d is described below
commit c016883970daa20b9eab8084c7ff2e29549200a5
Author: Rafael Benitez <[email protected]>
AuthorDate: Fri Oct 23 12:01:04 2026 -0300
fix(dataset): preserve pending metric and column edits (#45506)
---
.../DatasourceModal.pendingEdits.test.tsx | 196 ++++++++++++++
.../CollectionTable.debounce.test.tsx | 287 +++++++++++++++++++++
.../components/CollectionTable/index.tsx | 231 ++++++-----------
.../src/components/Datasource/types.ts | 10 -
4 files changed, 562 insertions(+), 162 deletions(-)
diff --git
a/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.pendingEdits.test.tsx
b/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.pendingEdits.test.tsx
new file mode 100644
index 00000000000..076e0e99b39
--- /dev/null
+++
b/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.pendingEdits.test.tsx
@@ -0,0 +1,196 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import type { ComponentProps } from 'react';
+import fetchMock from 'fetch-mock';
+import {
+ act,
+ cleanup,
+ fireEvent,
+ render,
+ screen,
+ userEvent,
+ waitFor,
+ within,
+} from 'spec/helpers/testing-library';
+import mockDatasource from 'spec/fixtures/mockDatasource';
+import { Constants } from '@superset-ui/core/components';
+import {
+ cleanupAsyncOperations,
+ setupDatasourceEditorMocks,
+} from
'src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditor.test.utils';
+import DatasourceModal from 'src/components/Datasource/DatasourceModal';
+
+const datasetEndpoint = 'glob:*/api/v1/dataset/7';
+const saveEndpoint = 'glob:*/api/v1/dataset/7?override_columns=*';
+
+type SavedItem = {
+ metric_name?: string;
+ column_name?: string;
+ expression?: string;
+};
+
+type SavePayload = { metrics: SavedItem[]; columns: SavedItem[] };
+
+type AceNode = HTMLElement & {
+ env: {
+ editor: { getValue: () => string; setValue: (value: string) => void };
+ };
+};
+
+/** Flushes the real controls' debounces while leaving their callbacks intact.
*/
+async function settleInputs() {
+ await act(async () => {
+ await new Promise(resolve =>
+ setTimeout(resolve, Math.max(Constants.FAST_DEBOUNCE, 300) + 25),
+ );
+ });
+}
+
+beforeEach(() => {
+ jest.useRealTimers();
+ fetchMock.clearHistory().removeRoutes();
+ setupDatasourceEditorMocks();
+});
+
+afterEach(async () => {
+ cleanup();
+ await cleanupAsyncOperations();
+ fetchMock.clearHistory().removeRoutes();
+});
+
+test.each(['metric', 'calculated column'] as const)(
+ 'preserves a new %s and the save payload when an earlier row edit is
pending',
+ async itemKind => {
+ const isMetric = itemKind === 'metric';
+ const placeholder = isMetric ? '<new metric>' : '<new column>';
+ const itemName = isMetric ? 'new_metric' : 'new_calculated_column';
+ const expression = isMetric ? 'SUM(num)' : 'num + 2';
+ const existingItemName = isMetric
+ ? mockDatasource['7__table'].metrics[0].metric_name
+ : 'existing_calculated_column';
+ const existingExpression = isMetric ? 'SUM(num) + 1' : 'num + 3';
+ const tabName = isMetric ? /metrics/i : /calculated columns/i;
+ fetchMock.put(saveEndpoint, {});
+ fetchMock.get(datasetEndpoint, { result: mockDatasource['7__table'] });
+ const props = {
+ datasource: {
+ ...mockDatasource['7__table'],
+ metrics: mockDatasource['7__table'].metrics.slice(0, 1),
+ columns: [
+ mockDatasource['7__table'].columns[0],
+ {
+ ...mockDatasource['7__table'].columns[1],
+ column_name: 'existing_calculated_column',
+ expression: 'num + 1',
+ },
+ ],
+ },
+ etag: '"dataset-v1"',
+ show: true,
+ onHide: jest.fn(),
+ onDatasourceSave: jest.fn(),
+ onChange: jest.fn(),
+ addSuccessToast: jest.fn(),
+ addDangerToast: jest.fn(),
+ } as unknown as ComponentProps<typeof DatasourceModal>;
+ render(<DatasourceModal {...props} />, {
+ useRedux: true,
+ initialState: { common: { currencies: ['USD'] } },
+ useRouter: true,
+ });
+ await userEvent.click(await screen.findByRole('tab', { name: tabName }));
+ const panel = screen.getByRole('tabpanel', { name: tabName });
+ await userEvent.click(
+ within(panel).getByRole('button', { name: /expand row/i }),
+ );
+ const existingExpandedRow = within(panel).getByRole('row', {
+ name: /^SQL expression/i,
+ });
+ await waitFor(() =>
+ expect(
+ existingExpandedRow.querySelector('.ace_editor'),
+ ).toBeInTheDocument(),
+ );
+ const existingEditor =
+ existingExpandedRow.querySelector<AceNode>('.ace_editor');
+ fireEvent.change(await within(panel).findByPlaceholderText('Description'),
{
+ target: { value: 'Updated existing item' },
+ });
+ act(() => existingEditor?.env.editor.setValue(existingExpression));
+ fireEvent.click(within(panel).getByRole('button', { name: /add item/i }));
+ expect(within(panel).getByDisplayValue(placeholder)).toBeVisible();
+ await settleInputs();
+ expect(within(panel).getByDisplayValue(placeholder)).toBeVisible();
+
+ await userEvent.click(within(panel).getByDisplayValue(placeholder));
+ // EditableTitle changes its wrapper when entering edit mode.
+ const nameInput = within(panel).getByDisplayValue(placeholder);
+ fireEvent.change(nameInput, { target: { value: itemName } });
+ fireEvent.blur(nameInput);
+ await settleInputs();
+ await userEvent.click(
+ within(
+ within(panel).getByRole('row', { name: new RegExp(existingItemName) }),
+ ).getByRole('button', { name: /collapse row/i }),
+ );
+ if (isMetric) {
+ await userEvent.click(
+ within(
+ within(panel).getByRole('row', { name: new RegExp(itemName) }),
+ ).getByRole('button', { name: /expand row/i }),
+ );
+ }
+ const expandedRow = within(panel).getByRole('row', {
+ name: /^SQL expression/i,
+ });
+ await waitFor(() =>
+ expect(expandedRow.querySelector('.ace_editor')).toBeInTheDocument(),
+ );
+ const editor = expandedRow.querySelector<AceNode>('.ace_editor');
+ act(() => editor?.env.editor.setValue(expression));
+ await settleInputs();
+
+ await userEvent.click(screen.getByTestId('datasource-modal-save'));
+ await userEvent.click(
+ await screen.findByRole('button', { name: 'Confirm' }),
+ );
+ await waitFor(() =>
+ expect(fetchMock.callHistory.calls(saveEndpoint)).toHaveLength(1),
+ );
+ const [request] = fetchMock.callHistory.calls(saveEndpoint);
+ const payload = JSON.parse(String(request.options.body)) as SavePayload;
+ const items = isMetric
+ ? payload.metrics
+ : payload.columns.filter(column => column.expression);
+ expect(items).toHaveLength(2);
+ expect(items).toEqual(
+ expect.arrayContaining([
+ expect.objectContaining({
+ [isMetric ? 'metric_name' : 'column_name']: itemName,
+ expression,
+ }),
+ expect.objectContaining({
+ [isMetric ? 'metric_name' : 'column_name']: existingItemName,
+ description: 'Updated existing item',
+ expression: existingExpression,
+ }),
+ ]),
+ );
+ },
+);
diff --git
a/superset-frontend/src/components/Datasource/components/CollectionTable/CollectionTable.debounce.test.tsx
b/superset-frontend/src/components/Datasource/components/CollectionTable/CollectionTable.debounce.test.tsx
new file mode 100644
index 00000000000..cf7c3e10f19
--- /dev/null
+++
b/superset-frontend/src/components/Datasource/components/CollectionTable/CollectionTable.debounce.test.tsx
@@ -0,0 +1,287 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import {
+ act,
+ fireEvent,
+ render,
+ screen,
+ within,
+ waitFor,
+} from 'spec/helpers/testing-library';
+import { Constants } from '@superset-ui/core/components';
+import TextControl from 'src/explore/components/controls/TextControl';
+import TextAreaControl from 'src/explore/components/controls/TextAreaControl';
+import Field from '../Field';
+import Fieldset from '../Fieldset';
+import CollectionTable from '.';
+
+const initialRows = [
+ { id: 1, column_name: 'first', description: '', verbose_name: '' },
+ { id: 2, column_name: 'second', description: '', verbose_name: '' },
+];
+
+const tableProps = {
+ collection: initialRows,
+ tableColumns: ['column_name', 'description', 'verbose_name'],
+ sortColumns: ['column_name'],
+ allowAddItem: true,
+ allowDeletes: true,
+ itemGenerator: () => ({
+ id: 3,
+ column_name: 'added',
+ description: '',
+ verbose_name: '',
+ }),
+ expandFieldset: (
+ <Fieldset>
+ <Field
+ fieldKey="description"
+ label="Description"
+ control={<TextControl placeholder="Description" />}
+ />
+ <Field
+ fieldKey="verbose_name"
+ label="Label"
+ control={<TextControl placeholder="Label" />}
+ />
+ </Fieldset>
+ ),
+};
+
+/** Commits pending text edits together, as controls can do within one render.
*/
+function flushEdits() {
+ act(() => jest.advanceTimersByTime(Constants.FAST_DEBOUNCE));
+}
+
+beforeEach(() => jest.useFakeTimers());
+afterEach(() => jest.useRealTimers());
+
+test('a pending expanded-row edit preserves an added row and its order', () =>
{
+ const onChange = jest.fn();
+ render(<CollectionTable {...tableProps} onChange={onChange} />);
+ fireEvent.click(screen.getAllByRole('button', { name: /expand row/i })[0]);
+ fireEvent.change(screen.getByPlaceholderText('Description'), {
+ target: { value: 'edited' },
+ });
+ fireEvent.click(screen.getByRole('button', { name: /add item/i }));
+ flushEdits();
+
+ expect(screen.getByText('added')).toBeVisible();
+ expect(onChange).toHaveBeenLastCalledWith([
+ expect.objectContaining({ id: 3 }),
+ expect.objectContaining({ id: 1, description: 'edited' }),
+ expect.objectContaining({ id: 2 }),
+ ]);
+});
+
+test('a pending expanded-row edit does not restore another deleted row', () =>
{
+ const onChange = jest.fn();
+ render(<CollectionTable {...tableProps} onChange={onChange} />);
+ fireEvent.click(screen.getAllByRole('button', { name: /expand row/i })[0]);
+ fireEvent.change(screen.getByPlaceholderText('Description'), {
+ target: { value: 'edited' },
+ });
+ const deletedRow = screen.getByRole('row', { name: /second/i });
+ fireEvent.click(
+ within(deletedRow).getByRole('button', {
+ name: /delete item/i,
+ }),
+ );
+ flushEdits();
+
+ expect(screen.queryByText('second')).not.toBeInTheDocument();
+ expect(onChange).toHaveBeenLastCalledWith([
+ expect.objectContaining({ id: 1, description: 'edited' }),
+ ]);
+});
+
+test('pending sibling fields and rows all survive one debounce batch', () => {
+ const onChange = jest.fn();
+ render(<CollectionTable {...tableProps} onChange={onChange} />);
+ screen
+ .getAllByRole('button', { name: /expand row/i })
+ .forEach(button => fireEvent.click(button));
+ fireEvent.change(screen.getAllByPlaceholderText('Description')[0], {
+ target: { value: 'first description' },
+ });
+ fireEvent.change(screen.getAllByPlaceholderText('Label')[0], {
+ target: { value: 'first label' },
+ });
+ fireEvent.change(screen.getAllByPlaceholderText('Description')[1], {
+ target: { value: 'second description' },
+ });
+ flushEdits();
+
+ expect(onChange).toHaveBeenLastCalledWith([
+ expect.objectContaining({
+ id: 1,
+ description: 'first description',
+ verbose_name: 'first label',
+ }),
+ expect.objectContaining({ id: 2, description: 'second description' }),
+ ]);
+});
+
+test('a pending edit preserves externally refreshed values, membership and
order', () => {
+ const onChange = jest.fn();
+ const { rerender } = render(
+ <CollectionTable {...tableProps} onChange={onChange} />,
+ );
+ fireEvent.click(screen.getAllByRole('button', { name: /expand row/i })[0]);
+ fireEvent.change(screen.getByPlaceholderText('Description'), {
+ target: { value: 'edited' },
+ });
+ const refreshed = [
+ { id: 4, column_name: 'external', description: '', verbose_name: '' },
+ { ...initialRows[0], verbose_name: 'refreshed label' },
+ ];
+ onChange.mockClear();
+ rerender(
+ <CollectionTable
+ {...tableProps}
+ collection={refreshed}
+ onChange={onChange}
+ />,
+ );
+ expect(onChange).not.toHaveBeenCalled();
+ flushEdits();
+
+ expect(screen.getByText('external')).toBeVisible();
+ expect(screen.queryByText('second')).not.toBeInTheDocument();
+ expect(onChange).toHaveBeenLastCalledWith([
+ expect.objectContaining({ id: 4 }),
+ expect.objectContaining({
+ id: 1,
+ description: 'edited',
+ verbose_name: 'refreshed label',
+ }),
+ ]);
+});
+
+test('an asynchronous field callback cannot resurrect the row it edited', ()
=> {
+ let pendingChange: (() => void) | undefined;
+ /** Models a control whose asynchronous edit can finish after it unmounts. */
+ function AsyncControl({ onChange }: { onChange?: (value: string) => void }) {
+ return (
+ <button
+ type="button"
+ onClick={() => {
+ pendingChange = () => onChange?.('late edit');
+ }}
+ >
+ Queue edit
+ </button>
+ );
+ }
+ const onChange = jest.fn();
+ render(
+ <CollectionTable
+ {...tableProps}
+ onChange={onChange}
+ expandFieldset={
+ <Fieldset>
+ <Field
+ fieldKey="description"
+ label="Description"
+ control={<AsyncControl />}
+ />
+ </Fieldset>
+ }
+ />,
+ );
+ fireEvent.click(screen.getAllByRole('button', { name: /expand row/i })[0]);
+ fireEvent.click(screen.getByRole('button', { name: 'Queue edit' }));
+ const deletedRow = screen.getByRole('row', { name: /first/i });
+ fireEvent.click(
+ within(deletedRow).getByRole('button', {
+ name: /delete item/i,
+ }),
+ );
+ onChange.mockClear();
+ act(() => pendingChange?.());
+
+ expect(screen.queryByText('first')).not.toBeInTheDocument();
+ expect(onChange).not.toHaveBeenCalled();
+});
+
+test.each([
+ 'expand another row',
+ 'add a row',
+ 'receive a fresh collection array',
+] as const)(
+ 'a pending SQL expression survives when you %s before its debounce fires',
+ async action => {
+ const onChange = jest.fn();
+ const sqlProps = {
+ ...tableProps,
+ onChange,
+ expandFieldset: (
+ <Fieldset>
+ <Field
+ fieldKey="expression"
+ label="SQL expression"
+ control={
+ <TextAreaControl
+ language="sql"
+ offerEditInModal={false}
+ debounceDelay={300}
+ />
+ }
+ />
+ </Fieldset>
+ ),
+ };
+ const { container, rerender } = render(<CollectionTable {...sqlProps} />);
+ fireEvent.click(
+ within(screen.getByRole('row', { name: /first/i })).getByRole('button', {
+ name: /expand row/i,
+ }),
+ );
+ await waitFor(() =>
+ expect(container.querySelector('.ace_editor')).toBeInTheDocument(),
+ );
+ // Ace exposes its live editor on the container; setValue fires the same
+ // change event as typing, without relying on jsdom's text layout.
+ const editor = container.querySelector<
+ HTMLElement & { env: { editor: { setValue: (value: string) => void } } }
+ >('.ace_editor');
+ act(() => editor?.env.editor.setValue('num + 1'));
+ expect(onChange).not.toHaveBeenCalled();
+
+ if (action === 'expand another row') {
+ fireEvent.click(
+ within(screen.getByRole('row', { name: /second/i })).getByRole(
+ 'button',
+ { name: /expand row/i },
+ ),
+ );
+ } else if (action === 'add a row') {
+ fireEvent.click(screen.getByRole('button', { name: /add item/i }));
+ } else {
+ rerender(<CollectionTable {...sqlProps} collection={[...initialRows]}
/>);
+ }
+ act(() => jest.advanceTimersByTime(300));
+
+ expect(onChange).toHaveBeenLastCalledWith([
+ ...(action === 'add a row' ? [expect.objectContaining({ id: 3 })] : []),
+ expect.objectContaining({ id: 1, expression: 'num + 1' }),
+ expect.objectContaining({ id: 2 }),
+ ]);
+ },
+);
diff --git
a/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx
b/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx
index 2b0eec42686..362d1fd2278 100644
---
a/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx
+++
b/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx
@@ -36,7 +36,7 @@ import Table, {
type TablePaginationConfig,
TableSize,
} from '@superset-ui/core/components/Table';
-import Fieldset from '../Fieldset';
+import Fieldset, { type FieldsetProps } from '../Fieldset';
import { recurseReactClone } from '../../utils';
import {
type CRUDCollectionProps,
@@ -70,15 +70,7 @@ function createKeyedCollection(arr: Array<object>) {
}) as CollectionItem,
);
- const collection: Record<PropertyKey, CollectionItem> = {};
- collectionArray.forEach((o: CollectionItem) => {
- collection[o.id] = o;
- });
-
- return {
- collection,
- collectionArray,
- };
+ return collectionArray;
}
export default function CRUDCollection({
@@ -103,18 +95,14 @@ export default function CRUDCollection({
const [expandedColumns, setExpandedColumns] = useState<
Record<PropertyKey, boolean>
>({});
- // Seed both pieces of state from a single createKeyedCollection() pass so
- // that items lacking an `id` get one consistent set of synthetic ids
- // (matching the prior class component, which keyed the collection once).
+ // Seed ids for the first render; the props-sync effect re-keys on each
+ // collection prop change.
const initialKeyed = useRef<ReturnType<typeof createKeyedCollection>>();
if (!initialKeyed.current) {
initialKeyed.current = createKeyedCollection(propsCollection);
}
- const [collection, setCollection] = useState<
- Record<PropertyKey, CollectionItem>
- >(() => initialKeyed.current!.collection);
const [collectionArray, setCollectionArray] = useState<CollectionItem[]>(
- () => initialKeyed.current!.collectionArray,
+ () => initialKeyed.current!,
);
const [sortColumn, setSortColumn] = useState<string>('');
const [sort, setSort] = useState<SortOrderEnum>(SortOrderEnum.Unsorted);
@@ -132,20 +120,45 @@ export default function CRUDCollection({
// edits whose onChange round trip to the parent hasn't landed back in
// props yet.
const unsortedOrderRef = useRef<Array<string | number>>(
- initialKeyed.current!.collectionArray.map(item => item.id),
+ initialKeyed.current!.map(item => item.id),
);
const sortRef = useRef<SortOrderEnum>(SortOrderEnum.Unsorted);
sortRef.current = sort;
const collectionArrayRef = useRef<CollectionItem[]>(collectionArray);
- collectionArrayRef.current = collectionArray;
+ const onChangeRef = useRef(onChange);
+ onChangeRef.current = onChange;
+ const fieldChangeHandlers = useRef(
+ new Map<
+ CollectionItem['id'],
+ NonNullable<FieldsetProps['onFieldChange']>
+ >(),
+ );
+
+ const setCurrentCollection = useCallback((items: CollectionItem[]) => {
+ // Advance before notifying the parent so queued edits share the same rows.
+ collectionArrayRef.current = items;
+ const ids = new Set(items.map(item => item.id));
+ fieldChangeHandlers.current.forEach((_handler, id) => {
+ if (!ids.has(id)) fieldChangeHandlers.current.delete(id);
+ });
+ if (sortRef.current === SortOrderEnum.Unsorted) {
+ unsortedOrderRef.current = items.map(item => item.id);
+ }
+ setCollectionArray(items);
+ }, []);
+
+ const changeCollection = useCallback(
+ (items: CollectionItem[]) => {
+ setCurrentCollection(items);
+ onChangeRef.current?.(items);
+ },
+ [setCurrentCollection],
+ );
// Sync with props.collection changes
useEffect(() => {
- const { collection: newCollection, collectionArray: newCollectionArray } =
- createKeyedCollection(propsCollection);
- setCollection(newCollection);
- setCollectionArray(newCollectionArray);
+ const newCollectionArray = createKeyedCollection(propsCollection);
// Refresh the restore order too, so that clearing a sort after an
// external sync (e.g. a source-control-synced column set) reflects the
// synced order and row set instead of stale, pre-sync ids.
@@ -162,102 +175,28 @@ export default function CRUDCollection({
if (!isEcho) {
unsortedOrderRef.current = newIds;
}
- }, [propsCollection]);
-
- useEffect(() => {
- if (sort === SortOrderEnum.Unsorted) {
- unsortedOrderRef.current = collectionArray.map(item => item.id);
- }
- }, [collectionArray, sort]);
+ setCurrentCollection(newCollectionArray);
+ }, [propsCollection, setCurrentCollection]);
const onCellChange = useCallback(
- (id: string | number, col: string, val: unknown) => {
- setCollection(prevCollection => {
- const updatedCollection = {
- ...prevCollection,
- [id]: {
- ...prevCollection[id],
- [col]: val,
- },
- };
- return updatedCollection;
- });
-
- setCollectionArray(prevCollectionArray => {
- const updatedCollectionArray = prevCollectionArray.map(item => {
- if (item.id === id) {
- return {
- ...item,
- [col]: val,
- };
- }
- return item;
- });
-
- if (onChange) {
- onChange(updatedCollectionArray);
- }
-
- return updatedCollectionArray;
- });
- },
- [onChange],
- );
-
- const changeCollection = useCallback(
- (
- newCollection: Record<PropertyKey, CollectionItem>,
- currentCollectionArray: CollectionItem[],
- ) => {
- // Preserve existing order instead of recreating from Object.keys()
- const existingIds = new Set(currentCollectionArray.map(item => item.id));
- const newCollectionArray: CollectionItem[] = [];
-
- // First pass: preserve existing order and update items
- for (const existingItem of currentCollectionArray) {
- if (newCollection[existingItem.id]) {
- newCollectionArray.push(newCollection[existingItem.id]);
- }
- }
-
- // Second pass: add new items
- for (const item of Object.values(newCollection)) {
- if (!existingIds.has(item.id)) {
- newCollectionArray.push(item);
- }
- }
-
- setCollection(newCollection);
- setCollectionArray(newCollectionArray);
-
- if (onChange) {
- onChange(newCollectionArray);
- }
+ (id: string | number, col: string | number, val: unknown) => {
+ const items = collectionArrayRef.current;
+ // A delayed control callback must not restore a deleted row.
+ if (!items.some(item => item.id === id)) return;
+ changeCollection(
+ items.map(item => (item.id === id ? { ...item, [col]: val } : item)),
+ );
},
- [onChange],
+ [changeCollection],
);
const deleteItem = useCallback(
(id: string | number) => {
- setCollection(prevCollection => {
- const newColl = { ...prevCollection };
- delete newColl[id];
- return newColl;
- });
-
- setCollectionArray(prevCollectionArray => {
- const newCollectionArray = prevCollectionArray.filter(
- item => item.id !== id,
- );
-
- if (onChange) {
- onChange(newCollectionArray);
- }
-
- return newCollectionArray;
- });
+ changeCollection(
+ collectionArrayRef.current.filter(item => item.id !== id),
+ );
},
- [onChange],
+ [changeCollection],
);
const onAddItem = useCallback(() => {
@@ -269,39 +208,13 @@ export default function CRUDCollection({
}
delete newItem.expanded;
- setCollection(prevCollection => ({
- ...prevCollection,
- [newItem.id]: newItem,
- }));
-
- setCollectionArray(prevCollectionArray => {
- const newCollectionArray = [newItem, ...prevCollectionArray];
-
- if (onChange) {
- onChange(newCollectionArray);
- }
-
- return newCollectionArray;
- });
+ changeCollection([newItem, ...collectionArrayRef.current]);
if (shouldStartExpanded) {
setExpandedColumns(prev => ({ ...prev, [newItem.id]: true }));
}
}
- }, [itemGenerator, onChange]);
-
- const onFieldsetChange = useCallback(
- (item: CollectionItem) => {
- changeCollection(
- {
- ...collection,
- [item.id]: item,
- },
- collectionArray,
- );
- },
- [changeCollection, collection, collectionArray],
- );
+ }, [itemGenerator, changeCollection]);
const getLabel = useCallback(
(col: string): string => {
@@ -384,7 +297,7 @@ export default function CRUDCollection({
// Sort the live, edited collection rather than propsCollection, so
// an edit that hasn't round-tripped back through onChange yet
// isn't dropped when a sort is applied.
- sortedArray = [...collectionArray];
+ sortedArray = [...collectionArrayRef.current];
// Negate the comparator for descending order (rather than reversing
// the array) so equal-key rows keep their relative order.
const direction = newSortOrder === SortOrderEnum.Desc ? -1 : 1;
@@ -393,35 +306,49 @@ export default function CRUDCollection({
direction * compareSort(a[col] as Sort, b[col] as Sort),
);
} else {
- // Restore the pre-sort order, but take each row's current value
- // from the live `collection` map (not propsCollection) so an edit
- // made while sorted survives clearing the sort. Any row not part
- // of the tracked order (e.g. added while sorted) is appended.
+ // Restore the pre-sort order using the latest row values. Any row
+ // added while sorted is appended after the tracked rows.
+ const itemsById = new Map(
+ collectionArrayRef.current.map(item => [item.id, item]),
+ );
const trackedIds = new Set(unsortedOrderRef.current);
- sortedArray = unsortedOrderRef.current
- .filter(id => collection[id])
- .map(id => collection[id]);
- collectionArray.forEach(item => {
+ sortedArray = unsortedOrderRef.current.flatMap(id => {
+ const item = itemsById.get(id);
+ return item ? [item] : [];
+ });
+ collectionArrayRef.current.forEach(item => {
if (!trackedIds.has(item.id)) {
sortedArray.push(item);
}
});
}
- setCollectionArray(sortedArray);
+ sortRef.current = newSortOrder;
+ setCurrentCollection(sortedArray);
setSortColumn(newSortColumn);
setSort(newSortOrder);
}
},
- [collection, collectionArray, sortColumns, sortColumn, sort],
+ [setCurrentCollection, sortColumns, sortColumn, sort],
);
const renderExpandableSection = useCallback(
(item: CollectionItem): ReactNode => {
- const propsGenerator = () => ({ item, onChange: onFieldsetChange });
+ const { id } = item;
+ // SQL controls cancel their pending debounce when this callback changes.
+ // Keep each row's handler stable across collection edits and expansion.
+ if (!fieldChangeHandlers.current.has(id)) {
+ fieldChangeHandlers.current.set(id, (fieldKey, value) =>
+ onCellChange(id, fieldKey, value),
+ );
+ }
+ const propsGenerator = () => ({
+ item,
+ onFieldChange: fieldChangeHandlers.current.get(id),
+ });
return recurseReactClone(expandFieldset, Fieldset, propsGenerator);
},
- [expandFieldset, onFieldsetChange],
+ [expandFieldset, onCellChange],
);
const renderCell = useCallback(
diff --git a/superset-frontend/src/components/Datasource/types.ts
b/superset-frontend/src/components/Datasource/types.ts
index e770bae314d..e4edcd8bca3 100644
--- a/superset-frontend/src/components/Datasource/types.ts
+++ b/superset-frontend/src/components/Datasource/types.ts
@@ -104,16 +104,6 @@ export enum SortOrder {
Unsorted = 0,
}
-export interface CRUDCollectionState {
- collection: Record<PropertyKey, any>;
- collectionArray: Record<PropertyKey, any>[];
- expandedColumns: Record<PropertyKey, any>;
- sortColumn: string;
- sort: SortOrder;
- currentPage: number;
- pageSize: number;
-}
-
export enum FoldersEditorItemType {
Folder = 'folder',
Column = 'column',
