feat: improve renderer interaction quality - #1976
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds batched MCP installation-status lookup, incremental marketplace loading with retry and concurrency states, settings route prefetch and pending feedback, accessible message disclosures and toolbar actions, layout and performance updates, tests, and architecture documentation. ChangesRenderer interaction performance
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant McpBuiltinMarket
participant McpClient
participant dispatchDeepchatRoute
participant McpPresenter
McpBuiltinMarket->>McpClient: listInstalledServerIds(source, sourceIds)
McpClient->>dispatchDeepchatRoute: invoke MCP route
dispatchDeepchatRoute->>McpPresenter: resolve installed IDs
McpPresenter-->>dispatchDeepchatRoute: installedSourceIds
dispatchDeepchatRoute-->>McpBuiltinMarket: merge installed status
Possibly related PRs
🚥 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/renderer/src/components/message/MessageToolbar.vue (2)
117-118: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeyboard activation only reaches the "copy" action, not "copy from top."
handleCopyImageStart/handleCopyImageEnddistinguish a short tap (copyImage) from a long press (copyImageFromTop), but the newkeydown.enter/keydown.spacebinding always callshandleCopyImageKeyboard, which only emitscopyImage. Keyboard-only users have no way to trigger the "copy from top" variant that mouse users get via long-press.Consider adding a keyboard-accessible path to the long-press variant (e.g. a modifier like
Shift+Enter), or documenting that it is mouse-only if intentional.Also applies to: 299-317
🤖 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/MessageToolbar.vue` around lines 117 - 118, Extend the keyboard activation handling in MessageToolbar so keyboard users can trigger both copyImage and copyImageFromTop, matching the existing handleCopyImageStart/handleCopyImageEnd behavior. Add a clearly defined keyboard gesture such as Shift+Enter (and an equivalent space-key gesture if supported) that routes to the long-press variant, while preserving the current Enter/Space short-copy behavior.
263-274: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate tip-timer logic across four call sites.
handleCopy, the long-press branch ofhandleCopyImageStart,handleCopyImageEnd, and the newhandleCopyImageKeyboardall repeat the same "show tip → clear stale timer → auto-hide after 2000ms → null out timer" block.handleCopyImageEnd(Lines 299-304) andhandleCopyImageKeyboard(Lines 310-315) are now identical in body. Extracting a small helper would remove the duplication and guarantee the four tips stay behaviorally consistent.♻️ Example helper extraction
+const flashTip = ( + tipRef: import('vue').Ref<boolean>, + getTimer: () => number | null, + setTimer: (id: number | null) => void +) => { + tipRef.value = true + const existing = getTimer() + if (existing !== null) window.clearTimeout(existing) + setTimer( + window.setTimeout(() => { + tipRef.value = false + setTimer(null) + }, 2000) + ) +} + const handleCopyImageKeyboard = (event: KeyboardEvent) => { if (event.repeat || props.isCapturingImage) return emit('copyImage') - showCopyImageTip.value = true - if (copyImageTipTimer !== null) window.clearTimeout(copyImageTipTimer) - copyImageTipTimer = window.setTimeout(() => { - showCopyImageTip.value = false - copyImageTipTimer = null - }, 2000) + flashTip(showCopyImageTip, () => copyImageTipTimer, (id) => (copyImageTipTimer = id)) }Also applies to: 283-286, 299-317
🤖 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/MessageToolbar.vue` around lines 263 - 274, Extract the repeated tip-display and timer-reset logic from handleCopy, handleCopyImageStart, handleCopyImageEnd, and handleCopyImageKeyboard into a shared helper. Have the helper show the relevant tip, clear any existing timer, schedule the 2000ms auto-hide, and null the timer afterward; update all four call sites to use it while preserving their existing tip state and timer variables.
🤖 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/MessageToolbar.vue`:
- Around line 375-389: The scoped .relative rule in MessageToolbar is currently
unused. Add the relative class to the button containing the absolutely
positioned tip spans so they anchor correctly, or remove the rule if no
anchoring is needed; prefer updating that button to preserve the intended
tooltip positioning.
---
Nitpick comments:
In `@src/renderer/src/components/message/MessageToolbar.vue`:
- Around line 117-118: Extend the keyboard activation handling in MessageToolbar
so keyboard users can trigger both copyImage and copyImageFromTop, matching the
existing handleCopyImageStart/handleCopyImageEnd behavior. Add a clearly defined
keyboard gesture such as Shift+Enter (and an equivalent space-key gesture if
supported) that routes to the long-press variant, while preserving the current
Enter/Space short-copy behavior.
- Around line 263-274: Extract the repeated tip-display and timer-reset logic
from handleCopy, handleCopyImageStart, handleCopyImageEnd, and
handleCopyImageKeyboard into a shared helper. Have the helper show the relevant
tip, clear any existing timer, schedule the 2000ms auto-hide, and null the timer
afterward; update all four call sites to use it while preserving their existing
tip state and timer variables.
🪄 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: 5327b07f-8866-4b64-8804-6d69f0269b5b
📒 Files selected for processing (31)
docs/architecture/renderer-interaction-performance/plan.mddocs/architecture/renderer-interaction-performance/spec.mddocs/architecture/renderer-interaction-performance/tasks.mdsrc/main/presenter/mcpPresenter/index.tssrc/main/routes/index.tssrc/renderer/api/McpClient.tssrc/renderer/settings/App.vuesrc/renderer/settings/components/McpBuiltinMarket.vuesrc/renderer/settings/main.tssrc/renderer/settings/settingsRouteComponents.tssrc/renderer/src/components/message/MessageBlockError.vuesrc/renderer/src/components/message/MessageBlockToolCall.vuesrc/renderer/src/components/message/MessageToolbar.vuesrc/renderer/src/components/sidepanel/ChatSidePanel.vuesrc/renderer/src/components/sidepanel/WorkspacePanel.vuesrc/renderer/src/views/ChatTabView.vuesrc/shared/contracts/routes.tssrc/shared/contracts/routes/mcp.routes.tssrc/shared/types/presenters/core.presenter.d.tstest/main/presenter/mcpPresenter.test.tstest/main/routes/contracts.test.tstest/main/routes/dispatcher.test.tstest/renderer/api/clients.test.tstest/renderer/components/ChatSidePanel.test.tstest/renderer/components/McpBuiltinMarket.test.tstest/renderer/components/SettingsApp.test.tstest/renderer/components/message/MessageBlockBasics.test.tstest/renderer/components/message/MessageBlockToolCall.test.tstest/renderer/components/message/MessageToolbar.trace.test.tstest/renderer/composables/useMessageWindow.test.tstest/renderer/performance/chatRendering.perf.test.ts
💤 Files with no reviewable changes (2)
- src/renderer/src/components/sidepanel/ChatSidePanel.vue
- src/renderer/src/components/sidepanel/WorkspacePanel.vue
Collaborator
Author
|
Addressed the CodeRabbit follow-ups in
Validation passed: component tests ( The generic docstring-coverage warning was not applied: these are private, self-explanatory |
…ion-quality # Conflicts: # test/renderer/composables/useMessageWindow.test.ts
This was referenced Jul 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
BEFORE
AFTER
Validation
pnpm run formatpnpm run i18npnpm run lintpnpm run typecheckRisks
Summary by CodeRabbit
aria-expanded/aria-controls) with improved expand/collapse behavior.