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..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 @@ -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,14 +46,20 @@ const baseComment = (over?: Partial): IComment => ...over, }) as IComment; -function renderItem(comment: IComment, canEdit = true) { +function renderItem( + comment: IComment, + canEdit = true, + canComment = true, + userSpaceRole?: string, +) { return render( , ); @@ -159,6 +173,65 @@ 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, + }); + + // 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 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, "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, "admin"); + expect(screen.queryByRole("button", { name: "Dismiss" })).toBeNull(); + }); + + it("hides Dismiss (shows the Applied badge) once applied", () => { + 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, "admin"); + 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 +257,32 @@ 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, owner/admin, not applied/resolved, top-level", () => { + expect(canShowDismiss(c(), true, true)).toBe(true); + }); + it("false without comment permission", () => { + 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, true)).toBe(false); + }); + it("false when already applied", () => { + expect(canShowDismiss(c({ suggestionAppliedAt: new Date() }), true, true)).toBe( + false, + ); + }); + it("false when resolved", () => { + expect(canShowDismiss(c({ resolvedAt: new Date() }), true, true)).toBe(false); + }); + it("false for a reply comment", () => { + 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 0e397245..aa9b253a 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); @@ -69,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]); @@ -130,6 +139,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}"]`, @@ -205,7 +227,7 @@ function CommentListItem({ /> )} - {(currentUser?.user?.id === comment.creatorId || userSpaceRole === 'admin') && ( + {isOwnerOrAdmin && ( ) : ( - canShowApply(comment, canEdit) && ( - + (canShowApply(comment, canEdit) || + canShowDismiss(comment, canComment, isOwnerOrAdmin)) && ( + + {canShowApply(comment, canEdit) && ( + + )} + {/* Dismiss ("Не применять", #329): removes the suggestion + without changing the page text. Gated on canComment. */} + {canShowDismiss(comment, canComment, isOwnerOrAdmin) && ( + + )} + ) )} 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..ff289e45 --- /dev/null +++ b/apps/client/src/features/comment/queries/comment-query.suggestion-outcome.test.tsx @@ -0,0 +1,279 @@ +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 { notifications } from "@mantine/notifications"; +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); + // #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 () => { + // 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); + // #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 a1942532..97783841 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,121 @@ 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 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). 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; + if (cache) { + queryClient.setQueryData( + RQ_KEY(variables.pageId), + removeCommentFromCache(cache, variables.commentId), + ); + } + 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 status = err?.response?.status; const currentText = err?.response?.data?.currentText; if (status === 409 && typeof currentText === "string") { const shortText = @@ -234,6 +317,58 @@ 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 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) { + 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..eb223fbe 100644 --- a/apps/client/src/features/comment/utils/suggestion.ts +++ b/apps/client/src/features/comment/utils/suggestion.ts @@ -115,3 +115,25 @@ 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 needs only canComment, not +// canEdit), BUT a childless dismiss IRREVERSIBLY hard-deletes the comment, so the +// server gates it on comment-owner-OR-space-admin (#338 F5). The button must +// mirror that authz or a non-owner non-admin sees a live Dismiss that always +// 403s → red error. Hence isOwnerOrAdmin is required IN ADDITION to canComment. +// Same not-applied/not-resolved/top-level conditions as Apply. +export function canShowDismiss( + comment: IComment, + canComment?: boolean, + isOwnerOrAdmin?: boolean, +): boolean { + return Boolean( + canComment && + isOwnerOrAdmin && + comment.suggestedText && + !comment.suggestionAppliedAt && + !comment.resolvedAt && + !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..0d10be04 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,207 @@ 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', () => { + // 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', + outcome: 'deleted', + })), + }; + const commentRepo = { findById: jest.fn() }; + const pageRepo = { findById: jest.fn() }; + const spaceAbility = { + createForUser: jest.fn(async () => ({ + cannot: jest.fn(() => !isAdmin), + })), + } 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, + spaceAbility, + }; + } + + const user: any = { id: 'u-1' }; + 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', + }; + 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); + }); + + // --- #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 f04f32e5..fbd2c190 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,59 @@ 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 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); + } + @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..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 @@ -13,17 +13,27 @@ 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, deletedRows = 1) { 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), + // #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() }; @@ -74,7 +84,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 +104,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 (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', + 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 +139,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.deleteCommentIfChildless).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 +202,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 +220,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 +244,43 @@ 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. + 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(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(result.outcome).toBe('resolved'); }); it('rejects a comment with no suggestedText', async () => { @@ -238,8 +305,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..41fc65be --- /dev/null +++ b/apps/server/src/core/comment/comment.service.dismiss-suggestion.spec.ts @@ -0,0 +1,229 @@ +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, 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() }; + 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 (atomic-conditional) + strip mark. + expect(commentRepo.deleteCommentIfChildless).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.deleteCommentIfChildless).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('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( + 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..8e3d435b 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,148 @@ 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. 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, + resourceId: comment.id, + spaceId: comment.spaceId, + metadata: { pageId: comment.pageId }, + }); + return settled; + } + + /** + * 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. + // 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, @@ -494,7 +606,86 @@ export class CommentService { metadata: { pageId: comment.pageId }, }); - return updatedComment; + return settled; + } + + /** + * 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 + * 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. + * + * RACE (#338 F4): 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 under a FOR UPDATE lock inside a transaction (a plain + * anti-join DELETE is NOT race-safe under READ COMMITTED — see the repo method + * docstring). 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, + provenance?: AuthProvenanceData, + ): Promise { + await this.deleteCommentMark(comment, user); + + 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' }; + } + + /** + * 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; +} diff --git a/apps/server/src/database/repos/comment/comment.repo.ts b/apps/server/src/database/repos/comment/comment.repo.ts index b2df6b62..afb3afec 100644 --- a/apps/server/src/database/repos/comment/comment.repo.ts +++ b/apps/server/src/database/repos/comment/comment.repo.ts @@ -139,6 +139,65 @@ export class CommentRepo { await this.db.deleteFrom('comments').where('id', '=', commentId).execute(); } + /** + * Delete an ephemeral suggestion row ONLY if it is still childless, returning + * the number of rows removed (0 or 1). Closes the data-loss race in + * dismiss/apply (#338 F4): the service reads `hasChildren`, then removes the + * anchor mark (a collab round-trip of tens-to-hundreds of ms), then calls this. + * `comments.parent_comment_id` is ON DELETE CASCADE, so a reply landing in that + * window would be cascade-destroyed by a blind delete. + * + * A single anti-join `DELETE … WHERE NOT EXISTS(child)` is NOT sufficient under + * READ COMMITTED: if a reply INSERT (holding FOR KEY SHARE on the parent, not + * yet committed) interleaves, the DELETE's snapshot does not see the + * uncommitted child, so `NOT EXISTS` is true and the parent qualifies; the + * DELETE then blocks on the child's key-share lock, and when it wakes the row + * was only LOCKED (not modified), so EvalPlanQual does NOT re-evaluate the + * predicate → the parent is deleted and the just-committed reply cascades away. + * + * So we do a lock-then-recheck in ONE transaction: + * 1. `SELECT id … FOR UPDATE` on the parent. FOR UPDATE conflicts with the + * FOR KEY SHARE a concurrent reply INSERT takes on its parent (FK), so a + * reply in the window serializes against us: it either commits before we + * acquire the lock, or it must wait until this tx ends. + * 2. Re-read childlessness with a FRESH statement in the SAME tx. Under RC a + * new statement gets a new snapshot, so a reply that committed while we + * waited on the lock is now visible. + * 3. Delete only if still childless (return 1); otherwise return 0 so the + * caller resolves the thread instead. The FOR UPDATE lock is held to + * end-of-tx, so no new reply can insert between the re-check and the delete. + */ + async deleteCommentIfChildless(commentId: string): Promise { + return this.db.transaction().execute(async (trx) => { + const parent = await trx + .selectFrom('comments') + .select('id') + .where('id', '=', commentId) + .forUpdate() + .executeTakeFirst(); + + // Already gone (e.g. a racing delete won) → nothing to remove. + if (!parent) return 0; + + const child = await trx + .selectFrom('comments') + .select('id') + .where('parentCommentId', '=', commentId) + .limit(1) + .executeTakeFirst(); + + // A reply exists (possibly one that just committed) → do NOT hard-delete; + // the cascade would destroy it. Caller falls back to resolving the thread. + if (child) return 0; + + await trx + .deleteFrom('comments') + .where('id', '=', commentId) + .execute(); + return 1; + }); + } + async hasChildren(commentId: string): Promise { const result = await this.db .selectFrom('comments') diff --git a/apps/server/test/integration/comment-delete-if-childless.int-spec.ts b/apps/server/test/integration/comment-delete-if-childless.int-spec.ts new file mode 100644 index 00000000..771a6a49 --- /dev/null +++ b/apps/server/test/integration/comment-delete-if-childless.int-spec.ts @@ -0,0 +1,160 @@ +import { Kysely } from 'kysely'; +import { CommentRepo } from '../../src/database/repos/comment/comment.repo'; +import { + getTestDb, + destroyTestDb, + buildTestDb, + createWorkspace, + createSpace, + createPage, + createUser, + createComment, +} from './db'; + +/** + * Real-DB coverage for CommentRepo.deleteCommentIfChildless (#338 F4/F6). + * + * This is the guard that keeps an ephemeral-suggestion hard-delete from + * cascade-destroying a reply (`comments.parent_comment_id` is ON DELETE CASCADE). + * The unit tests MOCK this method to 0/1, so only an int-spec actually exercises + * the SQL — the FOR UPDATE lock-then-recheck transaction — against Postgres. + * + * The concurrency case is the whole point: a plain anti-join + * `DELETE … WHERE NOT EXISTS(child)` passes (a) and (b) but SILENTLY loses a + * reply that commits mid-operation under READ COMMITTED (EvalPlanQual does not + * re-check a merely-locked row). Test (c) reproduces exactly that interleaving + * and asserts the row + reply both survive. + */ +describe('CommentRepo.deleteCommentIfChildless [integration]', () => { + let db: Kysely; + let repo: CommentRepo; + let workspaceId: string; + let spaceId: string; + let pageId: string; + let userId: string; + + beforeAll(async () => { + db = getTestDb(); + repo = new CommentRepo(db as any); + workspaceId = (await createWorkspace(db)).id; + spaceId = (await createSpace(db, workspaceId)).id; + pageId = (await createPage(db, { workspaceId, spaceId })).id; + userId = (await createUser(db, workspaceId)).id; + }); + + afterAll(async () => { + await destroyTestDb(); + }); + + async function rowExists(id: string): Promise { + const row = await db + .selectFrom('comments') + .select('id') + .where('id', '=', id) + .executeTakeFirst(); + return Boolean(row); + } + + function seedTopLevel() { + return createComment(db, { + workspaceId, + spaceId, + pageId, + creatorId: userId, + selection: 'old text', + suggestedText: 'new text', + }); + } + + function seedReply(parentId: string) { + return createComment(db, { + workspaceId, + spaceId, + pageId, + creatorId: userId, + parentCommentId: parentId, + }); + } + + it('(a) childless top-level → returns 1 and the row is gone', async () => { + const parent = await seedTopLevel(); + expect(await rowExists(parent.id)).toBe(true); + + const deleted = await repo.deleteCommentIfChildless(parent.id); + + expect(deleted).toBe(1); + expect(await rowExists(parent.id)).toBe(false); + }); + + it('(b) top-level WITH a committed reply → returns 0, parent AND reply survive (gate blocks the cascade)', async () => { + const parent = await seedTopLevel(); + const reply = await seedReply(parent.id); + + const deleted = await repo.deleteCommentIfChildless(parent.id); + + expect(deleted).toBe(0); + expect(await rowExists(parent.id)).toBe(true); + expect(await rowExists(reply.id)).toBe(true); + }); + + it('(c) reply COMMITS mid-operation (FOR UPDATE path) → returns 0, parent + reply survive; a blind anti-join would lose the reply', async () => { + const parent = await seedTopLevel(); + + // Second connection holds an open transaction that inserts a reply (taking + // FOR KEY SHARE on the parent via the FK) and does NOT commit until we open + // the gate — reproducing the "reply not yet committed" window. + const conn2 = buildTestDb(); + let openGate!: () => void; + const gate = new Promise((resolve) => { + openGate = resolve; + }); + let replyId: string | undefined; + + const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); + + try { + const replyTx = conn2.transaction().execute(async (trx) => { + const row = await trx + .insertInto('comments') + .values({ + workspaceId, + spaceId, + pageId, + creatorId: userId, + parentCommentId: parent.id, + }) + .returning(['id']) + .executeTakeFirstOrThrow(); + replyId = row.id as string; + // Hold the FOR KEY SHARE lock on the parent until the gate opens. + await gate; + }); + + // Let the reply INSERT acquire its lock before the delete starts. + await sleep(250); + + // deleteCommentIfChildless does SELECT ... FOR UPDATE on the parent, which + // conflicts with the reply's FOR KEY SHARE, so it BLOCKS here. + const deletePromise = repo.deleteCommentIfChildless(parent.id); + + // Give the delete time to reach (and block on) its FOR UPDATE, then let the + // reply commit. The delete then wakes, re-checks under the lock, sees the + // now-committed reply, and returns 0. + await sleep(250); + openGate(); + await replyTx; + + const deleted = await deletePromise; + + expect(deleted).toBe(0); + expect(await rowExists(parent.id)).toBe(true); + expect(replyId).toBeDefined(); + expect(await rowExists(replyId!)).toBe(true); + } finally { + // Always release the gate (in case an assertion threw before openGate) and + // close the extra connection so global-teardown can DROP the database. + openGate(); + await conn2.destroy(); + } + }); +}); diff --git a/apps/server/test/integration/db.ts b/apps/server/test/integration/db.ts index db795a2f..29033953 100644 --- a/apps/server/test/integration/db.ts +++ b/apps/server/test/integration/db.ts @@ -230,6 +230,40 @@ export async function createPage( return { id: row.id as string }; } +export async function createComment( + db: Kysely, + args: { + workspaceId: string; + spaceId: string; + pageId: string; + creatorId?: string | null; + parentCommentId?: string | null; + content?: unknown; + selection?: string | null; + suggestedText?: string | null; + type?: string | null; + }, +): Promise<{ id: string }> { + const id = randomUUID(); + const row = await db + .insertInto('comments') + .values({ + id, + workspaceId: args.workspaceId, + spaceId: args.spaceId, + pageId: args.pageId, + creatorId: args.creatorId ?? null, + parentCommentId: args.parentCommentId ?? null, + content: (args.content ?? null) as any, + selection: args.selection ?? null, + suggestedText: args.suggestedText ?? null, + type: args.type ?? 'page', + }) + .returning(['id']) + .executeTakeFirstOrThrow(); + return { id: row.id as string }; +} + export async function createRole( db: Kysely, args: {