bito-code-review[bot] commented on code in PR #34785:
URL: https://github.com/apache/superset/pull/34785#discussion_r3651880145
##########
superset-frontend/src/components/Datasource/DatasourceModal/index.tsx:
##########
@@ -181,6 +182,7 @@ const DatasourceModal:
FunctionComponent<DatasourceModalProps> = ({
owners: datasource.owners.map(
(o: Record<string, number>) => o.value || o.id,
),
+ drill_through_chart_id: datasource.drill_through_chart_id || null,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Falsy number incorrectly coerced to null</b></div>
<div id="fix">
Using `|| null` incorrectly converts falsy numbers like `0` to `null`. Since
chart IDs can be `0`, use `?? null` instead to only fall back to `null` for
`null`/`undefined`. This matches the type definition `number | null`.
</div>
</div>
<small><i>Code Review Run #1064ad</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/utils/cachedSupersetGet.ts:
##########
@@ -27,3 +27,20 @@ export const cachedSupersetGet = cacheWrapper(
supersetGetCache,
({ endpoint }) => endpoint || '',
);
+
+/**
+ * Invalidates cached dataset drill info for all dashboards containing this
dataset
+ */
+export const invalidateDatasetDrillCache = (datasetId: string | number) => {
+ const numericDatasetId =
+ typeof datasetId === 'string'
+ ? Number(datasetId.split('__')[0])
+ : Number(datasetId);
+
+ // Find and delete all cache entries for this dataset's drill info
+ const keysToDelete = Array.from(supersetGetCache.keys()).filter(key =>
+ key.includes(`/api/v1/dataset/${numericDatasetId}/drill_info/`),
+ );
+
+ keysToDelete.forEach(key => supersetGetCache.delete(key));
+};
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for new function</b></div>
<div id="fix">
The new `invalidateDatasetDrillCache` function has no unit tests. Per
project guidelines, new tools/features should have comprehensive unit tests
covering success paths, error scenarios, and edge cases to ensure robust error
handling and prevent regressions.
</div>
</div>
<small><i>Code Review Run #1064ad</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/Chart/DrillDetail/DrillDetailModal.test.tsx:
##########
@@ -32,6 +32,14 @@ jest.mock('react-router-dom', () => ({
}),
}));
+jest.mock('src/explore/exploreUtils', () => ({
+ ...jest.requireActual('src/explore/exploreUtils'),
+ getExploreUrl: jest.fn(
+ ({ formData }) =>
+ `/explore/?dashboard_page_id=&slice_id=${formData.slice_id}`,
+ ),
+}));
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Mock targets unused function</b></div>
<div id="fix">
The test currently mocks `getExploreUrl`, but the component uses
`generateExploreUrl` imported from 'src/explore/exploreUtils/formData'. Update
the mock to target `generateExploreUrl` in that module (or mock `postFormData`
to avoid real HTTP requests) to ensure the URL generation is stubbed during
tests.
</div>
</div>
<small><i>Code Review Run #1064ad</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/features/datasets/AddDataset/Footer/Footer.test.tsx:
##########
@@ -119,9 +119,9 @@ describe('Footer', () => {
const dropdownTrigger = screen.getByRole('button', { name: 'down' });
userEvent.click(dropdownTrigger);
- // Check that the dropdown menu option is visible
+ // Check that the dropdown menu option is in the document
await waitFor(() => {
- expect(screen.getByText('Create dataset')).toBeVisible();
+ expect(screen.getByText('Create dataset')).toBeInTheDocument();
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test assertion weakened</b></div>
<div id="fix">
This change weakens the assertion. `toBeVisible()` verifies the dropdown is
both in the DOM and actually rendered on screen, while `toBeInTheDocument()`
only checks DOM presence. If a CSS issue or animation prevents the dropdown
from being visible, `toBeInTheDocument()` would still pass but the feature
would be broken.
</div>
</div>
<small><i>Code Review Run #1064ad</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/Select/ChartSelect.test.tsx:
##########
@@ -0,0 +1,115 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { render } from 'spec/helpers/testing-library';
+import ChartSelectUsingAsync from './ChartSelect';
+
+const mockOnChange = jest.fn();
+
+const defaultProps = {
+ value: null,
+ onChange: mockOnChange,
+ datasetId: 123,
+ placeholder: 'Select a chart',
+ allowClear: true,
+ ariaLabel: 'Select drill-to-details chart',
+};
+
+test('renders chart select component with default props', () => {
+ const { container } = render(<ChartSelectUsingAsync {...defaultProps} />, {
+ useRedux: true,
+ });
+ expect(container.firstChild).toBeInTheDocument();
+});
+
+test('renders with custom placeholder', () => {
+ const customProps = {
+ ...defaultProps,
+ placeholder: 'Choose your chart',
+ };
+
+ const { container } = render(<ChartSelectUsingAsync {...customProps} />, {
+ useRedux: true,
+ });
+ expect(container.firstChild).toBeInTheDocument();
+});
+
+test('renders with selected value', () => {
+ const propsWithValue = {
+ ...defaultProps,
+ value: 456,
+ };
+
+ const { container } = render(<ChartSelectUsingAsync {...propsWithValue} />, {
+ useRedux: true,
+ });
+ expect(container.firstChild).toBeInTheDocument();
+});
+
+test('renders without dataset filter when datasetId is undefined', () => {
+ const propsWithoutDataset = {
+ ...defaultProps,
+ datasetId: undefined,
+ };
+
+ const { container } = render(
+ <ChartSelectUsingAsync {...propsWithoutDataset} />,
+ { useRedux: true },
+ );
+ expect(container.firstChild).toBeInTheDocument();
+});
+
+test('renders with custom aria label', () => {
+ const propsWithCustomAriaLabel = {
+ ...defaultProps,
+ ariaLabel: 'Custom chart selector',
+ };
+
+ const { container } = render(
+ <ChartSelectUsingAsync {...propsWithCustomAriaLabel} />,
+ { useRedux: true },
+ );
+ expect(container.firstChild).toBeInTheDocument();
+});
+
+test('renders as non-clearable when allowClear is false', () => {
+ const nonClearableProps = {
+ ...defaultProps,
+ allowClear: false,
+ };
+
+ const { container } = render(
+ <ChartSelectUsingAsync {...nonClearableProps} />,
+ { useRedux: true },
+ );
+ expect(container.firstChild).toBeInTheDocument();
+});
+
+test('passes through additional props to SelectAsyncControl', () => {
+ const propsWithExtra = {
+ ...defaultProps,
+ description: 'Test description',
+ hovered: true,
+ 'data-testid': 'chart-select',
+ };
+
+ const { container } = render(<ChartSelectUsingAsync {...propsWithExtra} />, {
+ useRedux: true,
+ });
+ expect(container.firstChild).toBeInTheDocument();
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Shallow tests skip core behavior</b></div>
<div id="fix">
These tests currently only assert that the component renders. To cover core
behavior, add tests that:
1) mock the `/api/v1/chart/` endpoint and verify that selecting an option
triggers `onChange` with the correct id,
2) capture the query parameters when `datasetId` is provided vs undefined,
3) check that additional props (like `description` and `data-testid`) are
passed through to the underlying `SelectAsyncControl`.
</div>
</div>
<small><i>Code Review Run #1064ad</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/migrations/versions/2025-08-21_01-54_852e99567fe7_.py:
##########
@@ -0,0 +1,35 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""merge heads: theme system and drill-through chart
+
+Revision ID: 852e99567fe7
+Revises: ('c233f5365c9e', 'f56ac3accfc9')
+Create Date: 2025-08-21 01:54:38.918459
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "852e99567fe7"
+down_revision = ("c233f5365c9e", "f56ac3accfc9")
+
+
+def upgrade():
+ pass
+
+
+def downgrade():
+ pass
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Merge revision missing migration logic</b></div>
<div id="fix">
The `upgrade()` and `downgrade()` functions are empty stubs (`pass`). This
merge revision must incorporate the actual schema changes from both parent
migrations — `c233f5365c9e` (theme system columns + indexes) and `f56ac3accfc9`
(drill-through chart FK). Without the migration logic, applying this revision
will leave the database incomplete, causing runtime errors when the application
tries to use `themes.is_system_default`, `themes.is_system_dark`, or
`tables.drill_through_chart_id`.
</div>
</div>
<small><i>Code Review Run #1064ad</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/migrations/versions/2025-08-21_01-54_852e99567fe7_.py:
##########
@@ -0,0 +1,35 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Invalid module name with trailing underscore</b></div>
<div id="fix">
The filename contains a trailing underscore which violates module naming
conventions. Consider renaming the file to remove the trailing underscore.
</div>
</div>
<small><i>Code Review Run #1064ad</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]
