fix: include libs directory in Electron build extraResources #303
Conversation
The @automaker/* packages in server-bundle/node_modules are symlinks pointing to ../../libs/. Without including the libs directory in extraResources, these symlinks are broken in the packaged app, causing 'Server failed to start' error on launch.
📝 WalkthroughWalkthroughAdds a packaging resource mapping to the UI app and implements sandbox compatibility checks in the server SDK that auto-disable sandbox mode for cloud-storage paths, plus unit tests covering cloud-path detection and sandbox behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @firstfloris, 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 rectifies a critical issue preventing the Electron application from launching correctly after being packaged. Specifically, it fixes a "Server failed to start" error that occurred because symbolic links within the 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a bug that caused the packaged Electron app to fail on startup. The issue was due to broken symlinks for local packages because their target directory (libs) was not being included in the final build. By adding the server-bundle/libs directory to the extraResources in apps/ui/package.json, you've ensured that these necessary files are present in the packaged application, resolving the problem. The change is concise, well-targeted, and the pull request description clearly explains the problem and solution. The fix appears correct and complete.
firstfloris
force-pushed
the
fix/electron-build-missing-libs
branch
from
551c462 to
927ce91
Compare
![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/server/tests/unit/lib/sdk-options.test.ts (1)
315-324: LGTM!The new tests verify that sandbox mode is correctly auto-disabled when the working directory is in a cloud storage location. Good coverage of the integration between
checkSandboxCompatibilityand the option creation functions.For consistency, consider adding an iCloud path test to
createChatOptionsas well, sincecreateAutoModeOptionstests both Dropbox and iCloud paths.Also applies to: 387-407
apps/server/src/lib/sdk-options.ts (2)
66-72: Windows path compatibility gap.The patterns use forward slashes, but on Windows,
path.resolve()returns backslash-separated paths. For example,C:\Users\test\Dropbox\projectwon't match'/Dropbox/'.Consider normalizing the resolved path to use forward slashes before pattern matching:
🔎 Proposed fix
export function isCloudStoragePath(cwd: string): boolean { const resolvedPath = path.resolve(cwd); - return CLOUD_STORAGE_PATH_PATTERNS.some((pattern) => resolvedPath.includes(pattern)); + // Normalize to forward slashes for cross-platform pattern matching + const normalizedPath = resolvedPath.replace(/\\/g, '/'); + return CLOUD_STORAGE_PATH_PATTERNS.some((pattern) => normalizedPath.includes(pattern)); }
64-64: Address TODO: File upstream issue.The TODO references filing an issue with the Claude CLI project. Consider tracking this to ensure it's not forgotten.
Would you like me to open an issue to track filing this upstream report?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/server/src/lib/sdk-options.tsapps/server/tests/unit/lib/sdk-options.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/server/tests/unit/lib/sdk-options.test.tsapps/server/src/lib/sdk-options.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/server/tests/unit/lib/sdk-options.test.tsapps/server/src/lib/sdk-options.ts
apps/server/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
createEventEmitter()fromlib/events.tsfor all server operations to emit events that stream to frontend via WebSocket
Files:
apps/server/src/lib/sdk-options.ts
🧬 Code graph analysis (1)
apps/server/tests/unit/lib/sdk-options.test.ts (1)
apps/server/src/lib/sdk-options.ts (4)
isCloudStoragePath(83-86)checkSandboxCompatibility(111-143)createChatOptions(418-446)createAutoModeOptions(458-483)
🔇 Additional comments (7)
apps/server/tests/unit/lib/sdk-options.test.ts (2)
15-57: Good test coverage for macOS cloud storage paths.The tests comprehensively cover macOS cloud storage detection for Dropbox, Google Drive, OneDrive, and iCloud Drive. The negative test cases for local and relative paths are also well-structured.
Consider adding Windows path coverage if Windows is a supported platform:
C:\Users\test\Dropbox\projectC:\Users\test\OneDrive\projectC:\Users\test\Google Drive\project
59-91: LGTM!The
checkSandboxCompatibilitytests cover all code paths: user-disabled, cloud storage auto-disabled, enabled for local paths, and the undefined parameter edge case. The assertions appropriately verify both theenabledstate anddisabledReason.apps/server/src/lib/sdk-options.ts (5)
83-86: LGTM!The function correctly resolves relative paths and checks against known cloud storage patterns. The substring matching approach is pragmatic—false positives (detecting non-cloud paths as cloud) are low-risk since they only result in sandbox being disabled, which is a safer default.
91-98: LGTM!The
SandboxCheckResultinterface is well-designed with a discriminateddisabledReasonfield that makes the return values self-documenting.
111-143: LGTM!The function correctly prioritizes user settings over cloud storage detection and provides meaningful context in the return value. The early return for falsy
enableSandboxModeensures explicit opt-out always takes precedence.The
console.warnat line 130 is appropriate here since this is a warning for operators/developers rather than an event to stream to the frontend.
428-442: LGTM!Clean refactoring that centralizes sandbox compatibility logic. The conditional spread pattern
...(sandboxCheck.enabled && { sandbox: {...} })is idiomatic and maintains the same behavior while adding cloud storage awareness.
465-479: LGTM!Consistent implementation with
createChatOptions. The sharedcheckSandboxCompatibilityfunction eliminates duplication and ensures uniform behavior across both option creators.
Shironex
added
Testers-Requested
|
can merge after test is fixed |
3 participants
Summary
Problem
The
@automaker/*packages inserver-bundle/node_modulesare symlinks pointing to../../libs/.... Thelibsdirectory was being prepared byprepare-server.mjsbut wasn't included in theextraResourcesconfiguration, so the symlinks were broken in the packaged app.Solution
Add the
server-bundle/libsdirectory toextraResourcesinapps/ui/package.jsonso the symlink targets are available in the packaged app.Testing
npm run build:electron:maclibs/directory is present in packaged app atContents/Resources/server/libs/Summary by CodeRabbit
Chores
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.