fix(charts): show user-friendly error for HTTP 413 payload too large#37131
fix(charts): show user-friendly error for HTTP 413 payload too large#37131Vansh5632 wants to merge 7 commits into
Conversation
- Add SupersetClient.postBlob() method for AJAX-based chart exports - Replace form submission with AJAX to enable error interception - Handle HTTP 413 errors with toast notifications instead of raw error pages - Add comprehensive error handling in Explore and Dashboard views - Include 8 new unit tests for exportChart error scenarios Fixes large chart export UX issue where nginx/proxy 413 errors would open in new tab showing raw JSON/HTML instead of friendly message.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
dosubot
Bot
added
api
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #effd4d
Actionable Suggestions - 1
-
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.jsx - 1
- Permalink handling bug · Line 207-207
Additional Suggestions - 2
-
superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts - 1
-
JSDoc return description mismatch · Line 152-157The JSDoc for the new postBlob method states it 'returns a blob', but the implementation returns Promise. While the Response can be used to access the blob via response.blob(), the documentation should match the actual return type to avoid confusion for callers expecting a blob directly.
Code suggestion
@@ -152,7 +152,7 @@ - /** - * POST request that returns a blob for file downloads. - * Unlike postForm, this uses AJAX so errors can be caught and handled. - * @param endpoint - API endpoint - * @param payload - Request payload - * @returns Promise resolving to Response with blob - */ + /** + * POST request that returns a Promise<Response> for file downloads. + * Unlike postForm, this uses AJAX so errors can be caught and handled. + * @param endpoint - API endpoint + * @param payload - Request payload + * @returns Promise resolving to Response with blob + */
-
-
superset-frontend/src/explore/exploreUtils/exploreUtils.test.jsx - 1
-
Deprecated test structure · Line 306-306The added test code uses describe blocks, which are deprecated per project guidelines in favor of flat test structures. The existing code includes a TODO comment to migrate from describe blocks, and this addition continues the pattern. Consider refactoring to individual test() calls to support the ongoing testing strategy migration.
-
Review Details
-
Files reviewed - 8 · Commit Range:
f4ac865..f4ac865- superset-frontend/packages/superset-ui-core/src/connection/SupersetClient.ts
- superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts
- superset-frontend/packages/superset-ui-core/src/connection/types.ts
- superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx
- superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.jsx
- superset-frontend/src/explore/exploreUtils/exploreUtils.test.jsx
- superset-frontend/src/explore/exploreUtils/index.js
- superset-frontend/src/utils/export.ts
-
Files skipped - 0
-
Tools
- Eslint (Linter) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
…ctionsMenu/index.jsx Co-authored-by: bito-code-review[bot] <188872107+bito-code-review[bot]@users.noreply.github.com>
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| reAuthenticate: () => getInstance().reAuthenticate(), | ||
| request: request => getInstance().request(request), | ||
| getCSRFToken: () => getInstance().getCSRFToken(), | ||
| postBlob: (endpoint, payload) => getInstance().postBlob(endpoint, payload), |
There was a problem hiding this comment.
Suggestion: The wrapper uses two separate parameters (endpoint, payload) but callers likely pass a single request object; this will cause the instance postBlob to receive an unexpected shape (the whole request as endpoint and payload undefined), leading to runtime errors—forward the single request argument to the instance method. [logic error]
Severity Level: Critical 🚨
- ❌ Explore export (exportChart) fails producing no download.
- ❌ Dashboard chart/table export fails to download.
- ⚠️ Browser console shows uncaught export-related errors.
- ⚠️ Automated tests for exports may start failing.| postBlob: (endpoint, payload) => getInstance().postBlob(endpoint, payload), | |
| postBlob: request => getInstance().postBlob(request), |
Steps of Reproduction ✅
1. Ensure SupersetClient is configured and the singleton exists by calling
SupersetClient.configure(...). (See
superset-frontend/packages/superset-ui-core/src/connection/SupersetClient.ts around
getInstance()/configure; the wrapper live at line 54.)
2. Trigger a UI chart export that the PR description changed to use postBlob — e.g., click
"Download CSV" in Explore which calls exportChart in exploreUtils (PR changes reference
exploreUtils/index.js:exportChart). That code calls SupersetClient.postBlob(request) with
a single request object (request contains endpoint, payload, filename metadata).
3. At runtime the wrapper in SupersetClient.ts line 54 executes, treating the single
object as the `endpoint` parameter and leaving `payload` undefined, then calls
getInstance().postBlob(endpoint, payload) with unexpected shapes.
4. If SupersetClientClass.postBlob expects a single request object, the instance method
receives wrong arguments and may throw, return an error, or produce malformed network
requests — observed as export failures or uncaught exceptions in the browser console and
failed downloads.
Note: the reproduction maps to the PR description which replaced form submissions with
SupersetClient.postBlob usage in exploreUtils and Dashboard chart export flows; the
wrapper at SupersetClient.ts:54 must forward the same single-argument shape used by those
callers.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-core/src/connection/SupersetClient.ts
**Line:** 54:54
**Comment:**
*Logic Error: The wrapper uses two separate parameters `(endpoint, payload)` but callers likely pass a single request object; this will cause the instance `postBlob` to receive an unexpected shape (the whole request as `endpoint` and `payload` undefined), leading to runtime errors—forward the single request argument to the instance method.
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.| | 'postBlob' | ||
| | 'put' | ||
| | 'request' | ||
| | 'init' |
There was a problem hiding this comment.
Suggestion: Adding 'postBlob' to the Pick list assumes SupersetClientClass already declares that method; if it doesn't, the Pick will not include it and consumers expecting postBlob will break. Make postBlob an optional member on the exported interface (typed to the correct request/response types) or ensure the class exposes the method. This preserves backward compatibility while providing proper types for the new API. [type error]
Severity Level: Critical 🚨
- ❌ TypeScript build failure blocking CI pipelines.
- ❌ Explore exportChart runtime TypeError on export.
- ❌ Dashboard chart export runtime errors.
- ⚠️ Consumers relying on SupersetClient typings mismatched.| | 'init' | |
| | 'put' | |
| | 'request' | |
| | 'init' | |
| | 'isAuthenticated' | |
| | 'reAuthenticate' | |
| | 'getGuestToken' | |
| > { | |
| // `postBlob` may be a new API implemented on SupersetClientClass. | |
| // Declare it optional here to avoid breaking consumers if the class | |
| // implementation isn't present at compile time, and provide a precise type. | |
| postBlob?: (request: RequestConfig) => Promise<SupersetClientResponse>; |
Steps of Reproduction ✅
1. Run TypeScript build for the frontend package: from repo root run `cd superset-frontend
&& npm run build` (this triggers tsc for packages). Type errors originate from
`superset-frontend/packages/superset-ui-core/src/connection/types.ts:152` where the Pick
includes `'postBlob'`. Expect TypeScript error like: "Type '"postBlob"' is not assignable
to parameter of type 'keyof SupersetClientClass'".
2. Inspect the imported class at
`superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass` (import
path from `types.ts` line 1: `import SupersetClientClass from './SupersetClientClass';`).
If that class file does not declare a `postBlob` member, the Pick in `types.ts` will fail
compilation as shown in step 1.
3. If TypeScript checking is skipped (e.g., during a Babel-only transpile or in a runtime
test environment), consumer code added in this PR calls `SupersetClient.postBlob(...)`
from Explore export functions (PR description references
`superset-frontend/src/explore/exploreUtils/index.js` exportChart usage). At runtime, if
`SupersetClientClass` instance lacks `postBlob`, this results in a runtime TypeError:
"SupersetClient.postBlob is not a function" when `exportChart` calls it.
4. Confirm by opening the PR-modified consumers:
`superset-frontend/src/explore/exploreUtils/index.js` (exportChart),
`superset-frontend/src/dashboard/Chart.jsx` (table export), and
`superset-frontend/src/hooks/useExploreAdditionalActionsMenu` (CSV/Excel/JSON exports). If
these files call `SupersetClient.postBlob` but `SupersetClientClass` does not declare it,
either TypeScript build fails (step 1) or runtime failures occur (step 3). If the addition
to the Pick was intentional, verify `SupersetClientClass` implements `postBlob` in
`superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-core/src/connection/types.ts
**Line:** 158:158
**Comment:**
*Type Error: Adding `'postBlob'` to the Pick list assumes `SupersetClientClass` already declares that method; if it doesn't, the Pick will not include it and consumers expecting `postBlob` will break. Make `postBlob` an optional member on the exported interface (typed to the correct request/response types) or ensure the class exposes the method. This preserves backward compatibility while providing proper types for the new API.
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.
|
CodeAnt AI finished reviewing your PR. |
Code Review Agent Run #c52491Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
- Add try-catch error handling for exportCSV, exportCSVPivoted, exportJson, and exportExcel functions - Display user-friendly error messages when chart data is too large (413 error) - Provide actionable guidance to users (reduce date range, limit rows, use fewer columns) - Add unit tests for 413 error handling in chart exports
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
CodeAnt AI Incremental review completed. |
|
@rusackas your review is appreciated |
There was a problem hiding this comment.
Code Review Agent Run #715822
Actionable Suggestions - 1
-
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/useExploreAdditionalActionsMenu.test.jsx - 1
- Incorrect test setup for button label · Line 102-102
Review Details
-
Files reviewed - 9 · Commit Range:
8d28df5..ff0cbb2- superset-frontend/packages/superset-ui-core/src/connection/SupersetClient.ts
- superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts
- superset-frontend/packages/superset-ui-core/src/connection/types.ts
- superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx
- superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.jsx
- superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/useExploreAdditionalActionsMenu.test.jsx
- superset-frontend/src/explore/exploreUtils/exploreUtils.test.jsx
- superset-frontend/src/explore/exploreUtils/index.js
- superset-frontend/src/utils/export.ts
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| mockExportChart.mockRejectedValue(error413); | ||
|
|
||
| const props = { | ||
| latestQueryFormData: { viz_type: 'table' }, |
There was a problem hiding this comment.
The test uses viz_type 'table' but expects 'Export to original .CSV' button, which only appears for pivot tables (viz_type 'pivot_table_v2'). For table viz, the button is 'Export to .CSV'.
Code suggestion
Check the AI-generated fix before applying
| latestQueryFormData: { viz_type: 'table' }, | |
| latestQueryFormData: { viz_type: 'pivot_table_v2' }, |
Citations
- Rule Violated: AGENTS.md:21
Code Review Run #715822
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
sadpandajoe
requested review from
amaannawab923,
Copilot and
michael-s-molina
| }); | ||
|
|
||
| // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks | ||
| describe('.exportChart()', () => { |
There was a problem hiding this comment.
if these are new tests, why are we adding a describe block?
There was a problem hiding this comment.
Sorry about that — thanks for pointing this out. I wasn’t aware of this guideline earlier. I’ll refactor the tests to follow the new structure and align with the project’s testing migration approach.
There was a problem hiding this comment.
Pull request overview
This PR improves error handling for chart exports that fail due to HTTP 413 (Payload Too Large) errors by replacing form-based submissions with AJAX requests. This allows the application to catch errors and display user-friendly toast notifications instead of opening raw error pages in new browser tabs.
Changes:
- Added
SupersetClient.postBlob()method for AJAX-based blob downloads with error handling - Refactored
exportChart()to use AJAX requests and throw errors with proper status codes - Updated export functions in Explore and Dashboard views to catch and display user-friendly error messages for HTTP 413 and other errors
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| superset-frontend/src/utils/export.ts | Exported downloadBlob utility function for reuse |
| superset-frontend/packages/superset-ui-core/src/connection/types.ts | Added postBlob to SupersetClientInterface |
| superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts | Implemented postBlob() method |
| superset-frontend/packages/superset-ui-core/src/connection/SupersetClient.ts | Exposed postBlob in SupersetClient singleton |
| superset-frontend/src/explore/exploreUtils/index.js | Replaced postForm with postBlob and added error handling |
| superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.jsx | Added try-catch error handling to all export functions; fixed shareByEmail and copyLink implementation |
| superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx | Added error handling to exportTable function |
| superset-frontend/src/explore/exploreUtils/exploreUtils.test.jsx | Added comprehensive tests for new export functionality |
| superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/useExploreAdditionalActionsMenu.test.jsx | Added tests for 413 error handling |
Comments suppressed due to low confidence (1)
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx:597
- The dependency array is missing
boundActionCreators.addDangerToastwhich is used in the catch block at lines 567 and 579. WhileboundActionCreatorsis memoized, it's better to explicitly includeboundActionCreators.addDangerToastin the dependency array for clarity and to match the pattern used elsewhere in this file (e.g., line 268 includes bothboundActionCreators.logEventandboundActionCreators.changeFilter).
[
slice.slice_id,
slice.viz_type,
isCached,
formData,
maxRows,
dataMask[props.id]?.ownState,
chartState,
props.id,
boundActionCreators.logEvent,
queriesResponse,
startExport,
resetExport,
streamingThreshold,
],
| // 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; | ||
| } | ||
|
|
There was a problem hiding this 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.
| // 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; | |
| } |
| 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(); | ||
| }); |
There was a problem hiding this 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.
| } | ||
| return result.url; | ||
| }); | ||
| await copyTextToClipboard(() => getChartPermalink(latestQueryFormData)); |
There was a problem hiding this 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)
| await copyTextToClipboard(() => getChartPermalink(latestQueryFormData)); | |
| await copyTextToClipboard(() => | |
| getChartPermalink(latestQueryFormData).then(result => result.url), | |
| ); |
| @@ -279,57 +294,108 @@ export const useExploreAdditionalActionsMenu = ( | |||
| startExport, | |||
There was a problem hiding this 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.
| startExport, | |
| startExport, | |
| addDangerToast, |
| import { useExploreAdditionalActionsMenu } from './index'; | ||
| import * as exploreUtils from 'src/explore/exploreUtils'; | ||
| import userEvent from '@testing-library/user-event'; | ||
| import { VizType } from '@superset-ui/core'; |
There was a problem hiding this comment.
Unused import VizType.
| import { VizType } from '@superset-ui/core'; |
Refactor exploreUtils.test.jsx to use flat test() calls instead of nested describe() blocks, following the project's testing migration strategy and 'avoid nesting when testing' principles.
Code Review Agent Run #0eaaeeActionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Thanks for the work thus far @Vansh5632. This has been open a while obviously, and has a bunch of conflicts and unresolved bot suggestions. Sorry for our slow turnaround on things (we're trying to close the gap and get the backlog under control). Let us know if you're still able to pick it back up, but I'll mark it as draft for now while it awaits work. Holler if I can help. |
|
Thanks for the update @rusackas . I completely understand the situation, and no worries about the turnaround time. I'll take a look at the merge conflicts and unresolved bot suggestions to see what I can resolve. Once I have addressed the issues and the PR is ready again, I'll let you know. I appreciate your support and will reach out if I need any help. |
… handling Port postBlob-based export error handling to TypeScript files migrated on master, address review feedback, and update tests for the new export flow. Co-authored-by: Cursor <cursoragent@cursor.com>
dosubot
Bot
added
explore
Resolve package-lock.json conflict by regenerating from the merged package.json after syncing with upstream.
There was a problem hiding this comment.
Code Review Agent Run #d807d9
Actionable Suggestions - 2
-
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.tsx - 1
- Semantic duplication in error handling · Line 402-424
-
superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts - 1
- Missing Unit Tests for postBlob · Line 160-170
Additional Suggestions - 1
-
superset-frontend/packages/superset-ui-core/src/connection/SupersetClient.ts - 1
-
Missing test coverage for postBlob · Line 55-55The `postBlob` method is correctly exposed on the `SupersetClient` singleton and the interface contract is satisfied. However, the test file `SupersetClient.test.ts` does not include `postBlob` in its method-exposure assertions (lines 39-54) or pre-configure throw tests (lines 56-70), creating a gap in test coverage for this public API method.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset-frontend/src/utils/export.ts - 1
- Missing export corrected · Line 32-32
-
superset-frontend/src/explore/exploreUtils/exportChart.test.ts - 1
- Incorrect regex in timestamp filename test · Line 450-450
Review Details
-
Files reviewed - 9 · Commit Range:
f4ac865..a780f2b- superset-frontend/packages/superset-ui-core/src/connection/SupersetClient.ts
- superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts
- superset-frontend/packages/superset-ui-core/src/connection/types.ts
- superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx
- superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.tsx
- superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/useExploreAdditionalActionsMenu.test.jsx
- superset-frontend/src/explore/exploreUtils/exportChart.test.ts
- superset-frontend/src/explore/exploreUtils/index.ts
- superset-frontend/src/utils/export.ts
-
Files skipped - 1
- superset-frontend/package-lock.json - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| } catch (error) { | ||
| const exportError = error as Error & { | ||
| status?: number; | ||
| statusText?: string; | ||
| response?: { status?: number }; | ||
| }; | ||
| const status = exportError.status || exportError.response?.status; | ||
| if (status === 413) { | ||
| addDangerToast( | ||
| t( | ||
| 'Export failed: The chart data is too large to download (413). Please try reducing the date range, limiting rows, or using fewer columns.', | ||
| ), | ||
| ); | ||
| } else { | ||
| const errorMessage = | ||
| exportError.message || | ||
| exportError.statusText || | ||
| t( | ||
| 'Failed to export chart data. Please try again or contact your administrator.', | ||
| ); | ||
| addDangerToast(errorMessage); | ||
| } | ||
| } |
There was a problem hiding this comment.
The exportCSV function duplicates the same error handling logic that already exists in the handleExportError callback (lines 437-462). This creates semantic duplication and inconsistent error messages (note the 413 error message differs: 'Export failed: The chart data is too large to download (413).' vs 'The chart data is too large to download.'). Use handleExportError(error) instead.
Code Review Run #d807d9
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| async postBlob( | ||
| endpoint: string, | ||
| payload: Record<string, any>, | ||
| ): Promise<Response> { | ||
| await this.ensureAuth(); | ||
| return this.post({ | ||
| endpoint, | ||
| postPayload: payload, | ||
| parseMethod: 'raw', | ||
| }); | ||
| } |
There was a problem hiding this comment.
The new postBlob method lacks unit tests. Rule [11730] requires comprehensive unit tests for new tools covering success paths, error scenarios, and edge cases. Compare with postForm which has tests at lines 668-775 in the same test file.
Code Review Run #d807d9
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Deduplicate export error handling via handleExportError, add postBlob test coverage, and tighten export filename assertion.
| */ | ||
| async postBlob( | ||
| endpoint: string, | ||
| payload: Record<string, any>, |
There was a problem hiding this comment.
Suggestion: Replace Record<string, any> with a stricter payload type (such as an existing reusable request payload type or Record<string, unknown>) so the new API method does not introduce any. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The new postBlob method is TypeScript code and its payload parameter uses Record<string, any>, which directly introduces the any type. This matches the rule against new or modified TypeScript/TSX code using any, so the suggestion is valid.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts
**Line:** 162:162
**Comment:**
*Custom Rule: Replace `Record<string, any>` with a stricter payload type (such as an existing reusable request payload type or `Record<string, unknown>`) so the new API method does not introduce `any`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| setCurrentReportDeleting: jest.fn(), | ||
| }; | ||
|
|
||
| const TestComponent = props => { |
There was a problem hiding this comment.
Suggestion: Convert this new frontend test file from .test.jsx to .test.tsx and add explicit prop typing (or reuse an existing hook prop type) so the test follows the TypeScript-first frontend standard. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly added frontend test file with a .jsx extension, which matches the rule against adding new JavaScript source files in the frontend. The suggestion to convert it to .tsx and add typing is therefore aligned with a real rule violation in the current code.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/useExploreAdditionalActionsMenu.test.jsx
**Line:** 61:61
**Comment:**
*Custom Rule: Convert this new frontend test file from `.test.jsx` to `.test.tsx` and add explicit prop typing (or reuse an existing hook prop type) so the test follows the TypeScript-first frontend standard.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| const exportCSVPivoted = useCallback(async () => { | ||
| if (!canDownloadCSV) { | ||
| return null; | ||
| } | ||
| try { | ||
| await exportChart({ | ||
| formData: latestQueryFormData as QueryFormData, | ||
| ownState, | ||
| resultType: 'post_processed', | ||
| resultFormat: 'csv', | ||
| }); | ||
| } catch (error) { | ||
| handleExportError(error); | ||
| } | ||
| return null; | ||
| }, [canDownloadCSV, latestQueryFormData, ownState, handleExportError]); | ||
|
|
||
| const exportJson = useCallback( | ||
| () => | ||
| canDownloadCSV | ||
| ? exportChart({ | ||
| formData: latestQueryFormData as QueryFormData, | ||
| ownState, | ||
| resultType: 'results', | ||
| resultFormat: 'json', | ||
| }) | ||
| : null, | ||
| [canDownloadCSV, latestQueryFormData, ownState], | ||
| ); | ||
| const exportJson = useCallback(async () => { | ||
| if (!canDownloadCSV) { | ||
| return null; | ||
| } | ||
| try { | ||
| await exportChart({ | ||
| formData: latestQueryFormData as QueryFormData, | ||
| ownState, | ||
| resultType: 'results', | ||
| resultFormat: 'json', | ||
| }); | ||
| } catch (error) { | ||
| handleExportError(error); | ||
| } | ||
| return null; | ||
| }, [canDownloadCSV, latestQueryFormData, ownState, handleExportError]); | ||
|
|
||
| const exportExcel = useCallback( | ||
| () => | ||
| canDownloadCSV | ||
| ? exportChart({ | ||
| formData: latestQueryFormData as QueryFormData, | ||
| ownState, | ||
| resultType: 'results', | ||
| resultFormat: 'xlsx', | ||
| }) | ||
| : null, | ||
| [canDownloadCSV, latestQueryFormData, ownState], | ||
| ); | ||
| const exportExcel = useCallback(async () => { | ||
| if (!canDownloadCSV) { | ||
| return null; | ||
| } | ||
| try { | ||
| await exportChart({ | ||
| formData: latestQueryFormData as QueryFormData, | ||
| ownState, | ||
| resultType: 'results', | ||
| resultFormat: 'xlsx', | ||
| }); | ||
| } catch (error) { | ||
| handleExportError(error); | ||
| } | ||
| return null; | ||
| }, [canDownloadCSV, latestQueryFormData, ownState, handleExportError]); |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
The error-handling coverage in Explore is incomplete: the "Export Current View → Export to .CSV" server path calls exportChart(...) without awaiting or wrapping it in try/catch, so HTTP 413 and other export failures reject the Promise without showing the new user-friendly toast.
Suggestion: Mirror the pattern used by exportCSV/exportCSVPivoted/exportJson/exportExcel by making the current-view CSV server path async, awaiting exportChart, and routing any caught errors through handleExportError so 413 and other failures consistently surface as toasts.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.tsx
**Line:** 444:493
**Comment:**
*HIGH: The error-handling coverage in Explore is incomplete: the "Export Current View → Export to .CSV" server path calls exportChart(...) without awaiting or wrapping it in try/catch, so HTTP 413 and other export failures reject the Promise without showing the new user-friendly toast.
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.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const extension = resultFormat === 'xlsx' ? 'xlsx' : resultFormat; | ||
| const timestamp = new Date() | ||
| .toISOString() | ||
| .replace(/[:.]/g, '-') | ||
| .slice(0, -5); | ||
| const filename = `chart_export_${timestamp}.${extension}`; | ||
|
|
||
| const blob = await response.blob(); | ||
| downloadBlob(blob, filename); |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
The download filename is now derived solely from the requested resultFormat, which ignores the actual response type; for multi-query table-like exports the backend returns a ZIP (Content-Type application/zip and "zip" download headers), so the frontend saves ZIP content with a .csv/.xlsx extension.
Suggestion: Inspect response headers (Content-Disposition/Content-Type) before naming the file, using a .zip extension and/or server-provided filename when the backend bundles multi-query results as a ZIP, and only fall back to the resultFormat-derived extension when no explicit type is provided.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** superset-frontend/src/explore/exploreUtils/index.ts
**Line:** 408:416
**Comment:**
*HIGH: The download filename is now derived solely from the requested resultFormat, which ignores the actual response type; for multi-query table-like exports the backend returns a ZIP (Content-Type application/zip and "zip" download headers), so the frontend saves ZIP content with a .csv/.xlsx extension.
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.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| return this.post({ | ||
| endpoint, | ||
| postPayload: payload, | ||
| parseMethod: 'raw', | ||
| }); |
There was a problem hiding this comment.
Suggestion: postBlob forwards postPayload without overriding the default stringify: true, so string fields are JSON-stringified again by callApi. Export callers already pass form_data as a JSON string, which becomes double-encoded (extra quotes), and the backend then parses it as a string instead of an object, causing export requests to fail (typically 400) instead of downloading. Pass stringify: false (or send a prebuilt FormData) to preserve raw form field values. [api mismatch]
Severity Level: Critical 🚨
- ❌ Dashboard chart CSV export returns HTTP 400, no download.
- ❌ Explore view chart exports fail against /api/v1/chart/data.
- ⚠️ Users see error toasts instead of downloaded export files.Steps of Reproduction ✅
1. Open a dashboard and trigger CSV export for a chart; tests at
`superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.test.tsx:8-31` show
that clicking "Export to .CSV" calls `exportChart(...)` from
`src/explore/exploreUtils/index.ts`.
2. In
`superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx:19-37,559-567`,
the `exportChart` function is invoked with `resultFormat: 'csv'` and `resultType: 'full'`,
and `onStartStreamingExport` left as `null`, so the non-streaming export path is used.
3. Inside `superset-frontend/src/explore/exploreUtils/index.ts:18-69`, `exportChart`
builds the payload and, for the v1 API (`url = '/api/v1/chart/data'`), calls
`SupersetClient.postBlob(url as string, { form_data: safeStringify(payload) })` at lines
41-47 and 65-67, where `safeStringify` (see
`plugins/preset-chart-deckgl/src/utils/safeStringify.ts:4-24`) returns a JSON string.
4. `SupersetClient.postBlob` in
`packages/superset-ui-core/src/connection/SupersetClientClass.ts:160-169` forwards this
payload via `this.post({ endpoint, postPayload: payload, parseMethod: 'raw' })` without
specifying `stringify`, so `request` flows into `callApiAndParseWithTimeout` and then
`callApi` in `packages/superset-ui-core/src/connection/callApi/callApi.ts:8-23,80-100`,
where `stringify` defaults to `true` and each form field value is transformed with
`valueString = stringify ? JSON.stringify(value) : String(value)`, causing `form_data`
(already a JSON string) to be JSON-stringified again and sent as `'"{...}"'` in the
multipart form body.
5. On the backend, `superset/charts/data/api.py:47-55` handles `/api/v1/chart/data` POST
requests by checking `request.form.get("form_data")` and running
`json.loads(request.form["form_data"])`; with the double-encoded payload this produces a
plain string instead of a dict, so `_create_query_context_from_form` (which expects a
dict, see `superset/tasks/async_queries.py:1-14`) raises a `ValidationError`, and
`ChartDataRestApi.data` catches this and returns `self.response_400(...)`, causing the
chart export HTTP 400 error and preventing the file from downloading.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts
**Line:** 165:169
**Comment:**
*Api Mismatch: `postBlob` forwards `postPayload` without overriding the default `stringify: true`, so string fields are JSON-stringified again by `callApi`. Export callers already pass `form_data` as a JSON string, which becomes double-encoded (extra quotes), and the backend then parses it as a string instead of an object, causing export requests to fail (typically 400) instead of downloading. Pass `stringify: false` (or send a prebuilt `FormData`) to preserve raw form field values.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Code Review Agent Run #c9aadfActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
fix(charts): show user-friendly error for HTTP 413 payload too large
fixes: #36887
SUMMARY
This PR fixes a poor user experience when exporting large chart datasets that exceed nginx/reverse proxy payload size limits (HTTP 413 errors). Previously, these errors would open in a new browser tab showing raw JSON or HTML error pages. Now, users see friendly toast notifications explaining the issue.
Key Changes:
SupersetClient.postBlob()method to enable AJAX-based chart exports with error handlingpostForm) with AJAX requests (postBlob) for all chart exportsdownloadBlobutility function for reuse across componentsTechnical Implementation:
exportChartinexploreUtils/index.jsto use try-catch withSupersetClient.postBlob()useExploreAdditionalActionsMenuto handle errors from CSV, JSON, and Excel exportsChart.jsxto handle errors from table exportsExpected Behavior:
ADDITIONAL INFORMATION
SupersetClient.postBlob()method (internal API)Additional Notes: