Conversation
WalkthroughCentralizes and replaces the Electron Builder config, simplifies the Electron Vite config to a function-driven plugin setup, moves some dependencies into runtime, adds packaging npm scripts, and enables hoisting via .npmrc. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Vite as Vite (serve/build)
participant EPlugin as vite-plugin-electron/simple
participant Builder as electron-builder
participant Verify as postbuild verify:node-pty
Dev->>Vite: run dev or build
Vite->>EPlugin: build main & preload -> emit to dist-electron/...
EPlugin->>Vite: renderer assets
Dev->>Builder: run npm script `package` -> electron-builder with config
Builder->>Builder: package using config (asar, files, extraResources, mac/win/linux targets)
Builder->>Builder: sign artifacts (if configured)
Builder->>Verify: run postbuild verification
Verify-->>Dev: success / failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🪛 GitHub Actions: CIapps/desktop/package.json[error] 1-1: sherif: dependencies should be ordered alphabetically. unordered-dependencies 🔇 Additional comments (2)
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: 2
🧹 Nitpick comments (1)
apps/desktop/electron-builder.ts (1)
27-31: Refine the asarUnpack pattern to match the actual target path.The pattern
**/node_modules/node-pty/**/*is overly broad. Since the file mapping (line 44) places node-pty atnode_modules/node-pty, the asarUnpack pattern should benode_modules/node-pty/**/*to precisely match that location.Apply this diff:
- asarUnpack: [ - "**/node_modules/node-pty/**/*", - ], + asarUnpack: [ + "node_modules/node-pty/**/*", + ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/electron-builder.ts(2 hunks)apps/desktop/package.json(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
apps/desktop/package.json
[error] sherif: dependencies should be ordered alphabetically. unordered-dependencies
🔇 Additional comments (3)
apps/desktop/electron-builder.ts (2)
41-46: LGTM!The file copy mapping correctly includes node-pty from the monorepo root, which is necessary for the native module to function properly in the packaged app.
50-50: LGTM!The updated comment clearly explains why npmRebuild is disabled in this monorepo setup.
apps/desktop/package.json (1)
19-19: LGTM!The postbuild script appropriately verifies that node-pty was unpacked correctly after the build completes.
| "zustand": "^5.0.8", | ||
| "open": "^10.2.0", | ||
| "@superset/ui": "workspace:*" |
There was a problem hiding this comment.
Fix the alphabetical ordering of dependencies.
The pipeline failure indicates that dependencies must be ordered alphabetically. Currently, open and @superset/ui are placed at the end, but they need to be inserted in alphabetical order.
Apply this diff to fix the ordering:
"@types/express": "^5.0.5",
+ "@superset/ui": "workspace:*",
"@xterm/addon-clipboard": "^0.1.0", "node-pty": "1.1.0-beta30",
+ "open": "^10.2.0",
"react": "^19.1.1",- "zustand": "^5.0.8",
- "open": "^10.2.0",
- "@superset/ui": "workspace:*"
+ "zustand": "^5.0.8"Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: CI
[error] sherif: dependencies should be ordered alphabetically. unordered-dependencies
🤖 Prompt for AI Agents
In apps/desktop/package.json around lines 81 to 83 the dependencies are not
alphabetically ordered: "open" and "@superset/ui" are at the end but must be
inserted in alphabetical order among the other dependencies; reorder the
dependency entries so keys are sorted lexicographically (e.g., place
"@superset/ui" where entries starting with "@" belong and place "open" in its
correct alphabetical position), update the JSON accordingly and run the
dependency/order linter or package manager check to confirm the pipeline passes.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/desktop/package.json (2)
19-21: Makeverify:node-ptyplatform-aware and align it with the new output layoutThe inline
node -escript:
- Hardcodes a macOS-only path (
release/mac-universal/...), so it will fail on Linux/Windows CI.- Ignores the versioned output directory now configured in
electron-builder.ts(directories.output: 'release/${version}'), so even mac builds are likely checked at the wrong path.- Is hard to maintain in a single long one-liner.
Consider moving this into a small script file that detects the platform and builds the expected unpacked path (including the version) before checking for
node-pty, then invoke it from the script:- "postbuild": "bun run verify:node-pty", - "verify:node-pty": "node -e \"const fs=require('fs');const path=require('path');const unpackedPath=path.join('release','mac-universal','Superset.app','Contents','Resources','app.asar.unpacked','node_modules','node-pty');const exists=fs.existsSync(unpackedPath);console.log(exists?'✅ node-pty unpacked successfully at: '+unpackedPath:'❌ node-pty missing in unpacked! Expected at: '+unpackedPath);process.exit(exists?0:1);\"", + "postbuild": "bun run verify:node-pty", + "verify:node-pty": "node scripts/verify-node-pty.cjs",and in
scripts/verify-node-pty.cjs(outline only):const fs = require('fs'); const path = require('path'); const os = require('os'); const pkg = require('../package.json'); const base = path.join('release', pkg.version); const platform = os.platform(); let unpackedPath; if (platform === 'darwin') { unpackedPath = path.join(base, 'mac-universal', 'Superset.app', 'Contents', 'Resources', 'app.asar.unpacked', 'node_modules', 'node-pty'); } else if (platform === 'linux') { unpackedPath = path.join(base, 'linux-unpacked', 'resources', 'app.asar.unpacked', 'node_modules', 'node-pty'); } else if (platform === 'win32') { unpackedPath = path.join(base, 'win-unpacked', 'resources', 'app.asar.unpacked', 'node_modules', 'node-pty'); } else { console.log(`⚠️ Unknown platform: ${platform}, skipping verification`); process.exit(0); } const exists = fs.existsSync(unpackedPath); console.log( exists ? `✅ node-pty unpacked successfully at: ${unpackedPath}` : `❌ node-pty missing in unpacked! Expected at: ${unpackedPath}`, ); process.exit(exists ? 0 : 1);This keeps the check readable, works across OSes, and tracks the versioned output directory.
82-84: Fix alphabetical ordering of dependencies to satisfy Sherif
dependenciesare still not sorted lexicographically:"open"and"@superset/ui"sit at the end instead of in their proper positions, which matches the Sherifunordered-dependenciesfailure.Reorder them like this:
@@ - "@types/express": "^5.0.5", - "@xterm/addon-clipboard": "^0.1.0", + "@types/express": "^5.0.5", + "@superset/ui": "workspace:*", + "@xterm/addon-clipboard": "^0.1.0", @@ - "nanoid": "^5.1.6", - "node-pty": "1.1.0-beta30", - "react": "^19.1.1", + "nanoid": "^5.1.6", + "node-pty": "1.1.0-beta30", + "open": "^10.2.0", + "react": "^19.1.1", @@ - "tailwind-merge": "^2.6.0", - "trpc-electron": "^0.1.2", - "zod": "^4.1.12", - "zustand": "^5.0.8", - "open": "^10.2.0", - "@superset/ui": "workspace:*" + "tailwind-merge": "^2.6.0", + "trpc-electron": "^0.1.2", + "zod": "^4.1.12", + "zustand": "^5.0.8"That should clear the CI error.
🧹 Nitpick comments (2)
apps/desktop/electron.vite.config.ts (1)
10-23: Consider limiting destructive/copy operations to build commands
rmSync('dist-electron', { recursive: true, force: true })and the recursivecpSync('resources', 'dist-electron/main/resources', ...)run on every invocation of the config, includingserve.This is not incorrect, but you might want to wrap these in
if (command === 'build')to avoid extra filesystem churn during dev and make the intent clearer:export default defineConfig(({ command }) => { const isServe = command === 'serve'; const isBuild = command === 'build'; if (isBuild) { rmSync('dist-electron', { recursive: true, force: true }); try { cpSync('resources', 'dist-electron/main/resources', { recursive: true }); console.log('✓ Resources folder copied to dist-electron'); } catch (err) { console.error('Failed to copy resources folder:', err); } } // ... });Purely a maintainability/clarity tweak.
apps/desktop/electron-builder.ts (1)
53-68: Double-check Linux icon format and path
linux.iconis set to'build/icon.icns', which is typically a macOS icon format. Electron Builder usually expects platform-appropriate icon assets for Linux (often PNGs in various sizes).If you already have a proper cross-platform icon pipeline, this might be intentional, but otherwise consider pointing
linux.iconat a Linux-appropriate asset (e.g., a.pngin your build assets) or to a directory containing them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
apps/desktop/.npmrc(1 hunks)apps/desktop/electron-builder.ts(1 hunks)apps/desktop/electron.vite.config.ts(1 hunks)apps/desktop/package.json(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
apps/desktop/package.json
[error] 1-1: Sherif: dependencies should be ordered alphabetically. unordered-dependencies.
🔇 Additional comments (2)
apps/desktop/.npmrc (1)
1-3: .npmrc hoist setting looks fineThe
shamefully-hoist=trueflag here is consistent with common electron-builder/pnpm workarounds and is scoped to the desktop app only; no issues from this change alone.apps/desktop/package.json (1)
99-99: Electron version is now pinned; confirm that’s intentionalDropping the caret on
"electron": "39.1.2"pins builds to exactly that version, which is good for reproducibility but will block automatic minor/patch upgrades.If that’s the goal (to stay in lockstep with a specific version used by electron-vite/electron-builder), this change is fine; otherwise, consider whether you still want a range.
| server: process.env.VSCODE_DEBUG | ||
| ? (() => { | ||
| const url = new URL(pkg.debug.env.VITE_DEV_SERVER_URL); | ||
| return { | ||
| host: url.hostname, | ||
| port: +url.port, | ||
| }; | ||
| })() | ||
| : undefined, |
There was a problem hiding this comment.
🧩 Analysis chain
Guard access to pkg.debug in the VSCode debug server override
In the server block you do:
const url = new URL(pkg.debug.env.VITE_DEV_SERVER_URL);but apps/desktop/package.json does not define a debug field. If VSCODE_DEBUG is set, this will throw at config evaluation time (Cannot read properties of undefined (reading 'env')), breaking your debug workflow.
Either add the expected debug.env.VITE_DEV_SERVER_URL structure to package.json, or guard this access and/or read from an environment variable instead, for example:
server: process.env.VSCODE_DEBUG
? (() => {
const devUrl =
process.env.VITE_DEV_SERVER_URL ?? pkg.debug?.env?.VITE_DEV_SERVER_URL;
if (!devUrl) {
throw new Error('VITE_DEV_SERVER_URL is not configured for VSCode debug');
}
const url = new URL(devUrl);
return { host: url.hostname, port: Number(url.port) };
})()
: undefined,This keeps the debug override robust even if pkg.debug is missing.
🏁 Script executed:
#!/bin/bash
# Check if debug field exists in package.json
if [ -f "apps/desktop/package.json" ]; then
echo "=== apps/desktop/package.json exists ==="
jq '.debug' "apps/desktop/package.json" 2>/dev/null || echo "No debug field or jq failed"
else
echo "apps/desktop/package.json not found"
fiLength of output: 45
🏁 Script executed:
#!/bin/bash
# Get the full context around lines 90-98 in electron.vite.config.ts
if [ -f "apps/desktop/electron.vite.config.ts" ]; then
echo "=== Full electron.vite.config.ts context (lines 1-110) ==="
head -110 "apps/desktop/electron.vite.config.ts" | tail -50
echo ""
echo "=== Checking for 'pkg' import/declaration ==="
grep -n "import.*pkg\|const pkg\|let pkg" "apps/desktop/electron.vite.config.ts" | head -5
else
echo "File not found"
fiLength of output: 2279
Guard access to pkg.debug in the VSCode debug server override
In the server block (lines 90–98) you access:
const url = new URL(pkg.debug.env.VITE_DEV_SERVER_URL);but apps/desktop/package.json does not define a debug field. When VSCODE_DEBUG is set, this will throw Cannot read properties of null (reading 'env') at config evaluation time, breaking your debug workflow.
Either add the expected debug.env.VITE_DEV_SERVER_URL structure to package.json, or guard this access and/or read from an environment variable instead, for example:
server: process.env.VSCODE_DEBUG
? (() => {
const devUrl =
process.env.VITE_DEV_SERVER_URL ?? pkg.debug?.env?.VITE_DEV_SERVER_URL;
if (!devUrl) {
throw new Error('VITE_DEV_SERVER_URL is not configured for VSCode debug');
}
const url = new URL(devUrl);
return { host: url.hostname, port: Number(url.port) };
})()
: undefined,This keeps the debug override robust even if pkg.debug is missing.
🤖 Prompt for AI Agents
In apps/desktop/electron.vite.config.ts around lines 90–98, the current VSCode
debug server override directly accesses pkg.debug.env.VITE_DEV_SERVER_URL which
can be undefined and will throw; change it to first read
process.env.VITE_DEV_SERVER_URL and fall back to
pkg.debug?.env?.VITE_DEV_SERVER_URL, validate the resulting URL string exists
(throw a clear error if not), then construct a URL from it and return host and
Number(port) to ensure safe parsing and avoid runtime throws when pkg.debug is
absent.
1 participant
Summary by CodeRabbit
Chores
New Features
✏️ Tip: You can customize this high-level summary in your review settings.