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>
---
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:
##########
@@ -516,6 +582,32 @@ 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
+ filters={filters}
+ internalFilters={internalFilters}
+ updateFilterValue={applyFilterValue}
+ />
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Mobile filter clear broken</b></div>
<div id="fix">
The FilterControls in the mobile drawer lacks a ref, so users cannot clear
filters when using the mobile drawer. This breaks the clear functionality on
mobile, as the clearFilters method won't reset the filter UI state in the
drawer.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
<FilterControls
ref={filterControlsRef}
filters={filters}
internalFilters={internalFilters}
updateFilterValue={applyFilterValue}
/>
````
</div>
</details>
</div>
<small><i>Code Review Run #9148bb</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:
##########
@@ -516,6 +582,32 @@ 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
+ filters={filters}
+ internalFilters={internalFilters}
+ updateFilterValue={applyFilterValue}
+ />
+ {cardSortSelectOptions && (
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inconsistent viewMode check for sort controls</b></div>
<div id="fix">
The CardSortSelect in the mobile filter drawer appears without checking
viewMode, unlike the desktop version where it's conditional on viewMode ===
'card'. This could show sort controls in table view on mobile, which may
confuse users since sorting is handled differently there.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
{viewMode === 'card' && cardSortSelectOptions && (
````
</div>
</details>
</div>
<small><i>Code Review Run #9148bb</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]
