Copilot commented on code in PR #38069:
URL: https://github.com/apache/superset/pull/38069#discussion_r2874021912
##########
superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts:
##########
@@ -115,19 +116,18 @@ export default class SupersetClientClass {
return this.fetchCSRFToken();
}
- async postForm(
- endpoint: string,
- payload: Record<string, any>,
- target = '_blank',
- ) {
- if (endpoint) {
+ async postForm(postFormConfig: PostFormConfig) {
+ if (postFormConfig.endpoint || postFormConfig.url) {
await this.ensureAuth();
const hiddenForm = document.createElement('form');
- hiddenForm.action = this.getUrl({ endpoint });
+ hiddenForm.action = this.getUrl({
+ endpoint: postFormConfig.endpoint,
+ url: postFormConfig.url,
+ });
hiddenForm.method = 'POST';
- hiddenForm.target = target;
+ hiddenForm.target = postFormConfig.target ?? '_blank';
const payloadWithToken: Record<string, any> = {
Review Comment:
`SupersetClientClass.postForm` now only accepts a config object, which is a
breaking change for any external consumers still calling `postForm(endpoint,
payload, target)`. At runtime, passing a string will also silently no-op
because `postFormConfig.endpoint/url` will be `undefined` on a string. Consider
supporting the legacy positional signature (with a deprecation path) or
throwing a clear error when the argument is not an object containing `endpoint`
or `url`.
##########
superset-frontend/src/explore/exploreUtils/index.ts:
##########
@@ -377,8 +381,11 @@ export const exportChart = async ({
});
} else {
// Fallback to original behavior for non-streaming exports
- SupersetClient.postForm(url as string, {
- form_data: safeStringify(payload),
+ SupersetClient.postForm({
+ url,
+ payload: {
+ form_data: safeStringify(payload),
+ },
Review Comment:
`url` is typed as `string | null`, but it’s passed directly to
`SupersetClient.postForm({ url, ... })`. Even though runtime control flow
ensures `url` should be non-null here, TypeScript won’t necessarily narrow it
across branches and this can break builds (and also allows
`onStartStreamingExport` to receive a nullable URL type). Consider adding a
single non-null guard after the legacy/v1 branch (or refactoring so `url` is a
plain `string` by this point).
--
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]
