-
Notifications
You must be signed in to change notification settings - Fork 525
fix: resolve Electron build module resolution for local packages #723
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Local @automaker/* packages were bundled using file: references that created symlinks, which break inside packaged Electron apps (AppImage, deb). This copies packages directly to node_modules and collects their external dependencies (e.g. p-limit) for proper npm installation. Also ensures parent directories exist before writing to Electron userData, preventing ENOENT errors on first launch. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @TataMata, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses critical build and runtime issues in packaged Electron applications. It refines the module resolution strategy for local packages by moving from symlinked Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughThe PR refactors the server preparation script to inline local packages directly into node_modules instead of using file: references and symlinks. Additionally, it adds parent directory creation logic to ensure target directories exist before writing files in the file system utility. Changes
Sequence DiagramsequenceDiagram
participant Script as Build Script
participant PkgJson as Local Package<br/>package.json Files
participant ExtDeps as External<br/>Dependencies
participant NodeMods as node_modules
participant Native as Native<br/>Modules
Script->>PkgJson: Scan each local package
activate PkgJson
Script->>Script: Collect external dependencies<br/>from each package
deactivate PkgJson
Script->>ExtDeps: Install external dependencies
activate ExtDeps
ExtDeps-->>NodeMods: Install packages
deactivate ExtDeps
Script->>NodeMods: Copy local packages directly<br/>(no symlinks)
activate NodeMods
NodeMods-->>NodeMods: Local packages inlined
deactivate NodeMods
Script->>Native: Rebuild native modules
activate Native
Native-->>NodeMods: Updated binaries
deactivate Native
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces several important fixes for the Electron build process. It correctly changes the strategy for handling local packages from using file: symlinks to directly copying them into node_modules, which resolves module resolution errors in packaged applications. It also adds logic to collect transitive dependencies from these local packages and fixes a file system error on first launch by ensuring directories are created before writing files. The changes are well-implemented. I have one suggestion to improve the dependency collection script by adding a warning for version conflicts, which would make the build process more robust.
| if (!dependencies[depName]) { | ||
| dependencies[depName] = depVersion; | ||
| console.log(` + ${depName}@${depVersion} (from ${pkgName})`); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current logic for collecting transitive dependencies from local packages doesn't handle version conflicts. If two local packages (or the server and a local package) depend on different versions of the same external dependency, the script will silently use the first version it encounters. This could lead to subtle bugs or runtime errors if the versions are incompatible.
It would be safer to add a warning when a version conflict is detected. This will make developers aware of potential issues during the build process.
if (!dependencies[depName]) {
dependencies[depName] = depVersion;
console.log(` + ${depName}@${depVersion} (from ${pkgName})`);
} else if (dependencies[depName] !== depVersion) {
console.warn(` ⚠️ Version conflict for ${depName}: existing version is ${dependencies[depName]}, but ${pkgName} requires ${depVersion}. Using existing version.`);
}
Summary
file:references (which create symlinks) with direct copying of@automaker/*packages tonode_modules, resolvingERR_MODULE_NOT_FOUNDerrors in packaged Electron apps (AppImage, deb)p-limitfrom@automaker/platform) are now collected and installed via npmuserData, preventing crashes on first app launchTest plan
npm run build:electron:linux(or mac/win equivalent) — build should complete without errors~/.config/Automaker/directory)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.