From 8d8ecaed82a2586e3788853342775096f0825f20 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 4 Jul 2026 15:33:44 +0300 Subject: [PATCH 1/3] =?UTF-8?q?feat(comment):=20ephemeral=20suggestion-edi?= =?UTF-8?q?ts=20=E2=80=94=20Apply/Dismiss=20remove=20the=20comment=20(#329?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agent suggestion-edits (comments with suggestedText, #315) piled up: Apply auto-resolved the thread, cluttering the resolved tab, and the anchors stayed in the document. Make them ephemeral: resolving (Apply OR the new Dismiss) makes the comment DISAPPEAR — hard-delete + remove the Yjs `comment` mark — UNLESS the thread has replies, in which case resolve it (preserve the discussion). Manual Resolve is unchanged. Scope: only comments with `suggestedText`. Server: - New collab event `deleteCommentMark` (collaboration.handler) mirroring resolveCommentMark, wiring the existing removeYjsMarkByAttribute to strip the anchor from the doc. - `finalizeAppliedSuggestion` forks on `hasChildren`: replies → apply + resolve (outcome 'resolved'); none → apply + hard-delete + mark removal (outcome 'deleted'). - New `dismissSuggestion` (validates top-level + suggestedText + not applied/not resolved) with the same fork; permission `canComment` (NOT canEdit — dismiss doesn't change page text); audit COMMENT_SUGGESTION_DISMISSED. New POST /comments/dismiss-suggestion; apply stays canEdit. - Both return `{ outcome: 'deleted' | 'resolved' }` so the client picks the optimistic action. Data-integrity (review F1): the shared `deleteEphemeralSuggestion` removes the anchor mark FIRST and FATALLY, then deletes the DB row only on success. The row delete is irreversible, so a mark-removal failure — including the COLLAB_DISABLE_REDIS "no live instance" hard-error — must abort the whole operation (→ 5xx, repeatable) rather than swallow the error and leave a permanent orphan anchor pointing at a deleted comment. `deleteCommentMark` is no longer best-effort (unlike resolve, where the row is kept and a failed mark is recoverable). Client: - `canShowDismiss` (canComment) alongside `canShowApply` (canEdit); a "Dismiss" button next to Apply in the suggestion block. - `useApplySuggestionMutation`/`useDismissSuggestionMutation` reconcile the cache on `outcome` ('deleted' → remove; 'resolved' → relocate to the resolved tab). - Idempotent races (review F2): BOTH apply and dismiss onError reduce 404/400 to success (comment already gone/resolved), dropping it from the cache instead of a red error — restores the #315 apply idempotency the ephemeral delete would otherwise break. - i18n Dismiss / "Не применять" (ru/en). Not done (flagged): deleteCommentMark on the normal /comments/delete path — left out (would change every non-suggestion delete + needs gateway injection; the interactive client already strips the mark via unsetComment). Out of scope per the issue. Tests: server — apply/dismiss delete-vs-resolve fork, all four dismiss state guards, the deleteCommentMark handler, controller authz (dismiss=canComment, apply=canEdit), AND a mark-removal-failure test proving the row is NOT deleted + the error propagates (F1). client — Dismiss show-conditions, outcome cache reconciliation, and 404 idempotent race for BOTH dismiss and apply (F2). Verified: server tsc clean; comment+collaboration jest 144 passed. client tsc clean; vitest 905 passed | 1 expected-fail. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../public/locales/en-US/translation.json | 5 +- .../public/locales/ru-RU/translation.json | 5 +- .../components/comment-list-item.test.tsx | 88 ++++++- .../comment/components/comment-list-item.tsx | 64 +++++- .../comment-query.suggestion-outcome.test.tsx | 205 +++++++++++++++++ .../features/comment/queries/comment-query.ts | 155 +++++++++++-- .../comment/services/comment-service.ts | 14 +- .../features/comment/types/comment.types.ts | 9 + .../src/features/comment/utils/suggestion.ts | 15 ++ .../collaboration.handler.spec.ts | 56 +++++ .../collaboration/collaboration.handler.ts | 35 +++ apps/server/src/common/events/audit-events.ts | 1 + .../core/comment/comment.controller.spec.ts | 142 ++++++++++++ .../src/core/comment/comment.controller.ts | 34 +++ .../comment.service.apply-suggestion.spec.ts | 153 ++++++++----- ...comment.service.dismiss-suggestion.spec.ts | 194 ++++++++++++++++ .../src/core/comment/comment.service.ts | 214 +++++++++++++++--- .../comment/dto/dismiss-suggestion.dto.ts | 6 + 18 files changed, 1267 insertions(+), 128 deletions(-) create mode 100644 apps/client/src/features/comment/queries/comment-query.suggestion-outcome.test.tsx create mode 100644 apps/server/src/core/comment/comment.service.dismiss-suggestion.spec.ts create mode 100644 apps/server/src/core/comment/dto/dismiss-suggestion.dto.ts diff --git a/apps/client/public/locales/en-US/translation.json b/apps/client/public/locales/en-US/translation.json index 55c250ca..89988d9b 100644 --- a/apps/client/public/locales/en-US/translation.json +++ b/apps/client/public/locales/en-US/translation.json @@ -1382,5 +1382,8 @@ "Applied": "Applied", "Suggestion applied": "Suggestion applied", "Failed to apply suggestion": "Failed to apply suggestion", - "The commented text changed since this suggestion was made; it was not applied.": "The commented text changed since this suggestion was made; it was not applied." + "The commented text changed since this suggestion was made; it was not applied.": "The commented text changed since this suggestion was made; it was not applied.", + "Dismiss": "Dismiss", + "Suggestion dismissed": "Suggestion dismissed", + "Failed to dismiss suggestion": "Failed to dismiss suggestion" } diff --git a/apps/client/public/locales/ru-RU/translation.json b/apps/client/public/locales/ru-RU/translation.json index 2a25d0da..0c792632 100644 --- a/apps/client/public/locales/ru-RU/translation.json +++ b/apps/client/public/locales/ru-RU/translation.json @@ -1245,5 +1245,8 @@ "Applied": "Применено", "Suggestion applied": "Предложение применено", "Failed to apply suggestion": "Не удалось применить предложение", - "The commented text changed since this suggestion was made; it was not applied.": "Прокомментированный текст изменился после создания предложения; оно не было применено." + "The commented text changed since this suggestion was made; it was not applied.": "Прокомментированный текст изменился после создания предложения; оно не было применено.", + "Dismiss": "Не применять", + "Suggestion dismissed": "Предложение отклонено", + "Failed to dismiss suggestion": "Не удалось отклонить предложение" } 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 ceb9cbc0..37450ac4 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 @@ -8,6 +8,7 @@ import { IComment } from "@/features/comment/types/comment.types"; // The comment mutation hooks reach out to react-query/network — stub them so the // component renders in isolation. We only assert the AI-badge rendering branch. const applyMutateAsync = vi.fn(); +const dismissMutateAsync = vi.fn(); vi.mock("@/features/comment/queries/comment-query", () => ({ useDeleteCommentMutation: () => ({ mutateAsync: vi.fn() }), useResolveCommentMutation: () => ({ mutateAsync: vi.fn() }), @@ -16,6 +17,10 @@ vi.mock("@/features/comment/queries/comment-query", () => ({ mutateAsync: applyMutateAsync, isPending: false, }), + useDismissSuggestionMutation: () => ({ + mutateAsync: dismissMutateAsync, + isPending: false, + }), })); // CommentEditor pulls in the full TipTap editor stack; replace it with a stub. @@ -24,7 +29,10 @@ vi.mock("@/features/comment/components/comment-editor", () => ({ })); import CommentListItem from "./comment-list-item"; -import { canShowApply } from "@/features/comment/utils/suggestion"; +import { + canShowApply, + canShowDismiss, +} from "@/features/comment/utils/suggestion"; const baseComment = (over?: Partial): IComment => ({ @@ -38,13 +46,13 @@ const baseComment = (over?: Partial): IComment => ...over, }) as IComment; -function renderItem(comment: IComment, canEdit = true) { +function renderItem(comment: IComment, canEdit = true, canComment = true) { return render( , @@ -159,6 +167,54 @@ describe("CommentListItem — suggested edit (#315)", () => { }); }); +describe("CommentListItem — dismiss suggestion (#329)", () => { + const suggestion = (over?: Partial): IComment => + baseComment({ + selection: "old wording here", + suggestedText: "new wording here", + ...over, + }); + + it("renders a Dismiss button alongside Apply when canEdit and canComment", () => { + renderItem(suggestion(), true, true); + 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); + 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); + expect(screen.queryByRole("button", { name: "Dismiss" })).toBeNull(); + expect(screen.queryByRole("button", { name: "Apply" })).toBeNull(); + }); + + it("hides Dismiss once the thread is resolved", () => { + renderItem(suggestion({ resolvedAt: new Date() }), true, true); + expect(screen.queryByRole("button", { name: "Dismiss" })).toBeNull(); + }); + + it("hides Dismiss (shows the Applied badge) once applied", () => { + renderItem(suggestion({ suggestionAppliedAt: new Date() }), true, true); + 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); + fireEvent.click(screen.getByRole("button", { name: "Dismiss" })); + expect(dismissMutateAsync).toHaveBeenCalledWith({ + commentId: "c-1", + pageId: "page-1", + }); + }); +}); + describe("canShowApply predicate", () => { const c = (over?: Partial): IComment => ({ suggestedText: "x", ...over }) as IComment; @@ -184,3 +240,29 @@ describe("canShowApply predicate", () => { expect(canShowApply(c({ parentCommentId: "p" }), true)).toBe(false); }); }); + +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("false without comment permission", () => { + expect(canShowDismiss(c(), false)).toBe(false); + }); + it("false when no suggestion", () => { + expect(canShowDismiss(c({ suggestedText: null }), true)).toBe(false); + }); + it("false when already applied", () => { + expect(canShowDismiss(c({ suggestionAppliedAt: new Date() }), true)).toBe( + false, + ); + }); + it("false when resolved", () => { + expect(canShowDismiss(c({ resolvedAt: new Date() }), true)).toBe(false); + }); + it("false for a reply comment", () => { + expect(canShowDismiss(c({ parentCommentId: "p" }), 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 0e397245..dc1b1c61 100644 --- a/apps/client/src/features/comment/components/comment-list-item.tsx +++ b/apps/client/src/features/comment/components/comment-list-item.tsx @@ -13,12 +13,14 @@ import { useHover } from "@mantine/hooks"; import { useApplySuggestionMutation, useDeleteCommentMutation, + useDismissSuggestionMutation, useResolveCommentMutation, useUpdateCommentMutation, } from "@/features/comment/queries/comment-query"; import { IComment } from "@/features/comment/types/comment.types"; import { canShowApply, + canShowDismiss, computeSuggestionDiff, } from "@/features/comment/utils/suggestion"; import { CustomAvatar } from "@/components/ui/custom-avatar.tsx"; @@ -54,6 +56,7 @@ function CommentListItem({ const deleteCommentMutation = useDeleteCommentMutation(comment.pageId); const resolveCommentMutation = useResolveCommentMutation(); const applySuggestionMutation = useApplySuggestionMutation(); + const dismissSuggestionMutation = useDismissSuggestionMutation(); const [currentUser] = useAtom(currentUserAtom); const createdAtAgo = useTimeAgo(comment.createdAt); @@ -130,6 +133,19 @@ function CommentListItem({ } } + async function handleDismissSuggestion() { + try { + await dismissSuggestionMutation.mutateAsync({ + commentId: comment.id, + pageId: comment.pageId, + }); + } catch (error) { + // Idempotent races are reconciled to success in the mutation's onError; + // anything else surfaces there as a notification. + console.error("Failed to dismiss suggestion:", error); + } + } + function handleCommentClick(comment: IComment) { const el = document.querySelector( `.comment-mark[data-comment-id="${comment.id}"]`, @@ -286,18 +302,42 @@ function CommentListItem({ {t("Applied")} ) : ( - canShowApply(comment, canEdit) && ( - + (canShowApply(comment, canEdit) || + canShowDismiss(comment, canComment)) && ( + + {canShowApply(comment, canEdit) && ( + + )} + {/* Dismiss ("Не применять", #329): removes the suggestion + without changing the page text. Gated on canComment. */} + {canShowDismiss(comment, canComment) && ( + + )} + ) )} 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 new file mode 100644 index 00000000..6fe54bef --- /dev/null +++ b/apps/client/src/features/comment/queries/comment-query.suggestion-outcome.test.tsx @@ -0,0 +1,205 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import React from "react"; +import { renderHook, waitFor } from "@testing-library/react"; +import { + QueryClient, + QueryClientProvider, + InfiniteData, +} from "@tanstack/react-query"; + +/** + * Coverage for the ephemeral-suggestion (#329) cache reconciliation in + * useApplySuggestionMutation / useDismissSuggestionMutation: the mutations act on + * the server `outcome` — 'deleted' drops the comment from the local list, + * 'resolved' relocates it (by stamping resolvedAt, which the tabs split on). + */ + +vi.mock("@mantine/notifications", () => ({ + notifications: { show: vi.fn() }, +})); + +vi.mock("@/features/comment/services/comment-service", () => ({ + applySuggestion: vi.fn(), + dismissSuggestion: vi.fn(), + createComment: vi.fn(), + updateComment: vi.fn(), + deleteComment: vi.fn(), + resolveComment: vi.fn(), + getPageComments: vi.fn(), +})); + +import { + applySuggestion, + dismissSuggestion, +} from "@/features/comment/services/comment-service"; +import { + useApplySuggestionMutation, + useDismissSuggestionMutation, + RQ_KEY, +} from "@/features/comment/queries/comment-query"; +import { IComment } from "@/features/comment/types/comment.types"; + +const PAGE_ID = "page-1"; + +function seededClient(comment: IComment) { + const queryClient = new QueryClient({ + defaultOptions: { mutations: { retry: false } }, + }); + const seed: InfiniteData = { + pageParams: [undefined], + pages: [{ items: [comment], meta: { hasNextPage: false, nextCursor: null } }], + }; + queryClient.setQueryData(RQ_KEY(PAGE_ID), seed); + const wrapper = ({ children }: { children: React.ReactNode }) => ( + {children} + ); + return { queryClient, wrapper }; +} + +function items(queryClient: QueryClient): IComment[] { + const cache = queryClient.getQueryData(RQ_KEY(PAGE_ID)) as + | InfiniteData + | undefined; + return cache?.pages.flatMap((p) => p.items) ?? []; +} + +const comment = (over?: Partial): IComment => + ({ + id: "c-1", + pageId: PAGE_ID, + content: "{}", + creatorId: "u-1", + workspaceId: "ws-1", + createdAt: new Date(), + suggestedText: "new", + ...over, + }) as IComment; + +describe("useApplySuggestionMutation — outcome handling (#329)", () => { + beforeEach(() => vi.clearAllMocks()); + + it("outcome=deleted → removes the comment from the list", async () => { + vi.mocked(applySuggestion).mockResolvedValue({ + id: "c-1", + pageId: PAGE_ID, + outcome: "deleted", + } as any); + const { queryClient, wrapper } = seededClient(comment()); + + const { result } = renderHook(() => useApplySuggestionMutation(), { + wrapper, + }); + await result.current.mutateAsync({ commentId: "c-1", pageId: PAGE_ID }); + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + + expect(items(queryClient)).toHaveLength(0); + }); + + it("outcome=resolved → keeps the comment and stamps resolvedAt/applied fields", async () => { + const resolvedAt = new Date(); + vi.mocked(applySuggestion).mockResolvedValue({ + id: "c-1", + pageId: PAGE_ID, + outcome: "resolved", + resolvedAt, + resolvedById: "u-1", + resolvedBy: { id: "u-1", name: "A" }, + suggestionAppliedAt: resolvedAt, + suggestionAppliedById: "u-1", + } as any); + const { queryClient, wrapper } = seededClient(comment()); + + const { result } = renderHook(() => useApplySuggestionMutation(), { + wrapper, + }); + await result.current.mutateAsync({ commentId: "c-1", pageId: PAGE_ID }); + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + + const list = items(queryClient); + expect(list).toHaveLength(1); + expect(list[0].resolvedAt).toBe(resolvedAt); + expect(list[0].suggestionAppliedAt).toBe(resolvedAt); + }); +}); + +describe("useDismissSuggestionMutation — outcome handling (#329)", () => { + beforeEach(() => vi.clearAllMocks()); + + it("outcome=deleted → removes the comment from the list", async () => { + vi.mocked(dismissSuggestion).mockResolvedValue({ + id: "c-1", + pageId: PAGE_ID, + outcome: "deleted", + } as any); + const { queryClient, wrapper } = seededClient(comment()); + + const { result } = renderHook(() => useDismissSuggestionMutation(), { + wrapper, + }); + await result.current.mutateAsync({ commentId: "c-1", pageId: PAGE_ID }); + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + + expect(items(queryClient)).toHaveLength(0); + }); + + it("outcome=resolved → keeps the comment and stamps resolvedAt", async () => { + const resolvedAt = new Date(); + vi.mocked(dismissSuggestion).mockResolvedValue({ + id: "c-1", + pageId: PAGE_ID, + outcome: "resolved", + resolvedAt, + resolvedById: "u-1", + resolvedBy: { id: "u-1", name: "A" }, + } as any); + const { queryClient, wrapper } = seededClient(comment()); + + const { result } = renderHook(() => useDismissSuggestionMutation(), { + wrapper, + }); + await result.current.mutateAsync({ commentId: "c-1", pageId: PAGE_ID }); + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + + const list = items(queryClient); + expect(list).toHaveLength(1); + expect(list[0].resolvedAt).toBe(resolvedAt); + }); + + it("idempotent race (404) → treated as success, comment removed from the list", async () => { + vi.mocked(dismissSuggestion).mockRejectedValue({ + response: { status: 404 }, + }); + const { queryClient, wrapper } = seededClient(comment()); + + const { result } = renderHook(() => useDismissSuggestionMutation(), { + wrapper, + }); + // mutateAsync rejects even though onError reconciles the cache; swallow it. + await result.current + .mutateAsync({ commentId: "c-1", pageId: PAGE_ID }) + .catch(() => undefined); + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(items(queryClient)).toHaveLength(0); + }); + + it("APPLY idempotent race (404) → treated as success, comment removed from the list", async () => { + // After #329 an applied reply-less suggestion is hard-deleted, so a racing + // second apply hits 404 — must reconcile to success like dismiss, not a red + // error (restores the #315 apply idempotency). + vi.mocked(applySuggestion).mockRejectedValue({ + response: { status: 404 }, + }); + 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)); + + expect(items(queryClient)).toHaveLength(0); + }); +}); diff --git a/apps/client/src/features/comment/queries/comment-query.ts b/apps/client/src/features/comment/queries/comment-query.ts index a1942532..a573a071 100644 --- a/apps/client/src/features/comment/queries/comment-query.ts +++ b/apps/client/src/features/comment/queries/comment-query.ts @@ -8,6 +8,7 @@ import { applySuggestion, createComment, deleteComment, + dismissSuggestion, getPageComments, resolveComment, updateComment, @@ -16,6 +17,7 @@ import { ICommentParams, IComment, IResolveComment, + ISuggestionOutcome, } from "@/features/comment/types/comment.types"; import { notifications } from "@mantine/notifications"; import { IPagination } from "@/lib/types.ts"; @@ -177,40 +179,102 @@ function updateCommentInCache( }; } +function removeCommentFromCache( + cache: InfiniteData>, + commentId: string, +): InfiniteData> { + return { + ...cache, + pages: cache.pages.map((page) => ({ + ...page, + items: page.items.filter((comment) => comment.id !== commentId), + })), + }; +} + +// Reconcile the local comment cache with an ephemeral-suggestion outcome (#329) +// returned by apply/dismiss: 'deleted' → drop the comment (it disappeared); +// 'resolved' → the thread had replies and was resolved, so carry the resolved +// state through (which relocates it to the resolved tab). +function applySuggestionOutcomeToCache( + queryClient: ReturnType, + pageId: string, + commentId: string, + data: ISuggestionOutcome, +) { + const cache = queryClient.getQueryData(RQ_KEY(pageId)) as + | InfiniteData> + | undefined; + if (!cache) return; + + if (data.outcome === "deleted") { + queryClient.setQueryData(RQ_KEY(pageId), removeCommentFromCache(cache, commentId)); + return; + } + + // 'resolved' (or an older server that omits outcome): reflect the resolved + // state and the applied stamps (apply sets them; dismiss leaves them null). + queryClient.setQueryData( + RQ_KEY(pageId), + updateCommentInCache(cache, commentId, (comment) => ({ + ...comment, + suggestionAppliedAt: data.suggestionAppliedAt, + suggestionAppliedById: data.suggestionAppliedById, + resolvedAt: data.resolvedAt, + resolvedById: data.resolvedById, + resolvedBy: data.resolvedBy, + })), + ); +} + export function useApplySuggestionMutation() { const queryClient = useQueryClient(); const { t } = useTranslation(); - return useMutation({ + return useMutation< + ISuggestionOutcome, + any, + { commentId: string; pageId: string } + >({ // No optimistic update: apply can fail with 409 (the commented text drifted), // so we only mutate the cache once the server confirms. mutationFn: ({ commentId }) => applySuggestion(commentId), onSuccess: (data, variables) => { - const cache = queryClient.getQueryData( - RQ_KEY(variables.pageId), - ) as InfiniteData> | undefined; - - if (cache) { - queryClient.setQueryData( - RQ_KEY(variables.pageId), - updateCommentInCache(cache, variables.commentId, (comment) => ({ - ...comment, - suggestionAppliedAt: data.suggestionAppliedAt, - suggestionAppliedById: data.suggestionAppliedById, - // The server auto-resolves the thread on apply — carry that through. - resolvedAt: data.resolvedAt, - resolvedById: data.resolvedById, - resolvedBy: data.resolvedBy, - })), - ); - } + // Ephemeral (#329): the server hard-deletes the applied suggestion when the + // thread has no replies ('deleted') or resolves it when it does ('resolved'). + applySuggestionOutcomeToCache( + queryClient, + variables.pageId, + variables.commentId, + data, + ); notifications.show({ message: t("Suggestion applied") }); }, - onError: (err: any) => { + onError: (err: any, variables) => { + const status = err?.response?.status; + // Idempotent races (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) { + const cache = queryClient.getQueryData(RQ_KEY(variables.pageId)) as + | InfiniteData> + | undefined; + if (cache) { + queryClient.setQueryData( + RQ_KEY(variables.pageId), + removeCommentFromCache(cache, variables.commentId), + ); + } + notifications.show({ message: t("Suggestion applied") }); + 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 status = err?.response?.status; const currentText = err?.response?.data?.currentText; if (status === 409 && typeof currentText === "string") { const shortText = @@ -234,6 +298,55 @@ export function useApplySuggestionMutation() { }); } +export function useDismissSuggestionMutation() { + const queryClient = useQueryClient(); + const { t } = useTranslation(); + + return useMutation< + ISuggestionOutcome, + any, + { commentId: string; pageId: string } + >({ + mutationFn: ({ commentId }) => dismissSuggestion(commentId), + onSuccess: (data, variables) => { + // Ephemeral (#329): dismiss hard-deletes the suggestion when the thread has + // no replies ('deleted') or resolves it when it does ('resolved'). + applySuggestionOutcomeToCache( + queryClient, + variables.pageId, + variables.commentId, + data, + ); + + 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. + const status = err?.response?.status; + if (status === 404 || status === 400) { + const cache = queryClient.getQueryData(RQ_KEY(variables.pageId)) as + | InfiniteData> + | undefined; + if (cache) { + queryClient.setQueryData( + RQ_KEY(variables.pageId), + removeCommentFromCache(cache, variables.commentId), + ); + } + notifications.show({ message: t("Suggestion dismissed") }); + return; + } + notifications.show({ + message: t("Failed to dismiss suggestion"), + color: "red", + }); + }, + }); +} + export function useResolveCommentMutation() { const queryClient = useQueryClient(); const { t } = useTranslation(); diff --git a/apps/client/src/features/comment/services/comment-service.ts b/apps/client/src/features/comment/services/comment-service.ts index f536519a..49dd9508 100644 --- a/apps/client/src/features/comment/services/comment-service.ts +++ b/apps/client/src/features/comment/services/comment-service.ts @@ -3,6 +3,7 @@ import { ICommentParams, IComment, IResolveComment, + ISuggestionOutcome, } from "@/features/comment/types/comment.types"; import { IPagination } from "@/lib/types.ts"; @@ -18,13 +19,24 @@ export async function resolveComment(data: IResolveComment): Promise { return req.data; } -export async function applySuggestion(commentId: string): Promise { +export async function applySuggestion( + commentId: string, +): Promise { // Mirrors resolveComment: let axios reject on non-2xx so the mutation can read // the 409 body (`{ message, currentText }`) off err.response.data. const req = await api.post("/comments/apply-suggestion", { commentId }); return req.data.data ?? req.data; } +export async function dismissSuggestion( + commentId: string, +): Promise { + // Dismiss ("Не применять") a suggested edit (#329): the server hard-deletes + // the comment (or resolves it when it has replies) and returns the outcome. + const req = await api.post("/comments/dismiss-suggestion", { commentId }); + return req.data.data ?? req.data; +} + export async function updateComment( data: Partial, ): Promise { diff --git a/apps/client/src/features/comment/types/comment.types.ts b/apps/client/src/features/comment/types/comment.types.ts index ac4eac64..a4b97228 100644 --- a/apps/client/src/features/comment/types/comment.types.ts +++ b/apps/client/src/features/comment/types/comment.types.ts @@ -60,6 +60,15 @@ export interface IResolveComment { resolved: boolean; } +// Result of applying or dismissing an ephemeral suggested edit (#329). The +// server hard-deletes the comment (`deleted`) unless the thread has replies, in +// which case it is resolved (`resolved`). The returned comment fields carry the +// resolved-branch state; `outcome` tells the client which optimistic action to +// take (drop the comment vs. move it to the resolved tab). +export type ISuggestionOutcome = IComment & { + outcome?: "deleted" | "resolved"; +}; + export interface ICommentParams extends QueryParams { pageId: string; } diff --git a/apps/client/src/features/comment/utils/suggestion.ts b/apps/client/src/features/comment/utils/suggestion.ts index b353b876..8e308a3d 100644 --- a/apps/client/src/features/comment/utils/suggestion.ts +++ b/apps/client/src/features/comment/utils/suggestion.ts @@ -115,3 +115,18 @@ export function computeSuggestionDiff( return { old: oldSegments, new: newSegments }; } + +// 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 { + return Boolean( + canComment && + comment.suggestedText && + !comment.suggestionAppliedAt && + !comment.resolvedAt && + !comment.parentCommentId, + ); +} diff --git a/apps/server/src/collaboration/collaboration.handler.spec.ts b/apps/server/src/collaboration/collaboration.handler.spec.ts index 21f70325..73067023 100644 --- a/apps/server/src/collaboration/collaboration.handler.spec.ts +++ b/apps/server/src/collaboration/collaboration.handler.spec.ts @@ -130,3 +130,59 @@ describe('CollaborationHandler.applyCommentSuggestion', () => { expect(value).toBe(42); }); }); + +describe('CollaborationHandler.deleteCommentMark', () => { + it('strips the comment mark for the given commentId (ephemeral suggestion #329)', async () => { + const doc = buildDocWithComment('Hello world', 'c1'); + const { hocuspocus, connection } = fakeHocuspocus(doc); + const handler = new CollaborationHandler(); + const handlers = handler.getHandlers(hocuspocus); + + await handlers.deleteCommentMark('doc-1', { commentId: 'c1', user }); + + // The mark is gone; the text itself stays (deleting the anchor, not the run). + const xmlText = ( + doc.getXmlFragment('default').get(0) as Y.XmlElement + ).get(0) as Y.XmlText; + expect(xmlText.toDelta()).toEqual([{ insert: 'Hello world' }]); + expect(connection.transact).toHaveBeenCalledTimes(1); + expect(connection.disconnect).toHaveBeenCalledTimes(1); + }); + + it('routes the removal through removeYjsMarkByAttribute with the right args', async () => { + const doc = buildDocWithComment('abc', 'c9'); + const { hocuspocus } = fakeHocuspocus(doc); + const spy = jest.spyOn(yjsUtil, 'removeYjsMarkByAttribute'); + const handler = new CollaborationHandler(); + const handlers = handler.getHandlers(hocuspocus); + + await handlers.deleteCommentMark('doc-1', { commentId: 'c9', user }); + + expect(spy).toHaveBeenCalledWith( + doc.getXmlFragment('default'), + 'comment', + 'commentId', + 'c9', + ); + spy.mockRestore(); + }); + + it('leaves a different comment\'s mark intact', async () => { + const doc = buildDocWithComment('keep me', 'other'); + const { hocuspocus } = fakeHocuspocus(doc); + const handler = new CollaborationHandler(); + const handlers = handler.getHandlers(hocuspocus); + + await handlers.deleteCommentMark('doc-1', { commentId: 'c1', user }); + + const xmlText = ( + doc.getXmlFragment('default').get(0) as Y.XmlElement + ).get(0) as Y.XmlText; + expect(xmlText.toDelta()).toEqual([ + { + insert: 'keep me', + attributes: { comment: { commentId: 'other', resolved: false } }, + }, + ]); + }); +}); diff --git a/apps/server/src/collaboration/collaboration.handler.ts b/apps/server/src/collaboration/collaboration.handler.ts index 263adec4..c01cfd47 100644 --- a/apps/server/src/collaboration/collaboration.handler.ts +++ b/apps/server/src/collaboration/collaboration.handler.ts @@ -6,6 +6,7 @@ import { tiptapExtensions, } from './collaboration.util'; import { + removeYjsMarkByAttribute, replaceYjsMarkedText, setYjsMark, updateYjsMarkAttribute, @@ -78,6 +79,40 @@ export class CollaborationHandler { }, ); }, + deleteCommentMark: async ( + documentName: string, + payload: { + commentId: string; + user: User; + }, + ) => { + const { commentId, user } = payload; + // Ephemeral suggestions (#329): when a suggestion-edit is dismissed or an + // applied one has no replies, the comment is hard-deleted and its inline + // anchor must vanish too. Mirror resolveCommentMark exactly, but instead + // of flipping the mark's `resolved` attribute we STRIP the `comment` mark + // entirely via removeYjsMarkByAttribute so no orphan highlight remains in + // the collaborative document. + // + // Routing this through collaboration.gateway's handleYjsEvent means the + // COLLAB_DISABLE_REDIS path invokes this handler directly (never a silent + // no-op) and a missing live instance is a hard error — the same guarantee + // applyCommentSuggestion/resolveCommentMark rely on. + await this.withYdocConnection( + hocuspocus, + documentName, + { user }, + (doc) => { + const fragment = doc.getXmlFragment('default'); + removeYjsMarkByAttribute( + fragment, + 'comment', + 'commentId', + commentId, + ); + }, + ); + }, applyCommentSuggestion: async ( documentName: string, payload: { diff --git a/apps/server/src/common/events/audit-events.ts b/apps/server/src/common/events/audit-events.ts index b7cfabe5..bb377847 100644 --- a/apps/server/src/common/events/audit-events.ts +++ b/apps/server/src/common/events/audit-events.ts @@ -52,6 +52,7 @@ export const AuditEvent = { COMMENT_RESOLVED: 'comment.resolved', COMMENT_REOPENED: 'comment.reopened', COMMENT_SUGGESTION_APPLIED: 'comment.suggestion_applied', + COMMENT_SUGGESTION_DISMISSED: 'comment.suggestion_dismissed', // Page PAGE_CREATED: 'page.created', diff --git a/apps/server/src/core/comment/comment.controller.spec.ts b/apps/server/src/core/comment/comment.controller.spec.ts index 707e9687..3cdbf502 100644 --- a/apps/server/src/core/comment/comment.controller.spec.ts +++ b/apps/server/src/core/comment/comment.controller.spec.ts @@ -1,4 +1,5 @@ import { + BadRequestException, ForbiddenException, NotFoundException, } from '@nestjs/common'; @@ -117,3 +118,144 @@ describe('CommentController apply-suggestion authz', () => { expect(commentService.applySuggestion).not.toHaveBeenCalled(); }); }); + +/** + * Authz-gate tests for the dismiss-suggestion route (#329). Dismissing a + * suggestion does NOT change the page text, so it authorizes with + * validateCanComment (NOT validateCanEdit) — a viewer allowed to comment but not + * edit can still dismiss. The gate MUST run BEFORE the service (which performs + * the delete/resolve + mark removal). These tests pin that boundary. + */ +describe('CommentController dismiss-suggestion authz', () => { + function makeController() { + const commentService = { + dismissSuggestion: jest.fn(async () => ({ + id: 'c-1', + outcome: 'deleted', + })), + }; + const commentRepo = { findById: jest.fn() }; + const pageRepo = { findById: jest.fn() }; + const spaceAbility = {} as any; + const pageAccessService = { + validateCanComment: jest.fn(async () => undefined), + validateCanEdit: jest.fn(async () => undefined), + }; + const wsService = {} as any; + const auditService = { log: jest.fn() }; + + const controller = new CommentController( + commentService as any, + commentRepo as any, + pageRepo as any, + spaceAbility, + pageAccessService as any, + wsService, + auditService as any, + ); + return { + controller, + commentService, + commentRepo, + pageRepo, + pageAccessService, + }; + } + + const user: any = { id: 'u-1' }; + const workspace: any = { id: 'ws-1' }; + const provenance: any = undefined; + const dto: any = { commentId: 'c-1' }; + const comment = { + id: 'c-1', + pageId: 'p-1', + spaceId: 'sp-1', + suggestedText: 'new text', + selection: 'old text', + }; + const page = { id: 'p-1', spaceId: 'sp-1', deletedAt: null }; + + it('authorizes with validateCanComment (NOT validateCanEdit) then calls the service', async () => { + const { + controller, + commentRepo, + pageRepo, + pageAccessService, + commentService, + } = makeController(); + commentRepo.findById.mockResolvedValue(comment); + pageRepo.findById.mockResolvedValue(page); + const dismissed = { id: 'c-1', outcome: 'deleted' }; + commentService.dismissSuggestion.mockResolvedValue(dismissed); + + const result = await controller.dismissSuggestion( + dto, + user, + workspace, + provenance, + ); + + expect(pageAccessService.validateCanComment).toHaveBeenCalledWith( + page, + user, + workspace.id, + ); + // Dismiss must NOT require edit access. + expect(pageAccessService.validateCanEdit).not.toHaveBeenCalled(); + expect(commentService.dismissSuggestion).toHaveBeenCalledWith( + comment, + user, + provenance, + ); + expect(result).toBe(dismissed); + }); + + it('validateCanComment throwing Forbidden rejects AND dismissSuggestion is never called', async () => { + const { + controller, + commentRepo, + pageRepo, + pageAccessService, + commentService, + } = makeController(); + commentRepo.findById.mockResolvedValue(comment); + pageRepo.findById.mockResolvedValue(page); + pageAccessService.validateCanComment.mockRejectedValue( + new ForbiddenException('no comment access'), + ); + + await expect( + controller.dismissSuggestion(dto, user, workspace, provenance), + ).rejects.toBeInstanceOf(ForbiddenException); + + expect(commentService.dismissSuggestion).not.toHaveBeenCalled(); + }); + + it('missing comment: NotFound without authorizing or dismissing', async () => { + const { controller, commentRepo, pageRepo, pageAccessService, commentService } = + makeController(); + commentRepo.findById.mockResolvedValue(null); + + await expect( + controller.dismissSuggestion(dto, user, workspace, provenance), + ).rejects.toBeInstanceOf(NotFoundException); + + expect(pageRepo.findById).not.toHaveBeenCalled(); + expect(pageAccessService.validateCanComment).not.toHaveBeenCalled(); + expect(commentService.dismissSuggestion).not.toHaveBeenCalled(); + }); + + it('propagates a service BadRequest (e.g. already applied/resolved) unchanged', async () => { + const { controller, commentRepo, pageRepo, commentService } = + makeController(); + commentRepo.findById.mockResolvedValue(comment); + pageRepo.findById.mockResolvedValue(page); + commentService.dismissSuggestion.mockRejectedValue( + new BadRequestException('already applied'), + ); + + await expect( + controller.dismissSuggestion(dto, user, workspace, provenance), + ).rejects.toBeInstanceOf(BadRequestException); + }); +}); diff --git a/apps/server/src/core/comment/comment.controller.ts b/apps/server/src/core/comment/comment.controller.ts index f04f32e5..07c6267c 100644 --- a/apps/server/src/core/comment/comment.controller.ts +++ b/apps/server/src/core/comment/comment.controller.ts @@ -15,6 +15,7 @@ import { CreateCommentDto } from './dto/create-comment.dto'; import { UpdateCommentDto } from './dto/update-comment.dto'; import { ResolveCommentDto } from './dto/resolve-comment.dto'; import { ApplySuggestionDto } from './dto/apply-suggestion.dto'; +import { DismissSuggestionDto } from './dto/dismiss-suggestion.dto'; import { PageIdDto, CommentIdDto } from './dto/comments.input'; import { AuthUser } from '../../common/decorators/auth-user.decorator'; import { AuthWorkspace } from '../../common/decorators/auth-workspace.decorator'; @@ -234,6 +235,39 @@ export class CommentController { return this.commentService.applySuggestion(comment, user, provenance); } + @HttpCode(HttpStatus.OK) + @Post('dismiss-suggestion') + async dismissSuggestion( + @Body() dto: DismissSuggestionDto, + @AuthUser() user: User, + @AuthWorkspace() workspace: Workspace, + @AuthProvenance() provenance: AuthProvenanceData, + ) { + const comment = await this.commentRepo.findById(dto.commentId, { + includeCreator: true, + includeResolvedBy: true, + }); + if (!comment) { + throw new NotFoundException('Comment not found'); + } + + const page = await this.pageRepo.findById(comment.pageId); + if (!page || page.deletedAt) { + throw new NotFoundException('Page not found'); + } + + // 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. + await this.pageAccessService.validateCanComment(page, user, workspace.id); + + return this.commentService.dismissSuggestion(comment, user, provenance); + } + @HttpCode(HttpStatus.OK) @Post('delete') async delete(@Body() input: CommentIdDto, @AuthUser() user: User, @AuthWorkspace() workspace: Workspace) { 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 aa11f41e..8fa8ffa8 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 @@ -13,17 +13,24 @@ import { AuditEvent, AuditResource } from '../../common/events/audit-events'; * * The collaboration gateway verdict is the pivot of the whole flow, so each test * pins a specific { applied, currentText } and asserts the DB persistence, - * auto-resolve, audit, ws broadcast, and error mapping that follow from it. + * settle (ephemeral delete vs. resolve), audit, ws broadcast, and error mapping + * that follow from it. + * + * Ephemeral rule (#329): once applied a suggestion DISAPPEARS (hard-delete + + * strip the inline anchor mark) UNLESS the thread has replies, in which case it + * is resolved to preserve the discussion. `hasChildren` selects the branch. */ describe('CommentService — applySuggestion', () => { const UPDATED = { id: 'c-1', __updated: true } as any; - function makeService(verdict: unknown) { + function makeService(verdict: unknown, hasChildren = false) { const commentRepo: any = { // Both the applied-stamp re-read and resolveComment's re-read go through // findById; return a recognizable enriched row. findById: jest.fn(async () => UPDATED), updateComment: jest.fn(async () => undefined), + hasChildren: jest.fn(async () => hasChildren), + deleteComment: jest.fn(async () => undefined), }; const pageRepo: any = {}; const wsService: any = { emitCommentEvent: jest.fn() }; @@ -74,7 +81,9 @@ describe('CommentService — applySuggestion', () => { .map((c: any[]) => c[0]) .find((patch: any) => 'suggestionAppliedAt' in patch); - it('applied=true → replaces text, persists applied stamps, auto-resolves, audits, returns updated', async () => { + // --- no replies → ephemeral delete branch ------------------------------- + + it('applied=true, no replies → replaces text, hard-deletes, strips the anchor mark, audits APPLIED, outcome=deleted', async () => { const { service, commentRepo, wsService, collaborationGateway, auditService } = makeService({ applied: true, currentText: 'new text' }); @@ -92,37 +101,34 @@ describe('CommentService — applySuggestion', () => { }), ); - // Applied stamps persisted. - const patch = appliedPatch(commentRepo); - expect(patch.suggestionAppliedAt).toBeInstanceOf(Date); - expect(patch.suggestionAppliedById).toBe('user-1'); + // Ephemeral: the redundant comment is hard-deleted and its inline anchor + // mark removed via the deleteCommentMark collab event. + expect(commentRepo.deleteComment).toHaveBeenCalledWith('c-1'); + expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith( + 'deleteCommentMark', + 'page.page-1', + expect.objectContaining({ commentId: 'c-1', user: expect.any(Object) }), + ); + // No applied stamps are written for a row about to be deleted. + expect(appliedPatch(commentRepo)).toBeUndefined(); - // Auto-resolved: resolveComment writes a resolvedAt/resolvedById patch too. - 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'); - - // Audit + broadcast + return. + // Broadcast a deletion, audit the (still-applied) suggestion, report outcome. + expect(wsService.emitCommentEvent).toHaveBeenCalledWith( + 'space-1', + 'page-1', + expect.objectContaining({ operation: 'commentDeleted', commentId: 'c-1' }), + ); expect(auditService.log).toHaveBeenCalledWith( expect.objectContaining({ event: AuditEvent.COMMENT_SUGGESTION_APPLIED, resourceType: AuditResource.COMMENT, resourceId: 'c-1', - spaceId: 'space-1', - metadata: { pageId: 'page-1' }, }), ); - expect(wsService.emitCommentEvent).toHaveBeenCalledWith( - 'space-1', - 'page-1', - expect.objectContaining({ operation: 'commentUpdated', comment: UPDATED }), - ); - expect(result).toBe(UPDATED); + expect(result.outcome).toBe('deleted'); }); - it('applied=false but currentText === suggestedText → idempotent success (no 409)', async () => { + it('applied=false but currentText === suggestedText, no replies → idempotent delete (no 409)', async () => { const { service, commentRepo, auditService } = makeService({ applied: false, currentText: 'new text', @@ -130,15 +136,55 @@ describe('CommentService — applySuggestion', () => { const result = await service.applySuggestion(suggestionComment(), user()); - // The stamps are still persisted (reconciling a crash between the doc - // mutation and the DB write) and the call succeeds. + expect(commentRepo.deleteComment).toHaveBeenCalledWith('c-1'); + expect(auditService.log).toHaveBeenCalledTimes(1); + expect(result.outcome).toBe('deleted'); + }); + + // --- has replies → resolve branch (discussion preserved) ---------------- + + it('applied=true, WITH replies → resolves (not delete), persists applied stamps, audits, outcome=resolved', async () => { + const { service, commentRepo, wsService, collaborationGateway, auditService } = + makeService({ applied: true, currentText: 'new text' }, true); + + const result = await service.applySuggestion(suggestionComment(), user()); + + // Applied stamps persisted. const patch = appliedPatch(commentRepo); expect(patch.suggestionAppliedAt).toBeInstanceOf(Date); expect(patch.suggestionAppliedById).toBe('user-1'); - expect(auditService.log).toHaveBeenCalledTimes(1); - expect(result).toBe(UPDATED); + + // Auto-resolved (resolveComment writes the resolve patch + resolve mark). + 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'); + + // NOT deleted; broadcast an update, not a deletion. + expect(commentRepo.deleteComment).not.toHaveBeenCalled(); + expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalledWith( + 'deleteCommentMark', + expect.anything(), + expect.anything(), + ); + expect(wsService.emitCommentEvent).toHaveBeenCalledWith( + 'space-1', + 'page-1', + expect.objectContaining({ operation: 'commentUpdated', comment: UPDATED }), + ); + + expect(auditService.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: AuditEvent.COMMENT_SUGGESTION_APPLIED, + }), + ); + expect(result.id).toBe('c-1'); + expect(result.outcome).toBe('resolved'); }); + // --- error / rejection branches ----------------------------------------- + it('applied=false and currentText differs → ConflictException with currentText in payload', async () => { const { service, commentRepo, auditService } = makeService({ applied: false, @@ -153,14 +199,14 @@ describe('CommentService — applySuggestion', () => { expect(err.getResponse()).toMatchObject({ currentText: 'someone else edited this', }); - // No persistence and no audit on a conflict. - expect(appliedPatch(commentRepo)).toBeUndefined(); + // No delete and no audit on a conflict. + expect(commentRepo.deleteComment).not.toHaveBeenCalled(); expect(auditService.log).not.toHaveBeenCalled(); }); - it('already-applied AND already-resolved → idempotent success, no collab call, no re-resolve (#315 double-click)', async () => { + it('already-applied WITH replies → idempotent success, no re-apply, resolve branch', async () => { const { service, collaborationGateway, commentRepo, auditService } = - makeService({ applied: true, currentText: 'new text' }); + makeService({ applied: true, currentText: 'new text' }, true); const result = await service.applySuggestion( suggestionComment({ @@ -171,17 +217,20 @@ describe('CommentService — applySuggestion', () => { user(), ); - // Idempotent SUCCESS, not a 409. The suggestion is already applied, so the - // collaborative document is never touched again and nothing is re-stamped - // or re-resolved. - expect(result).toBe(UPDATED); - expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalled(); - expect(commentRepo.updateComment).not.toHaveBeenCalled(); - // Same success shape as the applied path (broadcast + audit). + // Idempotent SUCCESS. The suggestion is already applied, so the document is + // never re-mutated (no applyCommentSuggestion) and nothing is re-stamped. + expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalledWith( + 'applyCommentSuggestion', + expect.anything(), + expect.anything(), + ); + expect(appliedPatch(commentRepo)).toBeUndefined(); + expect(commentRepo.deleteComment).not.toHaveBeenCalled(); expect(auditService.log).toHaveBeenCalledTimes(1); + expect(result.outcome).toBe('resolved'); }); - it('already-applied but NOT resolved (crash window) → idempotent success, self-heals resolve, no re-apply', async () => { + it('already-applied, no replies (double-click after a delete) → deletes idempotently', async () => { const { service, collaborationGateway, commentRepo } = makeService({ applied: true, currentText: 'new text', @@ -192,28 +241,14 @@ describe('CommentService — applySuggestion', () => { user(), ); - expect(result).toBe(UPDATED); - - // The suggestion is NOT re-applied to the document… + // No re-apply to the document; the childless applied comment is removed. expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalledWith( 'applyCommentSuggestion', expect.anything(), expect.anything(), ); - // …but the open thread is self-healed to resolved via resolveComment, which - // writes the resolve patch and updates the resolve mark. - 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 }), - ); - // The applied stamps are NOT re-written (already stamped). - expect(appliedPatch(commentRepo)).toBeUndefined(); + expect(commentRepo.deleteComment).toHaveBeenCalledWith('c-1'); + expect(result.outcome).toBe('deleted'); }); it('rejects a comment with no suggestedText', async () => { @@ -238,8 +273,8 @@ describe('CommentService — applySuggestion', () => { service.applySuggestion(suggestionComment(), user()), ).rejects.toThrow(InternalServerErrorException); - // Nothing persisted, nothing audited. - expect(appliedPatch(commentRepo)).toBeUndefined(); + // Nothing deleted, nothing audited. + expect(commentRepo.deleteComment).not.toHaveBeenCalled(); expect(auditService.log).not.toHaveBeenCalled(); }); }); 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 new file mode 100644 index 00000000..5c5553b2 --- /dev/null +++ b/apps/server/src/core/comment/comment.service.dismiss-suggestion.spec.ts @@ -0,0 +1,194 @@ +import { BadRequestException } from '@nestjs/common'; +import { CommentService } from './comment.service'; +import { AuditEvent, AuditResource } from '../../common/events/audit-events'; + +/** + * Coverage for CommentService.dismissSuggestion (#329). Dismiss ("Не применять") + * removes a suggested edit WITHOUT changing the page text: the comment + * disappears (hard-delete + strip the inline anchor mark) unless the thread has + * replies, in which case it is resolved to preserve the discussion. + * + * The permission gate (canComment, NOT canEdit) lives in the controller and is + * covered in comment.controller.spec.ts; here we pin the service's own state + * guards and the delete-vs-resolve fork. + */ +describe('CommentService — dismissSuggestion', () => { + const UPDATED = { id: 'c-1', __updated: true } as any; + + function makeService(hasChildren = false) { + const commentRepo: any = { + findById: jest.fn(async () => UPDATED), + updateComment: jest.fn(async () => undefined), + hasChildren: jest.fn(async () => hasChildren), + deleteComment: jest.fn(async () => undefined), + }; + const pageRepo: any = {}; + const wsService: any = { emitCommentEvent: jest.fn() }; + const collaborationGateway: any = { + handleYjsEvent: jest.fn(async () => undefined), + }; + const generalQueue: any = { add: jest.fn(() => Promise.resolve()) }; + const notificationQueue: any = { add: jest.fn(async () => undefined) }; + const auditService: any = { log: jest.fn() }; + + const service = new CommentService( + commentRepo, + pageRepo, + wsService, + collaborationGateway, + generalQueue, + notificationQueue, + auditService, + ); + + return { service, commentRepo, wsService, collaborationGateway, auditService }; + } + + const suggestionComment = (over?: Partial): any => ({ + id: 'c-1', + pageId: 'page-1', + spaceId: 'space-1', + workspaceId: 'ws-1', + creatorId: 'user-1', + parentCommentId: null, + selection: 'old text', + suggestedText: 'new text', + suggestionAppliedAt: null, + resolvedAt: null, + ...over, + }); + const user = (over?: Partial): any => ({ id: 'user-1', ...over }); + + it('no replies → hard-deletes, strips the anchor mark, does NOT touch page text, audits DISMISSED, outcome=deleted', async () => { + const { service, commentRepo, wsService, collaborationGateway, auditService } = + makeService(false); + + const result = await service.dismissSuggestion(suggestionComment(), user()); + + // Never applies the suggestion to the document. + expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalledWith( + 'applyCommentSuggestion', + expect.anything(), + expect.anything(), + ); + // Hard-delete + strip mark. + expect(commentRepo.deleteComment).toHaveBeenCalledWith('c-1'); + expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith( + 'deleteCommentMark', + 'page.page-1', + expect.objectContaining({ commentId: 'c-1', user: expect.any(Object) }), + ); + expect(wsService.emitCommentEvent).toHaveBeenCalledWith( + 'space-1', + 'page-1', + expect.objectContaining({ operation: 'commentDeleted', commentId: 'c-1' }), + ); + expect(auditService.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: AuditEvent.COMMENT_SUGGESTION_DISMISSED, + resourceType: AuditResource.COMMENT, + resourceId: 'c-1', + }), + ); + expect(result.outcome).toBe('deleted'); + }); + + it('no replies → if the anchor-mark removal FAILS, the row is NOT deleted and the error propagates (#329: no orphan anchor)', async () => { + const { service, commentRepo, wsService, collaborationGateway } = + makeService(false); + // Mark removal is FATAL and runs BEFORE the irreversible row delete: a collab + // failure (e.g. COLLAB_DISABLE_REDIS "no live instance") must abort the whole + // operation, leaving row + mark consistent — never a deleted row with an + // orphan anchor left in the document reporting success. + collaborationGateway.handleYjsEvent = jest.fn(async () => { + throw new Error('requires a live collaboration instance'); + }); + + await expect( + service.dismissSuggestion(suggestionComment(), user()), + ).rejects.toThrow(/live collaboration/); + + expect(commentRepo.deleteComment).not.toHaveBeenCalled(); + expect(wsService.emitCommentEvent).not.toHaveBeenCalledWith( + expect.anything(), + expect.anything(), + expect.objectContaining({ operation: 'commentDeleted' }), + ); + }); + + it('WITH replies → resolves (not delete), does NOT apply, audits DISMISSED, outcome=resolved', async () => { + const { service, commentRepo, wsService, collaborationGateway, auditService } = + makeService(true); + + const result = await service.dismissSuggestion(suggestionComment(), user()); + + // Resolved via resolveComment (resolve patch + resolve mark), NOT deleted. + 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(commentRepo.deleteComment).not.toHaveBeenCalled(); + expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith( + 'resolveCommentMark', + 'page.page-1', + expect.objectContaining({ commentId: 'c-1', resolved: true }), + ); + // No applied stamp — dismiss does not apply the edit. + const appliedPatch = commentRepo.updateComment.mock.calls + .map((c: any[]) => c[0]) + .find((p: any) => 'suggestionAppliedAt' in p); + expect(appliedPatch).toBeUndefined(); + + expect(auditService.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: AuditEvent.COMMENT_SUGGESTION_DISMISSED, + }), + ); + expect(result.outcome).toBe('resolved'); + }); + + it('rejects a reply (non-top-level) comment', async () => { + const { service, commentRepo } = makeService(); + await expect( + service.dismissSuggestion( + suggestionComment({ parentCommentId: 'parent-1' }), + user(), + ), + ).rejects.toThrow(BadRequestException); + expect(commentRepo.deleteComment).not.toHaveBeenCalled(); + }); + + it('rejects a comment without a suggested edit', async () => { + const { service, commentRepo } = makeService(); + await expect( + service.dismissSuggestion( + suggestionComment({ suggestedText: null }), + user(), + ), + ).rejects.toThrow(BadRequestException); + expect(commentRepo.deleteComment).not.toHaveBeenCalled(); + }); + + it('rejects an already-applied suggestion', async () => { + const { service, commentRepo } = makeService(); + await expect( + service.dismissSuggestion( + suggestionComment({ suggestionAppliedAt: new Date() }), + user(), + ), + ).rejects.toThrow(BadRequestException); + expect(commentRepo.deleteComment).not.toHaveBeenCalled(); + }); + + it('rejects an already-resolved thread', async () => { + const { service, commentRepo } = makeService(); + await expect( + service.dismissSuggestion( + suggestionComment({ resolvedAt: new Date() }), + user(), + ), + ).rejects.toThrow(BadRequestException); + expect(commentRepo.deleteComment).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/server/src/core/comment/comment.service.ts b/apps/server/src/core/comment/comment.service.ts index cc46a002..aad0ef9c 100644 --- a/apps/server/src/core/comment/comment.service.ts +++ b/apps/server/src/core/comment/comment.service.ts @@ -35,6 +35,12 @@ import { IAuditService, } from '../../integrations/audit/audit.service'; +// Ephemeral-suggestion settle result (#329): 'deleted' → the comment vanished +// (hard-delete + anchor mark stripped); 'resolved' → the thread had replies and +// was resolved instead. Returned to the client so it can pick the optimistic +// cache action. +export type SuggestionOutcome = 'deleted' | 'resolved'; + @Injectable() export class CommentService { private readonly logger = new Logger(CommentService.name); @@ -362,7 +368,7 @@ export class CommentService { comment: Comment, user: User, provenance?: AuthProvenanceData, - ): Promise { + ): Promise { // Structural guards. if (comment.parentCommentId) { throw new BadRequestException( @@ -449,42 +455,144 @@ export class CommentService { } /** - * Persist the applied stamps (idempotently), auto-resolve the thread and - * broadcast + audit the applied suggestion. Shared by the applied and the + * Dismiss ("Не применять") a suggested edit without touching the page text: + * the suggestion disappears. Ephemeral rule (#329) — a top-level suggestion + * comment is transient UI, so dismissing it hard-deletes the comment AND strips + * its inline anchor mark UNLESS the thread has replies, in which case the + * discussion is preserved by resolving it instead. + * + * Dismiss does NOT change the document text, so the controller authorizes it + * with canComment (NOT canEdit). This re-checks the comment's own state so the + * invariant holds regardless of caller. + */ + async dismissSuggestion( + comment: Comment, + user: User, + provenance?: AuthProvenanceData, + ): Promise { + // Structural guards (mirror applySuggestion). + if (comment.parentCommentId) { + throw new BadRequestException( + 'Only a top-level comment can carry a suggested edit', + ); + } + if (!comment.suggestedText) { + throw new BadRequestException( + 'This comment has no suggested edit to dismiss', + ); + } + // State guards: dismissing an already-applied or already-resolved thread is + // meaningless. On an apply↔dismiss race the loser sees the comment already + // gone (404 at the controller) or already resolved (this 400); the client + // treats both as "already resolved". + if (comment.suggestionAppliedAt) { + throw new BadRequestException( + 'Cannot dismiss a suggested edit that was already applied', + ); + } + if (comment.resolvedAt) { + throw new BadRequestException( + 'Cannot dismiss a suggested edit on a resolved comment thread', + ); + } + + const hasChildren = await this.commentRepo.hasChildren(comment.id); + + if (hasChildren) { + // Preserve the discussion: resolve (never delete) a thread with replies. + const updatedComment = await this.resolveComment( + comment, + true, + user, + provenance, + ); + this.auditService.log({ + event: AuditEvent.COMMENT_SUGGESTION_DISMISSED, + resourceType: AuditResource.COMMENT, + resourceId: comment.id, + spaceId: comment.spaceId, + metadata: { pageId: comment.pageId }, + }); + return { ...updatedComment, outcome: 'resolved' }; + } + + // Ephemeral: no replies → the suggestion vanishes entirely. + await this.deleteEphemeralSuggestion(comment, user); + this.auditService.log({ + event: AuditEvent.COMMENT_SUGGESTION_DISMISSED, + resourceType: AuditResource.COMMENT, + resourceId: comment.id, + spaceId: comment.spaceId, + metadata: { pageId: comment.pageId }, + }); + return { ...comment, outcome: 'deleted' }; + } + + /** + * Persist the applied stamps (idempotently), then settle the suggestion under + * the ephemeral rule (#329): a suggestion whose thread has NO replies + * DISAPPEARS after apply (hard-delete + strip the inline anchor mark), since + * the suggested text is now in the document and a stand-alone resolved thread + * would only pile up an orphan anchor. A thread WITH replies is preserved by + * auto-resolving it (the historical behaviour). Shared by the applied and the * idempotent "already-applied" branches of applySuggestion. + * + * Returns the comment augmented with `outcome` so the client can pick the + * optimistic action ('deleted' → drop it, 'resolved' → move to the resolved + * tab). */ private async finalizeAppliedSuggestion( comment: Comment, user: User, provenance?: AuthProvenanceData, - ): Promise { - if (!comment.suggestionAppliedAt) { - await this.commentRepo.updateComment( - { - suggestionAppliedAt: new Date(), - suggestionAppliedById: user.id, - }, - comment.id, - ); + ): Promise { + const hasChildren = await this.commentRepo.hasChildren(comment.id); + + if (hasChildren) { + // Thread has replies → preserve the discussion: stamp applied + resolve. + if (!comment.suggestionAppliedAt) { + await this.commentRepo.updateComment( + { + suggestionAppliedAt: new Date(), + suggestionAppliedById: user.id, + }, + comment.id, + ); + } + + // Auto-resolve the thread. resolveComment handles the resolve mark, its ws + // broadcast and the resolve notification. Stay defensive on re-entry. + if (!comment.resolvedAt) { + await this.resolveComment(comment, true, user, provenance); + } + + const updatedComment = await this.commentRepo.findById(comment.id, { + includeCreator: true, + includeResolvedBy: true, + }); + + this.wsService.emitCommentEvent(comment.spaceId, comment.pageId, { + operation: 'commentUpdated', + pageId: comment.pageId, + comment: updatedComment, + }); + + this.auditService.log({ + event: AuditEvent.COMMENT_SUGGESTION_APPLIED, + resourceType: AuditResource.COMMENT, + resourceId: comment.id, + spaceId: comment.spaceId, + metadata: { pageId: comment.pageId }, + }); + + return { ...updatedComment, outcome: 'resolved' }; } - // Auto-resolve the thread. resolveComment handles the resolve mark, its ws - // broadcast and the resolve notification. The guard above guarantees the - // thread was open when we entered, but stay defensive on re-entry. - if (!comment.resolvedAt) { - await this.resolveComment(comment, true, user, provenance); - } - - const updatedComment = await this.commentRepo.findById(comment.id, { - includeCreator: true, - includeResolvedBy: true, - }); - - this.wsService.emitCommentEvent(comment.spaceId, comment.pageId, { - operation: 'commentUpdated', - pageId: comment.pageId, - comment: updatedComment, - }); + // No replies → ephemeral: the suggested text is already in the document, so + // 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); this.auditService.log({ event: AuditEvent.COMMENT_SUGGESTION_APPLIED, @@ -494,7 +602,53 @@ export class CommentService { metadata: { pageId: comment.pageId }, }); - return updatedComment; + return { ...comment, outcome: 'deleted' }; + } + + /** + * 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). + * + * ORDER MATTERS: the anchor mark is removed FIRST and FATALLY (mirrors + * applySuggestion, which mutates the doc before writing the DB). The row + * delete is irreversible, so if the mark removal fails — including the + * COLLAB_DISABLE_REDIS "no live instance" hard-error — we must NOT delete the + * row and report success, or the document is left with a permanent orphan + * 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. + */ + private async deleteEphemeralSuggestion( + comment: Comment, + user: User, + ): 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, + }); + } + + /** + * Remove the inline `comment` mark for a comment from the collaborative + * document. FATAL, NOT best-effort: unlike resolveComment (which keeps the row, + * so a failed mark update is recoverable), this is used before an irreversible + * hard-delete, so the mark removal MUST succeed or throw. Under + * COLLAB_DISABLE_REDIS the gateway invokes the deleteCommentMark handler + * directly (never a silent no-op) and a missing live instance surfaces as a + * thrown error, which we let propagate so the caller aborts before deleting. + */ + private async deleteCommentMark(comment: Comment, user: User): Promise { + const documentName = `page.${comment.pageId}`; + await this.collaborationGateway.handleYjsEvent( + 'deleteCommentMark', + documentName, + { commentId: comment.id, user }, + ); } private async queueCommentNotification( diff --git a/apps/server/src/core/comment/dto/dismiss-suggestion.dto.ts b/apps/server/src/core/comment/dto/dismiss-suggestion.dto.ts new file mode 100644 index 00000000..74dc1ce7 --- /dev/null +++ b/apps/server/src/core/comment/dto/dismiss-suggestion.dto.ts @@ -0,0 +1,6 @@ +import { IsUUID } from 'class-validator'; + +export class DismissSuggestionDto { + @IsUUID() + commentId: string; +} -- 2.52.0 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 2/3] 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') -- 2.52.0 From d7fa6738e5f25d7578ec69a52aed7985fb00cf99 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 4 Jul 2026 19:04:25 +0300 Subject: [PATCH 3/3] 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 37450ac4..0a343bf7 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( , ); @@ -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 => ({ 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 dc1b1c61..aa9b253a 100644 --- a/apps/client/src/features/comment/components/comment-list-item.tsx +++ b/apps/client/src/features/comment/components/comment-list-item.tsx @@ -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 && ( ) : ( (canShowApply(comment, canEdit) || - canShowDismiss(comment, canComment)) && ( + canShowDismiss(comment, canComment, isOwnerOrAdmin)) && ( {canShowApply(comment, canEdit) && (