[CDX-470] Add additionalTrackingKeys option#472
Conversation
…vents Adds a new `additionalTrackingKeys` client option that duplicates tracking events to additional API keys. When configured, each tracking event queued via RequestQueue is also sent to each additional key with the `key` parameter swapped in both the URL and POST body.
There was a problem hiding this comment.
Pull request overview
Adds an additionalTrackingKeys client option to support duplicating tracking events across multiple Constructor.io API keys, primarily by cloning queued RequestQueue entries with the key swapped in the URL/body.
Changes:
- Introduces
additionalTrackingKeys?: string[]onConstructorClientOptionsand wires it throughConstructorIOoptions. - Updates
RequestQueueto enqueue duplicate tracking requests per additional key. - Adds unit + integration tests validating duplication behavior and invalid-key filtering.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/request-queue.js | Duplicates queued tracking requests for each configured additional key |
| src/types/index.d.ts | Adds additionalTrackingKeys?: string[] to the public options type |
| src/constructorio.js | Plumbs additionalTrackingKeys through normalized client options |
| spec/src/utils/request-queue.js | Unit tests for duplication, empty/missing option, invalid entries, GET behavior |
| spec/src/modules/tracker.js | Integration test asserting fetch is called for both primary + additional keys |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
esezen
left a comment
There was a problem hiding this comment.
Thanks! This is looking great. I left some comments
- Use Set to remove duplicate additionalTrackingKeys and exclude the primary apiKey - Assert duplicate request URL does not contain the original key - Add test verifying request bodies are identical except for the key field - Improve JSDoc description for additionalTrackingKeys parameter Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…-client-js Resolve conflicts: keep both useWindowParameters (from master) and additionalTrackingKeys (from this branch) in constructorio.js and index.d.ts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jjl014
left a comment
There was a problem hiding this comment.
Looking pretty solid to me. I did leave a couple comments that I'd like to get your thoughts on. Thanks!
| const encodedOriginalKey = helpers.encodeURIComponentRFC3986(this.options.apiKey); | ||
|
|
||
| const uniqueKeys = new Set( | ||
| additionalKeys.filter((key) => key && typeof key === 'string' && key !== this.options.apiKey), | ||
| ); | ||
|
|
||
| uniqueKeys.forEach((additionalKey) => { | ||
| const encodedAdditionalKey = helpers.encodeURIComponentRFC3986(additionalKey); | ||
| const swappedUrl = url.replace(`key=${encodedOriginalKey}`, `key=${encodedAdditionalKey}`); |
There was a problem hiding this comment.
Any reason why we're doing this validation and uniqueness check every time we queue an event?
It feels like an additional step that could be handled earlier (e.g. during client instantiation in the construtor or when setClientOptions is called)?
There was a problem hiding this comment.
Thanks @jjl014 This is a good consideration!
We can definitely handle the sanitization check earlier in the system and save the time to conduct the check every time we queue an event 👍
| this.options.serviceUrl = formattedServiceUrl || this.options.serviceUrl; | ||
| } | ||
|
|
||
| if (Array.isArray(additionalTrackingKeys)) { |
There was a problem hiding this comment.
Not a big deal, but if we wanted to stop sending events to multiple keys, we would have to send [] at the moment. Should also support null here?
There was a problem hiding this comment.
Yes I think this is a good point!
We should allow users to stop sending events to additional keys by passing null to additionalTrackingKeys , instead of forcing them to send an empty array []
|
Hi @jjl014
|
There was a problem hiding this comment.
Code Review
This PR adds an additionalTrackingKeys option that duplicates every tracking event to multiple API keys — a well-scoped feature with solid test coverage and clean sanitization logic.
Inline comments: 7 discussions added
Overall Assessment:
| return null; | ||
| } | ||
|
|
||
| const uniqueKeys = [...new Set( |
There was a problem hiding this comment.
Suggestion: [...new Set(array)] de-duplicates after filtering. This is fine, but it only removes exact-string duplicates. Keys that differ only in casing (e.g., 'Key-A' vs 'key-a') will both pass through. API keys are normally case-sensitive so this is likely intentional — a short comment would make that explicit and prevent future "bug" reports.
| additionalTrackingKeys: [additionalKey], | ||
| }); | ||
|
|
||
| let callCount = 0; |
There was a problem hiding this comment.
Important Issue: Both tracker integration tests use a manual callCount pattern combined with a shared fetchSpy — but neither test resets fetchSpy between the two tests. Because fetchSpy is presumably defined at a higher describe scope (not shown in the diff), if any earlier test in the suite called fetch, fetchSpy.getCall(0) may not point to the call made by this test. Also, the assertion expect(fetchSpy).to.have.been.calledTwice is global state — if the spy is not reset in a beforeEach, the assertion will fail if more than two prior calls exist.
Ensure fetchSpy.resetHistory() (or fetchSpy.reset() depending on sinon version) is called in a beforeEach within the additionalTrackingKeys describe block, or use a locally-scoped spy.
|
|
||
| // Assert an option on the client and every sub-module that shares its `options` reference. | ||
| // Uses `deep.equal` to enforce strict structural equality for objects and arrays. | ||
| const expectSharedOption = (instance, option, expected) => { |
There was a problem hiding this comment.
Suggestion: expectSharedOption uses .to.deep.equal for all options, but the original tests used .to.equal (strict reference equality) for primitive options like apiKey, userId, sessionId, and .to.deep.equal only for objects like testCells. The new helper applies deep.equal uniformly, which is slightly less precise for primitives (it won't catch the case where an option is accidentally cloned to a different reference for objects). For this codebase, where all modules share the same options reference, this distinction doesn't matter in practice — but it's worth a note since it silently changes assertion semantics for the refactored tests.
jjl014
left a comment
There was a problem hiding this comment.
Thanks for making the updates! The changes are looking good from what I can tell, but I noticed that some of the newly added tests and a variety of tests from other parts of the spec are failing.
I'm guessing the ones failing on master might be expected. I think @Mudaafi mentioned he was working on fixing them. However, I wouldn't expect the tests added from this PR to fail as well. 🤔
Summary
additionalTrackingKeysoption toConstructorClientOptionsthat accepts an array of API key stringsRequestQueueis duplicated for each additional key, with thekeyswapped in both the URL query string and POST bodyUsage
Changes
src/types/index.d.ts— AddedadditionalTrackingKeys?: string[]to options interfacesrc/constructorio.js— Pass through the new optionsrc/utils/request-queue.js— Duplicate queued requests for each additional keyspec/src/utils/request-queue.js— Unit tests (5 cases)spec/src/modules/tracker.js— Integration testTest plan
Resolves CDX-470