-
Notifications
You must be signed in to change notification settings - Fork 7
Support outbound-domains for iframes with srcdoc #67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -45,6 +45,40 @@ test.describe('Outbound domains tracking', () => { | |||||
| expect(iframeSrc).toContain('dub_id=test-click-id'); | ||||||
| }); | ||||||
|
|
||||||
| test('should handle iframe srcdoc attributes (Cal.com style)', async ({ | ||||||
| page, | ||||||
| }) => { | ||||||
| await page.goto('/outbound?dub_id=test-click-id'); | ||||||
|
|
||||||
| await page.waitForFunction(() => window._dubAnalytics !== undefined); | ||||||
|
|
||||||
| await page.waitForTimeout(2500); | ||||||
|
|
||||||
| // Check the first srcdoc iframe | ||||||
| const srcdocIframes = await page.$$('iframe[srcdoc]'); | ||||||
| expect(srcdocIframes.length).toBeGreaterThan(0); | ||||||
|
|
||||||
| const firstSrcdocIframe = srcdocIframes[0]; | ||||||
| const srcdocContent = await firstSrcdocIframe?.getAttribute('srcdoc'); | ||||||
|
|
||||||
| // Should contain the tracking parameter in the URLs within srcdoc | ||||||
| expect(srcdocContent).toContain('dub_id=test-click-id'); | ||||||
|
|
||||||
| // Check that both example.com and other.com URLs got the tracking parameter | ||||||
| expect(srcdocContent).toContain('example.com/booking?dub_id=test-click-id'); | ||||||
| expect(srcdocContent).toContain('other.com/widget.js?dub_id=test-click-id'); | ||||||
|
||||||
| expect(srcdocContent).toContain('other.com/widget.js?dub_id=test-click-id'); | |
| // expect(srcdocContent).toContain('other.com/widget.js?dub_id=test-click-id'); |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -31,6 +31,37 @@ const initOutboundDomains = () => { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function extractUrlFromSrcdoc(srcdoc) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!srcdoc) return null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Look for URLs in the srcdoc content | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // This is a basic implementation - may need refinement based on actual Cal.com usage | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const urlRegex = /https?:\/\/[^\s"'<>]+/g; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const matches = srcdoc.match(urlRegex); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (matches && matches.length > 0) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Return the first URL found - this might need to be more sophisticated | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // depending on how Cal.com structures their srcdoc content | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return matches[0]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function updateSrcdocWithTracking(element, originalUrl, newUrl) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!element.srcdoc) return false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Replace the original URL with the new URL in the srcdoc content | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const updatedSrcdoc = element.srcdoc.replace(originalUrl, newUrl); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!srcdoc) return null; | |
| // Look for URLs in the srcdoc content | |
| // This is a basic implementation - may need refinement based on actual Cal.com usage | |
| const urlRegex = /https?:\/\/[^\s"'<>]+/g; | |
| const matches = srcdoc.match(urlRegex); | |
| if (matches && matches.length > 0) { | |
| // Return the first URL found - this might need to be more sophisticated | |
| // depending on how Cal.com structures their srcdoc content | |
| return matches[0]; | |
| } | |
| return null; | |
| } | |
| function updateSrcdocWithTracking(element, originalUrl, newUrl) { | |
| if (!element.srcdoc) return false; | |
| try { | |
| // Replace the original URL with the new URL in the srcdoc content | |
| const updatedSrcdoc = element.srcdoc.replace(originalUrl, newUrl); | |
| if (!srcdoc) return []; | |
| // Look for URLs in the srcdoc content | |
| // This is a basic implementation - may need refinement based on actual Cal.com usage | |
| const urlRegex = /https?:\/\/[^\s"'<>]+/g; | |
| const matches = srcdoc.match(urlRegex); | |
| // Return all matched URLs as an array, or an empty array if none found | |
| return matches ? matches : []; | |
| } | |
| function updateSrcdocWithTracking(element, urlMap) { | |
| if (!element.srcdoc) return false; | |
| try { | |
| let updatedSrcdoc = element.srcdoc; | |
| // urlMap is an array of { originalUrl, newUrl } | |
| urlMap.forEach(({ originalUrl, newUrl }) => { | |
| // Replace all occurrences of the original URL with the new URL | |
| const regex = new RegExp(originalUrl.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), 'g'); | |
| updatedSrcdoc = updatedSrcdoc.replace(regex, newUrl); | |
| }); |
Copilot
AI
Oct 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String.replace without a global regex only replaces the first occurrence of originalUrl; if the same URL appears multiple times in srcdoc, later instances remain untagged. Use replaceAll (with proper escaping) or construct a global RegExp to ensure all occurrences are updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Major: String replace only replaces the first occurrence.
Line 56 uses replace() which only replaces the first occurrence of the URL. If the same URL appears multiple times in the srcdoc content, subsequent occurrences won't be updated.
Apply this diff:
- const updatedSrcdoc = element.srcdoc.replace(originalUrl, newUrl);
+ const updatedSrcdoc = element.srcdoc.replaceAll(originalUrl, newUrl);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function updateSrcdocWithTracking(element, originalUrl, newUrl) { | |
| if (!element.srcdoc) return false; | |
| try { | |
| // Replace the original URL with the new URL in the srcdoc content | |
| const updatedSrcdoc = element.srcdoc.replace(originalUrl, newUrl); | |
| element.srcdoc = updatedSrcdoc; | |
| return true; | |
| } catch (e) { | |
| console.error('Error updating srcdoc:', e); | |
| return false; | |
| } | |
| } | |
| function updateSrcdocWithTracking(element, originalUrl, newUrl) { | |
| if (!element.srcdoc) return false; | |
| try { | |
| // Replace the original URL with the new URL in the srcdoc content | |
| const updatedSrcdoc = element.srcdoc.replaceAll(originalUrl, newUrl); | |
| element.srcdoc = updatedSrcdoc; | |
| return true; | |
| } catch (e) { | |
| console.error('Error updating srcdoc:', e); | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/script/src/extensions/outbound-domains.js around lines 51 to 63, the
updateSrcdocWithTracking function uses String.prototype.replace which only
replaces the first occurrence; change the replacement to replace all occurrences
by creating a global replacement: escape originalUrl for safe use in a RegExp,
build a new RegExp(escapedOriginalUrl, 'g'), use srcdoc.replace(regexp, newUrl)
(or use split(originalUrl).join(newUrl) if you prefer avoiding regex), assign
the result back to element.srcdoc and keep the existing try/catch and return
values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The single-URL processing approach won't handle multiple outbound URLs in srcdoc.
This logic processes only one URL per element due to the Set check. For iframes with srcdoc containing multiple outbound URLs (like the test case with both example.com/booking and other.com/widget.js), only the first extracted URL will be tracked, and the element will be marked as processed. Other outbound URLs in the same srcdoc will never be updated.
This requires a more comprehensive refactor to:
- Extract all URLs from srcdoc
- Check each against outbound domains
- Update all matching URLs in a single pass
- Mark the element as processed only after all URLs have been handled
Example approach:
} else if (element.tagName.toLowerCase() === 'iframe') {
if (element.src) {
element.src = newUrlString;
outboundLinksUpdated.add(element);
} else if (element.srcdoc) {
// Extract all URLs from srcdoc
const urls = extractUrlsFromSrcdoc(element.srcdoc);
let updatedSrcdoc = element.srcdoc;
let hasUpdates = false;
urls.forEach(url => {
try {
const parsedUrl = new URL(url);
if (filteredDomains.some(domain => isMatchingDomain(url, domain))) {
if (!parsedUrl.searchParams.has(DUB_ID_VAR)) {
parsedUrl.searchParams.set(DUB_ID_VAR, existingCookie);
updatedSrcdoc = updatedSrcdoc.replaceAll(url, parsedUrl.toString());
hasUpdates = true;
}
}
} catch (e) {
// Skip invalid URLs
}
});
if (hasUpdates) {
element.srcdoc = updatedSrcdoc;
outboundLinksUpdated.add(element);
}
}
}🤖 Prompt for AI Agents
In packages/script/src/extensions/outbound-domains.js around lines 107 to 129,
the current logic only processes a single extracted URL per iframe.srcdoc and
marks the element as handled, which misses additional outbound URLs in the same
srcdoc; refactor to extract all URLs from srcdoc, iterate each URL, for each
valid URL check against the outbound domains and add the tracking param if
missing, build an updated srcdoc string replacing each matched URL with its
updated version, and only after processing all URLs set element.srcdoc to the
updated string and add the element to outboundLinksUpdated if any replacements
were made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a fixed timeout can introduce flakiness and slows the test unnecessarily. Prefer waiting on a concrete condition (e.g., await page.waitForFunction(() => document.querySelector('iframe[srcdoc]')?.getAttribute('srcdoc')?.includes('dub_id='))) instead of an arbitrary delay.