🐛 Flatten nested properties in screenshot upload payload

[Robdel12]

Summary

• When SDK clients pass { properties: { url, viewport } } as options to vizzlyScreenshot(), the client sent properties: { properties: { url } } — double-nesting the data
• The server reads properties.url (top-level) and got undefined, so screenshots.url was never populated for static site uploads
• This broke preview linking in the full-screen review UI since canLinkToPreview() needs a valid URL from comparison.current_url
• Fix: spread options.properties into the top level so url, viewport, etc. are directly accessible

Context

Server-side fallback already deployed (properties.properties?.url) but this fixes the source of the nesting bug.

Test plan

• SDK client tests pass (32/32)
• Re-run static-site CI build and verify screenshots.url is populated
• Verify preview links appear in full-screen review UI

[claude]

Code Review

I've reviewed PR #221 and the fix looks good overall. Here's my analysis:

Summary

The fix correctly addresses the double-nesting issue where { properties: { url, viewport } } was being sent as { properties: { properties: { url, viewport } } }.

Positive Aspects

1. Root cause fix: The change at src/client/index.js:210 properly flattens the structure by spreading both options and options.properties at the same level.

2. Server-side defense: The server already has unwrapProperties() in src/server/handlers/tdd-handler.js:25-32 as a fallback, making this a defense-in-depth approach.

3. Existing test coverage: The integration tests in tests/sdk/client.test.js verify the properties are sent correctly (line 377).

Issues & Concerns

1. Missing Test for Nested Properties Case

The current test at line 377 only tests flat options ({ browser: 'chrome' }), but doesn't test the actual bug scenario where users pass { properties: { url, viewport } }.

Recommendation: Add a test case like:

it('flattens nested properties object', async () => {
  await vizzlyScreenshot('test', Buffer.from('data'), {
    properties: {
      url: 'https://example.com',
      viewport: '1920x1080'
    },
    browser: 'chrome'
  });
  
  assert.strictEqual(requests.length, 1);
  assert.strictEqual(requests[0].body.properties.url, 'https://example.com');
  assert.strictEqual(requests[0].body.properties.viewport, '1920x1080');
  assert.strictEqual(requests[0].body.properties.browser, 'chrome');
  // Verify no double-nesting
  assert.strictEqual(requests[0].body.properties.properties, undefined);
});

2. Potential Property Collision

The spread order { ...options, ...options.properties } means if a user passes:

{ url: 'A', properties: { url: 'B' } }

The nested url: 'B' will override the top-level url: 'A'. This might be intentional, but should be documented or validated.

Question: Should we warn/error if there are conflicting properties, or is this override behavior intended?

3. Documentation Gap

The JSDoc at line 316 documents options.properties but doesn't explain the flattening behavior. Users might be confused about why both work:

vizzlyScreenshot('test', img, { url: 'foo' })           // Direct
vizzlyScreenshot('test', img, { properties: { url: 'foo' } }) // Nested

Recommendation: Update the JSDoc to clarify this behavior.

4. Static Site Client Impact

The static-site client at clients/static-site/src/screenshot.js:106 sets properties.url directly, which is exactly the pattern this fix addresses. Verify that:

• The static-site CI tests now populate screenshots.url correctly
• Preview links work in the full-screen review UI

Test Plan Completeness

The PR description mentions:

• ✅ SDK client tests pass (32/32)
• ⬜ Re-run static-site CI build and verify screenshots.url is populated
• ⬜ Verify preview links appear in full-screen review UI

Recommendation: Complete the remaining test plan items before merging.

Security Considerations

No security concerns. The sanitizeScreenshotProperties() function in src/utils/security.js:213-230 already handles viewport validation.

Performance

No performance impact. The spread operation is negligible.

Code Quality

• Clean, minimal change
• Follows existing patterns
• No linting issues expected

Verdict

✅ Approve with recommendations: The core fix is correct, but please add the test case for nested properties and verify the static-site integration works as expected.

[vizzly-testing]

Vizzly - Visual Test Results

CLI Reporter - 1 change needs review

Status	Count
Passed	18
Changed	1
Auto-approved	18

Changes needing review (1)

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

Review changes

CLI TUI - Approved

5 comparisons, no changes detected.

View build

---

_{fix/flatten-screenshot-properties · 35e76e16}

[Robdel12]

Closing in favor of a proper fix that addresses the root cause in the client SDK. Instead of patching the server to handle double-nested properties, the client now properly separates SDK options (fullPage, threshold) from user properties before sending, so properties is always a flat object.