[CDI-208] Add support for document_referrer and canonical_url in tracking methods.

[stanlp1]

• adds two parameters document_referrer and canonical_url to our tracking methods.

Parameters are not added yet.

[claude]

Pull Request Review: Add support for document_referrer and canonical_url

Thank you for this PR! I've reviewed the changes to add document_referrer and canonical_url tracking parameters. Overall, the implementation is solid and follows the existing patterns in the codebase. Here's my detailed feedback:

✅ Strengths

1. Consistent with existing patterns: The implementation follows the same structure as origin_referrer and other tracking parameters
2. Good test coverage: Added a new test case that verifies both parameters are sent correctly
3. Appropriate null handling: The helper functions properly check for DOM availability before accessing document properties
4. Non-breaking change: The parameters are optional and won't affect existing implementations

🔍 Code Quality Observations

src/utils/helpers.js:94-115

• The helper functions are well-structured and follow the existing canUseDOM() pattern
• The use of optional chaining (document?.referrer, document?.querySelector) is appropriate

src/modules/tracker.js:24-25,55-61

• Parameters are properly extracted at the top of applyParams function
• Conditional inclusion in aggregateParams is correct

⚠️ Potential Issues & Suggestions

1. Empty String vs Null Handling

In src/utils/helpers.js:96, document.referrer can be an empty string when there's no referrer (e.g., direct navigation, privacy settings). Consider whether empty strings should be sent as tracking parameters:

getDocumentReferrer: () => {
  if (utils.canUseDOM()) {
    return document?.referrer || null; // Return null instead of empty string
  }
  return null;
},

Current behavior: Empty string will be sent as document_referrer=
Suggested behavior: Don't send the parameter if it's empty (matches the conditional check on line 55)

2. Test Coverage Gaps

The new test only covers the happy path. Consider adding tests for:

• Behavior when canonical URL is missing
• Behavior when document.referrer is empty
• Behavior when running in non-DOM environment (Node.js/SSR)
• Tests for other tracking methods (trackInputFocus, trackItemDetailLoad, etc.) to ensure the parameters are sent consistently

Example test case:

it('Should not send document_referrer when referrer is empty', (done) => {
  // Test implementation
});

3. TypeScript Definitions Missing

The src/types/tracker.d.ts file doesn't appear to be updated. While these parameters are added automatically by the tracker and not part of the user-facing API, consider whether the internal types need updates for consistency.

4. Documentation

No documentation updates are included in this PR. Consider:

• Updating the README or API docs to mention these new tracking parameters
• Adding JSDoc comments to the new helper functions
• Documenting when/why these parameters are useful vs origin_referrer

🔒 Security Considerations

• ✅ No security concerns: Both document.referrer and canonical URLs are safe to transmit
• ✅ No XSS risk: Values are read from DOM properties, not user input
• ✅ Privacy-appropriate: document.referrer is already exposed to the page and follows browser privacy policies

⚡ Performance Considerations

• ✅ Minimal performance impact: Both document.referrer and querySelector are fast operations
• ✅ No unnecessary re-computation: Values are computed once per tracking call
• ⚠️ Minor consideration: querySelector('link[rel="canonical"]') runs on every tracking event. If tracking is high-frequency, consider caching the value (though this is likely premature optimization)

📋 Test Coverage

Current coverage: ✅ Basic functionality tested
Recommended additions:

• Edge cases (missing values, empty strings)
• Multiple tracking method types
• Non-browser environments

🎯 Recommendations

Before merging:

1. Consider the empty string vs null handling for document.referrer
2. Add edge case tests for missing/empty values

Nice to have (can be follow-up PRs):

1. Update documentation to explain the new parameters
2. Add tests for other tracking methods
3. Consider adding JSDoc comments to helper functions

Summary

This is a well-implemented feature that follows the codebase conventions. The main suggestion is to add more comprehensive test coverage for edge cases. The implementation is production-ready with minor improvements recommended.

Verdict: ✅ Approve with minor suggestions

---

Review generated by Claude Code

[mocca102]

LGTM! Why we're adding this?

[stanlp1]

LGTM! Why we're adding this?

https://constructor.slack.com/archives/C08NH8FRD96/p1764089472109149

Slack thread for context.

[jjl014]

LGTM! 🚀

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

Comprehensive test coverage across all tracking methods with consistent test patterns and proper JSDOM setup for canonical URL testing.

🚨 Critical Issues

[File: src/utils/helpers.js Lines: 94-115] Missing unit tests for new helper functions

• The two new helper functions getDocumentReferrer() and getCanonicalUrl() lack dedicated unit tests in spec/src/utils/helpers.js
• While these functions are indirectly tested through tracker tests, they should have their own unit tests similar to existing helper functions like getWindowLocation() (lines 193-214) and canUseDOM() (lines 95-105)
• Recommendation: Add unit tests covering:
  • getDocumentReferrer(): Test with and without DOM context, test when document.referrer is present/absent
  • getCanonicalUrl(): Test with and without DOM context, test when canonical link exists/doesn't exist, test when canonical link has no href attribute

⚠️ Important Issues

[File: src/utils/helpers.js Lines: 102-115] Potential edge case not handled in getCanonicalUrl()

• The function calls linkEle.getAttribute('href') which could return an empty string if the canonical link exists but has an empty href attribute
• Empty strings are truthy in the current implementation and would be sent as canonical_url= in tracking requests
• Recommendation: Add validation to return null for empty or invalid href values:

  if (linkEle) {
    const href = linkEle.getAttribute('href');
    canonicalURL = href && href.trim() !== '' ? href : null;
  }

[File: src/modules/tracker.js Lines: 55-61] Inconsistent parameter naming convention

• The implementation uses document_referrer and canonical_url (snake_case), while most other parameters in the codebase use camelCase (e.g., originReferrer, testCells)
• However, the actual query parameters use snake_case (e.g., origin_referrer, analytics_tags), so this appears intentional
• Clarification needed: Confirm this naming is aligned with the API specification requirements

[File: spec/src/modules/tracker.js Lines: 27-29] Test referrer value doesn't match realistic scenarios

• The test uses referrer = 'https://www.google.com/' as a constant, but document.referrer in real browsers contains the full URL of the previous page
• Recommendation: Consider testing with a more realistic referrer value that includes path and query parameters to ensure proper encoding

💡 Suggestions

[File: spec/src/modules/tracker.js Multiple locations] Test duplication across tracking methods

• The same test pattern "Should send along document_referrer and canonical_url query param" is repeated 25+ times across different tracking methods
• While this ensures comprehensive coverage, the tests are nearly identical with only the method name changing
• Suggestion: Consider creating a reusable test helper function or parameterized test to reduce duplication and maintenance burden:

  function testDocumentReferrerAndCanonicalUrl(methodName, methodCall) {
    it('Should send along document_referrer and canonical_url query param', (done) => {
      // shared test implementation
    });
  }

[File: src/utils/helpers.js Lines: 102-115] Variable naming inconsistency

• The function uses canonicalURL (with uppercase URL) while the parameter sent is canonical_url (lowercase)
• Suggestion: Rename the variable to canonicalUrl for consistency with JavaScript naming conventions

[General] Consider adding JSDoc comments

• The new helper functions getDocumentReferrer() and getCanonicalUrl() would benefit from JSDoc comments explaining their purpose, return values, and behavior
• This would improve code maintainability and align with patterns used elsewhere in the codebase

Overall Assessment: ⚠️ Needs Work

The implementation is functionally sound with excellent test coverage across tracking methods. However, the missing unit tests for the new helper functions and the unhandled edge case in getCanonicalUrl() should be addressed before merging. These are straightforward fixes that will significantly improve code quality and maintainability.