[CDX-437] Add missing params to search/browse results load events

[TarekAlQaddy]

https://linear.app/constructor/issue/CDX-437/android-sdk-add-resultid-param-to-searchbrowse-results-loaded-events
Fixed quizzes failing tests

[Copilot]

Pull request overview

This PR extends the Android SDK’s tracking for search_result_load and browse_result_load behavioral events to include additional result-metadata fields (result ID, pagination, sorting, selected filters), aligning the emitted payloads with the expected analytics contract for CDX-437.

Changes:

• Added result metadata fields (result_id, result_page, result_offset, sort_order, sort_by, selected_filters) to the search/browse result-load request bodies.
• Introduced new public ConstructorIo.trackSearchResultsLoaded / trackBrowseResultsLoaded overloads that accept these additional parameters and wire them through to internal tracking.
• Added unit tests asserting the new fields are included in the outgoing request payloads.

Reviewed changes

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

File	Description
library/src/main/java/io/constructor/core/ConstructorIo.kt	Adds new overloads for search/browse results loaded tracking and forwards new metadata fields into request bodies.
library/src/main/java/io/constructor/data/model/search/SearchResultLoadRequest.kt	Extends SearchResultLoadRequestBody with new optional metadata fields.
library/src/main/java/io/constructor/data/model/browse/BrowseResultLoadRequestBody.kt	Extends BrowseResultLoadRequestBody with new optional metadata fields.
library/src/test/java/io/constructor/core/ConstructorIoTrackingTest.kt	Adds tests validating new metadata fields are present in search/browse result-load tracking payloads.

Comments suppressed due to low confidence (2)

library/src/test/java/io/constructor/core/ConstructorIoTrackingTest.kt:894

• This test sets both resultPage and resultOffset, but the public KDoc for the event says they cannot be used together. If they are mutually exclusive, split into two tests (one for page, one for offset) or assert only one is sent.

        val items = arrayOf(TrackingItem("123", null, null, null))
        val observer = ConstructorIo.trackBrowseResultsLoadedInternal("group_id", "Movies", null, items, 10, resultId = "179b8a0e-3799-4a31-be87-127b06871de2", resultPage = 3, resultOffset = 20, sortOrder = "ascending", sortBy = "price", selectedFilters = mapOf("brand" to listOf("XYZ"), "color" to listOf("black"))).test()
        observer.assertComplete()
        val request = mockServer.takeRequest()
        val requestBody = getRequestBody(request)
        val path = "/v2/behavioral_action/browse_result_load?key=copper-key&i=wacko-the-guid&ui=player-three&s=67&c=cioand-2.43.0&_dt="
        assertEquals("group_id", requestBody["filter_name"])
        assertEquals("Movies", requestBody["filter_value"])
        assertEquals("10", requestBody["result_count"])
        assertEquals("179b8a0e-3799-4a31-be87-127b06871de2", requestBody["result_id"])
        assertEquals("3", requestBody["result_page"])
        assertEquals("20", requestBody["result_offset"])
        assertEquals("ascending", requestBody["sort_order"])

library/src/main/java/io/constructor/core/ConstructorIo.kt:1884

• Same as search: KDoc says resultPage and resultOffset cannot be used together, but this is not validated anywhere and tests currently send both. Align documentation, tests, and runtime validation so callers can't accidentally send an invalid combination (if the backend treats it as invalid).

     * @param resultId The result ID of the browse response that the results came from, i.e. "179b8a0e-3799-4a31-be87-127b06871de2"
     * @param analyticsTags Additional analytics tags to pass
     * @param resultPage The current page of the browse results, i.e. 3. Cannot be used with resultOffset
     * @param resultOffset The current offset of the browse results, used on scrolling sites, i.e. 20. Cannot be used with resultPage
     * @param sortOrder The sort order of the browse results, i.e. "ascending" or "descending"

---

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

[TarekAlQaddy]

@Mudaafi unfortunately, I had to overload the methods. The pattern mentioned in the story is not used in any of the tracking methods so I don't think the SDK would be consistent if we implemented it that way in only 2 method overloads. Maybe we introduce that change in a next major version and shift all tracking to be that way instead. please let me know what you think

[constructor-claude-bedrock]

Code Review

This PR adds resultId, resultPage, resultOffset, sortOrder, sortBy, and selectedFilters parameters to search and browse results-loaded tracking events, using the established builder pattern — a solid, well-structured addition.

Inline comments: 5 discussions added

Overall Assessment: ⚠️ Needs Work

[constructor-claude-bedrock]

Suggestion: The KDoc example for trackSearchResultsLoaded(request: SearchResultsLoadedData) omits setResultOffset(...) between setResultPage(3) and setSortOrder("ascending"), even though resultOffset is one of the new parameters being introduced. The example is inconsistent with the builder test below it (which does call setResultOffset(20)). The same omission exists in the trackBrowseResultsLoaded KDoc example.

Consider adding it for completeness:

 *     setResultPage(3)
 *     setResultOffset(20)
 *     setSortOrder("ascending")

[constructor-claude-bedrock]

Important Issue: The trackSearchResultLoadedWithRequestBuilder test calls the fire-and-forget public overload trackSearchResultsLoaded(request), which internally dispatches on Schedulers.io() and swallows errors. There is no assertion that the observable completed successfully (no observer.assertComplete() call), unlike the parallel trackSearchResultLoadedWithAllParams test which does call .test() and observer.assertComplete().

Because RxSchedulersOverrideRule maps all schedulers to trampoline, the request is actually sent synchronously in tests, so takeRequest() still works. However, the test provides no explicit completion guarantee — if the internal call were to throw before firing (e.g. a NullPointerException in the builder delegation), the test would silently pass.

Consider switching to the internal method directly and calling .test() + observer.assertComplete(), consistent with the rest of the test suite:

val observer = ConstructorIo.trackSearchResultsLoadedInternal(
    term = "titanic",
    resultCount = 10,
    items = arrayOf(TrackingItem("123", null, null, null)),
    resultId = "179b8a0e-...",
    ...
).test()
observer.assertComplete()

The same issue applies to trackBrowseResultLoadedWithRequestBuilder.

[constructor-claude-bedrock]

Important Issue: Same missing assertComplete() issue as trackSearchResultLoadedWithRequestBuilder — the trackBrowseResultLoadedWithRequestBuilder test calls the fire-and-forget overload with no observable assertion. An error in the builder-to-internal delegation would go undetected. Please add a completion or error assertion consistent with the rest of the tracking test suite.

[constructor-claude-bedrock]

Suggestion: The url field defaults to "Not Available" as a magic string, matching the internal method's default. While this is consistent with the existing codebase pattern, it's worth noting that this design means the sentinel value will be serialized to the API even when the caller never explicitly set a URL. If there is ever a need to distinguish "not set" from "available but unknown", a null default would be preferable. This matches how BrowseResultsLoadedData is then passed to trackBrowseResultsLoadedInternal, which also defaults to "Not Available" — so at minimum it's harmless, but worth a conscious decision. No action required if this is intentional.

[constructor-claude-bedrock]

Suggestion: Both trackSearchResultsLoadedInternal and trackBrowseResultsLoadedInternal now have 11 parameters, with the new ones appended at the end. This is consistent with the incremental approach used elsewhere in the codebase. However, the signatures are already very long and hard to read in a single line. While not a bug, for future maintainability consider whether named parameters are enforced at all call sites (they are in the new builder overloads, which is good), and whether the addition of yet more parameters in the future should prompt extracting a request object at the internal layer as well.