From fdbf15597ecba4f9114ea55fb41dde047dc7a0a8 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Mon, 2 Nov 2026 20:55:13 +0000 Subject: [PATCH] fix(review-items): Let a person only dismiss a notice A notice has nothing to answer, and a plugin often closes it by itself, so Done and Dismiss meant the same thing. Remove Done from the Slack card and the review desk, and remove the complete action. Drop "or say why not" from the GitHub ready-to-merge notice, because a reply does not close a notice. Co-Authored-By: David Cramer <23610+dcramer@users.noreply.github.com> --- .../client/review-desk/ReviewItemActions.tsx | 11 +-------- .../src/client/review-desk/queries.ts | 2 +- .../src/webhooks/pull-request-ready.ts | 2 +- .../tests/pull-request-ready.test.ts | 2 +- packages/junior/src/api/schema/review-item.ts | 3 +-- .../src/chat/ingress/slack-review-item.ts | 4 ---- .../junior/src/chat/review-items/actions.ts | 11 ++------- .../junior/src/chat/review-items/messages.ts | 6 ----- .../junior/src/chat/slack/review-item-card.ts | 24 ++++--------------- .../api/review-items/actions.test.ts | 5 +--- .../slack/review-item-card-webhook.test.ts | 15 ++++++++---- 11 files changed, 23 insertions(+), 62 deletions(-) diff --git a/packages/junior-dashboard/src/client/review-desk/ReviewItemActions.tsx b/packages/junior-dashboard/src/client/review-desk/ReviewItemActions.tsx index ddd1282b09..56d76dc117 100644 --- a/packages/junior-dashboard/src/client/review-desk/ReviewItemActions.tsx +++ b/packages/junior-dashboard/src/client/review-desk/ReviewItemActions.tsx @@ -102,7 +102,7 @@ export function ReviewItemActions(props: { item: ReviewItemPayload }) { } } - // Take and Dismiss are quiet, so the answer or Done stays the main action. + // Take and Dismiss are quiet, so the answer stays the main action. const secondaryActions = ( <> {item.canTake ? ( @@ -149,15 +149,6 @@ export function ReviewItemActions(props: { item: ReviewItemPayload }) { ) : (
- {item.kind === "notice" ? ( - - ) : null}
{secondaryActions}
diff --git a/packages/junior-dashboard/src/client/review-desk/queries.ts b/packages/junior-dashboard/src/client/review-desk/queries.ts index 315d43a8e9..fa0c7f4d09 100644 --- a/packages/junior-dashboard/src/client/review-desk/queries.ts +++ b/packages/junior-dashboard/src/client/review-desk/queries.ts @@ -137,7 +137,7 @@ export function useBlockedAutomationsData(enabled: boolean) { }); } -/** Take, submit, complete, or dismiss one Review item. */ +/** Take, submit, or dismiss one Review item. */ export function useReviewItemAction() { const queryClient = useQueryClient(); return useMutation({ diff --git a/packages/junior-github/src/webhooks/pull-request-ready.ts b/packages/junior-github/src/webhooks/pull-request-ready.ts index 560442728d..38490b62ed 100644 --- a/packages/junior-github/src/webhooks/pull-request-ready.ts +++ b/packages/junior-github/src/webhooks/pull-request-ready.ts @@ -137,7 +137,7 @@ async function syncOne( if (ready) { await items.upsert({ assigneeIdentityIds: [opener.identityId], - body: `**${state.title}** is approved, and GitHub says that it can merge. Merge it, or say why not: ${state.url}`, + body: `**${state.title}** is approved, and GitHub says that it can merge: ${state.url}`, dedupeKey, subject: { id: state.url, title: name, url: state.url }, title: `Ready to merge: ${name}`, diff --git a/packages/junior-github/tests/pull-request-ready.test.ts b/packages/junior-github/tests/pull-request-ready.test.ts index 4f1aba01d3..0450a0d20f 100644 --- a/packages/junior-github/tests/pull-request-ready.test.ts +++ b/packages/junior-github/tests/pull-request-ready.test.ts @@ -300,7 +300,7 @@ describe("ready to merge notice", () => { kind: "upsert", notice: { assigneeIdentityIds: ["identity-requester"], - body: `**Add the review desk notice** is approved, and GitHub says that it can merge. Merge it, or say why not: ${PULL_URL}`, + body: `**Add the review desk notice** is approved, and GitHub says that it can merge: ${PULL_URL}`, dedupeKey: DEDUPE_KEY, subject: { id: PULL_URL, diff --git a/packages/junior/src/api/schema/review-item.ts b/packages/junior/src/api/schema/review-item.ts index 65e782dab5..8dac4531ec 100644 --- a/packages/junior/src/api/schema/review-item.ts +++ b/packages/junior/src/api/schema/review-item.ts @@ -28,7 +28,7 @@ export const reviewItemSchema = z assignees: z.array(personSchema), automationId: z.string().min(1).optional(), body: z.string(), - /** True when the viewer can take, submit, complete, or dismiss it. */ + /** True when the viewer can take, submit, or dismiss it. */ canAct: z.boolean(), /** True when the viewer can act and is not already the only assignee. */ canTake: z.boolean(), @@ -114,7 +114,6 @@ export const reviewItemParamsSchema = z /** One person action on a Review item. */ export const reviewItemActionSchema = z.discriminatedUnion("action", [ z.object({ action: z.literal("take") }).strict(), - z.object({ action: z.literal("complete") }).strict(), z.object({ action: z.literal("dismiss") }).strict(), z .object({ diff --git a/packages/junior/src/chat/ingress/slack-review-item.ts b/packages/junior/src/chat/ingress/slack-review-item.ts index 5c821aec08..afc2f1abbc 100644 --- a/packages/junior/src/chat/ingress/slack-review-item.ts +++ b/packages/junior/src/chat/ingress/slack-review-item.ts @@ -283,10 +283,6 @@ export async function handleSlackReviewItemClick(args: { services, target, }; - if (click.kind === "done") { - await act({ ...common, action: "complete" }); - return; - } if (click.kind === "dismiss") { await act({ ...common, action: "dismiss" }); return; diff --git a/packages/junior/src/chat/review-items/actions.ts b/packages/junior/src/chat/review-items/actions.ts index ff014f124a..f5aefd2e9b 100644 --- a/packages/junior/src/chat/review-items/actions.ts +++ b/packages/junior/src/chat/review-items/actions.ts @@ -25,7 +25,7 @@ import { } from "./store"; /** What a person can do with a Review item. */ -export type ReviewItemAction = "take" | "submit" | "complete" | "dismiss"; +export type ReviewItemAction = "take" | "submit" | "dismiss"; /** An expected failure of a person action, for the API to report. */ export class ReviewItemActionError extends Error { @@ -61,7 +61,6 @@ export function reviewItemChange( ): ReviewItemChange { if (action === "take") return { kind: "taken", user }; if (action === "submit") return { kind: "submitted", user }; - if (action === "complete") return { kind: "completed", user }; return { kind: "dismissed", user }; } @@ -105,7 +104,7 @@ function submittedAnswer( /** * Do one person action on a Review item and tell its linked Conversations. - * The first submit, complete, or dismiss closes the item for every + * The first submit or dismiss closes the item for every * assignee. A retry of the same close sends any missing messages again and * succeeds, so a client can retry after a failure. With `firstCloseOnly`, * only the call that closes the item succeeds, and every other call gets a @@ -169,12 +168,6 @@ export async function actOnReviewItem(args: { "Only an assignee can act on this item.", ); } - if (action === "complete" && item.kind === "action") { - throw new ReviewItemActionError( - "invalid", - "An action item needs an answer. Submit it or dismiss it.", - ); - } const answer = action === "submit" ? submittedAnswer(item, args.answer) : undefined; closedNow = await closeReviewItem(db, { diff --git a/packages/junior/src/chat/review-items/messages.ts b/packages/junior/src/chat/review-items/messages.ts index 191b06f5f0..457c32b0e5 100644 --- a/packages/junior/src/chat/review-items/messages.ts +++ b/packages/junior/src/chat/review-items/messages.ts @@ -22,7 +22,6 @@ import type { ReviewItem } from "./store"; /** One change to a Review item that linked Conversations learn about. */ export type ReviewItemChange = | { kind: "submitted"; user: User } - | { kind: "completed"; user: User } | { kind: "dismissed"; user: User } | { kind: "taken"; user: User } | { kind: "replied"; who: string } @@ -52,11 +51,6 @@ export function renderReviewItemMessage( ? formatUserInputAnswer(item.request, item.answer) : "(no answer)", ].join("\n"); - case "completed": - return [ - `${personName(change.user)} marked ${label} done.`, - ...question, - ].join("\n"); case "dismissed": return [ `${personName(change.user)} dismissed ${label}.`, diff --git a/packages/junior/src/chat/slack/review-item-card.ts b/packages/junior/src/chat/slack/review-item-card.ts index d49a5473f8..32d62e3451 100644 --- a/packages/junior/src/chat/slack/review-item-card.ts +++ b/packages/junior/src/chat/slack/review-item-card.ts @@ -22,7 +22,6 @@ export const SLACK_REVIEW_ITEM_VIEW_ID = "junior_review_item"; const ACTION_ID_PREFIX = "junior_review_item:"; const CHOICE_ACTION_ID_PREFIX = `${ACTION_ID_PREFIX}choice:`; const ANSWER_ACTION_ID = `${ACTION_ID_PREFIX}answer`; -const DONE_ACTION_ID = `${ACTION_ID_PREFIX}done`; const DISMISS_ACTION_ID = `${ACTION_ID_PREFIX}dismiss`; // Slack allows 12,000 characters in all markdown blocks of one message. const MAX_BODY_CHARS = 3_000; @@ -67,18 +66,9 @@ function controls(item: ReviewItem): Button[] { text: plainText("Dismiss"), value: item.id, }; - if (item.kind === "notice" || !item.request) { - return [ - { - type: "button", - action_id: DONE_ACTION_ID, - style: "primary", - text: plainText("Done"), - value: item.id, - }, - dismiss, - ]; - } + // A notice has nothing to answer, and it can close by itself, so a person + // can only dismiss it. + if (item.kind === "notice" || !item.request) return [dismiss]; const buttonField = buttonUserInputField(item.request); if (!buttonField) { return [ @@ -142,8 +132,8 @@ function closedNote(item: ReviewItem): string { /** * Render a Review item as one Slack message. An open item shows its * controls: one button per choice when one click can answer it, an Answer - * button that opens a form otherwise, and Done for a notice. A closed item - * shows a note instead, so nobody can act on it again. + * button that opens a form otherwise, and only Dismiss for a notice. A + * closed item shows a note instead, so nobody can act on it again. */ export function buildSlackReviewItemCard(item: ReviewItem): { blocks: KnownBlock[]; @@ -191,7 +181,6 @@ export function buildHiddenSlackReviewItemCard(item: ReviewItem): { export type SlackReviewItemClick = | { kind: "choice"; itemId: string; label: string } | { kind: "answer"; itemId: string } - | { kind: "done"; itemId: string } | { kind: "dismiss"; itemId: string }; const choiceValueSchema = z.object({ @@ -226,9 +215,6 @@ export function parseSlackReviewItemClick(action: { if (action.action_id === ANSWER_ACTION_ID) { return { kind: "answer", itemId: value }; } - if (action.action_id === DONE_ACTION_ID) { - return { kind: "done", itemId: value }; - } if (action.action_id === DISMISS_ACTION_ID) { return { kind: "dismiss", itemId: value }; } diff --git a/packages/junior/tests/integration/api/review-items/actions.test.ts b/packages/junior/tests/integration/api/review-items/actions.test.ts index 5375f33f34..f1605970a6 100644 --- a/packages/junior/tests/integration/api/review-items/actions.test.ts +++ b/packages/junior/tests/integration/api/review-items/actions.test.ts @@ -151,11 +151,8 @@ describe("review item actions API", () => { [], ); expect( - (await api(fixture, cal).act(id, { action: "complete" })).status, + (await api(fixture, cal).act(id, { action: "dismiss" })).status, ).toBe(403); - expect( - (await api(fixture, ana).act(id, { action: "complete" })).status, - ).toBe(400); const submitted = await api(fixture, ana).act(id, { action: "submit", diff --git a/packages/junior/tests/integration/slack/review-item-card-webhook.test.ts b/packages/junior/tests/integration/slack/review-item-card-webhook.test.ts index e0d92e2a5a..547cdf5527 100644 --- a/packages/junior/tests/integration/slack/review-item-card-webhook.test.ts +++ b/packages/junior/tests/integration/slack/review-item-card-webhook.test.ts @@ -578,13 +578,18 @@ describe("Slack webhook: Review item cards", () => { dedupe_key: "deploy-key", }); + const posts = slackApiOutbox.messages(); + expect(posts.map((post) => post.params.channel)).toEqual(["C123"]); + // A notice has nothing to answer, so its only control is Dismiss. expect( - slackApiOutbox.messages().map((post) => post.params.channel), - ).toEqual(["C123"]); + blocksOf(posts[0]) + .find((block) => block.type === "actions") + ?.elements?.map((element) => element.action_id), + ).toEqual(["junior_review_item:dismiss"]); // Only an assignee can act. Ben gets an ephemeral note. await click({ - actionId: "junior_review_item:done", + actionId: "junior_review_item:dismiss", actionTs: "1712345.0003", itemId: item.id, userId: "U456", @@ -594,7 +599,7 @@ describe("Slack webhook: Review item cards", () => { ); await click({ - actionId: "junior_review_item:done", + actionId: "junior_review_item:dismiss", actionTs: "1712345.0004", itemId: item.id, userId: "U123", @@ -602,7 +607,7 @@ describe("Slack webhook: Review item cards", () => { const updates = slackApiOutbox.calls("chat.update"); expect(updates).toHaveLength(1); expect(JSON.stringify(blocksOf(updates[0]))).toContain( - "ana@example.com marked this done", + "ana@example.com dismissed this", ); });