fix: add cross-platform Node.js executable finder for desktop launches#208
fix: add cross-platform Node.js executable finder for desktop launches#208webdevcody merged 10 commits intomainfrom
Conversation
When the Electron app is launched from desktop environments (macOS Finder, Windows Explorer, Linux desktop launchers), the PATH environment variable is often limited and doesn't include Node.js installation paths. This adds a new `findNodeExecutable()` utility to @automaker/platform that: - Searches common installation paths (Homebrew, system, Program Files) - Supports version managers: NVM, fnm, nvm-windows, Scoop, Chocolatey - Falls back to shell resolution (which/where) when available - Enhances PATH for child processes via `buildEnhancedPath()` - Works cross-platform: macOS, Windows, and Linux 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a cross-platform Node.js executable finder and PATH helper in libs/platform, integrates them into the Electron startup to resolve and inject Node into spawned servers, adds tests for the finder, updates README to require building local packages before running, and removes Next.js ambient type declarations in apps/app/next-env.d.ts. Changes
Sequence Diagram(s)sequenceDiagram
participant Electron as Electron Main
participant Finder as Node Finder
participant OS as Operating System / Shell
participant Server as Backend Server (child)
Note over Electron,Finder: Startup (dev/prod)
Electron->>Finder: findNodeExecutable(options?)
Finder->>Finder: probe Homebrew, system dirs, nvm/fnm, Windows locations
alt probes fail
Finder->>OS: run "which" / "where"
OS-->>Finder: node path or none
end
Finder-->>Electron: { nodePath, source }
Electron->>Finder: buildEnhancedPath(nodePath, process.env.PATH)
Finder-->>Electron: enhanced PATH
Electron->>Server: spawn server using nodePath with env.PATH=enhanced
Server-->>Electron: started / error
alt server started
Electron->>Electron: create BrowserWindow
else startup failed
Electron->>User: show error dialog
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Comment |
Summary of ChangesHello @Shironex, 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 delivers a crucial fix for the Electron application's startup reliability by implementing a sophisticated, cross-platform Node.js executable finder. This enhancement ensures that the app's backend server can consistently locate and utilize the Node.js runtime, even when the application is launched from desktop shortcuts where the system's PATH environment variable might be restricted. The solution intelligently searches common installation locations and version managers, significantly improving the user experience by preventing startup failures and providing clearer error messages when issues occur. 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
|
This comment was marked as outdated.
This comment was marked as outdated.
![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 introduces a robust, cross-platform mechanism for locating the Node.js executable, which is a crucial improvement for an Electron application launched from a desktop environment. The implementation is comprehensive, covering various installation methods like Homebrew, NVM, Scoop, and system paths across macOS, Windows, and Linux. The addition of a user-facing error dialog on startup failure is also a great UX enhancement. I've identified a couple of potential bugs in the path-finding logic that could make the solution even more reliable.
![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 (2)
libs/platform/src/node-finder.ts (2)
183-197: Consider refining fnm Windows path lookup.The
fnm_multishellsdirectory (line 187) is used for fnm's shell session shims rather than the actual Node.js installations. While the shim may work, it's less reliable than the canonical installation path. The fallback path on line 189 (~/.fnm/node-versions) is correct.Consider swapping the order to prioritize the more reliable path:
🔎 Suggested reorder
// fnm on Windows const fnmWindowsPaths = [ + path.join(homeDir, '.fnm', 'node-versions'), path.join( process.env.LOCALAPPDATA || path.join(homeDir, 'AppData', 'Local'), - 'fnm_multishells' + 'fnm', 'node-versions' ), - path.join(homeDir, '.fnm', 'node-versions'), ];
334-337: Substring match may cause false positives in path deduplication.The
currentPath.includes(nodeDir)check can incorrectly match substrings. For example, ifnodeDiris/opt/nodeandcurrentPathcontains/opt/node-v18/bin, the check would incorrectly returncurrentPathunchanged.Consider matching against path segments:
🔎 Suggested fix
// Don't add if already present or if it's just '.' - if (nodeDir === '.' || currentPath.includes(nodeDir)) { + const pathSegments = currentPath.split(path.delimiter); + if (nodeDir === '.' || pathSegments.includes(nodeDir)) { return currentPath; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/app/next-env.d.ts(0 hunks)apps/ui/src/main.ts(15 hunks)libs/platform/src/index.ts(1 hunks)libs/platform/src/node-finder.ts(1 hunks)libs/platform/tests/node-finder.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/app/next-env.d.ts
🧰 Additional context used
🧬 Code graph analysis (2)
libs/platform/tests/node-finder.test.ts (1)
libs/platform/src/node-finder.ts (2)
findNodeExecutable(263-304)buildEnhancedPath(326-341)
libs/platform/src/node-finder.ts (1)
libs/platform/src/index.ts (4)
NodeFinderResult(52-52)NodeFinderOptions(53-53)findNodeExecutable(50-50)buildEnhancedPath(51-51)
🪛 GitHub Actions: Test Suite
libs/platform/tests/node-finder.test.ts
[error] 104-104: AssertionError: expected 'C:\Windows\System32' to be 'C:\Program Files\nodejs:C:\Windows\System32' // Object.is equality
🪛 GitHub Check: test
libs/platform/tests/node-finder.test.ts
[failure] 104-104: tests/node-finder.test.ts > node-finder > buildEnhancedPath > should handle Windows-style paths
AssertionError: expected 'C:\Windows\System32' to be 'C:\Program Files\nodejs:C:\Windows\Sy…' // Object.is equality
Expected: "C:\Program Files\nodejs:C:\Windows\System32"
Received: "C:\Windows\System32"
❯ tests/node-finder.test.ts:104:22
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e
- GitHub Check: claude-review
🔇 Additional comments (6)
libs/platform/src/index.ts (1)
47-54: LGTM!The new exports follow the established pattern in the file, correctly using
typefor type-only exports. This cleanly expands the public API surface with the cross-platform Node.js discovery utilities.libs/platform/src/node-finder.ts (1)
263-304: LGTM!The main
findNodeExecutablefunction is well-structured with clear fallback logic. The platform-specific search, shell resolution fallback, and ultimate fallback ensure Node.js can be found in various scenarios.libs/platform/tests/node-finder.test.ts (2)
6-54: LGTM!The
findNodeExecutabletests cover the key behaviors: skip search mode, logger integration, result structure validation, and actual discovery verification.
56-97: LGTM!The remaining
buildEnhancedPathtests cover the key edge cases well: fallback handling, path prepending, deduplication, and default parameter behavior.Also applies to: 107-114
apps/ui/src/main.ts (2)
130-179: LGTM!The integration of
findNodeExecutableandbuildEnhancedPathis well-implemented:
- Correctly skips search in dev mode where PATH is typically available
- Logs the Node.js source for debugging
- Enhances PATH for child processes to ensure they can also find Node.js
339-346: LGTM!The error dialog provides a clear, actionable message to users when startup fails, mentioning Node.js installation as a potential fix. This is a significant UX improvement over silent failures.
Shironex
added
Testers-Requested
- Fix PATH collision detection using proper path segment matching instead of substring includes() which could cause false positives - Reorder fnm Windows paths to prioritize canonical installation path over shell shims (fnm_multishells) - Make Windows path test platform-aware since path.dirname handles backslash paths differently on non-Windows systems 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
- Add path.normalize() for Windows mixed separator handling - Add validation to check Node executable exists after finding it - Improve error dialog with specific troubleshooting advice for Node.js related errors vs general errors - Include source info in validation error message 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
- Extract VERSION_DIR_PATTERN regex to named constant - Pass logger to findNodeViaShell for consistent debug logging - Fix buildEnhancedPath to not add trailing delimiter for empty currentPath 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
- Add null byte validation to shell command output (security hardening) - Expand VERSION_DIR_PATTERN comment to explain intentional pre-release support 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/platform/src/node-finder.ts (2)
70-72: Consider logging errors in the catch block.The empty catch block silently swallows directory read errors. While this is appropriate for a best-effort search, adding a logger call would improve debuggability.
🔎 Suggested improvement
- } catch { - // Directory read failed, skip this location + } catch (error) { + // Directory read failed, skip this location - this is expected if path doesn't exist or lacks permissions }Note: If you add a logger parameter to this function, you could also log the error for debugging purposes.
350-353: Consider case-insensitive path comparison on Windows.On Windows, filesystem paths are case-insensitive, but
path.normalize()doesn't change the case of path components. This meansC:\Program Filesandc:\program fileswould be treated as different paths here, even though they refer to the same location. In practice, this is unlikely to cause issues since environment variables are typically consistent in casing, but for maximum robustness, you could use case-insensitive comparison on Windows.🔎 Optional enhancement for Windows
const normalizedNodeDir = path.normalize(nodeDir); - const pathSegments = currentPath.split(path.delimiter).map((s) => path.normalize(s)); - if (normalizedNodeDir === '.' || pathSegments.includes(normalizedNodeDir)) { + const pathSegments = currentPath.split(path.delimiter).map((s) => path.normalize(s)); + const compareDir = process.platform === 'win32' ? normalizedNodeDir.toLowerCase() : normalizedNodeDir; + const compareSegments = process.platform === 'win32' + ? pathSegments.map(s => s.toLowerCase()) + : pathSegments; + if (normalizedNodeDir === '.' || compareSegments.includes(compareDir)) { return currentPath; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/platform/src/node-finder.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libs/platform/src/node-finder.ts (1)
libs/platform/src/index.ts (4)
NodeFinderResult(52-52)NodeFinderOptions(53-53)findNodeExecutable(50-50)buildEnhancedPath(51-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e
- GitHub Check: claude-review
🔇 Additional comments (9)
libs/platform/src/node-finder.ts (9)
1-45: LGTM! Well-structured type definitions and setup.The imports, constants, and type definitions are clear and appropriate. The
VERSION_DIR_PATTERNregex is intentionally permissive as documented, which is suitable for this use case with fallback to semantic version sorting.
77-121: LGTM! macOS search strategy is well-designed.The search order prioritizes Homebrew installations (with Apple Silicon first), then system Node, then version managers (NVM, fnm). The multiple fnm locations cover different installation scenarios on macOS.
123-162: LGTM! Linux search strategy covers common installation scenarios.The implementation checks standard system paths including Snap, then version managers. The fnm paths cover typical Linux installation locations.
164-224: Windows search strategy is comprehensive.The implementation covers major Windows installation methods: Program Files, nvm-windows, fnm, Scoop, and Chocolatey. The use of environment variables with fallbacks is appropriate.
226-256: LGTM! Shell-based fallback is well-implemented.The implementation correctly handles platform differences (
wherevswhich), includes security validation (null byte check), and gracefully handles errors with logging. Taking the first line fromwhereoutput is appropriate since it can return multiple paths.
258-317: LGTM! Main function has a well-designed fallback chain.The implementation provides a robust search strategy with clear fallback levels: platform-specific discovery → shell resolution → ultimate fallback. The
skipSearchoption and logging throughout are helpful for different deployment scenarios and debugging.
339-362: LGTM! Path deduplication is now robust.The implementation correctly addresses the past review concern about
includes()being unreliable. By normalizing paths and comparing exact path segments (line 352), it avoids false positives like matching/usr/binagainst/usr/bin-extra. The handling of edge cases (fallback 'node', empty currentPath, '.' directory) is thorough.
190-205: The fnm path prioritization comment is based on incorrect assumptions about Windows installation behavior.fnm's default installation directory is $HOME/.fnm, with no Windows-specific override documented in official sources. Evidence shows fnm actually uses %APPDATA%\Roaming\fnm\node-versions on Windows, not %LOCALAPPDATA%. The review's suggestion to prioritize LOCALAPPDATA lacks supporting evidence; neither the current path order nor LOCALAPPDATA reflects fnm's actual Windows behavior.
58-62: Use a proper semver comparison function instead of localeCompare for semantic version sorting.The code uses
localeComparewithnumeric: truefor "semantic version sort," but this approach cannot correctly handle semantic versioning rules, particularly for pre-release versions. According to the semver specification, pre-release versions (e.g.,1.0.0-alpha,1.0.0-beta) must sort lower than their release versions, and pre-release identifiers follow complex comparison rules involving numeric vs. non-numeric precedence.localeCompareis a locale-aware string comparison tool and does not parse or understand these semver-specific rules.Consider using the
semvernpm package or implementing a custom comparator that properly parses version components to ensure correct sorting for all valid semantic versions.
- Add isExecutable() helper to verify files have execute permission - On Unix: uses fs.constants.X_OK to check execute permission - On Windows: only checks file existence (X_OK not meaningful) - Replace fs.existsSync with isExecutable for all node path checks - Add JSDoc comment documenting version sorting limitations - Add test to verify found node binary is executable 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
libs/platform/src/node-finder.ts (1)
213-221: fnm path search order contradicts the comment.The comment states "prioritize canonical installation path over shell shims," but the code checks
~/.fnm/node-versionsbefore%LOCALAPPDATA%/fnm/node-versions. On Windows, the canonical fnm installation location is%LOCALAPPDATA%/fnm, so the search order should be reversed to match the comment.🔎 Proposed fix to prioritize canonical path
// fnm on Windows (prioritize canonical installation path over shell shims) const fnmWindowsPaths = [ - path.join(homeDir, '.fnm', 'node-versions'), path.join( process.env.LOCALAPPDATA || path.join(homeDir, 'AppData', 'Local'), 'fnm', 'node-versions' ), + path.join(homeDir, '.fnm', 'node-versions'), ];
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/platform/src/node-finder.ts(1 hunks)libs/platform/tests/node-finder.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/platform/tests/node-finder.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
libs/platform/src/node-finder.ts (1)
libs/platform/src/index.ts (4)
NodeFinderResult(52-52)NodeFinderOptions(53-53)findNodeExecutable(50-50)buildEnhancedPath(51-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e
- GitHub Check: claude-review
🔇 Additional comments (7)
libs/platform/src/node-finder.ts (7)
20-45: LGTM! Clean interface design.The interfaces are well-structured and cover all the search strategies comprehensively. The optional
skipSearchandloggerprovide useful flexibility for different execution contexts.
51-64: LGTM! Correct cross-platform executability check.The platform-specific handling (F_OK for Windows, X_OK for Unix-like systems) is appropriate and well-commented.
103-144: LGTM! Comprehensive macOS Node.js search paths.The search order (Homebrew → system → version managers) is logical, and the fnm locations cover both XDG and macOS-native installation patterns.
149-185: LGTM! Thorough Linux Node.js search paths.The inclusion of Snap alongside traditional system paths and version managers provides good coverage for common Linux distributions.
252-279: LGTM! Shell fallback with solid security validation.The null byte check on line 267 and executable validation provide good defense-in-depth. The error handling and logging make it clear when shell resolution fails (expected in desktop launch scenarios).
299-340: LGTM! Well-structured search orchestration.The search fallback chain (platform-specific → shell → ultimate fallback) is robust and handles desktop launch scenarios effectively. The logging at each discovery point aids troubleshooting.
362-385: LGTM! PATH deduplication properly addresses previous concerns.The path normalization and exact segment matching (lines 373-375) correctly handles the false-positive issue raised in previous reviews (e.g.,
/usr/binvs/usr/bin-extra). The edge-case handling for'.'and emptycurrentPathis appropriate.
- Added a step to build local shared packages before running Automaker - Updated the sequence of instructions for clarity
- Add PRE_RELEASE_PATTERN to identify beta, rc, alpha, nightly, canary, dev, pre versions - Modify findNodeFromVersionManager to try stable versions first - Pre-release versions are used as fallback if no stable version found - Add tests for pre-release detection and version prioritization 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code Review: Cross-platform Node.js Executable FinderOverall AssessmentThis is a well-designed solution to a real problem (desktop launcher PATH limitations). The code is clean, well-documented, and follows good practices. The implementation is thorough with excellent cross-platform support. ✅ Strengths1. Architecture & Design
2. Cross-Platform CoverageExcellent support across platforms:
3. Security Considerations
4. Code Quality
5. Testing
🔍 Issues & SuggestionsCritical: Semantic Version Sorting LogicIssue: The version sorting in .sort((a, b) => b.localeCompare(a, undefined, { numeric: true, sensitivity: 'base' }));Problem:
Recommendation: Consider using a proper semver library like import semver from 'semver';
const allVersions = fs
.readdirSync(basePath)
.filter((v) => VERSION_DIR_PATTERN.test(v))
.sort((a, b) => {
const cleanA = a.replace(/^v/, '');
const cleanB = b.replace(/^v/, '');
return semver.rcompare(cleanA, cleanB); // reverse compare (newest first)
});Or if avoiding dependencies, implement proper version comparison: function parseVersion(v: string): number[] {
return v.replace(/^v/, '').split('.').map(n => parseInt(n) || 0);
}
function compareVersions(a: string, b: string): number {
const partsA = parseVersion(a);
const partsB = parseVersion(b);
for (let i = 0; i < Math.max(partsA.length, partsB.length); i++) {
const diff = (partsB[i] || 0) - (partsA[i] || 0);
if (diff !== 0) return diff;
}
return 0;
}Medium: Error Dialog Message ClarityLocation: Current: dialog.showErrorBox(
'Automaker Failed to Start',
`The application failed to start.\n\n${errorMessage}\n\n${
isNodeError
? 'Please install Node.js from https://nodejs.org or via a package manager (Homebrew, nvm, fnm).'
: 'Please check the application logs for more details.'
}`
);Issue: The Suggestion: Make error detection more robust: const isNodeError = errorMessage.includes('Node.js') ||
errorMessage.includes('node executable') ||
errorMessage.includes('ENOENT') && errorMessage.includes('node');Low: Missing Edge Case Handling1. Symlink Resolution function isExecutable(filePath: string): boolean {
try {
// Resolve symlinks
const realPath = fs.realpathSync(filePath);
if (process.platform === 'win32') {
fs.accessSync(realPath, fs.constants.F_OK);
} else {
fs.accessSync(realPath, fs.constants.X_OK);
}
return true;
} catch {
return false;
}
}2. PATH Manipulation Edge Case if (normalizedNodeDir === '.' || normalizedNodeDir === '' || pathSegments.includes(normalizedNodeDir)) {
return currentPath;
}Low: Code Style ConsistencyQuote Style Change: The PR changes quote style from double to single quotes throughout
Low: DocumentationMissing:
Suggestion: Add a comment above the usage in /**
* Find Node.js executable for server spawn.
* When launched from desktop (Finder/Explorer), PATH is limited and may not include
* Node.js installation paths. This searches common installation locations.
* In dev mode, we skip the search since PATH is already correct when launched from terminal.
*/
const nodeResult = findNodeExecutable({
skipSearch: isDev,
logger: (msg) => console.log(`[Electron] ${msg}`),
});🧪 Testing RecommendationsCurrent Tests ✅
Missing Tests
|
Shironex
added
Enhancement
2 participants
Summary
findNodeExecutable()andbuildEnhancedPath()to@automaker/platformProblem
When the Electron app is launched from desktop environments (macOS Finder, Windows Explorer, Linux desktop launchers), the PATH environment variable is limited and doesn't include Node.js installation paths. This causes the server spawn to fail.
Solution
New
node-finder.tsmodule in@automaker/platformthat:/usr/bin, NVM, fnm/usr/bin,/usr/local/bin, Snap, NVM, fnmFeatures:
buildEnhancedPath()for ensuring child processes can find Nodewhich/where) when availableTest plan
npm run build:electron:mac:dirshould resolve
✏️ Tip: You can customize this high-level summary in your review settings.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.