feat(desktop): add Windows build to CI/CD pipeline#1617
Conversation
Add a `build-windows` job to the reusable build workflow targeting `windows-latest` with NSIS installer output. Update both stable and canary release workflows to handle `.exe` artifacts and `*-win.yml` auto-update manifests alongside existing macOS and Linux artifacts.
The root postinstall script is a bash .sh file that Bun on Windows cannot execute directly. Use --ignore-scripts and manually run the desktop native deps install step instead.
Replace the bash-only postinstall.sh with a Node script (postinstall.mjs) so bun install works on Windows without needing --ignore-scripts. The script does the same two things: runs sherif for workspace validation and installs native desktop deps.
Bun creates junctions instead of symlinks on Windows. rmSync() without recursive/force flags fails with EFAULT on junctions.
electron-builder names the Windows auto-update manifest latest.yml (no -win suffix), unlike macOS (latest-mac.yml) and Linux (latest-linux.yml). Update verify, upload, and release steps to match the actual filename.
… black screen GPU driver incompatibilities with Chromium's compositor can cause a completely black window on Windows. Extend the existing Linux GPU disable to Windows. Also add missing NEXT_PUBLIC_ELECTRIC_URL env var to macOS and Windows builds.
Vite adds `crossorigin` to script/link tags by default. Electron's ASAR file:// handler doesn't support CORS, so this causes the main JS bundle and CSS to silently fail to load, resulting in a black screen when the app is packaged. Add a post-order Vite plugin that strips the attribute.
- Forward renderer console messages to main process stdout - Add step-by-step logging to renderer entry point - Auto-open DevTools in production (temporary)
…back GitHub Actions resolves unset secrets to empty strings. The nullish coalescing operator (??) doesn't treat "" as missing, so env var defaults like "https://api.superset.sh" never kick in. This causes Zod URL validation to fail on empty strings, crashing the renderer before React mounts (black screen). Also removes temporary debug logging added in previous commit.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request adds Windows support to the desktop application build pipeline, including GitHub Actions workflow jobs for building and releasing Windows executables, Windows-specific Vite configuration to strip cross-origin attributes, platform-aware native module handling, hardware acceleration management, renderer console debugging, and a cross-platform postinstall script replacing the existing shell-based setup. Changes
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 |
There was a problem hiding this comment.
Pull request overview
This PR adds Windows support to the desktop application's CI/CD pipeline, addressing platform-specific build issues and fixing renderer loading problems that caused black screens in packaged builds.
Changes:
- Added Windows x64 build job to CI/CD pipeline with NSIS installer output and auto-update manifest handling
- Replaced bash-only
postinstall.shwith cross-platformscripts/postinstall.mjsfor Windows compatibility - Fixed black screen issues by stripping Vite's
crossoriginattribute, using||operator for empty string handling, and disabling GPU acceleration on Windows - Enhanced Windows compatibility in native module copying (junction handling) and added renderer console forwarding for production debugging
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
scripts/postinstall.mjs |
New cross-platform postinstall script replacing bash-only version, with recursion guard and workspace validation |
package.json |
Updated postinstall command to use Node.js instead of bash |
apps/desktop/vite/helpers.ts |
Added stripCrossOriginPlugin() to fix ASAR loading issues and changed defineEnv() to use || for empty string fallback |
apps/desktop/electron.vite.config.ts |
Integrated stripCrossOriginPlugin into renderer build pipeline |
apps/desktop/src/main/windows/main.ts |
Added console message forwarding from renderer to main process for production debugging |
apps/desktop/src/lib/electron-app/factories/app/setup.ts |
Extended GPU hardware acceleration disabling to Windows (previously Linux-only) |
apps/desktop/scripts/copy-native-modules.ts |
Added recursive: true, force: true to rmSync() for Windows junction compatibility |
.github/workflows/build-desktop.yml |
Added complete Windows build job with installer verification and artifact uploads |
.github/workflows/release-desktop.yml |
Added Windows .exe artifact handling and update manifest logic |
.github/workflows/release-desktop-canary.yml |
Added Windows canary build artifact handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
apps/desktop/src/lib/electron-app/factories/app/setup.ts (1)
73-76: Considerifstatement for side-effect call.Using
&&for a void side effect is pre-existing here, but anifblock communicates intent more clearly for readers.♻️ Optional readability improvement
-// Disable GPU hardware acceleration on Linux and Windows to prevent black/blank -// screens caused by GPU driver incompatibilities with Chromium's compositor. -// macOS uses a separate set of GPU workarounds (see below and MainWindow). -(PLATFORM.IS_LINUX || PLATFORM.IS_WINDOWS) && app.disableHardwareAcceleration(); +// Disable GPU hardware acceleration on Linux and Windows to prevent black/blank +// screens caused by GPU driver incompatibilities with Chromium's compositor. +// macOS uses a separate set of GPU workarounds (see below and MainWindow). +if (PLATFORM.IS_LINUX || PLATFORM.IS_WINDOWS) { + app.disableHardwareAcceleration(); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/electron-app/factories/app/setup.ts` around lines 73 - 76, Replace the terse logical-and side-effect with an explicit conditional: check PLATFORM.IS_LINUX || PLATFORM.IS_WINDOWS in an if statement and call app.disableHardwareAcceleration() inside its block (referencing PLATFORM.IS_LINUX, PLATFORM.IS_WINDOWS and app.disableHardwareAcceleration) so the intent is clear and readable.apps/desktop/src/main/windows/main.ts (1)
225-229: Preserve renderer log severity — useconsole.warn/console.errorfor levels ≥ 2.Every renderer log level (verbose, info, warning, error) is currently forwarded via
console.log. Errors won't surface as red entries in the terminal, and any log-level filtering in the main process or external log aggregators will miss them.♻️ Proposed fix
- window.webContents.on("console-message", (_event, level, message, line, sourceId) => { - const levelStr = ["verbose", "info", "warning", "error"][level] ?? "unknown"; - const source = sourceId ? ` (${sourceId}:${line})` : ""; - console.log(`[renderer:${levelStr}] ${message}${source}`); - }); + window.webContents.on("console-message", (_event, level, message, line, sourceId) => { + const levelStr = ["verbose", "info", "warning", "error"][level] ?? "unknown"; + const source = sourceId ? ` (${sourceId}:${line})` : ""; + const formatted = `[renderer:${levelStr}] ${message}${source}`; + if (level === 3) console.error(formatted); + else if (level === 2) console.warn(formatted); + else console.log(formatted); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/windows/main.ts` around lines 225 - 229, The renderer console handler in window.webContents.on("console-message") currently routes all messages through console.log; update the handler to preserve severity by switching on the numeric level (0: verbose, 1: info, 2: warning, 3: error) and call console.log for levels 0–1, console.warn for level 2, and console.error for level 3, keeping the existing levelStr and source formatting logic so the message string construction (levelStr, message, source) is reused but emitted with the appropriate console method.scripts/postinstall.mjs (1)
22-28: Minor:SUPERSET_POSTINSTALL_RUNNINGinrun()env is redundant.
process.env.SUPERSET_POSTINSTALL_RUNNINGis already set on line 20, so spreading{ ...process.env, SUPERSET_POSTINSTALL_RUNNING: "1" }duplicates it. It's harmless and defensive, but worth knowing it adds no extra protection.♻️ Simplified env spread
function run(cmd) { execSync(cmd, { stdio: "inherit", - env: { ...process.env, SUPERSET_POSTINSTALL_RUNNING: "1" }, + env: process.env, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/postinstall.mjs` around lines 22 - 28, The run function currently passes env: { ...process.env, SUPERSET_POSTINSTALL_RUNNING: "1" } which redundantly duplicates the env var already set on process.env; update the run(cmd) implementation (the run function) to remove the spread and either omit the env option entirely (let child inherit process.env) or pass env: process.env so it relies on the existing SUPERSET_POSTINSTALL_RUNNING value instead of recreating it.apps/desktop/vite/helpers.ts (1)
23-25:defineEnv("")with no fallback now producesundefinedin thedefinemap — verify Vite handles it gracefully.
JSON.stringify(undefined)returns the JS valueundefined(not the string"undefined"), which violates the declaredstringreturn type. When bothvalueis""and nofallbackis given, thedefineentry becomesundefined. Vite's documented contract fordefinevalues isstring; behaviour withundefinedis implementation-defined.This is a pre-existing gap (same for
value = undefined), but||widens the window to empty-string inputs. CI passes, so Vite is tolerating it, but consider making the return type explicit:♻️ Type-safe alternative
export function defineEnv( value: string | undefined, fallback?: string, -): string { - // Use || instead of ?? so empty strings (from unresolved GitHub Actions - // secrets) also fall through to the default value. - return JSON.stringify(value || fallback); +): string | undefined { + // Use || instead of ?? so empty strings (from unresolved GitHub Actions + // secrets) also fall through to the default value. + const resolved = value || fallback; + return resolved !== undefined ? JSON.stringify(resolved) : undefined; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/vite/helpers.ts` around lines 23 - 25, The current defineEnv helper can return the JS value undefined (via JSON.stringify(undefined)) when both value is "" or undefined and fallback is omitted; update defineEnv in helpers.ts so it always returns a JSON-stringified string: choose value when it is a non-empty string, otherwise use fallback if provided, and if neither is present fall back to the empty string, then JSON.stringify that resolved string before returning. Ensure you reference the defineEnv function and replace the current return logic so the function's declared string return type is always satisfied..github/workflows/build-desktop.yml (2)
266-280: Version suffix step: minor shell-injection surface viainputs.version_suffix.The
${{ inputs.version_suffix }}expression on line 272 is interpolated directly into the bash script. Since this is aworkflow_callinput (not user-controlledworkflow_dispatchfreetext from an external fork), the risk is low — callers are trusted internal workflows. However, if this workflow is ever exposed to less-trusted callers, a crafted suffix could break out of the string assignment.The macOS and Linux jobs have the same pattern (lines 69, 175), so this is pre-existing. Just flagging for awareness — if you ever open this to
workflow_dispatch, sanitize or quote the input.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-desktop.yml around lines 266 - 280, The step interpolates `${{ inputs.version_suffix }}` directly into the bash script which can allow shell-injection if untrusted; change the approach to avoid direct interpolation by passing the suffix via a safe environment variable or by properly quoting/sanitizing it before use: e.g., set an env var like VERSION_SUFFIX from the workflow input and use bash variable expansion (or printf %q) to build NEW_VERSION, or have the Node snippet read process.env.VERSION_SUFFIX instead of embedding '$NEW_VERSION' into the inline script (update references to CURRENT_VERSION, NEW_VERSION, and the inline node -e writer accordingly) so arbitrary characters cannot break out of the command string.
333-341: Windows manifest upload uses a broader glob than macOS/Linux — consider tightening.macOS uploads
*-mac.yml(line 140) and Linux uploads*-linux.yml(line 235), both targeting the specific auto-update manifest. The Windows upload uses*.ymlwith onlybuilder-debug.ymlexcluded. This could accidentally capture other electron-builder output files (e.g.,builder-effective-config.yml).Since the verify step on line 320 already confirms
latest.ymlexists, you could target it directly:🔧 Proposed tighter glob for Windows manifest
- name: Upload Windows auto-update manifest uses: actions/upload-artifact@v4 with: name: ${{ inputs.artifact_prefix }}-win-update-manifest - path: | - apps/desktop/release/*.yml - !apps/desktop/release/builder-debug.yml + path: apps/desktop/release/latest.yml retention-days: ${{ inputs.artifact_retention_days }} if-no-files-found: error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-desktop.yml around lines 333 - 341, The Windows artifact upload step ("Upload Windows auto-update manifest") currently uses a broad glob (apps/desktop/release/*.yml) which can pick up unrelated electron-builder outputs; change the with.path to target the exact auto-update manifest confirmed earlier (apps/desktop/release/latest.yml) instead of the glob, remove the unnecessary exclusion of builder-debug.yml, and keep the existing with.name (using inputs.artifact_prefix) and with.retention-days (inputs.artifact_retention_days) and if-no-files-found behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/build-desktop.yml:
- Around line 266-280: The step interpolates `${{ inputs.version_suffix }}`
directly into the bash script which can allow shell-injection if untrusted;
change the approach to avoid direct interpolation by passing the suffix via a
safe environment variable or by properly quoting/sanitizing it before use: e.g.,
set an env var like VERSION_SUFFIX from the workflow input and use bash variable
expansion (or printf %q) to build NEW_VERSION, or have the Node snippet read
process.env.VERSION_SUFFIX instead of embedding '$NEW_VERSION' into the inline
script (update references to CURRENT_VERSION, NEW_VERSION, and the inline node
-e writer accordingly) so arbitrary characters cannot break out of the command
string.
- Around line 333-341: The Windows artifact upload step ("Upload Windows
auto-update manifest") currently uses a broad glob (apps/desktop/release/*.yml)
which can pick up unrelated electron-builder outputs; change the with.path to
target the exact auto-update manifest confirmed earlier
(apps/desktop/release/latest.yml) instead of the glob, remove the unnecessary
exclusion of builder-debug.yml, and keep the existing with.name (using
inputs.artifact_prefix) and with.retention-days (inputs.artifact_retention_days)
and if-no-files-found behavior unchanged.
In `@apps/desktop/src/lib/electron-app/factories/app/setup.ts`:
- Around line 73-76: Replace the terse logical-and side-effect with an explicit
conditional: check PLATFORM.IS_LINUX || PLATFORM.IS_WINDOWS in an if statement
and call app.disableHardwareAcceleration() inside its block (referencing
PLATFORM.IS_LINUX, PLATFORM.IS_WINDOWS and app.disableHardwareAcceleration) so
the intent is clear and readable.
In `@apps/desktop/src/main/windows/main.ts`:
- Around line 225-229: The renderer console handler in
window.webContents.on("console-message") currently routes all messages through
console.log; update the handler to preserve severity by switching on the numeric
level (0: verbose, 1: info, 2: warning, 3: error) and call console.log for
levels 0–1, console.warn for level 2, and console.error for level 3, keeping the
existing levelStr and source formatting logic so the message string construction
(levelStr, message, source) is reused but emitted with the appropriate console
method.
In `@apps/desktop/vite/helpers.ts`:
- Around line 23-25: The current defineEnv helper can return the JS value
undefined (via JSON.stringify(undefined)) when both value is "" or undefined and
fallback is omitted; update defineEnv in helpers.ts so it always returns a
JSON-stringified string: choose value when it is a non-empty string, otherwise
use fallback if provided, and if neither is present fall back to the empty
string, then JSON.stringify that resolved string before returning. Ensure you
reference the defineEnv function and replace the current return logic so the
function's declared string return type is always satisfied.
In `@scripts/postinstall.mjs`:
- Around line 22-28: The run function currently passes env: { ...process.env,
SUPERSET_POSTINSTALL_RUNNING: "1" } which redundantly duplicates the env var
already set on process.env; update the run(cmd) implementation (the run
function) to remove the spread and either omit the env option entirely (let
child inherit process.env) or pass env: process.env so it relies on the existing
SUPERSET_POSTINSTALL_RUNNING value instead of recreating it.
|
updates? |
|
Heya, I'll circle back in a day or two to get to the comments, in the mean time I wish I could get some feedback from the devs to see if they'd like to include Windows support. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/electron.vite.config.ts (1)
18-18: Regex instripCrossOriginPluginis fragile for valuedcrossoriginattributesThe implementation at
apps/desktop/vite/helpers.tslines 77–80 uses:html.replace(/ crossorigin/g, "")This is safe for Vite's current output, which injects
crossoriginas a boolean attribute on<script type="module">and<link rel="modulepreload">tags. However, if the HTML template (or any plugin) ever emits a valued form such ascrossorigin="anonymous", the replacement leaves a dangling="anonymous"token and produces malformed HTML.Consider tightening the regex to also consume any optional value:
♻️ Suggested fix in
apps/desktop/vite/helpers.ts- return html.replace(/ crossorigin/g, ""); + return html.replace(/ crossorigin(?:="[^"]*")?/g, "");Also applies to: 220-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/electron.vite.config.ts` at line 18, stripCrossOriginPlugin currently removes only the literal " crossorigin" token which breaks when an attribute has a value (e.g. crossorigin="anonymous"); update the plugin's HTML replacement to remove the entire attribute including any optional value and its preceding whitespace by using a regex that matches the attribute plus optional =value (supporting double-quoted, single-quoted, or unquoted values) and replace that whole match, and apply the same fix to the other occurrence of the same replacement in the helper so both places remove the full crossorigin attribute.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/electron.vite.config.ts`:
- Line 18: stripCrossOriginPlugin currently removes only the literal "
crossorigin" token which breaks when an attribute has a value (e.g.
crossorigin="anonymous"); update the plugin's HTML replacement to remove the
entire attribute including any optional value and its preceding whitespace by
using a regex that matches the attribute plus optional =value (supporting
double-quoted, single-quoted, or unquoted values) and replace that whole match,
and apply the same fix to the other occurrence of the same replacement in the
helper so both places remove the full crossorigin attribute.
- Delete orphaned scripts/postinstall.sh (replaced by postinstall.mjs) - Filter console-message forwarder to warnings/errors only - Use console.warn/console.error to preserve log severity - Simplify postinstall.mjs run() by removing redundant env spread - Make defineEnv return type-safe when both args are undefined - Tighten Windows manifest upload glob to latest.yml
|
Addressed the review feedback in e0071d4. Here's a summary of what was changed and what was intentionally skipped: Changes made
Intentionally skipped
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.github/workflows/build-desktop.yml (3)
305-308: Windows installer will be unsigned — SmartScreen warning on every install.Unlike the macOS job, which passes code-signing credentials via
CSC_LINK/CSC_KEY_PASSWORD, this step has no signing env vars. electron-builder supports Windows code signing viaWIN_CSC_LINK/WIN_CSC_KEY_PASSWORD(PFX certificate) or theWINDOWS_CERTIFICATE_THUMBPRINT+ Azure Trusted Signing approach. Without signing, every user who installs the.exewill encounter a Windows Defender SmartScreen "Unknown Publisher" dialog.If signing is intentionally deferred, document it and consider adding a
#TODO: add WIN_CSC_LINKcomment. If secrets already exist (e.g., re-using the same PFX as the macOSCSC_LINK), they can be wired in directly:🔑 Proposed env block for Windows signing
- name: Build Electron app shell: bash working-directory: apps/desktop + env: + WIN_CSC_LINK: ${{ secrets.WIN_CERTIFICATE }} + WIN_CSC_KEY_PASSWORD: ${{ secrets.WIN_CERTIFICATE_PASSWORD }} run: bun run package -- --publish never --config ${{ inputs.electron_builder_config }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-desktop.yml around lines 305 - 308, The Windows build step "Build Electron app" currently omits code-signing env vars, causing unsigned installers and SmartScreen warnings; fix by adding Windows signing environment variables to that step (e.g., set WIN_CSC_LINK and WIN_CSC_KEY_PASSWORD for a PFX or set WINDOWS_CERTIFICATE_THUMBPRINT for Azure Trusted Signing) or, if signing is intentionally deferred, add a clear TODO comment and documentation in the step indicating signing is not configured; update the workflow step that runs "bun run package" in the Build Electron app job to export the chosen env vars (or a TODO) so electron-builder can sign the .exe.
239-339: High duplication across all three build jobs — consider a reusable composite action.The new Windows job replicates the version-suffix mutation step, the 10-variable
compile:appenv block, and the build/verify/upload structure almost verbatim from the Linux job. With all three jobs now aligned, this is a good time to consider extracting the shared logic into a composite action (e.g.,.github/actions/build-desktop-platform/action.yml) parameterised byos,arch, and platform-specific signing secrets. This would reduce the workflow from ~340 lines to roughly ~100 lines and ensure future env-var additions (likeNEXT_PUBLIC_ELECTRIC_URLin this PR) don't need to be applied in three places.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-desktop.yml around lines 239 - 339, The Windows job duplicates the same version mutation, compile env block, build/verify/upload steps found in the other platform jobs; refactor by creating a reusable composite action (e.g., .github/actions/build-desktop-platform/action.yml) that encapsulates the steps "Set version suffix", "Compile app with electron-vite", "Build Electron app", "Verify Windows installer + update manifest", "Upload EXE artifact", and "Upload Windows auto-update manifest"; parameterize the action with inputs for version_suffix, electron_builder_config, artifact_prefix, artifact_retention_days, os/arch, and any platform-specific signing secrets or env vars (like NEXT_PUBLIC_* and SENTRY_*), then replace the platform-specific jobs (including build-windows) to call the composite action with appropriate inputs to remove duplication.
253-260: Cache key based ongithub.shais ineffective — applies to all three jobs.The primary key
${{ runner.os }}-bun-${{ github.sha }}changes on every commit, so it almost never produces an exact hit. Every run falls back to therestore-keysprefix, writes a new cache entry, and accumulates stale entries over time. A lockfile-based key would only invalidate when dependencies actually change.This pattern is shared by all three build jobs; swapping the key in one place fixes all of them:
♻️ Proposed cache key fix (apply to macOS, Linux, and Windows jobs)
- key: ${{ runner.os }}-bun-${{ github.sha }} + key: ${{ runner.os }}-bun-${{ hashFiles('**/bun.lockb') }} restore-keys: | ${{ runner.os }}-bun-🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-desktop.yml around lines 253 - 260, The cache step "Cache dependencies" currently uses the per-commit key `${{ runner.os }}-bun-${{ github.sha }}` which invalidates the cache every commit; replace that key with a lockfile-based key (e.g., hash of bun.lockb or your package lockfile) so the cache only changes when dependencies change (update the value used in the `key` field in the "Cache dependencies" step and do the same change for the other two build jobs), and keep a broad `restore-keys` prefix like `${{ runner.os }}-bun-` so partial restores still work.apps/desktop/vite/helpers.ts (1)
73-83: Consider a more defensive regex for future Vite output compatibility.Vite's standard production build emits
crossoriginas a valueless boolean attribute (e.g.,<script type="module" crossorigin src="...">and<link rel="modulepreload" crossorigin href="...">), so the current regex works correctly for all known Vite output. However, if a future Vite version emitscrossorigin="anonymous"(which is semantically equivalent), the regex would leave an orphaned="anonymous"token, producing invalid HTML.♻️ Proposed defensive regex
- return html.replace(/ crossorigin/g, ""); + return html.replace(/ crossorigin(?:="[^"]*")?/g, "");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/vite/helpers.ts` around lines 73 - 83, The current stripCrossOriginPlugin.handler uses html.replace(/ crossorigin/g, "") which will leave orphaned =... tokens if Vite emits crossorigin="anonymous"; update the regex in stripCrossOriginPlugin (inside transformIndexHtml.handler) to defensively remove the attribute and any optional value and surrounding whitespace (use a pattern that matches optional =value with single/double/unquoted values and use the global flag) so it strips both valueless crossorigin and crossorigin="..." occurrences without leaving stray tokens.
🤖 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/vite/helpers.ts`:
- Around line 22-27: The defineEnv helper now returns string | undefined but
callers assign it directly into Vite's define object, causing undefined entries
to violate Vite's Record<string,string>; update the call sites that use
defineEnv (e.g., the entries for "process.env.SENTRY_DSN_DESKTOP",
"process.env.NEXT_PUBLIC_POSTHOG_KEY", "process.env.DESKTOP_VITE_PORT") to
either supply a fallback when calling defineEnv or filter out undefined values
before merging into define (for example compute const val = defineEnv(...); then
spread ...(val !== undefined && { "process.env.X": val }) so only string values
are included).
---
Nitpick comments:
In @.github/workflows/build-desktop.yml:
- Around line 305-308: The Windows build step "Build Electron app" currently
omits code-signing env vars, causing unsigned installers and SmartScreen
warnings; fix by adding Windows signing environment variables to that step
(e.g., set WIN_CSC_LINK and WIN_CSC_KEY_PASSWORD for a PFX or set
WINDOWS_CERTIFICATE_THUMBPRINT for Azure Trusted Signing) or, if signing is
intentionally deferred, add a clear TODO comment and documentation in the step
indicating signing is not configured; update the workflow step that runs "bun
run package" in the Build Electron app job to export the chosen env vars (or a
TODO) so electron-builder can sign the .exe.
- Around line 239-339: The Windows job duplicates the same version mutation,
compile env block, build/verify/upload steps found in the other platform jobs;
refactor by creating a reusable composite action (e.g.,
.github/actions/build-desktop-platform/action.yml) that encapsulates the steps
"Set version suffix", "Compile app with electron-vite", "Build Electron app",
"Verify Windows installer + update manifest", "Upload EXE artifact", and "Upload
Windows auto-update manifest"; parameterize the action with inputs for
version_suffix, electron_builder_config, artifact_prefix,
artifact_retention_days, os/arch, and any platform-specific signing secrets or
env vars (like NEXT_PUBLIC_* and SENTRY_*), then replace the platform-specific
jobs (including build-windows) to call the composite action with appropriate
inputs to remove duplication.
- Around line 253-260: The cache step "Cache dependencies" currently uses the
per-commit key `${{ runner.os }}-bun-${{ github.sha }}` which invalidates the
cache every commit; replace that key with a lockfile-based key (e.g., hash of
bun.lockb or your package lockfile) so the cache only changes when dependencies
change (update the value used in the `key` field in the "Cache dependencies"
step and do the same change for the other two build jobs), and keep a broad
`restore-keys` prefix like `${{ runner.os }}-bun-` so partial restores still
work.
In `@apps/desktop/vite/helpers.ts`:
- Around line 73-83: The current stripCrossOriginPlugin.handler uses
html.replace(/ crossorigin/g, "") which will leave orphaned =... tokens if Vite
emits crossorigin="anonymous"; update the regex in stripCrossOriginPlugin
(inside transformIndexHtml.handler) to defensively remove the attribute and any
optional value and surrounding whitespace (use a pattern that matches optional
=value with single/double/unquoted values and use the global flag) so it strips
both valueless crossorigin and crossorigin="..." occurrences without leaving
stray tokens.
| ): string | undefined { | ||
| // Use || instead of ?? so empty strings (from unresolved GitHub Actions | ||
| // secrets) also fall through to the default value. | ||
| const resolved = value || fallback; | ||
| return resolved !== undefined ? JSON.stringify(resolved) : undefined; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=ts -B2 -A2 'defineEnv\s*\(' --glob='**/*.ts' --glob='**/*.tsx'Repository: superset-sh/superset
Length of output: 7752
🏁 Script executed:
head -100 apps/desktop/electron.vite.config.tsRepository: superset-sh/superset
Length of output: 3432
🏁 Script executed:
cat -n apps/desktop/tsconfig.jsonRepository: superset-sh/superset
Length of output: 646
🏁 Script executed:
find . -path "*typescript/electron.json" -o -path "*typescript/electron*" | head -20Repository: superset-sh/superset
Length of output: 98
🏁 Script executed:
cat -n ./tooling/typescript/electron.jsonRepository: superset-sh/superset
Length of output: 524
🏁 Script executed:
cat -n ./tooling/typescript/base.jsonRepository: superset-sh/superset
Length of output: 867
🏁 Script executed:
# Check if there's a build configuration or CI that would compile TypeScript
find . -name "package.json" -path "*/apps/desktop/*" | xargs grep -l "typescript\|tsc" 2>/dev/null | head -5Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
cat -n apps/desktop/package.json | head -50Repository: superset-sh/superset
Length of output: 2459
🏁 Script executed:
# Check the actual Vite/electron-vite type definitions for define property
# First, let's see what version of electron-vite is being used
cat apps/desktop/package.json | grep -A 2 -B 2 "electron-vite"Repository: superset-sh/superset
Length of output: 880
🏁 Script executed:
# Let's check if there are any type assertions or workarounds in the electron.vite.config.ts file
# Get the full file to see if there are any type casts
wc -l apps/desktop/electron.vite.config.tsRepository: superset-sh/superset
Length of output: 104
🏁 Script executed:
tail -160 apps/desktop/electron.vite.config.tsRepository: superset-sh/superset
Length of output: 4213
🏁 Script executed:
# Let's check the actual type definitions for electron-vite's define config
# by looking at node_modules or checking what types are expected
find . -path "*node_modules/electron-vite*" -name "index.d.ts" 2>/dev/null | head -1Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Let's look at the defineEnv function implementation in detail
sed -n '19,27p' apps/desktop/vite/helpers.tsRepository: superset-sh/superset
Length of output: 399
🏁 Script executed:
# Let's try to run typecheck on the desktop app to see if there are actual TypeScript errors
cd apps/desktop && npm run typecheck 2>&1 | head -100Repository: superset-sh/superset
Length of output: 530
🏁 Script executed:
# Check if there are any type assertions or casts in the config
grep -n "as\|as const\|@ts-" apps/desktop/electron.vite.config.tsRepository: superset-sh/superset
Length of output: 192
🏁 Script executed:
# Let's check what Vite's type definition for define config actually accepts
# by searching for how it's typed in node_modules (if available)
find . -path "*node_modules/vite*" -name "*.d.ts" | xargs grep -l "define" | head -3Repository: superset-sh/superset
Length of output: 46
Update defineEnv return type handling—callers directly assign string | undefined to Vite's define config without filtering undefined.
The return type changed from string to string | undefined, but the call sites in electron.vite.config.ts assign the result directly to the define object. Many calls lack a fallback, meaning when environment variables are unset, defineEnv returns undefined. This breaks Vite's define type contract, which expects Record<string, string>.
Examples without fallback:
"process.env.SENTRY_DSN_DESKTOP": defineEnv(process.env.SENTRY_DSN_DESKTOP)(line 70)"process.env.NEXT_PUBLIC_POSTHOG_KEY": defineEnv(process.env.NEXT_PUBLIC_POSTHOG_KEY)(line 74)"process.env.DESKTOP_VITE_PORT": defineEnv(process.env.DESKTOP_VITE_PORT)(line 84)
Filter out undefined values at the call sites: ...(val !== undefined && { KEY: val }) or provide fallbacks for all calls.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/vite/helpers.ts` around lines 22 - 27, The defineEnv helper now
returns string | undefined but callers assign it directly into Vite's define
object, causing undefined entries to violate Vite's Record<string,string>;
update the call sites that use defineEnv (e.g., the entries for
"process.env.SENTRY_DSN_DESKTOP", "process.env.NEXT_PUBLIC_POSTHOG_KEY",
"process.env.DESKTOP_VITE_PORT") to either supply a fallback when calling
defineEnv or filter out undefined values before merging into define (for example
compute const val = defineEnv(...); then spread ...(val !== undefined && {
"process.env.X": val }) so only string values are included).
AviPeltz
left a comment
AviPeltz
left a comment
There was a problem hiding this comment.
Thanks for the contribution @Sandman-Ren — appreciate the effort to get Windows builds working. However, Windows isn't currently a priority for us, and our primary concern is that none of these changes jeopardize the macOS or Linux builds. Several changes in this PR touch shared code paths that run on all platforms, and we'd need those tightened up before considering a merge.
Here's a detailed review:
1. defineEnv: ?? → || and return type change (vite/helpers.ts)
This is the highest-risk change in the PR. It affects every env var substitution across all platform builds.
- Changing
??to||means empty strings ("") now fall through to the fallback. While the intent is to handle unresolved GitHub Actions secrets, this is a semantic change on macOS and Linux too. If any env var is intentionally set to""in our CI or local dev, it would now silently be replaced by the fallback value. - The return type changing from
stringtostring | undefinedis technically more correct (the old code could already returnundefinedfromJSON.stringify(undefined)), but this is still a behavioral change.
Suggestion: If this is only needed for the Windows CI runner, guard it there (e.g., set explicit defaults in the workflow env block) rather than changing the shared utility. Or at minimum, only apply the || fallback for specific variables known to be problematic, not globally.
2. stripCrossOriginPlugin runs on ALL platforms
This strips crossorigin from every <script> and <link> tag in the renderer HTML. While ASAR file:// on Windows may need this, the plugin is unconditionally applied on macOS and Linux too.
- On macOS, where the build is stable and working, removing
crossorigincould subtly change script loading behavior (e.g., error reporting, CORS handling). - The regex
/ crossorigin/gis also fragile — it would matchcrossoriginanywhere in the HTML, including inside attribute values or comments.
Suggestion: Either make this plugin Windows-only (if (process.platform !== 'win32') return html;), or explain why it's safe/necessary on macOS and Linux. If it doesn't cause issues on Mac, that's fine — but it needs to be validated, not assumed.
3. postinstall.sh → postinstall.mjs
This replaces the bash postinstall script with a Node.js equivalent. This runs on every bun install across all platforms.
- The logic looks equivalent, but
process.env.SUPERSET_POSTINSTALL_RUNNING = "1"in Node.js sets it on the current process's env, which is inherited byexecSyncchildren. This should work, but it's a change to a critical install-time script. - Deleting
postinstall.shmeans there's no fallback if the.mjsscript has issues.
Concern: This needs thorough testing on macOS before merge. We run bun install constantly, and a broken postinstall would block all development.
4. rmSync change in copy-native-modules.ts
Changing rmSync(modulePath) to rmSync(modulePath, { recursive: true, force: true }) to handle Windows junctions.
- On macOS/Linux,
rmSyncon a symlink withoutrecursivejust removes the symlink. With{ recursive: true }, it should still just remove the symlink (not follow it), but this is a change to behavior that's been stable. - The
force: trueflag suppresses errors if the path doesn't exist, which could mask real issues.
Suggestion: Consider guarding the { recursive: true, force: true } options behind a process.platform === 'win32' check, since Windows junctions are the only reason for the change.
5. Console message forwarding (main.ts)
Adding console-message listener and extra URL logging runs on all platforms. Low risk but adds noise to the main process stdout on Mac/Linux.
Suggestion: Gate this behind an env var or --verbose flag, or at minimum a process.platform === 'win32' check if it's only needed for Windows debugging.
6. CI workflow changes
The new build-windows job in build-desktop.yml is additive and doesn't affect Mac/Linux jobs — this part is fine. Adding NEXT_PUBLIC_ELECTRIC_URL to the existing Mac build env is also fine (looks like it was missing).
The release workflow changes (release-desktop.yml, release-desktop-canary.yml) for .exe artifact handling look correct and are guarded by file existence checks.
Summary
The CI/workflow changes are fine, but the shared code changes (defineEnv, stripCrossOriginPlugin, postinstall.mjs, rmSync options, console logging) all affect macOS and Linux builds without platform guards. We'd need either:
- Platform guards on Windows-specific fixes so Mac/Linux paths are untouched, or
- Proof that the Mac build is unaffected (e.g., a successful macOS CI run with these changes)
We're happy to reconsider once those concerns are addressed. Thanks again for the initiative!
|
Hi @AviPeltz, thanks for the detailed review and carefully pointing out the issues in my PR. I understand that Windows builds are not priority at the moment. I'd be happy to contribute to the Windows build effort in a way that does not tamper with the MacOS/Linux builds. I'll spend some time with your comments and make changes accordingly. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/vite/helpers.ts (1)
19-24:defineEnvreturn type mismatch — already tracked.
JSON.stringify(value ?? fallback)returns the JSundefinedvalue (not the string) when bothvalueandfallbackareundefined, violating the declared: stringreturn type and breaking Vite'sdefinecontract at the affected call sites.— this concern is covered by the existing review comment on this function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/vite/helpers.ts` around lines 19 - 24, The function defineEnv currently returns JSON.stringify(value ?? fallback) which yields the JS undefined (not a string) when both are undefined; update defineEnv to always produce a string by using a final default (e.g., JSON.stringify(value ?? fallback ?? null)) so it returns a valid string literal (like "null") instead of raw undefined; modify the implementation of defineEnv to use that ternary/default expression and keep the declared : string return type.
🤖 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/vite/helpers.ts`:
- Around line 71-82: The current stripCrossOriginPlugin uses process.platform
inside transformIndexHtml (handler) which is evaluated at build time and can be
wrong for cross-compilation; change the check to use an explicit build-target
env var (e.g., process.env.TARGET_PLATFORM or VITE_TARGET_PLATFORM) instead of
process.platform in the stripCrossOriginPlugin/transformIndexHtml handler, and
document the intent in a comment so CI can pass TARGET_PLATFORM=win32 for
Windows artifacts; ensure the handler falls back safely (e.g., treat undefined
as non-Windows) so behavior is deterministic and not tied to the build host.
---
Duplicate comments:
In `@apps/desktop/vite/helpers.ts`:
- Around line 19-24: The function defineEnv currently returns
JSON.stringify(value ?? fallback) which yields the JS undefined (not a string)
when both are undefined; update defineEnv to always produce a string by using a
final default (e.g., JSON.stringify(value ?? fallback ?? null)) so it returns a
valid string literal (like "null") instead of raw undefined; modify the
implementation of defineEnv to use that ternary/default expression and keep the
declared : string return type.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/scripts/copy-native-modules.tsapps/desktop/src/main/windows/main.tsapps/desktop/vite/helpers.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/main/windows/main.ts
- apps/desktop/scripts/copy-native-modules.ts
| export function stripCrossOriginPlugin(): Plugin { | ||
| return { | ||
| name: "strip-crossorigin", | ||
| transformIndexHtml: { | ||
| order: "post", | ||
| handler(html) { | ||
| if (process.platform !== "win32") return html; | ||
| return html.replace(/ crossorigin(?:="[^"]*")?/g, ""); | ||
| }, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
process.platform is evaluated at build time — cross-compilation silently skips the fix.
process.platform !== "win32" runs in the Vite/Node.js process during the build, not inside the packaged Electron app. This works correctly when the CI runner is windows-latest, but if a Windows artifact is ever produced from a macOS or Linux host (cross-compilation), the crossorigin stripping will be silently skipped, causing the black-screen regression to reappear with no indication.
Consider asserting the intent in a comment, or pass the target platform explicitly via an env var so the build graph doesn't couple correctness to the build host:
💡 Proposed hardening
export function stripCrossOriginPlugin(): Plugin {
return {
name: "strip-crossorigin",
transformIndexHtml: {
order: "post",
handler(html) {
- if (process.platform !== "win32") return html;
+ // NOTE: evaluated at Vite build time, not Electron runtime.
+ // For cross-compilation, set TARGET_PLATFORM=win32 explicitly.
+ const platform = process.env.TARGET_PLATFORM ?? process.platform;
+ if (platform !== "win32") return html;
return html.replace(/ crossorigin(?:="[^"]*")?/g, "");
},
},
};
}The CI workflow would then pass TARGET_PLATFORM=win32 when invoking the Windows build step, making the condition explicit and immune to the build-host assumption.
📝 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.
| export function stripCrossOriginPlugin(): Plugin { | |
| return { | |
| name: "strip-crossorigin", | |
| transformIndexHtml: { | |
| order: "post", | |
| handler(html) { | |
| if (process.platform !== "win32") return html; | |
| return html.replace(/ crossorigin(?:="[^"]*")?/g, ""); | |
| }, | |
| }, | |
| }; | |
| } | |
| export function stripCrossOriginPlugin(): Plugin { | |
| return { | |
| name: "strip-crossorigin", | |
| transformIndexHtml: { | |
| order: "post", | |
| handler(html) { | |
| // NOTE: evaluated at Vite build time, not Electron runtime. | |
| // For cross-compilation, set TARGET_PLATFORM=win32 explicitly. | |
| const platform = process.env.TARGET_PLATFORM ?? process.platform; | |
| if (platform !== "win32") return html; | |
| return html.replace(/ crossorigin(?:="[^"]*")?/g, ""); | |
| }, | |
| }, | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/vite/helpers.ts` around lines 71 - 82, The current
stripCrossOriginPlugin uses process.platform inside transformIndexHtml (handler)
which is evaluated at build time and can be wrong for cross-compilation; change
the check to use an explicit build-target env var (e.g.,
process.env.TARGET_PLATFORM or VITE_TARGET_PLATFORM) instead of process.platform
in the stripCrossOriginPlugin/transformIndexHtml handler, and document the
intent in a comment so CI can pass TARGET_PLATFORM=win32 for Windows artifacts;
ensure the handler falls back safely (e.g., treat undefined as non-Windows) so
behavior is deterministic and not tied to the build host.
Address PR review feedback by ensuring shared code paths are untouched
on macOS and Linux:
- Revert defineEnv back to ?? (|| was only needed for fork CI secrets)
- Gate stripCrossOriginPlugin to Windows only, tighten regex
- Gate rmSync({ recursive }) to Windows only, drop force flag
- Gate renderer console-message forwarding to Windows only
- Remove extra URL log line from did-finish-load
- Restore postinstall.sh alongside postinstall.mjs as fallback
Sandman-Ren
force-pushed
the
feat/windows-desktop-build
branch
from
23b1ea1 to
bf257f4
Compare
|
Thanks again for the detailed review @AviPeltz. I've made changes in response to your comments. Notably my changes in 1. was supposed to be a temporary workaround for not having some repository secrets while trying to replicate your CI GitHub Actions workflow in my fork repository for build validation. I have reverted 1. to its original state. I've added Windows gates for For 3. I've restored the See below for details of the changes made. Let me know once you and the team get a chance to review :) Changes made1. 2. 3. 4. 5. Console message forwarding: gated to Windows only 6. CI/workflow changes: additive only. Also added CI evidenceFork CI (lint, typecheck, test, build on Linux) — all passing:
Fork canary build — latest commit (
The macOS Vite compile passes all three stages, confirming all shared code paths ( Every shared code path is now either identical to |
Windows has a 260-character MAX_PATH limit. Some upstream file paths (e.g. deeply nested component directories) exceed this, causing `git checkout` to fail on Windows runners. Setting core.longPaths before checkout resolves this.
|
just wanted to give this a soft nudge and see if we can get any reviews. Many thanks for the time :) |
|
Hey sorry that got into the cleaning loop. I don't want to dismiss the good work done here but we're just not able to support or maintain a Windows version. I'd be happy to give a windows fork our proverbial blessing and eventually converge back when we have more resources to properly test and give a good windows experience |
|
No worries. Happy to keep a windows fork since I use this for my own dev work. Looking forward to getting back and work on windows support again in the future. Cheers! |
Hey Superset team,
I develop natively on Windows and would like to have a Windows installable for the purpose. I spent some time with Claude Code today and worked out a working installable on Windows and added a Windows build pipeline to the Actions workflows.
I'd be really happy to have Windows support for Superset. Please take a look at the PR and leave any feedback and I'd be happy to make changes as needed.
System Information
Summary
build-windowsjob to the desktop CI/CD pipeline (build-desktop.yml) targetingwindows-latestwith NSIS installer outputrelease-desktop.yml,release-desktop-canary.yml) to handle Windows.exeartifacts and auto-update manifestspostinstall.shwith cross-platformscripts/postinstall.mjscopy-native-modules.ts(rmSyncwith{ recursive: true, force: true })latest.yml, not*-win.yml)crossoriginattribute from script/link tags (incompatible with Electron's ASARfile://handler)||instead of??indefineEnvso empty strings from unresolved CI secrets fall back to defaultsTest plan
windows-latestCI runner.exeinstaller produced and installs correctlySummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores