fix(analytics-browser): exclude "port" when checking "excludeReferrer" in marketing attribution #1219
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a test page for Marketing Attribution functionality and fixes a domain matching issue in the isExcludedReferrer function. The fix ensures that domains with ports are properly handled when checking against excluded referrer patterns.
- Added a comprehensive HTML test page for testing marketing attribution with various UTM parameters and scenarios
- Fixed port handling in referrer domain exclusion logic by stripping port numbers before comparison
- Added test coverage for the port handling fix
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test-server/autocapture/marketing-attribution.html | New test page with UI for testing marketing attribution functionality |
| packages/analytics-browser/src/attribution/helpers.ts | Fixed isExcludedReferrer to strip ports from domains before comparison |
| packages/analytics-browser/test/attribution/helpers.test.ts | Added test cases for port handling in referrer exclusion |
| const referringDomainWithoutPort = referringDomain.split(':')[0]; | ||
| return excludeReferrers.some((value) => | ||
| value instanceof RegExp ? value.test(referringDomain) : value === referringDomain, | ||
| value instanceof RegExp ? value.test(referringDomainWithoutPort) : value === referringDomainWithoutPort, |
There was a problem hiding this comment.
[nitpick] The variable name referringDomainWithoutPort is verbose and could be simplified to domainWithoutPort or domain for better readability.
Copilot uses AI. Check for mistakes.
|
✅ No documentation updates required. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| const referringDomainWithoutPort = referringDomain.split(':')[0]; | ||
| return excludeReferrers.some((value) => | ||
| value instanceof RegExp ? value.test(referringDomain) : value === referringDomain, | ||
| value instanceof RegExp ? value.test(referringDomainWithoutPort) : value === referringDomainWithoutPort, |
There was a problem hiding this comment.
Bug: IPv6 Address Parsing Error
The referringDomain.split(':')[0] logic for port removal incorrectly truncates IPv6 addresses. Since IPv6 addresses contain colons (e.g., 2001:db8::1 or [2001:db8::1]:8080), this method truncates them (e.g., 2001:db8::1 becomes 2001, [2001:db8::1]:8080 becomes [2001), breaking domain exclusion matching for IPv6 hosts.
daniel-graham-amplitude
changed the title
|
Hi @daniel-graham-amplitude, is this from a customer ticket? http's default port is 80 and https default port is 443. Most of the time, in production, URL has no port. A port appears in a url usually for development or testing. If you're supporting this, we should also fix the ipv6 parsing error. |
|
@Mercy811 this is just for development purposes. I was trying to test out attribution to see how |
Summary
Checklist