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..3842dc20e9 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,
@@ -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({
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"),