feat(comment): store suggestedText + POST /comments/apply-suggestion (#315 phase 4)

Server side of agent comment suggestions.

- CreateCommentDto gains optional suggestedText (<=2000). CommentService.create
  accepts it ONLY for a top-level inline comment with a non-empty selection,
  requires it be non-empty and differ from selection (else BadRequest), and
  stores it.
- POST /comments/apply-suggestion (ApplySuggestionDto { commentId }): authorizes
  with validateCanEdit (applying edits page text) BEFORE any structural check or
  mutation, then CommentService.applySuggestion:
  - runs the phase-3 collab event applyCommentSuggestion on `page.<pageId>` to
    atomically check-and-replace the marked text, returning { applied, currentText };
  - applied → stamp suggestion_applied_at/by, auto-resolve the thread, ws
    commentUpdated, audit COMMENT_SUGGESTION_APPLIED;
  - already-applied (DB) → idempotent success (no re-apply), self-healing the
    resolve if it was missed — satisfies the issue's double-click / two-user
    race requirement;
  - collab verdict applied:false && currentText===suggestedText → idempotent
    success (crash between doc mutation and DB write);
  - text changed → 409 ConflictException carrying currentText;
  - gateway undefined/throw → hard error, never a silent success.
- audit-events: COMMENT_SUGGESTION_APPLIED.

Tests: create validation (reply/no-selection/equal-to-selection rejected;
valid stored) + applySuggestion verdict branches incl. both idempotent paths.
jest src/core/comment: 33 passed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
claude code agent 227
2026-07-03 19:09:23 +03:00
parent a9da8f7f15
commit ec542a924b
9 changed files with 594 additions and 2 deletions
@@ -51,6 +51,7 @@ export const AuditEvent = {
COMMENT_UPDATED: 'comment.updated',
COMMENT_RESOLVED: 'comment.resolved',
COMMENT_REOPENED: 'comment.reopened',
COMMENT_SUGGESTION_APPLIED: 'comment.suggestion_applied',
// Page
PAGE_CREATED: 'page.created',
@@ -14,6 +14,7 @@ import { CommentService } from './comment.service';
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 { PageIdDto, CommentIdDto } from './dto/comments.input';
import { AuthUser } from '../../common/decorators/auth-user.decorator';
import { AuthWorkspace } from '../../common/decorators/auth-workspace.decorator';
@@ -197,6 +198,42 @@ export class CommentController {
return updated;
}
@HttpCode(HttpStatus.OK)
@Post('apply-suggestion')
async applySuggestion(
@Body() dto: ApplySuggestionDto,
@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 about the comment
// (metadata-disclosure hygiene). Applying a suggestion rewrites the page
// text, so require edit access (NOT just comment access). Running this
// first means a cross-workspace user with a guessed comment UUID gets a
// uniform 403 regardless of the comment's type or suggestion state — it can
// never distinguish those before the access check. The structural 400s
// (top-level / has-a-suggested-edit) are re-checked by the service below.
await this.pageAccessService.validateCanEdit(page, user);
// The service re-validates the comment's state, returns idempotent success
// for an already-applied suggestion, and lets ConflictException (409, with
// currentText in the payload) propagate untouched.
return this.commentService.applySuggestion(comment, user, provenance);
}
@HttpCode(HttpStatus.OK)
@Post('delete')
async delete(@Body() input: CommentIdDto, @AuthUser() user: User, @AuthWorkspace() workspace: Workspace) {
@@ -0,0 +1,245 @@
import {
BadRequestException,
ConflictException,
InternalServerErrorException,
} from '@nestjs/common';
import { CommentService } from './comment.service';
import { AuditEvent, AuditResource } from '../../common/events/audit-events';
/**
* Focused coverage for CommentService.applySuggestion (comment.service.ts).
* The service is constructed directly with jest-mocked deps (the @InjectQueue
* tokens can't be resolved by Test.createTestingModule — see the sibling specs).
*
* 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.
*/
describe('CommentService — applySuggestion', () => {
const UPDATED = { id: 'c-1', __updated: true } as any;
function makeService(verdict: unknown) {
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),
};
const pageRepo: any = {};
const wsService: any = { emitCommentEvent: jest.fn() };
const collaborationGateway: any = {
handleYjsEvent: jest.fn(async () => verdict),
};
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>): 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>): any => ({ id: 'user-1', ...over });
// Pull the updateComment patch that carries the applied stamps.
const appliedPatch = (commentRepo: any) =>
commentRepo.updateComment.mock.calls
.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 () => {
const { service, commentRepo, wsService, collaborationGateway, auditService } =
makeService({ applied: true, currentText: 'new text' });
const result = await service.applySuggestion(suggestionComment(), user());
// The atomic replace was requested against the exact marked text.
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith(
'applyCommentSuggestion',
'page.page-1',
expect.objectContaining({
commentId: 'c-1',
expectedText: 'old text',
newText: 'new text',
user: expect.objectContaining({ id: 'user-1' }),
}),
);
// Applied stamps persisted.
const patch = appliedPatch(commentRepo);
expect(patch.suggestionAppliedAt).toBeInstanceOf(Date);
expect(patch.suggestionAppliedById).toBe('user-1');
// 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.
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);
});
it('applied=false but currentText === suggestedText → idempotent success (no 409)', async () => {
const { service, commentRepo, auditService } = makeService({
applied: false,
currentText: 'new text',
});
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.
const patch = appliedPatch(commentRepo);
expect(patch.suggestionAppliedAt).toBeInstanceOf(Date);
expect(patch.suggestionAppliedById).toBe('user-1');
expect(auditService.log).toHaveBeenCalledTimes(1);
expect(result).toBe(UPDATED);
});
it('applied=false and currentText differs → ConflictException with currentText in payload', async () => {
const { service, commentRepo, auditService } = makeService({
applied: false,
currentText: 'someone else edited this',
});
const err = await service
.applySuggestion(suggestionComment(), user())
.catch((e) => e);
expect(err).toBeInstanceOf(ConflictException);
expect(err.getResponse()).toMatchObject({
currentText: 'someone else edited this',
});
// No persistence and no audit on a conflict.
expect(appliedPatch(commentRepo)).toBeUndefined();
expect(auditService.log).not.toHaveBeenCalled();
});
it('already-applied AND already-resolved → idempotent success, no collab call, no re-resolve (#315 double-click)', async () => {
const { service, collaborationGateway, commentRepo, auditService } =
makeService({ applied: true, currentText: 'new text' });
const result = await service.applySuggestion(
suggestionComment({
suggestionAppliedAt: new Date(),
resolvedAt: new Date(),
resolvedById: 'user-1',
}),
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).
expect(auditService.log).toHaveBeenCalledTimes(1);
});
it('already-applied but NOT resolved (crash window) → idempotent success, self-heals resolve, no re-apply', async () => {
const { service, collaborationGateway, commentRepo } = makeService({
applied: true,
currentText: 'new text',
});
const result = await service.applySuggestion(
suggestionComment({ suggestionAppliedAt: new Date(), resolvedAt: null }),
user(),
);
expect(result).toBe(UPDATED);
// The suggestion is NOT re-applied to the document…
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();
});
it('rejects a comment with no suggestedText', async () => {
const { service, collaborationGateway } = makeService({
applied: true,
currentText: 'x',
});
await expect(
service.applySuggestion(
suggestionComment({ suggestedText: null }),
user(),
),
).rejects.toThrow(BadRequestException);
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalled();
});
it('gateway returning undefined → hard error, not a silent success', async () => {
const { service, commentRepo, auditService } = makeService(undefined);
await expect(
service.applySuggestion(suggestionComment(), user()),
).rejects.toThrow(InternalServerErrorException);
// Nothing persisted, nothing audited.
expect(appliedPatch(commentRepo)).toBeUndefined();
expect(auditService.log).not.toHaveBeenCalled();
});
});
@@ -60,6 +60,7 @@ describe('CommentService — behavior', () => {
};
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,
@@ -68,14 +69,17 @@ describe('CommentService — behavior', () => {
collaborationGateway,
generalQueue,
notificationQueue,
auditService,
);
return {
service,
commentRepo,
wsService,
collaborationGateway,
generalQueue,
notificationQueue,
auditService,
};
}
@@ -181,6 +185,95 @@ describe('CommentService — behavior', () => {
});
});
describe('create — suggested edit validation & storage', () => {
it('rejects a suggestedText on a reply (not a top-level comment)', async () => {
const parentComment = {
id: 'parent-1',
pageId: 'page-1',
parentCommentId: null,
};
const { service, commentRepo } = makeService({ parentComment });
await expect(
service.create(
{ page: page(), workspaceId: 'ws-1', user: user() },
{
content: JSON.stringify(docMentioning()),
parentCommentId: 'parent-1',
selection: 'hello world',
suggestedText: 'goodbye world',
} as any,
),
).rejects.toThrow(BadRequestException);
expect(commentRepo.insertComment).not.toHaveBeenCalled();
});
it('rejects a suggestedText without a selection', async () => {
const { service, commentRepo } = makeService();
await expect(
service.create(
{ page: page(), workspaceId: 'ws-1', user: user() },
{
content: JSON.stringify(docMentioning()),
suggestedText: 'new text',
} as any,
),
).rejects.toThrow(BadRequestException);
expect(commentRepo.insertComment).not.toHaveBeenCalled();
});
it('rejects a suggestedText identical to the selection (no-op)', async () => {
const { service, commentRepo } = makeService();
await expect(
service.create(
{ page: page(), workspaceId: 'ws-1', user: user() },
{
content: JSON.stringify(docMentioning()),
selection: 'same text',
// Only differs by surrounding whitespace → still a no-op after trim.
suggestedText: ' same text ',
} as any,
),
).rejects.toThrow(BadRequestException);
expect(commentRepo.insertComment).not.toHaveBeenCalled();
});
it('stores a valid suggestedText (trimmed) on the inserted row', async () => {
const { service, commentRepo } = makeService();
await service.create(
{ page: page(), workspaceId: 'ws-1', user: user() },
{
content: JSON.stringify(docMentioning()),
selection: 'old text',
type: 'inline',
suggestedText: ' new text ',
} as any,
);
const insertArg = commentRepo.insertComment.mock.calls[0][0];
expect(insertArg.suggestedText).toBe('new text');
expect(insertArg.selection).toBe('old text');
});
it('leaves suggestedText null for an ordinary comment', async () => {
const { service, commentRepo } = makeService();
await service.create(
{ page: page(), workspaceId: 'ws-1', user: user() },
{ content: JSON.stringify(docMentioning()) } as any,
);
const insertArg = commentRepo.insertComment.mock.calls[0][0];
expect(insertArg.suggestedText).toBeNull();
});
});
describe('resolveComment — provenance & resolve notifications', () => {
it('stamps resolvedSource:"agent" when an agent resolves', async () => {
const { service, commentRepo } = makeService();
@@ -61,6 +61,8 @@ describe('CommentService — broadcast carries the agent avatar stack', () => {
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,
@@ -68,6 +70,7 @@ describe('CommentService — broadcast carries the agent avatar stack', () => {
collaborationGateway,
generalQueue,
notificationQueue,
auditService,
);
return { service, commentRepo, wsService };
@@ -14,6 +14,7 @@ describe('CommentService', () => {
{} as any, // collaborationGateway
{} as any, // generalQueue
{} as any, // notificationQueue
{} as any, // auditService
);
});
+191 -1
View File
@@ -1,7 +1,10 @@
import {
BadRequestException,
ConflictException,
ForbiddenException,
Inject,
Injectable,
InternalServerErrorException,
Logger,
NotFoundException,
} from '@nestjs/common';
@@ -26,6 +29,11 @@ import {
AuthProvenanceData,
agentSourceFields,
} from '../../common/decorators/auth-provenance.decorator';
import { AuditEvent, AuditResource } from '../../common/events/audit-events';
import {
AUDIT_SERVICE,
IAuditService,
} from '../../integrations/audit/audit.service';
@Injectable()
export class CommentService {
@@ -40,6 +48,7 @@ export class CommentService {
private generalQueue: Queue,
@InjectQueue(QueueName.NOTIFICATION_QUEUE)
private notificationQueue: Queue,
@Inject(AUDIT_SERVICE) private auditService: IAuditService,
) {}
async findById(commentId: string) {
@@ -78,15 +87,50 @@ export class CommentService {
}
}
const selection = createCommentDto?.selection?.substring(0, 250) ?? null;
// A suggested edit rewrites the exact text under an inline comment mark, so
// it is only meaningful on a top-level inline comment that carries a
// selection, and only if the suggestion actually changes that text.
let suggestedText: string | null = null;
if (
createCommentDto.suggestedText !== undefined &&
createCommentDto.suggestedText !== null
) {
if (createCommentDto.parentCommentId) {
throw new BadRequestException(
'A suggested edit can only be attached to a top-level comment, not a reply',
);
}
if (!selection || selection.trim().length === 0) {
throw new BadRequestException(
'A suggested edit requires an inline comment with a non-empty text selection',
);
}
const trimmed = createCommentDto.suggestedText.trim();
if (trimmed.length === 0) {
throw new BadRequestException('A suggested edit cannot be empty');
}
// A no-op suggestion (identical to the selection) is meaningless and would
// make "apply" indistinguishable from "already applied".
if (trimmed === selection.trim()) {
throw new BadRequestException(
'A suggested edit must differ from the selected text',
);
}
suggestedText = trimmed;
}
const inserted = await this.commentRepo.insertComment({
pageId: page.id,
content: commentContent,
selection: createCommentDto?.selection?.substring(0, 250) ?? null,
selection,
type: createCommentDto.type ?? 'page',
parentCommentId: createCommentDto?.parentCommentId,
creatorId: user.id,
workspaceId: workspaceId,
spaceId: page.spaceId,
suggestedText,
// Agent-edit provenance: the user stays creatorId; this only annotates the
// source. Normal user requests leave the column default ('user').
...agentSourceFields(provenance, 'createdSource', 'aiChatId'),
@@ -299,6 +343,152 @@ export class CommentService {
return updatedComment;
}
/**
* Apply the suggested edit carried by a top-level inline comment: atomically
* replace the text under the comment mark in the collaborative document with
* the comment's suggestedText, then stamp the applied fields and auto-resolve
* the thread. The controller authorizes (validateCanEdit); this re-checks the
* comment's own state so the invariant holds regardless of caller.
*/
async applySuggestion(
comment: Comment,
user: User,
provenance?: AuthProvenanceData,
): Promise<Comment> {
// Structural guards.
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 apply');
}
// State guards. Order matters — the already-applied check precedes the
// resolved check because an applied comment is normally also resolved.
//
// Already applied → IDEMPOTENT SUCCESS (issue #315 DoD: double-click /
// two-user race → idempotent "already applied", NOT a 409). The suggestion
// is already in the document, so do NOT call the collab gateway again.
// finalizeAppliedSuggestion re-fetches/broadcasts the same success shape as
// the applied branch and, when the thread is still open (the rare "applied
// but not resolved" crash window), self-heals it via resolveComment.
if (comment.suggestionAppliedAt) {
return this.finalizeAppliedSuggestion(comment, user, provenance);
}
// Not-yet-applied on a resolved thread → reject. The client hides the apply
// button once a thread is resolved; this is the defensive server check.
if (comment.resolvedAt) {
throw new BadRequestException(
'Cannot apply a suggested edit on a resolved comment thread',
);
}
// Derive the document name the same way create()/resolveComment() do for
// the comment marks: `page.${pageId}`.
const documentName = `page.${comment.pageId}`;
let verdict: { applied: boolean; currentText: string | null } | undefined;
try {
verdict = await this.collaborationGateway.handleYjsEvent(
'applyCommentSuggestion',
documentName,
{
commentId: comment.id,
expectedText: comment.selection,
newText: comment.suggestedText,
user,
},
);
} catch (error) {
// A throwing gateway (or the phase-3 fallback failing) is a hard error —
// never silently succeed, the document may or may not have changed.
this.logger.error(
`Failed to apply suggested edit for comment ${comment.id}`,
error,
);
throw new InternalServerErrorException('Failed to apply the suggested edit');
}
if (!verdict) {
// Should not happen given the phase-3 fallback; treat as a hard error
// rather than assuming success.
throw new InternalServerErrorException('Failed to apply the suggested edit');
}
if (verdict.applied === true) {
return this.finalizeAppliedSuggestion(comment, user, provenance);
}
// Idempotent branch: the mutation didn't run now, but the text under the
// mark is ALREADY the suggested text (double-click, two-user race, or a
// crash between the doc mutation and the DB write). Reconcile the DB /
// resolved state and report success — do NOT 409.
if (
verdict.applied === false &&
verdict.currentText === comment.suggestedText
) {
return this.finalizeAppliedSuggestion(comment, user, provenance);
}
// The commented text changed since the suggestion was made. Surface the
// current text so the client can tell the user what it is now.
throw new ConflictException({
message:
'The commented text changed since this suggestion was made; it was not applied.',
currentText: verdict.currentText,
});
}
/**
* Persist the applied stamps (idempotently), auto-resolve the thread and
* broadcast + audit the applied suggestion. Shared by the applied and the
* idempotent "already-applied" branches of applySuggestion.
*/
private async finalizeAppliedSuggestion(
comment: Comment,
user: User,
provenance?: AuthProvenanceData,
): Promise<Comment> {
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. 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,
});
this.auditService.log({
event: AuditEvent.COMMENT_SUGGESTION_APPLIED,
resourceType: AuditResource.COMMENT,
resourceId: comment.id,
spaceId: comment.spaceId,
metadata: { pageId: comment.pageId },
});
return updatedComment;
}
private async queueCommentNotification(
content: any,
oldMentionIds: string[],
@@ -0,0 +1,6 @@
import { IsUUID } from 'class-validator';
export class ApplySuggestionDto {
@IsUUID()
commentId: string;
}
@@ -1,4 +1,12 @@
import { IsIn, IsJSON, IsObject, IsOptional, IsString, IsUUID } from 'class-validator';
import {
IsIn,
IsJSON,
IsObject,
IsOptional,
IsString,
IsUUID,
MaxLength,
} from 'class-validator';
import { z } from 'zod';
const yjsIdSchema = z.object({
@@ -43,4 +51,12 @@ export class CreateCommentDto {
anchor: any;
head: any;
};
// Optional suggested replacement for the selected text (a "suggested edit").
// Only valid on a top-level inline comment that carries a non-empty selection;
// enforced in CommentService.create.
@IsOptional()
@IsString()
@MaxLength(2000)
suggestedText?: string;
}