Conversation
Greptile SummaryThis PR fixes a race condition in the desktop onboarding reset flow where Key changes:
Issues found:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant SettingsPage
participant AppState
participant Main as Main Thread
participant BG as BG Thread
participant DesktopHomeView
participant UserDefaults
User->>SettingsPage: Tap Reset and Restart
SettingsPage->>AppState: resetOnboardingAndRestart()
AppState->>Main: schedule post(resetOnboardingRequested) async
AppState->>UserDefaults: removeObject x11 keys + synchronize
AppState->>BG: schedule sleep(0.15) then restartApp()
Note over Main: Next run-loop iteration
Main->>DesktopHomeView: onReceive(resetOnboardingRequested)
DesktopHomeView->>AppState: hasCompletedOnboarding = false
DesktopHomeView->>UserDefaults: onboardingStep = 0 via AppStorage
DesktopHomeView->>UserDefaults: onboardingJustCompleted = false via AppStorage
DesktopHomeView->>AppState: stopTranscription()
Note over BG: After 150 ms
BG->>AppState: restartApp()
AppState->>Main: NSApplication.terminate(nil)
Last reviewed commit: b717ab7 |
| // Update live @AppStorage state in the current app instance before touching | ||
| // raw UserDefaults so SwiftUI doesn't write stale onboarding values back. | ||
| DispatchQueue.main.async { | ||
| NotificationCenter.default.post(name: .resetOnboardingRequested, object: nil) | ||
| } |
There was a problem hiding this comment.
Misleading comment — notification fires after UserDefaults clearing on the main thread
The inline comment says "before touching raw UserDefaults", but both call sites (SettingsPage.swift button action and OmiApp.swift's @MainActor resetOnboarding()) invoke this function on the main thread. Because DispatchQueue.main.async simply enqueues a block for the next run-loop iteration, the synchronous UserDefaults.standard.removeObject loop below runs first. The notification is therefore dispatched after the keys are already cleared, not before.
This doesn't cause a functional bug in practice (clearing + writing 0/false produces the same final state), but the comment misrepresents the ordering and could mislead future readers about the intent of the guard.
Consider either:
- Posting the notification synchronously (before the removal loop) to match the comment's intent:
// Reset live @AppStorage before clearing raw UserDefaults
NotificationCenter.default.post(name: .resetOnboardingRequested, object: nil)
// Clear onboarding-related UserDefaults keys
for key in onboardingKeys { ... }- Or, keeping the current order and correcting the comment to reflect what actually happens:
// Clear raw UserDefaults first, then reset live @AppStorage via notification
// so any subsequent @AppStorage write-backs use 0/false (the desired reset state).
DispatchQueue.main.async {
NotificationCenter.default.post(name: .resetOnboardingRequested, object: nil)
}| DispatchQueue.global(qos: .utility).async { [self] in | ||
| Thread.sleep(forTimeInterval: 0.15) |
There was a problem hiding this comment.
Thread.sleep blocks a GCD thread pool worker
Thread.sleep(forTimeInterval: 0.15) ties up a thread in the shared GCD utility thread pool for 150 ms. While this runs just before termination and is unlikely to cause starvation in practice, it is an unconventional pattern when DispatchQueue.main.asyncAfter (or a Task { try await Task.sleep(...) }) would express the delay without blocking a pool thread.
DispatchQueue.main.asyncAfter(deadline: .now() + 0.15) { [self] in
self.restartApp()
}This also removes the need for DispatchQueue.global(qos: .utility).async wrapping.
| .onReceive(NotificationCenter.default.publisher(for: .resetOnboardingRequested)) { _ in | ||
| log("DesktopHomeView: resetOnboardingRequested — clearing live onboarding state for current app") | ||
| appState.hasCompletedOnboarding = false | ||
| onboardingStep = 0 | ||
| onboardingJustCompleted = false | ||
| appState.stopTranscription() | ||
| } |
There was a problem hiding this comment.
@AppStorage writes recreate the UserDefaults keys that were just removed
resetOnboardingAndRestart() removes "onboardingStep" and "onboardingJustCompleted" from UserDefaults synchronously. On the very next run-loop iteration, this handler fires and the @AppStorage setter assignments immediately write those same keys back into UserDefaults with their default values (0 / false).
The net result is functionally identical to having removed them — both paths produce the default value on next read. However, it is worth noting that after the notification fires the keys will be present in UserDefaults (value 0/false) rather than absent. If the intent is strictly "keys absent after reset," the two explicit @AppStorage writes here are unnecessary since @AppStorage returns the declared default automatically when a key is missing.
Summary
Testing