Skip to content

Conversation

@ZSnake
Copy link
Contributor

@ZSnake ZSnake commented Jan 28, 2026

  • Add trackMediaImpressionClick
  • Add tests and types

@ZSnake ZSnake marked this pull request as ready for review February 4, 2026 08:43
@ZSnake ZSnake requested review from esezen and jjl014 as code owners February 4, 2026 08:43
@constructor-claude-bedrock

This comment has been minimized.

Copy link
Contributor

@esezen esezen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great! I left some small comments

…rtions. Extract behavior base URL helper function
@constructor-claude-bedrock

This comment has been minimized.

@constructor-claude-bedrock

This comment has been minimized.

@constructor-claude-bedrock

This comment has been minimized.

@constructor-claude-bedrock

This comment has been minimized.

@ZSnake ZSnake force-pushed the REM-2574/display-ad-click branch from 3b0e1c0 to f247712 Compare February 12, 2026 16:03
@constructor-claude-bedrock
Copy link

Code Review Results

✅ Strengths

Well-structured implementation with comprehensive test coverage for both the new trackMediaImpressionClick method and the refactored trackMediaImpressionView, including edge cases like special characters, non-breaking spaces, timeout handling, and referrer parameters.

🚨 Critical Issues

None identified. The implementation meets quality standards.

⚠️ Important Issues

1. Code Duplication Between Methods

  • Files: src/modules/tracker.js:1369-1473

The trackMediaImpressionView and trackMediaImpressionClick methods contain nearly identical code with only the endpoint path differing (display_ad_view vs display_ad_click). This violates the DRY principle and makes maintenance harder.

Suggestion: Extract the shared logic into a private helper method:

_trackMediaImpression(eventType, parameters, networkParameters = {}) {
  if (parameters && typeof parameters === 'object' && !Array.isArray(parameters)) {
    const baseUrl = helpers.getBehaviorUrl(this.options.mediaServiceUrl);
    const requestPath = `${baseUrl.toString()}v2/ad_behavioral_action/${eventType}?`;
    
    const bodyParams = {};
    const { bannerAdId, placementId, analyticsTags } = parameters;
    
    if (!helpers.isNil(bannerAdId)) bodyParams.banner_ad_id = bannerAdId;
    if (!helpers.isNil(placementId)) bodyParams.placement_id = placementId;
    if (!helpers.isNil(analyticsTags)) bodyParams.analytics_tags = analyticsTags;
    
    const requestURL = `${requestPath}${applyParamsAsString({}, this.options)}`;
    const requestMethod = 'POST';
    const requestBody = applyParams(bodyParams, { ...this.options, requestMethod });
    
    this.requests.queue(requestURL, requestMethod, requestBody, networkParameters);
    this.requests.send();
    
    return true;
  }
  
  this.requests.send();
  return new Error('parameters are required of type object');
}

trackMediaImpressionView(parameters, networkParameters = {}) {
  return this._trackMediaImpression('display_ad_view', parameters, networkParameters);
}

trackMediaImpressionClick(parameters, networkParameters = {}) {
  return this._trackMediaImpression('display_ad_click', parameters, networkParameters);
}

2. Inconsistent Parameter Validation

  • File: src/modules/tracker.js:1383-1393 & src/modules/tracker.js:1449-1459

The methods check if bannerAdId and placementId are not nil before adding them to bodyParams, but these are documented as required parameters in the JSDoc. If they're required, the validation should explicitly check for their presence and throw/return an error if missing.

Current behavior: If bannerAdId or placementId are undefined/null, the request is sent without them (likely to fail on the server).

Suggestion: Add explicit validation:

const { bannerAdId, placementId, analyticsTags } = parameters;

if (!bannerAdId || !placementId) {
  this.requests.send();
  return new Error('bannerAdId and placementId are required parameters');
}

3. Missing JSDoc in Helper Function

  • File: src/utils/helpers.js:376-384

The new getBehaviorUrl helper function lacks JSDoc documentation. Given that this is a utility function used by multiple methods, it should have proper documentation explaining its purpose, parameters, and return value.

Suggestion:

/**
 * Prepends 'behavior.' to the hostname if not already present
 * @param {string} mediaServiceUrl - The media service URL
 * @returns {URL} URL object with 'behavior.' prepended to hostname if needed
 */
getBehaviorUrl: (mediaServiceUrl) => {
  const baseUrl = new URL(mediaServiceUrl);
  
  if (!baseUrl.hostname.startsWith('behavior')) {
    baseUrl.hostname = `behavior.${baseUrl.hostname}`;
  }
  
  return baseUrl;
},

💡 Suggestions

1. Test Setup Refactoring Opportunity

  • File: spec/src/modules/tracker.js:15517-15531 & spec/src/modules/tracker.js:15745-15759

The before hooks in both test suites make identical fetch calls to retrieve test data. Consider extracting this into a shared test helper or fixture to reduce duplication and improve test maintainability.

2. Consider Type Safety for analyticsTags

  • File: src/types/tracker.d.ts:450

The analyticsTags parameter is typed as Record<string, string> but the implementation doesn't validate this. Consider if the API accepts other types (numbers, booleans) or if runtime validation would be beneficial.

3. Environment Variable Documentation

  • File: .github/workflows/run-tests.yml:28 & .github/workflows/run-tests-bundled.yml:28

The new TEST_MEDIA_REQUEST_API_KEY secret is added but there's no documentation update. Consider adding a note in the README or a separate environment setup document about this new requirement for running tests.

Overall Assessment: ✅ Pass

The implementation is solid with excellent test coverage. The main concerns are code duplication and missing parameter validation, which should be addressed to improve maintainability and robustness. The refactoring to extract getBehaviorUrl is a good improvement to the codebase.

Copy link
Contributor

@esezen esezen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@esezen esezen merged commit 3e9cf25 into master Feb 12, 2026
10 of 12 checks passed
@esezen esezen deleted the REM-2574/display-ad-click branch February 12, 2026 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants