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 <noreply@paperclip.ing>
2026-11-11 05:11:49 +01:00 · 2026-11-10 16:38:58 -06:00
1 parent f30ac226d6
commit a3b34a9d88
@@ -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(),
    ]);
@@ -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<HTMLButtonElement>(
+      '[data-testid="task-chat-queued-discard-comment-1"]',
+    );
+    expect(discard?.disabled).toBe(false);
+    expect(container.querySelector<HTMLButtonElement>(
+      '[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<HTMLButtonElement>(
      '[data-testid="task-chat-queued-discard-comment-1"]',
    );
    expect(discard?.disabled).toBe(true);
-    discard?.click();
+    discard!.click();
    expect(props.onDiscard).not.toHaveBeenCalled();
  });

@@ -250,11 +250,7 @@ function SortableQueuedMessage({
      <button
        type="button"
        onClick={onDiscard}
-        disabled={
-          busy ||
-          (!queue.queueId && !entry.comment.id.startsWith("optimistic-")) ||
-          !entry.canDiscard
-        }
+        disabled={busy || !entry.canDiscard}
        title="Discard queued message"
        aria-label={`Discard queued message: ${label}`}
        className="flex h-7 w-7 shrink-0 items-center justify-center rounded-md text-muted-foreground transition-colors hover:bg-destructive/10 hover:text-destructive disabled:opacity-40"
@@ -449,12 +445,10 @@ export function TaskChatQueuedMessages({
    commentId: string,
    action: Exclude<QueueAction, null>,
  ) {
-    const locallyDiscardable =
-      action === "discard" && commentId.startsWith("optimistic-");
    if (
      pending ||
      reordering ||
-      (!queue.queueId && action !== "interrupt" && !locallyDiscardable)
+      (!queue.queueId && action !== "interrupt" && action !== "discard")
    ) {
      return;
    }
@@ -7546,6 +7546,61 @@ describe("IssueDetail", () => {
    });
  });

+  it.each(["success", "dispatch", "forbidden", "network"] as const)(
+    "discards a saved message before queue acknowledgement (%s)",
+    async (outcome) => {
+      const queue = createQueuedCommentQueue({ queueId: null, state: null });
+      const comment = queue.entries[0].comment;
+      mockIssuesApi.get.mockResolvedValue(createIssue({ status: "in_progress", assigneeAgentId: "agent-1" }));
+      mockAgentsApi.list.mockResolvedValue([createAgent({ adapterType: "paperclip_runner" })]);
+      mockIssuesApi.getQueuedComments.mockResolvedValue(queue);
+      mockIssuesApi.recentComments.mockResolvedValue({ comments: [comment], latestCommentId: comment.id, hasMore: false });
+      mockIssuesApi.listComments.mockResolvedValue([comment]);
+      const failure = outcome === "dispatch"
+        ? new ApiError("The queued message is already being dispatched", 409, {
+            details: { code: "queued_comment_already_dispatching" },
+          })
+        : outcome === "forbidden"
+          ? new ApiError("Only the queued message author can discard it", 403, {})
+          : new Error("Connection lost");
+      if (outcome === "success") mockIssuesApi.cancelComment.mockResolvedValue(comment);
+      else mockIssuesApi.cancelComment.mockRejectedValue(failure);
+
+      await act(async () => { renderIssueView(); });
+      const props = () => mockIssueChatThreadRender.mock.calls.at(-1)![0] as {
+        queuedCommentQueue: IssueQueuedCommentQueue | null;
+        comments: IssueComment[];
+        onDiscardQueuedComment: (commentId: string, revision: string) => Promise<void>;
+      };
+      await waitForAssertion(() => {
+        expect(props().queuedCommentQueue).toMatchObject({
+          queueId: null,
+          entries: [{ comment: { id: comment.id } }],
+        });
+      });
+      let result: unknown;
+      await act(async () => {
+        result = await props().onDiscardQueuedComment(comment.id, queue.revision).catch((error: unknown) => error);
+      });
+      expect(mockIssuesApi.cancelComment).toHaveBeenCalledWith("issue-1", comment.id);
+      expect(mockIssuesApi.discardQueuedComment).not.toHaveBeenCalled();
+      if (outcome === "success") {
+        expect(result).toBeUndefined();
+        await waitForAssertion(() => {
+          expect(props().queuedCommentQueue).toBeNull();
+          expect(props().comments.some((value) => value.id === comment.id)).toBe(false);
+        });
+      } else {
+        expect(result).toBe(failure);
+        expect(props().queuedCommentQueue?.entries[0].comment.id).toBe(comment.id);
+        expect(props().comments.some((value) => value.id === comment.id)).toBe(true);
+        if (outcome === "dispatch") {
+          expect(mockPushToast).toHaveBeenCalledWith(expect.objectContaining({ title: "Message is already being sent" }));
+        }
+      }
+    },
+  );
+
  it.each([true, false])("keeps a discarded queued comment out of the thread (expanded=%s)", async (expanded) => {
    queryClient.setQueryData(["issues", "history-expanded", "PAP-1"], expanded);
    const queue = createQueuedCommentQueue();
@@ -7624,8 +7679,8 @@ describe("IssueDetail", () => {
    });
  });

-  it.each(["discard", "cancel", "delete"] as const)("keeps %s responses in both comment caches after reopening by either task ref", async (action) => {
-    const queue = createQueuedCommentQueue();
+  it.each(["discard", "discard-unacknowledged", "cancel", "delete"] as const)("keeps %s responses in both comment caches after reopening by either task ref", async (action) => {
+    const queue = createQueuedCommentQueue(action === "discard-unacknowledged" ? { queueId: null, state: null } : {});
    const comment = queue.entries[0].comment;
    const earlier = createIssueComment({ id: "earlier-comment", createdAt: new Date("2026-04-21T00:00:01.000Z") });
    const deleted = { ...comment, body: "", deletedAt: new Date() };
@@ -7663,7 +7718,7 @@ describe("IssueDetail", () => {
      queryFn: () => new Promise<IssueRecentComments>((resolve) => { finishStaleRead = resolve; }),
    }).catch(() => undefined);
    await act(async () => {
-      if (action === "discard") await props().onDiscardQueuedComment(comment.id, queue.revision);
+      if (action === "discard" || action === "discard-unacknowledged") await props().onDiscardQueuedComment(comment.id, queue.revision);
      if (action === "cancel") props().onCancelQueued(comment.id);
      if (action === "delete") await props().onDeleteComment(comment.id);
    });
@@ -2472,20 +2472,25 @@ const IssueDetailChatTab = memo(function IssueDetailChatTab({
  const discardQueuedComment = useCallback(
    async (commentId: string, revision: string) => {
      const queueId = effectiveQueuedCommentQueue?.queueId;
-      if (!queueId)
-        throw new Error(
-          "The queued message is awaiting server acknowledgement.",
-        );
      try {
-        storeQueuedCommentQueue(
-          await issuesApi.discardQueuedComment(issueId, commentId, {
-            queueId,
-            revision,
-          }),
-        );
+        if (queueId) {
+          storeQueuedCommentQueue(
+            await issuesApi.discardQueuedComment(issueId, commentId, {
+              queueId,
+              revision,
+            }),
+          );
+        } else {
+          await issuesApi.cancelComment(issueId, commentId);
+        }
        setDiscardedQueuedCommentIds((current) =>
          new Set(current).add(commentId),
        );
+        if (!queueId) {
+          void queryClient.invalidateQueries({
+            queryKey: queryKeys.issues.queuedComments(issueId),
+          });
+        }
        // Discard deletes the persisted comment. Update both history caches
        // and task aliases so the row stays removed after this view unmounts.
        await onRemoveCommentFromCache(commentId);