Copilot commented on code in PR #36771:
URL: https://github.com/apache/superset/pull/36771#discussion_r2662898420
##########
superset-frontend/src/components/StreamingExportModal/useStreamingExport.ts:
##########
@@ -39,6 +41,40 @@ interface StreamingExportParams {
const NEWLINE_BYTE = 10; // '\n' character code
+/**
+ * Ensures URL has the application root prefix for subdirectory deployments.
+ * Applies makeUrl to relative paths that don't already include the app root.
+ * This guards against callers forgetting to prefix URLs when using native
fetch.
+ */
+const ensureUrlPrefix = (url: string): string => {
+ const appRoot = applicationRoot();
+ // Protocol-relative URLs (//example.com/...) should pass through unchanged
+ if (url.startsWith('//')) {
+ return url;
+ }
+ // Only process relative URLs (starting with /)
+ if (!url.startsWith('/')) {
+ return url;
+ }
+ // If no app root configured, return as-is
+ if (!appRoot) {
+ return url;
+ }
+ // If URL already has the app root prefix, return as-is
+ // Use strict check to avoid false positives with sibling paths (e.g., /app2
when appRoot is /app)
+ // Also handle query strings and hashes (e.g., /superset?foo=1 or
/superset#hash)
+ if (
+ url === appRoot ||
+ url.startsWith(`${appRoot}/`) ||
+ url.startsWith(`${appRoot}?`) ||
+ url.startsWith(`${appRoot}#`)
+ ) {
+ return url;
+ }
+ // Apply prefix via makeUrl
+ return makeUrl(url);
+};
Review Comment:
The `ensureUrlPrefix` function is designed as a defensive guard to catch
unprefixed URLs, but it relies on `makeUrl()` which internally calls
`ensureAppRoot()`. However, the primary fix for streaming exports is to call
`makeUrl()` at the point where the URL is constructed (in ResultSet/index.tsx
and exportChart in exploreUtils/index.js). This creates a potential issue: if a
caller already prefixes the URL before passing it to `startExport`,
`ensureUrlPrefix` will not double-prefix it (which is good), but if a caller
forgets to prefix, this guard will catch it. However, this defensive approach
may mask developer errors where URLs should be properly prefixed at the source.
Consider documenting in the `StreamingExportParams` interface that the `url`
parameter should already be properly prefixed with the application root when
passed to `startExport`, and that `ensureUrlPrefix` is only a defensive
fallback. This would make the contract clearer for future developers.
--
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]
