[CDX-280] Allow updating sendTrackingEvents in setClientOptions

[esezen]

Adds support for dynamically updating the sendTrackingEvents option via setClientOptions() to enable/disable tracking at runtime. Updates are now propagated to the RequestQueue instance, ensuring tracking behavior immediately reflects the new value.

[Copilot]

Pull Request Overview

This PR adds the ability to dynamically update the sendTrackingEvents option in the ConstructorIO client library, allowing developers to enable or disable tracking events after initialization.

• Added sendTrackingEvents parameter to the setClientOptions method
• Implemented strict boolean type checking to ensure only valid boolean values update the setting
• Added propagation of the setting to the internal tracker module

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/constructorio.js	Added support for updating sendTrackingEvents option with boolean type validation and tracker propagation
spec/src/constructorio.js	Added comprehensive test coverage for the new functionality including type validation and behavior verification

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[claude]

Code Review: PR #407 - Allow updating sendTrackingEvents

Summary

This PR adds the ability to dynamically update the sendTrackingEvents option via setClientOptions(), allowing applications to enable/disable tracking events at runtime (e.g., based on cookie consent changes).

✅ Strengths

Code Quality

• Type safety: Proper type checking with typeof sendTrackingEvents === 'boolean' prevents invalid values
• Consistent with existing patterns: Follows the same approach used for other options like segments and testCells
• Clear documentation: JSDoc updated to document the new parameter

Test Coverage

• Comprehensive edge case testing: Excellent coverage of invalid inputs (null, numbers, strings, objects, arrays)
• Integration testing: Tests verify that the setting properly propagates to tracker.requests.sendTrackingEvents
• Behavioral validation: Tests confirm actual tracking behavior changes when the flag is toggled

🔍 Issues & Recommendations

1. Inconsistent Option Update Pattern

Severity: Medium

The implementation deviates from the existing pattern in setClientOptions(). Other options like segments and testCells use truthy checks, but sendTrackingEvents uses strict type checking.

Current code (line 179-182 in src/constructorio.js):

if (typeof sendTrackingEvents === 'boolean') {
  this.options.sendTrackingEvents = sendTrackingEvents;
  this.tracker.requests.sendTrackingEvents = sendTrackingEvents;
}

Comparison with other options (lines 171-176):

if (segments) {
  this.options.segments = segments;
}

if (testCells) {
  this.options.testCells = testCells;
}

Why this matters: The strict type check is actually better for a boolean flag (prevents truthy/falsy confusion), but it creates inconsistency in the codebase. Consider whether other boolean options should follow this pattern.

Recommendation: This approach is correct for booleans. Consider documenting this pattern or refactoring other boolean options to use similar strict type checking in a future PR.

2. Missing Options Object Update

Severity: Medium

The code updates this.tracker.requests.sendTrackingEvents but doesn't update this.tracker.options.sendTrackingEvents. Looking at line 112 of src/modules/tracker.js, the tracker stores options separately.

Potential issue: If any code checks tracker.options.sendTrackingEvents, it will see the old value even though tracker.requests.sendTrackingEvents was updated.

Recommendation: Add this line after line 181 in src/constructorio.js:

this.tracker.options.sendTrackingEvents = sendTrackingEvents;

3. Test Import Not Used

Severity: Low

The test file adds const store = require('../../test/utils/store'); at line 8 but only uses it in one test (line 291). This import could be moved to just that test's scope.

Recommendation: Consider moving the import closer to where it's used for better clarity, or leave it if this is the project's convention.

4. Async Test Timing

Severity: Low

The tests use fixed timeouts (10ms, 50ms, 100ms) which could be flaky in slow CI environments.

Lines: Tests at lines 395-426 and 448-513 use setTimeout with hardcoded delays.

Recommendation: While these timeouts seem reasonable, consider using a test helper or increasing timeouts slightly if flakiness occurs in CI. The current implementation is acceptable for now.

🚀 Performance Considerations

• Minimal overhead: The implementation adds negligible performance impact
• No memory leaks: Direct property assignment without creating new objects or listeners
• Runtime updates: Allows dynamic behavior changes without re-initialization

🔒 Security Concerns

• No security issues identified: The type checking prevents injection of malicious values
• Input validation: Strict boolean type check is appropriate for this use case

📊 Test Coverage Assessment

Excellent coverage with tests for:

• ✅ Valid boolean values (true/false)
• ✅ Invalid types (null, numbers, strings, objects, arrays)
• ✅ Undefined/missing values
• ✅ Integration with tracker module
• ✅ Actual tracking behavior changes
• ✅ End-to-end scenario (enable → disable → re-enable)

📝 Action Items

Before merging:

1. Add this.tracker.options.sendTrackingEvents = sendTrackingEvents; at line 181 in src/constructorio.js
2. Verify this doesn't break any existing functionality by running the full test suite

Future considerations:

1. Consider applying the strict type checking pattern to other boolean options
2. Document this pattern in contributing guidelines if not already present

Verdict

Approve with minor changes. The implementation is solid and well-tested. The main issue is ensuring consistency with the tracker's options object. Once addressed, this is ready to merge.

---

Generated by Claude Code Review

[jjl014]

Looks good to me! 🚀

[jensmeichler]

Thank you very much :D

That saves us a lot of extra logic we'd have to wrap around all calls 👍

[claude]

Code Review - PR #407: Allow updating sendTrackingEvents in setClientOptions

Summary

This PR adds support for dynamically updating the sendTrackingEvents option via setClientOptions(). The implementation is solid with comprehensive test coverage. Below are my findings:

---

✅ Strengths

1. Excellent Test Coverage

• Comprehensive edge case testing for invalid types (null, numeric, string, object, array)
• Integration tests verifying the complete flow (enable → disable → re-enable)
• Tests confirm propagation to both tracker.options and tracker.requests.sendTrackingEvents
• Behavioral verification that events are actually suppressed when disabled

2. Type Safety Improvement

// Before: if (sendTrackingEvents !== undefined)
// After: if (typeof sendTrackingEvents === 'boolean')

This change (src/constructorio.js:179) is more robust and prevents truthy/falsy coercion issues (e.g., 0, '', null being treated as valid).

3. Proper Propagation

The implementation correctly updates both the options and the underlying RequestQueue instance via this.tracker.requests.sendTrackingEvents (src/constructorio.js:181).

---

🔍 Potential Issues & Recommendations

1. Architecture Consideration: Direct Property Access

this.tracker.requests.sendTrackingEvents = sendTrackingEvents;

Issue: This directly accesses and mutates an internal property of the RequestQueue instance, bypassing encapsulation.

Risk: If RequestQueue changes its internal structure or needs to perform additional logic when sendTrackingEvents changes, this code will break or behave incorrectly.

Recommendation: Consider adding a setter method to RequestQueue:

// In RequestQueue class (src/utils/request-queue.js)
setSendTrackingEvents(value) {
  this.sendTrackingEvents = value;
  // Any future logic can be added here
}

// In setClientOptions (src/constructorio.js)
this.tracker.requests.setSendTrackingEvents(sendTrackingEvents);

This provides better encapsulation and makes the API more maintainable.

2. Inconsistent Property Naming

• RequestQueue uses this.sendTrackingEvents (src/utils/request-queue.js:17)
• But the update targets this.tracker.requests.sendTrackingEvents

Verification Needed: Confirm these are the same property. If they are, the code is correct but could benefit from a comment explaining the relationship.

3. Race Condition Concern

// Test: spec/src/constructorio.js:394-423
instance.tracker.trackSessionStart();
setTimeout(() => {
  instance.setClientOptions({ sendTrackingEvents: false });
  // ...
}, 50);

Issue: There's a potential race condition if events are queued but not yet sent when sendTrackingEvents is toggled.

Questions:

• Should in-flight events be sent or discarded when tracking is disabled?
• Should queued events be cleared when disabling?

Recommendation: Document the expected behavior explicitly, possibly in the JSDoc for setClientOptions.

4. Missing JSDoc Update

The JSDoc comment (src/constructorio.js:161) correctly documents the parameter but could be enhanced:

/**
 * @param {boolean} [options.sendTrackingEvents] - Indicates if tracking events should be dispatched.
 *                                                   Changes are applied immediately and affect 
 *                                                   all subsequent tracking calls.
 */

---

🔒 Security

No security concerns identified. The type checking prevents injection or unexpected behavior from invalid inputs.

---

⚡ Performance

No performance concerns. The changes are minimal and only execute when setClientOptions is called.

---

🧪 Test Quality

The tests are well-structured and follow existing patterns in the codebase:

• Proper use of sinon spies
• Appropriate use of timeouts for async behavior
• Good coverage of both positive and negative cases

Minor Suggestion: The test at line 394 (spec/src/constructorio.js:394) could be slightly more explicit about what it's testing with a better description.

---

📋 Action Items

High Priority:

• Consider adding a setter method to RequestQueue for better encapsulation (see recommendation Feature/documentation via jsdoc #1)
• Document the behavior of in-flight/queued events when toggling sendTrackingEvents

Low Priority:

• Enhance JSDoc comment to clarify immediate effect
• Add comment explaining the property access path if it's not obvious

---

✅ Conclusion

This is a well-implemented feature with excellent test coverage. The code quality is good and follows the project's conventions (airbnb-base ESLint config). The primary concern is the architectural pattern of directly accessing internal properties, which could be improved for long-term maintainability. Otherwise, this is ready to merge after addressing the action items.

Overall Rating: 8.5/10

Great work! 🚀