feat(analytics): add extra params prop to gtm #1426
Conversation
Tagging OptionsShould a new tag be published when this PR is merged?
|
WalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Page
participant Analytics
participant GoogleTagManager as GTM
participant Browser
User->>Page: Load page
Page->>Analytics: Render (props include extraParams?)
Analytics->>GTM: Render with extraParams
Note right of GTM #DDEBF7: Build GTM script & iframe URLs\niterate extraParams, skip "id"/"ns", append key=value pairs
GTM->>Browser: Insert <script src="...&key=value..."> and <noscript><iframe src="...&key=value..."></iframe></noscript>
Browser-->>GTM: Load GTM resources
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)website/components/Analytics.tsx (1)
🔇 Additional comments (4)
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 |
pedrobernardina
force-pushed
the
feat/add-extra-params-to-analytics
branch
from
d8b9451 to
6a65c50
Compare
![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)
website/components/Analytics.tsx (1)
35-44: Simplify extraParams processing to improve efficiency.The current implementation creates an intermediate
URLSearchParamsobject only to immediately iterate through it. This adds unnecessary overhead and complexity.Apply this diff to simplify the logic:
- const extraParams = new URLSearchParams( - (props.extraParams ?? []).map((param) => [param.key, param.value]), - ); - - for (const [key, value] of extraParams.entries()) { - if (key && value) { - src.searchParams.append(key, value); - noscript.searchParams.append(key, value); - } - } + (props.extraParams ?? []) + .filter((param) => param.key && param.value) + .forEach((param) => { + src.searchParams.append(param.key, param.value); + noscript.searchParams.append(param.key, param.value); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/components/Analytics.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
website/components/Analytics.tsx (1)
compat/std/sections/Analytics.tsx (1)
Props(34-49)
🔇 Additional comments (2)
website/components/Analytics.tsx (2)
148-151: LGTM! Clean propagation of extraParams.The
extraParamsproperty is correctly added to thePropsinterface, destructured in the function signature, and consistently passed to allGoogleTagManagerinvocations. The optional nature maintains backward compatibility.Also applies to: 157-164, 177-181, 188-192
41-42: Verify duplicate query parameter handling
URLSearchParams.appendproduces multiple entries for identical keys (e.g.,?key=1&key=2). Usesetinstead if duplicates should override. Confirm this is the intended behavior.
File: website/components/Analytics.tsx (lines 41–42)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
website/components/Analytics.tsx (1)
36-36: Consider whether empty values should be allowed.The check
if (key && value)skips parameters with empty strings. While this prevents adding meaningless parameters, it also blocks intentional empty values (e.g.,?debug=).If empty values are a valid use case, consider adjusting the check:
- if (key && value) { + if (key) { src.searchParams.append(key, value); noscript.searchParams.append(key, value); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/components/Analytics.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
website/components/Analytics.tsx (1)
compat/std/sections/Analytics.tsx (1)
Props(34-49)
🔇 Additional comments (3)
website/components/Analytics.tsx (3)
117-127: LGTM! Clear interface definition.The
ExtraParamsinterface is well-defined with appropriate JSDoc annotations. The@titleannotation will provide a helpful UI hint when configuring these parameters.
16-16: Props correctly added to both interfaces.The
extraParamsproperty is consistently added to bothTagManagerPropsand the publicPropsinterface with clear documentation.Also applies to: 144-147
153-189: extraParams properly forwarded to GoogleTagManager.The
Analyticsfunction correctly destructuresextraParamsand passes it to bothGoogleTagManagerinstances (lines 173-177 and 184-188), ensuring consistent behavior across both render paths.
pedrobernardina
force-pushed
the
feat/add-extra-params-to-analytics
branch
from
6a65c50 to
aa2a4f4
Compare
pedrobernardina
force-pushed
the
feat/add-extra-params-to-analytics
branch
from
aa2a4f4 to
204f656
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
website/components/Analytics.tsx (2)
34-45: Harden extraParams handling: trim, case-insensitive reserved check, and use set() to avoid duplicates.
Great call skipping reserved keys. Improve robustness:
- Compare reserved keys case-insensitively.
- Trim key/value and skip empties.
- Use URLSearchParams.set to replace duplicates instead of append.
Also consider hoisting reservedKeys to module scope.Apply:
- const reservedKeys = new Set(["id", "ns"]); + const reservedKeys = new Set(["id", "ns"]); // case-insensitive check below - for (const { key, value } of (props.extraParams ?? [])) { - if (reservedKeys.has(key)) { - continue; - } - - if (key && value) { - src.searchParams.append(key, value); - noscript.searchParams.append(key, value); - } - } + for (const { key, value } of (props.extraParams ?? [])) { + const k = key?.trim(); + const v = value?.trim(); + if (!k || !v) continue; + if (reservedKeys.has(k.toLowerCase())) continue; + // Replace any existing value rather than duplicating + src.searchParams.set(k, v); + noscript.searchParams.set(k, v); + }
121-132: Consider exporting ExtraParams for reuse/introspection.
If other modules or schema tooling need this type, export it; otherwise OK to keep internal.-interface ExtraParams { +export interface ExtraParams {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/components/Analytics.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
website/components/Analytics.tsx (1)
compat/std/sections/Analytics.tsx (1)
Props(34-49)
🔇 Additional comments (5)
website/components/Analytics.tsx (5)
16-16: Prop added to TagManagerProps looks good.
Wiring for extraParams starts here; type choice is clear.
149-152: Prop added to public Props looks good.
Docs are clear; no issues.
159-165: Destructuring update LGTM.
No behavior change; keeps props flow simple.
178-182: Pass-through of extraParams to GoogleTagManager LGTM.
Correctly forwards in the trackingIds path.
189-193: Fallback pass-through LGTM.
Correctly forwards when using src without trackingIds.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
website/components/Analytics.tsx (1)
34-45: Consider case-insensitive reserved key check.The reserved keys check uses case-sensitive matching. If a user accidentally passes
"ID"or"Id", it would bypass the check and potentially create issues with GTM.Apply this diff to make the check case-insensitive:
- const reservedKeys = new Set(["id", "ns"]); + const reservedKeys = new Set(["id", "ns"]); for (const { key, value } of (props.extraParams ?? [])) { - if (reservedKeys.has(key)) { + if (reservedKeys.has(key.toLowerCase())) { continue; }Additionally, consider expanding the reserved set to include other GTM parameters such as
"gtm_auth","gtm_preview","gtm_cookies_win", and"l"if you want to prevent users from overriding those as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/components/Analytics.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
website/components/Analytics.tsx (1)
compat/std/sections/Analytics.tsx (1)
Props(34-49)
🔇 Additional comments (4)
website/components/Analytics.tsx (4)
16-17: LGTM!The optional
extraParamsproperty is properly typed and integrated into the interface.
122-132: LGTM!The
ExtraParamsinterface is well-documented with helpful JSDoc comments and includes a nice@titleannotation for improved UI presentation.
149-152: LGTM!The
extraParamsproperty is properly documented and consistently typed withTagManagerProps.
159-194: LGTM!The
Analyticscomponent properly destructuresextraParamsfrom props and consistently forwards it to bothGoogleTagManagerinvocations. The implementation correctly handles both the primary tracking IDs path (lines 178-182) and the fallback src-only path (lines 189-193).
pedrobernardina
force-pushed
the
feat/add-extra-params-to-analytics
branch
from
204f656 to
500bcba
Compare
2 participants
What is this Contribution About?
This contribution introduces a new extraParams prop to the GTM integration, allowing additional query parameters (e.g., version, source, etc.) to be appended dynamically to the GTM script and noscript URLs.
This makes the setup more flexible and prevents the need for hardcoded parameters in future updates.
Summary by CodeRabbit