You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/10/01 15:05:53 UTC
(superset) 01/04: fix(dataset-editor): stop losing edits on sort-clear, pick up synced SQL externally
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch fix-datasource-editor-sort-and-stale-sql
in repository https://gitbox.apache.org/repos/asf/superset.git
commit baa6cf1649fa319fedc93f53f7f6ed10a7570c99
Author: Evan Rusackas <ev...@rusackas.com>
AuthorDate: Wed Sep 30 21:59:43 2026 -0700
fix(dataset-editor): stop losing edits on sort-clear, pick up synced SQL externally
CollectionTable.handleTableChange() reset rows from the propsCollection
prop on both sorting and clearing a sort, so an inline edit whose
onChange hadn't round-tripped back into the parent's state yet got
silently discarded. It now sorts and restores order from the live
collection/collectionArray state instead, tracking the pre-sort row
order in a ref.
TextAreaControl's Ace editor used an uncontrolled `defaultValue`, so a
later `initialValue` prop change (e.g. "Sync columns from source")
never reached an already-mounted editor. It now diffs incoming values
against what the editor itself last emitted, and imperatively pushes a
real external change into the live Ace instance while preserving
cursor position, without remounting or fighting the user's own typing.
Follow-up from apache/superset#39257.
Co-Authored-By: Evan Rusackas <ev...@rusackas.com>
Co-Authored-By: Claude Sonnet 5 <no...@anthropic.com>
---
.../CollectionTable/CollectionTable.test.tsx | 49 ++++++++++++++++++-
.../components/CollectionTable/index.tsx | 37 ++++++++++++--
.../components/controls/TextAreaControl.test.tsx | 43 ++++++++++++++++
.../components/controls/TextAreaControl.tsx | 57 ++++++++++++++++++----
4 files changed, 171 insertions(+), 15 deletions(-)
diff --git a/superset-frontend/src/components/Datasource/components/CollectionTable/CollectionTable.test.tsx b/superset-frontend/src/components/Datasource/components/CollectionTable/CollectionTable.test.tsx
index bb607479f19..ea6dbff3b61 100644
--- a/superset-frontend/src/components/Datasource/components/CollectionTable/CollectionTable.test.tsx
+++ b/superset-frontend/src/components/Datasource/components/CollectionTable/CollectionTable.test.tsx
@@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { render } from 'spec/helpers/testing-library';
+import { fireEvent, render } from 'spec/helpers/testing-library';
import mockDatasource from 'spec/fixtures/mockDatasource';
import CollectionTable from '.';
@@ -34,3 +34,50 @@ test('renders a table', () => {
const rows = tableBody?.getElementsByTagName('tr');
expect(rows).toHaveLength(mockDatasource['7__table'].columns.length);
});
+
+test('preserves an edit made while sorted after the sort is cleared', () => {
+ const onChange = jest.fn();
+ const collection = [
+ { id: 1, column_name: 'b_col', type: 'VARCHAR' },
+ { id: 2, column_name: 'a_col', type: 'VARCHAR' },
+ ];
+
+ const { container } = render(
+ <CollectionTable
+ collection={collection}
+ tableColumns={['column_name', 'type']}
+ sortColumns={['column_name']}
+ itemRenderers={{
+ type: (val, onItemChange, _label, record) => (
+ <input
+ data-test={`type-input-${record.id}`}
+ value={val as string}
+ onChange={e => onItemChange(e.target.value)}
+ />
+ ),
+ }}
+ onChange={onChange}
+ />,
+ );
+
+ const sorter = container.querySelector('.ant-table-column-sorters');
+ expect(sorter).toBeInTheDocument();
+
+ // Ascending sort by column_name puts a_col (id 2) first.
+ fireEvent.click(sorter!);
+
+ const editedInput = container.querySelector(
+ '[data-test="type-input-2"]',
+ ) as HTMLInputElement;
+ expect(editedInput).toBeInTheDocument();
+ fireEvent.change(editedInput, { target: { value: 'EDITED' } });
+
+ // Cycle the sort back to unsorted (ascend -> descend -> cancel).
+ fireEvent.click(sorter!);
+ fireEvent.click(sorter!);
+
+ const inputAfterReset = container.querySelector(
+ '[data-test="type-input-2"]',
+ ) as HTMLInputElement;
+ expect(inputAfterReset.value).toBe('EDITED');
+});
diff --git a/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx b/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx
index 8793a05b8d3..010ccef2fe3 100644
--- a/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx
+++ b/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx
@@ -127,6 +127,13 @@ export default function CRUDCollection({
: 10,
);
const [currentPage, setCurrentPage] = useState<number>(1);
+ // The row order to restore when a sort is cleared. Tracked from
+ // collectionArray itself (not propsCollection) so it reflects in-progress
+ // 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),
+ );
// Sync with props.collection changes
useEffect(() => {
@@ -136,6 +143,12 @@ export default function CRUDCollection({
setCollectionArray(newCollectionArray);
}, [propsCollection]);
+ useEffect(() => {
+ if (sort === SortOrderEnum.Unsorted) {
+ unsortedOrderRef.current = collectionArray.map(item => item.id);
+ }
+ }, [collectionArray, sort]);
+
const onCellChange = useCallback(
(id: string | number, col: string, val: unknown) => {
setCollection(prevCollection => {
@@ -323,7 +336,7 @@ export default function CRUDCollection({
sortColumns?.includes(col) ||
newSortOrder === SortOrderEnum.Unsorted
) {
- let sortedArray = [...propsCollection] as CollectionItem[];
+ let sortedArray: CollectionItem[];
if (newSortOrder !== SortOrderEnum.Unsorted) {
const compareSort = (m: Sort, n: Sort) => {
@@ -341,6 +354,10 @@ export default function CRUDCollection({
return mStr.localeCompare(nStr);
};
+ // 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.sort((a: CollectionItem, b: CollectionItem) =>
compareSort(a[col] as Sort, b[col] as Sort),
);
@@ -348,9 +365,19 @@ export default function CRUDCollection({
sortedArray.reverse();
}
} else {
- const { collectionArray: resetArray } =
- createKeyedCollection(propsCollection);
- sortedArray = resetArray;
+ // 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.
+ const trackedIds = new Set(unsortedOrderRef.current);
+ sortedArray = unsortedOrderRef.current
+ .filter(id => collection[id])
+ .map(id => collection[id]);
+ collectionArray.forEach(item => {
+ if (!trackedIds.has(item.id)) {
+ sortedArray.push(item);
+ }
+ });
}
setCollectionArray(sortedArray);
@@ -358,7 +385,7 @@ export default function CRUDCollection({
setSort(newSortOrder);
}
},
- [propsCollection, sortColumns],
+ [collection, collectionArray, sortColumns],
);
const renderExpandableSection = useCallback(
diff --git a/superset-frontend/src/explore/components/controls/TextAreaControl.test.tsx b/superset-frontend/src/explore/components/controls/TextAreaControl.test.tsx
index 46921087eae..d7e1c11bba1 100644
--- a/superset-frontend/src/explore/components/controls/TextAreaControl.test.tsx
+++ b/superset-frontend/src/explore/components/controls/TextAreaControl.test.tsx
@@ -16,6 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
+import { useState } from 'react';
import {
fireEvent,
render,
@@ -61,4 +62,46 @@ describe('TextArea', () => {
fireEvent.change(textArea, { target: { value: 'x' } });
expect(defaultProps.onChange).toHaveBeenCalledWith('x');
});
+
+ test('picks up an externally changed initialValue without remounting the AceEditor', async () => {
+ function Wrapper() {
+ const [initialValue, setInitialValue] = useState('first');
+ return (
+ <>
+ <button
+ type="button"
+ onClick={() => setInitialValue('synced-from-source')}
+ >
+ sync
+ </button>
+ <TextAreaControl
+ name="expr"
+ language="sql"
+ initialValue={initialValue}
+ onChange={() => {}}
+ />
+ </>
+ );
+ }
+ const { container } = render(<Wrapper />);
+ await waitFor(() => {
+ expect(container.querySelector('.ace_text-input')).toBeInTheDocument();
+ });
+ // react-ace keeps a reference to the live editor instance on the
+ // container node's `env`, which lets us read the underlying Ace
+ // document directly; jsdom doesn't paint Ace's text layer, so the
+ // rendered DOM has no visible text to assert against.
+ const editorNode = container.querySelector('.ace_editor') as HTMLElement & {
+ env: { editor: { getValue: () => string } };
+ };
+ expect(editorNode.env.editor.getValue()).toBe('first');
+
+ fireEvent.click(screen.getByText('sync'));
+
+ await waitFor(() => {
+ expect(editorNode.env.editor.getValue()).toBe('synced-from-source');
+ });
+ // Same editor instance picked up the new value; it was not remounted.
+ expect(container.querySelector('.ace_editor')).toBe(editorNode);
+ });
});
diff --git a/superset-frontend/src/explore/components/controls/TextAreaControl.tsx b/superset-frontend/src/explore/components/controls/TextAreaControl.tsx
index 753b6fd8e73..60db863b569 100644
--- a/superset-frontend/src/explore/components/controls/TextAreaControl.tsx
+++ b/superset-frontend/src/explore/components/controls/TextAreaControl.tsx
@@ -38,6 +38,21 @@ interface HotkeyConfig {
func: () => void;
}
+interface AceEditorHandle {
+ commands: {
+ addCommand: (cmd: {
+ name: string;
+ bindKey: { win: string; mac: string };
+ exec: () => void;
+ }) => void;
+ };
+ getValue: () => string;
+ setValue: (value: string, cursorPos?: number) => void;
+ getCursorPosition: () => { row: number; column: number };
+ moveCursorToPosition: (pos: { row: number; column: number }) => void;
+ clearSelection: () => void;
+}
+
interface TextAreaControlProps {
name?: string;
onChange?: (value: string) => void;
@@ -95,6 +110,15 @@ function TextAreaControl({
}: TextAreaControlProps) {
const theme = useTheme();
+ const editorRef = useRef<AceEditorHandle | null>(null);
+ // Tracks the value the Ace editor's own buffer currently holds, so an
+ // `initialValue` prop change caused by the user's own typing (echoed back
+ // through onChange) can be told apart from a genuinely external update
+ // (e.g. a "sync from source" action elsewhere). Only the latter should
+ // push an imperative setValue(); doing it unconditionally would reset the
+ // cursor to the end of the document on every keystroke.
+ const lastEditorValueRef = useRef<string | undefined>(initialValue ?? value);
+
const debouncedOnChangeRef = useRef<ReturnType<
typeof debounce<(value: string) => void>
> | null>(null);
@@ -127,6 +151,7 @@ function TextAreaControl({
const handleChange = useCallback(
(val: string | { target: { value: string } }) => {
const finalValue = typeof val === 'object' ? val.target.value : val;
+ lastEditorValueRef.current = finalValue;
if (debouncedOnChangeRef.current) {
debouncedOnChangeRef.current(finalValue);
} else {
@@ -137,15 +162,8 @@ function TextAreaControl({
);
const onEditorLoad = useCallback(
- (editor: {
- commands: {
- addCommand: (cmd: {
- name: string;
- bindKey: { win: string; mac: string };
- exec: () => void;
- }) => void;
- };
- }) => {
+ (editor: AceEditorHandle) => {
+ editorRef.current = editor;
hotkeys?.forEach(keyConfig => {
editor.commands.addCommand({
name: keyConfig.name,
@@ -157,6 +175,27 @@ function TextAreaControl({
[hotkeys],
);
+ // Pick up an `initialValue` change that didn't originate from this
+ // editor's own typing (handleChange above would have already updated
+ // lastEditorValueRef for that case), without remounting the Ace instance
+ // or losing the user's current cursor position.
+ useEffect(() => {
+ const nextValue = initialValue ?? value;
+ const editor = editorRef.current;
+ if (
+ editor &&
+ nextValue !== undefined &&
+ nextValue !== lastEditorValueRef.current &&
+ nextValue !== editor.getValue()
+ ) {
+ const cursorPos = editor.getCursorPosition();
+ editor.setValue(nextValue, 1);
+ editor.clearSelection();
+ editor.moveCursorToPosition(cursorPos);
+ lastEditorValueRef.current = nextValue;
+ }
+ }, [initialValue, value]);
+
const renderEditor = useCallback(
(inModal = false) => {
const effectiveMinLines = inModal ? 40 : minLines || 12;