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
This pull request synchronizes the desktop application with the latest changes from the omi-desktop repository. Key updates include the unification of the AI chat bridge to use the ACP (Agent Client Protocol) exclusively, supporting both Omi's API key and user-provided Claude accounts via OAuth. The PR also introduces recurring task scheduling, parallel task agents, and significant improvements to the task management UI, such as drag-and-drop reordering and inline task creation. Additionally, robustness enhancements have been made to resource monitoring, including an extreme memory auto-restart mechanism and improved handling of external processes like FFmpeg. Feedback is focused on critical thread management and the use of deprecated APIs in system-level services.
| 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.
Using Thread.sleep on the main thread is a critical issue as it blocks the UI and event processing for 1.5 seconds. Since this logic is triggered from startMonitoring, which is typically called from the UI layer (e.g., in DesktopHomeView), this will cause a noticeable hang. In an asynchronous context, you should use Task.sleep to avoid blocking the thread.
| // Extreme threshold - auto-restart to prevent the system from becoming unresponsive. | ||
| // Without this, memory can climb to 7GB+, causing SQLite I/O failures and making | ||
| // the app impossible to reopen without a full computer restart. | ||
| if snapshot.memoryFootprintMB >= memoryAutoRestartThreshold && !autoRestartTriggered && !isDevBuild { |
There was a problem hiding this comment.
The variable isDevBuild appears to be a typo for the global isDev defined in OmiApp.swift. If isDevBuild is not defined within the scope of ResourceMonitor, this will cause a compilation error. Please verify the variable name.
|
|
||
| // Give Sentry 3 seconds to flush, then relaunch and terminate | ||
| DispatchQueue.main.asyncAfter(deadline: .now() + 3.0) { | ||
| let task = Process() |
There was a problem hiding this comment.
The launchPath property of Process is deprecated. It is recommended to use executableURL instead for better compatibility and safety.
| let task = Process() | |
| task.executableURL = URL(fileURLWithPath: "/usr/bin/open") |
|
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.