[CDX-472] Remove the limit of 100 for browse/search load events#457
[CDX-472] Remove the limit of 100 for browse/search load events#457TarekAlQaddy wants to merge 2 commits into
Conversation
![constructor-claude-bedrock[bot]](https://avatars.githubusercontent.com/u/9828194?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR removes the hardcoded 100-item limit from trackSearchResultsLoaded and trackBrowseResultsLoaded, and updates tests accordingly. The change is straightforward and the intent is clear, but it is incomplete: four other tracker methods still enforce the same slice(0, 100) limit (trackPurchase, trackRecommendationView, trackQuizResultsLoaded, and trackAgentResultView), which is inconsistent and likely unintentional.
Inline comments: 4 discussions added
Overall Assessment:
| }); | ||
| const itemIDs = [...Array(1000).keys()]; | ||
| const formattedItems = itemIDs.slice(0, 100).map((i) => ({ item_id: String(i) })); | ||
| const itemIDs = [...Array(150).keys()]; |
There was a problem hiding this comment.
Suggestion: The array size of 150 works, but it is arbitrary and does not document why 150 was chosen (it is just "greater than 100"). Consider using a named constant or at least a comment like // deliberately over the old 100-item cap to make the intent obvious to future readers, especially since the old tests used 1000 which made the intent equally clear.
|
|
||
| // Request | ||
| expect(fetchSpy).to.have.been.called; | ||
| expect(requestParams).to.have.property('items').to.deep.equal(items); |
There was a problem hiding this comment.
Suggestion: In the new trackBrowseResultsLoaded test, items is constructed as objects with item_id keys (already snake_case), and the assertion checks deep.equal(items). This works coincidentally because helpers.toSnakeCaseKeys is a no-op on already-snake_case keys. The assertion does not exercise the camelCase→snake_case transformation path. This is a minor coverage gap — not a blocker, but worth noting since the previous deleted test (which used optionalParameters) did cover more of the function's behaviour (section, result_count, result_page, etc.).
There was a problem hiding this comment.
Pull request overview
This PR removes the client-side cap of 100 items for browse and search “results loaded” tracking events, allowing the full provided item list to be sent to the API.
Changes:
- Removed the
slice(0, 100)trimming when transformingitems/item_ids/customer_idsfortrackSearchResultsLoaded. - Removed the
slice(0, 100)trimming when serializingitemsfortrackBrowseResultsLoaded. - Updated and added specs to assert that >100 items are sent through unchanged.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/modules/tracker.js |
Stops truncating item lists to 100 for search/browse result load events. |
spec/src/modules/tracker.js |
Updates tests to validate that all provided items (e.g., 150) are included in payloads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| it('Should limit number of customer_ids to 100 when using item_ids', (done) => { | ||
| it('Should send all items when more than 100 items are provided', (done) => { |
https://linear.app/constructor/issue/CDX-472/js-sdk-remove-items-limit-of-100-in-searchbrowse-load-events