✨ Surface honeydiff analysis data in comparisons output

[Robdel12]

Summary

• Add honeydiff object to --json output for comparisons containing SSIM/GMSD scores, cluster classification/metadata, fingerprint hash, diff regions, diff lines, and fingerprint data
• Fix URL resolution across all API response shapes — handles nested objects (single comparison endpoint), flat fields like diff_image_url/baseline_original_url (build detail), and nested diff_image (search)
• Add urls and honeydiff to builds --build <id> --comparisons --json comparison details
• Show classification, SSIM, GMSD, and fingerprint in --verbose single comparison display
• Append [classification] label to comparison lines in --verbose build and search list views

Corresponds to vizzly-testing/vizzly#277 which exposes these fields from the API.

Test plan

• node --test tests/commands/comparisons.test.js — 17/17 pass
• node --test tests/commands/builds.test.js — 12/12 pass
• Honeydiff fields in JSON for single comparison (top-level fields)
• Honeydiff fields in JSON for search results (nested diff_image)
• honeydiff: null when no analysis data present
• URLs resolved from flat field names (baseline_screenshot_url, diff_url)
• URLs resolved from build detail field names (baseline_original_url, diff_image_url)
• Builds command includes URLs and honeydiff in comparisonDetails
• Verbose display shows Classification, SSIM, GMSD, Fingerprint

[claude]

Code Review

This PR successfully surfaces honeydiff analysis data in the comparisons command output. The implementation is solid with comprehensive test coverage. Here are my findings:

Strengths

1. Excellent test coverage - 163 new lines of tests covering all major scenarios including both API response shapes, null states, and verbose display
2. Clear documentation - Inline comment at src/commands/comparisons.js:189-191 explains the dual response shape pattern well
3. Consistent handling - Code handles both top-level and nested diff_image fields throughout
4. Graceful degradation - Returns honeydiff: null when no data is present rather than omitting the field or returning partial data

Issues Found

1. Potential false positive in hasHoneydiff check (src/commands/comparisons.js:198)

The current logic:

let hasHoneydiff = clusterMetadata || ssimScore \!= null || gmsdScore \!= null || fingerprintHash;

Issue: If ssimScore is exactly 0, the check ssimScore \!= null evaluates to true, but then the final || fingerprintHash could short-circuit if fingerprintHash is truthy. However, the real issue is that 0 is a valid SSIM score, so this should be fine. But there's an edge case: if ALL values are falsy but ssimScore or gmsdScore is 0 (a valid score), we'd get hasHoneydiff = false due to the final || fingerprintHash evaluation.

Fix: Add explicit null check for fingerprintHash:

let hasHoneydiff = clusterMetadata || ssimScore \!= null || gmsdScore \!= null || fingerprintHash \!= null;

Wait, actually reviewing more carefully - the current code is:

let hasHoneydiff = clusterMetadata || ssimScore \!= null || gmsdScore \!= null || fingerprintHash;

This is checking if fingerprintHash is truthy (non-empty string), which is fine since hashes should never be empty strings. The nullish checks for scores are correct. This is actually fine as-is.

2. Inconsistent handling of diff_regions field (src/commands/comparisons.js:221)

diffRegions: comparison.diff_regions || diffImage.diff_regions || null,

Issue: Uses || operator which treats [] (empty array) as falsy. If comparison.diff_regions is an empty array, it will check diffImage.diff_regions instead, which is incorrect - an empty array means "no regions" not "check elsewhere".

Fix: Use nullish coalescing for consistency:

diffRegions: comparison.diff_regions ?? diffImage.diff_regions ?? null,

3. Inconsistent operator usage for diff_lines and fingerprint_data

src/commands/comparisons.js:222-223 use || operator:

diffLines: comparison.diff_lines || null,
fingerprintData: comparison.fingerprint_data || null,

Issue: Same problem as above - empty arrays would be treated as falsy. Also, these fields don't check diffImage fallback like other fields do.

Questions:

• Should diff_lines and fingerprint_data also check nested diffImage fields for consistency?
• Should these use ?? instead of || to properly handle empty arrays?

Minor Observations

1. Fingerprint not shown when classification is missing (src/commands/comparisons.js:282)
   The condition if (clusterMetadata || ssim \!= null || gmsd \!= null) controls displaying the entire honeydiff section, but fingerprint is only displayed inside. If only fingerprint exists (no scores/classification), it won't be shown. This might be intentional, but worth confirming.

2. Consistent naming - Good use of camelCase in JSON output (ssimScore, clusterClassification) vs snake_case from API

3. toFixed(4) for scores - Good precision choice for SSIM/GMSD display

Performance & Security

• No performance concerns - all operations are simple property access
• No security issues - data is directly from API responses
• No SQL injection or XSS vectors

Recommendations

1. Fix the ?? vs || inconsistency for array fields
2. Consider whether diff_lines and fingerprint_data should check diffImage fallback
3. Optional: Add JSDoc comments to the new getClassificationLabel function describing what classifications might look like
4. Consider showing fingerprint even when scores are missing (edge case)

Test Plan Verification

Based on the PR description, all tests pass:

• ✅ Honeydiff fields in JSON for single comparison
• ✅ Honeydiff fields in JSON for search results (nested)
• ✅ honeydiff: null when no data present
• ✅ Verbose display shows Classification, SSIM, GMSD, Fingerprint

The test file shows proper coverage of edge cases.

Verdict

This is a well-implemented feature with good test coverage. The main issues are minor consistency problems with || vs ?? operators that could cause bugs with edge cases (empty arrays). I'd recommend addressing the operator consistency before merging.

The code follows repository conventions from CLAUDE.md and maintains the existing architecture patterns.

[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

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

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

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

filter-failed-only · Firefox · 375×703 · 0.3% diff

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

...and 1 more in Vizzly.

Review changes

CLI TUI - Approved

5 comparisons, no changes detected.

View build

---

_{surface-honeydiff-analysis-data · e39e8c38}