[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}
There was a problem hiding this comment.
Code Review
This PR adds trackWindowParameters support so that userId, testCells, and userSegments from window.cnstrc / legacy window globals are used as live fallbacks in query modules, while intentionally excluding the tracker module. The approach is clever but has a few reliability and correctness concerns worth addressing before merging.
Inline comments: 6 discussions added
Overall Assessment:
| trackWindowParameters: trackWindowParameters || false, | ||
| }; | ||
|
|
||
| this.trackerOptions = { ...this.options }; |
There was a problem hiding this comment.
Important Issue: The spread { ...this.options } runs before applyWindowParameterGetters is called on this.options (three lines later). At spread time the properties are still plain data values, so trackerOptions ends up as a plain-value snapshot — this is actually the desired behavior (tracker should not see window globals). However, this correctness relies entirely on execution order and is non-obvious to future maintainers.
More importantly, there is a subtle bug: applyWindowParameterGetters installs live getter descriptors only on this.options. When setClientOptions later calls setOption('userId', userId) it writes this.options[key] = value, which invokes the setter of the getter-property and stores the new value in the backing closure — that's correct. But it also writes this.trackerOptions[key] = value (line 185), which correctly updates the plain property on trackerOptions. The two objects staying in sync is fragile: any future property added to setOption paths must remember to also update trackerOptions. Consider adding a comment explaining the intentional split, or making the pattern more explicit.
| }); | ||
|
|
||
| it('Should not include window globals when trackWindowParameters is false', (done) => { | ||
| window.cnstrc = { userId: 'window-user-id', testCells: { exp: 'var' }, userSegments: ['seg'] }; |
There was a problem hiding this comment.
Suggestion: Several test cases set window.cnstrc and then clean it up inside the .then() callback using delete window.cnstrc. If the then() callback throws before the delete (e.g., a failed assertion), or if the Promise rejects, window.cnstrc will leak into subsequent tests.
Consider using afterEach to clean up window globals, similar to the pattern already used in spec/src/utils/helpers.js for the applyWindowParameterGetters tests:
afterEach(() => { delete window.cnstrc; });This pattern appears in all four module test files (search.js, browse.js, autocomplete.js, recommendations.js) and tracker.js.
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
| trackWindowParameters: trackWindowParameters || false, | ||
| }; | ||
|
|
||
| this.trackerOptions = { ...this.options }; |
There was a problem hiding this comment.
The spread { ...this.options } copies plain values before applyWindowParameterGetters installs the getters three lines down. Why do we want to exclude tracker here?
| setClientOptions(options) { | ||
| if (Object.keys(options).length) { | ||
| const { apiKey, segments, testCells, sessionId, userId, sendTrackingEvents } = options; | ||
| const setOption = (key, value) => { |
There was a problem hiding this comment.
This is dependant on the previous comment but because there are now two independent options objects, every update has to write to both. Any future property added to a setClientOptions path must remember to update trackerOptions too, or the two drift apart silently.
| return filtered; | ||
| }, | ||
|
|
||
| applyWindowParameterGetters: (options) => { |
There was a problem hiding this comment.
This helper is exported and its getters reference window directly. It's only guarded by the canUseDOM() check at the call site in constructorio.js. If it's ever called independently in a DOM-less context (or a test forgets JSDOM), reading options.userId/testCells/segments throws ReferenceError. What do you think about adding canUseDOM() no-op guard inside the helper so it's safe on its own.
| segments: options.segments || undefined, | ||
| }; | ||
|
|
||
| Object.defineProperty(options, 'userId', { |
There was a problem hiding this comment.
There is a good amount of repeating code with these three. What do you think about extracting some of that logic into a helper?
| beaconMode: (beaconMode === false) ? false : true, // Defaults to 'true', | ||
| networkParameters: networkParameters || {}, | ||
| humanityCheckLocation: humanityCheckLocation || 'session', | ||
| trackWindowParameters: trackWindowParameters || false, |
There was a problem hiding this comment.
trackWindowParameters || false treats any truthy non-boolean (e.g. the string 'false') as enabled. And the gate at line 149 reads the raw constructor arg, not the normalized value — so even after normalizing this.options, the feature can still switch on from a bad input. Can we do trackWindowParameters === true?
| search.getSearchResults(query, { section }).then(() => { | ||
| const requestedUrlParams = helpers.extractUrlParamsFromFetch(fetchSpy); | ||
| expect(requestedUrlParams).to.have.property('ui').to.equal('window-user-id'); | ||
| delete window.cnstrc; |
There was a problem hiding this comment.
Several tests set window.cnstrc and clean it up with delete window.cnstrc inside the .then() callback. If an assertion throws first (or the promise rejects), the global leaks into subsequent tests. Can we move cleanup to afterEach?
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.