From b18672f66def19c130d3f1540a23c350bee89fc2 Mon Sep 17 00:00:00 2001
From: Kacper <kacperlachowiczwp.pl@wp.pl>
Date: Sun, 21 Dec 2025 15:01:32 +0100
Subject: [PATCH] refactor(platform): address code review feedback for
 node-finder
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- 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>
---
 libs/platform/src/node-finder.ts        | 17 ++++++++++++++---
 libs/platform/tests/node-finder.test.ts |  6 +++---
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/libs/platform/src/node-finder.ts b/libs/platform/src/node-finder.ts
index cc130478..9c29c308 100644
--- a/libs/platform/src/node-finder.ts
+++ b/libs/platform/src/node-finder.ts
@@ -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}`;
 }
diff --git a/libs/platform/tests/node-finder.test.ts b/libs/platform/tests/node-finder.test.ts
index f37d0d85..4304fbbb 100644
--- a/libs/platform/tests/node-finder.test.ts
+++ b/libs/platform/tests/node-finder.test.ts
@@ -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');
     });
   });
 });