From 5794d62e2d2afb4fe3375f164c9707f9f04e2ef6 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 4 Jul 2026 15:33:44 +0300 Subject: [PATCH] =?UTF-8?q?feat(comment):=20ephemeral=20suggestion-edits?= =?UTF-8?q?=20=E2=80=94=20Apply/Dismiss=20remove=20the=20comment=20(#329)?= 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 | 65 ++++-- .../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(+), 129 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 8b75b337..99164111 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( , @@ -157,6 +165,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; @@ -182,3 +238,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 0d4b5e02..ff62bdd3 100644 --- a/apps/client/src/features/comment/components/comment-list-item.tsx +++ b/apps/client/src/features/comment/components/comment-list-item.tsx @@ -13,11 +13,12 @@ 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 } from "@/features/comment/utils/suggestion"; +import { canShowApply, canShowDismiss } from "@/features/comment/utils/suggestion"; import { CustomAvatar } from "@/components/ui/custom-avatar.tsx"; import { currentUserAtom } from "@/features/user/atoms/current-user-atom.ts"; import { useTranslation } from "react-i18next"; @@ -51,6 +52,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); @@ -115,6 +117,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}"]`, @@ -255,18 +270,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 d14dea6e..d6c2370f 100644 --- a/apps/client/src/features/comment/utils/suggestion.ts +++ b/apps/client/src/features/comment/utils/suggestion.ts @@ -12,3 +12,18 @@ export function canShowApply(comment: IComment, canEdit?: boolean): boolean { !comment.parentCommentId, ); } + +// 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; +}