bito-code-review[bot] commented on code in PR #37141:
URL: https://github.com/apache/superset/pull/37141#discussion_r3708339632
##########
superset-frontend/src/dashboard/components/DashboardBuilder/state.ts:
##########
@@ -121,5 +121,6 @@ export const useNativeFilters = () => {
dashboardFiltersOpen,
toggleDashboardFiltersOpen,
nativeFiltersEnabled,
+ hasFilters: filterValues.length > 0 || chartCustomizations.length > 0,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing return type annotation</b></div>
<div id="fix">
The `useNativeFilters` hook lacks an explicit return type annotation. Adding
the new `hasFilters` property without typing the return object risks contract
drift as the hook evolves. Define the return shape (e.g., via an interface) to
ensure type safety across all consumers like `DashboardBuilder.tsx:482`.
</div>
</div>
<small><i>Code Review Run #d266f0</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/gridComponents/ChartHolder/ChartHolder.tsx:
##########
@@ -182,11 +197,29 @@ const ChartHolder = ({
}, [
component,
getComponentById,
+ isMobile,
+ editMode,
parentComponent.meta.width,
parentComponent.parents,
parentComponent.type,
]);
+ // Grid units of height for this chart. In mobile consumption mode the
+ // authored desktop height is capped to the viewport (minus app chrome) so
+ // tall charts don't dominate the single-column stacked layout. Used for
+ // both the ResizableContainer shell and the height handed to the plugin,
+ // so the two can't disagree.
+ const heightMultiple = useMemo(() => {
+ const authoredHeight = component.meta.height ?? GRID_MIN_ROW_UNITS;
+ if (isMobile && !editMode) {
+ const maxUnits = Math.floor(
+ (window.innerHeight - MOBILE_CHROME_HEIGHT) / GRID_BASE_UNIT,
+ );
+ return Math.max(GRID_MIN_ROW_UNITS, Math.min(authoredHeight, maxUnits));
+ }
+ return authoredHeight;
+ }, [component.meta.height, isMobile, editMode]);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>No tests for mobile height capping</b></div>
<div id="fix">
The new `heightMultiple` useMemo (lines 212–221) adds mobile
consumption-mode height capping with no corresponding tests. The diff adds
significant layout logic (clamping authored height to viewport, floor at
`GRID_MIN_ROW_UNITS`, ceiling at `maxUnits`) that is critical for mobile UX but
cannot be validated by the existing test suite. Adaptive rule [6262] requires
that 'Tests should verify the actual business logic and behavior they claim to
test'.
</div>
</div>
<small><i>Code Review Run #d266f0</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:
##########
@@ -702,48 +770,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);
+ return;
+ }
+ if (info.key === 'logout' && navbarRight.user_logout_url) {
+ navigateTo(navbarRight.user_logout_url);
+ return;
+ }
+ setMobileMenuOpen(false);
+ }}
+ items={mobileMenuItems}
+ css={css`
+ border-inline-end: none !important;
+ `}
+ />
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing onOpenChange prop on mobile</b></div>
<div id="fix">
The mobile `Menu` at line 794 lacks `onOpenChange={onMenuOpen}` that the
desktop `Menu` has at line 860. Without this prop, opening Data submenus in the
mobile drawer won't trigger the `checkAllowUploads()` logic, potentially
bypassing upload-permission checks on mobile. Pass `onMenuOpen` to the mobile
`Menu` and adjust the conditional `setMobileMenuOpen(false)` logic accordingly.
</div>
</div>
<small><i>Code Review Run #d266f0</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]
