Make deleted-comment cleanup atomic

Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
Dotta
2026-06-05 03:56:39 +00:00
parent 1afa337841
commit 9aa065a38c
5 changed files with 161 additions and 85 deletions
@@ -270,4 +270,48 @@ describeEmbeddedPostgres("documentAnnotationService", () => {
await expect(db.select().from(documentAnnotationComments)).resolves.toHaveLength(0); await expect(db.select().from(documentAnnotationComments)).resolves.toHaveLength(0);
}); });
it("does not report already-resolved empty threads as newly resolved during linked comment cleanup", async () => {
const { companyId, issueId, document } = await createIssueWithDocument();
const [issueComment] = await db
.insert(issueComments)
.values({
companyId,
issueId,
authorType: "user",
authorUserId: "board-user",
body: "Delete this linked comment from a resolved thread",
})
.returning();
const thread = await annotations.createThread(
issueId,
"plan",
{
baseRevisionId: document.latestRevisionId!,
baseRevisionNumber: document.latestRevisionNumber,
selector: {
quote: { exact: "selected text", prefix: "Alpha ", suffix: " omega" },
position: { normalizedStart: 6, normalizedEnd: 19, markdownStart: 6, markdownEnd: 19 },
},
body: "Linked annotation body",
issueCommentId: issueComment.id,
},
{ actorType: "user", actorId: "board-user", userId: "board-user" },
);
await db
.update(documentAnnotationThreads)
.set({ status: "resolved", resolvedByUserId: "board-user", resolvedAt: new Date("2026-06-05T03:05:00.000Z") })
.where(eq(documentAnnotationThreads.id, thread.id));
const cleanup = await annotations.cleanupForIssueCommentDeletion(
issueId,
issueComment.id,
{ actorType: "user", actorId: "board-user", userId: "board-user" },
);
expect(cleanup.deletedCommentIds).toEqual([thread.comments[0]!.id]);
expect(cleanup.resolvedThreadIds).toEqual([]);
});
}); });
@@ -195,7 +195,8 @@ describe.sequential("issue comment cancel routes", () => {
mockIssueService.assertCheckoutOwner.mockResolvedValue({ adoptedFromRunId: null }); mockIssueService.assertCheckoutOwner.mockResolvedValue({ adoptedFromRunId: null });
mockIssueService.getComment.mockResolvedValue(makeComment()); mockIssueService.getComment.mockResolvedValue(makeComment());
mockIssueService.removeComment.mockResolvedValue(makeComment()); mockIssueService.removeComment.mockResolvedValue(makeComment());
mockIssueService.tombstoneComment.mockResolvedValue(makeComment({ mockIssueService.tombstoneComment.mockImplementation(async (_commentId, _actor, options) => {
const deleted = makeComment({
body: "", body: "",
metadata: null, metadata: null,
deletedAt: new Date("2026-04-11T15:05:00.000Z"), deletedAt: new Date("2026-04-11T15:05:00.000Z"),
@@ -203,7 +204,10 @@ describe.sequential("issue comment cancel routes", () => {
deletedByAgentId: null, deletedByAgentId: null,
deletedByUserId: "local-board", deletedByUserId: "local-board",
deletedByRunId: null, deletedByRunId: null,
})); });
await options?.afterTombstone?.(deleted, "tx");
return deleted;
});
mockAccessService.canUser.mockResolvedValue(false); mockAccessService.canUser.mockResolvedValue(false);
mockAccessService.hasPermission.mockResolvedValue(false); mockAccessService.hasPermission.mockResolvedValue(false);
mockFeedbackService.listIssueVotesForUser.mockResolvedValue([]); mockFeedbackService.listIssueVotesForUser.mockResolvedValue([]);
@@ -337,13 +341,17 @@ describe.sequential("issue comment cancel routes", () => {
expect(JSON.stringify(res.body)).not.toContain("Sensitive original comment body"); expect(JSON.stringify(res.body)).not.toContain("Sensitive original comment body");
expect(JSON.stringify(res.body)).not.toContain("Sensitive metadata copy"); expect(JSON.stringify(res.body)).not.toContain("Sensitive metadata copy");
expect(mockIssueService.removeComment).not.toHaveBeenCalled(); expect(mockIssueService.removeComment).not.toHaveBeenCalled();
expect(mockIssueService.tombstoneComment).toHaveBeenCalledWith("comment-1", { expect(mockIssueService.tombstoneComment).toHaveBeenCalledWith(
"comment-1",
{
actorType: "user", actorType: "user",
agentId: null, agentId: null,
userId: "local-board", userId: "local-board",
runId: null, runId: null,
}); },
expect(mockIssueReferenceService.syncComment).toHaveBeenCalledWith("comment-1"); expect.objectContaining({ afterTombstone: expect.any(Function) }),
);
expect(mockIssueReferenceService.syncComment).toHaveBeenCalledWith("comment-1", "tx");
expect(mockDocumentAnnotationService.cleanupForIssueCommentDeletion).toHaveBeenCalledWith( expect(mockDocumentAnnotationService.cleanupForIssueCommentDeletion).toHaveBeenCalledWith(
"11111111-1111-4111-8111-111111111111", "11111111-1111-4111-8111-111111111111",
"comment-1", "comment-1",
@@ -351,8 +359,9 @@ describe.sequential("issue comment cancel routes", () => {
actorType: "user", actorType: "user",
userId: "local-board", userId: "local-board",
}), }),
"tx",
); );
expect(mockIssueReferenceService.deleteCommentSource).toHaveBeenCalledWith("annotation-comment-1"); expect(mockIssueReferenceService.deleteCommentSource).toHaveBeenCalledWith("annotation-comment-1", "tx");
const deletedActivity = mockLogActivity.mock.calls.find((call) => call[1]?.action === "issue.comment_deleted")?.[1]; const deletedActivity = mockLogActivity.mock.calls.find((call) => call[1]?.action === "issue.comment_deleted")?.[1];
expect(deletedActivity).toEqual(expect.objectContaining({ expect(deletedActivity).toEqual(expect.objectContaining({
action: "issue.comment_deleted", action: "issue.comment_deleted",
+18 -10
View File
@@ -5674,29 +5674,37 @@ export function issueRoutes(
return; return;
} }
const deleted = await svc.tombstoneComment(commentId, { let annotationCleanup = { deletedCommentIds: [] as string[], resolvedThreadIds: [] as string[] };
const deleted = await svc.tombstoneComment(
commentId,
{
actorType: actor.actorType, actorType: actor.actorType,
agentId: actor.agentId, agentId: actor.agentId,
userId: actor.actorType === "user" ? actor.actorId : null, userId: actor.actorType === "user" ? actor.actorId : null,
runId: actor.runId, runId: actor.runId,
}); },
if (!deleted) { {
res.status(404).json({ error: "Comment not found" }); afterTombstone: async (deletedComment, tx) => {
return; await issueReferencesSvc.syncComment(deletedComment.id, tx);
} annotationCleanup = await documentAnnotationsSvc.cleanupForIssueCommentDeletion(issue.id, deletedComment.id, {
await issueReferencesSvc.syncComment(deleted.id);
const annotationCleanup = await documentAnnotationsSvc.cleanupForIssueCommentDeletion(issue.id, deleted.id, {
actorType: actor.actorType, actorType: actor.actorType,
actorId: actor.actorId, actorId: actor.actorId,
agentId: actor.agentId, agentId: actor.agentId,
userId: actor.actorType === "user" ? actor.actorId : null, userId: actor.actorType === "user" ? actor.actorId : null,
runId: actor.runId, runId: actor.runId,
}); }, tx);
await Promise.all( await Promise.all(
annotationCleanup.deletedCommentIds.map((annotationCommentId) => annotationCleanup.deletedCommentIds.map((annotationCommentId) =>
issueReferencesSvc.deleteCommentSource(annotationCommentId) issueReferencesSvc.deleteCommentSource(annotationCommentId, tx)
), ),
); );
},
},
);
if (!deleted) {
res.status(404).json({ error: "Comment not found" });
return;
}
await logActivity(db, { await logActivity(db, {
companyId: issue.companyId, companyId: issue.companyId,
+13 -5
View File
@@ -330,7 +330,9 @@ export function documentAnnotationService(db: Db) {
issueId: string, issueId: string,
issueCommentId: string, issueCommentId: string,
actor: ActorInput, actor: ActorInput,
) => db.transaction(async (tx) => { dbOrTx: any = db,
) => {
const runCleanup = async (tx: any) => {
const linkedComments: Array<Pick<DocumentAnnotationComment, "id" | "threadId">> = await tx const linkedComments: Array<Pick<DocumentAnnotationComment, "id" | "threadId">> = await tx
.select({ .select({
id: documentAnnotationComments.id, id: documentAnnotationComments.id,
@@ -365,7 +367,7 @@ export function documentAnnotationService(db: Db) {
const emptyThreadIds = threadIds.filter((threadId) => (remainingByThreadId.get(threadId) ?? 0) === 0); const emptyThreadIds = threadIds.filter((threadId) => (remainingByThreadId.get(threadId) ?? 0) === 0);
if (emptyThreadIds.length > 0) { if (emptyThreadIds.length > 0) {
await tx const resolvedRows: Array<{ id: string }> = await tx
.update(documentAnnotationThreads) .update(documentAnnotationThreads)
.set({ .set({
status: "resolved", status: "resolved",
@@ -377,11 +379,17 @@ export function documentAnnotationService(db: Db) {
.where(and( .where(and(
inArray(documentAnnotationThreads.id, emptyThreadIds), inArray(documentAnnotationThreads.id, emptyThreadIds),
eq(documentAnnotationThreads.status, "open"), eq(documentAnnotationThreads.status, "open"),
)); ))
.returning({ id: documentAnnotationThreads.id });
return { deletedCommentIds, resolvedThreadIds: resolvedRows.map((row) => row.id) };
} }
return { deletedCommentIds, resolvedThreadIds: emptyThreadIds }; return { deletedCommentIds, resolvedThreadIds: [] };
}), };
return dbOrTx === db ? db.transaction(runCleanup) : runCleanup(dbOrTx);
},
updateThread: async ( updateThread: async (
issueId: string, issueId: string,
+8 -1
View File
@@ -34,6 +34,7 @@ import {
} from "@paperclipai/db"; } from "@paperclipai/db";
import type { import type {
AcceptedPlanDecomposition, AcceptedPlanDecomposition,
IssueComment,
IssueCommentAuthorType, IssueCommentAuthorType,
IssueCommentMetadata, IssueCommentMetadata,
IssueCommentPresentation, IssueCommentPresentation,
@@ -5733,6 +5734,9 @@ export function issueService(db: Db) {
userId?: string | null; userId?: string | null;
runId?: string | null; runId?: string | null;
}, },
options?: {
afterTombstone?: (comment: IssueComment, tx: any) => Promise<void>;
},
) => { ) => {
const currentUserRedactionOptions = { const currentUserRedactionOptions = {
enabled: (await instanceSettings.getGeneral()).censorUsernameInLogs, enabled: (await instanceSettings.getGeneral()).censorUsernameInLogs,
@@ -5763,7 +5767,10 @@ export function issueService(db: Db) {
.set({ updatedAt: now }) .set({ updatedAt: now })
.where(eq(issues.id, comment.issueId)); .where(eq(issues.id, comment.issueId));
return redactIssueComment(comment, currentUserRedactionOptions.enabled); const redacted = redactIssueComment(comment, currentUserRedactionOptions.enabled);
await options?.afterTombstone?.(redacted, tx);
return redacted;
}); });
}, },