fix(desktop): onboarding step flow improvements#6046
Conversation
…assword prompt SecItemCopyMatching triggers a macOS Keychain password dialog because the app's code signature isn't in the browser's Safe Storage ACL. The original `security find-generic-password` CLI approach doesn't prompt because it's a trusted system binary. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…d password prompt Same fix as GmailReaderService — use `security find-generic-password` instead of SecItemCopyMatching to avoid the Keychain password dialog. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rd prompts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…s, rename to Disk Access Move screen recording step before full disk access so the restart happens before any file scanning or insights start. Rename "Full Disk Access" to "Disk Access" for clarity. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR refactors the macOS desktop onboarding flow with several quality-of-life improvements: reordering permission steps (Screen Recording now appears before Full Disk Access), moving the Research and Goal steps to the end of the flow, parallelizing file scan with Gmail/Calendar enrichment, fixing graph edge persistence, replacing Security-framework keychain calls with the Key changes:
Issues found:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Trust] --> B[Name]
B --> C[Language]
C --> D["ScreenRecording ★ moved here"]
D -->|"Grant / Skip"| E["FullDiskAccess (renamed: Disk Access)"]
E --> F[FileScan]
F -->|parallel start| G["Task.detached: Gmail + Calendar + Web Research"]
F --> H[Microphone]
H --> I[Notifications]
I --> J[Accessibility]
J --> K[Automation]
K --> L[FloatingBarShortcut]
L --> M[FloatingBar]
M --> N[VoiceShortcut]
N --> O[VoiceDemo]
O --> P["Research ★ moved here (step 14)"]
P --> Q["Goal ★ moved here (step 15)"]
Q --> R[Tasks / Complete]
G -.->|"results ready by step 14"| P
style D fill:#f90,color:#000
style P fill:#f90,color:#000
style Q fill:#f90,color:#000
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if OnboardingChatPersistence.isMidOnboarding { | ||
| let bundleURL = Bundle.main.bundleURL | ||
| let task = Process() | ||
| task.executableURL = URL(fileURLWithPath: "/bin/sh") | ||
| task.arguments = ["-c", "sleep 1 && open \"\(bundleURL.path)\""] | ||
| try? task.run() | ||
| } |
There was a problem hiding this comment.
bundleURL.path is interpolated directly into a shell command string:
task.arguments = ["-c", "sleep 1 && open \"\(bundleURL.path)\""]While Bundle.main.bundleURL is OS-controlled and unlikely to contain adversarial content in practice, the double-quote-only escaping doesn't account for characters like $, backticks, or spaces inside directory names (e.g., if the app is installed in /Applications/My Apps/Omi.app). A path with spaces would break the shell argument even with the surrounding quotes since the path itself may contain further embedded quotes.
A more robust approach avoids shell interpolation entirely by using open directly:
let task = Process()
task.executableURL = URL(fileURLWithPath: "/usr/bin/open")
task.arguments = [bundleURL.path]
// Use a detached sleep-then-launch so the current process can exit first
let delayedTask = Process()
delayedTask.executableURL = URL(fileURLWithPath: "/bin/sh")
delayedTask.arguments = ["-c", "sleep 1"]Or, even simpler, just use NSWorkspace after a short delay by scheduling a detached task that has no shell injection surface. At minimum, the path should be properly quoted using ProcessInfo-safe methods.
| BrowserKeychainCache.shared.password(for: service) { | ||
| let query: [String: Any] = [ | ||
| kSecClass as String: kSecClassGenericPassword, | ||
| kSecAttrService as String: service, | ||
| kSecReturnData as String: true, | ||
| kSecMatchLimit as String: kSecMatchLimitOne, | ||
| ] | ||
| var result: AnyObject? | ||
| let status = SecItemCopyMatching(query as CFDictionary, &result) | ||
| guard status == errSecSuccess, let data = result as? Data, | ||
| let password = String(data: data, encoding: .utf8) | ||
| else { | ||
| if status != errSecItemNotFound { | ||
| log( | ||
| "CalendarReaderService: Keychain lookup for '\(service)' failed with status \(status)") | ||
| } | ||
| let process = Process() | ||
| process.executableURL = URL(fileURLWithPath: "/usr/bin/security") | ||
| process.arguments = ["find-generic-password", "-s", service, "-w"] | ||
| let pipe = Pipe() | ||
| let errPipe = Pipe() | ||
| process.standardOutput = pipe | ||
| process.standardError = errPipe | ||
| do { | ||
| try process.run() | ||
| process.waitUntilExit() | ||
| guard process.terminationStatus == 0 else { return nil } | ||
| let output = String(data: pipe.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8)? | ||
| .trimmingCharacters(in: .whitespacesAndNewlines) | ||
| return output?.isEmpty == false ? output : nil |
There was a problem hiding this comment.
process.waitUntilExit()
getKeychainPassword is a synchronous function on a Swift actor. When it calls process.waitUntilExit(), it blocks the actor's serial executor thread for the duration of the subprocess (the security CLI tool). This prevents the actor from processing any other messages while waiting, potentially causing queued operations (e.g., concurrent email reads) to stall.
The same issue exists in GmailReaderService.swift at the equivalent getKeychainPassword implementation.
The fix is to make the function async and replace the blocking wait with an async-safe subprocess runner (e.g., using AsyncStream/withCheckedContinuation around terminationHandler):
process.terminationHandler = { _ in
continuation.resume(returning: ...)
}
try process.run()
// await continuation instead of process.waitUntilExit()Alternatively, since BrowserKeychainCache caches the result, the blocking call only happens once per service per session — but it's still best practice not to block an actor's executor.
## Summary - Remove purple colors from onboarding — replaced with black/white theme - Fix graph edges not persisting (nodes saved but edges dropped due to validation bug) - Move Screen Recording permission before Full Disk Access so restart happens before scanning - Move "Your 2nd brain is live" + Goal steps to end of onboarding for better timing - Start Gmail/Calendar/web research in parallel with file scan using Task.detached - Fix Screen Recording button to actually open System Settings - Add auto-relaunch when app is terminated mid-onboarding (FDA "Quit & Reopen") - Clear graph on onboarding start so stale data doesn't appear - Remove redundant "Mapped" card and helper text from permission steps - Rename "Full Disk Access" to "Disk Access" - Add privaterelay.appleid.com to public domains to fix garbage web search for Apple relay users - Remove keychain cache persistence that triggered extra password prompts - Add enrichment goals logging - Re-trigger insights on research step if app restarted mid-onboarding ## Test plan - [ ] Sign in and go through full onboarding flow - [ ] Verify no purple colors in any step - [ ] Verify Screen Recording step appears before disk access - [ ] Verify file scan + Gmail/Calendar run in parallel - [ ] Verify graph shows connected nodes with edges - [ ] Verify goal suggestions include smart enrichment-based goals - [ ] Verify no keychain password prompts appear - [ ] Verify app relaunches after FDA "Quit & Reopen" 🤖 Generated with [Claude Code](https://claude.com/claude-code)
1 participant
Summary
Test plan
🤖 Generated with Claude Code