Cdx 429 add result offset parameter support to js sdk tracker by Sher-Bakhodirov · Pull Request #463 · Constructor-io/constructorio-client-javascript

Sher-Bakhodirov · 2026-06-10T15:48:53Z

Summary

Adds support for the resultOffset parameter across the tracker functions

Changes

Added the resultOffset parameter to the following tracker functions:

trackSearchResultsLoaded
trackBrowseResultsLoaded
trackBrowseResultClick
trackRecommendationView
trackRecommendationClick
trackGenericResultClick

Intentionally Excluded

resultOffset was not added to the following functions:

trackSearchResultClick — uses the legacy backend endpoint, which does not support result_offset.

Test Cleanup

The tests previously used the pattern Object.assign(requiredParameters, optionalParameters), which mutates the original requiredParameters object by merging the optional parameters into it. This caused optional parameters to leak into test cases that were only meant to use the required parameters. Updated these spots to avoid mutating the shared object.

…Parameters

…t-to-js-sdk-tracker

Copilot

Pull request overview

Adds resultOffset support to the JavaScript SDK tracker to better support infinite-scroll/offset-based result pagination across multiple tracking events, with corresponding TypeScript typing updates and test coverage.

Changes:

Added resultOffset parameter handling to several tracker event payloads (search results loaded, recommendations, browse, generic click, quizzes).
Updated src/types/tracker.d.ts to expose resultOffset?: number on the relevant tracker method parameter types.
Expanded tracker specs to validate result_offset inclusion/omission and backend error behavior when combined with resultPage, plus minor test refactors (spread vs Object.assign).

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

File	Description
`src/types/tracker.d.ts`	Extends Tracker TypeScript declarations to include `resultOffset` in multiple method parameter shapes.
`src/modules/tracker.js`	Adds `result_offset` request body support (and validation in quiz methods) for offset-based tracking.
`spec/src/modules/tracker.js`	Adds/updates tests to cover `resultOffset` behavior and refactors parameter merging to object spread.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

esezen · 2026-06-10T18:49:29Z

   * @param {string} [parameters.itemName] - Product item name
   * @param {string} [parameters.variationId] - Product item variation unique identifier
   * @param {string} [parameters.section="Products"] - Index section
+   * @param {number} [parameters.resultOffset] - Current offset of results, used on scrolling sites. Cannot be used with `resultPage`


@Sher-Bakhodirov I think this makes sense. Wdyt?

esezen

PR is looking good overall. I left some comments

esezen · 2026-06-10T18:49:29Z

   * @param {string} [parameters.itemName] - Product item name
   * @param {string} [parameters.variationId] - Product item variation unique identifier
   * @param {string} [parameters.section="Products"] - Index section
+   * @param {number} [parameters.resultOffset] - Current offset of results, used on scrolling sites. Cannot be used with `resultPage`


@Sher-Bakhodirov I think this makes sense. Wdyt?

esezen · 2026-06-10T18:50:56Z


      expect(
        tracker.trackMediaImpressionView(
-          Object.assign(requiredParameters, optionalParameters),


Why are we changing these?

Object.assign(requiredParameters, optionalParameters) mutates requiredParameters in place, so optional params leaked into tests that should only use the required ones. The change just avoids mutating the shared object. I can revert if you'd rather keep it out of this PR

Makes sense. We can keep it

esezen · 2026-06-10T18:54:08Z

@@ -676,6 +676,7 @@ class Tracker {
   * @param {object[]} parameters.items - List of product item unique identifiers in search results listing


Not related to this line but can we please add offset to trackSearchResultClickV2 and trackSearchResultClick as well?

For trackSearchResultClick, it's using a legacy endpoint that doesn't support result_offset. And trackSearchResultClickV2 uses an endpoint that wasn't listed in the ticket, so I think we'd want to verify it supports result_offset before adding it. I can include both if you can confirm

If V1 doesn't support it, we can skip it. Let's add it to V2. Thank you!

Sure thing
e299c75

constructor-claude-bedrock

Code Review

This PR adds resultOffset parameter support across multiple tracker functions and fixes test pollution caused by Object.assign mutating shared requiredParameters. The implementation is largely consistent and well-tested, but there are notable inconsistencies in how resultOffset is written to bodyParams and how input validation is applied across functions.

Inline comments: 7 discussions added

Overall Assessment: ⚠️ Needs Work

constructor-claude-bedrock · 2026-06-12T12:45:21Z

      expect(tracker.trackSearchResultsLoaded(term, parameters)).to.equal(true);
    });
+
+    it('Should return valid response when resultOffset is passed as an integer', (done) => {


Suggestion: Tests for trackSearchResultsLoaded, trackSearchResultClickV2, trackRecommendationView, trackRecommendationClick, trackBrowseResultsLoaded, and trackBrowseResultClick all include a "Should receive error from backend when both resultPage and resultOffset are passed" case — but trackGenericResultClick has no such conflict test. This is probably intentional since trackGenericResultClick has no resultPage concept, but if the backend could still reject some combination, it's worth a comment. The trackGenericResultClick tests also have no "resultOffset is not a number" test unlike trackQuizResultsLoaded/trackQuizResultClick, which is consistent with the missing type validation noted above.

constructor-claude-bedrock · 2026-06-12T12:45:21Z

      expect(tracker.trackSearchResultClickV2(term, { ...requiredParameters, ...optionalParameters, ...v2Parameters })).to.equal(true);
    });

+    it('V2 Should return valid response when resultOffset is passed as an integer', (done) => {


Suggestion: The trackSearchResultClickV2 tests are prefixed with V2 (e.g. 'V2 Should return valid response when resultOffset is passed as an integer'), but they live inside the trackSearchResultClick describe block alongside the V1 tests. All other resultOffset tests in this PR follow the standard sentence-case naming. Consider using a nested describe('trackSearchResultClickV2') block or at least a consistent naming prefix. This makes test output and debugging easier.

constructor-claude-bedrock · 2026-06-12T12:45:21Z

        // Request
        expect(fetchSpy).to.have.been.called;
-        expect(requestParams).to.have.property('section').to.equal(requiredParameters.section);
+        expect(requestParams).to.have.property('section').to.equal(optionalParameters.section);


Important Issue: This is a pre-existing bug fix bundled into the PR — the assertion previously checked requiredParameters.section but should check optionalParameters.section. While the fix is correct, it changes behavior of an existing test and should be called out explicitly in the PR description or as a separate commit to make the history clear. The PR description mentions test cleanup for Object.assign mutation, but this section fix is a distinct correctness fix that should be noted separately.

constructor-claude-bedrock · 2026-06-12T12:45:21Z

      });

-      expect(tracker.trackSearchResultsLoaded(term, Object.assign(requiredParameters, optionalParameters)))
+      expect(tracker.trackSearchResultsLoaded(term, { ...requiredParameters, ...optionalParameters }))


Suggestion: The "omit result_offset when resultOffset is not provided" tests (for all functions) only pass ...requiredParameters without resultOffset and assert the property is absent. Consider also testing the boundary where resultOffset: 0 is passed — since 0 is a valid integer offset but is falsy, and some guard patterns might accidentally filter it out. The isNil helper should handle this correctly, but an explicit test for resultOffset: 0 would confirm the behavior and prevent regression.

esezen

Thanks for making the changes!

Sher-Bakhodirov added 3 commits June 10, 2026 19:57

add result_offset to tracking methods

0176fb9

replace Object.assign with spread operator to avoid mutating required…

f18b8cd

…Parameters

fix trackGenericResultClick test

222c3dc

Sher-Bakhodirov requested a review from a team as a code owner June 10, 2026 15:48

Copilot AI review requested due to automatic review settings June 10, 2026 15:48

Copilot started reviewing on behalf of Sher-Bakhodirov June 10, 2026 15:49 View session

Merge branch 'master' into cdx-429-add-result_offset-parameter-suppor…

cb5e4e7

…t-to-js-sdk-tracker

Sher-Bakhodirov requested review from a team and TarekAlQaddy June 10, 2026 15:50

Copilot AI reviewed Jun 10, 2026

View reviewed changes

This comment was marked as outdated.

Sign in to view

esezen requested changes Jun 10, 2026

View reviewed changes

update jsdoc for trackGenericResultClick

911e89f

This comment was marked as outdated.

Sign in to view

add result_offset to trackSearchResultClickV2

e299c75

constructor-claude-bedrock Bot reviewed Jun 12, 2026

View reviewed changes

esezen approved these changes Jun 12, 2026

View reviewed changes

esezen merged commit cd608e9 into master Jun 15, 2026
15 of 17 checks passed

esezen deleted the cdx-429-add-result_offset-parameter-support-to-js-sdk-tracker branch June 15, 2026 14:13

		@@ -676,6 +676,7 @@ class Tracker {
		* @param {object[]} parameters.items - List of product item unique identifiers in search results listing

Uh oh!

Conversation

Sher-Bakhodirov commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Intentionally Excluded

Test Cleanup

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

esezen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

constructor-claude-bedrock Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

constructor-claude-bedrock Bot Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

constructor-claude-bedrock Bot Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

constructor-claude-bedrock Bot Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

constructor-claude-bedrock Bot Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

esezen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Sher-Bakhodirov commented Jun 10, 2026 •

edited

Loading