codeant-ai-for-open-source[bot] commented on code in PR #43400:
URL: https://github.com/apache/superset/pull/43400#discussion_r3832465022
##########
superset-frontend/src/core/dashboard/widgets/ChartWidget.tsx:
##########
@@ -197,18 +232,77 @@ export default function ChartWidget({ nodeId }: { nodeId:
string }) {
const node = provider.getNode(nodeId);
const dataBinding = node?.props?.dataBinding as DataBindingSpec | undefined;
- const bindingKey = JSON.stringify(dataBinding);
+ // A query-bound widget doesn't subscribe to individual filter nodes — it
+ // recomputes which filters currently apply to it (scoped by dataset
+ // match, see `collectActiveFilters.ts`) every time this component
+ // re-renders, and merges them in the same way its own authored filters
+ // already flow into `dataBinding.filters`.
+ const effectiveBinding = dataBinding
+ ? {
+ ...dataBinding,
+ filters: [
+ ...(dataBinding.filters ?? []),
+ ...getActiveFiltersForDataset(dataBinding.datasetId, nodeId),
+ ],
+ }
+ : undefined;
+ const bindingKey = JSON.stringify(effectiveBinding);
Review Comment:
**Suggestion:** Every filter emission causes every chart to recompute
`effectiveBinding` and start a new `fetchQueryData` request, while the cleanup
only ignores the result and does not abort the request. Rapid multi-select
changes therefore leave multiple requests running per chart and can create a
request storm across a dashboard. Debounce filter-driven fetches or pass an
abort signal through the query request and cancel superseded requests.
[performance]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Multi-select changes create overlapping chart-data requests.
- ⚠️ Large dashboards amplify filter-driven request load.
- ⚠️ Backend `/api/v1/chart/data` receives superseded queries.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/core/dashboard/widgets/ChartWidget.tsx
**Line:** 240:249
**Comment:**
*Performance: Every filter emission causes every chart to recompute
`effectiveBinding` and start a new `fetchQueryData` request, while the cleanup
only ignores the result and does not abort the request. Rapid multi-select
changes therefore leave multiple requests running per chart and can create a
request storm across a dashboard. Debounce filter-driven fetches or pass an
abort signal through the query request and cancel superseded requests.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43400&comment_hash=f5d659325beaf9b4aa5937d587d257cdc3013717a55024151cbeef8dc8bccb7c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43400&comment_hash=f5d659325beaf9b4aa5937d587d257cdc3013717a55024151cbeef8dc8bccb7c&reaction=dislike'>👎</a>
##########
superset-frontend/src/pages/DashboardBuilderV2/SchemaControlPanel.tsx:
##########
@@ -191,6 +216,60 @@ export default function SchemaControlPanel({ nodeId }: {
nodeId: string }) {
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [widgetType, seriesKey]);
+ // The effect above only reacts to `widgetType`/`series` — an edit to a
+ // plain control value (e.g. picking `datasetId`) needs its own trigger so
+ // a field that depends on it (`column`'s enum) gets re-enriched. Debounced,
+ // and skipped whenever the dependency values haven't actually changed,
+ // mirroring `SemanticLayerModal`'s identical rule for the Semantic Layer's
+ // own dynamic fields.
+ const maybeRefreshSchema = useCallback(
+ (data: WidgetProps) => {
+ const dynamicDeps = dynamicDepsRef.current;
+ if (Object.keys(dynamicDeps).length === 0) return;
+
+ const hasSatisfiedDeps = Object.values(dynamicDeps).some(deps =>
+ areDependenciesSatisfied(deps, data, schema ?? undefined),
+ );
+ if (!hasSatisfiedDeps) {
+ if (debounceTimerRef.current) {
+ clearTimeout(debounceTimerRef.current);
+ debounceTimerRef.current = null;
+ }
+ setRefreshingSchema(false);
+ lastDepSnapshotRef.current = '';
+ return;
+ }
+
+ const snapshot = serializeDependencyValues(dynamicDeps, data);
+ if (snapshot === lastDepSnapshotRef.current) return;
+ lastDepSnapshotRef.current = snapshot;
+
+ // Flip the loading state immediately so the dependent field shows it
+ // through the debounce window, rather than keeping its stale options
+ // on screen for the debounce's duration before the request even fires.
+ setRefreshingSchema(true);
+ if (debounceTimerRef.current) clearTimeout(debounceTimerRef.current);
+ debounceTimerRef.current = setTimeout(() => {
+ fetchControlSchema(widgetType, data, series)
+ .then(result => {
+ setSchema(sanitizeSchema(result));
+ dynamicDepsRef.current = getDynamicDependencies(result);
+ setError(null);
+ })
+ .catch(async e => setError(await describeError(e)))
+ .finally(() => setRefreshingSchema(false));
+ }, SCHEMA_REFRESH_DEBOUNCE_MS);
Review Comment:
**Suggestion:** The debounced schema refresh has no cancellation or
generation check. If dependency values change again while this request is
pending, an older response can overwrite the newer schema and dependency map,
and its `finally` can clear `refreshingSchema` while the newer request is still
active. Track the request generation or cancel/ignore stale responses before
updating state. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Inspector can show column options for the wrong dataset.
- ⚠️ Newer schema refreshes can lose their loading indicator.
- ⚠️ Dynamic dependency tracking can become stale.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/pages/DashboardBuilderV2/SchemaControlPanel.tsx
**Line:** 252:261
**Comment:**
*Race Condition: The debounced schema refresh has no cancellation or
generation check. If dependency values change again while this request is
pending, an older response can overwrite the newer schema and dependency map,
and its `finally` can clear `refreshingSchema` while the newer request is still
active. Track the request generation or cancel/ignore stale responses before
updating state.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43400&comment_hash=d57f5d2790431b01cbe29089b3d653f2693040cb154b289642a092377ecd698c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43400&comment_hash=d57f5d2790431b01cbe29089b3d653f2693040cb154b289642a092377ecd698c&reaction=dislike'>👎</a>
##########
superset-frontend/src/core/dashboard/widgets/FilterSelectWidget.tsx:
##########
@@ -0,0 +1,284 @@
+/**
+ * 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 { useEffect, useState } from 'react';
+import { dashboard as dashboardApi } from '@apache-superset/core';
+import { SupersetClient } from '@superset-ui/core';
+import type { JsonResponse } from '@superset-ui/core';
+import { Flex, Select, Typography } from '@superset-ui/core/components';
+import { t } from '@apache-superset/core/translation';
+import { provider, useDashboardRevision } from '../store';
+import { FILTER_BAR_APPLY_EVENT } from '../filterVocabulary';
+import type {
+ ResolvedFilter,
+ FilterValueChangedPayload,
+} from '../filterVocabulary';
+
+/**
+ * Distinct values for `column`, the same `GET .../column/<name>/values/`
+ * lookup Explore's own ad hoc filter popover uses to suggest a comparator
+ * (see `AdhocFilterEditPopoverSimpleTabContent`) — a purpose-built,
+ * dataset-agnostic endpoint, not the general chart-data query path
+ * `fetchQueryData` wraps. Only asked for when the author hasn't set
+ * `props.options` themselves (see the call site): an authored list always
+ * wins, since it's an explicit override, not a fallback.
+ */
+function useDistinctColumnValues(
+ datasetId: number | undefined,
+ column: string | undefined,
+): string[] {
+ const [values, setValues] = useState<string[]>([]);
+
+ useEffect(() => {
+ if (datasetId == null || !column) {
+ setValues([]);
+ return undefined;
+ }
+ let cancelled = false;
+ SupersetClient.get({
+ endpoint:
`/api/v1/datasource/table/${datasetId}/column/${column}/values/`,
+ })
+ .then((response: JsonResponse) => {
+ const result = response.json.result as unknown[];
+ if (!cancelled) {
+ setValues(result.filter(v => v != null).map(v => String(v)));
+ }
+ })
+ .catch(() => {
+ if (!cancelled) setValues([]);
+ });
+ return () => {
+ cancelled = true;
+ };
+ }, [datasetId, column]);
+
+ return values;
+}
+
+/**
+ * A `filter.select` widget's own selection → constraint logic — the
+ * filter-domain equivalent of a chart's `build_queries`. Frontend-only
+ * because it reacts to a *live viewer selection*, not authored config —
+ * unlike the widget's control schema (`datasetId`/`column`/`options`/...),
+ * which is backend-owned (see `superset/widgets/controls.py`'s
+ * `FilterSelectControls`), there is no document state here to serve from a
+ * schema: this only ever runs in response to this session's own clicks.
+ */
+function resolveSelectFilter(
+ column: string,
+ selection: string[],
+ datasource?: number,
+): ResolvedFilter | null {
+ if (!selection.length) return null;
+ return selection.length === 1
+ ? { column, operator: 'EQUALS', value: selection[0], datasource }
+ : { column, operator: 'IN', value: selection, datasource };
+}
+
+/**
+ * The built-in `filter.select` widget — a value/multi-select filter,
+ * registered like any other widget (see `registerBuiltInWidgets`). Unlike
+ * `ChartWidget`/`AgGridTableWidget`, its own props (`column`, `datasetId`,
+ * `options`, `defaultSelection`, `scope`) are authored config read straight
+ * from the node — but what it writes on interaction is *not*
+ * `dashboard.updateProps`. It calls `dashboard.emit`/`dashboard.getValue`
+ * instead — the same generic mechanism any widget uses to affect another —
+ * because a viewer's current selection is per-session state, not part of
+ * the shared, authored document every other viewer and editor sees (see
+ * the design note on `updateProps` in `@apache-superset/core`'s
+ * `dashboard` namespace).
+ *
+ * Options come from `props.options` when the author has set one — still a
+ * static, hand-authored list for this prototype — and otherwise from a
+ * live `useDistinctColumnValues` lookup against the target column, so a
+ * filter placed through the Inspector's schema-driven `datasetId`/`column`
+ * fields alone already has real values to offer. Narrowing those options
+ * by a parent filter's selection (cascading) is still deferred.
+ *
+ * A standalone filter (or one dropped anywhere other than a `filter.bar`)
+ * still emits on every change, immediately, same as always. One sitting
+ * inside a `filter.bar` instead holds each change as a local
+ * `pendingSelection` — visible right away in this Select's own displayed
+ * value, but not emitted to {@link dashboardApi.VALUE_CHANGED_EVENT} (the
+ * event every query-bound consumer reads) until the bar's own Apply
+ * button fires {@link FILTER_BAR_APPLY_EVENT} on the bar's id (see
+ * `FilterBarWidget`'s own render). This is the *only* thing
+ * being inside a bar changes about a filter — the value it eventually
+ * emits, and how every consumer resolves it, are identical either way.
+ */
+export default function FilterSelectWidget({ nodeId }: { nodeId: string }) {
+ // Covers both structural/layout changes and this node's own emitted
+ // value — `emit` ticks the same revision `commit` does (see
+ // `DashboardProvider`), so one subscription is enough.
+ useDashboardRevision();
+
+ const node = provider.getNode(nodeId);
+ const column = node?.props?.column as string | undefined;
+ const datasetId = node?.props?.datasetId as number | undefined;
+ const authoredOptions = node?.props?.options as string[] | undefined;
+ // Called unconditionally, same as every other hook here — `column`/
+ // `datasetId` being unset yet is handled inside the hook, not by
+ // skipping the call.
+ const queriedOptions = useDistinctColumnValues(datasetId, column);
+ const options = authoredOptions ?? queriedOptions;
+ const defaultSelection = node?.props?.defaultSelection as
+ string[] | undefined;
+
+ const parentId = provider.getParentId(nodeId);
+ const inFilterBar =
+ parentId !== undefined && provider.getNode(parentId)?.type ===
'filter.bar';
+
+ // Not yet applied, if set — see this component's own doc comment. Reset
+ // to `undefined` once flushed, so the displayed value goes back to
+ // reading straight off `currentValue`/`defaultSelection` below.
+ const [pendingSelection, setPendingSelection] = useState<
+ string[] | undefined
+ >(undefined);
+
+ const currentValue = provider.getValue(
+ nodeId,
+ dashboardApi.VALUE_CHANGED_EVENT,
+ ) as FilterValueChangedPayload | undefined;
+
+ // Applies the author's default exactly once, the first time this filter
+ // is ever rendered with no live selection yet — a later edit to
+ // `defaultSelection` doesn't retroactively override a viewer's own
+ // choice, the same way changing a form's default doesn't reset a value
+ // the user already typed into it.
+ useEffect(() => {
+ if (
+ column &&
+ defaultSelection &&
+ provider.getValue(nodeId, dashboardApi.VALUE_CHANGED_EVENT) === undefined
+ ) {
+ provider.emit(nodeId, dashboardApi.VALUE_CHANGED_EVENT, {
+ selection: defaultSelection,
+ resolved: resolveSelectFilter(column, defaultSelection, datasetId),
+ });
+ }
+ // Intentionally run once per node id, not on every column/default edit —
+ // see the comment above.
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [nodeId]);
Review Comment:
**Suggestion:** The default is applied only when the component first mounts
because the effect depends solely on `nodeId`. A newly added filter commonly
renders before `column`, `datasetId`, or `defaultSelection` is populated by the
Inspector; when those props are later configured, this effect never runs again,
so the authored default is not emitted and the filter starts unselected. Re-run
the initialization when the relevant configuration becomes available while
preserving the existing live-selection guard. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Authored filter defaults are not applied to chart queries.
- ⚠️ Newly configured filters appear selected but emit nothing.
- ⚠️ Users must change each filter manually.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/core/dashboard/widgets/FilterSelectWidget.tsx
**Line:** 164:178
**Comment:**
*Logic Error: The default is applied only when the component first
mounts because the effect depends solely on `nodeId`. A newly added filter
commonly renders before `column`, `datasetId`, or `defaultSelection` is
populated by the Inspector; when those props are later configured, this effect
never runs again, so the authored default is not emitted and the filter starts
unselected. Re-run the initialization when the relevant configuration becomes
available while preserving the existing live-selection guard.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43400&comment_hash=50f9b48d81d92c35964f3355655b62be429b17b7f9d58b2375c7a38a35b8fbd7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43400&comment_hash=50f9b48d81d92c35964f3355655b62be429b17b7f9d58b2375c7a38a35b8fbd7&reaction=dislike'>👎</a>
##########
superset-frontend/src/core/dashboard/widgets/FilterSelectWidget.tsx:
##########
@@ -0,0 +1,284 @@
+/**
+ * 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 { useEffect, useState } from 'react';
+import { dashboard as dashboardApi } from '@apache-superset/core';
+import { SupersetClient } from '@superset-ui/core';
+import type { JsonResponse } from '@superset-ui/core';
+import { Flex, Select, Typography } from '@superset-ui/core/components';
+import { t } from '@apache-superset/core/translation';
+import { provider, useDashboardRevision } from '../store';
+import { FILTER_BAR_APPLY_EVENT } from '../filterVocabulary';
+import type {
+ ResolvedFilter,
+ FilterValueChangedPayload,
+} from '../filterVocabulary';
+
+/**
+ * Distinct values for `column`, the same `GET .../column/<name>/values/`
+ * lookup Explore's own ad hoc filter popover uses to suggest a comparator
+ * (see `AdhocFilterEditPopoverSimpleTabContent`) — a purpose-built,
+ * dataset-agnostic endpoint, not the general chart-data query path
+ * `fetchQueryData` wraps. Only asked for when the author hasn't set
+ * `props.options` themselves (see the call site): an authored list always
+ * wins, since it's an explicit override, not a fallback.
+ */
+function useDistinctColumnValues(
+ datasetId: number | undefined,
+ column: string | undefined,
+): string[] {
+ const [values, setValues] = useState<string[]>([]);
+
+ useEffect(() => {
+ if (datasetId == null || !column) {
+ setValues([]);
+ return undefined;
+ }
+ let cancelled = false;
+ SupersetClient.get({
+ endpoint:
`/api/v1/datasource/table/${datasetId}/column/${column}/values/`,
+ })
Review Comment:
**Suggestion:** The column name is interpolated directly into a URL path.
Column names containing slashes, spaces, or other URL-significant characters
will be interpreted as path syntax or encoded inconsistently, causing the
distinct-values request to fail or address the wrong resource. Encode the
column path segment before constructing the endpoint. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Distinct options fail for slash-containing columns.
- ⚠️ Affected filters render without selectable values.
- ⚠️ Dashboard authors must provide manual options.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/core/dashboard/widgets/FilterSelectWidget.tsx
**Line:** 54:56
**Comment:**
*Api Mismatch: The column name is interpolated directly into a URL
path. Column names containing slashes, spaces, or other URL-significant
characters will be interpreted as path syntax or encoded inconsistently,
causing the distinct-values request to fail or address the wrong resource.
Encode the column path segment before constructing the endpoint.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43400&comment_hash=4c7efccd67724ff372d0380b1544a7d4187e01d9f3946cc6f55062dbe94c1dcc&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43400&comment_hash=4c7efccd67724ff372d0380b1544a7d4187e01d9f3946cc6f55062dbe94c1dcc&reaction=dislike'>👎</a>
--
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]
