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 several significant improvements, including the transition to the ACPBridge for AI chat, local-first task management in TasksStore, and enhanced UI/UX for tasks and onboarding. The improved accessibility checks and memory management for subprocesses like ffmpeg are also noteworthy. However, there is a regression in the window resizing logic in TasksPage and potential thread safety issues in TasksStore that should be addressed.
| NSAnimationContext.runAnimationGroup({ context in | ||
| context.duration = 0.25 | ||
| context.timingFunction = CAMediaTimingFunction(name: .easeInEaseOut) | ||
| window.animator().setFrame(frame, display: true) | ||
| } | ||
| }, completionHandler: { | ||
| // Clear preChatWindowWidth only after the resize animation completes. | ||
| // If the app quits mid-animation, this won't fire, leaving the saved | ||
| // width intact so restorePreChatWindowWidth() can shrink on next launch. | ||
| if !expand { | ||
| UserDefaults.standard.set(Double(0), forKey: "tasksPreChatWindowWidth") | ||
| } | ||
| }) |
There was a problem hiding this comment.
The local @State variable preChatWindowWidth is not reset to 0 when the chat panel is closed. While the UserDefaults key is cleared in the completion handler, the local state remains at its previous value. This will cause the if preChatWindowWidth == 0 check to be skipped the next time the chat is opened, potentially leading to incorrect window sizing if the window was resized while the chat was closed.
NSAnimationContext.runAnimationGroup({ context in
context.duration = 0.25
context.timingFunction = CAMediaTimingFunction(name: .easeInEaseOut)
window.animator().setFrame(frame, display: true)
}, completionHandler: {
// Clear preChatWindowWidth only after the resize animation completes.
// If the app quits mid-animation, this won't fire, leaving the saved
// width intact so restorePreChatWindowWidth() can shrink on next launch.
if !expand {
self.preChatWindowWidth = 0
UserDefaults.standard.set(Double(0), forKey: "tasksPreChatWindowWidth")
}
})| if let idx = self.incompleteTasks.firstIndex(where: { $0.id == localTask.id }) { | ||
| self.incompleteTasks[idx] = created | ||
| } |
There was a problem hiding this comment.
Modifying self.incompleteTasks (a @Published property) inside a background Task without ensuring it runs on the main actor is unsafe. Since TasksStore is a class and this Task is not explicitly isolated, this modification can occur on a background thread, leading to race conditions or runtime crashes in SwiftUI.
| if let idx = self.incompleteTasks.firstIndex(where: { $0.id == localTask.id }) { | |
| self.incompleteTasks[idx] = created | |
| } | |
| // Replace local_ entry with real backend-synced task | |
| await MainActor.run { | |
| if let idx = self.incompleteTasks.firstIndex(where: { $0.id == localTask.id }) { | |
| self.incompleteTasks[idx] = created | |
| } | |
| } |
| if let updatedTask = try? await ActionItemStorage.shared.getLocalActionItem(byBackendId: task.id) { | ||
| if task.completed { | ||
| if let index = completedTasks.firstIndex(where: { $0.id == task.id }) { | ||
| completedTasks[index] = updatedTask | ||
| } | ||
| } else { | ||
| if let index = incompleteTasks.firstIndex(where: { $0.id == task.id }) { | ||
| incompleteTasks[index] = updatedTask | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Modifying @Published properties like completedTasks and incompleteTasks from an async method without ensuring main-thread execution is unsafe. These updates should be wrapped in await MainActor.run to prevent threading issues in the UI layer.
// 2. Read back and update in-memory
if let updatedTask = try? await ActionItemStorage.shared.getLocalActionItem(byBackendId: task.id) {
await MainActor.run {
if task.completed {
if let index = completedTasks.firstIndex(where: { $0.id == task.id }) {
completedTasks[index] = updatedTask
}
} else {
if let index = incompleteTasks.firstIndex(where: { $0.id == task.id }) {
incompleteTasks[index] = updatedTask
}
}
}
}
|
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.