diff --git a/plugins/guided-review/README.md b/plugins/guided-review/README.md index 273b283..7eaa244 100644 --- a/plugins/guided-review/README.md +++ b/plugins/guided-review/README.md @@ -38,18 +38,29 @@ 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 the same hosts, drafts included. +Its review status combines your reviewers' latest verdicts, and changes requested by any reviewer outweigh approvals. +An approval GitLab removes, or GitHub dismisses, when you push shows as an approval reset. +On GitLab, an approval its reviewer revokes shows as an approval reset too. +A merge request you wrote and review is on both tabs, with the one review you started from either. - 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, by state under **States**, with **Needs my attention** or with **Hide drafts**, or search. + On **Authored by me**, the search also matches reviewers. + On **Authored by me**, **Needs my attention** keeps changes requested, a review since your last push, and failing CI. - Select a column heading to sort, and **Columns** to choose what the table shows. +- Each tab keeps its own filters, sort, columns and search. - 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. + On **Assigned to me**, it comes back when someone pushes to it, asks you to review again, or GitLab resets your approval. + On **Authored by me**, it comes back when a reviewer reviews again or its review status changes, an approval reset included, but not when you push. + A check-off on one tab leaves the other tab alone. +- 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..79dc1d3 100644 --- a/plugins/guided-review/client/check-off.ts +++ b/plugins/guided-review/client/check-off.ts @@ -3,40 +3,54 @@ import { useIsMutating, useMutation, useQueryClient } from "@tanstack/react-quer 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 { AuthoredInboxItem, CheckOffChange, Inbox, InboxItem } from "../shared/inbox.ts"; +import { withCheckedOff, type ListedItem } from "./inbox-filter.ts"; export const INBOX_QUERY_KEY = [PLUGIN_ID, "inbox"]; const MUTATION_KEY = [PLUGIN_ID, "check-off"]; -export type CheckOffControl = { - toggle(item: InboxItem): void; +/** What a tab's check-off records of its row, so the server can tell when it came back. */ +export type CheckOffRule = (item: Item, checkedOff: boolean) => CheckOffChange; + +export const ASSIGNED_CHECK_OFF: CheckOffRule = (item, checkedOff) => ({ + tab: "assigned", + url: item.url, + checkOff: checkedOff ? { headSha: item.headSha, state: item.state } : null, +}); + +export const AUTHORED_CHECK_OFF: CheckOffRule = (item, checkedOff) => ({ + tab: "authored", + url: item.url, + checkOff: checkedOff ? { status: item.status, latestReviewAt: item.latestReviewAt } : null, +}); + +export type CheckOffControl = { + toggle(item: Item): void; /** The rows checked off or unchecked since the list was last refreshed, filtered or searched, which stay where they are until then. */ held: ReadonlySet; release(): void; - /** A check-off is being saved, so a refresh now could read the list from before it. */ + /** A check-off on either tab is being saved, so a refresh now could read the list from before it. */ saving: boolean; /** Why the latest check-off could not be kept, as a sentence. */ error: string | null; }; /** - * Checking reviews off the list and unchecking them. + * Checking change requests off one tab of the list and unchecking them. * A toggle shows at once in the listed inbox and is saved behind it, and put back if the save fails. */ -export function useCheckOff(): CheckOffControl { +export function useCheckOff(rule: CheckOffRule): CheckOffControl { const setCheckedOff = useRpc(contracts.setCheckedOff); const queryClient = useQueryClient(); const [held, setHeld] = useState>(() => new Set()); const [error, setError] = useState(null); - const mark = (url: string, checkedOff: boolean) => - queryClient.setQueryData(INBOX_QUERY_KEY, (inbox) => - inbox && { ...inbox, items: inbox.items.map((item) => (item.url === url ? { ...item, checkedOff } : item)) }, - ); + const mark = ({ tab, url }: CheckOffChange, checkedOff: boolean) => + queryClient.setQueryData(INBOX_QUERY_KEY, (inbox) => inbox && withCheckedOff(inbox, tab, url, checkedOff)); const save = useMutation({ mutationKey: MUTATION_KEY, mutationFn: setCheckedOff, - onError: (failure, { url, checkOff }) => { - mark(url, checkOff === null); + onError: (failure, change) => { + mark(change, change.checkOff === null); setError(failure instanceof Error ? failure.message : String(failure)); }, }); @@ -45,11 +59,12 @@ export function useCheckOff(): CheckOffControl { return { toggle(item) { const checkedOff = !item.checkedOff; + const change = rule(item, checkedOff); void queryClient.cancelQueries({ queryKey: INBOX_QUERY_KEY }); - mark(item.url, checkedOff); + mark(change, checkedOff); setHeld((rows) => new Set(rows).add(item.url)); setError(null); - save.mutate({ url: item.url, checkOff: checkedOff ? { headSha: item.headSha, state: item.state } : null }); + save.mutate(change); }, held, release: () => setHeld(new Set()), @@ -59,7 +74,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: ListedItem): 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 ab5c788..25c618c 100644 --- a/plugins/guided-review/client/inbox-filter.test.ts +++ b/plugins/guided-review/client/inbox-filter.test.ts @@ -1,8 +1,22 @@ import assert from "node:assert/strict"; import test from "node:test"; -import { DEFAULT_INBOX_PREFERENCES, type InboxPreferences } 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 { DEFAULT_INBOX_PREFERENCES, type AssignedPreferences } from "../shared/inbox-preferences.ts"; +import { ReviewStatusSchema, type AuthoredInboxItem, type Inbox, type InboxItem } from "../shared/inbox.ts"; +import { + age, + ASSIGNED_RULES, + attentionCount, + AUTHORED_RULES, + countLabel, + hiddenCheckedOff, + matchesSearch, + needsAttention, + statesLabel, + statusesLabel, + tabLabel, + visibleItems, + withCheckedOff, +} from "./inbox-filter.ts"; function item(number: number, overrides: Partial = {}): InboxItem { return { @@ -32,9 +46,9 @@ 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[] { +function numbers(items: readonly { number: number }[]): number[] { return items.map((entry) => entry.number); } @@ -45,8 +59,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, "")), [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 +70,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 +83,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", () => { @@ -109,6 +123,12 @@ test("the states label names whichever side is shorter", () => { assert.equal(statesLabel(["requested", "commented", "changes-requested"]), "States: all but approved and approval reset"); }); +test("Authored's states label names its review statuses the same way", () => { + assert.equal(statusesLabel(AUTHORED_DEFAULTS.states), "States: all but approved"); + assert.equal(statusesLabel(["waiting", "approval-reset"]), "States: waiting and approval reset"); + assert.equal(statusesLabel([...ReviewStatusSchema.options]), "States: all"); +}); + test("needs attention is what was never reviewed, asked again, unapproved or changed since the review", () => { assert.equal(needsAttention(item(1)), true); assert.equal(needsAttention(item(1, { state: "unapproved" })), true); @@ -128,15 +148,131 @@ 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, status: "waiting", reviewers: [], reviewedSinceLastPush: false, latestReviewAt: null, ...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 filters by review status, by default everything but what is approved", () => { + const items = [ + mine(1, { status: "unreviewed" }), + mine(2, { status: "waiting" }), + mine(3, { status: "commented" }), + mine(4, { status: "changes-requested" }), + mine(5, { status: "approved" }), + mine(6, { status: "approval-reset" }), + ]; + + assert.deepEqual(numbers(visibleItems(AUTHORED_RULES, items, { ...AUTHORED_DEFAULTS, sort: { key: "created", descending: false } }, "")), [1, 2, 3, 4, 6]); + assert.deepEqual(numbers(visibleItems(AUTHORED_RULES, items, { ...AUTHORED_DEFAULTS, states: ["approved", "approval-reset"] }, "")), [5, 6]); +}); + +test("an authored change request needs attention on a change request, a review since the last push, or failing CI, and not on an approval reset", () => { + const items = [ + mine(1, { status: "changes-requested" }), + mine(2, { status: "commented", reviewedSinceLastPush: true }), + mine(3, { status: "waiting", ci: "failure" }), + mine(4, { status: "approval-reset" }), + mine(5, { status: "commented", reviewedSinceLastPush: null, ci: "pending" }), + mine(6, { status: "approved", reviewedSinceLastPush: false, ci: "success" }), + ]; + const attention = { ...AUTHORED_DEFAULTS, states: [...ReviewStatusSchema.options], needsAttention: true, sort: { key: "created" as const, descending: false } }; + + assert.deepEqual(numbers(visibleItems(AUTHORED_RULES, items, attention, "")), [1, 2, 3]); +}); + +test("Authored hides its checked-off change requests by default, shows them among the rest, or shows them alone", () => { + const items = [mine(1), mine(2, { checkedOff: true }), mine(3, { checkedOff: true, isDraft: true })]; + const any = { ...AUTHORED_DEFAULTS, sort: { key: "created" as const, descending: false } }; + + assert.deepEqual(numbers(visibleItems(AUTHORED_RULES, items, any, "")), [1]); + assert.deepEqual(numbers(visibleItems(AUTHORED_RULES, items, { ...any, checkedOff: "show" }, "")), [1, 2, 3]); + assert.deepEqual(numbers(visibleItems(AUTHORED_RULES, items, { ...any, checkedOff: "only" }, "")), [2, 3]); + assert.deepEqual(numbers(visibleItems(AUTHORED_RULES, items, { ...any, checkedOff: "only" }, "", new Set([mine(1).url]))), [1, 2, 3], "a held row stays"); + assert.equal(hiddenCheckedOff(AUTHORED_RULES, items, any, ""), 2); +}); + +test("checking a change request off marks it on that tab only, though the same one is on the other", () => { + const url = "https://gitlab.com/acme/uploader/-/merge_requests/7"; + const both = inboxOf([item(7, { url }), item(8)], [mine(7, { url })]); + + const assigned = withCheckedOff(both, "assigned", url, true); + assert.deepEqual([assigned.assigned.map((entry) => entry.checkedOff), assigned.authored.map((entry) => entry.checkedOff)], [[true, false], [false]]); + const authored = withCheckedOff(assigned, "authored", url, true); + assert.deepEqual([authored.assigned.map((entry) => entry.checkedOff), authored.authored.map((entry) => entry.checkedOff)], [[true, false], [true]]); + const unchecked = withCheckedOff(authored, "assigned", url, false); + assert.deepEqual([unchecked.assigned.map((entry) => entry.checkedOff), unchecked.authored.map((entry) => entry.checkedOff)], [[false, false], [true]]); +}); + +test("the search on Authored needs every word in the title, project, reviewers or number, not the viewer's own name", () => { + const mr = mine(12, { + forge: "gitlab", + project: "example-group/service", + title: "Add rate limiting", + reviewers: [ + { name: "kobe", state: "approved" }, + { name: "example-org/backend", state: "requested" }, + ], + }); + + for (const query of ["", "rate", "service !12", "LIMITING", "kobe", "backend rate"]) 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(assigned: InboxItem[], authored: AuthoredInboxItem[] = [], hosts: Inbox["hosts"] = []): Inbox { + return { hosts, assigned, 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 894b6d5..50b6258 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 InboxPreferences, 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, + ReviewStatusSchema, + 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. */ + state(item: Item): State; + needsAttention(item: Item): boolean; + /** 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,37 @@ 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"], +}; + +/** + * Whether the author has something to do on their change request: changes asked for, a review since their last push, or failing CI. + * An approval reset waits on the reviewer, not the author. + */ +export function authoredNeedsAttention(item: AuthoredInboxItem): boolean { + return item.status === "changes-requested" || item.reviewedSinceLastPush === true || item.ci === "failure"; +} + +/** The author is always the viewer, so the search leaves their name out. */ +export const AUTHORED_RULES: TabRules = { + state: (item) => item.status, + needsAttention: authoredNeedsAttention, + searched: (item) => [item.title, item.project, ...item.reviewers.map((reviewer) => reviewer.name)], + 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 +81,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: InboxPreferences, 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: InboxPreferences, 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. */ -const ATTENTION_FILTERS: InboxPreferences = { ...DEFAULT_INBOX_PREFERENCES, needsAttention: true }; +/** 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.assigned.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: InboxPreferences): boolean { +export function passesCheckOff(item: ListedItem, preferences: Pick, "checkedOff">): boolean { switch (preferences.checkedOff) { case "hide": return !item.checkedOff; @@ -63,16 +127,30 @@ export function passesCheckOff(item: InboxItem, preferences: InboxPreferences): } } -function matchesFilters(item: InboxItem, preferences: InboxPreferences, 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) && + preferences.states.includes(rules.state(item)) && (!preferences.hideDrafts || !item.isDraft) && - (!preferences.needsAttention || needsAttention(item)) && - matchesSearch(item, query) + (!preferences.needsAttention || rules.needsAttention(item)) && + matchesSearch(rules, item, query) ); } +const TAB_TITLES: Record = { assigned: "Assigned to me", authored: "Authored by me" }; + +/** 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]} · ${inbox[tab].length}${partial ? "+" : ""}`; +} + +/** The inbox with the change request at `url` checked off or not on `tab`, leaving the same one on the other tab as it was. */ +export function withCheckedOff(inbox: Inbox, tab: InboxTab, url: string, checkedOff: boolean): Inbox { + return { ...inbox, [tab]: inbox[tab].map((item) => (item.url === url ? { ...item, checkedOff } : item)) }; +} + /** Each reviewer state as a sentence names it mid-way. */ export const STATE_NAMES: Record = { requested: "requested", @@ -82,25 +160,40 @@ export const STATE_NAMES: Record = { unapproved: "approval reset", }; -/** - * Which states the list shows, naming whichever side is shorter: `States: requested and commented`, - * or `States: all but approved`. - */ +/** Each review status of an authored change request as a sentence names it mid-way. */ +export const STATUS_NAMES: Record = { + unreviewed: "unreviewed", + waiting: "waiting", + commented: "commented", + "changes-requested": "changes requested", + approved: "approved", + "approval-reset": "approval reset", +}; + +/** Which states the list shows, naming whichever side is shorter: `States: requested and commented`, or `States: all but approved`. */ export function statesLabel(states: readonly ReviewerState[]): string { - const hidden = ReviewerStateSchema.options.filter((state) => !states.includes(state)); - if (hidden.length === 0) return "States: all"; - if (states.length <= hidden.length) return `States: ${namesOf(states)}`; - return `States: all but ${namesOf(hidden)}`; + return shownLabel(states, ReviewerStateSchema.options, STATE_NAMES); } -function namesOf(states: readonly ReviewerState[]): string { - const names = states.map((state) => STATE_NAMES[state]); - return names.length === 1 ? names[0] : `${names.slice(0, -1).join(", ")} and ${names.at(-1)}`; +/** Which review statuses the Authored tab shows, named as `statesLabel` names states. */ +export function statusesLabel(statuses: readonly ReviewStatus[]): string { + return shownLabel(statuses, ReviewStatusSchema.options, STATUS_NAMES); +} + +function shownLabel(shown: readonly State[], all: readonly State[], names: Record): string { + const hidden = all.filter((state) => !shown.includes(state)); + if (hidden.length === 0) return "States: all"; + const namesOf = (states: readonly State[]) => { + const named = states.map((state) => names[state]); + return named.length === 1 ? named[0] : `${named.slice(0, -1).join(", ")} and ${named.at(-1)}`; + }; + if (shown.length <= hidden.length) return `States: ${namesOf(shown)}`; + return `States: all but ${namesOf(hidden)}`; } -/** 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-preferences.ts b/plugins/guided-review/client/inbox-preferences.ts index d6b9dd3..50155cf 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: Preferences; + 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..fdea1ec 100644 --- a/plugins/guided-review/client/inbox.tsx +++ b/plugins/guided-review/client/inbox.tsx @@ -1,17 +1,53 @@ 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 { CHECK_OFF_FILTERS, INBOX_COLUMNS, type CheckOffFilter, type InboxColumn, type InboxPreferences, 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, + ReviewStatusSchema, + 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 { useInboxPreferences } from "./inbox-preferences.ts"; +import { ASSIGNED_CHECK_OFF, AUTHORED_CHECK_OFF, CheckOffContext, INBOX_QUERY_KEY, useCheckOff, type CheckOffControl } from "./check-off.ts"; +import { + age, + ASSIGNED_RULES, + AUTHORED_RULES, + countLabel, + hiddenCheckedOff, + passesCheckOff, + STATE_NAMES, + STATUS_NAMES, + statesLabel, + statusesLabel, + tabLabel, + visibleItems, + type ListedItem, + type ListFilters, + type TabRules, +} from "./inbox-filter.ts"; +import { useInboxPreferences, type InboxPreferencesControl } from "./inbox-preferences.ts"; import { NewReview } from "./new-review.tsx"; import { describePreparing, describeProgress } from "./start-progress.ts"; import { useReviewStarts, type Pasted, type ReviewStarts } from "./start-review.ts"; @@ -22,73 +58,212 @@ 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[]; + specs: Record>; + /** The columns a card puts on its tinted line, in the order it puts them. */ + tinted: readonly Column[]; + states: StatesSetup; + 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(); - const checkOff = useCheckOff(); - const [query, setSearch] = useState(""); + const starts = useReviewStarts(openPanel, [...(inbox.data?.assigned ?? []), ...(inbox.data?.authored ?? [])]); + const assigned = useInboxPreferences("assigned"); + const authored = useInboxPreferences("authored"); + const checkOffs = { assigned: useCheckOff(ASSIGNED_CHECK_OFF), authored: useCheckOff(AUTHORED_CHECK_OFF) }; + 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); + checkOffs[tab].release(); + setQueries((current) => ({ ...current, [tab]: next })); }; const refresh = () => { - checkOff.release(); + checkOffs.assigned.release(); + checkOffs.authored.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. */ +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: InboxPreferencesControl>; + checkOff: CheckOffControl; +}) { + const { preferences, error } = control; + const [choosing, setChoosing] = useState<"columns" | "states" | null>(null); + const { held } = checkOff; + + 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) })); + // 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, preferences.columns) ? "table" : "cards"; + const rows = { + toggle: (row: ListedItem) => { + const item = listed.find((entry) => entry.url === row.url); + if (item !== undefined) checkOff.toggle(item); + }, + dimmed: (item: ListedItem) => held.has(item.url) && !passesCheckOff(item, preferences), + }; + + return ( + <>