fix: DR-6518 UTM persistence #7384
Conversation
tentative fix on docs utm persistence
Dangerous URL checkNo absolute URLs to prisma.io/docs found. |
WalkthroughAdds a new client-side Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
Redirect checkThis PR probably requires the following redirects to be added to static/_redirects:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/theme/NavbarItem/NavbarNavLink.tsx (1)
38-55: Consider using the URL API for more robust parsing.After fixing the hash fragment issue, you could further improve robustness by using the native URL API. This handles all edge cases automatically and is more maintainable.
Example refactor using URL API:
const appendUtmParams = (url: string): string => { if (!utmParams) { return url; } try { // URL API requires absolute URLs for parsing, so we use a base const urlObj = new URL(url, 'http://placeholder'); // Append each UTM parameter const params = new URLSearchParams(utmParams); params.forEach((value, key) => { urlObj.searchParams.append(key, value); }); // Return the pathname, search, and hash (without the placeholder origin) return `${urlObj.pathname}${urlObj.search}${urlObj.hash}`; } catch (e) { // Fallback to original URL if parsing fails return url; } };Note: This approach is more robust but requires that
utmParamsis in the formatparam1=value1¶m2=value2.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
content/300-accelerate/index.mdx(0 hunks)content/700-optimize/index.mdx(0 hunks)src/hooks/useUTMParams.ts(1 hunks)src/theme/NavbarItem/NavbarNavLink.tsx(3 hunks)
💤 Files with no reviewable changes (2)
- content/700-optimize/index.mdx
- content/300-accelerate/index.mdx
🧰 Additional context used
🧬 Code graph analysis (1)
src/theme/NavbarItem/NavbarNavLink.tsx (1)
src/hooks/useUTMParams.ts (1)
useUTMParams(3-15)
⏰ 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). (2)
- GitHub Check: Check internal links
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
src/theme/NavbarItem/NavbarNavLink.tsx (3)
35-36: LGTM! Hook usage is correct.The UTM parameters are correctly retrieved from the custom hook.
79-102: LGTM! External/internal link handling is correct.The logic correctly differentiates between external and internal links, applying UTM parameters only to internal navigation. The use of
prependBaseUrlToHrefis handled appropriately.
104-119: LGTM! The 'to' prop handling is correct.UTM parameters are correctly appended to the
toUrl, and all navigation props are preserved appropriately.
Deploying docs with
|
| Latest commit: |
fc8bf26
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ea303d86.docs-51g.pages.dev |
| Branch Preview URL: | https://fix-dr-6518-docs-utm-persist.docs-51g.pages.dev |
carlagn
requested review from
AmanVarshney01,
ArthurGamby,
aidankmcalister,
ankur-arch,
mhartington,
mhessdev and
nurul3101
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/useUTMPersistenceDocs.ts (1)
40-44: Pathname comparison logic appears incorrect.The condition
location.pathname === previousSearch.current.split('?')[0]will never work as intended.previousSearch.currentstoreslocation.search(e.g.,"?utm_source=foo"), so splitting by'?'and taking index0yields an empty string—not the pathname.Also, the
console.logon line 43 should be removed before merging to production.If the intent is to detect manual URL bar edits on the same page, you'd need to track the pathname separately:
const previousSearch = useRef(''); +const previousPathname = useRef(''); // ... inside useEffect, update both refs: -previousSearch.current = location.search; +previousSearch.current = location.search; +previousPathname.current = location.pathname; // ... and fix the comparison: -if (hadUTMs && !hasUTMs && location.pathname === previousSearch.current.split('?')[0]) { +if (hadUTMs && !hasUTMs && location.pathname === previousPathname.current) {
♻️ Duplicate comments (1)
src/hooks/useUTMParams.ts (1)
9-12: Validate and sanitize stored UTM parameters.The hook reads from sessionStorage without validation. If the stored value is malformed or tampered with, it could break URL construction or introduce injection risks downstream. A previous review flagged this same concern.
🧹 Nitpick comments (3)
src/utils/useUTMPersistenceDocs.ts (1)
52-54: Redundant ternary check.On line 54,
storedParams ? \?${storedParams}` : ''is inside a block that already guaranteesstoredParams` is truthy (line 53). The ternary is unnecessary.- const newSearch = storedParams ? `?${storedParams}` : ''; + const newSearch = `?${storedParams}`;src/theme/Navbar/Content/index.tsx (1)
70-75: DuplicatedappendUtmParamshelper.This same helper function is defined identically in
src/theme/Logo/index.tsx(lines 56-63). Consider extracting it to a shared utility—perhaps alongsideuseUTMParamsor insrc/utils/—to keep things DRY and ensure consistent behavior if the logic ever needs adjustment.Example extraction to
src/utils/utmHelpers.ts:export const appendUtmParams = (url: string, utmParams: string): string => { if (!utmParams) return url; const separator = url.includes('?') ? '&' : '?'; return `${url}${separator}${utmParams}`; };Then import and use in both components.
src/hooks/useUTMParams.ts (1)
27-33: Polling pattern has cleanup edge case and is somewhat heavy-handed.Two concerns here:
Memory leak potential: The
setTimeoutat line 31-33 captures the interval ID, but if the component unmounts before the 2-second timeout fires, thesetTimeoutcallback still runs after cleanup—though harmless sinceclearIntervalon an already-cleared ID is a no-op.Polling for 2s at 100ms is aggressive: This fires 20 times. Consider a lighter approach—perhaps a single
requestIdleCallbackorsetTimeoutretry pattern, or using a custom event dispatch from the persistence hook when it writes to sessionStorage (which would be synchronous and avoid polling entirely).A cleaner same-tab sync approach—dispatch a custom event when writing:
// In useUTMPersistenceDocs.ts when setting: sessionStorage.setItem('utm_params', utms); window.dispatchEvent(new Event('utm_params_updated')); // In useUTMParams.ts, listen for it: window.addEventListener('utm_params_updated', updateUTMParams);This eliminates polling entirely.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/hooks/useUTMParams.ts(1 hunks)src/theme/Logo/index.tsx(1 hunks)src/theme/Navbar/Content/index.tsx(3 hunks)src/utils/useUTMPersistenceDocs.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/utils/useUTMPersistenceDocs.ts (1)
src/theme/DocItem/Content/index.js (1)
location(30-30)
src/theme/Logo/index.tsx (1)
src/hooks/useUTMParams.ts (1)
useUTMParams(3-43)
src/theme/Navbar/Content/index.tsx (1)
src/hooks/useUTMParams.ts (1)
useUTMParams(3-43)
⏰ 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). (2)
- GitHub Check: Check internal links
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
src/utils/useUTMPersistenceDocs.ts (1)
30-31: LGTM on the refactor.Extracting
utmsinto an intermediate variable before storing is a clean, readable change. No functional difference, just slightly clearer intent.src/theme/Navbar/Content/index.tsx (1)
62-62: UTM integration looks correct.The hook usage and application to the docs link is straightforward and follows the established pattern. The separator logic correctly handles URLs with or without existing query parameters.
Also applies to: 85-85
src/theme/Logo/index.tsx (2)
43-63: Well-structured Logo component with UTM support.The component cleanly integrates UTM persistence while preserving all standard Docusaurus logo behavior (themed images, alt text fallbacks, target forwarding). The
appendUtmParamshelper here is identical to the one inNavbar/Content/index.tsx—as noted there, extracting it to a shared utility would reduce duplication.
65-71: Alt text logic is thoughtfully handled.Good attention to accessibility: when
navbarTitleis visible, the logo gets an empty alt (decorative), otherwise it uses the site title. This follows WCAG guidance for avoiding redundant announcements.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/useUTMPersistenceDocs.ts (1)
36-43: Manual UTM removal detection condition can never be trueRight now
previousSearch.currentstores onlylocation.search, but the detection uses:if (hadUTMs && !hasUTMs && location.pathname === previousSearch.current.split('?')[0]) {Since
previousSearch.currentis something like'?utm_source=…',previousSearch.current.split('?')[0]is always'', so this condition never matches and manual removals are never honored. That means users who delete UTMs from the URL will still have them auto-restored fromsessionStorage.Consider tracking the previous pathname separately (e.g.,
previousPathnameRef) or storingpathname + searchtogether so you can accurately detect “same page, UTMs removed” and avoid re-applying them.
♻️ Duplicate comments (1)
src/theme/NavbarItem/NavbarNavLink.tsx (1)
35-52: Handle hash fragments when appending UTM paramsThe new
appendUtmParamshelper fixes the logging concern and centralizes the UTM logic, but it still doesn’t account for#fragments:
- For a URL like
/docs/guide#section,url.split('?')leaves the hash attached tobaseUrl, so you end up with/docs/guide#section?utm_source=…, where the query appears after the fragment. In that form, the UTMs won’t be part of the actual URL query seen by the server or analytics.To fix this robustly, split off the hash first, then work with the pre-fragment part, and finally reattach the hash:
- const appendUtmParams = (url: string): string => { - if (!utmParams) { - return url; - } - - const [baseUrl, existingQuery] = url.split('?'); - if (existingQuery) { - const result = `${baseUrl}?${existingQuery}&${utmParams}`; - return result; - } else { - const result = `${baseUrl}?${utmParams}`; - return result; - } - }; + const appendUtmParams = (url: string): string => { + if (!utmParams) { + return url; + } + + // Separate hash fragment so query params are always before '#' + const hashIndex = url.indexOf('#'); + const hash = hashIndex !== -1 ? url.slice(hashIndex) : ''; + const urlWithoutHash = hashIndex !== -1 ? url.slice(0, hashIndex) : url; + + const [baseUrl, existingQuery] = urlWithoutHash.split('?'); + const queryString = existingQuery + ? `${existingQuery}&${utmParams}` + : utmParams; + + return `${baseUrl}?${queryString}${hash}`; + };This will correctly handle
hrefandtoURLs that include fragments while preserving your new UTM behavior.Also applies to: 88-91, 101-105
🧹 Nitpick comments (1)
src/utils/useUTMPersistenceDocs.ts (1)
30-31: Intermediateutmsvariable is fine but not strictly necessaryThis refactor is behaviorally identical and clear; no issues from a persistence standpoint. You could inline
getUTMParams(location.search)back intosetItemif you prefer to keep the effect slightly more compact, but it’s purely stylistic.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
functions/_middleware.ts(0 hunks)src/hooks/useUTMParams.ts(1 hunks)src/theme/NavbarItem/NavbarNavLink.tsx(3 hunks)src/utils/useUTMPersistenceDocs.ts(1 hunks)
💤 Files with no reviewable changes (1)
- functions/_middleware.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/useUTMParams.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/utils/useUTMPersistenceDocs.ts (1)
src/theme/DocItem/Content/index.js (1)
location(30-30)
src/theme/NavbarItem/NavbarNavLink.tsx (1)
src/hooks/useUTMParams.ts (1)
useUTMParams(3-42)
⏰ 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). (2)
- GitHub Check: Check internal links
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
src/theme/NavbarItem/NavbarNavLink.tsx (1)
77-86: External links correctly bypass UTM augmentationThe early return for
isExternalLinkkeeps external URLs untouched while still benefiting from the shared link content props. That keeps UTM behavior scoped to internal navigation, which is typically what you want for docs UTMs.
🍈 Lychee Link Check Report
📊 Results Overview
Errors per inputErrors in 200-orm/050-overview/500-databases/840-cockroachdb.mdx |
carlagn
dismissed stale reviews from coderabbitai[bot] and ArthurGamby
via
76c4fc7
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/theme/NavbarItem/NavbarNavLink.tsx (1)
37-50: Hash fragments are still not handled correctly.The
appendUtmParamsfunction splits only on?, which breaks URLs containing hash fragments. For a URL like/docs/guide#section, the result would be/docs/guide#section?utm_source=x— query parameters must precede the fragment.Apply this diff to properly handle hash fragments:
const appendUtmParams = (url: string): string => { if (!utmParams) { return url; } - const [baseUrl, existingQuery] = url.split('?'); - if (existingQuery) { - const result = `${baseUrl}?${existingQuery}&${utmParams}`; - return result; - } else { - const result = `${baseUrl}?${utmParams}`; - return result; - } + const hashIndex = url.indexOf('#'); + const hash = hashIndex !== -1 ? url.slice(hashIndex) : ''; + const urlWithoutHash = hashIndex !== -1 ? url.slice(0, hashIndex) : url; + + const [baseUrl, existingQuery] = urlWithoutHash.split('?'); + const queryString = existingQuery ? `${existingQuery}&${utmParams}` : utmParams; + + return `${baseUrl}?${queryString}${hash}`; };
🧹 Nitpick comments (2)
src/utils/useUTMPersistenceDocs.ts (2)
74-82: Duplicated logic betweenhandlePopStateandhandleHashChange.The UTM check-and-clear logic is identical in both handlers. Extracting this into a helper function improves maintainability and reduces the risk of the two diverging over time.
+ const clearStoredUTMsIfNeeded = () => { + const currentHasUTMs = hasUTMParams(window.location.search); + const storedParams = sessionStorage.getItem('utm_params'); + + if (!currentHasUTMs && storedParams) { + sessionStorage.removeItem('utm_params'); + isManualRemoval.current = true; + } + }; const handlePopState = () => { - setTimeout(() => { - const currentHasUTMs = hasUTMParams(window.location.search); - const storedParams = sessionStorage.getItem('utm_params'); - - if (!currentHasUTMs && storedParams) { - sessionStorage.removeItem('utm_params'); - isManualRemoval.current = true; - } - }, 50); + requestAnimationFrame(clearStoredUTMsIfNeeded); }; - const handleHashChange = () => { - const currentHasUTMs = hasUTMParams(window.location.search); - const storedParams = sessionStorage.getItem('utm_params'); - - if (!currentHasUTMs && storedParams) { - sessionStorage.removeItem('utm_params'); - isManualRemoval.current = true; - } - }; + const handleHashChange = clearStoredUTMsIfNeeded;
87-95: Multiple overlapping polling intervals for UTM state.This 1000ms interval runs alongside the 500ms interval in
useUTMParams(seesrc/hooks/useUTMParams.tslines 22-23). Both are polling sessionStorage for UTM state changes. This creates:
- Extra CPU overhead from two separate timers
- Potential race conditions when both read/modify sessionStorage in close succession
Consider consolidating the polling logic into a single location, or using a shared event-based approach (e.g., dispatch a custom event when UTMs change) instead of multiple independent intervals.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/hooks/useUTMParams.ts(1 hunks)src/theme/Logo/index.tsx(1 hunks)src/theme/NavbarItem/NavbarNavLink.tsx(3 hunks)src/utils/useUTMPersistenceDocs.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/hooks/useUTMParams.ts
- src/theme/Logo/index.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/theme/NavbarItem/NavbarNavLink.tsx (1)
src/hooks/useUTMParams.ts (1)
useUTMParams(3-34)
src/utils/useUTMPersistenceDocs.ts (1)
src/theme/DocItem/Content/index.js (1)
location(30-30)
⏰ 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). (2)
- GitHub Check: Check internal links
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
src/theme/NavbarItem/NavbarNavLink.tsx (1)
74-93: Good separation of external vs internal link handling.The logic correctly bypasses UTM appending for external links (line 74-81) while applying it to internal
href-based links (lines 84-93). This is a sensible approach — UTM persistence is typically only relevant for internal navigation tracking.src/utils/useUTMPersistenceDocs.ts (1)
45-56: UTM restoration replaces the entire query string.When restoring UTMs (line 48), the logic constructs
newSearch = ?${storedParams}, which means any other non-UTM query parameters that might have been in the URL will be lost. If this is intentional (only UTM params should persist), this is fine. If other query params should be preserved, this needs adjustment.Is dropping non-UTM query parameters during restoration the intended behavior?
🍈 Lychee Link Check Report
📊 Results Overview
Errors per inputErrors in 200-orm/800-more/350-ai-tools/100-cursor.mdx
Errors in 800-guides/380-vercel-app-deployment.mdx
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/utils/useUTMPersistenceDocs.ts (1)
22-40: Excellent simplification that eliminates the previous race conditions.This refactored approach is much cleaner than the previous version with intervals and multiple effects. The single-effect model with pathname tracking is easier to reason about. Let me walk through why it works:
- On initial landing with UTMs:
currentUTMsis truthy → stored immediately- On navigation without UTMs:
isNavigationis true → UTMs restored viahistory.replace- After the replace: Effect re-runs, now
currentUTMsexists → re-stores (idempotent)- Manual URL edit on same page:
isNavigationis false → storage cleared appropriatelyOne small observation:
historyis used inside the effect but isn't listed in the dependency array. While Docusaurus'shistoryobject is stable across renders (so this works in practice), the exhaustive-deps lint rule would flag it. Consider adding it for completeness:- }, [location.pathname, location.search, location.hash]); + }, [history, location.pathname, location.search, location.hash]);Also, the
!currentUTMscheck on line 35 is technically redundant since we're already in theelsebranch fromif (currentUTMs), though it does improve readability by making the condition explicit.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/theme/Layout/index.tsx(0 hunks)src/utils/useUTMPersistenceDocs.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/theme/Layout/index.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/utils/useUTMPersistenceDocs.ts (1)
src/theme/DocItem/Content/index.js (1)
location(30-30)
⏰ 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). (2)
- GitHub Check: Check internal links
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
src/utils/useUTMPersistenceDocs.ts (2)
4-5: Clean constant definitions with good type safety.Using
as constforUTM_KEYSensures a readonly tuple type, and centralizingSTORAGE_KEYprevents magic strings throughout the codebase. This is solid foundational work.
7-15: Well-structured utility function.This is a clean, pure function that extracts only the relevant UTM parameters. The approach of building a fresh
URLSearchParamsensures we don't carry over unrelated query params. Returns an empty string when no UTMs are present, which works nicely with JavaScript's falsy evaluation in the hook logic.
tentative fix on docs utm persistence
Summary by CodeRabbit
New Features
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.