Conversation
|
|
Reviewed the hardening commits. Direction is right and both of osmarluz's issues are genuinely addressed. Backend contract verified against 1. HIGH: the failure path becomes a 5-second retry storm, with no backoff
On failure the fetcher returns // node_modules/@superset-ui/embedded-sdk/lib/guestTokenRefresh.js:37-39
const ttl = isValidDate ? Math.max(MIN_REFRESH_WAIT_MS /* 10000 */, exp.getTime() - Date.now()) : DEFAULT_TOKEN_EXP_MS
return ttl - REFRESH_TIMING_BUFFER_MS // 5000For an expired token That is exactly the rate-limit pressure flagged as the trigger for Issue 1. As written, the change trades "the loop dies permanently" for "the loop hammers the endpoint permanently", and the user still sees broken charts in the meantime because the token we keep emitting is expired. Only a genuinely transient failure benefits from the current fallback. Suggested fix: retry with exponential backoff inside the callback (the SDK awaits it, so that is the only timing lever we control), cap the number of attempts, and only then resolve with 2. MEDIUM: mint failures are completely silent
The repo already has the pattern ( 3. MEDIUM: codegen was not run, so the mutation is untyped
|
AllanMichay
left a comment
AllanMichay
left a comment
There was a problem hiding this comment.
Review — guest token refresh
Diagnosis is correct and the approach is sound. The SDK's unmount() only does mountPoint.replaceChildren() (@superset-ui/embedded-sdk/lib/index.js:159-163) — it never clears its own setTimeout(refreshGuestToken, …), so a never-resolving promise really is the only way to stop the loop. Good catch, that isn't obvious without reading the SDK.
Six inline comments. Two I'd want addressed before merge:
Blocking-ish
Dashboard.tsxcleanup —cancel()during the initial embed hangsembedDashboardforever, leaving an orphaned iframe and an open MessageChannel. Fires on every dev mount under StrictMode.fetchSupersetGuestToken.ts—pnpm codegenwasn't run, so the mutation is untyped andsrc/generated/graphql.tsxis stale. Green CI doesn't prove otherwise (see the comment).
Should fix
3. Bare catch {} — silently regresses to the exact bug this PR fixes.
4. initialToken = '' default → jwtDecode throws inside the SDK.
5. haltRefreshLoop needs a why-comment.
6. Test can't distinguish the fix from the fallback.
Nit, description only: "runs with a no-cache fetch policy so a token is never served stale from the Apollo cache" — mutations never read the cache. no-cache here only prevents writing the token into it, which is a fine reason, just not the stated one.
Checked, all fine: ApolloClient<object> matches convention · client in the effect deps is safe (useApolloClient is stable) · org scoping is handled by the auth link's x-lago-organization at call time · inline gql + client.mutate in a .ts file has precedent (cacheUtils.ts:56) and codegen.yml globs src/**/*.ts, so the document is picked up.
The factory's unit tests are genuinely good — the cancellation cases in particular. Fix 1 and 2 and this is ready.
| mount() | ||
|
|
||
| return () => { | ||
| fetchGuestToken.cancel() |
There was a problem hiding this comment.
cancel() during the initial embed leaves an orphaned iframe + open MessageChannel forever.
The SDK does await Promise.all([fetchGuestToken(), mountIframe()]) (index.js:146). Cleanup order here is fetchGuestToken.cancel() → embedded?.unmount(). If cleanup fires before mount() resolves, fetchGuestToken() never settles → embedDashboard never resolves → embedded stays null → unmount() never runs. The iframe was already placed via replaceChildren (index.js:142), the Switchboard port is open, and the closure is pinned permanently.
React.StrictMode is on (src/main.tsx:116), so this fires on every dev mount. In prod the window is now a full network round-trip (~100-500ms) wide, so a dashboard tab switch or a fast navigate hits it.
The orphan race pre-exists (embedded is assigned after cleanup already ran), but this upgrades it from "resolves, GC eventually collects" to "pinned forever".
Suggested fix — let cancel() settle an in-flight call and hang only subsequent ones, plus a disposed guard in the effect:
// factory: only the `if (cancelled)` entry guard halts.
// Post-await, return `lastToken` regardless so embedDashboard can finish.
// Dashboard.tsx
let disposed = false
const mount = async () => {
embedded = await embedDashboard({ /* … */ })
if (disposed) {
embedded.unmount()
return
}
// …
}
return () => {
disposed = true
fetchGuestToken.cancel()
embedded?.unmount()
// …
}Note this inverts the 'never resolves if cancelled while a mutation is in flight' test, which currently locks in the hang.
| lastToken = data?.createSupersetGuestToken?.guestToken ?? lastToken | ||
|
|
||
| return cancelled ? haltRefreshLoop() : lastToken | ||
| } catch { |
There was a problem hiding this comment.
Bare catch {} gives zero observability.
If the mutation starts failing, dashboards silently regress to exactly the stale-token bug this PR fixes — no signal anywhere, and the next report is again "leave analytics open, change a filter, get an error".
At minimum log it; ideally report it. Same applies to the ?? lastToken branch on line 35 when the API returns a null token — that's an API contract violation worth surfacing, not swallowing.
|
|
||
| export type FetchSupersetGuestToken = (() => Promise<string>) & { cancel: () => void } | ||
|
|
||
| const haltRefreshLoop = (): Promise<string> => new Promise<string>(() => {}) |
There was a problem hiding this comment.
Worth a why-comment.
A deliberately never-resolving promise is the most surprising line in the PR, and it reads like a bug at a glance. One line explaining that the SDK's unmount() never clears its refresh setTimeout (@superset-ui/embedded-sdk/lib/index.js:156-163), so declining to resolve is the only way to break the chain, saves the next reader a trip through node_modules.
| request: { | ||
| query: CREATE_SUPERSET_GUEST_TOKEN, | ||
| variables: { input: { dashboardId: 'dash-1' } }, | ||
| }, |
There was a problem hiding this comment.
This assertion can't fail.
The mutation mock returns 'token-1', and dashboard.guestToken in dashboardsData is also 'token-1'. So await expect(config.fetchGuestToken()).resolves.toBe('token-1') (line 139) passes whether the token was freshly minted or the mutation blew up and the fallback kicked in — i.e. it passes even if this PR's entire mechanism is broken.
Return 'refreshed-token-1' from the mutation mock so the assertion actually proves the mutation path ran.
Also missing: a case asserting cancel() runs on unmount.
The analytics dashboards embed Apache Superset in an iframe. Superset only authorizes an embedded viewer through a short-lived guest token (5 minutes by default). The embed SDK re-invokes the `fetchGuestToken` callback shortly before the current token expires so the session can continue without interruption. Our callback returned the single token captured at page load, so the SDK kept handing back an already-expired token. After a few minutes every chart and filter request failed, and the only way to recover was a full page reload, which also reset the active tab and filters. This is the "leave analytics open, change a filter, get an error" behaviour customers reported. Extract a `fetchGuestToken` helper that mints a fresh, organization-scoped token on every call through the new `createSupersetGuestToken` mutation, and wire it into the dashboard embed. Each renewal now returns a valid token, keeping long analytics sessions alive without a reload. The mutation runs with a no-cache fetch policy so a token is never served stale from the Apollo cache.
## Context Merging main brought in Dashboard.test.tsx, which asserted the old behaviour where fetchGuestToken returned the token embedded in the supersetDashboards query response. The token is now minted on demand through the createSupersetGuestToken mutation, so the test failed on an unmocked mutation. ## Description Add a MockedProvider mock for the createSupersetGuestToken mutation returning the expected token, keeping the "embeds with the correct config" assertion meaningful against the new refresh flow.
## Context The Superset embed SDK drives token renewal itself: it calls our fetchGuestToken callback shortly before the current token expires and only re-arms the timer after the callback resolves. Two SDK behaviours made our renewal fragile: - The refresh loop stops permanently if the callback rejects, and our callback did a bare mutation with no error handling. A single network blip, expired session or rate-limited response killed renewal until a full page reload, intermittently reintroducing the very bug this change fixes. - unmount() only clears the iframe, never the pending refresh timer. Now that the callback performs a real mutation, the leaked loop kept minting tokens for the rest of the SPA session, and every dashboard revisit leaked another. ## Description Make the guest-token fetcher resilient and cancellable. On a failed mint it returns the last known token (seeded with the initial token) instead of rejecting, so the SDK keeps scheduling retries. It exposes a cancel() that the effect cleanup calls alongside unmount(); once cancelled the callback returns a never-settling promise, halting the refresh loop without an unhandled rejection.
The mutation document was hand-rolled in `fetchSupersetGuestToken.ts` and `pnpm codegen` was never run, so `createSupersetGuestToken` appeared nowhere in the committed `src/generated/graphql.tsx`. `data` off `client.mutate` was therefore `any`, leaving `data?.createSupersetGuestToken?.guestToken` checked against nothing — a field rename or schema drift would compile cleanly and fail at runtime. Green CI was not evidence to the contrary: the codegen job regenerates the file in place and runs tsc, it never diffs against the checked-in copy. The mutation is now available on lago-api main (getlago/lago-api#5855, c7ca20ec8), so the types can finally be generated. Regenerate `src/generated/graphql.tsx` and consume the generated artifacts: `CreateSupersetGuestTokenDocument` with `CreateSupersetGuestTokenMutation` / `CreateSupersetGuestTokenMutationVariables` as type parameters. The exported hand-rolled `CREATE_SUPERSET_GUEST_TOKEN` constant is gone; both test files now reference the generated document, so the mutation mock is consistent with the query mock alongside it. The bare `gql` block stays in the module — unassigned, the way `Dashboard.tsx` declares its query — because codegen scans `src/**` for operations and would stop emitting the types if the document only existed in `src/generated`. Rebasing onto main folded in the rate-card schema this commit previously carried as incidental drift, so the generated diff is now exactly the guest token mutation: input, payload, `Mutation` field, operation types, document and hook. Nothing else in the file moves.
## Context Review surfaced two problems with the previous hardening, both rooted in SDK behaviour. The fetcher returned `lastToken` on failure, and the SDK derives the next refresh delay from the token we hand back (`guestTokenRefresh.js`): `ttl = max(10s, exp - now)`, then `ttl - 5s`. Once that token is expired `exp - now` is negative, so the delay floors at 5s. A mint that kept failing was therefore re-invoked every 5 seconds for as long as the analytics tab stayed open — roughly 720 mutations an hour, each making the backend re-authenticate against Superset. That is the very rate-limit pressure that killed renewal in the first place, so the previous commit traded "the loop dies permanently" for "the loop hammers permanently", with the user still looking at broken charts either way. Cancellation was also too broad. The SDK awaits our callback inside `Promise.all([fetchGuestToken(), mountIframe()])` (`index.js:146`) and places its iframe in the DOM before that settles. Because a cancelled in-flight call returned a never-settling promise, cleanup during the initial embed left `embedDashboard` unresolved forever: `embedded` stayed null, so cleanup had no handle to unmount, and the iframe stayed in the DOM with its Switchboard port open. StrictMode fires this on every dev mount; in production the window is a full network round-trip wide. ## Description Retry inside the callback — the SDK awaits it, so that is the only timing lever we control. Up to 3 attempts with a 1s/2s backoff rides out a transient blip, then a per-streak cooldown (5s doubling to a 5 minute cap) stretches the cycle instead of hammering the endpoint. Consecutive-failure state resets as soon as a mint succeeds. `cancel()` wakes any pending backoff so effect cleanup never waits it out. Only the entry guard halts the refresh loop now. Past that point a call always settles, so `embedDashboard` can finish and hand back the `EmbeddedDashboard`. `Dashboard.tsx` gains a `disposed` flag that unmounts an embed which completed after cleanup already ran, closing the orphan-iframe race for good. Failures are no longer silent. The first failure of each streak is reported with an `errorType: 'SupersetGuestTokenMintError'` tag; later attempts in the same streak stay quiet so a sustained outage doesn't bury the signal. GraphQL errors are left to the errorLink in `apolloClient/init.ts`, which already captures them — only network errors, which reach no other reporter, are captured here. A successful response carrying no token violates the non-null schema contract, so that is reported too via `captureMessage`. `initialToken` is now required rather than defaulting to `''`: the SDK runs `jwtDecode` on whatever we return, which throws on a non-JWT. `mount()` also gets a `catch` that reports, since nothing else observed that promise — a throw there was an unhandled rejection and a silently blank dashboard.
## Context Review demonstrated that the tests added alongside the refresh fix could not fail, so they were locking in nothing. `settledOrPending` raced the subject against `Promise.race([promise, Promise.resolve(PENDING)])`. The sentinel is already fulfilled, so it always won the race whether or not the subject settled a microtask later. Deleting the cancellation guard the two cancel tests claimed to cover left all five tests green. `Dashboard.test.tsx` mocked the mutation with `guestToken: 'token-1'`, the same value the dashboards query seeds as `dashboard.guestToken`. Asserting `fetchGuestToken()` resolved to `'token-1'` therefore passed identically when the mutation never ran, or threw and fell back to the seed. Forcing every mint to throw left all nine tests green. ## Description Replace the racing helper with one that flushes microtasks and 0ms timers, then reports whether the subject actually settled — an assertion that can now fail. Give the Dashboard mutation mock a distinct `'refreshed-token'` so the assertion only passes when the token really came from the mutation. Cover the behaviour the previous commit introduced: retry attempts and their backoff schedule, the capped attempt count, the per-streak cooldown before a re-mint, the streak resetting after a success, reporting exactly once per streak, GraphQL errors deliberately left to the errorLink, a token-less response treated as a contract violation, and cancellation abandoning a pending backoff instead of waiting it out. `Dashboard.test.tsx` gains the orphaned-iframe regression: an embed that only resolves after cleanup ran must still be unmounted, and must not wire up filter observation. Each new guarantee was checked by breaking the implementation and confirming the matching test fails: neutering the cancel entry guard, the streak cooldown, the sleeper wake-up, and the `disposed` unmount each fail exactly one test and no others.
## Context Review noted that the `cancel()` wiring itself was never asserted. The factory's cancellation semantics are covered in `fetchSupersetGuestToken.test.ts`, but nothing checked that `Dashboard.tsx` actually calls it: deleting `fetchGuestToken.cancel()` from the effect cleanup left the entire suite green. That line is the only thing that halts the refresh chain. The SDK's `unmount()` is just `mountPoint.replaceChildren()` (`index.js:159-163`) — it never clears the `setTimeout(refreshGuestToken, …)` it scheduled — so without the cancel every dashboard switch or revisit leaves another perpetual minting chain behind for the rest of the SPA session. ## Description Capture `fetchGuestToken` off the embed config, unmount, then assert a subsequent call never settles. A cancelled fetcher hangs by design, and the mutation mock would otherwise resolve it, so the assertion fails the moment cleanup stops cancelling. `hasSettled` mirrors the helper in `fetchSupersetGuestToken.test.ts`, flushing microtasks and 0ms timers on real timers rather than jest's fake ones. Checked the same way as the surrounding tests: removing `fetchGuestToken.cancel()` from the cleanup fails this test and no other.
581d2bf to
33ee87d
Compare
|
@ansmonjol @AllanMichay I have addressed your comments, could you take another look please?. Thanks. |
## Context The comments added across these four files ran far ahead of what the codebase does. `src/` outside generated code and tests sits at 3.7% comment lines with a median file at 0.4%, and test files at 2.1%; `fetchSupersetGuestToken.ts` had reached 27.3%, roughly seven times the average and a bracket only a handful of gnarly utility files occupy. Several blocks ran eleven lines to justify a single constant, and most of the inline test comments restated the test name or the assertion directly beneath them. ## Description Drop the comments that carried no information the code does not already give: the line-by-line derivation of the SDK's refresh timing, the single-sleeper-slot note, the gloss on `isFirstFailureOfStreak` (the name says it), and the inline test comments that repeated their own `it` titles. Keep — condensed from seven or eleven lines to two or three — only what a reader cannot get without opening `node_modules`: why the retry and cooldown constants exist at all, why `haltRefreshLoop` deliberately never settles, why the cancellation check is an entry guard and nothing past it may hang, and why `reportMintError` skips GraphQL errors. In the tests, only the note that the mutation mock's token must differ from the query's seed survives, since reverting that value silently guts the assertion it feeds. Comment lines added by this branch drop from 83 to 30. No code changed: the diff touches comments only, and the suite, prettier, eslint and tsc are unaffected.
|
ansmonjol
left a comment
ansmonjol
left a comment
There was a problem hiding this comment.
Reviewed the diff against the @superset-ui/embedded-sdk@0.3.0 internals and the app's Apollo link chain. The core fix (mint on demand, never reject, cancel on teardown) is right, and the reasoning in the description matches what the SDK actually does.
One blocking issue: the new disposed teardown calls embedded.unmount(), which is mountPoint.replaceChildren() on a node that a later effect run may already own. Details inline.
Three other things worth resolving before merge: the catch branch in fetchSupersetGuestToken is unreachable under the app's errorPolicy: 'all', the mutation is not marked silentError so every failed mint raises a danger toast, and the local retry loop stacks on top of Apollo's RetryLink. The rest are non-blocking.
| // The SDK mounts its iframe before `embedDashboard` resolves, so cleanup that | ||
| // ran while this was in flight had no `embedded` to unmount. | ||
| if (disposed) { | ||
| embedded.unmount() |
There was a problem hiding this comment.
The intent here is right, but unmount() is not scoped to this embed. In the SDK it is literally:
function unmount() {
mountPoint.replaceChildren(); // lib/index.js
}a blind wipe of whatever the mount node currently holds. And mountIframe() does mountPoint.replaceChildren(iframe) on that same node. Both effect runs get the same document.getElementById(mountId), so a late-landing embed clears its successor's iframe.
Concretely: the effect re-runs while embed #1 is still awaiting its mint (currentMembership?.organization.id flips from undefined to the id once getCurrentUserInfos resolves; a supersetDashboards refetch returning a fresh guestToken does the same, see the deps comment below). Cleanup sets disposed = true, run #2 mounts iframe2. When embed #1 finally resolves, this branch runs unmount() and iframe2 is gone. The user gets a permanently blank dashboard until a full reload. Before this PR the stale embed was merely orphaned and the visible one kept working.
Note that dashboardRef does not protect against this: cleanup sets dashboardRef.current = '' before every re-run, so dashboard?.id === dashboardRef?.current is never true on re-entry.
Worth spelling out what the branch buys in each case, because only one of the three needs it:
| Case | Mount node state when the late unmount fires | Result |
|---|---|---|
| Component unmount (navigate away) | React already removed the div | no-op |
| Effect re-run, successor mounted | successor already did replaceChildren(iframe2) |
destroys iframe2 |
Effect re-run, new run early-returns on !dashboard |
iframe1 still attached, orphaned | correct, the real justification |
(The refresh chain itself is already fully handled by fetchGuestToken.cancel(): the next refreshGuestToken() hits the entry guard and gets haltRefreshLoop(). unmount() never touched the timer or the Switchboard port.)
Suggested fix, a generation guard so only the run that still owns the node may clear it:
const runIdRef = useRef(0)
useEffect(() => {
if (!dashboard || dashboard?.id === dashboardRef?.current) {
return
}
runIdRef.current += 1
const runId = runIdRef.current
// ...
// `unmount()` is `mountPoint.replaceChildren()` on the shared node, so only
// clear it if no later run has taken it over.
if (disposed) {
if (runIdRef.current === runId) {
embedded.unmount()
}
return
}The increment has to sit after the early return, so case 3 (new run bails on !dashboard) still unmounts.
The alternative is giving each run its own child container so the node is never shared, which is immune by construction, but it changes the DOM structure under #mountId and the generation guard is the smaller change.
| return lastToken | ||
| } | ||
|
|
||
| // `guestToken` is non-null in the schema: an empty payload is a contract |
There was a problem hiding this comment.
This branch is unreachable, and as a result every real backend error takes the "contract violation" path.
src/core/apolloClient/init.ts:312 sets defaultOptions.mutate.errorPolicy: 'all', ApolloClient.mutate merges it in, and QueryManager only throws an ApolloError for GraphQL errors when errorPolicy === 'none'. So when the backend answers { data: { createSupersetGuestToken: null }, errors: [...] }, mintToken() resolves with undefined, the catch below never runs, and reportMintError's error instanceof ApolloError && graphQLErrors.length > 0 de-dup guard never fires. Execution lands here instead, on captureMessage(..., { level: 'error' }), which is exactly the second Sentry report the PR set out to avoid, tagged as a schema violation.
The comment is also not accurate: createSupersetGuestToken is Maybe<SupersetGuestToken> in the generated schema, so a null payload is the schema's normal error shape rather than a violation.
And since the resolved errors array is never inspected, permanent failures (bad Superset config, revoked access) get retried three times as if they were transient.
Suggest reading errors off the mutation result and branching on it: GraphQL errors present, stay quiet and let the errorLink report; no errors and no token, then it really is a contract violation.
| >({ | ||
| mutation: CreateSupersetGuestTokenDocument, | ||
| variables: { input: { dashboardId } }, | ||
| fetchPolicy: 'no-cache', |
There was a problem hiding this comment.
This needs context: { silentError: true }.
errorLink (src/core/apolloClient/init.ts:261) fires addToast({ severity: 'danger', translateKey: 'text_622f7a3dc32ce100c46a5154' }) for every non-silenced GraphQL error; only AUTH_ERRORS and Forbidden are auto-silenced. So a Superset outage returning a generic error produces three danger toasts per invocation, then three more after each 5s, 10s, 20s cooldown, indefinitely, on a page the user is only reading. Before this PR a token failure was silent.
The established pattern is context: { silentError: true } (or silentErrorCodes), used at roughly 15 call sites, e.g. src/components/customers/CustomerActivityLogs.tsx:53.
| const isFirstFailureOfStreak = (attempt: number): boolean => | ||
| failureStreak === 0 && attempt === 1 | ||
|
|
||
| for (let attempt = 1; attempt <= MAX_MINT_ATTEMPTS; attempt += 1) { |
There was a problem hiding this comment.
This loop stacks on top of Apollo's RetryLink, so the effective request count is higher than the three the description reasons about.
init.ts:151-179 installs new RetryLink({ delay: { initial: 150, max: 5000, jitter: true }, attempts: { max: 3, retryIf: isNetworkError } }), which is four attempts per client.mutate for exactly the network-error class this loop also retries. During an outage each fetchGuestToken() issues 3 x 4 = 12 requests over roughly 15 to 20 seconds, which works against the stated goal of not hammering the backend.
Either pass context: { disableRetry: true } (the link honours it) and keep this loop, or drop the loop and let the link do the retrying.
| dashboardRef.current = '' | ||
| } | ||
| }, [dashboard, currentMembership?.organization.id, dashboardTitle, mountId]) | ||
| }, [dashboard, currentMembership?.organization.id, client, dashboardTitle, mountId]) |
There was a problem hiding this comment.
Depending on the whole dashboard object makes the effect re-run on every supersetDashboards response. The backend mints a fresh JWT per response, so guestToken differs, the normalized cache entry changes, data.supersetDashboards gets a new identity, and the useMemo yields a new dashboard. With cache-and-network that happens on every cold load with a warm IndexedDB cache: the whole iframe is torn down and re-embedded, the user loses their active tab and filters, and a second mint fires.
The effect only needs dashboard.id and dashboard.embeddedId; guestToken is read once as a seed and does not need to be fresh. Narrowing to primitives stops the churn and also shrinks the race window for the teardown issue above:
}, [
dashboard?.id,
dashboard?.embeddedId,
currentMembership?.organization.id,
client,
dashboardTitle,
mountId,
])The currentMembership?.organization.id flip still re-runs the effect once per cold load, so this does not remove the need for the guard, but it removes the recurring case.
| const sleep = (ms: number): Promise<void> => | ||
| new Promise<void>((resolve) => { | ||
| const timer = setTimeout(() => { | ||
| wakeSleeper = null |
There was a problem hiding this comment.
wakeSleeper is a single shared slot and is unconditionally nulled by whichever timer fires first.
Today the SDK's refresh chain is strictly serial so no two sleeps overlap, but nothing here enforces that. If two fetchGuestToken() calls ever overlap (a second embed sharing the fetcher, or any future caller), sleep A's callback nulls the slot while sleep B is pending, so a later cancel() sees null, never clears B's timer, and B's retry loop keeps running after teardown. Starting B while A is pending leaks A's timer the same way.
A per-sleep local (a Set of wakers, or an AbortSignal threaded through) removes the hazard without changing the current behaviour.
| expect(mockCaptureException).toHaveBeenCalledTimes(1) | ||
| }) | ||
|
|
||
| it('THEN leaves GraphQL errors to the errorLink rather than reporting them twice', async () => { |
There was a problem hiding this comment.
This test asserts a rejection the real client cannot produce, so it green-lights the unreachable guard flagged in fetchSupersetGuestToken.ts.
makeClient swaps in a bare jest.Mock, so mockRejectedValue(new ApolloError({ graphQLErrors })) works here. With the app's real errorPolicy: 'all', mutate resolves with { data: { createSupersetGuestToken: null }, errors } and never rejects. The branch under test is dead in production while the suite reports it as covered.
The test that would catch it does not exist: a resolved result carrying errors. Worth noting that the "token-less response" case at line 203 is silently exercising the real GraphQL-error path while asserting contract-violation semantics.
|
|
||
| // The SDK mounts its iframe before `embedDashboard` resolves, so an embed still | ||
| // in flight at cleanup time is orphaned unless it is unmounted once it lands. | ||
| it('THEN tears down an embed that only resolves after cleanup ran', async () => { |
There was a problem hiding this comment.
This test only covers the full-component-unmount path, where React removes the #mountId div anyway, so unmount() cannot do damage. The case that matters for the teardown issue is an effect re-run: embed #1 in flight, effect re-runs, embed #2 mounts into the same node, then embed #1 resolves.
Rough shape, driving the re-run through currentMembership?.organization.id, which is the trigger that fires on every cold load:
it('THEN does not clear the mount node when a later run already took it over', async () => {
let resolveFirstEmbed: (value: unknown) => void = () => {}
const firstUnmount = jest.fn()
mockEmbedDashboard
.mockReturnValueOnce(
new Promise((resolve) => {
resolveFirstEmbed = resolve
}),
)
.mockResolvedValue({ unmount: mockUnmount, observeDataMask: mockObserveDataMask })
const { rerender } = renderAnalytics()
await waitFor(() => expect(mockEmbedDashboard).toHaveBeenCalledTimes(1))
// Effect re-run: the second embed takes over the mount node.
mockCurrentMembership.mockReturnValue({ organization: { id: 'org-2' } })
rerender(/* same element */)
await waitFor(() => expect(mockEmbedDashboard).toHaveBeenCalledTimes(2))
resolveFirstEmbed({ unmount: firstUnmount, observeDataMask: jest.fn() })
await waitFor(() => expect(mockEmbedDashboard).toHaveBeenCalledTimes(2))
// The stale embed must not clear the node the live embed now owns.
expect(firstUnmount).not.toHaveBeenCalled()
})Using a distinct firstUnmount mock per embed is what makes the assertion meaningful, since the current shared mockUnmount cannot tell the two embeds apart. Keep the existing test as the case-3 coverage.
| ) | ||
|
|
||
| // Whether `promise` has settled once microtasks and 0ms timers have flushed. | ||
| const hasSettled = async (promise: Promise<unknown>): Promise<boolean> => { |
There was a problem hiding this comment.
This helper is byte-identical to the one in fetchSupersetGuestToken.test.ts:34, apart from setTimeout(resolve, 0) versus jest.advanceTimersByTimeAsync(0). Any future change to the settle-detection logic has to be applied twice or the two suites drift. Worth extracting one helper that takes the flush strategy as an argument.
| captureMessage: (...args: unknown[]) => mockCaptureMessage(...args), | ||
| })) | ||
|
|
||
| const makeClient = (mutate: jest.Mock) => ({ mutate }) as unknown as ApolloClient<object> |
There was a problem hiding this comment.
makeClient here and tokenResponse at line 29 have inferred return types. .agents/docs/typescript-conventions.md asks for explicit ones, and every other new helper in this PR (hasSettled, drainFailedInvocation, streakCooldown, haltRefreshLoop) has them. Typing tokenResponse against CreateSupersetGuestTokenMutation in particular would stop the fake payload drifting from the generated shape.
4 participants
Context
The analytics dashboards embed Apache Superset in an iframe. Superset only authorizes an embedded viewer through a short-lived guest token (5 minutes by default). The embed SDK re-invokes the
fetchGuestTokencallback shortly before the current token expires so the session can continue without interruption.Our callback returned the single token captured at page load, so the SDK kept handing back an already-expired token. After a few minutes every chart and filter request failed, and the only way to recover was a full page reload, which also reset the active tab and filters. This is the "leave analytics open, change a filter, get an error" behaviour customers reported.
Description
Extract a
fetchGuestTokenhelper that mints a fresh, organization-scoped token on every call through the newcreateSupersetGuestTokenmutation, and wire it into the dashboard embed. Each renewal now returns a valid token, keeping long analytics sessions alive without a reload. The mutation runs with ano-cachefetch policy so the token is never written into the Apollo cache, which this app persists to IndexedDB — a short-lived credential has no business being stored there.The failure and teardown paths are where the SDK's behaviour dictates ours:
unmount()only clears the iframe, never the pending refresh timer, so the effect cleanup calls acancel()that makes subsequent calls return a never-settling promise — the only way to halt a chain that ignores rejections. A call already in flight still settles: the SDK awaits our callback insidePromise.all([fetchGuestToken(), mountIframe()])and puts its iframe in the DOM before that resolves, so hanging there would leaveembedDashboardunresolved and the iframe orphaned with its Switchboard port open.Dashboard.tsxalso unmounts an embed that lands after cleanup already ran.errorType: 'SupersetGuestTokenMintError'tag; the rest of the streak stays quiet so a sustained outage doesn't bury the signal. GraphQL errors are left to the errorLink, which already captures them; network errors and token-less responses (a non-null schema contract violation) are captured here.pnpm codegenhas been run and the regenerated types committed, so the mutation is consumed throughCreateSupersetGuestTokenDocumentwith generated type parameters rather than a hand-rolled document and ananypayload.