✨ Add minClusterSize to config service

[Robdel12]

Summary

Adds minClusterSize support to the CLI config service, completing the CLI side of the settings cascade.

Changes

• Add minClusterSize: 2 to DEFAULT_CONFIG.comparison
• Add VIZZLY_MIN_CLUSTER_SIZE environment variable support
• Add validation for minClusterSize (must be positive integer ≥ 1, warning if > 100)

Related

This works with vizzly-testing/vizzly#180 to complete the full settings cascade:

• Screenshot-level → Build-level (SDK) → Project-level → System default

Test plan

• Verify config loads with default minClusterSize: 2
• Test VIZZLY_MIN_CLUSTER_SIZE env var override
• Test validation rejects invalid values (0, negative, non-integer)
• Test config file with custom minClusterSize value

[claude]

Code Review

This PR adds minClusterSize support to the CLI config service. The implementation is solid overall, with good test coverage and consistent patterns. Here are my findings:

---

✅ Strengths

1. Consistent patterns: The implementation mirrors the existing threshold validation and env var handling patterns perfectly
2. Good test coverage:
   • Unit tests for _upsertComparison method (tdd-service.test.js:346-438)
   • Integration test for passing minClusterSize through to runOptions (run.test.js:438-479)
   • Daemon mode test verifying the fix prevents stale result accumulation
3. Clear documentation: JSDoc on _upsertComparison explains the daemon mode behavior
4. Validation alignment: The validation logic (integer ≥ 1, warning at >100) follows the same structure as threshold validation

---

🔍 Missing Test Coverage

Critical gap: No validation tests for minClusterSize in tests/services/config-service.test.js

The validateConfig test suite has comprehensive coverage for threshold (lines 249-280) and other config fields, but no tests for minClusterSize validation. You should add:

1. Error case: Non-integer value (e.g., 3.5)
2. Error case: Zero or negative value (e.g., 0, -1)
3. Warning case: Very high value (e.g., 150)
4. Success case: Valid value (e.g., 5)

Secondary gap: No env var test for VIZZLY_MIN_CLUSTER_SIZE

While VIZZLY_THRESHOLD is cleaned up in beforeEach/afterEach hooks (lines 45, 52), there's no test verifying that VIZZLY_MIN_CLUSTER_SIZE actually overrides config values. Consider adding a test similar to how threshold env vars might be tested elsewhere.

---

🐛 Potential Issues

1. Missing env var cleanup

In tests/services/config-service.test.js, the beforeEach/afterEach hooks clean up VIZZLY_THRESHOLD and VIZZLY_PORT, but not VIZZLY_MIN_CLUSTER_SIZE:

beforeEach(() => {
  delete process.env.VIZZLY_THRESHOLD;
  delete process.env.VIZZLY_PORT;
  // Missing: delete process.env.VIZZLY_MIN_CLUSTER_SIZE;
});

This could cause test pollution if you add env var tests later.

2. Inconsistent source tracking

In config-service.js:108-113, when VIZZLY_MIN_CLUSTER_SIZE is set, it assigns:

sources.comparison = 'env';

This overwrites the source for the entire comparison object, not just minClusterSize. If both VIZZLY_THRESHOLD and VIZZLY_MIN_CLUSTER_SIZE are set, whichever is processed last will claim the source. This matches the existing pattern for threshold (line 106), so it's not a bug per se, but it's worth noting this is a known limitation.

---

💡 Minor Suggestions

1. Config merging test already includes minClusterSize

The test at line 352 already uses minClusterSize: 5 in the config and verifies it at line 362. Good! But consider adding a comment like:

assert.strictEqual(config.comparison.minClusterSize, 5); // From project config

This clarifies it's testing the merge, not just the value.

2. Consider edge case: minClusterSize = 1

The validation allows minClusterSize: 1, which means "every differing pixel is its own cluster." While technically valid, this might not be useful in practice. Consider whether a warning or different minimum makes sense, but this is a product decision, not a code issue.

---

📝 Test Plan Checklist

From the PR description, ensure you verify:

• ✅ Default loads correctly (covered by existing merge test at line 362)
• ❌ VIZZLY_MIN_CLUSTER_SIZE env var override (not tested)
• ❌ Validation rejects invalid values (not tested)
• ✅ Config file with custom value (covered by merge test)

---

🎯 Verdict

Approve with changes requested: The core implementation is correct and well-tested, but the validation test gap is significant given the test plan explicitly calls for validation testing. Add the missing validation tests to match the coverage of other config fields, then this is ready to merge.

---

📋 Recommended Additions

Add these tests to tests/services/config-service.test.js in the validateConfig describe block:

it('returns error for non-integer minClusterSize', async () => {
  let service = createConfigService({ workingDir: tempDir });
  let result = await service.validateConfig({
    comparison: { minClusterSize: 3.5 },
  });
  assert.strictEqual(result.valid, false);
  assert.ok(result.errors.some(e => e.includes('minClusterSize')));
});

it('returns error for zero or negative minClusterSize', async () => {
  let service = createConfigService({ workingDir: tempDir });
  let result = await service.validateConfig({
    comparison: { minClusterSize: 0 },
  });
  assert.strictEqual(result.valid, false);
  assert.ok(result.errors.some(e => e.includes('minClusterSize')));
});

it('returns warning for very high minClusterSize', async () => {
  let service = createConfigService({ workingDir: tempDir });
  let result = await service.validateConfig({
    comparison: { minClusterSize: 150 },
  });
  assert.strictEqual(result.valid, true);
  assert.ok(result.warnings.some(w => w.includes('100')));
});

And add env var cleanup:

beforeEach(() => {
  tempDir = createTempDir();
  delete process.env.VIZZLY_THRESHOLD;
  delete process.env.VIZZLY_MIN_CLUSTER_SIZE;
  delete process.env.VIZZLY_PORT;
});

afterEach(() => {
  cleanupTempDir(tempDir);
  delete process.env.VIZZLY_THRESHOLD;
  delete process.env.VIZZLY_MIN_CLUSTER_SIZE;
  delete process.env.VIZZLY_PORT;
  delete process.env.VIZZLY_HOME;
});