Repository navigation
Conversation
Contributor
📝 WalkthroughWalkthroughThe onboarding flow now records entry, step completion, abandonment, and categorized errors for provider authentication, API-key connections, project completion, and repository cloning. ChangesOnboarding analytics
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OnboardingLayout
participant OnboardingPage
participant ProjectPage
participant onboardingAnalytics
OnboardingLayout->>onboardingAnalytics: track onboarding entry and step completion
OnboardingPage->>onboardingAnalytics: track GitHub authentication errors
ProjectPage->>onboardingAnalytics: track project completion and failure reasons
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@apps/desktop/src/renderer/routes/_authenticated/onboarding/project/page.tsx`:
- Around line 65-66: Update the onboarding completion flow around
completeOnboarding.mutate and markOnboardingFinished so the finished state is
set only after the mutation resolves successfully. Keep
trackOnboardingStepCompleted in the existing completion path, but ensure failed
persistence leaves onboarding unfinished so abandonment tracking can run.
In
`@apps/desktop/src/renderer/routes/_authenticated/onboarding/utils/onboardingAnalytics/onboardingAnalytics.ts`:
- Around line 27-29: Update trackOnboardingStepCompleted to maintain
module-level state of already completed OnboardingStep values, returning without
tracking when the supplied step was previously recorded; otherwise record the
step and emit exactly one onboarding_step_completed event.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 94581ef9-071b-43d5-8f58-5c5ffe80295a
📒 Files selected for processing (6)
apps/desktop/src/renderer/routes/_authenticated/onboarding/components/ProviderConnectModal/ProviderConnectModal.tsxapps/desktop/src/renderer/routes/_authenticated/onboarding/layout.tsxapps/desktop/src/renderer/routes/_authenticated/onboarding/page.tsxapps/desktop/src/renderer/routes/_authenticated/onboarding/project/page.tsxapps/desktop/src/renderer/routes/_authenticated/onboarding/utils/onboardingAnalytics/index.tsapps/desktop/src/renderer/routes/_authenticated/onboarding/utils/onboardingAnalytics/onboardingAnalytics.ts
Comment on lines
+65
to
+66
| trackOnboardingStepCompleted("project"); | ||
| markOnboardingFinished(); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Mark onboarding finished only after persistence succeeds.
Line 66 sets finished before completeOnboarding succeeds. If the mutation fails, flushOnboardingAbandoned skips abandonment tracking even though the user remains in onboarding. Set this state after completeOnboarding.mutate() resolves.
Proposed fix
const finish = async (projectId: string) => {
trackOnboardingStepCompleted("project");
- markOnboardingFinished();
track("onboarding_finished", { outcome: "completed" });
try {
await apiTrpcClient.user.completeOnboarding.mutate();
+ markOnboardingFinished();
await refetchSession({ query: { disableCookieCache: true } });🤖 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 `@apps/desktop/src/renderer/routes/_authenticated/onboarding/project/page.tsx`
around lines 65 - 66, Update the onboarding completion flow around
completeOnboarding.mutate and markOnboardingFinished so the finished state is
set only after the mutation resolves successfully. Keep
trackOnboardingStepCompleted in the existing completion path, but ensure failed
persistence leaves onboarding unfinished so abandonment tracking can run.
Comment on lines
+27
to
+29
| export function trackOnboardingStepCompleted(step: OnboardingStep): void { | ||
| track("onboarding_step_completed", { step }); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Deduplicate step-completion events.
trackOnboardingStepCompleted emits an event on every call. A repeated activation of the Continue control can record duplicate providers completion events. Store completed steps and emit one completion event per step.
Proposed fix
let currentStep: OnboardingStep | null = null;
+const completedSteps = new Set<OnboardingStep>();
let abandonTimer: ReturnType<typeof setTimeout> | null = null;
export function trackOnboardingStepCompleted(step: OnboardingStep): void {
+ if (completedSteps.has(step)) return;
+ completedSteps.add(step);
track("onboarding_step_completed", { step });
}🤖 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
`@apps/desktop/src/renderer/routes/_authenticated/onboarding/utils/onboardingAnalytics/onboardingAnalytics.ts`
around lines 27 - 29, Update trackOnboardingStepCompleted to maintain
module-level state of already completed OnboardingStep values, returning without
tracking when the supplied step was previously recorded; otherwise record the
step and emit exactly one onboarding_step_completed event.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/onboarding/project/page.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/onboarding/project/page.tsx:66">
P2: The newly added markOnboardingFinished() (and trackOnboardingStepCompleted("project")) run before completeOnboarding.mutate() succeeds. If that mutation (or the subsequent refetchSession) rejects, the code returns early with an error toast, but the module-level finished flag has already been set to true. As a result flushOnboardingAbandoned() will now silently skip emission of onboarding_abandoned even though the user never actually finished onboarding, and onboarding_finished/step-completed are recorded for a flow that failed. Move the finish/step-completed marks (and the onboarding_finished event) to the success path after the try block completes so failed completions don't get reported as finished and abandonment stays trackable.</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/onboarding/project/page.tsx:154">
P2: V1 clone failures never emit the new `onboarding_error` event because `cloneFromUrl` swallows them before this outer catch runs. Expose the failure through an error callback or typed result and track it at the V1 mutation boundary.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/onboarding/layout.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/onboarding/layout.tsx:63">
P2: Window-close abandonment events can be lost because this `beforeunload` handler uses a normal asynchronous PostHog capture rather than an unload-safe `sendBeacon` capture. An unload-specific analytics path should pass `transport: "sendBeacon"` and `send_instantly: true` so `onboarding_abandoned` is sent before the renderer exits.</violation>
</file>
Architecture diagram
sequenceDiagram
participant UI as Onboarding UI Components
participant Layout as Onboarding Layout (layout.tsx)
participant Analytics as onboardingAnalytics Module
participant PH as PostHog (via track())
participant Auth as authClient / Session
participant Backend as Backend API
participant IPC as Electron IPC (clone, gh auth)
Note over UI,IPC: Onboarding Funnel Instrumentation Flow
Layout->>Auth: Get session (onboardedAt)
Auth-->>Layout: session data
alt User is onboarded
Layout->>UI: Redirect to root (no tracking)
else User is NOT onboarded
Layout->>Analytics: trackOnboardingEntered("providers")
Analytics->>Analytics: Check module-level flags
alt First entry (started === false)
Analytics->>PH: track("onboarding_started")
Analytics->>Analytics: Set started = true
end
alt Step changed (currentStep !== "providers")
Analytics->>PH: track("onboarding_step_started", {step: "providers"})
Analytics->>Analytics: Set currentStep = "providers"
end
Analytics-->>Layout: done
end
Layout->>Layout: Add beforeunload event listener
Layout->>Analytics: scheduleOnboardingAbandoned()
Analytics->>Analytics: setTimer setTimeout(flushOnboardingAbandoned, 0)
Note over UI,IPC: Step 1: Provider Connection
UI->>UI: User clicks Continue
UI->>Analytics: trackOnboardingStepCompleted("providers")
Analytics->>PH: track("onboarding_step_completed", {step: "providers"})
UI->>Layout: Navigate to /onboarding/project
Layout->>Analytics: trackOnboardingEntered("project")
alt Step changed (currentStep !== "project")
Analytics->>PH: track("onboarding_step_started", {step: "project"})
Analytics->>Analytics: Set currentStep = "project"
end
Note over UI,IPC: Step 2: Clone/Add Project (error paths)
UI->>IPC: cloneRepository(repoUrl)
alt Clone fails
IPC-->>UI: Error
UI->>Analytics: trackOnboardingError("project", "clone", reason)
alt repo_not_found
Analytics->>PH: track("onboarding_error", {step:"project", action:"clone", reason:"repo_not_found"})
else auth_failed
Analytics->>PH: track("onboarding_error", {step:"project", action:"clone", reason:"auth_failed"})
else network_error
Analytics->>PH: track("onboarding_error", {step:"project", action:"clone", reason:"network_error"})
else already_exists
Analytics->>PH: track("onboarding_error", {step:"project", action:"clone", reason:"already_exists"})
else clone_failed
Analytics->>PH: track("onboarding_error", {step:"project", action:"clone", reason:"clone_failed"})
end
else Clone succeeds
UI->>UI: finish(projectId)
UI->>Analytics: trackOnboardingStepCompleted("project")
Analytics->>PH: track("onboarding_step_completed", {step: "project"})
UI->>Analytics: markOnboardingFinished()
Analytics->>Analytics: Set finished = true
UI->>PH: track("onboarding_finished", {outcome: "completed"})
UI->>Backend: completeOnboarding.mutate()
alt API call fails
Backend-->>UI: Error
UI->>Analytics: trackOnboardingError("project", "complete_onboarding", reason)
alt api_unreachable
Analytics->>PH: track("onboarding_error", {step:"project", action:"complete_onboarding", reason:"api_unreachable"})
else auth_failed
Analytics->>PH: track("onboarding_error", {step:"project", action:"complete_onboarding", reason:"auth_failed"})
else request_failed
Analytics->>PH: track("onboarding_error", {step:"project", action:"complete_onboarding", reason:"request_failed"})
end
end
end
Note over UI,IPC: GH Auth Dialog (error path)
UI->>Auth: refetchGh()
Auth-->>UI: Result
alt auth incomplete (not installed or not authenticated)
UI->>Analytics: trackOnboardingError("providers", "gh_auth", "auth_incomplete")
Analytics->>PH: track("onboarding_error", {step:"providers", action:"gh_auth", reason:"auth_incomplete"})
end
Note over UI,IPC: Provider API Key Save (error path)
UI->>IPC: setApiKey.mutateAsync()
alt API key save fails
IPC-->>UI: Error
UI->>Analytics: trackOnboardingError("providers", "provider_connect", "api_key_failed")
Analytics->>PH: track("onboarding_error", {step:"providers", action:"provider_connect", reason:"api_key_failed"})
end
Note over UI,IPC: Abandonment Tracking
alt User closes tab (beforeunload)
Layout->>Analytics: flushOnboardingAbandoned()
Analytics->>PH: track("onboarding_abandoned", {step: currentStep})
else User unmounts layout (route change)
Layout->>Analytics: scheduleOnboardingAbandoned() [timer]
alt Re-entry before timer fires
Layout->>Analytics: trackOnboardingEntered() [cancels timer]
else Timer fires (true abandonment)
Analytics->>Analytics: flushOnboardingAbandoned()
Analytics->>PH: track("onboarding_abandoned", {step: currentStep})
end
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (projectId) await finish(projectId); | ||
| } | ||
| } catch (err) { | ||
| trackOnboardingError("project", "clone", cloneFailureReason(err)); |
Contributor
There was a problem hiding this comment.
P2: V1 clone failures never emit the new onboarding_error event because cloneFromUrl swallows them before this outer catch runs. Expose the failure through an error callback or typed result and track it at the V1 mutation boundary.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/onboarding/project/page.tsx, line 154:
<comment>V1 clone failures never emit the new `onboarding_error` event because `cloneFromUrl` swallows them before this outer catch runs. Expose the failure through an error callback or typed result and track it at the V1 mutation boundary.</comment>
<file context>
@@ -138,6 +151,7 @@ function OnboardingProjectPage() {
if (projectId) await finish(projectId);
}
} catch (err) {
+ trackOnboardingError("project", "clone", cloneFailureReason(err));
toast.error(
err instanceof Error ? err.message : "Failed to clone repository",
</file context>
| }, [isPending, onboarded, pathname]); | ||
|
|
||
| useEffect(() => { | ||
| window.addEventListener("beforeunload", flushOnboardingAbandoned); |
Contributor
There was a problem hiding this comment.
P2: Window-close abandonment events can be lost because this beforeunload handler uses a normal asynchronous PostHog capture rather than an unload-safe sendBeacon capture. An unload-specific analytics path should pass transport: "sendBeacon" and send_instantly: true so onboarding_abandoned is sent before the renderer exits.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/onboarding/layout.tsx, line 63:
<comment>Window-close abandonment events can be lost because this `beforeunload` handler uses a normal asynchronous PostHog capture rather than an unload-safe `sendBeacon` capture. An unload-specific analytics path should pass `transport: "sendBeacon"` and `send_instantly: true` so `onboarding_abandoned` is sent before the renderer exits.</comment>
<file context>
@@ -43,6 +51,22 @@ function OnboardingFlowLayout() {
+ }, [isPending, onboarded, pathname]);
+
+ useEffect(() => {
+ window.addEventListener("beforeunload", flushOnboardingAbandoned);
+ return () => {
+ window.removeEventListener("beforeunload", flushOnboardingAbandoned);
</file context>
| // dashboard's new-workspace modal pre-selected to the project just added. | ||
| const finish = async (projectId: string) => { | ||
| trackOnboardingStepCompleted("project"); | ||
| markOnboardingFinished(); |
Contributor
There was a problem hiding this comment.
P2: The newly added markOnboardingFinished() (and trackOnboardingStepCompleted("project")) run before completeOnboarding.mutate() succeeds. If that mutation (or the subsequent refetchSession) rejects, the code returns early with an error toast, but the module-level finished flag has already been set to true. As a result flushOnboardingAbandoned() will now silently skip emission of onboarding_abandoned even though the user never actually finished onboarding, and onboarding_finished/step-completed are recorded for a flow that failed. Move the finish/step-completed marks (and the onboarding_finished event) to the success path after the try block completes so failed completions don't get reported as finished and abandonment stays trackable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/onboarding/project/page.tsx, line 66:
<comment>The newly added markOnboardingFinished() (and trackOnboardingStepCompleted("project")) run before completeOnboarding.mutate() succeeds. If that mutation (or the subsequent refetchSession) rejects, the code returns early with an error toast, but the module-level finished flag has already been set to true. As a result flushOnboardingAbandoned() will now silently skip emission of onboarding_abandoned even though the user never actually finished onboarding, and onboarding_finished/step-completed are recorded for a flow that failed. Move the finish/step-completed marks (and the onboarding_finished event) to the success path after the try block completes so failed completions don't get reported as finished and abandonment stays trackable.</comment>
<file context>
@@ -57,6 +62,8 @@ function OnboardingProjectPage() {
// dashboard's new-workspace modal pre-selected to the project just added.
const finish = async (projectId: string) => {
+ trackOnboardingStepCompleted("project");
+ markOnboardingFinished();
track("onboarding_finished", { outcome: "completed" });
try {
</file context>
Contributor
|
This PR has had no activity for 53 days and will close in 7 days unless there is new activity. If this contribution is still important or relevant, please leave a comment to keep it open. If it closes, you are welcome to reopen it or ask a maintainer to reopen it. Thank you for contributing! |
Contributor
|
This PR was automatically closed due to inactivity to help keep the review queue manageable. This does not mean the contribution was rejected. If this contribution is still important or relevant, please reopen it or leave a comment asking a maintainer to reopen it. Thank you for contributing! |
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
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.
Part of SUPER-1729 — https://linear.app/superset-sh/issue/SUPER-1729
Restores renderer-side onboarding funnel instrumentation for the 2-step flow. Analytics only — no UX, navigation, gating, or experiment changes (
useNewWorkspaceScreenVariantand allnew-workspace-screenexposure logic untouched). All events go through the existingtrack()helper inrenderer/lib/analytics.Events emitted
onboarding_startedonboarding_step_startedsteponboarding_step_completedstepfinish()immediately beforeonboarding_finishedonboarding_abandonedstepbeforeunload; suppressed once finishedonboarding_errorstep,action,reasonaction: clone),completeOnboardingfailure (action: complete_onboarding), gh auth exiting unauthenticated (action: gh_auth), provider API-key save failure (action: provider_connect)onboarding_finishedoutcome: completedonboarding_provider_connectedprovider,methodreasonis always a machine-readable code mapped from the error (repo_not_found,auth_failed,network_error,already_exists,clone_failed,host_not_ready,api_unreachable,request_failed,auth_incomplete,api_key_failed) — never URLs, repo names, paths, or messages.Step slugs
providers— the historicalprovidersstep is the closest ancestor of this surface; the oldgh-cli/permissions/adopt-worktreessteps no longer exist as separate screens, and gh-CLI issues remain distinguishable viaonboarding_errorwithaction: gh_auth.project— exact match with the historical slug.Verification
bun run lint:fix→bun run lintexits 0;apps/desktopbun run typecheckclean. No colocated tests in the touched files.DESKTOP_VITE_PORT=3225, CDP 9615):posthog.capturewas wrapped viaPage.addScriptToEvaluateOnNewDocumentso no test events reached production PostHog;onboarded_atwas nulled for the dev user, the flow was walked with real UI clicks, thenonboarded_atrestored.Captured sequence (in order, from
window.__t5Events):[{"event":"onboarding_started","props":null}, {"event":"onboarding_step_started","props":{"step":"providers"}}, {"event":"onboarding_step_completed","props":{"step":"providers"}}, // Continue click {"event":"onboarding_step_started","props":{"step":"project"}}, {"event":"onboarding_error","props":{"step":"project","action":"clone","reason":"repo_not_found"}}, // clone of nonexistent repo {"event":"onboarding_abandoned","props":{"step":"project"}}, // beforeunload path {"event":"onboarding_step_started","props":{"step":"providers"}}, // re-entry via Back — note: no second onboarding_started {"event":"onboarding_step_completed","props":{"step":"providers"}}, {"event":"onboarding_step_started","props":{"step":"project"}}, {"event":"onboarding_step_completed","props":{"step":"project"}}, {"event":"onboarding_finished","props":{"outcome":"completed"}}] // real clone; app then redirected to #/v2-workspacesThe abandonment leg was triggered with a dispatched
beforeunloadevent (synthetic — a real reload wipes the in-page capture buffer); the layout-unmount path uses the same flush and its timer-cancel behavior is exercised by the Back/Continue route changes above, which correctly did not emitonboarding_abandoned. Everything else was end-to-end UI interaction on the live app.Kept intentionally additive for easy rebasing against #6116/#6117.
Summary by cubic
Restores renderer-side onboarding funnel analytics for the 2-step flow to meet SUPER-1729. No UX, navigation, gating, or experiment changes; all events go through
track().onboarding_started,onboarding_step_started,onboarding_step_completed,onboarding_abandoned,onboarding_error, and keepsonboarding_finished/onboarding_provider_connectedas-is. Deduped per session and step.beforeunload; scheduled on layout unmount and canceled on re-entry to avoid false positives.{ step, action, reason }with machine-readable reasons only (e.g.,repo_not_found,auth_failed,api_unreachable,clone_failed,api_key_failed).providers(tools/providers checklist) andproject(create/add project).completeOnboardingfailures.Written for commit 05d5212. Summary will update on new commits.
Summary by CodeRabbit