[REM-3441] Viewable impressions tracking

[ZSnake]

Viewable impressions tracking

For context, we're introducing viewable impressions tracking for Sponsored listings to get a better outlook on wether a sponsored product was actually viewed by the end user. This PR will introduce trackResultsImpressionView to track viewable impressions

References

• RFC
• Project

[Copilot]

Pull request overview

This PR adds support for tracking “viewable impressions” for results (sponsored listings) by introducing a new trackResultsImpressionView API, corresponding request payload types, and test coverage to validate request building and error mapping.

Changes:

• Added ConstructorIO.trackResultsImpressionView(...) public tracking API.
• Added request data model(s) to POST /v2/behavioral_action/impression_view with items, optional search_term, and optional filters.
• Added README usage snippet and new unit tests for request building + worker behavior.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
README.md	Documents usage for the new results impression view tracking API.
AutocompleteClient/FW/Logic/Worker/ConstructorIO.swift	Adds public trackResultsImpressionView method that builds and executes the tracking request.
AutocompleteClient/Constants/Constants.swift	Adds a new endpoint constant for results impression view tracking.
AutocompleteClient/FW/Logic/Request/CIOResultItem.swift	Adds a result item payload model for viewable impression tracking (note: duplicated elsewhere).
AutocompleteClient/FW/Logic/Request/CIOTrackResultsImpressionViewData.swift	Adds request payload builder for impression view tracking (note: duplicated elsewhere).
AutocompleteClient/FW/Logic/Request/Builder/CIOResultItem.swift	Adds a duplicate definition of CIOResultItem under Builder.
AutocompleteClient/FW/Logic/Request/Builder/CIOTrackResultsImpressionViewData.swift	Adds a duplicate definition of CIOTrackResultsImpressionViewData under Builder.
AutocompleteClientTests/FW/Logic/Worker/ConstructorIOTrackResultsImpressionViewTests.swift	Adds worker-level tests to ensure the tracking call hits the expected endpoint and maps errors.
AutocompleteClientTests/FW/Logic/Request/TrackResultsImpressionViewRequestBuilderTests.swift	Adds request builder tests validating method/url prefix and JSON body fields.
AutocompleteClient.xcodeproj/project.pbxproj	Adds new files to the Xcode project and updates AutocompleteClientTests build settings.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Mudaafi]

Just some small comments. Mind also merging with master as well?

[Mudaafi]

Do we need this new struct? We already have CIOItem which have all these values, any reason not to use that?

[constructor-claude-bedrock]

Code Review

This PR introduces trackResultsImpressionView for viewable impression tracking of Sponsored listings and follows the established patterns well (POST body with beacon: true, CIORequestData conformance, OHHTTPStubs integration tests). A few issues around API accessibility, file placement consistency, input validation, and test coverage need addressing.

Inline comments: 7 discussions added

Overall Assessment: ⚠️ Needs Work

[constructor-claude-bedrock]

Important Issue: The stored properties are internal (let with no access modifier) but the struct is public. This means external consumers can construct a CIOResultItem but cannot read back any of its properties — e.g. they cannot inspect item.itemID after the fact. The comparable existing model CIOItem has the same pattern, but that struct is used only internally in tracking calls and its properties are never read by callers either.

For CIOResultItem, if callers will ever need to hold onto these objects and inspect them (e.g. for logging or deduplication), all properties should be public:

public struct CIOResultItem {
    public let itemID: String
    public let itemName: String?
    public let variationID: String?
    public let slCampaignID: String?
    public let slCampaignOwner: String?
}

At minimum, this should be a deliberate and documented decision.

[constructor-claude-bedrock]

Suggestion: File placement inconsistency. All other CIOTrack*Data structs and shared model types (including CIOItem) live directly in AutocompleteClient/FW/Logic/Request/, not in the Builder/ subdirectory. Both CIOResultItem.swift and CIOTrackResultsImpressionViewData.swift are placed under Builder/ which breaks the established convention.

Consider moving both files up one level to AutocompleteClient/FW/Logic/Request/ to stay consistent with the rest of the codebase (e.g. CIOItem.swift, CIOTrackPurchaseData.swift, CIOTrackMediaImpressionData.swift are all in Request/).

[constructor-claude-bedrock]

Important Issue: There is no validation against an empty items array. Calling trackResultsImpressionView(items: []) will send a POST with "items": [] which is a meaningless no-op and may be rejected by the server or silently ignored.

The precedent in CIOTrackPurchaseData shows validation is done at the data layer (it caps items at 100). Consider adding a guard or at minimum documenting the expected minimum array size:

init(items: [CIOResultItem], searchTerm: String? = nil,
     filterName: String? = nil, filterValue: String? = nil) {
    precondition(!items.isEmpty, "items must not be empty")
    // or: self.items = items.isEmpty ? items : items  // and log a warning
    ...
}

[constructor-claude-bedrock]

Suggestion: filterName and filterValue are semantically coupled — one without the other is likely a caller mistake. The API accepts them independently with no validation, which can silently produce a malformed event (e.g. a filter_name with no filter_value in the body).

Consider validating them as a pair, either via a precondition or by modelling them as a single optional tuple/struct:

// Option A: runtime warning
if (filterName == nil) != (filterValue == nil) {
    assertionFailure("filterName and filterValue should both be provided or both be nil")
}

// Option B: model as a pair
let filter: (name: String, value: String)?

[constructor-claude-bedrock]

Suggestion: This line is significantly longer than the surrounding code (exceeds 120 characters). The function declaration above it is nicely multi-line formatted; the implementation should match. Wrap the call to align with the parameter style used in the declaration:

let data = CIOTrackResultsImpressionViewData(
    items: items,
    searchTerm: searchTerm,
    filterName: filterName,
    filterValue: filterValue
)

[constructor-claude-bedrock]

Suggestion: Missing test coverage for two edge cases:

1. Absent optional fields: There's no assertion that search_term is absent from the payload in testTrackResultsImpressionViewBuilder (the base test), nor that filter_name/filter_value are absent when not provided. The worker test testTrackResultsImpressionView_WithFilters does check XCTAssertNil(payload?["search_term"]), but the builder tests don't.

2. Empty items array: No test for items: [] — even if you decide this is unsupported, a test that documents the behaviour (crash, empty body, etc.) is valuable.

Example to add:

func testTrackResultsImpressionViewBuilder_NoOptionalFieldsInPayload() {
    let items = [CIOResultItem(itemID: "item-1")]
    let tracker = CIOTrackResultsImpressionViewData(items: items)
    builder.build(trackData: tracker)
    let request = builder.getRequest()
    let payload = try? JSONSerialization.jsonObject(with: request.httpBody!, options: []) as? [String: Any]
    XCTAssertNil(payload?["search_term"])
    XCTAssertNil(payload?["filter_name"])
    XCTAssertNil(payload?["filter_value"])
}

[constructor-claude-bedrock]

Suggestion: The body payload assertions run after self.wait(for: builder.expectation), which means if capturedRequest was never set (stub didn't fire), payload is nil and all the XCTAssertEqual calls silently pass because nil as? Bool is nil and nil == nil.

The XCTAssertNotNil(payload, ...) guard helps, but a test failure there will still not tell you why the request wasn't captured. Consider adding XCTAssertNotNil(capturedRequest, "Request should have been captured") before the body assertions as a more informative guard:

self.wait(for: builder.expectation)
XCTAssertNotNil(capturedRequest, "Stub should have captured a request")
guard let body = capturedRequest?.ohhttpStubs_httpBody else { return XCTFail("No HTTP body") }
let payload = try? JSONSerialization.jsonObject(with: body, options: []) as? [String: Any]

[Mudaafi]

merging #281 instead