fix(cli): track Android service account onboarding actions#2327
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR adds a ChangesAndroid service-account fork resume routing and telemetry
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Merging this PR will not alter performance
Comparing 
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 39e6b941-f044-4476-b0ae-db37f5dd7298
📒 Files selected for processing (6)
cli/package.jsoncli/src/build/onboarding/android/progress.tscli/src/build/onboarding/android/types.tscli/src/build/onboarding/android/ui/app.tsxcli/src/build/onboarding/telemetry.tscli/test/test-android-onboarding-progress.mjs
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/build/onboarding/android/ui/app.tsx (1)
949-950:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't retrofit the fork marker onto repaired legacy progress files.
These writes are unconditional, so a pre-fork progress file that re-enters the keystore phase for self-healing now gets
serviceAccountForkSeen: trueon save. On the next resume,getAndroidResumeStep(...)treats that file as a post-fork run and can send the user toservice-account-method-selectinstead of preserving the legacy OAuth default this PR is explicitly trying to keep.🐛 Proposed fix
await persist((p) => ({ ...p, keystoreKeyPassword: keyPw, _keystoreBase64: base64, - serviceAccountForkSeen: true, + ...(p.serviceAccountForkSeen || !p.completedSteps.keystoreReady + ? { serviceAccountForkSeen: true } + : {}), completedSteps: { ...p.completedSteps, keystoreReady: ready }, }))Apply the same guard in the generated-keystore path and the manual key-password completion path so only fresh post-fork runs gain the marker, while repaired legacy files preserve its absence.
Also applies to: 1005-1006, 1776-1777
🤖 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 `@cli/src/build/onboarding/android/ui/app.tsx` around lines 949 - 950, The writes unconditionally set serviceAccountForkSeen when updating completedSteps.keystoreReady, which retrofits the fork marker onto repaired legacy progress files; change the generated-keystore and manual key-password completion paths so they only set serviceAccountForkSeen = true when the run is actually post-fork (e.g. check an isPostForkRun flag or the existing serviceAccountForkSeen value on the loaded progress before mutating), otherwise leave serviceAccountForkSeen unchanged so getAndroidResumeStep(...) will continue to treat repaired legacy files as pre-fork; update the code paths that set completedSteps.keystoreReady to apply this guard.
🤖 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 `@cli/src/build/onboarding/telemetry.ts`:
- Around line 77-78: The loop currently copies input.tags into tags after
reserved fields are set, allowing callers to overwrite them; change the logic in
the telemetry helper so caller tags are merged first and then the reserved
fields are written (or alternatively skip reserved names inside the loop).
Specifically, update the code that iterates over input.tags (the for (const
[key, value] of Object.entries(input.tags ?? {})) block) to either (a) copy
caller tags first and then assign the reserved keys step, platform, app_id,
action afterwards, or (b) ignore keys named "step", "platform", "app_id", and
"action" during the loop so the helper's reserved values cannot be overwritten.
---
Outside diff comments:
In `@cli/src/build/onboarding/android/ui/app.tsx`:
- Around line 949-950: The writes unconditionally set serviceAccountForkSeen
when updating completedSteps.keystoreReady, which retrofits the fork marker onto
repaired legacy progress files; change the generated-keystore and manual
key-password completion paths so they only set serviceAccountForkSeen = true
when the run is actually post-fork (e.g. check an isPostForkRun flag or the
existing serviceAccountForkSeen value on the loaded progress before mutating),
otherwise leave serviceAccountForkSeen unchanged so getAndroidResumeStep(...)
will continue to treat repaired legacy files as pre-fork; update the code paths
that set completedSteps.keystoreReady to apply this guard.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 26b04ad6-7c1f-4e92-b16c-44a5f2de4096
📒 Files selected for processing (7)
cli/package.jsoncli/src/build/onboarding/android/progress.tscli/src/build/onboarding/android/types.tscli/src/build/onboarding/android/ui/app.tsxcli/src/build/onboarding/telemetry.tscli/test/test-android-onboarding-progress.mjscli/test/test-onboarding-telemetry.mjs
|
1 participant
Summary (AI generated)
serviceAccountForkSeenmarker so quitting at the service-account method fork resumes back to the fork instead of defaulting to OAuth.Motivation (AI generated)
The imported service-account onboarding path was visible through generic step telemetry, but did not expose the key user choices needed to understand the funnel. It also had an edge-case resume gap: a fresh user who reached the method-selection fork and quit before choosing could be resumed into the legacy OAuth path because no method value had been persisted yet.
Business Impact (AI generated)
Test Plan (AI generated)
bun run test:android-onboarding-progressbun run test:onboarding-telemetrybun run test:android-service-account-validationbun run lintbun run typecheckbun run buildNote: I also tried a direct one-off ESLint run on
cli/src/build/onboarding/android/ui/app.tsx; that file reports broad pre-existing TSX style issues under the root config, while the official CLI lint script does not target TSX files.Summary by CodeRabbit
New Features
Tests
Chores