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 is a large automated sync that introduces significant refactoring and new features. Key changes include unifying the AI chat bridge to a single ACP-based implementation, a major overhaul of the task management UI/UX with local-first creation and drag-and-drop, and numerous improvements to robustness and concurrency handling across the application. My review found one critical issue related to state management in the new local-first task creation flow that could lead to data desynchronization. Otherwise, the changes are of high quality and represent substantial improvements to the application.
| // Sync to backend in background | ||
| Task { | ||
| do { | ||
| var metadata: [String: Any]? = nil | ||
| if let tags = tags, !tags.isEmpty { | ||
| metadata = ["tags": tags] | ||
| } | ||
|
|
||
| let created = try await APIClient.shared.createActionItem( | ||
| description: description, | ||
| dueAt: dueAt, | ||
| source: "manual", | ||
| priority: priority, | ||
| category: tags?.first, | ||
| metadata: metadata, | ||
| recurrenceRule: recurrenceRule | ||
| ) | ||
|
|
||
| try await ActionItemStorage.shared.markSynced(id: localId, backendId: created.id) | ||
|
|
||
| // Replace local_ entry with real backend-synced task | ||
| if let idx = self.incompleteTasks.firstIndex(where: { $0.id == localTask.id }) { | ||
| self.incompleteTasks[idx] = created | ||
| } | ||
| log("TasksStore: Task synced to backend (local \(localId) → \(created.id))") | ||
| } catch { | ||
| logError("TasksStore: Failed to sync new task to backend (will retry on next launch)", error: error) | ||
| } | ||
| } |
There was a problem hiding this comment.
The new local-first task creation is a great UX improvement. However, there's a potential issue with how the temporary local ID is replaced by the permanent backend ID.
When a task is created, it's immediately added to the UI with a local ID (e.g., local_...). If the user interacts with this task (e.g., opens the task chat) before the backend sync completes, other components like TaskChatCoordinator will store state associated with this temporary local ID.
After the backend sync completes, this background Task replaces the local task object with the synced one, which has a new backend ID. The state in TaskChatCoordinator (and potentially other places) is not migrated to the new ID, becoming orphaned. This can lead to data loss for the chat session or other unpredictable behavior.
To fix this, you should implement a mechanism to notify relevant components about the ID change so they can migrate their state. For example, you could post a Notification with the old and new IDs, or have the TasksStore publish this mapping. The TaskChatCoordinator would then need a method to handle this migration, for instance by renaming its state keys from the local ID to the backend ID.
1 participant
Automated sync from omi-desktop at 474e1e1f.
This PR was created by the sync pipeline.