🐛 Support custom signature properties for TDD baseline variants

[Robdel12]

Summary

When downloading baselines from the cloud, use the project's baseline_signature_properties to generate unique filenames for each baseline variant. This fixes an issue where baseline screenshots with the same name but different custom properties would overwrite each other on disk.

Important Distinction: Variants vs Baseline Variants

Variants are screenshots with the same name but different properties (browser, viewport, custom properties like mobile, theme, etc.). They are used for organization and filtering in the Vizzly dashboard.

Baseline variants are the specific variants you configure in Project Settings → Baseline Signature Properties to be used for baseline matching. Only these configured properties affect which screenshots are compared against each other.

For example, if you set baseline_signature_properties: ['mobile']:

• Screenshots with mobile: true will only match baselines with mobile: true
• Screenshots with mobile: false will only match baselines with mobile: false
• Other properties (like theme) are ignored for baseline matching (used for filtering only)

The Problem

// Two baseline screenshots with same name but different configured properties
{ "name": "VCombobox_Showcase", "properties": { "mobile": false } }
{ "name": "VCombobox_Showcase", "properties": { "mobile": true } }

Previously both would be saved as VCombobox_Showcase.png, with the second overwriting the first.

The Solution

Now with baseline_signature_properties: ['mobile'] configured, they get unique filenames:

• VCombobox_Showcase_1280_chromium_false.png
• VCombobox_Showcase_1280_chromium_true.png

Changes

• Update generateScreenshotSignature() to accept custom properties array
• Update signatureToFilename() to sanitize spaces and special characters
• Extract signatureProperties from API response during baseline download
• Store signatureProperties in baseline metadata for persistence
• Use signatureProperties consistently across all signature generation calls

Test plan

• All existing tests pass
• Download baselines from a project with baseline_signature_properties configured
• Verify baseline variants get unique filenames
• Verify TDD comparisons match the correct baseline variant

Fixes #96

[claude]

Code Review

I've reviewed this PR and it looks solid overall! The implementation correctly addresses the variant overwriting issue. Here are my findings:

✅ Strengths

1. Clean Architecture: The signature generation logic is well-abstracted and consistently applied across all code paths
2. Security: The signatureToFilename() enhancement properly sanitizes filesystem-unsafe characters, preventing potential path traversal issues
3. Backward Compatibility: Empty signatureProperties array defaults ensure existing setups continue working
4. Consistent Behavior: The API response extraction pattern is applied in both downloadBaselinesFromBuildId() and downloadBaselines()
5. Metadata Persistence: Storing signatureProperties in baseline metadata ensures TDD comparisons work correctly across sessions

🔍 Issues & Recommendations

1. Missing Unit Tests for Core Logic (High Priority)

The generateScreenshotSignature() and signatureToFilename() functions have no direct unit tests. These are critical functions that handle the core matching logic.

Recommendation: Add tests covering:

• Custom properties from different nested locations (properties.device, properties.metadata.device, properties.metadata.properties.device)
• Empty/missing custom properties
• Special characters in property values (spaces, slashes, pipes)
• Property value normalization (trimming, string conversion)

Example test structure:

describe('generateScreenshotSignature', () => {
  it('should generate signature with custom properties', () => {
    const sig = generateScreenshotSignature(
      'Login',
      { viewport_width: 1920, browser: 'chrome', mobile: false },
      ['mobile']
    );
    expect(sig).toBe('Login|1920|chrome|false');
  });

  it('should handle custom properties in nested metadata', () => {
    const sig = generateScreenshotSignature(
      'Login',
      { 
        viewport_width: 1920, 
        metadata: { properties: { device: 'iPhone 15 Pro' } }
      },
      ['device']
    );
    expect(sig).toBe('Login|1920||iPhone 15 Pro');
  });

  it('should sanitize special characters in filenames', () => {
    const filename = signatureToFilename('Login|1920|chrome|iPhone 15 Pro');
    expect(filename).toBe('Login_1920_chrome_iPhone_15_Pro');
  });
});

2. Integration Test Gap (Medium Priority)

The existing test at tdd-baseline-download.spec.js:161 only verifies signatureProperties: []. There's no test for the actual variant handling scenario described in issue #96.

Recommendation: Add an integration test:

it('should download baselines with custom signature properties', async () => {
  const mockBuildWithVariants = {
    id: 'build123',
    signatureProperties: ['mobile'],
    screenshots: [
      {
        name: 'VCombobox_Showcase',
        url: 'https://example.com/desktop.png',
        metadata: { mobile: false },
        // ... other fields
      },
      {
        name: 'VCombobox_Showcase',
        url: 'https://example.com/mobile.png',
        metadata: { mobile: true },
        // ... other fields
      }
    ]
  };

  mockApiService.getBuild.mockResolvedValueOnce(mockBuildWithVariants);
  // ... mock fetches ...

  const result = await tddService.downloadBaselines('test', 'main', 'build123');

  expect(writeFileSync).toHaveBeenCalledWith(
    join(testDir, '.vizzly', 'baselines', 'VCombobox_Showcase_false.png'),
    expect.any(Buffer)
  );
  expect(writeFileSync).toHaveBeenCalledWith(
    join(testDir, '.vizzly', 'baselines', 'VCombobox_Showcase_true.png'),
    expect.any(Buffer)
  );
});

3. API Response Validation (Low Priority)

The code extracts signatureProperties from API responses but doesn't validate the array contents.

Recommendation: Add validation:

if (
  apiResponse.signatureProperties &&
  Array.isArray(apiResponse.signatureProperties) &&
  apiResponse.signatureProperties.every(prop => typeof prop === 'string')
) {
  this.signatureProperties = apiResponse.signatureProperties;
  // ...
}

4. Edge Case: Empty String Properties (Low Priority)

When a custom property value is empty string, the signature includes it as '' after String('').trim(). This is probably fine, but worth documenting.

Current behavior:

generateScreenshotSignature('Login', { mobile: '' }, ['mobile'])
// Returns: 'Login||'  (empty string for mobile)

Recommendation: Add a comment explaining this is intentional or consider filtering empty values.

5. Filename Collision Risk (Very Low Priority)

The signatureToFilename() function could theoretically create collisions:

'A|B__C'  → 'A_B__C'  → 'A_B_C'
'A|B|C'   → 'A_B_C'   → 'A_B_C'  // collision\!

This is extremely unlikely in practice since signatures are controlled by the system, but worth noting.

📋 Test Plan Completion

The PR description mentions:

• All existing tests pass
• Download baselines from a project with baseline_signature_properties configured
• Verify variants get unique filenames
• Verify TDD comparisons match the correct baseline variant

Recommendation: Complete the manual testing or add automated tests covering these scenarios before merging.

🎯 Summary

The implementation is sound and solves the reported issue. The main gap is test coverage for the new functionality. I'd recommend adding unit tests for the signature generation functions and at least one integration test demonstrating the variant handling before merging.

Approval Status: Approve with suggestions for improved test coverage.

---

src/services/tdd-service.js:60 - Line comment: The nested property lookup is thorough and well-documented
src/services/tdd-service.js:83 - Line comment: Good security enhancement with character sanitization
src/services/tdd-service.js:207 - Line comment: Consider validating that array elements are strings