Conversation
✅ Deploy Preview for tradetrust-gallery ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds Google Tag Manager integration: new GTM service and type, initializes on app startup, sends events from DocumentPreview and DocumentStatus, adds GTM deps, ignores Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant UI as React UI
participant GTM as gtmService
participant TM as TagManager
Note over UI,GTM: App startup (module eval)
UI->>GTM: import gtmService
alt VITE_GTM_ID present
GTM->>TM: initialize({ gtmId: VITE_GTM_ID })
end
Note over UI: User interactions
U->>UI: Click "Expand Preview"
UI->>GTM: sendGAEvent({ event: "document_preview", preview_title })
alt GTM configured
GTM->>TM: dataLayer({ dataLayer: eventData })
end
U->>UI: Click document status link
UI->>GTM: sendGAEvent({ event: "document_type", document_title, document_schema, document_status })
alt GTM configured
GTM->>TM: dataLayer({ dataLayer: eventData })
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
✨ Finishing touches
🧪 Generate unit tests
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: 1
🧹 Nitpick comments (12)
.gitignore (1)
13-14: Broaden env ignores and keep a tracked sample.Ignoring only
.envand.env.localis easy to miss other variants. Recommend ignoring all env files and committing a.env.example.-.env -.env.local +.env* +!.env.example.github/workflows/ci.yml (1)
24-25: Avoid injecting secrets on PR builds; inject only on pushes to main.Prevents analytics IDs from being present in non-prod builds while keeping PR builds working.
- - name: Build - run: npm run build - env: - VITE_GTM_ID: ${{ secrets.VITE_GTM_ID }} + - name: Build (PR) + if: ${{ github.event_name == 'pull_request' }} + run: npm run build + + - name: Build (main) + if: ${{ github.event_name == 'push' && github.ref == 'refs/heads/main' }} + run: npm run build + env: + VITE_GTM_ID: ${{ secrets.VITE_GTM_ID }}package.json (1)
24-24: Move type-only package to devDependencies.
@types/react-gtm-moduleis compile-time only; keep prod deps lean."dependencies": { - "@types/react-gtm-module": "^2.0.4", "react-gtm-module": "^2.0.11", }, "devDependencies": { + "@types/react-gtm-module": "^2.0.4", }Also applies to: 31-31
tsconfig.json (1)
14-16: Include Node types to avoid accidental loss due to explicit “types”.Specifying
typesrestricts ambient types. Addnodeif any Node globals/types are used (e.g., Buffer in polyfills).- "types": ["vite/client"] + "types": ["vite/client", "node"]src/types/types.ts (1)
31-34: Loosen event payload value types for future-proofing.GA/GTM dataLayer commonly carries numbers/booleans too.
export type GADataLayerEvent = { event: string; -} & Record<string, string>; +} & Record<string, string | number | boolean>;src/components/content/DocumentPreview.tsx (1)
5-5: Only emit analytics in production and lazy-load GTM.Prevents dev/preview noise and avoids loading GTM when not needed.
-import { sendGAEvent } from '../../lib/gtmService'; +import { sendGAEvent } from '../../lib/gtmService'; @@ - onClick={() => { - sendGAEvent({ + onClick={() => { + sendGAEvent({ event: 'document_preview', preview_title: name, }); setExpandPreview(true); }}Additionally, consider gating initialization in
gtmService:// src/lib/gtmService.ts const gtmId = import.meta.env.VITE_GTM_ID; if (import.meta.env.PROD && gtmId && typeof window !== 'undefined') { TagManager.initialize({ gtmId }); } export const sendGAEvent = (eventData: GADataLayerEvent) => { if (import.meta.env.PROD && gtmId && typeof window !== 'undefined') { TagManager.dataLayer({ dataLayer: eventData }); } };Also applies to: 21-27
src/main.tsx (1)
5-5: Conditionally import GTM in production.Reduces bundle impact and avoids analytics in non-prod.
-import './lib/gtmService'; // Import triggers GTM initialization +if (import.meta.env.PROD && import.meta.env.VITE_GTM_ID) { + import('./lib/gtmService'); // Lazy init GTM only in prod with ID +}src/components/content/DocumentStatus.tsx (1)
7-7: Track QR-tile clicks as well; verify no PII in event fields.You’re tracking the link click; consider also capturing the QR tile interaction for parity. Please confirm
title/schemacarry no PII per your analytics policy.Suggested QR-tile tracking:
- <div + <div className="inline-flex items-center justify-center p-2 rounded-xl border border-[#e2e3e7] cursor-pointer" - onClick={() => - onStatusClick({ + onClick={() => { + sendGAEvent({ + event: 'document_qr_click', + document_title: title, + document_schema: selectedSchema, + document_status: status.label, + }); + onStatusClick({ url: status.url[selectedSchema], documentId: id, label: status.label, documentTitle: title, - }) - } + }); + }} >Also applies to: 34-34, 47-54
src/lib/gtmService.ts (4)
4-10: Avoid import-time side effects; gate by browser/prod and make init idempotent.Prevents SSR/test crashes, accidental dev tracking, and duplicate script injection during HMR.
Apply:
-// Initialize GTM immediately when this module is imported -const gtmId = import.meta.env.VITE_GTM_ID; -if (gtmId) { - TagManager.initialize({ - gtmId, - }); -} +const gtmId = import.meta.env.VITE_GTM_ID as string | undefined; +const isBrowser = typeof window !== 'undefined'; +const isProd = import.meta.env.PROD; +let gtmInitialized = false; + +export const initGTM = () => { + if (!gtmInitialized && isBrowser && isProd && gtmId) { + TagManager.initialize({ gtmId }); + gtmInitialized = true; + } +};Outside this file (example in src/main.tsx):
import { initGTM } from 'lib/gtmService'; initGTM();
12-18: Guard sends with init/browser checks and ensure event name is present.Prevents no-op pushes before init and malformed events.
-export const sendGAEvent = (eventData: GADataLayerEvent) => { - if (gtmId) { - TagManager.dataLayer({ - dataLayer: eventData, - }); - } -}; +export const sendGAEvent = (eventData: GADataLayerEvent) => { + if (!isBrowser || !gtmId) return; + if (!('event' in eventData) || !eventData.event) return; + // Optional: if you adopted initGTM above, also guard on gtmInitialized + // if (!gtmInitialized) return; + TagManager.dataLayer({ dataLayer: eventData }); +};Additionally (in src/types/types.ts), future-proof GA payloads:
export type GADataLayerEvent = { event: string; } & Record<string, string | number | boolean>;
1-18: Optional: validate GTM ID format to catch misconfig at startup.Lightweight guard to fail fast in dev without affecting prod.
-const gtmId = import.meta.env.VITE_GTM_ID as string | undefined; +const gtmId = import.meta.env.VITE_GTM_ID as string | undefined; +const isValidGtmId = !gtmId || /^GTM-[A-Z0-9]+$/.test(gtmId); +if (isBrowser && !isValidGtmId && import.meta.env.DEV) { + // eslint-disable-next-line no-console + console.warn('[GTM] Invalid VITE_GTM_ID:', gtmId); +}
1-18: PII guardrails for events (doc only).Confirm that event payloads exclude PII and align with your GA4 taxonomy; consider a small allowlist of keys if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.github/workflows/ci.yml(1 hunks).gitignore(1 hunks)package.json(1 hunks)src/components/content/DocumentPreview.tsx(2 hunks)src/components/content/DocumentStatus.tsx(3 hunks)src/lib/gtmService.ts(1 hunks)src/main.tsx(1 hunks)src/types/types.ts(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/components/content/DocumentPreview.tsx (1)
src/lib/gtmService.ts (1)
sendGAEvent(12-18)
src/components/content/DocumentStatus.tsx (1)
src/lib/gtmService.ts (1)
sendGAEvent(12-18)
src/lib/gtmService.ts (1)
src/types/types.ts (1)
GADataLayerEvent(32-34)
🔇 Additional comments (1)
src/lib/gtmService.ts (1)
1-18: Overall approach LGTM.Thin wrapper over react-gtm-module, environment-driven enablement, and a single entrypoint is a clean integration surface.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/lib/gtmService.ts (1)
2-2: Fix non-relative import: breaks Vite resolution
import { GADataLayerEvent } from 'types/types';relies on TS baseUrl and will fail in Vite unless aliases/plugins are aligned. Switch to a relative import (simplest) or add matching aliases (vite + tsconfig).-import { GADataLayerEvent } from 'types/types'; +import { GADataLayerEvent } from '../types/types';
🧹 Nitpick comments (4)
src/lib/gtmService.ts (4)
5-11: SSR safety and quieter prod logsInitialize GTM only in browser contexts and avoid noisy prod logs.
const gtmId = import.meta.env.VITE_GTM_ID; -if (gtmId) { - console.log('Initializing GTM for analytics'); - TagManager.initialize({ - gtmId, - }); -} +const isBrowser = typeof window !== 'undefined'; +if (isBrowser && gtmId) { + if (import.meta.env.DEV) console.debug('GTM: initializing'); + TagManager.initialize({ gtmId }); +}If the app ever runs under SSR (or tests that simulate it), this prevents “window is not defined.” Can you confirm whether SSR is in use?
13-19: Return success flag and guard dataLayer pushMake the API easier to consume and safer in non-browser contexts.
-export const sendGAEvent = (eventData: GADataLayerEvent) => { - if (gtmId) { - TagManager.dataLayer({ - dataLayer: eventData, - }); - } -}; +export const sendGAEvent = (eventData: GADataLayerEvent): boolean => { + if (!isBrowser || !gtmId) return false; + try { + TagManager.dataLayer({ dataLayer: eventData }); + return true; + } catch (err) { + if (import.meta.env.DEV) console.debug('GTM dataLayer push failed', err); + return false; + } +};
5-11: Gate initialization on user consent (GDPR/CPRA) and env kill-switchInitialize GTM only after user consent via your CMP, and consider a
VITE_ENABLE_GTM=falsekill-switch to disable tracking quickly across environments. This prevents accidental tracking without consent and eases ops rollbacks.
13-19: Event payload flexibility (cross-file: src/types/types.ts lines 31-33)
GADataLayerEventrestricts values tostring; GA4 often uses number/boolean. ConsiderRecord<string, string | number | boolean>to avoid unnecessary stringification.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/gtmService.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/gtmService.ts (1)
src/types/types.ts (1)
GADataLayerEvent(32-34)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/lib/gtmService.ts (5)
1-2: Use a type-only import for the event payload.Slight perf/treeshake nit: import the type with
import typeto avoid emitting runtime code.-import { GADataLayerEvent } from '../types/types'; +import type { GADataLayerEvent } from '../types/types';
4-11: Guard for SSR, initialize once, and keep logs dev-only.Avoid window access on SSR/test runs and double-initialization on HMR; also silence prod logs.
// Initialize GTM immediately when this module is imported -const gtmId = import.meta.env.VITE_GTM_ID; -if (gtmId) { - console.log('Initializing GTM for analytics'); - TagManager.initialize({ - gtmId, - }); -} +const gtmId = import.meta.env.VITE_GTM_ID; +const isClient = typeof window !== 'undefined'; +let initialized = false; +if (isClient && gtmId && !initialized) { + if (import.meta.env.DEV) console.debug('[GTM] initialize'); + TagManager.initialize({ gtmId }); + initialized = true; +}Optional: export an
initGTM()and call it frommain.tsxto remove import side-effects entirely.
13-19: Validate payload and signal success/failure from sender.Add a minimal runtime check and return a boolean for callers to branch on.
-export const sendGAEvent = (eventData: GADataLayerEvent) => { - if (gtmId) { - TagManager.dataLayer({ - dataLayer: eventData, - }); - } -}; +export const sendGAEvent = (eventData: GADataLayerEvent): boolean => { + if (!isClient || !gtmId || !eventData?.event) return false; + TagManager.dataLayer({ dataLayer: eventData }); + return true; +};
13-19: Broaden GA payload value types (numbers/booleans are common).Current type restricts values to string; GA/GTM events often carry numeric/boolean fields (e.g., value, debug_mode).
Apply in src/types/types.ts:
export type GADataLayerEvent = { event: string; -} & Record<string, string>; +} & Record<string, string | number | boolean | null | undefined>;
4-11: Consent-aware initialization (if applicable).If you have a CMP, defer
initializeuntil consent is granted (and disable in jurisdictions where required) to avoid compliance issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/gtmService.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/gtmService.ts (1)
src/types/types.ts (1)
GADataLayerEvent(32-34)
3 participants
Summary
Added changes to support GA tracking using GTM
Summary by CodeRabbit
New Features
Chores