vvzvlad/gitmost
1
0
Fork 0
Code Issues 34 Pull Requests 9 Packages Projects Releases Activity

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

Browse Source 
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>
This commit is contained in:
claude code agent 227
2026-07-04 18:22:35 +03:00
parent 8d8ecaed82
commit e6d8eda8e5
8 changed files with 357 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/client/src/features/comment/queries/comment-query.suggestion-outcome.test.tsx
+74
View File
@@ -28,6 +28,7 @@ vi.mock("@/features/comment/services/comment-service", () => ({
getPageComments: vi.fn(),
}));
import { notifications } from "@mantine/notifications";
import {
applySuggestion,
dismissSuggestion,
@@ -181,6 +182,39 @@ describe("useDismissSuggestionMutation — outcome handling (#329)", () => {
await waitFor(() => expect(result.current.isError).toBe(true));
expect(items(queryClient)).toHaveLength(0);
// #338 F3: the idempotent race must still fire the SUCCESS toast, not just
// silently drop the comment.
expect(notifications.show).toHaveBeenCalledWith({
message: "Suggestion dismissed",
});
});
it("dismiss 400 (thread still alive) → NOT a success, comment kept, no green toast (#338 F2)", async () => {
// 400 means the thread is alive (already resolved / a reply raced in).
// Narrowed onError: only 404 is a success-noop; 400 must surface a real error
// and keep the comment in the cache.
vi.mocked(dismissSuggestion).mockRejectedValue({
response: { status: 400 },
});
const { queryClient, wrapper } = seededClient(comment());
const { result } = renderHook(() => useDismissSuggestionMutation(), {
wrapper,
});
await result.current
.mutateAsync({ commentId: "c-1", pageId: PAGE_ID })
.catch(() => undefined);
await waitFor(() => expect(result.current.isError).toBe(true));
// Comment NOT dropped from the cache.
expect(items(queryClient)).toHaveLength(1);
// A real (red) error, never the success message.
expect(notifications.show).toHaveBeenCalledWith(
expect.objectContaining({ color: "red" }),
);
expect(notifications.show).not.toHaveBeenCalledWith({
message: "Suggestion dismissed",
});
});
it("APPLY idempotent race (404) → treated as success, comment removed from the list", async () => {
@@ -201,5 +235,45 @@ describe("useDismissSuggestionMutation — outcome handling (#329)", () => {
await waitFor(() => expect(result.current.isError).toBe(true));
expect(items(queryClient)).toHaveLength(0);
// #338 F3: the idempotent race must still fire the SUCCESS toast.
expect(notifications.show).toHaveBeenCalledWith({
message: "Suggestion applied",
});
});
it("APPLY 400 (thread resolved, not applied) → NOT a success, comment kept, red error (#338 F2)", async () => {
// apply's only 400 is "Cannot apply … on a resolved comment thread" — the
// thread was resolved (often with discussion) but NOT applied. It must be a
// real error surfacing the server message, and must NOT drop the live thread.
vi.mocked(applySuggestion).mockRejectedValue({
response: {
status: 400,
data: {
message: "Cannot apply a suggested edit on a resolved comment thread",
},
},
});
const { queryClient, wrapper } = seededClient(comment());
const { result } = renderHook(() => useApplySuggestionMutation(), {
wrapper,
});
await result.current
.mutateAsync({ commentId: "c-1", pageId: PAGE_ID })
.catch(() => undefined);
await waitFor(() => expect(result.current.isError).toBe(true));
// The live thread is NOT dropped from the cache.
expect(items(queryClient)).toHaveLength(1);
// Surfaces the server's specific message as a red error, never a success.
expect(notifications.show).toHaveBeenCalledWith(
expect.objectContaining({
message: "Cannot apply a suggested edit on a resolved comment thread",
color: "red",
}),
);
expect(notifications.show).not.toHaveBeenCalledWith({
message: "Suggestion applied",
});
});
});
apps/client/src/features/comment/queries/comment-query.ts
+34 -12
View File
@@ -253,14 +253,19 @@ export function useApplySuggestionMutation() {
},
onError: (err: any, variables) => {
const status = err?.response?.status;
// Idempotent races (double-click, or apply↔dismiss): after #329 an applied
// Idempotent race (double-click, or apply↔dismiss): after #329 an applied
// reply-less suggestion is hard-deleted, so a second/racing apply hits 404
// (already gone) 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) {
// (already gone). ONLY 404 is a real success-noop — drop it from the cache
// and report success, the user's intent is already satisfied (restores the
// #315 apply idempotency the ephemeral delete would otherwise break).
//
// 400 is NOT success (#338 F2): apply's only 400 is "Cannot apply … on a
// resolved comment thread" — the thread was resolved (often WITH a live
// discussion) but the edit was NOT applied. Treating it as "Suggestion
// applied" is a false success that also drops a live thread from the cache.
// The #315 idempotent repeat does NOT produce 400 (childless → 404;
// with-replies → 200), so we never lose idempotency by excluding it here.
if (status === 404) {
const cache = queryClient.getQueryData(RQ_KEY(variables.pageId)) as
| InfiniteData<IPagination<IComment>>
| undefined;
@@ -273,6 +278,20 @@ export function useApplySuggestionMutation() {
notifications.show({ message: t("Suggestion applied") });
return;
}
// 400 => the thread was resolved and the edit could not be applied. Show a
// real error and KEEP the comment in the cache (it is still alive). Prefer
// the server's specific message when it carries one.
if (status === 400) {
const serverMsg = err?.response?.data?.message;
notifications.show({
message:
typeof serverMsg === "string" && serverMsg.length > 0
? serverMsg
: t("Failed to apply suggestion"),
color: "red",
});
return;
}
// 409 => the commented text changed since the suggestion was made. Surface
// a specific message (with the current text) rather than a generic error.
const currentText = err?.response?.data?.currentText;
@@ -321,12 +340,15 @@ export function useDismissSuggestionMutation() {
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.
// Idempotent race (double-click, or apply↔dismiss): the comment is already
// gone (404). ONLY 404 is a real success-noop — drop it from the cache and
// report success, the user's intent (make it disappear) is satisfied.
//
// 400 is NOT success (#338 F2): it means the thread is still ALIVE (already
// resolved, or a reply raced in), so treating it as "dismissed" would drop
// a live thread from the cache. Show a real error and keep the comment.
const status = err?.response?.status;
if (status === 404 || status === 400) {
if (status === 404) {
const cache = queryClient.getQueryData(RQ_KEY(variables.pageId)) as
| InfiniteData<IPagination<IComment>>
| undefined;