korbit-ai[bot] commented on code in PR #35790:
URL: https://github.com/apache/superset/pull/35790#discussion_r2450831151
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx:
##########
@@ -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 });
+ });
Review Comment:
### Unused Map creation with unnecessary object spreading <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The currentMap variable is created but never used in the subsequent logic,
representing wasted computation and memory allocation.
###### Why this matters
This creates unnecessary overhead by building a Map with spread operations
and index tracking that serves no purpose in the algorithm, consuming CPU
cycles and memory without benefit.
###### Suggested change ∙ *Feature Preview*
Remove the unused currentMap creation entirely, as shown in the previous
solution where only the newColumnsMap is needed for efficient lookups.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d2a6f176-dd26-4ccf-8db8-9f1bba7aa254/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d2a6f176-dd26-4ccf-8db8-9f1bba7aa254?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d2a6f176-dd26-4ccf-8db8-9f1bba7aa254?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d2a6f176-dd26-4ccf-8db8-9f1bba7aa254?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d2a6f176-dd26-4ccf-8db8-9f1bba7aa254)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:94fe7ad6-59b1-47d4-a4bb-137c0e6d8f20 -->
[](94fe7ad6-59b1-47d4-a4bb-137c0e6d8f20)
##########
superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx:
##########
@@ -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];
Review Comment:
### Inefficient array operations in changeCollection <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The changeCollection method creates multiple intermediate arrays and
performs redundant iterations over the collection data.
###### Why this matters
This approach creates unnecessary memory allocations and performs O(n)
operations multiple times, which could impact performance with large datasets
and frequent updates.
###### Suggested change ∙ *Feature Preview*
Combine the operations into a single pass to reduce iterations and memory
allocations:
```typescript
changeCollection(collection: any) {
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: newCollectionArray });
if (this.props.onChange) {
this.props.onChange(newCollectionArray);
}
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58caa36b-8f17-423b-86e2-965948c77967/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58caa36b-8f17-423b-86e2-965948c77967?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58caa36b-8f17-423b-86e2-965948c77967?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58caa36b-8f17-423b-86e2-965948c77967?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58caa36b-8f17-423b-86e2-965948c77967)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:5dbd0e31-de29-4b3c-82b3-a2d857e94cdf -->
[](5dbd0e31-de29-4b3c-82b3-a2d857e94cdf)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
