From e6d8eda8e53c877ac8e32ed1449d86d7cd8f0154 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 4 Jul 2026 18:22:35 +0300 Subject: [PATCH] fix(comment): dismiss owner/admin authz + atomic conditional delete + 404-only onError (#329 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Maintainer escalation decision (B) + reviewer findings on the ephemeral- suggestion PR. Authz (decision B): POST /comments/dismiss-suggestion now gates the destructive branch on owner-OR-space-admin, mirroring POST /comments/delete exactly (same SpaceCaslAction.Manage / SpaceCaslSubject.Settings, same owner short-circuit, same ForbiddenException). A non-owner non-admin who tries to dismiss another's childless suggestion gets Forbidden before the service runs. Apply stays on canEdit (accepting an edit is the editor's semantics), unchanged. F1 [blocking] — atomic conditional delete closes the hasChildren→delete race. New repo `deleteCommentIfChildless(id)` runs a single `DELETE FROM comments WHERE id=:id AND NOT EXISTS (SELECT 1 FROM comments child WHERE child.parent_comment_id = comments.id)` (verified by compiling the Kysely expression to SQL — the correlated subquery references the OUTER comments.id). deleteEphemeralSuggestion strips the mark first, then the conditional delete: if it removed the row → commentDeleted + outcome 'deleted'; if a reply raced in (0 rows) → fall back to resolveComment (outcome 'resolved') so the discussion and the new reply survive. No reply can be cascade-deleted anymore. F2 [warning] — the apply/dismiss onError success-noop is narrowed from 404||400 to 404 ONLY. A 400 means the comment is ALIVE (apply's 400 = the thread was resolved-not-applied), so it now shows a real error (surfacing the server message) and KEEPS the comment in cache instead of a false "applied" + dropping a live thread. F3 [suggestion] — the 404-race client tests assert the success toast fired. Tests: server — dismiss authz (owner ok / non-owner-non-admin Forbidden / space-admin ok), the delete→resolve race (hasChildren=false but conditional delete returns 0 → resolve, no commentDeleted), delete-path asserts switched to deleteCommentIfChildless; client — apply-400 and dismiss-400 (kept in cache, red, not success) + the toast assertions. server tsc clean, comment+collaboration jest green; client tsc clean, comment vitest 54 passed. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../comment-query.suggestion-outcome.test.tsx | 74 +++++++++++++++++++ .../features/comment/queries/comment-query.ts | 46 +++++++++--- .../core/comment/comment.controller.spec.ts | 67 ++++++++++++++++- .../src/core/comment/comment.controller.ts | 30 ++++++-- .../comment.service.apply-suggestion.spec.ts | 44 +++++++++-- ...comment.service.dismiss-suggestion.spec.ts | 43 ++++++++++- .../src/core/comment/comment.service.ts | 66 +++++++++++++---- .../database/repos/comment/comment.repo.ts | 31 ++++++++ 8 files changed, 357 insertions(+), 44 deletions(-) diff --git a/apps/client/src/features/comment/queries/comment-query.suggestion-outcome.test.tsx b/apps/client/src/features/comment/queries/comment-query.suggestion-outcome.test.tsx index 6fe54bef..ff289e45 100644 --- a/apps/client/src/features/comment/queries/comment-query.suggestion-outcome.test.tsx +++ b/apps/client/src/features/comment/queries/comment-query.suggestion-outcome.test.tsx @@ -28,6 +28,7 @@ vi.mock("@/features/comment/services/comment-service", () => ({ getPageComments: vi.fn(), })); +import { notifications } from "@mantine/notifications"; import { applySuggestion, dismissSuggestion, @@ -181,6 +182,39 @@ describe("useDismissSuggestionMutation — outcome handling (#329)", () => { await waitFor(() => expect(result.current.isError).toBe(true)); expect(items(queryClient)).toHaveLength(0); + // #338 F3: the idempotent race must still fire the SUCCESS toast, not just + // silently drop the comment. + expect(notifications.show).toHaveBeenCalledWith({ + message: "Suggestion dismissed", + }); + }); + + it("dismiss 400 (thread still alive) → NOT a success, comment kept, no green toast (#338 F2)", async () => { + // 400 means the thread is alive (already resolved / a reply raced in). + // Narrowed onError: only 404 is a success-noop; 400 must surface a real error + // and keep the comment in the cache. + vi.mocked(dismissSuggestion).mockRejectedValue({ + response: { status: 400 }, + }); + const { queryClient, wrapper } = seededClient(comment()); + + const { result } = renderHook(() => useDismissSuggestionMutation(), { + wrapper, + }); + await result.current + .mutateAsync({ commentId: "c-1", pageId: PAGE_ID }) + .catch(() => undefined); + await waitFor(() => expect(result.current.isError).toBe(true)); + + // Comment NOT dropped from the cache. + expect(items(queryClient)).toHaveLength(1); + // A real (red) error, never the success message. + expect(notifications.show).toHaveBeenCalledWith( + expect.objectContaining({ color: "red" }), + ); + expect(notifications.show).not.toHaveBeenCalledWith({ + message: "Suggestion dismissed", + }); }); it("APPLY idempotent race (404) → treated as success, comment removed from the list", async () => { @@ -201,5 +235,45 @@ describe("useDismissSuggestionMutation — outcome handling (#329)", () => { await waitFor(() => expect(result.current.isError).toBe(true)); expect(items(queryClient)).toHaveLength(0); + // #338 F3: the idempotent race must still fire the SUCCESS toast. + expect(notifications.show).toHaveBeenCalledWith({ + message: "Suggestion applied", + }); + }); + + it("APPLY 400 (thread resolved, not applied) → NOT a success, comment kept, red error (#338 F2)", async () => { + // apply's only 400 is "Cannot apply … on a resolved comment thread" — the + // thread was resolved (often with discussion) but NOT applied. It must be a + // real error surfacing the server message, and must NOT drop the live thread. + vi.mocked(applySuggestion).mockRejectedValue({ + response: { + status: 400, + data: { + message: "Cannot apply a suggested edit on a resolved comment thread", + }, + }, + }); + const { queryClient, wrapper } = seededClient(comment()); + + const { result } = renderHook(() => useApplySuggestionMutation(), { + wrapper, + }); + await result.current + .mutateAsync({ commentId: "c-1", pageId: PAGE_ID }) + .catch(() => undefined); + await waitFor(() => expect(result.current.isError).toBe(true)); + + // The live thread is NOT dropped from the cache. + expect(items(queryClient)).toHaveLength(1); + // Surfaces the server's specific message as a red error, never a success. + expect(notifications.show).toHaveBeenCalledWith( + expect.objectContaining({ + message: "Cannot apply a suggested edit on a resolved comment thread", + color: "red", + }), + ); + expect(notifications.show).not.toHaveBeenCalledWith({ + message: "Suggestion applied", + }); }); }); diff --git a/apps/client/src/features/comment/queries/comment-query.ts b/apps/client/src/features/comment/queries/comment-query.ts index a573a071..97783841 100644 --- a/apps/client/src/features/comment/queries/comment-query.ts +++ b/apps/client/src/features/comment/queries/comment-query.ts @@ -253,14 +253,19 @@ export function useApplySuggestionMutation() { }, onError: (err: any, variables) => { const status = err?.response?.status; - // Idempotent races (double-click, or apply↔dismiss): after #329 an applied + // Idempotent race (double-click, or apply↔dismiss): after #329 an applied // reply-less suggestion is hard-deleted, so a second/racing apply hits 404 - // (already gone) or 400 (already resolved) BEFORE the server's applied - // idempotent branch. Drop it from the cache and report success — the user's - // intent is already satisfied — rather than a scary error (mirrors dismiss; - // restores the #315 apply idempotency the ephemeral delete would otherwise - // break). - if (status === 404 || status === 400) { + // (already gone). ONLY 404 is a real success-noop — drop it from the cache + // and report success, the user's intent is already satisfied (restores the + // #315 apply idempotency the ephemeral delete would otherwise break). + // + // 400 is NOT success (#338 F2): apply's only 400 is "Cannot apply … on a + // resolved comment thread" — the thread was resolved (often WITH a live + // discussion) but the edit was NOT applied. Treating it as "Suggestion + // applied" is a false success that also drops a live thread from the cache. + // The #315 idempotent repeat does NOT produce 400 (childless → 404; + // with-replies → 200), so we never lose idempotency by excluding it here. + if (status === 404) { const cache = queryClient.getQueryData(RQ_KEY(variables.pageId)) as | InfiniteData> | undefined; @@ -273,6 +278,20 @@ export function useApplySuggestionMutation() { notifications.show({ message: t("Suggestion applied") }); return; } + // 400 => the thread was resolved and the edit could not be applied. Show a + // real error and KEEP the comment in the cache (it is still alive). Prefer + // the server's specific message when it carries one. + if (status === 400) { + const serverMsg = err?.response?.data?.message; + notifications.show({ + message: + typeof serverMsg === "string" && serverMsg.length > 0 + ? serverMsg + : t("Failed to apply suggestion"), + color: "red", + }); + return; + } // 409 => the commented text changed since the suggestion was made. Surface // a specific message (with the current text) rather than a generic error. const currentText = err?.response?.data?.currentText; @@ -321,12 +340,15 @@ export function useDismissSuggestionMutation() { notifications.show({ message: t("Suggestion dismissed") }); }, onError: (err: any, variables) => { - // Idempotent races (double-click, or apply↔dismiss): the comment is already - // gone (404) or already resolved (400). Drop it from the cache and report - // success rather than a scary error — the user's intent (make it disappear) - // is satisfied either way. + // Idempotent race (double-click, or apply↔dismiss): the comment is already + // gone (404). ONLY 404 is a real success-noop — drop it from the cache and + // report success, the user's intent (make it disappear) is satisfied. + // + // 400 is NOT success (#338 F2): it means the thread is still ALIVE (already + // resolved, or a reply raced in), so treating it as "dismissed" would drop + // a live thread from the cache. Show a real error and keep the comment. const status = err?.response?.status; - if (status === 404 || status === 400) { + if (status === 404) { const cache = queryClient.getQueryData(RQ_KEY(variables.pageId)) as | InfiniteData> | undefined; diff --git a/apps/server/src/core/comment/comment.controller.spec.ts b/apps/server/src/core/comment/comment.controller.spec.ts index 3cdbf502..0d10be04 100644 --- a/apps/server/src/core/comment/comment.controller.spec.ts +++ b/apps/server/src/core/comment/comment.controller.spec.ts @@ -127,7 +127,9 @@ describe('CommentController apply-suggestion authz', () => { * the delete/resolve + mark removal). These tests pin that boundary. */ describe('CommentController dismiss-suggestion authz', () => { - function makeController() { + // isAdmin=false → ability.cannot(Manage, Settings) returns true (i.e. the user + // is NOT a space admin). Flip to true to model a space admin. + function makeController(isAdmin = false) { const commentService = { dismissSuggestion: jest.fn(async () => ({ id: 'c-1', @@ -136,7 +138,11 @@ describe('CommentController dismiss-suggestion authz', () => { }; const commentRepo = { findById: jest.fn() }; const pageRepo = { findById: jest.fn() }; - const spaceAbility = {} as any; + const spaceAbility = { + createForUser: jest.fn(async () => ({ + cannot: jest.fn(() => !isAdmin), + })), + } as any; const pageAccessService = { validateCanComment: jest.fn(async () => undefined), validateCanEdit: jest.fn(async () => undefined), @@ -159,6 +165,7 @@ describe('CommentController dismiss-suggestion authz', () => { commentRepo, pageRepo, pageAccessService, + spaceAbility, }; } @@ -166,10 +173,12 @@ describe('CommentController dismiss-suggestion authz', () => { const workspace: any = { id: 'ws-1' }; const provenance: any = undefined; const dto: any = { commentId: 'c-1' }; + // Owned by the acting user (u-1) unless a test overrides creatorId. const comment = { id: 'c-1', pageId: 'p-1', spaceId: 'sp-1', + creatorId: 'u-1', suggestedText: 'new text', selection: 'old text', }; @@ -258,4 +267,58 @@ describe('CommentController dismiss-suggestion authz', () => { controller.dismissSuggestion(dto, user, workspace, provenance), ).rejects.toBeInstanceOf(BadRequestException); }); + + // --- #338 owner-or-space-admin gate (mirrors POST /comments/delete) -------- + // A childless dismiss irreversibly hard-deletes the comment, so canComment is + // not enough: only the comment owner or a space admin may dismiss. + + it('owner dismisses their own suggestion → allowed, no admin check needed', async () => { + const { controller, commentRepo, pageRepo, commentService, spaceAbility } = + makeController(false); + // comment.creatorId === user.id (owner). + commentRepo.findById.mockResolvedValue(comment); + pageRepo.findById.mockResolvedValue(page); + + await controller.dismissSuggestion(dto, user, workspace, provenance); + + // Owner short-circuits the admin lookup. + expect(spaceAbility.createForUser).not.toHaveBeenCalled(); + expect(commentService.dismissSuggestion).toHaveBeenCalledWith( + comment, + user, + provenance, + ); + }); + + it('non-owner non-admin → Forbidden AND the service is never called', async () => { + const { controller, commentRepo, pageRepo, commentService, spaceAbility } = + makeController(false); // NOT a space admin + commentRepo.findById.mockResolvedValue({ + ...comment, + creatorId: 'someone-else', + }); + pageRepo.findById.mockResolvedValue(page); + + await expect( + controller.dismissSuggestion(dto, user, workspace, provenance), + ).rejects.toBeInstanceOf(ForbiddenException); + + expect(spaceAbility.createForUser).toHaveBeenCalledWith(user, comment.spaceId); + expect(commentService.dismissSuggestion).not.toHaveBeenCalled(); + }); + + it('non-owner space admin → allowed to dismiss another user’s suggestion', async () => { + const { controller, commentRepo, pageRepo, commentService, spaceAbility } = + makeController(true); // space admin + commentRepo.findById.mockResolvedValue({ + ...comment, + creatorId: 'someone-else', + }); + pageRepo.findById.mockResolvedValue(page); + + await controller.dismissSuggestion(dto, user, workspace, provenance); + + expect(spaceAbility.createForUser).toHaveBeenCalledWith(user, comment.spaceId); + expect(commentService.dismissSuggestion).toHaveBeenCalled(); + }); }); diff --git a/apps/server/src/core/comment/comment.controller.ts b/apps/server/src/core/comment/comment.controller.ts index 07c6267c..fbd2c190 100644 --- a/apps/server/src/core/comment/comment.controller.ts +++ b/apps/server/src/core/comment/comment.controller.ts @@ -258,13 +258,33 @@ export class CommentController { // Authorize BEFORE revealing any structural detail (metadata-disclosure // hygiene, mirroring apply-suggestion). Dismissing a suggestion does NOT - // change the page text — it only removes/resolves the comment — so require - // comment access (canComment), NOT edit access. A viewer allowed to comment - // but not edit can still dismiss a suggestion. The structural 400s - // (top-level / has-a-suggested-edit / not applied / not resolved) are - // re-checked by the service below. + // change the page text — it only removes/resolves the comment — so the + // page-level gate is comment access (canComment), NOT edit access. A viewer + // allowed to comment but not edit can still dismiss their own suggestion. + // The structural 400s (top-level / has-a-suggested-edit / not applied / + // not resolved) are re-checked by the service below. await this.pageAccessService.validateCanComment(page, user, workspace.id); + // AUTHZ (#338): a childless dismiss IRREVERSIBLY hard-deletes the comment, + // so — beyond canComment — restrict it to the comment owner OR a space + // admin, exactly like POST /comments/delete. canComment alone is not enough: + // it would let any bystander commenter erase another user's suggestion for + // good. (apply-suggestion deliberately stays on canEdit: accepting an edit + // is the editor's semantics, not the suggestion author's.) + const isOwner = comment.creatorId === user.id; + if (!isOwner) { + const ability = await this.spaceAbility.createForUser( + user, + comment.spaceId, + ); + // Space admin can dismiss any suggestion. + if (ability.cannot(SpaceCaslAction.Manage, SpaceCaslSubject.Settings)) { + throw new ForbiddenException( + 'You can only dismiss your own suggestions', + ); + } + } + return this.commentService.dismissSuggestion(comment, user, provenance); } diff --git a/apps/server/src/core/comment/comment.service.apply-suggestion.spec.ts b/apps/server/src/core/comment/comment.service.apply-suggestion.spec.ts index 8fa8ffa8..4b856962 100644 --- a/apps/server/src/core/comment/comment.service.apply-suggestion.spec.ts +++ b/apps/server/src/core/comment/comment.service.apply-suggestion.spec.ts @@ -23,7 +23,7 @@ import { AuditEvent, AuditResource } from '../../common/events/audit-events'; describe('CommentService — applySuggestion', () => { const UPDATED = { id: 'c-1', __updated: true } as any; - function makeService(verdict: unknown, hasChildren = false) { + function makeService(verdict: unknown, hasChildren = false, deletedRows = 1) { const commentRepo: any = { // Both the applied-stamp re-read and resolveComment's re-read go through // findById; return a recognizable enriched row. @@ -31,6 +31,9 @@ describe('CommentService — applySuggestion', () => { updateComment: jest.fn(async () => undefined), hasChildren: jest.fn(async () => hasChildren), deleteComment: jest.fn(async () => undefined), + // #338 F1: the childless ephemeral delete is atomic-conditional and + // returns the number of rows removed (1 = deleted, 0 = a reply raced in). + deleteCommentIfChildless: jest.fn(async () => deletedRows), }; const pageRepo: any = {}; const wsService: any = { emitCommentEvent: jest.fn() }; @@ -101,9 +104,9 @@ describe('CommentService — applySuggestion', () => { }), ); - // Ephemeral: the redundant comment is hard-deleted and its inline anchor - // mark removed via the deleteCommentMark collab event. - expect(commentRepo.deleteComment).toHaveBeenCalledWith('c-1'); + // Ephemeral: the redundant comment is hard-deleted (atomic-conditional) and + // its inline anchor mark removed via the deleteCommentMark collab event. + expect(commentRepo.deleteCommentIfChildless).toHaveBeenCalledWith('c-1'); expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith( 'deleteCommentMark', 'page.page-1', @@ -136,7 +139,7 @@ describe('CommentService — applySuggestion', () => { const result = await service.applySuggestion(suggestionComment(), user()); - expect(commentRepo.deleteComment).toHaveBeenCalledWith('c-1'); + expect(commentRepo.deleteCommentIfChildless).toHaveBeenCalledWith('c-1'); expect(auditService.log).toHaveBeenCalledTimes(1); expect(result.outcome).toBe('deleted'); }); @@ -247,10 +250,39 @@ describe('CommentService — applySuggestion', () => { expect.anything(), expect.anything(), ); - expect(commentRepo.deleteComment).toHaveBeenCalledWith('c-1'); + expect(commentRepo.deleteCommentIfChildless).toHaveBeenCalledWith('c-1'); expect(result.outcome).toBe('deleted'); }); + it('applied=true, no replies at read time but a reply races in (conditional delete → 0 rows) → resolves instead, no hard-delete, outcome=resolved (#338 F1)', async () => { + // The suggested text is already applied to the document, but between the + // hasChildren read and the atomic delete a reply landed. The parent must NOT + // be hard-deleted (cascade would destroy the reply); resolve the thread. + const { service, commentRepo, wsService, collaborationGateway } = + makeService({ applied: true, currentText: 'new text' }, false, 0); + + const result = await service.applySuggestion(suggestionComment(), user()); + + expect(commentRepo.deleteCommentIfChildless).toHaveBeenCalledWith('c-1'); + // No deletion broadcast — the row + the racing reply survive. + expect(wsService.emitCommentEvent).not.toHaveBeenCalledWith( + expect.anything(), + expect.anything(), + expect.objectContaining({ operation: 'commentDeleted' }), + ); + // Fell back to resolving. + const resolvePatch = commentRepo.updateComment.mock.calls + .map((c: any[]) => c[0]) + .find((p: any) => 'resolvedAt' in p); + expect(resolvePatch.resolvedAt).toBeInstanceOf(Date); + expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith( + 'resolveCommentMark', + 'page.page-1', + expect.objectContaining({ commentId: 'c-1', resolved: true }), + ); + expect(result.outcome).toBe('resolved'); + }); + it('rejects a comment with no suggestedText', async () => { const { service, collaborationGateway } = makeService({ applied: true, diff --git a/apps/server/src/core/comment/comment.service.dismiss-suggestion.spec.ts b/apps/server/src/core/comment/comment.service.dismiss-suggestion.spec.ts index 5c5553b2..41fc65be 100644 --- a/apps/server/src/core/comment/comment.service.dismiss-suggestion.spec.ts +++ b/apps/server/src/core/comment/comment.service.dismiss-suggestion.spec.ts @@ -15,12 +15,15 @@ import { AuditEvent, AuditResource } from '../../common/events/audit-events'; describe('CommentService — dismissSuggestion', () => { const UPDATED = { id: 'c-1', __updated: true } as any; - function makeService(hasChildren = false) { + function makeService(hasChildren = false, deletedRows = 1) { const commentRepo: any = { findById: jest.fn(async () => UPDATED), updateComment: jest.fn(async () => undefined), hasChildren: jest.fn(async () => hasChildren), deleteComment: jest.fn(async () => undefined), + // #338 F1: the childless ephemeral delete is now atomic-conditional and + // returns the number of rows removed (1 = deleted, 0 = a reply raced in). + deleteCommentIfChildless: jest.fn(async () => deletedRows), }; const pageRepo: any = {}; const wsService: any = { emitCommentEvent: jest.fn() }; @@ -71,8 +74,8 @@ describe('CommentService — dismissSuggestion', () => { expect.anything(), expect.anything(), ); - // Hard-delete + strip mark. - expect(commentRepo.deleteComment).toHaveBeenCalledWith('c-1'); + // Hard-delete (atomic-conditional) + strip mark. + expect(commentRepo.deleteCommentIfChildless).toHaveBeenCalledWith('c-1'); expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith( 'deleteCommentMark', 'page.page-1', @@ -108,7 +111,7 @@ describe('CommentService — dismissSuggestion', () => { service.dismissSuggestion(suggestionComment(), user()), ).rejects.toThrow(/live collaboration/); - expect(commentRepo.deleteComment).not.toHaveBeenCalled(); + expect(commentRepo.deleteCommentIfChildless).not.toHaveBeenCalled(); expect(wsService.emitCommentEvent).not.toHaveBeenCalledWith( expect.anything(), expect.anything(), @@ -148,6 +151,38 @@ describe('CommentService — dismissSuggestion', () => { expect(result.outcome).toBe('resolved'); }); + it('reply races in after the childless read (conditional delete → 0 rows) → resolves instead, does NOT hard-delete, reply survives, outcome=resolved (#338 F1)', async () => { + // hasChildren=false selects the ephemeral branch (the read saw no replies), + // but the atomic delete matches 0 rows because a reply landed in the window + // between that read and the delete. The parent must NOT be hard-deleted + // (a cascade would destroy the just-added reply); the thread is resolved. + const { service, commentRepo, wsService, collaborationGateway } = + makeService(false, 0); + + const result = await service.dismissSuggestion(suggestionComment(), user()); + + // The conditional delete was attempted (and matched nothing). + expect(commentRepo.deleteCommentIfChildless).toHaveBeenCalledWith('c-1'); + // No commentDeleted broadcast — the row (and the racing reply) survive. + expect(wsService.emitCommentEvent).not.toHaveBeenCalledWith( + expect.anything(), + expect.anything(), + expect.objectContaining({ operation: 'commentDeleted' }), + ); + // Fell back to resolving the thread. + const resolvePatch = commentRepo.updateComment.mock.calls + .map((c: any[]) => c[0]) + .find((p: any) => 'resolvedAt' in p); + expect(resolvePatch.resolvedAt).toBeInstanceOf(Date); + expect(resolvePatch.resolvedById).toBe('user-1'); + expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith( + 'resolveCommentMark', + 'page.page-1', + expect.objectContaining({ commentId: 'c-1', resolved: true }), + ); + expect(result.outcome).toBe('resolved'); + }); + it('rejects a reply (non-top-level) comment', async () => { const { service, commentRepo } = makeService(); await expect( diff --git a/apps/server/src/core/comment/comment.service.ts b/apps/server/src/core/comment/comment.service.ts index aad0ef9c..812737d9 100644 --- a/apps/server/src/core/comment/comment.service.ts +++ b/apps/server/src/core/comment/comment.service.ts @@ -516,8 +516,10 @@ export class CommentService { return { ...updatedComment, outcome: 'resolved' }; } - // Ephemeral: no replies → the suggestion vanishes entirely. - await this.deleteEphemeralSuggestion(comment, user); + // Ephemeral: no replies → the suggestion vanishes entirely. The atomic + // conditional delete may still fall back to a resolve if a reply raced in + // (see deleteEphemeralSuggestion), so the outcome is whatever it settled on. + const settled = await this.deleteEphemeralSuggestion(comment, user, provenance); this.auditService.log({ event: AuditEvent.COMMENT_SUGGESTION_DISMISSED, resourceType: AuditResource.COMMENT, @@ -525,7 +527,7 @@ export class CommentService { spaceId: comment.spaceId, metadata: { pageId: comment.pageId }, }); - return { ...comment, outcome: 'deleted' }; + return settled; } /** @@ -592,7 +594,9 @@ export class CommentService { // the comment is redundant. Hard-delete it and strip its inline anchor. We // deliberately do NOT write the applied stamps first (the row is about to be // deleted); the audit event still records that the suggestion was applied. - await this.deleteEphemeralSuggestion(comment, user); + // The delete is atomic-conditional: if a reply raced in after the + // hasChildren read, it falls back to resolving instead (outcome 'resolved'). + const settled = await this.deleteEphemeralSuggestion(comment, user, provenance); this.auditService.log({ event: AuditEvent.COMMENT_SUGGESTION_APPLIED, @@ -602,13 +606,13 @@ export class CommentService { metadata: { pageId: comment.pageId }, }); - return { ...comment, outcome: 'deleted' }; + return settled; } /** - * Hard-delete an ephemeral suggestion comment and remove its inline `comment` - * anchor mark from the collaborative document, then broadcast the deletion. - * Shared by the apply/dismiss no-replies branches (#329). + * Settle an ephemeral suggestion whose thread looked childless: remove its + * inline `comment` anchor mark, then ATOMICALLY hard-delete the row only if it + * is still childless. Shared by the apply/dismiss no-replies branches (#329). * * ORDER MATTERS: the anchor mark is removed FIRST and FATALLY (mirrors * applySuggestion, which mutates the doc before writing the DB). The row @@ -618,19 +622,51 @@ export class CommentService { * anchor pointing at a comment that no longer exists (the exact data-integrity * bug #329 targets). Let the exception propagate (→ 5xx); the operation is * then repeatable with row + mark still consistent. + * + * RACE (#338 F1): the caller read `hasChildren` BEFORE the (slow) mark + * removal, so a reply can land in that window. `comments.parent_comment_id` is + * ON DELETE CASCADE, so an unconditional delete here would cascade-destroy the + * just-added reply forever. Instead we use `deleteCommentIfChildless`, which + * re-checks childlessness inside the delete statement. If it removes the row + * (outcome 'deleted') we broadcast the deletion as before. If it removes 0 + * rows (a reply interleaved) we do NOT hard-delete — we resolve the thread + * instead (outcome 'resolved'), preserving the discussion and the new reply. + * The anchor mark is already gone by then, an accepted degradation: the thread + * lands in the resolved tab without its inline highlight — far better than + * losing a reply. */ private async deleteEphemeralSuggestion( comment: Comment, user: User, - ): Promise { + provenance?: AuthProvenanceData, + ): Promise { await this.deleteCommentMark(comment, user); - await this.commentRepo.deleteComment(comment.id); - this.wsService.emitCommentEvent(comment.spaceId, comment.pageId, { - operation: 'commentDeleted', - pageId: comment.pageId, - commentId: comment.id, - }); + const deletedRows = await this.commentRepo.deleteCommentIfChildless( + comment.id, + ); + + if (deletedRows > 0) { + this.wsService.emitCommentEvent(comment.spaceId, comment.pageId, { + operation: 'commentDeleted', + pageId: comment.pageId, + commentId: comment.id, + }); + return { ...comment, outcome: 'deleted' }; + } + + // A reply interleaved between the hasChildren read and this delete, so the + // conditional delete matched nothing. Preserve the discussion + the new + // reply by resolving the thread instead of hard-deleting it. resolveComment + // handles the resolve patch, its ws broadcast and the resolve notification; + // its collab call is best-effort, so the already-stripped mark is fine. + const resolvedComment = await this.resolveComment( + comment, + true, + user, + provenance, + ); + return { ...resolvedComment, outcome: 'resolved' }; } /** diff --git a/apps/server/src/database/repos/comment/comment.repo.ts b/apps/server/src/database/repos/comment/comment.repo.ts index b2df6b62..29d3a6e5 100644 --- a/apps/server/src/database/repos/comment/comment.repo.ts +++ b/apps/server/src/database/repos/comment/comment.repo.ts @@ -139,6 +139,37 @@ export class CommentRepo { await this.db.deleteFrom('comments').where('id', '=', commentId).execute(); } + /** + * Atomic conditional delete for an ephemeral suggestion (#338 / #329 F1): + * delete the row ONLY if it is still childless, in a single statement, and + * return the number of rows removed (0 or 1). This closes a race: dismiss/apply + * read `hasChildren`, then remove the anchor mark (a collab round-trip of + * tens-to-hundreds of ms), then delete. `comments.parent_comment_id` is + * ON DELETE CASCADE, so if a reply lands in that window an unconditional delete + * would cascade-delete the just-added reply forever. The `NOT EXISTS` re-checks + * childlessness at delete time inside the same statement, so an interleaved + * reply makes this delete 0 rows and the caller can fall back to resolving the + * thread instead of destroying the discussion. + */ + async deleteCommentIfChildless(commentId: string): Promise { + const result = await this.db + .deleteFrom('comments') + .where('id', '=', commentId) + .where((eb) => + eb.not( + eb.exists( + eb + .selectFrom('comments as child') + .select('child.id') + .whereRef('child.parentCommentId', '=', 'comments.id'), + ), + ), + ) + .executeTakeFirst(); + + return Number(result?.numDeletedRows ?? 0n); + } + async hasChildren(commentId: string): Promise { const result = await this.db .selectFrom('comments')