Skip to content
40 changes: 40 additions & 0 deletions src/generated/graphql.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15872,6 +15872,13 @@ export type SupersetDashboardsQueryVariables = Exact<{ [key: string]: never; }>;

export type SupersetDashboardsQuery = { __typename?: 'Query', supersetDashboards: Array<{ __typename?: 'SupersetDashboard', id: string, embeddedId: string, dashboardTitle: string, guestToken: string }> };

export type CreateSupersetGuestTokenMutationVariables = Exact<{
input: CreateSupersetGuestTokenInput;
}>;


export type CreateSupersetGuestTokenMutation = { __typename?: 'Mutation', createSupersetGuestToken?: { __typename?: 'SupersetGuestToken', guestToken: string } | null };

export type GetApiKeyToEditQueryVariables = Exact<{
apiKeyId: Scalars['ID']['input'];
}>;
Expand Down Expand Up @@ -42541,6 +42548,39 @@ export type SupersetDashboardsQueryHookResult = ReturnType<typeof useSupersetDas
export type SupersetDashboardsLazyQueryHookResult = ReturnType<typeof useSupersetDashboardsLazyQuery>;
export type SupersetDashboardsSuspenseQueryHookResult = ReturnType<typeof useSupersetDashboardsSuspenseQuery>;
export type SupersetDashboardsQueryResult = Apollo.QueryResult<SupersetDashboardsQuery, SupersetDashboardsQueryVariables>;
export const CreateSupersetGuestTokenDocument = gql`
mutation createSupersetGuestToken($input: CreateSupersetGuestTokenInput!) {
createSupersetGuestToken(input: $input) {
guestToken
}
}
`;
export type CreateSupersetGuestTokenMutationFn = Apollo.MutationFunction<CreateSupersetGuestTokenMutation, CreateSupersetGuestTokenMutationVariables>;

/**
* __useCreateSupersetGuestTokenMutation__
*
* To run a mutation, you first call `useCreateSupersetGuestTokenMutation` within a React component and pass it any options that fit your needs.
* When your component renders, `useCreateSupersetGuestTokenMutation` returns a tuple that includes:
* - A mutate function that you can call at any time to execute the mutation
* - An object with fields that represent the current status of the mutation's execution
*
* @param baseOptions options that will be passed into the mutation, supported options are listed on: https://www.apollographql.com/docs/react/api/react-hooks/#options-2;
*
* @example
* const [createSupersetGuestTokenMutation, { data, loading, error }] = useCreateSupersetGuestTokenMutation({
* variables: {
* input: // value for 'input'
* },
* });
*/
export function useCreateSupersetGuestTokenMutation(baseOptions?: Apollo.MutationHookOptions<CreateSupersetGuestTokenMutation, CreateSupersetGuestTokenMutationVariables>) {
const options = {...defaultOptions, ...baseOptions}
return Apollo.useMutation<CreateSupersetGuestTokenMutation, CreateSupersetGuestTokenMutationVariables>(CreateSupersetGuestTokenDocument, options);
}
export type CreateSupersetGuestTokenMutationHookResult = ReturnType<typeof useCreateSupersetGuestTokenMutation>;
export type CreateSupersetGuestTokenMutationResult = Apollo.MutationResult<CreateSupersetGuestTokenMutation>;
export type CreateSupersetGuestTokenMutationOptions = Apollo.BaseMutationOptions<CreateSupersetGuestTokenMutation, CreateSupersetGuestTokenMutationVariables>;
export const GetApiKeyToEditDocument = gql`
query getApiKeyToEdit($apiKeyId: ID!) {
apiKey(id: $apiKeyId) {
Expand Down
35 changes: 31 additions & 4 deletions src/pages/dashboards/Dashboard.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { gql } from '@apollo/client'
import { gql, useApolloClient } from '@apollo/client'
import { captureException } from '@sentry/react'
import { embedDashboard, EmbeddedDashboard } from '@superset-ui/embedded-sdk'
import { debounce } from 'lodash'
import { useEffect, useMemo, useRef } from 'react'
Expand All @@ -15,6 +16,7 @@ import { useSupersetDashboardsQuery } from '~/generated/graphql'
import { useInternationalization } from '~/hooks/core/useInternationalization'
import { useCurrentUser } from '~/hooks/useCurrentUser'
import '~/main.css'
import { createFetchSupersetGuestToken } from '~/pages/dashboards/fetchSupersetGuestToken'
import ErrorImage from '~/public/images/maneki/error.svg'
import { PageHeader } from '~/styles'

Expand Down Expand Up @@ -42,6 +44,7 @@ export type DashboardProps = {
const Dashboard = ({ contentTitle, dashboardTitle, dashboardTitleTestKey }: DashboardProps) => {
const { translate } = useInternationalization()
const { currentMembership } = useCurrentUser()
const client = useApolloClient()

const dashboardRef = useRef<string>('')

Expand All @@ -63,6 +66,7 @@ const Dashboard = ({ contentTitle, dashboardTitle, dashboardTitleTestKey }: Dash
}

let embedded: null | EmbeddedDashboard = null
let disposed = false

const persistFilters = isFeatureFlagActive(FeatureFlags.SUPERSET_PERSISTENT_FILTERS)
// Filter persistence key is scoped to the org from the URL slug (resolved
Expand All @@ -87,6 +91,12 @@ const Dashboard = ({ contentTitle, dashboardTitle, dashboardTitleTestKey }: Dash
}, 500)
: null

const fetchGuestToken = createFetchSupersetGuestToken(
client,
dashboard.id,
dashboard.guestToken,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things about the seed token.

First, the description says the no-cache policy keeps the token out of a cache the app persists to IndexedDB. That is true of the mutation, but supersetDashboards still selects guestToken and SupersetDashboard has an id, so the token is normalized and setupCachePersistor (src/core/apolloClient/cachePersistor.ts:47) writes it to IndexedDB anyway. The no-cache policy is still worth having, but the claim in the description does not hold as written.

Second, initialToken is currently only a fallback. The SDK does await Promise.all([fetchGuestToken(), mountIframe()]) before emitting to the iframe, so the very first render now blocks on a network mutation even though a valid token is already in hand. init.ts:38 sets TIMEOUT = 300000 and timeoutLink sits after retryLink, so each link-level attempt gets its own five-minute budget; multiplied by this fetcher's three attempts, a hung backend can leave the analytics page blank for a very long time where the old code rendered instantly.

Returning initialToken directly on the first invocation and minting only from the first refresh onward keeps the fix and takes a round trip off first paint.

)

const mount = async () => {
const mountPoint = document.getElementById(mountId)

Expand All @@ -107,7 +117,7 @@ const Dashboard = ({ contentTitle, dashboardTitle, dashboardTitleTestKey }: Dash
id: dashboard.embeddedId,
supersetDomain: lagoSupersetUrl,
mountPoint,
fetchGuestToken: async () => dashboard?.guestToken,
fetchGuestToken,
dashboardUiConfig: {
hideTitle: true,
emitDataMasks: persistFilters,
Expand All @@ -119,21 +129,38 @@ const Dashboard = ({ contentTitle, dashboardTitle, dashboardTitleTestKey }: Dash
iframeSandboxExtras: ['allow-top-navigation', 'allow-popups-to-escape-sandbox'],
})

// 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()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

if (debouncedSaveFilters) {
embedded.observeDataMask(debouncedSaveFilters)
}

dashboardRef.current = dashboard.id
}

mount()
// Nothing else observes this promise: uncaught, a bad token from Superset is an
// unhandled rejection and a silently blank dashboard.
mount().catch((mountError) => {
captureException(mountError, {
tags: { errorType: 'SupersetDashboardMountError', component: 'Dashboard' },
extra: { dashboardId: dashboard.id },
})
})

return () => {
disposed = true
fetchGuestToken.cancel()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 nullunmount() 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.

debouncedSaveFilters?.cancel()
embedded?.unmount()
Comment thread
ancorcruz marked this conversation as resolved.
dashboardRef.current = ''
}
}, [dashboard, currentMembership?.organization.id, dashboardTitle, mountId])
}, [dashboard, currentMembership?.organization.id, client, dashboardTitle, mountId])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return (
<>
Expand Down
71 changes: 69 additions & 2 deletions src/pages/dashboards/__tests__/Dashboard.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { screen, waitFor } from '@testing-library/react'

import { GENERIC_PLACEHOLDER_TEST_ID } from '~/components/designSystem/GenericPlaceholder'
import { getItemFromLS, setItemFromLS } from '~/core/utils/localStorage'
import { SupersetDashboardsDocument } from '~/generated/graphql'
import { CreateSupersetGuestTokenDocument, SupersetDashboardsDocument } from '~/generated/graphql'
import { render, TestMocksType } from '~/test-utils'

import Dashboard, { DASHBOARD_MOUNT_TEST_ID } from '../Dashboard'
Expand Down Expand Up @@ -79,6 +79,14 @@ const dashboardsData = {

const successMock: TestMocksType = [
{ request: { query: SupersetDashboardsDocument }, result: { data: dashboardsData } },
{
request: {
query: CreateSupersetGuestTokenDocument,
variables: { input: { dashboardId: 'dash-1' } },
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Must differ from the seed above, or the assertion passes on the fallback too.
result: { data: { createSupersetGuestToken: { guestToken: 'refreshed-token' } } },
},
]

const errorMock: TestMocksType = [
Expand All @@ -105,6 +113,26 @@ const renderRevenue = (mocks: TestMocksType = successMock) =>
{ mocks },
)

// Whether `promise` has settled once microtasks and 0ms timers have flushed.
const hasSettled = async (promise: Promise<unknown>): Promise<boolean> => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let settled = false

promise.then(
() => {
settled = true
},
() => {
settled = true
},
)

await new Promise<void>((resolve) => {
setTimeout(resolve, 0)
})

return settled
}

describe('Dashboard', () => {
beforeEach(() => {
jest.clearAllMocks()
Expand Down Expand Up @@ -136,7 +164,7 @@ describe('Dashboard', () => {
expect(config.supersetDomain).toBe('https://localhost:8089')
expect(config.mountPoint).toBe(document.getElementById('superset-lago-dashboard'))
expect(config.dashboardUiConfig.hideTitle).toBe(true)
await expect(config.fetchGuestToken()).resolves.toBe('token-1')
await expect(config.fetchGuestToken()).resolves.toBe('refreshed-token')
})
})

Expand Down Expand Up @@ -228,5 +256,44 @@ describe('Dashboard', () => {

expect(mockUnmount).toHaveBeenCalled()
})

// The SDK's `unmount()` clears the iframe but not its refresh `setTimeout`
// (`index.js:159-163`), so cleanup has to cancel the fetcher itself.
it('THEN cancels the guest token fetcher so the refresh chain stops', async () => {
const { unmount } = renderAnalytics()

await waitFor(() => expect(mockEmbedDashboard).toHaveBeenCalledTimes(1))

const { fetchGuestToken } = mockEmbedDashboard.mock.calls[0][0]

unmount()

// A cancelled fetcher never settles; the mutation mock would have resolved it.
expect(await hasSettled(fetchGuestToken())).toBe(false)
})

// 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 () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let resolveEmbed: (value: unknown) => void = () => {}

mockEmbedDashboard.mockReturnValue(
new Promise((resolve) => {
resolveEmbed = resolve
}),
)

const { unmount } = renderAnalytics()

await waitFor(() => expect(mockEmbedDashboard).toHaveBeenCalledTimes(1))

unmount()
expect(mockUnmount).not.toHaveBeenCalled()

resolveEmbed({ unmount: mockUnmount, observeDataMask: mockObserveDataMask })

await waitFor(() => expect(mockUnmount).toHaveBeenCalledTimes(1))
expect(mockObserveDataMask).not.toHaveBeenCalled()
})
})
})
Loading
Loading