You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "dpreuss (via GitHub)" <gi...@apache.org> on 2026/09/02 16:16:42 UTC

[PR] fix(alerts): resolve async chart data when loading report filter values [superset]

dpreuss opened a new pull request, #43793:
URL: https://github.com/apache/superset/pull/43793

   ### SUMMARY
   
   When `GLOBAL_ASYNC_QUERIES` is enabled, `/api/v1/chart/data` often returns **HTTP 202** with a job id instead of synchronous `result[0].data`.
   
   `AlertReportModal` was reading `response.json.result[0].data` directly from `getChartDataRequest`, so selecting a **Dashboard Filter** never populated `optionFilterValues` and the **Value** control stayed disabled/greyed out.
   
   This change routes those responses through `handleChartDataResponse` (same pattern as Drill By), which awaits async results via `waitForAsyncData` when status is 202.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   
   N/A (behavior fix; Value dropdown enables after filter options load)
   
   ### TESTING INSTRUCTIONS
   
   1. Enable feature flags:
      - `ALERT_REPORTS=True`
      - `ALERT_REPORTS_FILTER=True`
      - `GLOBAL_ASYNC_QUERIES=True`
   2. Open **Alerts & Reports** → create/edit a report
   3. Content type = **Dashboard**, pick a dashboard with native filters
   4. Select a dashboard filter
   5. Confirm **Value** becomes enabled and lists options (may take a moment for the async query)
   6. Also smoke-test with `GLOBAL_ASYNC_QUERIES=False` to confirm sync path still works
   
   Unit tests: update `getChartDataRequest` mocks to include `response: { status: 200 }` so the GAQ branch of `handleChartDataResponse` is satisfied under the suite's `isFeatureEnabled: () => true` mock.
   
   ### ADDITIONAL INFORMATION
   
   - [x] Has associated issue: discovered while enabling `ALERT_REPORTS_FILTER` with GAQ on Superset 6.1.0
   - [ ] Required feature flags: `ALERT_REPORTS`, `ALERT_REPORTS_FILTER` (bug only surfaces with `GLOBAL_ASYNC_QUERIES`)
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   
   
   Made with [Cursor](https://cursor.com)


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(alerts): resolve async chart data when loading report filter values [superset]

Posted by "dpreuss (via GitHub)" <gi...@apache.org>.
dpreuss commented on code in PR #43793:
URL: https://github.com/apache/superset/pull/43793#discussion_r3916334987


##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -759,35 +762,35 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
       ownState: {},
     };
 
-    const data = await getChartDataRequest(filterValues).then(response => {
-      const rawData = response.json.result[0].data;
-      let filteredData = rawData;
-
-      if (vizType === 'filter_timecolumn') {
-        // filter for time columns types
-        filteredData = rawData.filter((item: any) => item.dtype === 2);
-      }
+    const { response, json } = await getChartDataRequest(filterValues);
+    const queriesResponse = await handleChartDataResponse(response, json);

Review Comment:
   Does just adding a catch take care of the async issue with GAQ? We want a fix, not just a notification that the data was not returned. 
   



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(alerts): resolve async chart data when loading report filter values [superset]

Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43793:
URL: https://github.com/apache/superset/pull/43793#issuecomment-5512763005

   <!-- Bito Reply -->
   The flagged issue is correct. The `addNativeFilterOptions` function (which appears to be the function containing the logic starting around line 1584 in `AlertReportModal.tsx`) was previously missing a `.catch()` block for the promise returned by `getChartDataRequest`, leading to unhandled rejections on failure.
   
   The fix implemented in the PR (lines 164-166 of the diff) correctly adds a `.catch()` block to handle these failures and display the appropriate error toast:
   
   ```typescript
         .catch(() => {
           addDangerToast(t('Failed to load dashboard filter values.'));
         });
   ```
   
   This ensures that if the async job fails, the user is notified via the dashboard-filter error toast instead of encountering an unhandled promise rejection.
   
   **superset-frontend/src/features/alerts/AlertReportModal.tsx**
   ```
   .catch(() => {
           addDangerToast(t('Failed to load dashboard filter values.'));
         });
   ```


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(alerts): resolve async chart data when loading report filter values [superset]

Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43793:
URL: https://github.com/apache/superset/pull/43793#issuecomment-5512700749

   AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(alerts): resolve async chart data when loading report filter values [superset]

Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43793:
URL: https://github.com/apache/superset/pull/43793#issuecomment-6123866404

   Thanks @dpreuss, routing the 202 through `handleChartDataResponse` is the right fix and matches what Drill By does.
   
   One gap though. You added the `.catch()` toast in `onChangeDashboardFilter`, but `fetchDashboardFilterValues` now awaits `handleChartDataResponse` too and its callers (`addNativeFilterOptions` and the time-filter branch of `onChangeDashboardFilter`) don't catch. If the async job fails, that's an unhandled rejection with no feedback. Could you give it the same treatment?
   
   The `babel-extract` failure looks like the translation bot noise rather than something from this change, so I wouldn't chase it.


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(alerts): resolve async chart data when loading report filter values [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #43793:
URL: https://github.com/apache/superset/pull/43793#discussion_r3916338609


##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -759,35 +762,35 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
       ownState: {},
     };
 
-    const data = await getChartDataRequest(filterValues).then(response => {
-      const rawData = response.json.result[0].data;
-      let filteredData = rawData;
-
-      if (vizType === 'filter_timecolumn') {
-        // filter for time columns types
-        filteredData = rawData.filter((item: any) => item.dtype === 2);
-      }
+    const { response, json } = await getChartDataRequest(filterValues);
+    const queriesResponse = await handleChartDataResponse(response, json);

Review Comment:
   No. Adding a `catch` only prevents an unhandled rejection and displays an error when the async query genuinely fails; it does not resolve GAQ data.
   
   The actual GAQ fix is already the call to `handleChartDataResponse(response, json)`. For a `202` response, that helper waits for the job via `waitForAsyncData` and resolves with the completed query result, so `queriesResponse[0].data` is populated before options are mapped.
   
   The promise should still be caught in `addNativeFilterOptions`, for example:
   
   ```typescript
   getChartDataRequest(filterValues)
     .then(({ response, json }) => handleChartDataResponse(response, json))
     .then(queriesResponse => {
       // Populate options from the completed async result.
     })
     .catch(() => {
       addDangerToast(t('Failed to load dashboard filter values.'));
     });
   ```
   
   Thus, the required data-loading fix is `handleChartDataResponse`; the `catch` is only failure handling. If `waitForAsyncData` rejects because the server-side job fails, there is no valid data to populate, so retry behavior would need to be explicitly added separately rather than silently treating the response as empty.



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(alerts): resolve async chart data when loading report filter values [superset]

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #43793:
URL: https://github.com/apache/superset/pull/43793#issuecomment-5515651973

   ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43793?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :x: Patch coverage is `59.09091%` with `9 lines` in your changes missing coverage. Please review.
   :white_check_mark: Project coverage is 79.28%. Comparing base ([`58f17ef`](https://app.codecov.io/gh/apache/superset/commit/58f17efa6f5c5f781e73f316742971af76c5fad6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`00c4f80`](https://app.codecov.io/gh/apache/superset/commit/00c4f80043b62fb253c5f85e597b626d7be00bf5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 14 commits behind head on master.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/43793?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [...-frontend/src/features/alerts/AlertReportModal.tsx](https://app.codecov.io/gh/apache/superset/pull/43793?src=pr&el=tree&filepath=superset-frontend%2Fsrc%2Ffeatures%2Falerts%2FAlertReportModal.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZlYXR1cmVzL2FsZXJ0cy9BbGVydFJlcG9ydE1vZGFsLnRzeA==) | 59.09% | [9 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43793?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #43793      +/-   ##
   ==========================================
   - Coverage   79.28%   79.28%   -0.01%     
   ==========================================
     Files        2890     2890              
     Lines      167082   167087       +5     
     Branches    38681    38681              
   ==========================================
     Hits       132479   132479              
   - Misses      32108    32113       +5     
     Partials     2495     2495              
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/superset/pull/43793/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [javascript](https://app.codecov.io/gh/apache/superset/pull/43793/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `74.79% <59.09%> (-0.01%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   </details>
   
   [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/43793?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(alerts): resolve async chart data when loading report filter values [superset]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #43793:
URL: https://github.com/apache/superset/pull/43793#issuecomment-5515523226

   <!-- translation-regression-bot -->
   ## ⚠️ Translation Regression Detected
   
   A source change in this PR renamed or reworded strings, invalidating existing translations (they are now `#, fuzzy`) in `sl`. Please resolve the affected `.po` files before merging.
   
   _Note: neither intentionally **deleting** a translatable string nor filling a previously-**untranslated** entry with a fuzzy guess (e.g. an AI backfill) is a regression — only a confirmed translation that a renamed/reworded source string turned fuzzy is flagged here._
   
   | Language | Invalidated translations |
   |----------|-------------------------:|
   | `sl` | 1316 |
   
   ### How to fix
   
   **1. Install dependencies** (if not already set up):
   
   ```bash
   pip install -r superset/translations/requirements.txt
   sudo apt-get install gettext   # or: brew install gettext
   ```
   
   **2. Re-extract strings and sync `.po` files:**
   
   ```bash
   ./scripts/translations/babel_update.sh
   ```
   
   This rewrites `superset/translations/messages.pot` from the current source files and merges the changes into every `.po` file. Strings whose `msgid` changed will be marked `#, fuzzy`.
   
   **3. Resolve the fuzzy entries** in the affected language files (`sl`):
   
   ```bash
   grep -n '#, fuzzy' superset/translations/<lang>/LC_MESSAGES/messages.po
   ```
   
   For each fuzzy entry, either rewrite the `msgstr` to match the new string and remove the `#, fuzzy` line, or clear the `msgstr` to `""` if you cannot provide a translation.
   
   **4. Commit your changes to the `.po` files.**
   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(alerts): resolve async chart data when loading report filter values [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #43793:
URL: https://github.com/apache/superset/pull/43793#discussion_r3916258199


##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -759,35 +762,35 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
       ownState: {},
     };
 
-    const data = await getChartDataRequest(filterValues).then(response => {
-      const rawData = response.json.result[0].data;
-      let filteredData = rawData;
-
-      if (vizType === 'filter_timecolumn') {
-        // filter for time columns types
-        filteredData = rawData.filter((item: any) => item.dtype === 2);
-      }
+    const { response, json } = await getChartDataRequest(filterValues);
+    const queriesResponse = await handleChartDataResponse(response, json);

Review Comment:
   **Suggestion:** Async job failures reject here, but `addNativeFilterOptions` does not catch this promise, producing an unhandled rejection instead of the dashboard-filter error toast. [error handling]
   
   **Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes`
   
   [![Use CodeAnt Skill](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/use-codeant-skill-flat-v2.svg)](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [![Fix in Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=df3f21d48648423ca6cfb06cacce21c9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [![Fix in VSCode Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=df3f21d48648423ca6cfb06cacce21c9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset-frontend/src/features/alerts/AlertReportModal.tsx
   **Line:** 766:766
   **Comment:**
   	*Error Handling: Async job failures reject here, but `addNativeFilterOptions` does not catch this promise, producing an unhandled rejection instead of the dashboard-filter error 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.
   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
   ```
   </details>
   <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43793&comment_hash=b9906c285bab9826bbafdb5dc01fa31b5d60ea0daad144f35772b6e0ea88c5a4&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43793&comment_hash=b9906c285bab9826bbafdb5dc01fa31b5d60ea0daad144f35772b6e0ea88c5a4&reaction=dislike'>👎</a>



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(alerts): resolve async chart data when loading report filter values [superset]

Posted by "dpreuss (via GitHub)" <gi...@apache.org>.
dpreuss commented on code in PR #43793:
URL: https://github.com/apache/superset/pull/43793#discussion_r3916334987


##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -759,35 +762,35 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
       ownState: {},
     };
 
-    const data = await getChartDataRequest(filterValues).then(response => {
-      const rawData = response.json.result[0].data;
-      let filteredData = rawData;
-
-      if (vizType === 'filter_timecolumn') {
-        // filter for time columns types
-        filteredData = rawData.filter((item: any) => item.dtype === 2);
-      }
+    const { response, json } = await getChartDataRequest(filterValues);
+    const queriesResponse = await handleChartDataResponse(response, json);

Review Comment:
   Does just adding a catch take care of the async issue with GAQ? We want a fix, not just a notification that the data was not returned. 
   



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org