🐛 Fix double-nested properties in client screenshot payload

[Robdel12]

Summary

• The client SDK was sending the entire options object as properties, causing double-nesting when SDKs passed { properties: { url } } — the server received properties: { properties: { url } } making url unreachable
• Now destructures SDK-internal options (fullPage, threshold) out and flattens options.properties into the top-level properties object
• Adds tests verifying properties flattening and that SDK options don't leak into properties
• Closes 🐛 Flatten nested properties in screenshot upload payload #221 — this fixes the root cause instead of patching around it server-side

Context

After the URL capture change (#220), Storybook and Static Site SDKs passed { properties: { url: page.url() } } to vizzlyScreenshot(). The client blindly forwarded the whole options object as properties, double-nesting the data. A server-side fallback (properties.properties?.url) was deployed as a stopgap — this fix makes that fallback unnecessary.

Test plan

• npm test — 1894 pass, 0 fail
• npm run lint — clean
• New test: verifies { properties: { url } } flattens to { url } in the payload
• New test: verifies fullPage and threshold are excluded from properties
• Existing integration tests still pass (no breaking changes)

[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.5% diff

Review changes

CLI TUI - Approved

5 comparisons, no changes detected.

View build

---

_{fix/flatten-client-properties · f4a18a0d}

[claude]

Code Review

Summary

This is a clean fix that properly addresses the double-nesting bug introduced when SDKs started passing { properties: { url } } in PR #220. The implementation correctly flattens user properties while filtering out SDK-internal options.

---

✅ Strengths

1. Root Cause Fix

• Solves the issue at the source (client) rather than working around it server-side
• Makes the server-side fallback (properties.properties?.url) unnecessary
• Clear improvement over patching symptoms

2. Clean Implementation
The destructuring approach is elegant:

let { fullPage, threshold, properties: userProperties, ...rest } = options;
properties: { ...rest, ...userProperties }

• fullPage and threshold are explicitly excluded from properties
• User's properties are flattened to top-level
• Remaining options (like browser, viewport) are preserved via ...rest
• Order matters: userProperties spread last allows users to override

3. Excellent Test Coverage
Two new tests cover the critical scenarios:

• Flattening nested properties: { url } → { url }
• Filtering SDK options (fullPage, threshold) from properties payload
• Tests verify both positive (what's included) and negative (what's excluded) cases

4. Backward Compatible

• SDKs not passing properties will work as before (empty object spread is safe)
• SDKs passing top-level options like { browser: 'chrome' } still work via ...rest
• No breaking changes to the public API

---

🔍 Observations

1. Option Precedence is Correct
The spread order { ...rest, ...userProperties } means:

• User's properties.browser overrides top-level options.browser
• This is the right choice — explicit nested properties should win
• Test case confirms: browser: 'firefox' appears in final properties alongside url

2. Edge Case Handling
The code gracefully handles:

• userProperties being undefined (spreading undefined is safe in JS)
• Empty properties: {} (spreads to nothing)
• Missing fullPage/threshold (destructuring with undefined is fine)

3. Potential Future Consideration
If more SDK-internal options are added later (e.g., retries, timeout), they'd need to be explicitly destructured out. Consider documenting which options are "SDK-internal" vs "user properties" to guide future maintainers. Not urgent, but worth noting.

---

🧪 Test Quality

Test: "flattens nested properties"

• ✅ Verifies the core fix
• ✅ Uses realistic data (URL string)
• ✅ Checks deep equality of result

Test: "excludes SDK options from properties"

• ✅ Tests the filtering behavior
• ✅ Verifies fullPage is still sent at top-level
• ✅ Confirms threshold doesn't leak into properties
• ✅ Tests the merge behavior (browser + url both present)

Possible Enhancement (not blocking):
A test case for option precedence would be valuable:

await vizzlyScreenshot('test', buffer, {
  browser: 'chrome',
  properties: { browser: 'firefox', url: 'http://example.com' }
});
// Expected: properties.browser === 'firefox' (nested wins)

---

🎯 Overall Assessment

Code Quality: ⭐⭐⭐⭐⭐

• Clean, idiomatic JavaScript
• Follows established patterns in the codebase
• No code smells or anti-patterns

Bug Fix Correctness: ⭐⭐⭐⭐⭐

• Addresses the root cause
• Handles all documented scenarios
• No regression risk identified

Test Coverage: ⭐⭐⭐⭐⭐

• Critical paths tested
• Edge cases covered
• Clear, maintainable test code

Documentation: ⭐⭐⭐⭐

• PR description clearly explains the problem and solution
• Links to related issues and PRs
• Test plan is thorough
• (Minor: inline code comment about spread order could help future maintainers)

---

✅ Recommendation

Approve and merge. This is a well-implemented fix with strong test coverage. The approach is sound, the code is clean, and there are no identified risks. The fix properly resolves issue #221 and eliminates the need for server-side workarounds.

No blocking issues found.