feat: add support for Google Tag Manager environment config #2045
Conversation
|
Warning Rate limit exceeded@sandeepdsvs has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes update the GoogleTagManager integration by introducing two new configuration parameters: Changes
Sequence Diagram(s)sequenceDiagram
participant GM as GoogleTagManager
participant NSL as NativeSdkLoader
participant SL as Script Loader
Note over GM: Config includes environmentID & authorizationToken
GM->>GM: Instantiate GoogleTagManager(config, analytics, destinationInfo)
GM->>NSL: init() calls loadNativeSdk(containerID, serverUrl, environmentID, authorizationToken)
NSL-->>NSL: Construct script URL with new query parameters if values exist
NSL->>SL: Request script load
SL-->>NSL: Script loaded
NSL-->>GM: Return load result
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/analytics-js-integrations/src/integrations/GoogleTagManager/browser.js (1)
20-21: Consider adding parameter validation.While the property assignments are correct, consider adding validation for
environmentIDandauthorizationTokento ensure they meet GTM's requirements when provided.- this.environmentID = config.environmentID; - this.authorizationToken = config.authorizationToken; + this.environmentID = config.environmentID ? String(config.environmentID).trim() : undefined; + this.authorizationToken = config.authorizationToken ? String(config.authorizationToken).trim() : undefined; + + if (this.environmentID && !/^[a-zA-Z0-9-_]+$/.test(this.environmentID)) { + logger.warn('Invalid environmentID format. It should only contain alphanumeric characters, hyphens, and underscores.'); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/analytics-js-integrations/src/integrations/GoogleTagManager/browser.js(2 hunks)packages/analytics-js-integrations/src/integrations/GoogleTagManager/nativeSdkLoader.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Security and code quality checks
- GitHub Check: Bundle size checks
- GitHub Check: Unit Tests and Lint
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
packages/analytics-js-integrations/src/integrations/GoogleTagManager/nativeSdkLoader.js (3)
3-3: LGTM! Function signature updated correctly.The addition of
environmentIDandauthorizationTokenparameters aligns with GTM's environment configuration requirements.
19-19: LGTM! Script source URL constructed correctly.The URL construction properly combines all parameters in a clean and maintainable way.
14-16:❓ Verification inconclusive
LGTM! URL parameters constructed securely.
The implementation correctly:
- Follows GTM's URL parameter format
- Uses proper URL encoding for security
- Handles optional parameters gracefully
Please verify the GTM environment configuration format:
🌐 Web query:
What is the correct format for Google Tag Manager environment preview URL parameters?💡 Result:
Google Tag Manager's environment preview URLs use specific parameters to enable debugging and testing. Based on search results:
Standard preview mode URL format:
https://example.com/?gtm_auth=AUTH_TOKEN>m_preview=ENV_ID>m_debug=xKey parameters:
gtm_auth: Authentication token generated when creating/sharing a preview link [6][9]gtm_preview: Environment ID indicating which container configuration to load [3][9]gtm_debug=x: Flag enabling debug mode interface [7][8]For server-side tagging, additional parameters appear in requests:
https://sgtm.example.com?id=GTM_SERVER_ID>m_auth=AUTH_TOKEN>m_preview=ENV_IDThis structure routes requests through load balancers while maintaining preview context[3].
Important implementation details:
- Parameters are automatically appended when using GTM's native preview mode[7]
- Multiple environments require distinct auth tokens and environment IDs[6][9]
- Server-side configurations use environment variables like
PREVIEW_SERVER_URLto manage routing[3]- Debug parameters can be manually removed if they break site functionality (
>m_debug=)[7]For shared preview links, GTM generates URLs with these parameters pre-configured for specific environments[6][9]. The authentication tokens ensure only authorized users can access unpublished container configurations.
Citations:
- 1: https://analytify.io/google-tag-manager-preview-mode/
- 2: https://www.optimizesmart.com/gtm-server-side-preview-mode/
- 3: https://developers.google.com/tag-platform/tag-manager/server-side/cloud-run-preview-tagging-servers
- 4: https://developers.google.com/tag-platform/tag-manager/server-side/manual-setup-guide
- 5: https://support.google.com/tagmanager/answer/6311518?hl=en
- 6: https://www.bounteous.com/insights/2016/05/16/deploying-google-tag-manager-multiple-website-environments/
- 7: https://support.google.com/tagmanager/answer/6107056?hl=en
- 8: https://crystallize.com/answers/tech-dev/google-tag-manager-test-environment
- 9: https://marketlytics.com/blog/google-tag-manager-environments/
- 10: https://www.simoahava.com/analytics/variable-guide-google-tag-manager/
URL Parameter Construction: Please Verify GTM Parameters
The URL parameters are built securely with appropriate encoding and optional handling. However, note two potential discrepancies versus the standard GTM preview URL format:
Environment Parameter: Your code constructs the environment segment as
>m_preview=env-${encodeURIComponent(environmentID)}
whereas the standard format is typically
>m_preview=${encodeURIComponent(environmentID)}.Debug Flag: Instead of using the standard debug parameter (
>m_debug=x), your implementation uses a constant parameter
>m_cookies_win=x.Please verify that these differences are intentional for your specific GTM configuration. If not, consider aligning with the documented format:
?gtm_auth=AUTH_TOKEN>m_preview=ENV_ID>m_debug=xpackages/analytics-js-integrations/src/integrations/GoogleTagManager/browser.js (1)
30-30: LGTM! Init method updated correctly.The
loadNativeSdkcall properly forwards the new configuration parameters.
size-limit report 📦
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2045 +/- ##
========================================
Coverage 61.41% 61.42%
========================================
Files 478 478
Lines 16404 16406 +2
Branches 3238 3230 -8
========================================
+ Hits 10075 10077 +2
- Misses 5113 5140 +27
+ Partials 1216 1189 -27 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/analytics-js-integrations/__tests__/integrations/GoogleTagManager/browser.test.js (1)
47-55: Update test description and add edge cases for GTM environment config.The test description is outdated and doesn't reflect the new environment parameters. Additionally, consider testing edge cases:
- When environment config is missing
- When only one of the environment parameters is provided
Update the test suite:
-it('should call loadNativeSdk with containerID and serverUrl', () => { +it('should call loadNativeSdk with container and environment config', () => { googleTagManager.init(); expect(loadNativeSdk).toHaveBeenCalledWith( config.containerID, config.serverUrl, config.environmentID, config.authorizationToken, ); }); +it('should handle missing environment config gracefully', () => { + const minimalConfig = { + containerID: 'DUMMY_CONTAINER_ID', + serverUrl: 'DUMMY_SERVER_URL', + }; + const gtm = new GoogleTagManager(minimalConfig, analytics, destinationInfo); + gtm.init(); + expect(loadNativeSdk).toHaveBeenCalledWith( + minimalConfig.containerID, + minimalConfig.serverUrl, + undefined, + undefined, + ); +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/analytics-js-integrations/__tests__/integrations/GoogleTagManager/browser.test.js(2 hunks)packages/analytics-js-integrations/src/integrations/GoogleTagManager/nativeSdkLoader.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/analytics-js-integrations/src/integrations/GoogleTagManager/nativeSdkLoader.js
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Security and code quality checks
- GitHub Check: Unit Tests and Lint
- GitHub Check: Bundle size checks
🔇 Additional comments (1)
packages/analytics-js-integrations/__tests__/integrations/GoogleTagManager/browser.test.js (1)
81-173: Well-structured test coverage for core GTM functionality!The test suite demonstrates thorough coverage of core GTM functionality including:
- Proper dataLayer initialization and cleanup
- Comprehensive testing of identify, track, and page methods
- Good validation of event properties and user traits
- Edge case handling for page views
packages/analytics-js-integrations/__tests__/integrations/GoogleTagManager/browser.test.js
Show resolved
Hide resolved
packages/analytics-js-integrations/src/integrations/GoogleTagManager/nativeSdkLoader.js
Show resolved
Hide resolved
sandeepdsvs
force-pushed
the
feat.gtag-env-config-support
branch
from
18712e0 to
0656731
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/analytics-js-integrations/__tests__/integrations/GoogleTagManager/nativeSdkLoader.test.js (2)
19-35: Consider adding more test coverage.While the basic functionality tests are good, consider adding:
- Negative test cases (e.g., invalid containerID)
- Error handling scenarios
- More assertions for dataLayer event properties (e.g., verify all expected properties are present)
Example test case:
test('should handle invalid containerID gracefully', () => { expect(() => loadNativeSdk('', null, null, null)).toThrow(); expect(() => loadNativeSdk(null, null, null, null)).toThrow(); });
37-51: Add more script element validations.Consider adding assertions for:
- Script element position in DOM (verify it's inserted before the first script)
- The 'l' parameter defaulting to "dataLayer"
Example assertions:
// Verify script position const allScripts = document.getElementsByTagName('script'); expect(allScripts[0]).toBe(insertedScript); expect(allScripts[1].id).toBe('dummy-script'); // Verify dataLayer parameter expect(insertedScript.src).toContain('l=dataLayer');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/analytics-js-integrations/__tests__/integrations/GoogleTagManager/nativeSdkLoader.test.js(1 hunks)packages/analytics-js-integrations/src/integrations/GoogleTagManager/nativeSdkLoader.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/analytics-js-integrations/src/integrations/GoogleTagManager/nativeSdkLoader.js
🧰 Additional context used
🧠 Learnings (1)
packages/analytics-js-integrations/__tests__/integrations/GoogleTagManager/nativeSdkLoader.test.js (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1708
File: packages/analytics-js/__tests__/nativeSdkLoader.js:31-33
Timestamp: 2024-11-12T15:14:23.319Z
Learning: The loading snippet in `packages/analytics-js/__tests__/nativeSdkLoader.js` is a standard part of the SDK, and no changes are desired on it.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Security and code quality checks
- GitHub Check: Unit Tests and Lint
- GitHub Check: Bundle size checks
🔇 Additional comments (1)
packages/analytics-js-integrations/__tests__/integrations/GoogleTagManager/nativeSdkLoader.test.js (1)
1-18: Well-structured test setup!The test setup and teardown procedures are thorough, properly managing both DOM elements and global state between tests.
...es/analytics-js-integrations/__tests__/integrations/GoogleTagManager/nativeSdkLoader.test.js
Show resolved
Hide resolved
|
PR Description
Add support for Google Tag Manager environment config
Resolves INT-3252
Linear task (optional)
Linear task link
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit