Repository navigation
test(embedded-sdk): cross-document test rig for the navigation fix - #44608
Conversation
superset-embedded-sdk/index.test.ts mocks MessageChannel and Switchboard, so it cannot exercise the parts of apache#44470 that only exist between two real documents: the port-transfer handshake, what survives a navigation the dashboard makes on its own, and how many guest tokens the host's endpoint is actually asked for. This rig loads the built UMD bundle in headless Chromium against a host app and a stand-in Superset page on two origins, over the real @superset-ui/switchboard. `node testrig/drive.mjs` runs 39 checks covering the internal-navigation reload, the PortClosedError rejections, the refresh-timer handoff, and both directions of the race between the initial token fetch and a navigation racing it. Not wired into CI: it needs a Chromium binary, and apache#44470's own unit tests already gate the merge. Kept as a manual/local tool per rusackas's review, split out of apache#44470 to keep that PR to just the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| const newestSrc = Math.max( | ||
| ...readdirSync(join(sdkRoot, "src")).map((f) => | ||
| statSync(join(sdkRoot, "src", f)).mtimeMs, | ||
| ), | ||
| ); | ||
| if (existsSync(bundle) && statSync(bundle).mtimeMs > newestSrc) return; | ||
| console.log("building the sdk bundle…"); |
There was a problem hiding this comment.
Suggestion: The stale check ignores webpack.config.js and dependency changes, so the rig can serve an older SDK bundle while reporting results for current source code.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Possible bug
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-embedded-sdk/testrig/drive.mjs
**Line:** 530:536
**Comment:**
*Possible Bug: The stale check ignores `webpack.config.js` and dependency changes, so the rig can serve an older SDK bundle while reporting results for current source code.
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
There was a problem hiding this comment.
Confirmed, checked buildIfStale() directly: it only diffs src/* mtimes against the built bundle, never webpack.config.js. Real, lowest priority of the bunch given how rarely that file changes relative to src/.
There was a problem hiding this comment.
Fixed in 338ead8. buildIfStale() now checks BUILD_INPUTS (webpack.config.js, babel.config.js, tsconfig.json, package.json, package-lock.json) alongside src/.
| const server = await start(); | ||
| const browser = await launchBrowser(); | ||
| try { |
There was a problem hiding this comment.
Suggestion: If launchBrowser() fails, execution never enters try, so the HTTP servers and Chromium temporary directory remain active and can break later runs.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Resource leak
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-embedded-sdk/testrig/drive.mjs
**Line:** 551:553
**Comment:**
*Resource Leak: If `launchBrowser()` fails, execution never enters `try`, so the HTTP servers and Chromium temporary directory remain active and can break later runs.
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 fixThere was a problem hiding this comment.
Confirmed. main()'s try/finally only wraps the code after both await start() and await launchBrowser() succeed, so a rejection from either one skips server.stop()/browser.stop() entirely. Same root cause as the orphaned-chromium finding below, worth fixing together.
There was a problem hiding this comment.
Fixed in 338ead8. server and browser are both set up inside main()'s try now, so the finally cleans up whichever one did come up even if the other fails.
| export function start() { | ||
| return new Promise((resolve) => { | ||
| let listening = 0; |
There was a problem hiding this comment.
Suggestion: start() never rejects when either port is unavailable, so a bind error becomes an unhandled server error instead of a controlled startup failure.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Possible bug
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-embedded-sdk/testrig/server.mjs
**Line:** 158:160
**Comment:**
*Possible Bug: `start()` never rejects when either port is unavailable, so a bind error becomes an unhandled server error instead of a controlled startup failure.
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 fixThere was a problem hiding this comment.
Confirmed. start()'s new Promise((resolve) => {...}) has no reject path at all, on a port-bind failure listen()'s callback never fires (that's a 'listening' callback, not a general completion callback), so the promise hangs forever, and the server's own unhandled 'error' event throws separately. Real, and plausible to hit in practice (re-running the rig while a previous instance is still bound to the port).
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 338ead8.
start() now wraps each server’s listen() in a rejecting promise and waits with Promise.allSettled(). On bind failure it closes any listening server and throws a controlled startup error.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 219ca1a.
start() now wraps each server’s listen() in a rejecting promise and waits with Promise.allSettled(). On bind failure it closes any listening server and throws a controlled startup error.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of a017548.
start() now wraps each server’s listen() in a rejecting promise and waits with Promise.allSettled(). On bind failure it closes any listening server and throws a controlled startup error.
If that's not right, unresolve this thread and CodeAnt will leave it open.
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #a69ba8
Actionable Suggestions - 5
-
superset-embedded-sdk/testrig/host.html - 2
- Unguarded heldFirstFetch deref · Line 121-129
- Unguarded null dashboard deref · Line 160-164
-
superset-embedded-sdk/testrig/drive.mjs - 3
- CWE-755: Unguarded JSON.parse crash · Line 78-78
- Orphaned chromium on ws failure · Line 131-135
- Predicate rejection crashes run · Line 196-198
Additional Suggestions - 16
-
superset-embedded-sdk/testrig/host.html - 2
-
Unguarded iframe deref · Line 198-203`rigNavigateGuest` calls `document.querySelector("#mount iframe").contentWindow` without checking the query result. Before `embedDashboard` mounts the iframe (or after unmount), `querySelector` returns null and `.contentWindow` throws TypeError.
-
failnth NaN silent no-op · Line 95-95`Number(search.get("failnth") || 0)` yields NaN for a non-numeric `failnth` value. Since `call === NaN` is always false, the failnth feature silently no-ops on malformed input instead of rejecting call N.
-
-
superset-embedded-sdk/testrig/drive.mjs - 12
-
Magic CDP timeout literal · Line 99-99The 30s CDP command timeout is a bare literal; `waitFor`/`expect` in this file already name their timeouts (`timeoutMs`), so a named constant (e.g. `CDP_TIMEOUT_MS`) would match the file's own convention and keep the rig's timing budget tunable in one place.
-
Magic startup timeout literal · Line 129-129The 20s browser-startup timeout is a bare literal encoding an operational threshold; naming it (e.g. `BROWSER_STARTUP_TIMEOUT_MS`) keeps both rig timeouts (`CDP.send` uses 30s) discoverable and tunable in one place.
-
Unused sessionId in return · Line 190-190All four call sites — `mainRun` (234), `refreshRun` (368), `staleInitialFetchRun` (412), `supersededTokenRun` (471) — destructure only `{ evaluate }`; nothing reads the returned `sessionId`. Drop it from the return object so the contract doesn't advertise a consumer that doesn't exist.
-
Sleep idiom duplicated · Line 202-202`waitFor` builds `new Promise((r) => setTimeout(r, intervalMs))` (202) — the exact idiom `sleep` (206) wraps and the rest of the file uses. Reuse `sleep(intervalMs)`; `waitFor` only runs after module evaluation, so the `const` binding is initialized by call time.
-
Tally lost on run crash · Line 570-573When any run throws (e.g. `openPage`'s `waitFor` timing out on host load at drive.mjs:186-189, called without `expect`'s catch), the `finally` stops browser/server but the tally built from `results` at 563-567 is skipped and `.catch` exits with only the error. This contradicts the rig's stated aim (comment at lines 56-58) of reporting per-scenario results instead of stopping at the first failure. Print the accumulated tally in the catch before exiting.
-
Delayed-retry blind spot · Line 457-461This check observes only 500ms after `failFirstToken()`, but the SDK's own failure retry (`DEFAULT_TOKEN_REFRESH_RETRY_MS` = 10s, used by `refreshGuestToken` in `src/index.ts`) fires at 10s — after `unmount()` ends the observation. If the stale rejection is ever routed into that retry path, the mint lands unobserved and "mints nothing further" still passes. Consider waiting past the retry window before recounting.
-
Verbose listener never removed · Line 157-166Each `openPage` call pushes a listener into `cdp.listeners` (drive.mjs:158) and nothing ever removes it — `listeners` is only initialized (76), iterated (85), and pushed (158). One closure accumulates per opened page for the process lifetime; only the `sessionId` guard keeps stale ones inert. Return a dispose from `openPage` or remove the listener on target close to keep the set bounded.
-
Inconsistent helper naming · Line 222-222In `makeHelpers`, every zero-arg probe is a bare noun or verb — `events`, `errors`, `embedState`, `tokensMinted`, `hasIframe` — except `getActiveTabs` (222). Aligning it (e.g. `activeTabs`) keeps the helper table uniform; the rename must also update its call site at drive.mjs:451.
-
Build errors silenced · Line 539-539`buildIfStale` runs `npx webpack` with `stdio: "ignore"` when not verbose, so a failed build throws "Command failed: npx webpack…" carrying none of webpack's diagnostics (verified on Node 20.12.1: `error.stderr` is null). The rig dies with no clue why. `stdio: "pipe"` keeps successful builds equally quiet while the thrown error carries the build output.
-
Unused import binding · Line 28-28`HOST_PORT` is imported from `./server.mjs` but never referenced anywhere in `drive.mjs` (verified by grep: only occurrence is the import itself; `start()` at line 551 is the sole use of the module). Unused bindings in an import statement are dead code and mislead readers into thinking the driver depends on the rig's port constant. Drop `HOST_PORT` from the import list.
-
Duplicated scenario setup · Line 469-483`supersededTokenRun` repeats `staleInitialFetchRun`'s rigging verbatim — `/reset`, `openPage(...?slowfirst=1)`, `click("embed")`, the `heldFirstFetch` and `eventsOn(1, "started")` waits, then `navigateGuest` (469-483 vs 411-429). If the ritual changes (event name, query params), both scenarios need synchronized edits. A shared setup helper keeps the two race scenarios from drifting while preserving their narration.
-
Missing state reset in mainRun · Line 243-247mainRun asserts absolute minted counts (`h.tokensMinted() === 1` at 245, `=== 2` at 264, `=== beforeStray + 1` at 335) but, unlike `refreshRun`, `staleInitialFetchRun` and `supersededTokenRun` (lines 367, 411, 469), never calls `fetch(`${hostOrigin}/reset`)` first. It is only correct because `main()` happens to run it first on a fresh server; a leading reset would make it self-contained like its siblings.
-
-
superset-embedded-sdk/testrig/embedded.html - 2
-
base64url decoded via atob · Line 80-80`server.mjs` mints the token with Node's `Buffer.toString("base64url")`, but this line decodes with `atob`, which is standard base64 and throws `InvalidCharacterError` on `-`/`_`. It only works today because the fixed claims happen to encode without those chars; any payload change (e.g. a longer `serial` or a non-ASCII `username`) breaks the rig. Decode base64url explicitly.
-
theme mode system unhandled · Line 104-104The real `setThemeMode` in `superset-frontend/src/embedded/index.tsx` accepts `default | dark | system` and maps each to a `ThemeMode`. This rig only toggles on `mode === 'dark'`, so a `system` request silently removes the dark class and returns `{success:true}`. If a test ever exercises `system`, the rig reports success for a mode it did not apply.
-
Review Details
-
Files reviewed - 4 · Commit Range:
a22ce65..a22ce65- superset-embedded-sdk/testrig/drive.mjs
- superset-embedded-sdk/testrig/embedded.html
- superset-embedded-sdk/testrig/host.html
- superset-embedded-sdk/testrig/server.mjs
-
Files skipped - 1
- superset-embedded-sdk/testrig/README.md - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| window.rigReleaseFirstToken = async () => { | ||
| const token = await mintToken(); | ||
| rig.heldFirstFetch.resolve(token); | ||
| log("host", "released the held fetchGuestToken() #1"); | ||
| }; | ||
| window.rigFailFirstToken = () => { | ||
| rig.heldFirstFetch.reject(new Error("rig: host token endpoint is down")); | ||
| log("error", "failed the held fetchGuestToken() #1"); | ||
| }; |
There was a problem hiding this comment.
rigReleaseFirstToken/rigFailFirstToken dereference rig.heldFirstFetch, which is null unless ?slowfirst=1 and call 1 is still held. Calling them otherwise throws TypeError. Also, if mintToken() rejects in rigReleaseFirstToken, the held promise is never settled and the SDK's fetchGuestToken hangs forever.
Code suggestion
Check the AI-generated fix before applying
| window.rigReleaseFirstToken = async () => { | |
| const token = await mintToken(); | |
| rig.heldFirstFetch.resolve(token); | |
| log("host", "released the held fetchGuestToken() #1"); | |
| }; | |
| window.rigFailFirstToken = () => { | |
| rig.heldFirstFetch.reject(new Error("rig: host token endpoint is down")); | |
| log("error", "failed the held fetchGuestToken() #1"); | |
| }; | |
| window.rigReleaseFirstToken = async () => { | |
| if (!rig.heldFirstFetch) return; | |
| const token = await mintToken(); | |
| rig.heldFirstFetch.resolve(token); | |
| log("host", "released the held fetchGuestToken() #1"); | |
| }; | |
| window.rigFailFirstToken = () => { | |
| if (!rig.heldFirstFetch) return; | |
| rig.heldFirstFetch.reject(new Error("rig: host token endpoint is down")); | |
| log("error", "failed the held fetchGuestToken() #1"); | |
| }; |
Code Review Run #a69ba8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
The suggestion is correct and should be applied. The current implementation of rigReleaseFirstToken and rigFailFirstToken lacks checks for rig.heldFirstFetch, which causes a TypeError if called when no fetch is held. Additionally, the proposed fix ensures that if mintToken() fails, the promise is properly handled, preventing the SDK's fetchGuestToken from hanging indefinitely.
superset-embedded-sdk/testrig/host.html
window.rigReleaseFirstToken = async () => {
if (!rig.heldFirstFetch) return;
const token = await mintToken();
rig.heldFirstFetch.resolve(token);
log("host", "released the held fetchGuestToken() #1");
};
window.rigFailFirstToken = () => {
if (!rig.heldFirstFetch) return;
rig.heldFirstFetch.reject(new Error("rig: host token endpoint is down"));
log("error", "failed the held fetchGuestToken() #1");
};
There was a problem hiding this comment.
Fixed in 338ead8 with a takeHeldFirstFetch() helper that guards both callers, and rigReleaseFirstToken now rejects the held promise if mintToken() throws instead of leaving it hanging.
There was a problem hiding this comment.
The suggestion is correct and addresses the identified issues. By adding checks for rig.heldFirstFetch, it prevents potential TypeErrors when the functions are called unexpectedly, and by properly handling the promise rejection if mintToken() fails, it ensures the SDK does not hang indefinitely.
superset-embedded-sdk/testrig/host.html
window.rigReleaseFirstToken = async () => {
if (!rig.heldFirstFetch) return;
const token = await mintToken();
rig.heldFirstFetch.resolve(token);
log("host", "released the held fetchGuestToken() #1");
};
window.rigFailFirstToken = () => {
if (!rig.heldFirstFetch) return;
rig.heldFirstFetch.reject(new Error("rig: host token endpoint is down"));
log("error", "failed the held fetchGuestToken() #1");
};
| document.getElementById("theme").onclick = () => { | ||
| rig.dashboard.setThemeConfig({ token: { colorPrimary: "#ff0066" } }); | ||
| rig.dashboard.setThemeMode("dark"); | ||
| log("host", "theme pushed"); | ||
| }; |
There was a problem hiding this comment.
theme, tabs, and unmount handlers dereference rig.dashboard without a null guard. rig.dashboard stays null until embedDashboard resolves and remains null if it rejects, so clicking these before resolution (or after a rejection) throws a TypeError. rigGetActiveTabs already guards this; apply the same check here.
Code suggestion
Check the AI-generated fix before applying
| document.getElementById("theme").onclick = () => { | |
| rig.dashboard.setThemeConfig({ token: { colorPrimary: "#ff0066" } }); | |
| rig.dashboard.setThemeMode("dark"); | |
| log("host", "theme pushed"); | |
| }; | |
| document.getElementById("theme").onclick = () => { | |
| if (!rig.dashboard) { | |
| log("error", "theme: no dashboard yet"); | |
| return; | |
| } | |
| rig.dashboard.setThemeConfig({ token: { colorPrimary: "#ff0066" } }); | |
| rig.dashboard.setThemeMode("dark"); | |
| log("host", "theme pushed"); | |
| }; |
Code Review Run #a69ba8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Confirmed. theme/tabs/unmount all deref rig.dashboard directly with no guard, while rigGetActiveTabs (the one the driver script actually calls) already does if (!rig.dashboard) return .... Only reachable in --headed mode by a human clicking ahead of embedDashboard() resolving, so it's a console error in a manual debugging session rather than a silent wrong result, lower stakes than the drive.mjs findings but still worth the one-line guard for consistency.
There was a problem hiding this comment.
The suggestion is appropriate and improves the code by preventing potential TypeErrors when the dashboard is not yet initialized. Applying this guard ensures consistency with other parts of the driver script that already implement this check.
superset-embedded-sdk/testrig/host.html
document.getElementById("theme").onclick = () => {
if (!rig.dashboard) {
log("error", "theme: no dashboard yet");
return;
}
rig.dashboard.setThemeConfig({ token: { colorPrimary: "#ff0066" } });
rig.dashboard.setThemeMode("dark");
log("host", "theme pushed");
};
There was a problem hiding this comment.
Fixed in 338ead8. theme/tabs/unmount all route through a hasDashboard() guard now, same pattern rigGetActiveTabs already used.
There was a problem hiding this comment.
The suggestion is appropriate and improves the code by preventing potential TypeErrors when the dashboard is not yet initialized. Applying this guard ensures consistency with other parts of the driver script that already implement this check.
superset-embedded-sdk/testrig/host.html
document.getElementById("theme").onclick = () => {
if (!rig.dashboard) {
log("error", "theme: no dashboard yet");
return;
}
rig.dashboard.setThemeConfig({ token: { colorPrimary: "#ff0066" } });
rig.dashboard.setThemeMode("dark");
log("host", "theme pushed");
};
| this.pending = new Map(); | ||
| this.listeners = []; | ||
| ws.addEventListener("message", (event) => { | ||
| const msg = JSON.parse(event.data); |
There was a problem hiding this comment.
JSON.parse(event.data) runs inside the socket's message listener with no guard, so any non-JSON or binary frame throws inside the listener; in Node an exception escaping an EventTarget listener surfaces as an uncaught exception and kills the rig process, losing every check result. Wrap the parse in try/catch and skip malformed frames. (CWE-755)
Code Review Run #a69ba8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Confirmed, no try/catch around JSON.parse(event.data) in the CDP message listener. An exception thrown inside a ws message listener isn't caught anywhere upstream here, real crash risk even if CDP frames are rarely malformed in practice.
There was a problem hiding this comment.
The suggestion to wrap JSON.parse(event.data) in a try/catch block is correct and highly recommended. In the current implementation, an unhandled exception during parsing would propagate to the event listener, potentially causing an uncaught exception that terminates the process. Applying this guard ensures the rig remains stable even if it receives malformed or non-JSON frames.
superset-embedded-sdk/testrig/drive.mjs
ws.addEventListener("message", (event) => {
try {
const msg = JSON.parse(event.data);
// ... handle message
} catch (e) {
console.error("Failed to parse CDP message:", e);
}
There was a problem hiding this comment.
Fixed in 338ead8, the CDP message listener wraps JSON.parse in try/catch and drops unparsable frames instead of throwing.
There was a problem hiding this comment.
The suggestion to wrap JSON.parse(event.data) in a try/catch block is correct and highly recommended. In the current implementation, an unhandled exception during parsing would propagate to the event listener, potentially causing an uncaught exception that terminates the process. Applying this guard ensures the rig remains stable even if it receives malformed or non-JSON frames.
superset-embedded-sdk/testrig/drive.mjs
ws.addEventListener("message", (event) => {
try {
const msg = JSON.parse(event.data);
// ... handle message
} catch (e) {
console.error("Failed to parse CDP message:", e);
}
| const ws = new WebSocket(wsUrl); | ||
| await new Promise((resolve, reject) => { | ||
| ws.addEventListener("open", resolve, { once: true }); | ||
| ws.addEventListener("error", reject, { once: true }); | ||
| }); |
There was a problem hiding this comment.
Between spawn (line 116) and the return, nothing cleans up if the WebSocket handshake rejects: the error propagates out of launchBrowser before main's try/finally, so stop() never runs and the SIGKILL/rmSync cleanup is skipped, leaving an orphaned chromium process and a temp profile dir. Wrap the post-spawn body in try/catch that kills the child and removes the dir before rethrowing.
Code Review Run #a69ba8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Confirmed via the same code path as the codeant finding on this file: if the WebSocket 'error' listener fires during the handshake, the already-spawned chromium child process has no cleanup on that path, only stop() (returned after success) kills it. Same root cause as the try/finally gap in main(), worth fixing as one change.
There was a problem hiding this comment.
The suggestion to wrap the post-spawn logic in a try/catch block is correct. Currently, if the WebSocket handshake fails, the error propagates out of launchBrowser before the cleanup logic in main can execute, leaving the Chromium process orphaned and the temporary directory unremoved. Applying this change ensures that the child process is killed and the directory is cleaned up regardless of whether the handshake succeeds or fails.
superset-embedded-sdk/testrig/drive.mjs
const ws = new WebSocket(wsUrl);
try {
await new Promise((resolve, reject) => {
ws.addEventListener("open", resolve, { once: true });
ws.addEventListener("error", reject, { once: true });
});
} catch (err) {
child.kill();
rmSync(userDataDir, { recursive: true, force: true });
throw err;
}
There was a problem hiding this comment.
Fixed in 338ead8. launchBrowser() wraps the post-spawn connect() call in try/catch and calls stop() before rethrowing, so a handshake failure doesn't orphan the process.
There was a problem hiding this comment.
The fix implemented in the referenced commit correctly addresses the issue of orphaned Chromium processes. By wrapping the post-spawn WebSocket handshake in a try/catch block, the code ensures that if the handshake fails, the child process is terminated and the temporary directory is removed before the error propagates, preventing resource leaks.
superset-embedded-sdk/testrig/drive.mjs
const ws = new WebSocket(wsUrl);
try {
await new Promise((resolve, reject) => {
ws.addEventListener("open", resolve, { once: true });
ws.addEventListener("error", reject, { once: true });
});
} catch (err) {
child.kill();
rmSync(userDataDir, { recursive: true, force: true });
throw err;
}
| for (;;) { | ||
| last = await predicate(); | ||
| if (last) return last; |
There was a problem hiding this comment.
waitFor (drive.mjs:196-203) lets a rejected predicate escape, and openPage's readiness poll calls evaluate, whose cdp.send rejects on CDP errors (drive.mjs:82). A navigation destroying the execution context mid-poll rejects evaluate and crashes the whole run instead of retrying until the 15s deadline. expect catches predicate errors; openPage's direct call does not. Consider tolerating predicate rejections until the deadline.
Code Review Run #a69ba8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Confirmed, and this is the one I'd weight most heavily: openPage()'s own waitFor(() => evaluate(...), "the host app to load") call isn't wrapped the way expect() wraps its waitFor calls, so a predicate rejection propagates straight out. evaluate() rejects on a CDP-level error, exactly what happens when a navigation destroys the execution context mid-poll, which is precisely the race this rig exists to test. As written, hitting that race at the wrong moment can crash the whole 39-check run instead of retrying.
There was a problem hiding this comment.
The reviewer is correct. In openPage(), the waitFor call is invoked directly without the expect() wrapper, meaning any rejection from the evaluate() predicate—such as a CDP error caused by a navigation destroying the execution context—will propagate and terminate the entire test run. Wrapping this call in a try-catch block or using a similar retry mechanism to the one implemented in expect() would allow the test to tolerate these transient failures until the timeout is reached.
superset-embedded-sdk/testrig/drive.mjs
// Wait for the host app's own globals rather than a load event.
await waitFor(
() => evaluate("typeof window.rig === 'object' && !!window.supersetEmbeddedSdk"),
"the host app to load",
);
There was a problem hiding this comment.
Fixed in 338ead8, actually at the root: waitFor() itself now catches predicate rejections and keeps retrying until the deadline, so openPage() didn't need an expect() wrapper after all.
There was a problem hiding this comment.
The user is correct. Since waitFor() now handles predicate rejections by retrying until the deadline, the expect() wrapper is no longer required for openPage() to avoid crashing the test run on transient CDP errors.
left a comment
There was a problem hiding this comment.
Heya @chadek, nice rig, and appreciate splitting this out of #44470 so that PR stays focused. Went through all 8 bot findings here (both bito and codeant) and traced each one against the actual code rather than just trusting the summaries, all 8 turned out to be real, replied on each thread with what I found.
Two of them are the ones I'd actually block on, since they undercut the rig's own job:
openPage()'s own waitFor(() => evaluate(...), "the host app to load") call isn't wrapped the way expect() wraps its waitFor calls, so a predicate rejection propagates straight out instead of retrying. evaluate() rejects on a CDP-level error, which is exactly what happens when a navigation destroys the execution context mid-poll, the precise race this rig exists to reproduce. Hit that at the wrong moment and the whole 39-check run crashes instead of retrying to the 15s deadline.
The unguarded JSON.parse(event.data) in the CDP message listener has the same shape: rare in practice, but an exception thrown inside a ws message listener here isn't caught anywhere upstream, so it takes the whole process down mid-run with no diagnostic.
The rest (resource leak on a launchBrowser()/handshake failure, server.mjs's start() never rejecting on a port-bind failure, the unguarded rig.dashboard derefs in host.html's manual click handlers, and buildIfStale() not watching webpack.config.js) are real too but lower stakes, mostly annoyances for whoever's running this locally rather than things that would produce a false pass/fail. Your call whether those are worth bundling into this PR or a fast follow.
Given this is dev-only tooling (not wired into CI, not shipping anywhere), I don't think this needs the same bar as product code, but a test rig that can crash on the exact race it's built to catch is worth tightening before anyone leans on its results.
Addresses review on apache#44608: - waitFor treats a rejected predicate as "not yet" and keeps polling to the deadline, so a navigation that destroys the execution context mid-poll (the race the rig reproduces) no longer crashes the run - the CDP message listener drops unparsable frames instead of throwing out of the listener - launchBrowser cleans up chromium and its profile if the devtools handshake fails, and main() starts the servers and the browser inside its try so whichever came up is stopped - chromium runs in its own process group and is stopped as a group, waiting for it to exit before removing the profile: its helper processes otherwise wrote back into the profile after removal, leaving a temp dir behind on every run; SIGINT/SIGTERM stop it too - server start() rejects on a port-bind failure instead of hanging - host.html guards rig.dashboard and rig.heldFirstFetch, settles the held fetch only once, and rejects it if minting the token fails - buildIfStale also watches the build config and dependency manifests Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
commented
Sep 25, 2026
|
Thanks for tracing each of these. All 8 are fixed in 338ead8, covering the two you'd block on and the four lower-stakes ones. While re-running the rig to check the fixes I found one more leak, in the same area as the launch/handshake one: even a passing run left a /tmp/embedded-sdk-rig-* profile behind. stop() SIGKILLed only the main Chromium process, and its helper processes kept writing into the profile after it was removed. Chromium now runs in its own process group and is stopped as a group, and the profile is only deleted once it has exited. SIGINT/SIGTERM stop it too, since Ctrl-C no longer reaches a separate process group. Checked locally with real Chromium:
|
Code Review Agent Run #dde8e6Actionable 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 |
|
Thanks for your attention on this @rusackas, it will really help us out when next release will be out. Any idea of an approximated timeline for the next release date target? |
2 participants
SUMMARY
Split out of #44470 per rusackas's review so that PR stays just the fix. This is the local test rig referenced in that PR's testing instructions.
superset-embedded-sdk/src/index.test.tsmocksMessageChannelandSwitchboard, so it can't exercise the parts of the bug that only exist between two real documents: the port-transfer handshake, what survives a navigation the dashboard makes on its own, and how many guest tokens the host's endpoint is actually asked for.This rig runs a host app and a stand-in for Superset's embedded page on two origins, loads the built UMD bundle in headless Chromium, and drives it over the real
@superset-ui/switchboard.node testrig/drive.mjsruns 39 checks covering:getstill in flight when the user navigates rejecting asPortClosedErrorinstead of hanging, and the same onunmount()Depends on #44468's fix (#44470) to pass. This PR itself carries no fix code, only the test tooling — checked against
masteras it stands today,drive.mjsfails, reproducing the bug. I ran it locally againstfix/embedded-sdk-internal-navigation(i.e. this branch's files, on top of that fix) and got 39/39 passing; the README documents running it againstgit show HEAD~1:...ofsrc/index.tsto watch the checks fail for the same reason. Recommend merging #44470 first, or reviewing this diff on its own and trusting the local run.Not wired into CI (
.github/workflows/embedded-sdk-test.ymlonly runsnpm test/npm run build): it needs a Chromium binary onPATH, and the unit tests already gate the merge. It's meant as a manual/local tool for exercising the cross-document behavior when touching this handshake again.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — no product code change, test tooling only.
TESTING INSTRUCTIONS
Needs a
chromium,chromium-browserorgoogle-chromeonPATH, orCHROMIUM_PATHpointing at one. Run it on top of #44470's branch to see 39/39 pass, or onmasteralone to watch it reproduce the bug.To poke at it by hand:
node testrig/server.mjs, then openhttp://localhost:8100. Seetestrig/README.mdfor the query params that reach the two token-fetch races.ADDITIONAL INFORMATION
🤖 Generated with Claude Code