🗑️ Delete unused service wrappers & simplify createServices

[Robdel12]

Summary

Deleted thin wrapper classes (only used by createServices):

• api-service.js, auth-service.js, config-service.js, project-service.js
• tdd-service.js re-export (now imports from tdd/ directly)
• BuildManager class (kept pure functions)
• html-report-generator.js and report-generator/ assets

Simplified createServices() to only create:

• testRunner (EventEmitter for plugin API)
• serverManager (plugin API)

Added:

• report-generator/ module with functional operations
• Test helpers in tests/helpers/

Stats

• Net deletion: ~700 lines from services layer
• Services layer reduced from 783 → 681 lines

Migration path (future)

The testRunner and serverManager classes remain for plugin API backwards compatibility. Future work:

• Document deprecation, recommend plugins use functional modules
• Remove class wrappers in next major version

Test plan

• All 56 test files pass (1086 tests)
• CI passes
• Plugin API still works

---

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

Depends on #133

[claude]

Code Review: Delete unused service wrappers & simplify createServices

This is an excellent refactoring that significantly improves code quality and maintainability. The PR successfully achieves its goal of removing wrapper classes and promoting functional programming patterns.

✅ Strengths

1. Architectural Improvements

• Clean separation of concerns: The new report-generator/ module cleanly separates pure functions (core.js) from I/O operations (operations.js), making code easier to test and reason about
• Dependency injection done right: The operations.js functions accept dependencies as parameters, enabling easy mocking in tests without complex DI containers
• Reduced complexity: createServices() now only creates 2 services (down from many more), simplifying the architecture significantly

2. Code Quality

• Excellent test coverage: New modules have comprehensive tests (333 lines for core.spec.js, 307 for operations.spec.js)
• XSS protection: serializeForHtml() properly escapes <, >, and & characters to prevent script injection when embedding data in HTML - well tested with dedicated test cases (src/report-generator/core.js:111-116)
• Clean function design: Pure functions like buildReportDir(), validateReportData(), and validateBundlesExist() are highly testable and reusable
• Good documentation: JSDoc comments clearly explain parameters and return types

3. Testing

• Test helpers: New cli-runner.js and temp-dir.js helpers provide reusable testing utilities that reduce duplication
• Integration tests: Comprehensive CLI integration tests (cli.spec.js - 544 lines added) ensure end-to-end functionality
• Migration of existing tests: All test imports properly updated to use functional modules directly

💡 Observations & Minor Suggestions

1. Package.json changes
The package.json build script removes copy-assets step since report.css no longer exists in services. This is correct, but ensure CI builds the reporter bundle before generating reports, or users will hit the "Reporter bundles not found" error. The fallback HTML is nice, but the full React experience is better.

2. package-lock.json peer dependencies
Multiple packages got marked as "peer": true (e.g., @babel/core, browserslist, postcss, typescript, vitest). This appears to be an npm lock file optimization/cleanup rather than an intentional change. Verify this doesn't affect installation - peer dependencies should still resolve correctly, but worth double-checking in a clean install scenario.

3. Dependency injection pattern consistency
The generateReport() function in operations.js takes all deps in a single object (lines 105-110), which is great for testing. Consider documenting this pattern in CLAUDE.md as a convention for future functional modules.

4. Error handling in operations
generateReport() has good error handling (try/catch at line 122), but the error message on line 158 re-wraps the original error. This is fine, but you might lose stack traces. Consider:

output.error(`Failed to generate static report: ${error.message}`);
throw error; // Or new VizzlyError(...) if you want custom error types

5. BuildManager class removal
The BuildManager class is completely removed (good!), leaving only pure functions. However, I don't see any usages of these functions being imported elsewhere in the diff. Verify that all call sites were updated to use the functional exports instead of the class methods.

🔒 Security

✅ XSS Protection: The serializeForHtml() function properly prevents XSS by escaping critical characters before embedding JSON in HTML (core.js:111-116). Well tested.

✅ No new security concerns identified in this refactor.

⚡ Performance

The refactor is performance-neutral or slightly positive:

• Removed class instantiation overhead (minimal impact)
• Pure functions enable better tree-shaking
• No blocking operations introduced

🎯 Test Coverage

Excellent coverage of new code:

• report-generator/core.spec.js: Tests all pure functions with edge cases
• report-generator/operations.spec.js: Tests I/O operations with mocked dependencies
• integration/cli.spec.js: Comprehensive end-to-end tests for the CLI

Deleted test files properly cleaned up:

• api-service.spec.js, auth-service.spec.js removed (106, 407 lines respectively)
• build-manager.spec.js mostly removed (only 1 line remains - probably testing the pure functions)

📝 Documentation

The PR description is clear and follows the CLAUDE.md guidelines:

• ✅ Includes gitmoji prefix
• ✅ Summary section covering all changes
• ✅ Stats showing net deletion
• ✅ Migration path for future work

One suggestion: Update CLAUDE.md's "Common Development Patterns" section to document the new functional patterns (pure functions in core.js, dependency-injected operations in operations.js) as the preferred approach.

✨ Summary

This is a high-quality refactor that:

• ✅ Removes ~700 lines of wrapper code
• ✅ Improves testability with pure functions and dependency injection
• ✅ Maintains backwards compatibility for plugin API
• ✅ Has comprehensive test coverage
• ✅ Follows security best practices

Recommendation: APPROVE with the minor verification items noted above (peer dependencies, BuildManager call sites).

Great work on part 3 of the functional decomposition refactor! 🎉