vvzvlad/gitmost
1
0
Fork 0
Code Issues 16 Pull Requests 10 Packages Projects Releases Activity

fix(comment): transactional childless-delete race fix + client dismiss gate + DB int-spec (#329 review round 2)

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
claude code agent 227
2026-07-04 19:04:25 +03:00
parent e6d8eda8e5
commit d7fa6738e5
7 changed files with 313 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/client/src/features/comment/components/comment-list-item.test.tsx
+36 -16
View File
@@ -46,7 +46,12 @@ const baseComment = (over?: Partial<IComment>): IComment =>
...over,
}) as IComment;
function renderItem(comment: IComment, canEdit = true, canComment = true) {
function renderItem(
comment: IComment,
canEdit = true,
canComment = true,
userSpaceRole?: string,
) {
return render(
<MantineProvider>
<CommentListItem
@@ -54,6 +59,7 @@ function renderItem(comment: IComment, canEdit = true, canComment = true) {
pageId="page-1"
canComment={canComment}
canEdit={canEdit}
userSpaceRole={userSpaceRole}
/>
</MantineProvider>,
);
@@ -175,38 +181,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",
@@ -245,24 +262,27 @@ describe("canShowDismiss predicate", () => {
const c = (over?: Partial<IComment>): 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);
});
});
apps/client/src/features/comment/components/comment-list-item.tsx
+9 -3
View File
@@ -72,6 +72,12 @@ function CommentListItem({
[comment.selection, comment.suggestedText],
);
// 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]);
@@ -221,7 +227,7 @@ function CommentListItem({
/>
)}
{(currentUser?.user?.id === comment.creatorId || userSpaceRole === 'admin') && (
{isOwnerOrAdmin && (
<CommentMenu
onEditComment={handleEditToggle}
onDeleteComment={handleDeleteComment}
@@ -303,7 +309,7 @@ function CommentListItem({
</Badge>
) : (
(canShowApply(comment, canEdit) ||
canShowDismiss(comment, canComment)) && (
canShowDismiss(comment, canComment, isOwnerOrAdmin)) && (
<Group gap="xs" mt={6}>
{canShowApply(comment, canEdit) && (
<Button
@@ -322,7 +328,7 @@ function CommentListItem({
)}
{/* Dismiss ("Не применять", #329): removes the suggestion
without changing the page text. Gated on canComment. */}
{canShowDismiss(comment, canComment) && (
{canShowDismiss(comment, canComment, isOwnerOrAdmin) && (
<Button
size="compact-xs"
variant="subtle"
apps/client/src/features/comment/utils/suggestion.ts
+12 -5
View File
@@ -117,13 +117,20 @@ export function computeSuggestionDiff(
}
// Whether the suggested-edit (#329) "Не применять" (Dismiss) button should be
// shown. Dismiss does NOT change the page text, so it is gated on canComment
// (looser than canEdit): a viewer allowed to comment but not edit can still
// dismiss a suggestion. Same not-applied/not-resolved/top-level conditions as
// Apply.
export function canShowDismiss(comment: IComment, canComment?: boolean): boolean {
// shown. Dismiss does NOT change the page text (so it needs only canComment, not
// canEdit), BUT a childless dismiss IRREVERSIBLY hard-deletes the comment, so the
// server gates it on comment-owner-OR-space-admin (#338 F5). The button must
// mirror that authz or a non-owner non-admin sees a live Dismiss that always
// 403s → red error. Hence isOwnerOrAdmin is required IN ADDITION to canComment.
// Same not-applied/not-resolved/top-level conditions as Apply.
export function canShowDismiss(
comment: IComment,
canComment?: boolean,
isOwnerOrAdmin?: boolean,
): boolean {
return Boolean(
canComment &&
isOwnerOrAdmin &&
comment.suggestedText &&
!comment.suggestionAppliedAt &&
!comment.resolvedAt &&