feat: add conversations to cmdk#1934
Conversation
Greptile SummaryThis PR wires conversations into the command palette (cmdk) by adding a
Confidence Score: 3/5Needs fixes before merging — selecting a conversation from the palette while on a different task navigates to the task but leaves the conversation un-opened, and the shared remove() helper has a latent cross-type footgun. The navigation handler calls getTaskView synchronously before navigate(), so openConversation silently no-ops whenever the user is not already on the target task. This is the primary new user-facing flow introduced by the PR and will produce the wrong outcome in the common cross-task case. The unscoped remove() in SearchService is a second issue that could corrupt the FTS index if any two tables ever share an ID. command-palette-modal.tsx (navigation handler) and search-service.ts (remove helper)
|
| Filename | Overview |
|---|---|
| src/renderer/features/command-palette/command-palette-modal.tsx | Adds conversation results to the palette with a navigation handler that races with view provisioning — openConversation silently no-ops when the target task isn't already loaded |
| src/main/core/search/search-service.ts | Adds conversation indexing with FTS and recents support; remove() is type-unscoped, risking cross-type ID collisions |
| src/main/core/conversations/conversation-events.ts | New event bus for conversation CRUD; _emit is technically public, consistent with existing event classes |
| src/main/db/initialize.ts | Bumps FTS version to 2, adds task_id UNINDEXED column — correctly drops and recreates the virtual table on version mismatch |
| src/shared/search.ts | Adds 'conversation' to SearchItemKind and taskId field to SearchItem; clean shared type extension |
Sequence Diagram
sequenceDiagram
participant User
participant CmdK as CommandPaletteModal
participant SS as SearchService
participant FTS as search_index (FTS5)
participant Nav as navigate()
participant TV as TaskViewStore
Note over User,TV: Select conversation from palette
User->>CmdK: select conversation item
CmdK->>TV: getTaskView(projectId,taskId)?.openConversation(id)
Note right of TV: Returns undefined if task not provisioned
CmdK->>Nav: "navigate('task', {projectId, taskId})"
Nav-->>User: task view loaded (conversation may not open)
Note over SS,FTS: Index maintenance
SS->>FTS: "conversation:created => upsertConversation"
SS->>FTS: "conversation:renamed => upsertConversationById"
SS->>FTS: "conversation:deleted => remove(item_id)"
Comments Outside Diff (1)
-
src/main/core/search/search-service.ts, line 234-240 (link)remove()deletes byitem_idacross all item types — a deleted task can silently remove a conversation with the same IDThe
removehelper runsDELETE FROM search_index WHERE item_id = ?without filtering onitem_type. Whentask:deletedortask:archivedfires,this.remove(taskId)will delete any row whoseitem_idmatches that value, regardless of type.conversation:deletednow also routes through the same helper, so correctness relies entirely on global UUID uniqueness across tables. Scoping the delete toWHERE item_id = ? AND item_type = ?would make the invariant explicit and safe.Prompt To Fix With AI
This is a comment left during a code review. Path: src/main/core/search/search-service.ts Line: 234-240 Comment: **`remove()` deletes by `item_id` across all item types — a deleted task can silently remove a conversation with the same ID** The `remove` helper runs `DELETE FROM search_index WHERE item_id = ?` without filtering on `item_type`. When `task:deleted` or `task:archived` fires, `this.remove(taskId)` will delete any row whose `item_id` matches that value, regardless of type. `conversation:deleted` now also routes through the same helper, so correctness relies entirely on global UUID uniqueness across tables. Scoping the delete to `WHERE item_id = ? AND item_type = ?` would make the invariant explicit and safe. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/renderer/features/command-palette/command-palette-modal.tsx:133-138
**`openConversation` silently no-ops when called from a different task context**
`getTaskView` requires the target task to already be provisioned in the renderer store. If the user is on a different task (or no task) when they select a conversation from the palette, `getTaskView` returns `undefined`, `openConversation` is never called, and `navigate('task', ...)` just takes them to the task's default view — the specific conversation is never opened. The bug reproduces any time the command palette is opened outside the exact task that owns the conversation. `getTaskView` is also documented to be called only inside `observer` components or MobX reactions; invoking it from a plain event handler may miss reactive state updates.
### Issue 2 of 2
src/main/core/search/search-service.ts:234-240
**`remove()` deletes by `item_id` across all item types — a deleted task can silently remove a conversation with the same ID**
The `remove` helper runs `DELETE FROM search_index WHERE item_id = ?` without filtering on `item_type`. When `task:deleted` or `task:archived` fires, `this.remove(taskId)` will delete any row whose `item_id` matches that value, regardless of type. `conversation:deleted` now also routes through the same helper, so correctness relies entirely on global UUID uniqueness across tables. Scoping the delete to `WHERE item_id = ? AND item_type = ?` would make the invariant explicit and safe.
Reviews (1): Last reviewed commit: "feat: add conversations to cmdk" | Re-trigger Greptile
| const handleNavigateToConversation = (item: SearchItem) => { | ||
| if (!item.projectId || !item.taskId) return; | ||
| getTaskView(item.projectId, item.taskId)?.tabManager.openConversation(item.id); | ||
| onClose(); | ||
| navigate('task', { projectId: item.projectId, taskId: item.taskId }); | ||
| }; |
There was a problem hiding this comment.
openConversation silently no-ops when called from a different task context
getTaskView requires the target task to already be provisioned in the renderer store. If the user is on a different task (or no task) when they select a conversation from the palette, getTaskView returns undefined, openConversation is never called, and navigate('task', ...) just takes them to the task's default view — the specific conversation is never opened. The bug reproduces any time the command palette is opened outside the exact task that owns the conversation. getTaskView is also documented to be called only inside observer components or MobX reactions; invoking it from a plain event handler may miss reactive state updates.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/features/command-palette/command-palette-modal.tsx
Line: 133-138
Comment:
**`openConversation` silently no-ops when called from a different task context**
`getTaskView` requires the target task to already be provisioned in the renderer store. If the user is on a different task (or no task) when they select a conversation from the palette, `getTaskView` returns `undefined`, `openConversation` is never called, and `navigate('task', ...)` just takes them to the task's default view — the specific conversation is never opened. The bug reproduces any time the command palette is opened outside the exact task that owns the conversation. `getTaskView` is also documented to be called only inside `observer` components or MobX reactions; invoking it from a plain event handler may miss reactive state updates.
How can I resolve this? If you propose a fix, please make it concise.
No description provided.