codeant-ai-for-open-source[bot] commented on code in PR #37902:
URL: https://github.com/apache/superset/pull/37902#discussion_r2794553882
##########
superset-frontend/packages/superset-ui-core/src/chart/components/ChartDataProvider.tsx:
##########
@@ -67,103 +66,102 @@ export type ChartDataProviderState = {
error?: ProvidedProps['error'];
};
-class ChartDataProvider extends PureComponent<
- ChartDataProviderProps,
- ChartDataProviderState
-> {
- readonly chartClient: ChartClient;
-
- constructor(props: ChartDataProviderProps) {
- super(props);
- this.state = { status: 'uninitialized' };
- this.chartClient = new ChartClient({ client: props.client });
- }
-
- componentDidMount() {
- this.handleFetchData();
- }
-
- componentDidUpdate(prevProps: ChartDataProviderProps) {
- const { formData, sliceId } = this.props;
- if (formData !== prevProps.formData || sliceId !== prevProps.sliceId) {
- this.handleFetchData();
- }
- }
-
- private extractSliceIdAndFormData() {
- const { formData, sliceId } = this.props;
- return formData ? { formData } : { sliceId: sliceId as number };
- }
-
- private handleFetchData = () => {
- const {
- loadDatasource,
- formDataRequestOptions,
- datasourceRequestOptions,
- queryRequestOptions,
- } = this.props;
-
- this.setState({ status: 'loading' }, () => {
- try {
- this.chartClient
- .loadFormData(
- this.extractSliceIdAndFormData(),
- formDataRequestOptions,
- )
- .then(formData =>
- Promise.all([
- loadDatasource
- ? this.chartClient.loadDatasource(
- formData.datasource,
- datasourceRequestOptions,
- )
- : Promise.resolve(undefined),
- this.chartClient.loadQueryData(formData, queryRequestOptions),
- ]).then(
- ([datasource, queriesData]) =>
- // eslint-disable-next-line
@typescript-eslint/consistent-type-assertions
- ({
- datasource,
- formData,
- queriesData,
- }) as Payload,
- ),
- )
- .then(this.handleReceiveData)
- .catch(this.handleError);
- } catch (error) {
- this.handleError(error as Error);
- }
- });
- };
-
- private handleReceiveData = (payload?: Payload) => {
- const { onLoaded } = this.props;
- if (onLoaded) onLoaded(payload);
- this.setState({ payload, status: 'loaded' });
- };
-
- private handleError = (error: ProvidedProps['error']) => {
- const { onError } = this.props;
- if (onError) onError(error);
- this.setState({ error, status: 'error' });
- };
-
- render() {
- const { children } = this.props;
- const { status, payload, error } = this.state;
-
- switch (status) {
- case 'loading':
- return children({ loading: true });
- case 'loaded':
- return children({ payload });
- case 'error':
- return children({ error });
- case 'uninitialized':
- default:
- return null;
+function ChartDataProvider({
+ children,
+ client,
+ formData,
+ sliceId,
+ loadDatasource,
+ onError,
+ onLoaded,
+ formDataRequestOptions,
+ datasourceRequestOptions,
+ queryRequestOptions,
+}: ChartDataProviderProps) {
+ const [state, setState] = useState<ChartDataProviderState>({
+ status: 'uninitialized',
+ });
+
+ const chartClient = useMemo(() => new ChartClient({ client }), [client]);
+
+ const extractSliceIdAndFormData = useCallback(
+ (): SliceIdAndOrFormData =>
+ formData ? { formData } : { sliceId: sliceId as number },
+ [formData, sliceId],
+ );
+
+ const handleReceiveData = useCallback(
+ (payload?: Payload) => {
+ if (onLoaded) onLoaded(payload);
+ setState({ payload, status: 'loaded' });
+ },
+ [onLoaded],
+ );
+
+ const handleError = useCallback(
+ (error: ProvidedProps['error']) => {
+ if (onError) onError(error);
+ setState({ error, status: 'error' });
+ },
+ [onError],
+ );
+
+ const handleFetchData = useCallback(() => {
+ setState({ status: 'loading' });
+ try {
+ chartClient
+ .loadFormData(extractSliceIdAndFormData(), formDataRequestOptions)
+ .then(loadedFormData =>
+ Promise.all([
+ loadDatasource
+ ? chartClient.loadDatasource(
+ loadedFormData.datasource,
+ datasourceRequestOptions,
+ )
+ : Promise.resolve(undefined),
+ chartClient.loadQueryData(loadedFormData, queryRequestOptions),
+ ]).then(
+ ([datasource, queriesData]) =>
+ ({
+ datasource,
+ formData: loadedFormData,
+ queriesData,
+ }) as Payload,
+ ),
+ )
+ .then(handleReceiveData)
+ .catch(handleError);
+ } catch (error) {
+ handleError(error as Error);
}
+ }, [
+ chartClient,
+ extractSliceIdAndFormData,
+ formDataRequestOptions,
+ loadDatasource,
+ datasourceRequestOptions,
+ queryRequestOptions,
+ handleReceiveData,
+ handleError,
+ ]);
+
+ // Fetch data on mount and when formData or sliceId changes
+ useEffect(() => {
+ handleFetchData();
+ }, [formData, sliceId, handleFetchData]);
Review Comment:
**Suggestion:** The `useEffect` that triggers data fetching currently
depends on `handleFetchData` as well as `formData` and `sliceId`, but
`handleFetchData` itself is recreated whenever callback props (like `onLoaded`,
`onError`) or request options change identity; this causes `ChartDataProvider`
to refetch data whenever those props change even if `formData` and `sliceId`
are unchanged, which is a behavioral change from the class-based implementation
and can lead to unnecessary or repeated network requests. To align with the
original semantics—only refetching when `formData` or `sliceId` change—you
should make the effect depend only on those two values so that changing
callbacks or options doesn't inadvertently trigger a new fetch. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ ChartDataProvider refetches when callbacks/options change, increasing
network load.
- ⚠️ Behavior contradicts comment 'only when formData or sliceId'.
```
</details>
```suggestion
}, [formData, sliceId]);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/packages/superset-ui-core/src/chart/components/ChartDataProvider.tsx`
and note that `handleFetchData` (lines 109–146) is created with
`useCallback` and depends
on `handleReceiveData` and `handleError`, which in turn depend on `onLoaded`
and `onError`
props (lines 93–107).
2. Note the effect at lines 148–151 in the same file: `useEffect(() => {
handleFetchData(); }, [formData, sliceId, handleFetchData]);` and the
preceding comment
"Fetch data on mount and when formData or sliceId changes".
3. In
`superset-frontend/packages/superset-ui-core/test/chart/components/ChartDataProvider.test.tsx`,
inside the `describe('callbacks', () => { ... })` block around lines
278–328, add a new
test that: (a) calls `setup({ onLoaded: jest.fn() })`, waits for the initial
load to
settle with `act`, (b) rerenders `ChartDataProvider` via
`rerender(<ChartDataProvider
{...props} onLoaded={jest.fn()} />);` without changing `formData` or
`sliceId`, and (c)
asserts how many times `mockLoadFormData` and `mockLoadQueryData` (defined
at lines 28–49)
are called.
4. Run this new test with the current implementation (e.g. `npm test --
ChartDataProvider.test.tsx`) and observe that `mockLoadFormData` and
`mockLoadQueryData`
are called again after the rerender with a new `onLoaded` callback, even
though `formData`
and `sliceId` are unchanged; this happens because `onLoaded`'s identity
changes, which
changes `handleReceiveData`, which changes `handleFetchData`, which is in
the `useEffect`
dependency array and thus retriggers a fetch, contrary to the stated intent
in the
comment.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/chart/components/ChartDataProvider.tsx
**Line:** 151:151
**Comment:**
*Logic Error: The `useEffect` that triggers data fetching currently
depends on `handleFetchData` as well as `formData` and `sliceId`, but
`handleFetchData` itself is recreated whenever callback props (like `onLoaded`,
`onError`) or request options change identity; this causes `ChartDataProvider`
to refetch data whenever those props change even if `formData` and `sliceId`
are unchanged, which is a behavioral change from the class-based implementation
and can lead to unnecessary or repeated network requests. To align with the
original semantics—only refetching when `formData` or `sliceId` change—you
should make the effect depend only on those two values so that changing
callbacks or options doesn't inadvertently trigger a new fetch.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=a94a47a865a1e33c54666fe625def9f64632e17fdad00060c42c774c3d7ff2c8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=a94a47a865a1e33c54666fe625def9f64632e17fdad00060c42c774c3d7ff2c8&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-ui-core/src/chart/components/reactify.tsx:
##########
@@ -49,66 +54,78 @@ export interface RenderFuncType<Props> {
propTypes?: WeakValidationMap<Props & ReactifyProps>;
}
+export interface ReactifiedComponentRef {
+ container?: HTMLDivElement;
+}
+
+type ReactifiedComponent<Props> = ForwardRefExoticComponent<
+ PropsWithoutRef<Props & ReactifyProps> &
RefAttributes<ReactifiedComponentRef>
+> & {
+ defaultProps?: Partial<Props & ReactifyProps>;
+ propTypes?: WeakValidationMap<Props & ReactifyProps>;
+};
+
export default function reactify<Props extends object>(
renderFn: RenderFuncType<Props>,
callbacks?: LifeCycleCallbacks,
-): ComponentClass<Props & ReactifyProps> {
- class ReactifiedComponent extends Component<Props & ReactifyProps> {
- container?: HTMLDivElement;
-
- constructor(props: Props & ReactifyProps) {
- super(props);
- this.setContainerRef = this.setContainerRef.bind(this);
- }
-
- componentDidMount() {
- this.execute();
- }
-
- componentDidUpdate() {
- this.execute();
- }
-
- componentWillUnmount() {
- this.container = undefined;
- if (callbacks?.componentWillUnmount) {
- callbacks.componentWillUnmount.bind(this)();
+): ReactifiedComponent<Props> {
+ const ReactifiedComponent = forwardRef<
+ ReactifiedComponentRef,
+ Props & ReactifyProps
+ >(function ReactifiedComponent(props, ref) {
+ const containerRef = useRef<HTMLDivElement>(null);
+
+ // Expose container via ref for external access
+ useImperativeHandle(
+ ref,
+ () => ({
+ get container() {
+ return containerRef.current ?? undefined;
+ },
+ }),
+ [],
+ );
+
+ // Execute renderFn on mount and every update (mimics componentDidMount +
componentDidUpdate)
+ useEffect(() => {
+ if (containerRef.current) {
+ renderFn(containerRef.current, props);
}
- }
+ });
- setContainerRef(ref: HTMLDivElement) {
- this.container = ref;
- }
+ // Cleanup on unmount
+ useEffect(
+ () => () => {
+ if (callbacks?.componentWillUnmount) {
+ callbacks.componentWillUnmount();
+ }
Review Comment:
**Suggestion:** The new hook-based implementation of `reactify` calls the
optional `componentWillUnmount` callback without binding `this`, whereas the
previous class-based version explicitly did
`callbacks.componentWillUnmount.bind(this)()`. Any existing
`componentWillUnmount` implementations that rely on `this` (for example
accessing `this.container` to clean up DOM or listeners) will now see `this ===
undefined` and can throw at runtime or silently skip required cleanup. To
preserve compatibility, you should invoke the callback with a `this` object
that at least exposes the expected `container` property, mimicking the old
behavior. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ NVD3 legacy charts throw error on unmounting React component.
- ❌ Tooltip cleanup skipped causing stuck tooltips on dashboards.
- ⚠️ React error boundaries may capture unmount crash logs.
```
</details>
```suggestion
// Preserve legacy behavior where `this` was a component instance
// exposing a `container` property.
callbacks.componentWillUnmount.call({ container: undefined });
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Note the legacy NVD3 wrapper at
`superset-frontend/plugins/legacy-preset-chart-nvd3/src/ReactNVD3.tsx:26-33`,
where
`function componentWillUnmount() { const { id } = this.props; ... }` reads
`this.props.id`
to decide whether to call `removeTooltip(id)` or `hideTooltips(true)`.
2. Observe that the same file wires this callback into `reactify` at
`ReactNVD3.tsx:35`
via `const ReactNVD3 = reactify(Component, { componentWillUnmount });`, so
`reactify`'s
`callbacks.componentWillUnmount` is this unbound function relying on its
`this` context.
3. In the new hook-based implementation of `reactify` at
`superset-frontend/packages/superset-ui-core/src/chart/components/reactify.tsx:97-105`,
the cleanup effect calls `callbacks.componentWillUnmount();` without
`.bind(this)` or
`.call(...)`, so in strict mode the function's `this` is `undefined` instead
of the former
component instance with `this.props` and `this.container`.
4. Render any NVD3 legacy chart that uses `ReactNVD3` (e.g. via the chart
framework, which
imports this default export) and then unmount it (closing the Explore view,
removing the
chart from a dashboard, or programmatically unmounting its React subtree);
during unmount,
React runs the cleanup effect, `callbacks.componentWillUnmount()` executes
with `this ===
undefined`, and `componentWillUnmount` at `ReactNVD3.tsx:26-33` throws a
`TypeError` when
reading `this.props`, preventing tooltip cleanup and surfacing a React error
on unmount.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/chart/components/reactify.tsx
**Line:** 101:101
**Comment:**
*Logic Error: The new hook-based implementation of `reactify` calls the
optional `componentWillUnmount` callback without binding `this`, whereas the
previous class-based version explicitly did
`callbacks.componentWillUnmount.bind(this)()`. Any existing
`componentWillUnmount` implementations that rely on `this` (for example
accessing `this.container` to clean up DOM or listeners) will now see `this ===
undefined` and can throw at runtime or silently skip required cleanup. To
preserve compatibility, you should invoke the callback with a `this` object
that at least exposes the expected `container` property, mimicking the old
behavior.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=64ca622e57e9b78b4498ca6db777163c4f8bb5b2cf608ccace92dbaa2521f8d1&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=64ca622e57e9b78b4498ca6db777163c4f8bb5b2cf608ccace92dbaa2521f8d1&reaction=dislike'>👎</a>
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -176,243 +163,310 @@ const MessageSpan = styled.span`
color: ${({ theme }) => theme.colorText};
`;
-class Chart extends PureComponent<ChartProps, {}> {
- static defaultProps = defaultProps;
-
- renderStartTime: any;
-
- constructor(props: ChartProps) {
- super(props);
- this.handleRenderContainerFailure =
- this.handleRenderContainerFailure.bind(this);
- }
-
- componentDidMount() {
- if (this.props.triggerQuery) {
- this.runQuery();
- }
- }
-
- componentDidUpdate() {
- if (this.props.triggerQuery) {
- this.runQuery();
- }
- }
-
- shouldRenderChart() {
- return (
- this.props.isInView ||
+function Chart({
+ addFilter = () => BLANK,
+ onFilterMenuOpen = () => BLANK,
+ onFilterMenuClose = () => BLANK,
+ initialValues = BLANK,
+ setControlValue = () => BLANK,
+ triggerRender = false,
+ dashboardId,
+ chartStackTrace,
+ force = false,
+ isInView = true,
+ ...restProps
+}: ChartProps): JSX.Element {
+ const {
+ actions,
+ chartId,
+ datasource,
+ formData,
+ timeout,
+ ownState,
+ chartAlert,
+ chartStatus,
+ queriesResponse = [],
+ errorMessage,
+ chartIsStale,
+ width,
+ height,
+ datasetsStatus,
+ onQuery,
+ annotationData,
+ labelColors: _labelColors,
+ sharedLabelColors: _sharedLabelColors,
+ vizType,
+ isFiltersInitialized: _isFiltersInitialized,
+ latestQueryFormData,
+ triggerQuery,
+ postTransformProps,
+ emitCrossFilters,
+ onChartStateChange,
+ } = restProps;
+
+ const renderStartTimeRef = useRef<number>(0);
Review Comment:
**Suggestion:** The `renderStartTimeRef` used when logging chart render
failures is never updated from its initial value of `0`, so `start_offset` in
the `LOG_ACTIONS_RENDER_CHART` error event is always `0`, which breaks the
timing metrics; initialize it with `Logger.getTimestamp()` so that error logs
record a meaningful start offset. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Chart error logs use incorrect start_offset and duration.
- ⚠️ Misleading render timing metrics for LOG_ACTIONS_RENDER_CHART events.
```
</details>
```suggestion
const renderStartTimeRef = useRef<number>(Logger.getTimestamp());
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a dashboard chart or Explore chart so that `ChartContainer`
(`superset-frontend/src/components/Chart/ChartContainer.tsx:24`) renders the
`Chart`
component from `superset-frontend/src/components/Chart/Chart.tsx`.
2. Observe that `Chart` wraps `ChartRenderer` in an `ErrorBoundary` at
`superset-frontend/src/components/Chart/Chart.tsx:458-469` with
`onError={handleRenderContainerFailure}`.
3. Cause a runtime error in the chart rendering tree so that React's error
boundary path
is exercised (for example, a bug in a viz plugin rendered by `SuperChart` in
`ChartRenderer` at
`superset-frontend/src/components/Chart/ChartRenderer.tsx:600-626` that
throws during render). This will trigger `ErrorBoundary` to call
`handleRenderContainerFailure` in `Chart.tsx:244-261`.
4. In `handleRenderContainerFailure`, the code logs
`LOG_ACTIONS_RENDER_CHART` using
`start_offset: renderStartTimeRef.current` and `duration:
Logger.getTimestamp() -
renderStartTimeRef.current`
(`superset-frontend/src/components/Chart/Chart.tsx:253-259`),
but `renderStartTimeRef` is only initialized as `useRef<number>(0)` at line
207 and never
updated elsewhere in this file (verified via repository-wide search for
`renderStartTimeRef`), so `start_offset` is always `0` and `duration` is
computed from
that zero baseline, producing misleading timing metrics for all such error
logs.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Chart/Chart.tsx
**Line:** 207:207
**Comment:**
*Logic Error: The `renderStartTimeRef` used when logging chart render
failures is never updated from its initial value of `0`, so `start_offset` in
the `LOG_ACTIONS_RENDER_CHART` error event is always `0`, which breaks the
timing metrics; initialize it with `Logger.getTimestamp()` so that error logs
record a meaningful start offset.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=d8c90eac9a65889ba627e9febefcd6c03db87cab5f502118abec7b906d13eae8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=d8c90eac9a65889ba627e9febefcd6c03db87cab5f502118abec7b906d13eae8&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/components/Dashboard.tsx:
##########
@@ -90,165 +90,61 @@ interface VisibilityEventData {
ts: number;
}
-class Dashboard extends PureComponent<DashboardProps> {
- static contextType = PluginContext;
-
- // Use type assertion when accessing context instead of declare field
- // to avoid babel transformation issues in Jest
-
- static defaultProps = {
- timeout: 60,
- userId: '',
- };
-
- appliedFilters: ActiveFilters;
-
- appliedOwnDataCharts: JsonObject;
-
- visibilityEventData: VisibilityEventData;
-
- static onBeforeUnload(hasChanged: boolean): void {
- if (hasChanged) {
- window.addEventListener('beforeunload', Dashboard.unload);
- } else {
- window.removeEventListener('beforeunload', Dashboard.unload);
- }
- }
-
- static unload(): string {
- const message = t('You have unsaved changes.');
- // Gecko + IE: returnValue is typed as boolean but historically accepts
string
- (window.event as BeforeUnloadEvent).returnValue = message;
- return message; // Gecko + Webkit, Safari, Chrome etc.
- }
-
- constructor(props: DashboardProps) {
- super(props);
- this.appliedFilters = props.activeFilters ?? {};
- this.appliedOwnDataCharts = props.ownDataCharts ?? {};
- this.visibilityEventData = { start_offset: 0, ts: 0 };
- this.onVisibilityChange = this.onVisibilityChange.bind(this);
- }
-
- componentDidMount(): void {
- const bootstrapData = getBootstrapData();
- const { editMode, isPublished, layout } = this.props;
- const eventData: Record<string, unknown> = {
- is_soft_navigation: Logger.timeOriginOffset > 0,
- is_edit_mode: editMode,
- mount_duration: Logger.getTimestamp(),
- is_empty: isDashboardEmpty(layout),
- is_published: isPublished,
- bootstrap_data_length: JSON.stringify(bootstrapData).length,
- };
- const directLinkComponentId = getLocationHash();
- if (directLinkComponentId) {
- eventData.target_id = directLinkComponentId;
- }
- this.props.actions.logEvent(LOG_ACTIONS_MOUNT_DASHBOARD, eventData);
-
- // Handle browser tab visibility change
- if (document.visibilityState === 'hidden') {
- this.visibilityEventData = {
- start_offset: Logger.getTimestamp(),
- ts: new Date().getTime(),
- };
- }
- window.addEventListener('visibilitychange', this.onVisibilityChange);
- this.applyCharts();
- }
-
- componentDidUpdate(prevProps: DashboardProps): void {
- this.applyCharts();
- const currentChartIds = getChartIdsFromLayout(prevProps.layout);
- const nextChartIds = getChartIdsFromLayout(this.props.layout);
-
- if (prevProps.dashboardId !== this.props.dashboardId) {
- // single-page-app navigation check
- return;
- }
-
- if (currentChartIds.length < nextChartIds.length) {
- const newChartIds = nextChartIds.filter(
- key => currentChartIds.indexOf(key) === -1,
- );
- newChartIds.forEach(newChartId =>
- this.props.actions.addSliceToDashboard(
- newChartId,
- getLayoutComponentFromChartId(this.props.layout, newChartId),
- ),
- );
- } else if (currentChartIds.length > nextChartIds.length) {
- // remove chart
- const removedChartIds = currentChartIds.filter(
- key => nextChartIds.indexOf(key) === -1,
- );
- removedChartIds.forEach(removedChartId =>
- this.props.actions.removeSliceFromDashboard(removedChartId),
- );
- }
- }
-
- applyCharts(): void {
- const {
- activeFilters,
- ownDataCharts,
- chartConfiguration,
- hasUnsavedChanges,
- editMode,
- } = this.props;
- const { appliedFilters, appliedOwnDataCharts } = this;
- if (!chartConfiguration) {
- // For a first loading we need to wait for cross filters charts data
loaded to get all active filters
- // for correct comparing of filters to avoid unnecessary requests
- return;
- }
-
- if (
- !editMode &&
- (!areObjectsEqual(appliedOwnDataCharts, ownDataCharts, {
- ignoreUndefined: true,
- }) ||
- !areObjectsEqual(appliedFilters, activeFilters, {
- ignoreUndefined: true,
- }))
- ) {
- this.applyFilters();
- }
-
- if (hasUnsavedChanges) {
- Dashboard.onBeforeUnload(true);
- } else {
- Dashboard.onBeforeUnload(false);
- }
+function onBeforeUnload(hasChanged: boolean): void {
+ if (hasChanged) {
+ window.addEventListener('beforeunload', unload);
+ } else {
+ window.removeEventListener('beforeunload', unload);
}
+}
- componentWillUnmount(): void {
- window.removeEventListener('visibilitychange', this.onVisibilityChange);
- this.props.actions.clearDataMaskState();
- this.props.actions.clearAllChartStates();
- }
+function unload(): string {
+ const message = t('You have unsaved changes.');
+ // Gecko + IE: returnValue is typed as boolean but historically accepts
string
Review Comment:
**Suggestion:** The `beforeunload` handler relies on `window.event` instead
of the event parameter, which is non-standard in modern browsers and can be
undefined, leading to a runtime TypeError and preventing the unsaved-changes
warning from working consistently. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- CRITICAL: Unsaved dashboard edits may be lost without browser warning.
- CRITICAL: Runtime TypeError possible in nonstandard `window.event`
environments.
- WARNING: Inconsistent beforeunload handling versus shared
`useBeforeUnload` hook.
```
</details>
```suggestion
function unload(event: BeforeUnloadEvent): string {
const message = t('You have unsaved changes.');
// Set returnValue on the actual event object to trigger the browser prompt
event.returnValue = message;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Load any dashboard page so that `Dashboard` is mounted via the Redux
container at
`superset-frontend/src/dashboard/containers/Dashboard.ts:34-56`, which passes
`hasUnsavedChanges: dashboardState.hasUnsavedChanges` into the `Dashboard`
component
(`DashboardProps.hasUnsavedChanges` declared in `Dashboard.tsx:70-75`).
2. Put the dashboard into a state where `dashboardState.hasUnsavedChanges`
is true (e.g.,
by dispatching an `ON_CHANGE` or `SET_UNSAVED_CHANGES` action, which the
reducer handles
by setting `hasUnsavedChanges: true` at
`superset-frontend/src/dashboard/reducers/dashboardState.ts:245-247,
271-274`). This flows
through the container so that the `hasUnsavedChanges` prop received by
`Dashboard` becomes
`true`.
3. With `hasUnsavedChanges === true`, the `applyCharts` hook in
`superset-frontend/src/dashboard/components/Dashboard.tsx:225-256` calls
`onBeforeUnload(true)` at lines 244-247, which in turn attaches the
`beforeunload`
listener `unload` via `window.addEventListener('beforeunload', unload)` in
`Dashboard.tsx:93-99`.
4. Attempt to close or reload the browser tab while in this state. The
browser invokes the
registered `beforeunload` handler `unload` from `Dashboard.tsx:101-105`.
That function
currently reads `(window.event as BeforeUnloadEvent).returnValue =
message;`. In
environments where `window.event` is not populated for
`addEventListener`-registered
handlers (a non-standard, browser-dependent behavior documented on MDN and
avoided
elsewhere in this codebase, e.g., `useBeforeUnload` at
`superset-frontend/src/hooks/useBeforeUnload/index.ts:30-41` correctly
relies on the event
parameter), `window.event` will be `undefined`. The property write
`(...).returnValue =
message` then throws a runtime TypeError instead of setting
`event.returnValue`, which can
prevent the unsaved-changes warning dialog from appearing and may abort the
intended
unload handling.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/Dashboard.tsx
**Line:** 101:103
**Comment:**
*Logic Error: The `beforeunload` handler relies on `window.event`
instead of the event parameter, which is non-standard in modern browsers and
can be undefined, leading to a runtime TypeError and preventing the
unsaved-changes warning from working consistently.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=5bed28ae6f0dfddee95c89ac5664c23ae8148e4770b131d10a5214fc0714d119&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=5bed28ae6f0dfddee95c89ac5664c23ae8148e4770b131d10a5214fc0714d119&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -401,251 +652,122 @@ export class TableRenderer extends Component<
rowKeys,
rowTotals,
colTotals,
- arrowCollapsed: subtotalOptions.arrowCollapsed,
- arrowExpanded: subtotalOptions.arrowExpanded,
+ arrowCollapsed: mergedSubtotalOptions.arrowCollapsed,
+ arrowExpanded: mergedSubtotalOptions.arrowExpanded,
colSubtotalDisplay,
rowSubtotalDisplay,
cellCallbacks,
rowTotalCallbacks,
colTotalCallbacks,
grandTotalCallback,
namesMapping,
- allowRenderHtml: props.allowRenderHtml,
- };
- }
-
- clickHandler(
- pivotData: InstanceType<typeof PivotData>,
- rowValues: string[],
- colValues: string[],
- ) {
- const colAttrs = this.props.cols;
- const rowAttrs = this.props.rows;
- const value = pivotData.getAggregator(rowValues, colValues).value();
- const filters: Record<string, string> = {};
- const colLimit = Math.min(colAttrs.length, colValues.length);
- for (let i = 0; i < colLimit; i += 1) {
- const attr = colAttrs[i];
- if (colValues[i] !== null) {
- filters[attr] = colValues[i];
- }
- }
- const rowLimit = Math.min(rowAttrs.length, rowValues.length);
- for (let i = 0; i < rowLimit; i += 1) {
- const attr = rowAttrs[i];
- if (rowValues[i] !== null) {
- filters[attr] = rowValues[i];
- }
- }
- const { clickCallback } = this.props.tableOptions;
- return (e: MouseEvent) => clickCallback?.(e, value, filters, pivotData);
- }
-
- clickHeaderHandler(
- pivotData: InstanceType<typeof PivotData>,
- values: string[],
- attrs: string[],
- attrIdx: number,
- callback: HeaderClickCallback | undefined,
- isSubtotal = false,
- isGrandTotal = false,
- ) {
- const filters: Record<string, string> = {};
- for (let i = 0; i <= attrIdx; i += 1) {
- const attr = attrs[i];
- filters[attr] = values[i];
- }
- return (e: MouseEvent) =>
- callback?.(
- e,
- values[attrIdx],
- filters,
- pivotData,
- isSubtotal,
- isGrandTotal,
- );
- }
-
- collapseAttr(rowOrCol: boolean, attrIdx: number, allKeys: string[][]) {
- return (e: MouseEvent<HTMLSpanElement>) => {
- // Collapse an entire attribute.
- e.stopPropagation();
- const keyLen = attrIdx + 1;
- const collapsed = allKeys
- .filter((k: string[]) => k.length === keyLen)
- .map(flatKey);
-
- const updates: Record<string, boolean> = {};
- collapsed.forEach((k: string) => {
- updates[k] = true;
- });
-
- if (rowOrCol) {
- this.setState(state => ({
- collapsedRows: { ...state.collapsedRows, ...updates },
- }));
- } else {
- this.setState(state => ({
- collapsedCols: { ...state.collapsedCols, ...updates },
- }));
- }
- };
- }
-
- expandAttr(rowOrCol: boolean, attrIdx: number, allKeys: string[][]) {
- return (e: MouseEvent<HTMLSpanElement>) => {
- // Expand an entire attribute. This implicitly implies expanding all of
the
- // parents as well. It's a bit inefficient but ah well...
- e.stopPropagation();
- const updates: Record<string, boolean> = {};
- allKeys.forEach((k: string[]) => {
- for (let i = 0; i <= attrIdx; i += 1) {
- updates[flatKey(k.slice(0, i + 1))] = false;
- }
- });
-
- if (rowOrCol) {
- this.setState(state => ({
- collapsedRows: { ...state.collapsedRows, ...updates },
- }));
- } else {
- this.setState(state => ({
- collapsedCols: { ...state.collapsedCols, ...updates },
- }));
- }
- };
- }
-
- toggleRowKey(flatRowKey: string) {
- return (e: MouseEvent<HTMLSpanElement>) => {
- e.stopPropagation();
- this.setState(state => ({
- collapsedRows: {
- ...state.collapsedRows,
- [flatRowKey]: !state.collapsedRows[flatRowKey],
- },
- }));
- };
- }
-
- toggleColKey(flatColKey: string) {
- return (e: MouseEvent<HTMLSpanElement>) => {
- e.stopPropagation();
- this.setState(state => ({
- collapsedCols: {
- ...state.collapsedCols,
- [flatColKey]: !state.collapsedCols[flatColKey],
- },
- }));
+ allowRenderHtml,
};
- }
-
- calcAttrSpans(attrArr: string[][], numAttrs: number) {
- // Given an array of attribute values (i.e. each element is another array
with
- // the value at every level), compute the spans for every attribute value
at
- // every level. The return value is a nested array of the same shape. It
has
- // -1's for repeated values and the span number otherwise.
-
- const spans = [];
- // Index of the last new value
- const li = Array(numAttrs).map(() => 0);
- let lv: (string | null)[] = Array(numAttrs).map(() => null);
- for (let i = 0; i < attrArr.length; i += 1) {
- // Keep increasing span values as long as the last keys are the same. For
- // the rest, record spans of 1. Update the indices too.
- const cv = attrArr[i];
- const ent = [];
- let depth = 0;
- const limit = Math.min(lv.length, cv.length);
- while (depth < limit && lv[depth] === cv[depth]) {
- ent.push(-1);
- spans[li[depth]][depth] += 1;
- depth += 1;
- }
- while (depth < cv.length) {
- li[depth] = i;
- ent.push(1);
- depth += 1;
- }
- spans.push(ent);
- lv = cv;
- }
- return spans;
- }
+ }, [
+ cols,
+ rows,
+ tableOptions,
+ namesMappingProp,
+ subtotalOptions,
+ props,
+ allowRenderHtml,
+ clickHandler,
+ ]);
+
+ const visibleKeys = useCallback(
+ (
+ keys: string[][],
+ collapsed: Record<string, boolean>,
+ numAttrs: number,
+ subtotalDisplay: SubtotalDisplay,
+ ) =>
+ keys.filter(
+ (key: string[]) =>
+ // Is the key hidden by one of its parents?
+ !key.some(
+ (_k: string, j: number) => collapsed[flatKey(key.slice(0, j))],
+ ) &&
+ // Leaf key.
+ (key.length === numAttrs ||
+ // Children hidden. Must show total.
+ flatKey(key) in collapsed ||
+ // Don't hide totals.
+ !subtotalDisplay.hideOnExpand),
+ ),
+ [],
+ );
- getAggregatedData(
- pivotData: InstanceType<typeof PivotData>,
- visibleColName: string[],
- rowPartialOnTop: boolean | undefined,
- ) {
- // Transforms flat row keys into a hierarchical group structure where each
level
- // represents a grouping dimension. For each row key path, it calculates
the
- // aggregated value for the specified column and builds a nested object
that
- // preserves the hierarchy while storing aggregation values at each level.
- const groups: Record<string, HierarchicalNode> = {};
- const rows = pivotData.rowKeys;
- rows.forEach(rowKey => {
- const aggValue =
- pivotData.getAggregator(rowKey, visibleColName).value() ?? 0;
-
- if (rowPartialOnTop) {
- const parent = rowKey
- .slice(0, -1)
- .reduce(
- (acc: Record<string, HierarchicalNode>, key: string) =>
- (acc[key] ??= {}) as Record<string, HierarchicalNode>,
- groups,
- );
- parent[rowKey.at(-1)!] = { currentVal: aggValue as number };
- } else {
- rowKey.reduce((acc: Record<string, HierarchicalNode>, key: string) => {
- acc[key] = acc[key] || { currentVal: 0 };
- (acc[key] as HierarchicalNode).currentVal = aggValue as number;
- return acc[key] as Record<string, HierarchicalNode>;
- }, groups);
- }
- });
- return groups;
- }
+ const isDashboardEditMode = useCallback(
+ () => document.contains(document.querySelector('.dashboard--editing')),
+ [],
+ );
- sortAndCacheData(
- groups: Record<string, HierarchicalNode>,
- sortOrder: string,
- rowEnabled: boolean | undefined,
- rowPartialOnTop: boolean | undefined,
- maxRowIndex: number,
- ) {
- // Processes hierarchical data by first sorting it according to the
specified order
- // and then converting the sorted structure into a flat array format. This
function
- // serves as an intermediate step between hierarchical data representation
and
- // flat array representation needed for rendering.
- const sortedGroups = sortHierarchicalObject(
- groups,
- sortOrder,
- rowPartialOnTop,
- );
- return convertToArray(
- sortedGroups,
- rowEnabled,
- rowPartialOnTop,
- maxRowIndex,
- );
- }
+ // Compute base pivot settings, memoized based on relevant props
+ const basePivotSettings = useMemo(() => {
+ // Clear sort cache when props change
+ sortCacheRef.current.clear();
+ return getBasePivotSettings();
+ }, [getBasePivotSettings]);
+
+ // Reset sort state when props change
+ useEffect(() => {
+ setSortingOrder([]);
+ setActiveSortColumn(null);
+ setSortedRowKeys(null);
+ }, [props]);
Review Comment:
**Suggestion:** The effect that resets the sorting state depends on the
`props` object, which is recreated on every render (because of the
`...restProps` spread), causing the effect to run on every render and
immediately clear `sortingOrder`, `activeSortColumn`, and `sortedRowKeys` after
a sort is triggered, so the user can never see or maintain a sorted state.
[logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ PivotTable sorting appears broken in Explore view.
- ❌ Dashboard PivotTable charts cannot remain sorted by column.
- ⚠️ Users cannot analyze data ordered by metric results.
```
</details>
```suggestion
// Reset sort state when structural props change
useEffect(() => {
setSortingOrder([]);
setActiveSortColumn(null);
setSortedRowKeys(null);
}, [
cols,
rows,
aggregatorName,
tableOptions,
subtotalOptions,
namesMappingProp,
allowRenderHtml,
]);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open any Pivot Table v2 chart in Explore (registered via
`PivotTableChartPluginV2` in
`superset-frontend/src/visualizations/presets/MainPreset.ts:86` which uses
`PivotTableChart.tsx:44` and ultimately
`react-pivottable/PivotTable.tsx:20-27` to render
`TableRenderer`).
2. With the chart rendered, click a sortable column header's sort icon in
the pivot table
UI (sorting handled by `sortData` in `TableRenderers.tsx:760-809`, which
updates
`sortedRowKeys`, `sortingOrder`, and `activeSortColumn` state).
3. React re-renders `TableRenderer` with the same logical props; during
render, the
memoized `props` object is rebuilt in `useMemo` at
`TableRenderers.tsx:300-323`, because
its dependency array includes `restProps`, a new object created on every
render from the
rest parameter in the component signature at `TableRenderers.tsx:276-286`.
4. After this render, the `useEffect` at `TableRenderers.tsx:712-717`, which
depends on
`props`, detects a changed `props` reference on every render and runs,
immediately
resetting `sortingOrder`, `activeSortColumn`, and `sortedRowKeys` back to
their initial
values, so the user never sees a persistent sorted table—the sort appears to
do nothing in
both Explore and dashboards that use `VizType.PivotTable`.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx
**Line:** 712:717
**Comment:**
*Logic Error: The effect that resets the sorting state depends on the
`props` object, which is recreated on every render (because of the
`...restProps` spread), causing the effect to run on every render and
immediately clear `sortingOrder`, `activeSortColumn`, and `sortedRowKeys` after
a sort is triggered, so the user can never see or maintain a sorted 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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=fdba3b5f4cf1a0575996b244a3c75375c2f91985592962182914620289bc0f9f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=fdba3b5f4cf1a0575996b244a3c75375c2f91985592962182914620289bc0f9f&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx:
##########
@@ -32,279 +32,295 @@ export interface DataEntry {
}
interface TTestTableProps {
- alpha: number;
+ alpha?: number;
data: DataEntry[];
groups: string[];
- liftValPrec: number;
+ liftValPrec?: number;
metric: string;
- pValPrec: number;
+ pValPrec?: number;
}
-interface TTestTableState {
- control: number;
- liftValues: (string | number)[];
- pValues: (string | number)[];
-}
-
-const defaultProps = {
- alpha: 0.05,
- liftValPrec: 4,
- pValPrec: 6,
-};
+function TTestTable({
+ alpha = 0.05,
+ data,
+ groups,
+ liftValPrec = 4,
+ metric,
+ pValPrec = 6,
+}: TTestTableProps) {
+ const [control, setControl] = useState(0);
+ const [liftValues, setLiftValues] = useState<(string | number)[]>([]);
+ const [pValues, setPValues] = useState<(string | number)[]>([]);
-class TTestTable extends Component<TTestTableProps, TTestTableState> {
- static defaultProps = defaultProps;
+ const computeLift = useCallback(
+ (values: DataPointValue[], controlValues: DataPointValue[]): string => {
+ // Compute the lift value between two time series
+ let sumValues = 0;
+ let sumControl = 0;
+ values.forEach((value, i) => {
+ sumValues += value.y;
+ sumControl += controlValues[i].y;
+ });
- constructor(props: TTestTableProps) {
- super(props);
- this.state = {
- control: 0,
- liftValues: [],
- pValues: [],
- };
- }
+ return (((sumValues - sumControl) / sumControl) * 100).toFixed(
+ liftValPrec,
+ );
+ },
+ [liftValPrec],
+ );
- componentDidMount() {
- const { control } = this.state;
- this.computeTTest(control); // initially populate table
- }
+ const computePValue = useCallback(
+ (
+ values: DataPointValue[],
+ controlValues: DataPointValue[],
+ ): string | number => {
+ // Compute the p-value from Student's t-test
+ // between two time series
+ let diffSum = 0;
+ let diffSqSum = 0;
+ let finiteCount = 0;
+ values.forEach((value, i) => {
+ const diff = controlValues[i].y - value.y;
+ /* eslint-disable-next-line */
+ if (isFinite(diff)) {
+ finiteCount += 1;
+ diffSum += diff;
+ diffSqSum += diff * diff;
+ }
+ });
+ const tvalue = -Math.abs(
+ diffSum *
+ Math.sqrt(
+ (finiteCount - 1) / (finiteCount * diffSqSum - diffSum * diffSum),
+ ),
+ );
+ try {
+ return (2 * new dist.Studentt(finiteCount - 1).cdf(tvalue)).toFixed(
+ pValPrec,
+ ); // two-sided test
+ } catch (error) {
+ return NaN;
+ }
+ },
+ [pValPrec],
+ );
- getLiftStatus(row: number): string {
- const { control, liftValues } = this.state;
- // Get a css class name for coloring
- if (row === control) {
- return 'control';
- }
- const liftVal = liftValues[row];
- if (Number.isNaN(liftVal) || !Number.isFinite(liftVal)) {
- return 'invalid'; // infinite or NaN values
- }
+ const computeTTest = useCallback(
+ (controlIndex: number) => {
+ // Compute lift and p-values for each row
+ // against the selected control
+ const newPValues: (string | number)[] = [];
+ const newLiftValues: (string | number)[] = [];
+ if (!data) {
+ return;
+ }
+ for (let i = 0; i < data.length; i += 1) {
+ if (i === controlIndex) {
+ newPValues.push('control');
+ newLiftValues.push('control');
+ } else {
+ newPValues.push(
+ computePValue(data[i].values, data[controlIndex].values),
+ );
+ newLiftValues.push(
+ computeLift(data[i].values, data[controlIndex].values),
+ );
+ }
+ }
+ setControl(controlIndex);
+ setLiftValues(newLiftValues);
+ setPValues(newPValues);
+ },
+ [data, computeLift, computePValue],
+ );
- return Number(liftVal) >= 0 ? 'true' : 'false'; // green on true, red on
false
- }
+ // Initially populate table on mount
+ useEffect(() => {
+ computeTTest(control);
+ }, [computeTTest, control]);
Review Comment:
**Suggestion:** When the underlying `data` prop changes to a shorter array
after the user has selected a control row with a higher index, the `useEffect`
hook will call `computeTTest(control)` using an out-of-range `control` index,
leading to `data[control]` being `undefined` inside
`computeLift`/`computePValue` and causing a runtime error when accessing
`.values[i].y`; the effect should clamp or reset the control index based on the
current `data.length` and recompute safely. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Paired t-test chart crashes after filter-driven data shrink.
- ⚠️ Dashboards using PairedTTest tiles may intermittently fail.
```
</details>
```suggestion
// Recompute table when data or control row changes, keeping control index
in range
useEffect(() => {
if (!data || data.length === 0) {
setControl(0);
setLiftValues([]);
setPValues([]);
return;
}
const safeControlIndex = Math.min(control, data.length - 1);
if (safeControlIndex !== control) {
setControl(safeControlIndex);
computeTTest(safeControlIndex);
} else {
computeTTest(control);
}
}, [computeTTest, control, data]);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or open a Paired T-Test chart (viz type `VizType.PairedTTest`) in
Explore or a
dashboard; this viz type is registered via `MainPreset.ts:135` using `new
PairedTTestChartPlugin().configure({ key: VizType.PairedTTest })`, which
loads
`plugins/legacy-plugin-chart-paired-t-test/src/PairedTTest.tsx` through the
chart plugin
in `plugins/legacy-plugin-chart-paired-t-test/src/index.ts:41-44`.
2. When the chart renders, `PairedTTest` (`PairedTTest.tsx:109-139`) maps
metrics to
`<TTestTable>` components (`TTestTable.tsx:43-50`) passing `data[metric]` as
the `data`
prop; `TTestTable` initializes `control` state to `0` (`TTestTable.tsx:51`)
and the effect
at `TTestTable.tsx:137-140` calls `computeTTest(control)` to populate
`liftValues` and
`pValues`.
3. With an initial query where `data[metric]` has multiple rows (length N >
1), click on
the last table row in the rendered t-test table; this triggers
`handleRowClick`
(`TTestTable.tsx:186-191`), which calls `computeTTest(rowIndex)`
(`TTestTable.tsx:108-135`) and sets `control` in state to that high
`rowIndex` (e.g., N -
1).
4. Change chart controls (e.g., filters, groupby, or time range) so that the
next query
result `data[metric]` has fewer rows (length M where M <= previous `control`
index);
Superset re-renders `PairedTTest` with the new `data` while preserving
`TTestTable` state
(same `key` in `PairedTTest.tsx:123-132`), so `control` still holds the old
index. On the
subsequent render, the `useEffect` at `TTestTable.tsx:137-140` runs and calls
`computeTTest(control)` with the out-of-range index. Inside `computeTTest`
(`TTestTable.tsx:108-135`), the loop accesses `data[controlIndex].values`
(lines 122-127);
since `data[controlIndex]` is now `undefined`, this results in a runtime
TypeError
(attempting to read `.values` of `undefined`), causing the Paired T-Test
chart to error
and fail to render.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx
**Line:** 137:140
**Comment:**
*Logic Error: When the underlying `data` prop changes to a shorter
array after the user has selected a control row with a higher index, the
`useEffect` hook will call `computeTTest(control)` using an out-of-range
`control` index, leading to `data[control]` being `undefined` inside
`computeLift`/`computePValue` and causing a runtime error when accessing
`.values[i].y`; the effect should clamp or reset the control index based on the
current `data.length` and recompute safely.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=b0fda6c32b671f9ce44108013884563a4aa9c2f9b710403307aa2924b527c9a0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=b0fda6c32b671f9ce44108013884563a4aa9c2f9b710403307aa2924b527c9a0&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.tsx:
##########
@@ -335,126 +318,110 @@ class Markdown extends PureComponent<MarkdownProps,
MarkdownState> {
onReady={(handle: EditorInstance) => {
// The handle provides access to the underlying editor for resize
if (handle && typeof handle.focus === 'function') {
- this.setEditor(handle);
+ setEditor(handle);
}
}}
data-test="editor"
/>
- );
- }
-
- renderPreviewMode(): JSX.Element {
- const { hasError } = this.state;
+ ),
+ [id, markdownSource, handleMarkdownChange, setEditor],
+ );
- return (
+ const renderPreviewMode = useMemo(
+ () => (
<SafeMarkdown
source={
hasError
? MARKDOWN_ERROR_MESSAGE
- : this.state.markdownSource || MARKDOWN_PLACE_HOLDER
+ : markdownSource || MARKDOWN_PLACE_HOLDER
}
- htmlSanitization={this.props.htmlSanitization}
- htmlSchemaOverrides={this.props.htmlSchemaOverrides}
+ htmlSanitization={htmlSanitization}
+ htmlSchemaOverrides={htmlSchemaOverrides}
/>
- );
- }
-
- render() {
- const { isFocused, editorMode } = this.state;
-
- const {
- component,
- parentComponent,
- index,
- depth,
- availableColumnCount,
- columnWidth,
- onResize,
- onResizeStop,
- handleComponentDrop,
- editMode,
- } = this.props;
-
- // inherit the size of parent columns
- const widthMultiple =
- parentComponent.type === COLUMN_TYPE
- ? parentComponent.meta.width || GRID_MIN_COLUMN_COUNT
- : component.meta.width || GRID_MIN_COLUMN_COUNT;
-
- const isEditing = editorMode === 'edit';
-
- return (
- <Draggable
- component={component}
- parentComponent={parentComponent}
- orientation={parentComponent.type === ROW_TYPE ? 'column' : 'row'}
- index={index}
- depth={depth}
- onDrop={handleComponentDrop}
- disableDragDrop={isFocused}
- editMode={editMode}
- >
- {({ dragSourceRef }: DragChildProps) => (
- <WithPopoverMenu
- onChangeFocus={this.handleChangeFocus}
- shouldFocus={this.shouldFocusMarkdown}
- menuItems={[
- <MarkdownModeDropdown
- key={`${component.id}-mode`}
- id={`${component.id}-mode`}
- value={this.state.editorMode}
- onChange={this.handleChangeEditorMode}
- />,
- ]}
- editMode={editMode}
+ ),
Review Comment:
**Suggestion:** Inside the Markdown component, the `hasError` flag is
checked in the preview rendering but never set to `true`, so even when an error
occurs the user will never see `MARKDOWN_ERROR_MESSAGE`; adding an
`ErrorBoundary` around the preview and an `onError` handler that sets
`hasError` and shows the danger toast restores functional error handling.
[logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Markdown preview fallback message is currently unreachable on errors.
- ⚠️ Users lose guidance to revert problematic markdown content.
- ⚠️ Error handling inconsistent with Chart and similar components.
```
</details>
```suggestion
const handleRenderError = useCallback(
(error: Error, info: { componentStack: string } | null): void => {
setHasError(true);
if (editorMode === 'preview') {
addDangerToast(
t(
'This markdown component has an error. Please revert your recent
changes.',
),
);
}
},
[addDangerToast, editorMode],
);
const renderEditMode = useMemo(
() => (
<EditorHost
id={`markdown-editor-${id}`}
onChange={handleMarkdownChange}
width="100%"
height="100%"
value={
// this allows "select all => delete" to give an empty editor
typeof markdownSource === 'string'
? markdownSource
: MARKDOWN_PLACE_HOLDER
}
language="markdown"
readOnly={false}
lineNumbers={false}
wordWrap
onReady={(handle: EditorInstance) => {
// The handle provides access to the underlying editor for resize
if (handle && typeof handle.focus === 'function') {
setEditor(handle);
}
}}
data-test="editor"
/>
),
[id, markdownSource, handleMarkdownChange, setEditor],
);
const renderPreviewMode = useMemo(
() => (
<ErrorBoundary onError={handleRenderError} showMessage={false}>
<SafeMarkdown
source={
hasError
? MARKDOWN_ERROR_MESSAGE
: markdownSource || MARKDOWN_PLACE_HOLDER
}
htmlSanitization={htmlSanitization}
htmlSchemaOverrides={htmlSchemaOverrides}
/>
</ErrorBoundary>
),
[
hasError,
markdownSource,
htmlSanitization,
htmlSchemaOverrides,
handleRenderError,
],
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a dashboard containing a markdown cell so that the `Markdown`
component in
`superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.tsx`
is used
via the `MARKDOWN_TYPE` mapping in
`src/dashboard/components/gridComponents/index.ts:43`
and type definition in `src/dashboard/util/componentTypes.ts:26`. In view
mode
(`editMode=false`), the component renders its preview using the
`renderPreviewMode` memo
at `Markdown.tsx:330–342`.
2. Note that the preview rendering logic selects between a fallback message
and the
markdown content based on the `hasError` flag: `source={hasError ?
MARKDOWN_ERROR_MESSAGE
: markdownSource || MARKDOWN_PLACE_HOLDER}` at `Markdown.tsx:332–337`, where
`MARKDOWN_ERROR_MESSAGE` is defined at `Markdown.tsx:93` as `t('This
markdown component
has an error.')`. However, `hasError` is initialized to `false` in state at
line 161 and
only ever set back to `false` in the undo/redo `useEffect` (line 177) and
when switching
to preview mode at line 252; there is no `setHasError(true)` anywhere in the
file.
3. Cause a runtime error in the markdown rendering path—for example, by
introducing a bug
in `SafeMarkdown` (imported at `Markdown.tsx:26` and used at
`Markdown.tsx:332–340`) or by
triggering an unexpected error in custom markdown content—so that the
preview render
fails. Because there is no `ErrorBoundary` around `SafeMarkdown` in
`renderPreviewMode`
and no `try/catch` logic in `Markdown.tsx`, the error will bubble to an
ancestor boundary
(such as the `ErrorBoundary` in `src/views/App.tsx:94–100` or the
dashboard-level
boundary) rather than being handled within this component. The internal
`hasError` flag
remains `false`, so the component never switches to `MARKDOWN_ERROR_MESSAGE`.
4. Compare this behavior with both the rest of the codebase and prior
expectations: the
translations file
`superset/translations/en/LC_MESSAGES/messages.po:10989–10992` contains
both `"This markdown component has an error."` and `"This markdown component
has an error.
Please revert your recent changes."`, indicating a design where markdown
errors show a
specific fallback message and guidance. The Chart component at
`src/components/Chart/Chart.tsx:458–469` wraps its render in an
`ErrorBoundary` with an
`onError` handler that logs and reports errors. In `Markdown.tsx`, by
contrast,
`addDangerToast` (line 75) is never called and `hasError` is never set to
`true`, so
markdown-specific error handling is effectively disabled. The suggested
change—adding a
`handleRenderError` callback that calls `setHasError(true)` and
`addDangerToast(...)`, and
wrapping `SafeMarkdown` in `<ErrorBoundary onError={handleRenderError}
showMessage={false}>`—aligns Markdown error handling with the established
pattern and
makes the fallback message reachable.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.tsx
**Line:** 301:341
**Comment:**
*Logic Error: Inside the Markdown component, the `hasError` flag is
checked in the preview rendering but never set to `true`, so even when an error
occurs the user will never see `MARKDOWN_ERROR_MESSAGE`; adding an
`ErrorBoundary` around the preview and an `onError` handler that sets
`hasError` and shows the danger toast restores functional error handling.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=46d9fb4d24e7f3fe49de079247aab328e7676eded8ec9c0f577804938ca5d479&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=46d9fb4d24e7f3fe49de079247aab328e7676eded8ec9c0f577804938ca5d479&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/controls/CollectionControl/index.tsx:
##########
@@ -78,137 +67,158 @@ const SortableDragger = SortableHandle(() => (
/>
));
-class CollectionControl extends Component<CollectionControlProps> {
- static defaultProps = defaultProps;
+const defaultItemGenerator = () => ({ key: nanoid(11) });
+const defaultKeyAccessor = (o: CollectionItem) => o.key ?? '';
- constructor(props: CollectionControlProps) {
- super(props);
- this.onAdd = this.onAdd.bind(this);
- }
+export default function CollectionControl({
+ name,
+ label = null,
+ description = null,
+ placeholder = t('Empty collection'),
+ addTooltip = t('Add an item'),
+ itemGenerator = defaultItemGenerator,
+ keyAccessor = defaultKeyAccessor,
+ onChange = () => {},
+ value = [],
+ isFloat,
+ isInt,
+ controlName,
+}: CollectionControlProps) {
+ const theme = useTheme();
- onChange(i: number, value: CollectionItem) {
- const currentValue = this.props.value ?? [];
- const newValue = [...currentValue];
- newValue[i] = { ...currentValue[i], ...value };
- this.props.onChange?.(newValue);
- }
+ const handleChange = useCallback(
+ (i: number, itemValue: CollectionItem) => {
+ const newValue = [...value];
+ newValue[i] = { ...value[i], ...itemValue };
+ onChange(newValue);
+ },
+ [value, onChange],
+ );
- onAdd() {
- const currentValue = this.props.value ?? [];
- const newItem = this.props.itemGenerator?.();
+ const handleAdd = useCallback(() => {
+ const newItem = itemGenerator();
// Cast needed: original JS allowed undefined items from itemGenerator
- this.props.onChange?.(
- currentValue.concat([newItem] as unknown as CollectionItem[]),
- );
- }
+ onChange(value.concat([newItem] as unknown as CollectionItem[]));
+ }, [value, onChange, itemGenerator]);
- onSortEnd({ oldIndex, newIndex }: { oldIndex: number; newIndex: number }) {
- const currentValue = this.props.value ?? [];
- this.props.onChange?.(arrayMove(currentValue, oldIndex, newIndex));
- }
+ const handleSortEnd = useCallback(
+ ({ oldIndex, newIndex }: { oldIndex: number; newIndex: number }) => {
+ onChange(arrayMove(value, oldIndex, newIndex));
+ },
+ [value, onChange],
+ );
- removeItem(i: number) {
- const currentValue = this.props.value ?? [];
- this.props.onChange?.(currentValue.filter((o, ix) => i !== ix));
- }
+ const removeItem = useCallback(
+ (i: number) => {
+ onChange(value.filter((o, ix) => i !== ix));
+ },
+ [value, onChange],
+ );
- renderList() {
- const currentValue = this.props.value ?? [];
- if (currentValue.length === 0) {
- return <div className="text-muted">{this.props.placeholder}</div>;
+ const renderList = () => {
+ if (value.length === 0) {
+ return <div className="text-muted">{placeholder}</div>;
}
const Control = (controlMap as Record<string, React.ComponentType<any>>)[
- this.props.controlName
+ controlName
];
- const keyAccessor =
- this.props.keyAccessor ?? ((o: CollectionItem) => o.key ?? '');
return (
<SortableList
useDragHandle
lockAxis="y"
- onSortEnd={this.onSortEnd.bind(this)}
+ onSortEnd={handleSortEnd}
bordered
- css={(theme: SupersetTheme) => ({
- borderRadius: theme.borderRadius,
+ css={(themeArg: SupersetTheme) => ({
+ borderRadius: themeArg.borderRadius,
})}
>
- {currentValue.map((o: CollectionItem, i: number) => {
- // label relevant only for header, not here
- const { label, theme, ...commonProps } = this.props;
- return (
- <SortableListItem
- selectable={false}
- className="clearfix"
- css={(theme: SupersetTheme) => ({
- alignItems: 'center',
- justifyContent: 'flex-start',
- display: 'flex',
- paddingInline: theme.sizeUnit * 6,
+ {value.map((o: CollectionItem, i: number) => (
+ <SortableListItem
+ selectable={false}
+ className="clearfix"
+ css={(themeArg: SupersetTheme) => ({
+ alignItems: 'center',
+ justifyContent: 'flex-start',
+ display: 'flex',
+ paddingInline: themeArg.sizeUnit * 6,
+ })}
+ key={keyAccessor(o)}
+ index={i}
+ >
+ <SortableDragger />
+ <div
+ css={(themeArg: SupersetTheme) => ({
+ flex: 1,
+ marginLeft: themeArg.sizeUnit * 2,
+ marginRight: themeArg.sizeUnit * 2,
})}
- key={keyAccessor(o)}
- index={i}
>
- <SortableDragger />
- <div
- css={(theme: SupersetTheme) => ({
- flex: 1,
- marginLeft: theme.sizeUnit * 2,
- marginRight: theme.sizeUnit * 2,
- })}
- >
- <Control
- {...commonProps}
- {...o}
- onChange={this.onChange.bind(this, i)}
- />
- </div>
- <IconTooltip
- className="pointer"
- placement="right"
- onClick={this.removeItem.bind(this, i)}
- tooltip={t('Remove item')}
- mouseEnterDelay={0}
- mouseLeaveDelay={0}
- css={(theme: SupersetTheme) => ({
- padding: 0,
- minWidth: 'auto',
- height: 'auto',
- lineHeight: 1,
- cursor: 'pointer',
- '& svg path': {
- fill: theme.colorIcon,
- transition: `fill ${theme.motionDurationMid} ease-out`,
- },
- '&:hover svg path': {
- fill: theme.colorError,
- },
- })}
- >
- <Icons.CloseOutlined iconSize="s" />
- </IconTooltip>
- </SortableListItem>
- );
- })}
+ <Control
+ name={name}
+ description={description}
+ placeholder={placeholder}
+ addTooltip={addTooltip}
+ itemGenerator={itemGenerator}
+ keyAccessor={keyAccessor}
+ value={value}
+ isFloat={isFloat}
+ isInt={isInt}
+ controlName={controlName}
+ {...o}
+ onChange={(itemValue: CollectionItem) =>
+ handleChange(i, itemValue)
+ }
+ />
+ </div>
+ <IconTooltip
+ className="pointer"
+ placement="right"
+ onClick={() => removeItem(i)}
+ tooltip={t('Remove item')}
+ mouseEnterDelay={0}
+ mouseLeaveDelay={0}
+ css={(themeArg: SupersetTheme) => ({
+ padding: 0,
+ minWidth: 'auto',
+ height: 'auto',
+ lineHeight: 1,
+ cursor: 'pointer',
+ '& svg path': {
+ fill: themeArg.colorIcon,
+ transition: `fill ${themeArg.motionDurationMid} ease-out`,
+ },
+ '&:hover svg path': {
+ fill: themeArg.colorError,
+ },
+ })}
+ >
+ <Icons.CloseOutlined iconSize="s" />
+ </IconTooltip>
+ </SortableListItem>
+ ))}
</SortableList>
);
- }
+ };
- render() {
- return (
- <div data-test="CollectionControl" className="CollectionControl">
- <HeaderContainer>
- <ControlHeader {...this.props} />
- <AddIconButton onClick={this.onAdd}>
- <Icons.PlusOutlined
- iconSize="s"
- iconColor={this.props.theme.colorTextLightSolid}
- />
- </AddIconButton>
- </HeaderContainer>
- {this.renderList()}
- </div>
- );
- }
-}
+ // Props for ControlHeader (excluding label and theme which are handled
separately)
+ const controlHeaderProps = {
+ name,
+ label,
Review Comment:
**Suggestion:** The functional refactor of the collection control no longer
forwards control-header–related props (like `validationErrors`, `hovered`,
`renderTrigger`, `warning`, etc.) down to `ControlHeader`, so this control will
not display validation errors or other header UI state even when the parent
passes them, unlike the previous class implementation which spread all props
into `ControlHeader`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Collection-type Explore controls lose per-control error icons.
- ⚠️ Hover-only header icons never appear for collection controls.
- ⚠️ Users must infer invalid collections from global messages.
- ⚠️ Behavior diverges from other controls using ControlHeader.
```
</details>
```suggestion
...headerProps
}: CollectionControlProps & { [key: string]: unknown }) {
const theme = useTheme();
const handleChange = useCallback(
(i: number, itemValue: CollectionItem) => {
const newValue = [...value];
newValue[i] = { ...value[i], ...itemValue };
onChange(newValue);
},
[value, onChange],
);
const handleAdd = useCallback(() => {
const newItem = itemGenerator();
// Cast needed: original JS allowed undefined items from itemGenerator
onChange(value.concat([newItem] as unknown as CollectionItem[]));
}, [value, onChange, itemGenerator]);
const handleSortEnd = useCallback(
({ oldIndex, newIndex }: { oldIndex: number; newIndex: number }) => {
onChange(arrayMove(value, oldIndex, newIndex));
},
[value, onChange],
);
const removeItem = useCallback(
(i: number) => {
onChange(value.filter((o, ix) => i !== ix));
},
[value, onChange],
);
const renderList = () => {
if (value.length === 0) {
return <div className="text-muted">{placeholder}</div>;
}
const Control = (controlMap as Record<string, React.ComponentType<any>>)[
controlName
];
return (
<SortableList
useDragHandle
lockAxis="y"
onSortEnd={handleSortEnd}
bordered
css={(themeArg: SupersetTheme) => ({
borderRadius: themeArg.borderRadius,
})}
>
{value.map((o: CollectionItem, i: number) => (
<SortableListItem
selectable={false}
className="clearfix"
css={(themeArg: SupersetTheme) => ({
alignItems: 'center',
justifyContent: 'flex-start',
display: 'flex',
paddingInline: themeArg.sizeUnit * 6,
})}
key={keyAccessor(o)}
index={i}
>
<SortableDragger />
<div
css={(themeArg: SupersetTheme) => ({
flex: 1,
marginLeft: themeArg.sizeUnit * 2,
marginRight: themeArg.sizeUnit * 2,
})}
>
<Control
name={name}
description={description}
placeholder={placeholder}
addTooltip={addTooltip}
itemGenerator={itemGenerator}
keyAccessor={keyAccessor}
value={value}
isFloat={isFloat}
isInt={isInt}
controlName={controlName}
{...o}
onChange={(itemValue: CollectionItem) =>
handleChange(i, itemValue)
}
/>
</div>
<IconTooltip
className="pointer"
placement="right"
onClick={() => removeItem(i)}
tooltip={t('Remove item')}
mouseEnterDelay={0}
mouseLeaveDelay={0}
css={(themeArg: SupersetTheme) => ({
padding: 0,
minWidth: 'auto',
height: 'auto',
lineHeight: 1,
cursor: 'pointer',
'& svg path': {
fill: themeArg.colorIcon,
transition: `fill ${themeArg.motionDurationMid} ease-out`,
},
'&:hover svg path': {
fill: themeArg.colorError,
},
})}
>
<Icons.CloseOutlined iconSize="s" />
</IconTooltip>
</SortableListItem>
))}
</SortableList>
);
};
// Props for ControlHeader, including any header-related props passed from
the parent
const controlHeaderProps = {
name,
label,
description,
...headerProps,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Note how generic Explore controls are wired: `ControlPanelsContainer`
creates props for
each control, including `validationErrors`, and passes them into the generic
`Control`
component at `src/explore/components/ControlPanelsContainer.tsx` (e.g.
`validationErrors={validationErrors}` around lines 573 and 693 from Grep
output).
2. `Control` then renders the specific control component
(`CollectionControl` among
others) and forwards header-related props plus `hovered` down via JSX spread:
`ControlComponent onChange={onChange} hovered={hovered} {...props}` at
`src/explore/components/Control.tsx:120` (from Grep), so `CollectionControl`
receives
`validationErrors`, `hovered`, `renderTrigger`, etc.
3. In tests, `CollectionControl` is instantiated with `validationErrors` and
`hovered` to
mirror real Explore usage: see `createProps()` in
`src/explore/components/controls/CollectionControl/CollectionControl.test.tsx:50-84`,
where `hovered: false` (line 73) and `validationErrors: []` (line 81) are
passed into
`<CollectionControl {...props} />` (lines 87–88).
4. In the refactored implementation at
`src/explore/components/controls/CollectionControl/index.tsx:73-86`, the
function
destructures only named props (`name`, `label`, `description`, etc.) and
later builds
`controlHeaderProps` with just `{ name, label, description }` (lines
203–208), so any
`validationErrors`, `hovered`, `renderTrigger`, or `warning` props coming
from
`Control.tsx` are dropped and never forwarded into `ControlHeader`, which
expects those
props at `src/explore/components/ControlHeader.tsx:27-40`; as a result, when
a
CollectionControl has validation errors (e.g. for a Time Table control using
`{ type:
'CollectionControl' }` defined in
`src/visualizations/TimeTable/config/controlPanel/controlPanel.ts:42` per
Grep), the
per-control error icon and hover-triggered icons in the header are not
rendered even
though `validationErrors` are set upstream.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/controls/CollectionControl/index.tsx
**Line:** 86:206
**Comment:**
*Logic Error: The functional refactor of the collection control no
longer forwards control-header–related props (like `validationErrors`,
`hovered`, `renderTrigger`, `warning`, etc.) down to `ControlHeader`, so this
control will not display validation errors or other header UI state even when
the parent passes them, unlike the previous class implementation which spread
all props into `ControlHeader`.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=a59a0dbd9b686c54cfd44b6843827692b460fa2ace08a50956b63a7a63fbb1d8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=a59a0dbd9b686c54cfd44b6843827692b460fa2ace08a50956b63a7a63fbb1d8&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/components/SliceAdder.tsx:
##########
@@ -173,295 +163,265 @@ function getFilteredSortedSlices(
.filter(createFilter(searchTerm, KEYS_TO_FILTERS))
.sort(sortByComparator(sortBy));
}
-class SliceAdder extends Component<SliceAdderProps, SliceAdderState> {
- private slicesRequest?: AbortController | Promise<void>;
-
- static defaultProps = {
- selectedSliceIds: [],
- editMode: false,
- errorMessage: '',
- };
-
- constructor(props: SliceAdderProps) {
- super(props);
- this.state = {
- filteredSlices: [],
- searchTerm: '',
- sortBy: DEFAULT_SORT_KEY,
- selectedSliceIdsSet: new Set(props.selectedSliceIds),
- showOnlyMyCharts: getItem(
- LocalStorageKeys.DashboardEditorShowOnlyMyCharts,
- true,
- ),
- };
- this.rowRenderer = this.rowRenderer.bind(this);
- this.searchUpdated = this.searchUpdated.bind(this);
- this.handleSelect = this.handleSelect.bind(this);
- this.userIdForFetch = this.userIdForFetch.bind(this);
- this.onShowOnlyMyCharts = this.onShowOnlyMyCharts.bind(this);
- }
- userIdForFetch() {
- return this.state.showOnlyMyCharts ? this.props.userId : undefined;
- }
-
- componentDidMount() {
- this.slicesRequest = this.props.fetchSlices(
- this.userIdForFetch(),
- '',
- this.state.sortBy,
- );
- }
-
- componentDidUpdate(prevProps: SliceAdderProps) {
- const nextState: SliceAdderState = {} as SliceAdderState;
- if (this.props.lastUpdated !== prevProps.lastUpdated) {
- nextState.filteredSlices = getFilteredSortedSlices(
- this.props.slices,
- this.state.searchTerm,
- this.state.sortBy,
- this.state.showOnlyMyCharts,
- this.props.userId,
- );
- }
-
- if (prevProps.selectedSliceIds !== this.props.selectedSliceIds) {
- nextState.selectedSliceIdsSet = new Set(this.props.selectedSliceIds);
- }
-
- if (Object.keys(nextState).length) {
- this.setState(nextState);
- }
- }
-
- componentWillUnmount() {
- // Clears the redux store keeping only selected items
- const selectedSlices = pickBy(this.props.slices, (value: Slice) =>
- this.state.selectedSliceIdsSet.has(value.slice_id),
- );
-
- this.props.updateSlices(selectedSlices);
- if (this.slicesRequest instanceof AbortController) {
- this.slicesRequest.abort();
- }
- }
-
- handleChange = debounce(value => {
- this.searchUpdated(value);
- this.slicesRequest = this.props.fetchSlices(
- this.userIdForFetch(),
- value,
- this.state.sortBy,
- );
- }, 300);
-
- searchUpdated(searchTerm: string) {
- this.setState(prevState => ({
- searchTerm,
- filteredSlices: getFilteredSortedSlices(
- this.props.slices,
+function SliceAdder({
+ fetchSlices,
+ updateSlices,
+ isLoading,
+ slices,
+ errorMessage = '',
+ userId,
+ selectedSliceIds = [],
+ editMode = false,
+ dashboardId,
+}: SliceAdderProps) {
+ const theme = useTheme();
+ const slicesRequestRef = useRef<AbortController | Promise<void>>();
+
+ const [searchTerm, setSearchTerm] = useState('');
+ const [sortBy, setSortBy] = useState<keyof Slice>(DEFAULT_SORT_KEY);
+ const [selectedSliceIdsSet, setSelectedSliceIdsSet] = useState(
+ () => new Set(selectedSliceIds),
+ );
+ const [showOnlyMyCharts, setShowOnlyMyCharts] = useState(() =>
+ getItem(LocalStorageKeys.DashboardEditorShowOnlyMyCharts, true),
+ );
+
+ const filteredSlices = useMemo(
+ () =>
+ getFilteredSortedSlices(
+ slices,
searchTerm,
- prevState.sortBy,
- prevState.showOnlyMyCharts,
- this.props.userId,
- ),
- }));
- }
-
- handleSelect(sortBy: keyof Slice) {
- this.setState(prevState => ({
- sortBy,
- filteredSlices: getFilteredSortedSlices(
- this.props.slices,
- prevState.searchTerm,
sortBy,
- prevState.showOnlyMyCharts,
- this.props.userId,
- ),
- }));
- this.slicesRequest = this.props.fetchSlices(
- this.userIdForFetch(),
- this.state.searchTerm,
- sortBy,
- );
- }
-
- rowRenderer({ index, style }: { index: number; style: React.CSSProperties })
{
- const { filteredSlices, selectedSliceIdsSet } = this.state;
- const cellData = filteredSlices[index];
-
- const isSelected = selectedSliceIdsSet.has(cellData.slice_id);
- const type = CHART_TYPE;
- const id = NEW_CHART_ID;
-
- const meta = {
- chartId: cellData.slice_id,
- sliceName: cellData.slice_name,
- };
- return (
- <DragDroppable
- key={cellData.slice_id}
- component={{ type, id, meta }}
- parentComponent={{
- id: NEW_COMPONENTS_SOURCE_ID,
- type: NEW_COMPONENT_SOURCE_TYPE,
- }}
- index={index}
- depth={0}
- disableDragDrop={isSelected}
- editMode={this.props.editMode}
- // we must use a custom drag preview within the List because
- // it does not seem to work within a fixed-position container
- useEmptyDragPreview
- // List library expect style props here
- // actual style should be applied to nested AddSliceCard component
- style={{}}
- >
- {({ dragSourceRef }: { dragSourceRef: ConnectDragSource }) => (
- <AddSliceCard
- innerRef={dragSourceRef}
- style={style}
- sliceName={cellData.slice_name}
- lastModified={cellData.changed_on_humanized}
- visType={cellData.viz_type}
- datasourceUrl={cellData.datasource_url}
- datasourceName={cellData.datasource_name}
- thumbnailUrl={cellData.thumbnail_url}
- isSelected={isSelected}
- />
- )}
- </DragDroppable>
- );
- }
-
- onShowOnlyMyCharts = (showOnlyMyCharts: boolean) => {
- if (!showOnlyMyCharts) {
- this.slicesRequest = this.props.fetchSlices(
- undefined,
- this.state.searchTerm,
- this.state.sortBy,
- );
- }
- this.setState(prevState => ({
- showOnlyMyCharts,
- filteredSlices: getFilteredSortedSlices(
- this.props.slices,
- prevState.searchTerm,
- prevState.sortBy,
showOnlyMyCharts,
- this.props.userId,
+ userId,
),
- }));
- setItem(LocalStorageKeys.DashboardEditorShowOnlyMyCharts,
showOnlyMyCharts);
- };
+ [slices, searchTerm, sortBy, showOnlyMyCharts, userId],
+ );
+
+ const userIdForFetch = useCallback(
+ () => (showOnlyMyCharts ? userId : undefined),
+ [showOnlyMyCharts, userId],
+ );
+
+ // componentDidMount
+ useEffect(() => {
+ slicesRequestRef.current = fetchSlices(userIdForFetch(), '', sortBy);
+ // Only run on mount
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, []);
+
+ // Update selectedSliceIdsSet when selectedSliceIds prop changes
+ useEffect(() => {
+ setSelectedSliceIdsSet(new Set(selectedSliceIds));
+ }, [selectedSliceIds]);
+
+ // componentWillUnmount
+ useEffect(
+ () => () => {
+ // Clears the redux store keeping only selected items
+ const selectedSlices = pickBy(slices, (value: Slice) =>
+ selectedSliceIdsSet.has(value.slice_id),
+ );
- render() {
- const { theme } = this.props;
- return (
+ updateSlices(selectedSlices);
+ if (slicesRequestRef.current instanceof AbortController) {
+ slicesRequestRef.current.abort();
+ }
+ },
+ // Only run on unmount - capture current values
+ // eslint-disable-next-line react-hooks/exhaustive-deps
Review Comment:
**Suggestion:** The cleanup effect in the function component is declared
with an empty dependency array and directly closes over `slices` and
`selectedSliceIdsSet`, so on unmount it will use only their initial values
instead of the latest ones, causing the Redux store to be updated with an
out-of-date set of slices and selections. This can drop newly selected charts
or reintroduce stale ones when the component unmounts. Use refs to track the
latest `slices` and `selectedSliceIdsSet` and read from those refs in the
unmount cleanup so that the final state reflects the user's current selections.
[logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dashboard edit sessions lose metadata for newly added charts.
- ⚠️ Redux sliceEntities.slices diverges from actual dashboard sliceIds.
```
</details>
```suggestion
const latestSlicesRef = useRef(slices);
const latestSelectedSliceIdsSetRef = useRef(selectedSliceIdsSet);
useEffect(() => {
latestSlicesRef.current = slices;
}, [slices]);
useEffect(() => {
latestSelectedSliceIdsSetRef.current = selectedSliceIdsSet;
}, [selectedSliceIdsSet]);
const filteredSlices = useMemo(
() =>
getFilteredSortedSlices(
slices,
searchTerm,
sortBy,
showOnlyMyCharts,
userId,
),
[slices, searchTerm, sortBy, showOnlyMyCharts, userId],
);
const userIdForFetch = useCallback(
() => (showOnlyMyCharts ? userId : undefined),
[showOnlyMyCharts, userId],
);
// componentDidMount
useEffect(() => {
slicesRequestRef.current = fetchSlices(userIdForFetch(), '', sortBy);
// Only run on mount
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
// Update selectedSliceIdsSet when selectedSliceIds prop changes
useEffect(() => {
setSelectedSliceIdsSet(new Set(selectedSliceIds));
}, [selectedSliceIds]);
// componentWillUnmount
useEffect(
() => () => {
// Clears the redux store keeping only selected items
const selectedSlices = pickBy(
latestSlicesRef.current,
(value: Slice) =>
latestSelectedSliceIdsSetRef.current.has(value.slice_id),
);
updateSlices(selectedSlices);
if (slicesRequestRef.current instanceof AbortController) {
slicesRequestRef.current.abort();
}
},
[updateSlices],
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open any dashboard in edit mode so that `DashboardBuilder` renders
`BuilderComponentPane` (see
`superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:707`,
where `<BuilderComponentPane topOffset={barTopOffset} />` is rendered when
`editMode` is
true).
2. In the builder side pane, open the "Charts" tab which renders the
connected
`SliceAdder` container (see
`superset-frontend/src/dashboard/components/BuilderComponentPane/index.tsx:82-88`,
where
`<SliceAdder />` is used, and
`superset-frontend/src/dashboard/containers/SliceAdder.tsx:29-43`, which maps
`sliceEntities.slices` and `dashboardState.sliceIds` to the `slices` and
`selectedSliceIds` props of `SliceAdder`).
3. While staying in edit mode, change the available slices and selected
slice IDs after
the initial mount, e.g. search/filter charts (triggering `fetchSlices`) or
update the
dashboard layout so that `dashboardState.sliceIds` changes; `SliceAdder`'s
internal state
`selectedSliceIdsSet` is updated via the effect at `SliceAdder.tsx:214-217`,
and `slices`
from Redux is updated by `fetchSlices` (see
`superset-frontend/src/dashboard/actions/sliceEntities.ts:124-193`).
4. Exit edit mode or navigate away from the dashboard so that
`BuilderComponentPane` (and
thus `SliceAdder`) unmounts; on unmount, the cleanup effect at
`SliceAdder.tsx:219-235`
runs with an empty dependency array, so its closure still references the
initial `slices`
and the initial `selectedSliceIdsSet` from mount time. It calls
`updateSlices(selectedSlices)` (wired to `sliceEntities.updateSlices` in
`superset-frontend/src/dashboard/actions/sliceEntities.ts:118-122`), which
overwrites
`state.sliceEntities.slices` with a map based on stale IDs. Any slices added
or removed
after mount are ignored, so Redux ends up with an out-of-date slice map used
by other
components (e.g. chart rendering and filter code that read
`state.sliceEntities.slices` as
seen in
`superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx` and
`superset-frontend/src/dashboard/components/nativeFilters/FilterBar/utils.ts`),
leading to
newly added charts lacking slice metadata or removed charts lingering in
state.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/SliceAdder.tsx
**Line:** 190:233
**Comment:**
*Logic Error: The cleanup effect in the function component is declared
with an empty dependency array and directly closes over `slices` and
`selectedSliceIdsSet`, so on unmount it will use only their initial values
instead of the latest ones, causing the Redux store to be updated with an
out-of-date set of slices and selections. This can drop newly selected charts
or reintroduce stale ones when the component unmounts. Use refs to track the
latest `slices` and `selectedSliceIdsSet` and read from those refs in the
unmount cleanup so that the final state reflects the user's current selections.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=cd38c6ee766dc5e5374df8ea8755e99108a653d6114504bb223144c62f9cdd0c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=cd38c6ee766dc5e5374df8ea8755e99108a653d6114504bb223144c62f9cdd0c&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/legacy-plugin-chart-horizon/src/HorizonRow.tsx:
##########
@@ -52,162 +50,140 @@ interface HorizonRowProps {
yDomain?: [number, number];
}
-const defaultProps: Partial<HorizonRowProps> = {
- className: '',
- width: 800,
- height: 20,
- bands: DEFAULT_COLORS.length >> 1,
- colors: DEFAULT_COLORS,
- colorScale: 'series',
- mode: 'offset',
- offsetX: 0,
- title: '',
- yDomain: undefined,
-};
-
-class HorizonRow extends PureComponent<HorizonRowProps> {
- static defaultProps = defaultProps;
-
- private canvas: HTMLCanvasElement | null = null;
-
- componentDidMount() {
- this.drawChart();
- }
-
- componentDidUpdate() {
- this.drawChart();
- }
-
- componentWillUnmount() {
- this.canvas = null;
- }
-
- drawChart() {
- if (this.canvas) {
- const {
- data: rawData,
- yDomain,
- width = 800,
- height = 20,
- bands = DEFAULT_COLORS.length >> 1,
- colors = DEFAULT_COLORS,
- colorScale,
- offsetX = 0,
- mode,
- } = this.props;
-
- const data =
- colorScale === 'change'
- ? rawData.map(d => ({ ...d, y: d.y - rawData[0].y }))
- : rawData;
-
- const context = this.canvas.getContext('2d');
- if (!context) return;
- context.imageSmoothingEnabled = false;
- context.clearRect(0, 0, width, height);
- // Reset transform
- context.setTransform(1, 0, 0, 1, 0, 0);
- context.translate(0.5, 0.5);
-
- const step = width / data.length;
- // the data frame currently being shown:
- const startIndex = Math.floor(Math.max(0, -(offsetX / step)));
- const endIndex = Math.floor(
- Math.min(data.length, startIndex + width / step),
- );
-
- // skip drawing if there's no data to be drawn
- if (startIndex > data.length) {
- return;
+function HorizonRow({
+ className = '',
+ width = 800,
+ height = 20,
+ data: rawData,
+ bands = DEFAULT_COLORS.length >> 1,
+ colors = DEFAULT_COLORS,
+ colorScale = 'series',
+ mode = 'offset',
+ offsetX = 0,
+ title = '',
+ yDomain,
+}: HorizonRowProps) {
+ const canvasRef = useRef<HTMLCanvasElement | null>(null);
+
+ const drawChart = useCallback(() => {
+ const canvas = canvasRef.current;
+ if (!canvas) return;
+
+ const data =
+ colorScale === 'change'
Review Comment:
**Suggestion:** When the color scale is set to "change" and the input data
array is empty, accessing `rawData[0].y` will throw at runtime because
`rawData[0]` is `undefined`; guarding against an empty array before using the
first element prevents this crash while preserving existing behavior for
non-empty data. [null pointer]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Horizon chart crashes when a series has no values.
- ❌ Affects Horizon charts using the "change" color scale.
- ⚠️ Can break dashboards containing such Horizon visualizations.
```
</details>
```suggestion
colorScale === 'change' && rawData.length > 0
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In the Superset Explore view, select the Horizon chart type, which is
wired via
`HorizonChartPlugin` in
`superset-frontend/src/visualizations/presets/MainPreset.ts` and
implemented by `HorizonChart` in
`plugins/legacy-plugin-chart-horizon/src/HorizonChart.tsx:70-81`, ultimately
rendering
`HorizonRow` from `HorizonRow.tsx:53-65`.
2. In the Horizon chart control panel, set the "Horizon color scale" control
(`name:
'horizon_color_scale'` defined in
`plugins/legacy-plugin-chart-horizon/src/controlPanel.ts:83`) to the
`change` option so
that `transformProps` in
`plugins/legacy-plugin-chart-horizon/src/transformProps.ts:21-37`
forwards `horizon_color_scale` as `colorScale: 'change'` to `HorizonChart`.
3. Run or filter a query so that at least one series in
`queriesData[0].data` (passed
through `transformProps` as `data` to `HorizonChart` at
`HorizonChart.tsx:70-81`) contains
an empty `values` array; this results in a `row` where `row.values` is `[]`,
which
`HorizonChart` passes directly as the `data` prop (`rawData`) to
`HorizonRow` in
`HorizonRow.tsx:103-110`.
4. When the chart renders, `HorizonRow`'s `drawChart` function in
`HorizonRow.tsx:68-75`
executes `rawData.map(d => ({ ...d, y: d.y - rawData[0].y }))` with
`colorScale ===
'change'` and `rawData === []`, causing `rawData[0]` to be `undefined` and
throwing a
runtime TypeError when accessing `rawData[0].y`, which breaks rendering of
that Horizon
chart visualization.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-plugin-chart-horizon/src/HorizonRow.tsx
**Line:** 73:73
**Comment:**
*Null Pointer: When the color scale is set to "change" and the input
data array is empty, accessing `rawData[0].y` will throw at runtime because
`rawData[0]` is `undefined`; guarding against an empty array before using the
first element prevents this crash while preserving existing behavior for
non-empty data.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=7d50fc75354249cb1a6a605df3610ca4bc8d15a1a1a03d50a717f1a43812c2eb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=7d50fc75354249cb1a6a605df3610ca4bc8d15a1a1a03d50a717f1a43812c2eb&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -728,25 +757,23 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
id="btn_modal_save_goto_dash"
buttonSize="small"
disabled={
- !this.state.newSliceName ||
- !this.state.dashboard ||
- (this.props.datasource?.type !== DatasourceType.Table &&
- !this.state.datasetName)
+ !newSliceName ||
Review Comment:
**Suggestion:** The "Save & go to dashboard" button does not take the
loading state into account when determining its disabled state, so while a save
operation is in progress `saveOrOverwrite(true)` can be triggered multiple
times by repeated clicks, causing duplicate saves/updates and inconsistent
state; this is especially problematic because the plain "Save" button is
correctly disabled by `isLoading`, so the behavior is inconsistent. You should
add `isLoading` to the disabled condition for the "Save & go to dashboard"
button to prevent multiple submissions during an ongoing save. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Explore save flow may create duplicate chart entries.
- ❌ Dashboard may receive chart added multiple times or positions.
- ⚠️ Extra backend calls to `/api/v1/chart` and `/api/v1/dashboard`.
- ⚠️ Inconsistent UX: Save disabled, Save & go still clickable.
```
</details>
```suggestion
isLoading ||
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open Explore and load a chart so that `ExploreViewContainer` renders (see
`superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:903-924`),
then
click the main save button in the header to show the save modal; this sets
`saveModal.isVisible = true` and renders `SaveModal` (`index.tsx:1054-1061`).
2. In the save modal
(`superset-frontend/src/explore/components/SaveModal.tsx`), ensure
`newSliceName`, `dashboard`, and (for query datasources) `datasetName` are
all non-empty
so that the "Save & go to dashboard" button's disabled expression at
`SaveModal.tsx:756-763` evaluates to `false` and the button becomes enabled.
3. Click "Save & go to dashboard" once to start the save; this calls
`saveOrOverwrite(true)` (`SaveModal.tsx:764`), which immediately sets
`isLoading` to
`true` (`SaveModal.tsx:391-394`) and begins async work
(`actions.saveDataset`,
`actions.updateSlice`/`createSlice`, and `SupersetClient` calls at
`SaveModal.tsx:408-505`), but the button's disabled condition does not
reference
`isLoading`, so React re-renders with the button still enabled while the
async operations
are in progress.
4. While the first save is still in flight (e.g., under slow network/API),
rapidly click
"Save & go to dashboard" again; `saveOrOverwrite(true)` is invoked multiple
times in
parallel because nothing in the disabled condition (`SaveModal.tsx:756-763`)
or in the
click handler prevents re-entry based on `isLoading`, leading to duplicate
saves/updates
(multiple `actions.createSlice`/`actions.updateSlice` and
`addChartToDashboardTab` calls)
and duplicated or inconsistent chart/dashboard state compared to the
single-click
expectation, whereas the plain "Save" button is correctly disabled by
`isLoading`
(`SaveModal.tsx:768-777`).
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/SaveModal.tsx
**Line:** 760:760
**Comment:**
*Logic Error: The "Save & go to dashboard" button does not take the
loading state into account when determining its disabled state, so while a save
operation is in progress `saveOrOverwrite(true)` can be triggered multiple
times by repeated clicks, causing duplicate saves/updates and inconsistent
state; this is especially problematic because the plain "Save" button is
correctly disabled by `isLoading`, so the behavior is inconsistent. You should
add `isLoading` to the disabled condition for the "Save & go to dashboard"
button to prevent multiple submissions during an ongoing save.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=6112eb6a26355c44aa9a0bf3b642330014d50b63a1c19e8faaa8aad68e10eb1b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=6112eb6a26355c44aa9a0bf3b642330014d50b63a1c19e8faaa8aad68e10eb1b&reaction=dislike'>👎</a>
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -824,571 +813,650 @@ const mapStateToProps = (state: RootState) => ({
const connector = connect(mapStateToProps, mapDispatchToProps);
type PropsFromRedux = ConnectedProps<typeof connector>;
-type DatasourceEditorProps = DatasourceEditorOwnProps &
- PropsFromRedux & {
- theme?: SupersetTheme;
- };
+type DatasourceEditorProps = DatasourceEditorOwnProps & PropsFromRedux;
+
+function DatasourceEditor({
+ datasource: propsDatasource,
+ onChange = () => {},
+ addSuccessToast,
+ addDangerToast,
+ setIsEditing = () => {},
+ database,
+ runQuery,
+ resetQuery,
+ formatQuery: formatQueryAction,
+}: DatasourceEditorProps) {
+ const theme = useTheme();
+ const isComponentMounted = useRef(false);
+ const abortControllers = useRef<AbortControllers>({
+ formatQuery: null,
+ formatSql: null,
+ syncMetadata: null,
+ fetchUsageData: null,
+ });
+
+ // Initialize datasource state with transformed owners and metrics
+ const [datasource, setDatasource] = useState<DatasourceObject>(() => ({
+ ...propsDatasource,
+ owners: propsDatasource.owners.map(owner => ({
+ value: owner.value || owner.id,
+ label: owner.label || `${owner.first_name} ${owner.last_name}`,
+ })),
+ metrics: propsDatasource.metrics?.map(metric => {
+ const {
+ certified_by: certifiedByMetric,
+ certification_details: certificationDetails,
+ } = metric;
+ const {
+ certification: {
+ details = undefined,
+ certified_by: certifiedBy = undefined,
+ } = {},
+ warning_markdown: warningMarkdown,
+ } = JSON.parse(metric.extra || '{}') || {};
+ return {
+ ...metric,
+ certification_details: certificationDetails || details,
+ warning_markdown: warningMarkdown || '',
+ certified_by: certifiedBy || certifiedByMetric,
+ };
+ }),
+ }));
-class DatasourceEditor extends PureComponent<
- DatasourceEditorProps,
- DatasourceEditorState
-> {
- private isComponentMounted: boolean;
+ const [errors, setErrors] = useState<string[]>([]);
+ const [isSqla] = useState(
+ propsDatasource.datasource_type === 'table' ||
+ propsDatasource.type === 'table',
+ );
+ const [isEditMode, setIsEditMode] = useState(false);
+ const [databaseColumns, setDatabaseColumns] = useState<Column[]>(
+ propsDatasource.columns.filter(col => !col.expression),
+ );
+ const [calculatedColumns, setCalculatedColumns] = useState<Column[]>(
+ propsDatasource.columns.filter(col => !!col.expression),
+ );
+ const [folders, setFolders] = useState<DatasourceFolder[]>(
+ propsDatasource.folders || [],
+ );
+ const [metadataLoading, setMetadataLoading] = useState(false);
+ const [activeTabKey, setActiveTabKey] = useState(TABS_KEYS.SOURCE);
+ const [datasourceType, setDatasourceType] = useState(
+ propsDatasource.sql
+ ? DATASOURCE_TYPES.virtual.key
+ : DATASOURCE_TYPES.physical.key,
+ );
+ const [usageCharts, setUsageCharts] = useState<ChartUsageData[]>([]);
+ const [usageChartsCount, setUsageChartsCount] = useState(0);
+
+ const findDuplicates = useCallback(
+ <T,>(arr: T[], accessor: (obj: T) => string): string[] => {
+ const seen: Record<string, null> = {};
+ const dups: string[] = [];
+ arr.forEach((obj: T) => {
+ const item = accessor(obj);
+ if (item in seen) {
+ dups.push(item);
+ } else {
+ seen[item] = null;
+ }
+ });
+ return dups;
+ },
+ [],
+ );
- private abortControllers: AbortControllers;
+ const validate = useCallback(
+ (callback: () => void) => {
+ let validationErrors: string[] = [];
+ let dups: string[];
- static defaultProps = {
- onChange: () => {},
- setIsEditing: () => {},
- };
+ // Looking for duplicate column_name
+ dups = findDuplicates(datasource.columns, obj => obj.column_name);
+ validationErrors = validationErrors.concat(
+ dups.map(name => t('Column name [%s] is duplicated', name)),
+ );
- constructor(props: DatasourceEditorProps) {
- super(props);
- this.state = {
- datasource: {
- ...props.datasource,
- owners: props.datasource.owners.map(owner => ({
- value: owner.value || owner.id,
- label: owner.label || `${owner.first_name} ${owner.last_name}`,
- })),
- metrics: props.datasource.metrics?.map(metric => {
- const {
- certified_by: certifiedByMetric,
- certification_details: certificationDetails,
- } = metric;
- const {
- certification: {
- details = undefined,
- certified_by: certifiedBy = undefined,
- } = {},
- warning_markdown: warningMarkdown,
- } = JSON.parse(metric.extra || '{}') || {};
- return {
- ...metric,
- certification_details: certificationDetails || details,
- warning_markdown: warningMarkdown || '',
- certified_by: certifiedBy || certifiedByMetric,
- };
- }),
- },
- errors: [],
- isSqla:
- props.datasource.datasource_type === 'table' ||
- props.datasource.type === 'table',
- isEditMode: false,
- databaseColumns: props.datasource.columns.filter(col => !col.expression),
- calculatedColumns: props.datasource.columns.filter(
- col => !!col.expression,
- ),
- folders: props.datasource.folders || [],
- metadataLoading: false,
- activeTabKey: TABS_KEYS.SOURCE,
- datasourceType: props.datasource.sql
- ? DATASOURCE_TYPES.virtual.key
- : DATASOURCE_TYPES.physical.key,
- usageCharts: [],
- usageChartsCount: 0,
- };
+ // Looking for duplicate metric_name
+ dups = findDuplicates(datasource.metrics ?? [], obj => obj.metric_name);
+ validationErrors = validationErrors.concat(
+ dups.map(name => t('Metric name [%s] is duplicated', name)),
+ );
- this.isComponentMounted = false;
- this.abortControllers = {
- formatQuery: null,
- formatSql: null,
- syncMetadata: null,
- fetchUsageData: null,
- };
+ // Making sure calculatedColumns have an expression defined
+ const noFilterCalcCols = calculatedColumns.filter(
+ col => !col.expression && !col.json,
+ );
+ validationErrors = validationErrors.concat(
+ noFilterCalcCols.map(col =>
+ t('Calculated column [%s] requires an expression', col.column_name),
+ ),
+ );
- this.onChange = this.onChange.bind(this);
- this.onChangeEditMode = this.onChangeEditMode.bind(this);
- this.onDatasourcePropChange = this.onDatasourcePropChange.bind(this);
- this.onDatasourceChange = this.onDatasourceChange.bind(this);
- this.tableChangeAndSyncMetadata =
- this.tableChangeAndSyncMetadata.bind(this);
- this.syncMetadata = this.syncMetadata.bind(this);
- this.setColumns = this.setColumns.bind(this);
- this.validateAndChange = this.validateAndChange.bind(this);
- this.handleTabSelect = this.handleTabSelect.bind(this);
- this.formatSql = this.formatSql.bind(this);
- this.fetchUsageData = this.fetchUsageData.bind(this);
- this.handleFoldersChange = this.handleFoldersChange.bind(this);
- }
+ // validate currency code (skip 'AUTO' - it's a placeholder for
auto-detection)
+ try {
+ datasource.metrics?.forEach(
+ metric =>
+ metric.currency?.symbol &&
+ metric.currency.symbol !== 'AUTO' &&
+ new Intl.NumberFormat('en-US', {
+ style: 'currency',
+ currency: metric.currency.symbol,
+ }),
+ );
+ } catch {
+ validationErrors = validationErrors.concat([
+ t('Invalid currency code in saved metrics'),
+ ]);
+ }
+
+ // Validate folders
+ if (folders?.length > 0) {
+ const folderValidation = validateFolders(folders);
+ validationErrors = validationErrors.concat(folderValidation.errors);
+ }
+
+ setErrors(validationErrors);
+ callback();
+ },
+ [datasource, calculatedColumns, folders, findDuplicates],
+ );
- onChange() {
+ const onChangeInternal = useCallback(() => {
// Emptying SQL if "Physical" radio button is selected
- // Currently the logic to know whether the source is
- // physical or virtual is based on whether SQL is empty or not.
- const { datasourceType, datasource } = this.state;
const sql =
datasourceType === DATASOURCE_TYPES.physical.key ? '' : datasource.sql;
const newDatasource = {
- ...this.state.datasource,
+ ...datasource,
sql,
- columns: [...this.state.databaseColumns,
...this.state.calculatedColumns],
- folders: this.state.folders,
+ columns: [...databaseColumns, ...calculatedColumns],
+ folders,
};
- this.props.onChange?.(newDatasource, this.state.errors);
- }
+ onChange(newDatasource, errors);
+ }, [
+ datasource,
+ datasourceType,
+ databaseColumns,
+ calculatedColumns,
+ folders,
+ errors,
+ onChange,
+ ]);
Review Comment:
**Suggestion:** The validation callback currently calls
`setErrors(validationErrors)` and then immediately invokes `onChangeInternal`,
which reads the `errors` state; because state updates are asynchronous in
function components, the parent `onChange` always receives stale errors from
the previous validation run instead of the freshly computed `validationErrors`.
This breaks the contract that callers see up-to-date validation errors and can
cause the UI to think the dataset is valid when it is not. To fix this, pass
the freshly computed `validationErrors` into the callback and have
`onChangeInternal` accept and forward those errors instead of reading from
`errors` state. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dataset modal Save button may allow invalid configurations.
- ⚠️ Save button may stay disabled after fixing validation errors.
- ⚠️ Affects all dataset edits via DatasourceModal UI.
```
</details>
```suggestion
(callback: (validationErrors: string[]) => void) => {
let validationErrors: string[] = [];
let dups: string[];
// Looking for duplicate column_name
dups = findDuplicates(datasource.columns, obj => obj.column_name);
validationErrors = validationErrors.concat(
dups.map(name => t('Column name [%s] is duplicated', name)),
);
// Looking for duplicate metric_name
dups = findDuplicates(datasource.metrics ?? [], obj =>
obj.metric_name);
validationErrors = validationErrors.concat(
dups.map(name => t('Metric name [%s] is duplicated', name)),
);
// Making sure calculatedColumns have an expression defined
const noFilterCalcCols = calculatedColumns.filter(
col => !col.expression && !col.json,
);
validationErrors = validationErrors.concat(
noFilterCalcCols.map(col =>
t('Calculated column [%s] requires an expression',
col.column_name),
),
);
// validate currency code (skip 'AUTO' - it's a placeholder for
auto-detection)
try {
datasource.metrics?.forEach(
metric =>
metric.currency?.symbol &&
metric.currency.symbol !== 'AUTO' &&
new Intl.NumberFormat('en-US', {
style: 'currency',
currency: metric.currency.symbol,
}),
);
} catch {
validationErrors = validationErrors.concat([
t('Invalid currency code in saved metrics'),
]);
}
// Validate folders
if (folders?.length > 0) {
const folderValidation = validateFolders(folders);
validationErrors = validationErrors.concat(folderValidation.errors);
}
setErrors(validationErrors);
callback(validationErrors);
},
[datasource, calculatedColumns, folders, findDuplicates],
);
const onChangeInternal = useCallback(
(validationErrors: string[] = errors) => {
// Emptying SQL if "Physical" radio button is selected
const sql =
datasourceType === DATASOURCE_TYPES.physical.key ? '' :
datasource.sql;
const newDatasource = {
...datasource,
sql,
columns: [...databaseColumns, ...calculatedColumns],
folders,
};
onChange(newDatasource, validationErrors);
},
[
datasource,
datasourceType,
databaseColumns,
calculatedColumns,
folders,
errors,
onChange,
],
);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the dataset edit modal, which renders `DatasourceModal` at
`superset-frontend/src/components/Datasource/DatasourceModal/index.tsx:88-399`.
This
mounts `DatasourceEditor` (async-loaded from
`../components/DatasourceEditor`) with
`onChange={onDatasourceChange}` at line 380.
2. Note that `onDatasourceChange` in `DatasourceModal` (lines 238-247)
receives `(data,
err)` from `DatasourceEditor` and stores `err` into local `errors` state via
`setErrors(err)`. The Save button in the modal is disabled when
`errors.length > 0` (lines
350-367).
3. In `DatasourceEditor` (`DatasourceEditor.tsx`), validation is performed
in `validate`
at lines 908-961, which computes `validationErrors`, calls
`setErrors(validationErrors)`,
and then immediately invokes the `callback` parameter. The callback passed by
`validateAndChange` (lines 987-989) is `onChangeInternal`.
4. `onChangeInternal` at lines 964-985 constructs `newDatasource` and calls
`onChange(newDatasource, errors)`, but `errors` here is the React state
value from line
866 that has not yet been updated by the preceding
`setErrors(validationErrors)`. Because
React state updates are asynchronous, the `err` argument seen by
`DatasourceModal.onDatasourceChange` (and thus the Save button
enable/disable logic) lags
one change behind the actual validation result, allowing the Save button to
remain enabled
when new validation errors exist or remain disabled after errors are fixed
until a
subsequent edit.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx
**Line:** 909:985
**Comment:**
*Logic Error: The validation callback currently calls
`setErrors(validationErrors)` and then immediately invokes `onChangeInternal`,
which reads the `errors` state; because state updates are asynchronous in
function components, the parent `onChange` always receives stale errors from
the previous validation run instead of the freshly computed `validationErrors`.
This breaks the contract that callers see up-to-date validation errors and can
cause the UI to think the dataset is valid when it is not. To fix this, pass
the freshly computed `validationErrors` into the callback and have
`onChangeInternal` accept and forward those errors instead of reading from
`errors` 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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=5b3f9870ab1c688187d2517b25d17e2383f761a3b3adf741c138e41bc6c610c4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=5b3f9870ab1c688187d2517b25d17e2383f761a3b3adf741c138e41bc6c610c4&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]
