[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.
There was a problem hiding this comment.
Code Review
This PR consistently and correctly adds originReferrer support as a origin_referrer query parameter across all SABR modules (Search, Autocomplete, Browse, Recommendations, Quizzes), following the existing pattern from the tracker module.
Inline comments: 4 discussions added
Overall Assessment:
| fetch: fetchSpy, | ||
| }); | ||
|
|
||
| search.getSearchResults(query, { section }, { originReferrer }).then((res) => { |
There was a problem hiding this comment.
Important Issue: The test only verifies the happy path. Following the repo's testing convention (as seen in other parameters like userIp, userId, segments), there should be a companion test verifying that origin_referrer is not present in the URL when originReferrer is omitted from userParameters. This ensures the falsy-guard (if (originReferrer)) works as intended and that the parameter isn't accidentally leaked. The same gap applies to all six new tests across search.js, autocomplete.js, browse.js, recommendations.js, and quizzes.js.
| fetch: fetchSpy, | ||
| }); | ||
|
|
||
| search.getVoiceSearchResults(voiceSearchQuery, { section }, { originReferrer }).then((res) => { |
There was a problem hiding this comment.
Suggestion: The getVoiceSearchResults test uses apiKey: testApiKey directly instead of spreading ...validOptions like the parallel getSearchResults test does. This inconsistency with the rest of the test suite is pre-existing for other voice search tests, but since this is a new test being added it's a good opportunity to align with the pattern used in getSearchResults for consistency.
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)