Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion src/components/LandingNavbar/LandingNavbar.helpers.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { isSectionActive, toNavbarAction, toNotificationLocale } from './LandingNavbar.helpers'
import { isSameTabNavigation, isSectionActive, toNavbarAction, toNotificationLocale } from './LandingNavbar.helpers'

describe('when deciding which navbar section owns the current page', () => {
it('should light up Discover on the What is On calendar', () => {
Expand Down Expand Up @@ -67,3 +67,30 @@ describe('when naming the analytics action for a navbar link', () => {
expect(toNavbarAction('learn')).toBe('learn')
})
})

describe('when deciding whether a navbar click replaces the current page', () => {
const linkWith = (target?: string) => {
const link = document.createElement('a')
if (target) link.setAttribute('target', target)
return link
}
const click = (currentTarget: EventTarget, keys: Partial<Record<'ctrlKey' | 'metaKey' | 'shiftKey', boolean>> = {}) => ({
currentTarget,
ctrlKey: false,
metaKey: false,
shiftKey: false,
...keys
})

it('should treat a plain click on a same-tab link as leaving the page', () => {
expect(isSameTabNavigation(click(linkWith()))).toBe(true)
})

it('should keep the page for a link that opens in a new tab', () => {
expect(isSameTabNavigation(click(linkWith('_blank')))).toBe(false)
})

it.each(['ctrlKey', 'metaKey', 'shiftKey'] as const)('should keep the page when %s opens the link elsewhere', key => {
expect(isSameTabNavigation(click(linkWith(), { [key]: true }))).toBe(false)
})
})
11 changes: 10 additions & 1 deletion src/components/LandingNavbar/LandingNavbar.helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,13 @@ function toNavbarAction(labelKey: string): string {
return labelKey.slice(labelKey.lastIndexOf('.') + 1)
}

export { isSectionActive, toNavbarAction, toNotificationLocale }
/**
* Whether a primary click on a navbar link replaces the current page. A link that opens in a new tab
* (`target="_blank"`, or ctrl/cmd/shift held) leaves this page alive, so its click can wait for analytics.
*/
function isSameTabNavigation(event: { currentTarget: EventTarget; ctrlKey: boolean; metaKey: boolean; shiftKey: boolean }): boolean {
const target = event.currentTarget instanceof Element ? event.currentTarget.getAttribute('target') : null
return target !== '_blank' && !event.ctrlKey && !event.metaKey && !event.shiftKey

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Alt-click (Option-click on macOS) on a link downloads it in Chrome and keeps the page open, but this helper counts it as leaving the page. Before analytics is ready, that click goes out by beacon instead of waiting in the queue for the SDK. It is still counted once, just without the SDK's full context. Consider adding altKey to the parameter type and the check (&& !event.altKey), plus an it.each case for it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied in 09c759c: isSameTabNavigation also treats altKey as keeping the page, and the it.each covers it.

}

export { isSameTabNavigation, isSectionActive, toNavbarAction, toNotificationLocale }
127 changes: 118 additions & 9 deletions src/components/LandingNavbar/LandingNavbar.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ jest.mock('decentraland-ui2', () => ({
}))
jest.mock('decentraland-ui2/dist/components/Notifications/utils', () => ({ NotificationComponentByType: {} }))

jest.mock('@dcl/hooks', () => ({ useAnalytics: jest.fn() }))
jest.mock('@dcl/hooks', () => ({ useAnalytics: jest.fn(), isBotClient: jest.fn() }))
jest.mock('../../modules/analyticsSessionGate', () => ({ isAnalyticsDisabledForSession: jest.fn() }))
jest.mock('../../modules/segmentBeacon', () => ({ postSegmentEvent: jest.fn() }))
jest.mock('../../modules/segmentAnonymousId', () => ({ ensureSegmentAnonymousId: jest.fn() }))
jest.mock('../../intl/LocaleContext', () => ({ useLocale: jest.fn() }))
jest.mock('../../hooks/adapters/useFormatMessage', () => ({
useFormatMessage: jest.fn()
Expand Down Expand Up @@ -529,18 +532,124 @@ describe('when the visitor clicks a navbar link', () => {
})

describe('and analytics has not finished loading', () => {
beforeEach(async () => {
let postSegmentEvent: jest.Mock
let isBotClient: jest.Mock
let isAnalyticsDisabledForSession: jest.Mock

const clickLearn = () => {
renderAt('/events')
const mobileMenu = screen.getByRole('navigation', { name: 'Mobile navigation' })
fireEvent.click(preventNavigation(within(mobileMenu).getByRole('link', { name: /navbar\.learn/i, hidden: true })))
}

beforeEach(() => {
postSegmentEvent = jest.requireMock('../../modules/segmentBeacon').postSegmentEvent
isBotClient = jest.requireMock('@dcl/hooks').isBotClient
isAnalyticsDisabledForSession = jest.requireMock('../../modules/analyticsSessionGate').isAnalyticsDisabledForSession
;(jest.requireMock('../../modules/segmentAnonymousId').ensureSegmentAnonymousId as jest.Mock).mockReturnValue('anon-1')
;(jest.requireMock('@dcl/hooks').useAnalytics as jest.Mock).mockReturnValue({ isInitialized: false, track })
isBotClient.mockReturnValue(false)
isAnalyticsDisabledForSession.mockReturnValue(false)
})

describe('and the visitor is a person in a session with analytics on', () => {
beforeEach(async () => {
clickLearn()
})

it('should send the click through the beacon, which survives the page unloading', () => {
expect(track).not.toHaveBeenCalled()
expect(postSegmentEvent).toHaveBeenCalledTimes(1)
expect(postSegmentEvent).toHaveBeenCalledWith(
'Click',
{
place: 'Landing Navbar',
event: 'click',
action: 'learn',
section: 'learn',
href: 'https://decentraland.org/blog/',
track_called_at: expect.any(Number),
track_delivered_at: expect.any(Number),
track_deferred: true
},
'anon-1'
)
})
})

describe('and the link opens in a new tab', () => {
let rerender: ReturnType<typeof render>['rerender']

beforeEach(async () => {
;({ rerender } = renderAt('/events'))
const createTab = desktopSection(/navbar\.create$/i)
await user.hover(createTab.parentElement!)
fireEvent.click(
preventNavigation(within(createTab.parentElement!).getByRole('link', { name: /creator_documentation/i, hidden: true }))
)
})

it('should not use the beacon, since the page stays open', () => {
expect(postSegmentEvent).not.toHaveBeenCalled()
expect(track).not.toHaveBeenCalled()
})

describe('and analytics finishes loading', () => {
beforeEach(() => {
;(jest.requireMock('@dcl/hooks').useAnalytics as jest.Mock).mockReturnValue({ isInitialized: true, track })
// A new prop, since the memoized navbar would otherwise skip the render that reads the new readiness.
rerender(
<MemoryRouter initialEntries={['/events']}>
<LandingNavbar {...props} isSignedIn={false} onClickJumpIn={jest.fn()} />
</MemoryRouter>
)
})

it('should send the queued click through analytics', () => {
expect(track).toHaveBeenCalledTimes(1)
expect(track).toHaveBeenCalledWith(
'Click',
expect.objectContaining({ action: 'creator_documentation', section: 'create', track_deferred: true })
)
})
})
})

describe('and the session started on an exempt page', () => {
beforeEach(async () => {
isAnalyticsDisabledForSession.mockReturnValue(true)
clickLearn()
})

it('should not send anything', () => {
expect(track).not.toHaveBeenCalled()
expect(postSegmentEvent).not.toHaveBeenCalled()
})
})

describe('and the visitor is a bot', () => {
beforeEach(async () => {
isBotClient.mockReturnValue(true)
clickLearn()
})

it('should not send anything', () => {
expect(track).not.toHaveBeenCalled()
expect(postSegmentEvent).not.toHaveBeenCalled()
})
})
})

describe('and analytics is ready', () => {
beforeEach(() => {
renderAt('/events')
const createTab = desktopSection(/navbar\.create$/i)
await user.hover(createTab.parentElement!)
fireEvent.click(
preventNavigation(within(createTab.parentElement!).getByRole('link', { name: /creator_documentation/i, hidden: true }))
)
const mobileMenu = screen.getByRole('navigation', { name: 'Mobile navigation' })
fireEvent.click(preventNavigation(within(mobileMenu).getByRole('link', { name: /navbar\.learn/i, hidden: true })))
})

it('should not send anything', () => {
expect(track).not.toHaveBeenCalled()
it('should send the click through analytics and not the beacon', () => {
expect(track).toHaveBeenCalledTimes(1)
expect(jest.requireMock('../../modules/segmentBeacon').postSegmentEvent).not.toHaveBeenCalled()
})
})

Expand Down
52 changes: 42 additions & 10 deletions src/components/LandingNavbar/LandingNavbar.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
import { memo, useCallback, useEffect, useMemo, useRef, useState } from 'react'
import { useLocation } from 'react-router-dom'
import { useAnalytics } from '@dcl/hooks'
import { isBotClient, useAnalytics } from '@dcl/hooks'
import { useFormatMessage } from '../../hooks/adapters/useFormatMessage'
import { useDeferredTrack } from '../../hooks/useDeferredTrack'
import { MIN_DISPLAY_BALANCE } from '../../hooks/useManaBalances'
import { useLocale } from '../../intl/LocaleContext'
import { isAnalyticsDisabledForSession } from '../../modules/analyticsSessionGate'
import { SectionViewedTrack, SegmentEvent } from '../../modules/segment'
import { ensureSegmentAnonymousId } from '../../modules/segmentAnonymousId'
import { postSegmentEvent } from '../../modules/segmentBeacon'
import { assetUrl } from '../../utils/assetUrl'
import { getAvatarBackgroundColor, getDisplayName } from '../../utils/avatarColor'
// Module-level cache for notification type→component map from ui2.
Expand All @@ -29,7 +33,7 @@ import {
ShoppingBagIcon,
WearableIcon
} from './icons'
import { isSectionActive, toNavbarAction, toNotificationLocale } from './LandingNavbar.helpers'
import { isSameTabNavigation, isSectionActive, toNavbarAction, toNotificationLocale } from './LandingNavbar.helpers'
import { DROPDOWN_SECTIONS, MENU_CONFIG, USER_MENU_ITEMS } from './navbarConfig'
import type { DropdownSection } from './navbarConfig'
import {
Expand Down Expand Up @@ -255,16 +259,44 @@ const LandingNavbar = memo(function LandingNavbar({
const { isInitialized, track } = useAnalytics()
const [mobileMenuOpen, setMobileMenuOpen] = useState(false)

// Read through a ref (as useDownloadClick does) so a click sees Segment's current readiness even if it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] This comment claims more than the ref does. A ref assigned during render only ever holds the value from the last render, so a click cannot see a readiness change that has not been rendered yet. What the ref does is keep trackNavbar stable, so it no longer depends on isInitialized and doesn't invalidate the memoized handlers. Suggest rewording the comment to say that. useDownloadClick has the same wording, so it could be fixed there too.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d881b17: both comments (here and in useDownloadClick) now say the ref keeps the handler's identity when analytics becomes ready.

// finished loading since the last render.
const isInitializedRef = useRef(isInitialized)
isInitializedRef.current = isInitialized

const deferredTrack = useDeferredTrack()

const trackNavbar = useCallback(
(action: string, link?: { section: string; href: string }) => {
if (!isInitialized) return
track(SegmentEvent.CLICK, { place: SectionViewedTrack.LANDING_NAVBAR, event: 'click', action, ...link })
(action: string, link?: { section: string; href: string }, leavesPage = true) => {
const properties = { place: SectionViewedTrack.LANDING_NAVBAR, event: 'click', action, ...link }
if (isInitializedRef.current) {
track(SegmentEvent.CLICK, properties)
return
}
// Analytics loads once the page is idle and is ready only after Segment fetched its settings. A click
// that stays on the page (new tab, Jump In) waits in the queue with the SDK's full context.
if (!leavesPage) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] In a session that started on an exempt page, or for a bot, analytics never initializes, so this queue is never flushed. Nothing is sent (correct), but each click that keeps the page leaves a closure in queueRef for the life of the navbar. Checking the gate before both branches makes the rule explicit and avoids that:

if (isAnalyticsDisabledForSession() || isBotClient(navigator.userAgent)) return
if (!leavesPage) {
  deferredTrack(SegmentEvent.CLICK, properties)
  return
}
postDeferredClick(properties)

This doesn't block the merge.

deferredTrack(SegmentEvent.CLICK, properties)
return
}
// One that replaces the page would be lost with it; the beacon survives the unload. It follows the same
// rules that keep the SDK off: sessions that started on an exempt page, and bots.
if (isAnalyticsDisabledForSession() || isBotClient(navigator.userAgent)) return
// Same deferral fields as useDownloadClick's beacon and useDeferredTrack, so every deferred click carries them.
const calledAt = Date.now()
postSegmentEvent(
SegmentEvent.CLICK,
// eslint-disable-next-line @typescript-eslint/naming-convention
{ ...properties, track_called_at: calledAt, track_delivered_at: calledAt, track_deferred: true },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] This copies the deferred-beacon payload from useDownloadClick.ts (track_called_at / track_delivered_at / track_deferred + ensureSegmentAnonymousId()), and the two have already drifted: here track_delivered_at reuses calledAt, while useDownloadClick takes a fresh Date.now(). A small shared helper (e.g. postDeferredClick(properties) next to postSegmentEvent) would keep the beacon callers identical, which the comment above says is the intent.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied in b7dc1dc. New postDeferredClick(properties) in src/modules/deferredClickBeacon.ts (with a spec) builds the deferral fields once, and both useDownloadClick and the navbar call it. track_delivered_at now takes a fresh Date.now() for both, matching what useDownloadClick did before. useDownloadClick specs pass unchanged.

ensureSegmentAnonymousId()
)
},
[isInitialized, track]
[deferredTrack, track]
)

const trackNavbarLink = useCallback(
(section: string, labelKey: string, href: string) => trackNavbar(toNavbarAction(labelKey), { section, href }),
(section: string, labelKey: string, href: string, leavesPage = true) =>
trackNavbar(toNavbarAction(labelKey), { section, href }, leavesPage),
[trackNavbar]
)

Expand All @@ -273,10 +305,10 @@ const LandingNavbar = memo(function LandingNavbar({
const navbarLinkHandlers = useCallback(
(section: string, labelKey: string, href: string) => ({
onClick: (event: React.MouseEvent) => {
if (event.button === 0) trackNavbarLink(section, labelKey, href)
if (event.button === 0) trackNavbarLink(section, labelKey, href, isSameTabNavigation(event))
},
onAuxClick: (event: React.MouseEvent) => {
if (event.button === 1) trackNavbarLink(section, labelKey, href)
if (event.button === 1) trackNavbarLink(section, labelKey, href, false)
}
}),
[trackNavbarLink]
Expand Down Expand Up @@ -556,7 +588,7 @@ const LandingNavbar = memo(function LandingNavbar({
{scrolled && onClickJumpIn ? (
<NavJumpInButton
onClick={e => {
trackNavbar('jump_in')
trackNavbar('jump_in', undefined, false)
onClickJumpIn(e)
}}
>
Expand Down
Loading