Skip to content

Conversation

@stanlp1
Copy link
Contributor

@stanlp1 stanlp1 commented Dec 18, 2025

  • There is an edge cases where android apps will add a referrer that is not a valid url when opening links in a webview.

Ex: android-app://com.google.android.googlequicksearchbox/

  • Transforms this into a "valid" url by replacing "android-app" with "https" to pass our api validation.
  • Adds tests

@stanlp1 stanlp1 requested a review from a team December 18, 2025 22:19
@constructor-claude-bedrock
Copy link

Code Review Results

✅ Strengths

Well-tested implementation with comprehensive edge case coverage for Android app referrer transformation.

🚨 Critical Issues

None

⚠️ Important Issues

1. [File: src/utils/helpers.js Line: L52-53] Potential bug with multiple 'android-app' occurrences
The current implementation uses replace() which only replaces the first occurrence. If the URL contains multiple instances of 'android-app' (e.g., in the path), only the first would be replaced.

Suggestion: Use replaceAll() for consistency or be more specific with the replacement:

if (url?.startsWith('android-app')) {
  url = url.replace(/^android-app/, 'https');
}

2. [File: src/utils/helpers.js Line: L52-54] Mutation of function parameter
The function parameter url is being reassigned directly, which can be confusing since parameters appear immutable but aren't.

Suggestion: Use a new variable:

cleanAndValidateUrl: (url, baseUrl = undefined) => {
  let validatedUrl = null;

  try {
    let processedUrl = url;
    // Handle android app referrers
    if (processedUrl?.startsWith('android-app')) {
      processedUrl = processedUrl?.replace('android-app', 'https');
    }

    validatedUrl = (new URL(processedUrl, baseUrl)).toString();
  } catch (e) {
    // do nothing
  }

  return validatedUrl;
},

3. [File: src/utils/helpers.js Line: L52] Redundant optional chaining on reassigned variable
After checking url?.startsWith('android-app'), the subsequent url?.replace() uses optional chaining again, but we already know url exists from the previous check.

Suggestion: Remove the second optional chaining:

if (url?.startsWith('android-app')) {
  url = url.replace('android-app', 'https');
}

💡 Suggestions

1. [File: src/utils/helpers.js Line: L58] Silent error swallowing
The catch block with // do nothing silently swallows all errors. While this might be intentional for robustness, it could hide unexpected issues during development.

Consideration: Consider logging errors in development mode or documenting why all errors are intentionally ignored.

2. [File: spec/src/utils/helpers.js Line: L219] Test could verify more specific behavior
The test for getCanonicalUrl with Android app URLs is good, but it doesn't verify that the canonical URL is being processed through the same cleanAndValidateUrl function. Consider adding a test that verifies the integration explicitly.

3. [General] Documentation for edge case handling
This PR addresses a specific Android WebView edge case. Consider adding a code comment explaining why this transformation is necessary (e.g., "Android WebViews pass android-app:// schemes which aren't valid HTTP URLs for API validation").

Overall Assessment: ⚠️ Needs Work

The implementation correctly addresses the Android app referrer issue with comprehensive test coverage. However, there are a couple of important issues around parameter mutation and potential edge cases with the string replacement that should be addressed before merging. The changes are well-isolated and the test coverage is thorough.

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