feat(analytics): fire eea_uplift funnel on modal open, cover all remediation#2332
Open
kushagrasarathe wants to merge 2 commits into
Open
feat(analytics): fire eea_uplift funnel on modal open, cover all remediation#2332kushagrasarathe wants to merge 2 commits into
kushagrasarathe wants to merge 2 commits into
Conversation
…diation The eea_uplift_started event fired only on the advisory modal's 'Complete now' CTA, so it missed (a) users who open the uplift modal and abandon, and (b) the entire urgent post-cliff cohort, whose remediation is now blocking (fixable-rejection → KYC modal) rather than a future-dated advisory. Fire at modal-open instead, across both remediation paths and both channels (deposit + withdraw): - blocking/urgent: gate.reason.code === 'eea_uplift*' (BE sets it to the questionnaire cluster). - advisory/upcoming: requirementKey in the eea_uplift set (sof_individual_ primary_purpose, has_foreign_tax_registration, place_of_birth_missing, nationalities) — derived from prod remediation data. Event now carries source + urgent + effective_date so urgent vs upcoming (dob-type) split in PostHog. PoA / gov-id-expired co-occur on the cliff but are excluded (separate remediation) to avoid over-firing on generic doc rejections for non-EEA users.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds a shared EEA uplift trigger utility, refactors funnel tracking to use typed trigger snapshots, and updates add-money and withdraw bank pages to derive uplift start events from gate or advisory state while resetting funnel state on modal dismissal paths. ChangesEEA Uplift Funnel Analytics
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Contributor
Code-analysis diffPainscore total: 5858.65 → 5864.24 (+5.59) 🆕 New findings (29)
…and 9 more. ✅ Resolved (29)
…and 9 more. 📈 Painscore deltas (top movers)
|
Contributor
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/hooks/useEeaUpliftFunnel.ts (1)
13-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a discriminated union on
sourcefor stronger guarantees.
UpliftStartTriggermakesrequirementKey/actionKey/effectiveDateoptional for both sources, so nothing at compile time stops anadvisorytrigger from missingeffectiveDate(which the PR intends to always carry for the advisory/upcoming path) or catches a caller passingeffectiveDateon ablockingtrigger where it's meaningless. A discriminated union would let TS enforce the field combinations documented in the JSDoc.export type UpliftStartTrigger = | { source: 'advisory'; requirementKey: string; actionKey: string; effectiveDate: string } | { source: 'blocking'; requirementKey?: string }Not blocking — current shape works fine at runtime given the callers observed in the stack.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useEeaUpliftFunnel.ts` around lines 13 - 18, The UpliftStartTrigger shape in useEeaUpliftFunnel should be tightened into a discriminated union keyed by source so TypeScript enforces the intended field combinations. Update the UpliftStartTrigger type to separate the advisory and blocking variants, making requirementKey/actionKey/effectiveDate required for advisory and omitting effectiveDate for blocking, then adjust any nearby usages in useEeaUpliftFunnel to match the new source-based narrowing.src/utils/eea-uplift.utils.ts (1)
31-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType assertion bypasses
GateState's discriminated union.
(gate as { reason?: { code?: string } })sidesteps the fact that severalGateStatevariants (loading,ready,pending,needs-identity,needs-enrollment) don't declare areasonfield at all, and it silently accepts any string forcoderegardless ofCapabilityReason's actual shape. A narrowing check preserves the same runtime behavior while keeping type safety.♻️ Suggested refactor
export function eeaUpliftReasonCode(gate: GateState): string | undefined { - const code = (gate as { reason?: { code?: string } }).reason?.code - return code?.startsWith('eea_uplift') ? code : undefined + const code = 'reason' in gate ? gate.reason?.code : undefined + return code?.startsWith('eea_uplift') ? code : undefined }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/eea-uplift.utils.ts` around lines 31 - 34, The eeaUpliftReasonCode helper is bypassing GateState’s discriminated union with an unsafe type assertion. Update eeaUpliftReasonCode to use proper narrowing on gate instead of casting to { reason?: { code?: string } }, and only read reason.code after confirming the variant actually exposes reason. Keep the same runtime behavior for eea_uplift-prefixed codes, but align the types with GateState and CapabilityReason so the function remains safe without assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/hooks/useEeaUpliftFunnel.ts`:
- Around line 13-18: The UpliftStartTrigger shape in useEeaUpliftFunnel should
be tightened into a discriminated union keyed by source so TypeScript enforces
the intended field combinations. Update the UpliftStartTrigger type to separate
the advisory and blocking variants, making
requirementKey/actionKey/effectiveDate required for advisory and omitting
effectiveDate for blocking, then adjust any nearby usages in useEeaUpliftFunnel
to match the new source-based narrowing.
In `@src/utils/eea-uplift.utils.ts`:
- Around line 31-34: The eeaUpliftReasonCode helper is bypassing GateState’s
discriminated union with an unsafe type assertion. Update eeaUpliftReasonCode to
use proper narrowing on gate instead of casting to { reason?: { code?: string }
}, and only read reason.code after confirming the variant actually exposes
reason. Keep the same runtime behavior for eea_uplift-prefixed codes, but align
the types with GateState and CapabilityReason so the function remains safe
without assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2933f7b-e51a-4255-8ada-2ee82ba42c2e
📒 Files selected for processing (6)
src/app/(mobile-ui)/add-money/[country]/bank/page.tsxsrc/app/(mobile-ui)/withdraw/[country]/bank/page.tsxsrc/hooks/useEeaUpliftFunnel.test.tssrc/hooks/useEeaUpliftFunnel.tssrc/utils/eea-uplift.utils.test.tssrc/utils/eea-uplift.utils.ts
Addresses code-review findings on the eea_uplift funnel:
- phantom completions: the blocking path arms the started latch, but the
InitiateKycModal onClose/onContactSupport did not reset it (only the
advisory/Sumsub path did). A later unrelated KYC approval would fire a
false eea_uplift_completed. Reset the latch on modal dismiss.
- re-fire: trackStarted is now idempotent per armed attempt, so repeat
Continue clicks in one attempt don't inflate the started count.
- DRY + convention: collapse the four duplicated trigger-construction sites
into upliftTriggerFrom{Gate,Advisory} helpers and move UpliftStartTrigger
out of the hook into eea-uplift.utils (per the no-types-in-hooks rule).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/utils/eea-uplift.utils.ts (1)
48-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType cast bypasses
GateStatediscriminated-union narrowing.
(gate as { reason?: { code?: string } })sidesteps TypeScript's exhaustiveness checking on theGateStateunion. It works today because JS returnsundefinedfor missing properties, but ifCapabilityReason.codeor thereasonfield's shape changes incapability-gate.ts, this cast won't surface a compile error — it'll silently produceundefinedand this function will just stop matching.Consider narrowing with
'reason' in gateinstead, which stays type-safe against the actual union:♻️ Safer narrowing without a blanket cast
export function upliftTriggerFromGate(gate: GateState): UpliftStartTrigger | null { - const code = (gate as { reason?: { code?: string } }).reason?.code + const code = 'reason' in gate ? gate.reason?.code : undefined if (!code?.startsWith('eea_uplift')) return null return { requirementKey: code, source: 'blocking' } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/eea-uplift.utils.ts` around lines 48 - 52, The `upliftTriggerFromGate` helper is bypassing `GateState` narrowing with a blanket cast, which defeats union safety. Update the logic to narrow the `gate` value using the discriminant/shape check (for example, `'reason' in gate`) before reading `reason.code`, so the function stays aligned with `GateState` and `CapabilityReason` in `capability-gate.ts` without relying on an unsafe cast.src/app/(mobile-ui)/add-money/[country]/bank/page.tsx (1)
267-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider folding the derive+track pattern into the funnel hook.
The
const trigger = upliftTriggerFrom*(...); if (trigger) trackUpliftStarted(trigger)snippet is repeated here and again in the withdraw page (4 occurrences total). ExtendinguseEeaUpliftFunnelwith e.g.trackStartedFromGate(gate)/trackStartedFromAdvisory(advisory)would let call sites drop theeea-uplift.utilsimport and null-check entirely, keeping the "when does start fire" logic fully inside the hook (matching the PR's stated goal of consolidating trigger construction into shared helpers).♻️ Sketch of the hook-side consolidation
export function useEeaUpliftFunnel(channel: UpliftChannel) { const startedRef = useRef<UpliftStartTrigger | null>(null) const trackStarted = useCallback( (trigger: UpliftStartTrigger) => { if (sameTrigger(startedRef.current, trigger)) return startedRef.current = trigger posthog.capture(ANALYTICS_EVENTS.EEA_UPLIFT_STARTED, upliftEventProps(channel, trigger)) }, [channel] ) + + const trackStartedFromGate = useCallback((gate: GateState) => { + const trigger = upliftTriggerFromGate(gate) + if (trigger) trackStarted(trigger) + }, [trackStarted]) + + const trackStartedFromAdvisory = useCallback((advisory: GateAdvisory | undefined) => { + const trigger = upliftTriggerFromAdvisory(advisory) + if (trigger) trackStarted(trigger) + }, [trackStarted]) ... - return { trackStarted, trackCompleted, reset } + return { trackStarted, trackStartedFromGate, trackStartedFromAdvisory, trackCompleted, reset } }Also applies to: 280-283
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/`(mobile-ui)/add-money/[country]/bank/page.tsx around lines 267 - 270, Fold the repeated uplift derive+track logic into useEeaUpliftFunnel so call sites no longer need upliftTriggerFromGate/upliftTriggerFromAdvisory or a null-check. Add hook methods like trackStartedFromGate(gate) and trackStartedFromAdvisory(advisory) that internally resolve the trigger and call trackUpliftStarted, then update the bank page (and the matching withdraw page usage) to use those methods and remove the direct eea-uplift.utils import.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/app/`(mobile-ui)/add-money/[country]/bank/page.tsx:
- Around line 267-270: Fold the repeated uplift derive+track logic into
useEeaUpliftFunnel so call sites no longer need
upliftTriggerFromGate/upliftTriggerFromAdvisory or a null-check. Add hook
methods like trackStartedFromGate(gate) and trackStartedFromAdvisory(advisory)
that internally resolve the trigger and call trackUpliftStarted, then update the
bank page (and the matching withdraw page usage) to use those methods and remove
the direct eea-uplift.utils import.
In `@src/utils/eea-uplift.utils.ts`:
- Around line 48-52: The `upliftTriggerFromGate` helper is bypassing `GateState`
narrowing with a blanket cast, which defeats union safety. Update the logic to
narrow the `gate` value using the discriminant/shape check (for example,
`'reason' in gate`) before reading `reason.code`, so the function stays aligned
with `GateState` and `CapabilityReason` in `capability-gate.ts` without relying
on an unsafe cast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e8fd9b8-4a9b-4944-b8ff-7a835f9ade4f
📒 Files selected for processing (6)
src/app/(mobile-ui)/add-money/[country]/bank/page.tsxsrc/app/(mobile-ui)/withdraw/[country]/bank/page.tsxsrc/hooks/useEeaUpliftFunnel.test.tssrc/hooks/useEeaUpliftFunnel.tssrc/utils/eea-uplift.utils.test.tssrc/utils/eea-uplift.utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/useEeaUpliftFunnel.test.ts
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes the
eea_uplift_started/eea_uplift_completedfunnel reliable for watching who attempts the EEA uplift and whether they finish — so session recordings can be filtered by the event. Two gaps fixed:2026-06-29-cliff, the urgent uplift is no longer a future-dated advisory; the BE reclassifies it to blocking (fixable-rejection→ KYC modal), a path that called no tracking. Now covered.Fires across both remediation paths and both channels (deposit + withdraw):
gate.reason.codestarts witheea_uplift(BE sets reason code to the questionnaire cluster).requirementKey∈ {sof_individual_primary_purpose,has_foreign_tax_registration,place_of_birth_missing,nationalities} — derived from prod remediation data (172 EEA-affected users).Event now carries
source(advisory|blocking),urgent(blocking = past the cliff = urgent), andeffective_date, so urgent vs upcoming (dob-type) split directly in PostHog.Excluded
proof_of_address_document/government_id_expired: they co-occur on the cliff but are separate remediation (clusternull) — keying them to eea would over-fire on ordinary doc rejections for non-EEA users.Risks
startedcounts rise (now includes abandoners) — that's the intent.completedstill latches on a recorded start, so completion rate stays meaningful.document_rejected(freeform classifier) instead ofeea_upliftwouldn't be tagged — best signal available FE-side without a BE flag.QA
npm test— 107 suites green; neweea-uplift.utils(10) + rewrittenuseEeaUpliftFunnel(6) tests, incl. key-set match, reason-code prefix, urgent flag, and the co-occurring-non-uplift exclusion.handleAmountContinue).