bito-code-review[bot] commented on code in PR #37141:
URL: https://github.com/apache/superset/pull/37141#discussion_r2692186356
##########
superset-frontend/src/components/ListView/ListView.test.tsx:
##########
@@ -359,3 +359,84 @@ describe('ListView', () => {
expect(mockedPropsComprehensive.fetchData).toHaveBeenCalled();
});
});
+
+// Mobile support tests
+test('respects forceViewMode prop and hides view toggle', () => {
+ // Omit cardSortSelectOptions to avoid CardSortSelect needing initialSort
+ const { cardSortSelectOptions, ...propsWithoutSort } =
mockedPropsComprehensive;
+ render(
+ <QueryParamProvider location={makeMockLocation()}>
+ <ListView
+ {...propsWithoutSort}
+ renderCard={() => <div>Card</div>}
+ forceViewMode="card"
+ />
+ </QueryParamProvider>,
+ { store: mockStore() },
+ );
+
+ // View toggle should not be present when forceViewMode is set
+ expect(screen.queryByLabelText('card-view')).not.toBeInTheDocument();
+ expect(screen.queryByLabelText('list-view')).not.toBeInTheDocument();
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test expects hidden toggle</b></div>
<div id="fix">
The test verifies that the view toggle is hidden when forceViewMode is set,
but the current ListView implementation renders the toggle regardless of this
prop. This mismatch will cause the test to fail. The toggle should not be shown
when the view mode is forced to avoid user confusion.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
- {cardViewEnabled && (
- <ViewModeToggle mode={viewMode} setMode={setViewMode} />
- )}
+ {cardViewEnabled && !forceViewMode && (
+ <ViewModeToggle mode={viewMode} setMode={setViewMode} />
+ )}
```
</div>
</details>
</div>
<small><i>Code Review Run #9148bb</i></small>
</div><div>
<div id="suggestion">
<div id="issue"><b>Misleading aria-label test selectors</b></div>
<div id="fix">
The ViewModeToggle component renders buttons with `aria-pressed` but no
`aria-label`. Tests use `queryByLabelText('card-view')` and
`queryByLabelText('list-view')` which will find no elements, causing this
assertion to always pass vacuously. Either use `queryAllByRole('button')` and
filter by `aria-pressed` attribute, or add `aria-label` props to the toggle
buttons in `ViewModeToggle`.
</div>
</div>
<small><i>Code Review Run #3c89a6</i></small>
</div><div>
<div id="suggestion">
<div id="issue"><b>Invalid test selector for view toggle</b></div>
<div id="fix">
Test selectors at lines 403-404 use `queryByLabelText('card-view')` and
`queryByLabelText('list-view')`, but the toggle buttons in ListView.tsx (lines
289-312) have no aria-label attributes. They only have `aria-pressed` for state
and tooltip titles. The tests will return null instead of the expected
elements. Use `queryByRole('button', { pressed: true })` or add explicit
testids/aria-labels to the buttons.
</div>
</div>
<small><i>Code Review Run #ee4452</i></small>
</div><div>
<div id="suggestion">
<div id="issue"><b>Missing aria-label for toggle buttons</b></div>
<div id="fix">
Test queries for `aria-label='card-view'` and `aria-label='list-view'` but
the ViewModeToggle component (ListView.tsx:288-313) only sets `aria-pressed`,
not `aria-label`. These queries will always return null.
</div>
</div>
<small><i>Code Review Run #959314</i></small>
</div><div>
<div id="suggestion">
<div id="issue"><b>Broken aria-label queries in view toggle tests</b></div>
<div id="fix">
The `getByLabelText('card-view')` and `getByLabelText('list-view')` queries
will always return `null`. The ViewModeToggle buttons (ListView.tsx:291, 304)
use `aria-pressed` but carry no `aria-label` or visible text — so no element in
the DOM can ever match these label queries. The assertions at lines 403 and 404
are dead code: they pass vacuously (null ≠ inTheDocument is always true) and
verify nothing about the `forceViewMode` prop's effect on the view toggle.
</div>
</div>
<small><i>Code Review Run #5b6916</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/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx:
##########
@@ -357,11 +416,15 @@ export const useHeaderActionsMenu = ({
expandedSlices,
handleMenuClick,
isLoading,
+ isMobile,
+ isPublished,
+ isStarred,
lastModifiedTime,
layout,
onSave,
refreshFrequency,
reportMenuItem,
+ saveFaveStar,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead dependency in useMemo array</b></div>
<div id="fix">
`saveFaveStar` is listed in the useMemo dependency array but is never used
inside the callback body — it is only referenced in `handleMenuClick` which has
its own separate dependency array. This dead dependency could cause unnecessary
re-computations when `saveFaveStar` changes.
</div>
</div>
<small><i>Code Review Run #5b6916</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/pages/DashboardList/index.tsx:
##########
@@ -177,6 +179,9 @@ const DASHBOARD_COLUMNS_TO_FETCH = [
function DashboardList(props: DashboardListProps) {
const { addDangerToast, addSuccessToast, user } = props;
+ const isNotMobile = !useIsMobile();
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Misleading name hides boolean meaning</b></div>
<div id="fix">
The variable `isNotMobile` is a double-negative (`!useIsMobile()`). On
mobile `useIsMobile()` returns `true`; after negation `isNotMobile` is `false`.
This naming requires all downstream consumers to think in negatives (e.g.,
`!isNotMobile` to check mobile). Rename to `isMobile` with value
`useIsMobile()` for clarity, then flip all conditional usages (e.g.,
`!isNotMobile ? X : undefined` → `isMobile ? X : undefined`).
</div>
</div>
<small><i>Code Review Run #5b6916</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/pages/Home/Home.mobile.test.tsx:
##########
@@ -0,0 +1,179 @@
+/**
+ * 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.
+ */
+
+/**
+ * Mobile-specific tests for the Home/Welcome page.
+ *
+ * These tests verify that certain desktop-only sections are hidden
+ * on mobile viewports.
+ */
+
+import fetchMock from 'fetch-mock';
+import { render, screen, waitFor } from 'spec/helpers/testing-library';
+import Welcome from 'src/pages/Home';
+
+import { mockMobileMatchMedia } from 'spec/helpers/mobileTestUtils';
+
+// Enable only the mobile consumption mode flag so the mobile branch renders
+jest.mock('@superset-ui/core', () => ({
+ ...jest.requireActual('@superset-ui/core'),
+ isFeatureEnabled: jest.fn(
+ (flag: string) => flag === 'MOBILE_CONSUMPTION_MODE',
+ ),
+}));
+
+// Simulate a mobile viewport for the useIsMobile hook
+mockMobileMatchMedia();
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Resource leak - mock not cleaned up</b></div>
<div id="fix">
The `mockMobileMatchMedia()` function returns a cleanup function that
restores `window.matchMedia`, but it is not captured or called. This leaks the
mock into any test that runs after this file, potentially causing unrelated
tests to incorrectly detect a mobile viewport.
</div>
</div>
<small><i>Code Review Run #5b6916</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]
