korbit-ai[bot] commented on code in PR #35584:
URL: https://github.com/apache/superset/pull/35584#discussion_r2417494571
##########
superset-frontend/src/utils/export.ts:
##########
@@ -16,34 +16,60 @@
* specific language governing permissions and limitations
* under the License.
*/
-import parseCookie from 'src/utils/parseCookie';
+import { SupersetClient, logging } from '@superset-ui/core';
import rison from 'rison';
-import { nanoid } from 'nanoid';
+import contentDisposition from 'content-disposition';
import { ensureAppRoot } from './pathUtils';
-export default function handleResourceExport(
+export default async function handleResourceExport(
resource: string,
ids: number[],
done: () => void,
- interval = 200,
-): void {
- const token = nanoid();
- const url = ensureAppRoot(
- `/api/v1/${resource}/export/?q=${rison.encode(ids)}&token=${token}`,
+): Promise<void> {
+ const endpoint = ensureAppRoot(
+ `/api/v1/${resource}/export/?q=${rison.encode(ids)}`,
);
- // create new iframe for export
- const iframe = document.createElement('iframe');
- iframe.style.display = 'none';
- iframe.src = url;
- document.body.appendChild(iframe);
+ try {
+ // Use fetch with blob response instead of iframe to avoid CSP frame-src
violations
+ const response = await SupersetClient.get({
+ endpoint,
+ headers: {
+ Accept: 'application/zip, application/x-zip-compressed, text/plain',
+ },
+ parseMethod: 'raw',
+ });
- const timer = window.setInterval(() => {
- const cookie: { [cookieId: string]: string } = parseCookie();
- if (cookie[token] === 'done') {
- window.clearInterval(timer);
- document.body.removeChild(iframe);
- done();
+ // Parse filename from Content-Disposition header
+ const disposition = response.headers.get('Content-Disposition');
+ let fileName = `${resource}_export.zip`;
+
+ if (disposition) {
+ try {
+ const parsed = contentDisposition.parse(disposition);
+ if (parsed?.parameters?.filename) {
+ fileName = parsed.parameters.filename;
+ }
+ } catch (error) {
+ logging.warn('Failed to parse Content-Disposition header:', error);
+ }
}
- }, interval);
+
+ // Convert response to blob and trigger download
+ const blob = await response.blob();
+ const url = window.URL.createObjectURL(blob);
+ const a = document.createElement('a');
+ a.href = url;
+ a.download = fileName;
+ document.body.appendChild(a);
+ a.click();
+ document.body.removeChild(a);
Review Comment:
### Unnecessary DOM layout thrashing <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Synchronous DOM manipulation with appendChild/removeChild causes unnecessary
layout thrashing for the download trigger.
###### Why this matters
The browser performs layout calculations when adding and immediately
removing the anchor element, which is wasteful since the element is never meant
to be visible.
###### Suggested change ∙ *Feature Preview*
Use a more efficient approach that avoids DOM manipulation:
```typescript
const a = document.createElement('a');
a.href = url;
a.download = fileName;
a.style.display = 'none';
a.click();
```
Or use the modern approach:
```typescript
if ('showSaveFilePicker' in window) {
// Use File System Access API when available
} else {
// Fallback to current approach
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fc9c882b-b0eb-4f77-b218-117a8723f669/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fc9c882b-b0eb-4f77-b218-117a8723f669?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fc9c882b-b0eb-4f77-b218-117a8723f669?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fc9c882b-b0eb-4f77-b218-117a8723f669?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fc9c882b-b0eb-4f77-b218-117a8723f669)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:7b7271ec-7f28-4c84-bfcb-fe24163ee3a1 -->
[](7b7271ec-7f28-4c84-bfcb-fe24163ee3a1)
##########
superset-frontend/src/utils/export.ts:
##########
@@ -16,34 +16,60 @@
* specific language governing permissions and limitations
* under the License.
*/
-import parseCookie from 'src/utils/parseCookie';
+import { SupersetClient, logging } from '@superset-ui/core';
import rison from 'rison';
-import { nanoid } from 'nanoid';
+import contentDisposition from 'content-disposition';
import { ensureAppRoot } from './pathUtils';
-export default function handleResourceExport(
+export default async function handleResourceExport(
resource: string,
ids: number[],
done: () => void,
- interval = 200,
-): void {
- const token = nanoid();
- const url = ensureAppRoot(
- `/api/v1/${resource}/export/?q=${rison.encode(ids)}&token=${token}`,
+): Promise<void> {
+ const endpoint = ensureAppRoot(
+ `/api/v1/${resource}/export/?q=${rison.encode(ids)}`,
);
- // create new iframe for export
- const iframe = document.createElement('iframe');
- iframe.style.display = 'none';
- iframe.src = url;
- document.body.appendChild(iframe);
+ try {
+ // Use fetch with blob response instead of iframe to avoid CSP frame-src
violations
+ const response = await SupersetClient.get({
+ endpoint,
+ headers: {
+ Accept: 'application/zip, application/x-zip-compressed, text/plain',
+ },
+ parseMethod: 'raw',
+ });
- const timer = window.setInterval(() => {
- const cookie: { [cookieId: string]: string } = parseCookie();
- if (cookie[token] === 'done') {
- window.clearInterval(timer);
- document.body.removeChild(iframe);
- done();
+ // Parse filename from Content-Disposition header
+ const disposition = response.headers.get('Content-Disposition');
+ let fileName = `${resource}_export.zip`;
+
+ if (disposition) {
+ try {
+ const parsed = contentDisposition.parse(disposition);
+ if (parsed?.parameters?.filename) {
+ fileName = parsed.parameters.filename;
+ }
+ } catch (error) {
+ logging.warn('Failed to parse Content-Disposition header:', error);
+ }
}
- }, interval);
+
+ // Convert response to blob and trigger download
+ const blob = await response.blob();
+ const url = window.URL.createObjectURL(blob);
+ const a = document.createElement('a');
+ a.href = url;
+ a.download = fileName;
+ document.body.appendChild(a);
+ a.click();
+ document.body.removeChild(a);
+ window.URL.revokeObjectURL(url);
Review Comment:
### Mixed responsibilities <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The file download logic is mixed with the resource export logic, violating
the Single Responsibility Principle.
###### Why this matters
This mixing of concerns makes the code less reusable and harder to test. The
file download functionality could be needed elsewhere and should be separated.
###### Suggested change ∙ *Feature Preview*
```typescript
// Extract to separate utility function
async function downloadBlob(blob: Blob, fileName: string): Promise<void> {
const url = window.URL.createObjectURL(blob);
try {
const a = document.createElement('a');
a.href = url;
a.download = fileName;
document.body.appendChild(a);
a.click();
document.body.removeChild(a);
} finally {
window.URL.revokeObjectURL(url);
}
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/284f416e-112a-4eec-ba90-ca52b55d8ef1/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/284f416e-112a-4eec-ba90-ca52b55d8ef1?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/284f416e-112a-4eec-ba90-ca52b55d8ef1?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/284f416e-112a-4eec-ba90-ca52b55d8ef1?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/284f416e-112a-4eec-ba90-ca52b55d8ef1)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:0da801e4-8a2d-4d3d-98f6-05cf9448db01 -->
[](0da801e4-8a2d-4d3d-98f6-05cf9448db01)
##########
superset-frontend/src/utils/export.ts:
##########
@@ -16,34 +16,60 @@
* specific language governing permissions and limitations
* under the License.
*/
-import parseCookie from 'src/utils/parseCookie';
+import { SupersetClient, logging } from '@superset-ui/core';
import rison from 'rison';
-import { nanoid } from 'nanoid';
+import contentDisposition from 'content-disposition';
import { ensureAppRoot } from './pathUtils';
-export default function handleResourceExport(
+export default async function handleResourceExport(
resource: string,
ids: number[],
done: () => void,
- interval = 200,
-): void {
- const token = nanoid();
- const url = ensureAppRoot(
- `/api/v1/${resource}/export/?q=${rison.encode(ids)}&token=${token}`,
+): Promise<void> {
+ const endpoint = ensureAppRoot(
+ `/api/v1/${resource}/export/?q=${rison.encode(ids)}`,
);
- // create new iframe for export
- const iframe = document.createElement('iframe');
- iframe.style.display = 'none';
- iframe.src = url;
- document.body.appendChild(iframe);
+ try {
+ // Use fetch with blob response instead of iframe to avoid CSP frame-src
violations
+ const response = await SupersetClient.get({
+ endpoint,
+ headers: {
+ Accept: 'application/zip, application/x-zip-compressed, text/plain',
+ },
+ parseMethod: 'raw',
+ });
- const timer = window.setInterval(() => {
- const cookie: { [cookieId: string]: string } = parseCookie();
- if (cookie[token] === 'done') {
- window.clearInterval(timer);
- document.body.removeChild(iframe);
- done();
+ // Parse filename from Content-Disposition header
+ const disposition = response.headers.get('Content-Disposition');
+ let fileName = `${resource}_export.zip`;
+
+ if (disposition) {
+ try {
+ const parsed = contentDisposition.parse(disposition);
+ if (parsed?.parameters?.filename) {
+ fileName = parsed.parameters.filename;
+ }
+ } catch (error) {
+ logging.warn('Failed to parse Content-Disposition header:', error);
+ }
}
- }, interval);
+
+ // Convert response to blob and trigger download
+ const blob = await response.blob();
+ const url = window.URL.createObjectURL(blob);
Review Comment:
### Unbounded memory usage for large exports <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Creating object URLs for potentially large export files without size
validation can cause memory issues and browser crashes.
###### Why this matters
Large export files loaded entirely into memory as blobs can exhaust
available RAM, especially on resource-constrained devices or when multiple
exports are triggered simultaneously.
###### Suggested change ∙ *Feature Preview*
Add content-length validation before blob creation and consider streaming
downloads for large files:
```typescript
const contentLength = response.headers.get('Content-Length');
const MAX_BLOB_SIZE = 100 * 1024 * 1024; // 100MB limit
if (contentLength && parseInt(contentLength, 10) > MAX_BLOB_SIZE) {
// Use streaming approach or direct link download
window.location.href = endpoint;
return;
}
const blob = await response.blob();
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4e60cc02-9860-4f69-93a3-92397676e6e8/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4e60cc02-9860-4f69-93a3-92397676e6e8?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4e60cc02-9860-4f69-93a3-92397676e6e8?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4e60cc02-9860-4f69-93a3-92397676e6e8?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4e60cc02-9860-4f69-93a3-92397676e6e8)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:fcda2314-f400-47ef-bad8-1d3be86951eb -->
[](fcda2314-f400-47ef-bad8-1d3be86951eb)
--
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]
