refactor(dc-ui): extract DcCopyButton and wire copy-text - #2131
Conversation
- 将 DcCopyButton 从 components/copy-button 并入 components/button（props 抽到 props.ts，dc-ui 统一走 components barrel） - MessageToolbar 新增 copy-text prop；用户/助手消息复制均由父级传入复制内容，icon 成功态恢复正常 - 消费方（ArtifactBlock / CodeArtifact / McpJsonViewer / McpServers / TraceDialog / MessageBlockToolCall）统一经 copy-text 传内容 - DcToggleRow 行内开关补齐 w-full 布局 - 新增 composer 草稿持久化（useComposerSubmit + composerDraftPersistence）及测试，docs/design-system.md 同步登记
📝 WalkthroughWalkthroughThe UI package adds a shared ChangesCopy controls
Composer draft persistence
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MessageItemAssistant
participant MessageToolbar
participant DcCopyButton
participant useClipboard
MessageItemAssistant->>MessageToolbar: pass computed copyText
MessageToolbar->>DcCopyButton: render copy control
DcCopyButton->>useClipboard: copy copyText
DcCopyButton-->>MessageToolbar: emit copied or error
sequenceDiagram
participant ChatComposer
participant useComposerSubmit
participant composerDraftPersistence
participant localStorage
ChatComposer->>useComposerSubmit: mutate draft
useComposerSubmit->>composerDraftPersistence: debounce persistence
composerDraftPersistence->>localStorage: save session draft
useComposerSubmit->>composerDraftPersistence: load on session switch
composerDraftPersistence->>localStorage: read session draft
composerDraftPersistence-->>useComposerSubmit: return validated draft
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/dc-ui/components/button/DcCopyButton.vue`:
- Around line 32-45: Add a focused Vitest regression test in test/renderer for
DcCopyButton.copyText covering observable behavior: pass copyText to the
clipboard helper, emit copied, set the success state, reset it after 1200 ms,
and emit error when copying rejects. Keep the test minimal and avoid testing
implementation details.
- Around line 8-16: The DcCopyButton copyText contract is inconsistent between
its TypeScript interface, runtime props, and documentation. In
src/dc-ui/components/button/DcCopyButton.vue lines 8-16, make copyText required
in both DcCopyButtonProps and the runtime props definition, preserving the
existing click behavior; alternatively, implement an explicit disabled state for
empty copyText. In docs/design-system.md line 621, update the documentation only
if optional copy-text is intentionally retained.
- Line 72: Update DcCopyButton’s onClick handling so copyText does not overwrite
the inherited attrs.onClick listener; preserve and invoke the caller’s click
handlers and modifiers while retaining the copy behavior, preventing parent
click propagation from being lost.
In `@src/dc-ui/components/button/props.ts`:
- Line 5: Run Oxfmt on the module containing the DcButtonProps interface and
remove the leading indentation before the top-level export, leaving the
surrounding code unchanged.
In `@src/renderer/src/components/message/MessageToolbar.vue`:
- Around line 68-75: The MessageToolbar copy flow no longer emits the declared
copy event, breaking listeners such as MessageItemUser.vue. Update the
DcCopyButton integration in MessageToolbar to forward its completion through the
existing copy event declared by the component, preserving the current event
contract and modifiers; do not leave the declaration without an emission.
In `@src/renderer/src/features/chat-page/model/composerDraftPersistence.ts`:
- Around line 17-19: Update hasStorage and the storage flows
loadComposerDraftFromStorage, saveComposerDraftToStorage, and
clearComposerDraftFromStorage so accessing window.localStorage is protected by
the existing try/catch handling. Ensure getter failures follow the same
best-effort fallback as other storage errors instead of escaping before storage
operations begin.
- Around line 30-45: Strengthen validation in loadComposerDraftFromStorage() so
every parsed files entry has the expected Composer file shape, activeSkills
contains only valid string activity values, and document has its required nested
structure and field types before casting or restoring the draft. Ensure
malformed shapes such as files: [null] return null without invoking
copyComposerFiles(), and add focused regression coverage for these documented
invalid drafts.
In `@test/renderer/features/chat-page/composables/useComposerSubmit.test.ts`:
- Around line 1019-1106: Update the createHarness test helper’s stop teardown to
call actions.dispose() before scope.stop(). Ensure every harness is disposed so
useComposerSubmit’s pagehide/beforeunload listeners and pending persistence work
are cleared between tests.
🪄 Autofix
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 Plus
Run ID: 0f3b432b-2804-4006-b341-d3a73302012f
📒 Files selected for processing (24)
docs/design-system.mdsrc/dc-ui/components/button/DcButton.vuesrc/dc-ui/components/button/DcCopyButton.vuesrc/dc-ui/components/button/index.tssrc/dc-ui/components/button/props.tssrc/dc-ui/components/copy-button/DcCopyButton.vuesrc/dc-ui/components/copy-button/index.tssrc/dc-ui/components/index.tssrc/dc-ui/components/toggle-row/DcToggleRow.vuesrc/dc-ui/index.tssrc/renderer/src/components/artifacts/ArtifactBlock.vuesrc/renderer/src/components/artifacts/CodeArtifact.vuesrc/renderer/src/components/mcp-config/components/McpJsonViewer.vuesrc/renderer/src/components/mcp-config/components/McpServers.vuesrc/renderer/src/components/message/MessageBlockToolCall.vuesrc/renderer/src/components/message/MessageItemAssistant.vuesrc/renderer/src/components/message/MessageItemUser.vuesrc/renderer/src/components/message/MessageToolbar.vuesrc/renderer/src/components/trace/TraceDialog.vuesrc/renderer/src/features/chat-page/composables/useComposerSubmit.tssrc/renderer/src/features/chat-page/model/composerDraftPersistence.tstest/renderer/components/ChatPage.test.tstest/renderer/features/chat-page/composables/useComposerSubmit.test.tstest/renderer/features/chat-page/model/composerDraftPersistence.test.ts
💤 Files with no reviewable changes (2)
- src/dc-ui/components/copy-button/index.ts
- src/dc-ui/components/copy-button/DcCopyButton.vue
zerob13
left a comment
zerob13
left a comment
There was a problem hiding this comment.
Conclusion: this is not the smallest correct implementation, and I would not merge it as-is.
The copy defect is narrow: relocate the existing component mechanically, pass copyText from MessageToolbar, preserve the public contract, and add focused regression coverage. Instead, this PR also adds an unrelated composer-persistence feature and a toggle-row layout change, creates a broad @dc-ui/components barrel, extracts button props solely to support a render-function proxy, and rewrites DcCopyButton while dropping showToast / successDuration and changing clipboard error semantics. That is unnecessary scope and abstraction for the stated PR.
Recommended split:
- Copy-button PR: keep the template implementation and existing API, move/re-export it from
components/button, update direct imports, wirecopy-text, and test the observable copy state. - Composer-draft PR: implement and validate persistence independently, including the real keyed
ChatPagemount path, bounded attachment storage, session deletion/cleanup, and corrupted/quota cases. - Keep the
DcToggleRowlayout fix separate or justify it in the PR scope.
The new broad component barrel is not needed for this move; @dc-ui/components/button already provides the correct ownership boundary and avoids making every consumer depend on a package-wide export surface.
UI regressions introduced by the rewrite:
Expected focus: [Copy*] --Enter--> [Check*]
Current focus: [Copy*] --Enter--> [Check] -> focus moves to <body>
Expected toolbar: [12 px icon inside 16 px control]
Current toolbar: [16 px icon fills 16 px control]
The seven existing unresolved CodeRabbit threads are still actionable; I did not duplicate them below. The inline comments in this review are additional findings.
Validation performed at this head: 143 relevant Vitest tests passed; format, lint, web typecheck, and i18n passed; GitHub PR Check and Package Check are green. The local runtime was Node 24.15.0 while the repository requires >=24.18.0, so local commands emitted an engine warning. The green tests do not cover the actual keyed remount restore path described below.
| const storedIncomingDraft = | ||
| sessionDrafts.get(sessionId) ?? createEmptyComposerDraft(draftRevisions.get(sessionId) ?? 0) | ||
| sessionDrafts.get(sessionId) ?? | ||
| loadComposerDraftFromStorage(sessionId) ?? |
There was a problem hiding this comment.
[P1] Restore persisted drafts on the real mount path
loadComposerDraftFromStorage() is only reached through switchComposerSession(). In ChatPage.vue, however, the immediate session watcher runs before useComposerSubmit() is constructed, so it calls the initial no-op switchComposerSessionDraft; the real function is assigned only afterward. ChatTabView.vue also keys the route shell by session ID, so every session switch creates a fresh ChatPage and that watcher never runs again on the instance. A saved draft is therefore not restored when the user reopens the session, and the next dispose() can flush the empty live state and remove the stored key. Initialize the draft from storage during composable construction (or explicitly invoke the assigned function once after construction) and test the actual ChatPage remount without manually calling switchComposerSession().
| window.localStorage.removeItem(storageKey(sessionId)) | ||
| return | ||
| } | ||
| window.localStorage.setItem(storageKey(sessionId), JSON.stringify(draft)) |
There was a problem hiding this comment.
[P1] Do not store raw attachment payloads in localStorage
ComposerSessionDraft.files contains full MessageFile objects. Image uploads populate both content with the complete base64 image and thumbnail with another base64 payload; prompt/file attachments may also contain their full text. Serializing that plus the TipTap document into synchronous localStorage can exceed the origin quota or stall the renderer. This catch then silently drops the entire write, including the user's text draft. Persist a bounded representation (for example text/document plus validated file descriptors) or use application storage designed for large records, and add an attachment/quota regression test.
| ), | ||
| onClick: copyText | ||
| }, | ||
| slots |
There was a problem hiding this comment.
[P2] Preserve the visible label fallback
The previous template rendered <slot>{{ label }}</slot>. Passing the raw slots object here removes that fallback. CodeArtifact, McpJsonViewer, McpServers, and TraceDialog pass label without a default slot, so their copy controls change from icon + visible text to icon-only. Preserve label as the default-slot fallback (or retain the template implementation) and cover one text-button consumer.
| { | ||
| ...inheritedAttrs, | ||
| ...buttonProps, | ||
| key: icon.value, |
There was a problem hiding this comment.
[P2] Keep keyboard focus while changing the success icon
Using icon.value as the component key makes Vue unmount and recreate the entire DcButton whenever copied changes. A keyboard-activated copy therefore replaces the focused DOM node and moves focus to <body>; the next keyboard action and tooltip focus state are lost. The button and its icon already react to the icon prop, so this component-level key is unnecessary. Remove it and add a focus-retention assertion.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/renderer/components/DcCopyButton.test.ts`:
- Around line 87-89: Reformat the template value in the test setup around the
DcCopyButton component so the source stays within 100 columns while preserving
the exact rendered markup and behavior. Follow the project’s Oxfmt style,
including single quotes and no semicolons.
🪄 Autofix
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 Plus
Run ID: 0565cbdb-88bd-45e5-a7e7-6ebc2a3ab5b7
📒 Files selected for processing (7)
src/dc-ui/components/button/DcCopyButton.vuesrc/renderer/src/components/message/MessageToolbar.vuesrc/renderer/src/features/chat-page/model/composerDraftPersistence.tstest/renderer/components/DcCopyButton.test.tstest/renderer/components/message/MessageToolbar.trace.test.tstest/renderer/features/chat-page/composables/useComposerSubmit.test.tstest/renderer/features/chat-page/model/composerDraftPersistence.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/renderer/features/chat-page/composables/useComposerSubmit.test.ts
- test/renderer/components/message/MessageToolbar.trace.test.ts
- src/renderer/src/features/chat-page/model/composerDraftPersistence.ts
- src/renderer/src/components/message/MessageToolbar.vue
yyhhyyyyyy
left a comment
yyhhyyyyyy
left a comment
There was a problem hiding this comment.
I reviewed the latest head (557b875). The latest commit fixes several earlier issues, including the required copyText contract, inherited click handlers/modifiers, the MessageToolbar copy event, storage getter failures, nested payload validation, and test teardown.
However, I still do not think this PR is safe to merge as-is. I found no P0 issues, but there are three P1 blockers and several P2 regressions.
P1 blockers:
- Persisted drafts are not restored on the real keyed
ChatPagemount path.
ChatPage.vue runs its immediate session watcher before useComposerSubmit() is constructed, so the watcher calls the initial no-op switchComposerSessionDraft. The real switchComposerSession function is assigned only afterward. Because ChatTabView.vue keys the route shell by session ID, every session switch creates a fresh ChatPage; the watcher does not run again on that instance.
As a result, a stored draft is not restored when reopening a session. A later dispose() can then flush the empty live composer and remove the previously stored draft. The current tests manually call switchComposerSession() and therefore do not cover the actual mount path. Please seed storage and test a real ChatPage mount without manually invoking the composable method.
- Full attachment payloads are synchronously serialized into
localStorage.
ComposerSessionDraft.files contains complete MessageFile objects. Image attachments contain the full base64 source in content and another base64 payload in thumbnail; file attachments may also contain their full text. Every draft save performs JSON.stringify(draft) and synchronous localStorage.setItem() on the renderer thread.
A normal screenshot or a few attachments can exceed the origin quota, and every later edit will retry the same large synchronous serialization. The exception is swallowed, so the text draft can silently fail to persist while the renderer may repeatedly stall. Please persist a bounded representation, or move full attachment persistence behind an asynchronous preload/IPC-backed storage service. This also needs oversized-attachment/quota coverage.
- Session deletion does not remove persisted draft data.
clearComposerDraftFromStorage() currently has no production caller. Deleting a session removes the database session and messages, but leaves deepchat.composerDraft.v1.<sessionId> behind indefinitely, including pasted image/file contents. Cleanup needs to be part of the central session-removal lifecycle and must also prevent the active ChatPage unmount from writing the deleted draft back afterward.
Additional P2 regressions:
disposeComposerSubmit()runs from the parentonUnmountedhook, after the child composer ref has been cleared. A rapid keyed unmount before the debounce fires can therefore persist raw text while losing the latest TipTap document and active skills. Flush before child teardown or retain the latest complete snapshot in the composable.- The old
<slot>{{ label }}</slot>fallback was removed.CodeArtifact,McpJsonViewer,McpServers,TraceDialog, and tool-call controls passlabelwithout a default slot, so text buttons become icon-only. When no label/tooltip is provided, using the fullcopyTextasaria-labelcan also make a screen reader announce the complete artifact source. key: icon.valueis applied to the entireDcButton. The copy/check transition therefore replaces the focused button and drops keyboard focus. The inner icon key is sufficient; the button itself should remain stable.- Copying now has two owners.
DcCopyButtonfirst uses the web Clipboard API, then@copiedcauses the message item to copy again through the Electron preload bridge. On unsupported web Clipboard APIs, VueUse may resolve without copying and show a false success state; on permission rejection,copiedis not emitted, so the reliable preload path is skipped. Please use one clipboard adapter exactly once and enter the success state only after that adapter succeeds. - Most
icon-size="3"props were removed fromMessageToolbar, so icons grow from 12 px to theDcButtondefault of 16 px inside 16 px controls. The edit action remains 12 px, making the toolbar inconsistent. Please restore the existing density contract.
The copy-button fix is valid and necessary, but the composer-persistence feature is substantial and unrelated. I recommend splitting this into a focused copy-button PR and a separate composer-persistence PR with the recovery, storage-boundary, deletion, quota, and keyed-remount cases covered before merge.
* fix(dc-ui): restore copy label and keep focus Render the label attr as the default-slot fallback again so text consumers (CodeArtifact, McpJsonViewer, McpServers, TraceDialog) keep their visible caption, and drop the component-level icon key so the success icon swap no longer recreates the button and steals keyboard focus. * style(message): pin toolbar icons to 12px Apply icon-size="3" to every toolbar DcButton so the 12px icon contract from the pre-refactor toolbar is preserved. * fix(chat): restore and trim composer drafts Load the persisted draft while useComposerSubmit is constructed: the ChatPage session watcher fires before the composable exists and each session switch remounts ChatPage, so switchComposerSession alone never restored a stored draft and dispose could flush empty state over it. Strip base64 content, thumbnails and resolved representations from persisted files so localStorage holds bounded descriptors instead of raw attachment payloads.
3 participants
概述
components/copy-button并入components/button（旧目录删除），props.ts承载 DcButtonProps，dc-ui 顶层统一走componentsbarrel 导出。copied状态，但此前copy-text未传入，复制流程根本没启动，icon 永远停在 copy。现在所有消费方均经copy-text传入复制内容，成功态（✅ + emerald，1200ms 回落）恢复生效。copy-textprop；用户/助手消息复制内容由父级（MessageItemUser / MessageItemAssistant）计算后传入（原handleCopy逻辑抽成copyTextcomputed，行为不变）。w-full，保证开关靠右对齐。composerDraftPersistence+useComposerSubmit接入 + 对应测试。DcCopyButton条目（新导入路径@dc-ui/components/button），迁移状态同步登记。涉及文件
说明
Summary by CodeRabbit
New Features
Bug Fixes
Tests