Copilot commented on code in PR #37131:
URL: https://github.com/apache/superset/pull/37131#discussion_r2713842089
##########
superset-frontend/src/explore/exploreUtils/index.js:
##########
@@ -282,8 +283,58 @@ export const exportChart = async ({
exportType: resultFormat,
});
} else {
- // Fallback to original behavior for non-streaming exports
- SupersetClient.postForm(url, { form_data: safeStringify(payload) });
+ // Use AJAX blob download instead of form submission to enable error
handling
+ try {
+ const response = await SupersetClient.postBlob(url, {
+ form_data: payload,
+ });
+
+ // Check if response is OK (status 200-299)
+ if (!response.ok) {
+ // Create error object with status for proper handling
+ const error = new Error(
+ `HTTP ${response.status} ${response.statusText}`,
+ );
+ error.status = response.status;
+ error.statusText = response.statusText;
+ error.response = response;
+ throw error;
+ }
+
Review Comment:
The check `if (!response.ok)` at line 293 is unreachable code. When using
`parseMethod: 'raw'`, the `parseResponse` function in SupersetClient
automatically rejects non-ok responses (see parseResponse.ts line 41-42). This
means `postBlob` will only return successful responses, and errors will be
caught in the catch block starting at line 315. Lines 293-302 should be removed
as they will never execute.
```suggestion
```
##########
superset-frontend/src/explore/exploreUtils/exploreUtils.test.jsx:
##########
@@ -299,4 +301,212 @@ describe('exploreUtils', () => {
expect(postFormSpy).toHaveBeenCalledTimes(1);
});
});
+
+ // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
+ describe('.exportChart()', () => {
+ let postBlobSpy;
+ let downloadBlobSpy;
+ let mockBlob;
+
+ beforeEach(() => {
+ // Create a mock blob
+ mockBlob = new Blob(['test data'], { type: 'text/csv' });
+
+ // Mock SupersetClient.postBlob
+ postBlobSpy = jest.spyOn(SupersetClient, 'postBlob');
+
+ // Mock downloadBlob from utils/export
+ downloadBlobSpy = jest.spyOn(exportUtils, 'downloadBlob');
+ downloadBlobSpy.mockImplementation(jest.fn());
+ });
+
+ afterEach(() => {
+ postBlobSpy.mockRestore();
+ downloadBlobSpy.mockRestore();
+ });
+
+ test('successfully exports chart as CSV', async () => {
+ // Mock successful response
+ const mockResponse = {
+ ok: true,
+ status: 200,
+ blob: jest.fn().mockResolvedValue(mockBlob),
+ };
+ postBlobSpy.mockResolvedValue(mockResponse);
+
+ await exportChart({
+ formData: { ...formData, viz_type: 'my_custom_viz' },
+ resultFormat: 'csv',
+ resultType: 'full',
+ });
+
+ expect(postBlobSpy).toHaveBeenCalledTimes(1);
+ expect(mockResponse.blob).toHaveBeenCalled();
+ expect(downloadBlobSpy).toHaveBeenCalledWith(
+ mockBlob,
+ expect.stringContaining('.csv'),
+ );
+ });
+
+ test('successfully exports chart as Excel', async () => {
+ // Mock successful response
+ const mockResponse = {
+ ok: true,
+ status: 200,
+ blob: jest.fn().mockResolvedValue(mockBlob),
+ };
+ postBlobSpy.mockResolvedValue(mockResponse);
+
+ await exportChart({
+ formData: { ...formData, viz_type: 'my_custom_viz' },
+ resultFormat: 'xlsx',
+ resultType: 'results',
+ });
+
+ expect(postBlobSpy).toHaveBeenCalledTimes(1);
+ expect(mockResponse.blob).toHaveBeenCalled();
+ expect(downloadBlobSpy).toHaveBeenCalledWith(
+ mockBlob,
+ expect.stringContaining('.xlsx'),
+ );
+ });
+
+ test('throws error with status 413 when payload is too large', async () =>
{
+ // Mock 413 response
+ const mockResponse = {
+ ok: false,
+ status: 413,
+ statusText: 'Payload Too Large',
+ blob: jest.fn(),
+ };
+ postBlobSpy.mockResolvedValue(mockResponse);
+
+ await expect(
+ exportChart({
+ formData: { ...formData, viz_type: 'my_custom_viz' },
+ resultFormat: 'csv',
+ }),
+ ).rejects.toMatchObject({
+ status: 413,
+ message: expect.stringContaining('413'),
+ });
+
+ expect(postBlobSpy).toHaveBeenCalledTimes(1);
+ // Blob should not be called if response is not ok
+ expect(mockResponse.blob).not.toHaveBeenCalled();
+ // Download should not be triggered
+ expect(downloadBlobSpy).not.toHaveBeenCalled();
+ });
+
+ test('throws error with status 500 for server errors', async () => {
+ // Mock 500 response
+ const mockResponse = {
+ ok: false,
+ status: 500,
+ statusText: 'Internal Server Error',
+ blob: jest.fn(),
+ };
+ postBlobSpy.mockResolvedValue(mockResponse);
+
+ await expect(
+ exportChart({
+ formData: { ...formData, viz_type: 'my_custom_viz' },
+ resultFormat: 'json',
+ }),
+ ).rejects.toMatchObject({
+ status: 500,
+ message: expect.stringContaining('500'),
+ });
+
+ expect(downloadBlobSpy).not.toHaveBeenCalled();
+ });
Review Comment:
These tests incorrectly mock the behavior of `SupersetClient.postBlob`. The
tests use `mockResolvedValue` with `ok: false` responses, but `postBlob` (with
`parseMethod: 'raw'`) actually rejects the promise when the response is not ok
(see parseResponse.ts line 41-42). The tests should use `mockRejectedValue`
with a Response object instead, similar to the test at line 424-443. The
current tests are passing false confidence that the error handling code at
lines 293-302 works, when in reality that code is unreachable.
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.jsx:
##########
@@ -279,57 +294,108 @@ export const useExploreAdditionalActionsMenu = (
startExport,
Review Comment:
The dependency array is missing `addDangerToast` which is used in the catch
block at lines 271 and 283. This could lead to stale closures where the
function references an outdated version of `addDangerToast`. Add
`addDangerToast` to the dependency array.
```suggestion
startExport,
addDangerToast,
```
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.jsx:
##########
@@ -279,57 +294,108 @@ export const useExploreAdditionalActionsMenu = (
startExport,
]);
- const exportCSVPivoted = useCallback(
- () =>
- canDownloadCSV
- ? exportChart({
- formData: latestQueryFormData,
- ownState,
- resultType: 'post_processed',
- resultFormat: 'csv',
- })
- : null,
- [canDownloadCSV, latestQueryFormData, ownState],
- );
+ const exportCSVPivoted = useCallback(async () => {
+ if (!canDownloadCSV) {
+ return null;
+ }
+ try {
+ await exportChart({
+ formData: latestQueryFormData,
+ ownState,
+ resultType: 'post_processed',
+ resultFormat: 'csv',
+ });
+ } catch (error) {
+ const status = error?.status || error?.response?.status;
+ if (status === 413) {
+ addDangerToast(
+ t(
+ 'The chart data is too large to download. Please try reducing the
date range, limiting rows, or using fewer columns.',
+ ),
+ );
+ } else {
+ const errorMessage =
+ error?.message ||
+ error?.statusText ||
+ t(
+ 'Failed to export chart data. Please try again or contact your
administrator.',
+ );
+ addDangerToast(errorMessage);
+ }
+ }
+ return null;
+ }, [canDownloadCSV, latestQueryFormData, ownState, addDangerToast]);
- const exportJson = useCallback(
- () =>
- canDownloadCSV
- ? exportChart({
- formData: latestQueryFormData,
- ownState,
- resultType: 'results',
- resultFormat: 'json',
- })
- : null,
- [canDownloadCSV, latestQueryFormData, ownState],
- );
+ const exportJson = useCallback(async () => {
+ if (!canDownloadCSV) {
+ return null;
+ }
+ try {
+ await exportChart({
+ formData: latestQueryFormData,
+ ownState,
+ resultType: 'results',
+ resultFormat: 'json',
+ });
+ } catch (error) {
+ const status = error?.status || error?.response?.status;
+ if (status === 413) {
+ addDangerToast(
+ t(
+ 'The chart data is too large to download. Please try reducing the
date range, limiting rows, or using fewer columns.',
+ ),
+ );
+ } else {
+ const errorMessage =
+ error?.message ||
+ error?.statusText ||
+ t(
+ 'Failed to export chart data. Please try again or contact your
administrator.',
+ );
+ addDangerToast(errorMessage);
+ }
+ }
+ return null;
+ }, [canDownloadCSV, latestQueryFormData, ownState, addDangerToast]);
- const exportExcel = useCallback(
- () =>
- canDownloadCSV
- ? exportChart({
- formData: latestQueryFormData,
- ownState,
- resultType: 'results',
- resultFormat: 'xlsx',
- })
- : null,
- [canDownloadCSV, latestQueryFormData, ownState],
- );
+ const exportExcel = useCallback(async () => {
+ if (!canDownloadCSV) {
+ return null;
+ }
+ try {
+ await exportChart({
+ formData: latestQueryFormData,
+ ownState,
+ resultType: 'results',
+ resultFormat: 'xlsx',
+ });
+ } catch (error) {
+ const status = error?.status || error?.response?.status;
+ if (status === 413) {
+ addDangerToast(
+ t(
+ 'The chart data is too large to download. Please try reducing the
date range, limiting rows, or using fewer columns.',
+ ),
+ );
+ } else {
+ const errorMessage =
+ error?.message ||
+ error?.statusText ||
+ t(
+ 'Failed to export chart data. Please try again or contact your
administrator.',
+ );
+ addDangerToast(errorMessage);
+ }
+ }
+ return null;
+ }, [canDownloadCSV, latestQueryFormData, ownState, addDangerToast]);
const copyLink = useCallback(async () => {
try {
if (!latestQueryFormData) {
throw new Error();
}
- await copyTextToClipboard(async () => {
- const result = await getChartPermalink(latestQueryFormData);
- if (!result?.url) {
- throw new Error('Failed to generate permalink');
- }
- return result.url;
- });
+ await copyTextToClipboard(() => getChartPermalink(latestQueryFormData));
Review Comment:
The function passed to `copyTextToClipboard` should return
`Promise<string>`, but `getChartPermalink` returns `Promise<PermalinkResult>`
which is an object with `url` and `key` properties. This will cause the
clipboard to contain "[object Object]" instead of the URL. The correct
implementation should extract the `url` property from the result, like this:
`() => getChartPermalink(latestQueryFormData).then(result => result.url)`
```suggestion
await copyTextToClipboard(() =>
getChartPermalink(latestQueryFormData).then(result => result.url),
);
```
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/useExploreAdditionalActionsMenu.test.jsx:
##########
@@ -0,0 +1,156 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import { render, screen, waitFor } from 'spec/helpers/testing-library';
+import { useExploreAdditionalActionsMenu } from './index';
+import * as exploreUtils from 'src/explore/exploreUtils';
+import userEvent from '@testing-library/user-event';
+import { VizType } from '@superset-ui/core';
Review Comment:
Unused import VizType.
```suggestion
```
--
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]
