From dbbd417256c52a8c16e020537f6aee5e91d9e0ae Mon Sep 17 00:00:00 2001 From: Sleeyax Date: Mon, 12 Oct 2026 13:46:19 +0200 Subject: [PATCH 01/16] refactor(guided-review): keep the review list's preferences and check-offs per tab The preferences file and the check-offs file each hold an `assigned` and an `authored` key, so the Authored by me tab can have its own filters, sort, columns and check-offs. A file in the old flat shape reads as `assigned`, so upgrading keeps every filter and check-off the reviewer has now. `authored` starts at its own defaults: the six review statuses with all but `approved` shown, and the columns check-off, change, title, reviewers, updated, CI, state and local. Each field still falls back to its default on its own, per tab. The Assigned tab reads and writes the `assigned` key and behaves as before; authored check-offs are kept untouched until that tab lists anything. Refs #150 Co-Authored-By: Claude Opus 5.5 --- .../guided-review/client/inbox-filter.test.ts | 6 +- plugins/guided-review/client/inbox-filter.ts | 12 +- .../guided-review/client/inbox-preferences.ts | 20 +-- plugins/guided-review/client/inbox.tsx | 36 ++--- .../guided-review/server/inbox-check-offs.ts | 49 +++++-- .../server/inbox-preferences.test.ts | 137 ++++++++++++++---- .../guided-review/server/inbox-preferences.ts | 7 +- plugins/guided-review/server/inbox-tabs.ts | 10 ++ .../guided-review/server/review-inbox.test.ts | 36 ++++- .../guided-review/shared/inbox-preferences.ts | 82 +++++++---- plugins/guided-review/shared/inbox.ts | 11 ++ 11 files changed, 291 insertions(+), 115 deletions(-) create mode 100644 plugins/guided-review/server/inbox-tabs.ts diff --git a/plugins/guided-review/client/inbox-filter.test.ts b/plugins/guided-review/client/inbox-filter.test.ts index ab5c788..005af50 100644 --- a/plugins/guided-review/client/inbox-filter.test.ts +++ b/plugins/guided-review/client/inbox-filter.test.ts @@ -1,6 +1,6 @@ import assert from "node:assert/strict"; import test from "node:test"; -import { DEFAULT_INBOX_PREFERENCES, type InboxPreferences } from "../shared/inbox-preferences.ts"; +import { DEFAULT_INBOX_PREFERENCES, type AssignedPreferences } from "../shared/inbox-preferences.ts"; import type { InboxItem } from "../shared/inbox.ts"; import { age, attentionCount, countLabel, hiddenCheckedOff, matchesSearch, needsAttention, statesLabel, visibleItems } from "./inbox-filter.ts"; @@ -32,7 +32,7 @@ function item(number: number, overrides: Partial = {}): InboxItem { }; } -const ALL: InboxPreferences = { ...DEFAULT_INBOX_PREFERENCES, states: ["requested", "commented", "changes-requested", "approved", "unapproved"] }; +const ALL: AssignedPreferences = { ...DEFAULT_INBOX_PREFERENCES.assigned, states: ["requested", "commented", "changes-requested", "approved", "unapproved"] }; function numbers(items: InboxItem[]): number[] { return items.map((entry) => entry.number); @@ -45,7 +45,7 @@ test("by default the newest update comes first and what the reviewer approved is item(3, { updatedAt: "2026-09-02T00:00:00Z" }), ]; - assert.deepEqual(numbers(visibleItems(items, DEFAULT_INBOX_PREFERENCES, "")), [3, 1]); + assert.deepEqual(numbers(visibleItems(items, DEFAULT_INBOX_PREFERENCES.assigned, "")), [3, 1]); assert.deepEqual(numbers(visibleItems(items, ALL, "")), [2, 3, 1]); }); diff --git a/plugins/guided-review/client/inbox-filter.ts b/plugins/guided-review/client/inbox-filter.ts index 894b6d5..ab75322 100644 --- a/plugins/guided-review/client/inbox-filter.ts +++ b/plugins/guided-review/client/inbox-filter.ts @@ -1,4 +1,4 @@ -import { DEFAULT_INBOX_PREFERENCES, type InboxPreferences, type InboxSortKey } from "../shared/inbox-preferences.ts"; +import { DEFAULT_INBOX_PREFERENCES, type AssignedPreferences, type InboxSortKey } from "../shared/inbox-preferences.ts"; import { ReviewerStateSchema, type InboxItem, type ReviewerState } from "../shared/inbox.ts"; import { numberLabel } from "../shared/reference.ts"; @@ -30,7 +30,7 @@ const NONE_HELD: ReadonlySet = new Set(); * The items the list shows, filtered as the preferences say and by the search, in their sort order; ties go to the latest update. * A `held` row is shown whether or not the check-off filter would take it, so a row just checked off or unchecked stays put. */ -export function visibleItems(items: readonly InboxItem[], preferences: InboxPreferences, query: string, held = NONE_HELD): InboxItem[] { +export function visibleItems(items: readonly InboxItem[], preferences: AssignedPreferences, query: string, held = NONE_HELD): InboxItem[] { const value = SORT_VALUES[preferences.sort.key]; const direction = preferences.sort.descending ? -1 : 1; return items @@ -39,12 +39,12 @@ export function visibleItems(items: readonly InboxItem[], preferences: InboxPref } /** How many checked-off items the list leaves out that every other filter and the search would show. */ -export function hiddenCheckedOff(items: readonly InboxItem[], preferences: InboxPreferences, query: string, held = NONE_HELD): number { +export function hiddenCheckedOff(items: readonly InboxItem[], preferences: AssignedPreferences, query: string, held = NONE_HELD): number { return items.filter((item) => item.checkedOff && !held.has(item.url) && !passesCheckOff(item, preferences) && matchesFilters(item, preferences, query)).length; } /** The list's default filters with only what needs the reviewer's attention kept, whatever the reviewer last set. */ -const ATTENTION_FILTERS: InboxPreferences = { ...DEFAULT_INBOX_PREFERENCES, needsAttention: true }; +const ATTENTION_FILTERS: AssignedPreferences = { ...DEFAULT_INBOX_PREFERENCES.assigned, needsAttention: true }; /** How many items need the reviewer's attention: the sidebar's count. */ export function attentionCount(items: readonly InboxItem[]): number { @@ -52,7 +52,7 @@ export function attentionCount(items: readonly InboxItem[]): number { } /** Whether the check-off filter shows the item, which a held row is not asked. */ -export function passesCheckOff(item: InboxItem, preferences: InboxPreferences): boolean { +export function passesCheckOff(item: InboxItem, preferences: AssignedPreferences): boolean { switch (preferences.checkedOff) { case "hide": return !item.checkedOff; @@ -63,7 +63,7 @@ export function passesCheckOff(item: InboxItem, preferences: InboxPreferences): } } -function matchesFilters(item: InboxItem, preferences: InboxPreferences, query: string): boolean { +function matchesFilters(item: InboxItem, preferences: AssignedPreferences, query: string): boolean { return ( (preferences.provider === "all" || item.forge === preferences.provider) && preferences.states.includes(item.state) && diff --git a/plugins/guided-review/client/inbox-preferences.ts b/plugins/guided-review/client/inbox-preferences.ts index d6b9dd3..c4ce563 100644 --- a/plugins/guided-review/client/inbox-preferences.ts +++ b/plugins/guided-review/client/inbox-preferences.ts @@ -2,11 +2,11 @@ import { useRpc } from "@getpaseo/plugin/client"; import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; import * as contracts from "../shared/contracts.ts"; import { PLUGIN_ID } from "../shared/identity.ts"; -import { DEFAULT_INBOX_PREFERENCES, type InboxPreferences } from "../shared/inbox-preferences.ts"; +import { DEFAULT_INBOX_PREFERENCES, type InboxPreferences, type InboxTab } from "../shared/inbox-preferences.ts"; -export type InboxPreferencesControl = { - preferences: InboxPreferences; - change(update: Partial): void; +export type InboxPreferencesControl = { + preferences: InboxPreferences[Tab]; + change(update: Partial): void; /** Why the latest change could not be kept, as a sentence. */ error: string | null; }; @@ -14,11 +14,10 @@ export type InboxPreferencesControl = { const QUERY_KEY = [PLUGIN_ID, "inbox-preferences"]; /** - * The review list's filters, sort and columns as the daemon keeps them. A change shows at once and - * is saved whole behind it; the answer is not read back, since a slower answer to an earlier save - * would undo a later change. + * One tab's filters, sort and columns as the daemon keeps them. + * A change shows at once and is saved behind it, with the other tab's, whole; the answer is not read back, since a slower answer to an earlier save would undo a later change. */ -export function useInboxPreferences(): InboxPreferencesControl { +export function useInboxPreferences(tab: Tab): InboxPreferencesControl { const getPreferences = useRpc(contracts.getInboxPreferences); const savePreferences = useRpc(contracts.saveInboxPreferences); const queryClient = useQueryClient(); @@ -26,9 +25,10 @@ export function useInboxPreferences(): InboxPreferencesControl { const save = useMutation({ mutationFn: savePreferences }); return { - preferences: stored.data ?? DEFAULT_INBOX_PREFERENCES, + preferences: (stored.data ?? DEFAULT_INBOX_PREFERENCES)[tab], change(update) { - const next = { ...(queryClient.getQueryData(QUERY_KEY) ?? DEFAULT_INBOX_PREFERENCES), ...update }; + const current = queryClient.getQueryData(QUERY_KEY) ?? DEFAULT_INBOX_PREFERENCES; + const next: InboxPreferences = { ...current, [tab]: { ...current[tab], ...update } }; queryClient.setQueryData(QUERY_KEY, next); save.mutate(next); }, diff --git a/plugins/guided-review/client/inbox.tsx b/plugins/guided-review/client/inbox.tsx index d523333..14dcda3 100644 --- a/plugins/guided-review/client/inbox.tsx +++ b/plugins/guided-review/client/inbox.tsx @@ -5,7 +5,7 @@ import { useQuery } from "@tanstack/react-query"; import React, { useContext, useState } from "react"; import { Pressable, Text, View } from "react-native"; import * as contracts from "../shared/contracts.ts"; -import { CHECK_OFF_FILTERS, INBOX_COLUMNS, type CheckOffFilter, type InboxColumn, type InboxPreferences, type InboxSortKey } from "../shared/inbox-preferences.ts"; +import { ASSIGNED_COLUMNS, CHECK_OFF_FILTERS, type AssignedColumn, type AssignedPreferences, type CheckOffFilter, type InboxSortKey } from "../shared/inbox-preferences.ts"; import { headMoved, ReviewerStateSchema, type Inbox, type InboxItem, type LocalReview, type ReviewerState } from "../shared/inbox.ts"; import { numberLabel } from "../shared/reference.ts"; import { Button } from "./button.tsx"; @@ -27,13 +27,13 @@ export function ReviewInbox({ colors, compact, openPanel }: { colors: Colors; co const getInbox = useRpc(contracts.getInbox); const inbox = useQuery({ queryKey: INBOX_QUERY_KEY, queryFn: () => getInbox({}), refetchOnWindowFocus: false }); const starts = useReviewStarts(openPanel, inbox.data?.items ?? []); - const { preferences, change: changePreferences, error } = useInboxPreferences(); + const { preferences, change: changePreferences, error } = useInboxPreferences("assigned"); const checkOff = useCheckOff(); const [query, setSearch] = useState(""); const [width, setWidth] = useState(null); const [choosing, setChoosing] = useState<"columns" | "states" | null>(null); - const change = (update: Partial) => { + const change = (update: Partial) => { checkOff.release(); changePreferences(update); }; @@ -103,7 +103,7 @@ export function ReviewInbox({ colors, compact, openPanel }: { colors: Colors; co ); } -const COLUMN_HINTS: Partial> = { +const COLUMN_HINTS: Partial> = { checked: "Set a review aside here until it is pushed to or you are asked again", title: "Always shown", state: "Where you stand, and whether it changed since your review", @@ -120,16 +120,16 @@ function ColumnChooser({ }: { open: boolean; onOpenChange: (open: boolean) => void; - columns: readonly InboxColumn[]; - change: (update: Partial) => void; + columns: readonly AssignedColumn[]; + change: (update: Partial) => void; }) { - const toggle = (column: InboxColumn, shown: boolean) => - change({ columns: INBOX_COLUMNS.filter((candidate) => (candidate === column ? shown : columns.includes(candidate))) }); + const toggle = (column: AssignedColumn, shown: boolean) => + change({ columns: ASSIGNED_COLUMNS.filter((candidate) => (candidate === column ? shown : columns.includes(candidate))) }); return ( - {INBOX_COLUMNS.map((column) => ( + {ASSIGNED_COLUMNS.map((column) => ( void; states: readonly ReviewerState[]; - change: (update: Partial) => void; + change: (update: Partial) => void; }) { const all = ReviewerStateSchema.options; const toggle = (state: ReviewerState, shown: boolean) => @@ -189,8 +189,8 @@ function Toolbar({ setQuery, }: { colors: Colors; - preferences: InboxPreferences; - change: (update: Partial) => void; + preferences: AssignedPreferences; + change: (update: Partial) => void; query: string; setQuery: (query: string) => void; }) { @@ -358,7 +358,7 @@ type ColumnWidth = { width: number } | { flex: number; min: number }; /** `label` names the column in the chooser, and heads it unless `header` says otherwise. */ type ColumnSpec = { label: string; header?: string; size: ColumnWidth; sort?: InboxSortKey; cell: (item: InboxItem, colors: Colors, now: Date) => React.ReactNode }; -const COLUMNS: Record = { +const COLUMNS: Record = { checked: { label: "Checked off", header: "", size: { width: 20 }, cell: (item, colors) => }, change: { label: "PR/MR", @@ -398,7 +398,7 @@ function columnStyle(size: ColumnWidth) { } /** The narrowest the table can be with these columns before its cells crowd; anything narrower gets cards. */ -function tableWidth(columns: readonly InboxColumn[]): number { +function tableWidth(columns: readonly AssignedColumn[]): number { const cells = columns.reduce((total, column) => { const { size } = COLUMNS[column]; return total + ("width" in size ? size.width : size.min); @@ -579,8 +579,8 @@ function Table({ }: { colors: Colors; items: InboxItem[]; - preferences: InboxPreferences; - change: (update: Partial) => void; + preferences: AssignedPreferences; + change: (update: Partial) => void; starts: ReviewStarts; }) { const now = new Date(); @@ -648,10 +648,10 @@ function Table({ } /** A row stacked for a narrow screen: the title, then whichever of the other columns are shown, as lines. */ -function Card({ colors, item, columns, starts }: { colors: Colors; item: InboxItem; columns: readonly InboxColumn[]; starts: ReviewStarts }) { +function Card({ colors, item, columns, starts }: { colors: Colors; item: InboxItem; columns: readonly AssignedColumn[]; starts: ReviewStarts }) { const now = new Date(); const { dimmed } = useContext(CheckOffContext); - const shown = (column: InboxColumn) => columns.includes(column); + const shown = (column: AssignedColumn) => columns.includes(column); const meta = [ ...(shown("change") ? [item.project] : []), ...(shown("author") ? [item.author] : []), diff --git a/plugins/guided-review/server/inbox-check-offs.ts b/plugins/guided-review/server/inbox-check-offs.ts index c9fa6ba..c93c1ea 100644 --- a/plugins/guided-review/server/inbox-check-offs.ts +++ b/plugins/guided-review/server/inbox-check-offs.ts @@ -1,12 +1,23 @@ import path from "node:path"; -import { CheckOffSchema, type CheckOff, type ReviewerState, type ReviewRequest, type ReviewRequestHost } from "../shared/inbox.ts"; +import type { z } from "zod"; +import { + AuthoredCheckOffSchema, + CheckOffSchema, + type AuthoredCheckOff, + type CheckOff, + type ReviewerState, + type ReviewRequest, + type ReviewRequestHost, +} from "../shared/inbox.ts"; +import { byTab } from "./inbox-tabs.ts"; import { readJson, writeJson } from "./json-file.ts"; import { oneAtATimePer } from "./one-at-a-time.ts"; const FILE = "inbox-check-offs.json"; -/** Check-offs by the change request's URL. */ -type CheckOffs = Record; +/** Each tab's check-offs by the change request's URL, kept apart so a check-off on one tab leaves the other alone. */ +type CheckOffs = { assigned: AssignedCheckOffs; authored: Record }; +type AssignedCheckOffs = Record; /** The states only the other side brings about: being asked again, or an approval taken back. */ const RETURNING_STATES: readonly ReviewerState[] = ["requested", "unapproved"]; @@ -25,25 +36,20 @@ export class InboxCheckOffsFile { async set(url: string, checkOff: CheckOff | null): Promise { await this.#change((checkOffs) => { - const { [url]: _, ...rest } = checkOffs; - return checkOff === null ? rest : { ...rest, [url]: checkOff }; + const { [url]: _, ...rest } = checkOffs.assigned; + return { ...checkOffs, assigned: checkOff === null ? rest : { ...rest, [url]: checkOff } }; }); } /** Follows a listing of every host, and answers the URLs still checked off. */ async follow(hosts: readonly ReviewRequestHost[]): Promise> { - return new Set(Object.keys(await this.#change((checkOffs) => followListing(checkOffs, hosts)))); + const followed = await this.#change((checkOffs) => ({ ...checkOffs, assigned: followListing(checkOffs.assigned, hosts) })); + return new Set(Object.keys(followed.assigned)); } async #read(): Promise { - const stored = await readJson(this.#file); - if (typeof stored !== "object" || stored === null) return {}; - const checkOffs: CheckOffs = {}; - for (const [url, value] of Object.entries(stored)) { - const parsed = CheckOffSchema.safeParse(value); - if (parsed.success) checkOffs[url] = parsed.data; - } - return checkOffs; + const stored = byTab(await readJson(this.#file)); + return { assigned: byUrl(stored.assigned, CheckOffSchema), authored: byUrl(stored.authored, AuthoredCheckOffSchema) }; } async #change(update: (checkOffs: CheckOffs) => CheckOffs): Promise { @@ -60,10 +66,10 @@ export class InboxCheckOffsFile { * Drops the check-offs whose change request came back, and those a host listed in full no longer has. * The rest keep the state the reviewer is now in, so one they moved on themselves, by commenting, still comes back when they are asked again. */ -function followListing(checkOffs: CheckOffs, hosts: readonly ReviewRequestHost[]): CheckOffs { +function followListing(checkOffs: AssignedCheckOffs, hosts: readonly ReviewRequestHost[]): AssignedCheckOffs { const requests = new Map(hosts.flatMap((host) => host.requests.map((request) => [request.url, request] as const))); const complete = new Set(hosts.filter((host) => host.error === null && !host.truncated).map((host) => host.host)); - const followed: CheckOffs = {}; + const followed: AssignedCheckOffs = {}; for (const [url, checkOff] of Object.entries(checkOffs)) { const request = requests.get(url); if (request === undefined) { @@ -80,6 +86,17 @@ function cameBack(checkOff: CheckOff, request: ReviewRequest): boolean { return pushed || (RETURNING_STATES.includes(request.state) && request.state !== checkOff.state); } +/** One tab's check-offs as the file holds them, leaving out any the plugin no longer reads. */ +function byUrl(stored: unknown, schema: z.ZodType): Record { + if (typeof stored !== "object" || stored === null) return {}; + const checkOffs: Record = {}; + for (const [url, value] of Object.entries(stored)) { + const parsed = schema.safeParse(value); + if (parsed.success) checkOffs[url] = parsed.data; + } + return checkOffs; +} + function hostOf(url: string): string | null { try { return new URL(url).host.toLowerCase(); diff --git a/plugins/guided-review/server/inbox-preferences.test.ts b/plugins/guided-review/server/inbox-preferences.test.ts index fc03be5..364d26f 100644 --- a/plugins/guided-review/server/inbox-preferences.test.ts +++ b/plugins/guided-review/server/inbox-preferences.test.ts @@ -12,70 +12,147 @@ async function dataDirectory(t: TestContext): Promise { return data; } -test("with nothing saved the list hides what the reviewer approved and sorts by the latest update", async (t) => { +test("with nothing saved each tab hides what is approved and sorts by the latest update", async (t) => { const preferences = await new InboxPreferencesFile(await dataDirectory(t)).read(); assert.deepEqual(preferences, { - provider: "all", - states: ["requested", "commented", "changes-requested", "unapproved"], - hideDrafts: false, - checkedOff: "hide", - needsAttention: false, - sort: { key: "updated", descending: true }, - columns: ["checked", "change", "title", "author", "updated", "state", "local"], + assigned: { + provider: "all", + states: ["requested", "commented", "changes-requested", "unapproved"], + hideDrafts: false, + checkedOff: "hide", + needsAttention: false, + sort: { key: "updated", descending: true }, + columns: ["checked", "change", "title", "author", "updated", "state", "local"], + }, + authored: { + provider: "all", + states: ["unreviewed", "waiting", "commented", "changes-requested", "approval-reset"], + hideDrafts: false, + checkedOff: "hide", + needsAttention: false, + sort: { key: "updated", descending: true }, + columns: ["checked", "change", "title", "reviewers", "updated", "ci", "state", "local"], + }, }); }); -test("what is saved is read back after a restart", async (t) => { +test("what is saved for each tab is read back after a restart", async (t) => { const data = await dataDirectory(t); const saved: InboxPreferences = { - provider: "gitlab", - states: ["approved", "requested"], - hideDrafts: true, - checkedOff: "only", - needsAttention: true, - sort: { key: "size", descending: false }, - columns: ["title", "ci", "size"], + assigned: { + provider: "gitlab", + states: ["approved", "requested"], + hideDrafts: true, + checkedOff: "only", + needsAttention: true, + sort: { key: "size", descending: false }, + columns: ["title", "ci", "size"], + }, + authored: { + provider: "github", + states: ["approved", "waiting"], + hideDrafts: true, + checkedOff: "show", + needsAttention: true, + sort: { key: "created", descending: false }, + columns: ["reviewers", "title"], + }, }; await new InboxPreferencesFile(data).save(saved); - assert.deepEqual(await new InboxPreferencesFile(data).read(), { ...saved, states: ["requested", "approved"], columns: ["title", "size", "ci"] }); + assert.deepEqual(await new InboxPreferencesFile(data).read(), { + assigned: { ...saved.assigned, states: ["requested", "approved"], columns: ["title", "size", "ci"] }, + authored: { ...saved.authored, states: ["waiting", "approved"], columns: ["title", "reviewers"] }, + }); }); -test("the title is always among the columns", async (t) => { +test("saving a change to one tab keeps the other tab's preferences", async (t) => { + const data = await dataDirectory(t); + const file = new InboxPreferencesFile(data); + await file.save({ ...DEFAULT_INBOX_PREFERENCES, authored: { ...DEFAULT_INBOX_PREFERENCES.authored, hideDrafts: true } }); + + const read = await file.read(); + await file.save({ ...read, assigned: { ...read.assigned, provider: "github" } }); + + const after = await new InboxPreferencesFile(data).read(); + assert.equal(after.assigned.provider, "github"); + assert.deepEqual(after.authored, { ...DEFAULT_INBOX_PREFERENCES.authored, hideDrafts: true }); +}); + +test("the title is always among a tab's columns", async (t) => { const file = new InboxPreferencesFile(await dataDirectory(t)); - assert.deepEqual((await file.save({ ...DEFAULT_INBOX_PREFERENCES, columns: ["author"] })).columns, ["title", "author"]); + const saved = await file.save({ + assigned: { ...DEFAULT_INBOX_PREFERENCES.assigned, columns: ["author"] }, + authored: { ...DEFAULT_INBOX_PREFERENCES.authored, columns: ["reviewers"] }, + }); + + assert.deepEqual([saved.assigned.columns, saved.authored.columns], [["title", "author"], ["title", "reviewers"]]); +}); + +test("a file from before the tabs reads as the Assigned tab's, with the Authored tab at its defaults", async (t) => { + const data = await dataDirectory(t); + await writeFile( + path.join(data, "inbox-preferences.json"), + JSON.stringify({ provider: "gitlab", states: ["approved"], hideDrafts: true, checkedOff: "only", needsAttention: true, sort: { key: "size", descending: false }, columns: ["title", "ci"] }), + ); + + assert.deepEqual(await new InboxPreferencesFile(data).read(), { + assigned: { + provider: "gitlab", + states: ["approved"], + hideDrafts: true, + checkedOff: "only", + needsAttention: true, + sort: { key: "size", descending: false }, + columns: ["title", "ci"], + }, + authored: DEFAULT_INBOX_PREFERENCES.authored, + }); }); -test("a field the plugin no longer knows falls back on its own, keeping the rest of the file", async (t) => { +test("a field the plugin no longer knows falls back on its own, per tab, keeping the rest of the file", async (t) => { const data = await dataDirectory(t); await writeFile( path.join(data, "inbox-preferences.json"), - JSON.stringify({ provider: "bitbucket", hideApproved: false, states: ["merged", "approved"], hideDrafts: true, checkedOff: "archived", sort: { key: "stars" }, columns: ["title", "reactions"], extra: 1 }), + JSON.stringify({ + assigned: { provider: "bitbucket", hideApproved: false, states: ["merged", "approved"], hideDrafts: true, checkedOff: "archived", sort: { key: "stars" }, columns: ["title", "reactions"], extra: 1 }, + authored: { provider: "gitlab", states: ["requested", "waiting"], checkedOff: "only", columns: ["author", "reviewers"] }, + }), ); assert.deepEqual(await new InboxPreferencesFile(data).read(), { - ...DEFAULT_INBOX_PREFERENCES, - states: ["approved"], - hideDrafts: true, - columns: ["title"], + assigned: { ...DEFAULT_INBOX_PREFERENCES.assigned, states: ["approved"], hideDrafts: true, columns: ["title"] }, + authored: { ...DEFAULT_INBOX_PREFERENCES.authored, provider: "gitlab", states: ["waiting"], checkedOff: "only", columns: ["title", "reviewers"] }, }); }); -test("a list of states that would show nothing falls back to the default", async (t) => { +test("a tab the file has no preferences for reads at its defaults", async (t) => { const data = await dataDirectory(t); - await writeFile(path.join(data, "inbox-preferences.json"), JSON.stringify({ states: ["merged"] })); + await writeFile(path.join(data, "inbox-preferences.json"), JSON.stringify({ assigned: { hideDrafts: true }, authored: "everything" })); + + assert.deepEqual(await new InboxPreferencesFile(data).read(), { + assigned: { ...DEFAULT_INBOX_PREFERENCES.assigned, hideDrafts: true }, + authored: DEFAULT_INBOX_PREFERENCES.authored, + }); +}); + +test("a list of states that would show nothing falls back to the tab's default", async (t) => { + const data = await dataDirectory(t); + await writeFile(path.join(data, "inbox-preferences.json"), JSON.stringify({ assigned: { states: ["merged"] }, authored: { states: ["requested"] } })); + + const read = await new InboxPreferencesFile(data).read(); - assert.deepEqual((await new InboxPreferencesFile(data).read()).states, DEFAULT_INBOX_PREFERENCES.states); + assert.deepEqual([read.assigned.states, read.authored.states], [DEFAULT_INBOX_PREFERENCES.assigned.states, DEFAULT_INBOX_PREFERENCES.authored.states]); }); test("saves sent together all land, the last one kept", async (t) => { const data = await dataDirectory(t); const file = new InboxPreferencesFile(data); - await Promise.all([true, false, true].map((hideDrafts) => file.save({ ...DEFAULT_INBOX_PREFERENCES, hideDrafts }))); + await Promise.all([true, false, true].map((hideDrafts) => file.save({ ...DEFAULT_INBOX_PREFERENCES, authored: { ...DEFAULT_INBOX_PREFERENCES.authored, hideDrafts } }))); - assert.equal((await file.read()).hideDrafts, true); + assert.equal((await file.read()).authored.hideDrafts, true); }); diff --git a/plugins/guided-review/server/inbox-preferences.ts b/plugins/guided-review/server/inbox-preferences.ts index 832f6d2..059abe9 100644 --- a/plugins/guided-review/server/inbox-preferences.ts +++ b/plugins/guided-review/server/inbox-preferences.ts @@ -1,13 +1,14 @@ import path from "node:path"; import { InboxPreferencesSchema, type InboxPreferences } from "../shared/inbox-preferences.ts"; +import { byTab } from "./inbox-tabs.ts"; import { readJson, writeJson } from "./json-file.ts"; import { oneAtATimePer } from "./one-at-a-time.ts"; const FILE = "inbox-preferences.json"; /** - * The review list's filters, sort and columns, kept in the data directory because a plugin client - * has no storage of its own. They are the daemon's, so every client of it shares them. + * Each tab's filters, sort and columns, kept in the data directory because a plugin client has no storage of its own. + * They are the daemon's, so every client of it shares them. */ export class InboxPreferencesFile { readonly #file: string; @@ -18,7 +19,7 @@ export class InboxPreferencesFile { } async read(): Promise { - return InboxPreferencesSchema.parse((await readJson(this.#file)) ?? {}); + return InboxPreferencesSchema.parse(byTab(await readJson(this.#file))); } async save(preferences: InboxPreferences): Promise { diff --git a/plugins/guided-review/server/inbox-tabs.ts b/plugins/guided-review/server/inbox-tabs.ts new file mode 100644 index 0000000..4150df3 --- /dev/null +++ b/plugins/guided-review/server/inbox-tabs.ts @@ -0,0 +1,10 @@ +import { INBOX_TABS } from "../shared/inbox-preferences.ts"; + +/** + * A file kept per tab of the review list, as `{ assigned, authored }`. + * One written before the list had tabs holds the Assigned tab's alone, flat, and reads as that. + */ +export function byTab(stored: unknown): Record { + if (typeof stored !== "object" || stored === null || Array.isArray(stored)) return {}; + return INBOX_TABS.some((tab) => tab in stored) ? (stored as Record) : { assigned: stored }; +} diff --git a/plugins/guided-review/server/review-inbox.test.ts b/plugins/guided-review/server/review-inbox.test.ts index 55d0cc0..54312b0 100644 --- a/plugins/guided-review/server/review-inbox.test.ts +++ b/plugins/guided-review/server/review-inbox.test.ts @@ -1,5 +1,5 @@ import assert from "node:assert/strict"; -import { mkdtemp, rm } from "node:fs/promises"; +import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import test, { type TestContext } from "node:test"; @@ -17,7 +17,7 @@ const GITLAB_ID = "gitlab/gitlab.com/acme/uploader/7"; const HEAD = "b".repeat(40); const PUSHED = "d".repeat(40); -type Host = { github: FakeForge; gitlab: FakeForge; workspaces: FakeWorkspaces; guideAgents: FakeGuideAgents; service: ReviewService; restart: () => ReviewService }; +type Host = { data: string; github: FakeForge; gitlab: FakeForge; workspaces: FakeWorkspaces; guideAgents: FakeGuideAgents; service: ReviewService; restart: () => ReviewService }; async function withForges(t: TestContext): Promise { const data = await mkdtemp(path.join(os.tmpdir(), "guided-review-inbox-")); @@ -32,7 +32,7 @@ async function withForges(t: TestContext): Promise { const guideAgents = fakeGuideAgents(); guideAgents.answer = () => sampleGuideReply(); const restart = () => new ReviewService({ forges: [github, gitlab], workspaces, guideAgents, dataDirectory: data }); - return { github, gitlab, workspaces, guideAgents, service: restart(), restart }; + return { data, github, gitlab, workspaces, guideAgents, service: restart(), restart }; } async function startReview(service: ReviewService, url: string) { @@ -251,3 +251,33 @@ test("a check-off goes once a host listed in full no longer has it, and stays wh gitlab.reviewRequests = [listed("gitlab", [request(GITLAB_URL)])]; assert.deepEqual(await checkedOff(service), []); }); + +test("a check-off kept before the list had tabs holds on the Assigned tab", async (t) => { + const { data, gitlab, service } = await withForges(t); + gitlab.reviewRequests = [listed("gitlab", [request(GITLAB_URL)])]; + const file = path.join(data, "inbox-check-offs.json"); + await writeFile(file, JSON.stringify({ [GITLAB_URL]: { headSha: HEAD, state: "requested" } })); + + assert.deepEqual(await checkedOff(service), [GITLAB_URL]); + + await service.setCheckedOff({ url: GITHUB_URL, checkOff: { headSha: HEAD, state: "requested" } }); + assert.deepEqual(JSON.parse(await readFile(file, "utf8")), { + assigned: { [GITLAB_URL]: { headSha: HEAD, state: "requested" }, [GITHUB_URL]: { headSha: HEAD, state: "requested" } }, + authored: {}, + }); +}); + +test("a check-off on the Authored tab leaves the Assigned row alone, and outlasts the Assigned tab's check-offs", async (t) => { + const { data, gitlab, service } = await withForges(t); + gitlab.reviewRequests = [listed("gitlab", [request(GITLAB_URL)])]; + const file = path.join(data, "inbox-check-offs.json"); + const authored = { [GITLAB_URL]: { status: "waiting", latestReviewAt: null } }; + await writeFile(file, JSON.stringify({ assigned: {}, authored })); + + assert.deepEqual(await checkedOff(service), []); + + await service.setCheckedOff({ url: GITLAB_URL, checkOff: { headSha: HEAD, state: "requested" } }); + gitlab.reviewRequests = [listed("gitlab", [request(GITLAB_URL, { headSha: PUSHED })])]; + assert.deepEqual(await checkedOff(service), [], "came back on Assigned"); + assert.deepEqual(JSON.parse(await readFile(file, "utf8")).authored, authored); +}); diff --git a/plugins/guided-review/shared/inbox-preferences.ts b/plugins/guided-review/shared/inbox-preferences.ts index 985294b..82db8e1 100644 --- a/plugins/guided-review/shared/inbox-preferences.ts +++ b/plugins/guided-review/shared/inbox-preferences.ts @@ -1,14 +1,15 @@ import { z } from "zod"; -import { ReviewerStateSchema, type ReviewerState } from "./inbox.ts"; +import { ReviewerStateSchema, ReviewStatusSchema } from "./inbox.ts"; -/** Every column the review list can show, in the order it shows them; the title is what a row is, so it is always shown. */ -export const INBOX_COLUMNS = ["checked", "change", "title", "author", "updated", "created", "size", "ci", "state", "local"] as const; -export type InboxColumn = (typeof INBOX_COLUMNS)[number]; +/** The review list's tabs: the change requests the reviewer is asked to review, and those they authored. */ +export const INBOX_TABS = ["assigned", "authored"] as const; +export type InboxTab = (typeof INBOX_TABS)[number]; -export const DEFAULT_INBOX_COLUMNS: readonly InboxColumn[] = ["checked", "change", "title", "author", "updated", "state", "local"]; - -/** With nothing saved the list leaves out what the reviewer approved. */ -export const DEFAULT_INBOX_STATES: readonly ReviewerState[] = ReviewerStateSchema.options.filter((state) => state !== "approved"); +/** Every column a tab can show, in the order it shows them; the title is what a row is, so it is always shown. */ +export const ASSIGNED_COLUMNS = ["checked", "change", "title", "author", "updated", "created", "size", "ci", "state", "local"] as const; +export const AUTHORED_COLUMNS = ["checked", "change", "title", "reviewers", "updated", "created", "size", "ci", "state", "local"] as const; +export type AssignedColumn = (typeof ASSIGNED_COLUMNS)[number]; +export type AuthoredColumn = (typeof AUTHORED_COLUMNS)[number]; /** Whether the list leaves out what the reviewer checked off, shows it among the rest, or shows nothing else. */ export const CHECK_OFF_FILTERS = ["hide", "show", "only"] as const; @@ -18,27 +19,56 @@ export const INBOX_SORT_KEYS = ["updated", "created", "size"] as const; export type InboxSortKey = (typeof INBOX_SORT_KEYS)[number]; /** - * How the reviewer last left the review list. Each field falls back to its default on its own, so - * a file written by an older or newer plugin keeps whatever of it still means something. + * How the reviewer last left one tab of the review list. + * Each field falls back to its default on its own, so a file written by an older or newer plugin keeps whatever of it still means something. */ -export const InboxPreferencesSchema = z.object({ - provider: z.enum(["all", "github", "gitlab"]).catch("all"), - /** The reviewer states the list shows, never none, since a list filtered down to nothing is no use. */ - states: z - .array(z.string()) - .transform((states) => ReviewerStateSchema.options.filter((state) => states.includes(state))) - .pipe(z.array(ReviewerStateSchema).min(1)) - .catch([...DEFAULT_INBOX_STATES]), - hideDrafts: z.boolean().catch(false), - checkedOff: z.enum(CHECK_OFF_FILTERS).catch("hide"), - needsAttention: z.boolean().catch(false), - sort: z.object({ key: z.enum(INBOX_SORT_KEYS), descending: z.boolean() }).catch({ key: "updated", descending: true }), - columns: z - .array(z.string()) - .transform((columns) => INBOX_COLUMNS.filter((column) => column === "title" || columns.includes(column))) - .catch([...DEFAULT_INBOX_COLUMNS]), +function tabPreferencesSchema(tab: { + states: readonly State[]; + defaultStates: readonly State[]; + columns: readonly Column[]; + defaultColumns: readonly Column[]; +}) { + const schema = z.object({ + provider: z.enum(["all", "github", "gitlab"]).catch("all"), + /** The states the tab shows, never none, since a list filtered down to nothing is no use. */ + states: z + .array(z.string()) + .transform((states) => tab.states.filter((state) => states.includes(state))) + .refine((states) => states.length > 0) + .catch([...tab.defaultStates]), + hideDrafts: z.boolean().catch(false), + checkedOff: z.enum(CHECK_OFF_FILTERS).catch("hide"), + needsAttention: z.boolean().catch(false), + sort: z.object({ key: z.enum(INBOX_SORT_KEYS), descending: z.boolean() }).catch({ key: "updated", descending: true }), + columns: z + .array(z.string()) + .transform((columns) => tab.columns.filter((column) => column === "title" || columns.includes(column))) + .catch([...tab.defaultColumns]), + }); + return schema.catch(() => schema.parse({})); +} + +/** With nothing saved the Assigned tab leaves out what the reviewer approved. */ +export const AssignedPreferencesSchema = tabPreferencesSchema({ + states: ReviewerStateSchema.options, + defaultStates: ReviewerStateSchema.options.filter((state) => state !== "approved"), + columns: ASSIGNED_COLUMNS, + defaultColumns: ["checked", "change", "title", "author", "updated", "state", "local"], }); +/** With nothing saved the Authored tab leaves out what someone approved, and shows CI, which is the author's to fix. */ +export const AuthoredPreferencesSchema = tabPreferencesSchema({ + states: ReviewStatusSchema.options, + defaultStates: ReviewStatusSchema.options.filter((status) => status !== "approved"), + columns: AUTHORED_COLUMNS, + defaultColumns: ["checked", "change", "title", "reviewers", "updated", "ci", "state", "local"], +}); + +/** Both tabs' preferences, which travel together since the client saves them whole. */ +export const InboxPreferencesSchema = z.object({ assigned: AssignedPreferencesSchema, authored: AuthoredPreferencesSchema }); + +export type AssignedPreferences = z.output; +export type AuthoredPreferences = z.output; export type InboxPreferences = z.output; export const DEFAULT_INBOX_PREFERENCES: InboxPreferences = InboxPreferencesSchema.parse({}); diff --git a/plugins/guided-review/shared/inbox.ts b/plugins/guided-review/shared/inbox.ts index ad31a08..2eefa99 100644 --- a/plugins/guided-review/shared/inbox.ts +++ b/plugins/guided-review/shared/inbox.ts @@ -7,6 +7,12 @@ import { ReviewHeaderSchema, START_PHASES } from "./review-header.ts"; */ export const ReviewerStateSchema = z.enum(["requested", "commented", "changes-requested", "approved", "unapproved"]); +/** + * Where the author's change request stands with its reviewers, combined from each one's latest review, in the same words on both forges. + * `approval-reset` is an approval taken back by a push. + */ +export const ReviewStatusSchema = z.enum(["unreviewed", "waiting", "commented", "changes-requested", "approved", "approval-reset"]); + /** The head's CI as one word; null when it has none, or one in a state neither of these is, like a cancelled pipeline. */ export const CiStateSchema = z.enum(["success", "failure", "pending"]); @@ -42,6 +48,7 @@ export const ReviewRequestSchema = z.object({ }); export type ReviewerState = z.output; +export type ReviewStatus = z.output; export type CiState = z.output; export type ReviewRequest = z.output; @@ -83,6 +90,9 @@ export const InboxItemSchema = ReviewRequestSchema.extend({ /** What the reviewer saw of a change request when they checked it off. */ export const CheckOffSchema = z.object({ headSha: z.string(), state: ReviewerStateSchema }); +/** What the author saw of their change request when they checked it off: where it stood with its reviewers, and when the newest review arrived. */ +export const AuthoredCheckOffSchema = z.object({ status: ReviewStatusSchema, latestReviewAt: z.string().nullable() }); + /** Every host's review requests, each joined with the review started here, if any. */ export const InboxSchema = z.object({ hosts: z.array(ReviewRequestHostSchema.omit({ requests: true })), @@ -91,6 +101,7 @@ export const InboxSchema = z.object({ export type LocalReview = z.output; export type CheckOff = z.output; +export type AuthoredCheckOff = z.output; export type InboxItem = z.output; export type Inbox = z.output; From 580e7ac397bcf199a8e4ce2bf3bf40c850478428 Mon Sep 17 00:00:00 2001 From: Sleeyax Date: Mon, 12 Oct 2026 13:51:04 +0200 Subject: [PATCH 02/16] refactor(guided-review): name the forge's inbox listing for change requests, not review requests The listing is about to hold the change requests the viewer authored beside those they review, so `listReviewRequests` becomes `listChangeRequests` and `ReviewRequestHost` becomes `ListedHost`. Nothing it lists changes. Refs #146 Co-Authored-By: Claude Opus 5.5 --- plugins/guided-review/server/fake-forge.ts | 12 ++--- .../guided-review/server/forge/github.test.ts | 12 ++--- plugins/guided-review/server/forge/github.ts | 4 +- .../guided-review/server/forge/gitlab.test.ts | 14 +++--- plugins/guided-review/server/forge/gitlab.ts | 10 ++-- plugins/guided-review/server/forge/port.ts | 4 +- .../guided-review/server/inbox-check-offs.ts | 6 +-- .../guided-review/server/review-inbox.test.ts | 50 +++++++++---------- plugins/guided-review/server/review-inbox.ts | 2 +- plugins/guided-review/shared/inbox.ts | 6 +-- 10 files changed, 60 insertions(+), 60 deletions(-) diff --git a/plugins/guided-review/server/fake-forge.ts b/plugins/guided-review/server/fake-forge.ts index f7c4090..5f10776 100644 --- a/plugins/guided-review/server/fake-forge.ts +++ b/plugins/guided-review/server/fake-forge.ts @@ -3,7 +3,7 @@ import path from "node:path"; import { parsePullRequestUrl } from "./forge/github.ts"; import { parseMergeRequestUrl } from "./forge/gitlab.ts"; import { lineRefOf, type Draft, type DraftLocation } from "../shared/drafts.ts"; -import type { ReviewRequestHost } from "../shared/inbox.ts"; +import type { ListedHost } from "../shared/inbox.ts"; import { ForgeError, type BranchChangeRequest, @@ -72,8 +72,8 @@ export type FakeForge = Forge & { submitOutcome: SubmitOutcome | null; /** The URL of every change request whose pending review was discarded. */ discarded: string[]; - /** What `listReviewRequests` answers, host by host. */ - reviewRequests: ReviewRequestHost[]; + /** What `listChangeRequests` answers, host by host. */ + listedHosts: ListedHost[]; }; /** Where the forge says a draft on `anchor` is: a line by the side both forges put it on. */ @@ -118,7 +118,7 @@ export function fakeForge(kind: ForgeKind = "github"): FakeForge { submissions: [], submitOutcome: null, discarded: [], - reviewRequests: [], + listedHosts: [], async matchUrl(url) { return PARSERS[kind](url); }, @@ -250,8 +250,8 @@ export function fakeForge(kind: ForgeKind = "github"): FakeForge { forge.discarded.push(ref.url); forge.drafts.delete(ref.url); }, - async listReviewRequests() { - return structuredClone(forge.reviewRequests); + async listChangeRequests() { + return structuredClone(forge.listedHosts); }, }; return forge; diff --git a/plugins/guided-review/server/forge/github.test.ts b/plugins/guided-review/server/forge/github.test.ts index cf0eb4c..288c633 100644 --- a/plugins/guided-review/server/forge/github.test.ts +++ b/plugins/guided-review/server/forge/github.test.ts @@ -719,7 +719,7 @@ test("a discussion on the pull request is a comment on its conversation, as GitH test("lists what the viewer is asked to review and what they reviewed, as two searches on github.com", async () => { const { forge, run } = forgeReplaying([{ stdout: fixture("review-search-requested.json") }, { stdout: fixture("review-search-reviewed.json") }]); - const [host, ...others] = await forge.listReviewRequests(); + const [host, ...others] = await forge.listChangeRequests(); assert.deepEqual(others, []); assert.deepEqual( @@ -765,7 +765,7 @@ test("lists what the viewer is asked to review and what they reviewed, as two se test("a PR the viewer is asked to review again reads as requested and changed, with the team it was asked of", async () => { const { forge } = forgeReplaying([{ stdout: fixture("review-search-requested.json") }, { stdout: fixture("review-search-reviewed.json") }]); - const [host] = await forge.listReviewRequests(); + const [host] = await forge.listChangeRequests(); const again = host?.requests.find((request) => request.number === 7); assert.equal(again?.author, "ghost"); @@ -780,7 +780,7 @@ test("a PR the viewer is asked to review again reads as requested and changed, w test("a PR the viewer reviewed and is not asked again reads by their review, changed when the head moved past it", async () => { const { forge } = forgeReplaying([{ stdout: fixture("review-search-requested.json") }, { stdout: fixture("review-search-reviewed.json") }]); - const [host] = await forge.listReviewRequests(); + const [host] = await forge.listChangeRequests(); const byUrl = (url: string) => host?.requests.find((request) => request.url === url); const commented = byUrl("https://github.com/themouette/claude-vm/pull/96"); @@ -794,7 +794,7 @@ test("a PR the viewer reviewed and is not asked again reads by their review, cha test("a PR the viewer reviewed without being asked, by name or through a team, is left out", async () => { const { forge } = forgeReplaying([{ stdout: fixture("review-search-requested.json") }, { stdout: fixture("review-search-reviewed.json") }]); - const [host] = await forge.listReviewRequests(); + const [host] = await forge.listChangeRequests(); const urls = host?.requests.map((request) => request.url); assert.ok(!urls?.includes("https://github.com/magisterquis/connectproxy/pull/2")); @@ -808,7 +808,7 @@ test("a search with more results than it returned marks the host as truncated", requested.data.search.issueCount = 120; const { forge } = forgeReplaying([{ stdout: JSON.stringify(requested) }, { stdout: fixture("review-search-reviewed.json") }]); - const [host] = await forge.listReviewRequests(); + const [host] = await forge.listChangeRequests(); assert.equal(host?.truncated, true); }); @@ -819,7 +819,7 @@ test("a search gh cannot run comes back as the host's error rather than a throw" { stdout: fixture("review-search-reviewed.json") }, ]); - const hosts = await forge.listReviewRequests(); + const hosts = await forge.listChangeRequests(); assert.deepEqual(hosts, [ { forge: "github", host: "github.com", requests: [], truncated: false, error: "gh failed: To get started with GitHub CLI, please run: gh auth login" }, diff --git a/plugins/guided-review/server/forge/github.ts b/plugins/guided-review/server/forge/github.ts index 06099e5..0751567 100644 --- a/plugins/guided-review/server/forge/github.ts +++ b/plugins/guided-review/server/forge/github.ts @@ -3,7 +3,7 @@ import type { CommandRunner } from "../command-runner.ts"; import { oneAtATimePer } from "../one-at-a-time.ts"; import { createCli } from "./cli.ts"; import type { Draft, DraftLocation } from "../../shared/drafts.ts"; -import type { CiState, ReviewerState, ReviewRequest, ReviewRequestHost } from "../../shared/inbox.ts"; +import type { CiState, ReviewerState, ReviewRequest, ListedHost } from "../../shared/inbox.ts"; import type { Verdict } from "../../shared/submit.ts"; import { CLONE_TIMEOUT_MS, MAX_BRANCH_CHANGE_REQUESTS, MAX_COMMITS, MAX_LINKED_ISSUES, userOf } from "./common.ts"; import { SubmitSteps } from "./submit-steps.ts"; @@ -748,7 +748,7 @@ export function createGitHubForge(options: GitHubForgeOptions): Forge { }); }, - async listReviewRequests(): Promise { + async listChangeRequests(): Promise { const search = (query: string) => gh.json(ReviewSearchResponse, ["api", "graphql", "--hostname", GITHUB_HOST, "--input", "-"], { input: JSON.stringify({ query: REVIEW_SEARCH_QUERY, variables: { query } }), diff --git a/plugins/guided-review/server/forge/gitlab.test.ts b/plugins/guided-review/server/forge/gitlab.test.ts index 4b16cef..2461daa 100644 --- a/plugins/guided-review/server/forge/gitlab.test.ts +++ b/plugins/guided-review/server/forge/gitlab.test.ts @@ -1182,7 +1182,7 @@ test("reads every host glab reports, logged in or not, and why glab cannot use o test("lists the open MRs the viewer reviews on each host glab is logged in to, after checking its version", async () => { const { forge, run } = forgeReplaying(LIST_REVIEWS); - const hosts = await forge.listReviewRequests(); + const hosts = await forge.listChangeRequests(); assert.deepEqual( run.calls.map((call) => call.args), @@ -1222,7 +1222,7 @@ test("lists the open MRs the viewer reviews on each host glab is logged in to, a test("an MR reads by the viewer's own reviewer record, changed when the head commit is newer than it", async () => { const { forge } = forgeReplaying(LIST_REVIEWS); - const [host] = await forge.listReviewRequests(); + const [host] = await forge.listChangeRequests(); const summary = host?.requests.map(({ number, state, changedSinceReview, ci, author, isDraft }) => ({ number, state, changedSinceReview, ci, author, isDraft })); assert.deepEqual(summary, [ @@ -1238,7 +1238,7 @@ test("a host glab is not logged in to, or one older than 19.2, comes back with i const report = { exitCode: 1, stderr: fixture("auth-status-all-unreachable.stderr") + fixture("auth-status-all.stderr") }; const { forge } = forgeReplaying([report, version("19.5.0-pre"), { stdout: fixture("review-list.json") }]); - const hosts = await forge.listReviewRequests(); + const hosts = await forge.listChangeRequests(); assert.deepEqual( hosts.map(({ host, error, requests }) => ({ host, error, count: requests.length })), @@ -1254,7 +1254,7 @@ test("a host glab is not logged in to, or one older than 19.2, comes back with i ); const { forge: old } = forgeReplaying([AUTH_STATUS_ALL, version("18.11.2-ee")]); - assert.deepEqual(await old.listReviewRequests(), [ + assert.deepEqual(await old.listChangeRequests(), [ { forge: "gitlab", host: "gitlab.com", requests: [], truncated: false, error: "gitlab.com runs GitLab 18.11.2-ee; listing your reviews needs 19.2 or newer." }, ]); }); @@ -1263,21 +1263,21 @@ test("a GraphQL answer with errors and no user is the host's error, and more MRs const failed = JSON.stringify({ data: { currentUser: null }, errors: [{ message: "Query has complexity of 301, which exceeds max complexity of 250" }] }); const { forge } = forgeReplaying([AUTH_STATUS_ALL, version("19.5.0-pre"), { stdout: failed }]); assert.equal( - (await forge.listReviewRequests())[0]?.error, + (await forge.listChangeRequests())[0]?.error, "GitLab on gitlab.com did not list your reviews: Query has complexity of 301, which exceeds max complexity of 250.", ); const more = JSON.parse(fixture("review-list.json")); more.data.currentUser.reviewRequestedMergeRequests.pageInfo.hasNextPage = true; const { forge: paged } = forgeReplaying([AUTH_STATUS_ALL, version("19.5.0-pre"), { stdout: JSON.stringify(more) }]); - assert.equal((await paged.listReviewRequests())[0]?.truncated, true); + assert.equal((await paged.listChangeRequests())[0]?.truncated, true); }); test("a host listed as logged in is not checked again before reading an MR on it", async () => { const head = { stdout: JSON.stringify({ state: "opened", description: null, sha: "a".repeat(40), diff_refs: { head_sha: "a".repeat(40) } }) }; const { forge, run } = forgeReplaying([...LIST_REVIEWS, { stdout: fixture("project.json") }, head]); - await forge.listReviewRequests(); + await forge.listChangeRequests(); run.calls.length = 0; assert.deepEqual(await forge.fetchHead(MR_3931), { headSha: "a".repeat(40), state: "open", description: "" }); diff --git a/plugins/guided-review/server/forge/gitlab.ts b/plugins/guided-review/server/forge/gitlab.ts index f50401c..cc6a7c9 100644 --- a/plugins/guided-review/server/forge/gitlab.ts +++ b/plugins/guided-review/server/forge/gitlab.ts @@ -3,7 +3,7 @@ import { z } from "zod"; import type { Draft, DraftLocation, LineRef } from "../../shared/drafts.ts"; import type { CommandRunner } from "../command-runner.ts"; import type { Verdict } from "../../shared/submit.ts"; -import type { CiState, ReviewerState, ReviewRequest, ReviewRequestHost } from "../../shared/inbox.ts"; +import type { CiState, ReviewerState, ReviewRequest, ListedHost } from "../../shared/inbox.ts"; import { createCli, type Cli } from "./cli.ts"; import { GITHUB_HOST } from "./github.ts"; import { CLONE_TIMEOUT_MS, COMMIT_READS_AT_ONCE, mapAtMost, MAX_BRANCH_CHANGE_REQUESTS, MAX_COMMITS, MAX_LINKED_ISSUES, userOf } from "./common.ts"; @@ -432,7 +432,7 @@ export function createGitLabForge(options: GitLabForgeOptions): Forge { ); }; - const reviewRequestsOn = async (host: string): Promise> => { + const listingOn = async (host: string): Promise> => { const { version } = await glab.json(VersionResponse, ["api", "--hostname", host, "version"]); if (!isAtLeast(version, REVIEW_LIST_VERSION)) { throw new ForgeError(`${host} runs GitLab ${version}; listing your reviews needs ${REVIEW_LIST_VERSION.join(".")} or newer.`); @@ -829,10 +829,10 @@ export function createGitLabForge(options: GitLabForgeOptions): Forge { } }, - async listReviewRequests(): Promise { + async listChangeRequests(): Promise { const statuses = parseAuthStatus(await glab.report(["auth", "status", "--all"])); return Promise.all( - statuses.map(async ({ host, loggedIn, problem }): Promise => { + statuses.map(async ({ host, loggedIn, problem }): Promise => { const listed = { forge: "gitlab" as const, host, requests: [], truncated: false }; if (!loggedIn) { const reason = problem === null ? "" : ` (${problem})`; @@ -840,7 +840,7 @@ export function createGitLabForge(options: GitLabForgeOptions): Forge { } hosts.set(host, Promise.resolve()); try { - return { ...(await reviewRequestsOn(host)), error: null }; + return { ...(await listingOn(host)), error: null }; } catch (error) { if (!(error instanceof ForgeError)) throw error; return { ...listed, error: error.message }; diff --git a/plugins/guided-review/server/forge/port.ts b/plugins/guided-review/server/forge/port.ts index 245ecbc..86ffbb2 100644 --- a/plugins/guided-review/server/forge/port.ts +++ b/plugins/guided-review/server/forge/port.ts @@ -1,6 +1,6 @@ import type { DiffLine } from "../../shared/diff.ts"; import type { Draft } from "../../shared/drafts.ts"; -import type { ReviewRequestHost } from "../../shared/inbox.ts"; +import type { ListedHost } from "../../shared/inbox.ts"; import type { SubmitStep, Verdict } from "../../shared/submit.ts"; /** @@ -98,7 +98,7 @@ export interface Forge { * The open change requests the current user is a reviewer of, on every host of this forge the CLI * is logged in to. A host that cannot be listed comes back with its error rather than throwing. */ - listReviewRequests(): Promise; + listChangeRequests(): Promise; } /** diff --git a/plugins/guided-review/server/inbox-check-offs.ts b/plugins/guided-review/server/inbox-check-offs.ts index c93c1ea..4f31e0d 100644 --- a/plugins/guided-review/server/inbox-check-offs.ts +++ b/plugins/guided-review/server/inbox-check-offs.ts @@ -7,7 +7,7 @@ import { type CheckOff, type ReviewerState, type ReviewRequest, - type ReviewRequestHost, + type ListedHost, } from "../shared/inbox.ts"; import { byTab } from "./inbox-tabs.ts"; import { readJson, writeJson } from "./json-file.ts"; @@ -42,7 +42,7 @@ export class InboxCheckOffsFile { } /** Follows a listing of every host, and answers the URLs still checked off. */ - async follow(hosts: readonly ReviewRequestHost[]): Promise> { + async follow(hosts: readonly ListedHost[]): Promise> { const followed = await this.#change((checkOffs) => ({ ...checkOffs, assigned: followListing(checkOffs.assigned, hosts) })); return new Set(Object.keys(followed.assigned)); } @@ -66,7 +66,7 @@ export class InboxCheckOffsFile { * Drops the check-offs whose change request came back, and those a host listed in full no longer has. * The rest keep the state the reviewer is now in, so one they moved on themselves, by commenting, still comes back when they are asked again. */ -function followListing(checkOffs: AssignedCheckOffs, hosts: readonly ReviewRequestHost[]): AssignedCheckOffs { +function followListing(checkOffs: AssignedCheckOffs, hosts: readonly ListedHost[]): AssignedCheckOffs { const requests = new Map(hosts.flatMap((host) => host.requests.map((request) => [request.url, request] as const))); const complete = new Set(hosts.filter((host) => host.error === null && !host.truncated).map((host) => host.host)); const followed: AssignedCheckOffs = {}; diff --git a/plugins/guided-review/server/review-inbox.test.ts b/plugins/guided-review/server/review-inbox.test.ts index 54312b0..a9ae477 100644 --- a/plugins/guided-review/server/review-inbox.test.ts +++ b/plugins/guided-review/server/review-inbox.test.ts @@ -3,7 +3,7 @@ import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import test, { type TestContext } from "node:test"; -import { headMoved, type ReviewRequest, type ReviewRequestHost } from "../shared/inbox.ts"; +import { headMoved, type ReviewRequest, type ListedHost } from "../shared/inbox.ts"; import { fakeForge, sampleChangeRequest, type FakeForge } from "./fake-forge.ts"; import { fakeGuideAgents, sampleGuideReply, type FakeGuideAgents } from "./fake-guide-agents.ts"; import { fakeWorkspaces, type FakeWorkspaces } from "./fake-workspaces.ts"; @@ -66,14 +66,14 @@ function request(url: string, overrides: Partial = {}): ReviewReq }; } -function listed(forge: "github" | "gitlab", requests: ReviewRequest[], extra: Partial = {}): ReviewRequestHost { +function listed(forge: "github" | "gitlab", requests: ReviewRequest[], extra: Partial = {}): ListedHost { return { forge, host: forge === "github" ? "github.com" : "gitlab.com", requests, truncated: false, error: null, ...extra }; } test("lists every forge's review requests with their hosts, and none started here has a local review", async (t) => { const { github, gitlab, service } = await withForges(t); - github.reviewRequests = [listed("github", [request(GITHUB_URL)], { truncated: true })]; - gitlab.reviewRequests = [ + github.listedHosts = [listed("github", [request(GITHUB_URL)], { truncated: true })]; + gitlab.listedHosts = [ listed("gitlab", [request(GITLAB_URL)]), listed("gitlab", [], { host: "gitlab.example.com", error: "glab is not logged in to gitlab.example.com." }), ]; @@ -95,13 +95,13 @@ test("a change request reviewed here carries its review, its guide's state and w const { github, service } = await withForges(t); await startReview(service, GITHUB_URL); - github.reviewRequests = [listed("github", [request(GITHUB_URL)])]; + github.listedHosts = [listed("github", [request(GITHUB_URL)])]; const [item] = (await service.inbox()).items; assert.equal(item?.local?.header?.headSha, HEAD); assert.deepEqual({ ...item?.local, header: null }, { header: null, preparing: null, guide: "ready", workspaceId: "wks_0000000000000001" }); assert.equal(headMoved(item!, item!.local!), false); - github.reviewRequests = [listed("github", [request(GITHUB_URL, { headSha: PUSHED })])]; + github.listedHosts = [listed("github", [request(GITHUB_URL, { headSha: PUSHED })])]; const [pushed] = (await service.inbox()).items; assert.equal(headMoved(pushed!, pushed!.local!), true); }); @@ -111,7 +111,7 @@ test("a start that failed before the change request was read lists as failed, wi github.changeRequests.delete(GITHUB_URL); await startReview(service, GITHUB_URL); - github.reviewRequests = [listed("github", [request(GITHUB_URL)])]; + github.listedHosts = [listed("github", [request(GITHUB_URL)])]; const local = (await service.inbox()).items[0]?.local; assert.equal(local?.preparing?.phase, "failed"); assert.match(local?.preparing?.message ?? "", /^Could not read /); @@ -123,7 +123,7 @@ test("a review whose workspace was archived has no workspace to open, and the li await startReview(service, GITHUB_URL); workspaces.archive("wks_0000000000000001"); - github.reviewRequests = [listed("github", [request(GITHUB_URL)])]; + github.listedHosts = [listed("github", [request(GITHUB_URL)])]; const listedLocal = (await service.inbox()).items[0]?.local; assert.equal(listedLocal?.workspaceId, null); assert.deepEqual(await service.localReviews({ reviewIds: [GITHUB_ID, GITLAB_ID] }), { @@ -159,7 +159,7 @@ test("on GitLab the drafts are counted for an MR reviewed here only, and a faile { id: "1", body: "Why the retry cap?", location: { kind: "general" } }, { id: "2", body: "Typo", location: { kind: "file", path: "src/upload.ts" } }, ]); - gitlab.reviewRequests = [listed("gitlab", [request(GITLAB_URL), request(other, { number: 8 })])]; + gitlab.listedHosts = [listed("gitlab", [request(GITLAB_URL), request(other, { number: 8 })])]; const items = (await service.inbox()).items; assert.deepEqual( @@ -178,19 +178,19 @@ test("a GitLab review published from here says whether the head moved since, ove await startReview(service, GITLAB_URL); const reviewId = "gitlab/gitlab.com/acme/uploader/7"; - gitlab.reviewRequests = [listed("gitlab", [request(GITLAB_URL, { state: "commented", changedSinceReview: null })])]; + gitlab.listedHosts = [listed("gitlab", [request(GITLAB_URL, { state: "commented", changedSinceReview: null })])]; assert.equal((await service.inbox()).items[0]?.changedSinceReview, null, "nothing published from here yet"); const submitted = await service.submit({ reviewId, headSha: HEAD, forgeHeadSha: HEAD, verdict: "comment", body: "Reads well." }); assert.equal(submitted.status, "submitted"); - gitlab.reviewRequests = [listed("gitlab", [request(GITLAB_URL, { state: "commented", changedSinceReview: true })])]; + gitlab.listedHosts = [listed("gitlab", [request(GITLAB_URL, { state: "commented", changedSinceReview: true })])]; assert.equal((await service.inbox()).items[0]?.changedSinceReview, false, "the head is still the one reviewed"); - gitlab.reviewRequests = [listed("gitlab", [request(GITLAB_URL, { state: "commented", headSha: PUSHED, changedSinceReview: false })])]; + gitlab.listedHosts = [listed("gitlab", [request(GITLAB_URL, { state: "commented", headSha: PUSHED, changedSinceReview: false })])]; assert.equal((await service.inbox()).items[0]?.changedSinceReview, true, "pushed since"); - gitlab.reviewRequests = [listed("gitlab", [request(GITLAB_URL, { state: "requested", changedSinceReview: null })])]; + gitlab.listedHosts = [listed("gitlab", [request(GITLAB_URL, { state: "requested", changedSinceReview: null })])]; assert.equal((await service.inbox()).items[0]?.changedSinceReview, null, "asked again, which the listing says"); }); @@ -200,8 +200,8 @@ async function checkedOff(service: ReviewService): Promise { test("a check-off holds while the change request stands still, survives a restart and is undone by unchecking", async (t) => { const { github, gitlab, service, restart } = await withForges(t); - github.reviewRequests = [listed("github", [request(GITHUB_URL)])]; - gitlab.reviewRequests = [listed("gitlab", [request(GITLAB_URL)])]; + github.listedHosts = [listed("github", [request(GITHUB_URL)])]; + gitlab.listedHosts = [listed("gitlab", [request(GITLAB_URL)])]; await service.setCheckedOff({ url: GITLAB_URL, checkOff: { headSha: HEAD, state: "requested" } }); assert.deepEqual(await checkedOff(service), [GITLAB_URL]); @@ -217,7 +217,7 @@ test("a check-off comes back on a push, or when the reviewer is asked again or l await service.setCheckedOff({ url: GITLAB_URL, checkOff }); const seen: boolean[] = []; for (const listing of listings) { - gitlab.reviewRequests = [listed("gitlab", [request(GITLAB_URL, listing)])]; + gitlab.listedHosts = [listed("gitlab", [request(GITLAB_URL, listing)])]; seen.push((await checkedOff(service)).length === 1); } return seen; @@ -236,25 +236,25 @@ test("a check-off comes back on a push, or when the reviewer is asked again or l test("a check-off goes once a host listed in full no longer has it, and stays while its host fails or has more", async (t) => { const { gitlab, service } = await withForges(t); - gitlab.reviewRequests = [listed("gitlab", [request(GITLAB_URL)])]; + gitlab.listedHosts = [listed("gitlab", [request(GITLAB_URL)])]; await service.setCheckedOff({ url: GITLAB_URL, checkOff: { headSha: HEAD, state: "requested" } }); - gitlab.reviewRequests = [listed("gitlab", [], { error: "glab is not logged in to gitlab.com." })]; + gitlab.listedHosts = [listed("gitlab", [], { error: "glab is not logged in to gitlab.com." })]; await service.inbox(); - gitlab.reviewRequests = [listed("gitlab", [], { truncated: true })]; + gitlab.listedHosts = [listed("gitlab", [], { truncated: true })]; await service.inbox(); - gitlab.reviewRequests = [listed("gitlab", [request(GITLAB_URL)])]; + gitlab.listedHosts = [listed("gitlab", [request(GITLAB_URL)])]; assert.deepEqual(await checkedOff(service), [GITLAB_URL]); - gitlab.reviewRequests = [listed("gitlab", [])]; + gitlab.listedHosts = [listed("gitlab", [])]; await service.inbox(); - gitlab.reviewRequests = [listed("gitlab", [request(GITLAB_URL)])]; + gitlab.listedHosts = [listed("gitlab", [request(GITLAB_URL)])]; assert.deepEqual(await checkedOff(service), []); }); test("a check-off kept before the list had tabs holds on the Assigned tab", async (t) => { const { data, gitlab, service } = await withForges(t); - gitlab.reviewRequests = [listed("gitlab", [request(GITLAB_URL)])]; + gitlab.listedHosts = [listed("gitlab", [request(GITLAB_URL)])]; const file = path.join(data, "inbox-check-offs.json"); await writeFile(file, JSON.stringify({ [GITLAB_URL]: { headSha: HEAD, state: "requested" } })); @@ -269,7 +269,7 @@ test("a check-off kept before the list had tabs holds on the Assigned tab", asyn test("a check-off on the Authored tab leaves the Assigned row alone, and outlasts the Assigned tab's check-offs", async (t) => { const { data, gitlab, service } = await withForges(t); - gitlab.reviewRequests = [listed("gitlab", [request(GITLAB_URL)])]; + gitlab.listedHosts = [listed("gitlab", [request(GITLAB_URL)])]; const file = path.join(data, "inbox-check-offs.json"); const authored = { [GITLAB_URL]: { status: "waiting", latestReviewAt: null } }; await writeFile(file, JSON.stringify({ assigned: {}, authored })); @@ -277,7 +277,7 @@ test("a check-off on the Authored tab leaves the Assigned row alone, and outlast assert.deepEqual(await checkedOff(service), []); await service.setCheckedOff({ url: GITLAB_URL, checkOff: { headSha: HEAD, state: "requested" } }); - gitlab.reviewRequests = [listed("gitlab", [request(GITLAB_URL, { headSha: PUSHED })])]; + gitlab.listedHosts = [listed("gitlab", [request(GITLAB_URL, { headSha: PUSHED })])]; assert.deepEqual(await checkedOff(service), [], "came back on Assigned"); assert.deepEqual(JSON.parse(await readFile(file, "utf8")).authored, authored); }); diff --git a/plugins/guided-review/server/review-inbox.ts b/plugins/guided-review/server/review-inbox.ts index bb088f3..36abdef 100644 --- a/plugins/guided-review/server/review-inbox.ts +++ b/plugins/guided-review/server/review-inbox.ts @@ -44,7 +44,7 @@ export class ReviewInbox { } async list(): Promise { - const listed = (await Promise.all(this.#forges.map((forge) => forge.listReviewRequests()))).flat(); + const listed = (await Promise.all(this.#forges.map((forge) => forge.listChangeRequests()))).flat(); const requests = listed.flatMap((host) => host.requests); const checkedOff = await this.#checkOffs.follow(listed); const items = await Promise.all(requests.map(async (request) => ({ ...(await this.#item(request)), checkedOff: checkedOff.has(request.url) }))); diff --git a/plugins/guided-review/shared/inbox.ts b/plugins/guided-review/shared/inbox.ts index 2eefa99..9bac384 100644 --- a/plugins/guided-review/shared/inbox.ts +++ b/plugins/guided-review/shared/inbox.ts @@ -53,7 +53,7 @@ export type CiState = z.output; export type ReviewRequest = z.output; /** What one host of a forge listed, or why it could not, so one host failing leaves the others listed. */ -export const ReviewRequestHostSchema = z.object({ +export const ListedHostSchema = z.object({ forge: z.enum(["github", "gitlab"]), host: z.string(), requests: z.array(ReviewRequestSchema), @@ -63,7 +63,7 @@ export const ReviewRequestHostSchema = z.object({ error: z.string().nullable(), }); -export type ReviewRequestHost = z.output; +export type ListedHost = z.output; /** * What this plugin has of a change request's review, if the reviewer started one here: the start @@ -95,7 +95,7 @@ export const AuthoredCheckOffSchema = z.object({ status: ReviewStatusSchema, lat /** Every host's review requests, each joined with the review started here, if any. */ export const InboxSchema = z.object({ - hosts: z.array(ReviewRequestHostSchema.omit({ requests: true })), + hosts: z.array(ListedHostSchema.omit({ requests: true })), items: z.array(InboxItemSchema), }); From 94bd5ff7a8e30e623d0aac276cc40535ea0ad081 Mon Sep 17 00:00:00 2001 From: Sleeyax Date: Mon, 12 Oct 2026 13:56:08 +0200 Subject: [PATCH 03/16] feat(guided-review): list the viewer's own open GitHub PRs beside their review requests The inbox listing searches `is:pr is:open author:@me archived:false` on GitHub as a third search, of the same size and in the same call as the other two, so one Refresh lists both and a host that fails says so once. Each host now answers its review requests and the change requests the viewer authored, with whether each list was cut off, so a tab's total and its check-offs can tell a partial list from a full one. The inbox lists the authored change requests beside the review requests, each joined with the review started here by the same ID, so a change request on both tabs shares one review, its guide and its drafts. An authored change request is a review request's fields without the reviewer's own state, team and changed-since-review, which later tickets extend with the review status and reviewers. GitLab answers no authored change requests yet. Refs #146 Co-Authored-By: Claude Opus 5.5 --- plugins/guided-review/client/inbox.tsx | 2 +- .../github/review-search-authored.json | 56 ++++++++++ .../guided-review/server/forge/github.test.ts | 101 +++++++++++++---- plugins/guided-review/server/forge/github.ts | 102 ++++++++++++------ .../guided-review/server/forge/gitlab.test.ts | 6 +- plugins/guided-review/server/forge/gitlab.ts | 5 +- plugins/guided-review/server/forge/port.ts | 4 +- .../guided-review/server/inbox-check-offs.ts | 9 +- .../guided-review/server/review-inbox.test.ts | 72 ++++++++++--- plugins/guided-review/server/review-inbox.ts | 56 ++++++---- plugins/guided-review/shared/inbox.ts | 53 ++++++--- 11 files changed, 351 insertions(+), 115 deletions(-) create mode 100644 plugins/guided-review/server/forge/fixtures/github/review-search-authored.json diff --git a/plugins/guided-review/client/inbox.tsx b/plugins/guided-review/client/inbox.tsx index 14dcda3..cef6c02 100644 --- a/plugins/guided-review/client/inbox.tsx +++ b/plugins/guided-review/client/inbox.tsx @@ -276,7 +276,7 @@ function Note({ colors, text, tone }: { colors: Colors; text: string; tone?: "da function HostNotes({ colors, hosts }: { colors: Colors; hosts: Inbox["hosts"] }) { const notes = hosts.flatMap((host): { key: string; tone?: "danger"; text: string }[] => { if (host.error !== null) return [{ key: host.host, tone: "danger", text: `${host.host}: ${host.error}` }]; - if (host.truncated) return [{ key: host.host, text: `${host.host} has more than this lists; only the first page is shown.` }]; + if (host.truncated.assigned) return [{ key: host.host, text: `${host.host} has more than this lists; only the first page is shown.` }]; return []; }); if (notes.length === 0) return null; diff --git a/plugins/guided-review/server/forge/fixtures/github/review-search-authored.json b/plugins/guided-review/server/forge/fixtures/github/review-search-authored.json new file mode 100644 index 0000000..f573e43 --- /dev/null +++ b/plugins/guided-review/server/forge/fixtures/github/review-search-authored.json @@ -0,0 +1,56 @@ +{ + "data": { + "search": { + "issueCount": 2, + "nodes": [ + { + "number": 31, + "url": "https://github.com/example-org/uploader/pull/31", + "title": "Resume uploads after a dropped connection", + "isDraft": false, + "createdAt": "2026-10-01T08:30:00Z", + "updatedAt": "2026-10-06T16:45:12Z", + "headRefOid": "3c9e0b7a1f2d4e6c8b0a9f8e7d6c5b4a39281706", + "additions": 128, + "deletions": 14, + "changedFiles": 5, + "author": { + "login": "sleeyax" + }, + "statusCheckRollup": { + "state": "FAILURE" + }, + "reviews": { + "nodes": [ + { + "viewerDidAuthor": true, + "comments": { + "totalCount": 1 + } + } + ] + } + }, + { + "number": 4, + "url": "https://github.com/example-org/docs/pull/4", + "title": "Describe the retry settings", + "isDraft": true, + "createdAt": "2026-10-05T11:00:00Z", + "updatedAt": "2026-10-05T11:20:00Z", + "headRefOid": "9a8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b", + "additions": 6, + "deletions": 0, + "changedFiles": 1, + "author": { + "login": "sleeyax" + }, + "statusCheckRollup": null, + "reviews": { + "nodes": [] + } + } + ] + } + } +} diff --git a/plugins/guided-review/server/forge/github.test.ts b/plugins/guided-review/server/forge/github.test.ts index 288c633..1e8ba74 100644 --- a/plugins/guided-review/server/forge/github.test.ts +++ b/plugins/guided-review/server/forge/github.test.ts @@ -5,6 +5,7 @@ import test from "node:test"; import { fakeCommandRunner, type FakeCommandRunner, type ScriptedResult } from "../fake-command-runner.ts"; import { ADD_THREAD_MUTATION, + AUTHORED_SEARCH_QUERY, COMMIT_PULL_REQUESTS_QUERY, createGitHubForge, MAX_COMPARE_FILES, @@ -716,8 +717,17 @@ test("a discussion on the pull request is a comment on its conversation, as GitH assert.deepEqual(run.calls[0]?.args, ["api", "--hostname", "github.com", "--method", "POST", "repos/sleeyax/paseo-plugins/issues/105/comments", "--input", "-"]); }); -test("lists what the viewer is asked to review and what they reviewed, as two searches on github.com", async () => { - const { forge, run } = forgeReplaying([{ stdout: fixture("review-search-requested.json") }, { stdout: fixture("review-search-reviewed.json") }]); +/** A listing's three searches as gh answers them, in the order they go out: asked to review, reviewed, authored. */ +function searches(answers: { requested?: ScriptedResult; reviewed?: ScriptedResult; authored?: ScriptedResult } = {}): ScriptedResult[] { + return [ + answers.requested ?? { stdout: fixture("review-search-requested.json") }, + answers.reviewed ?? { stdout: fixture("review-search-reviewed.json") }, + answers.authored ?? { stdout: fixture("review-search-authored.json") }, + ]; +} + +test("lists what the viewer is asked to review, what they reviewed and what they authored, as three searches on github.com", async () => { + const { forge, run } = forgeReplaying(searches()); const [host, ...others] = await forge.listChangeRequests(); @@ -727,16 +737,18 @@ test("lists what the viewer is asked to review and what they reviewed, as two se [ ["api", "graphql", "--hostname", "github.com", "--input", "-"], ["api", "graphql", "--hostname", "github.com", "--input", "-"], + ["api", "graphql", "--hostname", "github.com", "--input", "-"], ], ); assert.deepEqual(graphqlCalls(run), [ { query: REVIEW_SEARCH_QUERY, variables: { query: REVIEW_SEARCHES.requested } }, { query: REVIEW_SEARCH_QUERY, variables: { query: REVIEW_SEARCHES.reviewed } }, + { query: AUTHORED_SEARCH_QUERY, variables: { query: REVIEW_SEARCHES.authored } }, ]); assert.equal(host?.forge, "github"); assert.equal(host?.host, "github.com"); assert.equal(host?.error, null); - assert.equal(host?.truncated, false); + assert.deepEqual(host?.truncated, { assigned: false, authored: false }); assert.equal(host?.requests.length, 7); assert.deepEqual(host?.requests[0], { @@ -763,7 +775,7 @@ test("lists what the viewer is asked to review and what they reviewed, as two se }); test("a PR the viewer is asked to review again reads as requested and changed, with the team it was asked of", async () => { - const { forge } = forgeReplaying([{ stdout: fixture("review-search-requested.json") }, { stdout: fixture("review-search-reviewed.json") }]); + const { forge } = forgeReplaying(searches()); const [host] = await forge.listChangeRequests(); const again = host?.requests.find((request) => request.number === 7); @@ -778,7 +790,7 @@ test("a PR the viewer is asked to review again reads as requested and changed, w }); test("a PR the viewer reviewed and is not asked again reads by their review, changed when the head moved past it", async () => { - const { forge } = forgeReplaying([{ stdout: fixture("review-search-requested.json") }, { stdout: fixture("review-search-reviewed.json") }]); + const { forge } = forgeReplaying(searches()); const [host] = await forge.listChangeRequests(); const byUrl = (url: string) => host?.requests.find((request) => request.url === url); @@ -792,7 +804,7 @@ test("a PR the viewer reviewed and is not asked again reads by their review, cha }); test("a PR the viewer reviewed without being asked, by name or through a team, is left out", async () => { - const { forge } = forgeReplaying([{ stdout: fixture("review-search-requested.json") }, { stdout: fixture("review-search-reviewed.json") }]); + const { forge } = forgeReplaying(searches()); const [host] = await forge.listChangeRequests(); const urls = host?.requests.map((request) => request.url); @@ -803,26 +815,75 @@ test("a PR the viewer reviewed without being asked, by name or through a team, i assert.ok(urls?.includes("https://github.com/stretchr/testify/pull/1467")); }); -test("a search with more results than it returned marks the host as truncated", async () => { - const requested = JSON.parse(fixture("review-search-requested.json")); - requested.data.search.issueCount = 120; - const { forge } = forgeReplaying([{ stdout: JSON.stringify(requested) }, { stdout: fixture("review-search-reviewed.json") }]); +test("a search with more results than it returned marks its list on the host as truncated", async () => { + const more = (name: string) => { + const answer = JSON.parse(fixture(name)); + answer.data.search.issueCount = 120; + return { stdout: JSON.stringify(answer) }; + }; - const [host] = await forge.listChangeRequests(); + const [assigned] = await forgeReplaying(searches({ requested: more("review-search-requested.json") })).forge.listChangeRequests(); + const [authored] = await forgeReplaying(searches({ authored: more("review-search-authored.json") })).forge.listChangeRequests(); - assert.equal(host?.truncated, true); + assert.deepEqual(assigned?.truncated, { assigned: true, authored: false }); + assert.deepEqual(authored?.truncated, { assigned: false, authored: true }); }); -test("a search gh cannot run comes back as the host's error rather than a throw", async () => { - const { forge } = forgeReplaying([ - { exitCode: 4, stderr: "To get started with GitHub CLI, please run: gh auth login\n" }, - { stdout: fixture("review-search-reviewed.json") }, - ]); +test("a search gh cannot run comes back as the host's error once, with both lists empty, rather than a throw", async () => { + const failed = { exitCode: 4, stderr: "To get started with GitHub CLI, please run: gh auth login\n" }; + const error = { + forge: "github", + host: "github.com", + requests: [], + authored: [], + truncated: { assigned: false, authored: false }, + error: "gh failed: To get started with GitHub CLI, please run: gh auth login", + }; - const hosts = await forge.listChangeRequests(); + assert.deepEqual(await forgeReplaying(searches({ requested: failed })).forge.listChangeRequests(), [error]); + assert.deepEqual(await forgeReplaying(searches({ authored: failed })).forge.listChangeRequests(), [error]); +}); + +test("the viewer's own open PRs list as authored, drafts included, with their own pending drafts", async () => { + const [host] = await forgeReplaying(searches()).forge.listChangeRequests(); - assert.deepEqual(hosts, [ - { forge: "github", host: "github.com", requests: [], truncated: false, error: "gh failed: To get started with GitHub CLI, please run: gh auth login" }, + assert.deepEqual(host?.authored, [ + { + forge: "github", + host: "github.com", + project: "example-org/uploader", + number: 31, + url: "https://github.com/example-org/uploader/pull/31", + title: "Resume uploads after a dropped connection", + author: "sleeyax", + isDraft: false, + createdAt: "2026-10-01T08:30:00Z", + updatedAt: "2026-10-06T16:45:12Z", + headSha: "3c9e0b7a1f2d4e6c8b0a9f8e7d6c5b4a39281706", + additions: 128, + deletions: 14, + fileCount: 5, + ci: "failure", + pendingDrafts: 1, + }, + { + forge: "github", + host: "github.com", + project: "example-org/docs", + number: 4, + url: "https://github.com/example-org/docs/pull/4", + title: "Describe the retry settings", + author: "sleeyax", + isDraft: true, + createdAt: "2026-10-05T11:00:00Z", + updatedAt: "2026-10-05T11:20:00Z", + headSha: "9a8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b", + additions: 6, + deletions: 0, + fileCount: 1, + ci: null, + pendingDrafts: 0, + }, ]); }); diff --git a/plugins/guided-review/server/forge/github.ts b/plugins/guided-review/server/forge/github.ts index 0751567..1f199f8 100644 --- a/plugins/guided-review/server/forge/github.ts +++ b/plugins/guided-review/server/forge/github.ts @@ -3,7 +3,7 @@ import type { CommandRunner } from "../command-runner.ts"; import { oneAtATimePer } from "../one-at-a-time.ts"; import { createCli } from "./cli.ts"; import type { Draft, DraftLocation } from "../../shared/drafts.ts"; -import type { CiState, ReviewerState, ReviewRequest, ListedHost } from "../../shared/inbox.ts"; +import type { AuthoredChangeRequest, CiState, ListedChangeRequest, ListedHost, ReviewerState, ReviewRequest } from "../../shared/inbox.ts"; import type { Verdict } from "../../shared/submit.ts"; import { CLONE_TIMEOUT_MS, MAX_BRANCH_CHANGE_REQUESTS, MAX_COMMITS, MAX_LINKED_ISSUES, userOf } from "./common.ts"; import { SubmitSteps } from "./submit-steps.ts"; @@ -346,31 +346,36 @@ const SUBMIT_LABELS: Record = { * GitHub drops a review request once the reviewer reviews, so what they are a reviewer of is two * searches: the requests outstanding, team requests included, and the ones they answered. Pages are * kept at half a GraphQL page, because a full one of these timed out as a 502. + * What the viewer authored is a third search, of the same size. * `reviewed-by` also finds reviews nobody asked for, so `wasAskedToReview` keeps only those asked of the viewer. */ export const REVIEW_SEARCHES = { requested: "is:pr is:open review-requested:@me archived:false", reviewed: "is:pr is:open reviewed-by:@me -author:@me -review-requested:@me archived:false", + authored: "is:pr is:open author:@me archived:false", } as const; const REVIEW_SEARCH_SIZE = 50; const REVIEW_REQUEST_EVENTS = 20; /** + * What every listed PR is read with. * A pending review is visible only to its author, so the first one is the viewer's, and its comments * are their drafts. A PR author can be a deleted account, which GitHub gives as null. */ +const LISTED_PULL_REQUEST_FIELDS = `number url title isDraft createdAt updatedAt headRefOid additions deletions changedFiles + author { login } + statusCheckRollup { state } + reviews(states: PENDING, first: 1) { nodes { viewerDidAuthor comments { totalCount } } }`; + export const REVIEW_SEARCH_QUERY = `query GuidedReviewInbox($query: String!) { viewer { login } search(type: ISSUE, query: $query, first: ${REVIEW_SEARCH_SIZE}) { issueCount nodes { ... on PullRequest { - number url title isDraft createdAt updatedAt headRefOid additions deletions changedFiles - author { login } - statusCheckRollup { state } + ${LISTED_PULL_REQUEST_FIELDS} viewerLatestReview { state commit { oid } } viewerLatestReviewRequest { requestedReviewer { __typename ... on Team { combinedSlug } } } - reviews(states: PENDING, first: 1) { nodes { viewerDidAuthor comments { totalCount } } } timelineItems(itemTypes: [REVIEW_REQUESTED_EVENT], first: ${REVIEW_REQUEST_EVENTS}) { nodes { ... on ReviewRequestedEvent { requestedReviewer { __typename ... on User { login } } } } } @@ -379,25 +384,40 @@ export const REVIEW_SEARCH_QUERY = `query GuidedReviewInbox($query: String!) { } }`; +export const AUTHORED_SEARCH_QUERY = `query GuidedReviewAuthored($query: String!) { + search(type: ISSUE, query: $query, first: ${REVIEW_SEARCH_SIZE}) { + issueCount + nodes { + ... on PullRequest { + ${LISTED_PULL_REQUEST_FIELDS} + } + } + } +}`; + +const ListedPullRequest = z.object({ + number: z.number(), + url: z.string(), + title: z.string(), + isDraft: z.boolean(), + createdAt: z.string(), + updatedAt: z.string(), + headRefOid: z.string(), + additions: z.number(), + deletions: z.number(), + changedFiles: z.number(), + author: z.object({ login: z.string() }).nullable(), + statusCheckRollup: z.object({ state: z.enum(["EXPECTED", "ERROR", "FAILURE", "PENDING", "SUCCESS"]) }).nullable(), + reviews: z.object({ nodes: z.array(z.object({ viewerDidAuthor: z.boolean(), comments: z.object({ totalCount: z.number() }) })) }), +}); + const ReviewSearchResponse = z.object({ data: z.object({ viewer: z.object({ login: z.string() }), search: z.object({ issueCount: z.number(), nodes: z.array( - z.object({ - number: z.number(), - url: z.string(), - title: z.string(), - isDraft: z.boolean(), - createdAt: z.string(), - updatedAt: z.string(), - headRefOid: z.string(), - additions: z.number(), - deletions: z.number(), - changedFiles: z.number(), - author: z.object({ login: z.string() }).nullable(), - statusCheckRollup: z.object({ state: z.enum(["EXPECTED", "ERROR", "FAILURE", "PENDING", "SUCCESS"]) }).nullable(), + ListedPullRequest.extend({ viewerLatestReview: z .object({ state: z.enum(["PENDING", "COMMENTED", "APPROVED", "CHANGES_REQUESTED", "DISMISSED"]), @@ -407,7 +427,6 @@ const ReviewSearchResponse = z.object({ viewerLatestReviewRequest: z .object({ requestedReviewer: z.object({ __typename: z.string(), combinedSlug: z.string().optional() }).nullable() }) .nullable(), - reviews: z.object({ nodes: z.array(z.object({ viewerDidAuthor: z.boolean(), comments: z.object({ totalCount: z.number() }) })) }), timelineItems: z.object({ nodes: z.array(z.object({ requestedReviewer: z.object({ __typename: z.string(), login: z.string().optional() }).nullable() })), }), @@ -417,9 +436,14 @@ const ReviewSearchResponse = z.object({ }), }); +const AuthoredSearchResponse = z.object({ + data: z.object({ search: z.object({ issueCount: z.number(), nodes: z.array(ListedPullRequest) }) }), +}); + +type ListedPullRequest = z.output; type SearchedPullRequest = z.output["data"]["search"]["nodes"][number]; -const CI_STATES: Record["state"], CiState> = { +const CI_STATES: Record["state"], CiState> = { SUCCESS: "success", FAILURE: "failure", ERROR: "failure", @@ -449,10 +473,21 @@ function wasAskedToReview(pr: SearchedPullRequest, login: string): boolean { * dismissed review is still one they wrote, and a pending one is not written yet. */ function reviewRequestOf(pr: SearchedPullRequest, requested: boolean): ReviewRequest | null { - const ref = parsePullRequestUrl(pr.url); - if (ref === null) return null; + const listed = listedOf(pr); + if (listed === null) return null; const review = pr.viewerLatestReview?.state === "PENDING" ? null : pr.viewerLatestReview; const team = pr.viewerLatestReviewRequest?.requestedReviewer; + return { + ...listed, + state: requested || review === null ? "requested" : REVIEWER_STATES[review.state], + viaTeam: requested && team?.__typename === "Team" ? (team.combinedSlug ?? null) : null, + changedSinceReview: review === null ? null : requested || review.commit?.oid !== pr.headRefOid, + }; +} + +function listedOf(pr: ListedPullRequest): ListedChangeRequest | null { + const ref = parsePullRequestUrl(pr.url); + if (ref === null) return null; return { forge: "github", host: ref.host, @@ -469,9 +504,6 @@ function reviewRequestOf(pr: SearchedPullRequest, requested: boolean): ReviewReq deletions: pr.deletions, fileCount: pr.changedFiles, ci: pr.statusCheckRollup ? CI_STATES[pr.statusCheckRollup.state] : null, - state: requested || review === null ? "requested" : REVIEWER_STATES[review.state], - viaTeam: requested && team?.__typename === "Team" ? (team.combinedSlug ?? null) : null, - changedSinceReview: review === null ? null : requested || review.commit?.oid !== pr.headRefOid, pendingDrafts: pr.reviews.nodes.find((pending) => pending.viewerDidAuthor)?.comments.totalCount ?? 0, }; } @@ -749,21 +781,27 @@ export function createGitHubForge(options: GitHubForgeOptions): Forge { }, async listChangeRequests(): Promise { - const search = (query: string) => - gh.json(ReviewSearchResponse, ["api", "graphql", "--hostname", GITHUB_HOST, "--input", "-"], { - input: JSON.stringify({ query: REVIEW_SEARCH_QUERY, variables: { query } }), + const search = (response: T, query: string, search: string) => + gh.json(response, ["api", "graphql", "--hostname", GITHUB_HOST, "--input", "-"], { + input: JSON.stringify({ query, variables: { query: search } }), }); + const cut = (...answers: { data: { search: { issueCount: number; nodes: unknown[] } } }[]) => + answers.some(({ data }) => data.search.issueCount > data.search.nodes.length); try { - const [requested, reviewed] = await Promise.all([search(REVIEW_SEARCHES.requested), search(REVIEW_SEARCHES.reviewed)]); + const [requested, reviewed, mine] = await Promise.all([ + search(ReviewSearchResponse, REVIEW_SEARCH_QUERY, REVIEW_SEARCHES.requested), + search(ReviewSearchResponse, REVIEW_SEARCH_QUERY, REVIEW_SEARCHES.reviewed), + search(AuthoredSearchResponse, AUTHORED_SEARCH_QUERY, REVIEW_SEARCHES.authored), + ]); const requests = [ ...requested.data.search.nodes.map((pr) => reviewRequestOf(pr, true)), ...reviewed.data.search.nodes.filter((pr) => wasAskedToReview(pr, reviewed.data.viewer.login)).map((pr) => reviewRequestOf(pr, false)), ].filter((request) => request !== null); - const truncated = [requested, reviewed].some(({ data }) => data.search.issueCount > data.search.nodes.length); - return [{ forge: "github", host: GITHUB_HOST, requests, truncated, error: null }]; + const authored: AuthoredChangeRequest[] = mine.data.search.nodes.map(listedOf).filter((listed) => listed !== null); + return [{ forge: "github", host: GITHUB_HOST, requests, authored, truncated: { assigned: cut(requested, reviewed), authored: cut(mine) }, error: null }]; } catch (error) { if (!(error instanceof ForgeError)) throw error; - return [{ forge: "github", host: GITHUB_HOST, requests: [], truncated: false, error: error.message }]; + return [{ forge: "github", host: GITHUB_HOST, requests: [], authored: [], truncated: { assigned: false, authored: false }, error: error.message }]; } }, }; diff --git a/plugins/guided-review/server/forge/gitlab.test.ts b/plugins/guided-review/server/forge/gitlab.test.ts index 2461daa..c211378 100644 --- a/plugins/guided-review/server/forge/gitlab.test.ts +++ b/plugins/guided-review/server/forge/gitlab.test.ts @@ -1195,7 +1195,7 @@ test("lists the open MRs the viewer reviews on each host glab is logged in to, a assert.equal(hosts.length, 1); const [host] = hosts; assert.equal(host?.error, null); - assert.equal(host?.truncated, false); + assert.deepEqual(host?.truncated, { assigned: false, authored: false }); assert.deepEqual(host?.requests[0], { forge: "gitlab", host: "gitlab.com", @@ -1255,7 +1255,7 @@ test("a host glab is not logged in to, or one older than 19.2, comes back with i const { forge: old } = forgeReplaying([AUTH_STATUS_ALL, version("18.11.2-ee")]); assert.deepEqual(await old.listChangeRequests(), [ - { forge: "gitlab", host: "gitlab.com", requests: [], truncated: false, error: "gitlab.com runs GitLab 18.11.2-ee; listing your reviews needs 19.2 or newer." }, + { forge: "gitlab", host: "gitlab.com", requests: [], authored: [], truncated: { assigned: false, authored: false }, error: "gitlab.com runs GitLab 18.11.2-ee; listing your reviews needs 19.2 or newer." }, ]); }); @@ -1270,7 +1270,7 @@ test("a GraphQL answer with errors and no user is the host's error, and more MRs const more = JSON.parse(fixture("review-list.json")); more.data.currentUser.reviewRequestedMergeRequests.pageInfo.hasNextPage = true; const { forge: paged } = forgeReplaying([AUTH_STATUS_ALL, version("19.5.0-pre"), { stdout: JSON.stringify(more) }]); - assert.equal((await paged.listChangeRequests())[0]?.truncated, true); + assert.deepEqual((await paged.listChangeRequests())[0]?.truncated, { assigned: true, authored: false }); }); test("a host listed as logged in is not checked again before reading an MR on it", async () => { diff --git a/plugins/guided-review/server/forge/gitlab.ts b/plugins/guided-review/server/forge/gitlab.ts index cc6a7c9..e13f99d 100644 --- a/plugins/guided-review/server/forge/gitlab.ts +++ b/plugins/guided-review/server/forge/gitlab.ts @@ -447,7 +447,8 @@ export function createGitLabForge(options: GitLabForgeOptions): Forge { forge: "gitlab", host, requests: nodes.map((mr) => reviewRequestOf(mr, user.username)).filter((request) => request !== null), - truncated: pageInfo.hasNextPage, + authored: [], + truncated: { assigned: pageInfo.hasNextPage, authored: false }, }; }; @@ -833,7 +834,7 @@ export function createGitLabForge(options: GitLabForgeOptions): Forge { const statuses = parseAuthStatus(await glab.report(["auth", "status", "--all"])); return Promise.all( statuses.map(async ({ host, loggedIn, problem }): Promise => { - const listed = { forge: "gitlab" as const, host, requests: [], truncated: false }; + const listed = { forge: "gitlab" as const, host, requests: [], authored: [], truncated: { assigned: false, authored: false } }; if (!loggedIn) { const reason = problem === null ? "" : ` (${problem})`; return { ...listed, error: `glab is not logged in to ${host}${reason}. Run \`glab auth login --hostname ${host}\` on the daemon's host.` }; diff --git a/plugins/guided-review/server/forge/port.ts b/plugins/guided-review/server/forge/port.ts index 86ffbb2..1bf69e3 100644 --- a/plugins/guided-review/server/forge/port.ts +++ b/plugins/guided-review/server/forge/port.ts @@ -95,8 +95,8 @@ export interface Forge { discardReview(ref: ChangeRequestRef): Promise; /** - * The open change requests the current user is a reviewer of, on every host of this forge the CLI - * is logged in to. A host that cannot be listed comes back with its error rather than throwing. + * The open change requests the current user is a reviewer of, and those they authored, on every host of this forge the CLI is logged in to. + * A host that cannot be listed comes back with its error rather than throwing. */ listChangeRequests(): Promise; } diff --git a/plugins/guided-review/server/inbox-check-offs.ts b/plugins/guided-review/server/inbox-check-offs.ts index 4f31e0d..4baffd4 100644 --- a/plugins/guided-review/server/inbox-check-offs.ts +++ b/plugins/guided-review/server/inbox-check-offs.ts @@ -9,6 +9,7 @@ import { type ReviewRequest, type ListedHost, } from "../shared/inbox.ts"; +import type { InboxTab } from "../shared/inbox-preferences.ts"; import { byTab } from "./inbox-tabs.ts"; import { readJson, writeJson } from "./json-file.ts"; import { oneAtATimePer } from "./one-at-a-time.ts"; @@ -41,10 +42,10 @@ export class InboxCheckOffsFile { }); } - /** Follows a listing of every host, and answers the URLs still checked off. */ - async follow(hosts: readonly ListedHost[]): Promise> { + /** Follows a listing of every host, and answers the URLs still checked off on each tab. */ + async follow(hosts: readonly ListedHost[]): Promise>> { const followed = await this.#change((checkOffs) => ({ ...checkOffs, assigned: followListing(checkOffs.assigned, hosts) })); - return new Set(Object.keys(followed.assigned)); + return { assigned: new Set(Object.keys(followed.assigned)), authored: new Set(Object.keys(followed.authored)) }; } async #read(): Promise { @@ -68,7 +69,7 @@ export class InboxCheckOffsFile { */ function followListing(checkOffs: AssignedCheckOffs, hosts: readonly ListedHost[]): AssignedCheckOffs { const requests = new Map(hosts.flatMap((host) => host.requests.map((request) => [request.url, request] as const))); - const complete = new Set(hosts.filter((host) => host.error === null && !host.truncated).map((host) => host.host)); + const complete = new Set(hosts.filter((host) => host.error === null && !host.truncated.assigned).map((host) => host.host)); const followed: AssignedCheckOffs = {}; for (const [url, checkOff] of Object.entries(checkOffs)) { const request = requests.get(url); diff --git a/plugins/guided-review/server/review-inbox.test.ts b/plugins/guided-review/server/review-inbox.test.ts index a9ae477..8e9c989 100644 --- a/plugins/guided-review/server/review-inbox.test.ts +++ b/plugins/guided-review/server/review-inbox.test.ts @@ -3,7 +3,7 @@ import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import test, { type TestContext } from "node:test"; -import { headMoved, type ReviewRequest, type ListedHost } from "../shared/inbox.ts"; +import { headMoved, type AuthoredChangeRequest, type ListedHost, type ReviewRequest } from "../shared/inbox.ts"; import { fakeForge, sampleChangeRequest, type FakeForge } from "./fake-forge.ts"; import { fakeGuideAgents, sampleGuideReply, type FakeGuideAgents } from "./fake-guide-agents.ts"; import { fakeWorkspaces, type FakeWorkspaces } from "./fake-workspaces.ts"; @@ -66,29 +66,67 @@ function request(url: string, overrides: Partial = {}): ReviewReq }; } +function authored(url: string, overrides: Partial = {}): AuthoredChangeRequest { + const { state: _, viaTeam: __, changedSinceReview: ___, ...listed } = request(url, { author: "reviewer" }); + return { ...listed, ...overrides }; +} + +const COMPLETE = { assigned: false, authored: false }; + function listed(forge: "github" | "gitlab", requests: ReviewRequest[], extra: Partial = {}): ListedHost { - return { forge, host: forge === "github" ? "github.com" : "gitlab.com", requests, truncated: false, error: null, ...extra }; + return { forge, host: forge === "github" ? "github.com" : "gitlab.com", requests, authored: [], truncated: COMPLETE, error: null, ...extra }; } -test("lists every forge's review requests with their hosts, and none started here has a local review", async (t) => { +test("lists every forge's review requests and the viewer's own change requests with their hosts, and none started here has a local review", async (t) => { const { github, gitlab, service } = await withForges(t); - github.listedHosts = [listed("github", [request(GITHUB_URL)], { truncated: true })]; + const mine = "https://github.com/acme/uploader/pull/9"; + github.listedHosts = [listed("github", [request(GITHUB_URL)], { authored: [authored(mine, { number: 9 })], truncated: { assigned: true, authored: false } })]; gitlab.listedHosts = [ - listed("gitlab", [request(GITLAB_URL)]), + listed("gitlab", [request(GITLAB_URL)], { truncated: { assigned: false, authored: true } }), listed("gitlab", [], { host: "gitlab.example.com", error: "glab is not logged in to gitlab.example.com." }), ]; const inbox = await service.inbox(); assert.deepEqual(inbox.hosts, [ - { forge: "github", host: "github.com", truncated: true, error: null }, - { forge: "gitlab", host: "gitlab.com", truncated: false, error: null }, - { forge: "gitlab", host: "gitlab.example.com", truncated: false, error: "glab is not logged in to gitlab.example.com." }, + { forge: "github", host: "github.com", truncated: { assigned: true, authored: false }, error: null }, + { forge: "gitlab", host: "gitlab.com", truncated: { assigned: false, authored: true }, error: null }, + { forge: "gitlab", host: "gitlab.example.com", truncated: COMPLETE, error: "glab is not logged in to gitlab.example.com." }, ]); assert.deepEqual(inbox.items, [ { ...request(GITHUB_URL), reviewId: GITHUB_ID, local: null, checkedOff: false }, { ...request(GITLAB_URL), reviewId: GITLAB_ID, local: null, checkedOff: false }, ]); + assert.deepEqual(inbox.authored, [{ ...authored(mine, { number: 9 }), reviewId: "github/github.com/acme/uploader/9", local: null, checkedOff: false }]); +}); + +test("a change request of the viewer's reviewed here carries its review, its guide's state, whether the head moved and its drafts", async (t) => { + const { github, service } = await withForges(t); + await startReview(service, GITHUB_URL); + + github.listedHosts = [listed("github", [], { authored: [authored(GITHUB_URL, { pendingDrafts: 2 })] })]; + const [item] = (await service.inbox()).authored; + assert.equal(item?.reviewId, GITHUB_ID); + assert.deepEqual({ ...item?.local, header: null }, { header: null, preparing: null, guide: "ready", workspaceId: "wks_0000000000000001" }); + assert.equal(headMoved(item!, item!.local!), false); + assert.equal(item?.pendingDrafts, 2); + + github.listedHosts = [listed("github", [], { authored: [authored(GITHUB_URL, { headSha: PUSHED })] })]; + const [pushed] = (await service.inbox()).authored; + assert.equal(headMoved(pushed!, pushed!.local!), true); +}); + +test("a change request listed on both tabs shares one review here", async (t) => { + const { gitlab, service } = await withForges(t); + await startReview(service, GITLAB_URL); + gitlab.drafts.set(GITLAB_URL, [{ id: "1", body: "Why the retry cap?", location: { kind: "general" } }]); + + gitlab.listedHosts = [listed("gitlab", [request(GITLAB_URL)], { authored: [authored(GITLAB_URL)] })]; + const inbox = await service.inbox(); + + assert.equal(inbox.authored[0]?.reviewId, GITLAB_ID); + assert.deepEqual(inbox.authored[0]?.local, inbox.items[0]?.local); + assert.deepEqual([inbox.items[0]?.pendingDrafts, inbox.authored[0]?.pendingDrafts], [1, 1]); }); test("a change request reviewed here carries its review, its guide's state and whether the forge's head moved past it", async (t) => { @@ -241,7 +279,7 @@ test("a check-off goes once a host listed in full no longer has it, and stays wh gitlab.listedHosts = [listed("gitlab", [], { error: "glab is not logged in to gitlab.com." })]; await service.inbox(); - gitlab.listedHosts = [listed("gitlab", [], { truncated: true })]; + gitlab.listedHosts = [listed("gitlab", [], { truncated: { assigned: true, authored: false } })]; await service.inbox(); gitlab.listedHosts = [listed("gitlab", [request(GITLAB_URL)])]; assert.deepEqual(await checkedOff(service), [GITLAB_URL]); @@ -269,15 +307,17 @@ test("a check-off kept before the list had tabs holds on the Assigned tab", asyn test("a check-off on the Authored tab leaves the Assigned row alone, and outlasts the Assigned tab's check-offs", async (t) => { const { data, gitlab, service } = await withForges(t); - gitlab.listedHosts = [listed("gitlab", [request(GITLAB_URL)])]; + gitlab.listedHosts = [listed("gitlab", [request(GITLAB_URL)], { authored: [authored(GITLAB_URL)] })]; const file = path.join(data, "inbox-check-offs.json"); - const authored = { [GITLAB_URL]: { status: "waiting", latestReviewAt: null } }; - await writeFile(file, JSON.stringify({ assigned: {}, authored })); + const kept = { [GITLAB_URL]: { status: "waiting", latestReviewAt: null } }; + await writeFile(file, JSON.stringify({ assigned: {}, authored: kept })); - assert.deepEqual(await checkedOff(service), []); + const inbox = await service.inbox(); + assert.deepEqual([inbox.items[0]?.checkedOff, inbox.authored[0]?.checkedOff], [false, true]); await service.setCheckedOff({ url: GITLAB_URL, checkOff: { headSha: HEAD, state: "requested" } }); - gitlab.listedHosts = [listed("gitlab", [request(GITLAB_URL, { headSha: PUSHED })])]; - assert.deepEqual(await checkedOff(service), [], "came back on Assigned"); - assert.deepEqual(JSON.parse(await readFile(file, "utf8")).authored, authored); + gitlab.listedHosts = [listed("gitlab", [request(GITLAB_URL, { headSha: PUSHED })], { authored: [authored(GITLAB_URL, { headSha: PUSHED })] })]; + const pushed = await service.inbox(); + assert.deepEqual([pushed.items[0]?.checkedOff, pushed.authored[0]?.checkedOff], [false, true], "came back on Assigned only"); + assert.deepEqual(JSON.parse(await readFile(file, "utf8")).authored, kept); }); diff --git a/plugins/guided-review/server/review-inbox.ts b/plugins/guided-review/server/review-inbox.ts index 36abdef..c350177 100644 --- a/plugins/guided-review/server/review-inbox.ts +++ b/plugins/guided-review/server/review-inbox.ts @@ -1,4 +1,4 @@ -import type { Inbox, InboxItem, LocalReview, ReviewRequest } from "../shared/inbox.ts"; +import type { AuthoredChangeRequest, AuthoredInboxItem, Inbox, InboxItem, ListedChangeRequest, LocalReview, ReviewRequest } from "../shared/inbox.ts"; import type { ChangeRequestRef, Forge } from "./forge/port.ts"; import { awaitsScope, type GuideGenerations } from "./guide-generation.ts"; import type { InboxCheckOffsFile } from "./inbox-check-offs.ts"; @@ -17,7 +17,7 @@ export type ReviewInboxOptions = { }; /** - * The review requests of every forge, joined with the reviews started here and what the reviewer checked off: the start screen's list. + * The review requests of every forge and the viewer's own change requests, joined with the reviews started here and what the viewer checked off on each tab: the start screen's list. * Each forge answers per host, so one that cannot be listed leaves the rest listed. * * GitLab's listing cannot count drafts, which take a call per MR, so they are counted only for the @@ -45,10 +45,14 @@ export class ReviewInbox { async list(): Promise { const listed = (await Promise.all(this.#forges.map((forge) => forge.listChangeRequests()))).flat(); - const requests = listed.flatMap((host) => host.requests); const checkedOff = await this.#checkOffs.follow(listed); - const items = await Promise.all(requests.map(async (request) => ({ ...(await this.#item(request)), checkedOff: checkedOff.has(request.url) }))); - return { hosts: listed.map(({ requests: _, ...host }) => host), items }; + const items = await Promise.all( + listed.flatMap((host) => host.requests).map(async (request) => ({ ...(await this.#item(request)), checkedOff: checkedOff.assigned.has(request.url) })), + ); + const authored = await Promise.all( + listed.flatMap((host) => host.authored).map(async (mine) => ({ ...(await this.#authoredItem(mine)), checkedOff: checkedOff.authored.has(mine.url) })), + ); + return { hosts: listed.map(({ requests: _, authored: __, ...host }) => host), items, authored }; } /** What this plugin has of the review `reviewId` names: null when it was never started here. */ @@ -67,26 +71,38 @@ export class ReviewInbox { } async #item(request: ReviewRequest): Promise> { - const reviewId = idOf(request); - const local = reviewId === null ? null : await this.local(reviewId); - const record = reviewId === null ? null : await this.#store.get(reviewId); + const { reviewId, local, record } = await this.#joined(request); if (record === null) return { ...request, reviewId, local }; - return { ...request, ...(await this.#fromReview(request, record)), reviewId, local }; - } - - /** What the review started here knows better than the forge's listing. */ - async #fromReview(request: ReviewRequest, record: ReviewRecord): Promise> { - const paragraphs = (await this.#store.getDrafts(record.id)).paragraphs.length; - if (request.forge === "github") { - return { pendingDrafts: request.pendingDrafts === null ? null : request.pendingDrafts + paragraphs, changedSinceReview: request.changedSinceReview }; - } - const reviewed = request.state !== "requested" && record.submittedHeadSha !== undefined; + const reviewed = request.forge === "gitlab" && request.state !== "requested" && record.submittedHeadSha !== undefined; return { - pendingDrafts: await this.#countDrafts(record.ref), + ...request, + pendingDrafts: await this.#pendingDrafts(request, record), changedSinceReview: reviewed ? record.submittedHeadSha !== request.headSha : request.changedSinceReview, + reviewId, + local, }; } + async #authoredItem(mine: AuthoredChangeRequest): Promise> { + const { reviewId, local, record } = await this.#joined(mine); + if (record === null) return { ...mine, reviewId, local }; + return { ...mine, pendingDrafts: await this.#pendingDrafts(mine, record), reviewId, local }; + } + + /** The review started here of a listed change request, which is one review whichever tab lists it. */ + async #joined(listed: ListedChangeRequest): Promise<{ reviewId: string | null; local: LocalReview | null; record: ReviewRecord | null }> { + const reviewId = idOf(listed); + if (reviewId === null) return { reviewId, local: null, record: null }; + return { reviewId, local: await this.local(reviewId), record: await this.#store.get(reviewId) }; + } + + /** The viewer's unpublished comments, counting the ones kept here, which the forge's listing cannot see. */ + async #pendingDrafts(listed: ListedChangeRequest, record: ReviewRecord): Promise { + if (listed.forge === "gitlab") return this.#countDrafts(record.ref); + const paragraphs = (await this.#store.getDrafts(record.id)).paragraphs.length; + return listed.pendingDrafts === null ? null : listed.pendingDrafts + paragraphs; + } + async #countDrafts(ref: ChangeRequestRef): Promise { const forge = this.#forges.find((candidate) => candidate.kind === ref.forge); try { @@ -99,7 +115,7 @@ export class ReviewInbox { } -function idOf({ forge, host, project, number, url }: ReviewRequest): string | null { +function idOf({ forge, host, project, number, url }: ListedChangeRequest): string | null { try { return reviewIdOf({ forge, host, project, number, url }); } catch { diff --git a/plugins/guided-review/shared/inbox.ts b/plugins/guided-review/shared/inbox.ts index 9bac384..317ad40 100644 --- a/plugins/guided-review/shared/inbox.ts +++ b/plugins/guided-review/shared/inbox.ts @@ -16,8 +16,8 @@ export const ReviewStatusSchema = z.enum(["unreviewed", "waiting", "commented", /** The head's CI as one word; null when it has none, or one in a state neither of these is, like a cancelled pipeline. */ export const CiStateSchema = z.enum(["success", "failure", "pending"]); -/** An open change request the reviewer is one of the reviewers of, as a forge lists it. */ -export const ReviewRequestSchema = z.object({ +/** An open change request as a forge lists it, whichever tab of the review list it is on. */ +export const ListedChangeRequestSchema = z.object({ forge: z.enum(["github", "gitlab"]), /** The web host, lower-cased: `github.com`. */ host: z.string(), @@ -35,6 +35,12 @@ export const ReviewRequestSchema = z.object({ deletions: z.number().int(), fileCount: z.number().int(), ci: CiStateSchema.nullable(), + /** The viewer's unpublished comments on it; null where the forge cannot count them cheaply. */ + pendingDrafts: z.number().int().nullable(), +}); + +/** An open change request the reviewer is one of the reviewers of, as a forge lists it. */ +export const ReviewRequestSchema = ListedChangeRequestSchema.extend({ state: ReviewerStateSchema, /** The team the review was asked of, `org/team`, when it was not asked of the reviewer by name. */ viaTeam: z.string().nullable(), @@ -43,23 +49,32 @@ export const ReviewRequestSchema = z.object({ * again. Null when they never reviewed it, or the forge cannot say. */ changedSinceReview: z.boolean().nullable(), - /** The reviewer's unpublished comments on it; null where the forge cannot count them cheaply. */ - pendingDrafts: z.number().int().nullable(), }); +/** An open change request the viewer authored, as a forge lists it; `author` is always the viewer. */ +export const AuthoredChangeRequestSchema = ListedChangeRequestSchema; + export type ReviewerState = z.output; export type ReviewStatus = z.output; export type CiState = z.output; export type ReviewRequest = z.output; +export type ListedChangeRequest = z.output; +export type AuthoredChangeRequest = z.output; -/** What one host of a forge listed, or why it could not, so one host failing leaves the others listed. */ +/** + * What one host of a forge listed, or why it could not, so one host failing leaves the others listed. + * Both lists come from one listing, so a host fails for both at once and says so once. + */ export const ListedHostSchema = z.object({ forge: z.enum(["github", "gitlab"]), host: z.string(), + /** What the viewer is asked to review: the Assigned tab. */ requests: z.array(ReviewRequestSchema), - /** The host has more than were listed. */ - truncated: z.boolean(), - /** Why the host could not be listed, as a sentence; its `requests` are then empty. */ + /** What the viewer authored: the Authored tab. */ + authored: z.array(AuthoredChangeRequestSchema), + /** Which of the host's lists has more than was listed, by the tab it is on. */ + truncated: z.object({ assigned: z.boolean(), authored: z.boolean() }), + /** Why the host could not be listed, as a sentence; both its lists are then empty. */ error: z.string().nullable(), }); @@ -79,13 +94,17 @@ export const LocalReviewSchema = z.object({ workspaceId: z.string().nullable(), }); -export const InboxItemSchema = ReviewRequestSchema.extend({ - /** The ID a review of it is kept under; null when it cannot be kept. */ +/** What the review list joins to a listed change request, on either tab. */ +const JOINED = { + /** The ID a review of it is kept under, the same on both tabs; null when it cannot be kept. */ reviewId: z.string().nullable(), local: LocalReviewSchema.nullable(), - /** The reviewer set it aside on this list, the forge none the wiser, until it moves on (`server/inbox-check-offs.ts`). */ + /** The viewer set it aside on this tab, the forge none the wiser, until it moves on (`server/inbox-check-offs.ts`). */ checkedOff: z.boolean(), -}); +}; + +export const InboxItemSchema = ReviewRequestSchema.extend(JOINED); +export const AuthoredInboxItemSchema = AuthoredChangeRequestSchema.extend(JOINED); /** What the reviewer saw of a change request when they checked it off. */ export const CheckOffSchema = z.object({ headSha: z.string(), state: ReviewerStateSchema }); @@ -93,19 +112,23 @@ export const CheckOffSchema = z.object({ headSha: z.string(), state: ReviewerSta /** What the author saw of their change request when they checked it off: where it stood with its reviewers, and when the newest review arrived. */ export const AuthoredCheckOffSchema = z.object({ status: ReviewStatusSchema, latestReviewAt: z.string().nullable() }); -/** Every host's review requests, each joined with the review started here, if any. */ +/** Every host's review requests and the viewer's own change requests, each joined with the review started here, if any. */ export const InboxSchema = z.object({ - hosts: z.array(ListedHostSchema.omit({ requests: true })), + hosts: z.array(ListedHostSchema.omit({ requests: true, authored: true })), + /** The Assigned tab. */ items: z.array(InboxItemSchema), + /** The Authored tab. */ + authored: z.array(AuthoredInboxItemSchema), }); export type LocalReview = z.output; export type CheckOff = z.output; export type AuthoredCheckOff = z.output; export type InboxItem = z.output; +export type AuthoredInboxItem = z.output; export type Inbox = z.output; /** Whether the forge's head moved past the one the review here was read at. */ -export function headMoved(item: ReviewRequest, local: LocalReview): boolean { +export function headMoved(item: Pick, local: LocalReview): boolean { return local.header !== null && item.headSha !== "" && item.headSha !== local.header.headSha; } From c7f9c672521113cec81f8105bf2d51923dd47f5a Mon Sep 17 00:00:00 2001 From: Sleeyax Date: Mon, 12 Oct 2026 14:03:52 +0200 Subject: [PATCH 04/16] feat(guided-review): show the viewer's own change requests on an Authored by me tab MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The start tab's review section switches between `Assigned to me · n` and `Authored by me · n`, opening on Assigned. A tab's total is what it lists before any filter, with a `+` when a host listed only part of it, and no number until the list is in. Authored rows start, open and continue a review as Assigned rows do, with the guide's state, a moved head and the drafts in the Here column. Each tab keeps its own forge filter, Hide drafts, sort, columns and search; New review and Refresh serve both from the section header. The search on Authored looks in the title, project and number, since the author is always the viewer. Authored offers no States, Needs my attention, check-off filter or the state, reviewers and check-off columns until it lists review statuses and check-offs. A host's truncation note shows on the tab it cut short, and its error on both. The sidebar count still counts only Assigned. Refs #146 Co-Authored-By: Claude Opus 5.5 --- plugins/guided-review/README.md | 17 +- plugins/guided-review/client/check-off.ts | 3 +- .../guided-review/client/inbox-filter.test.ts | 129 +++- plugins/guided-review/client/inbox-filter.ts | 113 +++- plugins/guided-review/client/inbox.tsx | 577 +++++++++++++----- plugins/guided-review/client/start-review.ts | 2 +- .../client/start-sidebar-item.tsx | 2 +- 7 files changed, 645 insertions(+), 198 deletions(-) diff --git a/plugins/guided-review/README.md b/plugins/guided-review/README.md index 273b283..34a23c2 100644 --- a/plugins/guided-review/README.md +++ b/plugins/guided-review/README.md @@ -38,18 +38,23 @@ To guide the branch you already have checked out, choose **Guided Review: guide If your branch is clean and behind the PR/MR, the plugin fast-forwards it and writes the guide in your workspace. Otherwise it leaves your branch alone and uses a new workspace. -### Pick from your review requests +### Pick from your review requests or your own -**Assigned to me for review** lists the open pull requests and merge requests you are a reviewer of, on github.com and on every GitLab host `glab` is logged in to. +The start tab lists open pull requests and merge requests in two tabs. +**Assigned to me** lists the ones you are a reviewer of, on github.com and on every GitLab host `glab` is logged in to. +**Authored by me** lists your own on github.com, drafts included. +Each tab shows how many it lists, with a `+` when a host has more than one page. - Select **Review** to start a review, **Open** to open one, or **Continue** to reopen one whose workspace was closed. -- Filter by forge, by state under **States**, with **Needs my attention** or **Hide drafts**, or search. +- Filter by forge or with **Hide drafts**, or search. + On **Assigned to me**, also filter by state under **States** or with **Needs my attention**. - Select a column heading to sort, and **Columns** to choose what the table shows. -- Tick a row's box to check it off while it waits on someone else. +- Each tab keeps its own filters, sort, columns and search. +- On **Assigned to me**, tick a row's box to check it off while it waits on someone else. It comes back when someone pushes to it, asks you to review again, or GitLab resets your approval. -- Select **Refresh** to list again. +- Select **Refresh** to list both tabs again. -The count beside **Guided Review** in the sidebar is how many reviews need your attention. +The count beside **Guided Review** in the sidebar is how many review requests on **Assigned to me** need your attention. If a review workspace shows no panel, select **Open Guided Review** in the workspace's header. diff --git a/plugins/guided-review/client/check-off.ts b/plugins/guided-review/client/check-off.ts index 51066b2..7b2690e 100644 --- a/plugins/guided-review/client/check-off.ts +++ b/plugins/guided-review/client/check-off.ts @@ -4,6 +4,7 @@ import { createContext, useState } from "react"; import * as contracts from "../shared/contracts.ts"; import { PLUGIN_ID } from "../shared/identity.ts"; import type { Inbox, InboxItem } from "../shared/inbox.ts"; +import type { ListedItem } from "./inbox-filter.ts"; export const INBOX_QUERY_KEY = [PLUGIN_ID, "inbox"]; const MUTATION_KEY = [PLUGIN_ID, "check-off"]; @@ -59,7 +60,7 @@ export function useCheckOff(): CheckOffControl { } /** What a row's checkbox needs from the list, so the column's cell takes no more than every other cell. */ -export const CheckOffContext = createContext<{ toggle(item: InboxItem): void; dimmed(item: InboxItem): boolean }>({ +export const CheckOffContext = createContext<{ toggle(item: InboxItem): void; dimmed(item: ListedItem): boolean }>({ toggle: () => {}, dimmed: () => false, }); diff --git a/plugins/guided-review/client/inbox-filter.test.ts b/plugins/guided-review/client/inbox-filter.test.ts index 005af50..aa12e8e 100644 --- a/plugins/guided-review/client/inbox-filter.test.ts +++ b/plugins/guided-review/client/inbox-filter.test.ts @@ -1,8 +1,20 @@ import assert from "node:assert/strict"; import test from "node:test"; import { DEFAULT_INBOX_PREFERENCES, type AssignedPreferences } from "../shared/inbox-preferences.ts"; -import type { InboxItem } from "../shared/inbox.ts"; -import { age, attentionCount, countLabel, hiddenCheckedOff, matchesSearch, needsAttention, statesLabel, visibleItems } from "./inbox-filter.ts"; +import type { AuthoredInboxItem, Inbox, InboxItem } from "../shared/inbox.ts"; +import { + age, + ASSIGNED_RULES, + attentionCount, + AUTHORED_RULES, + countLabel, + hiddenCheckedOff, + matchesSearch, + needsAttention, + statesLabel, + tabLabel, + visibleItems, +} from "./inbox-filter.ts"; function item(number: number, overrides: Partial = {}): InboxItem { return { @@ -34,7 +46,7 @@ function item(number: number, overrides: Partial = {}): InboxItem { const ALL: AssignedPreferences = { ...DEFAULT_INBOX_PREFERENCES.assigned, states: ["requested", "commented", "changes-requested", "approved", "unapproved"] }; -function numbers(items: InboxItem[]): number[] { +function numbers(items: readonly { number: number }[]): number[] { return items.map((entry) => entry.number); } @@ -45,8 +57,8 @@ test("by default the newest update comes first and what the reviewer approved is item(3, { updatedAt: "2026-09-02T00:00:00Z" }), ]; - assert.deepEqual(numbers(visibleItems(items, DEFAULT_INBOX_PREFERENCES.assigned, "")), [3, 1]); - assert.deepEqual(numbers(visibleItems(items, ALL, "")), [2, 3, 1]); + assert.deepEqual(numbers(visibleItems(ASSIGNED_RULES, items, DEFAULT_INBOX_PREFERENCES.assigned, "")), [3, 1]); + assert.deepEqual(numbers(visibleItems(ASSIGNED_RULES, items, ALL, "")), [2, 3, 1]); }); test("filters by forge and drafts, and sorts by creation or size either way", () => { @@ -56,9 +68,9 @@ test("filters by forge and drafts, and sorts by creation or size either way", () item(3, { createdAt: "2026-08-02T00:00:00Z", additions: 1, deletions: 1 }), ]; - assert.deepEqual(numbers(visibleItems(items, { ...ALL, provider: "gitlab" }, "")), [1]); - assert.deepEqual(numbers(visibleItems(items, { ...ALL, hideDrafts: true, sort: { key: "created", descending: true } }, "")), [3, 1]); - assert.deepEqual(numbers(visibleItems(items, { ...ALL, sort: { key: "size", descending: false } }, "")), [3, 1, 2]); + assert.deepEqual(numbers(visibleItems(ASSIGNED_RULES, items, { ...ALL, provider: "gitlab" }, "")), [1]); + assert.deepEqual(numbers(visibleItems(ASSIGNED_RULES, items, { ...ALL, hideDrafts: true, sort: { key: "created", descending: true } }, "")), [3, 1]); + assert.deepEqual(numbers(visibleItems(ASSIGNED_RULES, items, { ...ALL, sort: { key: "size", descending: false } }, "")), [3, 1, 2]); }); test("shows only the reviewer states chosen, updated since or not", () => { @@ -69,36 +81,36 @@ test("shows only the reviewer states chosen, updated since or not", () => { item(4, { state: "requested", changedSinceReview: true }), ]; - assert.deepEqual(numbers(visibleItems(items, { ...ALL, states: ["requested"] }, "")), [1, 4]); - assert.deepEqual(numbers(visibleItems(items, { ...ALL, states: ["commented", "changes-requested"] }, "")), [2, 3]); + assert.deepEqual(numbers(visibleItems(ASSIGNED_RULES, items, { ...ALL, states: ["requested"] }, "")), [1, 4]); + assert.deepEqual(numbers(visibleItems(ASSIGNED_RULES, items, { ...ALL, states: ["commented", "changes-requested"] }, "")), [2, 3]); }); test("checked-off items are hidden by default, shown among the rest, or shown alone", () => { const items = [item(1), item(2, { checkedOff: true }), item(3, { checkedOff: true, isDraft: true })]; - assert.deepEqual(numbers(visibleItems(items, ALL, "")), [1]); - assert.deepEqual(numbers(visibleItems(items, { ...ALL, checkedOff: "show" }, "")), [1, 2, 3]); - assert.deepEqual(numbers(visibleItems(items, { ...ALL, checkedOff: "only" }, "")), [2, 3]); - assert.deepEqual(numbers(visibleItems(items, { ...ALL, checkedOff: "only", hideDrafts: true }, "")), [2], "with the other filters as well"); + assert.deepEqual(numbers(visibleItems(ASSIGNED_RULES, items, ALL, "")), [1]); + assert.deepEqual(numbers(visibleItems(ASSIGNED_RULES, items, { ...ALL, checkedOff: "show" }, "")), [1, 2, 3]); + assert.deepEqual(numbers(visibleItems(ASSIGNED_RULES, items, { ...ALL, checkedOff: "only" }, "")), [2, 3]); + assert.deepEqual(numbers(visibleItems(ASSIGNED_RULES, items, { ...ALL, checkedOff: "only", hideDrafts: true }, "")), [2], "with the other filters as well"); }); test("a held row stays whatever the check-off filter says, but not past the other filters", () => { const items = [item(1), item(2, { checkedOff: true }), item(3, { checkedOff: true, isDraft: true })]; const held = new Set(items.map((entry) => entry.url)); - assert.deepEqual(numbers(visibleItems(items, ALL, "", held)), [1, 2, 3]); - assert.deepEqual(numbers(visibleItems(items, { ...ALL, checkedOff: "only" }, "", held)), [1, 2, 3]); - assert.deepEqual(numbers(visibleItems(items, { ...ALL, hideDrafts: true }, "", held)), [1, 2]); + assert.deepEqual(numbers(visibleItems(ASSIGNED_RULES, items, ALL, "", held)), [1, 2, 3]); + assert.deepEqual(numbers(visibleItems(ASSIGNED_RULES, items, { ...ALL, checkedOff: "only" }, "", held)), [1, 2, 3]); + assert.deepEqual(numbers(visibleItems(ASSIGNED_RULES, items, { ...ALL, hideDrafts: true }, "", held)), [1, 2]); }); test("the checked-off count is what the check-off filter alone hides", () => { const items = [item(1), item(2, { checkedOff: true }), item(3, { checkedOff: true, isDraft: true }), item(4, { checkedOff: true })]; - assert.equal(hiddenCheckedOff(items, ALL, ""), 3); - assert.equal(hiddenCheckedOff(items, { ...ALL, hideDrafts: true }, ""), 2, "a draft the drafts filter hides anyway"); - assert.equal(hiddenCheckedOff(items, ALL, "", new Set([items[1]!.url])), 2, "a held row is shown"); - assert.equal(hiddenCheckedOff(items, { ...ALL, checkedOff: "show" }, ""), 0); - assert.equal(hiddenCheckedOff(items, { ...ALL, checkedOff: "only" }, ""), 0); + assert.equal(hiddenCheckedOff(ASSIGNED_RULES, items, ALL, ""), 3); + assert.equal(hiddenCheckedOff(ASSIGNED_RULES, items, { ...ALL, hideDrafts: true }, ""), 2, "a draft the drafts filter hides anyway"); + assert.equal(hiddenCheckedOff(ASSIGNED_RULES, items, ALL, "", new Set([items[1]!.url])), 2, "a held row is shown"); + assert.equal(hiddenCheckedOff(ASSIGNED_RULES, items, { ...ALL, checkedOff: "show" }, ""), 0); + assert.equal(hiddenCheckedOff(ASSIGNED_RULES, items, { ...ALL, checkedOff: "only" }, ""), 0); }); test("the states label names whichever side is shorter", () => { @@ -128,15 +140,80 @@ test("the attention count keeps what needs attention among what the default filt item(7, { forge: "gitlab", state: "requested", isDraft: true }), ]; - assert.equal(attentionCount(items), 4); - assert.equal(attentionCount([]), 0); + assert.equal(attentionCount(inboxOf(items)), 4); + assert.equal(attentionCount(inboxOf([])), 0); }); test("the search needs every word somewhere in the title, project, author or number as the forge writes it", () => { const mr = item(12, { forge: "gitlab", project: "example-group/service", title: "Add rate limiting", author: "kobe" }); - for (const query of ["", "rate", "RATE limit", "service !12", "kobe", " limiting "]) assert.equal(matchesSearch(mr, query), true, query); - for (const query of ["#12", "rate github", "uploader"]) assert.equal(matchesSearch(mr, query), false, query); + for (const query of ["", "rate", "RATE limit", "service !12", "kobe", " limiting "]) assert.equal(matchesSearch(ASSIGNED_RULES, mr, query), true, query); + for (const query of ["#12", "rate github", "uploader"]) assert.equal(matchesSearch(ASSIGNED_RULES, mr, query), false, query); +}); + +function mine(number: number, overrides: Partial = {}): AuthoredInboxItem { + const { state: _, viaTeam: __, changedSinceReview: ___, ...listed } = item(number, { author: "me" }); + return { ...listed, ...overrides }; +} + +const AUTHORED_DEFAULTS = DEFAULT_INBOX_PREFERENCES.authored; + +test("Authored lists newest first by default, drafts in their place, and filters by forge and drafts and sorts by its own preferences", () => { + const items = [ + mine(1, { updatedAt: "2026-09-01T00:00:00Z", forge: "gitlab", additions: 1 }), + mine(2, { updatedAt: "2026-09-03T00:00:00Z", isDraft: true, additions: 50 }), + mine(3, { updatedAt: "2026-09-02T00:00:00Z", additions: 5 }), + ]; + + assert.deepEqual(numbers(visibleItems(AUTHORED_RULES, items, AUTHORED_DEFAULTS, "")), [2, 3, 1]); + assert.deepEqual(numbers(visibleItems(AUTHORED_RULES, items, { ...AUTHORED_DEFAULTS, provider: "github" }, "")), [2, 3]); + assert.deepEqual(numbers(visibleItems(AUTHORED_RULES, items, { ...AUTHORED_DEFAULTS, hideDrafts: true }, "")), [3, 1]); + assert.deepEqual(numbers(visibleItems(AUTHORED_RULES, items, { ...AUTHORED_DEFAULTS, sort: { key: "size", descending: false } }, "")), [1, 3, 2]); +}); + +test("Authored offers no states or needs attention to filter by yet, so neither hides anything", () => { + const items = [mine(1), mine(2)]; + + assert.equal(AUTHORED_RULES.state, null); + assert.equal(AUTHORED_RULES.needsAttention, null); + assert.deepEqual(numbers(visibleItems(AUTHORED_RULES, items, { ...AUTHORED_DEFAULTS, states: ["approved"], needsAttention: true }, "")), [1, 2]); +}); + +test("the search on Authored needs every word in the title, project or number, not the viewer's own name", () => { + const mr = mine(12, { forge: "gitlab", project: "example-group/service", title: "Add rate limiting" }); + + for (const query of ["", "rate", "service !12", "LIMITING"]) assert.equal(matchesSearch(AUTHORED_RULES, mr, query), true, query); + for (const query of ["me", "#12", "uploader"]) assert.equal(matchesSearch(AUTHORED_RULES, mr, query), false, query); +}); + +function inboxOf(items: InboxItem[], authored: AuthoredInboxItem[] = [], hosts: Inbox["hosts"] = []): Inbox { + return { hosts, items, authored }; +} + +function host(name: string, truncated: Partial> = {}): Inbox["hosts"][number] { + return { forge: "github", host: name, truncated: { assigned: false, authored: false, ...truncated }, error: null }; +} + +test("a tab's label gives its listed total, with a + when a host listed only part of it, and no number until it is listed", () => { + const inbox = inboxOf([item(1), item(2, { checkedOff: true })], [mine(3)], [host("github.com", { authored: true }), host("gitlab.com")]); + + assert.equal(tabLabel("assigned", undefined), "Assigned to me"); + assert.equal(tabLabel("authored", undefined), "Authored by me"); + assert.equal(tabLabel("assigned", inbox), "Assigned to me · 2"); + assert.equal(tabLabel("authored", inbox), "Authored by me · 1+"); + assert.equal(tabLabel("authored", inboxOf([], [])), "Authored by me · 0"); +}); + +test("the sidebar's attention count leaves the viewer's own change requests out", () => { + const authored = [mine(1, { ci: "failure" }), mine(2)]; + + assert.equal(attentionCount(inboxOf([item(3)], authored)), 1); + assert.equal(attentionCount(inboxOf([], authored)), 0); +}); + +test("Authored counts change requests rather than reviews", () => { + assert.equal(countLabel(1, 1, 0, AUTHORED_RULES.noun), "1 change request"); + assert.equal(countLabel(2, 5, 0, AUTHORED_RULES.noun), "2 of 5 change requests"); }); test("an age is the largest whole unit", () => { diff --git a/plugins/guided-review/client/inbox-filter.ts b/plugins/guided-review/client/inbox-filter.ts index ab75322..e0aa532 100644 --- a/plugins/guided-review/client/inbox-filter.ts +++ b/plugins/guided-review/client/inbox-filter.ts @@ -1,7 +1,35 @@ -import { DEFAULT_INBOX_PREFERENCES, type AssignedPreferences, type InboxSortKey } from "../shared/inbox-preferences.ts"; -import { ReviewerStateSchema, type InboxItem, type ReviewerState } from "../shared/inbox.ts"; +import { DEFAULT_INBOX_PREFERENCES, type AssignedPreferences, type InboxSortKey, type InboxTab } from "../shared/inbox-preferences.ts"; +import { + ReviewerStateSchema, + type AuthoredInboxItem, + type Inbox, + type InboxItem, + type ListedChangeRequest, + type ReviewerState, + type ReviewStatus, +} from "../shared/inbox.ts"; import { numberLabel } from "../shared/reference.ts"; +/** What every row of either tab has to be filtered by. */ +export type ListedItem = ListedChangeRequest & { checkedOff: boolean }; + +/** What a tab's filters read of the preferences; the columns are the table's business. */ +export type ListFilters = Pick & { + states: readonly State[]; +}; + +/** How a tab reads its rows. */ +export type TabRules = { + /** Where a row stands, as the States filter reads it; null while the tab has no states to filter by, so the filter is not offered. */ + state: ((item: Item) => State) | null; + /** Whether a row needs the viewer's attention; null while the tab cannot tell, so the filter is not offered. */ + needsAttention: ((item: Item) => boolean) | null; + /** What the search looks in, besides the number as the forge writes it. */ + searched(item: Item): string[]; + /** What a row is called, one and many. */ + noun: readonly [string, string]; +}; + /** * Whether the reviewer has something to do on it: never reviewed, asked again, an approval the forge * took back, or the change moved on since their review. @@ -10,14 +38,29 @@ export function needsAttention(item: InboxItem): boolean { return item.state === "requested" || item.state === "unapproved" || item.changedSinceReview === true; } -/** Every word of `query` is in the title, the project, the author or the number as the forge writes it. */ -export function matchesSearch(item: InboxItem, query: string): boolean { +export const ASSIGNED_RULES: TabRules = { + state: (item) => item.state, + needsAttention, + searched: (item) => [item.title, item.project, item.author], + noun: ["review", "reviews"], +}; + +/** The author is always the viewer, so the search leaves their name out. */ +export const AUTHORED_RULES: TabRules = { + state: null, + needsAttention: null, + searched: (item) => [item.title, item.project], + noun: ["change request", "change requests"], +}; + +/** Every word of `query` is in what the tab searches or the number as the forge writes it. */ +export function matchesSearch(rules: TabRules, item: Item, query: string): boolean { const words = query.toLowerCase().split(/\s+/).filter((word) => word !== ""); - const text = [item.title, item.project, item.author, numberLabel(item.forge, item.number)].join(" ").toLowerCase(); + const text = [...rules.searched(item), numberLabel(item.forge, item.number)].join(" ").toLowerCase(); return words.every((word) => text.includes(word)); } -const SORT_VALUES: Record number> = { +const SORT_VALUES: Record number> = { updated: (item) => Date.parse(item.updatedAt), created: (item) => Date.parse(item.createdAt), size: (item) => item.additions + item.deletions, @@ -30,29 +73,42 @@ const NONE_HELD: ReadonlySet = new Set(); * The items the list shows, filtered as the preferences say and by the search, in their sort order; ties go to the latest update. * A `held` row is shown whether or not the check-off filter would take it, so a row just checked off or unchecked stays put. */ -export function visibleItems(items: readonly InboxItem[], preferences: AssignedPreferences, query: string, held = NONE_HELD): InboxItem[] { +export function visibleItems( + rules: TabRules, + items: readonly Item[], + preferences: ListFilters, + query: string, + held = NONE_HELD, +): Item[] { const value = SORT_VALUES[preferences.sort.key]; const direction = preferences.sort.descending ? -1 : 1; return items - .filter((item) => matchesFilters(item, preferences, query) && (held.has(item.url) || passesCheckOff(item, preferences))) + .filter((item) => matchesFilters(rules, item, preferences, query) && (held.has(item.url) || passesCheckOff(item, preferences))) .sort((a, b) => direction * (value(a) - value(b)) || SORT_VALUES.updated(b) - SORT_VALUES.updated(a)); } /** How many checked-off items the list leaves out that every other filter and the search would show. */ -export function hiddenCheckedOff(items: readonly InboxItem[], preferences: AssignedPreferences, query: string, held = NONE_HELD): number { - return items.filter((item) => item.checkedOff && !held.has(item.url) && !passesCheckOff(item, preferences) && matchesFilters(item, preferences, query)).length; +export function hiddenCheckedOff( + rules: TabRules, + items: readonly Item[], + preferences: ListFilters, + query: string, + held = NONE_HELD, +): number { + return items.filter((item) => item.checkedOff && !held.has(item.url) && !passesCheckOff(item, preferences) && matchesFilters(rules, item, preferences, query)) + .length; } -/** The list's default filters with only what needs the reviewer's attention kept, whatever the reviewer last set. */ +/** The Assigned tab's default filters with only what needs the reviewer's attention kept, whatever the reviewer last set. */ const ATTENTION_FILTERS: AssignedPreferences = { ...DEFAULT_INBOX_PREFERENCES.assigned, needsAttention: true }; -/** How many items need the reviewer's attention: the sidebar's count. */ -export function attentionCount(items: readonly InboxItem[]): number { - return items.filter((item) => matchesFilters(item, ATTENTION_FILTERS, "") && passesCheckOff(item, ATTENTION_FILTERS)).length; +/** How many review requests need the reviewer's attention: the sidebar's count, which leaves their own change requests out. */ +export function attentionCount(inbox: Inbox): number { + return inbox.items.filter((item) => matchesFilters(ASSIGNED_RULES, item, ATTENTION_FILTERS, "") && passesCheckOff(item, ATTENTION_FILTERS)).length; } /** Whether the check-off filter shows the item, which a held row is not asked. */ -export function passesCheckOff(item: InboxItem, preferences: AssignedPreferences): boolean { +export function passesCheckOff(item: ListedItem, preferences: Pick, "checkedOff">): boolean { switch (preferences.checkedOff) { case "hide": return !item.checkedOff; @@ -63,16 +119,29 @@ export function passesCheckOff(item: InboxItem, preferences: AssignedPreferences } } -function matchesFilters(item: InboxItem, preferences: AssignedPreferences, query: string): boolean { +function matchesFilters(rules: TabRules, item: Item, preferences: ListFilters, query: string): boolean { return ( (preferences.provider === "all" || item.forge === preferences.provider) && - preferences.states.includes(item.state) && + (rules.state === null || preferences.states.includes(rules.state(item))) && (!preferences.hideDrafts || !item.isDraft) && - (!preferences.needsAttention || needsAttention(item)) && - matchesSearch(item, query) + (!preferences.needsAttention || rules.needsAttention === null || rules.needsAttention(item)) && + matchesSearch(rules, item, query) ); } +const TAB_TITLES: Record = { assigned: "Assigned to me", authored: "Authored by me" }; + +function listedOn(inbox: Inbox, tab: InboxTab): readonly ListedItem[] { + return tab === "assigned" ? inbox.items : inbox.authored; +} + +/** A tab's name with how many it lists, a `+` when a host listed only part of it, and no number until it is listed. */ +export function tabLabel(tab: InboxTab, inbox: Inbox | undefined): string { + if (inbox === undefined) return TAB_TITLES[tab]; + const partial = inbox.hosts.some((host) => host.truncated[tab]); + return `${TAB_TITLES[tab]} · ${listedOn(inbox, tab).length}${partial ? "+" : ""}`; +} + /** Each reviewer state as a sentence names it mid-way. */ export const STATE_NAMES: Record = { requested: "requested", @@ -98,9 +167,9 @@ function namesOf(states: readonly ReviewerState[]): string { return names.length === 1 ? names[0] : `${names.slice(0, -1).join(", ")} and ${names.at(-1)}`; } -/** How many reviews the list shows, of how many when the filters or the search hide some, and how many of those are checked off. */ -export function countLabel(shown: number, total: number, checkedOff = 0): string { - const noun = total === 1 ? "review" : "reviews"; +/** How many rows the list shows, of how many when the filters or the search hide some, and how many of those are checked off. */ +export function countLabel(shown: number, total: number, checkedOff = 0, [one, many] = ASSIGNED_RULES.noun): string { + const noun = total === 1 ? one : many; const count = shown === total ? `${total} ${noun}` : `${shown} of ${total} ${noun}`; return checkedOff > 0 ? `${count} · ${checkedOff} checked off` : count; } diff --git a/plugins/guided-review/client/inbox.tsx b/plugins/guided-review/client/inbox.tsx index cef6c02..41a0be0 100644 --- a/plugins/guided-review/client/inbox.tsx +++ b/plugins/guided-review/client/inbox.tsx @@ -1,16 +1,49 @@ import { useRpc } from "@getpaseo/plugin/client"; import { Modal, TextInput } from "@getpaseo/plugin/client/react-native"; import { ExternalLink, SettingsAction, SettingsCard, SettingsSection, SettingsSwitch } from "@getpaseo/plugin/client/ui"; -import { useQuery } from "@tanstack/react-query"; +import { useQuery, type UseQueryResult } from "@tanstack/react-query"; import React, { useContext, useState } from "react"; import { Pressable, Text, View } from "react-native"; import * as contracts from "../shared/contracts.ts"; -import { ASSIGNED_COLUMNS, CHECK_OFF_FILTERS, type AssignedColumn, type AssignedPreferences, type CheckOffFilter, type InboxSortKey } from "../shared/inbox-preferences.ts"; -import { headMoved, ReviewerStateSchema, type Inbox, type InboxItem, type LocalReview, type ReviewerState } from "../shared/inbox.ts"; +import { + ASSIGNED_COLUMNS, + AUTHORED_COLUMNS, + CHECK_OFF_FILTERS, + INBOX_TABS, + type AssignedColumn, + type AuthoredColumn, + type CheckOffFilter, + type InboxSortKey, + type InboxTab, +} from "../shared/inbox-preferences.ts"; +import { + headMoved, + ReviewerStateSchema, + type AuthoredInboxItem, + type Inbox, + type InboxItem, + type LocalReview, + type ReviewerState, + type ReviewStatus, +} from "../shared/inbox.ts"; import { numberLabel } from "../shared/reference.ts"; import { Button } from "./button.tsx"; -import { CheckOffContext, INBOX_QUERY_KEY, useCheckOff } from "./check-off.ts"; -import { age, countLabel, hiddenCheckedOff, passesCheckOff, STATE_NAMES, statesLabel, visibleItems } from "./inbox-filter.ts"; +import { CheckOffContext, INBOX_QUERY_KEY, useCheckOff, type CheckOffControl } from "./check-off.ts"; +import { + age, + ASSIGNED_RULES, + AUTHORED_RULES, + countLabel, + hiddenCheckedOff, + passesCheckOff, + STATE_NAMES, + statesLabel, + tabLabel, + visibleItems, + type ListedItem, + type ListFilters, + type TabRules, +} from "./inbox-filter.ts"; import { useInboxPreferences } from "./inbox-preferences.ts"; import { NewReview } from "./new-review.tsx"; import { describePreparing, describeProgress } from "./start-progress.ts"; @@ -22,73 +55,208 @@ const FORGE_NAMES = { github: "GitHub", gitlab: "GitLab" } as const; const CHECK_OFF_LABELS: Record = { hide: "Hide checked off", show: "With checked off", only: "Checked off only" }; -/** The open change requests the reviewer reviews, to pick one and start or continue its review. */ +/** A row of either tab, with the review started here if any. */ +type Row = ListedItem & { reviewId: string | null; local: LocalReview | null }; + +/** One tab's filters, sort and columns. */ +type TabPreferences = ListFilters & { columns: Column[] }; + +/** The States chooser of a tab that has states to filter by. */ +type StatesSetup = { + all: readonly State[]; + label(states: readonly State[]): string; + name(state: State): string; + hint(state: State): string | undefined; +}; + +/** What a tab shows and offers, beside how its filters read a row. */ +type TabView = { + tab: InboxTab; + rules: TabRules; + /** Every column the tab can have, in the order it shows them. */ + columns: readonly Column[]; + /** The columns the tab can fill; one it cannot is neither offered nor shown, whatever the preferences say. */ + specs: Partial>>; + /** Null while the tab has no states to filter by. */ + states: StatesSetup | null; + searchHint: string; + listing: string; + failure: string; + empty: string; +}; + +/** The change requests the viewer reviews and the ones they authored, as two tabs, to pick one and start or continue its review. */ export function ReviewInbox({ colors, compact, openPanel }: { colors: Colors; compact: boolean; openPanel: (workspaceId: string) => void }) { const getInbox = useRpc(contracts.getInbox); const inbox = useQuery({ queryKey: INBOX_QUERY_KEY, queryFn: () => getInbox({}), refetchOnWindowFocus: false }); - const starts = useReviewStarts(openPanel, inbox.data?.items ?? []); - const { preferences, change: changePreferences, error } = useInboxPreferences("assigned"); + const starts = useReviewStarts(openPanel, [...(inbox.data?.items ?? []), ...(inbox.data?.authored ?? [])]); + const assigned = useInboxPreferences("assigned"); + const authored = useInboxPreferences("authored"); const checkOff = useCheckOff(); - const [query, setSearch] = useState(""); + const [tab, setTab] = useState("assigned"); + const [queries, setQueries] = useState>({ assigned: "", authored: "" }); const [width, setWidth] = useState(null); - const [choosing, setChoosing] = useState<"columns" | "states" | null>(null); - const change = (update: Partial) => { - checkOff.release(); - changePreferences(update); - }; const setQuery = (next: string) => { checkOff.release(); - setSearch(next); + setQueries((current) => ({ ...current, [tab]: next })); }; const refresh = () => { checkOff.release(); starts.forget(); void inbox.refetch(); }; - const items = (inbox.data ? visibleItems(inbox.data.items, preferences, query, checkOff.held) : []).map((item) => ({ - ...item, - local: starts.local(item.reviewId, item.local), - })); - // Until the list has been measured it is not drawn, rather than as cards for a frame. - const shape = compact ? "cards" : width === null ? null : width >= tableWidth(preferences.columns) ? "table" : "cards"; - const rows = { toggle: checkOff.toggle, dimmed: (item: InboxItem) => checkOff.held.has(item.url) && !passesCheckOff(item, preferences) }; + const shared = { + colors, + compact, + width, + inbox, + starts, + refresh, + query: queries[tab], + setQuery, + tabs: , + }; return ( + {tab === "assigned" ? ( + + ) : ( + + )} + + ); +} + +/** The two tabs as one switch, each with how many it lists. */ +function TabSwitch({ colors, tab, setTab, inbox }: { colors: Colors; tab: InboxTab; setTab: (tab: InboxTab) => void; inbox: Inbox | undefined }) { + return ( + + {INBOX_TABS.map((candidate) => { + const selected = candidate === tab; + return ( + setTab(candidate)} + accessibilityRole="tab" + accessibilityState={{ selected }} + style={({ pressed }) => ({ + paddingVertical: spacing[1], + paddingHorizontal: spacing[3], + backgroundColor: selected ? colors.accent : undefined, + opacity: pressed ? 0.85 : 1, + })} + > + + {tabLabel(candidate, inbox)} + + + ); + })} + + ); +} + +/** The section as one tab shows it; a tab without a check-off control has no check-offs to show or set. */ +function TabSection({ + colors, + compact, + width, + inbox, + starts, + refresh, + query, + setQuery, + tabs, + view, + listed = [], + preferences: control, + checkOff, +}: { + colors: Colors; + compact: boolean; + width: number | null; + inbox: UseQueryResult; + starts: ReviewStarts; + refresh: () => void; + query: string; + setQuery: (query: string) => void; + tabs: React.ReactNode; + view: TabView; + listed: readonly Item[] | undefined; + preferences: { preferences: TabPreferences; change(update: Partial>): void; error: string | null }; + checkOff: CheckOffControl | null; +}) { + const { preferences, error } = control; + const [choosing, setChoosing] = useState<"columns" | "states" | null>(null); + const held = checkOff?.held ?? new Set(); + + const change = (update: Partial>) => { + checkOff?.release(); + control.change(update); + }; + const items = visibleItems(view.rules, listed, preferences, query, held).map((item) => ({ ...item, local: starts.local(item.reviewId, item.local) })); + const columns = preferences.columns.filter((column) => view.specs[column] !== undefined); + // Until the list has been measured it is not drawn, rather than as cards for a frame. + const shape = compact ? "cards" : width === null ? null : width >= tableWidth(view.specs, columns) ? "table" : "cards"; + const rows = { toggle: checkOff?.toggle ?? (() => {}), dimmed: (item: ListedItem) => held.has(item.url) && !passesCheckOff(item, preferences) }; + + return ( + <>