codeant-ai-for-open-source[bot] commented on code in PR #45957:
URL: https://github.com/apache/superset/pull/45957#discussion_r4445373600
##########
superset-embedded-sdk/src/guestTokenRefresh.test.ts:
##########
@@ -122,4 +122,162 @@ describe("guest token refresh", () => {
expect(DEFAULT_TOKEN_REFRESH_RETRY_MS).toBe(10000);
expect(DEFAULT_TOKEN_REFRESH_RETRY_MS).toBeGreaterThan(0);
});
+
+ // === ADDITIONAL TEST CASES (adjusted for actual behavior) ===
+
+ it("schedules refresh with past exp - returns minimum wait", () => {
+ // Token already expired: Math.max(MIN, negative) = MIN, then MIN - buffer
+ const ttl = 100;
+ const exp = Date.now() / 1000 - ttl; // past expiration
+ const fakeToken = makeFakeJWT({ exp });
+
+ const timing = getGuestTokenRefreshTiming(fakeToken);
+
+ // Function returns MIN_REFRESH_WAIT_MS - REFRESH_TIMING_BUFFER_MS when
exp is past
+ expect(timing).toBe(MIN_REFRESH_WAIT_MS - REFRESH_TIMING_BUFFER_MS);
+ });
+
+ it("schedules refresh with very long TTL", () => {
+ const ttl = 86400; // 24 hours
+ 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);
+ });
+
+ it("schedules refresh with TTL just above minimum", () => {
+ // TTL just enough to exceed minimum wait after buffer
+ const ttlMs = MIN_REFRESH_WAIT_MS + REFRESH_TIMING_BUFFER_MS + 1; // just
above minimum
+ const ttl = ttlMs / 1000;
+ const exp = Date.now() / 1000 + ttl;
+ const fakeToken = makeFakeJWT({ exp });
+
+ const timing = getGuestTokenRefreshTiming(fakeToken);
+
+ expect(timing).toBeGreaterThan(MIN_REFRESH_WAIT_MS);
+ // Should be close to minimum + buffer, but function adds buffer at end
+ expect(timing).toBeLessThanOrEqual(ttl * 1000 - REFRESH_TIMING_BUFFER_MS +
100);
+ });
+
+ 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();
+ });
Review Comment:
**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`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=37d7b0d7675c4025838169c29dc08690&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=37d7b0d7675c4025838169c29dc08690&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45957&comment_hash=0e0a1a281e9e8f7ae9401b70d8e1df5b4e1f528aa0c56ff7d7a06b3917ffe013&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45957&comment_hash=0e0a1a281e9e8f7ae9401b70d8e1df5b4e1f528aa0c56ff7d7a06b3917ffe013&reaction=dislike'>๐</a>
##########
superset-embedded-sdk/src/index.test.ts:
##########
@@ -516,25 +516,115 @@ describe("embedDashboard", () => {
});
test("initial fetch failing with no reload tears the iframe down", async ()
=> {
- const initialToken = deferred<string>();
- const mockFetchGuestToken = vi
- .fn()
- .mockImplementation(() => initialToken.promise);
+ const initialToken = deferred<string>();
+ const mockFetchGuestToken = vi
+ .fn()
+ .mockImplementation(() => initialToken.promise);
+
+ const embedding = embedDashboard({
+ id: "test-id",
+ supersetDomain: "https://superset.example.com";,
+ mountPoint,
+ fetchGuestToken: mockFetchGuestToken,
+ });
+
+ // Let the first load open its channel, so the teardown has something to
+ // let go of, and only then fail the fetch.
+ await vi.waitFor(() => expect(switchboards).toHaveLength(1));
+ initialToken.reject(new Error("host down"));
+
+ await expect(embedding).rejects.toThrow("host down");
+ expect(mountPoint.querySelector("iframe")).toBeNull();
+ expect(ports[0].close).toHaveBeenCalled();
+ });
- const embedding = embedDashboard({
- id: "test-id",
- supersetDomain: "https://superset.example.com";,
- mountPoint,
- fetchGuestToken: mockFetchGuestToken,
+ // === ADDITIONAL TEST SUITES ===
+
+ describe("embedDashboard happy path", () => {
+ beforeEach(() => {
+ mockSwitchboard = makeMockSwitchboard();
+ switchboards = [mockSwitchboard];
+ ports = [{ close: vi.fn() }];
+ getImpls = [() => ({ type: "handshake" })];
+ });
+
+ it("successfully embeds dashboard with valid token", async () => {
+ const mockFetch = vi.fn().mockResolvedValue("valid-jwt-token");
+
+ const result = await embedDashboard({
+ id: "test-dashboard",
+ supersetDomain: "http://localhost:8200";,
+ mountPoint,
+ fetchGuestToken: mockFetch,
+ });
+
+ expect(result).toBeDefined();
+ expect(typeof result.getScrollSize).toBe("function");
+ expect(typeof result.unmount).toBe("function");
+ });
});
- // Let the first load open its channel, so the teardown has something to
- // let go of, and only then fail the fetch.
- await vi.waitFor(() => expect(switchboards).toHaveLength(1));
- initialToken.reject(new Error("host down"));
+ describe("embedDashboard security", () => {
+ 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();
+ });
Review Comment:
**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`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f71fd6390866467691e170d414660dbf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f71fd6390866467691e170d414660dbf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45957&comment_hash=265962f8532f78a0b07a7348e556d9984c6b3ba112b11e450c308ee6f6204713&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45957&comment_hash=265962f8532f78a0b07a7348e556d9984c6b3ba112b11e450c308ee6f6204713&reaction=dislike'>๐</a>
##########
superset-embedded-sdk/src/index.test.ts:
##########
@@ -516,25 +516,115 @@ describe("embedDashboard", () => {
});
test("initial fetch failing with no reload tears the iframe down", async ()
=> {
- const initialToken = deferred<string>();
- const mockFetchGuestToken = vi
- .fn()
- .mockImplementation(() => initialToken.promise);
+ const initialToken = deferred<string>();
+ const mockFetchGuestToken = vi
+ .fn()
+ .mockImplementation(() => initialToken.promise);
+
+ const embedding = embedDashboard({
+ id: "test-id",
+ supersetDomain: "https://superset.example.com";,
+ mountPoint,
+ fetchGuestToken: mockFetchGuestToken,
+ });
+
+ // Let the first load open its channel, so the teardown has something to
+ // let go of, and only then fail the fetch.
+ await vi.waitFor(() => expect(switchboards).toHaveLength(1));
+ initialToken.reject(new Error("host down"));
+
+ await expect(embedding).rejects.toThrow("host down");
+ expect(mountPoint.querySelector("iframe")).toBeNull();
+ expect(ports[0].close).toHaveBeenCalled();
+ });
- const embedding = embedDashboard({
- id: "test-id",
- supersetDomain: "https://superset.example.com";,
- mountPoint,
- fetchGuestToken: mockFetchGuestToken,
+ // === ADDITIONAL TEST SUITES ===
+
+ describe("embedDashboard happy path", () => {
+ beforeEach(() => {
+ mockSwitchboard = makeMockSwitchboard();
+ switchboards = [mockSwitchboard];
+ ports = [{ close: vi.fn() }];
+ getImpls = [() => ({ type: "handshake" })];
+ });
+
+ it("successfully embeds dashboard with valid token", async () => {
+ const mockFetch = vi.fn().mockResolvedValue("valid-jwt-token");
+
+ const result = await embedDashboard({
+ id: "test-dashboard",
+ supersetDomain: "http://localhost:8200";,
+ mountPoint,
+ fetchGuestToken: mockFetch,
+ });
+
+ expect(result).toBeDefined();
+ expect(typeof result.getScrollSize).toBe("function");
+ expect(typeof result.unmount).toBe("function");
+ });
});
- // Let the first load open its channel, so the teardown has something to
- // let go of, and only then fail the fetch.
- await vi.waitFor(() => expect(switchboards).toHaveLength(1));
- initialToken.reject(new Error("host down"));
+ describe("embedDashboard security", () => {
+ 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();
+ });
+
+ 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();
+ });
Review Comment:
**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`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f3b1db3cbeea4ce19893dcf97fbe927f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f3b1db3cbeea4ce19893dcf97fbe927f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45957&comment_hash=8fcedb45247c2d4523209afbc72064bf1c009311b7ed4bff58c10d5500fe27da&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45957&comment_hash=8fcedb45247c2d4523209afbc72064bf1c009311b7ed4bff58c10d5500fe27da&reaction=dislike'>๐</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
