[CDX-201] Tracking other related parameters#459
Conversation
… browse, recommendations, and search
…ing options and improve handling of window parameters
![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
This PR adds an opt-in trackWindowParameters client option that, when enabled in DOM environments, falls back to specific window globals for userId, testCells, and segments across the main query modules (search/browse/autocomplete/recommendations), while ensuring the tracker module does not use those window fallbacks. It also expands test coverage for these behaviors.
Changes:
- Added
helpers.applyWindowParameterGettersto implement window-global fallbacks foruserId,testCells, andsegments. - Added
trackWindowParametersoption wiring in the client constructor, and ensured tracker uses a separate options object. - Added recommendations support for
testCellsquery params and added extensive tests for window fallback behavior across modules.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/helpers.js | Adds applyWindowParameterGetters to provide window-based fallback getters for user-related options. |
| src/types/index.d.ts | Adds trackWindowParameters?: boolean to the public TS options type. |
| src/modules/recommendations.js | Adds testCells → ef-* query param support for recommendations requests. |
| src/constructorio.js | Wires trackWindowParameters into client options and isolates tracker from window fallbacks. |
| spec/src/utils/helpers.js | Adds unit tests for applyWindowParameterGetters behavior and priority rules. |
| spec/src/modules/tracker.js | Adds tests ensuring tracker does not include window-global fallbacks, but still respects explicit option updates. |
| spec/src/modules/search.js | Adds tests verifying search requests include/exclude window globals depending on trackWindowParameters. |
| spec/src/modules/recommendations.js | Adds tests verifying recommendations requests include/exclude window globals depending on trackWindowParameters. |
| spec/src/modules/browse.js | Adds tests verifying browse requests include/exclude window globals depending on trackWindowParameters. |
| spec/src/modules/autocomplete.js | Adds tests verifying autocomplete requests include/exclude window globals depending on trackWindowParameters. |
| cspell.json | Adds cnstrc to the dictionary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| applyWindowParameterGetters: (options) => { | ||
| const backing = { | ||
| userId: options.userId || undefined, | ||
| testCells: options.testCells || undefined, | ||
| segments: options.segments || undefined, | ||
| }; |
| beaconMode: (beaconMode === false) ? false : true, // Defaults to 'true', | ||
| networkParameters: networkParameters || {}, | ||
| humanityCheckLocation: humanityCheckLocation || 'session', | ||
| trackWindowParameters: trackWindowParameters || false, |
|
|
||
| this.trackerOptions = { ...this.options }; | ||
|
|
||
| if (trackWindowParameters && helpers.canUseDOM()) { |
esezen
left a comment
esezen
left a comment
There was a problem hiding this comment.
1. Does the tracker need window params too?
Right now trackerOptions is snapshotted before applyWindowParameterGetters runs (src/constructorio.js:147), so the tracker module is deliberately excluded from window fallbacks — search/browse/autocomplete/recommendations get them, tracking events don't. Why are we doing this? I thought we wanted to include tracker in this implementation as well.
2. Renaming trackWindowParameters
The track prefix collides with the SDK's tracker vocabulary. What do you think about renaming it? Some options I thought about:
useWindowParametersuseDefaultWindowParametersfallbackToWindowParameters
… tracker, and improve safety
There was a problem hiding this comment.
Code Review
This PR adds useWindowParameters support to fall back to window.cnstrc globals for userId, testCells, and userSegments via property getters on the shared options object. The approach is creative but has a critical contradiction between the PR description and actual behavior, plus a few correctness and coverage issues.
Inline comments: 7 discussions added
Overall Assessment:
| expect(tracker.trackSessionStart()).to.equal(true); | ||
| }); | ||
|
|
||
| it('Should include window globals in tracking requests when useWindowParameters is true', (done) => { |
There was a problem hiding this comment.
Important Issue: This test contradicts the PR description which says the tracker should NOT use window globals. See the comment on applyWindowParameterGetters for the full analysis.
Additionally, this test is missing a corresponding negative test ("Should NOT include window globals in tracking requests when useWindowParameters is false"), which all four other module test files have. Consistency here would verify the feature boundary behaves correctly across all modules.
| }); | ||
| }); | ||
|
|
||
| it('Should include window global userId when useWindowParameters is true and options.userId is absent', (done) => { |
There was a problem hiding this comment.
Suggestion: The search.js tests include a "Should prefer options.userId over window global when useWindowParameters is true" priority test (around line 185), but browse.js, autocomplete.js, and recommendations.js do not have equivalent priority tests for userId, testCells, and segments. For consistency and to fully verify that explicit options take precedence over window globals in all modules, consider adding these priority tests across all four module test files — or at minimum document that the priority is exclusively tested in search.js.
| useWindowParameters: useWindowParameters === true, | ||
| }; | ||
|
|
||
| if (useWindowParameters === true) { |
There was a problem hiding this comment.
Suggestion: applyWindowParameterGetters installs Object.defineProperty getters on this.options. Since this.options is a plain object, this works fine. However, calling applyWindowParameterGetters after this.options is fully constructed (line 149) means the initial/backing values captured in the getter closures for userId, segments, and testCells are whatever was set at construction time.
Note that testCells is pre-processed through helpers.toValidTestCells(testCells) on line 137 before the getter is installed. So backingTestCells is already filtered. This is consistent, but it would help to add a brief comment explaining this ordering dependency — e.g.:
// NOTE: applyWindowParameterGetters must be called AFTER this.options is
// fully initialized so the initial backing values (e.g. toValidTestCells)
// are captured correctly.
if (useWindowParameters === true) {
helpers.applyWindowParameterGetters(this.options);
}
|
Hey @esezen! Thanks for the thoughtful review! |
This pull request introduces support for using global window variables as fallback values for
userId,testCells, anduserSegmentsin the main query modules (search,browse,autocomplete, andrecommendations) when the newtrackWindowParametersoption is enabled. It also ensures that thetrackermodule does not use these window globals, and adds comprehensive tests to validate this behavior. Additionally, a utility function for applying these window-based parameter getters is introduced and thoroughly tested.