bito-code-review[bot] commented on code in PR #43277:
URL: https://github.com/apache/superset/pull/43277#discussion_r3800379987
##########
superset-frontend/src/dashboard/util/getDetailedComponentWidth.ts:
##########
@@ -108,7 +109,8 @@ export default function getDetailedComponentWidth({
} else if (
component.type === DYNAMIC_TYPE ||
component.type === MARKDOWN_TYPE ||
- component.type === CHART_TYPE
+ component.type === CHART_TYPE ||
+ component.type === FILTER_TYPE
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test coverage gap for FILTER_TYPE</b></div>
<div id="fix">
FILTER_TYPE was added to the minimumWidth logic but lacks corresponding test
coverage. Other types (CHART_TYPE, MARKDOWN_TYPE) have explicit assertions in
the minimumWidth describe block (lines 176-194). Without a test, future
modifications to this function could silently break FILTER_TYPE behavior
without regression detection.
</div>
</div>
<small><i>Code Review Run #68455e</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/FiltersConfigModal/FiltersConfigForm/utils.ts:
##########
@@ -94,12 +94,25 @@ export const shouldShowTimeRangePicker = (
currentDataset: (Dataset & { column_types: GenericDataType[] }) | undefined,
): boolean => (currentDataset ? hasTemporalColumns(currentDataset) : true);
-export const doesColumnMatchFilterType = (filterType: string, column: Column)
=>
- !column.type_generic ||
- !(filterType in FILTER_SUPPORTED_TYPES) ||
- FILTER_SUPPORTED_TYPES[
- filterType as keyof typeof FILTER_SUPPORTED_TYPES
- ]?.includes(column.type_generic);
+export const doesColumnMatchFilterType = (filterType: string, column: Column)
=> {
+ const supportedTypes =
+ FILTER_SUPPORTED_TYPES[filterType as keyof typeof FILTER_SUPPORTED_TYPES];
+ if (!supportedTypes) {
+ return true;
+ }
+ const isTemporal =
+ Boolean(column.is_dttm) || column.type_generic ===
GenericDataType.Temporal;
+ if (
+ supportedTypes.length === 1 &&
+ supportedTypes.includes(GenericDataType.Temporal)
+ ) {
+ return isTemporal;
+ }
+ if (!column.type_generic) {
+ return true;
+ }
+ return supportedTypes.includes(column.type_generic);
+};
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test coverage for is_dttm path</b></div>
<div id="fix">
The new `is_dttm` temporal-detection path (line 104) has no test coverage.
The `createColumn` fixture helper at line 69-72 only accepts `column_name` and
`type_generic`; it cannot construct a column with `is_dttm: true`. Without a
test, the behavior change that allows `is_dttm` columns to match temporal-only
filters could silently break if the logic is modified.
</div>
</div>
<small><i>Code Review Run #68455e</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/index.tsx:
##########
@@ -215,6 +226,11 @@ const FilterBar: FC<FiltersBarProps> = ({
() => new Set(filtersInScope.map(f => f.id)),
[filtersInScope],
);
+ const [allFiltersInScope] = useSelectFiltersInScope(allNativeFilterValues);
+ const allInScopeFilterIds = useMemo(
+ () => new Set(allFiltersInScope.map(f => f.id)),
+ [allFiltersInScope],
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead code in useMemo</b></div>
<div id="fix">
The `allInScopeFilterIds` set computed at lines 230-232 is never consumed —
`grep` confirms no reads after declaration. This dead code adds unnecessary
computation overhead on every render of this component.
</div>
</div>
<small><i>Code Review Run #68455e</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/util/componentIsResizable.ts:
##########
@@ -21,11 +21,12 @@ import {
CHART_TYPE,
MARKDOWN_TYPE,
DYNAMIC_TYPE,
+ FILTER_TYPE,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test coverage gap for new type</b></div>
<div id="fix">
The logic change is correct, but the test file
`componentIsResizable.test.ts` (line 43) only lists `[COLUMN_TYPE, CHART_TYPE,
MARKDOWN_TYPE]` as resizable without including `FILTER_TYPE`. Since this test
directly validates the `componentIsResizable` function's behavior,
`FILTER_TYPE` should be added to the test's `resizable` array and imported to
ensure regression coverage.
</div>
</div>
<small><i>Code Review Run #68455e</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]
