Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review: approve ✅
This is a small change that follows existing patterns: navbar clicks made before analytics is ready now go through the same unload-safe beacon that useDownloadClick uses.
What I checked
- No double counting. Each click takes exactly one path. When analytics is ready it goes through
trackand returns; otherwise it goes through the beacon. A beacon click never enters the analytics-next buffer, so it cannot be sent again once Segment finishes loading. - The beacon follows the same rules that keep the SDK off.
- Bots:
@dcl/hooks@2.1.0does exportisBotClient, and itsAnalyticsProvideruses the same check to skip loading. - Exempt sessions:
postSegmentEventdeliberately skips the exempt-path check (bypassExemptPathGate), so the explicitisAnalyticsDisabledForSession()call is required, and it is there. - No writeKey:
postSegmentEventalready returns early. - Both checks run before
ensureSegmentAnonymousId(), so bots and exempt sessions never get anajs_anonymous_idcookie written.
- Bots:
- The navbar links are plain
hrefanchors (full page loads), so the beacon is the right transport for them. - Tests. The
afterEachwithjest.resetAllMocks()plus the newbeforeEachkeep each case isolated. The new cases cover a person, an exempt session, a bot, and analytics being ready. The ref pattern and the[track]dependencies matchuseDownloadClickanduseDeferredTrack. No unused imports.
Minor findings (P2, not blocking)
- P2:
sign_in,jump_inand middle-clicks into a new tab do not leave the page, but they also take the beacon path before analytics is ready. This includes the window where the SDK instance exists and would queue them with full context. Details inline. - P2: The download CTA beacon marks its events with
track_deferred: true(plustrack_called_atandtrack_delivered_at), but the navbar beacon does not. Details inline. - P2: No test covers a middle-click before analytics loads, or analytics becoming ready after the first render (a
rerenderbefore the click). The second is the case the ref exists for.
CI: lint, audit, title-matches-convention, lint-package-json and Vercel pass; test was still pending when I reviewed.
Conventions: the title feat: … and the branch feat/navbar-early-click-beacon follow ADR-6.
Consumer impact: none. This is an internal component change with no public surface.
Reviewed by Jarvis 🤖 · Requested by braianj via GitHub
| // 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()) |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
| // 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()) |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
Coverage Report for CI Build 37869264982Coverage increased (+0.008%) to 94.189%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review
Verdict: Approve. I found no P0 or P1 issues.
The change does what the description says. Before analytics is ready, a click that leaves the page now goes through postSegmentEvent. A click that keeps the page open (target="_blank", ctrl/cmd/shift-click, middle-click, Jump In) is queued in useDeferredTrack and sent once Segment is ready. After that, clicks go through track.
What I checked
isBotClient(userAgent)is exported by@dcl/hooks2.1.0, and it is the same checkAnalyticsProvideruses to keep the SDK off for bots. Together withisAnalyticsDisabledForSession(), the beacon is skipped in the same cases where the SDK never loads. This is stricter thanuseDownloadClick, which skips the exempt-path check on purpose for conversion CTAs. That fits navbar clicks, which are not conversions.- Navbar links are plain
<a href>elements, so a same-tab click really does load a new page. The beacon is the right transport for these clicks, and each click is sent once only: the beacon branch returns before reaching the queue. ensureSegmentAnonymousId()creates and saves theajs_anonymous_idbefore Segment boots, so beacon clicks and later SDK events share the same anonymous id.trackNavbarnow reads readiness fromisInitializedRef, which matchesuseDownloadClick. RemovingisInitializedfrom the deps is correct.- The tests cover beacon, new tab then drain, exempt session, bot, and the ready path, plus the helper's modifier and
_blankcases.
Findings
- [P2] The beacon payload has
track_deferred: truebut nottrack_called_at/track_delivered_at.useDownloadClick's beacon branch and theuseDeferredTrackqueue both send all three (see the inline comment).
CI: lint, audit, and title checks pass. test was still running when I reviewed.
Reviewed by Jarvis 🤖 · Requested by braianj via GitHub
| if (isAnalyticsDisabledForSession() || isBotClient(navigator.userAgent)) return | ||
| // Flagged like useDownloadClick's beacon clicks, so beacon-sent and SDK-sent clicks can be told apart. | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| postSegmentEvent(SegmentEvent.CLICK, { ...properties, track_deferred: true }, ensureSegmentAnonymousId()) |
There was a problem hiding this comment.
[P2] The comment says this is "flagged like useDownloadClick's beacon clicks", but that beacon (src/hooks/useDownloadClick.ts) also sends track_called_at and track_delivered_at, and so does every queued click from useDeferredTrack (including the new-tab path just above). As written, a navbar beacon Click is the only deferred click without those fields. Any query that uses them (for example, to measure the delay between the click and when it was sent) will get nulls for these rows. Suggested change to keep the payloads the same:
| postSegmentEvent(SegmentEvent.CLICK, { ...properties, track_deferred: true }, ensureSegmentAnonymousId()) | |
| 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 }, | |
| ensureSegmentAnonymousId() | |
| ) |
If you take this, the postSegmentEvent assertion in LandingNavbar.spec.tsx will need expect.any(Number) for the two timestamps.
Note: this suggestion replaces only line 287 and includes its own eslint-disable-next-line, so also delete the existing one on line 286 when you apply it.
There was a problem hiding this comment.
Applied in 78291c4: the beacon payload now carries track_called_at and track_delivered_at (both set to the click time, since the beacon is sent on the spot) alongside track_deferred: true. The spec asserts the timestamps with expect.any(Number).
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review: send pre-ready navbar clicks through the beacon
Verdict: Approve. I found no P0 or P1 issues. The approach is sound and every handler sends each click down exactly one path: track, the useDeferredTrack queue, or the beacon. Nothing is counted twice.
Verified
- Click classification matches what each handler actually does.
- Navbar links are plain
<a>elements, so a same-tab click does a full page load and the beacon is right. - Sign In ends in
redirectToAuth, which callswindow.location.replace. The page unloads, so the beacon is right. - Jump In calls
preventDefaultand opens the modal or launcher. The page stays, so the queue is right. DesktopTabWithDropdownuseswindow.open(url, '_self'), which always navigates the same tab, so the defaultleavesPage = trueis right.- Middle click goes through
onAuxClickwithfalse, so the queue is right.
- Navbar links are plain
- The beacon follows the same rules that keep the SDK off. In
@dcl/hooks2.1.0,AnalyticsProviderstays off only for an empty write key or a bot.- An empty write key makes
postSegmentEventreturn early. - Bots are checked with the same
isBotClient, which 2.1.0 exports for this purpose. - The
isAnalyticsDisabledForSession()check puts back the exempt-path rule thatpostSegmentEventskips on purpose. - Neither the SDK nor the beacon has a consent or Do Not Track check, so the beacon skips nothing the SDK applies.
- An empty write key makes
- Reading
isInitializedthrough a ref avoids beaconing a click with a stalefalseafter the render where Segment became ready. This is the same pattern asuseDownloadClick. - Security:
hrefandactioncome only from static navbar config, and the body is built withJSON.stringifyand sent astext/plainwithcredentials: 'omit'. No new PII or injection surface.
Findings (all non-blocking)
- [P2]
isSameTabNavigationignoresaltKey(inline). - [P2] The deferred-beacon payload is copied from
useDownloadClickand has already drifted from it (inline). - [P2] The positional
leavesPageboolean is hard to read at the call sites.trackNavbar('jump_in', undefined, false)reads better as{ leavesPage: false }. - [P2] Test gaps. The specs cover Learn (beacon), a new-tab link (queue), exempt sessions and bots. They don't cover:
- the Sign In beacon
- the
DesktopTabWithDropdownbeacon - the Jump In queue
- the payload in the "analytics is ready" case (for example, that
track_deferredis not set)
- [P2] Follow-up, out of scope here. The navbar is the only
postSegmentEventcaller that filters bots. The download beacons (useDownloadClick,downloadTracking,downloadPageExit,downloadFunnelExit, and others) still send for bots. Moving the bot check intopostSegmentEventwould apply one rule to every caller.
CI
lint, audit, lint-package-json, title convention and Vercel pass. test was still pending when this review was posted. I couldn't run the specs locally in this environment, so please confirm test is green before merging.
Reviewed by Jarvis 🤖 · Requested by braianj via GitHub
| */ | ||
| 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 |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
Applied in 09c759c: isSameTabNavigation also treats altKey as keeping the page, and the it.each covers it.
| postSegmentEvent( | ||
| SegmentEvent.CLICK, | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| { ...properties, track_called_at: calledAt, track_delivered_at: calledAt, track_deferred: true }, |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
|
Summary-level P2s:
|
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Summary
This is a well-scoped change. Every navbar call site is put on the right path:
- Clicks that replace the page go through the beacon. These are same-tab
<a>links (logo, Learn, credits, user menu, internal dropdown items), the section tab buttons (window.open(url, '_self')) and Sign In (redirectToAuth→location.replace). - Clicks that keep the page open wait in the
useDeferredTrackqueue. These aretarget="_blank"items, modifier clicks, middle clicks and Jump In (useHangOutActioncallspreventDefault).
Each branch returns early, so a click never goes through the beacon and also gets queued, and nothing is sent twice. The beacon gate (exempt-session check plus isBotClient) matches the rules that keep the SDK off. isBotClient is exported by the installed @dcl/hooks@2.1.0. Extracting postDeferredClick into its own module does not change what useDownloadClick sends.
Security: I found no issues. The beacon path checks the exempt-page gate and the bot check before sending. The href values come from fixed navbar config and carry no user input or wallet. userId is only present after the SDK has identified the user, the same as SDK-sent events.
Verification (local, head a379f0d): jest on LandingNavbar, deferredClickBeacon and useDownloadClick: 111/111 passing. tsc --noEmit: clean. eslint on the touched files: clean. CI checks were still pending when I posted this review.
Findings
All findings are P2. None blocks the merge.
- [P2]
LandingNavbar.helpers.ts:56: the parameter type is written inline in a.helpers.tsfile, which CLAUDE.md forbids ("Types / interfaces:<thing>.types.ts. Never inline in.client.ts,.helpers.ts…"). - [P2]
LandingNavbar.tsx:261: the ref comment claims more than the ref does. - [P2]
deferredClickBeacon.ts:8: the doc comment says callers apply the exempt and bot gates, butuseDownloadClickapplies neither. - [P2] Test gaps in
LandingNavbar.spec.tsx. These cases are not covered at the component level:- A middle click (
onAuxClick→ queue) before analytics is ready. - A ctrl/cmd click on a same-tab link through the component. Only the helper is tested for this.
- A beaconed click followed by analytics finishing, asserting that
trackis not called afterwards. This would lock in the no-double-send behavior.
- A middle click (
Reviewed by Jarvis 🤖 · Requested by braianj via GitHub
| * 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: { |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
Fixed in e709356: the event is now Pick<MouseEvent, 'currentTarget' | 'ctrlKey' | 'metaKey' | 'shiftKey' | 'altKey'> from React, so the instanceof Element guard is gone too.
| 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 |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
Fixed in d881b17: both comments (here and in useDownloadClick) now say the ref keeps the handler's identity when analytics becomes ready.
| /** | ||
| * Sends a `Click` made before analytics was ready through the unload-safe beacon, for clicks that leave the page. | ||
| * Adds the same deferral fields `useDeferredTrack` gives queued clicks, so every deferred click carries them | ||
| * whichever transport sent it. Callers apply their own gates (exempt session, bots) before calling. |
There was a problem hiding this comment.
[P2] useDownloadClick is the other caller, and it applies neither gate. Skipping the exempt-session check there is intentional, because /download is exempt but its CTAs must still be counted. Skipping the bot check, though, means a crawler that clicks a download CTA before Segment is ready still sends a beacon. This was already the case before this PR. Two options:
- Reword the comment so it does not promise gating that only the navbar does.
- Move the
isBotClient(navigator.userAgent)check intopostDeferredClick. Both callers would then share it, and the navbar would only keep its exempt-session check.
There was a problem hiding this comment.
Reworded in 916d4d4: the comment now says postDeferredClick applies no gate and names what each caller skips. I left the bot check where it is, because moving it into postDeferredClick would stop counting bot clicks on download CTAs and change the download funnel numbers. That goes in a separate PR, once the data team has seen it.
|
The test gaps from the last review are covered in ed2c8c3: a pre-ready middle click and ctrl/cmd click on a same-tab link are queued and drained once analytics is ready, and a beaconed click is not sent again through |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review
Verdict: Approve. I found no P0 or P1 issues.
The routing is correct. Before analytics is ready, each click goes to exactly one transport (beacon or queue), never both. Once analytics is ready, every click goes through track as before. I checked each caller's leavesPage value:
- Leave the page: plain same-tab links, the desktop section tab (
window.open(url, '_self'), which stays in the same tab even with a modifier key held), and both Sign In buttons (useSignInRedirectalways callsredirectToAuth). - Keep the page:
_blanklinks, ctrl/cmd/shift/alt clicks, middle click, and Jump In (it only opens the download modal or the desktop app).
Moving the isInitialized read into a ref is safe: track keeps the same identity while analytics loads, so trackNavbar doesn't capture a stale value. The beacon gate (isAnalyticsDisabledForSession + isBotClient) uses the same conditions that keep the SDK off. It is needed because postSegmentEvent bypasses the exempt-path gate. The postDeferredClick extraction in useDownloadClick sends the same payload as before, and the download CTAs are still ungated on purpose.
Tests cover the beacon path, the queue for new-tab, modifier and middle clicks (including the flush with no double send), the exempt and bot sessions, Sign In, and all four modifier keys in isSameTabNavigation.
Minor (P2)
- P2
LandingNavbar.tsx:277: in exempt or bot sessions, clicks that keep the page are still pushed intouseDeferredTrack's queue. Analytics never loads there, so the queue is never flushed. Nothing is sent, but the closures stay in memory for the life of the component. See the inline comment. - P2 (by design, noting it for the data team): a same-tab click made while analytics.js is loaded but not ready goes out through the beacon, so it carries the beacon's context instead of the SDK's (no campaign/UTM enrichment). Analysis that splits on
track_deferred = trueshould keep this in mind.
CI: lint, lint-package-json, title and Vercel are passing. test and audit were still running when I posted this.
Reviewed by Jarvis 🤖 · Requested by braianj via GitHub
| } | ||
| // 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) { |
There was a problem hiding this comment.
[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.
This branch was successfully deployed
3 participants
Navbar clicks made before analytics is ready are now sent instead of dropped. Analytics loads once the page is idle, and since
@dcl/hooks2.1.0 it reports ready only after Segment has fetched its settings, so until then the click used to be lost.Before analytics is ready, a click that replaces the page (a same-tab link, Sign In) goes through the same unload-safe beacon the download CTAs use (
postSegmentEvent), with the sametrack_deferred/track_called_at/track_delivered_atfields as every other deferred click. It is skipped for sessions that started on an exempt page (isAnalyticsDisabledForSession) and for bots (isBotClient), the same rules that keep the SDK off. A click that keeps the page open (a new-tab link, ctrl/cmd/shift click, middle click, Jump In) waits inuseDeferredTrack's queue instead and is sent by analytics.js with its full context once it is ready. Once analytics is ready, every click goes throughtrackas today.Planned for the release after the one carrying #903 to #906.
How to test (preview, desktop Chrome, a new incognito window, DevTools Network filtered to
api.e.decentraland.org, throttling set to Slow 4G):/eventsand click Learn within the first second, before thesettingsrequest finishes. Expected: a POST to/v1/track(the beacon endpoint) goes out before thesettingsrequest finishes, and the blog loads.Clicksent by analytics.js (/v1/t), not a beacon./privacyin a new incognito window, wait a few seconds, then click Learn. Expected: noClickrequest (no/v1/trackor/v1/t).needs manual verification: open/eventsunder Slow 4G, hover Create and immediately click Creator documentation (new tab). Expected: no/v1/trackbeacon. Oncesettingsloads, aClickwithtrack_deferred: truegoes out through/v1/t. A component test covers this.