feat: enable GTM script #2322
feat: enable GTM script #2322
Conversation
wilsonrivera
requested review from
JivusAyrus,
StarpTech and
thisisnithin
as code owners
…okies-during-cosmo-sign-up-in2
WalkthroughMake Google Tag Manager components props-driven (accepting a gtmId), replace Next.js Script usage with raw script tags for GTM, conditionally render GTM markup when gtmId is present, and adjust next.config.mjs CSP to always allow googletagmanager in frame-src and to include several analytics script-src domains only in production. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
studio/next.config.mjs (2)
58-60: Consider making frame-src GTM source conditional on production.The
frame-srcdirective includeshttps://www.googletagmanager.comunconditionally, butscript-srconly allows GTM sources in production (lines 70-80). This inconsistency means GTM frames are permitted in all environments even though GTM scripts won't load outside production.Apply this diff to align frame-src with the production-only approach:
- frame-src 'self' https://js.stripe.com https://hooks.stripe.com https://www.googletagmanager.com ${ - isPreview ? "https://vercel.live/ https://vercel.com" : "" + frame-src 'self' https://js.stripe.com https://hooks.stripe.com ${ + isProduction ? "https://www.googletagmanager.com" : "" + } ${isPreview ? "https://vercel.live/ https://vercel.com" : "" };
65-65: Consider making analytics image sources production-only.The
img-srcdirective includes*.ads.linkedin.comand*.google.comunconditionally, which are analytics/advertising domains. For consistency with the script-src approach (lines 70-80), these should likely be restricted to production only.Apply this diff to make img-src analytics sources conditional:
img-src 'self'${ isPreview ? " https://vercel.live/ https://vercel.com *.pusher.com/ data: blob:" : "" - } *.ads.linkedin.com *.google.com; + }${isProduction ? " *.ads.linkedin.com *.google.com" : ""};studio/src/components/layout/analytics/gtm-script.tsx (1)
3-5: Optional: Remove trailing semicolon after interface.TypeScript interfaces typically don't require a semicolon after the closing brace. While not incorrect, removing it aligns with common conventions.
Apply this diff:
export interface GtmScriptProps { gtmId: string | undefined; -}; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
studio/next.config.mjs(2 hunks)studio/src/components/layout/analytics/gtm-script.tsx(3 hunks)studio/src/pages/_document.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
studio/src/pages/_document.tsx (2)
studio/src/components/layout/analytics/gtm-script.tsx (2)
GtmScript(7-63)GtmNoScript(65-80)studio/next.config.mjs (1)
isProduction(6-6)
🪛 ast-grep (0.39.7)
studio/src/components/layout/analytics/gtm-script.tsx
[warning] 15-15: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
[warning] 30-30: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
studio/src/components/layout/analytics/gtm-script.tsx
[error] 16-16: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
[error] 31-31: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
studio/next.config.mjs (2)
6-6: LGTM!The
isProductionconstant is correctly defined and provides a clear way to conditionally enable analytics integrations.
70-80: LGTM!The conditional inclusion of analytics vendor sources in
script-srcfor production environments is well-implemented and aligns with the PR's objective to enable GTM in production.studio/src/components/layout/analytics/gtm-script.tsx (4)
7-10: LGTM!The guard clause properly prevents rendering GTM scripts when
gtmIdis not provided, making the component safe to use unconditionally.
14-27: dangerouslySetInnerHTML usage is safe here.The static analysis warnings about
dangerouslySetInnerHTMLare false positives in this context. ThegtmIdis properly escaped viaJSON.stringify()(line 24), and the source is a controlled environment variable. The GTM initialization code is static and safe.However, note that switching from Next.js
<Script>component to raw<script>tags changes the loading behavior—these scripts now execute synchronously during HTML parsing rather than using Next.js's script optimization strategies.
29-60: LGTM!The Osano consent integration properly handles both synchronous (already loaded) and asynchronous (not yet loaded) scenarios. The
dangerouslySetInnerHTMLusage here contains only static JavaScript with no dynamic content interpolation, making it safe.
65-80: LGTM!The
GtmNoScriptcomponent correctly acceptsgtmIdas a prop and safely interpolates it into the iframe URL using a template literal. The guard clause maintains consistency withGtmScript.studio/src/pages/_document.tsx (2)
38-50: LGTM!The GTM initialization is correctly structured with the default consent configuration loaded before the GTM script. Passing
gtmIdas a prop aligns with the refactored component interface, and the production-only rendering is appropriate.
133-133: LGTM!The
GtmNoScriptcomponent correctly receives thegtmIdprop and is positioned at the top of the body, following Google Tag Manager's recommended implementation pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
studio/src/components/layout/analytics/gtm-script.tsx (1)
1-3: Optional: Remove semicolon after interface for TypeScript convention.The semicolon on line 3 after the closing brace is valid but unconventional. Most TypeScript style guides omit it for interface and type declarations.
Apply this diff:
export interface GtmScriptProps { gtmId: string | undefined; -}; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
studio/src/components/layout/analytics/gtm-script.tsx(3 hunks)
🧰 Additional context used
🪛 ast-grep (0.39.7)
studio/src/components/layout/analytics/gtm-script.tsx
[warning] 13-13: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
[warning] 28-28: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
studio/src/components/layout/analytics/gtm-script.tsx
[error] 14-14: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
[error] 29-29: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
studio/src/components/layout/analytics/gtm-script.tsx (2)
5-61: Implementation is correct; static analysis warnings are false positives.The use of
dangerouslySetInnerHTMLon lines 14 and 29 is flagged by static analysis tools, but this is a legitimate and safe use case:
- All GTM script content is hardcoded and controlled by developers
- The only dynamic value (
gtmId) is properly escaped usingJSON.stringify()on line 22- This follows the standard pattern for integrating third-party analytics scripts per GTM documentation
The conditional rendering (lines 6-8) and the overall implementation are well-structured.
63-78: LGTM!The
GtmNoScriptimplementation correctly handles the conditional rendering and follows GTM documentation for the noscript fallback. The template literal safely injects thegtmIdinto the iframe URL.
wilsonrivera
deleted the
wilson/eng-8446-persist-ad-url-params-and-cookies-during-cosmo-sign-up-in2
branch
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Checklist