From a3b34a9d88adea6ed91967de9e981555d4277c0c Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Tue, 10 Nov 2026 16:38:58 -0600 Subject: [PATCH] fix(ui): allow queued message deletion before queue acknowledgement (#20553) ## Thinking Path Paperclip lets a user queue a message while an agent works. The user must be able to remove that message before delivery starts. A new local message can clear the displayed queue ID until the server accepts it. This also disables trash for older saved messages. The page rejects the action before it sends a request. This change keeps trash available when the server says the user can discard the message. When the queue ID is absent, the page uses the existing comment cancellation route. The server finds the queue and checks the author and dispatch state in the same transaction. ## Linked Issues or Issue Description **What happened?** Trash stays disabled for a saved queued message while another message waits for queue acknowledgement. The page handler also throws before it sends a cancellation request. **Expected behavior** Trash removes the selected queued message before dispatch. A message that has started dispatch stays visible with an error. Other queued messages stay queued. **Steps to reproduce** 1. Send a follow-up while an agent works. 2. Send another follow-up before the queue read includes it. 3. Try to discard the first saved message. **Paperclip version or commit** Reproduced from source at `0e3fdebea825`. **Deployment mode** Source checkout on Linux. UI tests use jsdom. Route tests use isolated embedded PostgreSQL. Related work: #15865 changes mixed-queue interrupt handling. #15939 changes recovery after a queued message is discarded. #15465 changes discard permissions for agent-authored entries. This change fixes trash for a user's own saved messages while queue acknowledgement is pending. ## What Changed - Keep trash enabled for a saved message with discard permission when the displayed queue ID is absent. - Use the existing author-only cancellation route when queue identity is pending. Keep the revision-checked route when queue identity is present. - Keep failed cancellations visible. Refresh the queue after cancellation and remove successful deletions from comment caches. - Cover deletion, dispatch conflicts, permission failures, network failures, history reloads, final-message cleanup, and concurrent dispatch. ## Verification - Red: three regression cases failed on the original production code. The trash button was disabled. Both page cases made no cancellation request. - Green: five complete UI suites pass, with 767 tests and one existing skipped test. These cover the page, queue component, thread, queue merge, and optimistic comments. - Green: 11 focused PostgreSQL route cases pass. They cover both routes, queue promotion, final-message cleanup, authorship, dispatch conflicts, and a concurrent dispatch claim. - `pnpm check:token-gates` and `git diff --check` pass. - `pnpm --filter @paperclipai/ui typecheck` passes. - The GitHub tree is `d4f0d61cbef2620c1fd62e6988ad80ccb1ba6173`. It is identical to the tested local tree after merging master `902f3767960d`. - `pnpm verify` selected more than 1,500 server test files. That broad local run was stopped without a pass claim. CI will supply broad build and test coverage. The preflight helper could not build Rust because Cargo is absent. - [CI Build](https://github.com/paperclipai/paperclip/actions/runs/42835584447/job/129897150252) and [browser E2E](https://github.com/paperclipai/paperclip/actions/runs/42835584447/job/129901676175) pass for head `44dea22faea51a6664a6f45852a42f62f5c72bbd`. - [Greptile review](https://github.com/paperclipai/paperclip/pull/20553#issuecomment-6770202777) gives the current head 5/5 with zero comments and no unresolved review threads. - [Final CI run](https://github.com/paperclipai/paperclip/actions/runs/42835584447) passes for head `44dea22faea51a6664a6f45852a42f62f5c72bbd`: 55 check runs passed and 4 were skipped. No failures or pending checks remain. Typecheck, Build, all general test shards, serialized suites, Runner checks, and browser E2E passed. Snyk also passed. ## Risks The fallback has no client queue revision. The existing server transaction finds the queue for the selected comment and checks ownership and dispatch state. Tests cover rejection after dispatch and when another user owns the message. No API contract or schema changes are required. No visual tokens change. Existing user documentation remains accurate. ## Model Used OpenAI GPT-6 in Codex. The runtime does not expose a more specific provider revision or context window size. Used reasoning, repository tools, code edits, and test execution. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip --- .../issue-queued-comments-routes.test.ts | 37 +++++++---- .../task-chat/TaskChatQueuedMessages.test.tsx | 40 ++++++++++-- .../task-chat/TaskChatQueuedMessages.tsx | 10 +-- ui/src/pages/IssueDetail.test.tsx | 61 ++++++++++++++++++- ui/src/pages/IssueDetail.tsx | 25 +++++--- 5 files changed, 135 insertions(+), 38 deletions(-) diff --git a/server/src/__tests__/issue-queued-comments-routes.test.ts b/server/src/__tests__/issue-queued-comments-routes.test.ts index 1aa33edb2c1..442dbeb0390 100644 --- a/server/src/__tests__/issue-queued-comments-routes.test.ts +++ b/server/src/__tests__/issue-queued-comments-routes.test.ts @@ -1539,7 +1539,7 @@ describeEmbeddedPostgres("issue queued-comment routes", () => { expect(Object.keys(edited.body.entries[0]).sort()).toEqual(Object.keys(initial.body.entries[0]).sort()); }); - it("cancels the deferred wake when the final message is discarded before promotion", async () => { + it.each(["discard", "cancel"] as const)("cancels the deferred wake when the final message is discarded before promotion (%s)", async (mode) => { const seeded = await seedQueue(); await db.delete(issueComments).where(eq(issueComments.id, seeded.commentIds[1])); await db @@ -1559,7 +1559,9 @@ describeEmbeddedPostgres("issue queued-comment routes", () => { const initial = await request(app(seeded.companyId)) .get(`/api/issues/${seeded.issueId}/queued-comments`); const discarded = await request(app(seeded.companyId)) - .delete(`/api/issues/${seeded.issueId}/queued-comments/${seeded.commentIds[0]}`) + .delete(mode === "cancel" + ? `/api/issues/${seeded.issueId}/comments/${seeded.commentIds[0]}?mode=cancel` + : `/api/issues/${seeded.issueId}/queued-comments/${seeded.commentIds[0]}`) .send({ queueId: seeded.wakeId, revision: initial.body.revision }); expect(discarded.status, JSON.stringify(discarded.body)).toBe(200); const [wake, runs] = await Promise.all([ @@ -1573,9 +1575,9 @@ describeEmbeddedPostgres("issue queued-comment routes", () => { expect(runs.map((run) => run.id)).toEqual([seeded.runId]); }); - it("routes legacy queued-comment cancellation through the promoted queue", async () => { + it.each([false, true])("routes comment cancellation through the queue (promoted=%s)", async (promoted) => { const seeded = await seedQueue(); - const queueRunId = await promoteQueue(seeded); + const queueRunId = promoted ? await promoteQueue(seeded) : null; const cancelled = await request(app(seeded.companyId)) .delete(`/api/issues/${seeded.issueId}/comments/${seeded.commentIds[0]}?mode=cancel`); expect(cancelled.status, JSON.stringify(cancelled.body)).toBe(200); @@ -1585,20 +1587,23 @@ describeEmbeddedPostgres("issue queued-comment routes", () => { .from(agentWakeupRequests) .where(eq(agentWakeupRequests.id, seeded.wakeId)) .then((rows) => rows[0]), - db.select({ contextSnapshot: heartbeatRuns.contextSnapshot }) + queueRunId ? db.select({ contextSnapshot: heartbeatRuns.contextSnapshot }) .from(heartbeatRuns) .where(eq(heartbeatRuns.id, queueRunId)) - .then((rows) => rows[0]), + .then((rows) => rows[0]) : null, ]); expect((wake?.payload as any)?._paperclipWakeContext?.wakeCommentIds).toEqual([ seeded.commentIds[1], ]); - expect((queueRun?.contextSnapshot as any)?.wakeCommentIds).toEqual([ + if (promoted) expect((queueRun?.contextSnapshot as any)?.wakeCommentIds).toEqual([ seeded.commentIds[1], ]); + expect(await db.select().from(issueComments).where(eq(issueComments.id, seeded.commentIds[0]))).toEqual([]); + const remaining = await request(app(seeded.companyId)).get(`/api/issues/${seeded.issueId}/queued-comments`); + expect(remaining.body.entries.map((entry: any) => entry.comment.id)).toEqual([seeded.commentIds[1]]); }); - it("reports an explicit conflict after queued-run dispatch has begun", async () => { + it.each(["discard", "cancel"] as const)("reports an explicit conflict after queued-run dispatch has begun (%s)", async (mode) => { const seeded = await seedQueue(); const initial = await request(app(seeded.companyId)) .get(`/api/issues/${seeded.issueId}/queued-comments`); @@ -1613,7 +1618,9 @@ describeEmbeddedPostgres("issue queued-comment routes", () => { .where(eq(heartbeatRuns.id, queueRunId)); const discard = await request(app(seeded.companyId)) - .delete(`/api/issues/${seeded.issueId}/queued-comments/${seeded.commentIds[0]}`) + .delete(mode === "cancel" + ? `/api/issues/${seeded.issueId}/comments/${seeded.commentIds[0]}?mode=cancel` + : `/api/issues/${seeded.issueId}/queued-comments/${seeded.commentIds[0]}`) .send({ queueId: seeded.wakeId, revision: initial.body.revision }); expect(discard.status).toBe(409); expect(discard.body.details?.code).toBe("queued_comment_already_dispatching"); @@ -1625,7 +1632,7 @@ describeEmbeddedPostgres("issue queued-comment routes", () => { expect(comment?.id).toBe(seeded.commentIds[0]); }); - it("limits edit and trash to the comment owner", async () => { + it.each(["discard", "cancel"] as const)("limits edit and trash to the comment owner (%s)", async (mode) => { const seeded = await seedQueue(); const initial = await request(app(seeded.companyId, "other-operator")) .get(`/api/issues/${seeded.issueId}/queued-comments`); @@ -1637,7 +1644,9 @@ describeEmbeddedPostgres("issue queued-comment routes", () => { .send({ queueId: seeded.wakeId, revision: initial.body.revision, body: "not mine" }); expect(edit.status).toBe(403); const discard = await request(app(seeded.companyId, "other-operator")) - .delete(`/api/issues/${seeded.issueId}/queued-comments/${seeded.commentIds[0]}`) + .delete(mode === "cancel" + ? `/api/issues/${seeded.issueId}/comments/${seeded.commentIds[0]}?mode=cancel` + : `/api/issues/${seeded.issueId}/queued-comments/${seeded.commentIds[0]}`) .send({ queueId: seeded.wakeId, revision: initial.body.revision }); expect(discard.status).toBe(403); }); @@ -1868,7 +1877,7 @@ describeEmbeddedPostgres("issue queued-comment routes", () => { expect(steerNativeSessionMock.mock.calls).toHaveLength(steeringCalls); }); - it("serializes discard against queued-run claim", async () => { + it.each(["discard", "cancel"] as const)("serializes discard against queued-run claim (%s)", async (mode) => { const seeded = await seedQueue(); await db.delete(issueComments).where(eq(issueComments.id, seeded.commentIds[1])); await db @@ -1906,7 +1915,9 @@ describeEmbeddedPostgres("issue queued-comment routes", () => { const [discard] = await Promise.all([ request(app(seeded.companyId)) - .delete(`/api/issues/${seeded.issueId}/queued-comments/${seeded.commentIds[0]}`) + .delete(mode === "cancel" + ? `/api/issues/${seeded.issueId}/comments/${seeded.commentIds[0]}?mode=cancel` + : `/api/issues/${seeded.issueId}/queued-comments/${seeded.commentIds[0]}`) .send({ queueId: seeded.wakeId, revision: initial.body.revision }), heartbeat.resumeQueuedRuns(), ]); diff --git a/ui/src/components/task-chat/TaskChatQueuedMessages.test.tsx b/ui/src/components/task-chat/TaskChatQueuedMessages.test.tsx index f8faac83c84..a1a03831c32 100644 --- a/ui/src/components/task-chat/TaskChatQueuedMessages.test.tsx +++ b/ui/src/components/task-chat/TaskChatQueuedMessages.test.tsx @@ -5,6 +5,7 @@ import { flushSync } from "react-dom"; import { createRoot, type Root } from "react-dom/client"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { IssueQueuedCommentQueue } from "@paperclipai/shared"; +import { mergePendingIssueQueuedComments } from "@/lib/issue-queued-comment-queue"; import { ThemeProvider } from "@/context/ThemeContext"; import { MemoryRouter } from "@/lib/router"; import { @@ -387,8 +388,9 @@ describe("TaskChatQueuedMessages", () => { expect(container.textContent).toContain("Queued message discarded."); }); - it("keeps a too-late discard visible with an explicit error", async () => { + it.each(["wake-1", null])("keeps a too-late discard visible with an explicit error (queue=%s)", async (queueId) => { render({ + queue: { ...queue, queueId }, onDiscard: vi.fn().mockRejectedValue({ body: { details: { code: "queued_comment_already_dispatching" } }, }), @@ -429,13 +431,43 @@ describe("TaskChatQueuedMessages", () => { expect(container.textContent).not.toContain("Queued message discarded."); }); - it("does not expose queue controls before a real queue id is acknowledged", () => { - const props = render({ queue: { ...queue, queueId: null, state: null } }); + it("can discard a saved message while queue acknowledgement is pending", async () => { + const merged = mergePendingIssueQueuedComments({ + issueId: queue.issueId, + authoritativeQueue: queue, + pendingComments: [{ + comment: { ...queue.entries[0].comment, id: "optimistic-next" }, + targetRunId: queue.targetRunId, + }], + fallbackProtocol: queue.protocol, + })!; + expect(merged.queueId).toBeNull(); + const props = render({ queue: merged }); + const discard = container.querySelector( + '[data-testid="task-chat-queued-discard-comment-1"]', + ); + expect(discard?.disabled).toBe(false); + expect(container.querySelector( + '[data-testid="task-chat-queued-steer-comment-1"]', + )?.disabled).toBe(true); + await act(async () => { discard!.click(); }); + expect(props.onDiscard).toHaveBeenCalledWith("comment-1", "rev-1"); + expect(container.querySelector('[data-testid="task-chat-queued-message-comment-1"]')).toBeNull(); + }); + + it("keeps another author's saved message protected while acknowledgement is pending", () => { + const props = render({ + queue: { + ...queue, + queueId: null, + entries: [{ ...queue.entries[0], canDiscard: false }], + }, + }); const discard = container.querySelector( '[data-testid="task-chat-queued-discard-comment-1"]', ); expect(discard?.disabled).toBe(true); - discard?.click(); + discard!.click(); expect(props.onDiscard).not.toHaveBeenCalled(); }); diff --git a/ui/src/components/task-chat/TaskChatQueuedMessages.tsx b/ui/src/components/task-chat/TaskChatQueuedMessages.tsx index 442f763f6c7..8772e7e274e 100644 --- a/ui/src/components/task-chat/TaskChatQueuedMessages.tsx +++ b/ui/src/components/task-chat/TaskChatQueuedMessages.tsx @@ -250,11 +250,7 @@ function SortableQueuedMessage({