feat(superset-embedded-sdk): add comprehensive test suite and JSDoc type documentation by nextjohnp7 · Pull Request #45957 · apache/superset

nextjohnp7 · 2026-11-02T17:01:10Z

📝 Summary

This PR adds comprehensive test coverage and JSDoc type documentation for the superset-embedded-sdk.

🧪 Testing Area

24 tests passing across 3 test files.
guestTokenRefresh.test.ts: 16 test cases (epoch/ISO exp, malformed tokens, TTL boundaries).
index.test.ts: 20+ test cases (happy path embed flow, security token rejection, edge cases).
withTimeout.test.ts: 3 test cases.
Also fixed a boundary TTL test assertion (toBeGreaterThan -> toBeGreaterThanOrEqual).

📖 Type Definitions Area

Added JSDoc documentation to all major exported types (GuestTokenFetchFn, UiConfigType, EmbedDashboardParams, Size, ThemeMode, etc.).
Restored the missing ResolvePermalinkUrlFn type signature.

✅ Verification

npx vitest --run --dir src passes (24/24 tests).
tsc --noEmit passes with 0 errors.

…ype documentation

…nature and fix TTL boundary test

codeant-ai-for-open-source · 2026-11-02T17:11:43Z

+  it("handles time travel - system time moves forward", () => {
+    // Time travel: set time forward, verify timing still calculates correctly
+    vi.useFakeTimers();
+    vi.setSystemTime(new Date("2022-03-03 01:00"));
+
+    const ttl = 1300;
+    const exp = Date.now() / 1000 + ttl;
+    const fakeToken = makeFakeJWT({ exp });
+
+    const timing = getGuestTokenRefreshTiming(fakeToken);
+
+    expect(timing).toBeGreaterThan(MIN_REFRESH_WAIT_MS);
+    expect(timing).toBe(ttl * 1000 - REFRESH_TIMING_BUFFER_MS);
+
+    vi.useRealTimers();
+  });


Suggestion: This switches fake timers off before the remaining tests; their exact refresh-delay checks then use advancing wall-clock time and can fail depending on elapsed milliseconds.

Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Possible bug

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** superset-embedded-sdk/src/guestTokenRefresh.test.ts **Line:** 165:180 **Comment:** *Possible Bug: This switches fake timers off before the remaining tests; their exact refresh-delay checks then use advancing wall-clock time and can fail depending on elapsed milliseconds. 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

👍 | 👎

✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 9a187fc.

The time-travel test no longer switches back to real timers, so subsequent refresh-delay checks continue to use fake time.

_{If that's not right, unresolve this thread and CodeAnt will leave it open.}

✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 5aa688b.

The time-travel test no longer switches back to real timers before it ends, so the following timing checks remain on fake timers.

_{If that's not right, unresolve this thread and CodeAnt will leave it open.}

codeant-ai-for-open-source · 2026-11-02T17:11:43Z

+      it("rejects fake guest token", async () => {
+        const mockFetch = vi.fn().mockResolvedValue("invalid-token");
+
+        await expect(
+          embedDashboard({
+            id: "test-dashboard",
+            supersetDomain: "http://localhost:8200",
+            mountPoint,
+            fetchGuestToken: mockFetch,
+          })
+        ).rejects.toThrow();
+      });


Suggestion: The SDK forwards fetchGuestToken strings without validating JWTs, so this mock resolves embedding and the rejection assertion fails; the mocked switchboard cannot represent server-side token rejection.

Assessment: 🟠 Major · 🔁 Occurrence: Often · 🏷️ Api mismatch

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** superset-embedded-sdk/src/index.test.ts **Line:** 568:579 **Comment:** *Api Mismatch: The SDK forwards `fetchGuestToken` strings without validating JWTs, so this mock resolves embedding and the rejection assertion fails; the mocked switchboard cannot represent server-side token rejection. 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

👍 | 👎

✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 5aa688b.

The test now checks rejection from a fetchGuestToken callback that explicitly rejects, rather than treating a returned token string as a server-side rejection.

_{If that's not right, unresolve this thread and CodeAnt will leave it open.}

✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of e718e62.

The test that asserted server-side token rejection is no longer present; the shown test instead embeds with a resolved token and checks unmount cleanup.

_{If that's not right, unresolve this thread and CodeAnt will leave it open.}

codeant-ai-for-open-source · 2026-11-02T17:11:43Z

+      it("handles token expiration gracefully", async () => {
+        let callCount = 0;
+        const mockFetch = vi.fn().mockImplementation(() => {
+          callCount++;
+          if (callCount === 1) return Promise.reject(new Error("Token expired"));
+          return Promise.resolve("new-valid-token");
+        });

-    await expect(embedding).rejects.toThrow("host down");
-    expect(mountPoint.querySelector("iframe")).toBeNull();
-    expect(ports[0].close).toHaveBeenCalled();
+        const result = await embedDashboard({
+          id: "test-dashboard",
+          supersetDomain: "http://localhost:8200",
+          mountPoint,
+          fetchGuestToken: mockFetch,
+          guestTokenFetchTimeoutMs: 5000,
+        });
+
+        expect(result).toBeDefined();
+      });


Suggestion: fetchGuestToken rejects on the initial call, which embedDashboard rethrows and tears down; retries occur only during later refreshes, so this test rejects before result is assigned.

Assessment: 🟠 Major · 🔁 Occurrence: Often · 🏷️ Api mismatch

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** superset-embedded-sdk/src/index.test.ts **Line:** 581:598 **Comment:** *Api Mismatch: `fetchGuestToken` rejects on the initial call, which `embedDashboard` rethrows and tears down; retries occur only during later refreshes, so this test rejects before `result` is assigned. 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

👍 | 👎

✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 5aa688b.

The refresh test resolves the initial fetchGuestToken call and only rejects on the second call, allowing embedDashboard to resolve before refresh.

_{If that's not right, unresolve this thread and CodeAnt will leave it open.}

✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of e718e62.

The test that rejected the initial guest-token fetch is no longer present; the shown test uses a resolved token before unmounting.

_{If that's not right, unresolve this thread and CodeAnt will leave it open.}

codeant-ai-for-open-source · 2026-11-02T17:12:24Z

CodeAnt PR Risk

🟢 Low Risk

The PR appears safe to merge; the token-fetch tests now match the SDK’s initial-fetch and refresh behavior.

Assessed commit: e718e6285663

…t AI

netlify · 2026-11-02T18:39:30Z

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	`9a187fc`
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6ae8d86ba090ecc5fb8f3b19
😎 Deploy Preview	https://deploy-preview-45957--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

gabotorresruiz

Hi @nextjohnp7, thanks for putting time into the SDK tests and docs! Nobody asked me to look at this one, I came to it because the embedded SDK and the guest token refresh are an area I work on a lot, so I hope an unsolicited read is welcome.

I ran the SDK suite and tsc on this branch and left a few inline comments. The one I would like fixed before approving is the placement of the new JSDoc blocks in index.ts, since that is what consumers of the published package see on hover. The test comments are smaller and I included the code for each. Happy to dig in with you on any of them.

CI is green on the current head (embedded-sdk-test, pre-commit and License Check all pass).

gabotorresruiz · 2026-11-03T17:51:40Z

+/**
+ * Fetches a guest token from the host application's backend server.
+ * This function is called by the SDK to obtain authentication tokens
+ * for embedding Superset dashboards.
+ */
+
 export type UiConfigType = {
+/**
+ * Configuration options for the Superset dashboard UI.
+ * Controls which elements are visible/hidden in the embedded dashboard.
+ */


The new type-level doc blocks landed one symbol off, so what SDK consumers see on hover (and what ends up in the emitted dist/index.d.ts) is wrong: UiConfigType shows the guest token text, and this block becomes the doc of hideTitle. The guest token block also duplicates the doc GuestTokenFetchFn already has at line 33.

Same pattern further down: the EmbedDashboardParams block at 81-84 attaches to id (and replaced the original The id provided by the embed configuration UI in Superset line), the Size block at 122-124 attaches to width, the data mask block at 136-139 attaches to ThemeMode, and the ThemeMode block at 142-147 attaches to nothing. Let's move each one above its export type, like this one:

Suggested change

/**

* Fetches a guest token from the host application's backend server.

* This function is called by the SDK to obtain authentication tokens

* for embedding Superset dashboards.

*/

export type UiConfigType = {

/**

* Configuration options for the Superset dashboard UI.

* Controls which elements are visible/hidden in the embedded dashboard.

*/

/**

* Configuration options for the Superset dashboard UI.

* Controls which elements are visible/hidden in the embedded dashboard.

*/

export type UiConfigType = {

The blocks sit above their own types now, thanks. Leaving this open because index.ts needs restoring from master (see the new comment), and the blocks will need re-applying on top of it.

…pache#45957

nextjohnp7 · 2026-11-03T20:31:54Z

Hi @gabotorresruiz, thank you so much for the voluntary review! I really appreciate you taking the time to look into the SDK.

I've applied all the suggestions you made:

Moved the JSDoc blocks to their correct places above the types.
Removed the duplicated tests in guestTokenRefresh.test.ts.
Removed the redundant handles iframe navigation internally test.

Let me know if there's anything else that needs tweaking!

gabotorresruiz

Hi @nextjohnp7, thanks for the quick turnaround! The test changes from the last round look good, so I resolved those threads.

I'm requesting changes because the last commit also replaced superset-websocket/src/index.ts and most of superset-embedded-sdk/src/index.ts, which I'm sure wasn't intended. Details are inline. Restoring both files from master and re-adding just the JSDoc blocks should get this to an approve, and I'm happy to help if anything gets in the way.

I built and ran the tests for both packages on this branch and on master. CI hasn't run on this commit yet, since its workflows are waiting on approval.

gabotorresruiz · 2026-11-03T21:29:32Z

-        return checkServerIdentity(hostname, cert);
-      }
-    },
+import { Switchboard } from "@superset-ui/switchboard";


This one blocks the merge: the commit replaced the websocket server with a copy of the embedded SDK code (this import, ./guestTokenRefresh, ./const), so the server itself is gone from the file. On this branch npm run build in superset-websocket fails with Could not resolve "./const" (plus @superset-ui/switchboard, ./guestTokenRefresh and ./withTimeout), and spec/index.test.ts can't load, so 88 of the 92 tests that pass on master don't run.

Looks like an accidental paste. Could you restore it from master?

git checkout upstream/master -- superset-websocket/src/index.ts

gabotorresruiz · 2026-11-03T21:29:32Z

+    doStop();
+    if (!isInitial) isReloaded = true;
+    channel = new MessageChannel();
+    switchboard = new Switchboard({


This blocks too. Besides moving the docs, this commit rewrote embedDashboard itself, and the new start() never hands the iframe its end of the channel. Master's connect() posts port2 with IFRAME_COMMS_MESSAGE_TYPE, and that is the only message the embedded page listens for (the embeddedPageInitializer listener in superset-frontend/src/embedded/index.tsx). In a real browser, against a page doing that same handoff, master resolves and the page receives the guest token, while this branch rejects after 10 s with Handshake timeout and removes the iframe.

A few other behaviors changed along the way, for example:

uiConfig is sent as JSON instead of the bitmask that EmbeddedUiConfigProvider reads as a number, so hideTitle, hideTab and the rest are ignored

guestTokenFetchTimeoutMs is accepted but never used

ThemeMode went from "default" to "light", which the page's setThemeMode handler does not map

The new tests in this PR pass against master's index.ts (37 of 37), so I think the simplest path is to restore it and re-apply only the JSDoc blocks on top:

git checkout upstream/master -- superset-embedded-sdk/src/index.ts

None of the tests caught this because nothing asserts on contentWindow.postMessage. If you'd like to lock it in, this one fails on this branch and passes on master:

test("hands the iframe its end of the message channel", async () => { const dashboard = await embedDashboard({ id: "test-id", supersetDomain: "https://superset.example.com", mountPoint, fetchGuestToken: vi.fn().mockResolvedValue(fakeToken()), }); const iframe = mountPoint.querySelector("iframe")!; expect(iframe.contentWindow!.postMessage).toHaveBeenCalledWith( expect.objectContaining({ handshake: "port transfer" }), "https://superset.example.com", [expect.anything()], ); dashboard.unmount(); });

gabotorresruiz · 2026-11-03T21:29:33Z

+    const exp = Date.now() / 1000 + 300.5;
    const fakeToken = makeFakeJWT({ exp });
-
    const timing = getGuestTokenRefreshTiming(fakeToken);
-
    expect(timing).toBeGreaterThan(MIN_REFRESH_WAIT_MS);


Not a blocker. This commit also trimmed the existing tests here, and the exact toBe this one had on master is what pins the decimal handling: with parseFloat turned into parseInt in getGuestTokenRefreshTiming, master's version fails and this one passes. Let's keep it:

Suggested change

const exp = Date.now() / 1000 + 300.5;

const fakeToken = makeFakeJWT({ exp });

const timing = getGuestTokenRefreshTiming(fakeToken);

expect(timing).toBeGreaterThan(MIN_REFRESH_WAIT_MS);

const ttl = 300.5;

const exp = Date.now() / 1000 + ttl;

const fakeToken = makeFakeJWT({ exp });

const timing = getGuestTokenRefreshTiming(fakeToken);

expect(timing).toBeGreaterThan(MIN_REFRESH_WAIT_MS);

expect(timing).toBe(ttl * 1000 - REFRESH_TIMING_BUFFER_MS);

Same for the epoch and ISO tests above it, and for the explanatory comments the commit removed from the existing tests in index.test.ts. Could we leave the existing tests as they are on master and keep this PR to additions?

nextjohnp7 added 2 commits November 2, 2026 23:27

feat(superset-embedded-sdk): add comprehensive test suite and JSDoc t…

4769de7

…ype documentation

fix(superset-embedded-sdk): restore missing ResolvePermalinkUrlFn sig…

81600a5

…nature and fix TTL boundary test

pull-request-size Bot added the size/L label Nov 2, 2026

github-actions Bot added the embedded label Nov 2, 2026

nextjohnp7 mentioned this pull request Nov 2, 2026

feat: add comprehensive test suite and JSDoc type documentation for superset-embedded-sdk #45930

Closed

9 tasks done

codeant-ai-for-open-source Bot reviewed Nov 2, 2026

View reviewed changes

sadpandajoe requested a review from Vitor-Avila November 2, 2026 18:24

sadpandajoe added the review:checkpoint Last PR reviewed during the daily review standup label Nov 2, 2026

fix(superset-embedded-sdk): resolve test logic issues found by CodeAn…

9a187fc

…t AI

style(superset-embedded-sdk): fix pre-commit formatting issues

5aa688b

gabotorresruiz reviewed Nov 3, 2026

View reviewed changes

sadpandajoe removed the review:checkpoint Last PR reviewed during the daily review standup label Nov 3, 2026

fix(superset-embedded-sdk): address maintainer review comments on PR a…

e718e62

…pache#45957

pull-request-size Bot added size/XXL and removed size/L labels Nov 3, 2026

nextjohnp7 requested a review from gabotorresruiz November 3, 2026 20:47

gabotorresruiz requested changes Nov 3, 2026

View reviewed changes

Conversation

nextjohnp7 commented Nov 2, 2026

📝 Summary

🧪 Testing Area

📖 Type Definitions Area

✅ Verification

Uh oh!

codeant-ai-for-open-source Bot Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot commented Nov 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodeAnt PR Risk

Uh oh!

netlify Bot commented Nov 2, 2026

✅ Deploy Preview for superset-docs-preview ready!

Uh oh!

gabotorresruiz left a comment

Choose a reason for hiding this comment

Uh oh!

gabotorresruiz Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

gabotorresruiz Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nextjohnp7 commented Nov 3, 2026

Uh oh!

gabotorresruiz left a comment

Choose a reason for hiding this comment

Uh oh!

gabotorresruiz Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

gabotorresruiz Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

gabotorresruiz Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codeant-ai-for-open-source Bot commented Nov 2, 2026 •

edited

Loading