bito-code-review[bot] commented on code in PR #37141:
URL: https://github.com/apache/superset/pull/37141#discussion_r3642708012
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.tsx:
##########
@@ -110,6 +111,22 @@ const StyledDiv = styled.div`
i.warning {
color: ${theme.colorWarning};
}
+
+ /* Mobile consumption mode: show the full chart title without
+ truncation (controls and links are render-gated in SliceHeader) */
+ ${
+ isMobileConsumptionEnabled()
+ ? `@media (max-width: ${theme.screenSMMax}px) {
+ [data-test='slice-header'] .header-title {
+ -webkit-line-clamp: unset;
+ display: block;
+ white-space: normal;
+ overflow: visible;
+ text-overflow: unset;
+ }
+ }`
+ : ''
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test coverage for feature</b></div>
<div id="fix">
The added CSS block (lines 115-129) correctly applies mobile title styling
when `isMobileConsumptionEnabled()` returns true, but the existing test suite
lacks coverage for this feature-flagged behavior. BITO.md rule [6262] requires
tests to verify actual business logic, not just component rendering. The
current `DashboardWrapper.test.tsx` only validates that children render,
leaving the mobile consumption mode styling untested.
</div>
</div>
<small><i>Code Review Run #3c89a6</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/dashboards/DashboardCard.tsx:
##########
@@ -212,10 +214,12 @@ function DashboardCard({
isStarred={favoriteStatus}
/>
)}
- <KebabMenuButton
- menuItems={menuItems}
- dataTest="dashboard-card-menu"
- />
+ {!isMobile && (
+ <KebabMenuButton
+ menuItems={menuItems}
+ dataTest="dashboard-card-menu"
+ />
+ )}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing mobile behavior tests</b></div>
<div id="fix">
The diff introduces conditional rendering of `KebabMenuButton` based on
mobile state (line 217-222), but the existing test suite
(DashboardCard.test.tsx) has no coverage for this behavior. Per BITO.md
adaptive rule [6262], tests should verify actual business logic. Without tests,
regression of this mobile-specific behavior could go undetected.
</div>
</div>
<small><i>Code Review Run #3c89a6</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/hooks/useIsMobile.ts:
##########
@@ -0,0 +1,71 @@
+/**
+ * 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 { useEffect, useState } from 'react';
+import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core';
+import { useTheme } from '@apache-superset/core/theme';
+
+// Matches antd's screenSMMax token; used only when no theme is in scope.
+const FALLBACK_MOBILE_MAX_WIDTH = 767;
+
+/**
+ * Whether the mobile consumption-only experience is enabled for this
+ * deployment. Non-hook variant for use inside styled-component
+ * interpolations; prefer `useIsMobile` in components.
+ */
+export function isMobileConsumptionEnabled(): boolean {
+ return isFeatureEnabled(FeatureFlag.MobileConsumptionMode);
+}
+
+/**
+ * Returns true when MOBILE_CONSUMPTION_MODE is enabled AND the viewport is
+ * at or below the theme's `screenSMMax` breakpoint. All mobile-specific
+ * behavior (route guarding, consumption-only chrome, drawer navigation)
+ * should key off this hook so the flag remains a single kill switch.
+ *
+ * The matchMedia subscription is only installed when the flag is on, and
+ * state only changes when the match flips, so with the flag off (or on
+ * desktop) this hook never causes a re-render — consumers are inert.
+ *
+ * The initial value is always false (desktop), so the first paint never
+ * takes the mobile branch by accident.
+ */
+export function useIsMobile(): boolean {
+ const enabled = isMobileConsumptionEnabled();
+ const theme = useTheme();
+ const maxWidth = theme?.screenSMMax ?? FALLBACK_MOBILE_MAX_WIDTH;
+ const [isSmallScreen, setIsSmallScreen] = useState(false);
+
+ useEffect(() => {
+ if (!enabled) {
+ return undefined;
+ }
+ const mediaQuery = window.matchMedia(`(max-width: ${maxWidth}px)`);
+ const update = () => setIsSmallScreen(mediaQuery.matches);
+ update();
+ // Safari < 14 lacks addEventListener on MediaQueryList
+ if (mediaQuery.addEventListener) {
+ mediaQuery.addEventListener('change', update);
+ return () => mediaQuery.removeEventListener('change', update);
+ }
+ mediaQuery.addListener(update);
+ return () => mediaQuery.removeListener(update);
+ }, [enabled, maxWidth]);
+
+ return enabled && isSmallScreen;
+}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for new hook</b></div>
<div id="fix">
Add unit tests for this new hook per project guidelines. The hook handles
stateful viewport tracking with a feature flag kill switch and theme-dependent
breakpoint, so tests should cover all execution paths: flag enabled/disabled,
media query match/not-match, theme changes mid-lifecycle, and the inert initial
state guarantee.
</div>
</div>
<small><i>Code Review Run #3c89a6</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/DashboardList.test.tsx:
##########
@@ -50,6 +50,15 @@ jest.mock('src/utils/getBootstrapData', () =>
mockUserSubjectsBootstrapData([1]),
);
+// Mock useBreakpoint to return desktop breakpoints (prevents mobile rendering)
+jest.mock('antd', () => ({
+ ...jest.requireActual('antd'),
+ Grid: {
+ ...jest.requireActual('antd').Grid,
+ useBreakpoint: () => ({ xs: true, sm: true, md: true, lg: true, xl: true
}),
+ },
+}));
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Semantic duplication of useBreakpoint mock</b></div>
<div id="fix">
The `useBreakpoint` mock (lines 54-60) duplicates an identical pattern found
in 4 other test files (`Home.test.tsx`, `Header.test.tsx`,
`RightMenu.test.tsx`, `DashboardBuilder.test.tsx`). The repository provides
`mockAntdWithDesktopBreakpoint()` in `spec/helpers/mobileTestUtils` as the
canonical solution, which also includes `xxl: true` for completeness.
Consolidating via the utility reduces maintenance burden and ensures
consistency.
</div>
</div>
<small><i>Code Review Run #3c89a6</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.test.tsx:
##########
@@ -146,6 +146,15 @@ jest.mock('@superset-ui/core', () => ({
isFeatureEnabled: jest.fn(),
}));
+// Mock useBreakpoint to return desktop breakpoints (prevents mobile rendering)
+jest.mock('antd', () => ({
+ ...jest.requireActual('antd'),
+ Grid: {
+ ...jest.requireActual('antd').Grid,
+ useBreakpoint: () => ({ xs: true, sm: true, md: true, lg: true, xl: true
}),
+ },
+}));
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Use shared mobile test utility</b></div>
<div id="fix">
This mock duplicates logic from `spec/helpers/mobileTestUtils.ts` which
provides `mockAntdWithDesktopBreakpoint()` with full documentation, consistent
breakpoint values, and community-reviewed edge-case handling. Using the shared
utility improves maintainability and ensures consistent behavior across the
test suite.
</div>
</div>
<small><i>Code Review Run #3c89a6</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/home/RightMenu.tsx:
##########
@@ -704,48 +772,98 @@ const RightMenu = ({
</Tag>
);
})()}
- <Menu
- css={css`
- display: flex;
- flex-direction: row;
- align-items: center;
- height: 100%;
- border-bottom: none !important;
-
- /* Remove the underline from menu items */
- .ant-menu-item:after,
- .ant-menu-submenu:after {
- content: none !important;
- }
-
- .submenu-with-caret {
+ {/* Mobile: hamburger menu with drawer */}
+ {isMobile && (
+ <>
+ <Button
+ buttonStyle="link"
+ onClick={() => setMobileMenuOpen(true)}
+ aria-label={t('Menu')}
+ >
+ <Icons.MenuOutlined iconSize="l" />
+ </Button>
+ <Drawer
+ title={null}
+ placement="right"
+ onClose={() => setMobileMenuOpen(false)}
+ open={mobileMenuOpen}
+ width={280}
+ styles={{
+ header: { display: 'none' },
+ body: { padding: 0 },
+ }}
+ >
+ <Menu
+ mode="inline"
+ selectable={false}
+ onClick={info => {
+ handleMenuSelection(info);
+ // The reused desktop items navigate via anchors that only
+ // span their label text, but the drawer's tap target is the
+ // full menu row — navigate explicitly so row taps work.
+ if (info.key === 'info' && navbarRight.user_info_url) {
+ navigateTo(navbarRight.user_info_url);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing mobile menu test coverage</b></div>
<div id="fix">
Mobile menu click handler for 'info' key lacks test coverage. Rule 6262
requires tests to verify actual business logic behavior — specifically that
tapping Info on mobile triggers navigateTo(navbarRight.user_info_url).
Currently no test validates this mobile-specific navigation flow.
</div>
</div>
<small><i>Code Review Run #3c89a6</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/DashboardBuilder/DashboardBuilder.test.tsx:
##########
@@ -719,3 +734,85 @@ test('should maintain layout when switching between tabs',
async () => {
expect(gridContainer).toBeInTheDocument();
expect(tabPanels.length).toBeGreaterThan(0);
});
+
+// Mobile support tests
+// Note: The main mobile tests require mocking useBreakpoint to return mobile
breakpoints
+// which is done at the module level. These tests verify mobile-related
component behavior.
+
+test('should not render filter bar panel on desktop when nativeFiltersEnabled
is false', () => {
+ (useStoredSidebarWidth as jest.Mock).mockImplementation(() => [
+ 100,
+ jest.fn(),
+ ]);
+ (fetchFaveStar as jest.Mock).mockReturnValue({ type: 'mock-action' });
+ (setActiveTab as jest.Mock).mockReturnValue({ type: 'mock-action' });
+
+ jest.spyOn(useNativeFiltersModule, 'useNativeFilters').mockReturnValue({
+ showDashboard: true,
+ missingInitialFilters: [],
+ dashboardFiltersOpen: true,
+ toggleDashboardFiltersOpen: jest.fn(),
+ nativeFiltersEnabled: false,
+ hasFilters: false,
+ });
+
+ const { queryByTestId } = render(<DashboardBuilder />, {
+ useRedux: true,
+ store: storeWithState({
+ ...mockState,
+ dashboardLayout: undoableDashboardLayout,
+ }),
+ useDnd: true,
+ useTheme: true,
+ useRouter: true,
+ });
+
+ // Filter panel should not be present when native filters are disabled
+ expect(queryByTestId('dashboard-filters-panel')).not.toBeInTheDocument();
+});
+
+test('should render dashboard content wrapper', () => {
+ (useStoredSidebarWidth as jest.Mock).mockImplementation(() => [
+ 100,
+ jest.fn(),
+ ]);
+ (fetchFaveStar as jest.Mock).mockReturnValue({ type: 'mock-action' });
+ (setActiveTab as jest.Mock).mockReturnValue({ type: 'mock-action' });
+
+ const { getByTestId } = render(<DashboardBuilder />, {
+ useRedux: true,
+ store: storeWithState({
+ ...mockState,
+ dashboardLayout: undoableDashboardLayout,
+ }),
+ useDnd: true,
+ useTheme: true,
+ useRouter: true,
+ });
+
+ // Dashboard content wrapper should always be present
+ expect(getByTestId('dashboard-content-wrapper')).toBeInTheDocument();
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Semantically duplicated test case</b></div>
<div id="fix">
This test is semantically duplicated within the diff — existing test at line
166 asserts the same 'dashboard-content-wrapper' element exists. Test 2 adds no
new coverage, only maintenance divergence risk.
</div>
</div>
<small><i>Code Review Run #3c89a6</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/initialization/__init__.py:
##########
@@ -1063,6 +1063,7 @@ def enforce_session_validity() -> Any:
@self.superset_app.context_processor
def get_common_bootstrap_data() -> dict[str, Any]:
# Import here to avoid circular imports
+ from superset.extensions import feature_flag_manager
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Redundant inline import</b></div>
<div id="fix">
The inline import of `feature_flag_manager` is unnecessary — it's already
imported at module level and can be referenced directly in the return statement
at line 1078.
</div>
</div>
<small><i>Code Review Run #3c89a6</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/ListView/ListView.tsx:
##########
@@ -659,6 +702,40 @@ export function ListView<T extends object = any>({
)}
</div>
</div>
+
+ {/* Mobile filter drawer */}
+ {filterable && setMobileFiltersOpen && (
+ <Drawer
+ title={mobileFiltersDrawerTitle || t('Search')}
+ placement="left"
+ onClose={() => setMobileFiltersOpen(false)}
+ open={mobileFiltersOpen}
+ width={300}
+ >
+ <MobileFilterDrawerContent>
+ <FilterControls
+ ref={filterControlsRef}
+ filters={filters}
+ internalFilters={internalFilters}
+ updateFilterValue={applyFilterValue}
+ />
+ {viewMode === 'card' && cardSortSelectOptions && (
+ <CardSortSelect
+ initialSort={sortBy}
+ onChange={(value: SortColumn[]) => setSortBy(value)}
+ options={cardSortSelectOptions}
+ />
+ )}
+ <ClearAllButton
+ type="button"
+ disabled={!hasActiveFilters}
+ onClick={() => filterControlsRef.current?.clearFilters()}
+ >
+ {t('Clear all')}
+ </ClearAllButton>
+ </MobileFilterDrawerContent>
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inconsistent Clear All tooltip</b></div>
<div id="fix">
The drawer ClearAllButton at line 729 lacks the Tooltip that the inline
version has (lines 518-531). When no filters are active, the drawer button
should show the same 'No filters applied' tooltip as the inline version for
consistent UX. The same `hasActiveFilters` state is available in both render
paths.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
options={cardSortSelectOptions}
/>
)}
<Tooltip
title={!hasActiveFilters ? t('No filters applied') : undefined}
>
<span>
<ClearAllButton
type="button"
disabled={!hasActiveFilters}
onClick={() => filterControlsRef.current?.clearFilters()}
>
{t('Clear all')}
</ClearAllButton>
</span>
</Tooltip>
</MobileFilterDrawerContent>
````
</div>
</details>
</div>
<small><i>Code Review Run #3c89a6</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/playwright/tests/mobile/mobile-dashboard.spec.ts:
##########
@@ -0,0 +1,325 @@
+/**
+ * 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 { test, expect, devices } from '@playwright/test';
+
+// NOTE: These tests exercise the mobile consumption experience and require
+// the MOBILE_CONSUMPTION_MODE feature flag to be enabled in the target
+// environment (FEATURE_FLAGS = {"MOBILE_CONSUMPTION_MODE": True}).
+import { TIMEOUT } from '../../utils/constants';
+
+/**
+ * Mobile dashboard viewing tests verify that dashboards can be viewed
+ * and interacted with on mobile devices.
+ *
+ * These tests assume the World Bank's Health sample dashboard exists.
+ */
+
+// Use iPhone 12 viewport for mobile tests
+const mobileViewport = devices['iPhone 12'];
+
+test.describe('Mobile Dashboard Viewing', () => {
+ test.use({
+ viewport: mobileViewport.viewport,
+ userAgent: mobileViewport.userAgent,
+ });
+
+ test.beforeEach(async ({ page }) => {
+ // Navigate to dashboard list to find a dashboard
+ await page.goto('dashboard/list/');
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inconsistent URL constant usage</b></div>
<div id="fix">
Hardcoded URL `'dashboard/list/'` on line 45 bypasses the `URL` constant
from `'../../utils/urls'`. Existing file `mobile-navigation.spec.ts` (line 70)
correctly uses `URL.DASHBOARD_LIST`. Inconsistent URL handling makes the
codebase harder to maintain and risks broken links if paths change.
</div>
</div>
<small><i>Code Review Run #3c89a6</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/home/RightMenu.tsx:
##########
@@ -644,6 +654,64 @@ const RightMenu = ({
handleLogout,
]);
+ // Build mobile menu items - consumption only (no create/admin actions)
+ const mobileMenuItems = useMemo(() => {
+ const items: MenuItem[] = [];
+
+ // Add Dashboards link at top (from main menu)
+ // Match on the FAB-internal `name`, which is stable across locales
+ // (`label` is translated and would break in non-English deployments)
+ const dashboardsMenu = menu?.find(item => item.name === 'Dashboards');
+ if (dashboardsMenu) {
+ const dashboardUrl = dashboardsMenu.url || '/dashboard/list/';
+ items.push({
+ key: 'dashboards',
+ label: isFrontendRoute(dashboardUrl) ? (
+ <Link to={dashboardUrl}>{t('Dashboards')}</Link>
+ ) : (
+ <Typography.Link href={dashboardUrl}>
+ {t('Dashboards')}
+ </Typography.Link>
+ ),
+ icon: <Icons.DashboardOutlined />,
+ });
+ }
+
+ // Add theme menu (flatten children directly)
+ menuItems.forEach(item => {
+ if (!item || !('key' in item)) return;
+
+ // Only include theme-sub-menu and language picker
+ if (item.key === 'theme-sub-menu' || item.key === 'language-picker') {
+ items.push({ type: 'divider', key: `divider-before-${item.key}` });
+
+ if ('children' in item && item.children) {
+ // Theme menu already has a nested group, so just add its children
directly
+ item.children.forEach(child => {
+ items.push(child);
+ });
+ } else {
+ items.push(item);
+ }
+ }
+
+ // Extract user-related items from settings
+ if (item.key === 'settings' && 'children' in item && item.children) {
+ item.children.forEach(child => {
+ if (!child || !('key' in child)) return;
+
+ // Only include user-section and about-section
+ if (child.key === 'user-section' || child.key === 'about-section') {
+ items.push({ type: 'divider', key: `divider-before-${child.key}`
});
+ items.push(child);
+ }
+ });
+ }
+ });
+
+ return items;
+ }, [menu, menuItems, isFrontendRoute]);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing mobile menu tests</b></div>
<div id="fix">
The `mobileMenuItems` useMemo (lines 658-713) has no corresponding test
coverage. Existing tests in RightMenu.test.tsx mock `useBreakpoint` for
desktop-only behavior but lack tests for the new mobile Drawer,
`mobileMenuOpen` state, `setMobileMenuOpen`, or the `mobileMenuItems`
construction logic including dashboard URL fallback, theme/language flattening,
and user/about extraction.
</div>
</div>
<small><i>Code Review Run #3c89a6</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/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>
---
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]
