-
Notifications
You must be signed in to change notification settings - Fork 1
chore: click_app event / fire pixel on app clicks #78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
…n app.shapeshift.com clicks
📝 WalkthroughWalkthroughExtends LocalizedLink to preserve string and UrlObject hrefs (pathname, query, hash), detect and bypass external/anchor/mailto/tel/non-root links, prepend language for internal root links (except default), add app.shapeshift.com analytics + new-tab handling, and update tsconfig exclude to include Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant LocalizedLink
participant Analytics as __adrsbl
participant Router
participant Window
User->>LocalizedLink: click(href)
alt href is anchor/mailto/tel/external host/non-root internal
LocalizedLink->>Router: no localization / default behavior
else host == app.shapeshift.com
LocalizedLink->>Analytics: invoke __adrsbl (if present)
Analytics-->>LocalizedLink: ack?
LocalizedLink->>Window: window.open(fullUrl, "_blank")
alt open blocked/error
LocalizedLink->>Router: navigate(fullUrl) (fallback)
end
else internal root-relative
LocalizedLink->>LocalizedLink: prepend lang (if not default), preserve query/hash
LocalizedLink->>Router: navigate(localized href)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings
📚 Learning: 2025-09-02T02:04:59.408Z
Applied to files:
🧬 Code graph analysis (1)app/[lang]/_components/LocalizedLink.tsx (1)
🔇 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/[lang]/_components/LocalizedLink.tsx
(1 hunks)tsconfig.json
(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T02:04:59.408Z
Learnt from: thesmithdao
PR: shapeshift/website-frontend#66
File: app/[lang]/page.tsx:262-296
Timestamp: 2025-09-02T02:04:59.408Z
Learning: User thesmithdao prefers to avoid refactoring to LocalizedLink components for landing info cards in app/[lang]/page.tsx, preferring to keep the implementation clean with plain anchor tags.
Applied to files:
app/[lang]/_components/LocalizedLink.tsx
🧬 Code graph analysis (1)
app/[lang]/_components/LocalizedLink.tsx (1)
app/[lang]/_utils/i18nconfig.ts (2)
getLanguageFromPath
(59-64)DEFAULT_LANGUAGE
(52-52)
905db99
to
1e90018
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/[lang]/_components/LocalizedLink.tsx (1)
93-101
: Previous critical issue resolved; consider minor optimization.The past issue with UrlObject navigation has been correctly fixed—both
window.open
and the fallbackwindow.location.assign
now usefullUrlFromObj(hrefObj)
to generate the complete absolute URL. Well done!Minor optimization: the absolute URL is computed twice (lines 93-95 and line 98). You could compute it once before the outer
try
block to reduce duplication:} e.preventDefault() + const absolute = hrefObj + ? fullUrlFromObj(hrefObj) + : hrefString || (typeof href === 'string' ? href : '') try { - const absolute = hrefObj - ? fullUrlFromObj(hrefObj) - : hrefString || (typeof href === 'string' ? href : '') window.open(absolute, '_blank', 'noopener,noreferrer') } catch { - const absolute = hrefObj ? fullUrlFromObj(hrefObj) : hrefString if (absolute) { window.location.assign(absolute) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/[lang]/_components/LocalizedLink.tsx
(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: thesmithdao
PR: shapeshift/website-frontend#66
File: app/[lang]/page.tsx:262-296
Timestamp: 2025-09-02T02:04:59.408Z
Learning: User thesmithdao prefers to avoid refactoring to LocalizedLink components for landing info cards in app/[lang]/page.tsx, preferring to keep the implementation clean with plain anchor tags.
📚 Learning: 2025-09-02T02:04:59.408Z
Learnt from: thesmithdao
PR: shapeshift/website-frontend#66
File: app/[lang]/page.tsx:262-296
Timestamp: 2025-09-02T02:04:59.408Z
Learning: User thesmithdao prefers to avoid refactoring to LocalizedLink components for landing info cards in app/[lang]/page.tsx, preferring to keep the implementation clean with plain anchor tags.
Applied to files:
app/[lang]/_components/LocalizedLink.tsx
🧬 Code graph analysis (1)
app/[lang]/_components/LocalizedLink.tsx (1)
app/[lang]/_utils/i18nconfig.ts (2)
getLanguageFromPath
(59-64)DEFAULT_LANGUAGE
(52-52)
🔇 Additional comments (3)
app/[lang]/_components/LocalizedLink.tsx (3)
48-57
: Verify undefined/null query parameter handling.When a query parameter value is
undefined
ornull
, line 56 appends an empty string (e.g.,?key=
). Confirm this is the intended behavior—typically, such values are omitted from query strings entirely. If omission is preferred, remove theelse
clause:for (const [k, v] of Object.entries(obj.query as Record<string, unknown>)) { if (Array.isArray(v)) { for (const item of v) { params.append(k, String(item)) } } else if (v !== undefined && v !== null) { params.append(k, String(v)) - } else { - params.append(k, '') } }
109-130
: LGTM: External link detection is comprehensive.The logic correctly identifies external links, anchors, mailto, tel, and non-root paths, bypassing localization for these cases. The conditional
onClick
on line 126 properly applieshandleClick
only for app links while preserving the originalonClick
for other external links.
132-145
: LGTM: Localization preserves URL structure correctly.The localization logic correctly:
- Detects existing language prefixes (line 133)
- Prepends the current language only when needed and not default (lines 138-140)
- Preserves UrlObject properties (query, hash) by returning a modified UrlObject (lines 142-144) instead of stringifying
This ensures Next.js Link receives the appropriate href format and all URL components are maintained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both internal and external links seem to be working as expected. Some minor tweaks requested.
1e90018
to
c3a80ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/[lang]/_components/LocalizedLink.tsx (1)
93-103
: Simplify the fallback chain for consistency.Line 96 includes a redundant fallback
(typeof href === 'string' ? href : '')
sincehrefString
is already set tohref
when it's a string (line 33). The catch block (line 99) doesn't include this fallback, creating an inconsistency.Consider simplifying line 96 to match line 99:
try { - const absolute = hrefObj - ? fullUrlFromObj(hrefObj) - : hrefString || (typeof href === 'string' ? href : '') + const absolute = hrefObj ? fullUrlFromObj(hrefObj) : hrefString window.open(absolute, '_blank', 'noopener,noreferrer') } catch { const absolute = hrefObj ? fullUrlFromObj(hrefObj) : hrefString
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/[lang]/_components/LocalizedLink.tsx
(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: thesmithdao
PR: shapeshift/website-frontend#66
File: app/[lang]/page.tsx:262-296
Timestamp: 2025-09-02T02:04:59.408Z
Learning: User thesmithdao prefers to avoid refactoring to LocalizedLink components for landing info cards in app/[lang]/page.tsx, preferring to keep the implementation clean with plain anchor tags.
📚 Learning: 2025-09-02T02:04:59.408Z
Learnt from: thesmithdao
PR: shapeshift/website-frontend#66
File: app/[lang]/page.tsx:262-296
Timestamp: 2025-09-02T02:04:59.408Z
Learning: User thesmithdao prefers to avoid refactoring to LocalizedLink components for landing info cards in app/[lang]/page.tsx, preferring to keep the implementation clean with plain anchor tags.
Applied to files:
app/[lang]/_components/LocalizedLink.tsx
🧬 Code graph analysis (1)
app/[lang]/_components/LocalizedLink.tsx (1)
app/[lang]/_utils/i18nconfig.ts (2)
getLanguageFromPath
(59-64)DEFAULT_LANGUAGE
(52-52)
🔇 Additional comments (4)
app/[lang]/_components/LocalizedLink.tsx (4)
70-80
: LGTM! App link detection is comprehensive.The detection logic correctly handles both string hrefs and
UrlObject
with host information. The regex pattern appropriately matchesapp.shapeshift.com
links with case-insensitive matching.
105-107
: LGTM! onClick propagation is handled correctly.The user-provided
onClick
handler is properly forwarded after the analytics and navigation logic, ensuring compatibility with existing click handlers.
110-131
: LGTM! External link handling is well-structured.The logic correctly identifies external links (including anchors, mailto, tel, and UrlObject with host/protocol) and bypasses localization for them. The conditional
onClick
on line 127 ensures app links get the analytics + new-tab behavior while other external links retain their original click handlers.
133-146
: LGTM! Localization logic correctly preserves UrlObject structure.The implementation properly:
- Detects existing language prefixes to avoid double-prefixing
- Only adds language prefix when not using the default language
- Preserves the original type (string or UrlObject) which maintains query and hash parameters
Based on coding guidelines (relevant_code_snippets show DEFAULT_LANGUAGE and getLanguageFromPath usage).
c3a80ae
to
bfed48e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/[lang]/_components/LocalizedLink.tsx
(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: thesmithdao
PR: shapeshift/website-frontend#66
File: app/[lang]/page.tsx:262-296
Timestamp: 2025-09-02T02:04:59.408Z
Learning: User thesmithdao prefers to avoid refactoring to LocalizedLink components for landing info cards in app/[lang]/page.tsx, preferring to keep the implementation clean with plain anchor tags.
📚 Learning: 2025-09-02T02:04:59.408Z
Learnt from: thesmithdao
PR: shapeshift/website-frontend#66
File: app/[lang]/page.tsx:262-296
Timestamp: 2025-09-02T02:04:59.408Z
Learning: User thesmithdao prefers to avoid refactoring to LocalizedLink components for landing info cards in app/[lang]/page.tsx, preferring to keep the implementation clean with plain anchor tags.
Applied to files:
app/[lang]/_components/LocalizedLink.tsx
🧬 Code graph analysis (1)
app/[lang]/_components/LocalizedLink.tsx (1)
app/[lang]/_utils/i18nconfig.ts (2)
getLanguageFromPath
(59-64)DEFAULT_LANGUAGE
(52-52)
🔇 Additional comments (8)
app/[lang]/_components/LocalizedLink.tsx (8)
13-14
: LGTM!The ESLint disable and global declaration are appropriate for integrating with the Addressable pixel library.
16-19
: LGTM!The type definition properly combines Next.js Link props with standard anchor attributes.
24-33
: LGTM!The component properly supports both string and UrlObject hrefs, extracting the pathname for processing while preserving the original object structure.
75-85
: LGTM!The app link detection correctly handles both string and UrlObject hrefs with appropriate pattern matching.
110-112
: Verify onClick propagation behavior is intentional.When
isAppLink
is true, the handler callspreventDefault()
and opens the link in a new tab, but still invokes the originalonClick
afterward. Confirm this is the intended behavior, as users might expect theironClick
to control navigation. If theonClick
is meant only for side effects (e.g., analytics), this is fine; otherwise, consider callingonClick
beforepreventDefault()
to allow it to cancel the action.
115-136
: LGTM!The external link detection correctly handles both string patterns (http, #, mailto, tel, non-root paths) and UrlObject indicators (host, hostname, protocol), ensuring only internal root paths are localized.
138-151
: LGTM!The language prefix logic correctly handles both string and UrlObject hrefs, preserving query and hash parameters when returning a UrlObject for localized internal links.
153-165
: LGTM!The final return properly renders the localized link with all props forwarded, and the displayName is set for debugging.
bfed48e
to
2faf556
Compare
Summary
Acceptance
Output:
Summary by CodeRabbit
New Features
Bug Fixes
Chores