feat: align checkpoint tracking with download-first onboarding redesign#387
feat: align checkpoint tracking with download-first onboarding redesign#387juanmahidalgo wants to merge 2 commits into
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.
Code Review — PR #387
feat: align checkpoint tracking with download-first onboarding redesign
Summary
Clean refactor that narrows auth's checkpoint ownership to CP2 only, removes CP3/CP4 tracking, deletes the now-redundant markReturningUser, and defaults userIdentifier to the Segment anonymousId. All CI checks pass (tests, e2e, audit, lint). The architecture is sound — auth owns only its checkpoint, and the anonymousId-based identity stitching across landing/auth/explorer is well-designed.
ADR-6 Conventions ✅
- PR title:
feat: align checkpoint tracking with download-first onboarding redesign— correct format - Branch:
feat/checkpoints-redesign— correct prefix
Security Review ✅
- No hardcoded secrets or credentials
- No new PII exposure (wallet/email sent to Segment is existing behavior)
- No XSS vectors introduced
- No auth/authz changes
anonymousIdusage is standard Segment practice- No security issues found
Findings
[P2 — Missing unit tests for new trackCheckpoint logic]
trackCheckpoint.ts now has meaningful business logic: safeAnonymousId() extraction, defaulting userIdentifier/identifierType, silent event dropping when no identifier is available, and deferred firing via trackCheckpointWhenReady. There are no dedicated unit tests for this module. The deleted markReturningUser.spec.ts tested the old indirection, but nothing tests the new defaulting/fallback behavior directly. Consider adding tests for: (a) anonymousId used when no explicit identifier is passed, (b) event dropped when analytics isn't available, (c) trackCheckpointWhenReady defers correctly.
[P2 — Double getAnalytics() call]
trackCheckpoint() calls getAnalytics() (line 42) and then safeAnonymousId() calls it again (line 22). Minor redundancy — could pass the analytics instance through or extract it once at the top of trackCheckpoint.
[P2 — Silent event dropping may complicate funnel debugging]
When anonymousId isn't available and no explicit identifier is passed, the event is silently dropped (line 49). This is documented in the JSDoc and intentional, but if Segment is slow to initialize, early CP2-reached events could be lost. The trackCheckpointWhenReady wrapper mitigates this for the mount case, but a console.debug in development builds could help surface dropped events during debugging.
[P2 — YAGNI: 'landing' | 'explorer' source types]
The widened source union includes 'landing' and 'explorer', neither of which is used in this PR. Pragmatic for the coordinated multi-repo rollout mentioned in the description, so not blocking — just noting.
Verdict: ✅ APPROVE
No P0 or P1 issues. The changes are well-structured, thoroughly commented, and correctly implement the new checkpoint model. All four CP2-completed paths are covered (Metamask, Email OTP, auto-login, social/OAuth), markReturningUser deletion is safe (zero remaining references), and CI is fully green.
Reviewed by Jarvis 🤖 · Requested by Juan Manuel Hidalgo via Slack
| */ | ||
| function trackCheckpoint(params: CheckpointParams): void { | ||
| const analytics = getAnalytics() | ||
| if (!analytics) return |
There was a problem hiding this comment.
[P2] Minor: getAnalytics() is called here and again inside safeAnonymousId() (line 22). You could extract it once and pass it through to avoid the double call:
function trackCheckpoint(params: CheckpointParams): void {
const analytics = getAnalytics()
if (!analytics) return
const anonymousId = safeAnonymousId(analytics) // pass it in
// ...
}| const userIdentifier = params.userIdentifier ?? anonymousId | ||
| const identifierType = params.identifierType ?? (anonymousId ? 'anon' : undefined) | ||
|
|
||
| if (!userIdentifier || !identifierType) return |
There was a problem hiding this comment.
[P2] Suggestion: Silent drops here could make it harder to debug funnel gaps. Consider adding a console.debug('[trackCheckpoint] dropped — no identifier available', params) in development builds so dropped events are visible during testing.
2 participants
Onboarding redesign — Download First (auth)
Summary
Aligns the auth UI with the new 3-checkpoint model. The
authpage is now responsible only for CP2 (authenticated):reachedon LoginPage mount andcompletedafter each auth method (Magic / Metamask / Email OTP) succeeds. CP3/CP4 tracking (profile creation, avatar setup) is removed — the new model leaves those out of the funnel.Why
The old code tracked CP3 and CP4 from
QuickSetupPage,SetupPage, andAvatarSetupPage, plus a CP2 reached at multiple inconsistent points (post-OAuth, post-Metamask connect, post-Email-OTP send). With the redesigned funnel — CP1 download / CP2 auth / CP3 in-world — auth only owns CP2. Identity through the funnel is anchored on the SegmentanonymousId(shared with landing ondecentraland.org), so callers no longer need to computeuserIdentifiermanually:trackCheckpointdefaults to it.markReturningUseris gone because explicit CP2 completed calls already cover that case.Changes
src/shared/onboarding/trackCheckpoint.tsuserIdentifiertoanalytics.user().anonymousId()withidentifierType='anon'. Widened types:identifierType ∈ {anon,email,wallet},source ∈ {auth,landing,explorer}.src/components/Pages/LoginPage/LoginPage.tsxmarkReturningUserimport.src/components/Pages/LoginPage/AutoLoginRedirect.tsxmarkReturningUser.src/components/Pages/CallbackPage/CallbackPage.tsxwalletandemail(when available). DroppedmarkReturningUser.src/components/Pages/QuickSetupPage/QuickSetupPage.tsxtrackCheckpointimport.src/components/Pages/SetupPage/SetupPage.tsxsrc/components/Pages/AvatarSetupPage/AvatarSetupPage.tsxsrc/shared/onboarding/markReturningUser.ts(deleted)src/shared/onboarding/markReturningUser.spec.ts(deleted)src/components/Pages/LoginPage/LoginPage.spec.tsxjest.mock('../../../shared/onboarding/markReturningUser', …).Test plan
pnpm test— affected suites pass (LoginPage, CallbackPage, AvatarSetupPage, QuickSetupPage).Onboarding CheckpointcheckpointId=2, action=reached, identifierType=anon, userIdentifier=<ajs_anonymous_id>.walletandemailpopulated; sameanonymousId.wallet, noemail.walletandemail.completed_at IS NOT NULLandwallet/emailare filled byCOALESCE.Related PRs
auth-server—feat/onboarding-redesign-download-first(schema, validators, nudges).events-notifier—feat/onboarding-redesign-download-first(Segment → auth-server bridge).landing-site—feat/onboarding-redesign-download-first(CP1 landing flow).