fix(review-desk): Use one rule for items that need a person

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.

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,
   });
 

packages/junior-dashboard/src/client/review-desk/ReviewDeskPage.tsx

@@ -202,7 +202,7 @@ export function ReviewDeskPage() {
             <ReviewListLoading />
           ) : empty ? (
             <EmptyMessage>
-              {q ? `No items match "${q}".` : "Nothing needs you right now."}
+              {q ? `No items match "${q}".` : "No open items."}
             </EmptyMessage>
           ) : (
             <>

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,
@@ -141,7 +141,7 @@ const NO_CONVERSATIONS: ReadonlySet<string> = 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<string> {
   const query = useQuery({

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: [] }),

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(

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();

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
 

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})`;

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<ReviewItemFilters, "limit">,
+  userId: string,
 ): Promise<number> {
   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<string[]> {
   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();
 }

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<typeof formTargetSchema>;
+type SlackReviewItemFormTarget = z.output<typeof formTargetSchema>;
 
 /** Render the answer form of an action item. */
 export function buildSlackReviewItemModal(

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("[email protected]");
     const ben = await user("[email protected]");
     const cal = await user("[email protected]");
@@ -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: ["[email protected]"],
+      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("[email protected]");
     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("[email protected] took review item"),