From d7713cb57537a484e0dbbe34450a7dab61bd952d Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 4 Jul 2026 19:04:25 +0300 Subject: [PATCH] fix(comment): transactional childless-delete race fix + client dismiss gate + DB int-spec (#329 review round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F4 [critical] — the anti-join `DELETE … WHERE NOT EXISTS(child)` was still racy under Postgres READ COMMITTED: a reply INSERT holds FOR KEY SHARE on the parent; the DELETE's start snapshot doesn't see the uncommitted child (NOT EXISTS true), blocks on the reply's lock, and when the reply commits the parent was only LOCKED (not modified) so EvalPlanQual does NOT re-check → the DELETE proceeds and CASCADE destroys the just-committed reply. Replaced with a transaction: SELECT the parent FOR UPDATE (conflicts with the reply's FOR KEY SHARE → serializes the concurrent reply), re-check for a child with a FRESH statement in the same tx (a new RC snapshot sees a just-committed reply), delete only if still childless (return 1) else return 0 (caller resolves). The FOR UPDATE lock is held to end-of-tx so no reply can insert between the re-check and the delete. Signature unchanged, so the service + its mocked unit tests are untouched; docstrings updated. F5 [warning] — the client Dismiss button was gated only on canComment, but the server now gates dismiss on owner-or-space-admin, so a non-owner non-admin saw a button the server 403s. `canShowDismiss` now also requires `isOwnerOrAdmin = currentUser?.user?.id === comment.creatorId || userSpaceRole === "admin"` (the same gate the comment delete-menu already uses); threaded into both call sites. F6 [warning] — added a REAL-DB int-spec (apps/server/test/integration/comment-delete-if-childless.int-spec.ts, + a createComment seeder): (a) childless → returns 1, row gone; (b) committed reply → returns 0, parent+reply survive; (c) CONCURRENCY — a second connection inserts a reply (FOR KEY SHARE) and commits mid-operation while deleteCommentIfChildless blocks on FOR UPDATE → asserts it returns 0 and both rows survive (a blind anti-join would lose the reply here). Ran against live Postgres — 3/3 pass. server tsc clean; comment jest 53 + int-spec 3 (live Postgres) pass. client tsc clean; comment vitest 56 pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../components/comment-list-item.test.tsx | 52 ++++-- .../comment/components/comment-list-item.tsx | 12 +- .../src/features/comment/utils/suggestion.ts | 17 +- .../src/core/comment/comment.service.ts | 17 +- .../database/repos/comment/comment.repo.ts | 78 ++++++--- .../comment-delete-if-childless.int-spec.ts | 160 ++++++++++++++++++ apps/server/test/integration/db.ts | 34 ++++ 7 files changed, 313 insertions(+), 57 deletions(-) create mode 100644 apps/server/test/integration/comment-delete-if-childless.int-spec.ts diff --git a/apps/client/src/features/comment/components/comment-list-item.test.tsx b/apps/client/src/features/comment/components/comment-list-item.test.tsx index 99164111..eb35c601 100644 --- a/apps/client/src/features/comment/components/comment-list-item.test.tsx +++ b/apps/client/src/features/comment/components/comment-list-item.test.tsx @@ -46,7 +46,12 @@ const baseComment = (over?: Partial): IComment => ...over, }) as IComment; -function renderItem(comment: IComment, canEdit = true, canComment = true) { +function renderItem( + comment: IComment, + canEdit = true, + canComment = true, + userSpaceRole?: string, +) { return render( , ); @@ -173,38 +179,49 @@ describe("CommentListItem — dismiss suggestion (#329)", () => { ...over, }); - it("renders a Dismiss button alongside Apply when canEdit and canComment", () => { - renderItem(suggestion(), true, true); + // A space admin (userSpaceRole="admin") satisfies the owner-or-admin gate + // regardless of who authored the comment; the tests below use it as the lever + // since the currentUser atom is unseeded (null) in this harness. + it("renders a Dismiss button alongside Apply when canEdit and canComment (owner/admin)", () => { + renderItem(suggestion(), true, true, "admin"); expect(screen.getByRole("button", { name: "Apply" })).toBeDefined(); expect(screen.getByRole("button", { name: "Dismiss" })).toBeDefined(); }); - it("shows Dismiss but NOT Apply for a commenter who cannot edit", () => { - renderItem(suggestion(), false, true); + it("shows Dismiss but NOT Apply for an admin commenter who cannot edit", () => { + renderItem(suggestion(), false, true, "admin"); expect(screen.queryByRole("button", { name: "Apply" })).toBeNull(); expect(screen.getByRole("button", { name: "Dismiss" })).toBeDefined(); }); it("hides Dismiss when the viewer cannot comment", () => { - renderItem(suggestion(), false, false); + renderItem(suggestion(), false, false, "admin"); expect(screen.queryByRole("button", { name: "Dismiss" })).toBeNull(); expect(screen.queryByRole("button", { name: "Apply" })).toBeNull(); }); + it("hides Dismiss for a non-owner non-admin even with canComment (#338 F5: mirrors server 403)", () => { + // canComment=true but NOT a space admin and NOT the comment owner (the + // currentUser atom is null while the comment is authored by user-1), so the + // server would 403 a dismiss — the button must not be shown at all. + renderItem(suggestion(), false, true, "member"); + expect(screen.queryByRole("button", { name: "Dismiss" })).toBeNull(); + }); + it("hides Dismiss once the thread is resolved", () => { - renderItem(suggestion({ resolvedAt: new Date() }), true, true); + renderItem(suggestion({ resolvedAt: new Date() }), true, true, "admin"); expect(screen.queryByRole("button", { name: "Dismiss" })).toBeNull(); }); it("hides Dismiss (shows the Applied badge) once applied", () => { - renderItem(suggestion({ suggestionAppliedAt: new Date() }), true, true); + renderItem(suggestion({ suggestionAppliedAt: new Date() }), true, true, "admin"); expect(screen.queryByRole("button", { name: "Dismiss" })).toBeNull(); expect(screen.getByText("Applied")).toBeDefined(); }); it("calls the dismiss mutation when the Dismiss button is clicked", () => { dismissMutateAsync.mockClear(); - renderItem(suggestion(), true, true); + renderItem(suggestion(), true, true, "admin"); fireEvent.click(screen.getByRole("button", { name: "Dismiss" })); expect(dismissMutateAsync).toHaveBeenCalledWith({ commentId: "c-1", @@ -243,24 +260,27 @@ describe("canShowDismiss predicate", () => { const c = (over?: Partial): IComment => ({ suggestedText: "x", ...over }) as IComment; - it("true when suggestion present, can comment, not applied/resolved, top-level", () => { - expect(canShowDismiss(c(), true)).toBe(true); + it("true when suggestion present, can comment, owner/admin, not applied/resolved, top-level", () => { + expect(canShowDismiss(c(), true, true)).toBe(true); }); it("false without comment permission", () => { - expect(canShowDismiss(c(), false)).toBe(false); + expect(canShowDismiss(c(), false, true)).toBe(false); + }); + it("false when not owner and not admin (#338 F5)", () => { + expect(canShowDismiss(c(), true, false)).toBe(false); }); it("false when no suggestion", () => { - expect(canShowDismiss(c({ suggestedText: null }), true)).toBe(false); + expect(canShowDismiss(c({ suggestedText: null }), true, true)).toBe(false); }); it("false when already applied", () => { - expect(canShowDismiss(c({ suggestionAppliedAt: new Date() }), true)).toBe( + expect(canShowDismiss(c({ suggestionAppliedAt: new Date() }), true, true)).toBe( false, ); }); it("false when resolved", () => { - expect(canShowDismiss(c({ resolvedAt: new Date() }), true)).toBe(false); + expect(canShowDismiss(c({ resolvedAt: new Date() }), true, true)).toBe(false); }); it("false for a reply comment", () => { - expect(canShowDismiss(c({ parentCommentId: "p" }), true)).toBe(false); + expect(canShowDismiss(c({ parentCommentId: "p" }), true, true)).toBe(false); }); }); diff --git a/apps/client/src/features/comment/components/comment-list-item.tsx b/apps/client/src/features/comment/components/comment-list-item.tsx index ff62bdd3..6bda2794 100644 --- a/apps/client/src/features/comment/components/comment-list-item.tsx +++ b/apps/client/src/features/comment/components/comment-list-item.tsx @@ -56,6 +56,12 @@ function CommentListItem({ const [currentUser] = useAtom(currentUserAtom); const createdAtAgo = useTimeAgo(comment.createdAt); + // Owner-or-space-admin gate (#338): mirrors the server authz for both the + // comment menu (edit/delete) and the suggestion Dismiss button, so we never + // render an action the server will 403. + const isOwnerOrAdmin = + currentUser?.user?.id === comment.creatorId || userSpaceRole === "admin"; + useEffect(() => { setContent(comment.content); }, [comment]); @@ -205,7 +211,7 @@ function CommentListItem({ /> )} - {(currentUser?.user?.id === comment.creatorId || userSpaceRole === 'admin') && ( + {isOwnerOrAdmin && ( ) : ( (canShowApply(comment, canEdit) || - canShowDismiss(comment, canComment)) && ( + canShowDismiss(comment, canComment, isOwnerOrAdmin)) && ( {canShowApply(comment, canEdit) && (