[WIP]: feat(onboarding): funnel analytics + variant tagging for the new flow#7960
[WIP]: feat(onboarding): funnel analytics + variant tagging for the new flow#7960talissoncosta wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
97fe9e6 to
10fa440
Compare
3a4e76d to
0a430fe
Compare
0a430fe to
5492b47
Compare
Docker builds report
|
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Failed testsfirefox › tests/environment-permission-test.pw.ts › Environment Permission Tests › Environment-level permissions control access to features, identities, and segments @enterprise Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
|
Visual Regression19 screenshots compared. See report for details. |
The single-page flow auto-creates the project and flag via RTK, which emit no analytics, so the onboarding funnel had no project/feature step for the new flow. Emit the milestone events (First Project created, First Feature created) on a real create - the milestone the user reached - but not the generic Project/Feature created action events, which stay reserved for resources a user makes by hand. Contributes to #7738 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Set an onboarding_variant user property (control | single_page, from the onboarding_quickstart_flow flag) once, after Flagsmith resolves it at identify. As a user property it attaches to every event the user emits, so the shared funnel can be split by variant with a single Group by, without per-event tagging or touching the shared stores. Contributes to #7738 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Emit single_page-only diagnostics for the flow's internal drop-off: onboarding snippet copied (install / wire), AI-connect used, and the in-page flag toggle, each carrying org / project / environment IDs (no PII). Kept separate from the control comparison funnel since the control has no equivalent. Contributes to #7738 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
5492b47 to
0267ae7
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 013a9e4c-c310-451f-b31a-9d8a93809700
📒 Files selected for processing (7)
frontend/common/constants.tsfrontend/common/utils/getOnboardingVariant.tsfrontend/web/components/pages/onboarding/OnboardingConnectPanel/ConnectWithAiPanel.tsxfrontend/web/components/pages/onboarding/OnboardingConnectPanel/OnboardingConnectPanel.tsxfrontend/web/components/pages/onboarding/OnboardingFlow/OnboardingFlow.tsxfrontend/web/components/pages/onboarding/hooks/bootstrapOnboarding.tsfrontend/web/project/api.ts
| @@ -0,0 +1,10 @@ | |||
| import Utils from './utils' | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the alias import path instead of a relative import.
As per coding guidelines, frontend/**/*.{js,jsx,ts,tsx} should "Use only common/, components/, and project/ import paths; do not use relative imports."
♻️ Proposed fix
-import Utils from './utils'
+import Utils from 'common/utils'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import Utils from './utils' | |
| import Utils from 'common/utils' |
Source: Coding guidelines
| // Auto-created setup is a milestone reached, not a user action: fire First | ||
| // Project created, not the generic Project created (reserved for projects a | ||
| // user makes by hand). First is safe here - we only get here with no project. | ||
| API.trackEvent(Constants.events.CREATE_FIRST_PROJECT) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Guard milestone tracking so analytics failures can't abort onboarding.
Neither API.trackEvent(Constants.events.CREATE_FIRST_PROJECT) nor the CREATE_FIRST_FEATURE call is guarded. If trackEvent throws synchronously (e.g. an uninitialised analytics SDK), the exception propagates through ensureProject/ensureFlag and aborts the whole bootstrap sequence — even though the project/flag were already successfully created. ensureOnboardingTag right below explicitly wraps its logic in try/catch because tagging is "cosmetic only - never block onboarding on tagging"; the same defensive rationale applies to these milestone events.
♻️ Suggested fix
- API.trackEvent(Constants.events.CREATE_FIRST_PROJECT)
+ try {
+ API.trackEvent(Constants.events.CREATE_FIRST_PROJECT)
+ } catch {
+ // Analytics is best-effort - never block onboarding on tracking failures.
+ } if (isFirstFeature) {
- API.trackEvent(Constants.events.CREATE_FIRST_FEATURE)
+ try {
+ API.trackEvent(Constants.events.CREATE_FIRST_FEATURE)
+ } catch {
+ // Analytics is best-effort - never block onboarding on tracking failures.
+ }
}Also applies to: 192-196
| const trackSnippetCopied = (snippet: 'install' | 'wire') => | ||
| API.trackEvent({ | ||
| ...Constants.events.ONBOARDING_SNIPPET_COPIED, | ||
| extra: { ...diagnosticIds, snippet }, | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the inline union type.
As per coding guidelines, frontend/**/*.{ts,tsx} should "Extract inline union types into named types (for example, use type Status = 'A' | 'B' instead of an inline union)." The 'install' | 'wire' parameter type should be named.
♻️ Proposed fix
+type OnboardingSnippet = 'install' | 'wire'
+
- const trackSnippetCopied = (snippet: 'install' | 'wire') =>
+ const trackSnippetCopied = (snippet: OnboardingSnippet) =>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const trackSnippetCopied = (snippet: 'install' | 'wire') => | |
| API.trackEvent({ | |
| ...Constants.events.ONBOARDING_SNIPPET_COPIED, | |
| extra: { ...diagnosticIds, snippet }, | |
| }) | |
| type OnboardingSnippet = 'install' | 'wire' | |
| const trackSnippetCopied = (snippet: OnboardingSnippet) => | |
| API.trackEvent({ | |
| ...Constants.events.ONBOARDING_SNIPPET_COPIED, | |
| extra: { ...diagnosticIds, snippet }, | |
| }) |
Source: Coding guidelines
| // Pin the onboarding variant as a user property once the flag has | ||
| // resolved, so every event (incl. autocapture pageviews) is attributable | ||
| // to control vs single_page in one funnel (#7738). | ||
| API.flagsmithIdentify()?.then(() => | ||
| API.trackTraits({ onboarding_variant: getOnboardingVariant() }), | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add .catch() to avoid an unhandled promise rejection.
identify()'s try/catch is synchronous, so it won't catch a rejection from this async chain. If flagsmithIdentify() rejects, the appended .then() never runs and the rejection goes unhandled (console warnings, noisy error monitoring). The existing flagsmithIdentify internal chain (lines 151-173) has the same gap, but this new addition extends the unhandled surface further.
🛡️ Suggested fix
API.flagsmithIdentify()?.then(() =>
API.trackTraits({ onboarding_variant: getOnboardingVariant() }),
- )
+ ).catch((err) => console.error('Error tracking onboarding_variant', err))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Pin the onboarding variant as a user property once the flag has | |
| // resolved, so every event (incl. autocapture pageviews) is attributable | |
| // to control vs single_page in one funnel (#7738). | |
| API.flagsmithIdentify()?.then(() => | |
| API.trackTraits({ onboarding_variant: getOnboardingVariant() }), | |
| ) | |
| // Pin the onboarding variant as a user property once the flag has | |
| // resolved, so every event (incl. autocapture pageviews) is attributable | |
| // to control vs single_page in one funnel (`#7738`). | |
| API.flagsmithIdentify()?.then(() => | |
| API.trackTraits({ onboarding_variant: getOnboardingVariant() }), | |
| ).catch((err) => console.error('Error tracking onboarding_variant', err)) |
docs/if required so people know about the feature.Changes
Contributes to #7738
Instruments the new single-page onboarding so it sits in one Amplitude funnel, split by variant, alongside the current flow.
onboarding_variantuser property (control|single_page), set once at identify from theonboarding_quickstart_flowflag. As a user property it splits any funnel by variant with a single "Group by".First Project created/First Feature createdon a real create (org already fired). Milestone events only, not the genericProject/Feature createdaction events, which stay reserved for resources a user makes by hand.Onboarding snippet copied(install / wire),Onboarding AI connect used,Onboarding flag toggled.Activation (first SDK evaluation) is a follow-up, pending #7623's emit shape.
How did you test this code?
Manually, with
onboarding_quickstart_flowon and analytics logging enabled:User registerfires and theonboarding_variant = single_pageuser property is set.First Organisation / Project / Feature createdeach fire once, and not again on revisit.onboarding_variant = control.