[CDX-379] Validate testCells

[esezen]

No description provided.

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

Excellent comprehensive test coverage with thorough edge case handling and proper validation logic implementation across all affected modules.

🚨 Critical Issues

None identified.

⚠️ Important Issues

None identified.

💡 Suggestions

1. Consider Object.entries for more idiomatic iteration

• File: src/utils/helpers.js Line: L384-388

The current implementation uses Object.keys(testCells).forEach() to iterate and access values via bracket notation. Consider using Object.entries() for cleaner code:

// Current approach
Object.keys(testCells).forEach((key) => {
  if (typeof testCells[key] === 'string' && testCells[key] !== '') {
    filtered[key] = testCells[key];
  }
});

// Alternative approach
Object.entries(testCells).forEach(([key, value]) => {
  if (typeof value === 'string' && value !== '') {
    filtered[key] = value;
  }
});

This eliminates repeated bracket lookups and makes the intent clearer.

2. Consider using reduce for functional style

• File: src/utils/helpers.js Line: L377-391

The toValidTestCells function could be written in a more functional style using reduce:

toValidTestCells: (testCells) => {
  if (!testCells || typeof testCells !== 'object' || Array.isArray(testCells)) {
    return {};
  }

  return Object.entries(testCells).reduce((acc, [key, value]) => {
    if (typeof value === 'string' && value !== '') {
      acc[key] = value;
    }
    return acc;
  }, {});
},

This eliminates the need for the intermediate filtered object and makes the transformation more declarative.

3. Test consistency: consider using .to.be.false for boolean assertions

• File: spec/src/modules/autocomplete.js Line: L127-128
• File: spec/src/modules/browse.js Line: L112-113
• File: spec/src/modules/search.js Line: L108-109
• File: spec/src/modules/tracker.js Line: L364-365

The tests use .to.not.have.property() to verify invalid testCells are filtered out. While correct, you could also consider checking that these properties explicitly don't exist or have undefined values for clarity, though the current approach is perfectly valid.

Overall Assessment: ✅ Pass

This PR successfully addresses CDX-379 by implementing proper validation for testCells. The implementation is clean, well-tested, and follows defensive programming principles by filtering out invalid input types. The validation ensures that only non-empty string values are accepted, which prevents potential API issues. The change is applied consistently across the constructor and setClientOptions method, with comprehensive test coverage in both unit tests and integration tests across all affected modules (autocomplete, browse, search, tracker). The fix for using .deep.equal instead of .equal in existing tests is also appropriate for object comparisons.

[Copilot]

Pull request overview

Adds validation/sanitization for testCells so only valid non-empty string entries are retained and sent as request params.

Changes:

• Added helpers.toValidTestCells to filter invalid testCells entries.
• Applied filtering in ConstructorIO constructor and setClientOptions.
• Added unit/integration tests ensuring invalid testCells are excluded from request params.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/utils/helpers.js	Adds toValidTestCells filtering helper.
src/constructorio.js	Uses toValidTestCells when setting options.testCells.
spec/src/utils/helpers.js	Adds unit tests for toValidTestCells.
spec/src/modules/tracker.js	Verifies only valid testCells are serialized into tracking requests.
spec/src/modules/search.js	Verifies only valid testCells are serialized into search requests.
spec/src/modules/browse.js	Verifies only valid testCells are serialized into browse requests.
spec/src/modules/autocomplete.js	Verifies only valid testCells are serialized into autocomplete requests.
spec/src/constructorio.js	Adds constructor/setClientOptions tests and updates assertions to deep equality.

---

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

[Copilot]

This changes the default options.testCells value from undefined (when testCells isn't provided) to {}. That can be a breaking behavior change for consumers who check presence vs emptiness. Consider only adding testCells to the options object when the caller actually provided it (e.g., conditionally set it, or have toValidTestCells return undefined when input is undefined).

[Copilot]

Using if (testCells) prevents callers from explicitly clearing testCells via null/undefined in setClientOptions (the assignment block won't run). If clearing is intended/expected, switch the conditional to check whether testCells was provided (e.g., property existence check) and then set this.options.testCells to the filtered result (which could be {}) accordingly.

[Copilot]

The comment says 'non-empty string values' but the implementation allows whitespace-only strings (e.g. ' '). If whitespace-only values should be treated as empty, update the predicate accordingly (and add a test case); otherwise, consider clarifying the comment to indicate that whitespace-only strings are considered valid.

[Copilot]

The comment says 'non-empty string values' but the implementation allows whitespace-only strings (e.g. ' '). If whitespace-only values should be treated as empty, update the predicate accordingly (and add a test case); otherwise, consider clarifying the comment to indicate that whitespace-only strings are considered valid.

Suggested change

	if (typeof testCells[key] === 'string' && testCells[key] !== '') {
	if (typeof testCells[key] === 'string' && testCells[key].trim() !== '') {