fix(coding-agent): serialize queued input mutations#4680
fix(coding-agent): serialize queued input mutations#4680DarkPhilosophy wants to merge 8 commits into
Conversation
…sions; add coalescing mode (refs can1357#2890)
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
roboomp
left a comment
roboomp
left a comment
There was a problem hiding this comment.
P2: the queue-serialization direction is valuable, but the coalescing path still has correctness gaps before this is merge-ready.
Blocking: callback-less UI coalescing never updates local submission signatures, so delivered coalesced turns can clear an in-progress draft; coalesced image submissions also bypass text-only-model image-description companions.
Should-fix: long wrapped queued prompts can collapse without advertising the expand shortcut.
Thanks for narrowing the coalescing behavior and covering the queue ordering cases.
| ); | ||
| const imageCount = mergedImages.length; | ||
| onQueued?.(mergedText, imageCount, replacedText); | ||
| if (!onQueued) this.onLocalQueueCoalesced?.(shiftedPerSendText, mergedText, replacedText, imageCount); |
There was a problem hiding this comment.
blocking: This fallback listener is never wired in production. InputController records local queued submissions via withLocalSubmission(... session.prompt(...)) without passing onQueued (input-controller.ts:823, :919, :1231), and there is no assignment to session.onLocalQueueCoalesced in src/. In coalescing mode, local a + b delivers as a\nb, but locallySubmittedUserSignatures still contains the per-send signatures, so EventController treats the delivered user turn as non-local and clears the editor at event-controller.ts:451. Please wire this listener at the interactive-session boundary or pass an onQueued callback from the UI queue paths.
| content.push(...normalizedImages); | ||
| } | ||
| const queueMode = mode === "followUp" ? this.settings.get("followUpMode") : this.settings.get("steeringMode"); | ||
| if (queueMode === "coalescing" && this.#tryCoalesceQueuedUserMessage(text, normalizedImages, mode, onQueued)) { |
There was a problem hiding this comment.
blocking: This coalescing early-return skips the text-only-model image-description companion for the image(s) being merged. #buildImageDescriptionNotice() is only called below on the non-coalesced path, so with images.describeForTextModels enabled a queued first followed by second [Image #1] stores the image on the merged user message but enqueues no image-attachment-description for it. The text-only model then receives the attachment marker without the hidden description that the normal queue path provides. Build/enqueue the companion before coalescing, include it in the replacement, or decline image coalescing when the active model needs descriptions.
| this.ctx.pendingMessagesContainer.addChild(new Spacer(1)); | ||
| const expanded = this.ctx.pendingQueueExpanded; | ||
| const collapseLines = Math.max(1, this.ctx.settings?.get("pendingQueueCollapseLines") ?? 5); | ||
| const canExpandQueue = allMessages.some(entry => entry.message.split("\n").length > collapseLines); |
There was a problem hiding this comment.
should-fix: canExpandQueue counts only hard newline-separated logical lines, but QueuedMessageBox collapses by visual wrapped rows after Bun.wrapAnsi() (queued-message-box.ts:96). A single long queued prompt can show a +N rows footer while the footer omits the ${expandKey} hint because message.split("\n").length is still 1. Please base this on the same wrapped-row count as the box, or include the expand hint whenever the rendered box is collapsed.
…covery-coalescing
…covery-coalescing
2 participants
What
coalescingqueue mode through settings/RPC/UI surfaces.This supersedes and narrows the behavior from #2890: coalescing is no longer always-on; it is explicit opt-in, while the race-safety lock is always active.
Why
Rapid text/image submissions can overlap while image normalization is still running. Text-only input is effectively instant, but a local 1706×960 screenshot requires resize/encode work before it appears in the pending queue, so the user can observe a short delay on image submissions. The queue mutation itself should still remain deterministic under that delay.
Testing
bun check— passed locally and included@oh-my-pi/pi-coding-agent.bun test packages/coding-agent/test/agent-session-queue-merge.test.ts— 26 pass / 0 fail.bun test packages/coding-agent/test/input-controller-compaction-image.test.ts packages/coding-agent/test/input-controller-followup-image.test.ts packages/coding-agent/test/input-controller-skill-queue.test.ts packages/coding-agent/test/input-controller-escape.test.ts packages/coding-agent/test/modes/components/custom-editor-up-undo-send.test.ts packages/coding-agent/test/modes/components/wrapper-selector-mouse-offset.test.ts— 82 pass / 0 fail.bun test packages/coding-agent/test/input-controller-*.test.ts— 135 pass / 0 fail.28771298436completed successfully; all required jobs passed.