bito-code-review[bot] commented on code in PR #37555:
URL: https://github.com/apache/superset/pull/37555#discussion_r2819461198
##########
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]);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Search breaks pagination</b></div>
<div id="fix">
The client-side search filters the current charts array, but the pagination
remains server-driven. When users search and then click pagination buttons, it
fetches unfiltered data from the server, causing the table to display incorrect
results. This breaks the expected search behavior. Consider either disabling
pagination during search or implementing local pagination for the filtered
results.
</div>
</div>
<small><i>Code Review Run #59bf11</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DatasetUsageTab/DatasetUsageTab.test.tsx:
##########
@@ -547,3 +547,136 @@ test('handles AbortError without setState after unmount',
async () => {
consoleErrorSpy.mockRestore();
});
+
+test('can search charts by chart name', async () => {
+ setupTest();
+
+ await waitFor(() => {
+ expect(screen.getByText('Test Chart 1')).toBeInTheDocument();
+ expect(screen.getByText('Test Chart 2')).toBeInTheDocument();
+ });
+
+ const searchInput = screen.getByPlaceholderText(
+ 'Search charts by name, owner, or dashboard',
+ );
+ expect(searchInput).toBeInTheDocument();
+
+ await userEvent.type(searchInput, 'Chart 1');
+
+ await waitFor(() => {
+ expect(screen.getByText('Test Chart 1')).toBeInTheDocument();
+ expect(screen.queryByText('Test Chart 2')).not.toBeInTheDocument();
+ });
+
+ await userEvent.clear(searchInput);
+
+ await waitFor(() => {
+ expect(screen.getByText('Test Chart 1')).toBeInTheDocument();
+ expect(screen.getByText('Test Chart 2')).toBeInTheDocument();
+ });
+});
+
+test('can search charts by owner name', async () => {
+ setupTest();
+
+ await waitFor(() => {
+ expect(screen.getByText('Test Chart 1')).toBeInTheDocument();
+ });
+
+ const searchInput = screen.getByPlaceholderText(
+ 'Search charts by name, owner, or dashboard',
+ );
+
+ await userEvent.type(searchInput, 'Bob');
+
+ await waitFor(() => {
+ expect(screen.queryByText('Test Chart 1')).not.toBeInTheDocument();
+ expect(screen.getByText('Test Chart 2')).toBeInTheDocument();
+ });
+});
+
+test('can search charts by dashboard title', async () => {
+ setupTest();
+
+ await waitFor(() => {
+ expect(screen.getByText('Test Chart 1')).toBeInTheDocument();
+ });
+
+ const searchInput = screen.getByPlaceholderText(
+ 'Search charts by name, owner, or dashboard',
+ );
+
+ await userEvent.type(searchInput, 'Test Dashboard');
+
+ await waitFor(() => {
+ expect(screen.getByText('Test Chart 1')).toBeInTheDocument();
+ expect(screen.queryByText('Test Chart 2')).not.toBeInTheDocument();
+ });
+});
+
+test('chart search is case-insensitive', async () => {
+ setupTest();
+
+ await waitFor(() => {
+ expect(screen.getByText('Test Chart 1')).toBeInTheDocument();
+ });
+
+ const searchInput = screen.getByPlaceholderText(
+ 'Search charts by name, owner, or dashboard',
+ );
+
+ await userEvent.type(searchInput, 'CHART 1');
+
+ await waitFor(() => {
+ expect(screen.getByText('Test Chart 1')).toBeInTheDocument();
+ expect(screen.queryByText('Test Chart 2')).not.toBeInTheDocument();
+ });
+});
+
+test('shows No items when search has no results', async () => {
+ setupTest();
+
+ await waitFor(() => {
+ expect(screen.getByText('Test Chart 1')).toBeInTheDocument();
+ });
+
+ const searchInput = screen.getByPlaceholderText(
+ 'Search charts by name, owner, or dashboard',
+ );
+
+ await userEvent.type(searchInput, 'nonexistent chart');
+
+ await waitFor(() => {
+ expect(screen.queryByText('Test Chart 1')).not.toBeInTheDocument();
+ expect(screen.queryByText('Test Chart 2')).not.toBeInTheDocument();
+ expect(screen.getByText('No items')).toBeInTheDocument();
+ });
+});
+
+test('updates pagination total when filtering', async () => {
+ setupTest();
+
+ await waitFor(() => {
+ expect(screen.getByText('Test Chart 1')).toBeInTheDocument();
+ });
+
+ const searchInput = screen.getByPlaceholderText(
+ 'Search charts by name, owner, or dashboard',
+ );
+
+ await userEvent.type(searchInput, 'Chart 1');
+
+ // When searching, pagination should reflect filtered count
+ await waitFor(() => {
+ const paginationText = screen.getByText(/1 \/ 1/);
+ expect(paginationText).toBeInTheDocument();
+ });
+
+ await userEvent.clear(searchInput);
+
+ // When not searching, pagination should show total count
+ await waitFor(() => {
+ const paginationText = screen.getByText(/1 \/ 1/);
+ expect(paginationText).toBeInTheDocument();
+ });
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inadequate Test Assertion</b></div>
<div id="fix">
The 'updates pagination total when filtering' test checks for '1 / 1' in
both filtered and unfiltered states, but with only 2 mock charts, the
pagination display remains identical since both totals are below the 25-item
page size. This means the test passes without verifying that pagination
actually updates to reflect the filtered count, undermining its purpose of
testing the behavior logic.
</div>
</div>
<small><i>Code Review Run #59bf11</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx:
##########
@@ -1980,12 +2023,29 @@ class DatasourceEditor extends PureComponent {
),
children: (
<StyledTableTabWrapper>
- {this.renderDefaultColumnSettings()}
- <DefaultColumnSettingsTitle>
- {t('Column Settings')}
- </DefaultColumnSettingsTitle>
+ <Input.Search
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Removed dataset settings UI</b></div>
<div id="fix">
The removal of {this.renderDefaultColumnSettings()} eliminates the UI for
setting default datetime column and currency code column, which are
dataset-level configurations that apply to all columns including calculated
ones. This appears to be misplaced removal, as the function uses both database
and calculated columns. Consider restoring this in the SETTINGS tab.
</div>
</div>
<small><i>Code Review Run #59bf11</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/extractExtraMetrics.ts:
##########
@@ -26,16 +26,21 @@ import {
export function extractExtraMetrics(
formData: QueryFormData,
): QueryFormMetric[] {
- const { groupby, timeseries_limit_metric, x_axis_sort, metrics } = formData;
+ const { groupby, timeseries_limit_metric, metrics } = formData;
const extra_metrics: QueryFormMetric[] = [];
const limitMetric = ensureIsArray(timeseries_limit_metric)[0];
- if (
- !(groupby || []).length &&
- limitMetric &&
- getMetricLabel(limitMetric) === x_axis_sort &&
- !metrics?.some(metric => getMetricLabel(metric) === x_axis_sort)
- ) {
- extra_metrics.push(limitMetric);
+
+ if (!(groupby || []).length && limitMetric) {
+ const limitMetricLabel = getMetricLabel(limitMetric);
+ const isLimitMetricInMetrics = metrics?.some(
+ metric => getMetricLabel(metric) === limitMetricLabel,
+ );
+
+ // Add limit metric as extra if it's not already in display metrics
+ // This ensures it's fetched for sorting but not displayed as a separate
series
+ if (!isLimitMetricInMetrics) {
+ extra_metrics.push(limitMetric);
+ }
}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Logic Error: Incorrect Extra Metrics Extraction</b></div>
<div id="fix">
The change removes the check for `x_axis_sort` matching `limitMetric`, which
alters the function's behavior. Previously, `limitMetric` was only added as an
extra metric if it was the sort field and not in display metrics. Now, it's
added whenever `limitMetric` exists and isn't in metrics, regardless of sorting
needs. This will cause incorrect extra metrics to be fetched, potentially
affecting chart performance and data accuracy. The existing tests confirm this
logic was intentional.
</div>
</div>
<small><i>Code Review Run #59bf11</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
