From 9ab0f02e4eaf8dca66622606024201c7b0f4def4 Mon Sep 17 00:00:00 2001 From: Mehmet Salih Yavuz Date: Wed, 22 Oct 2025 10:41:57 +0300 Subject: [PATCH 1/6] fix(DatasourceEditor): preserve calculated column order when editing sql --- .../components/CollectionTable/index.tsx | 22 +++++--- .../DatasourceEditor/DatasourceEditor.jsx | 54 ++++++++++++++++++- 2 files changed, 68 insertions(+), 8 deletions(-) diff --git a/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx b/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx index a9bbda126a4b..9caa79ee59cc 100644 --- a/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx +++ b/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx @@ -53,9 +53,6 @@ const StyledButtonWrapper = styled.span` type CollectionItem = { id: string | number; [key: string]: any }; -function createCollectionArray(collection: Record) { - return Object.keys(collection).map(k => collection[k] as CollectionItem); -} function createKeyedCollection(arr: Array) { const collectionArray = arr.map( @@ -109,6 +106,7 @@ export default class CRUDCollection extends PureComponent< const { collection, collectionArray } = createKeyedCollection( this.props.collection, ); + this.setState(prevState => ({ collection, collectionArray, @@ -117,7 +115,7 @@ export default class CRUDCollection extends PureComponent< } } - onCellChange(id: number, col: string, val: boolean) { + onCellChange(id: number, col: string, val: any) { this.setState(prevState => { const updatedCollection = { ...prevState.collection, @@ -197,11 +195,21 @@ export default class CRUDCollection extends PureComponent< } changeCollection(collection: any) { - const newCollectionArray = createCollectionArray(collection); - this.setState({ collection, collectionArray: newCollectionArray }); + // Preserve existing order instead of recreating from Object.keys() + // Map existing items to their updated versions from the collection + const newCollectionArray = this.state.collectionArray + .map(existingItem => collection[existingItem.id] || existingItem) + .filter(item => collection[item.id]); // Remove deleted items + + // Add any new items that weren't in the existing array + const existingIds = new Set(this.state.collectionArray.map(item => item.id)); + const newItems = Object.values(collection).filter((item: any) => !existingIds.has(item.id)); + const finalCollectionArray = [...newCollectionArray, ...newItems]; + + this.setState({ collection, collectionArray: finalCollectionArray }); if (this.props.onChange) { - this.props.onChange(newCollectionArray); + this.props.onChange(finalCollectionArray); } } diff --git a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx index 04f4f18ef34b..e7a5459d6781 100644 --- a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx +++ b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx @@ -278,7 +278,7 @@ function ColumnCollectionTable({ label={t('SQL expression')} control={ @@ -691,11 +691,13 @@ class DatasourceEditor extends PureComponent { const { datasourceType, datasource } = this.state; const sql = datasourceType === DATASOURCE_TYPES.physical.key ? '' : datasource.sql; + const newDatasource = { ...this.state.datasource, sql, columns: [...this.state.databaseColumns, ...this.state.calculatedColumns], }; + this.props.onChange(newDatasource, this.state.errors); } @@ -1704,6 +1706,7 @@ class DatasourceEditor extends PureComponent { minLines={5} textAreaStyles={{ minWidth: '200px', maxWidth: '450px' }} resize="both" + debounceDelay={300} /> ), description: (v, onChange, label) => ( @@ -1888,6 +1891,55 @@ class DatasourceEditor extends PureComponent { ); } + componentDidUpdate(prevProps) { + // Preserve calculated columns order when props change to prevent jumping + if (this.props.datasource !== prevProps.datasource) { + const newCalculatedColumns = this.props.datasource.columns.filter(col => !!col.expression); + const currentCalculatedColumns = this.state.calculatedColumns; + + // Check if this is just an update to existing calculated columns (not addition/removal) + if (newCalculatedColumns.length === currentCalculatedColumns.length) { + // Create a map of current calculated columns by their identifier + const currentMap = new Map(); + currentCalculatedColumns.forEach((col, index) => { + const id = col.id || col.column_name; + currentMap.set(id, { ...col, originalIndex: index }); + }); + + // Try to preserve the order by matching with existing calculated columns + const orderedCalculatedColumns = []; + const usedIds = new Set(); + + // First, add existing columns in their current order + currentCalculatedColumns.forEach(currentCol => { + const id = currentCol.id || currentCol.column_name; + const updatedCol = newCalculatedColumns.find(newCol => + (newCol.id || newCol.column_name) === id + ); + if (updatedCol) { + orderedCalculatedColumns.push(updatedCol); + usedIds.add(id); + } + }); + + // Then add any new columns that weren't in the current list + newCalculatedColumns.forEach(newCol => { + const id = newCol.id || newCol.column_name; + if (!usedIds.has(id)) { + orderedCalculatedColumns.push(newCol); + } + }); + + + // Update state with preserved order + this.setState({ + calculatedColumns: orderedCalculatedColumns, + databaseColumns: this.props.datasource.columns.filter(col => !col.expression), + }); + } + } + } + componentDidMount() { Mousetrap.bind('ctrl+shift+f', e => { e.preventDefault(); From f3a7ee49128de6f00b2790522305d65d83a8ce53 Mon Sep 17 00:00:00 2001 From: Mehmet Salih Yavuz Date: Wed, 22 Oct 2025 11:12:12 +0300 Subject: [PATCH 2/6] formatting --- .../components/CollectionTable/index.tsx | 15 ++++++----- .../DatasourceEditor/DatasourceEditor.jsx | 27 ++++++++++--------- 2 files changed, 24 insertions(+), 18 deletions(-) diff --git a/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx b/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx index 9caa79ee59cc..ecabff63d797 100644 --- a/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx +++ b/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx @@ -53,7 +53,6 @@ const StyledButtonWrapper = styled.span` type CollectionItem = { id: string | number; [key: string]: any }; - function createKeyedCollection(arr: Array) { const collectionArray = arr.map( (o: any) => @@ -106,7 +105,7 @@ export default class CRUDCollection extends PureComponent< const { collection, collectionArray } = createKeyedCollection( this.props.collection, ); - + this.setState(prevState => ({ collection, collectionArray, @@ -200,12 +199,16 @@ export default class CRUDCollection extends PureComponent< const newCollectionArray = this.state.collectionArray .map(existingItem => collection[existingItem.id] || existingItem) .filter(item => collection[item.id]); // Remove deleted items - + // Add any new items that weren't in the existing array - const existingIds = new Set(this.state.collectionArray.map(item => item.id)); - const newItems = Object.values(collection).filter((item: any) => !existingIds.has(item.id)); + const existingIds = new Set( + this.state.collectionArray.map(item => item.id), + ); + const newItems = Object.values(collection).filter( + (item: any) => !existingIds.has(item.id), + ); const finalCollectionArray = [...newCollectionArray, ...newItems]; - + this.setState({ collection, collectionArray: finalCollectionArray }); if (this.props.onChange) { diff --git a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx index e7a5459d6781..1c653b7ac970 100644 --- a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx +++ b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx @@ -691,13 +691,13 @@ class DatasourceEditor extends PureComponent { const { datasourceType, datasource } = this.state; const sql = datasourceType === DATASOURCE_TYPES.physical.key ? '' : datasource.sql; - + const newDatasource = { ...this.state.datasource, sql, columns: [...this.state.databaseColumns, ...this.state.calculatedColumns], }; - + this.props.onChange(newDatasource, this.state.errors); } @@ -1894,9 +1894,11 @@ class DatasourceEditor extends PureComponent { componentDidUpdate(prevProps) { // Preserve calculated columns order when props change to prevent jumping if (this.props.datasource !== prevProps.datasource) { - const newCalculatedColumns = this.props.datasource.columns.filter(col => !!col.expression); + const newCalculatedColumns = this.props.datasource.columns.filter( + col => !!col.expression, + ); const currentCalculatedColumns = this.state.calculatedColumns; - + // Check if this is just an update to existing calculated columns (not addition/removal) if (newCalculatedColumns.length === currentCalculatedColumns.length) { // Create a map of current calculated columns by their identifier @@ -1905,23 +1907,23 @@ class DatasourceEditor extends PureComponent { const id = col.id || col.column_name; currentMap.set(id, { ...col, originalIndex: index }); }); - + // Try to preserve the order by matching with existing calculated columns const orderedCalculatedColumns = []; const usedIds = new Set(); - + // First, add existing columns in their current order currentCalculatedColumns.forEach(currentCol => { const id = currentCol.id || currentCol.column_name; - const updatedCol = newCalculatedColumns.find(newCol => - (newCol.id || newCol.column_name) === id + const updatedCol = newCalculatedColumns.find( + newCol => (newCol.id || newCol.column_name) === id, ); if (updatedCol) { orderedCalculatedColumns.push(updatedCol); usedIds.add(id); } }); - + // Then add any new columns that weren't in the current list newCalculatedColumns.forEach(newCol => { const id = newCol.id || newCol.column_name; @@ -1929,12 +1931,13 @@ class DatasourceEditor extends PureComponent { orderedCalculatedColumns.push(newCol); } }); - - + // Update state with preserved order this.setState({ calculatedColumns: orderedCalculatedColumns, - databaseColumns: this.props.datasource.columns.filter(col => !col.expression), + databaseColumns: this.props.datasource.columns.filter( + col => !col.expression, + ), }); } } From 22d41f1f454be99f750cc5b2c25c68cb6bf14f39 Mon Sep 17 00:00:00 2001 From: Mehmet Salih Yavuz Date: Wed, 22 Oct 2025 11:16:01 +0300 Subject: [PATCH 3/6] perf: address PR review comments - remove unused Map and optimize array operations --- .../components/CollectionTable/index.tsx | 34 +++++++++++-------- .../DatasourceEditor/DatasourceEditor.jsx | 7 ---- 2 files changed, 19 insertions(+), 22 deletions(-) diff --git a/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx b/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx index ecabff63d797..e2aef4565301 100644 --- a/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx +++ b/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx @@ -195,24 +195,28 @@ export default class CRUDCollection extends PureComponent< changeCollection(collection: any) { // Preserve existing order instead of recreating from Object.keys() - // Map existing items to their updated versions from the collection - const newCollectionArray = this.state.collectionArray - .map(existingItem => collection[existingItem.id] || existingItem) - .filter(item => collection[item.id]); // Remove deleted items - - // Add any new items that weren't in the existing array - const existingIds = new Set( - this.state.collectionArray.map(item => item.id), - ); - const newItems = Object.values(collection).filter( - (item: any) => !existingIds.has(item.id), - ); - const finalCollectionArray = [...newCollectionArray, ...newItems]; + // Single-pass optimization to reduce iterations and memory allocations + const existingIds = new Set(this.state.collectionArray.map(item => item.id)); + const newCollectionArray: CollectionItem[] = []; + + // First pass: preserve existing order and update items + for (const existingItem of this.state.collectionArray) { + if (collection[existingItem.id]) { + newCollectionArray.push(collection[existingItem.id]); + } + } + + // Second pass: add new items + for (const item of Object.values(collection) as CollectionItem[]) { + if (!existingIds.has(item.id)) { + newCollectionArray.push(item); + } + } - this.setState({ collection, collectionArray: finalCollectionArray }); + this.setState({ collection, collectionArray: newCollectionArray }); if (this.props.onChange) { - this.props.onChange(finalCollectionArray); + this.props.onChange(newCollectionArray); } } diff --git a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx index 1c653b7ac970..97d4b02861e5 100644 --- a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx +++ b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx @@ -1901,13 +1901,6 @@ class DatasourceEditor extends PureComponent { // Check if this is just an update to existing calculated columns (not addition/removal) if (newCalculatedColumns.length === currentCalculatedColumns.length) { - // Create a map of current calculated columns by their identifier - const currentMap = new Map(); - currentCalculatedColumns.forEach((col, index) => { - const id = col.id || col.column_name; - currentMap.set(id, { ...col, originalIndex: index }); - }); - // Try to preserve the order by matching with existing calculated columns const orderedCalculatedColumns = []; const usedIds = new Set(); From 5e9be8e74f91c1160584a86a5fca489109c2231e Mon Sep 17 00:00:00 2001 From: Mehmet Salih Yavuz Date: Wed, 22 Oct 2025 20:56:17 +0300 Subject: [PATCH 4/6] fix: ci --- .../Datasource/components/CollectionTable/index.tsx | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx b/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx index e2aef4565301..6124f555d3c0 100644 --- a/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx +++ b/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx @@ -196,16 +196,18 @@ export default class CRUDCollection extends PureComponent< changeCollection(collection: any) { // Preserve existing order instead of recreating from Object.keys() // Single-pass optimization to reduce iterations and memory allocations - const existingIds = new Set(this.state.collectionArray.map(item => item.id)); + const existingIds = new Set( + this.state.collectionArray.map(item => item.id), + ); const newCollectionArray: CollectionItem[] = []; - + // First pass: preserve existing order and update items for (const existingItem of this.state.collectionArray) { if (collection[existingItem.id]) { newCollectionArray.push(collection[existingItem.id]); } } - + // Second pass: add new items for (const item of Object.values(collection) as CollectionItem[]) { if (!existingIds.has(item.id)) { From f4e575691ecaaf8fb3a5d128c8212ff77b92e055 Mon Sep 17 00:00:00 2001 From: Mehmet Salih Yavuz Date: Wed, 22 Oct 2025 22:22:45 +0300 Subject: [PATCH 5/6] chore: cleanup --- .../components/Datasource/components/CollectionTable/index.tsx | 3 +-- .../components/DatasourceEditor/DatasourceEditor.jsx | 3 --- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx b/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx index 6124f555d3c0..01887b3adde1 100644 --- a/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx +++ b/superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx @@ -114,7 +114,7 @@ export default class CRUDCollection extends PureComponent< } } - onCellChange(id: number, col: string, val: any) { + onCellChange(id: number, col: string, val: boolean) { this.setState(prevState => { const updatedCollection = { ...prevState.collection, @@ -195,7 +195,6 @@ export default class CRUDCollection extends PureComponent< changeCollection(collection: any) { // Preserve existing order instead of recreating from Object.keys() - // Single-pass optimization to reduce iterations and memory allocations const existingIds = new Set( this.state.collectionArray.map(item => item.id), ); diff --git a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx index 97d4b02861e5..22ede5c41def 100644 --- a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx +++ b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx @@ -1706,7 +1706,6 @@ class DatasourceEditor extends PureComponent { minLines={5} textAreaStyles={{ minWidth: '200px', maxWidth: '450px' }} resize="both" - debounceDelay={300} /> ), description: (v, onChange, label) => ( @@ -1899,7 +1898,6 @@ class DatasourceEditor extends PureComponent { ); const currentCalculatedColumns = this.state.calculatedColumns; - // Check if this is just an update to existing calculated columns (not addition/removal) if (newCalculatedColumns.length === currentCalculatedColumns.length) { // Try to preserve the order by matching with existing calculated columns const orderedCalculatedColumns = []; @@ -1925,7 +1923,6 @@ class DatasourceEditor extends PureComponent { } }); - // Update state with preserved order this.setState({ calculatedColumns: orderedCalculatedColumns, databaseColumns: this.props.datasource.columns.filter( From 108099cad40c8c97fe7bc38ae213cc3591b60a16 Mon Sep 17 00:00:00 2001 From: Mehmet Salih Yavuz Date: Thu, 23 Oct 2025 17:27:57 +0300 Subject: [PATCH 6/6] Trigger CI workflows