[CDX-487] Add trackSearchSubmit that supports analytics tags

[TarekAlQaddy]

Added analytics tags into a new method, we started adopting the new pattern of having just one data param to the tracking method and following the builder pattern so we don't have to overload the methods whenever we add a new param to the tracking event.

[Copilot]

Pull request overview

Adds a new builder-based API for trackSearchSubmit so callers can attach request-level analytics tags (merged with default analytics tags configured at initialization) without adding more overloads.

Changes:

• Introduces SearchSubmitData builder object to encapsulate search submit tracking parameters (group + analytics tags).
• Adds ConstructorIo.trackSearchSubmit(request: SearchSubmitData) and extends internal tracking to append merged analytics tags as query params.
• Updates README and unit tests to reflect analytics tags behavior and the new builder usage.

Reviewed changes

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

Show a summary per file

File	Description
README.md	Documents using SearchSubmitData.build(...) to send search-submit analytics tags.
library/src/test/java/io/constructor/core/ConstructorIoTrackingTest.kt	Updates existing search-submit request assertions and adds tests for analytics tags + builder-based request.
library/src/test/java/io/constructor/core/ConstructorioTestCellTest.kt	Updates expected search-submit URL to include default analytics tags.
library/src/test/java/io/constructor/core/ConstructorioSegmentsTest.kt	Updates expected search-submit URL to include default analytics tags.
library/src/main/java/io/constructor/data/builder/SearchSubmitData.kt	Adds new builder class for search submit tracking parameters.
library/src/main/java/io/constructor/core/ConstructorIo.kt	Adds new trackSearchSubmit(SearchSubmitData) overload and appends merged analytics tags into encoded query params.
library/src/main/java/io/constructor/core/Constants.kt	Adds analytics_tags[%s] query constant for per-tag parameterization.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[HHHindawy]

This LGTM!

[constructor-claude-bedrock]

Code Review

This PR adds a trackSearchSubmit(request: SearchSubmitTrackingData) overload following the established builder pattern, wiring in analytics tags support via trackSearchSubmitInternal. The implementation is consistent with the rest of the codebase and mostly correct, but there are a few issues worth addressing.

Inline comments: 5 discussions added

Overall Assessment: ⚠️ Needs Work

[constructor-claude-bedrock]

Suggestion: The primary constructor is public (the default), which means callers can construct SearchSubmitTrackingData directly, bypassing the builder entirely:

// This compiles and bypasses the builder:
SearchSubmitTrackingData("term", "query", null, mapOf("k" to "v"))

If the intent is to enforce usage through the builder DSL, consider making the primary constructor internal or private. If direct construction is intentional (e.g. for testing or for callers who don't need the DSL), that's fine as-is, but it should be a deliberate choice. Other fetch-side builders in this codebase (e.g. SearchRequest) are similar, so this is consistent if intentional.

[constructor-claude-bedrock]

Important Issue: The test asserts a specific URL parameter order that relies on the iteration order of mergeAnalyticsTags. The merge is defaultAnalyticsTags + analyticsTags, which in Kotlin produces a LinkedHashMap preserving the order of the left map, with collisions resolved to the right-map value but keeping the key in the left map's position. So appVersion appears first (from defaults), then appPlatform, then the new test key. This works today because the mock returns mapOf("appVersion" to "123", "appPlatform" to "Android") in that order.

However, relying on query-parameter ordering for correctness is brittle — HTTP servers are not required to treat parameter order as significant, and a future change to the test setup's defaultAnalyticsTags declaration order would silently break this test. Consider asserting on parsed query parameters individually rather than a full string prefix, similar to how body-based tests use assertEquals(requestBody["analytics_tags"], ...). Alternatively, use assertThat(path).contains("analytics_tags%5BappVersion%5D=150") etc. for each expected tag independently.

[constructor-claude-bedrock]

Important Issue: There is no error path test for the new public trackSearchSubmit(request: SearchSubmitTrackingData) overload. The existing trackSearchSubmit500 and trackSearchSubmitTimeout tests only exercise trackSearchSubmitInternal directly. While the internal paths are covered, it would be valuable to have at least one test that calls the public API via the builder and verifies the error is logged (or that the completable fails) — consistent with how other public tracking methods in this file are tested. This validates the full subscription wiring in the new overload.

[constructor-claude-bedrock]

Suggestion: This constant uses a %s format placeholder, making it a format string rather than a plain constant. Consider documenting this expectation with a brief KDoc comment (e.g., /** Format string; supply the tag key as the argument, e.g. ANALYTICS_TAGS.format("myKey") */) to make the intended usage clear to future maintainers. This is especially important since the pattern differs from all other constants in this object, which are plain strings.

[HHHindawy]

LGTM!