🐛 Fix TDD comparison caching and cloud minClusterSize

[Robdel12]

Summary

• Fix TDD: still-failing screenshots aren't updated #158 - TDD daemon mode now properly updates comparison results when screenshots change. Previously, re-running tests would accumulate stale results causing fixed screenshots to still appear as failures.
• Fix Set minClusterSize on app.vizzly.dev #160 - The minClusterSize config setting is now sent to the cloud API when using vizzly run. The setting was loaded from config but wasn't being passed through to the API payload.

Changes

• Added _upsertComparison() helper method to TddService that replaces existing comparisons by ID instead of always appending
• Updated all 6 locations in compareScreenshot() and createNewBaseline() to use the new upsert method
• Added minClusterSize to runOptions in the run command

Test plan

• All existing tests pass
• Linting passes
• Manual test: Run TDD daemon, break two things, fix one, verify only remaining failure shows
• Manual test: Run vizzly run with minClusterSize in config, verify it appears in API payload

Closes #158
Closes #160

[claude]

Code Review

I've reviewed PR #161 and here's my detailed feedback:

Overall Assessment

✅ The changes are solid and address the reported issues correctly. Both fixes are straightforward and appropriate for their respective problems.

---

🐛 Fix #158 - TDD Daemon Comparison Caching

The Problem:
In daemon mode, when tests are re-run, stale comparison results accumulate because results are always appended via this.comparisons.push(result). This means fixed screenshots still show as failures.

The Solution:
Added an _upsertComparison() helper that replaces existing comparisons by ID instead of always appending. This is clean and effective.

Positive Points:

• ✅ The comparison ID generation is stable (based on signature hash from src/tdd/core/signature.js:114), so the same screenshot will always have the same ID across runs
• ✅ All 6 locations where comparisons.push() was used are correctly updated
• ✅ The method is properly bound in the constructor to preserve this context
• ✅ Good JSDoc documentation explaining the purpose

Minor Concerns:

1. Unnecessary binding: The this._upsertComparison = this._upsertComparison.bind(this) on line 263 is not necessary. The method is only called as this._upsertComparison() within the class, so it will always have the correct context. This binding would only be needed if the method were passed as a callback. Not a bug, just unnecessary.

2. Missing test coverage: There are no automated tests verifying this fix works. The test plan mentions manual testing, but an automated test would prevent regression. Consider adding a test that:

   • Runs a comparison that fails
   • Re-runs the same comparison with a passing result
   • Verifies only 1 comparison exists (not 2)

---

🐛 Fix #160 - Cloud minClusterSize Config

The Problem:
The minClusterSize setting was loaded from config but wasn't being passed to the cloud API, so cloud builds couldn't use this filtering.

The Solution:
Added minClusterSize: config.comparison.minClusterSize to the runOptions object in src/commands/run.js:239.

Positive Points:

• ✅ Simple, direct fix
• ✅ Follows the same pattern as threshold (line 238)
• ✅ The config loading already supports this field (verified in codebase)

Concerns:

• ⚠️ No test coverage: The tests/commands/run.test.js file exists but doesn't verify that config values are passed through to runOptions. Consider adding a test that verifies the minClusterSize from config appears in the final runOptions.

---

Security & Performance

• ✅ No security concerns
• ✅ No performance concerns (the upsert uses findIndex which is O(n), but comparison counts are typically small)

---

Suggestions for Improvement

1. Add automated test for TDD: still-failing screenshots aren't updated #158 fix:

// In tests/tdd/tdd-service.test.js
it('should replace existing comparison when re-running same screenshot', async () => {
  // First run - create a failed comparison
  // Second run - same screenshot now passes
  // Assert: comparisons.length === 1 and status === 'passed'
});

2. Add test for Set minClusterSize on app.vizzly.dev #160 fix:

// In tests/commands/run.test.js
it('should pass minClusterSize from config to runOptions', async () => {
  // Mock config with minClusterSize: 5
  // Verify runOptions.minClusterSize === 5
});

3. Remove unnecessary binding: Delete line 263 (this._upsertComparison = this._upsertComparison.bind(this)) since the method is never used as a detached callback.

---

Recommendation

Approve with minor suggestions. The fixes correctly solve the reported issues. The missing test coverage is the main gap, but given these are straightforward fixes with clear manual test plans, this is acceptable. However, adding automated tests would significantly improve confidence and prevent regressions.

cc @Robdel12