♻️ Refactor ApiService into functional decomposition

[Robdel12]

Summary

Extracts API logic from monolithic ApiService class into functional modules for better testability and maintainability.

New Architecture

src/api/
├── core.js          # Pure functions (headers, SHA, error parsing)
├── client.js        # Client factory with token refresh
├── endpoints.js     # API endpoint functions
└── index.js         # Public exports

Key Changes

• 55 new tests with ZERO vi.mock - pure input/output assertions and dependency injection
• Functional API pattern: getBuild(client, id) instead of api.getBuild(id)
• ApiService preserved as wrapper for backwards compatibility
• Migrated internal consumers: server-manager, test-runner, uploader, tdd-service, api-handler

Testing Approach

File	Mocks	Pattern
tests/api/core.spec.js	Zero	Pure function I/O
tests/api/client.spec.js	Zero	Real factory behavior
tests/api/endpoints.spec.js	Zero	Stub client passed in
tests/server/handlers/api-handler.spec.js	Zero	Dependency injection

Stats

• Net reduction: -1114 lines (removed mock-heavy legacy tests)
• All 936 tests passing

Test plan

• npm run lint passes
• npm run build passes
• npm test - all 936 tests pass
• New API tests run without mocks

[claude]

Code Review

This is an excellent refactoring that successfully decomposes the monolithic ApiService into a functional, testable architecture. The PR demonstrates strong software engineering practices with significant improvements to testability and maintainability.

✅ Strengths

Architecture & Design

• Excellent separation of concerns: Pure functions in core.js, client factory in client.js, and endpoint functions in endpoints.js create a clean, modular structure
• Zero mock tests: The functional approach enables 55 new tests with pure input/output assertions, eliminating fragile vi.mock calls
• Backwards compatibility preserved: The ApiService wrapper ensures existing code continues to work while allowing gradual migration
• Dependency injection: The createApiHandler now accepts an uploadScreenshot function parameter, making it trivially testable

Code Quality

• Comprehensive JSDoc: All functions have clear documentation with parameter types and return values
• Pure functions: core.js contains side-effect-free functions that are deterministic and easy to reason about
• Good error handling: Graceful degradation in checkShas when SHA deduplication fails (lines 140-156 in endpoints.js)
• Token refresh logic: Well-implemented automatic token refresh with proper retry prevention

Testing

• High coverage: Net reduction of -1114 lines while adding 55 meaningful tests shows code consolidation
• Testable patterns: Mock client factory pattern in tests enables clean, focused unit tests
• No brittle mocks: Tests verify behavior through dependency injection rather than module mocking

🔍 Issues Found

1. CRITICAL: Mutable export in client.js

Location: src/api/client.js:24

export let DEFAULT_API_URL = 'https://app.vizzly.dev';

Issue: Using let for exports creates a mutable binding that can be reassigned, which is error-prone and makes code behavior unpredictable.

Recommendation:

export const DEFAULT_API_URL = 'https://app.vizzly.dev';

2. Minor: Inconsistent null handling

Location: src/api/core.js:189-190

export function buildScreenshotCheckObject(sha256, name, metadata = {}) {
  let meta = metadata || {};

Issue: The parameter already defaults to {}, so the || {} check is redundant. However, this actually handles the case where metadata is explicitly passed as null, which is defensive programming.

Recommendation: Either remove the redundant check or document that null is an expected input:

// Option 1: Trust the default
export function buildScreenshotCheckObject(sha256, name, metadata = {}) {
  return {
    sha256,
    name,
    browser: metadata.browser || 'chrome',
    // ...

// Option 2: Document null handling
/**
 * @param {Object|null} metadata - Screenshot metadata (null treated as {})
 */
export function buildScreenshotCheckObject(sha256, name, metadata = {}) {
  let meta = metadata || {};

3. Minor: Console.debug left in production code

Location: src/api/endpoints.js:142-143

console.debug(
  'SHA check failed, continuing without deduplication:',

Issue: While using console.debug is better than console.log, this bypasses the CLI's output system which handles log levels consistently.

Recommendation: Use the output utility:

import * as output from '../utils/output.js';
// ...
} catch (error) {
  output.debug('SHA check failed, continuing without deduplication:', error.message);

4. Minor: Empty catch block swallows errors

Location: src/api/client.js:152-154

} catch {
  return null;
}

Issue: The token refresh silently swallows all errors, making debugging difficult. While this is intentional for graceful degradation, it would be helpful to log at debug level.

Recommendation:

} catch (error) {
  // Token refresh failed, fall back to normal auth error handling
  // Don't log at error level as this is expected when offline/refresh token expired
  return null;
}

Or use the output system if available in this scope.

💡 Suggestions

1. Consider TypeScript or JSDoc types for better IDE support

The JSDoc is excellent, but consider using more specific types:

/**
 * @typedef {Object} ApiClient
 * @property {(endpoint: string, options?: Object, isRetry?: boolean) => Promise<Object>} request
 * @property {() => string} getBaseUrl
 * @property {() => string|null} getToken
 * @property {() => string} getUserAgent
 */

2. Export error classes from index.js

For consistency, consider re-exporting error parsing utilities from api/index.js so consumers have a single import source.

3. Add integration tests

The PR mentions "Integration tests for actual HTTP calls are in tests/api/integration/" but I don't see those files in the diff. Consider adding a few smoke tests against a test server or using MSW (Mock Service Worker) for HTTP-level integration testing.

🔒 Security Review

✅ No security issues found

• Proper Bearer token handling with Authorization header
• SHA256 hashing uses Node's crypto module correctly
• No command injection risks (all data properly encoded)
• URL construction properly handles trailing slashes and special characters
• Error messages don't leak sensitive information

⚡ Performance Considerations

✅ Good performance practices

• SHA deduplication reduces bandwidth
• Batch operations supported (getBatchHotspots)
• Token refresh prevents unnecessary re-authentication
• Base64 encoding is unavoidable for image transport

Minor suggestion: Consider streaming for very large screenshots instead of loading entire buffers, though this is likely not a practical concern for screenshot sizes.

📊 Test Coverage

The PR claims all 936 tests pass with a net reduction of 1114 lines. This is impressive - reducing code while improving testability is the hallmark of good refactoring.

Recommendation: Ensure CI is green before merging. The test coverage thresholds (75% lines/functions, 70% branches per CLAUDE.md) should be maintained.

✅ Alignment with CLAUDE.md

• ✅ Uses gitmoji in PR title (♻️ refactor)
• ✅ Comprehensive summary explaining the why and what
• ✅ Service-oriented design philosophy maintained
• ✅ Functional programming approach aligns with testability goals
• ✅ No AI attribution (correctly omitted per guidelines)

Final Verdict

APPROVE with minor fixes

This refactoring is well-executed and ready to merge after addressing the critical issue:

Required before merge:

1. Change export let DEFAULT_API_URL to export const DEFAULT_API_URL

Recommended (non-blocking):

1. Replace console.debug with output utility
2. Add comment explaining error swallowing in catch blocks
3. Consider removing redundant || {} in buildScreenshotCheckObject or documenting null handling

Excellent work on this refactoring! The functional decomposition significantly improves code quality and maintainability. 🎉