refactor(platform): address code review feedback for node-finder

- 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 commit is contained in:
Kacper
2025-12-21 15:01:32 +01:00
parent 887fb93b3b
commit b18672f66d
2 changed files with 17 additions and 6 deletions

View File

@@ -10,6 +10,9 @@ import fs from 'fs';
import path from 'path';
import os from 'os';
/** Pattern to match version directories (e.g., "v18.17.0", "18.17.0") */
const VERSION_DIR_PATTERN = /^v?\d+/;
/** Result of finding Node.js executable */
export interface NodeFinderResult {
/** Path to the Node.js executable */
@@ -50,7 +53,7 @@ function findNodeFromVersionManager(
try {
const versions = fs
.readdirSync(basePath)
.filter((v) => /^v?\d+/.test(v))
.filter((v) => VERSION_DIR_PATTERN.test(v))
// Semantic version sort - newest first using localeCompare with numeric option
.sort((a, b) => b.localeCompare(a, undefined, { numeric: true, sensitivity: 'base' }));
@@ -219,7 +222,10 @@ function findNodeWindows(homeDir: string): NodeFinderResult | null {
/**
* Try to find Node.js using shell commands (which/where)
*/
function findNodeViaShell(platform: NodeJS.Platform): NodeFinderResult | null {
function findNodeViaShell(
platform: NodeJS.Platform,
logger: (message: string) => void = () => {}
): NodeFinderResult | null {
try {
const command = platform === 'win32' ? 'where node' : 'which node';
const result = execSync(command, {
@@ -238,6 +244,7 @@ function findNodeViaShell(platform: NodeJS.Platform): NodeFinderResult | null {
}
} catch {
// Shell command failed (likely when launched from desktop without PATH)
logger('Shell command failed to find Node.js (expected when launched from desktop)');
}
return null;
@@ -293,7 +300,7 @@ export function findNodeExecutable(options: NodeFinderOptions = {}): NodeFinderR
}
// Fallback - try shell resolution (works when launched from terminal)
result = findNodeViaShell(platform);
result = findNodeViaShell(platform, logger);
if (result) {
logger(`Found Node.js via ${result.source} at: ${result.nodePath}`);
return result;
@@ -342,5 +349,9 @@ export function buildEnhancedPath(nodePath: string, currentPath: string = ''): s
}
// Use platform-appropriate path separator
// Handle empty currentPath without adding trailing delimiter
if (!currentPath) {
return nodeDir;
}
return `${nodeDir}${path.delimiter}${currentPath}`;
}

View File

@@ -87,12 +87,12 @@ describe('node-finder', () => {
expect(result).toBe(currentPath);
});
it('should handle empty currentPath', () => {
it('should handle empty currentPath without trailing delimiter', () => {
const nodePath = '/opt/homebrew/bin/node';
const result = buildEnhancedPath(nodePath, '');
expect(result).toBe(`/opt/homebrew/bin${delimiter}`);
expect(result).toBe('/opt/homebrew/bin');
});
it('should handle Windows-style paths', () => {
@@ -118,7 +118,7 @@ describe('node-finder', () => {
const result = buildEnhancedPath(nodePath);
expect(result).toBe(`/usr/local/bin${delimiter}`);
expect(result).toBe('/usr/local/bin');
});
});
});