Skip to content

Remove BYOK step from desktop onboarding#9309

Merged
Git-on-my-level merged 4 commits into
mainfrom
cursor/315f43b1
Jul 9, 2026
Merged

Remove BYOK step from desktop onboarding#9309
Git-on-my-level merged 4 commits into
mainfrom
cursor/315f43b1

Conversation

@Git-on-my-level

@Git-on-my-level Git-on-my-level commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Removes the optional Bring Your Own Keys onboarding step that confused casual users; Goal now goes straight to Tasks
  • Migrates mid-onboarding users who were on BYOK/Tasks onto the Tasks step
  • Removes the pill and bottom gradient behind the “This is your 2nd brain” graph label

BYOK remains available in Settings → Advanced and from the usage-limit paywall.

Test plan

  • Fresh onboarding: after Goal, next screen is Tasks (no API keys step)
  • Mid-flow users previously on BYOK or Tasks land on Tasks after update
  • Graph pane “This is your 2nd brain” label has no background/gradient
  • OnboardingFlowTests pass
  • BYOK still works from Settings → Advanced

Made with Cursor

Review in cubic

Drop the optional API-keys screen that confused casual users, migrate mid-flow users onto Tasks, and remove the 2nd-brain label background.

Co-authored-by: Cursor <cursoragent@cursor.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9de330c0b5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +106 to +107
if !hasRemovedBYOKStep, migratedStep > 17 {
migratedStep -= 1

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Adjust BYOK removal for pending notification migration

When a profile still has onboardingNotificationPermissionStepRemoved=false and is paused on the old BYOK page, its persisted index is 18 because the removed notification-permission page was before BYOK. This new shift first maps 18→17, and then OnboardingView applies the pending notification removal (currentStep >= 8 → -1), so that user lands on Goal (16) instead of Tasks. Apply the notification-permission migration before this BYOK removal or account for that pending flag here.

Useful? React with 👍 / 👎.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 7 files

Confidence score: 5/5

  • Safe to merge after the addressed issues were fixed.

You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread desktop/macos/Desktop/Sources/Onboarding/OnboardingFlow.swift
Fold notification-permission removal into migratedStep before BYOK removal so legacy BYOK resumes land on Tasks, and cover the changed onboarding files in harness-smoke.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Git-on-my-level

Copy link
Copy Markdown
Collaborator Author

Addressed review + CI:

  1. Migration ordering — moved onboardingNotificationPermissionStepRemoved into OnboardingFlow.migratedStep() before BYOK removal, so a legacy BYOK index (18 with notifications still counted) lands on Tasks (17) instead of Goal (16). Added testMigrationRemovesBYOKAfterPendingNotificationPermissionRemoval.
  2. E2E coverage CI — added OnboardingFlow.swift, OnboardingStepScaffold.swift, and OnboardingView.swift to harness-smoke.yaml covers.

OnboardingFlowTests: 7/7 passed locally.

1 similar comment
@Git-on-my-level

Copy link
Copy Markdown
Collaborator Author

Addressed review + CI:

  1. Migration ordering — moved onboardingNotificationPermissionStepRemoved into OnboardingFlow.migratedStep() before BYOK removal, so a legacy BYOK index (18 with notifications still counted) lands on Tasks (17) instead of Goal (16). Added testMigrationRemovesBYOKAfterPendingNotificationPermissionRemoval.
  2. E2E coverage CI — added OnboardingFlow.swift, OnboardingStepScaffold.swift, and OnboardingView.swift to harness-smoke.yaml covers.

OnboardingFlowTests: 7/7 passed locally.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 4 files (changes from recent commits).

You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread desktop/macos/e2e/flows/harness-smoke.yaml Outdated
Comment thread desktop/macos/Desktop/Sources/Onboarding/OnboardingFlow.swift Outdated
Match BYOK removal semantics: exact index 8 stays at Accessibility instead of sliding back to Microphone.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Git-on-my-level

Copy link
Copy Markdown
Collaborator Author

Follow-up on the latest cubic notes:

  1. Notification-permission exact index — changed the folded migration from >= 8 to > 8, so users paused on the removed notification-permission step advance to Accessibility (8) instead of Microphone (7). Regression covered in testMigrationRemovesBYOKAfterPendingNotificationPermissionRemoval.
  2. harness-smoke covers metadata — leaving the onboarding covers: entries in place. CI’s --strict e2e coverage gate requires changed Swift sources to be listed, and this file already covers other onboarding sources the smoke path doesn’t execute (OnboardingChatView, floating-bar/voice demos). A dedicated onboarding flow would be a separate follow-up; removing the entries would re-break Desktop Swift CI.

Keep harness-smoke honest about what it exercises, and satisfy the strict coverage gate with a manual onboarding-flow that targets Goal→Tasks after BYOK removal.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Git-on-my-level

Copy link
Copy Markdown
Collaborator Author

Follow-up on the remaining cubic note:

  • Removed OnboardingFlow / OnboardingStepScaffold / OnboardingView from harness-smoke covers
  • Added dedicated e2e/flows/onboarding-flow.yaml (tier: manual) that covers those files and documents Goal → Tasks (no BYOK) plus the 2nd-brain label check

desktop-flow-lint and check-e2e-flow-coverage.py --strict both pass for the changed onboarding sources.

@Git-on-my-level Git-on-my-level merged commit aca796e into main Jul 9, 2026
20 checks passed
@Git-on-my-level Git-on-my-level deleted the cursor/315f43b1 branch July 9, 2026 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant