Skip to content
Open
Show file tree
Hide file tree
Changes from 8 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
30 changes: 29 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,31 @@ 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' | 'altKey', boolean>> = {}) => ({
currentTarget,
ctrlKey: false,
metaKey: false,
shiftKey: false,
altKey: 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', 'altKey'] as const)('should keep the page when %s opens the link elsewhere', key => {
expect(isSameTabNavigation(click(linkWith(), { [key]: true }))).toBe(false)
})
})
17 changes: 16 additions & 1 deletion src/components/LandingNavbar/LandingNavbar.helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,19 @@ 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) or downloads it (alt/option held) leaves this page alive, so its click can wait for analytics.
*/
function isSameTabNavigation(event: {

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] CLAUDE.md does not allow inline types in .helpers.ts files. This hand-written shape could be replaced with Pick<React.MouseEvent, 'currentTarget' | 'ctrlKey' | 'metaKey' | 'shiftKey' | 'altKey'>, or a named type in LandingNavbar.types.ts. Either option also keeps the type in sync with the DOM event.

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 e709356: the event is now Pick<MouseEvent, 'currentTarget' | 'ctrlKey' | 'metaKey' | 'shiftKey' | 'altKey'> from React, so the instanceof Element guard is gone too.

currentTarget: EventTarget
ctrlKey: boolean
metaKey: boolean
shiftKey: boolean
altKey: boolean
}): boolean {
const target = event.currentTarget instanceof Element ? event.currentTarget.getAttribute('target') : null
return target !== '_blank' && !event.ctrlKey && !event.metaKey && !event.shiftKey && !event.altKey
}

export { isSameTabNavigation, isSectionActive, toNavbarAction, toNotificationLocale }
210 changes: 201 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,207 @@ 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 the visitor clicks Sign In', () => {
beforeEach(async () => {
renderAt('/events')
await user.click(screen.getAllByRole('button', { name: 'component.landing.navbar.sign_in' })[0])
})

it('should send it through the beacon, since signing in redirects away', () => {
expect(postSegmentEvent).toHaveBeenCalledTimes(1)
expect(postSegmentEvent).toHaveBeenCalledWith(
'Click',
expect.objectContaining({ action: 'sign_in', track_deferred: true }),
'anon-1'
)
})
})

describe('and the visitor clicks a section tab', () => {
let open: jest.SpyInstance

beforeEach(() => {
open = jest.spyOn(window, 'open').mockImplementation(() => null)
renderAt('/events')
fireEvent.click(desktopSection(/navbar\.discover$/i))
})

afterEach(() => {
open.mockRestore()
})

it('should send it through the beacon before the tab navigates in the same tab', () => {
expect(postSegmentEvent).toHaveBeenCalledWith(
'Click',
expect.objectContaining({ action: 'discover', section: 'discover', href: '/events', track_deferred: true }),
'anon-1'
)
expect(postSegmentEvent.mock.invocationCallOrder[0]).toBeLessThan(open.mock.invocationCallOrder[0])
})
})

describe('and the visitor clicks Jump In', () => {
let rerender: ReturnType<typeof render>['rerender']
let onClickJumpIn: jest.Mock

beforeEach(async () => {
onClickJumpIn = jest.fn()
Object.defineProperty(window, 'scrollY', { configurable: true, value: 100 })
;({ rerender } = renderAt('/', { isLandingPage: true, onClickJumpIn }))
fireEvent.scroll(window)
await user.click(screen.getByRole('button', { name: /jump_in/i }))
})

it('should queue it instead of using the beacon, since the page stays open', () => {
expect(postSegmentEvent).not.toHaveBeenCalled()
expect(track).not.toHaveBeenCalled()
expect(onClickJumpIn).toHaveBeenCalledTimes(1)
})

describe('and analytics finishes loading', () => {
beforeEach(() => {
;(jest.requireMock('@dcl/hooks').useAnalytics as jest.Mock).mockReturnValue({ isInitialized: true, track })
rerender(
<MemoryRouter initialEntries={['/']}>
<LandingNavbar {...props} isSignedIn={false} isLandingPage onClickJumpIn={jest.fn()} />
</MemoryRouter>
)
})

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

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()
})

it('should send the plain click payload, without deferral fields', () => {
expect(track).toHaveBeenCalledWith('Click', {
place: 'Landing Navbar',
event: 'click',
action: 'learn',
section: 'learn',
href: 'https://decentraland.org/blog/'
})
})
})

Expand Down
44 changes: 34 additions & 10 deletions src/components/LandingNavbar/LandingNavbar.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
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 { postDeferredClick } from '../../modules/deferredClickBeacon'
import { SectionViewedTrack, SegmentEvent } from '../../modules/segment'
import { assetUrl } from '../../utils/assetUrl'
import { getAvatarBackgroundColor, getDisplayName } from '../../utils/avatarColor'
Expand All @@ -29,7 +32,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 +258,37 @@ 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 }: { leavesPage?: boolean } = {}) => {
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
postDeferredClick(properties)
},
[isInitialized, track]
[deferredTrack, track]
)

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

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