♻️ Refactor commands to use functional modules directly

[Robdel12]

Summary

• Commands now import from functional modules instead of createServices()
  • API: import { createApiClient, getBuild } from '../api/index.js'
  • Auth: import { createAuthClient, whoami } from '../auth/index.js'
  • Test runner: import { runTests } from '../test-runner/index.js'
• Added createTokenStore() helper to auth module for commands
• Commands create lightweight inline objects for serverManager/buildManager instead of using class instances

Test plan

• All tests pass
• CI passes
• Manual test: vizzly run, vizzly tdd, vizzly upload commands work

---

Part 2 of 3 for functional decomposition refactor (VIZ-128)

Depends on #132

[claude]

Code Review: Functional Refactor of Commands

This PR successfully migrates commands from the service container pattern to direct functional imports. The refactoring improves code clarity and reduces indirection. However, there are several important issues to address.

Critical Issues

1. Code Duplication: createServerManager Function

Location: src/commands/run.js:40-84 and src/commands/tdd.js

The createServerManager function is duplicated across both run.js and tdd.js. This violates DRY principles and creates maintenance burden. If the server manager interface changes, you'll need to update both files identically.

Recommendation: Extract createServerManager to src/server-manager/index.js as an exported helper, then import it in both commands.

2. Circular Dependency Risk in createTokenStore

Location: src/auth/index.js:46-70

The late-import pattern using dynamic import() to avoid circular dependencies is a code smell. Since you're already re-exporting the token functions at the top of the file (lines 11-15), just use them directly. This would be simpler, synchronous, and eliminate the circular dependency concern.

Medium Priority Issues

3. Inconsistent Error Handling in Cleanup

Location: src/commands/run.js:111-149 and src/commands/tdd.js:53-64

The cleanup functions silently swallow all errors with empty catch blocks. At minimum, log errors in verbose mode for debugging.

4. Missing Null Check in run.js Cleanup

Location: src/commands/run.js:129-144

The finalize build logic checks if (buildId && config) but doesn't validate that serverManager is available, even though it's used in the deps object.

5. Process Signal Handler Cleanup

Location: src/commands/run.js:158-159

Signal handlers are registered but never removed, which could cause memory leaks in long-running processes or test suites.

What's Good

1. Clearer dependency flow - Direct imports make it obvious where functions come from
2. Reduced abstraction - Removing the service container layer simplifies the mental model
3. Consistency - All commands now follow the same pattern
4. Better testability - Dependency injection via deps parameters makes mocking easier

Testing Concerns

The PR description shows manual testing is incomplete. Before merging, ensure all manual tests pass, especially:

• vizzly tdd start (daemon mode)
• vizzly tdd run (one-off mode)
• vizzly run with --wait (build polling)
• Error scenarios

Summary

This is a solid refactoring moving in the right direction. Main concerns:

1. Fix code duplication (critical)
2. Simplify createTokenStore (critical)
3. Improve error handling visibility (medium)
4. Complete manual testing (blocker for merge)

Recommendation: Request changes for items 1 and 2, then approve after fixes and manual testing completion.