Add optional seedItemIds to trackRecommendationClick#256
Add optional seedItemIds to trackRecommendationClick#256Sher-Bakhodirov wants to merge 2 commits into
Conversation
![constructor-claude-bedrock[bot]](https://avatars.githubusercontent.com/u/9828194?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Adds support for optionally including seedItemIds when sending trackRecommendationClick events, allowing the API to receive the seed item(s) that generated the recommendation.
Changes:
- Extend
trackRecommendationClickto acceptseedItemIds(string or string array) and send it asseed_item_idsin the POST body. - Update TypeScript declarations to include the new optional parameter.
- Add unit tests covering omission and inclusion of
seed_item_idsfor different input shapes.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/types/tracker.d.ts |
Adds seedItemIds?: string[] | string to trackRecommendationClick parameter types. |
src/modules/tracker.js |
Adds JSDoc + parameter normalization/mapping to send seed_item_ids in the request body. |
spec/src/modules/tracker.js |
Adds tests for seedItemIds behavior (omitted/array/string/invalid type). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @param {number} [parameters.numResultsPerPage] - Number of results on page | ||
| * @param {object} [parameters.analyticsTags] - Pass additional analytics data | ||
| * @param {string[]|string} [parameters.seedItemIds] - Item ID(s) used to generate recommendations | ||
| * @param {object} userParameters - Parameters relevant to the user request |
|
|
||
| expect(tracker.trackRecommendationClick({ ...requiredParameters, seedItemIds }, userParameters)).to.equal(true); | ||
| }); | ||
| it('Should return valid response and omit seed_items_id if seedItemIds is not string or array', (done) => { |
There was a problem hiding this comment.
Code Review
This PR adds an optional seedItemIds parameter to trackRecommendationView and trackRecommendationClick, with corresponding tests and TypeScript type definitions. The implementation is largely solid, but has a few issues worth addressing.
Inline comments: 5 discussions added
Overall Assessment:
| expect(tracker.trackRecommendationView(requiredParameters, { ...userParameters, userId })).to.equal(true); | ||
| }); | ||
|
|
||
| it('Should return valid response and omit seed_item_ids if no seedItemIds provided', (done) => { |
There was a problem hiding this comment.
Suggestion: There is no blank line between the previous it(...) block's closing }); and this new it(...) block. All other adjacent it(...) blocks in this file are separated by a blank line for readability. Add a blank line before each new it(...) block to match the surrounding style (this applies to both the trackRecommendationView and trackRecommendationClick test suites).
| expect(tracker.trackRecommendationView({ ...requiredParameters, seedItemIds }, userParameters)).to.equal(true); | ||
| }); | ||
| it('Should return valid response and omit seed_item_ids if seedItemIds is not string or array', (done) => { | ||
| const seedItemIds = { seedItemIds: ['123', '456'] }; |
There was a problem hiding this comment.
Suggestion: The variable name seedItemIds is assigned an object { seedItemIds: ['123', '456'] } to represent an invalid type. This is confusing because the variable name implies it holds item IDs, but it's actually an object. A clearer name like invalidSeedItemIds or seedItemIdsObject would make the intent of the test immediately obvious without needing to read the value. This applies to both the trackRecommendationView and trackRecommendationClick versions of this test.
|
|
||
| expect(tracker.trackRecommendationView({ ...requiredParameters, seedItemIds }, userParameters)).to.equal(true); | ||
| }); | ||
| it('Should return valid response and omit seed_item_ids if seedItemIds is not string or array', (done) => { |
There was a problem hiding this comment.
Important Issue: Missing test coverage for an edge case: an empty array (seedItemIds = []). The seedItemIds?.length guard in the implementation will silently drop an empty array (length is 0). There should be a test asserting that seed_item_ids is omitted when seedItemIds is [], to make this intentional behaviour explicit and prevent regressions. This applies to both trackRecommendationView and trackRecommendationClick.
No description provided.