bito-code-review[bot] commented on code in PR #43736:
URL: https://github.com/apache/superset/pull/43736#discussion_r4190270103
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ColumnSelect.tsx:
##########
@@ -139,20 +195,25 @@ export function ColumnSelect({
'columns.is_dttm',
'columns.type_generic',
'columns.filterable',
+ ...(showVerboseNames ? ['columns.verbose_name'] : []),
],
})}`,
})
.then(({ json: { result } }) => {
- const lookupValue = Array.isArray(value) ? value : [value];
- const valueExists = result.columns.some((column: Column) =>
- lookupValue?.includes(column.column_name),
- );
- if (!valueExists) {
+ if (isStale()) {
+ return;
+ }
+ if (!isValueInColumns(result.columns)) {
resetColumnField();
}
setColumns(result.columns);
+ onColumnsLoaded?.(filterColumnNames(result.columns), requestKey);
}, handleError)
- .finally(() => setLoading(false));
+ .finally(() => {
+ if (!isStale()) {
+ setLoading(false);
+ }
+ });
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Stale error toast unguarded</b></div>
<div id="fix">
The new stale-response guard drops stale responses in `.then` and skips
`setLoading` in `.finally`, but `handleError` (line 161) still fires for a
stale request, showing an error toast for a datasource the user already
switched away from. Guard `handleError` with `isStale()` for consistency with
the guard's stated purpose.
</div>
</div>
<small><i>Code Review Run #79b0a8</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/transformers/customizationTransformer.ts:
##########
@@ -96,6 +102,52 @@ function buildCustomizationTarget(
return buildNativeFilterTarget(formInputs);
}
+/**
+ * A Group By allowlist that selects every groupable column is equivalent to
+ * "no restriction". Collapse it back to unset so the saved config does not
+ * freeze a snapshot of the dataset's columns: a column added to the dataset
+ * later then stays available to viewers, exactly like a control that never
+ * stored an allowlist.
+ */
+export function collapseFullColumnsAllowlist(
+ controlValues: ChartCustomizationsFormItem['controlValues'] | undefined,
+ groupableColumns: string[] | undefined,
+): ChartCustomizationsFormItem['controlValues'] {
+ const allowlist = controlValues?.columnsAllowlist;
+ if (
+ !controlValues ||
+ !Array.isArray(allowlist) ||
+ !groupableColumns?.length
+ ) {
+ return controlValues ?? {};
+ }
+ const selected = new Set<string>(allowlist);
+ if (!groupableColumns.every(column => selected.has(column))) {
+ return controlValues;
+ }
+ return omit(controlValues, 'columnsAllowlist');
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Empty allowlist not collapsed</b></div>
<div id="fix">
`collapseFullColumnsAllowlist` collapses an allowlist selecting every
groupable column, but leaves `columnsAllowlist: []` intact. The viewer treats
an empty allowlist as 'no restriction' too (see `applyColumnAllowlist`), so
saving `[]` is a redundant snapshot that is inconsistent with the
full-selection collapse. Consider collapsing the empty case as well for
consistent saved config.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
const selected = new Set<string>(allowlist);
if (
allowlist.length > 0 &&
!groupableColumns.every(column => selected.has(column))
) {
return controlValues;
}
return omit(controlValues, 'columnsAllowlist');
````
</div>
</details>
</div>
<small><i>Code Review Run #79b0a8</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/GroupByFilterCard.tsx:
##########
@@ -414,6 +417,31 @@ const GroupByFilterCard: FC<GroupByFilterCardProps> = ({
[sortAscending],
);
+ // Builder-configured allowlist of columns viewers are allowed to group by.
+ const columnsAllowlist = customizationItem.controlValues?.columnsAllowlist as
+ | string[]
+ | undefined;
+
+ const allowedColumnOptions = useMemo(
+ () =>
+ applyColumnAllowlist(
+ columnOptions,
+ columnsAllowlist,
+ ensureIsArray<string>(currentValue),
+ ),
+ [columnOptions, columnsAllowlist, currentValue],
+ );
+
+ // Every column the builder allowed is gone from the dataset (dropped or no
+ // longer groupable). Rather than a silently empty control, viewers get a
+ // message; the control is not opened up to all columns behind the builder's
+ // back.
+ const allowlistUnavailable =
+ !loading &&
+ columnOptions.length > 0 &&
+ ensureIsArray(columnsAllowlist).length > 0 &&
+ applyColumnAllowlist(columnOptions, columnsAllowlist).length === 0;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unmemoized per-render recompute</b></div>
<div id="fix">
`allowlistUnavailable` (lines 439-443) re-runs `applyColumnAllowlist` on
every render — and this component re-renders on every redux `dataMask` update
via the `state.dataMask` selector (lines 391-393) — partially duplicating the
memoized `allowedColumnOptions` filter above. Wrapping it in `useMemo` keyed on
`[loading, columnOptions, columnsAllowlist]` matches the sibling memo and
removes the redundant per-render scan.
</div>
</div>
<small><i>Code Review Run #79b0a8</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]
