♻️ Functional decomposition: build-manager tests + auth-service refactor

[Robdel12]

Summary

Progress on the functional decomposition roadmap (#128):

• build-manager tests: Removed all vi.mock calls - tests now exercise real behavior with 42 tests passing
• auth-service: Full refactor to functional decomposition with new src/auth/ module

Changes

build-manager.spec.js

• Removed vi.mock for crypto (was mocking UUID generation)
• Removed vi.mock for VizzlyError (unnecessary)
• Tests now use real implementations
• Net reduction of 64 lines

New src/auth/ module

• core.js - Pure functions for headers, payloads, validation, error parsing
• client.js - HTTP client factory with dependency injection
• operations.js - Auth operations taking httpClient and tokenStore as parameters
• index.js - Public exports

auth-service.js

• Now a thin wrapper (~110 lines) delegating to functional module
• Maintains backwards compatibility for existing consumers

New tests in tests/auth/

• core.spec.js - 29 tests for pure functions
• operations.spec.js - 17 tests using in-memory token store and mock HTTP client
• test-helpers.js - Reusable createInMemoryTokenStore() and createMockHttpClient()
• Zero vi.mock usage

Test plan

• All 987 tests pass
• Lint passes
• Existing auth-service tests still pass (backwards compat)

[claude]

Code Review: Functional Decomposition - build-manager tests + auth-service refactor

Excellent work on progressing the functional decomposition roadmap! This PR demonstrates strong architectural improvements with clean separation of concerns. Here's my detailed feedback:

---

✅ Strengths

1. Excellent Test Quality Improvements

• build-manager.spec.js: Removing vi.mock calls is a huge win. Tests now exercise real behavior, making them more reliable and maintainable.
• Zero mocking in auth tests: The new tests/auth/ suite uses dependency injection beautifully - no module mocks needed.
• Reusable test helpers: createInMemoryTokenStore() and createMockHttpClient() are clean, well-documented utilities that make tests readable.

2. Clean Functional Architecture

The new src/auth/ module is well-structured:

• core.js: Pure functions - easy to test, reason about, and reuse
• client.js: HTTP client factory with clear DI boundaries
• operations.js: Composable auth operations
• index.js: Clean public API

3. Backwards Compatibility

The AuthService wrapper maintains the existing API perfectly. This allows gradual migration without breaking existing consumers - smart pragmatic approach.

4. Strong Documentation

• JSDoc comments throughout
• Clear file-level descriptions explaining purpose
• Inline comments where logic needs clarification

---

🔍 Code Quality Observations

Minor: Error Handling in client.js

In parseResponseBody() (lines 19-29), the try-catch silently swallows parsing errors:

} catch {
  return response.statusText || '';
}

Suggestion: Consider logging the error for debugging:

} catch (err) {
  // Parsing failed, fallback to statusText
  return response.statusText || '';
}

This helps diagnose issues if the API returns malformed responses.

Minor: Pattern Matching in createMockHttpClient()

The findResponse() function (test-helpers.js:40-60) uses loose pattern matching:

if (endpoint.includes(pattern) || pattern.includes(endpoint)) {
  return responses[pattern];
}

This could match unintentionally. For example, /api/auth would match both /api/auth/login and /api/auth/logout.

Suggestion: Document this behavior or use more explicit matching (regex or exact keys).

---

🧪 Test Coverage

Great coverage on:

• Core pure functions (29 tests in core.spec.js)
• Operations with DI (17 tests in operations.spec.js)
• Build manager lifecycle (42 tests)

Consider adding:

1. Edge case: What happens if fetch() throws (network error, DNS failure)?
2. Concurrency: Multiple simultaneous refresh() calls - are tokens corrupted?
3. Token expiration: Test the expiresAt field is actually used somewhere (or document if it's for future use)

---

🔒 Security

✅ No security concerns identified

• Tokens properly cleared on logout
• Bearer token format correct
• No token leakage in error messages
• User-Agent header appropriate

---

⚡ Performance

✅ No performance issues

• Pure functions are inherently fast
• No unnecessary object allocations
• HTTP client reuse is efficient

Minor observation: buildRequestHeaders() creates new objects each call. This is fine for auth (infrequent calls), but if used in hot paths, consider object pooling.

---

📋 Suggestions

1. Consider Exporting Test Helpers

The test-helpers.js utilities are so good they could be useful for other teams testing against Vizzly. Consider:

• Moving to src/testing/ for public export
• Adding to package.json exports: @vizzly-testing/cli/testing

2. Auth Client Lifecycle

Currently each AuthService instance creates its own client. If multiple services need auth, consider a singleton or factory pattern to share the client.

3. TypeScript Potential

These pure functions would benefit enormously from TypeScript. The validateTokens() function's return type { valid: boolean, error: Error|null } is a great candidate for a discriminated union.

Not necessary now, but worth considering for the roadmap.

---

📊 Metrics

• Line changes: +1367 / -415 (net +952)
• Files changed: 9
• Test count: 987 (all passing per PR description)
• Mock reduction: -2 vi.mock calls in build-manager tests

---

✅ Final Verdict

Approve with minor suggestions. This PR is well-executed and moves the codebase in the right direction:

• Tests are more reliable (no module mocking)
• Code is more maintainable (pure functions, DI)
• Architecture is cleaner (separation of concerns)
• Backwards compatibility preserved

The suggestions above are optional improvements - they don't block merging.

Great work on the functional decomposition! Looking forward to seeing this pattern applied to other services. 🚀