From 5b988da5d6229ff688a632344c19502f1344bc48 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Sun, 1 Nov 2026 19:43:16 +0000 Subject: [PATCH 1/2] fix(review-desk): Use one rule for items that need a person Co-Authored-By: David Cramer --- .../junior-dashboard/e2e/review-desk.spec.ts | 2 +- packages/junior-dashboard/src/client/App.tsx | 2 +- .../src/client/review-desk/ReviewDeskPage.tsx | 2 +- .../src/client/review-desk/queries.ts | 2 +- .../src/mock-reporting/routes.ts | 2 +- .../junior/src/api/review-items/routes.ts | 14 ++----- packages/junior/src/api/schema/review-item.ts | 11 +++-- .../junior/src/chat/review-items/README.md | 17 ++++++-- .../junior/src/chat/review-items/access.ts | 17 +++++++- .../junior/src/chat/review-items/store.ts | 36 ++++++---------- .../junior/src/chat/slack/review-item-card.ts | 2 +- .../api/review-items/actions.test.ts | 41 +++++++++++++++---- 12 files changed, 92 insertions(+), 56 deletions(-) diff --git a/packages/junior-dashboard/e2e/review-desk.spec.ts b/packages/junior-dashboard/e2e/review-desk.spec.ts index d52e4bfe58..52514d8df0 100644 --- a/packages/junior-dashboard/e2e/review-desk.spec.ts +++ b/packages/junior-dashboard/e2e/review-desk.spec.ts @@ -100,7 +100,7 @@ async function mockReviewItems(page: Page) { await page.route("**/api/review-items**", async (route) => { const url = new URL(route.request().url()); const path = url.pathname.replace(/^.*\/api\/review-items/, ""); - if (path === "/count") return route.fulfill({ json: { mine: 3 } }); + if (path === "/count") return route.fulfill({ json: { needsViewer: 3 } }); if (path === "" || path === "/") { const q = url.searchParams.get("q")?.toLowerCase() ?? ""; // A search also finds archived items, after the open ones. diff --git a/packages/junior-dashboard/src/client/App.tsx b/packages/junior-dashboard/src/client/App.tsx index 80de7eb53c..e7ce52094b 100644 --- a/packages/junior-dashboard/src/client/App.tsx +++ b/packages/junior-dashboard/src/client/App.tsx @@ -119,7 +119,7 @@ export function DashboardShell() { loggedIn, pathname: location.pathname, primaryUserPages, - reviewCount: reviewCountQuery.data?.mine, + reviewCount: reviewCountQuery.data?.needsViewer, userPagesPending: userPagesQuery.isPending, }); diff --git a/packages/junior-dashboard/src/client/review-desk/ReviewDeskPage.tsx b/packages/junior-dashboard/src/client/review-desk/ReviewDeskPage.tsx index c42dae246e..919b8fe69a 100644 --- a/packages/junior-dashboard/src/client/review-desk/ReviewDeskPage.tsx +++ b/packages/junior-dashboard/src/client/review-desk/ReviewDeskPage.tsx @@ -202,7 +202,7 @@ export function ReviewDeskPage() { ) : empty ? ( - {q ? `No items match "${q}".` : "Nothing needs you right now."} + {q ? `No items match "${q}".` : "No open items."} ) : ( <> diff --git a/packages/junior-dashboard/src/client/review-desk/queries.ts b/packages/junior-dashboard/src/client/review-desk/queries.ts index 0680767db7..5b7b220806 100644 --- a/packages/junior-dashboard/src/client/review-desk/queries.ts +++ b/packages/junior-dashboard/src/client/review-desk/queries.ts @@ -88,7 +88,7 @@ export function useLinkedConversationTitles( }).map((query) => query.data ?? undefined); } -/** Count the open Review items assigned to the viewer, for the nav. */ +/** Count the open action items assigned to the viewer, for the nav. */ export function useReviewItemCountData(enabled: boolean) { return useQuery({ enabled, diff --git a/packages/junior-dashboard/src/mock-reporting/routes.ts b/packages/junior-dashboard/src/mock-reporting/routes.ts index 9aa86f72c1..e719ff2e09 100644 --- a/packages/junior-dashboard/src/mock-reporting/routes.ts +++ b/packages/junior-dashboard/src/mock-reporting/routes.ts @@ -303,7 +303,7 @@ export function createMockReportingApi(): Hono<{ jsonResponse(reviewItemListSchema, { items: [] }), ); app.get("/review-items/count", () => - jsonResponse(reviewItemCountSchema, { mine: 0 }), + jsonResponse(reviewItemCountSchema, { needsViewer: 0 }), ); app.get("/review-items/waiting-conversations", () => jsonResponse(reviewItemWaitingConversationsSchema, { conversationIds: [] }), diff --git a/packages/junior/src/api/review-items/routes.ts b/packages/junior/src/api/review-items/routes.ts index 89a4caa5f9..002331e253 100644 --- a/packages/junior/src/api/review-items/routes.ts +++ b/packages/junior/src/api/review-items/routes.ts @@ -28,7 +28,7 @@ import { ReviewItemActionError, } from "@/chat/review-items/actions"; import { - countReviewItems, + countReviewItemsNeedingUser, listWaitingConversationIds, searchReviewItems, type ReviewItem, @@ -45,10 +45,7 @@ const ERROR_STATUS = { } as const; /** Build the API payload of one Review item for its viewer. */ -export function reviewItemPayload( - item: ReviewItem, - viewer: User, -): ReviewItemPayload { +function reviewItemPayload(item: ReviewItem, viewer: User): ReviewItemPayload { return { addedByConversationId: item.addedByConversationId, assignedToEveryone: item.assignedToEveryone, @@ -122,16 +119,13 @@ export function createReviewItemRoutes(services: { app.get("/count", requireViewer, async (context) => { const viewer = context.get("viewer"); return jsonResponse(reviewItemCountSchema, { - mine: await countReviewItems(getDb(), reviewItemReaderForUser(viewer), { - assignee: { userId: viewer.id }, - statuses: ["open"], - }), + needsViewer: await countReviewItemsNeedingUser(getDb(), viewer.id), }); }); app.get("/waiting-conversations", requireViewer, async (context) => { const viewer = context.get("viewer"); return jsonResponse(reviewItemWaitingConversationsSchema, { - conversationIds: await listWaitingConversationIds(getDb(), viewer), + conversationIds: await listWaitingConversationIds(getDb(), viewer.id), }); }); app.get( diff --git a/packages/junior/src/api/schema/review-item.ts b/packages/junior/src/api/schema/review-item.ts index 6a14e3f414..0c224e26f5 100644 --- a/packages/junior/src/api/schema/review-item.ts +++ b/packages/junior/src/api/schema/review-item.ts @@ -72,15 +72,18 @@ export const reviewItemListSchema = z .object({ items: z.array(reviewItemSchema) }) .strict(); -/** Open Review items that wait on the viewer. */ +/** The count of Review items for the nav. */ export const reviewItemCountSchema = z .object({ - /** Open items assigned to the viewer as a User. */ - mine: z.number().int().nonnegative(), + /** + * Open action items assigned to the viewer as a User + * (`reviewItemNeedsUser`). Notices and items for Everyone do not count. + */ + needsViewer: z.number().int().nonnegative(), }) .strict(); -/** Conversations with an open item assigned to the viewer or Everyone. */ +/** Conversations linked to an open action item assigned to the viewer. */ export const reviewItemWaitingConversationsSchema = z .object({ conversationIds: z.array(z.string().min(1)) }) .strict(); diff --git a/packages/junior/src/chat/review-items/README.md b/packages/junior/src/chat/review-items/README.md index 3e8209b772..1bb035daaa 100644 --- a/packages/junior/src/chat/review-items/README.md +++ b/packages/junior/src/chat/review-items/README.md @@ -143,10 +143,19 @@ a new Turn. ## Waiting on you -A Conversation waits on a User when it has an open linked item that is -assigned to the User or to Everyone (`listWaitingConversationIds`). An item -with no assignees does not wait on anyone. The dashboard shows a lavender dot -for these Conversations. The dot replaces the other status markers. +An item needs a User when it is an open `action` item and the User is an +assignee (`reviewItemNeedsUser` in `access.ts`). A notice, an item assigned +to Everyone, and an item with no assignees need no one. One rule gives both +of these: + +- The count in the dashboard nav (`needsViewer` from + `/api/review-items/count`). +- The "Waiting on you" marker. A Conversation waits on a User when it is + linked to an item that needs the User (`listWaitingConversationIds`). The + dashboard shows a lavender dot for these Conversations. The dot replaces + the other status markers. + +The review desk list shows more: every open item that the reader can see. ## Slack cards diff --git a/packages/junior/src/chat/review-items/access.ts b/packages/junior/src/chat/review-items/access.ts index 437a7e0749..53498ff980 100644 --- a/packages/junior/src/chat/review-items/access.ts +++ b/packages/junior/src/chat/review-items/access.ts @@ -37,7 +37,7 @@ export interface ReviewItemReader { } /** Return whether two tenants are the same. Two missing tenants match. */ -export function isSameTenant( +function isSameTenant( a: ProviderTenant | undefined, b: ProviderTenant | undefined, ): boolean { @@ -71,6 +71,21 @@ export function reviewItemAssignedTo(userId: string): SQL { return sql`exists (select 1 from ${juniorReviewItemAssignees} where ${juniorReviewItemAssignees.itemId} = ${juniorReviewItems.id} and ${juniorReviewItemAssignees.userId} = ${userId})`; } +/** + * SQL: the Review item needs this User. Only an open `action` item that is + * assigned to the User needs them. A notice, an item for Everyone, and an + * item without assignees need no one. An assigned User can always see the + * item, so this rule needs no visibility check. The nav count and the + * "Waiting on you" marker use this rule. + */ +export function reviewItemNeedsUser(userId: string): SQL { + return and( + eq(juniorReviewItems.status, "open"), + eq(juniorReviewItems.kind, "action"), + reviewItemAssignedTo(userId), + )!; +} + /** SQL: the Review item links this Conversation. */ export function reviewItemLinkedTo(conversationId: string): SQL { return sql`exists (select 1 from ${juniorReviewItemConversations} where ${juniorReviewItemConversations.itemId} = ${juniorReviewItems.id} and ${juniorReviewItemConversations.conversationId} = ${conversationId})`; diff --git a/packages/junior/src/chat/review-items/store.ts b/packages/junior/src/chat/review-items/store.ts index 2045f25417..2f6ccf680b 100644 --- a/packages/junior/src/chat/review-items/store.ts +++ b/packages/junior/src/chat/review-items/store.ts @@ -10,7 +10,6 @@ import { or, type SQL, } from "drizzle-orm"; -import type { User } from "@sentry/junior-plugin-api"; import type { ProviderTenant } from "@/api/conversations/access"; import type { JuniorDatabase } from "@/db/db"; import { @@ -26,12 +25,12 @@ import type { import { reviewItemAssignedTo, reviewItemLinkedTo, - reviewItemReaderForUser, + reviewItemNeedsUser, reviewItemVisibleTo, type ReviewItemReader, } from "./access"; -export type ReviewItemKind = "notice" | "action"; +type ReviewItemKind = "notice" | "action"; /** * Close reason of an action item that a person answered with a reply in @@ -42,7 +41,7 @@ export type ReviewItemStatus = "open" | "done" | "dismissed"; export type ReviewItemVisibility = "public" | "private"; /** The thing a Review item is about, such as a pull request. */ -export interface ReviewItemSubject { +interface ReviewItemSubject { /** Stable id that Search matches, such as a URL. */ id: string; title?: string; @@ -431,7 +430,7 @@ export async function takeReviewItem( } /** Filters for Review item search. Every filter is optional. */ -export interface ReviewItemFilters { +interface ReviewItemFilters { /** One assigned User, or Everyone. */ assignee?: { everyone: true } | { userId: string }; automationId?: string; @@ -479,16 +478,15 @@ function searchConditions( return conditions; } -/** Count the Review items that a reader can see and that match filters. */ -export async function countReviewItems( +/** Count the open Review items that need a User (`reviewItemNeedsUser`). */ +export async function countReviewItemsNeedingUser( db: JuniorDatabase, - reader: ReviewItemReader, - filters: Omit, + userId: string, ): Promise { const rows = await db .select({ count: count() }) .from(juniorReviewItems) - .where(and(...searchConditions(reader, filters))); + .where(reviewItemNeedsUser(userId)); return rows[0]?.count ?? 0; } @@ -516,13 +514,12 @@ export async function searchReviewItems( } /** - * List the Conversations that wait on a User: each has an open linked item - * that the User can see and that is assigned to the User or to Everyone. An - * item without assignees does not wait on anyone. + * List the Conversations that wait on a User: each is linked to an open item + * that needs the User (`reviewItemNeedsUser`). */ export async function listWaitingConversationIds( db: JuniorDatabase, - user: User, + userId: string, ): Promise { const rows = await db .selectDistinct({ @@ -533,15 +530,6 @@ export async function listWaitingConversationIds( juniorReviewItems, eq(juniorReviewItems.id, juniorReviewItemConversations.itemId), ) - .where( - and( - eq(juniorReviewItems.status, "open"), - reviewItemVisibleTo(reviewItemReaderForUser(user)), - or( - eq(juniorReviewItems.assignedToEveryone, true), - reviewItemAssignedTo(user.id), - ), - ), - ); + .where(reviewItemNeedsUser(userId)); return rows.map((row) => row.conversationId).sort(); } diff --git a/packages/junior/src/chat/slack/review-item-card.ts b/packages/junior/src/chat/slack/review-item-card.ts index eeb7bbc481..2905fc5bdf 100644 --- a/packages/junior/src/chat/slack/review-item-card.ts +++ b/packages/junior/src/chat/slack/review-item-card.ts @@ -242,7 +242,7 @@ const formTargetSchema = z.object({ }); /** The card and thread that a Review item form answers. */ -export type SlackReviewItemFormTarget = z.output; +type SlackReviewItemFormTarget = z.output; /** Render the answer form of an action item. */ export function buildSlackReviewItemModal( 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 64de7d2e75..d167386bb8 100644 --- a/packages/junior/tests/integration/api/review-items/actions.test.ts +++ b/packages/junior/tests/integration/api/review-items/actions.test.ts @@ -98,6 +98,7 @@ describe("review item actions API", () => { const fixture = await createConversationFixture(); await slackConversation("slack:CPUB:1.000001"); await slackConversation("slack:CPUB:1.000002"); + await slackConversation("slack:CPUB:1.000003"); const ana = await user("ana@example.com"); const ben = await user("ben@example.com"); const cal = await user("cal@example.com"); @@ -110,10 +111,19 @@ describe("review item actions API", () => { dedupe_key: "fair-source:entry:acme", request: REQUEST, }); + // A notice does not need Ana, so it does not count. + await addItem({ + kind: "notice", + title: "Weekly report is ready", + body: "Read it when you have time.", + assignees: ["ana@example.com"], + conversation_ids: ["slack:CPUB:1.000003"], + dedupe_key: "report:weekly", + }); const anaCount = await (await api(fixture, ana).get("/count")).json(); - expect(anaCount).toEqual({ mine: 1 }); - // The linked Conversations wait on Ana, but not on Cal. + expect(anaCount).toEqual({ needsViewer: 1 }); + // The Conversations of the action item wait on Ana, but not on Cal. expect( await (await api(fixture, ana).get("/waiting-conversations")).json(), ).toEqual({ @@ -123,7 +133,10 @@ describe("review item actions API", () => { await (await api(fixture, cal).get("/waiting-conversations")).json(), ).toEqual({ conversationIds: [] }); const calList = await (await api(fixture, cal).get("")).json(); - expect(calList.items).toMatchObject([{ id, canAct: false }]); + expect(calList.items).toMatchObject([ + { kind: "notice" }, + { id, canAct: false }, + ]); // A person outside the Slack workspace does not see the public item. const outsider = { ...cal, identities: [] }; expect((await (await api(fixture, outsider).get("")).json()).items).toEqual( @@ -169,12 +182,15 @@ describe("review item actions API", () => { (await api(fixture, ben).act(id, { action: "dismiss" })).status, ).toBe(409); expect(await (await api(fixture, ben).get("/count")).json()).toEqual({ - mine: 0, + needsViewer: 0, }); expect( await (await api(fixture, ben).get("/waiting-conversations")).json(), ).toEqual({ conversationIds: [] }); - expect((await (await api(fixture, ben).get("")).json()).items).toEqual([]); + // The list still shows every open item that Ben can see. + expect( + (await (await api(fixture, ben).get("")).json()).items, + ).toMatchObject([{ kind: "notice" }]); // A search still finds the closed item. const found = await (await api(fixture, ben).get("?q=acme")).json(); expect(found.items).toMatchObject([{ id, status: "done" }]); @@ -185,15 +201,23 @@ describe("review item actions API", () => { await slackConversation("slack:CPUB:1.000001"); const cal = await user("cal@example.com"); const id = await addItem({ - kind: "notice", + kind: "action", title: "Pull request needs review", body: "It waits on a reviewer.", assignees: ["everyone"], dedupe_key: "pr:7", + request: REQUEST, }); const list = await (await api(fixture, cal).get("")).json(); expect(list.items).toMatchObject([{ id, canAct: true, canTake: true }]); + // An item for Everyone needs no one until a person takes it. + expect(await (await api(fixture, cal).get("/count")).json()).toEqual({ + needsViewer: 0, + }); + expect( + await (await api(fixture, cal).get("/waiting-conversations")).json(), + ).toEqual({ conversationIds: [] }); // The take committed, but its messages did not go out. A retry of the // take sends them. @@ -207,8 +231,11 @@ describe("review item actions API", () => { status: "open", }); expect(await (await api(fixture, cal).get("/count")).json()).toEqual({ - mine: 1, + needsViewer: 1, }); + expect( + await (await api(fixture, cal).get("/waiting-conversations")).json(), + ).toEqual({ conversationIds: ["slack:CPUB:1.000001"] }); const texts = await pendingTexts("slack:CPUB:1.000001"); expect(texts).toEqual([ expect.stringContaining("cal@example.com took review item"), From 30577815e0599ac8c1875df1b76a4c002a0db441 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Sun, 1 Nov 2026 20:14:46 +0000 Subject: [PATCH 2/2] docs(review-desk): Fix the waiting Conversations comment Co-Authored-By: David Cramer --- packages/junior-dashboard/src/client/review-desk/queries.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/junior-dashboard/src/client/review-desk/queries.ts b/packages/junior-dashboard/src/client/review-desk/queries.ts index 5b7b220806..3842dc20e9 100644 --- a/packages/junior-dashboard/src/client/review-desk/queries.ts +++ b/packages/junior-dashboard/src/client/review-desk/queries.ts @@ -141,7 +141,7 @@ const NO_CONVERSATIONS: ReadonlySet = new Set(); /** * Return the Conversations that wait on the viewer. Today only the review - * desk sets this: an open linked item assigned to the viewer or Everyone. + * desk sets this: a linked open action item assigned to the viewer. */ export function useWaitingConversationIds(): ReadonlySet { const query = useQuery({