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 9c1d1bee4a4efd6bef52487a7ed704923cdc35c0 Author: Evan Rusackas <[email protected]> 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 <[email protected]> Co-Authored-By: Claude Sonnet 5 <[email protected]> --- .../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;
