[CDX-441] Add support for origin_referrer in SABR#254
Conversation
There was a problem hiding this comment.
Pull request overview
Adds originReferrer support across SABR request builders so callers can pass a client page URL through userParameters, which is then sent to the API as the origin_referrer query parameter (matching existing tracker behavior).
Changes:
- Extended
UserParametersTypeScript type withoriginReferrer. - Added
origin_referrerquery param support in Search, Autocomplete, Browse, Recommendations, and Quizzes URL builders. - Added/updated module specs to assert
origin_referreris included in generated request URLs.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/index.d.ts | Adds originReferrer to the shared UserParameters type. |
| src/modules/search.js | Appends origin_referrer to Search and Voice Search requests when provided. |
| src/modules/autocomplete.js | Appends origin_referrer to Autocomplete requests when provided. |
| src/modules/browse.js | Appends origin_referrer to Browse requests when provided. |
| src/modules/recommendations.js | Appends origin_referrer to Recommendations requests when provided. |
| src/modules/quizzes.js | Appends origin_referrer to Quizzes requests when provided. |
| spec/src/modules/search.js | Adds assertions for origin_referrer in Search and Voice Search tests. |
| spec/src/modules/autocomplete.js | Adds assertion for origin_referrer in Autocomplete tests. |
| spec/src/modules/browse.js | Adds assertion for origin_referrer in Browse tests. |
| spec/src/modules/recommendations.js | Adds assertion for origin_referrer in Recommendations tests. |
| spec/src/modules/quizzes.js | Adds assertions for origin_referrer in Quiz next-question and results tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This PR adds originReferrer support to the SABR modules (Search, Autocomplete, Browse, Recommendations, Quizzes) by passing it as the origin_referrer query parameter, consistent with the existing tracker behavior.
Inline comments: 5 discussions added
Overall Assessment:
| }); | ||
| }); | ||
|
|
||
| it('Should return a response with valid ids and origin referrer', (done) => { |
There was a problem hiding this comment.
Important Issue: The test description "Should return a response with valid ids and origin referrer" (used for getBrowseResultsForItemIds) is also reused verbatim for the getBrowseGroups test block below (line ~1462), which doesn't deal with item IDs at all. The getBrowseGroups test description should be updated to something like "Should return a response with valid groups and origin referrer" to avoid confusion when reading test output.
| expect(res).to.have.property('request').to.be.an('object'); | ||
| expect(res).to.have.property('response').to.be.an('object'); | ||
| expect(res).to.have.property('result_id').to.be.an('string'); | ||
| expect(requestedUrlParams).to.have.property('origin_referrer').to.equal(originReferrer); |
There was a problem hiding this comment.
Suggestion: Tests across all modules only verify that origin_referrer is present when originReferrer is provided. Consider adding a negative test (or at minimum one representative negative test per module) that verifies origin_referrer is not present in the URL when originReferrer is omitted from userParameters. This is especially valuable given the truthy+type guard pattern used in the implementation, as it confirms edge cases like empty strings are handled correctly.
| fetch: fetchSpy, | ||
| }); | ||
|
|
||
| browse.getBrowseResultsForItemIds(ids, null, { originReferrer }).then((res) => { |
There was a problem hiding this comment.
Suggestion: null is passed as the second argument (parameters) to getBrowseResultsForItemIds. While this likely works due to the if (parameters) guard in the implementation, it would be more idiomatic and self-documenting to pass an empty object {} instead—matching the pattern used by every other test in this PR (e.g., getBrowseResults, getBrowseFacets, etc.).
| }); | ||
|
|
||
| return quizzes.getQuizNextQuestion(validQuizId, { answers: validAnswers }).then((res) => { | ||
| return quizzes.getQuizNextQuestion(validQuizId, { answers: validAnswers, quizSessionId }).then((res) => { |
There was a problem hiding this comment.
Suggestion: Several existing quiz tests were modified to add quizSessionId to the parameters (e.g., getQuizNextQuestion with answers, getQuizResults with page/resultsPerPage/filters/section). These changes appear unrelated to the originReferrer feature being introduced and are not explained in the PR description. Adding quizSessionId to existing tests changes their test scope (they now also implicitly test session ID behavior). These changes should be explained, or kept in a separate commit/PR to keep the diff focused.
Add
originReferrersupport for SABR requestsSummary
originReferrertoUserParametersfor Search, Autocomplete, Browse, Recommendations, and Quizzes modulesorigin_referrerquery parameter in request URLs (matching the existing tracker behavior)Tests
getSearchResults,getVoiceSearchResults)getAutocompleteResults)getBrowseResults)getRecommendations)getQuizNextQuestion,getQuizResults)