codeant-ai-for-open-source[bot] commented on code in PR #37555:
URL: https://github.com/apache/superset/pull/37555#discussion_r2834926835
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -2205,7 +2236,7 @@ class DatasourceEditor extends PureComponent<
</Fieldset>
</FormContainer>
}
- collection={sortedMetrics}
+ collection={filteredMetrics}
Review Comment:
**Suggestion:** Passing only the filtered metrics array into the editable
CollectionTable means that when a user edits, reorders, or deletes metrics
while a search filter is active, all metrics that do not match the filter will
be dropped from the dataset because the change handler receives and saves only
the filtered subset, not the full list. To avoid silently losing metrics, the
table should operate on the full sorted list instead of the filtered subset.
[logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dataset metrics silently removed when editing under search filter.
- ❌ Charts using removed metrics break or stop rendering.
- ⚠️ Dataset editor can corrupt metric configuration unexpectedly.
```
</details>
```suggestion
collection={sortedMetrics}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render the dataset editor UI which uses `DatasourceEditor` (exported as
`DataSourceComponent` at the bottom of
`superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx`)
for an existing dataset that has multiple metrics in
`props.datasource.metrics`.
2. In `DatasourceEditor.render()` (around lines 2353–2371), switch to the
"Metrics" tab,
which calls `this.renderMetricCollection()` (defined around line 2120).
3. In `renderMetricCollection()` (lines ~2120–2145), type a search string
into the
`<Input.Search>` control (line ~2137). This updates
`this.state.metricSearchTerm` and
causes `filteredMetrics` (lines ~2124–2133) to contain only metrics whose
`metric_name` or
`verbose_name` matches the search.
4. Edit or delete a metric in the `CollectionTable` rendered in
`renderMetricCollection()`
(lines ~2145–2310). Because `collection={filteredMetrics}` (line 2239) and
`onChange={this.onDatasourcePropChange.bind(this, 'metrics')}` (lines
~2241–2242),
`onDatasourcePropChange` (defined around lines 154–177) is called with
*only* the filtered
subset. It then sets `this.state.datasource.metrics` to that subset and
triggers
`onChange` up to the parent, permanently dropping all non-matching metrics
from the
dataset configuration.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx
**Line:** 2239:2239
**Comment:**
*Logic Error: Passing only the filtered metrics array into the editable
CollectionTable means that when a user edits, reorders, or deletes metrics
while a search filter is active, all metrics that do not match the filter will
be dropped from the dataset because the change handler receives and saves only
the filtered subset, not the full list. To avoid silently losing metrics, the
table should operate on the full sorted list instead of the filtered subset.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37555&comment_hash=db9c4ab7049993c84b053d7d4c8e9a8f2f1641907dd48f604f154e5108a42872&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37555&comment_hash=db9c4ab7049993c84b053d7d4c8e9a8f2f1641907dd48f604f154e5108a42872&reaction=dislike'>👎</a>
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -2349,9 +2377,28 @@ class DatasourceEditor extends PureComponent<
</Button>
</StyledButtonWrapper>
</ColumnButtonWrapper>
+ <Input.Search
+ placeholder={t('Search columns by name')}
+ value={this.state.columnSearchTerm}
+ onChange={e =>
+ this.setState({ columnSearchTerm: e.target.value })
+ }
+ style={{ marginBottom: 16, width: 300 }}
+ allowClear
+ />
<ColumnCollectionTable
className="columns-table"
- columns={this.state.databaseColumns}
+ columns={
+ this.state.columnSearchTerm
+ ? this.state.databaseColumns.filter(col =>
+ col.column_name
+ ?.toLowerCase()
+ .includes(
+ this.state.columnSearchTerm.toLowerCase(),
+ ),
+ )
+ : this.state.databaseColumns
+ }
Review Comment:
**Suggestion:** In the Columns tab, passing a filtered subset of columns
into ColumnCollectionTable means that when the user edits or deletes columns
while a search filter is active, any columns that do not match the filter will
be removed from state because the onColumnsChange callback receives only the
filtered subset. To prevent unintended column loss, ColumnCollectionTable
should receive the full columns list rather than the filtered one. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dataset columns disappear after editing under a column search.
- ❌ Charts depending on removed columns may fail or misbehave.
- ⚠️ Dataset schema can be unintentionally corrupted by editors.
```
</details>
```suggestion
columns={this.state.databaseColumns}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render the dataset editor UI using `DatasourceEditor` (default export
`DataSourceComponent` in
`superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx`)
for a dataset with many physical columns in `props.datasource.columns`.
2. In `DatasourceEditor.render()` (lines ~2353–2413), switch to the
"Columns" tab. This
renders the `<Input.Search>` for columns (lines ~2380–2388) and the
`<ColumnCollectionTable>` (starting around line 2389).
3. Type a search term in the columns search box so that
`this.state.columnSearchTerm` is
non-empty. The `columns` prop passed into `ColumnCollectionTable` (lines
2391–2401)
becomes a filtered subset of `this.state.databaseColumns`, only those whose
`column_name`
matches the search.
4. Edit or delete a column in the filtered table. `ColumnCollectionTable`
(defined earlier
in the same file around lines 140–230) passes its `collection={columns}`
prop straight to
the underlying `CollectionTable` and forwards changes via
`onChange={onColumnsChange}`. In
the Columns tab, `onColumnsChange` is `databaseColumns => this.setColumns({
databaseColumns })` (lines ~2398–2405), so `this.state.databaseColumns` is
overwritten
with the filtered subset, permanently dropping all non-matching columns from
the dataset.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx
**Line:** 2391:2401
**Comment:**
*Logic Error: In the Columns tab, passing a filtered subset of columns
into ColumnCollectionTable means that when the user edits or deletes columns
while a search filter is active, any columns that do not match the filter will
be removed from state because the onColumnsChange callback receives only the
filtered subset. To prevent unintended column loss, ColumnCollectionTable
should receive the full columns list rather than the filtered one.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37555&comment_hash=8e6469611979003eee3e2ed40bcebfbdffc8afc309468087fa43938f349b8d94&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37555&comment_hash=8e6469611979003eee3e2ed40bcebfbdffc8afc309468087fa43938f349b8d94&reaction=dislike'>👎</a>
##########
superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx:
##########
@@ -416,6 +435,16 @@ export default class CRUDCollection extends PureComponent<
}
: undefined;
+ // Build controlled pagination config
+ const paginationConfig: false | TablePaginationConfig | undefined =
+ pagination === false || pagination === undefined
+ ? pagination
+ : {
+ ...(typeof pagination === 'object' ? pagination : {}),
+ current: this.state.currentPage,
+ pageSize: this.state.pageSize,
Review Comment:
**Suggestion:** When the underlying collection shrinks (for example due to
filtering/search), the controlled `currentPage` can exceed the number of
available pages, causing the table to render an empty page instead of
automatically resetting to a valid page; you should clamp the current page
based on the current collection size before passing it to the Table's
pagination config. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Metrics tab search can show blank page with results.
- ⚠️ Users may think metrics were deleted after filtering.
- ⚠️ Affects datasource editor performance-focused metrics workflow.
```
</details>
```suggestion
const totalItems = this.state.collectionArray.length;
const pageSize = this.state.pageSize || 10;
const maxPage =
totalItems > 0 ? Math.max(1, Math.ceil(totalItems / pageSize)) : 1;
const currentPage = Math.min(this.state.currentPage || 1, maxPage);
const paginationConfig: false | TablePaginationConfig | undefined =
pagination === false || pagination === undefined
? pagination
: {
...(typeof pagination === 'object' ? pagination : {}),
current: currentPage,
pageSize,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open any dataset in the Datasource Editor UI (implemented in
`superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx`,
class `DatasourceEditor`, file start line 307) that has more than 50 metrics
so that
pagination is needed.
2. Navigate to the "Metrics" tab, which renders the metrics table via
`this.renderMetricCollection()` and uses `<CollectionTable />`
(`CRUDCollection`) with
`pagination={{ pageSize: 25, showSizeChanger: true, pageSizeOptions: [10,
25, 50, 100] }}`
and `collection={filteredMetrics}` (see `renderMetricCollection` in
`DatasourceEditor.tsx`).
3. In the metrics table, use the pagination controls (from
`@superset-ui/core/components/Table`) to go to page 3; this triggers
`CRUDCollection.handleTableChange` in `CollectionTable/index.tsx` (around
lines 249–270),
which updates `this.state.currentPage` to 3 without changing anything else.
4. In the search box above the metrics table (`Input.Search` in
`renderMetricCollection`),
type a filter that reduces `filteredMetrics` to fewer than 25 items (one
page).
`DatasourceEditor` re-renders with the smaller `collection` prop,
`CRUDCollection.componentDidUpdate` rebuilds `collectionArray`, but
`currentPage` remains
3. The subsequent `render()` call builds `paginationConfig` using the
existing code at
lines 438–446, passing `current: this.state.currentPage` (3) and `pageSize:
this.state.pageSize` to `<Table>`. The table then renders an empty page
(showing the empty
text like "No items") even though matching metrics exist, until the user
manually changes
back to page 1.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx
**Line:** 439:445
**Comment:**
*Logic Error: When the underlying collection shrinks (for example due
to filtering/search), the controlled `currentPage` can exceed the number of
available pages, causing the table to render an empty page instead of
automatically resetting to a valid page; you should clamp the current page
based on the current collection size before passing it to the Table's
pagination config.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37555&comment_hash=3c3ee2622145ac221b1232f575176ac1519cb4b3cc70fcf86208462472024cbd&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37555&comment_hash=3c3ee2622145ac221b1232f575176ac1519cb4b3cc70fcf86208462472024cbd&reaction=dislike'>👎</a>
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DatasetUsageTab/index.tsx:
##########
@@ -289,15 +294,52 @@ const DatasetUsageTab = ({
[handleSortChange, sortColumn, sortDirection],
);
+ const filteredCharts = useMemo(() => {
+ if (!searchTerm) return charts;
+
+ const lowerSearch = searchTerm.toLowerCase();
+ return charts.filter(chart => {
+ // Search in chart name
+ if (chart.slice_name?.toLowerCase().includes(lowerSearch)) return true;
+
+ // Search in owner names
+ if (
+ chart.owners?.some(
+ owner =>
+ owner.first_name?.toLowerCase().includes(lowerSearch) ||
+ owner.last_name?.toLowerCase().includes(lowerSearch),
+ )
+ )
+ return true;
+
+ // Search in dashboard titles
+ if (
+ chart.dashboards?.some(dashboard =>
+ dashboard.dashboard_title?.toLowerCase().includes(lowerSearch),
+ )
+ )
+ return true;
+
+ return false;
+ });
+ }, [charts, searchTerm]);
+
return (
Review Comment:
**Suggestion:** When a user is on a later page and then enters a search
term, the filtered data set can shrink to fewer pages but the `currentPage`
state is left unchanged, so the table can request/render a non-existent page
and show an empty "No items" state even though matches exist on page 1;
resetting `currentPage` when search results reduce the available pages avoids
this inconsistent pagination state. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dataset Usage search can show empty results incorrectly.
- ⚠️ Users may think charts have no usage records.
```
</details>
```suggestion
useEffect(() => {
if (!searchTerm) {
return;
}
const maxPage = Math.max(1, Math.ceil(filteredCharts.length /
PAGE_SIZE));
if (currentPage > maxPage) {
setCurrentPage(1);
}
}, [searchTerm, filteredCharts.length, currentPage]);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run Superset with this PR code and open a dataset in the datasource
editor; navigate to
the "Usage" tab which renders `DatasetUsageTab`
(`superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DatasetUsageTab/index.tsx`).
2. Ensure the selected dataset has more than `PAGE_SIZE` (25) related charts
so that
pagination is active; use the table pagination controls rendered by
`<Table>` (pagination
prop defined around lines 340–347 in the same file) to switch to page 3 or
higher, which
updates the `currentPage` state via `handleTableChange` and
`handlePageChange`.
3. While still on page 3+, type a search term into the search box rendered by
`<Input.Search>` (lines 327–335), so that `searchTerm` state changes and
`filteredCharts`
(lines 297–325) recomputes to a non-empty subset of `charts` for that page;
note that
`currentPage` is not changed anywhere in response to `searchTerm`.
4. Observe that the `<Table>` component receives `data={filteredCharts}` and
`pagination={{ current: currentPage, total: filteredCharts.length, pageSize:
PAGE_SIZE,
... }}` (lines 336–347); with `currentPage` still set to 3 while `total` is
now at most 25
(one page), the underlying table pagination logic (Ant Design-based
`@superset-ui/core/components/Table`) will render an out-of-range page,
resulting in an
empty table with the `locale.emptyText` value `"No items"` even though
`filteredCharts`
contains matching rows for the search.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DatasetUsageTab/index.tsx
**Line:** 327:327
**Comment:**
*Logic Error: When a user is on a later page and then enters a search
term, the filtered data set can shrink to fewer pages but the `currentPage`
state is left unchanged, so the table can request/render a non-existent page
and show an empty "No items" state even though matches exist on page 1;
resetting `currentPage` when search results reduce the available pages avoids
this inconsistent pagination state.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37555&comment_hash=37bc6d2c986961f8bc0374023a02c3e2186af69746e0b68478b8334df38896da&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37555&comment_hash=37bc6d2c986961f8bc0374023a02c3e2186af69746e0b68478b8334df38896da&reaction=dislike'>👎</a>
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -2372,12 +2419,29 @@ class DatasourceEditor extends PureComponent<
),
children: (
<StyledTableTabWrapper>
- {this.renderDefaultColumnSettings()}
- <DefaultColumnSettingsTitle>
- {t('Column Settings')}
- </DefaultColumnSettingsTitle>
+ <Input.Search
+ placeholder={t('Search calculated columns by name')}
+ value={this.state.calculatedColumnSearchTerm}
+ onChange={e =>
+ this.setState({
+ calculatedColumnSearchTerm: e.target.value,
+ })
+ }
+ style={{ marginBottom: 16, width: 300 }}
+ allowClear
+ />
<ColumnCollectionTable
- columns={this.state.calculatedColumns}
+ columns={
+ this.state.calculatedColumnSearchTerm
+ ? this.state.calculatedColumns.filter(col =>
+ col.column_name
+ ?.toLowerCase()
+ .includes(
+
this.state.calculatedColumnSearchTerm.toLowerCase(),
+ ),
+ )
+ : this.state.calculatedColumns
Review Comment:
**Suggestion:** In the Calculated Columns tab, providing a filtered subset
of calculated columns to ColumnCollectionTable means that edits or deletions
performed while a search is active will cause all non-matching calculated
columns to disappear from the dataset, since the change handler receives only
the filtered subset. The table should instead receive the full
calculatedColumns array to avoid inadvertently dropping hidden items. [logic
error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Non-matching calculated columns lost when editing under search.
- ❌ Charts relying on removed calculated columns may stop working.
- ⚠️ Dataset's derived fields can be unintentionally pruned.
```
</details>
```suggestion
columns={this.state.calculatedColumns}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render `DatasourceEditor` (default export `DataSourceComponent` in
`superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx`)
for a dataset that has several calculated columns (columns with `expression`
set) in
`props.datasource.columns`.
2. In `DatasourceEditor.render()` (lines ~2412–2470), switch to the
"Calculated columns"
tab. This tab renders an `<Input.Search>` bound to
`this.state.calculatedColumnSearchTerm`
(lines ~2422–2431) and a `<ColumnCollectionTable>` whose `columns` prop is
conditionally
filtered (lines 2433–2443).
3. Enter a search term so that `this.state.calculatedColumnSearchTerm` is
non-empty,
causing `columns` passed to `ColumnCollectionTable` to be a filtered subset
of
`this.state.calculatedColumns` where `column_name` matches the search.
4. Edit or delete a calculated column in the table. `ColumnCollectionTable`
passes its
`collection={columns}` through to `CollectionTable` and calls
`onColumnsChange={calculatedColumns => this.setColumns({ calculatedColumns
})}` (lines
~2445–2452). `setColumns` (defined around lines 200–214) then sets
`this.state.calculatedColumns` to the filtered subset, dropping all
non-matching
calculated columns from state and from the dataset once `onChange` is
propagated.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx
**Line:** 2433:2443
**Comment:**
*Logic Error: In the Calculated Columns tab, providing a filtered
subset of calculated columns to ColumnCollectionTable means that edits or
deletions performed while a search is active will cause all non-matching
calculated columns to disappear from the dataset, since the change handler
receives only the filtered subset. The table should instead receive the full
calculatedColumns array to avoid inadvertently dropping hidden items.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37555&comment_hash=a9778d0ac58859c0b1d474e92ea92a1202ad4a42bfa8e5b2a9fabf958ec24fe7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37555&comment_hash=a9778d0ac58859c0b1d474e92ea92a1202ad4a42bfa8e5b2a9fabf958ec24fe7&reaction=dislike'>👎</a>
--
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]
