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 235e20626e1656f1780fa433682a38007d96f31a Author: Evan Rusackas <[email protected]> AuthorDate: Thu Oct 1 00:58:34 2026 -0700 fix(dataset-editor): refresh restore order on sync, split editor refs, suppress sync echo Addresses review feedback on #44858: - CollectionTable: unsortedOrderRef now refreshes whenever propsCollection changes (not just while unsorted), so clearing a sort after an external sync restores the synced order/row set instead of a stale pre-sync one. - TextAreaControl: the inline editor and the "edit in modal" editor are now tracked with separate refs that null out on unmount, so an external sync can no longer target a destroyed or wrong editor instance; onEditorLoad also seeds its last-known value from the editor's actual buffer instead of a snapshot that can be stale by the time the async Ace module loads. - TextAreaControl: the sync effect's imperative setValue() no longer echoes back out through onChange, since Ace's setValue fires its own change event that would otherwise look like a user edit. Co-Authored-By: Evan Rusackas <[email protected]> Co-Authored-By: Claude Sonnet 5 <[email protected]> --- .../CollectionTable/CollectionTable.test.tsx | 49 +++++++ .../components/CollectionTable/index.tsx | 4 + .../components/controls/TextAreaControl.test.tsx | 89 ++++++++++++ .../components/controls/TextAreaControl.tsx | 152 +++++++++++++++------ 4 files changed, 255 insertions(+), 39 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 ea6dbff3b61..4f803ec06cb 100644 --- a/superset-frontend/src/components/Datasource/components/CollectionTable/CollectionTable.test.tsx +++ b/superset-frontend/src/components/Datasource/components/CollectionTable/CollectionTable.test.tsx @@ -81,3 +81,52 @@ test('preserves an edit made while sorted after the sort is cleared', () => { ) as HTMLInputElement; expect(inputAfterReset.value).toBe('EDITED'); }); + +test('restores the synced order after a sort is cleared following an external reorder', () => { + const collection = [ + { id: 1, column_name: 'c_col', type: 'VARCHAR' }, + { id: 2, column_name: 'a_col', type: 'VARCHAR' }, + { id: 3, column_name: 'b_col', type: 'VARCHAR' }, + ]; + + const { container, rerender } = render( + <CollectionTable + collection={collection} + tableColumns={['column_name', 'type']} + sortColumns={['column_name']} + />, + ); + + const sorter = container.querySelector('.ant-table-column-sorters'); + expect(sorter).toBeInTheDocument(); + + // Ascending sort by column_name. + fireEvent.click(sorter!); + + // Simulate an external sync (e.g. the source columns were reordered) + // landing while sorted: same ids, new canonical order. + const syncedCollection = [ + { id: 3, column_name: 'b_col', type: 'VARCHAR' }, + { id: 1, column_name: 'c_col', type: 'VARCHAR' }, + { id: 2, column_name: 'a_col', type: 'VARCHAR' }, + ]; + rerender( + <CollectionTable + collection={syncedCollection} + tableColumns={['column_name', 'type']} + sortColumns={['column_name']} + />, + ); + + // Cycle the sort back to unsorted (ascend -> descend -> cancel). + fireEvent.click(sorter!); + fireEvent.click(sorter!); + + const rows = container.querySelectorAll('.ant-table-tbody tr'); + expect(rows).toHaveLength(3); + // The restored order reflects the synced order, not the stale + // pre-sync order captured before the sort was ever applied. + expect(rows[0].textContent).toContain('b_col'); + expect(rows[1].textContent).toContain('c_col'); + expect(rows[2].textContent).toContain('a_col'); +}); diff --git a/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx b/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx index 010ccef2fe3..5db60bd60e4 100644 --- a/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx +++ b/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx @@ -141,6 +141,10 @@ export default function CRUDCollection({ createKeyedCollection(propsCollection); setCollection(newCollection); setCollectionArray(newCollectionArray); + // 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. + unsortedOrderRef.current = newCollectionArray.map(item => item.id); }, [propsCollection]); useEffect(() => { diff --git a/superset-frontend/src/explore/components/controls/TextAreaControl.test.tsx b/superset-frontend/src/explore/components/controls/TextAreaControl.test.tsx index d7e1c11bba1..ae6d33c8a7a 100644 --- a/superset-frontend/src/explore/components/controls/TextAreaControl.test.tsx +++ b/superset-frontend/src/explore/components/controls/TextAreaControl.test.tsx @@ -104,4 +104,93 @@ describe('TextArea', () => { // Same editor instance picked up the new value; it was not remounted. expect(container.querySelector('.ace_editor')).toBe(editorNode); }); + + test('does not re-notify onChange when an external sync sets the editor value', async () => { + const onChange = jest.fn(); + 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={onChange} + /> + </> + ); + } + const { container } = render(<Wrapper />); + await waitFor(() => { + expect(container.querySelector('.ace_text-input')).toBeInTheDocument(); + }); + const editorNode = container.querySelector('.ace_editor') as HTMLElement & { + env: { editor: { getValue: () => string } }; + }; + + fireEvent.click(screen.getByText('sync')); + + await waitFor(() => { + expect(editorNode.env.editor.getValue()).toBe('synced-from-source'); + }); + // Ace's setValue() fires its own change event; that echo must not be + // mistaken for the user typing and bounced back up through onChange. + expect(onChange).not.toHaveBeenCalled(); + }); + + test('keeps syncing the inline editor after the edit-in-modal dialog is opened and closed', 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(); + }); + const inlineEditorNode = container.querySelector( + '.ace_editor', + ) as HTMLElement & { + env: { editor: { getValue: () => string } }; + }; + + // Open, then close, the "edit in modal" dialog. Its own Ace instance + // mounts and is destroyed (destroyOnHidden) in the process; this used + // to leave the single shared editor ref pointing at that destroyed + // instance instead of the still-visible inline editor. + fireEvent.click(screen.getByRole('button', { name: /edit.*in modal/i })); + await waitFor(() => { + expect(container.querySelectorAll('.ace_editor')).toHaveLength(2); + }); + fireEvent.click(screen.getByTestId('close-modal-btn')); + await waitFor(() => { + expect(container.querySelectorAll('.ace_editor')).toHaveLength(1); + }); + + fireEvent.click(screen.getByText('sync')); + + await waitFor(() => { + expect(inlineEditorNode.env.editor.getValue()).toBe('synced-from-source'); + }); + }); }); diff --git a/superset-frontend/src/explore/components/controls/TextAreaControl.tsx b/superset-frontend/src/explore/components/controls/TextAreaControl.tsx index 60db863b569..82670b20bda 100644 --- a/superset-frontend/src/explore/components/controls/TextAreaControl.tsx +++ b/superset-frontend/src/explore/components/controls/TextAreaControl.tsx @@ -16,7 +16,14 @@ * specific language governing permissions and limitations * under the License. */ -import { useCallback, useEffect, useRef, useMemo } from 'react'; +import { + useCallback, + useEffect, + useRef, + useMemo, + type MutableRefObject, + type ReactNode, +} from 'react'; import { debounce } from 'lodash-es'; import { Input, @@ -53,6 +60,28 @@ interface AceEditorHandle { clearSelection: () => void; } +// The inline editor and the "edit in modal" editor are two Ace instances +// showing the same value; the modal one is unmounted (destroyOnHidden) each +// time the modal closes. Clearing the matching ref on unmount stops a later +// external sync from calling methods on that destroyed instance. +function EditorUnmountGuard({ + targetRef, + children, +}: { + targetRef: MutableRefObject<AceEditorHandle | null>; + children: ReactNode; +}) { + // eslint-disable-next-line react-hooks/exhaustive-deps -- targetRef is a + // stable useRef object; run only on mount/unmount, not on every render. + useEffect( + () => () => { + targetRef.current = null; + }, + [], + ); + return <>{children}</>; +} + interface TextAreaControlProps { name?: string; onChange?: (value: string) => void; @@ -110,14 +139,26 @@ 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 + // The inline editor and the "edit in modal" editor are tracked separately: + // both can be mounted at once (modal open), and the modal instance is + // unmounted/remounted independently of the inline one, so a single shared + // ref would end up pointing at whichever loaded last (see + // EditorUnmountGuard above for the unmount half of this). + const inlineEditorRef = useRef<AceEditorHandle | null>(null); + const modalEditorRef = useRef<AceEditorHandle | null>(null); + // Tracks the value each 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 lastInlineValueRef = useRef<string | undefined>(initialValue ?? value); + const lastModalValueRef = useRef<string | undefined>(initialValue ?? value); + // Ace's setValue() fires its normal change event, so the imperative sync + // below would otherwise echo straight back out through onChange and look + // like the user just typed the synced value. Set while that call is in + // flight so handleChange can tell the echo apart from a real edit. + const isSyncingRef = useRef(false); const debouncedOnChangeRef = useRef<ReturnType< typeof debounce<(value: string) => void> @@ -148,10 +189,20 @@ function TextAreaControl({ [], ); + // `inModal` tells us which editor's buffer this edit came from, so only + // that editor's "last known value" is marked as seen — the other editor + // (inline or modal) still looks stale to the sync effect below and picks + // the edit up too, keeping both presentations of the same value in sync. const handleChange = useCallback( - (val: string | { target: { value: string } }) => { + (val: string | { target: { value: string } }, inModal = false) => { const finalValue = typeof val === 'object' ? val.target.value : val; - lastEditorValueRef.current = finalValue; + (inModal ? lastModalValueRef : lastInlineValueRef).current = finalValue; + if (isSyncingRef.current) { + // This change event is the echo of our own imperative setValue() + // during an external sync, not the user typing; don't re-notify + // the parent with the value it just sent down. + return; + } if (debouncedOnChangeRef.current) { debouncedOnChangeRef.current(finalValue); } else { @@ -162,8 +213,15 @@ function TextAreaControl({ ); const onEditorLoad = useCallback( - (editor: AceEditorHandle) => { - editorRef.current = editor; + (editor: AceEditorHandle, inModal = false) => { + const ref = inModal ? modalEditorRef : inlineEditorRef; + const lastValueRef = inModal ? lastModalValueRef : lastInlineValueRef; + ref.current = editor; + // Loading the Ace module is async, so by the time this fires + // `initialValue`/`value` may have already moved past whatever was + // captured when this ref was created; read the buffer's actual + // mounted content instead of trusting that stale snapshot. + lastValueRef.current = editor.getValue(); hotkeys?.forEach(keyConfig => { editor.commands.addCommand({ name: keyConfig.name, @@ -175,25 +233,37 @@ 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. + // Pick up an `initialValue` change that didn't originate from either + // editor's own typing (handleChange above would have already updated the + // relevant lastValueRef for that case), without remounting the Ace + // instance or losing the user's current cursor position. Runs against + // both editors independently since either, both, or neither may be + // mounted at a given time. 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; + if (nextValue === undefined) { + return; } + const syncEditor = ( + editor: AceEditorHandle | null, + lastValueRef: MutableRefObject<string | undefined>, + ) => { + if ( + editor && + nextValue !== lastValueRef.current && + nextValue !== editor.getValue() + ) { + const cursorPos = editor.getCursorPosition(); + isSyncingRef.current = true; + editor.setValue(nextValue, 1); + isSyncingRef.current = false; + editor.clearSelection(); + editor.moveCursorToPosition(cursorPos); + lastValueRef.current = nextValue; + } + }; + syncEditor(inlineEditorRef.current, lastInlineValueRef); + syncEditor(modalEditorRef.current, lastModalValueRef); }, [initialValue, value]); const renderEditor = useCallback( @@ -220,21 +290,25 @@ function TextAreaControl({ } const codeEditor = ( - <div> - <TextAreaEditor - mode={language} - style={style} - minLines={effectiveMinLines} - maxLines={inModal ? 1000 : maxLines} - editorProps={{ $blockScrolling: true }} - onLoad={onEditorLoad} - defaultValue={initialValue ?? value} - readOnly={readOnly} - key={name} - {...restProps} - onChange={handleChange} - /> - </div> + <EditorUnmountGuard + targetRef={inModal ? modalEditorRef : inlineEditorRef} + > + <div> + <TextAreaEditor + mode={language} + style={style} + minLines={effectiveMinLines} + maxLines={inModal ? 1000 : maxLines} + editorProps={{ $blockScrolling: true }} + onLoad={editor => onEditorLoad(editor, inModal)} + defaultValue={initialValue ?? value} + readOnly={readOnly} + key={name} + {...restProps} + onChange={val => handleChange(val, inModal)} + /> + </div> + </EditorUnmountGuard> ); if (tooltipOptions && Object.keys(tooltipOptions).length > 0) {
