feat(chat): inline skill input context#1908
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBackend changes thread enabled skill and plugin policy through tool execution and skill filtering. Frontend changes represent chat skills, files, and command prompts as TipTap nodes and pass inline items through message composition, storage, and rendering. ChangesSkill/plugin allowlist propagation
Inline chat items
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Nice |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/components/chat/composables/useChatInputMentions.ts (1)
323-383: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winHardcoded user-facing strings violate i18n guidelines.
confirmText: 'Send'(line 329),confirmText: 'Insert'(line 373), anddescription: 'Fill prompt arguments before insertion.'(line 372) are user-facing strings displayed inCommandFormView.vuebut are not internationalized. As per coding guidelines, all user-facing strings should use vue-i18n keys fromsrc/renderer/src/i18n.The composable already receives translated strings via options (e.g.,
compactCommandDescription). Consider addingsendTextandinsertTextoptions, or importinguseI18ndirectly in the composable since it's called within a component setup.🌍 Proposed fix: add i18n options
export interface UseChatInputMentionsOptions { // ... existing options ... compactCommandDescription?: ComputedRef<string> + sendButtonText?: ComputedRef<string> + insertButtonText?: ComputedRef<string> + promptArgsDescription?: ComputedRef<string> }Then in
ChatInputBox.vue:const mentions = useChatInputMentions({ // ... existing options ... compactCommandDescription: computed(() => t('chat.compaction.commandDescription')), + sendButtonText: computed(() => t('common.send')), + insertButtonText: computed(() => t('common.insert')), + promptArgsDescription: computed(() => t('chat.input.promptArgsDescription')) })And in
useChatInputMentions.ts:- confirmText: 'Send', + confirmText: options.sendButtonText?.value ?? 'Send',- description: action.prompt.description || 'Fill prompt arguments before insertion.', - confirmText: 'Insert', + description: action.prompt.description || options.promptArgsDescription?.value ?? '', + confirmText: options.insertButtonText?.value ?? 'Insert',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/chat/composables/useChatInputMentions.ts` around lines 323 - 383, The user-facing strings in useChatInputMentions are hardcoded and should be localized. Update the command and prompt paths in this composable so the confirm labels and prompt-description text come from vue-i18n instead of literal strings, using either new options passed in from the caller or useI18n within the composable. Keep the changes focused around insertCommandFormNode, the request-prompt-args branch, and the existing translated options pattern used by ChatInputBox.Source: Coding guidelines
🧹 Nitpick comments (5)
src/main/presenter/agentRuntimePresenter/index.ts (1)
3448-3459: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant
resolveAgentExtensionPolicyfetch in the pre-stream path.
resolveActiveSkillNamesForToolProfile(Line 6718) already callsresolveAgentExtensionPolicyinternally. Immediately after using it at Line 3448-3449,runStreamForMessagecallsresolveAgentExtensionPolicyagain at the new Line 3450 for the same sessionId — a duplicate async round trip toconfigPresenter.resolveDeepChatAgentConfig. The same pattern repeats inloadToolDefinitionsForSession(Line 6631-6637) wheneveractiveSkillNamesOverrideis undefined. Given this file explicitly instruments pre-stream steps for slowness (logSlowPreStreamStep), avoiding the extra round trip is worth doing.Consider letting
resolveActiveSkillNamesForToolProfileaccept an optional pre-resolved policy (mirroring howresolveToolProfilealready acceptsextensionPolicy?: AgentExtensionPolicy), so callers that already have the policy don't trigger a second resolution.Also applies to: 6631-6643, 6718-6736
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/agentRuntimePresenter/index.ts` around lines 3448 - 3459, `runStreamForMessage` and `loadToolDefinitionsForSession` are doing a redundant `resolveAgentExtensionPolicy` call for the same session after `resolveActiveSkillNamesForToolProfile` already resolves it internally. Update `resolveActiveSkillNamesForToolProfile` to accept an optional pre-resolved policy, similar to `resolveToolProfile`, and thread that policy through the pre-stream path so `loadToolDefinitionsForSession` can reuse it instead of triggering a second async fetch via `configPresenter.resolveDeepChatAgentConfig`.test/main/presenter/skillPresenter/skillTools.test.ts (1)
155-163: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd test coverage for
allowedPluginIdsfiltering.The new
allowedPluginIdsparameter inhandleSkillListandhandleSkillViewis untested. Both methods now perform plugin-based authorization filtering, but no test verifies that a skill with anownerPluginIdnot in the allowlist is correctly blocked. The mock data also lacksownerPluginIdon any skill, so the plugin filtering path is never exercised.Consider adding:
- Mock skill metadata with
ownerPluginIdset.- A
handleSkillListtest passingallowedPluginIdsand verifying plugin-owned skills are filtered.- A
handleSkillViewtest passingallowedPluginIdsand verifying rejection whenownerPluginIdis not in the allowlist.♻️ Suggested test additions
{ name: 'git-commit', description: 'Git commit message generator', path: '/skills/git-commit/SKILL.md', skillRoot: '/skills/git-commit', allowedTools: ['run_terminal_cmd'], - metadata: { tags: ['git'] } + metadata: { tags: ['git'] } + }, + { + name: 'plugin-skill', + description: 'A plugin-provided skill', + path: '/skills/plugin-skill/SKILL.md', + skillRoot: '/skills/plugin-skill', + allowedTools: ['read_file'], + ownerPluginId: 'plugin-abc' } ]+ it('filters skills by allowedPluginIds', async () => { + const result = await skillTools.handleSkillList( + 'conv-123', + undefined, + undefined, + ['plugin-abc'] + ) + + expect(result.totalCount).toBe(1) + expect(result.skills.map((s) => s.name)).toEqual(['plugin-skill']) + }) + + it('blocks plugin-owned skills when plugin id is not in allowlist', async () => { + const result = await skillTools.handleSkillList( + 'conv-123', + undefined, + undefined, + ['other-plugin'] + ) + + expect(result.totalCount).toBe(0) + })+ it('rejects viewing a plugin-owned skill not in the allowedPluginIds', async () => { + const result = await skillTools.handleSkillView( + 'conv-123', + { name: 'plugin-skill' }, + undefined, + ['other-plugin'] + ) + + expect(mockSkillPresenter.viewSkill).not.toHaveBeenCalled() + expect(result).toEqual({ + success: false, + name: 'plugin-skill', + error: "Skill 'plugin-skill' is not enabled for this agent" + }) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/presenter/skillPresenter/skillTools.test.ts` around lines 155 - 163, The new allowedPluginIds authorization path in handleSkillList and handleSkillView is not covered, so add tests that exercise plugin-based filtering. Update the mock skill metadata to include ownerPluginId on at least one skill, then add a handleSkillList case that passes an allowlist and verifies skills with a non-allowed ownerPluginId are excluded. Also add a handleSkillView case that passes allowedPluginIds and confirms a skill owned by a blocked plugin is rejected, using the existing skillTools and mockSkillPresenter setup.src/renderer/src/components/chat/nodes/commandForm.ts (1)
33-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused
pendingCommandandpendingPromptattributes.These attributes are defined on the node schema but never read by
CommandFormView.vueor any other consumer. The pending form state is managed viapendingFormDatainuseChatInputMentions.ts. Dead schema attributes add noise and may confuse future maintainers.♻️ Proposed fix
fields: { default: '[]' }, - pendingCommand: { - default: null - }, - pendingPrompt: { - default: null - } } },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/chat/nodes/commandForm.ts` around lines 33 - 38, Remove the unused node schema attributes pendingCommand and pendingPrompt from the CommandForm definition, since CommandFormView.vue and the rest of the chat flow rely on pendingFormData from useChatInputMentions.ts instead. Update the CommandForm schema so only actively consumed fields remain, and make sure any references in the CommandForm-related node setup or consumers are cleaned up if present.src/renderer/src/components/chat/ChatInputBox.vue (1)
189-218: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove dead code:
insertSkillChipNodeandinsertFileAttachmentNodeare never called.These functions are defined but not invoked by any watcher, handler, or
defineExpose. The sync functions (syncSkillNodes,syncFileNodes) handle all node insertion/removal. Additionally, theinsertFileAttachmentNodecomment says "at the end of the first paragraph" but inserts at position 1 (the start), which is misleading.♻️ Proposed fix
-/** Insert a SkillChip node at the start of the first paragraph */ -function insertSkillChipNode(skillName: string) { - const firstChild = editor.state.doc.firstChild - if (!firstChild) return - const insertPos = 1 - editor - .chain() - .focus() - .insertContentAt(insertPos, { type: 'skillChip', attrs: { skillName } }) - .run() -} - -/** Insert a FileAttachment node at the end of the first paragraph */ -function insertFileAttachmentNode(file: MessageFile) { - const firstChild = editor.state.doc.firstChild - if (!firstChild) return - const insertPos = 1 - editor - .chain() - .focus() - .insertContentAt(insertPos, { - type: 'fileAttachment', - attrs: { - fileName: file.name || 'file', - filePath: file.path || file.name, - mimeType: file.mimeType || '' - } - }) - .run() -} - /** Ensure editor SkillChip nodes mirror skillsData.activeSkills */ function syncSkillNodes() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/chat/ChatInputBox.vue` around lines 189 - 218, Remove the unused helper functions insertSkillChipNode and insertFileAttachmentNode from ChatInputBox.vue since they are dead code and not referenced by any watcher, handler, or defineExpose; keep the syncSkillNodes and syncFileNodes flows as the only node insertion/removal paths. If you decide to keep insertFileAttachmentNode for future use, update its comment to match the actual insertion position or change the insertion logic so the implementation matches the “end of the first paragraph” description.test/renderer/components/ChatInputBox.test.ts (1)
27-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMock extensions are adequate but
descendantsreturningundefinedlimits sync logic coverage.The
descendants: vi.fn()mock returnsundefinedby default, sosyncSkillNodes/syncFileNodeswill see no existing nodes and skip deletion paths entirely. Consider providing a mock implementation that returns an empty array or configurable node list to test both insertion and removal flows.♻️ Proposed fix
- descendants: vi.fn(), + descendants: vi.fn((cb: (node: any, pos: number) => void | boolean) => { + // Default: no nodes; tests can override via mockReturnValue + return undefined + }),Then in specific tests:
editor.state.doc.descendants.mockImplementation((cb) => { cb({ type: { name: 'fileAttachment' }, attrs: { filePath: '/tmp/a.txt' }, nodeSize: 1 }, 1) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/renderer/components/ChatInputBox.test.ts` around lines 27 - 79, The MockEditor in ChatInputBox tests leaves state.doc.descendants as a bare vi.fn(), so syncSkillNodes and syncFileNodes can’t exercise existing-node deletion paths. Update the MockEditor mock to give descendants a usable implementation (for example, returning a configurable node list or invoking the callback for supplied nodes) so tests can cover both insertion and removal behavior. Keep the change localized to MockEditor and any tests that need to simulate fileAttachment or skill nodes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/src/components/chat/ChatInputBox.vue`:
- Around line 220-304: The sync logic in syncSkillNodes and syncFileNodes is
using stale ProseMirror positions within a single chained transaction, so
multiple deletes/inserts can target the wrong place. Update the deletion loops
to remove nodes in reverse document order (or otherwise map positions through
the transaction) in both syncSkillNodes and syncFileNodes, and avoid reusing
cursorPos/findFileInsertPos() from the pre-transaction state when inserting.
Recompute the insertion anchor from the current editor state or map it through
the chain before each insert so SkillChip and fileAttachment nodes stay aligned.
In `@src/renderer/src/components/chat/nodes/CommandFormView.vue`:
- Around line 32-38: The `CommandFormView` inputs currently submit on every
Enter press via `@keydown.enter.prevent="handleSubmit"`, which is too aggressive
for multi-field prompt argument forms. Update the `Input` handling so Enter only
calls `handleSubmit` when the form has a single field (the
`request-command-input` case), and otherwise allow normal field
navigation/behavior for multi-field scenarios. Use the existing `field.name`,
`formValues`, and `handleSubmit` logic to conditionally apply the Enter handler
based on the number of fields.
In `@src/renderer/src/components/chat/nodes/FileAttachmentView.vue`:
- Around line 9-15: The remove button in FileAttachmentView.vue is missing an
accessible name, so add an aria-label to the button used by the handleRemove
action. Update the button element that wraps the lucide:x Icon so screen readers
can announce its purpose clearly, keeping the existing click/removal behavior
unchanged.
In `@src/renderer/src/components/chat/nodes/SkillChipView.vue`:
- Around line 9-15: The remove button in SkillChipView.vue lacks an accessible
name, so screen readers cannot identify its purpose. Update the button element
in SkillChipView so the remove action has an explicit aria-label on the existing
icon-only button, keeping the current handleRemove behavior unchanged.
---
Outside diff comments:
In `@src/renderer/src/components/chat/composables/useChatInputMentions.ts`:
- Around line 323-383: The user-facing strings in useChatInputMentions are
hardcoded and should be localized. Update the command and prompt paths in this
composable so the confirm labels and prompt-description text come from vue-i18n
instead of literal strings, using either new options passed in from the caller
or useI18n within the composable. Keep the changes focused around
insertCommandFormNode, the request-prompt-args branch, and the existing
translated options pattern used by ChatInputBox.
---
Nitpick comments:
In `@src/main/presenter/agentRuntimePresenter/index.ts`:
- Around line 3448-3459: `runStreamForMessage` and
`loadToolDefinitionsForSession` are doing a redundant
`resolveAgentExtensionPolicy` call for the same session after
`resolveActiveSkillNamesForToolProfile` already resolves it internally. Update
`resolveActiveSkillNamesForToolProfile` to accept an optional pre-resolved
policy, similar to `resolveToolProfile`, and thread that policy through the
pre-stream path so `loadToolDefinitionsForSession` can reuse it instead of
triggering a second async fetch via
`configPresenter.resolveDeepChatAgentConfig`.
In `@src/renderer/src/components/chat/ChatInputBox.vue`:
- Around line 189-218: Remove the unused helper functions insertSkillChipNode
and insertFileAttachmentNode from ChatInputBox.vue since they are dead code and
not referenced by any watcher, handler, or defineExpose; keep the syncSkillNodes
and syncFileNodes flows as the only node insertion/removal paths. If you decide
to keep insertFileAttachmentNode for future use, update its comment to match the
actual insertion position or change the insertion logic so the implementation
matches the “end of the first paragraph” description.
In `@src/renderer/src/components/chat/nodes/commandForm.ts`:
- Around line 33-38: Remove the unused node schema attributes pendingCommand and
pendingPrompt from the CommandForm definition, since CommandFormView.vue and the
rest of the chat flow rely on pendingFormData from useChatInputMentions.ts
instead. Update the CommandForm schema so only actively consumed fields remain,
and make sure any references in the CommandForm-related node setup or consumers
are cleaned up if present.
In `@test/main/presenter/skillPresenter/skillTools.test.ts`:
- Around line 155-163: The new allowedPluginIds authorization path in
handleSkillList and handleSkillView is not covered, so add tests that exercise
plugin-based filtering. Update the mock skill metadata to include ownerPluginId
on at least one skill, then add a handleSkillList case that passes an allowlist
and verifies skills with a non-allowed ownerPluginId are excluded. Also add a
handleSkillView case that passes allowedPluginIds and confirms a skill owned by
a blocked plugin is rejected, using the existing skillTools and
mockSkillPresenter setup.
In `@test/renderer/components/ChatInputBox.test.ts`:
- Around line 27-79: The MockEditor in ChatInputBox tests leaves
state.doc.descendants as a bare vi.fn(), so syncSkillNodes and syncFileNodes
can’t exercise existing-node deletion paths. Update the MockEditor mock to give
descendants a usable implementation (for example, returning a configurable node
list or invoking the callback for supplied nodes) so tests can cover both
insertion and removal behavior. Keep the change localized to MockEditor and any
tests that need to simulate fileAttachment or skill nodes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9de2567a-184f-4445-8681-59c20f822a4b
📒 Files selected for processing (20)
src/main/presenter/agentRuntimePresenter/dispatch.tssrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/agentRuntimePresenter/types.tssrc/main/presenter/skillPresenter/skillTools.tssrc/main/presenter/toolPresenter/agentTools/agentToolManager.tssrc/main/presenter/toolPresenter/index.tssrc/renderer/src/components/chat/ChatInputBox.vuesrc/renderer/src/components/chat/composables/useChatInputMentions.tssrc/renderer/src/components/chat/mentions/CommandInputDialog.vuesrc/renderer/src/components/chat/nodes/CommandFormView.vuesrc/renderer/src/components/chat/nodes/FileAttachmentView.vuesrc/renderer/src/components/chat/nodes/SkillChipView.vuesrc/renderer/src/components/chat/nodes/commandForm.tssrc/renderer/src/components/chat/nodes/fileAttachment.tssrc/renderer/src/components/chat/nodes/skillChip.tssrc/renderer/src/components/chat/nodes/symbols.tssrc/shared/types/presenters/tool.presenter.d.tstest/main/presenter/skillPresenter/skillTools.test.tstest/main/presenter/toolPresenter/agentTools/agentToolManagerSettings.test.tstest/renderer/components/ChatInputBox.test.ts
💤 Files with no reviewable changes (1)
- src/renderer/src/components/chat/mentions/CommandInputDialog.vue
zhangmo8
changed the title
Review: fix(chat): inline skill input context已切到本地分支
整体方向认可：后端把 以下是在代码里发现的问题，按优先级排序： 1. 死代码：
|
zerob13
left a comment
zerob13
left a comment
There was a problem hiding this comment.
Code Review Summary
这个 PR 实现了将 skill chips、file attachments 和 slash command 参数表单作为内联 TipTap 节点渲染到聊天编辑器中的功能。整体架构合理，但有几个需要关注的问题：
✅ 优点
- 架构设计清晰：使用 TipTap 自定义节点 + Vue NodeView 的方式实现内联编辑，符合 WYSIWYG 编辑器最佳实践
- 双向同步机制：通过
syncSkillNodes和syncFileNodes实现 editor ↔ state 的双向同步，避免状态不一致 - 类型安全：
UserMessageInlineItem类型定义完整，涵盖 skill 和 file 两种内联项 - 测试覆盖：为
ChatInputBox和MessageItemUser新增了内联节点相关的测试用例
⚠️ 主要问题
1. 双向同步的竞态条件风险 ⚙️
function reconcileEditorNodes() {
if (isSyncingNodes) return // ✅ 有保护
// 问题：如果用户快速删除多个 chip，可能导致状态不一致
const editorSkillNames = new Set(getEditorSkillNames())
activeSkillNames.value
.filter((name) => !editorSkillNames.has(name))
.forEach((name) => {
void skillsData.deactivateSkill(name) // ⚠️ async 操作可能延迟
})
}建议：考虑使用 debounce 或批量更新机制，避免频繁触发状态变更。
2. 命令表单提交的时序依赖 🔄
function handleSubmit() {
if (!canSubmit.value) return
actions?.prepareCommandFormSubmit() // 设置标志
props.deleteNode() // 删除节点
if (actions?.submitCommandForm) {
actions.submitCommandForm({ ...formValues }) // 提交表单
}
}问题：isSubmittingCommandForm 标志依赖于严格的调用顺序，如果 deleteNode() 触发了异步更新，可能导致 reconcileEditorNodes 误判。
建议：使用 Promise 链或 nextTick 确保时序正确。
3. 内联项序列化缺少位置验证 📍
const inlineContentBlocks = computed(() => {
const items = message.content.inlineItems || []
// ⚠️ 没有验证 offset 是否在有效范围内
const sorted = [...items].sort((a, b) => a.offset - b.offset)
// ...
})风险：如果 offset 越界或重叠，可能导致渲染错误。
建议：添加边界检查和重叠检测逻辑。
4. skill runtime 作用域变更影响面较大 🎯
const activeSkills = (Array.isArray(activeSkillNames) ? activeSkillNames : pinnedSkills).filter(
(skillName) => listedSkillNames.has(skillName)
)变更：skill_list 和 skill_view 现在使用 enabledSkillNames 作为允许列表，而不是当前激活的 skills。
问题：这个语义变化可能影响现有工作流，特别是依赖于「只看到当前激活 skills」的场景。
建议：
- 在 PR 描述中明确说明这是 breaking change
- 考虑添加兼容层或配置开关
- 确保所有相关文档和测试都更新
🐛 潜在 Bug
5. CommandForm 节点在某些情况下可能不被清理
if (!hasCommandFormNode() && !isSubmittingCommandForm) {
mentions.closeDialog()
}场景：如果用户在表单提交前手动关闭编辑器或切换会话，isSubmittingCommandForm 可能卡在 true。
建议：在 onUnmounted 中重置标志。
6. 文件删除逻辑使用倒序遍历但可能影响索引
for (let i = files.selectedFiles.value.length - 1; i >= 0; i -= 1) {
const file = files.selectedFiles.value[i]
if (!editorFilePaths.has(file.path || file.name)) {
files.deleteFile(i) // ⚠️ deleteFile 内部实现需要确保索引安全
}
}建议：确认 deleteFile 不会修改数组长度导致索引错位。
📝 代码质量建议
- Magic Number：
max-w-[120px]和max-w-[160px]应该提取为常量或 Tailwind 配置 - Error Handling：
JSON.parse(props.node.attrs.fields)在 catch 块中应该记录错误日志 - 性能优化：
getEditorSkillNames和getEditorFilePaths频繁调用，考虑缓存结果 - i18n 遗漏：
CommandFormView.vue中的一些文本可能需要国际化
🧪 测试建议
由于测试超时，建议手动验证以下场景：
- 快速连续删除多个 skill chips
- 在命令表单未提交时切换会话
- 粘贴包含内联项的长文本
- 文件和 skill 混合使用时的渲染顺序
- 编辑器失焦后重新聚焦的状态恢复
✨ 总体评价
这是一个高质量的 PR，实现了复杂的编辑器内联功能。主要风险在于双向同步的边界情况和 skill runtime 作用域的语义变更。建议：
- 补充 边界条件测试
- 在 CHANGELOG 中标注 breaking change
- 考虑添加 性能监控（大量内联项时的渲染性能）
- 更新相关 用户文档
代码逻辑清晰，架构合理，通过后续迭代解决上述问题即可合并。 💪
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/agentRuntimePresenter/index.ts (1)
3451-3451: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftPolicy resolution should fail closed.
resolveAgentExtensionPolicyreturns{}on config-resolution errors, and the new stream/deferred-tool consumers treat missingenabledSkillNames/enabledPluginIdsas unrestricted access. A transient config failure therefore reopens skill/plugin access instead of denying it; make that fallback explicit or surface the error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/agentRuntimePresenter/index.ts` at line 3451, Policy resolution is currently failing open because resolveAgentExtensionPolicy can return an empty object on config errors, and the stream/deferred-tool paths then interpret missing enabledSkillNames and enabledPluginIds as unrestricted access. Update agentRuntimePresenter’s policy flow so the fallback is fail-closed: either surface the error from resolveAgentExtensionPolicy or return an explicit denying policy that blocks all skills/plugins, and make sure the stream/deferred-tool consumers use that explicit result instead of treating absent fields as allow-all.
🧹 Nitpick comments (3)
src/shared/chat.d.ts (1)
64-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffDeduplicate the shared chat message types
UserMessageInlineItemis declared verbatim insrc/shared/chat.d.ts,src/shared/types/core/chat.ts, andsrc/shared/types/agent-interface.d.ts.UserMessageContentis also split across those shared files, so the shapes can drift over time. Consider centralizing the shared pieces in one canonical type and re-exporting them from the others.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/chat.d.ts` around lines 64 - 77, `UserMessageInlineItem` and `UserMessageContent` are duplicated across the shared chat type definitions, so centralize these shapes in one canonical shared type and re-export them from `chat.d.ts`, `types/core/chat.ts`, and `types/agent-interface.d.ts`. Update the shared type declarations to reference the single source of truth rather than repeating the union definitions, and keep the existing symbols (`UserMessageInlineItem`, `UserMessageContent`) available through re-exports for callers.src/renderer/src/components/message/MessageContent.vue (1)
26-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
titletooltips to skill/file chips for truncated labels.Skill/file chip labels are truncated (
max-w-[160px]/max-w-[120px]) but, unlike the mention chip above which sets:title, there's no way to see the full name/path on hover.💡 Proposed fix
<span v-else-if="block.type === 'skill'" class="inline-flex items-center gap-1 rounded-md border border-primary/30 bg-primary/10 px-1.5 py-0.5 text-xs text-primary align-middle" data-testid="user-message-inline-skill" + :title="block.skillName" > <Icon icon="lucide:sparkles" class="h-3 w-3 shrink-0" /> <span class="truncate max-w-[160px]">{{ block.skillName }}</span> </span> <!-- File chip --> <span v-else-if="block.type === 'file'" class="inline-flex items-center gap-1 rounded-md border border-muted-foreground/25 bg-muted/25 px-1.5 py-0.5 text-xs text-muted-foreground align-middle" data-testid="user-message-inline-file" + :title="block.filePath || block.fileName" > <Icon :icon="getFileIcon(block)" class="h-3 w-3 shrink-0" /> <span class="truncate max-w-[120px]">{{ block.fileName }}</span> </span>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/message/MessageContent.vue` around lines 26 - 44, Add hover tooltips to the truncated skill and file chips in MessageContent so the full value is visible on hover. Update the inline chip spans in the skill/file block branches to set a title using block.skillName and block.fileName, mirroring the mention chip’s existing title behavior.src/renderer/src/components/message/MessageItemUser.vue (1)
161-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate "base text" derivation.
getVisibleMessageText'sbaseText(161-166) and the newinlineBaseTextcomputed (210-217) both re-implement the identical "blocks-or-text" fallback logic. Extracting a single shared helper (e.g.resolveBaseText(message)) would avoid the two implementations silently diverging later.Also applies to: 209-217
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/message/MessageItemUser.vue` around lines 161 - 171, The visible message text logic is duplicating the same blocks-or-text fallback in both getVisibleMessageText and the inlineBaseText path, so extract a shared helper such as resolveBaseText(message) and reuse it in MessageItemUser.vue. Update getVisibleMessageText and the inline text composition to call that helper instead of re-implementing the blocks/content.text fallback, keeping the logic in one place to prevent divergence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/src/components/message/MessageItemUser.vue`:
- Around line 163-170: The copy output is missing inline skill/file chip names
because `getCopyText` only uses `content` blocks or `content.text`, unlike the
new inline rendering path in `MessageItemUser.vue`. Update `getCopyText` to
mirror the `inlineText` composition used here by reading
`message.content.inlineItems` and appending each `skillName` or `fileName`
alongside the existing visible block/text content, so copied text preserves
those references.
---
Outside diff comments:
In `@src/main/presenter/agentRuntimePresenter/index.ts`:
- Line 3451: Policy resolution is currently failing open because
resolveAgentExtensionPolicy can return an empty object on config errors, and the
stream/deferred-tool paths then interpret missing enabledSkillNames and
enabledPluginIds as unrestricted access. Update agentRuntimePresenter’s policy
flow so the fallback is fail-closed: either surface the error from
resolveAgentExtensionPolicy or return an explicit denying policy that blocks all
skills/plugins, and make sure the stream/deferred-tool consumers use that
explicit result instead of treating absent fields as allow-all.
---
Nitpick comments:
In `@src/renderer/src/components/message/MessageContent.vue`:
- Around line 26-44: Add hover tooltips to the truncated skill and file chips in
MessageContent so the full value is visible on hover. Update the inline chip
spans in the skill/file block branches to set a title using block.skillName and
block.fileName, mirroring the mention chip’s existing title behavior.
In `@src/renderer/src/components/message/MessageItemUser.vue`:
- Around line 161-171: The visible message text logic is duplicating the same
blocks-or-text fallback in both getVisibleMessageText and the inlineBaseText
path, so extract a shared helper such as resolveBaseText(message) and reuse it
in MessageItemUser.vue. Update getVisibleMessageText and the inline text
composition to call that helper instead of re-implementing the
blocks/content.text fallback, keeping the logic in one place to prevent
divergence.
In `@src/shared/chat.d.ts`:
- Around line 64-77: `UserMessageInlineItem` and `UserMessageContent` are
duplicated across the shared chat type definitions, so centralize these shapes
in one canonical shared type and re-export them from `chat.d.ts`,
`types/core/chat.ts`, and `types/agent-interface.d.ts`. Update the shared type
declarations to reference the single source of truth rather than repeating the
union definitions, and keep the existing symbols (`UserMessageInlineItem`,
`UserMessageContent`) available through re-exports for callers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d7bda1e-3147-47f8-8bcf-9bd8d9d7e451
📒 Files selected for processing (20)
src/main/presenter/agentRuntimePresenter/contextBuilder.tssrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/agentRuntimePresenter/messageStore.tssrc/main/presenter/agentRuntimePresenter/pendingInputCoordinator.tssrc/main/presenter/agentRuntimePresenter/pendingInputStore.tssrc/main/presenter/agentSessionPresenter/index.tssrc/renderer/src/components/chat/ChatInputBox.vuesrc/renderer/src/components/chat/messageListItems.tssrc/renderer/src/components/message/MessageContent.vuesrc/renderer/src/components/message/MessageItemUser.vuesrc/renderer/src/pages/ChatPage.vuesrc/renderer/src/pages/NewThreadPage.vuesrc/renderer/src/stores/ui/message.tssrc/shared/chat.d.tssrc/shared/contracts/common.tssrc/shared/contracts/routes/sessions.routes.tssrc/shared/types/agent-interface.d.tssrc/shared/types/core/chat.tstest/renderer/components/ChatInputBox.test.tstest/renderer/components/message/MessageItemUser.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/src/components/chat/ChatInputBox.vue
2 participants
Summary
commandFormnode to avoid focus loss.skill_list/skill_viewuse agent enabled skill/plugin policy instead of incorrectly treating current active skills as the allowlist.UI layout
BEFORE:
AFTER:
Notes
skill_viewcan now inspect available enabled skills even before they are active for the current message.skill_viewstill activates a skill only for the current message/tool loop; it does not pin the skill to the conversation.Verification
git diff --checkSummary by CodeRabbit
inlineItemsthrough to persistence and rendering.enabledSkillNamesandenabledPluginIds, including explicit “no value” handling.allowedPluginIdssupport forskill_listandskill_view.