feat(desktop): add user message copy and edit actions#553
Conversation
|
Solid execution. The main thing I wanted to see is right: edit-regenerate flows through the same A few things worth a look before merge:
Review assisted by Claude Code agent (@Augustas11). |
kc-zero-lab
left a comment
There was a problem hiding this comment.
Thanks for the PR — the feature direction is solid, and I like that edit-regenerate reuses the existing runStreamingPrompt path instead of creating a parallel chat send flow. That should preserve buyer proxy/payment routing, pinned peer/service selection, streaming behavior, and channel reuse much better than a separate implementation.
I would still hold merge on a couple of correctness issues around retry/session behavior.
Should fix before merge
1. Edit branching should be idempotent under retry
In apps/desktop/src/main/pi-chat-engine.ts, the new edit path does:
if (options?.branchBeforeLastUserMessage) {
const branch = sessionManager.getBranch();
const lastUserEntry = [...branch]
.reverse()
.find((entry) => entry.type === 'message' && entry.message?.role === 'user');
...
sessionManager.branch(lastUserEntry.parentId);
}This recalculates “last user message” from the current branch every time the request enters runStreamingPrompt.
That is risky for payment/request retry paths. If the edited user message has already been committed before a retry re-enters this path, the retry can discover the newly edited message as the last user entry and branch to its parent. In the worst case, repeated retries can walk the leaf one parent too far back instead of converging on the same branch point.
Recommendation: make the operation target explicit, e.g. pass the original user message id/parent id from the renderer/main state, or add a guard so retrying the same edit is idempotent. At minimum, add regression coverage proving two consecutive edit dispatches/retries converge to the same leaf and do not truncate extra history.
2. Manual/payment retry should preserve edit context
The auto retry path now carries options, which is good:
dispatchChatRequest(convId, ctx.content, ctx.attachments, ctx.selection, ctx.options);But retryAfterPayment() still reconstructs from uiState.chatMessages and calls:
dispatchChatRequest(convId, content, attachments.length > 0 ? attachments : undefined);That loses:
options: { editLastUserMessage: true }- the original
selectionoverride - any explicit retry context from the failed edit request
So an edit-regenerate flow that hits the payment approval/manual retry path can degrade into a normal send/regenerate path. I’d store and reuse the original ChatRetryContext for the payment card action rather than reconstructing from the visible messages.
3. Attachment extraction contract is fragile
extractPreparedAttachmentsFromContent() only pulls attachments from type === 'file' blocks:
if (block.type === 'file' && block.attachment && typeof block.attachment === 'object')That appears to work because buildUserMessageContent() stores the canonical prepared attachment on the file block and adds image blocks separately for image payload/rendering. But this is not obvious from the helper, and future changes could easily break edit-regenerate for images/other attachment block types.
Please either:
- add a short comment saying
file.attachmentis the canonical prepared attachment source and image blocks are derived payload/render blocks, or - centralize/strongly type this contract so extraction and message construction cannot drift.
Nice-to-have polish
ChatView.tsx/modules/chat.ts: edit submit trims the draft twice. For editing, consider validating withdraft.trim()but sending the original draft so intentional whitespace/code-block formatting is preserved.ChatBubble.tsx: the edit textarea could use an explicitaria-label, e.g.aria-label="Edit message text".- Product wording: “Send” in the inline edit UI might be clearer as “Save & regenerate” because this action does more than save local text.
- Longer term: consider an
editedAt/edited marker. Keeping originalcreatedAtis acceptable for now, but users may expect edited messages to be distinguishable.
What looks good
- Reusing
runStreamingPromptfor edit-regenerate is the right architectural choice. - Preload/bridge/action plumbing is straightforward and scoped.
- Restricting edit to the latest user message and blocking while sending are good guardrails.
- Moving
extractPlainTextinto shared chat utils improves copy/edit reuse. - Preserving attachments visually while editing is a good UX improvement.
Overall: good feature, but I’d like to see #1 and #2 fixed or covered with regression tests before merge.
cc41b7d to
400408c
Compare
kc-zero-lab
left a comment
There was a problem hiding this comment.
Thanks for fixing the conflicts. I did a focused review for merge safety, code quality/simplicity, duplication, and test coverage.
I would not merge this as-is yet.
Critical issue:
- In
apps/desktop/src/renderer/modules/chat.ts, the edit-regenerate stuck-request retry path can mark the edit branch as prepared even whenchatAiEditLastUserMessage(...)returns an immediate failure such asRequest already in progress. - Flow: first attempt calls the edit endpoint,
sendStreamRequest()setseditBranchPrepared = true, caller aborts the stuck request, retry then goes through normalchatAiSendStream(...)instead of the edit endpoint. - That avoids branching twice, but it can also avoid branching at all after the abort/retry, so the edited message may be appended to the current branch instead of replacing the last user turn.
- Suggested fix: only mark the edit branch prepared after the edit endpoint actually succeeded / started the edit branch, or after stream/error evidence proves the main side already performed the branch. For a plain immediate
already in progressresult, abort and retry the edit endpoint again.
Code quality / simplicity:
- The retry-context additions are directionally right, but the
editLastUserMessage+editBranchPreparedstate is subtle and easy to misuse. I’d make the state transition explicit:needsEditBranch -> editBranchPrepared -> normalRetry, rather than deriving it insidegetRetryOptions()before knowing whether the edit request actually took effect. - There is some duplicated retry construction across normal request, payment retry, stream error, and non-stream paths. Not blocking by itself, but this PR makes retry semantics more complex; a small helper for “current retry context with safe edit options” would reduce future regressions.
extractPreparedAttachmentsFromContent()is a good extraction, but tests should cover mixed text/file/image content so we don’t accidentally resend renderer-only image blocks as durable attachments.
Tests required before merge:
- Add/adjust a test for the stuck in-flight edit path where the first
chatAiEditLastUserMessagereturnsRequest already in progress; after abort, the retry should callchatAiEditLastUserMessageagain, notchatAiSendStreamdirectly. - Add/keep a payment retry test proving that once the edit branch is genuinely prepared, manual retry uses normal send and does not branch one parent farther back.
- Add a regression test for attachment preservation on edit: file attachments are preserved, renderer-only image blocks are not treated as durable attachment sources.
Safe to merge score: 2/5.
The feature shape is good, and the UI/API integration looks reasonable, but the retry branching bug can corrupt conversation history in exactly the edge case this PR is trying to harden. I’d fix that before merging.
kc-zero-lab
left a comment
There was a problem hiding this comment.
Re-reviewed latest head df6dc247.
The previous merge-blocking edit retry issue looks fixed:
- edit branch preparation is now confirmed by the main process, not assumed by the renderer
- stuck in-flight edit retry now retries the edit endpoint when
editBranchPrepared: false - payment retry correctly carries
editBranchPrepared: trueand falls back to normal send only after the edit branch is confirmed
Code quality looks acceptable for this feature. The retry flow is still somewhat subtle, but the source-of-truth boundary is much clearer now and the added tests cover the important branching/payment cases.
Validation checked:
- renderer typecheck
- desktop main build
- peer-routing regression tests, all 11 passing
- diff whitespace check
Safe to merge score: 4/5.
Approving. I’d still consider a future cleanup to centralize retry-context construction, but I don’t see that as merge-blocking.
Summary
extractPlainTextCloses #500
Closes #501
Main-process / session behavior changed
runStreamingPromptpath as normal sends.Reviewer focus
Validation
Regression coverage added
file.attachmentattachments