feat(conversation): add selection child threads #1225
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughThis PR implements a feature to create child conversations from selected text in messages. It adds a context menu item "New Thread from Selection" that captures selection context (text, offsets, surrounding content), creates a new conversation as a child of the parent message, and displays it in a new tab with selection highlights rendered and clickable. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ContextMenu as Context Menu
participant MainProc as Main Process
participant IPCRenderer as Renderer (IPC)
participant Store as Chat Store
participant Presenter as Thread Presenter
participant DB as Database
participant UI as UI Components
User->>ContextMenu: Select text + right-click
ContextMenu->>MainProc: Capture selection event
MainProc->>IPCRenderer: Send context-menu-new-thread<br/>(text, x, y)
rect rgb(230, 240, 250)
Note over IPCRenderer: SelectedTextContextMenu Handler
IPCRenderer->>IPCRenderer: Resolve message element<br/>& content container
IPCRenderer->>IPCRenderer: Calculate offsets &<br/>context (before/after)
IPCRenderer->>IPCRenderer: Compute contentHash
end
IPCRenderer->>Store: createChildThreadFromSelection<br/>(parentMessageId, parentSelection)
rect rgb(240, 250, 240)
Note over Store,DB: Child Conversation Creation
Store->>Presenter: Create conversation<br/>with parent references
Presenter->>DB: Insert conversation<br/>(parent_conversation_id,<br/>parent_message_id,<br/>parent_selection)
DB-->>Presenter: New conversation ID
Presenter-->>Store: conversation created
end
Store->>Store: Refresh childThreadsByMessageId
Store->>Presenter: openThreadInNewTab(conversationId)
Presenter->>MainProc: Open in new tab
MainProc-->>IPCRenderer: Tab opened
rect rgb(250, 245, 230)
Note over UI: Render & Highlight
UI->>UI: Load child conversation
UI->>UI: Apply selection highlights<br/>via scrollToSelectionHighlight
UI->>UI: Render highlight spans<br/>with hover/active styles
end
User->>UI: Click selection highlight
UI->>Store: openThreadInNewTab<br/>(parentConversationId)
Store->>Presenter: Navigate to parent thread
Presenter-->>UI: Display parent context
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
src/renderer/src/components/chat-input/composables/usePromptInputEditor.ts (1)
273-279: Consider adding type safety for mentionData.content.The mentionContent derivation logic is sound, but consider adding runtime type validation for
mentionData.contentto ensure type safety, especially since it can come from various sources (context mentions, MCP entries, etc.).🔎 Suggested type validation
const insertMentionToEditor = (mentionData: CategorizedData, position: number): boolean => { try { + // Validate content is string if present + if (mentionData.content !== undefined && typeof mentionData.content !== 'string') { + console.warn('Invalid mention content type, expected string:', typeof mentionData.content) + } const mentionContent = mentionData.content ?? (mentionData.mcpEntry ? JSON.stringify(mentionData.mcpEntry) : '')src/renderer/src/events.ts (1)
1-8: Chinese comments should be replaced with English.The file contains Chinese comments which violate the coding guidelines. All logs and comments in TypeScript files should be in English.
As per coding guidelines, new code should use English for comments and logs.
🔎 Suggested translation
/** - * 事件系统常量定义 - * 看似这里和 main/events.ts 重复了,其实不然,这里只包含了main上来给renderer的事件 + * Event system constants definition + * This file defines events sent from main process to renderer process * - * 按功能领域分类事件名,采用统一的命名规范: - * - 使用冒号分隔域和具体事件 - * - 使用小写并用连字符连接多个单词 + * Event naming conventions: + * - Use colon to separate domain and specific event + * - Use lowercase with hyphens for multi-word names */src/main/events.ts (1)
1-9: Chinese comments should be replaced with English.The file contains Chinese comments which violate the coding guidelines requiring English for all logs and comments in TypeScript files.
As per coding guidelines, use English for logs and comments in TypeScript files.
🔎 Suggested translation
/** - * 事件系统常量定义 + * Event system constants definition * - * 按功能领域分类事件名,采用统一的命名规范: - * - 使用冒号分隔域和具体事件 - * - 使用小写并用连字符连接多个单词 + * Event naming conventions: + * - Use colon to separate domain and specific event + * - Use lowercase with hyphens for multi-word names * - * 看似这里和 renderer/events.ts 重复了,其实不然,这里只包含了main->renderer 和 main->main 的事件 + * This file defines events for main->renderer and main->main communication */src/renderer/src/components/message/MessageItemUser.vue (1)
203-215: Add error handling for thread navigation.The mention click handler correctly validates the context category and parent IDs, but lacks error handling for the
openThreadInNewTabcall which could fail.As per coding guidelines, always use try-catch to handle possible errors in TypeScript code and provide meaningful error messages.
🔎 Suggested error handling
const handleMentionClick = (block: UserMessageMentionBlock) => { if (block.category !== 'context') { return } const activeThread = chatStore.activeThread if (!activeThread?.parentConversationId || !activeThread.parentMessageId) { return } - chatStore.openThreadInNewTab(activeThread.parentConversationId, { - messageId: activeThread.parentMessageId, - childConversationId: activeThread.id - }) + try { + chatStore.openThreadInNewTab(activeThread.parentConversationId, { + messageId: activeThread.parentMessageId, + childConversationId: activeThread.id + }) + } catch (error) { + console.error('Failed to open parent conversation:', error) + } }src/renderer/src/components/ChatView.vue (1)
149-168: Consider adding error handling for the mention insertion.The watcher correctly guards against null values and consumes the context mention after insertion. However,
appendCustomMentioncould potentially throw if the editor is in an unexpected state.🔎 Suggested improvement
watch( () => [chatStore.activeContextMention, chatInput.value] as const, ([mention, input]) => { if (!mention || !input) return const activeThreadId = chatStore.getActiveThreadId() if (!activeThreadId) return const mentionData: CategorizedData = { id: mention.id, label: mention.label, category: mention.category, type: 'item', content: mention.content } - const inserted = input.appendCustomMention?.(mentionData) - if (inserted) { - chatStore.consumeContextMention(activeThreadId) + try { + const inserted = input.appendCustomMention?.(mentionData) + if (inserted) { + chatStore.consumeContextMention(activeThreadId) + } + } catch (error) { + console.error('Failed to append context mention:', error) } }, { immediate: true } )src/renderer/src/components/message/MessageContent.vue (1)
77-85: Inconsistent fallback ingetMentionLabelfor context category.When
block.category === 'context'andblock.idis missing, the function falls back toblock.category(which would be the string'context'). This seems less useful than falling back toblock.contentor a more descriptive label.🔎 Suggested fix
const getMentionLabel = (block: UserMessageMentionBlock) => { if (block.category === 'prompts') { return block.id || block.content } if (block.category === 'context') { - return block.id || block.category + return block.id || block.content || 'context' } return block.content }src/renderer/src/components/chat-input/ChatInput.vue (1)
902-912: Scheduled timeouts may not be cleaned up on unmount.The
scheduleCaretToEndfunction schedules multiple timeouts that could fire after the component unmounts. While theeditor.isDestroyedcheck provides some protection, consider tracking and clearing these timeouts inonUnmounted.🔎 Suggested improvement
+const pendingCaretTimeouts: ReturnType<typeof setTimeout>[] = [] + const scheduleCaretToEnd = () => { const delays = [0, 50, 120] for (const delay of delays) { - setTimeout(() => { + const timeoutId = setTimeout(() => { if (editor.isDestroyed) return requestAnimationFrame(() => { setCaretToEnd() }) }, delay) + pendingCaretTimeouts.push(timeoutId) } } // In onUnmounted, add: + pendingCaretTimeouts.forEach(clearTimeout) + pendingCaretTimeouts.length = 0src/renderer/src/views/ChatTabView.vue (1)
129-133: Pending scroll retry logic is solid; consider explicit timer cleanup on unmountThe retry loop around
pendingScrollRetryTimer/pendingScrollRetryCountandMAX_PENDING_SCROLL_RETRYis well‑structured and guards against infinite retries, including variant reset and fallback tohandleScrollToMessage.One small robustness improvement: add an
onBeforeUnmount(or equivalent teardown) that clearspendingScrollRetryTimerif it’s still set, mirroring the pattern used forhighlightRefreshTimerinMessageList.vue. That avoids any stray retries firing shortly after this view is destroyed, even though today the maximum window is already small (~12 × 60ms).Also applies to: 149-260
src/renderer/src/components/message/MessageList.vue (1)
68-69: Selection highlighting & scroll API look good; consider sharing hash/offset utilitiesThe selection highlight pipeline (computing offsets from
ParentSelection, wrapping text ranges in.selection-highlightspans, and exposingscrollToSelectionHighlight) is well put together and cleans up correctly viaclearSelectionHighlightsandonBeforeUnmount.The only maintainability concern is duplication of
hashTextand the offset‑resolution logic withSelectedTextContextMenu.vue. It would be safer long‑term to move these into a small shared helper (e.g. under@/lib/selection), so both the context‑menu payload builder and the highlighter stay in lockstep if the selection format or matching heuristics change.Also applies to: 82-83, 136-137, 203-213, 214-385, 387-413, 415-425, 495-509
src/renderer/src/components/message/SelectedTextContextMenu.vue (1)
7-15: New-thread-from-selection flow is correct; consider sharing selection helpersThe end‑to‑end logic for
handleNewThreadFromSelection(selection detection, resolving intoParentSelection, and delegating tochatStore.createChildThreadFromSelectionwith localized error notification) looks sound.Two minor suggestions:
hashTextandresolveOffsetsmirror the logic inMessageList.vue. Pulling these into a shared helper would avoid subtle drift if the selection format or matching heuristics evolve.- Right now non‑message selections / unresolved offsets only trigger
console.warn. If UX feedback shows confusion, you might later add a small i18n error toast for those cases as well.Also applies to: 30-71, 73-143, 145-167
src/main/presenter/sqlitePresenter/tables/conversations.ts (1)
28-31: Parent-conversation schema wiring is correct; consider centralizing row→conversation mappingThe new
parent_conversation_id,parent_message_id, andparent_selectioncolumns, migration (v9), and mappings inget,list,listByParentConversationId, andlistByParentMessageIdsall line up with the sharedCONVERSATION/ParentSelectiontypes and should behave correctly.There is quite a bit of duplicated mapping logic when turning a
ConversationRowinto aCONVERSATION(settings, acpWorkdirMap, parent* fields). Extracting a small private helper likemapRowToConversation(row: ConversationRow & { agent_workspace_path: string | null; acp_workdir_map: string | null }): CONVERSATIONand reusing it in all four methods would reduce drift risk if you add/change settings fields again.Also applies to: 132-141, 146-148, 150-181, 209-213, 242-248, 465-517, 521-597, 599-680
src/main/presenter/threadPresenter/index.ts (1)
1-6: Child-conversation creation from selection is well designed; only minor cleanups possibleThe implementation of
createChildConversationFromSelectioncorrectly:
- Validates the parent conversation and message.
- Derives new settings by merging parent settings with overrides and clearing
selectedVariantsMap.- Creates the child conversation and persists
parentConversationId,parentMessageId, and a parsedParentSelection.- Optionally opens the child in a new tab (or falls back to the provided tab id) and broadcasts a thread list update.
Two small optional cleanups:
sqlitePresenter.getConversation(parentConversationId)already throws when the conversation is missing, so the subsequentif (!parentConversation) throw ...is effectively unreachable and could be removed.- If you want clearer error diagnostics when the parent message is missing, you could catch/augment the error from
messageManager.getMessage(parentMessageId)with the parent ids before rethrowing, but functionally it’s fine as is.Also applies to: 253-327, 776-854, 856-862
| type PendingContextMention = { | ||
| id: string | ||
| label: string | ||
| category: 'context' | ||
| content: string | ||
| } | ||
|
|
||
| type PendingScrollTarget = { | ||
| messageId?: string | ||
| childConversationId?: string | ||
| } | ||
|
|
There was a problem hiding this comment.
Child-thread/context state wiring is solid; fix one i18n nit and consider tiny refactors
The new store state and helpers around child threads and context look well thought out:
childThreadsByMessageId,pendingContextMentions, andpendingScrollTargetByConversationplus theactiveContextMention/activePendingScrollTargetcomputeds giveMessageList.vueandChatTabView.vueexactly what they need.refreshChildThreadsForActiveThreadandmaybeQueueContextMentionintegrate neatly intoloadMessages, and the SCROLL_TO_MESSAGE listener +queueScrollTarget/consumePendingScrollMessagefit the pending scroll retry loop.
A few small points:
- In
formatContextLabel, the fallback'context'is a user-facing label (used inPendingContextMention.label). Per repo i18n rules, this should come fromt(...)rather than being hard-coded, e.g.t('chat.context.defaultLabel')or similar. createChildThreadFromSelectionpassesparentConversation.settingsintocreateChildConversationFromSelection, whose implementation then merges again with the parent’s settings. That duplication is harmless but you could simplify by only passing overrides if you ever introduce them.clearSelectedVariantForMessagecorrectly firesupdateSelectedVariantasynchronously and returns a boolean; the currentvoidcall is fine, but if you ever need to react to persistence failures you might want to await it in the future.
Overall, the new store surface area (including exports) lines up well with the presenter and view logic.
Also applies to: 66-69, 143-153, 229-271, 419-437, 439-462, 464-479, 481-529, 650-690, 1402-1407, 1592-1600, 1757-1803
🤖 Prompt for AI Agents
In src/renderer/src/stores/chat.ts around lines 31 to 42 (and also at the other
listed ranges), replace the hard-coded user-facing fallback label 'context' with
an i18n call (e.g. t('chat.context.defaultLabel')) so
PendingContextMention.label uses a localized string; while here, optionally
simplify createChildThreadFromSelection by not passing the full
parentConversation.settings into createChildConversationFromSelection (pass only
overrides) to avoid redundant merging, and consider awaiting
updateSelectedVariant in clearSelectedVariantForMessage when you need to observe
persistence failures.
Summary by CodeRabbit
New Features
Enhancements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.