Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces significant improvements to the desktop application, including a unified AI bridge (ACP), local-first task management, and enhanced stability measures like memory monitoring and ffmpeg watchdog. The transition to the ACP bridge for all chat functionalities and the implementation of OAuth for Claude accounts are major highlights. Feedback focuses on correcting API usage for process management, avoiding blocking calls on the main thread, and improving safety by avoiding force unwraps.
| let task = Process() | ||
| task.launchPath = "/usr/bin/open" | ||
| task.arguments = ["-n", Bundle.main.bundleURL.path] | ||
| try? task.run() | ||
| NSApp.terminate(nil) |
There was a problem hiding this comment.
The Process.run() method requires the executableURL property to be set. Setting launchPath (which is deprecated) does not satisfy this requirement, and task.run() will likely fail or throw an error. Additionally, launchPath is deprecated in favor of executableURL since macOS 10.13.
| let task = Process() | |
| task.launchPath = "/usr/bin/open" | |
| task.arguments = ["-n", Bundle.main.bundleURL.path] | |
| try? task.run() | |
| NSApp.terminate(nil) | |
| let task = Process() | |
| task.executableURL = URL(fileURLWithPath: "/usr/bin/open") | |
| task.arguments = ["-n", Bundle.main.bundleURL.path] | |
| try? task.run() | |
| NSApp.terminate(nil) |
| killUsernoted.waitUntilExit() | ||
| log("Restarted usernoted to force notification re-discovery") | ||
|
|
||
| // Wait for usernoted to restart before retrying | ||
| Thread.sleep(forTimeInterval: 1.5) |
There was a problem hiding this comment.
Calling waitUntilExit() and Thread.sleep() on the main thread blocks the UI run loop, causing the application to become unresponsive for several seconds. Since startMonitoring uses a completion handler and is typically invoked from the UI layer, this logic should be dispatched to a background queue to avoid freezing the interface.
| try await ActionItemStorage.shared.syncTaskActionItems([created]) | ||
| let inserted = try await ActionItemStorage.shared.insertLocalActionItem(record) | ||
| let localTask = inserted.toTaskActionItem() | ||
| let localId = inserted.id! |
There was a problem hiding this comment.
Force unwrapping inserted.id! is risky. While the database is expected to return an ID for a new record, any unexpected failure or misconfiguration in the persistence layer will cause a runtime crash. It is safer to use optional binding.
| let localId = inserted.id! | |
| guard let localId = inserted.id else { return nil } |
|
Hey @m13v 👋 Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request. After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:
Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out. Thank you for being part of the Omi community! 💜 |
1 participant
Automated sync from omi-desktop at 474e1e1f.
This PR was created by the sync pipeline.