[TarekAlQaddy]

• Added support for all the missing params, matching the JS SDK
• Used the builder pattern so we have a dedicated structure for search/browse load events data that we can modify without the need to overload the methods.
• Fixed quizzes failing tests

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

[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

[Alexey-Pavlov]

LGTM! Left a few comments, could you please take a look?
Also, maybe we should update README.md in 12. Instrument Behavioral Events > Search Events?

[Alexey-Pavlov]

These never set sectionName/url/analyticsTags, so those forwardings are untested

[Alexey-Pavlov]

Just a few more things. Could you please take a look? 🙏

[Alexey-Pavlov]

This example sets both setResultPage(3) and setResultOffset(20), but the API accepts only one of them.

The browse snippet gets the same 400

Suggested change

	setResultPage(3)
	setResultOffset(20)
	setResultPage(3) // or setResultOffset(20) on infinite-scroll pages: set only one of the two

[Alexey-Pavlov]

Suggested change

	setResultPage(3)
	setResultOffset(20)
	setResultPage(3) // or setResultOffset(20) on infinite-scroll pages: set only one of the two

[Alexey-Pavlov]

Can we merge master and update paths to cioand-2.46.0?

[TarekAlQaddy]

@Alexey-Pavlov done ✅

[constructor-claude-bedrock]

Code Review

This PR adds missing params (result_id, result_page, result_offset, sort_order, sort_by, selected_filters) to search/browse results load tracking events, introducing new builder-based request objects (SearchResultsLoadedData, BrowseResultsLoadedData) and fixing quiz integration tests. The implementation is generally clean and consistent with existing patterns, but there are a few issues worth addressing.

Inline comments: 7 discussions added

Overall Assessment: ⚠️ Needs Work

[constructor-claude-bedrock]

Important Issue: The README (and KDoc) comments throughout state that resultPage and resultOffset are mutually exclusive — "set only one of the two". However, neither the Builder.build() method nor the SearchResultsLoadedData class enforces this invariant. A caller can silently set both, and the API will receive two conflicting values with no warning.

Consider adding a guard in build():

fun build(): SearchResultsLoadedData {
    require(resultPage == null || resultOffset == null) {
        "Only one of resultPage or resultOffset may be set, not both."
    }
    return SearchResultsLoadedData(this)
}

[constructor-claude-bedrock]

Important Issue: Same mutual-exclusivity concern as SearchResultsLoadedData: resultPage and resultOffset can both be set without any validation. The BrowseResultsLoadedData.Builder.build() should also enforce:

fun build(): BrowseResultsLoadedData {
    require(resultPage == null || resultOffset == null) {
        "Only one of resultPage or resultOffset may be set, not both."
    }
    return BrowseResultsLoadedData(this)
}

[constructor-claude-bedrock]

Important Issue: The trackSearchResultLoadedWithAllParams test passes both resultPage = 3 and resultOffset = 20 at the same time. According to the README and documentation these are mutually exclusive. This test is validating a usage pattern that is explicitly documented as incorrect, which could be misleading. The test name also implies this is a valid "all params" combination.

Either:

1. Split into two separate tests: one for resultPage and one for resultOffset, or
2. If both-at-once is intentionally allowed at the SDK layer (with the API deciding precedence), document that clearly.

[constructor-claude-bedrock]

Important Issue: Same issue as the search counterpart — trackBrowseResultLoadedWithAllParams simultaneously sets both resultPage = 3 and resultOffset = 20, which the README documents as mutually exclusive. This should be split into separate tests.

[constructor-claude-bedrock]

Suggestion: The trackBrowseResultLoadedWithRequestBuilder test similarly sets both setResultPage(3) and setResultOffset(20) in the builder. Beyond the correctness concern, there is no test covering the browse request builder's default section name behavior (the equivalent of trackSearchResultLoadedWithRequestBuilderDefaultsSectionName). Consider adding:

@Test
fun trackBrowseResultLoadedWithRequestBuilderDefaultsSectionName() {
    ...
    val request = BrowseResultsLoadedData.build("group_id", "Movies", 10)
    ConstructorIo.trackBrowseResultsLoaded(request)
    ...
    assertEquals("Products", requestBody["section"])
    assertEquals("Not Available", requestBody["url"])
}

[constructor-claude-bedrock]

Suggestion: The positional arguments used when calling trackBrowseResultsLoadedInternal inside this method are inconsistent with the named-argument style used in the search counterpart. request.filterName and request.filterValue are passed positionally:

val completable = trackBrowseResultsLoadedInternal(
    request.filterName,
    request.filterValue,
    items = request.items?.toTypedArray(),
    ...

For readability and consistency with trackSearchResultsLoaded(request), prefer fully named arguments:

val completable = trackBrowseResultsLoadedInternal(
    filterName = request.filterName,
    filterValue = request.filterValue,
    items = request.items?.toTypedArray(),
    ...

[constructor-claude-bedrock]

Suggestion: The same hardcoded quizSessionId UUID (bc48a85d-2f45-4c91-ba3a-dcf655b33831) is duplicated across every quiz integration test that was modified. Consider extracting it to a shared constant at the top of the test class to make it easier to update and to signal that all tests share the same session:

private val TEST_QUIZ_SESSION_ID = "bc48a85d-2f45-4c91-ba3a-dcf655b33831"

This also makes it clearer whether using the same session ID across tests is intentional.

[Alexey-Pavlov]

LGTM! Thanks for working on this!