bito-code-review[bot] commented on code in PR #44608: URL: https://github.com/apache/superset/pull/44608#discussion_r4095853613
########## superset-embedded-sdk/testrig/host.html: ########## @@ -0,0 +1,208 @@ +<!doctype html> +<html lang="en"> +<!-- + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with this + work for additional information regarding copyright ownership. The ASF + licenses this file to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance with the License. + You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 +--> +<head> + <meta charset="utf-8" /> + <title>embedded-sdk test rig — host app</title> + <style> + body { margin: 0; font: 14px/1.5 system-ui, sans-serif; display: grid; + grid-template-columns: 1fr 420px; height: 100vh; } + main { padding: 16px; overflow: auto; } + aside { border-left: 1px solid #ddd; padding: 16px; overflow: auto; background: #fafafa; } + h1 { font-size: 16px; margin: 0 0 12px; } + #mount { border: 1px solid #ccc; border-radius: 8px; height: 320px; } + #mount iframe { width: 100%; height: 100%; border: 0; border-radius: 8px; } + button { font: inherit; margin: 0 6px 6px 0; padding: 4px 10px; } + #log { font: 12px/1.45 ui-monospace, monospace; white-space: pre-wrap; } + .t { color: #888; } + .err { color: #b00; } + .ok { color: #070; } + </style> +</head> +<body> + <main> + <h1>Host app</h1> + <div> + <button id="embed">embed</button> + <button id="theme">setThemeMode(dark) + setThemeConfig</button> + <button id="tabs">getActiveTabs()</button> + <button id="unmount">unmount</button> + <button id="clear">clear log</button> + </div> + <div id="mount"></div> + <p class="t">Tokens minted by this host's endpoint: <b id="minted">0</b></p> + </main> + <aside><div id="log"></div></aside> + +<script src="/sdk.js"></script> +<script> + const SUPERSET_ORIGIN = "__SUPERSET_ORIGIN__"; + + // Everything the driver reads. + const rig = (window.rig = { + events: [], // what the embedded page reported over its side channel + log: [], // what the host app saw + errors: [], // rejections the SDK handed back + tokenFetches: 0, // calls the SDK made to fetchGuestToken + dashboard: null, + embedState: "idle", // pending | resolved | rejected + embedError: null, + heldFirstFetch: null, + }); + + function log(kind, ...parts) { + const line = `${new Date().toISOString().slice(11, 23)} ${kind} ${parts + .map((p) => (typeof p === "string" ? p : JSON.stringify(p))) + .join(" ")}`; + rig.log.push(line); + const el = document.createElement("div"); + el.className = kind === "error" ? "err" : kind === "ok" ? "ok" : ""; + el.textContent = line; + document.getElementById("log").prepend(el); + } + + // The embedded page's side channel (not the SDK's MessageChannel). + window.addEventListener("message", (event) => { + if (!event.data?.__rig) return; + rig.events.push({ ...event.data, at: Date.now() }); + log("guest", `page ${event.data.page}`, event.data.event, event.data.detail ?? ""); + }); + + async function refreshStats() { + const stats = await fetch("/stats").then((r) => r.json()); + document.getElementById("minted").textContent = stats.tokensMinted; + rig.tokensMinted = stats.tokensMinted; + return stats; + } + window.rigRefreshStats = refreshStats; + + // TTL is a query param so a run can exercise the refresh timer in seconds + // rather than minutes. + const search = new URLSearchParams(location.search); + const ttl = search.get("ttl") || "300"; + // `?slowfirst=1` holds the very first fetchGuestToken() until the driver + // settles it by hand, which is the only way to have a navigation happen + // while the initial token is still in flight. `?failnth=N` makes call N + // reject, standing in for a host endpoint that is down. + const slowFirst = search.get("slowfirst"); + const failNth = Number(search.get("failnth") || 0); + + async function mintToken() { + const { token } = await fetch(`/guest-token?ttl=${ttl}`).then((r) => r.json()); + refreshStats(); + return token; + } + + async function fetchGuestToken() { + rig.tokenFetches += 1; + const call = rig.tokenFetches; + log("host", `fetchGuestToken() #${call}`); + if (call === failNth) { + log("error", `fetchGuestToken() #${call} rejects (rig)`); + throw new Error(`rig: host token endpoint is down (call #${call})`); + } + if (slowFirst && call === 1) { + log("host", "fetchGuestToken() #1 held by the rig"); + return new Promise((resolve, reject) => { + rig.heldFirstFetch = { resolve, reject }; + }); + } + return mintToken(); + } + + // Settle the held first fetch, long after the SDK asked for it. + 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"); + }; + + document.getElementById("embed").onclick = () => { + // Deliberately not awaited: a run needs to watch what happens while + // `embedDashboard` is still pending, and to see how it ends up settling. + rig.embedState = "pending"; + supersetEmbeddedSdk + .embedDashboard({ + id: "rig-dashboard", + supersetDomain: SUPERSET_ORIGIN, + mountPoint: document.getElementById("mount"), + fetchGuestToken, + debug: true, + dashboardUiConfig: search.get("hang") + ? { urlParams: { hang: "1" } } + : undefined, + }) + .then( + (dashboard) => { + rig.dashboard = dashboard; + rig.embedState = "resolved"; + log("ok", "embedDashboard resolved"); + }, + (err) => { + rig.embedState = "rejected"; + rig.embedError = { name: err.name, message: err.message }; + log("error", `embedDashboard rejected: ${err.message}`); + }, + ); + }; + + document.getElementById("theme").onclick = () => { + rig.dashboard.setThemeConfig({ token: { colorPrimary: "#ff0066" } }); + rig.dashboard.setThemeMode("dark"); + log("host", "theme pushed"); + }; Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Unguarded null dashboard deref</b></div> <div id="fix"> `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. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion 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"); }; ```` </div> </details> </div> <small><i>Code Review Run #a69ba8</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-embedded-sdk/testrig/drive.mjs: ########## @@ -0,0 +1,573 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +// Drives the rig in headless Chromium over the DevTools protocol, with no +// dependencies beyond a chromium binary. `node drive.mjs [--headed] [--verbose]` + +import { spawn, execFileSync } from "node:child_process"; +import { mkdtempSync, existsSync, statSync, readdirSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join, dirname } from "node:path"; +import { fileURLToPath } from "node:url"; +import { start, HOST_PORT } from "./server.mjs"; + +const here = dirname(fileURLToPath(import.meta.url)); +const sdkRoot = join(here, ".."); +const headed = process.argv.includes("--headed"); +const verbose = process.argv.includes("--verbose"); + +const CHROMIUM = + process.env.CHROMIUM_PATH || + ["chromium", "chromium-browser", "google-chrome", "google-chrome-stable"].find( + (bin) => { + try { + execFileSync("which", [bin], { stdio: "ignore" }); + return true; + } catch { + return false; + } + }, + ); + +// ------------------------------------------------------------------ results +const results = []; +function check(name, ok, detail = "") { + results.push({ name, ok, detail }); + const mark = ok ? "\x1b[32m✓\x1b[0m" : "\x1b[31m✗\x1b[0m"; + console.log(` ${mark} ${name}${detail && !ok ? `\n ${detail}` : ""}`); +} + +// Waits for something to become true, and records the wait itself as a check, +// so a run against code that never gets there reports a failure per scenario +// instead of stopping at the first one. +async function expect(name, predicate, timeoutMs = 15_000) { + try { + await waitFor(predicate, name, timeoutMs); + check(name, true); + return true; + } catch (err) { + check(name, false, err.message); + return false; + } +} + +// ---------------------------------------------------------------- cdp client +class CDP { + constructor(ws) { + this.ws = ws; + this.nextId = 0; + this.pending = new Map(); + this.listeners = []; + ws.addEventListener("message", (event) => { + const msg = JSON.parse(event.data); + if (msg.id && this.pending.has(msg.id)) { + const { resolve, reject } = this.pending.get(msg.id); + this.pending.delete(msg.id); + if (msg.error) reject(new Error(JSON.stringify(msg.error))); + else resolve(msg.result); + } else { + this.listeners.forEach((fn) => fn(msg)); + } + }); + } + + send(method, params = {}, sessionId) { + const id = (this.nextId += 1); + const payload = { id, method, params }; + if (sessionId) payload.sessionId = sessionId; + this.ws.send(JSON.stringify(payload)); + return new Promise((resolve, reject) => { + this.pending.set(id, { resolve, reject }); + setTimeout(() => { + if (this.pending.delete(id)) reject(new Error(`${method} timed out`)); + }, 30_000); + }); + } +} + +async function launchBrowser() { + const userDataDir = mkdtempSync(join(tmpdir(), "embedded-sdk-rig-")); + const args = [ + ...(headed ? [] : ["--headless=new"]), + "--remote-debugging-port=0", + `--user-data-dir=${userDataDir}`, + "--no-first-run", + "--no-default-browser-check", + "--disable-gpu", + "--disable-dev-shm-usage", + "about:blank", + ]; + const child = spawn(CHROMIUM, args, { stdio: ["ignore", "pipe", "pipe"] }); + const wsUrl = await new Promise((resolve, reject) => { + let buffered = ""; + const onChunk = (chunk) => { + buffered += chunk; + const match = buffered.match(/ws:\/\/\S+/); + if (match) resolve(match[0]); + }; + child.stdout.on("data", onChunk); + child.stderr.on("data", onChunk); + child.on("exit", (code) => + reject(new Error(`chromium exited (${code}) before listening:\n${buffered}`)), + ); + setTimeout(() => reject(new Error("chromium never reported a devtools url")), 20_000); + }); + const ws = new WebSocket(wsUrl); + await new Promise((resolve, reject) => { + ws.addEventListener("open", resolve, { once: true }); + ws.addEventListener("error", reject, { once: true }); + }); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Orphaned chromium on ws failure</b></div> <div id="fix"> 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. </div> </div> <small><i>Code Review Run #a69ba8</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-embedded-sdk/testrig/host.html: ########## @@ -0,0 +1,208 @@ +<!doctype html> +<html lang="en"> +<!-- + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with this + work for additional information regarding copyright ownership. The ASF + licenses this file to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance with the License. + You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 +--> +<head> + <meta charset="utf-8" /> + <title>embedded-sdk test rig — host app</title> + <style> + body { margin: 0; font: 14px/1.5 system-ui, sans-serif; display: grid; + grid-template-columns: 1fr 420px; height: 100vh; } + main { padding: 16px; overflow: auto; } + aside { border-left: 1px solid #ddd; padding: 16px; overflow: auto; background: #fafafa; } + h1 { font-size: 16px; margin: 0 0 12px; } + #mount { border: 1px solid #ccc; border-radius: 8px; height: 320px; } + #mount iframe { width: 100%; height: 100%; border: 0; border-radius: 8px; } + button { font: inherit; margin: 0 6px 6px 0; padding: 4px 10px; } + #log { font: 12px/1.45 ui-monospace, monospace; white-space: pre-wrap; } + .t { color: #888; } + .err { color: #b00; } + .ok { color: #070; } + </style> +</head> +<body> + <main> + <h1>Host app</h1> + <div> + <button id="embed">embed</button> + <button id="theme">setThemeMode(dark) + setThemeConfig</button> + <button id="tabs">getActiveTabs()</button> + <button id="unmount">unmount</button> + <button id="clear">clear log</button> + </div> + <div id="mount"></div> + <p class="t">Tokens minted by this host's endpoint: <b id="minted">0</b></p> + </main> + <aside><div id="log"></div></aside> + +<script src="/sdk.js"></script> +<script> + const SUPERSET_ORIGIN = "__SUPERSET_ORIGIN__"; + + // Everything the driver reads. + const rig = (window.rig = { + events: [], // what the embedded page reported over its side channel + log: [], // what the host app saw + errors: [], // rejections the SDK handed back + tokenFetches: 0, // calls the SDK made to fetchGuestToken + dashboard: null, + embedState: "idle", // pending | resolved | rejected + embedError: null, + heldFirstFetch: null, + }); + + function log(kind, ...parts) { + const line = `${new Date().toISOString().slice(11, 23)} ${kind} ${parts + .map((p) => (typeof p === "string" ? p : JSON.stringify(p))) + .join(" ")}`; + rig.log.push(line); + const el = document.createElement("div"); + el.className = kind === "error" ? "err" : kind === "ok" ? "ok" : ""; + el.textContent = line; + document.getElementById("log").prepend(el); + } + + // The embedded page's side channel (not the SDK's MessageChannel). + window.addEventListener("message", (event) => { + if (!event.data?.__rig) return; + rig.events.push({ ...event.data, at: Date.now() }); + log("guest", `page ${event.data.page}`, event.data.event, event.data.detail ?? ""); + }); + + async function refreshStats() { + const stats = await fetch("/stats").then((r) => r.json()); + document.getElementById("minted").textContent = stats.tokensMinted; + rig.tokensMinted = stats.tokensMinted; + return stats; + } + window.rigRefreshStats = refreshStats; + + // TTL is a query param so a run can exercise the refresh timer in seconds + // rather than minutes. + const search = new URLSearchParams(location.search); + const ttl = search.get("ttl") || "300"; + // `?slowfirst=1` holds the very first fetchGuestToken() until the driver + // settles it by hand, which is the only way to have a navigation happen + // while the initial token is still in flight. `?failnth=N` makes call N + // reject, standing in for a host endpoint that is down. + const slowFirst = search.get("slowfirst"); + const failNth = Number(search.get("failnth") || 0); + + async function mintToken() { + const { token } = await fetch(`/guest-token?ttl=${ttl}`).then((r) => r.json()); + refreshStats(); + return token; + } + + async function fetchGuestToken() { + rig.tokenFetches += 1; + const call = rig.tokenFetches; + log("host", `fetchGuestToken() #${call}`); + if (call === failNth) { + log("error", `fetchGuestToken() #${call} rejects (rig)`); + throw new Error(`rig: host token endpoint is down (call #${call})`); + } + if (slowFirst && call === 1) { + log("host", "fetchGuestToken() #1 held by the rig"); + return new Promise((resolve, reject) => { + rig.heldFirstFetch = { resolve, reject }; + }); + } + return mintToken(); + } + + // Settle the held first fetch, long after the SDK asked for it. + 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"); + }; Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Unguarded heldFirstFetch deref</b></div> <div id="fix"> `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. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion 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"); }; ```` </div> </details> </div> <small><i>Code Review Run #a69ba8</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-embedded-sdk/testrig/drive.mjs: ########## @@ -0,0 +1,573 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +// Drives the rig in headless Chromium over the DevTools protocol, with no +// dependencies beyond a chromium binary. `node drive.mjs [--headed] [--verbose]` + +import { spawn, execFileSync } from "node:child_process"; +import { mkdtempSync, existsSync, statSync, readdirSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join, dirname } from "node:path"; +import { fileURLToPath } from "node:url"; +import { start, HOST_PORT } from "./server.mjs"; + +const here = dirname(fileURLToPath(import.meta.url)); +const sdkRoot = join(here, ".."); +const headed = process.argv.includes("--headed"); +const verbose = process.argv.includes("--verbose"); + +const CHROMIUM = + process.env.CHROMIUM_PATH || + ["chromium", "chromium-browser", "google-chrome", "google-chrome-stable"].find( + (bin) => { + try { + execFileSync("which", [bin], { stdio: "ignore" }); + return true; + } catch { + return false; + } + }, + ); + +// ------------------------------------------------------------------ results +const results = []; +function check(name, ok, detail = "") { + results.push({ name, ok, detail }); + const mark = ok ? "\x1b[32m✓\x1b[0m" : "\x1b[31m✗\x1b[0m"; + console.log(` ${mark} ${name}${detail && !ok ? `\n ${detail}` : ""}`); +} + +// Waits for something to become true, and records the wait itself as a check, +// so a run against code that never gets there reports a failure per scenario +// instead of stopping at the first one. +async function expect(name, predicate, timeoutMs = 15_000) { + try { + await waitFor(predicate, name, timeoutMs); + check(name, true); + return true; + } catch (err) { + check(name, false, err.message); + return false; + } +} + +// ---------------------------------------------------------------- cdp client +class CDP { + constructor(ws) { + this.ws = ws; + this.nextId = 0; + this.pending = new Map(); + this.listeners = []; + ws.addEventListener("message", (event) => { + const msg = JSON.parse(event.data); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>CWE-755: Unguarded JSON.parse crash</b></div> <div id="fix"> `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](https://cwe.mitre.org/data/definitions/755.html)) </div> </div> <small><i>Code Review Run #a69ba8</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-embedded-sdk/testrig/drive.mjs: ########## @@ -0,0 +1,573 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +// Drives the rig in headless Chromium over the DevTools protocol, with no +// dependencies beyond a chromium binary. `node drive.mjs [--headed] [--verbose]` + +import { spawn, execFileSync } from "node:child_process"; +import { mkdtempSync, existsSync, statSync, readdirSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join, dirname } from "node:path"; +import { fileURLToPath } from "node:url"; +import { start, HOST_PORT } from "./server.mjs"; + +const here = dirname(fileURLToPath(import.meta.url)); +const sdkRoot = join(here, ".."); +const headed = process.argv.includes("--headed"); +const verbose = process.argv.includes("--verbose"); + +const CHROMIUM = + process.env.CHROMIUM_PATH || + ["chromium", "chromium-browser", "google-chrome", "google-chrome-stable"].find( + (bin) => { + try { + execFileSync("which", [bin], { stdio: "ignore" }); + return true; + } catch { + return false; + } + }, + ); + +// ------------------------------------------------------------------ results +const results = []; +function check(name, ok, detail = "") { + results.push({ name, ok, detail }); + const mark = ok ? "\x1b[32m✓\x1b[0m" : "\x1b[31m✗\x1b[0m"; + console.log(` ${mark} ${name}${detail && !ok ? `\n ${detail}` : ""}`); +} + +// Waits for something to become true, and records the wait itself as a check, +// so a run against code that never gets there reports a failure per scenario +// instead of stopping at the first one. +async function expect(name, predicate, timeoutMs = 15_000) { + try { + await waitFor(predicate, name, timeoutMs); + check(name, true); + return true; + } catch (err) { + check(name, false, err.message); + return false; + } +} + +// ---------------------------------------------------------------- cdp client +class CDP { + constructor(ws) { + this.ws = ws; + this.nextId = 0; + this.pending = new Map(); + this.listeners = []; + ws.addEventListener("message", (event) => { + const msg = JSON.parse(event.data); + if (msg.id && this.pending.has(msg.id)) { + const { resolve, reject } = this.pending.get(msg.id); + this.pending.delete(msg.id); + if (msg.error) reject(new Error(JSON.stringify(msg.error))); + else resolve(msg.result); + } else { + this.listeners.forEach((fn) => fn(msg)); + } + }); + } + + send(method, params = {}, sessionId) { + const id = (this.nextId += 1); + const payload = { id, method, params }; + if (sessionId) payload.sessionId = sessionId; + this.ws.send(JSON.stringify(payload)); + return new Promise((resolve, reject) => { + this.pending.set(id, { resolve, reject }); + setTimeout(() => { + if (this.pending.delete(id)) reject(new Error(`${method} timed out`)); + }, 30_000); + }); + } +} + +async function launchBrowser() { + const userDataDir = mkdtempSync(join(tmpdir(), "embedded-sdk-rig-")); + const args = [ + ...(headed ? [] : ["--headless=new"]), + "--remote-debugging-port=0", + `--user-data-dir=${userDataDir}`, + "--no-first-run", + "--no-default-browser-check", + "--disable-gpu", + "--disable-dev-shm-usage", + "about:blank", + ]; + const child = spawn(CHROMIUM, args, { stdio: ["ignore", "pipe", "pipe"] }); + const wsUrl = await new Promise((resolve, reject) => { + let buffered = ""; + const onChunk = (chunk) => { + buffered += chunk; + const match = buffered.match(/ws:\/\/\S+/); + if (match) resolve(match[0]); + }; + child.stdout.on("data", onChunk); + child.stderr.on("data", onChunk); + child.on("exit", (code) => + reject(new Error(`chromium exited (${code}) before listening:\n${buffered}`)), + ); + setTimeout(() => reject(new Error("chromium never reported a devtools url")), 20_000); + }); + const ws = new WebSocket(wsUrl); + await new Promise((resolve, reject) => { + ws.addEventListener("open", resolve, { once: true }); + ws.addEventListener("error", reject, { once: true }); + }); + return { + cdp: new CDP(ws), + stop: () => { + try { + child.kill("SIGKILL"); + } finally { + rmSync(userDataDir, { recursive: true, force: true }); + } + }, + }; +} + +// ------------------------------------------------------------------- a page +async function openPage(cdp, url) { + const { targetId } = await cdp.send("Target.createTarget", { url: "about:blank" }); + const { sessionId } = await cdp.send("Target.attachToTarget", { + targetId, + flatten: true, + }); + await cdp.send("Runtime.enable", {}, sessionId); + await cdp.send("Page.enable", {}, sessionId); + if (verbose) { + cdp.listeners.push((msg) => { + if (msg.method === "Runtime.consoleAPICalled" && msg.sessionId === sessionId) { + const text = msg.params.args + .map((a) => a.value ?? a.description ?? a.type) + .join(" "); + console.log(` [page] ${text}`); + } + }); + } + + const evaluate = async (expression) => { + const r = await cdp.send( + "Runtime.evaluate", + { expression, awaitPromise: true, returnByValue: true }, + sessionId, + ); + if (r.exceptionDetails) { + throw new Error( + r.exceptionDetails.exception?.description || + r.exceptionDetails.text || + JSON.stringify(r.exceptionDetails), + ); + } + return r.result.value; + }; + + await cdp.send("Page.navigate", { url }, sessionId); + // 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", + ); + return { evaluate, sessionId }; +} + +async function waitFor(predicate, what, timeoutMs = 15_000, intervalMs = 100) { + const deadline = Date.now() + timeoutMs; + let last; + for (;;) { + last = await predicate(); + if (last) return last; Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Predicate rejection crashes run</b></div> <div id="fix"> `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. </div> </div> <small><i>Code Review Run #a69ba8</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
