feat(comment): ephemeral suggestion-edits — Apply/Dismiss remove the comment (#329)

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) <noreply@anthropic.com>
2026-07-04 15:33:44 +03:00
parent 348dcd0802
commit 8d8ecaed82
18 changed files with 1267 additions and 128 deletions
@@ -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);
+  });
+});