bito-code-review[bot] commented on code in PR #44216:
URL: https://github.com/apache/superset/pull/44216#discussion_r4080114105
##########
superset-frontend/src/dashboard/util/isValidChild.ts:
##########
@@ -65,6 +66,7 @@ const parentMaxDepthLookup: Record<string, Record<string,
number>> = {
[CHART_TYPE]: depthOne,
[DYNAMIC_TYPE]: depthOne,
[MARKDOWN_TYPE]: depthOne,
+ [FILTER_TYPE]: depthOne,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing tests for FILTER entries</b></div>
<div id="fix">
The five new `FILTER_TYPE` entries (lines 69, 81, 93, 105, 114) change
drop-target behavior, but `isValidChild.test.ts` — whose comment says every
unique parent>child relationship is tested — contains no FILTER cases (verified
via grep). Add GRID>FILTER, ROW>FILTER, TAB>FILTER, COLUMN>FILTER and
FILTER-as-parent cases so regressions in these depth mappings are caught.
</div>
</div>
<small><i>Code Review Run #d0dfc3</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/nativeFilters/FilterBar/FilterControls/FilterValue.tsx:
##########
@@ -400,8 +401,9 @@ const FilterValue: FC<FilterValueProps> = ({
() => ({
filterBarOrientation: orientation,
isOverflowingFilterBar: overflow,
+ inCanvas: Boolean((filter as Filter & { inCanvas?: boolean })?.inCanvas),
}),
- [orientation, overflow],
+ [orientation, overflow, filter],
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Broad memo dep defeats stability</b></div>
<div id="fix">
`displaySettings` now depends on the whole `filter` object, whose identity
changes on every dataMask update (`FilterHolder.tsx` `filterWithDataMask`,
`FilterControls.tsx` `addDataMaskToCustomization` both build new objects).
`SuperChart` then receives a new `displaySettings` each interaction, rebuilding
`ChartProps` and re-running `transformProps`. Depending on the primitive
`(filter as Filter & { inCanvas?: boolean })?.inCanvas` keeps `inCanvas` fresh
while restoring memo stability.
</div>
</div>
<small><i>Code Review Run #d0dfc3</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/nativeFilters/FilterBar/FilterControls/FilterValue.tsx:
##########
@@ -400,8 +401,9 @@ const FilterValue: FC<FilterValueProps> = ({
() => ({
filterBarOrientation: orientation,
isOverflowingFilterBar: overflow,
+ inCanvas: Boolean((filter as Filter & { inCanvas?: boolean })?.inCanvas),
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Undeclared inCanvas via type cast</b></div>
<div id="fix">
`inCanvas` is declared on neither `Filter` nor `ChartCustomization`
(`packages/superset-ui-core/src/query/types/Dashboard.ts:69-112`), so it is
bolted on via `as` casts here and in `FilterHolder.tsx:126-127`, and the two
casts already diverge (`inCanvas: boolean` vs `inCanvas?: boolean`). Declaring
`inCanvas?: boolean` on `FilterElementWithDataMask` in
`FilterControls/types.ts` restores compiler checking and removes both
assertions.
</div>
</div>
<small><i>Code Review Run #d0dfc3</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/packages/superset-ui-core/src/components/PopoverDropdown/index.tsx:
##########
@@ -83,14 +84,15 @@ const PopoverDropdown = (props: PopoverDropdownProps) => {
renderOption = (option: OptionProps) => (
<div className={option.className}>{option.label}</div>
),
+ overlayStyle,
} = props;
const theme = useTheme();
const selected = options.find(opt => opt.value === value);
return (
<Dropdown
trigger={['click']}
- overlayStyle={{ zIndex: theme.zIndexBase }}
+ overlayStyle={overlayStyle}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dropped default zIndex</b></div>
<div id="fix">
The deleted line shows every consumer previously received `zIndex:
theme.zIndexBase`. Callers that do not pass the new prop (`Header.tsx`,
`MarkdownModeDropdown.tsx`, `BackgroundStyleDropdown.tsx`) now fall through to
antd's default popup z-index, silently changing overlay stacking on normal
renders. Consider preserving the theme-driven fallback:
`overlayStyle={overlayStyle ?? { zIndex: theme.zIndexBase }}`.
</div>
</div>
<small><i>Code Review Run #d0dfc3</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/FilterHolder/FilterHolder.tsx:
##########
@@ -0,0 +1,712 @@
+/**
+ * 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 { useCallback, useMemo, useState, ReactNode } from 'react';
+import cx from 'classnames';
+import { useDispatch, useSelector } from 'react-redux';
+import { ResizeCallback, ResizeStartCallback } from 're-resizable';
+import { css, useTheme } from '@apache-superset/core/theme';
+import { t } from '@apache-superset/core/translation';
+import { DataMask, Divider, Filter, isNativeFilter } from '@superset-ui/core';
+import { Button, Select } from '@superset-ui/core/components';
+import { Icons } from '@superset-ui/core/components/Icons';
+import PopoverDropdown from '@superset-ui/core/components/PopoverDropdown';
+import {
+ FilterBarOrientation,
+ LayoutItem,
+ RootState,
+} from 'src/dashboard/types';
+import { updateDataMask } from 'src/dataMask/actions';
+import { Draggable } from 'src/dashboard/components/dnd/DragDroppable';
+import DragHandle from 'src/dashboard/components/dnd/DragHandle';
+import HoverMenu from 'src/dashboard/components/menu/HoverMenu';
+import IconButton from 'src/dashboard/components/IconButton';
+import WithPopoverMenu from 'src/dashboard/components/menu/WithPopoverMenu';
+import DeleteComponentButton from
'src/dashboard/components/DeleteComponentButton';
+import ResizableContainer from
'src/dashboard/components/resizable/ResizableContainer';
+import FilterControl from
'src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl';
+import { COLUMN_TYPE, ROW_TYPE } from 'src/dashboard/util/componentTypes';
+import {
+ GRID_BASE_UNIT,
+ GRID_MIN_COLUMN_COUNT,
+ GRID_MIN_ROW_UNITS,
+ GRID_COLUMN_COUNT,
+} from 'src/dashboard/util/constants';
+
+export interface FilterHolderProps {
+ id: string;
+ parentId: string;
+ component: LayoutItem;
+ parentComponent: LayoutItem;
+ index: number;
+ depth: number;
+ editMode: boolean;
+
+ // grid related
+ availableColumnCount: number;
+ columnWidth: number;
+ onResizeStart: ResizeStartCallback;
+ onResize: ResizeCallback;
+ onResizeStop: ResizeCallback;
+
+ // dnd
+ deleteComponent: (id: string, parentId: string) => void;
+ updateComponents: (updates: Record<string, LayoutItem>) => void;
+ handleComponentDrop: (...args: unknown[]) => unknown;
+}
+
+const isValueEmpty = (value: unknown): boolean =>
+ value == null ||
+ (Array.isArray(value) && value.length === 0) ||
+ (typeof value === 'string' && value.trim() === '');
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Range clear not treated empty</b></div>
<div id="fix">
`isValueEmpty` treats only length-0 arrays as empty, but the repo's
canonical cleared value for `filter_range` is `[null, null]` (see
`dataMask/reducer.ts:128-130` and `FilterBar/index.tsx:529`). So a required
range filter cleared to `[null, null]` yields `isValueEmpty(...) === false`,
leaving `validateStatus` unset in `handleFilterSelectionChange` and enabling
the Apply button in `isApplyDisabled`. Treat all-null arrays as empty to match
the established convention.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
const isValueEmpty = (value: unknown): boolean =>
value == null ||
(Array.isArray(value) &&
(value.length === 0 || value.every(v => v === null))) ||
(typeof value === 'string' && value.trim() === '');
````
</div>
</details>
</div>
<small><i>Code Review Run #d0dfc3</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]
