bito-code-review[bot] commented on code in PR #44608: URL: https://github.com/apache/superset/pull/44608#discussion_r4131244617
########## 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: <!-- Bito Reply --> 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"); }; ``` ########## 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: <!-- Bito Reply --> 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"); }; ``` ########## 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: <!-- Bito Reply --> 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); } ``` -- 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]
