fix: google tag manager script #955
Conversation
|
Warning: Component files have been updated but no migrations have been added. See https://github.com/yext/visual-editor/blob/main/packages/visual-editor/src/components/migrations/README.md for more information. |
WalkthroughAdds a new GTMBody React component that reads visual editor config via useDocument, safely parses and validates googleTagManagerId, and conditionally injects a noscript Google Tag Manager iframe before rendering its children. applyAnalytics parsing was hardened to guard against invalid JSON and may return GTM or GA4 script blocks based on config. GTMBody is exported and used to wrap the Render component in the directory, locator, and main templates; when the GTM ID is absent or invalid, GTMBody renders only its children. Sequence Diagram(s)sequenceDiagram
participant Template as "Template (directory / locator / main)"
participant GTMBody as "GTMBody"
participant Doc as "Document (useDocument)"
participant GTM as "Google Tag Manager (noscript iframe)"
participant Render as "Render (children)"
Template->>GTMBody: render (wrap Render)
GTMBody->>Doc: read streamDocument.__.visualEditorConfig
Doc-->>GTMBody: visualEditorConfig (string | undefined)
alt config missing or parse fails
GTMBody-->>Template: render Render (children) only
else config parsed
GTMBody->>GTM: validate googleTagManagerId (GTM-[A-Z0-9]+)
alt id valid
GTMBody->>GTM: inject noscript iframe with GTM-<id>
GTMBody-->>Template: render iframe + Render (children)
else id missing or invalid
GTMBody-->>Template: render Render (children) only
end
end
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-12-12T19:26:15.855ZApplied to files:
🧬 Code graph analysis (1)packages/visual-editor/src/utils/applyAnalytics.ts (1)
⏰ 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)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/visual-editor/src/utils/applyAnalytics.ts (1)
1-38: Add error handling for JSON parsing and validate IDs to prevent XSS.The function lacks error handling for
JSON.parse, which can throw ifvisualEditorConfigcontains malformed JSON. Additionally, the GTM and GA4 IDs are directly interpolated into script tags without validation, creating an XSS risk if the config is compromised.GTM IDs follow the pattern GTM-[A-Z0-9]+ and GA4 IDs follow G-[A-Z0-9]+. All variables in a web application need to be protected through validation and escaping, known as perfect injection resistance.
This pattern also appears in
applyHeaderScript.ts(line 6), which has the same unprotectedJSON.parsecall.Consider:
- Wrapping
JSON.parsein a try-catch block (similar to the pattern inapplyTheme.ts)- Validating that IDs match their expected formats before interpolation into script tags
🧹 Nitpick comments (1)
packages/visual-editor/src/components/GTMBody.tsx (1)
8-39: Consider memoizing the parsed configuration.The component parses JSON on every render. If the component re-renders frequently, consider using
useMemoto parse the configuration only whenstreamDocumentchanges.🔎 Proposed optimization
+import React, { useMemo } from "react"; import { useDocument } from "@yext/visual-editor"; export const GTMBody: React.FC<{ children: React.ReactNode }> = ({ children, }) => { const streamDocument = useDocument(); - if (!streamDocument?.__?.visualEditorConfig) { - return <>{children}</>; - } - - const googleTagManagerId: string = JSON.parse( - streamDocument.__.visualEditorConfig - )?.googleTagManagerId; + const googleTagManagerId = useMemo(() => { + if (!streamDocument?.__?.visualEditorConfig) { + return null; + } + + try { + return JSON.parse(streamDocument.__.visualEditorConfig)?.googleTagManagerId; + } catch (e) { + console.error('Failed to parse visualEditorConfig:', e); + return null; + } + }, [streamDocument]); - if (!googleTagManagerId) { + if (!googleTagManagerId || !/^GTM-[A-Z0-9]+$/.test(googleTagManagerId)) { return <>{children}</>; } return ( <> {/* Google Tag Manager (noscript) */} <iframe src={`https://www.googletagmanager.com/ns.html?id=${googleTagManagerId}`} height="0" width="0" style={{ display: "none", visibility: "hidden" }} ></iframe> {/* End Google Tag Manager (noscript) */} {children} </> ); };
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/visual-editor/src/components/GTMBody.tsx(1 hunks)packages/visual-editor/src/components/index.ts(1 hunks)packages/visual-editor/src/utils/applyAnalytics.ts(1 hunks)packages/visual-editor/src/vite-plugin/templates/directory.tsx(2 hunks)packages/visual-editor/src/vite-plugin/templates/locator.tsx(2 hunks)packages/visual-editor/src/vite-plugin/templates/main.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-06T14:55:12.395Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 862
File: packages/visual-editor/src/utils/schema/resolveSchema.ts:118-135
Timestamp: 2025-11-06T14:55:12.395Z
Learning: In `packages/visual-editor/src/utils/schema/resolveSchema.ts`, the `OpeningHoursSchema` and `PhotoGallerySchema` functions from `yext/pages-components` contain internal type validation and handle invalid inputs gracefully (returning empty objects or undefined) rather than throwing TypeErrors, so no pre-validation guards are needed before calling them.
Applied to files:
packages/visual-editor/src/vite-plugin/templates/main.tsxpackages/visual-editor/src/vite-plugin/templates/directory.tsxpackages/visual-editor/src/vite-plugin/templates/locator.tsx
📚 Learning: 2025-10-29T22:00:03.843Z
Learnt from: mkouzel-yext
Repo: yext/visual-editor PR: 833
File: packages/visual-editor/src/components/Locator.tsx:1050-1057
Timestamp: 2025-10-29T22:00:03.843Z
Learning: In packages/visual-editor/src/components/Locator.tsx, the AppliedFilters component is intentionally rendered in two locations (inside the filter modal and outside it) as per the design requirements. This dual rendering should not be flagged as a duplicate issue.
Applied to files:
packages/visual-editor/src/vite-plugin/templates/locator.tsx
🧬 Code graph analysis (2)
packages/visual-editor/src/components/GTMBody.tsx (1)
packages/visual-editor/src/components/index.ts (1)
GTMBody(28-28)
packages/visual-editor/src/vite-plugin/templates/locator.tsx (3)
packages/visual-editor/src/components/GTMBody.tsx (1)
GTMBody(8-39)packages/visual-editor/src/components/index.ts (1)
GTMBody(28-28)packages/visual-editor/src/components/configs/locatorConfig.tsx (1)
locatorConfig(33-73)
⏰ 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: call_unit_test / unit_tests (20.x)
- GitHub Check: call_unit_test / unit_tests (18.x)
- GitHub Check: semgrep/ci
🔇 Additional comments (4)
packages/visual-editor/src/vite-plugin/templates/directory.tsx (1)
28-28: LGTM! GTM integration correctly applied.The
GTMBodycomponent is properly imported and wraps theRendercomponent, which will inject the GTM noscript iframe when a validgoogleTagManagerIdis present. This pattern is consistent across all templates in the PR.Also applies to: 132-138
packages/visual-editor/src/vite-plugin/templates/main.tsx (1)
29-29: LGTM! GTM integration correctly applied.The
GTMBodycomponent properly wraps theRendercomponent, maintaining consistency with the other templates in this PR.Also applies to: 134-140
packages/visual-editor/src/components/index.ts (1)
28-28: LGTM! Public API extension.The
GTMBodycomponent is now properly exported for use across the visual editor.packages/visual-editor/src/vite-plugin/templates/locator.tsx (1)
28-28: LGTM! GTM integration correctly applied.The
GTMBodywrapper is consistent with the implementation in the directory and main templates.Also applies to: 143-149
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/visual-editor/src/components/GTMBody.tsx (1)
27-27: Improve type accuracy for optional property.The
googleTagManagerIdis typed asstringbut will beundefinedif the property doesn't exist in the parsed config. Consider usingstring | undefinedor removing the explicit type annotation to let TypeScript infer the optional type.🔎 Proposed refactor
- const googleTagManagerId: string = visualEditorConfig?.googleTagManagerId; + const googleTagManagerId = visualEditorConfig?.googleTagManagerId as string | undefined;Or simply:
- const googleTagManagerId: string = visualEditorConfig?.googleTagManagerId; + const googleTagManagerId = visualEditorConfig?.googleTagManagerId;packages/visual-editor/src/utils/applyAnalytics.ts (1)
31-42: Add GA4 ID validation for future-proofing.Similar to the GTM ID, the
googleAnalyticsIdshould be validated before injection into script tags. GA4 measurement IDs follow the patternG-XXXXXXXXXXorGT-XXXXXXXXX.🔎 Proposed fix
// Google Analytics 4 (GA4) // Note that this does not yet exist in platform. Adding for future support. - const googleAnalyticsId: string = visualEditorConfig?.googleAnalyticsId; + const googleAnalyticsId = visualEditorConfig?.googleAnalyticsId; - if (googleAnalyticsId) { + if (googleAnalyticsId && /^G(T)?-[A-Z0-9]+$/.test(googleAnalyticsId)) { return `<!-- Google tag (gtag.js) --> <script async src="https://www.googletagmanager.com/gtag/js?id=${googleAnalyticsId}"></script> <script> window.dataLayer = window.dataLayer || []; function gtag(){dataLayer.push(arguments);} gtag('js', new Date()); gtag('config', '${googleAnalyticsId}'); </script>`; } };
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/visual-editor/src/components/GTMBody.tsx(1 hunks)packages/visual-editor/src/utils/applyAnalytics.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T19:26:15.855Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 942
File: packages/visual-editor/src/utils/schema/schemaBlocks.ts:29-35
Timestamp: 2025-12-12T19:26:15.855Z
Learning: In packages/visual-editor/src/utils/schema/schemaBlocks.ts, when siteDomain is missing (non-live/preview environments), schema id fields should use relative URLs rather than bare fragments, since Google will only crawl production sites where siteDomain is populated.
Applied to files:
packages/visual-editor/src/components/GTMBody.tsx
🧬 Code graph analysis (1)
packages/visual-editor/src/components/GTMBody.tsx (1)
packages/visual-editor/src/components/index.ts (1)
GTMBody(28-28)
⏰ 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: call_unit_test / unit_tests (20.x)
- GitHub Check: call_unit_test / unit_tests (18.x)
- GitHub Check: semgrep/ci
🔇 Additional comments (1)
packages/visual-editor/src/utils/applyAnalytics.ts (1)
6-14: LGTM: Duplicate parsing eliminated.The guarded JSON parsing addresses the previous review feedback by parsing once and reusing the config object. The try-catch with early return ensures safe handling of malformed JSON.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/visual-editor/src/utils/applyAnalytics.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T19:26:15.855Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 942
File: packages/visual-editor/src/utils/schema/schemaBlocks.ts:29-35
Timestamp: 2025-12-12T19:26:15.855Z
Learning: In packages/visual-editor/src/utils/schema/schemaBlocks.ts, when siteDomain is missing (non-live/preview environments), schema id fields should use relative URLs rather than bare fragments, since Google will only crawl production sites where siteDomain is populated.
Applied to files:
packages/visual-editor/src/utils/applyAnalytics.ts
⏰ 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: call_unit_test / unit_tests (18.x)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: semgrep/ci
🔇 Additional comments (2)
packages/visual-editor/src/utils/applyAnalytics.ts (2)
6-14: LGTM! Robust error handling addresses previous concerns.The try/catch block with early return properly addresses the duplicate JSON parsing issue flagged in previous reviews. The warning message is clear and helpful for debugging.
16-27: LGTM! GTM validation properly addresses XSS concerns.The regex validation
/^GTM-[A-Z0-9]+$/before script injection addresses the security concern from previous reviews and ensures consistency withGTMBody.tsx. The GTM script implementation follows the standard pattern.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/visual-editor/src/components/GTMBody.tsx (1)
17-30: Consider simplifying by removinguseMemo.The
useMemohook may be premature optimization here since:
JSON.parseis not particularly expensive for config objects- The result is immediately extracted to a primitive string on line 32
streamDocumentfromuseDocumentis likely stable across rendersThe code could be simplified by parsing inline while keeping the error handling.
🔎 Potential simplification
- const visualEditorConfig: Record<string, any> | null = useMemo(() => { - if (!streamDocument?.__?.visualEditorConfig) { - return null; - } - - try { - return JSON.parse(streamDocument.__.visualEditorConfig); - } catch (_) { - console.warn( - "Failed to parse visualEditorConfig for GTM. Skipping adding GTM iframe." - ); - return null; - } - }, [streamDocument]); + let visualEditorConfig: Record<string, any> | null = null; + if (streamDocument?.__?.visualEditorConfig) { + try { + visualEditorConfig = JSON.parse(streamDocument.__.visualEditorConfig); + } catch (_) { + console.warn( + "Failed to parse visualEditorConfig for GTM. Skipping adding GTM iframe." + ); + } + } const googleTagManagerId: string = visualEditorConfig?.googleTagManagerId;However, if
streamDocumentcan change frequently during the component lifecycle, keepinguseMemomay be beneficial.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/visual-editor/src/components/GTMBody.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T19:26:15.855Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 942
File: packages/visual-editor/src/utils/schema/schemaBlocks.ts:29-35
Timestamp: 2025-12-12T19:26:15.855Z
Learning: In packages/visual-editor/src/utils/schema/schemaBlocks.ts, when siteDomain is missing (non-live/preview environments), schema id fields should use relative URLs rather than bare fragments, since Google will only crawl production sites where siteDomain is populated.
Applied to files:
packages/visual-editor/src/components/GTMBody.tsx
🧬 Code graph analysis (1)
packages/visual-editor/src/components/GTMBody.tsx (1)
packages/visual-editor/src/components/index.ts (1)
GTMBody(28-28)
🪛 Biome (2.1.2)
packages/visual-editor/src/components/GTMBody.tsx
[error] 17-17: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ 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: call_unit_test / unit_tests (20.x)
- GitHub Check: call_unit_test / unit_tests (18.x)
- GitHub Check: semgrep/ci
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/visual-editor/src/components/GTMBody.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-12T19:26:15.855Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 942
File: packages/visual-editor/src/utils/schema/schemaBlocks.ts:29-35
Timestamp: 2025-12-12T19:26:15.855Z
Learning: In packages/visual-editor/src/utils/schema/schemaBlocks.ts, when siteDomain is missing (non-live/preview environments), schema id fields should use relative URLs rather than bare fragments, since Google will only crawl production sites where siteDomain is populated.
Applied to files:
packages/visual-editor/src/components/GTMBody.tsx
📚 Learning: 2025-12-15T20:59:25.571Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 943
File: packages/visual-editor/src/components/contentBlocks/image/Image.tsx:175-191
Timestamp: 2025-12-15T20:59:25.571Z
Learning: In the ImageWrapper component at packages/visual-editor/src/components/contentBlocks/image/Image.tsx, when an Image is wrapped in a MaybeLink, the aria-label should be omitted because the image's alt text serves as the accessible name for the link. Adding an aria-label would override the image's alt text.
Applied to files:
packages/visual-editor/src/components/GTMBody.tsx
⏰ 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: call_unit_test / unit_tests (18.x)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: semgrep/ci
🔇 Additional comments (1)
packages/visual-editor/src/components/GTMBody.tsx (1)
30-30: The GTM ID regex pattern is correct. Google Tag Manager container IDs follow the formatGTM-followed exclusively by uppercase letters and digits, making the validation/^GTM-[A-Z0-9]+$/appropriate and not overly restrictive.
We were incorrectly adding the GA4 script instead of GTM. I left GA4 support in case we want it in the future.