🐛 Fix code review issues for menubar support

Addresses feedback from PR #200 code review:

**Critical fixes:**
- Make saveUserPath() non-blocking to avoid CLI startup delays
- Add comment documenting the race condition mitigation strategy

**Bug fixes:**
- Remove broken osascript notification (file watching is primary)
- Clean up local .vizzly files when detecting stale registry entries

**Code quality:**
- Add input validation to register() (required fields, number checks)
- Fix unregister() to use AND logic when both port and directory provided
- Fix flaky test by using high port range (48500+) to avoid conflicts

Author: Robdel12

src/cli.js

@@ -763,8 +763,8 @@ program
     await projectRemoveCommand(options, globalOptions);
   });
 
-// Save user's PATH for menubar app before parsing commands
+// Save user's PATH for menubar app (non-blocking, runs in background)
 // This auto-configures the menubar app so it can find npx/node
-await saveUserPath().catch(() => {});
+saveUserPath().catch(() => {});
 
 program.parse();

src/commands/tdd-daemon.js

@@ -50,8 +50,14 @@ export async function tddStartCommand(options = {}, globalOptions = {}) {
       }
       return;
     } else {
-      // Stale entry - clean it up
+      // Stale entry - clean it up (registry and local files)
       registry.unregister({ directory: process.cwd() });
+
+      let vizzlyDir = join(process.cwd(), '.vizzly');
+      let pidFile = join(vizzlyDir, 'server.pid');
+      let serverFile = join(vizzlyDir, 'server.json');
+      if (existsSync(pidFile)) unlinkSync(pidFile);
+      if (existsSync(serverFile)) unlinkSync(serverFile);
     }
   }
 
@@ -62,24 +68,27 @@ export async function tddStartCommand(options = {}, globalOptions = {}) {
   if (options.port) {
     // User specified a port - use it (will fail if busy)
     port = options.port;
+
+    // Check if user-specified port is in use
+    if (await isServerRunning(port)) {
+      output.header('tdd', 'local');
+      output.print(
+        `  ${output.statusDot('error')} Port ${port} is already in use`
+      );
+      output.blank();
+      output.hint('Try a different port: vizzly tdd start --port 47393');
+      output.hint('Or let Vizzly auto-allocate: vizzly tdd start');
+      return;
+    }
   } else {
     // Auto-allocate an available port
+    // Note: There's a small race window between finding a port and binding.
+    // The registry acts as a soft reservation, and findAvailablePort does
+    // an actual TCP bind test to minimize this window.
     port = await registry.findAvailablePort();
     autoAllocated = port !== 47392;
   }
 
-  // If user specified a port, check if it's in use
-  if (options.port && (await isServerRunning(port))) {
-    output.header('tdd', 'local');
-    output.print(
-      `  ${output.statusDot('error')} Port ${port} is already in use`
-    );
-    output.blank();
-    output.hint('Try a different port: vizzly tdd start --port 47393');
-    output.hint('Or let Vizzly auto-allocate: vizzly tdd start');
-    return;
-  }
-
   try {
     // Ensure .vizzly directory exists
     let vizzlyDir = join(process.cwd(), '.vizzly');

src/tdd/server-registry.js

@@ -1,4 +1,3 @@
-import { execSync } from 'node:child_process';
 import { randomBytes } from 'node:crypto';
 import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs';
 import { createServer } from 'node:net';
@@ -55,18 +54,30 @@ export class ServerRegistry {
    * Register a new server in the registry
    */
   register(serverInfo) {
+    // Validate required fields
+    if (!serverInfo.pid || !serverInfo.port || !serverInfo.directory) {
+      throw new Error('Missing required fields: pid, port, directory');
+    }
+
+    let port = Number(serverInfo.port);
+    let pid = Number(serverInfo.pid);
+
+    if (Number.isNaN(port) || Number.isNaN(pid)) {
+      throw new Error('Invalid port or pid - must be numbers');
+    }
+
     let registry = this.read();
 
     // Remove any existing entry for this port or directory (shouldn't happen, but be safe)
     registry.servers = registry.servers.filter(
-      s => s.port !== serverInfo.port && s.directory !== serverInfo.directory
+      s => s.port !== port && s.directory !== serverInfo.directory
     );
 
     // Add the new server
     registry.servers.push({
       id: serverInfo.id || randomBytes(8).toString('hex'),
-      port: Number(serverInfo.port),
-      pid: Number(serverInfo.pid),
+      port,
+      pid,
       directory: serverInfo.directory,
       startedAt: serverInfo.startedAt || new Date().toISOString(),
       configPath: serverInfo.configPath || null,
@@ -81,16 +92,22 @@ export class ServerRegistry {
   }
 
   /**
-   * Unregister a server by port or directory
+   * Unregister a server by port and/or directory
+   * When both are provided, matches servers with BOTH criteria (AND logic)
+   * When only one is provided, matches servers with that criteria
    */
   unregister({ port, directory }) {
     let registry = this.read();
     let initialCount = registry.servers.length;
 
-    if (port) {
+    if (port && directory) {
+      // Both specified - match servers with both port AND directory
+      registry.servers = registry.servers.filter(
+        s => !(s.port === port && s.directory === directory)
+      );
+    } else if (port) {
       registry.servers = registry.servers.filter(s => s.port !== port);
-    }
-    if (directory) {
+    } else if (directory) {
       registry.servers = registry.servers.filter(
         s => s.directory !== directory
       );
@@ -155,20 +172,14 @@ export class ServerRegistry {
 
   /**
    * Notify the menubar app that the registry changed
-   * Uses macOS DistributedNotificationCenter via osascript
+   *
+   * NOTE: The menubar app primarily uses FSEvents file watching on servers.json.
+   * This method is a placeholder for future notification mechanisms (e.g., XPC).
+   * For now, file watching provides reliable, immediate updates.
    */
   notifyMenubar() {
-    if (process.platform !== 'darwin') return;
-
-    try {
-      // Post a distributed notification that the menubar app listens for
-      execSync(
-        `osascript -e 'tell application "System Events" to post notification with name "dev.vizzly.serverChanged"'`,
-        { stdio: 'ignore', timeout: 1000 }
-      );
-    } catch (_err) {
-      // Non-fatal - menubar might not be running or osascript might fail
-    }
+    // File watching on servers.json is the primary notification mechanism.
+    // This method exists for future enhancements (XPC, etc.) but is currently a no-op.
   }
 
   /**

tests/tdd/server-registry.test.js

@@ -184,18 +184,21 @@ describe('tdd/server-registry', () => {
   });
 
   describe('findAvailablePort', () => {
+    // Use high port range (48500+) to avoid conflicts with running TDD servers
+    let testBasePort = 48500;
+
     it('returns default port when nothing is using it', async () => {
-      let port = await registry.findAvailablePort(47392);
-      assert.strictEqual(port, 47392);
+      let port = await registry.findAvailablePort(testBasePort);
+      assert.strictEqual(port, testBasePort);
     });
 
     it('skips ports registered in the registry', async () => {
-      // Register servers on 47392 and 47393 with current PID so they're not cleaned up
-      registry.register({ pid: process.pid, port: 47392, directory: '/a' });
-      registry.register({ pid: process.pid, port: 47393, directory: '/b' });
+      // Register servers on testBasePort and testBasePort+1 with current PID so they're not cleaned up
+      registry.register({ pid: process.pid, port: testBasePort, directory: '/a' });
+      registry.register({ pid: process.pid, port: testBasePort + 1, directory: '/b' });
 
-      let port = await registry.findAvailablePort(47392);
-      assert.strictEqual(port, 47394);
+      let port = await registry.findAvailablePort(testBasePort);
+      assert.strictEqual(port, testBasePort + 2);
     });
 
     it('skips ports actually in use by other processes', async () => {