Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
81 changes: 76 additions & 5 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,86 @@ describe('when the visitor clicks a navbar link', () => {
})

describe('and analytics has not finished loading', () => {
beforeEach(async () => {
;(jest.requireMock('@dcl/hooks').useAnalytics as jest.Mock).mockReturnValue({ isInitialized: false, track })
let postSegmentEvent: jest.Mock
let isBotClient: jest.Mock
let isAnalyticsDisabledForSession: jest.Mock

const clickCreatorDocumentation = async () => {
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 }))
)
}

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

it('should not send anything', () => {
expect(track).not.toHaveBeenCalled()
describe('and the visitor is a person in a session with analytics on', () => {
beforeEach(async () => {
await clickCreatorDocumentation()
})

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: 'creator_documentation',
section: 'create',
href: 'https://docs.decentraland.org/creator'
},
'anon-1'
)
})
})

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

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

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

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

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
24 changes: 20 additions & 4 deletions src/components/LandingNavbar/LandingNavbar.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
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 { 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 Down Expand Up @@ -255,12 +258,25 @@ 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 trackNavbar = useCallback(
(action: string, link?: { section: string; href: string }) => {
if (!isInitialized) return
track(SegmentEvent.CLICK, { place: SectionViewedTrack.LANDING_NAVBAR, event: 'click', action, ...link })
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. Most navbar
// links leave the page, so a click before that 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
postSegmentEvent(SegmentEvent.CLICK, properties, ensureSegmentAnonymousId())

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] Some navbar actions do not leave the page: sign_in (button), jump_in (button), and middle-clicks into a new tab (#905). Before analytics is ready they also take the beacon path, so they lose the full analytics-next context (traits, campaign, the SDK-generated messageId) that they would get if queued with useDeferredTrack. That includes the phase where the SDK instance exists but is still fetching settings. The cost is data quality, not lost data. If you want, keep the beacon for anchor clicks that navigate away and route the button and middle-click actions through useDeferredTrack. Fine to leave as is if the uniform payload matters more.

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 6bf7784. Only clicks that replace the page (a same-tab link, Sign In, which redirects) take the beacon now. Clicks that keep the page open (a target="_blank" link, ctrl/cmd/shift click, middle click, Jump In) go to useDeferredTrack, which queues them and sends them through analytics-next with full context once it is ready. The decision comes from isSameTabNavigation (helper with tests). The component spec covers both paths, including the queue draining when readiness flips.

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] useDownloadClick tags its beacon clicks with track_deferred: true (plus track_called_at and track_delivered_at), so the warehouse can tell beacon-sent Click events apart from SDK-sent ones. Here they can only be told apart through context.library.name = "dcl-sites-beacon". Consider adding at least track_deferred: true so dashboards can split navbar clicks the same way they split download clicks.

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.

Added in bf52e07: beacon-sent navbar clicks carry track_deferred: true, like useDownloadClick. Queued ones get it from useDeferredTrack. One caveat for dashboards: stg_landing_click / int_landing_tracks in DCL_NEXT only keep action, section and href from extra_data, so splitting there needs that allowlist widened. Until then, context.library.name = "dcl-sites-beacon" is the split available in the warehouse.

},
[isInitialized, track]
[track]
)

const trackNavbarLink = useCallback(
Expand Down
Loading