♻️ Refactor static site screenshot naming to use properties

[Robdel12]

Problem

The static site plugin was generating screenshot names like configuration/billing@default which failed CLI validation because forward slashes aren't allowed in screenshot names (security restriction in src/utils/security.js).

Solution

Refactored to use Vizzly's properties system for viewport metadata instead of embedding it in the name.

Before:

• Name: configuration/billing@default ❌ (fails validation)
• Properties: (empty)

After:

• Name: configuration-billing ✅ (clean, unique per page)
• Properties: { viewport: 'default', viewportWidth: 1920, viewportHeight: 1080 }

Changes

• Remove viewport from screenshot name generation
• Add generateScreenshotProperties() for viewport metadata
• Sanitize page paths (replace / and \ with -, handle ..)
• Pass properties to vizzlyScreenshot() for Vizzly grouping
• Update tests for new naming scheme

Benefits

✅ No naming conflicts with security validation
✅ Viewport info in properties for better grouping
✅ Cleaner, more semantic approach
✅ All paths work: /configuration/billing, /api/v1/users, etc.

Testing

• ✅ 76 tests passing
• ✅ 74% coverage on screenshot.js
• ✅ All edge cases covered

[claude]

PR Review: Refactor static site screenshot naming to use properties

Summary

This PR successfully refactors the static-site plugin to avoid screenshot naming conflicts with Vizzly's security validation. The approach of moving viewport information from the name into properties is architecturally sound and aligns with Vizzly's design patterns.

✅ Strengths

1. Problem Resolution

• Correctly addresses the root cause: forward slashes in names like configuration/billing@default fail security validation in src/utils/security.js:39
• The solution properly leverages Vizzly's properties system, which is validated to accept custom string properties (see validateScreenshotProperties in src/utils/security.js:174-195)

2. Code Quality

• Clean separation of concerns with two focused functions: generateScreenshotName() and generateScreenshotProperties()
• Good use of pure functions that are easily testable
• Clear documentation with JSDoc comments
• Consistent code style with the rest of the codebase

3. Test Coverage

• Comprehensive test suite with 74% coverage on screenshot.js
• Tests cover all edge cases: root paths, nested paths, empty paths, backslashes
• Good separation of test suites for name and properties generation

🔍 Issues & Recommendations

1. Path Sanitization Gap (Medium Priority)

Issue: Line 39 in screenshot.js replaces .. with . but doesn't handle all edge cases:

cleanPath = cleanPath.replace(/\.\./g, '.');

Concerns:

• Input: ../../config → Output: ..config (still contains .. at start)
• Input: .../.../secret → Output: ...-.../secret (triple dots become valid)
• The replacement happens AFTER slash replacement, so /../ becomes -.. which then becomes -.

Recommendation: Consider more robust sanitization:

// Remove all instances of .. (not just replace)
cleanPath = cleanPath.replace(/\.{2,}/g, '');
// Or use a whitelist approach
cleanPath = cleanPath.replace(/[^a-zA-Z0-9._-]/g, '-');

Note: This is currently mitigated by sanitizeScreenshotName() in src/utils/security.js:34 which throws on .., but it's better to clean the input proactively.

2. Missing Test Coverage (Medium Priority)

Tests needed:

• Backslash handling: { path: 'foo\\bar' } should become foo-bar
• Path traversal edge cases:
  • { path: '../../etc/passwd' }
  • { path: '../../../sensitive' }
  • { path: 'normal/.../path' } (triple dots)
• Trailing slashes: { path: '/about/' }
• Special characters that might appear in URLs: { path: '/search?q=test' }, { path: '/user#settings' }

3. Property Validation Compatibility (Low Priority)

Current implementation:

return {
  viewport: viewport.name,
  viewportWidth: viewport.width,
  viewportHeight: viewport.height,
};

Observation: Looking at validateScreenshotProperties() in src/utils/security.js:153-172, it expects viewport to be an object with width/height, not a string. Your current structure passes viewport name as a string.

Impact: This works because the validator allows custom string properties (lines 183-194), but viewportWidth and viewportHeight are treated as generic properties, not as part of the viewport object.

Recommendation: Consider aligning with the validator's expected structure:

return {
  viewport: {
    width: viewport.width,
    height: viewport.height,
  },
  viewportName: viewport.name, // or use a different key
};

Or keep as-is if the flat structure better serves your use case. Just document the decision.

4. Input Validation (Low Priority)

Current code assumes:

• page.path always exists and is a string
• viewport.name, viewport.width, viewport.height always exist

Recommendation: Add defensive checks:

export function generateScreenshotName(page) {
  if (!page || typeof page.path !== 'string') {
    throw new Error('Invalid page object: must have a string path property');
  }
  // ... rest of function
}

export function generateScreenshotProperties(viewport) {
  if (!viewport || typeof viewport.name !== 'string') {
    throw new Error('Invalid viewport object');
  }
  // ... rest of function
}

🔒 Security Analysis

✅ No security concerns identified

• Path sanitization removes slashes and backslashes appropriately
• The .. handling could be improved (see issue ✨ Add npmignore file, reduce release size #1 above)
• Screenshot names will pass through sanitizeScreenshotName() validation in the SDK
• Properties are validated server-side via validateScreenshotProperties()

⚡ Performance Considerations

✅ No performance issues

• Simple string operations are efficient
• Pure functions with no I/O or async operations
• Minimal memory allocation

📊 Test Coverage Analysis

Current: 74% coverage is good, but could be improved to 85%+ by adding:

1. Edge case tests mentioned in issue ♻️ Run lint before tests #2
2. Error case tests if input validation is added (issue 🔥 Remove deprecated utilities and types #4)
3. Integration test calling captureAndSendScreenshot() to verify end-to-end flow

💡 Suggestions

Nice-to-have improvements:

1. Example documentation: Add a comment showing example transformations:

   // Examples:
   // '/about' → 'about'
   // '/blog/post-1' → 'blog-post-1'
   // '/api/v1/users' → 'api-v1-users'

2. Maximum length handling: Screenshot names could theoretically become very long with deeply nested paths. Consider truncation with a suffix:

   const MAX_LENGTH = 200;
   if (cleanPath.length > MAX_LENGTH) {
     cleanPath = cleanPath.substring(0, MAX_LENGTH - 8) + '-' + 
                 crypto.createHash('md5').update(cleanPath).digest('hex').substring(0, 7);
   }

3. Collision detection: Document what happens with paths like:

   • /foo/bar and /foo-bar (both become foo-bar)
   • Consider adding a hash suffix if uniqueness is critical

🎯 Verdict

Recommended for approval with minor improvements

This is a solid refactoring that solves the immediate problem effectively. The code is clean, well-tested, and follows the repository's patterns. The issues identified are minor and can be addressed in a follow-up PR if preferred.

Suggested action items before merge:

1. ✅ Add test for backslash handling
2. ✅ Add tests for path traversal edge cases (../../, ..., etc.)
3. 🔄 Consider improving .. sanitization (can be follow-up)
4. 🔄 Add input validation (can be follow-up)

---

Review generated by Claude Code - analyzed 2 changed files with 89 additions and 40 deletions