fix(desktop): resolve Windows build compatibility issues#694
Conversation
- Fix symlink handling in copy-native-modules script (unlinkSync → rmSync) - Convert electron-builder config from TS to JS to avoid symlink issues - Update package.json build scripts to use new JS config - Disable npmRebuild to eliminate Python dependency on Windows
📝 WalkthroughWalkthroughThis change introduces a new Electron Builder configuration file ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/desktop/electron-builder.js (3)
18-18: Consider handling additional semver prefixes.The current regex only strips
^prefixes. While safe (returns original if no match), it won't handle other semver prefixes like~or>=.♻️ More robust version parsing
- electronVersion: pkg.devDependencies.electron.replace(/^\^/, ""), + electronVersion: pkg.devDependencies.electron.replace(/^[\^~>=<]+/, ""),
92-94: Clarify that npmRebuild is disabled for all platforms.The comment suggests this is Windows-specific, but the setting applies to all platforms. Since native modules are pre-copied via the
copy-native-modulesscript for all platforms, consider updating the comment for clarity.📝 Suggested comment update
- // Rebuild native modules for Electron's Node.js version - // Disabled on Windows - native modules already copied by copy-native-modules script + // Rebuild native modules for Electron's Node.js version + // Disabled - native modules are already copied by copy-native-modules script before building npmRebuild: false,
139-139: Inconsistent artifact naming between platforms.Windows uses JavaScript interpolation (
${pkg.version}) while Linux uses electron-builder template variables (\${version}). Consider aligning both platforms to use electron-builder variables for consistency.♻️ Consistent template variable usage
- artifactName: `${productName}-${pkg.version}-\${arch}.\${ext}`, + artifactName: `superset-\${version}-\${arch}.\${ext}`,This matches the Linux configuration pattern at line 127.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/electron-builder.jsapps/desktop/electron-builder.jsonapps/desktop/package.jsonapps/desktop/scripts/copy-native-modules.ts
🧰 Additional context used
📓 Path-based instructions (4)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/scripts/copy-native-modules.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/scripts/copy-native-modules.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/scripts/copy-native-modules.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/scripts/copy-native-modules.tsapps/desktop/electron-builder.js
🧠 Learnings (5)
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Applied to files:
apps/desktop/scripts/copy-native-modules.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/lib/electron-router-dom.ts : Do not import Node.js modules like node:path or dotenv in electron-router-dom.ts and similar shared files - they run in both main and renderer processes
Applied to files:
apps/desktop/scripts/copy-native-modules.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/lib/*.ts : Never import Node.js modules in shared code like electron-router-dom.ts - it runs in both main and renderer processes
Applied to files:
apps/desktop/scripts/copy-native-modules.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Use Bun as the package manager - never use npm/yarn/pnpm
Applied to files:
apps/desktop/scripts/copy-native-modules.tsapps/desktop/package.json
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use Biome for formatting and linting - run at root level with `bun run lint:fix` or `biome check --write`
Applied to files:
apps/desktop/package.json
🔇 Additional comments (6)
apps/desktop/scripts/copy-native-modules.ts (2)
16-16: LGTM - Import addition supports Windows compatibility.The
unlinkSyncimport is correctly added to support the improved symlink removal logic below.
49-55: Robust symlink removal with appropriate fallback.The try-catch approach correctly prioritizes
unlinkSync(the standard method for symlinks) with a defensive fallback tormSync. Therecursive: falseoption is appropriate since we've already confirmed this is a symlink at line 43.apps/desktop/electron-builder.js (3)
6-11: LGTM - Appropriate CommonJS format for electron-builder configuration.The use of CommonJS (
require/module.exports) is correct for electron-builder config files, and the metadata extraction frompackage.jsonsafely handles different author formats.
55-90: Well-structured native module packaging configuration.The explicit packaging of native modules with matching
asarUnpackpatterns (lines 35-43) ensures proper runtime access. The comments clearly explain the dependency chain (e.g., better-sqlite3 → bindings → file-uri-to-path).
96-146: Platform configurations are well-structured.The macOS (arm64), Linux (AppImage/deb), and Windows (NSIS x64) configurations are appropriate for modern desktop distribution. The NSIS settings allow installation directory customization, improving user experience.
apps/desktop/package.json (1)
24-26: LGTM - Build scripts correctly reference the new JS configuration.The migration from
electron-builder.tstoelectron-builder.jsaligns with the PR's objective to resolve Windows symlink compatibility issues. Bothbuildandpackagescripts consistently reference the new configuration file.
|
Hey — just a heads up, this was closed as part of an automated stale PR cleanup. If you think this was done in error, feel free to reopen it! |
|
Hey — this was closed by an automated cleanup of PRs with major merge conflicts that are 3+ weeks old. If you think this was done incorrectly, please feel free to reopen it. Sorry for any inconvenience! |
2 participants
Fix symlink handling in copy-native-modules script (unlinkSync → rmSync)
Convert electron-builder config from TS to JS to avoid symlink issues
Update package.json build scripts to use new JS config
Disable npmRebuild to eliminate Python dependency on Windows
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
Release Notes
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.