Adjust build script to work with isolated node modules (bun 1.3)#172
Adjust build script to work with isolated node modules (bun 1.3)#172saddlepaddle merged 1 commit intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThe changes introduce automation for handling native modules in the Electron desktop build process, specifically targeting Bun 1.3+ isolated installs. A new script is added to convert symlinked native modules (node-pty) into real copied files. Build pipeline scripts and Electron builder configuration are updated to incorporate this preparation step before packaging. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/scripts/copy-native-modules.ts (2)
24-24: Consider more conventional path resolution.While
dirname(import.meta.dirname)works correctly to navigate up one level, the more conventional approach would be:- const nodeModulesDir = join(dirname(import.meta.dirname), "node_modules"); + const nodeModulesDir = join(import.meta.dirname, "..", "node_modules");This makes the intent (going up one directory) more explicit.
36-48: Add error handling for filesystem operations.The script lacks error handling around
rmSyncandcpSync. If these operations fail due to permissions, disk space, or other issues, the error messages may be unclear.Consider wrapping the filesystem operations in a try-catch:
if (stats.isSymbolicLink()) { // Resolve symlink to get real path const realPath = realpathSync(modulePath); console.log(` ${moduleName}: symlink -> replacing with real files`); console.log(` Real path: ${realPath}`); - // Remove the symlink - rmSync(modulePath); - - // Copy the actual files - cpSync(realPath, modulePath, { recursive: true }); - - console.log(` Copied to: ${modulePath}`); + try { + // Remove the symlink + rmSync(modulePath); + + // Copy the actual files + cpSync(realPath, modulePath, { recursive: true }); + + console.log(` Copied to: ${modulePath}`); + } catch (error) { + console.error(` [ERROR] Failed to copy ${moduleName}:`, error); + process.exit(1); + } } else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/.gitignore(1 hunks)apps/desktop/electron-builder.ts(1 hunks)apps/desktop/package.json(1 hunks)apps/desktop/scripts/copy-native-modules.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/electron-builder.tsapps/desktop/scripts/copy-native-modules.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/electron-builder.tsapps/desktop/scripts/copy-native-modules.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid usinganytype - use explicit types instead for type safety
Use camelCase for variable and function names following existing codebase patterns
Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
Follow existing patterns and match the codebase style when writing new code
Files:
apps/desktop/electron-builder.tsapps/desktop/scripts/copy-native-modules.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.951Z
Learning: Use Bun as the package manager - no npm, yarn, or pnpm
📚 Learning: 2025-11-28T01:03:47.951Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.951Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net, etc.) in renderer process code - browser environment only
Applied to files:
apps/desktop/.gitignoreapps/desktop/electron-builder.tsapps/desktop/scripts/copy-native-modules.ts
📚 Learning: 2025-11-28T01:03:47.951Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.951Z
Learning: Applies to apps/desktop/src/lib/**/*.ts : Never import Node.js modules in shared code like `src/lib/electron-router-dom.ts` - this code runs in both main and renderer processes
Applied to files:
apps/desktop/.gitignoreapps/desktop/scripts/copy-native-modules.ts
📚 Learning: 2025-11-28T01:03:47.951Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.951Z
Learning: Use Bun as the package manager - no npm, yarn, or pnpm
Applied to files:
apps/desktop/package.jsonapps/desktop/scripts/copy-native-modules.ts
📚 Learning: 2025-11-28T01:03:47.951Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.951Z
Learning: Applies to **/*.{ts,tsx} : Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
Applied to files:
apps/desktop/electron-builder.ts
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Please use alias as defined in `tsconfig.json` when possible
Applied to files:
apps/desktop/electron-builder.ts
📚 Learning: 2025-11-28T01:03:47.951Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.951Z
Learning: Applies to apps/desktop/src/main/**/*.{ts,tsx} : Node.js modules (fs, path, os, net, etc.) can be used in main process code only
Applied to files:
apps/desktop/electron-builder.ts
🔇 Additional comments (4)
apps/desktop/package.json (1)
21-24: LGTM! Scripts correctly integrated.The new scripts are properly wired into the build pipeline:
copy:native-modulesruns the native module preparation scriptprebuildincludes the copy step after compilationprepackageensures native modules are copied before packagingThis correctly handles the Bun 1.3+ isolated install scenario.
apps/desktop/scripts/copy-native-modules.ts (2)
16-19: LGTM! Imports and constants well-defined.The use of Node.js APIs is appropriate for a build script, and the
as constassertion provides type safety for the module list.
26-52: LGTM! Core logic correctly handles symlinked modules.The script correctly:
- Uses
lstatSyncto detect symlinks without following them- Resolves the real path before copying
- Handles both symlink and non-symlink cases appropriately
This properly addresses the Bun 1.3+ isolated install scenario where native modules are symlinked.
apps/desktop/electron-builder.ts (1)
41-48: electron-builder syntax is correct and properly documented.Verification confirms that electron-builder 26.0.12 fully supports the FileSet configuration with
from,to, andfilterproperties as shown in lines 41–48. The explicitfilter: ["**/*"]is valid (though redundant with the documented default). This is the correct approach for copying native modules in the build configuration.
| # production | ||
| /build | ||
| /dist | ||
| /.native-modules |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check 1: Look for .native-modules directory references
fd -t d "^.native-modules$" -d 1 2>/dev/null || echo "No .native-modules dir found in root"
# Check 2: Search for .native-modules references in the codebase
rg "\.native-modules" --type-list | head -5
rg "\.native-modules" -n 2>/dev/null | head -20
# Check 3: Find the copy-native-modules.ts script
fd "copy-native-modules" -t fRepository: superset-sh/superset
Length of output: 514
🏁 Script executed:
#!/bin/bash
# Check script size first
wc -l apps/desktop/scripts/copy-native-modules.ts
# Read the script
cat -n apps/desktop/scripts/copy-native-modules.ts
# Search for .native-modules references (corrected)
echo "=== .native-modules references ==="
rg "native-modules" -n 2>/dev/null || echo "No matches found"
# Check if it's mentioned in package.json
echo "=== Checking package.json ==="
fd "package.json" -t f | xargs grep -l "native-modules" 2>/dev/null || echo "Not found in package.json"Repository: superset-sh/superset
Length of output: 2685
🏁 Script executed:
#!/bin/bash
# Comprehensive search for actual .native-modules directory references
echo "=== Searching for .native-modules directory (not script name) ==="
rg "\.native-modules" --type-list | head -5 && echo "---" || true
rg "\.native-modules" -n 2>/dev/null || echo "No exact matches"
# Check .gitignore content around line 12
echo ""
echo "=== .gitignore content ==="
cat -n apps/desktop/.gitignore
# Check if there are any build or dist patterns
echo ""
echo "=== Checking build/output related configs ==="
fd "tsconfig|webpack|vite|build" -t f apps/desktop/ | head -10Repository: superset-sh/superset
Length of output: 1226
The .native-modules entry is unnecessary and can be removed.
The copy-native-modules.ts script works with node_modules/node-pty, which is already covered by the /node_modules ignore pattern on line 2. No part of the codebase creates or references a .native-modules directory—this entry appears to be unused dead code in .gitignore.
🤖 Prompt for AI Agents
In apps/desktop/.gitignore around line 12 the entry "/.native-modules" is
dead/unnecessary because node-native artifacts are already ignored via
"/node_modules" and no code creates or references a ".native-modules" directory;
remove the "/.native-modules" line from the .gitignore to clean up unused
entries and keep the file minimal.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.