fix(matrixify): handle async (202) chart-data responses in Matrixify grid cells#42033
Closed
jenwitteng wants to merge 3 commits into
Closed
fix(matrixify): handle async (202) chart-data responses in Matrixify grid cells#42033jenwitteng wants to merge 3 commits into
jenwitteng wants to merge 3 commits into
Conversation
dosubot
Bot
added
change:frontend
Contributor
Code Review Agent Run #00a593Actionable Suggestions - 0Additional Suggestions - 4
Filtered 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
|
||
| const addListener = (id: string, fn: ListenerFn) => { | ||
| listenersByJobId.set(id, fn); | ||
| const addListener = (id: string, fn: any) => { |
Contributor
There was a problem hiding this comment.
Suggestion: Replace the any parameter type with the existing listener function type so the listener contract is enforced at compile time. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The final TypeScript code still uses any in a changed line (fn: any), which directly violates the custom rule prohibiting any types in new or modified code. A concrete function type should be used instead.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 16)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/middleware/asyncEvent.ts
**Line:** 70:70
**Comment:**
*Custom Rule: Replace the `any` parameter type with the existing listener function type so the listener contract is enforced at compile time.
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
Contributor
|
The suggestion to replace const addListener = (id: string, fn: ListenerFn) => {
listenersByJobId[id] = fn;
};I have implemented this change. Would you like me to check the rest of the comments on this PR and implement fixes for them as well? superset-frontend/src/middleware/asyncEvent.ts |
jenwitteng
changed the title
Comment on lines
+145
to
+149
| logging.warn('received event with status', asyncEvent.status); | ||
| // Non-terminal status: drop this (stale) job listener but keep the | ||
| // abort handler so an unmount/supersede can still reject the pending | ||
| // promise (full cleanup here would leave it hanging on abort). | ||
| removeListener(jobId); |
Contributor
There was a problem hiding this comment.
Suggestion: In the non-terminal status branch, the listener is removed immediately, so if a job emits pending/running before done, the subsequent terminal event has no listener and the waiting promise can hang forever. Keep the listener registered for non-terminal states and only clean it up on terminal states (done, error) or abort. [logic error]
Severity Level: Critical 🚨
❌ Async chart queries can hang waiting for results.
❌ Native filter values never resolve under async queries.
⚠️ Async event retries log noisy 'listener not found' warnings.Steps of Reproduction ✅
1. Enable GLOBAL_ASYNC_QUERIES so async events middleware is active, as wired by `init()`
in `superset-frontend/src/middleware/asyncEvent.ts` (lines ~12-36) and the backend
`AsyncEventsRestApi.events` endpoint in `superset/async_events/api.py:35-41`.
2. Trigger a chart data request that returns HTTP 202 with async job metadata, which flows
into `handleChartDataResponse` in
`superset-frontend/src/components/Chart/chartAction.ts:17-45`; for status 202 it calls
`waitForAsyncData(result, signal)` to await async results.
3. For that job_id, let the backend emit a non-terminal async event (e.g. status "pending"
or "running") via `/api/v1/async_event/` so the frontend receives it and passes it to
`processEvents` in `superset-frontend/src/middleware/asyncEvent.ts:184-207`, which looks
up `listenersByJobId[jobId]` and invokes the listener created inside `waitForAsyncData`.
4. When the listener runs, `switch (asyncEvent.status)` in `asyncEvent.ts:12-41` hits the
`default` branch for the non-terminal status, logs the warning and calls
`removeListener(jobId)` (lines 145-149 in the PR hunk) without resolving or rejecting the
promise; subsequent terminal events (`status: "done"` or `"error"`) for the same job_id
are processed by `processEvents` but find no listener, fall into the retry/race-condition
branch, and ultimately log "listener not found for job_id" without ever settling the
original promise, leaving charts and native filters that relied on `waitForAsyncData`
stuck waiting indefinitely.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/middleware/asyncEvent.ts
**Line:** 145:149
**Comment:**
*Logic Error: In the non-terminal status branch, the listener is removed immediately, so if a job emits `pending`/`running` before `done`, the subsequent terminal event has no listener and the waiting promise can hang forever. Keep the listener registered for non-terminal states and only clean it up on terminal states (`done`, `error`) or abort.
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
Contributor
Code Review Agent Run #50dae7Actionable 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 |
When GLOBAL_ASYNC_QUERIES is enabled, uncached chart-data requests return HTTP 202 with job metadata (channel_id, job_id, result_url) instead of inline results. StatefulChart (used by chart components that fetch their own data) previously treated this metadata as chart data, causing charts to render "No data" until a refresh warmed the cache. StatefulChart now: - Detects HTTP 202 responses via the standardized async-query shape (channel_id + job_id + result_url) - Delegates to an injected handleAsyncChartData hook (wired from ChartRenderer, reusing handleChartDataResponse / waitForAsyncData) - Fails loudly if 202 arrives with no handler, instead of silently showing empty data - Supports abort signals to cancel async waits mid-flight - Cleans up event listeners properly to avoid stale-callback races The handler flows through the existing hooks chain, keeping superset-ui-core decoupled from app-level async-event middleware. Additional improvements: - asyncEvent: refine listener cleanup and stale-callback handling - chartAction: support abort signal propagation for async downloads - StatefulChart: harden lifecycle handling and supersession races - Add comprehensive test coverage for async flows Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
…perset/ prefix - Import ensureAppRoot from navigationUtils instead of pathUtils - Use /explore_json/ instead of /superset/explore_json/ to match upstream convention
…status handling - Replace any type with ListenerFn for type safety - Fix critical bug: keep listener registered for non-terminal status events (pending, running) so they can receive terminal events (done, error) - Previous code removed listener on non-terminal status, causing promises to hang forever when jobs emit status updates before completion
jenwitteng
force-pushed
the
upstream-async-202-squashed
branch
from
July 17, 2026 06:02
c1b17df to
3a92b01
Compare
| json: legacyBody, | ||
| }); | ||
| // Force the legacy API path for this viz type | ||
| (getChartMetadataRegistry as any).mockReturnValue({ |
Contributor
There was a problem hiding this comment.
Suggestion: Replace the any cast with a concrete mock type (or unknown with a safe narrowing) to preserve type checking in this test. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The rule llmcfg-no-any-types forbids new or changed TypeScript code that uses any. This line explicitly casts getChartMetadataRegistry to any, so the violation is real and directly matches the rule.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 16)
(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/chart/components/StatefulChart.test.tsx
**Line:** 808:808
**Comment:**
*Custom Rule: Replace the `any` cast with a concrete mock type (or `unknown` with a safe narrowing) to preserve type checking in this test.
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
Comment on lines
+204
to
211
| if (retriesByJobId[jobId] <= MAX_RETRIES) { | ||
| setTimeout(() => { | ||
| processEvents([asyncEvent]); | ||
| }, RETRY_DELAY * retries); | ||
| }, RETRY_DELAY * retriesByJobId[jobId]); | ||
| } else { | ||
| retriesByJobId.delete(jobId); | ||
| delete retriesByJobId[jobId]; | ||
| logging.warn('listener not found for job_id', asyncEvent.job_id); | ||
| } |
Contributor
There was a problem hiding this comment.
Suggestion: The retry path drops an event permanently after MAX_RETRIES, but there is no fallback to deliver that terminal event when the listener is registered later. This can leave the corresponding async chart request hanging forever (no resolve/reject) if the event arrives before listener registration and registration is delayed (for example in heavy UI work or background-tab throttling). Keep unmatched terminal events in a per-job buffer and flush them from addListener, or explicitly fail pending jobs when retries are exhausted. [race condition]
Severity Level: Critical 🚨
❌ Charts using async queries can hang indefinitely awaiting events.
❌ Native filter controls may never load default async values.
⚠️ AsyncEvent tests don't cover listener-missing race drop behavior.Steps of Reproduction ✅
1. Start the Superset frontend with the GlobalAsyncQueries feature flag enabled so the
async event middleware `init` runs (superset-frontend/src/middleware/asyncEvent.ts:34-60,
with `init();` at line 62 starting polling via `loadEventsFromApi` or WebSocket via
`wsConnect`).
2. Trigger a chart-data request that is handled by `exploreJSON` in
superset-frontend/src/components/Chart/chartAction.ts:734-942 (e.g., loading a chart or
dashboard), which eventually calls `handleChartDataResponse` (lines 700-752) and, when the
HTTP response status is 202, invokes `waitForAsyncData(...)` (lines 36-45 in the excerpt
at 680-759) with the async event payload.
3. In the async middleware, `waitForAsyncData`
(superset-frontend/src/middleware/asyncEvent.ts:107-171) calls `addListener(jobId,
listener)` (lines 78-80, 171) to register a listener in `listenersByJobId`. If an async
event for that job (status DONE or ERROR) is delivered by `loadEventsFromApi` (lines
226-246) before the listener is registered (due to network/UI delay or background tab
throttling), `loadEventsFromApi` still calls `fetchEvents(eventArgs)` and then
`processEvents(events)` (line 234).
4. Inside `processEvents` (superset-frontend/src/middleware/asyncEvent.ts:191-215), when
`listenersByJobId[jobId]` is undefined the else branch (lines 199-211) runs: it increments
`retriesByJobId[jobId]` (lines 201-202), schedules `processEvents([asyncEvent])` via
`setTimeout` (lines 204-207), and after more than `MAX_RETRIES` (line 59) deletes
`retriesByJobId[jobId]` and logs a warning (lines 208-210). The event is dropped
permanently while `setLastId(asyncEvent)` (line 213) advances `lastReceivedEventId`. When
`waitForAsyncData` is eventually called for that job from ChartAction.ts (lines 36-45),
FilterValue.tsx (lines 25-37), or FiltersConfigForm.tsx (lines 24-34), no further events
for that job ever reach the listener, so the Promise returned by `waitForAsyncData` never
resolves or rejects and the associated chart or filter stays stuck in a loading state.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/middleware/asyncEvent.ts
**Line:** 204:211
**Comment:**
*Race Condition: The retry path drops an event permanently after `MAX_RETRIES`, but there is no fallback to deliver that terminal event when the listener is registered later. This can leave the corresponding async chart request hanging forever (no resolve/reject) if the event arrives before listener registration and registration is delayed (for example in heavy UI work or background-tab throttling). Keep unmatched terminal events in a per-job buffer and flush them from `addListener`, or explicitly fail pending jobs when retries are exhausted.
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 generation = pollingGeneration; | ||
| const eventArgs = lastReceivedEventId ? { last_id: lastReceivedEventId } : {}; | ||
| if (listenersByJobId.size) { | ||
| if (Object.keys(listenersByJobId).length) { |
Contributor
There was a problem hiding this comment.
Suggestion: Conditionally skipping fetchEvents when there are no active listeners prevents lastReceivedEventId from advancing during idle periods, so the next active request can trigger a large backlog fetch and replay of stale events. That creates avoidable latency spikes and extra timer churn before current jobs are processed. Continue polling to advance lastReceivedEventId (even without listeners), and only gate listener dispatching. [performance]
Severity Level: Major ⚠️
⚠️ First async chart after idle can incur backlog delay.
⚠️ Async polling performs extra retries for stale unhandled events.
⚠️ Potential spikes in network and timer work on resume.Steps of Reproduction ✅
1. On frontend startup, the async middleware `init`
(superset-frontend/src/middleware/asyncEvent.ts:34-60) is executed (line 62), loading
`lastReceivedEventId` from localStorage (lines 48-52) and starting polling via
`loadEventsFromApi` when `transport === TRANSPORT_POLLING` (lines 54-56).
2. While there are no active async chart jobs, `listenersByJobId` remains empty because
the only place that registers listeners is `waitForAsyncData` (asyncEvent.ts:107-171),
which is invoked from chart and native filter code when an HTTP 202 chart-data response is
received (ChartAction.ts:17-45 at 680-759, FilterValue.tsx:18-37 at 270-329,
FiltersConfigForm.tsx:15-35 at 520-579).
3. During this idle period, any async events emitted on the server-side for this user are
not fetched by the client because `loadEventsFromApi` (asyncEvent.ts:226-246) wraps the
call to `fetchEvents(eventArgs)` and subsequent `processEvents(events)` inside `if
(Object.keys(listenersByJobId).length) { ... }` (lines 229-240); with zero listeners, the
function returns after the guard and `lastReceivedEventId` (used in `eventArgs` at line
228) never advances.
4. When a new chart-data request later receives a 202 and registers a listener via
`waitForAsyncData`, the next invocation of `loadEventsFromApi` finally calls
`fetchEvents(eventArgs)` with a stale `lastReceivedEventId`. The backend returns the
backlog of all events since that cursor, and `processEvents(events)`
(asyncEvent.ts:191-215) iterates them: events for jobs without listeners go through the
retry loop (lines 199-211) before being dropped, and only then are current job events
processed. This creates an avoidable latency spike for the first active async chart and
extra `setTimeout` churn as stale events are retried and logged before the latest job’s
data is delivered.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/middleware/asyncEvent.ts
**Line:** 229:240
**Comment:**
*Performance: Conditionally skipping `fetchEvents` when there are no active listeners prevents `lastReceivedEventId` from advancing during idle periods, so the next active request can trigger a large backlog fetch and replay of stale events. That creates avoidable latency spikes and extra timer churn before current jobs are processed. Continue polling to advance `lastReceivedEventId` (even without listeners), and only gate listener dispatching.
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
Contributor
Code Review Agent Run #49e034Actionable 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 |
Contributor
Author
|
i think i did create base on 6.1 and overwrite to master.. |
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
With
GLOBAL_ASYNC_QUERIESenabled, uncached chart-data requests return HTTP 202 with job metadata (channel_id,job_id,result_url) rather than inline results. Every Matrixify grid cell renders throughStatefulChart(@superset-ui/core), which previously treated that 202 body as chart data — so each cell showed "No data" until a manual refresh warmed the cache.StatefulChartnow detects a 202 and delegates to an injectedhandleAsyncChartDatahook, which polls the async event channel and resolves the cached results. The handler is wired in fromChartRenderer(reusing the existinghandleChartDataResponse/waitForAsyncData) and flows through the standard hooks chain, keepingsuperset-ui-coredecoupled from app-level async-event middleware. If a 202 arrives with no handler,StatefulChartfails loudly instead of silently rendering empty data.Because a matrix fires many concurrent per-cell requests that can be superseded by filter/formData changes and unmounted by scrolling, the change also hardens the async lifecycle:
{ result: [body] }to match the V1 signature the async handler expects (legacy async previously receivedundefined); V1 bodies pass through unchanged.AbortController; a late sync/async result or error is discarded if a newer request has superseded it, so stale data can't overwrite fresh chart data.waitForAsyncDataaccepts anAbortSignal, removes its event listener on abort/unmount (no leaked listeners keeping the poller busy), and cancels the in-flight cached-result download when aborted mid-fetch.fetchDatareads current props via a ref (it's memoized with an empty dep list), so updated filters/formData are used on refetch.waitForAsyncDatarejections are unwrapped so the parsed client-error message survives instead of collapsing to a generic "An error occurred".BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE: Matrixify cells show "No data" on first load when cache is cold.
AFTER: Cells show loading state, then resolve and render charts without requiring a manual refresh.
TESTING INSTRUCTIONS
GLOBAL_ASYNC_QUERIES(and its required Celery worker + cache/results backend + websocket/polling config)force)ADDITIONAL INFORMATION
GLOBAL_ASYNC_QUERIES(behavior only differs when enabled; unchanged when off)