feat(comment): эфемерные предложения-правки — Apply/Dismiss убирают комментарий (#329) #338

Merged

vvzvlad merged 3 commits from fix/329-ephemeral-suggestions into develop

2026-07-04 19:22:15 +03:00

Author	SHA1	Message	Date
claude code agent 227	d7fa6738e5	fix(comment): transactional childless-delete race fix + client dismiss gate + DB int-spec (#329 review round 2) F4 [critical] — the anti-join `DELETE … WHERE NOT EXISTS(child)` was still racy under Postgres READ COMMITTED: a reply INSERT holds FOR KEY SHARE on the parent; the DELETE's start snapshot doesn't see the uncommitted child (NOT EXISTS true), blocks on the reply's lock, and when the reply commits the parent was only LOCKED (not modified) so EvalPlanQual does NOT re-check → the DELETE proceeds and CASCADE destroys the just-committed reply. Replaced with a transaction: SELECT the parent FOR UPDATE (conflicts with the reply's FOR KEY SHARE → serializes the concurrent reply), re-check for a child with a FRESH statement in the same tx (a new RC snapshot sees a just-committed reply), delete only if still childless (return 1) else return 0 (caller resolves). The FOR UPDATE lock is held to end-of-tx so no reply can insert between the re-check and the delete. Signature unchanged, so the service + its mocked unit tests are untouched; docstrings updated. F5 [warning] — the client Dismiss button was gated only on canComment, but the server now gates dismiss on owner-or-space-admin, so a non-owner non-admin saw a button the server 403s. `canShowDismiss` now also requires `isOwnerOrAdmin = currentUser?.user?.id === comment.creatorId \|\| userSpaceRole === "admin"` (the same gate the comment delete-menu already uses); threaded into both call sites. F6 [warning] — added a REAL-DB int-spec (apps/server/test/integration/comment-delete-if-childless.int-spec.ts, + a createComment seeder): (a) childless → returns 1, row gone; (b) committed reply → returns 0, parent+reply survive; (c) CONCURRENCY — a second connection inserts a reply (FOR KEY SHARE) and commits mid-operation while deleteCommentIfChildless blocks on FOR UPDATE → asserts it returns 0 and both rows survive (a blind anti-join would lose the reply here). Ran against live Postgres — 3/3 pass. server tsc clean; comment jest 53 + int-spec 3 (live Postgres) pass. client tsc clean; comment vitest 56 pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-04 19:17:52 +03:00
claude code agent 227	e6d8eda8e5	fix(comment): dismiss owner/admin authz + atomic conditional delete + 404-only onError (#329 review) Maintainer escalation decision (B) + reviewer findings on the ephemeral- suggestion PR. Authz (decision B): POST /comments/dismiss-suggestion now gates the destructive branch on owner-OR-space-admin, mirroring POST /comments/delete exactly (same SpaceCaslAction.Manage / SpaceCaslSubject.Settings, same owner short-circuit, same ForbiddenException). A non-owner non-admin who tries to dismiss another's childless suggestion gets Forbidden before the service runs. Apply stays on canEdit (accepting an edit is the editor's semantics), unchanged. F1 [blocking] — atomic conditional delete closes the hasChildren→delete race. New repo `deleteCommentIfChildless(id)` runs a single `DELETE FROM comments WHERE id=:id AND NOT EXISTS (SELECT 1 FROM comments child WHERE child.parent_comment_id = comments.id)` (verified by compiling the Kysely expression to SQL — the correlated subquery references the OUTER comments.id). deleteEphemeralSuggestion strips the mark first, then the conditional delete: if it removed the row → commentDeleted + outcome 'deleted'; if a reply raced in (0 rows) → fall back to resolveComment (outcome 'resolved') so the discussion and the new reply survive. No reply can be cascade-deleted anymore. F2 [warning] — the apply/dismiss onError success-noop is narrowed from 404\|\|400 to 404 ONLY. A 400 means the comment is ALIVE (apply's 400 = the thread was resolved-not-applied), so it now shows a real error (surfacing the server message) and KEEPS the comment in cache instead of a false "applied" + dropping a live thread. F3 [suggestion] — the 404-race client tests assert the success toast fired. Tests: server — dismiss authz (owner ok / non-owner-non-admin Forbidden / space-admin ok), the delete→resolve race (hasChildren=false but conditional delete returns 0 → resolve, no commentDeleted), delete-path asserts switched to deleteCommentIfChildless; client — apply-400 and dismiss-400 (kept in cache, red, not success) + the toast assertions. server tsc clean, comment+collaboration jest green; client tsc clean, comment vitest 54 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-04 19:17:19 +03:00
claude code agent 227	8d8ecaed82	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 19:17:19 +03:00

Author

SHA1

Message

Date

claude code agent 227

d7fa6738e5

fix(comment): transactional childless-delete race fix + client dismiss gate + DB int-spec (#329 review round 2)

F4 [critical] — the anti-join `DELETE … WHERE NOT EXISTS(child)` was still racy
under Postgres READ COMMITTED: a reply INSERT holds FOR KEY SHARE on the parent;
the DELETE's start snapshot doesn't see the uncommitted child (NOT EXISTS true),
blocks on the reply's lock, and when the reply commits the parent was only LOCKED
(not modified) so EvalPlanQual does NOT re-check → the DELETE proceeds and CASCADE
destroys the just-committed reply. Replaced with a transaction: SELECT the parent
FOR UPDATE (conflicts with the reply's FOR KEY SHARE → serializes the concurrent
reply), re-check for a child with a FRESH statement in the same tx (a new RC
snapshot sees a just-committed reply), delete only if still childless (return 1)
else return 0 (caller resolves). The FOR UPDATE lock is held to end-of-tx so no
reply can insert between the re-check and the delete. Signature unchanged, so the
service + its mocked unit tests are untouched; docstrings updated.

F5 [warning] — the client Dismiss button was gated only on canComment, but the
server now gates dismiss on owner-or-space-admin, so a non-owner non-admin saw a
button the server 403s. `canShowDismiss` now also requires
`isOwnerOrAdmin = currentUser?.user?.id === comment.creatorId || userSpaceRole ===
"admin"` (the same gate the comment delete-menu already uses); threaded into both
call sites.

F6 [warning] — added a REAL-DB int-spec
(apps/server/test/integration/comment-delete-if-childless.int-spec.ts, + a
createComment seeder): (a) childless → returns 1, row gone; (b) committed reply →
returns 0, parent+reply survive; (c) CONCURRENCY — a second connection inserts a
reply (FOR KEY SHARE) and commits mid-operation while deleteCommentIfChildless
blocks on FOR UPDATE → asserts it returns 0 and both rows survive (a blind
anti-join would lose the reply here). Ran against live Postgres — 3/3 pass.

server tsc clean; comment jest 53 + int-spec 3 (live Postgres) pass. client tsc
clean; comment vitest 56 pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

2026-07-04 19:17:52 +03:00

claude code agent 227

e6d8eda8e5

fix(comment): dismiss owner/admin authz + atomic conditional delete + 404-only onError (#329 review)

Maintainer escalation decision (B) + reviewer findings on the ephemeral-
suggestion PR.

Authz (decision B): POST /comments/dismiss-suggestion now gates the destructive
branch on owner-OR-space-admin, mirroring POST /comments/delete exactly (same
SpaceCaslAction.Manage / SpaceCaslSubject.Settings, same owner short-circuit,
same ForbiddenException). A non-owner non-admin who tries to dismiss another's
childless suggestion gets Forbidden before the service runs. Apply stays on
canEdit (accepting an edit is the editor's semantics), unchanged.

F1 [blocking] — atomic conditional delete closes the hasChildren→delete race.
New repo `deleteCommentIfChildless(id)` runs a single
`DELETE FROM comments WHERE id=:id AND NOT EXISTS (SELECT 1 FROM comments child
WHERE child.parent_comment_id = comments.id)` (verified by compiling the Kysely
expression to SQL — the correlated subquery references the OUTER comments.id).
deleteEphemeralSuggestion strips the mark first, then the conditional delete: if
it removed the row → commentDeleted + outcome 'deleted'; if a reply raced in
(0 rows) → fall back to resolveComment (outcome 'resolved') so the discussion and
the new reply survive. No reply can be cascade-deleted anymore.

F2 [warning] — the apply/dismiss onError success-noop is narrowed from 404||400
to 404 ONLY. A 400 means the comment is ALIVE (apply's 400 = the thread was
resolved-not-applied), so it now shows a real error (surfacing the server
message) and KEEPS the comment in cache instead of a false "applied" + dropping a
live thread.

F3 [suggestion] — the 404-race client tests assert the success toast fired.

Tests: server — dismiss authz (owner ok / non-owner-non-admin Forbidden /
space-admin ok), the delete→resolve race (hasChildren=false but conditional
delete returns 0 → resolve, no commentDeleted), delete-path asserts switched to
deleteCommentIfChildless; client — apply-400 and dismiss-400 (kept in cache, red,
not success) + the toast assertions.

server tsc clean, comment+collaboration jest green; client tsc clean, comment
vitest 54 passed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

2026-07-04 19:17:19 +03:00

claude code agent 227

8d8ecaed82

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 19:17:19 +03:00

feat(comment): эфемерные предложения-правки — Apply/Dismiss убирают комментарий (#329) #338

3 Commits