Skip to content

Fix onboarding crashes, shortcut text, and data sources UX#6390

Merged
kodjima33 merged 4 commits into
mainfrom
worktree-fix+onboarding-issues
Apr 7, 2026
Merged

Fix onboarding crashes, shortcut text, and data sources UX#6390
kodjima33 merged 4 commits into
mainfrom
worktree-fix+onboarding-issues

Conversation

@kodjima33
Copy link
Copy Markdown
Collaborator

Summary

  • Fix crash on "English" click: UNUserNotificationCenter.current() was being called during SwiftUI view body evaluation, causing an assertion failure (SIGABRT). Deferred the call with DispatchQueue.main.async.
  • Update shortcut step text: Both shortcut screens now clearly state which shortcut is being configured ("Ask a question" vs "Audio ask a question") so users understand why buttons may not light up if shortcuts are already assigned.
  • Data sources step UX: Added skip button, replaced disabled "Finishing..." button with a progress spinner, and show "Continue" only when ready.

Test plan

  • Click "English" in onboarding language step — app should not crash
  • Verify shortcut setup screens show updated text
  • Verify data sources step has skip button and progress indicator
  • Verify "Continue" appears after scanning completes

🤖 Generated with Claude Code

kodjima33 and others added 4 commits April 7, 2026 15:02
UNUserNotificationCenter.current() asserts when called during SwiftUI
view body evaluation. Wrap in DispatchQueue.main.async to defer the
call until after the rendering pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Always show skip button so users aren't stuck waiting
- Replace disabled "Finishing..." button with a spinner + status text
- Show "Continue" button only when scanning is complete
- Gmail/email connection no longer blocks the continue flow

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 7, 2026

Greptile Summary

This PR fixes a SIGABRT crash when tapping "English" during onboarding by deferring UNUserNotificationCenter.current() off the SwiftUI view-body evaluation path, updates shortcut screen copy to name each shortcut explicitly, and reworks the Data Sources step to show a progress spinner while scanning and add a skip option. It also inserts a new OnboardingExportsStepView at step 16, renumbering Goal → 17 and Tasks → 18.

  • Migration double-increment: OnboardingFlow.migratedStep() applies both the hasInsertedDataSourcesStep guard (>= 16) and the new hasInsertedExportsStep guard (>= 16) sequentially. A user who needs both migrations applied (both flags false) will have their step incremented from 16 → 17 → 18, landing on Tasks and skipping the Goal step entirely.

Confidence Score: 4/5

Mostly safe to merge; one P1 migration bug can cause existing mid-onboarding users to skip the Goal step.

Score of 4 reflects a concrete P1 regression for a subset of users (those with hasInsertedDataSourcesStep=false who are at step 16) where Goal is silently skipped. All other findings are P2 style/animation issues that don't block correctness.

OnboardingFlow.swift (not in diff but called from OnboardingView.swift) — the hasInsertedExportsStep migration threshold needs to be >= 17, not >= 16.

Important Files Changed

Filename Overview
desktop/Desktop/Sources/AppState.swift Defers UNUserNotificationCenter call to main-queue async to prevent SIGABRT; logic correct, minor indentation inconsistency in nested closure.
desktop/Desktop/Sources/OnboardingDataSourcesStepView.swift Replaces disabled Finishing… button with spinner + conditional Continue button and adds skip support; transition modifier won't animate without withAnimation.
desktop/Desktop/Sources/OnboardingView.swift Inserts Exports step at index 16, renumbers Goal/Tasks; migration threshold for hasInsertedExportsStep (>= 16) conflicts with DataSources threshold and can skip Goal for users needing both migrations.
desktop/Desktop/Sources/OnboardingFloatingBarShortcutStepView.swift Copy update to name the Ask a question shortcut explicitly; no logic changes.
desktop/Desktop/Sources/OnboardingVoiceShortcutStepView.swift Copy update to name the Audio ask a question shortcut explicitly; no logic changes.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["onAppear — compute migratedStep()"] --> B{hasInsertedDataSourcesStep?}
    B -- false, step≥16 --> C[step += 1]
    B -- true --> D[no change]
    C --> E{hasInsertedExportsStep?}
    D --> E
    E -- false, step≥16 --> F[step += 1]
    E -- true --> G[no change]
    F --> H{Both flags were false AND step was 16?}
    G --> I[Correct: step lands on Goal=17]
    H -- yes --> J["❌ step=18 = Tasks\n(Goal skipped)"]
    H -- no --> I
Loading

Comments Outside Diff (1)

  1. desktop/Desktop/Sources/OnboardingView.swift, line 76 (link)

    P1 Migration double-increment skips Goal step

    In OnboardingFlow.migratedStep(), both the hasInsertedDataSourcesStep guard and the new hasInsertedExportsStep guard use threshold >= 16. For a user with both flags false (mid-onboarding before the DataSources update shipped), the DataSources migration increments their step from 16 → 17, and then the Exports migration fires again (17 ≥ 16) and increments 17 → 18 (Tasks), skipping the Goal step at 17 entirely. The Exports migration threshold should be >= 17 so the two migrations are independent when applied together:

    // In OnboardingFlow.swift:
    if !hasInsertedExportsStep, migratedStep >= 17 {
        migratedStep += 1
    }

Reviews (1): Last reviewed commit: "Add skip button and progress indicator t..." | Re-trigger Greptile

Comment on lines +744 to 745
UNUserNotificationCenter.current().getNotificationSettings { settings in
DispatchQueue.main.async {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Inner closure under-indented

The getNotificationSettings completion closure body is indented at the same level as the outer DispatchQueue.main.async block rather than one level deeper, making the nesting structure hard to read. This is a style-only issue — the logic is correct.

Suggested change
UNUserNotificationCenter.current().getNotificationSettings { settings in
DispatchQueue.main.async {
UNUserNotificationCenter.current().getNotificationSettings { settings in
DispatchQueue.main.async {

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +38 to 53
if coordinator.isResearchComplete {
Button("Continue") {
onContinue()
}
.buttonStyle(OnboardingCardButtonStyle(isPrimary: true))
.transition(.opacity.combined(with: .scale(scale: 0.95)))
} else {
HStack(spacing: 8) {
ProgressView()
.controlSize(.small)
.tint(OmiColors.textTertiary)
Text("Scanning your data sources...")
.font(.system(size: 13, weight: .medium))
.foregroundColor(OmiColors.textTertiary)
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 .transition() has no effect without withAnimation

The .transition(.opacity.combined(with: .scale(scale: 0.95))) on the Continue button only fires when the view is inserted/removed inside an explicit animated transaction. Since coordinator.isResearchComplete is published state that changes on a background task, the branch flip likely happens outside any withAnimation {} context and the transition will be a hard cut. To make the animation actually play, trigger the relevant state change inside withAnimation, or add an .animation(.easeInOut, value: coordinator.isResearchComplete) modifier on the surrounding VStack.

@kodjima33
Copy link
Copy Markdown
Collaborator Author

Mac mini test: PASS - all onboarding fixes verified

Build: Compiled and launched successfully (ad-hoc signed, --yolo mode)
Bundle: com.omi.onboarding-fix

Fix 1 - Crash on English click (UNUserNotificationCenter):

  • App log confirms checkNotificationPermission() ran without crashing: Notification settings: auth=denied, alertStyle=BANNER
  • App ran for several minutes without any assertion/crash

Fix 2 - Shortcut step text:

  • Binary contains: Let's set "Ask a question" shortcut.
  • Binary contains: Let's set "Audio ask a question" shortcut.

Fix 3 - Data sources step:

  • Binary contains: Scanning your data sources... ✓ (progress spinner text)
  • OnboardingDataSourcesStepView passes showsSkip: true and onSkip callback ✓
  • Skip button wired to advance currentStep to 16 with analytics event DataSources_Skipped

@kodjima33 kodjima33 merged commit 56c7d06 into main Apr 7, 2026
3 checks passed
@kodjima33 kodjima33 deleted the worktree-fix+onboarding-issues branch April 7, 2026 19:16
Glucksberg pushed a commit to Glucksberg/omi-local that referenced this pull request Apr 28, 2026
…ip button)

## Summary
- **Fix crash on "English" click**: `UNUserNotificationCenter.current()`
was being called during SwiftUI view body evaluation, causing an
assertion failure (SIGABRT). Deferred the call with
`DispatchQueue.main.async`.
- **Update shortcut step text**: Both shortcut screens now clearly state
which shortcut is being configured ("Ask a question" vs "Audio ask a
question") so users understand why buttons may not light up if shortcuts
are already assigned.
- **Data sources step UX**: Added skip button, replaced disabled
"Finishing..." button with a progress spinner, and show "Continue" only
when ready.

## Test plan
- [ ] Click "English" in onboarding language step — app should not crash
- [ ] Verify shortcut setup screens show updated text
- [ ] Verify data sources step has skip button and progress indicator
- [ ] Verify "Continue" appears after scanning completes

🤖 Generated with [Claude Code](https://claude.com/claude-code)
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