✨ Log screenshot events for menubar display

[Robdel12]

Summary

Each screenshot comparison now logs to server.log with:

• screenshot name
• status (passed/failed/new/etc)
• diffPercentage

This enables the menubar app to show individual screenshot results in the log viewer.

Test plan

• Start TDD server, run tests, verify server.log contains screenshot entries
• Verify menubar log viewer displays screenshot events with status icons

[claude]

Code Review

Summary

This PR adds screenshot-level logging to enable the menubar app to display individual screenshot results. The implementation is clean and follows the existing logging patterns in the codebase.

✅ Positive Aspects

1. Minimal, focused change: The PR adds exactly 7 lines in a single location, which is appropriate for this feature.
2. Correct placement: The logging occurs after the comparison is updated and before the response is returned, ensuring accurate data is logged.
3. Structured logging: Uses the existing output.info() with metadata object, consistent with the codebase's logging approach.
4. Good default handling: diffPercentage || 0 provides a sensible default for cases where diffPercentage might be undefined.

🔍 Observations & Questions

1. Status values: From analyzing the codebase, I found these possible status values:

   • 'passed' - comparison passed threshold or filtered via regions/hotspots
   • 'failed' - visual difference detected above threshold
   • 'new' - new baseline created
   • 'accepted' - screenshot accepted as baseline
   • 'rejected' - changes rejected by user
   • 'error' - comparison error occurred

   The current logging captures whatever status the comparison has. Confirm the menubar app handles all these status values appropriately.

2. Logging timing: The log is emitted after updateComparison() but before the HTTP response. This means:

   • Failed comparisons get logged (status: 'failed')
   • Passed comparisons get logged (status: 'passed')
   • New baselines get logged (status: 'new')
   • But later user actions (accept/reject) won't emit new logs

   Is this the intended behavior? Should accept/reject actions at lines 582 and 612 also emit screenshot logs?

3. Test coverage: The PR description mentions a manual test plan but there are no automated tests added. Consider adding a test case to tests/server/handlers/tdd-handler.test.js that verifies:

   • The log is emitted when a screenshot is compared
   • The log contains the correct screenshot name, status, and diffPercentage
   • The output.info is called with the expected structure

🐛 Potential Issues

Minor: Status may be 'failed' when it returns 'diff'

Looking at lines 491-497, when comparison.status === 'failed', the HTTP response returns status: 'diff' to the client. However, the log at line 487 uses comparison.status which would be 'failed'.

This creates an inconsistency:

• Log shows: status: 'failed'
• HTTP response shows: status: 'diff'

The menubar app might need to handle both 'failed' and 'diff' as equivalent, or the logging should normalize this to 'diff' for consistency with the API response.

💡 Suggestions

1. Add test coverage: Create a test that verifies the logging behavior:

it('should log screenshot events with name, status, and diffPercentage', async () => {
  let mockOutput = createMockOutput();
  // ... test implementation
  assert.ok(mockOutput.calls.some(c => 
    c.method === 'info' && 
    c.args[0].startsWith('Screenshot:') &&
    c.args[1].screenshot === 'expected-name' &&
    c.args[1].status === 'passed'
  ));
});

2. Consider logging accept/reject actions: If the menubar needs to show when a user accepts or rejects baselines, add similar logging at lines 582 and 612.

3. Clarify status mapping: Document or normalize the status value to match what the HTTP API returns (e.g., use 'diff' instead of 'failed').

🔒 Security & Performance

• ✅ No security concerns (logging doesn't expose sensitive data)
• ✅ No performance concerns (single log call per screenshot)
• ✅ Uses existing sanitized name, preventing log injection

📊 Test Plan Enhancement

Current test plan:

• Start TDD server, run tests, verify server.log contains screenshot entries
• Verify menubar log viewer displays screenshot events with status icons

Suggested additions:

• Test with screenshots that have special characters in names
• Test all status types: passed, failed, new, accepted, rejected
• Test with missing/undefined diffPercentage
• Verify log format is valid JSON (if consumed as JSON)
• Test with high volume of screenshots (performance check)

Overall Assessment

This is a solid, focused PR that accomplishes its goal. The main recommendation is to add automated test coverage and clarify the status value mapping between the comparison object and HTTP response. The implementation follows existing patterns and shouldn't cause any issues in production.

[vizzly-testing]

Vizzly - Visual Test Results

CLI Reporter - 7 changes need review

Status	Count
Passed	12
Changed	7
Auto-approved	12

Changes needing review (7)

filter-failed-only · Firefox · 1920×1080 · 0.1% diff

fullscreen-viewer · Firefox · 1920×1080 · 0.1% diff

bulk-accept-dialog · Firefox · 1920×1080 · 0.3% diff

viewer-slide-mode · Firefox · 1920×1080 · 0.5% diff

viewer-zoomed-100 · Firefox · 1920×1080 · 0.3% diff

fullscreen-viewer · Firefox · 375×667 · 100.7% diff

...and 1 more in Vizzly.

Review changes

CLI TUI - Approved

5 comparisons, no changes detected.

View build

---

_{feat/screenshot-logging · d9c3b113}