♻️ Refactor ConfigService into functional decomposition

[Robdel12]

Summary

• Extract pure functions from ConfigService into src/config/core.js (deepMerge, serialization, validation, config building)
• Create src/config/operations.js with dependency-injected I/O operations (getConfig, updateConfig, etc.)
• Add 91 new tests using simple input/output assertions instead of vi.mock
• Keep ConfigService class as thin backwards-compatible wrapper, marked @deprecated

This follows the established pattern from #125 (TddService), #127 (ApiService), and #129 (auth-service).

Test plan

• All 1078 existing tests pass
• 91 new tests for config module (57 core + 34 operations)
• No vi.mock in new tests
• Build succeeds
• Lint passes

[claude]

Code Review - PR #130

This is an excellent refactoring that follows established patterns from previous PRs. The functional decomposition is well-executed with clean separation of concerns. Here is my detailed feedback:

✅ Strengths

Architecture & Design:

• Clean separation between pure functions (core.js) and I/O operations (operations.js)
• Dependency injection pattern makes testing trivial without mocks
• Excellent backwards compatibility via deprecated ConfigService wrapper
• Consistent with established pattern from PRs ♻️ Refactor TddService into functional decomposition #125, ♻️ Refactor ApiService into functional decomposition #127, ♻️ Functional decomposition: build-manager tests + auth-service refactor #129

Code Quality:

• 91 new tests with zero vi.mock usage - exactly as intended
• Comprehensive test helpers that are reusable and well-documented
• Clear JSDoc comments throughout
• Well-organized with logical section separators

Testing:

• Tests use simple input/output assertions
• In-memory implementations avoid filesystem/network dependencies
• Test helpers (createMockExplorer, createInMemoryFs, etc.) are clean and focused
• Good coverage of edge cases (null/undefined, nested objects, arrays, etc.)

🔍 Issues & Concerns

1. Potential Bug in deepMerge with Prototype Pollution ⚠️

Location: src/config/core.js:93

for (let key in source) {  // Uses for...in which iterates inherited properties

Issue: Using for...in without hasOwnProperty check could lead to prototype pollution if malicious config includes __proto__ or constructor keys.

Recommendation:

for (let key in source) {
  if (\!Object.hasOwnProperty.call(source, key)) continue;
  // ... rest of logic
}

Or safer:

for (let key of Object.keys(source)) {
  // ... logic
}

2. Missing Validation in buildMergedConfig

Location: src/config/core.js:128-166

The function does not validate that inputs are actually objects. If someone passes non-object values, it could cause runtime errors.

Recommendation: Add basic type checking at the start:

export function buildMergedConfig({
  projectConfig = {},
  globalConfig = {},
  envOverrides = {},
}) {
  // Ensure all inputs are objects
  if (typeof projectConfig \!== 'object' || projectConfig === null) {
    projectConfig = {};
  }
  // ... similar for globalConfig and envOverrides

3. Error Handling in writeProjectConfigFile

Location: src/config/operations.js:196-202

let pkgContent = await readFile(filepath);
let pkg = JSON.parse(pkgContent);

Issue: No try-catch for JSON.parse. If package.json is malformed, this throws an unhelpful error.

Recommendation:

try {
  let pkg = JSON.parse(pkgContent);
  pkg.vizzly = config;
  await writeFile(filepath, JSON.stringify(pkg, null, 2));
} catch (error) {
  throw new VizzlyError(
    `Failed to update package.json: ` + error.message,
    'INVALID_PACKAGE_JSON'
  );
}

4. Missing Edge Case Test for Circular References

The deepMerge function could stack overflow on circular references. While config files should not have these, it would be good to document this limitation or add protection.

Recommendation: Add a test documenting expected behavior or add cycle detection.

5. Inconsistent Return Types in serializeConfig

Location: src/config/core.js:358-380

The function returns either { content, format } or { error }, which makes it harder to use safely.

Recommendation: Consider always returning the same shape:

return { content, format, error: null };
// or
return { content: null, format: null, error };

This prevents destructuring errors.

💡 Suggestions

Performance Consideration

Location: src/config/core.js:137-140

for (let key of Object.keys(CONFIG_DEFAULTS)) {
  mergedConfig[key] = CONFIG_DEFAULTS[key];
  sources[key] = 'default';
}

For nested objects like CONFIG_DEFAULTS.server, this creates a shallow copy. If defaults are mutated elsewhere, it could affect the merged config. Consider deep cloning defaults:

mergedConfig[key] = typeof CONFIG_DEFAULTS[key] === 'object' 
  ? { ...CONFIG_DEFAULTS[key] } 
  : CONFIG_DEFAULTS[key];

Documentation

The JSDoc is good, but consider adding examples for complex functions like buildMergedConfig showing the layering behavior.

Test Coverage

While coverage is excellent, consider adding:

• Test for very deeply nested config objects (5+ levels)
• Test for config with 100+ keys (performance test)
• Test for concurrent updates to global config

🎯 Summary

This is high-quality work that maintains consistency with the codebase's evolution. The main concern is the potential security issue with deepMerge and prototype pollution - this should be addressed before merging.

Recommendation: Approve with changes for the prototype pollution fix, consider the other issues as nice-to-haves.

Priority fixes:

1. Prototype pollution in deepMerge (security issue)
2. JSON.parse error handling (reliability)
3. Input validation in buildMergedConfig (robustness)

Great work overall! The functional decomposition pattern is proving very effective for this codebase.