feat(desktop): add Windows build and runtime support#2100
Open
mengfoong-dev wants to merge 2 commits intosuperset-sh:mainfrom
Open
feat(desktop): add Windows build and runtime support#2100mengfoong-dev wants to merge 2 commits intosuperset-sh:mainfrom
mengfoong-dev wants to merge 2 commits intosuperset-sh:mainfrom
Conversation
- Cross-platform postinstall: replace bash-only postinstall.sh with
Node.js postinstall.mjs that works on Windows, macOS, and Linux
- Fix ELECTRON_RUN_AS_NODE: clear the env var before require("electron")
via rollup banner so the app works when launched from VS Code terminal
- Disable GPU hardware acceleration on Windows (in addition to Linux)
to prevent black/blank screens from GPU driver incompatibilities
- Strip crossorigin attributes from HTML on Windows — Electron's ASAR
file:// handler doesn't support CORS, causing silent load failures
- Fix junction removal on Windows: use rmSync({ recursive: true })
since Windows junctions are directory-like, not file symlinks
- Disable npmRebuild on Windows in electron-builder — native modules
are handled by install:deps/copy:native-modules, and node-gyp fails
without Visual Studio Build Tools
- Add NSIS installer shortcuts (desktop + start menu)
- Forward renderer console warnings/errors to main process on Windows
- Fix defineEnv to use || instead of ?? so empty strings from
unresolved CI secrets fall back to defaults
Tested: built and launched Superset.exe on Windows 11 Pro x64.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds Windows-specific runtime and installer behaviors, cross-platform postinstall and build-plugin adjustments, expands protocol/CORS handling for a custom superset-app scheme, and tweaks hardware-acceleration and native-module copy/rebuild logic. Changes
Sequence Diagram(s)sequenceDiagram
participant R as Renderer (superset-app://app)
participant M as Main (protocol handler)
participant FS as FileSystem (renderer/dist)
participant NET as Network (API)
R->>M: Request renderer asset (superset-app://app/index.html)
M->>FS: Read file from `renderer/dist/...`
FS-->>M: Return file contents
M-->>R: Serve asset with CORS-enabled headers
R->>NET: API request (Origin: superset-app://app)
NET->>M: OnBeforeSendHeaders (strip Origin for API paths) %% note: header mutation step handled by main
M->>NET: Forwarded request without Origin
NET-->>M: Response (with CORS headers set by Main onHeadersReceived)
M-->>R: Return API response with Access-Control-Allow-Origin: superset-app://app
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No issues found across 8 files
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/electron.vite.config.ts (1)
99-112:⚠️ Potential issue | 🟠 MajorMove environment variable cleanup out of the shared bundle banner, or scope it only to the GUI entry point.
The banner at lines 111–112 is emitted into all three main entry bundles (
index,terminal-host,pty-subprocess), but this is incorrect for the daemon processes.terminal-hostandpty-subprocessare spawned as separate processes withELECTRON_RUN_AS_NODE=1explicitly set in their environment and require it to run in Node mode. Additionally, deleting the variable inside the bundle has no effect on process mode—Electron initializes this before the bundle code executes, so the process is already locked into Node mode by then. Consider removing the shared banner or moving environment variable scrubbing to the parent process startup if needed for the GUI entry only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/electron.vite.config.ts` around lines 99 - 112, The shared output.banner ('delete process.env.ELECTRON_RUN_AS_NODE;') is being injected into all three main entry bundles (input keys "index", "terminal-host", "pty-subprocess"), which breaks daemon processes that must retain ELECTRON_RUN_AS_NODE=1; remove the banner from the shared output (the output object that sets dir via resolve(devPath, "main")) and either (a) add the environment-variable scrub only to the GUI entry bundle (the "index" entry) by scoping a banner/output option to that entry, or (b) move the env cleanup into the GUI parent startup code instead of bundling it; update build/rollup options accordingly so only the GUI process gets the banner and the terminal-host and pty-subprocess bundles remain untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/postinstall.mjs`:
- Around line 29-48: The current tryRun wrapper swallows all errors from "bun
run --filter=@superset/desktop install:deps" on Windows; change it so only
known, tolerated node-gyp/build-tools errors are ignored and all other failures
are rethrown. Update tryRun (and the Windows branch that calls it) to capture
the thrown error, match its message/exit code against an allowlist of
node-gyp/Visual Studio Build Tools patterns, and only log-and-continue for those
matches; otherwise rethrow the error so bun install fails fast. Additionally,
after a tolerated failure, invoke the desktop runtime validation (the existing
validate-native-runtime/copy-native-modules check or the script
apps/desktop/scripts/validate-native-runtime.ts) to assert required runtime
packages are present and fail if they are missing.
---
Outside diff comments:
In `@apps/desktop/electron.vite.config.ts`:
- Around line 99-112: The shared output.banner ('delete
process.env.ELECTRON_RUN_AS_NODE;') is being injected into all three main entry
bundles (input keys "index", "terminal-host", "pty-subprocess"), which breaks
daemon processes that must retain ELECTRON_RUN_AS_NODE=1; remove the banner from
the shared output (the output object that sets dir via resolve(devPath, "main"))
and either (a) add the environment-variable scrub only to the GUI entry bundle
(the "index" entry) by scoping a banner/output option to that entry, or (b) move
the env cleanup into the GUI parent startup code instead of bundling it; update
build/rollup options accordingly so only the GUI process gets the banner and the
terminal-host and pty-subprocess bundles remain untouched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2789c9a-af0d-44ff-b192-6a196b1fbbdb
📒 Files selected for processing (8)
apps/desktop/electron-builder.tsapps/desktop/electron.vite.config.tsapps/desktop/scripts/copy-native-modules.tsapps/desktop/src/lib/electron-app/factories/app/setup.tsapps/desktop/src/main/windows/main.tsapps/desktop/vite/helpers.tspackage.jsonscripts/postinstall.mjs
Comment on lines
+29
to
+48
| /** Run a command but don't fail if it errors (for optional native deps on Windows). */ | ||
| function tryRun(cmd, label) { | ||
| try { | ||
| execSync(cmd, { stdio: "inherit", env }); | ||
| } catch { | ||
| console.warn(`[postinstall] ${label} failed (non-fatal on Windows) — continuing`); | ||
| } | ||
| } | ||
|
|
||
| // Run sherif for workspace validation | ||
| run("sherif"); | ||
|
|
||
| // Install native dependencies for desktop app. | ||
| // On Windows, native module compilation may fail if Visual Studio Build Tools | ||
| // are not installed. This is non-fatal — prebuilt binaries will be used when available. | ||
| if (process.platform === "win32") { | ||
| tryRun("bun run --filter=@superset/desktop install:deps", "install:deps"); | ||
| } else { | ||
| run("bun run --filter=@superset/desktop install:deps"); | ||
| } |
Contributor
There was a problem hiding this comment.
Don't swallow every Windows install:deps failure.
This makes bun install succeed even when the desktop native runtime is only partially prepared, and the first hard failure then moves downstream into apps/desktop/scripts/copy-native-modules.ts / apps/desktop/scripts/validate-native-runtime.ts. If you want to tolerate missing Build Tools, only downgrade known node-gyp cases and then assert that the required runtime packages are actually present.
Suggested guard
import { execSync } from "node:child_process";
+import { existsSync } from "node:fs";
+import { join } from "node:path";
@@
function tryRun(cmd, label) {
try {
execSync(cmd, { stdio: "inherit", env });
} catch {
console.warn(`[postinstall] ${label} failed (non-fatal on Windows) — continuing`);
}
}
+
+function assertDesktopNativeDepsInstalled() {
+ const required = [
+ "better-sqlite3",
+ "node-pty",
+ "@ast-grep/napi",
+ "libsql",
+ ];
+ const missing = required.filter(
+ (name) =>
+ !existsSync(join("apps/desktop/node_modules", name, "package.json")),
+ );
+ if (missing.length) {
+ throw new Error(
+ `[postinstall] Missing desktop native deps after install:deps: ${missing.join(", ")}`,
+ );
+ }
+}
@@
if (process.platform === "win32") {
tryRun("bun run --filter=@superset/desktop install:deps", "install:deps");
+ assertDesktopNativeDepsInstalled();
} else {
run("bun run --filter=@superset/desktop install:deps");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/postinstall.mjs` around lines 29 - 48, The current tryRun wrapper
swallows all errors from "bun run --filter=@superset/desktop install:deps" on
Windows; change it so only known, tolerated node-gyp/build-tools errors are
ignored and all other failures are rethrown. Update tryRun (and the Windows
branch that calls it) to capture the thrown error, match its message/exit code
against an allowlist of node-gyp/Visual Studio Build Tools patterns, and only
log-and-continue for those matches; otherwise rethrow the error so bun install
fails fast. Additionally, after a tolerated failure, invoke the desktop runtime
validation (the existing validate-native-runtime/copy-native-modules check or
the script apps/desktop/scripts/validate-native-runtime.ts) to assert required
runtime packages are present and fail if they are missing.
…ture flags - Register superset-app:// custom protocol to serve renderer files, fixing dynamic import failures (code-split chunks) on file:// protocol in Electron on Windows - Add CORS header bypass for API requests from the custom protocol origin - Load renderer via superset-app://app/index.html#/ on Windows in production - Default PostHog feature flag to false when not configured, preventing CollectionsProvider from blocking render indefinitely Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/main/index.ts">
<violation number="1" location="apps/desktop/src/main/index.ts:304">
P1: Sanitize `superset-app://` paths before joining to `rendererDir`; as written, `../` traversal can escape the renderer directory and expose arbitrary local files via the custom protocol.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx:69">
P2: Defaulting `useElectricCloud` to false causes collections to be created with the proxy URL while feature flags are still loading. Since collections are cached and capture `electricUrl` at creation, they won’t switch to the cloud URL when the flag later resolves to true, so the app can stay stuck on the proxy backend.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+304
to
+305
| const filePath = path.join(rendererDir, urlPath); | ||
| return net.fetch(pathToFileURL(filePath).toString()); |
Contributor
There was a problem hiding this comment.
P1: Sanitize superset-app:// paths before joining to rendererDir; as written, ../ traversal can escape the renderer directory and expose arbitrary local files via the custom protocol.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/index.ts, line 304:
<comment>Sanitize `superset-app://` paths before joining to `rendererDir`; as written, `../` traversal can escape the renderer directory and expose arbitrary local files via the custom protocol.</comment>
<file context>
@@ -283,6 +292,48 @@ if (!gotTheLock) {
+ let urlPath = new URL(request.url).pathname;
+ // Remove leading slash on Windows
+ if (urlPath.startsWith("/")) urlPath = urlPath.slice(1);
+ const filePath = path.join(rendererDir, urlPath);
+ return net.fetch(pathToFileURL(filePath).toString());
+ };
</file context>
Suggested change
| const filePath = path.join(rendererDir, urlPath); | |
| return net.fetch(pathToFileURL(filePath).toString()); | |
| const resolvedPath = path.resolve(rendererDir, urlPath); | |
| const relativePath = path.relative(rendererDir, resolvedPath); | |
| if (relativePath.startsWith("..") || path.isAbsolute(relativePath)) { | |
| return new Response("Not found", { status: 404 }); | |
| } | |
| return net.fetch(pathToFileURL(resolvedPath).toString()); |
| } | ||
| // When PostHog is not configured (no key), feature flags stay undefined forever. | ||
| // Default to false (use proxy) so the app doesn't block rendering. | ||
| const isElectricCloud = useElectricCloud ?? false; |
Contributor
There was a problem hiding this comment.
P2: Defaulting useElectricCloud to false causes collections to be created with the proxy URL while feature flags are still loading. Since collections are cached and capture electricUrl at creation, they won’t switch to the cloud URL when the flag later resolves to true, so the app can stay stuck on the proxy backend.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx, line 69:
<comment>Defaulting `useElectricCloud` to false causes collections to be created with the proxy URL while feature flags are still loading. Since collections are cached and capture `electricUrl` at creation, they won’t switch to the cloud URL when the flag later resolves to true, so the app can stay stuck on the proxy backend.</comment>
<file context>
@@ -64,12 +64,12 @@ export function CollectionsProvider({ children }: { children: ReactNode }) {
- }
+ // When PostHog is not configured (no key), feature flags stay undefined forever.
+ // Default to false (use proxy) so the app doesn't block rendering.
+ const isElectricCloud = useElectricCloud ?? false;
setElectricUrl(
</file context>
Suggested change
| const isElectricCloud = useElectricCloud ?? false; | |
| const isElectricCloud = | |
| useElectricCloud ?? (env.NEXT_PUBLIC_POSTHOG_KEY ? undefined : false); | |
| if (isElectricCloud === undefined) { | |
| return null; | |
| } |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/desktop/src/lib/window-loader.ts (1)
27-32: Windows branch ignoreshtmlFileandqueryparameters.The function signature accepts
htmlFileandqueryparameters, but the Windows branch hardcodessuperset-app://app/index.html#/and ignores both. While this works for the current single call site (which always passes"index.html"and no query), it creates a silent inconsistency if future callers rely on these parameters.Consider either:
- Documenting this limitation in the JSDoc
- Using the parameters to construct the URL dynamically
Option: Use htmlFile dynamically
} else if (process.platform === "win32") { // Production (Windows): use custom protocol for proper dynamic import support. // file:// protocol breaks ES module dynamic imports (code-split chunks) on Windows. - const url = "superset-app://app/index.html#/"; + const fileName = path.basename(props.htmlFile); + const queryString = props.query + ? `?${new URLSearchParams(props.query).toString()}` + : ""; + const url = `superset-app://app/${fileName}${queryString}#/`; console.log("[window-loader] Loading custom protocol URL:", url); props.browserWindow.loadURL(url);This would require adding
import path from "node:path";at the top.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/window-loader.ts` around lines 27 - 32, The Windows branch in window-loader.ts hardcodes the URL and ignores the function parameters htmlFile and query; update the branch that calls props.browserWindow.loadURL to construct the custom-protocol URL using the htmlFile and query args (e.g., use htmlFile instead of "index.html" and append serialized/encoded query if present) so callers are consistent; add the required import (import path from "node:path") at the top if you need to resolve or normalize htmlFile, and ensure the constructed URL still uses the superset-app://app/ scheme and preserves the existing hash/prefix behavior used by the Windows branch (reference the branch that currently logs "[window-loader] Loading custom protocol URL:" and the props.browserWindow.loadURL call).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/index.ts`:
- Around line 299-306: The protocol handler (appProtocolHandler) currently joins
the unvalidated urlPath into rendererDir allowing path traversal (e.g.,
../../../). Fix by resolving the requested path (use path.resolve) and verifying
the resolved path is inside rendererDir (e.g., compare
resolved.startsWith(rendererDir) or ensure path.relative(rendererDir, resolved)
doesn’t start with '..'); if the check fails, return a safe error response
instead of calling net.fetch on the out-of-bounds filePath. Keep the remaining
flow (pathToFileURL and net.fetch) but only use them after the directory
containment check.
- Around line 316-334: The CORS bypass uses hardcoded URL strings; update
appSession.webRequest.onBeforeSendHeaders and
appSession.webRequest.onHeadersReceived to build the URL pattern list from the
environment vars defined in env.main.ts (NEXT_PUBLIC_API_URL,
NEXT_PUBLIC_STREAMS_URL, NEXT_PUBLIC_ELECTRIC_URL, NEXT_PUBLIC_POSTHOG_HOST)
instead of literal "api.superset.sh", "streams.superset.sh", "electric-proxy",
and posthog URLs; ensure the constructed array includes streams and
electric-proxy entries and use the same array for both handlers (fall back to
sensible defaults or skip entries if an env var is missing) so
onBeforeSendHeaders and onHeadersReceived remain consistent.
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx`:
- Around line 67-75: The code currently collapses an unresolved feature flag
into false by using `const isElectricCloud = useElectricCloud ?? false`, causing
`setElectricUrl` (in the `CollectionsProvider`) to point at the proxy before the
flag resolves and preventing the preload effect (which depends only on
`activeOrganizationId`) from re-running; change the logic so `useElectricCloud`
is treated tri-state (true/false/undefined) — only set the `electricUrl` to the
proxy when `useElectricCloud === false`, set it to cloud URL when
`useElectricCloud === true`, and do not change `electricUrl` while
`useElectricCloud` is `undefined`; additionally, ensure the preload effect that
fetches collections (the effect referencing `activeOrganizationId`) also depends
on the resolved flag (either `useElectricCloud` or derived `electricUrl`) so it
re-triggers when the feature flag resolves and avoids using the cached
proxy-bound collections from `collections.ts`.
---
Nitpick comments:
In `@apps/desktop/src/lib/window-loader.ts`:
- Around line 27-32: The Windows branch in window-loader.ts hardcodes the URL
and ignores the function parameters htmlFile and query; update the branch that
calls props.browserWindow.loadURL to construct the custom-protocol URL using the
htmlFile and query args (e.g., use htmlFile instead of "index.html" and append
serialized/encoded query if present) so callers are consistent; add the required
import (import path from "node:path") at the top if you need to resolve or
normalize htmlFile, and ensure the constructed URL still uses the
superset-app://app/ scheme and preserves the existing hash/prefix behavior used
by the Windows branch (reference the branch that currently logs "[window-loader]
Loading custom protocol URL:" and the props.browserWindow.loadURL call).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: faf6440c-eddf-4904-953d-dd423990b043
📒 Files selected for processing (3)
apps/desktop/src/lib/window-loader.tsapps/desktop/src/main/index.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx
Comment on lines
+299
to
+306
| const rendererDir = path.join(__dirname, "../renderer"); | ||
| const appProtocolHandler = (request: Request) => { | ||
| let urlPath = new URL(request.url).pathname; | ||
| // Remove leading slash on Windows | ||
| if (urlPath.startsWith("/")) urlPath = urlPath.slice(1); | ||
| const filePath = path.join(rendererDir, urlPath); | ||
| return net.fetch(pathToFileURL(filePath).toString()); | ||
| }; |
Contributor
There was a problem hiding this comment.
Path traversal vulnerability in protocol handler.
The urlPath extracted from the request URL is joined directly to rendererDir without validation. A malicious request like superset-app://app/../../../etc/passwd could escape the renderer directory and serve arbitrary files.
🛡️ Proposed fix: Validate resolved path stays within rendererDir
const appProtocolHandler = (request: Request) => {
let urlPath = new URL(request.url).pathname;
// Remove leading slash on Windows
if (urlPath.startsWith("/")) urlPath = urlPath.slice(1);
+ // Decode URL-encoded characters and normalize
+ urlPath = decodeURIComponent(urlPath);
const filePath = path.join(rendererDir, urlPath);
+ // Prevent path traversal attacks
+ const resolvedPath = path.resolve(filePath);
+ if (!resolvedPath.startsWith(path.resolve(rendererDir) + path.sep) && resolvedPath !== path.resolve(rendererDir)) {
+ return new Response("Forbidden", { status: 403 });
+ }
return net.fetch(pathToFileURL(filePath).toString());
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const rendererDir = path.join(__dirname, "../renderer"); | |
| const appProtocolHandler = (request: Request) => { | |
| let urlPath = new URL(request.url).pathname; | |
| // Remove leading slash on Windows | |
| if (urlPath.startsWith("/")) urlPath = urlPath.slice(1); | |
| const filePath = path.join(rendererDir, urlPath); | |
| return net.fetch(pathToFileURL(filePath).toString()); | |
| }; | |
| const rendererDir = path.join(__dirname, "../renderer"); | |
| const appProtocolHandler = (request: Request) => { | |
| let urlPath = new URL(request.url).pathname; | |
| // Remove leading slash on Windows | |
| if (urlPath.startsWith("/")) urlPath = urlPath.slice(1); | |
| // Decode URL-encoded characters and normalize | |
| urlPath = decodeURIComponent(urlPath); | |
| const filePath = path.join(rendererDir, urlPath); | |
| // Prevent path traversal attacks | |
| const resolvedPath = path.resolve(filePath); | |
| if (!resolvedPath.startsWith(path.resolve(rendererDir) + path.sep) && resolvedPath !== path.resolve(rendererDir)) { | |
| return new Response("Forbidden", { status: 403 }); | |
| } | |
| return net.fetch(pathToFileURL(filePath).toString()); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/index.ts` around lines 299 - 306, The protocol handler
(appProtocolHandler) currently joins the unvalidated urlPath into rendererDir
allowing path traversal (e.g., ../../../). Fix by resolving the requested path
(use path.resolve) and verifying the resolved path is inside rendererDir (e.g.,
compare resolved.startsWith(rendererDir) or ensure path.relative(rendererDir,
resolved) doesn’t start with '..'); if the check fails, return a safe error
response instead of calling net.fetch on the out-of-bounds filePath. Keep the
remaining flow (pathToFileURL and net.fetch) but only use them after the
directory containment check.
Comment on lines
+316
to
+334
| appSession.webRequest.onBeforeSendHeaders( | ||
| { urls: ["https://api.superset.sh/*", "https://*.posthog.com/*", "https://*.sentry.io/*", "https://app.outlit.ai/*"] }, | ||
| (details, callback) => { | ||
| // Replace custom protocol origin with the expected web origin | ||
| if (details.requestHeaders.Origin === "superset-app://app") { | ||
| delete details.requestHeaders.Origin; | ||
| } | ||
| callback({ requestHeaders: details.requestHeaders }); | ||
| }, | ||
| ); | ||
| appSession.webRequest.onHeadersReceived( | ||
| { urls: ["https://api.superset.sh/*"] }, | ||
| (details, callback) => { | ||
| const headers = details.responseHeaders ?? {}; | ||
| headers["access-control-allow-origin"] = ["superset-app://app"]; | ||
| headers["access-control-allow-credentials"] = ["true"]; | ||
| callback({ responseHeaders: headers }); | ||
| }, | ||
| ); |
Contributor
There was a problem hiding this comment.
Hardcoded URLs in CORS bypass may drift from environment configuration.
The URL patterns are hardcoded, but apps/desktop/src/main/env.main.ts defines these URLs via environment variables (NEXT_PUBLIC_API_URL, NEXT_PUBLIC_STREAMS_URL, NEXT_PUBLIC_ELECTRIC_URL, NEXT_PUBLIC_POSTHOG_HOST). This creates a maintenance risk where env changes won't be reflected in the CORS bypass.
Additionally:
onBeforeSendHeaderscovers 4 URL patterns, butonHeadersReceivedonly coversapi.superset.sh/*- Missing
streams.superset.shandelectric-proxyURLs that are also configured in env
♻️ Suggested approach: Build URL patterns from env
if (PLATFORM.IS_WINDOWS) {
+ // Import env from ./env.main or access via shared config
+ const corsUrls = [
+ `${env.NEXT_PUBLIC_API_URL}/*`,
+ `${env.NEXT_PUBLIC_STREAMS_URL}/*`,
+ `${env.NEXT_PUBLIC_POSTHOG_HOST}/*`,
+ "https://*.sentry.io/*",
+ "https://app.outlit.ai/*",
+ ];
const appSession = session.fromPartition("persist:superset");
appSession.webRequest.onBeforeSendHeaders(
- { urls: ["https://api.superset.sh/*", "https://*.posthog.com/*", "https://*.sentry.io/*", "https://app.outlit.ai/*"] },
+ { urls: corsUrls },
(details, callback) => {Also consider applying onHeadersReceived to the same URL set for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/index.ts` around lines 316 - 334, The CORS bypass uses
hardcoded URL strings; update appSession.webRequest.onBeforeSendHeaders and
appSession.webRequest.onHeadersReceived to build the URL pattern list from the
environment vars defined in env.main.ts (NEXT_PUBLIC_API_URL,
NEXT_PUBLIC_STREAMS_URL, NEXT_PUBLIC_ELECTRIC_URL, NEXT_PUBLIC_POSTHOG_HOST)
instead of literal "api.superset.sh", "streams.superset.sh", "electric-proxy",
and posthog URLs; ensure the constructed array includes streams and
electric-proxy entries and use the same array for both handlers (fall back to
sensible defaults or skip entries if an env var is missing) so
onBeforeSendHeaders and onHeadersReceived remain consistent.
Comment on lines
+67
to
75
| // When PostHog is not configured (no key), feature flags stay undefined forever. | ||
| // Default to false (use proxy) so the app doesn't block rendering. | ||
| const isElectricCloud = useElectricCloud ?? false; | ||
|
|
||
| setElectricUrl( | ||
| useElectricCloud | ||
| isElectricCloud | ||
| ? env.NEXT_PUBLIC_ELECTRIC_URL | ||
| : env.NEXT_PUBLIC_ELECTRIC_PROXY_URL, | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts'
echo '--- collections.ts (first 260 lines) ---'
head -260 "$FILE" | nl -ba
echo
echo '--- electricUrl state + collection cache references ---'
rg -n -C3 'electricUrl|setElectricUrl|getCollections|preloadCollections|Map|cache' "$FILE"Repository: superset-sh/superset
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts'
echo '--- collections.ts (first 260 lines with line numbers) ---'
head -260 "$FILE" | cat -n
echo
echo '--- electricUrl state + collection cache references ---'
rg -n -C3 'electricUrl|setElectricUrl|getCollections|preloadCollections|Map|cache' "$FILE" || trueRepository: superset-sh/superset
Length of output: 15682
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx'
echo '--- CollectionsProvider.tsx (lines 40-80) ---'
sed -n '40,80p' "$FILE" | cat -n -v
echo
echo '--- useEffect blocks and related logic ---'
rg -n -B2 -A5 'useEffect|useElectricCloud|setElectricUrl|preloadActiveOrganizationCollections' "$FILE"Repository: superset-sh/superset
Length of output: 3509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Searching for FEATURE_FLAGS.ELECTRIC_CLOUD definition ---'
rg -n 'FEATURE_FLAGS|ELECTRIC_CLOUD' apps/desktop/src/renderer --type ts --type tsx -B2 -A2 | head -50
echo
echo '--- Checking PostHog hook behavior in imports ---'
rg -n 'useFeatureFlagEnabled' apps/desktop/src/renderer --type ts --type tsx | head -20Repository: superset-sh/superset
Length of output: 170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Searching for FEATURE_FLAGS.ELECTRIC_CLOUD definition ---'
rg -n 'FEATURE_FLAGS|ELECTRIC_CLOUD' apps/desktop/src/renderer -g '*.ts' -g '*.tsx' -B2 -A2 | head -60
echo
echo '--- Checking env.renderer for PostHog key ---'
cat apps/desktop/src/renderer/env.renderer.ts | cat -nRepository: superset-sh/superset
Length of output: 10539
Don't treat a loading flag the same as a disabled flag.
Line 69 collapses both "PostHog is not configured" and "the flag has not resolved yet" into false. On a PostHog-enabled startup, that means lines 71-75 point electricUrl at the proxy before the real flag arrives. Meanwhile, lines 63-65 preload collections only once per activeOrganizationId. If the flag later resolves to true, the preload effect never re-triggers because activeOrganizationId hasn't changed. Since collections.ts caches collections per org with the electricUrl value baked in at creation time, cloud-enabled users can stay on the proxy for that session.
Suggested fix
- // When PostHog is not configured (no key), feature flags stay undefined forever.
- // Default to false (use proxy) so the app doesn't block rendering.
- const isElectricCloud = useElectricCloud ?? false;
-
- setElectricUrl(
- isElectricCloud
- ? env.NEXT_PUBLIC_ELECTRIC_URL
- : env.NEXT_PUBLIC_ELECTRIC_PROXY_URL,
- );
+ const isPostHogConfigured = Boolean(env.NEXT_PUBLIC_POSTHOG_KEY);
+ const isElectricCloud = isPostHogConfigured ? useElectricCloud : false;
+ const electricBaseUrl =
+ isElectricCloud === true
+ ? env.NEXT_PUBLIC_ELECTRIC_URL
+ : env.NEXT_PUBLIC_ELECTRIC_PROXY_URL;
+
+ useEffect(() => {
+ setElectricUrl(electricBaseUrl);
+ }, [electricBaseUrl]);
- useEffect(() => {
- preloadActiveOrganizationCollections(activeOrganizationId);
- }, [activeOrganizationId]);
+ useEffect(() => {
+ if (isPostHogConfigured && isElectricCloud === undefined) return;
+ preloadActiveOrganizationCollections(activeOrganizationId);
+ }, [activeOrganizationId, electricBaseUrl, isPostHogConfigured, isElectricCloud]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx`
around lines 67 - 75, The code currently collapses an unresolved feature flag
into false by using `const isElectricCloud = useElectricCloud ?? false`, causing
`setElectricUrl` (in the `CollectionsProvider`) to point at the proxy before the
flag resolves and preventing the preload effect (which depends only on
`activeOrganizationId`) from re-running; change the logic so `useElectricCloud`
is treated tri-state (true/false/undefined) — only set the `electricUrl` to the
proxy when `useElectricCloud === false`, set it to cloud URL when
`useElectricCloud === true`, and do not change `electricUrl` while
`useElectricCloud` is `undefined`; additionally, ensure the preload effect that
fetches collections (the effect referencing `activeOrganizationId`) also depends
on the resolved flag (either `useElectricCloud` or derived `electricUrl`) so it
re-triggers when the feature flag resolves and avoids using the cached
proxy-bound collections from `collections.ts`.
This was referenced Mar 13, 2026
15 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds Windows build and runtime support for the Superset desktop app. Consolidates and improves upon fixes from existing Windows PRs (#1617, #1119, #694).
Changes
postinstall.shwith Node.jspostinstall.mjssobun installworks on Windowsrequire("electron")— VS Code and other Electron hosts set this, preventing Electron from entering browser modecrossoriginattributes on Windows: Electron's ASARfile://handler doesn't support CORS, causing scripts/styles to silently fail to loadrmSync({ recursive: true })since Windows junctions are directory-like, not file symlinksnpmRebuildon Windows: Native modules are handled byinstall:deps/copy:native-modules, andnode-gypfails without Visual Studio Build ToolsdefineEnvfallback: Use||instead of??so empty strings from unresolved CI secrets fall back to defaultsTest plan
bun installcompletes on Windows 11 Pro x64bun run compile:app(electron-vite build) succeedselectron-builder --win --dirproducesrelease/win-unpacked/Superset.exelaunches, shows window with title "Superset"Environment
🤖 Generated with Claude Code
Summary by cubic
Adds Windows build and runtime support for Superset Desktop. Users can install, build, and launch on Windows with a working installer and stable runtime.
New Features
Bug Fixes
Written for commit 6bd2fca. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Improvements
New Features
Other