fix: gate analytics scripts on cookie consent#1168
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAnalytics component now short-circuits rendering when cookie consent is not "accepted". Tests added to assert no scripts render for unset or "rejected" consent and that the GTM script plus a rendered Google Analytics element include the gaId when consent is "accepted". Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Analytics
participant GTM_Script
User->>Browser: Load page
Browser->>Analytics: Mount component
Analytics->>Browser: Read localStorage.cookie-consent
alt consent == "accepted"
Analytics->>GTM_Script: Inject external GTM script (data-src includes gaId)
GTM_Script-->>Browser: External script loads
Browser-->>Analytics: Google analytics element rendered with gaId
else consent != "accepted"
Analytics-->>Browser: Return null (no script injected)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/__tests__/components/analytics.test.tsx (1)
19-33: Optional: exercise the post-useEffectstate in the negative cases too.For the "no consent" and "rejected" tests, assertions run synchronously right after
render, so they pass purely because initialhasConsentisfalse—useEffecthasn't had a chance to run yet. If the component is ever refactored to initializehasConsentdifferently (e.g., eager read to avoid first-paint flicker), these tests would silently keep passing while no longer testing the post-effect path. Consider wrapping the negative assertions inwaitForso they also observe the state after the effect fires.♻️ Proposed tweak
- render(<Analytics gaId="G-TEST" />); - - expect(screen.queryByTestId("external-script")).not.toBeInTheDocument(); - expect(screen.queryByTestId("google-analytics")).not.toBeInTheDocument(); + render(<Analytics gaId="G-TEST" />); + + await waitFor(() => { + expect(screen.queryByTestId("external-script")).not.toBeInTheDocument(); + expect(screen.queryByTestId("google-analytics")).not.toBeInTheDocument(); + });(and mark both
itcallbacksasync.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/components/analytics.test.tsx` around lines 19 - 33, The tests for the Analytics component are asserting immediately after render so they only observe the initial hasConsent state (before useEffect runs); make both negative test cases async and wrap the assertions in a waitFor to verify the post-useEffect DOM (i.e., that elements with testIds "external-script" and "google-analytics" remain absent after the effect runs). Ensure you still set localStorage where intended (e.g., "cookie-consent" = "rejected") before render, and reference the Analytics component and its internal hasConsent/useEffect behavior when updating the tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/__tests__/components/analytics.test.tsx`:
- Around line 19-33: The tests for the Analytics component are asserting
immediately after render so they only observe the initial hasConsent state
(before useEffect runs); make both negative test cases async and wrap the
assertions in a waitFor to verify the post-useEffect DOM (i.e., that elements
with testIds "external-script" and "google-analytics" remain absent after the
effect runs). Ensure you still set localStorage where intended (e.g.,
"cookie-consent" = "rejected") before render, and reference the Analytics
component and its internal hasConsent/useEffect behavior when updating the
tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33e141d8-2d2f-4e92-9e85-f4151d1f29a7
📒 Files selected for processing (2)
src/__tests__/components/analytics.test.tsxsrc/components/layout/analytics.tsx
|
Follow-up pushed in |
|
Addressed CodeRabbit's follow-up nitpick on the Analytics regression tests. What changed:
Local validation:
|
1 participant
Summary
Tests
git diff --checkAttempted but local dependencies are not installed in this checkout:
npm test -- --run src/__tests__/components/analytics.test.tsx(vitest: command not found)npx vitest run src/__tests__/components/analytics.test.tsx(fails to load project config because localvitest/configand@vitejs/plugin-reactare not installed)Summary by CodeRabbit
Bug Fixes
Tests