✨ Add auto-mode detection and build lifecycle to Storybook plugin

[Robdel12]

Summary

• Add automatic TDD/Run mode detection to Storybook plugin
• Enable direct vizzly storybook usage without vizzly run wrapper
• Display mode indicators (📍 TDD / ☁️ Cloud) and build URLs
• Clean up log output for better developer experience
• Add comprehensive test coverage for concurrency control and hooks
• Fix logger level override in plugin registration

Changes

Auto-Mode Detection

The plugin now automatically detects whether to use TDD server or cloud mode:

1. TDD mode - If .vizzly/server.json exists and server responds to health check
2. Run mode - If VIZZLY_TOKEN environment variable is set
3. Warning - If neither is available

Build Lifecycle Management

When in Run mode, the plugin now:

• Creates builds via test runner service
• Starts screenshot server automatically
• Listens for build-created event to display build URL
• Finalizes builds with execution time and success/failure status
• Properly cleans up server on completion or error

Improved Logging

• Override logger level from 'warn' to 'info' for progress visibility
• Display mode indicator at start (📍 TDD / ☁️ Cloud)
• Show build URL at start and end of process
• Clean, minimal output with emojis for visual clarity
• Remove verbose status messages

Test Coverage

Added 15 new tests across 2 test files:

• tests/concurrency.spec.js - Concurrency control logic (4 tests)
• tests/hooks.spec.js - Interaction hooks system (11 tests)

Total: 67 tests passing

Test Plan

• Run vizzly storybook with TDD server running - shows TDD mode
• Run VIZZLY_TOKEN=xxx vizzly storybook - shows Run mode with build URL
• Run without TDD server or token - shows warning
• All 67 tests pass
• Build lifecycle completes successfully in Run mode
• Logger outputs progress messages correctly

[claude]

Code Review: Auto-Mode Detection & Build Lifecycle

Excellent work on this PR! The auto-mode detection is a significant UX improvement. Here's my detailed feedback:

---

✅ Strengths

1. Architecture & Design

• Clean separation of concerns between TDD and Run mode detection
• Proper lifecycle management with build creation and finalization
• Good use of event-driven architecture (build-created event)
• Graceful degradation when cloud mode fails

2. Error Handling

• Comprehensive try-catch blocks with proper cleanup in finally
• Proper error propagation and build status reporting
• Silent failure handling for finalization errors is appropriate

3. Test Coverage

• 15 new tests covering concurrency control and hooks
• Good edge case coverage (null hooks, errors, priorities)
• Tests are well-structured and readable

4. Developer Experience

• Clear visual indicators (📍 TDD / ☁️ Cloud)
• Helpful warnings when neither mode is available
• Clean, minimal output with progress visibility

---

🔍 Issues & Concerns

1. Critical: Test Code Duplication (src/index.js:72-91)

The mapWithConcurrency function is duplicated between production code and test code. This violates DRY principles.

Issue: tests/concurrency.spec.js redefines the exact same function instead of importing it.

Recommendation:

// In tests/concurrency.spec.js
import { mapWithConcurrency } from '../src/index.js';  // Import instead of redefine

Why it matters: If the implementation changes, tests won't catch regressions unless they're testing the actual production code.

---

2. Performance: Fetch Without Timeout (src/index.js:144-146)

The health check fetch call has no timeout, which could hang indefinitely.

Current code:

let response = await fetch(`http://localhost:${serverInfo.port}/health`);
return response.ok;

Recommendation:

let controller = new AbortController();
let timeoutId = setTimeout(() => controller.abort(), 2000); // 2s timeout
try {
  let response = await fetch(
    `http://localhost:${serverInfo.port}/health`,
    { signal: controller.signal }
  );
  return response.ok;
} catch {
  return false;
} finally {
  clearTimeout(timeoutId);
}

---

3. Resource Leak: Logger Mutation (src/plugin.js:21-22)

Logger level is set to 'info' but never restored to original level.

Current code:

let originalLevel = logger.level;  // Stored but never used
logger.level = 'info';

Impact: This could affect subsequent commands in the same process.

Recommendation:

register(program, { config, logger, services }) {
  let originalLevel = logger.level;
  
  program
    .command('storybook <path>')
    // ... options ...
    .action(async (path, options) => {
      logger.level = 'info';  // Set before execution
      try {
        // ... existing code ...
      } finally {
        logger.level = originalLevel;  // Restore in finally block
      }
    });
}

---

4. Code Smell: Complex Git Detection Logic (src/index.js:222-241)

The git utils import logic is overly complex with multiple fallback paths.

Concerns:

• Hard to test and maintain
• Uses unstable import.meta.resolve (still experimental in some Node versions)
• Silent failures with triple-nested try-catch

Recommendation:
Extract to a separate utility function with clear documentation about the resolution strategy. Consider using a more explicit module resolution approach:

async function loadGitUtils() {
  // Try package first
  try {
    return await import('@vizzly-testing/cli/dist/utils/git.js');
  } catch {}
  
  // Try monorepo relative path
  try {
    return await import('../../../src/utils/git.js');
  } catch {}
  
  return null;
}

---

5. Security: Environment Variable Mutation (src/index.js:284-286)

The code mutates process.env which can affect other parts of the application.

Current code:

process.env.VIZZLY_SERVER_URL = `http://localhost:${runOptions.port}`;
process.env.VIZZLY_BUILD_ID = buildId;
process.env.VIZZLY_ENABLED = 'true';

Concerns:

• Global state mutation
• Not cleaned up on error
• Could leak between test runs

Recommendation:
Clean up environment variables in the finally block:

let envBackup = {
  url: process.env.VIZZLY_SERVER_URL,
  buildId: process.env.VIZZLY_BUILD_ID,
  enabled: process.env.VIZZLY_ENABLED,
};

try {
  process.env.VIZZLY_SERVER_URL = `http://localhost:${runOptions.port}`;
  // ...
} finally {
  // Restore original values
  if (envBackup.url !== undefined) process.env.VIZZLY_SERVER_URL = envBackup.url;
  else delete process.env.VIZZLY_SERVER_URL;
  // ... restore others
}

---

6. Race Condition: Build URL Event (src/index.js:215-220)

The build-created event listener is set up after createBuild might have already emitted the event.

Current code:

testRunner.once('build-created', buildInfo => {
  // ...
});

buildId = await testRunner.createBuild(runOptions, false);

Why it's a problem: If createBuild is synchronous or very fast, the event could fire before the listener is attached.

Recommendation:
Ensure listener is registered before calling createBuild, or handle the URL in the return value:

// Option 1: Register before calling
testRunner.once('build-created', buildInfo => { /* ... */ });
buildId = await testRunner.createBuild(runOptions, false);

// Option 2: Handle in response
let buildResult = await testRunner.createBuild(runOptions, false);
if (buildResult.url) {
  buildUrl = buildResult.url;
  logger.info(`🔗 ${buildUrl}`);
}

---

💡 Minor Suggestions

1. Type Safety (src/index.js:167-169): hasApiToken uses optional chaining but could be more explicit:

   function hasApiToken(config) {
     return Boolean(config?.apiKey || process.env.VIZZLY_TOKEN);
   }

2. Magic Numbers (src/index.js:261-262): Extract hardcoded defaults to named constants:

   const DEFAULT_PORT = 47392;
   const DEFAULT_TIMEOUT = 30000;

3. Logging Consistency (src/index.js:295-296): Mix of emoji styles. Consider standardizing:

   logger.warn('⚠️  No TDD server or API token found');
   logger.info('ℹ️  Run \`vizzly tdd start\` or set VIZZLY_TOKEN');

4. Error Context (src/index.js:343): Add more context to error messages:

   logger.error(`Failed to process stories from ${storybookPath}: ${error.message}`);

---

🧪 Test Coverage Gaps

1. Missing Integration Tests: No tests for isTddModeAvailable() or hasApiToken()
2. Mode Detection: No tests verifying the mode detection logic with various environment states
3. Cleanup Verification: No tests ensuring environment variables are cleaned up after errors
4. Build Lifecycle: No tests for the complete build creation → finalization flow

Recommended additions:

describe('mode detection', () => {
  it('should detect TDD mode when server.json exists', async () => { /* ... */ });
  it('should detect Run mode when token is present', async () => { /* ... */ });
  it('should warn when neither mode is available', async () => { /* ... */ });
});

---

📊 Overall Assessment

Rating: ⭐⭐⭐⭐ (4/5)

This is a solid PR that significantly improves the developer experience. The architecture is sound and the feature implementation is mostly clean. However, there are some issues that should be addressed:

Must Fix:

• ❗ Test code duplication (✨ Add npmignore file, reduce release size #1)
• ❗ Fetch timeout (♻️ Run lint before tests #2)
• ❗ Logger level restoration (♻️ Refactor server architecture with new handlers #3)

Should Fix:

• Race condition in event handling (✅ Add comprehensive test suite #6)
• Environment variable cleanup (♻️ Update core services and commands #5)

Nice to Have:

• Refactor git utils detection (🔥 Remove deprecated utilities and types #4)
• Add integration tests for mode detection
• Extract magic numbers to constants

---

🚀 Next Steps

1. Address the "Must Fix" issues before merging
2. Consider adding integration tests for the new mode detection logic
3. Update documentation if the race condition fix changes behavior

Great work overall! The auto-mode detection will make the Storybook plugin much more intuitive to use. 🎉