Setup screenshot testing

[briananders]

Set up screenshot testing to visually compare local builds against the production site.

This PR introduces a new npm run test:screenshot command that builds the site, then uses Playwright (WebKit) to render each HTML file from the local build and briananders.com at 1280px and 400px viewports. It performs a pixel-by-pixel comparison using pixelmatch to identify visual differences, saving local, production, and diff screenshots to test/screenshots/ along with a JSON report. This helps catch visual regressions and inconsistencies.

---

 

[cursor]

Cursor Agent can help with this pull request. Just @cursor in comments and I'll start working on changes in this branch.
_{Learn more about Cursor Agents}

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[claude]

Pull Request Review: Screenshot Testing

Summary

This PR introduces visual regression testing using Playwright and pixelmatch. The implementation is solid overall, but I've identified several areas for improvement around code quality, error handling, performance, and maintainability.

---

🟢 Strengths

1. Good Test Structure: Follows existing test patterns from build.test.mjs and golden.test.mjs
2. Comprehensive Comparison: Tests multiple viewports (1280px and 400px) which covers desktop and mobile
3. Useful Artifacts: Saves local, production, and diff screenshots with JSON results
4. Proper Cleanup: Uses before and after hooks to manage browser lifecycle

---

🔴 Issues & Recommendations

1. Code Quality & Best Practices

Missing Error Recovery (test/screenshot.test.mjs:228-242)

The test catches errors per-file but continues execution. However, if a production page returns 404 or has broken content, the test will fail without clear diagnostics.

Recommendation: Add response status checking:

const response = await page.goto(productionUrl, { waitUntil: 'networkidle' });
if (!response || response.status() !== 200) {
  throw new Error(`Production page returned status ${response?.status()}`);
}

Hard-coded Timeout (test/screenshot.test.mjs:66)

await page.waitForTimeout(1000);

This arbitrary 1-second delay is fragile and slows down tests unnecessarily.

Recommendation: Replace with deterministic waiting:

// Wait for specific condition instead of arbitrary time
await page.waitForSelector('body.loaded', { state: 'attached', timeout: 5000 })
  .catch(() => {}); // Optional: gracefully handle if no .loaded class exists

Missing Viewport Height Differences (test/screenshot.test.mjs:19-22)

Both viewports have the same height (800px). This might miss issues with content that differs at different viewport heights.

Recommendation: Consider varying heights or using fullPage screenshots (which you already do, so the height might be less critical).

---

2. Potential Bugs

Race Condition in PNG Parsing (test/screenshot.test.mjs:82-89)

The loadPNG function could fail silently if parsing completes synchronously before the listener is attached (unlikely but possible).

Recommendation: This is probably fine, but for robustness:

function loadPNG(buffer) {
  return new Promise((resolve, reject) => {
    const img = new PNG();
    img.on('error', reject);
    img.on('parsed', function() { resolve(this); });
    img.parse(buffer);
  });
}

Unsafe Path Sanitization (test/screenshot.test.mjs:212)

const safePath = relativePath.replace(/[^a-zA-Z0-9]/g, '_').replace(/\.html$/, '');

This could create filename collisions. For example:

• posts/my-post.html → posts_my_post
• posts/my_post.html → posts_my_post (collision!)

Recommendation: Use a hash or preserve directory structure:

const safePath = relativePath
  .replace(/\.html$/, '')
  .replace(/\//g, '_')
  .replace(/[^a-zA-Z0-9_-]/g, '_');

Or better yet, preserve directory structure in the screenshots folder.

---

3. Performance Considerations

Serial Screenshot Processing (test/screenshot.test.mjs:194-267)

Each HTML file is processed sequentially with nested viewport loops. For a site with many pages, this will be very slow.

Recommendation: Process files in parallel with concurrency control:

import pLimit from 'p-limit';
const limit = pLimit(4); // 4 concurrent pages

const results = await Promise.all(
  htmlFiles.map(htmlFile => limit(async () => {
    // ... screenshot logic
  }))
);

This could reduce test time from O(n) to O(n/4).

Browser Context Reuse (test/screenshot.test.mjs:189)

Only one context is created for all tests, which is good. However, you could potentially reuse pages per viewport to avoid recreation overhead.

---

4. Security Concerns

Arbitrary File System Access (test/screenshot.test.mjs:43)

The test directly accesses file://${htmlFile} URLs. While this is necessary for local testing, ensure that:

1. The packageDir is properly validated
2. No symlinks or path traversal could leak sensitive files

Current Status: ✅ Looks safe - the findHtmlFiles function only walks the package directory.

Production URL Construction (test/screenshot.test.mjs:202)

const productionUrl = `${productionBaseUrl}${urlPath}`;

Current Status: ✅ Safe - urlPath is derived from filesystem paths, not user input.

---

5. Test Coverage

Missing Test Cases

The test doesn't verify:

1. What happens when build fails? The runProductionBuild in before will cause all tests to skip.
2. What happens when production site is down? Network errors aren't explicitly tested.
3. What is an acceptable diff threshold? The test collects diffs but doesn't fail on them (commented out).

Recommendations:

// Add threshold check
const ACCEPTABLE_DIFF_THRESHOLD = 0.1; // 0.1% difference
if (diffResult.diffPercentage > ACCEPTABLE_DIFF_THRESHOLD) {
  // Mark as warning, not failure
  results.push({...result, warning: 'Diff exceeds threshold'});
}

No CI Integration Guidance

The PR doesn't include CI workflow updates. Screenshots tests should probably:

• Run on a schedule (not on every commit, too slow)
• Only run when explicitly requested
• Upload diff artifacts for review

---

6. Maintainability

Magic Numbers (test/screenshot.test.mjs:107)

threshold: 0.1,

This threshold should be a named constant with documentation.

Recommendation:

// Threshold of 0.1 allows for minor anti-aliasing differences
// while catching real visual regressions
const PIXEL_DIFF_THRESHOLD = 0.1;

Missing JSDoc (test/screenshot.test.mjs:26-40, 43-58, etc.)

Helper functions lack documentation explaining their purpose and parameters.

Recommendation: Add JSDoc comments, especially for complex functions like pixelDiff.

---

🔒 Security Assessment

✅ No security vulnerabilities identified. The test:

• Doesn't execute untrusted code
• Doesn't expose sensitive data
• Properly sanitizes file paths
• Uses official, well-maintained dependencies

---

📊 Performance Impact

Build Time: The production build runs during before, which adds ~1-5 minutes.
Test Time: For ~50 HTML files × 2 viewports = ~5-10 minutes sequentially.

Recommendation: Add this to the README:

npm run test:screenshot  # Warning: This takes 5-10 minutes

---

🧪 Test Coverage Gap

The new test file isn't covered by any other tests. Consider:

1. Adding a unit test for helper functions (especially filePathToUrlPath)
2. Mock testing to verify behavior without hitting production

---

📝 Documentation Needs

1. README.md should document the new test command
2. .gitignore correctly excludes screenshot artifacts ✅
3. package.json has the new command ✅

Missing:

• When to run this test (before deploys? on schedule?)
• How to interpret diff screenshots
• What to do when diffs are found

---

✅ Final Verdict

Approval Status: ✅ Approve with recommendations

This is a valuable addition to the test suite. The issues identified are mostly nice-to-haves that can be addressed in follow-up PRs. The core functionality is sound.

Must-Fix Before Merge

None - all issues are minor improvements.

Should-Fix Soon

1. Add parallel processing for performance
2. Add documentation to README
3. Fix path collision bug in filename sanitization

Nice-to-Have

1. Configurable diff thresholds
2. Add JSDoc comments
3. CI/CD integration

---

Great work on adding visual regression testing! 🎉

Generated with Claude Code