From e22260c79317b496c4d7419d4c2e246aa11de963 Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Tue, 25 Aug 2026 19:22:00 -0500 Subject: [PATCH 01/11] config: tabs (implicit authors fallback, board.tabs overlay, readonly in modal) --- bun.lock | 4 +- config.team.example.json | 8 ++- package.json | 2 +- src/__tests__/board.test.ts | 3 +- src/__tests__/config-store-latch.test.ts | 11 ++++ src/__tests__/config.test.ts | 15 +++++ src/client/__tests__/config-shapes.test.ts | 14 +++- src/config.ts | 74 ++++++++++++++++++++++ 8 files changed, 125 insertions(+), 6 deletions(-) diff --git a/bun.lock b/bun.lock index 3bfe4ad..626d63b 100644 --- a/bun.lock +++ b/bun.lock @@ -6,7 +6,7 @@ "name": "mr-board", "dependencies": { "@mattstack/glance": "^0.19.0", - "@mattstack/rt-client": "^0.4.1", + "@mattstack/rt-client": "^0.5.0", "@mattstack/settings-kit": "^0.1.2", "@mattstack/tui-kit": "file:../tui-kit", "invadrs": "^0.2.0", @@ -42,7 +42,7 @@ "@mattstack/glance": ["@mattstack/glance@0.19.0", "", { "dependencies": { "@gitbeaker/rest": "^43.8.0", "@octokit/core": "^7.0.7", "@octokit/graphql": "^9.0.4", "@octokit/plugin-paginate-rest": "^15.0.0", "@octokit/plugin-retry": "^8.1.1", "@octokit/plugin-throttling": "^11.0.5", "@octokit/request-error": "^7.1.1" } }, "sha512-H+QMuyC3IZ3SXl8TpdjIKVhZvx+vP2ltVdt88CDz8e6YFponB2fplyOrHXLvGFITN1EnnEM6DQyYTcQ06F+7fg=="], - "@mattstack/rt-client": ["@mattstack/rt-client@0.4.1", "", { "dependencies": { "jsonc-parser": "^3.3.1" }, "peerDependencies": { "@mattstack/glance": ">=0.13.0" } }, "sha512-FFXGJWQeqlIb6t0gA9XkMN7Y0V86rIN8BOzTmU67rRKgjT/zrUl3nVFX5IjrTJh1uQgORog62+k4hvQNZ/RK4A=="], + "@mattstack/rt-client": ["@mattstack/rt-client@0.5.0", "", { "dependencies": { "jsonc-parser": "^3.3.1" }, "peerDependencies": { "@mattstack/glance": ">=0.13.0" } }, "sha512-wDwoq1okNAJA+JBlZhIxYSxcl1+O6xC4FQg98/RsXNoU4fHtPQzoLnWf+S7kbKNMnm6YpEn9JKK8+VYeIXtgPg=="], "@mattstack/settings-kit": ["@mattstack/settings-kit@0.1.2", "", { "peerDependencies": { "@mattstack/rt-client": "^0.4.1", "react": ">=18" }, "optionalPeers": ["react"] }, "sha512-OjeE8GwP2OSjt82kM+zfFDFfzyujWPD0Epg/wxq8+p7aIkMRMudLEMMWTe6VmEx4QPPT73kVxxHB1sBt0E6zbA=="], diff --git a/config.team.example.json b/config.team.example.json index 826fb53..c7066a7 100644 --- a/config.team.example.json +++ b/config.team.example.json @@ -35,5 +35,11 @@ "approved": "white_check_mark" } }, - "switchboard": { "url": "" } + "switchboard": { "url": "" }, + "tabs": [ + { "id": "team", "label": "Team", "source": { "kind": "authors" } }, + { "id": "codeowner-queue", "label": "Codeowner Queue", + "source": { "kind": "codeowners", "section": "Acme", "excludeMembers": true }, + "slackChannel": "team-codeowners" } + ] } diff --git a/package.json b/package.json index 48c84eb..ce6e3bd 100644 --- a/package.json +++ b/package.json @@ -19,7 +19,7 @@ }, "dependencies": { "@mattstack/glance": "^0.19.0", - "@mattstack/rt-client": "^0.4.1", + "@mattstack/rt-client": "^0.5.0", "@mattstack/settings-kit": "^0.1.2", "@mattstack/tui-kit": "file:../tui-kit", "invadrs": "^0.2.0", diff --git a/src/__tests__/board.test.ts b/src/__tests__/board.test.ts index 401a5f1..0c0129c 100644 --- a/src/__tests__/board.test.ts +++ b/src/__tests__/board.test.ts @@ -2,7 +2,7 @@ import { describe, expect, test } from "bun:test"; import type { PullRequest } from "@mattstack/glance"; import { aggregateSyncScope, boardDemand, buildBoard, buildRoster, projectPathFromWebUrl, stripDraftPrefix, type BoardMR } from "../data.ts"; import { SnapshotCache, type FetchResult } from "../cache.ts"; -import { DEFAULT_SLACK_EMOJI, type BoardConfig } from "../config.ts"; +import { DEFAULT_SLACK_EMOJI, IMPLICIT_TABS, type BoardConfig } from "../config.ts"; import { extractTicketId } from "../ticket.ts"; const config: BoardConfig = { @@ -32,6 +32,7 @@ const config: BoardConfig = { emoji: DEFAULT_SLACK_EMOJI, }, switchboard: { url: "" }, + tabs: IMPLICIT_TABS, }; function pr(overrides: Partial): PullRequest { diff --git a/src/__tests__/config-store-latch.test.ts b/src/__tests__/config-store-latch.test.ts index 7bfdbd0..e9aa013 100644 --- a/src/__tests__/config-store-latch.test.ts +++ b/src/__tests__/config-store-latch.test.ts @@ -199,6 +199,17 @@ describe("loadConfigFrom: config.json-optional boot once the team store owns the }); }); +describe("loadConfigFrom: board.tabs overlay", () => { + test("board.tabs store value overlays config.json", () => { + const p = tmpConfig(); + const resolve = fakeResolve({ + "board.tabs": [{ id: "q", label: "Q", source: { kind: "codeowners", section: "Acme", excludeMembers: true }, slackChannel: "team-codeowners" }], + }); + const cfg = loadConfigFrom(p, resolve); + expect(cfg.tabs[0]!.id).toBe("q"); + }); +}); + describe("saveMemberHidden: latch-gated writer", () => { test("unowned: writes config.json's inline hidden flag, store untouched", () => { const p = tmpConfig({ ...base, members: [{ username: "alice" }, { username: "bob" }] }); diff --git a/src/__tests__/config.test.ts b/src/__tests__/config.test.ts index 16f5b47..203d962 100644 --- a/src/__tests__/config.test.ts +++ b/src/__tests__/config.test.ts @@ -107,6 +107,21 @@ describe("parseConfig", () => { expect(parseConfig(JSON.stringify(base)).rtRepos).toEqual({}); }); + test("config without tabs gets the implicit authors tab", () => { + const cfg = parseConfig(JSON.stringify(base)); + expect(cfg.tabs).toEqual([{ id: "team", label: "Team", source: { kind: "authors" } }]); + }); + + test("tabs validate: unique ids, codeowners needs a section", () => { + expect(() => parseConfig(JSON.stringify({ ...base, tabs: [ + { id: "a", label: "A", source: { kind: "codeowners" } }, + ] }))).toThrow(/section/); + expect(() => parseConfig(JSON.stringify({ ...base, tabs: [ + { id: "a", label: "A", source: { kind: "authors" } }, + { id: "a", label: "B", source: { kind: "authors" } }, + ] }))).toThrow(/duplicate tab id/); + }); + test("port, host, reviewSkill, respondSkill, teamClone are gone from the parsed shape", () => { const cfg = parseConfig(JSON.stringify({ ...base, port: 9999, host: "0.0.0.0", reviewSkill: "x:review", respondSkill: "x:respond", teamClone: "~/team", diff --git a/src/client/__tests__/config-shapes.test.ts b/src/client/__tests__/config-shapes.test.ts index bb9298f..119d74f 100644 --- a/src/client/__tests__/config-shapes.test.ts +++ b/src/client/__tests__/config-shapes.test.ts @@ -33,15 +33,27 @@ function def(over: Partial & { key: string }): ConfigDef { }; } +/** Composite board.* registry keys with no edit UI yet -- rowKind's + "readonly" fallback (no COMPOSITE_SHAPES entry) is the intended + rendering for these, not a coverage gap. */ +const DELIBERATELY_READONLY_COMPOSITES = ["board.tabs"]; + describe("COMPOSITE_SHAPES", () => { - test("covers every composite board.* key in the registry", () => { + test("covers every composite board.* key in the registry except the deliberately-readonly ones", () => { const composites = allDefs() .filter((d) => d.key.startsWith("board.") && (d.type === "object" || d.type === "array")) .map((d) => d.key) + .filter((k) => !DELIBERATELY_READONLY_COMPOSITES.includes(k)) .sort(); expect(Object.keys(COMPOSITE_SHAPES).sort()).toEqual(composites); }); + test("board.tabs is a composite key deliberately left readonly", () => { + expect(allDefs().find((d) => d.key === "board.tabs")?.type).toBe("array"); + expect(COMPOSITE_SHAPES["board.tabs"]).toBeUndefined(); + expect(rowKind(def({ key: "board.tabs", type: "array" }))).toBe("readonly"); + }); + test("names no key the registry lacks", () => { const known = new Set(allDefs().map((d) => d.key)); for (const key of Object.keys(COMPOSITE_SHAPES)) expect(known.has(key)).toBe(true); diff --git a/src/config.ts b/src/config.ts index d75419c..618f393 100644 --- a/src/config.ts +++ b/src/config.ts @@ -33,6 +33,19 @@ export function daemonRepoField(config: Pick, projectPat return repoIdentityField(config.rtRepos[projectPath]); } +export interface TabConfig { + id: string; + label: string; + source: { kind: "authors" } | { kind: "codeowners"; section: string; excludeMembers?: boolean }; + /** Overrides slack.channel for this tab's index, reactions, and posts. */ + slackChannel?: string; + /** Overrides review-launch skill resolution for this tab. Empty/absent = normal resolution. */ + reviewSkill?: string; +} + +/** No config.json/store tabs = one classic authors-roster tab, never zero tabs. */ +export const IMPLICIT_TABS: TabConfig[] = [{ id: "team", label: "Team", source: { kind: "authors" } }]; + export interface Member { username: string; /** Optional display name; falls back to the GitLab profile lookup, then username. */ @@ -90,6 +103,8 @@ export interface BoardConfig { /** Peer-boards relay. Empty url disables every peer feature (publish, poll, nudge endpoint) cleanly. Token comes from SWITCHBOARD_TOKEN, not config. */ switchboard: SwitchboardBoardConfig; + /** Board tabs, in display order. Absent in config.json/store = IMPLICIT_TABS. */ + tabs: TabConfig[]; } export interface SwitchboardBoardConfig { @@ -202,6 +217,7 @@ export function parseConfig(raw: string, source = "config.json"): BoardConfig { } const slack = parseSlack(cfg.slack, source); const switchboard = parseSwitchboard(cfg.switchboard, source); + const tabs = parseTabs(cfg.tabs, source); const rtRepos = (cfg.rtRepos && typeof cfg.rtRepos === "object" && !Array.isArray(cfg.rtRepos)) ? Object.fromEntries(Object.entries(cfg.rtRepos).filter(([, v]) => typeof v === "string")) : {}; @@ -226,9 +242,66 @@ export function parseConfig(raw: string, source = "config.json"): BoardConfig { rtRepos, slack, switchboard, + tabs, }; } +/** Absent = IMPLICIT_TABS (the classic single authors-roster board), never zero tabs. */ +function parseTabs(raw: unknown, source: string): TabConfig[] { + if (raw === undefined) return IMPLICIT_TABS; + if (!Array.isArray(raw)) throw new Error(`${source} "tabs" must be an array`); + const seenIds = new Set(); + return raw.map((entry, i) => { + const label = `tabs[${i}]`; + if (!entry || typeof entry !== "object") { + throw new Error(`${source} "${label}" must be an object`); + } + const t = entry as Partial; + if (!t.id || typeof t.id !== "string") { + throw new Error(`${source} "${label}" is missing a non-empty "id"`); + } + if (seenIds.has(t.id)) { + throw new Error(`${source} has a duplicate tab id "${t.id}" in ${label}`); + } + seenIds.add(t.id); + if (!t.label || typeof t.label !== "string") { + throw new Error(`${source} "${label}" is missing a non-empty "label"`); + } + if (!t.source || typeof t.source !== "object") { + throw new Error(`${source} "${label}.source" must be an object`); + } + const src = t.source as { kind?: string; section?: string; excludeMembers?: unknown }; + if (src.kind !== "authors" && src.kind !== "codeowners") { + throw new Error(`${source} "${label}.source.kind" must be "authors" or "codeowners"`); + } + let source_: TabConfig["source"]; + if (src.kind === "authors") { + source_ = { kind: "authors" }; + } else { + if (!src.section || typeof src.section !== "string") { + throw new Error(`${source} "${label}.source.section" is required for a codeowners tab`); + } + if (src.excludeMembers !== undefined && typeof src.excludeMembers !== "boolean") { + throw new Error(`${source} "${label}.source.excludeMembers" must be a boolean`); + } + source_ = { kind: "codeowners", section: src.section, ...(src.excludeMembers !== undefined ? { excludeMembers: src.excludeMembers } : {}) }; + } + if (t.slackChannel !== undefined && typeof t.slackChannel !== "string") { + throw new Error(`${source} "${label}.slackChannel" must be a string`); + } + if (t.reviewSkill !== undefined && typeof t.reviewSkill !== "string") { + throw new Error(`${source} "${label}.reviewSkill" must be a string`); + } + return { + id: t.id, + label: t.label, + source: source_, + ...(t.slackChannel !== undefined ? { slackChannel: t.slackChannel } : {}), + ...(t.reviewSkill !== undefined ? { reviewSkill: t.reviewSkill } : {}), + }; + }); +} + /** Shared with saveSwitchboardUrl's owned-branch write, so a trailing slash never lands in the store either — peer/onboard.ts builds `${url}/invites` verbatim, and a stored slash would double up into `//invites`. */ @@ -391,6 +464,7 @@ function withBoardStoreFallback(fileConfig: BoardConfig, resolve: GetSettingFn): doctorCwd: cwds?.doctor ?? fileConfig.doctorCwd, rtRepos: rtReposStore ? Object.fromEntries(rtReposStore.map((r) => [r.project, r.repo])) : fileConfig.rtRepos, switchboard: { url: storeValue("board.switchboardUrl", resolve) ?? fileConfig.switchboard.url }, + tabs: storeValue("board.tabs", resolve) ?? fileConfig.tabs, }; return parseConfig(JSON.stringify(merged), "a board.* team settings-store value"); From 905dda3ed60f68d81c3ec6e3b385ebfde9e82a98 Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Tue, 25 Aug 2026 19:30:57 -0500 Subject: [PATCH 02/11] board: sections demand, tagged rows through buildBoard, tabs in /data.json --- src/__tests__/board.test.ts | 73 +++++++++++++++++++++++++++++++++++-- src/__tests__/cache.test.ts | 2 +- src/cache.ts | 10 ++++- src/data.ts | 57 +++++++++++++++++++++++------ src/server.ts | 21 ++++++++--- 5 files changed, 140 insertions(+), 23 deletions(-) diff --git a/src/__tests__/board.test.ts b/src/__tests__/board.test.ts index 0c0129c..c7a0f27 100644 --- a/src/__tests__/board.test.ts +++ b/src/__tests__/board.test.ts @@ -2,7 +2,7 @@ import { describe, expect, test } from "bun:test"; import type { PullRequest } from "@mattstack/glance"; import { aggregateSyncScope, boardDemand, buildBoard, buildRoster, projectPathFromWebUrl, stripDraftPrefix, type BoardMR } from "../data.ts"; import { SnapshotCache, type FetchResult } from "../cache.ts"; -import { DEFAULT_SLACK_EMOJI, IMPLICIT_TABS, type BoardConfig } from "../config.ts"; +import { DEFAULT_SLACK_EMOJI, IMPLICIT_TABS, type BoardConfig, type TabConfig } from "../config.ts"; import { extractTicketId } from "../ticket.ts"; const config: BoardConfig = { @@ -35,6 +35,13 @@ const config: BoardConfig = { tabs: IMPLICIT_TABS, }; +/** A team tab plus one codeowners tab watching "Acme" -- used by the + boardDemand and tagged-row buildBoard tests below. */ +const tabsWithCodeowners: TabConfig[] = [ + { id: "t", label: "T", source: { kind: "authors" } }, + { id: "q", label: "Q", source: { kind: "codeowners", section: "Acme" } }, +]; + function pr(overrides: Partial): PullRequest { return { id: "gitlab:1", @@ -259,6 +266,47 @@ describe("buildBoard", () => { }); }); +describe("buildBoard tagged rows (codeowner tabs)", () => { + const withTabs: BoardConfig = { ...config, tabs: tabsWithCodeowners }; + const now = Date.parse("2026-07-11T00:00:00Z"); + + test("keeps a tagged stranger and stamps codeownerSections", () => { + const stranger = pr({ id: "gitlab:900", iid: 9, author: { id: "gitlab:99", username: "outsider", name: "Outsider", avatarUrl: null } }); + const tags = new Map([[stranger.id, ["Acme"]]]); + const out = buildBoard([stranger], withTabs, now, tags); + expect(out).toHaveLength(1); + expect(out[0]!.codeownerSections).toEqual(["Acme"]); + }); + + test("still drops an untagged stranger, and tag-kept rows skip the prefix filter", () => { + const withPrefixes: BoardConfig = { ...withTabs, ticketPrefixes: ["CV"] }; + // untagged stranger -> dropped + const untaggedStranger = pr({ id: "gitlab:901", iid: 10, author: { id: "gitlab:100", username: "ghost", name: "Ghost", avatarUrl: null } }); + // tagged stranger with no ticket prefix while ticketPrefixes=["CV"] -> kept + const taggedNoPrefix = pr({ + id: "gitlab:902", + iid: 11, + author: { id: "gitlab:101", username: "outsider", name: "Outsider", avatarUrl: null }, + sourceBranch: "no-ticket", + title: "no ticket here", + }); + // tagged MR from a section no tab declares -> dropped + const taggedWrongSection = pr({ + id: "gitlab:903", + iid: 12, + author: { id: "gitlab:102", username: "outsider2", name: "Outsider2", avatarUrl: null }, + sourceBranch: "no-ticket", + title: "no ticket either", + }); + const tags = new Map([ + [taggedNoPrefix.id, ["Acme"]], + [taggedWrongSection.id, ["OtherSection"]], + ]); + const out = buildBoard([untaggedStranger, taggedNoPrefix, taggedWrongSection], withPrefixes, now, tags); + expect(out.map((m) => m.iid)).toEqual([11]); + }); +}); + describe("buildRoster", () => { const members = [{ username: "alice" }, { username: "bob", name: "Bobby" }, { username: "carol" }]; @@ -294,6 +342,12 @@ describe("boardDemand", () => { expect(d.authors).toEqual(["a", "b"]); // hidden is a display state, not a demand state expect(d.declaredAt).toBeGreaterThan(0); }); + + test("declares the union of tab sections, and omits the field when no tab is codeowners", () => { + const withTabs: BoardConfig = { ...config, tabs: tabsWithCodeowners }; + expect(boardDemand(withTabs, 1).codeownerSections).toEqual(["Acme"]); + expect(boardDemand(config, 1).codeownerSections).toBeUndefined(); + }); }); describe("aggregateSyncScope", () => { @@ -303,7 +357,12 @@ describe("aggregateSyncScope", () => { }); test("no reads yields null syncedAt/windowDays and an empty uncovered list", () => { - expect(aggregateSyncScope([])).toEqual({ dataSyncedAt: null, scopeUncovered: [], scopeWindowDays: null }); + expect(aggregateSyncScope([])).toEqual({ + dataSyncedAt: null, + scopeUncovered: [], + scopeWindowDays: null, + scopeUncoveredSections: [], + }); }); test("unions scope.uncovered across reads and takes the min windowDays", () => { @@ -320,12 +379,20 @@ describe("aggregateSyncScope", () => { expect(agg.scopeWindowDays).toBeNull(); expect(agg.scopeUncovered).toEqual([]); }); + + test("unions uncoveredSections", () => { + const agg = aggregateSyncScope([ + { syncedAt: 1, scope: { authors: [], windowDays: 30, uncovered: [], sections: [], uncoveredSections: ["Acme"] } }, + { syncedAt: 2 }, + ]); + expect(agg.scopeUncoveredSections).toEqual(["Acme"]); + }); }); /** Wrap a bare mrs array as the FetchResult shape SnapshotCache now expects, for tests that only care about the mrs field. */ function fetchResult(mrs: unknown[]): FetchResult { - return { mrs: mrs as BoardMR[], dataSyncedAt: null, scopeUncovered: [], scopeWindowDays: null }; + return { mrs: mrs as BoardMR[], dataSyncedAt: null, scopeUncovered: [], scopeWindowDays: null, scopeUncoveredSections: [] }; } describe("SnapshotCache", () => { diff --git a/src/__tests__/cache.test.ts b/src/__tests__/cache.test.ts index 9b42831..fb579a9 100644 --- a/src/__tests__/cache.test.ts +++ b/src/__tests__/cache.test.ts @@ -3,7 +3,7 @@ import { SnapshotCache, type FetchResult } from "../cache.ts"; /** Wrap a bare mrs array as the FetchResult shape SnapshotCache expects. */ function fetchResult(mrs: unknown[]): FetchResult { - return { mrs: mrs as FetchResult["mrs"], dataSyncedAt: null, scopeUncovered: [], scopeWindowDays: null }; + return { mrs: mrs as FetchResult["mrs"], dataSyncedAt: null, scopeUncovered: [], scopeWindowDays: null, scopeUncoveredSections: [] }; } describe("SnapshotCache forced-refresh failure", () => { diff --git a/src/cache.ts b/src/cache.ts index 7765314..f7ea49a 100644 --- a/src/cache.ts +++ b/src/cache.ts @@ -112,7 +112,15 @@ export class SnapshotCache { return settle( this.snapshot ? { ...this.snapshot, fetchError: message } - : { mrs: [], fetchedAt: this.now(), fetchError: message, dataSyncedAt: null, scopeUncovered: [], scopeWindowDays: null }, + : { + mrs: [], + fetchedAt: this.now(), + fetchError: message, + dataSyncedAt: null, + scopeUncovered: [], + scopeWindowDays: null, + scopeUncoveredSections: [], + }, ); }) .finally(() => { diff --git a/src/data.ts b/src/data.ts index 6133099..cd51e40 100644 --- a/src/data.ts +++ b/src/data.ts @@ -32,6 +32,11 @@ export type BoardMR = MRDashboardProps & { /** rt repo name for daemon reads (config.rtRepos[projectPath]); null when the project has no mapping, which surfaces as a fetch error server-side. */ rtRepo: string | null; + /** Codeowner sections this row was tagged into that a configured codeowners + tab also declares. Empty when the row wasn't tag-kept (a member's own + MR) or its tags matched no configured tab. Display filtering by tab is + Task 11's job -- buildBoard only stamps the intersection. */ + codeownerSections: string[]; }; export interface Snapshot { @@ -48,6 +53,9 @@ export interface Snapshot { /** Narrowest `scope.windowDays` among the daemon reads; null when no read carried a scope. */ scopeWindowDays: number | null; + /** Union of `scope.uncoveredSections` across the daemon reads: codeowner + sections some project's sync hasn't swept yet. */ + scopeUncoveredSections: string[]; } /** Parse "group/project" out of a GitLab MR web URL. */ @@ -84,20 +92,37 @@ function scrubAvatarUrls(value: unknown): void { } /** - * Shape raw MRs into a flat board list: authored by a configured member, open, - * not draft, in a configured project. Each MR is tagged with its author, created - * / updated timestamps, unresolved-thread count, and derived pipeline state. The - * client owns all grouping and sorting, so this list is unsorted. + * Shape raw MRs into a flat board list: authored by a configured member (or + * tagged into a configured codeowners tab's section), open, not draft, in a + * configured project. Each MR is tagged with its author, created / updated + * timestamps, unresolved-thread count, derived pipeline state, and its kept + * codeowner tags. The client owns all grouping and sorting, so this list is + * unsorted. `tags` (keyed by pr.id) is the per-MR codeowner sections a daemon + * read reported; omitted entirely for callers that never declare + * codeownerSections demand (e.g. the single-member fetch). */ -export function buildBoard(prs: PullRequest[], config: BoardConfig, now: number = Date.now()): BoardMR[] { +export function buildBoard( + prs: PullRequest[], + config: BoardConfig, + now: number = Date.now(), + tags?: Map, +): BoardMR[] { const members = new Set(config.members.map((m) => m.username)); const projects = new Set(config.projects); const staleCutoff = now - config.staleAfterDays * 86_400_000; const prefixes = new Set(config.ticketPrefixes); + const tabSections = new Set(config.tabs.flatMap((t) => (t.source.kind === "codeowners" ? [t.source.section] : []))); const out: BoardMR[] = []; for (const pr of prs) { if (pr.state !== "opened") continue; - if (!pr.author || !members.has(pr.author.username)) continue; + if (!pr.author) continue; + const isMember = members.has(pr.author.username); + // Only tags matching a currently-configured tab count -- a tab removed + // from config must not keep stale-tagged rows on the board. + const tagged = (tags?.get(pr.id) ?? []).filter((s) => tabSections.has(s)); + // A tagged row is on the board regardless of author; per-tab display + // filtering by section is Task 11's job, not buildBoard's. + if (!isMember && tagged.length === 0) continue; // Someone else's draft isn't yours to act on, so it stays off the board; // your own show up with a DRAFT chip and a "mark ready" action. With // defaultMember "all" there's no single "you", so no drafts are shown. @@ -106,7 +131,9 @@ export function buildBoard(prs: PullRequest[], config: BoardConfig, now: number if (pr.updatedAt && Date.parse(pr.updatedAt) < staleCutoff) continue; // Team filter: keep only MRs whose Linear ticket prefix is configured. // No prefixes configured → keep everything. Untagged MRs are dropped. - if (prefixes.size > 0) { + // Ticket-prefix filtering is a roster-board concept; a row kept by its + // codeowner tag rides the board regardless of whose ticket it carries. + if (prefixes.size > 0 && tagged.length === 0) { const ticket = extractTicketId(pr.sourceBranch, pr.title); const prefix = ticket ? ticket.slice(0, ticket.indexOf("-")) : null; if (!prefix || !prefixes.has(prefix)) continue; @@ -132,6 +159,7 @@ export function buildBoard(prs: PullRequest[], config: BoardConfig, now: number isDraft: pr.draft === true, repositoryId: pr.repositoryId, rtRepo: config.rtRepos[path] ?? null, + codeownerSections: tagged, }); } return out; @@ -161,9 +189,11 @@ export function stripDraftPrefix(title: string): string { // (the actual listen port is env/default-derived, not config), so callers // pass the resolved port explicitly. export function boardDemand(config: BoardConfig, port: number): DemandDecl { + const sections = [...new Set(config.tabs.flatMap((t) => (t.source.kind === "codeowners" ? [t.source.section] : [])))]; return { client: `mr-board:${port}`, authors: config.members.map((m) => m.username), + ...(sections.length > 0 ? { codeownerSections: sections } : {}), declaredAt: Date.now(), }; } @@ -171,7 +201,7 @@ export function boardDemand(config: BoardConfig, port: number): DemandDecl { /** One project's sync facts, the shape aggregateSyncScope folds across projects. */ export interface SyncScopeRead { syncedAt: number; - scope?: { authors: string[]; windowDays: number; uncovered: string[] }; + scope?: { authors: string[]; windowDays: number; uncovered: string[]; sections?: string[]; uncoveredSections?: string[] }; } /** @@ -179,23 +209,26 @@ export interface SyncScopeRead { * board-wide picture. `dataSyncedAt` is the oldest syncedAt (the board is * only as fresh as its stalest project); `scopeUncovered` unions every * project's uncovered authors; `scopeWindowDays` is the narrowest window - * (the tightest constraint any project reported). A project that errored - * before yielding a read is simply absent from `reads`. + * (the tightest constraint any project reported); `scopeUncoveredSections` + * unions every project's uncovered codeowner sections. A project that + * errored before yielding a read is simply absent from `reads`. */ export function aggregateSyncScope( reads: SyncScopeRead[], -): { dataSyncedAt: number | null; scopeUncovered: string[]; scopeWindowDays: number | null } { +): { dataSyncedAt: number | null; scopeUncovered: string[]; scopeWindowDays: number | null; scopeUncoveredSections: string[] } { let dataSyncedAt: number | null = null; let scopeWindowDays: number | null = null; const uncovered = new Set(); + const uncoveredSections = new Set(); for (const read of reads) { dataSyncedAt = dataSyncedAt === null ? read.syncedAt : Math.min(dataSyncedAt, read.syncedAt); if (read.scope) { scopeWindowDays = scopeWindowDays === null ? read.scope.windowDays : Math.min(scopeWindowDays, read.scope.windowDays); for (const author of read.scope.uncovered) uncovered.add(author); + for (const section of read.scope.uncoveredSections ?? []) uncoveredSections.add(section); } } - return { dataSyncedAt, scopeUncovered: [...uncovered], scopeWindowDays }; + return { dataSyncedAt, scopeUncovered: [...uncovered], scopeWindowDays, scopeUncoveredSections: [...uncoveredSections] }; } /** diff --git a/src/server.ts b/src/server.ts index cc38adf..0c739bb 100644 --- a/src/server.ts +++ b/src/server.ts @@ -168,12 +168,16 @@ function attachPeerState(mrs: T[], now: nu const FETCH_CONCURRENCY = 4; /** fetchTeamMRs' result: the opened MRs plus the aggregated sync facts from - every project read, for the caller to fold into the snapshot. */ + every project read, for the caller to fold into the snapshot. `tags` is + every tagged MR's codeowner sections, keyed by pr.id, for buildBoard to + intersect against the configured tabs. */ interface TeamMRsResult { prs: PullRequest[]; dataSyncedAt: number | null; scopeUncovered: string[]; scopeWindowDays: number | null; + scopeUncoveredSections: string[]; + tags: Map; } /** @@ -193,6 +197,7 @@ interface TeamMRsResult { */ async function fetchTeamMRs(force = false): Promise { const byId = new Map(); + const tags = new Map(); const errors: string[] = []; const reads: SyncScopeRead[] = []; const demand = boardDemand(config, port); @@ -209,11 +214,13 @@ async function fetchTeamMRs(force = false): Promise { } reads.push({ syncedAt: res.data.syncedAt, scope: res.data.scope }); for (const entry of Object.values(res.data.mrs)) { - if (entry.pr.state === "opened") byId.set(entry.pr.id, entry.pr); + if (entry.pr.state !== "opened") continue; + byId.set(entry.pr.id, entry.pr); + if (entry.codeownerSections?.length) tags.set(entry.pr.id, entry.codeownerSections); } } if (errors.length) throw new Error(errors.join(" · ")); - return { prs: [...byId.values()], ...aggregateSyncScope(reads) }; + return { prs: [...byId.values()], ...aggregateSyncScope(reads), tags }; } /** Author string for a herdr tab label: the display name, else the username. */ @@ -274,10 +281,10 @@ const cache = new SnapshotCache(async () => { // latched for the background refreshes that follow. const force = forceNextFetch; forceNextFetch = false; - const { prs, dataSyncedAt, scopeUncovered, scopeWindowDays } = await fetchTeamMRs(force); - const mrs = buildBoard(prs, config); + const { prs, dataSyncedAt, scopeUncovered, scopeWindowDays, scopeUncoveredSections, tags } = await fetchTeamMRs(force); + const mrs = buildBoard(prs, config, undefined, tags); await enrichReviewerComments(mrs); - return { mrs, dataSyncedAt, scopeUncovered, scopeWindowDays }; + return { mrs, dataSyncedAt, scopeUncovered, scopeWindowDays, scopeUncoveredSections }; }); /** @@ -559,7 +566,9 @@ const httpServer = Bun.serve({ dataSyncedAt: snapshot.dataSyncedAt, scopeUncovered: snapshot.scopeUncovered, scopeWindowDays: snapshot.scopeWindowDays, + scopeUncoveredSections: snapshot.scopeUncoveredSections, staleAfterDays: config.staleAfterDays, + tabs: config.tabs, }), { headers: { "content-type": "application/json" } }, ); From bbe4399bcbc0481efd0d06654fc41c0843614779 Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Tue, 25 Aug 2026 19:44:30 -0500 Subject: [PATCH 03/11] client: tab bar, per-tab filtering, codeowner-queue syncing badge Co-Authored-By: Claude Fable 5 --- src/__tests__/view.test.ts | 54 ++++++++++++++++++++++- src/client/board/Board.tsx | 81 +++++++++++++++++++++++------------ src/client/board/Controls.tsx | 22 +++++++++- src/client/types.ts | 7 +++ src/view.ts | 25 ++++++++++- 5 files changed, 156 insertions(+), 33 deletions(-) diff --git a/src/__tests__/view.test.ts b/src/__tests__/view.test.ts index 4350d4c..611d07c 100644 --- a/src/__tests__/view.test.ts +++ b/src/__tests__/view.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from "bun:test"; import type { BoardMR } from "../data.ts"; -import { filterByMember, sortMRs, groupMRs, commentDot, dataAgeLabel, statusFlags, nestStacks, memberPeerState, joinRowState } from "../view.ts"; +import type { TabConfig } from "../config.ts"; +import { filterByMember, filterByTab, sortMRs, groupMRs, commentDot, dataAgeLabel, statusFlags, nestStacks, memberPeerState, joinRowState } from "../view.ts"; function mr(overrides: Partial): BoardMR { return { @@ -28,6 +29,35 @@ describe("filterByMember", () => { }); }); +describe("filterByTab", () => { + const members = new Set(["ada"]); + const rows = [ + mr({ iid: 1, author: { username: "ada" } as any, codeownerSections: ["Acme"] } as any), + mr({ iid: 2, author: { username: "outsider" } as any, codeownerSections: ["Acme"] } as any), + mr({ iid: 3, author: { username: "outsider" } as any, codeownerSections: [] } as any), + ]; + + test("authors tab passes every row through, deferring to filterByMember downstream", () => { + const team: TabConfig = { id: "t", label: "T", source: { kind: "authors" } }; + expect(filterByTab(rows, team, members)).toHaveLength(3); + }); + + test("codeowners tab filters to the section and excludes roster authors", () => { + const q: TabConfig = { id: "q", label: "Q", source: { kind: "codeowners", section: "Acme", excludeMembers: true } }; + expect(filterByTab(rows, q, members).map((m) => m.iid)).toEqual([2]); + }); + + test("codeowners tab without excludeMembers keeps roster authors in the section", () => { + const q: TabConfig = { id: "q", label: "Q", source: { kind: "codeowners", section: "Acme" } }; + expect(filterByTab(rows, q, members).map((m) => m.iid)).toEqual([1, 2]); + }); + + test("codeowners tab drops rows outside the section regardless of authorship", () => { + const q: TabConfig = { id: "q", label: "Q", source: { kind: "codeowners", section: "Acme", excludeMembers: true } }; + expect(filterByTab(rows, q, members).some((m) => m.iid === 3)).toBe(false); + }); +}); + describe("commentDot", () => { test("no summary → no dot (fetch skipped/failed)", () => { expect(commentDot(undefined)).toBeNull(); @@ -400,6 +430,7 @@ describe("parseViewState", () => { member: "bob", group: "status", sort: "progress", + tab: "", }); }); test("ignores unknown member and invalid group/sort", () => { @@ -421,6 +452,22 @@ describe("parseViewState", () => { test("stored value still wins over defaultMember", () => { expect(parseViewState("", { member: "alice" }, members, "bob").member).toBe("alice"); }); + + test("no validTabs known yet resolves tab to empty, matching DEFAULT_VIEW", () => { + expect(parseViewState("?tab=q", null, members).tab).toBe(""); + }); + + test("a known tab id from the URL wins", () => { + expect(parseViewState("?tab=q", null, members, "all", ["t", "q"]).tab).toBe("q"); + }); + + test("an unknown tab id falls back to the first configured tab", () => { + expect(parseViewState("?tab=zzz", null, members, "all", ["t", "q"]).tab).toBe("t"); + }); + + test("no tab in the URL falls back to the first configured tab", () => { + expect(parseViewState("", null, members, "all", ["t", "q"]).tab).toBe("t"); + }); }); describe("serializeViewState", () => { @@ -428,7 +475,10 @@ describe("serializeViewState", () => { expect(serializeViewState(DEFAULT_VIEW)).toBe(""); }); test("includes non-defaults", () => { - expect(serializeViewState({ member: "bob", group: "status", sort: "oldest" })).toBe("?member=bob&group=status"); + expect(serializeViewState({ member: "bob", group: "status", sort: "oldest", tab: "" })).toBe("?member=bob&group=status"); + }); + test("includes a set tab, even a first-tab id", () => { + expect(serializeViewState({ member: "all", group: "age", sort: "oldest", tab: "team" })).toBe("?tab=team"); }); }); diff --git a/src/client/board/Board.tsx b/src/client/board/Board.tsx index c29cb47..25de565 100644 --- a/src/client/board/Board.tsx +++ b/src/client/board/Board.tsx @@ -1,6 +1,6 @@ import { useCallback, useEffect, useRef, useState } from "react"; import type { BoardMR } from "../../data.ts"; -import { filterByMember, sortMRs, groupMRs, parseViewState, serializeViewState, dataAgeLabel } from "../../view.ts"; +import { filterByMember, filterByTab, sortMRs, groupMRs, parseViewState, serializeViewState, dataAgeLabel } from "../../view.ts"; import type { ViewState } from "../../view.ts"; import { selectionOf, postableOf } from "../../selection.ts"; import type { @@ -93,7 +93,9 @@ export function Board() { } catch { stored = null; } - setState(parseViewState(location.search, stored, usernames, d.defaultMember)); + // Tab validated once here, same as member -- never re-derived from the + // URL on later polls, so a live tab pick survives the 60s refresh cycle. + setState(parseViewState(location.search, stored, usernames, d.defaultMember, d.tabs.map((t) => t.id))); } else { setState((prev) => (prev.member === "all" || usernames.includes(prev.member) ? prev : { ...prev, member: "all" })); } @@ -405,17 +407,32 @@ export function Board() { ? `board shows ${data.staleAfterDays} days but rt syncs ${data.scopeWindowDays} days... align configs` : null; + // data.tabs is always non-empty (server falls back to IMPLICIT_TABS); state.tab + // itself may briefly lag on the very first render before onData's validation + // pass lands, so fall back to the first tab rather than trust it blindly. + const activeTab = data.tabs.find((t) => t.id === state.tab) ?? data.tabs[0]!; + const isCodeownersTab = activeTab.source.kind === "codeowners"; + // A codeowners tab's "who counts as roster" set for excludeMembers. + const rosterUsernames = new Set(data.members.map((m) => m.username)); + const tabSyncing = + activeTab.source.kind === "codeowners" && data.scopeUncoveredSections.includes(activeTab.source.section); + // Server state wins; otherwise show an optimistic "queued" badge if pending. const mrs = overlay(data.mrs, optimisticLifecycle.state); - const filtered = filterByMember(mrs, state.member); + const tabFiltered = filterByTab(mrs, activeTab, rosterUsernames); + // Codeowners tabs bypass member filtering entirely (and the sidebar that + // drives it) -- the queue is scoped by section, not by roster author. + const filtered = isCodeownersTab ? tabFiltered : filterByMember(tabFiltered, state.member); const groups = groupMRs(filtered, state.group, data.members.map((m) => m.username), now).map((g) => ({ label: g.label, mrs: sortMRs(g.mrs, state.sort), })); - const activeMember = state.member === "all" ? null : data.members.find((m) => m.username === state.member) ?? null; + const activeMember = + !isCodeownersTab && state.member !== "all" ? data.members.find((m) => m.username === state.member) ?? null : null; // Show each row's author only when the view mixes authors: the All view - // grouped by anything but author (where the group header isn't the name). - const showAuthor = state.member === "all" && state.group !== "author"; + // grouped by anything but author (where the group header isn't the name), + // or a codeowners tab, which is never narrowed to one author. + const showAuthor = isCodeownersTab || (state.member === "all" && state.group !== "author"); // Under author grouping the header IS the name, so rows normally drop the // author tag -- but a stack pulled to its root's group can carry a // co-author's MR under someone else's header. Tag the rows whenever a group @@ -467,20 +484,26 @@ export function Board() { canPostSummary: data.slackEnabled && data.local && postableMrs.length > 0, postingSummary, onPostSummary: () => handlePostSummary(postableMrs), + tabs: data.tabs, + tabSyncing, }; return (
- {/* Desktop roster (hidden on mobile, where it moves into the drawer). */} - update({ member })} - onSettings={openSettings} - onConfig={openConfig} - scopeUncovered={data.scopeUncovered} - /> + {/* Desktop roster (hidden on mobile, where it moves into the drawer). + Also hidden on a codeowners tab: it isn't filtered by member, so the + roster has nothing to drive. */} + {!isCodeownersTab && ( + update({ member })} + onSettings={openSettings} + onConfig={openConfig} + scopeUncovered={data.scopeUncovered} + /> + )}
@@ -570,18 +593,20 @@ export function Board() { {ICONS.close}
- { - update({ member }); - setMenuOpen(false); - }} - onSettings={openSettings} - onConfig={openConfig} - scopeUncovered={data.scopeUncovered} - /> + {!isCodeownersTab && ( + { + update({ member }); + setMenuOpen(false); + }} + onSettings={openSettings} + onConfig={openConfig} + scopeUncovered={data.scopeUncovered} + /> + )}
diff --git a/src/client/board/Controls.tsx b/src/client/board/Controls.tsx index 497580e..4b1a58b 100644 --- a/src/client/board/Controls.tsx +++ b/src/client/board/Controls.tsx @@ -1,7 +1,8 @@ import { GROUP_KEYS, SORT_KEYS } from "../../view.ts"; import type { ViewState } from "../../view.ts"; +import type { TabConfig } from "../../config.ts"; import type { ThemeMode, ViewMode } from "../types.ts"; -import { CopyButton, ICONS, LabeledSeg, Segmented } from "@mattstack/tui-kit"; +import { Chip, CopyButton, ICONS, LabeledSeg, Segmented } from "@mattstack/tui-kit"; import { GROUP_LABEL, SORT_LABEL } from "./format.ts"; import { SLACK_ICON } from "./chips.tsx"; @@ -21,6 +22,8 @@ function Controls({ onPostSummary, canPostSummary, postingSummary, + tabs, + tabSyncing, stacked = false, }: { state: ViewState; @@ -36,17 +39,32 @@ function Controls({ onPostSummary?: () => void; canPostSummary?: boolean; postingSummary?: boolean; + /** Board tabs, in display order; the bar itself only renders past one tab. */ + tabs: TabConfig[]; + /** Whether the active codeowners tab's section is still mid-backfill. */ + tabSyncing: boolean; stacked?: boolean; }) { const group = update({ group: g })} />; const sort = update({ sort: s })} />; const viewSeg = ; const themeSeg = ; + const tabIds = tabs.map((t) => t.id); + const tabLabels = Object.fromEntries(tabs.map((t) => [t.id, t.label])); + const tabBar = tabs.length > 1 && ( + update({ tab })} /> + ); + const syncingChip = tabSyncing && ( + + codeowner queue syncing + + ); // Drawer: labeled full-width rows, so a mobile user can tell what each does. if (stacked) { return ( <> + {tabBar &&
tab{tabBar}{syncingChip}
}
group{group}
sort{sort}
view{viewSeg}
@@ -83,6 +101,8 @@ function Controls({ board's own plain buttons (the refresh button above, the selection bar's post/clear), which are not CopyButton instances. */} {canCopy && } + {tabBar} + {syncingChip} {group} {sort} {viewSeg} diff --git a/src/client/types.ts b/src/client/types.ts index f49c08d..70ea0e6 100644 --- a/src/client/types.ts +++ b/src/client/types.ts @@ -2,6 +2,7 @@ import type { MouseEvent } from "react"; import type { BoardMR } from "../data.ts"; import type { SlackTemplates } from "../template.ts"; import type { RespondStatus } from "../respond-outcome.ts"; +import type { TabConfig } from "../config.ts"; export interface RosterMember { username: string; @@ -64,6 +65,9 @@ export interface BoardData { dataSyncedAt: number | null; /** Authors this board demanded but rt hasn't finished backfilling yet. */ scopeUncovered: string[]; + /** Codeowners sections this board demanded but rt hasn't finished backfilling + yet -- drives the "codeowner queue syncing" badge on the matching tab. */ + scopeUncoveredSections: string[]; /** Narrowest sync window (days) among the daemon reads; null when none carried one. */ scopeWindowDays: number | null; /** The board's own configured stale cutoff (days), for comparing against @@ -75,6 +79,9 @@ export interface BoardData { /** Peering health: "ok" when the switchboard accepts us, "unauthorized" when it rejects us, null when this board isn't peering at all. */ peering: "ok" | "unauthorized" | null; + /** Board tabs, in display order. Always non-empty (config.tabs falls back to + IMPLICIT_TABS server-side). */ + tabs: TabConfig[]; } export type ThemeMode = "light" | "dark" | "system"; diff --git a/src/view.ts b/src/view.ts index 2bef110..55cd61a 100644 --- a/src/view.ts +++ b/src/view.ts @@ -1,6 +1,7 @@ import type { BoardMR } from "./data.ts"; import { hasChangesRequested } from "./data.ts"; import { projectKeyOf } from "./triage/stack.ts"; +import type { TabConfig } from "./config.ts"; export type GroupKey = "age" | "author" | "status" | "review"; export type SortKey = "oldest" | "progress"; @@ -153,6 +154,18 @@ export function filterByMember(mrs: BoardMR[], member: string): BoardMR[] { return member === "all" ? mrs : mrs.filter((m) => m.author.username === member); } +/** An authors tab passes every row through -- member filtering for it stays + downstream in filterByMember. A codeowners tab narrows to rows tagged with + its section; excludeMembers additionally drops the roster's own authors, so + the tab reads as the outside-the-team queue for that section. */ +export function filterByTab(mrs: BoardMR[], tab: TabConfig, members: Set): BoardMR[] { + if (tab.source.kind === "authors") return mrs; + const { section, excludeMembers } = tab.source; + return mrs.filter( + (mr) => mr.codeownerSections.includes(section) && (!excludeMembers || !members.has(mr.author.username)), + ); +} + /** Return a new array ordered by the chosen sort. Never mutates the input. */ export function sortMRs(mrs: BoardMR[], sort: SortKey): BoardMR[] { // Order by last activity (updatedAt) — the same axis the row's age token and @@ -317,16 +330,22 @@ export interface ViewState { member: string; group: GroupKey; sort: SortKey; + tab: string; } -export const DEFAULT_VIEW: ViewState = { member: "all", group: "age", sort: "oldest" }; +export const DEFAULT_VIEW: ViewState = { member: "all", group: "age", sort: "oldest", tab: "" }; -/** URL query params win, then stored localStorage values, then defaults. Invalid values are dropped. */ +/** URL query params win, then stored localStorage values, then defaults. Invalid + values are dropped. `validTabs` mirrors `validMembers`: an unknown or empty + tab (including "no tabs known yet", the state before /data.json's first + reply) resolves to the first configured tab, matching DEFAULT_VIEW.tab when + validTabs is empty. */ export function parseViewState( search: string, stored: Partial | null, validMembers: string[], defaultMember: string = "all", + validTabs: string[] = [], ): ViewState { const params = new URLSearchParams(search); const members = ["all", ...validMembers]; @@ -344,6 +363,7 @@ export function parseViewState( member: resolve("member", members, memberFallback), group: resolve("group", GROUP_KEYS, "age"), sort: resolve("sort", SORT_KEYS, "oldest"), + tab: resolve("tab", validTabs, validTabs[0] ?? ""), }; } @@ -382,6 +402,7 @@ export function serializeViewState(v: ViewState): string { if (v.member !== DEFAULT_VIEW.member) params.set("member", v.member); if (v.group !== DEFAULT_VIEW.group) params.set("group", v.group); if (v.sort !== DEFAULT_VIEW.sort) params.set("sort", v.sort); + if (v.tab !== DEFAULT_VIEW.tab) params.set("tab", v.tab); const s = params.toString(); return s ? `?${s}` : ""; } From 5ca863a6eb010e9b9d459c2f4adbe2f2a3e82faa Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Tue, 25 Aug 2026 20:47:19 -0500 Subject: [PATCH 04/11] fix: codeowners-tab layout collapse + clear selection on tab change .tui-app's unconditional 220px/1fr grid stranded .tui-main in the empty roster track once stopped rendering on a codeowners tab; a .tui-no-sidebar modifier drops to a single 1fr column instead. A tab switch is a differently-scoped queue, unlike member/group/sort, so update() now clears the live selection only when the tab id actually changes (tabChangeClearsSelection in selection.ts). Co-Authored-By: Claude Fable 5 --- src/__tests__/selection.test.ts | 18 +++++++++++++++++- src/client/board/Board.tsx | 8 ++++++-- src/selection.ts | 10 ++++++++++ src/style.css | 4 ++++ 4 files changed, 37 insertions(+), 3 deletions(-) diff --git a/src/__tests__/selection.test.ts b/src/__tests__/selection.test.ts index aef4c21..1c008c5 100644 --- a/src/__tests__/selection.test.ts +++ b/src/__tests__/selection.test.ts @@ -1,5 +1,5 @@ import { describe, test, expect } from "bun:test"; -import { selectionOf, postableOf } from "../selection.ts"; +import { selectionOf, postableOf, tabChangeClearsSelection } from "../selection.ts"; const a = { webUrl: "https://gl/a", iid: 1 }; const b = { webUrl: "https://gl/b", iid: 2 }; @@ -48,3 +48,19 @@ describe("postableOf", () => { expect(postableOf([orphan, a])).toEqual([a]); }); }); + +describe("tabChangeClearsSelection", () => { + test("an actual tab-id change clears the selection", () => { + expect(tabChangeClearsSelection({ tab: "q" }, "team")).toBe(true); + }); + + test("re-sending the already-active tab id is not a change", () => { + expect(tabChangeClearsSelection({ tab: "team" }, "team")).toBe(false); + }); + + test("a member/group/sort-only patch (no tab key) never clears it", () => { + expect(tabChangeClearsSelection({ member: "bob" }, "team")).toBe(false); + expect(tabChangeClearsSelection({ group: "status" }, "team")).toBe(false); + expect(tabChangeClearsSelection({ sort: "progress" }, "team")).toBe(false); + }); +}); diff --git a/src/client/board/Board.tsx b/src/client/board/Board.tsx index 25de565..28b7ac5 100644 --- a/src/client/board/Board.tsx +++ b/src/client/board/Board.tsx @@ -2,7 +2,7 @@ import { useCallback, useEffect, useRef, useState } from "react"; import type { BoardMR } from "../../data.ts"; import { filterByMember, filterByTab, sortMRs, groupMRs, parseViewState, serializeViewState, dataAgeLabel } from "../../view.ts"; import type { ViewState } from "../../view.ts"; -import { selectionOf, postableOf } from "../../selection.ts"; +import { selectionOf, postableOf, tabChangeClearsSelection } from "../../selection.ts"; import type { DraftInfo, BoardMRWithReview, @@ -68,12 +68,14 @@ export function Board() { setTheme(m); }; const update = (patch: Partial) => { + const clearsSelection = tabChangeClearsSelection(patch, state.tab); setState((prev) => { const next = { ...prev, ...patch }; localStorage.setItem(STATE_KEY, JSON.stringify(next)); history.replaceState(null, "", serializeViewState(next) || location.pathname); return next; }); + if (clearsSelection) setSelected(new Set()); }; // Re-resolve the view state's member against the roster the instant real @@ -489,7 +491,9 @@ export function Board() { }; return ( -
+
{/* Desktop roster (hidden on mobile, where it moves into the drawer). Also hidden on a codeowners tab: it isn't filtered by member, so the roster has nothing to drive. */} diff --git a/src/selection.ts b/src/selection.ts index ca53734..4876396 100644 --- a/src/selection.ts +++ b/src/selection.ts @@ -1,6 +1,7 @@ /** Board selection, kept out of client.tsx so it can be tested without a DOM. Selection is keyed by webUrl rather than by position, which is what lets it survive the refresh poll and any member/group/sort change. */ +import type { ViewState } from "./view.ts"; /** The selected MRs, in board order. A url that's no longer on the board just drops out -- MRs merge and leave while a selection is open, and that needs @@ -20,3 +21,12 @@ export function postableOf !!m.webUrl && !m.slack?.posted); } + +/** Whether a ViewState patch should drop the current selection. A codeowners + tab is a differently-scoped queue (its own Slack channel, in a later task), + so unlike a member/group/sort change -- which a selection deliberately + survives -- an actual tab-id change must not carry a selection across. + Re-sending the already-active tab id (a re-click) is not a change. */ +export function tabChangeClearsSelection(patch: Partial, currentTab: string): boolean { + return patch.tab !== undefined && patch.tab !== currentTab; +} diff --git a/src/style.css b/src/style.css index 2c032d6..0e3e612 100644 --- a/src/style.css +++ b/src/style.css @@ -332,6 +332,10 @@ /* team sidebar layout */ .tui-app { display: grid; grid-template-columns: 220px 1fr; gap: 20px; align-items: start; } .tui-app.tui-wide { max-width: 1400px; } +/* No rendered (a codeowners tab, scoped by section rather than + roster) -- drop the 220px track, or grid auto-placement strands the lone + .tui-main child in that empty first column instead of the 1fr one. */ +.tui-app.tui-no-sidebar { grid-template-columns: 1fr; } .tui-main { min-width: 0; } /* The roster gets the same framed-panel backdrop as the MR groups -- bare transparent buttons on the page grid disappear in dark mode. Item hover/ From 05ac635803c35ad16bf654953ca41bf9e1670920 Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Tue, 25 Aug 2026 20:58:18 -0500 Subject: [PATCH 05/11] slack: per-channel index; channelForMR routes reactions and resolves readIndex/writeIndex are now keyed by channel name (slack-index-.json), migrating the legacy single-channel index into the first channel that reads it. channelForMR (data.ts) routes a roster MR to config.slack.channel and a tagged stranger to its codeowners tab's slackChannel. server.ts's five config.slack.channel call sites (agent-signal reactions, /slack/resolve, /slack/post, the auto-resolve sweeper) now resolve per-MR via channelForMR, with /slack/resolve and /slack/post also accepting an optional validated channel body override. Co-Authored-By: Claude Fable 5 --- src/__tests__/board.test.ts | 68 ++++++++++++++++++++++++++++++++++++- src/__tests__/slack.test.ts | 54 ++++++++++++++++++++++++++++- src/data.ts | 27 +++++++++++++++ src/server.ts | 30 ++++++++++++---- src/slack.ts | 47 +++++++++++++++++++------ 5 files changed, 207 insertions(+), 19 deletions(-) diff --git a/src/__tests__/board.test.ts b/src/__tests__/board.test.ts index c7a0f27..b8d83ac 100644 --- a/src/__tests__/board.test.ts +++ b/src/__tests__/board.test.ts @@ -1,6 +1,16 @@ import { describe, expect, test } from "bun:test"; import type { PullRequest } from "@mattstack/glance"; -import { aggregateSyncScope, boardDemand, buildBoard, buildRoster, projectPathFromWebUrl, stripDraftPrefix, type BoardMR } from "../data.ts"; +import { + aggregateSyncScope, + boardDemand, + buildBoard, + buildRoster, + channelForMR, + configuredSlackChannels, + projectPathFromWebUrl, + stripDraftPrefix, + type BoardMR, +} from "../data.ts"; import { SnapshotCache, type FetchResult } from "../cache.ts"; import { DEFAULT_SLACK_EMOJI, IMPLICIT_TABS, type BoardConfig, type TabConfig } from "../config.ts"; import { extractTicketId } from "../ticket.ts"; @@ -350,6 +360,62 @@ describe("boardDemand", () => { }); }); +describe("channelForMR", () => { + const tabsWithSlackChannel: TabConfig[] = [ + { id: "t", label: "T", source: { kind: "authors" } }, + { id: "q", label: "Q", source: { kind: "codeowners", section: "Acme", excludeMembers: true }, slackChannel: "team-codeowners" }, + ]; + const withSlackTab: BoardConfig = { ...config, tabs: tabsWithSlackChannel }; + + test("routes a roster MR to the default channel", () => { + expect( + channelForMR(withSlackTab, { author: { id: "gitlab:1", username: "alice", name: "Alice", avatarUrl: null }, codeownerSections: [] }), + ).toBe("code-review"); + }); + + test("routes a tagged stranger to their codeowners tab's channel", () => { + expect( + channelForMR(withSlackTab, { + author: { id: "gitlab:2", username: "outsider", name: "Outsider", avatarUrl: null }, + codeownerSections: ["Acme"], + }), + ).toBe("team-codeowners"); + }); + + test("falls back to the default channel when a stranger's tags match no tab's slackChannel", () => { + expect( + channelForMR(withSlackTab, { + author: { id: "gitlab:3", username: "outsider", name: "Outsider", avatarUrl: null }, + codeownerSections: ["SomeOtherSection"], + }), + ).toBe("code-review"); + }); + + test("a stranger with no tags falls back to the default channel", () => { + expect( + channelForMR(withSlackTab, { author: { id: "gitlab:4", username: "outsider", name: "Outsider", avatarUrl: null }, codeownerSections: [] }), + ).toBe("code-review"); + }); +}); + +describe("configuredSlackChannels", () => { + test("is the default channel plus every distinct tab slackChannel", () => { + const withTabs: BoardConfig = { + ...config, + tabs: [ + { id: "t", label: "T", source: { kind: "authors" } }, + { id: "q", label: "Q", source: { kind: "codeowners", section: "Acme" }, slackChannel: "team-codeowners" }, + { id: "r", label: "R", source: { kind: "codeowners", section: "Billing" }, slackChannel: "team-codeowners" }, + ], + }; + expect(configuredSlackChannels(withTabs).sort()).toEqual(["code-review", "team-codeowners"]); + }); + + test("is just the default channel when no tab overrides it", () => { + expect(configuredSlackChannels(config)).toEqual(["code-review"]); + }); +}); + describe("aggregateSyncScope", () => { test("dataSyncedAt is the min syncedAt across reads", () => { const agg = aggregateSyncScope([{ syncedAt: 300 }, { syncedAt: 100 }, { syncedAt: 200 }]); diff --git a/src/__tests__/slack.test.ts b/src/__tests__/slack.test.ts index 640f454..2576809 100644 --- a/src/__tests__/slack.test.ts +++ b/src/__tests__/slack.test.ts @@ -1,13 +1,20 @@ -import { describe, expect, test } from "bun:test"; +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { mkdtempSync, rmSync, existsSync, writeFileSync } from "fs"; +import { join } from "path"; +import { tmpdir } from "os"; import { buildPermalink, buildThreadPermalink, extractMrUrls, matchReviewMessage, slackRefPath, + slackIndexPath, + readIndex, + writeIndex, attachSlack, type SlackMessage, type SlackRef, + type SlackIndex, } from "../slack.ts"; const URL_A = "https://gitlab.com/acme/webapp/-/merge_requests/4821"; @@ -78,6 +85,51 @@ describe("slackRefPath", () => { }); }); +describe("slackIndexPath", () => { + test("is stable per channel and distinct across channels", () => { + expect(slackIndexPath("code-review")).not.toBe(slackIndexPath("team-codeowners")); + expect(slackIndexPath("code-review")).toBe(slackIndexPath("code-review")); + }); + + test("slugs unsafe characters and lives under the given dir", () => { + expect(slackIndexPath("pod/weird name!", "/s")).toBe("/s/slack-index-pod-weird-name-.json"); + }); +}); + +describe("readIndex / writeIndex per-channel migration", () => { + let dir: string; + beforeEach(() => { dir = mkdtempSync(join(tmpdir(), "slack-idx-")); }); + afterEach(() => { rmSync(dir, { recursive: true, force: true }); }); + + const legacy: SlackIndex = { channelId: "C1", teamDomain: "acme.slack.com", lastTs: "100.0", messages: [] }; + + test("migrates the legacy single-channel index to the first channel that reads it", () => { + writeFileSync(join(dir, "slack-index.json"), JSON.stringify(legacy)); + const migrated = readIndex("code-review", dir); + expect(migrated).toEqual(legacy); + expect(existsSync(join(dir, "slack-index.json"))).toBe(false); // legacy consumed + expect(existsSync(slackIndexPath("code-review", dir))).toBe(true); + }); + + test("a second channel reading after the legacy file is consumed starts fresh", () => { + writeFileSync(join(dir, "slack-index.json"), JSON.stringify(legacy)); + readIndex("code-review", dir); // consumes the legacy file + expect(readIndex("team-codeowners", dir)).toBeNull(); + }); + + test("with no legacy file, a fresh install just starts with null per channel", () => { + expect(readIndex("code-review", dir)).toBeNull(); + expect(readIndex("team-codeowners", dir)).toBeNull(); + }); + + test("writeIndex writes to the channel-specific path, independent of other channels", () => { + const idx: SlackIndex = { channelId: "C2", teamDomain: "acme.slack.com", lastTs: "200.0", messages: [] }; + writeIndex("team-codeowners", idx, dir); + expect(readIndex("team-codeowners", dir)).toEqual(idx); + expect(readIndex("code-review", dir)).toBeNull(); + }); +}); + describe("attachSlack", () => { test("attaches the client slice by webUrl, leaves others untouched", () => { const refs = new Map([ diff --git a/src/data.ts b/src/data.ts index cd51e40..6e3e2a3 100644 --- a/src/data.ts +++ b/src/data.ts @@ -198,6 +198,33 @@ export function boardDemand(config: BoardConfig, port: number): DemandDecl { }; } +/** Which Slack channel an MR's review-request lives in: roster MRs use the + board channel; a tag-only row uses its codeowners tab's channel (first + match in tab order), falling back to the board channel when none of its + tags carry a slackChannel. */ +export function channelForMR(config: BoardConfig, mr: Pick): string { + const isMember = config.members.some((m) => m.username === mr.author.username); + if (!isMember) { + for (const tab of config.tabs) { + if (tab.source.kind === "codeowners" && tab.slackChannel && mr.codeownerSections.includes(tab.source.section)) { + return tab.slackChannel; + } + } + } + return config.slack.channel; +} + +/** Every Slack channel this board's config can route a review-request to: + the default channel plus every tab-level override, deduped. Used to + validate a client-supplied channel override. */ +export function configuredSlackChannels(config: Pick): string[] { + const channels = new Set([config.slack.channel]); + for (const tab of config.tabs) { + if (tab.slackChannel) channels.add(tab.slackChannel); + } + return [...channels]; +} + /** One project's sync facts, the shape aggregateSyncScope folds across projects. */ export interface SyncScopeRead { syncedAt: number; diff --git a/src/server.ts b/src/server.ts index 0c739bb..d6b585f 100644 --- a/src/server.ts +++ b/src/server.ts @@ -10,7 +10,7 @@ import { loadConfig, loadGitLabToken, loadSlackToken, loadSwitchboardToken, load import { memoizeAsync } from "./memoize-async.ts"; import { resolveBoardSkill, type BoardSkillKind } from "./manifest-bindings.ts"; import { upsertEnvKeys } from "./env-file.ts"; -import { aggregateSyncScope, boardDemand, buildBoard, buildRoster, projectPathFromWebUrl, type BoardMR, type SyncScopeRead } from "./data.ts"; +import { aggregateSyncScope, boardDemand, buildBoard, buildRoster, channelForMR, configuredSlackChannels, projectPathFromWebUrl, type BoardMR, type SyncScopeRead } from "./data.ts"; import { GitLabProvider, ReadBackFailedError, NoteMutator, parseRepoId } from "@mattstack/glance"; import { summarizeDiscussions, threadStatusCounts, unresolvedReviewerCount } from "./discussions.ts"; import { readProjectMRs, readDiscussions, subscribe } from "@mattstack/rt-client"; @@ -920,9 +920,12 @@ const httpServer = Bun.serve({ // The sweeper usually resolves the ref first, but a review launched and // finished inside one sweep interval can beat it here. try { + const signalSnapshot = await cache.get(); + const signalMr = signalSnapshot.mrs.find((m) => m.webUrl === signal.mrUrl); + const signalChannel = signalMr ? channelForMR(config, signalMr) : config.slack.channel; const existing = readSlackRefs().get(signal.mrUrl); if (existing?.status !== "found" || !existing.messageTs) { - await resolveSlackRef(slackToken, config.slack.channel, signal.mrUrl, signal.iid); + await resolveSlackRef(slackToken, signalChannel, signal.mrUrl, signal.iid); } const ref = await reactToMR(slackToken, signal.mrUrl, emoji); return new Response(JSON.stringify({ ok: true, reacted: true, reactions: ref.reactions ?? [] }), { @@ -1172,8 +1175,16 @@ const httpServer = Bun.serve({ } const parsed = parseReviewRequestBody(body); if (!parsed) return new Response("expected { mrUrl: string, iid: number }", { status: 400 }); + const { channel } = (body ?? {}) as { channel?: unknown }; + const allowedChannels = configuredSlackChannels(config); + if (channel !== undefined && (typeof channel !== "string" || !allowedChannels.includes(channel))) { + return new Response(`"channel" must be one of ${allowedChannels.join(", ")}`, { status: 400 }); + } try { - const ref = await resolveSlackRef(slackToken, config.slack.channel, parsed.mrUrl, parsed.iid); + const snapshot = await cache.get(); + const mr = snapshot.mrs.find((m) => m.webUrl === parsed.mrUrl); + const resolvedChannel = typeof channel === "string" ? channel : mr ? channelForMR(config, mr) : config.slack.channel; + const ref = await resolveSlackRef(slackToken, resolvedChannel, parsed.mrUrl, parsed.iid); return new Response( JSON.stringify({ ok: true, status: ref.status, permalink: ref.permalink, reactions: ref.reactions ?? [] }), { headers: { "content-type": "application/json" } }, @@ -1200,7 +1211,7 @@ const httpServer = Bun.serve({ } catch { return new Response("invalid json", { status: 400 }); } - const { mrUrls, header } = (body ?? {}) as { mrUrls?: unknown; header?: unknown }; + const { mrUrls, header, channel } = (body ?? {}) as { mrUrls?: unknown; header?: unknown; channel?: unknown }; if (!Array.isArray(mrUrls) || mrUrls.length === 0 || !mrUrls.every((u) => typeof u === "string")) { return new Response("expected { mrUrls: string[] }", { status: 400 }); } @@ -1208,6 +1219,11 @@ const httpServer = Bun.serve({ if (header !== undefined && headerOverride === null) { return new Response(`"header" must be a non-empty string of at most ${MAX_HEADER_LEN} characters`, { status: 400 }); } + const allowedChannels = configuredSlackChannels(config); + if (channel !== undefined && (typeof channel !== "string" || !allowedChannels.includes(channel))) { + return new Response(`"channel" must be one of ${allowedChannels.join(", ")}`, { status: 400 }); + } + const targetChannel = typeof channel === "string" ? channel : config.slack.channel; const snapshot = await cache.get(); const byUrl = new Map(snapshot.mrs.map((m) => [m.webUrl, m] as const)); const picked = (mrUrls as string[]).map((u) => byUrl.get(u)).filter((m): m is BoardMR => !!m); @@ -1223,7 +1239,7 @@ const httpServer = Bun.serve({ const freshlyFound: Array<{ iid: number; permalink?: string }> = []; try { for (const m of toResolve) { - const ref = await resolveSlackRef(slackToken, config.slack.channel, m.webUrl!, m.iid); + const ref = await resolveSlackRef(slackToken, targetChannel, m.webUrl!, m.iid); if (ref.status === "found") freshlyFound.push({ iid: m.iid, permalink: ref.permalink }); } } catch (err) { @@ -1268,7 +1284,7 @@ const httpServer = Bun.serve({ try { const refs = await postToSlack( slackToken, - config.slack.channel, + targetChannel, text, picked.map((m) => ({ webUrl: m.webUrl!, iid: m.iid })), ); @@ -1365,7 +1381,7 @@ async function autoResolveSlackRefs(): Promise { if (!targets.length) return; for (const mr of targets) { try { - await resolveSlackRef(slackToken, config.slack.channel, mr.webUrl!, mr.iid); + await resolveSlackRef(slackToken, channelForMR(config, mr), mr.webUrl!, mr.iid); } catch (err) { console.error(`auto-resolve !${mr.iid} failed: ${err instanceof Error ? err.message : err}`); } diff --git a/src/slack.ts b/src/slack.ts index 0af8470..7fc2c77 100644 --- a/src/slack.ts +++ b/src/slack.ts @@ -1,10 +1,11 @@ -import { readFileSync, writeFileSync, mkdirSync, readdirSync, existsSync } from "fs"; +import { readFileSync, writeFileSync, mkdirSync, readdirSync, existsSync, renameSync } from "fs"; import { join } from "path"; import { APP_ROOT } from "./app-root.ts"; const STATE_ROOT = join(APP_ROOT, "state"); export const SLACK_REF_DIR = join(STATE_ROOT, "slack"); -const INDEX_PATH = join(STATE_ROOT, "slack-index.json"); +/** Pre-tabs installs had exactly one channel and one index at this path. */ +const LEGACY_INDEX_NAME = "slack-index.json"; /** How far back the first index build reaches. Review requests older than the board's stale window are irrelevant, so we never page the whole channel. */ @@ -124,17 +125,43 @@ async function resolveChannelId(token: string, channelName: string): Promise { - const existing = readIndex(); + const existing = readIndex(channelName); const channelId = existing?.channelId ?? (await resolveChannelId(token, channelName)); const domain = existing?.teamDomain ?? (await teamDomain(token)); const oldest = existing?.lastTs && existing.lastTs !== "0" @@ -170,7 +197,7 @@ export async function syncIndex(token: string, channelName: string, now: number const merged = [...(existing?.messages ?? []), ...fresh]; const lastTs = merged.reduce((max, m) => (parseFloat(m.ts) > parseFloat(max) ? m.ts : max), existing?.lastTs ?? "0"); const index: SlackIndex = { channelId, teamDomain: domain, lastTs, messages: merged }; - writeIndex(index); + writeIndex(channelName, index); return index; } @@ -235,7 +262,7 @@ export async function postToSlack( now: number = Date.now(), ): Promise { if (!mrs.length) throw new Error("nothing to post"); - const existing = readIndex(); + const existing = readIndex(channelName); const channelId = existing?.channelId ?? (await resolveChannelId(token, channelName)); const domain = existing?.teamDomain ?? (await teamDomain(token)); const ts = await postMessage(token, channelId, text); From 6ece9b5ad1a2b18438f6b5baf7c15c619381471e Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Tue, 25 Aug 2026 21:09:52 -0500 Subject: [PATCH 06/11] slack: id-verify legacy index adoption; endpoint channel-validation tests The prior channel-agnostic migration let whichever channel synced first after upgrade inherit the legacy index's channelId unconditionally, so a codeowners-tab channel winning the race would permanently read/post/react against the actual default channel with no error. readIndex no longer touches the legacy file; syncIndex/postToSlack now resolve the requested channel's real id first and only adopt the legacy file (adoptLegacyIndex) when its cached channelId matches, leaving it in place otherwise for the channel it actually belongs to. Also adds endpoint-level tests (server-slack-channel.test.ts) proving /slack/resolve and /slack/post 400 on a channel body value outside the configured set, using the existing subprocess-boot test seam. Co-Authored-By: Claude Fable 5 --- src/__tests__/server-slack-channel.test.ts | 94 ++++++++++++++++++++++ src/__tests__/slack.test.ts | 70 ++++++++++++---- src/slack.ts | 66 ++++++++++----- 3 files changed, 194 insertions(+), 36 deletions(-) create mode 100644 src/__tests__/server-slack-channel.test.ts diff --git a/src/__tests__/server-slack-channel.test.ts b/src/__tests__/server-slack-channel.test.ts new file mode 100644 index 0000000..ede3828 --- /dev/null +++ b/src/__tests__/server-slack-channel.test.ts @@ -0,0 +1,94 @@ +import { afterAll, expect, test } from "bun:test"; +import { join } from "path"; +import { mkdirSync, mkdtempSync, writeFileSync } from "fs"; +import { tmpdir } from "os"; + +// Proves /slack/resolve and /slack/post reject a `channel` body value outside +// the configured set (config.slack.channel + every tab's slackChannel, see +// configuredSlackChannels in data.ts) with a 400 before ever touching Slack +// or GitLab. Real (non-fixture) boot -- fixture mode always reports slack as +// unconfigured (server.ts's getSlackToken short-circuits to null under +// BOARD_FIXTURE), which would hit the earlier "slack not configured" 400 +// before ever reaching the channel check this test targets. A fake $HOME +// with a team settings store (mirrors server-healthz-fast.test.ts) plus a +// non-empty SLACK_TOKEN gets past that gate without a real Slack workspace; +// the channel-validation 400 fires before either handler calls cache.get() +// or makes any Slack API call, so no live GitLab/Slack connectivity is +// needed for this assertion to hold. +const fakeHome = mkdtempSync(join(tmpdir(), "board-slack-channel-")); + +const teamDir = join(fakeHome, ".mattstack", "teams", "testteam", "mattstack"); +mkdirSync(teamDir, { recursive: true }); +writeFileSync( + join(teamDir, "settings.team.jsonc"), + JSON.stringify({ + "board.gitlabHost": "https://gitlab.example.com", + "board.projects": ["g/p"], + "board.members": [{ username: "alice" }], + }), +); + +const PORT = 47945; +const proc = Bun.spawn(["bun", "run", join(import.meta.dir, "..", "server.ts")], { + env: { + ...process.env, + HOME: fakeHome, + // Keeps the booted server from writing state/board-port into the repo. + BOARD_APP_ROOT: fakeHome, + PORT: String(PORT), + GITLAB_TOKEN: "", + // Truthy but fake: only needs to clear the `!slackToken` gate. Both + // endpoints reject an unconfigured `channel` before making any real + // Slack API call, so this token is never actually used over the wire. + SLACK_TOKEN: "fake-slack-token", + SWITCHBOARD_TOKEN: "", + SWITCHBOARD_ADMIN_TOKEN: "", + }, + stdout: "pipe", + stderr: "pipe", +}); + +afterAll(() => proc.kill()); + +async function ready(): Promise { + for (let i = 0; i < 100; i++) { + try { + if ((await fetch(`http://127.0.0.1:${PORT}/healthz`)).ok) return; + } catch { + // server not listening yet + } + await new Promise((r) => setTimeout(r, 100)); + } + throw new Error("server never came up"); +} + +test("/slack/resolve rejects a channel outside the configured set", async () => { + await ready(); + const res = await fetch(`http://127.0.0.1:${PORT}/slack/resolve`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + mrUrl: "https://gitlab.example.com/g/p/-/merge_requests/1", + iid: 1, + channel: "not-a-real-channel", + }), + }); + expect(res.status).toBe(400); + // "code-review" is the only configured channel (DEFAULT_SLACK.channel; no + // board.tabs override in this fake store) -- named in the error body. + expect(await res.text()).toContain("code-review"); +}, 15_000); + +test("/slack/post rejects a channel outside the configured set", async () => { + await ready(); + const res = await fetch(`http://127.0.0.1:${PORT}/slack/post`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + mrUrls: ["https://gitlab.example.com/g/p/-/merge_requests/1"], + channel: "not-a-real-channel", + }), + }); + expect(res.status).toBe(400); + expect(await res.text()).toContain("code-review"); +}, 15_000); diff --git a/src/__tests__/slack.test.ts b/src/__tests__/slack.test.ts index 2576809..742f96e 100644 --- a/src/__tests__/slack.test.ts +++ b/src/__tests__/slack.test.ts @@ -9,8 +9,10 @@ import { matchReviewMessage, slackRefPath, slackIndexPath, + legacyIndexPath, readIndex, writeIndex, + adoptLegacyIndex, attachSlack, type SlackMessage, type SlackRef, @@ -96,25 +98,16 @@ describe("slackIndexPath", () => { }); }); -describe("readIndex / writeIndex per-channel migration", () => { +describe("readIndex / writeIndex per-channel", () => { let dir: string; beforeEach(() => { dir = mkdtempSync(join(tmpdir(), "slack-idx-")); }); afterEach(() => { rmSync(dir, { recursive: true, force: true }); }); - const legacy: SlackIndex = { channelId: "C1", teamDomain: "acme.slack.com", lastTs: "100.0", messages: [] }; - - test("migrates the legacy single-channel index to the first channel that reads it", () => { - writeFileSync(join(dir, "slack-index.json"), JSON.stringify(legacy)); - const migrated = readIndex("code-review", dir); - expect(migrated).toEqual(legacy); - expect(existsSync(join(dir, "slack-index.json"))).toBe(false); // legacy consumed - expect(existsSync(slackIndexPath("code-review", dir))).toBe(true); - }); - - test("a second channel reading after the legacy file is consumed starts fresh", () => { - writeFileSync(join(dir, "slack-index.json"), JSON.stringify(legacy)); - readIndex("code-review", dir); // consumes the legacy file - expect(readIndex("team-codeowners", dir)).toBeNull(); + test("readIndex never touches the legacy file -- adoption is a separate, id-verified step", () => { + const legacy: SlackIndex = { channelId: "C1", teamDomain: "acme.slack.com", lastTs: "100.0", messages: [] }; + writeFileSync(legacyIndexPath(dir), JSON.stringify(legacy)); + expect(readIndex("code-review", dir)).toBeNull(); + expect(existsSync(legacyIndexPath(dir))).toBe(true); // untouched }); test("with no legacy file, a fresh install just starts with null per channel", () => { @@ -130,6 +123,53 @@ describe("readIndex / writeIndex per-channel migration", () => { }); }); +describe("adoptLegacyIndex", () => { + let dir: string; + beforeEach(() => { dir = mkdtempSync(join(tmpdir(), "slack-idx-")); }); + afterEach(() => { rmSync(dir, { recursive: true, force: true }); }); + + // The legacy index's channelId is the real Slack id of whichever channel a + // pre-tabs install was actually using -- here, "code-review"'s. + const legacy: SlackIndex = { channelId: "C1", teamDomain: "acme.slack.com", lastTs: "100.0", messages: [] }; + + test("adopts when the resolved channel id matches (the default channel winning the race)", () => { + writeFileSync(legacyIndexPath(dir), JSON.stringify(legacy)); + const adopted = adoptLegacyIndex("code-review", "C1", dir); + expect(adopted).toEqual(legacy); + expect(existsSync(legacyIndexPath(dir))).toBe(false); // consumed + expect(readIndex("code-review", dir)).toEqual(legacy); + }); + + test("refuses when the resolved channel id does not match (a codeowners tab winning the race), leaving the legacy file for its real owner", () => { + writeFileSync(legacyIndexPath(dir), JSON.stringify(legacy)); + // team-codeowners's own resolved id ("C2") is not code-review's ("C1"). + const adopted = adoptLegacyIndex("team-codeowners", "C2", dir); + expect(adopted).toBeNull(); + expect(readIndex("team-codeowners", dir)).toBeNull(); // no fresh file written by adoptLegacyIndex itself + expect(existsSync(legacyIndexPath(dir))).toBe(true); // legacy file untouched, still there for code-review + expect(readIndex("code-review", dir)).toBeNull(); // not adopted for code-review either -- still pending + + // The default channel can still adopt it afterward. + const laterAdopted = adoptLegacyIndex("code-review", "C1", dir); + expect(laterAdopted).toEqual(legacy); + expect(existsSync(legacyIndexPath(dir))).toBe(false); + expect(readIndex("code-review", dir)).toEqual(legacy); + }); + + test("returns null with no legacy file to adopt", () => { + expect(adoptLegacyIndex("code-review", "C1", dir)).toBeNull(); + }); + + test("returns null (does not overwrite) when the channel already has its own index", () => { + const own: SlackIndex = { channelId: "C1", teamDomain: "acme.slack.com", lastTs: "50.0", messages: [] }; + writeIndex("code-review", own, dir); + writeFileSync(legacyIndexPath(dir), JSON.stringify(legacy)); + expect(adoptLegacyIndex("code-review", "C1", dir)).toBeNull(); + expect(readIndex("code-review", dir)).toEqual(own); // untouched + expect(existsSync(legacyIndexPath(dir))).toBe(true); // legacy left alone too + }); +}); + describe("attachSlack", () => { test("attaches the client slice by webUrl, leaves others untouched", () => { const refs = new Map([ diff --git a/src/slack.ts b/src/slack.ts index 7fc2c77..85a5653 100644 --- a/src/slack.ts +++ b/src/slack.ts @@ -133,27 +133,17 @@ export function slackIndexPath(channelName: string, dir: string = STATE_ROOT): s return join(dir, `slack-index-${slug}.json`); } -/** - * Read a channel's index, migrating the legacy single-channel index into - * place the first time any channel asks for it. The rename consumes the - * legacy file, so at most one channel ever inherits that history -- in - * practice the pre-tabs default channel, since it's the one every existing - * install already keeps syncing. A codeowners tab's channel is new with - * tabs, so it has no history to lose by starting fresh. - */ +/** Where the pre-tabs single-channel index lives, if this install predates tabs. */ +export function legacyIndexPath(dir: string = STATE_ROOT): string { + return join(dir, LEGACY_INDEX_NAME); +} + +/** Read a channel's own index. No legacy fallback here -- adoption needs a + resolved channel id to verify against (see adoptLegacyIndex), which this + plain fs read has no way to obtain. */ export function readIndex(channelName: string, dir: string = STATE_ROOT): SlackIndex | null { - const path = slackIndexPath(channelName, dir); - const legacyPath = join(dir, LEGACY_INDEX_NAME); - if (!existsSync(path) && existsSync(legacyPath)) { - try { - renameSync(legacyPath, path); - } catch { - // Lost a rename race with another process's migration; read whatever - // landed at `path` below rather than fail the read. - } - } try { - return JSON.parse(readFileSync(path, "utf8")) as SlackIndex; + return JSON.parse(readFileSync(slackIndexPath(channelName, dir), "utf8")) as SlackIndex; } catch { return null; } @@ -164,14 +154,47 @@ export function writeIndex(channelName: string, index: SlackIndex, dir: string = writeFileSync(slackIndexPath(channelName, dir), JSON.stringify(index, null, 2) + "\n"); } +/** + * Adopt the legacy single-channel index for `channelName`, but only when its + * cached channelId actually matches `resolvedChannelId` -- the id the caller + * just resolved by name from Slack. The legacy file predates tabs and stores + * no channel name, only a channelId, so an unverified adoption would let + * whichever channel asks first inherit a DIFFERENT channel's id and silently + * operate against the wrong Slack channel from then on. A mismatch leaves + * the legacy file untouched so the channel it actually belongs to can still + * adopt it later; the caller starts fresh with `resolvedChannelId` instead. + */ +export function adoptLegacyIndex(channelName: string, resolvedChannelId: string, dir: string = STATE_ROOT): SlackIndex | null { + const path = slackIndexPath(channelName, dir); + const legacyPath = legacyIndexPath(dir); + if (existsSync(path) || !existsSync(legacyPath)) return null; + let legacy: SlackIndex; + try { + legacy = JSON.parse(readFileSync(legacyPath, "utf8")) as SlackIndex; + } catch { + return null; + } + if (legacy.channelId !== resolvedChannelId) return null; + try { + renameSync(legacyPath, path); + } catch { + // Lost a rename race with another process's adoption; the file now at + // `path` (written by whoever won) is still correct for this channel. + } + return legacy; +} + /** * Bring the local channel index up to date and return it. First run seeds from * the last INITIAL_LOOKBACK_DAYS; later runs fetch only messages after lastTs. * Message text/ts never change once posted, so the index only ever grows. */ export async function syncIndex(token: string, channelName: string, now: number = Date.now()): Promise { - const existing = readIndex(channelName); + let existing = readIndex(channelName); const channelId = existing?.channelId ?? (await resolveChannelId(token, channelName)); + // No per-channel index yet: the resolved id above lets us verify (not just + // assume) whether the legacy single-channel index actually belongs here. + if (!existing) existing = adoptLegacyIndex(channelName, channelId); const domain = existing?.teamDomain ?? (await teamDomain(token)); const oldest = existing?.lastTs && existing.lastTs !== "0" ? existing.lastTs @@ -262,8 +285,9 @@ export async function postToSlack( now: number = Date.now(), ): Promise { if (!mrs.length) throw new Error("nothing to post"); - const existing = readIndex(channelName); + let existing = readIndex(channelName); const channelId = existing?.channelId ?? (await resolveChannelId(token, channelName)); + if (!existing) existing = adoptLegacyIndex(channelName, channelId); const domain = existing?.teamDomain ?? (await teamDomain(token)); const ts = await postMessage(token, channelId, text); const multi = mrs.length > 1; From 5f2f9b8945dc4086d0a2939b59b2ec04baf395dc Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Tue, 25 Aug 2026 21:14:32 -0500 Subject: [PATCH 07/11] review launch: per-tab reviewSkill override, tabId in the launch payload --- src/__tests__/board.test.ts | 14 ++++++++++++++ src/client/board/Board.tsx | 6 +++--- src/data.ts | 14 ++++++++++++++ src/server.ts | 7 ++++--- 4 files changed, 35 insertions(+), 6 deletions(-) diff --git a/src/__tests__/board.test.ts b/src/__tests__/board.test.ts index b8d83ac..45e91a5 100644 --- a/src/__tests__/board.test.ts +++ b/src/__tests__/board.test.ts @@ -8,6 +8,7 @@ import { channelForMR, configuredSlackChannels, projectPathFromWebUrl, + reviewSkillForTab, stripDraftPrefix, type BoardMR, } from "../data.ts"; @@ -416,6 +417,19 @@ describe("configuredSlackChannels", () => { }); }); +describe("reviewSkillForTab", () => { + test("prefers the tab's reviewSkill and falls back to normal resolution", () => { + const cfg = { ...config, tabs: [ + { id: "t", label: "T", source: { kind: "authors" as const } }, + { id: "q", label: "Q", source: { kind: "codeowners" as const, section: "Acme" }, reviewSkill: "external:review" }, + ] }; + const fallback = () => "acme:review"; + expect(reviewSkillForTab(cfg, "q", "u", fallback)).toBe("external:review"); + expect(reviewSkillForTab(cfg, "t", "u", fallback)).toBe("acme:review"); + expect(reviewSkillForTab(cfg, undefined, "u", fallback)).toBe("acme:review"); + }); +}); + describe("aggregateSyncScope", () => { test("dataSyncedAt is the min syncedAt across reads", () => { const agg = aggregateSyncScope([{ syncedAt: 300 }, { syncedAt: 100 }, { syncedAt: 200 }]); diff --git a/src/client/board/Board.tsx b/src/client/board/Board.tsx index 28b7ac5..993f743 100644 --- a/src/client/board/Board.tsx +++ b/src/client/board/Board.tsx @@ -208,15 +208,15 @@ export function Board() { axis: "review", path: "/review", verbing: "launching review", noun: "review", optimistic: optimisticLifecycle, addToast, reload: load, }); - const handleLaunch = useCallback((mr: BoardMR, note?: string) => launchReview(mr, {}, note), [launchReview]); + const handleLaunch = useCallback((mr: BoardMR, note?: string) => launchReview(mr, { tabId: state.tab }, note), [launchReview, state.tab]); const reReviewAction = useLaunchAction({ axis: "review", path: "/review", verbing: "re-reviewing", noun: "review", optimistic: optimisticLifecycle, addToast, reload: load, }); const handleReReview = useCallback( - (mr: BoardMR, note?: string) => reReviewAction(mr, { reReview: true }, note), - [reReviewAction], + (mr: BoardMR, note?: string) => reReviewAction(mr, { reReview: true, tabId: state.tab }, note), + [reReviewAction, state.tab], ); const respondAction = useLaunchAction({ diff --git a/src/data.ts b/src/data.ts index 6e3e2a3..98a5924 100644 --- a/src/data.ts +++ b/src/data.ts @@ -281,3 +281,17 @@ export function buildRoster(members: Member[], mrs: BoardMR[], names: Map mr.author.username === member.username).length, })); } + +export function reviewSkillForTab( + config: BoardConfig, + tabId: string | undefined, + mrUrl: string, + fallback: (kind: "review", mrUrl: string) => string, +): string { + const tab = tabId ? config.tabs.find((t) => t.id === tabId) : undefined; + if (tab?.reviewSkill) { + console.log(`review skill: ${tab.reviewSkill} (tab ${tab.id})`); + return tab.reviewSkill; + } + return fallback("review", mrUrl); +} diff --git a/src/server.ts b/src/server.ts index d6b585f..e1414ac 100644 --- a/src/server.ts +++ b/src/server.ts @@ -10,7 +10,7 @@ import { loadConfig, loadGitLabToken, loadSlackToken, loadSwitchboardToken, load import { memoizeAsync } from "./memoize-async.ts"; import { resolveBoardSkill, type BoardSkillKind } from "./manifest-bindings.ts"; import { upsertEnvKeys } from "./env-file.ts"; -import { aggregateSyncScope, boardDemand, buildBoard, buildRoster, channelForMR, configuredSlackChannels, projectPathFromWebUrl, type BoardMR, type SyncScopeRead } from "./data.ts"; +import { aggregateSyncScope, boardDemand, buildBoard, buildRoster, channelForMR, configuredSlackChannels, projectPathFromWebUrl, reviewSkillForTab, type BoardMR, type SyncScopeRead } from "./data.ts"; import { GitLabProvider, ReadBackFailedError, NoteMutator, parseRepoId } from "@mattstack/glance"; import { summarizeDiscussions, threadStatusCounts, unresolvedReviewerCount } from "./discussions.ts"; import { readProjectMRs, readDiscussions, subscribe } from "@mattstack/rt-client"; @@ -628,6 +628,7 @@ const httpServer = Bun.serve({ if (!parsed) return new Response("expected { mrUrl: string, iid: number }", { status: 400 }); const resume = (body as { resume?: unknown })?.resume === true; const reReview = (body as { reReview?: unknown })?.reReview === true; + const tabId = (body as { tabId?: unknown })?.tabId; const noteParse = parseLaunchNote(body); if (!noteParse.ok) return new Response(noteParse.error, { status: 400 }); const note = noteParse.note; @@ -655,7 +656,7 @@ const httpServer = Bun.serve({ void launchReReview(parsed.mrUrl, parsed.iid, { cwd: config.reviewCwd, workspaceLabel: config.reviewsWorkspace, - skill: resolveLaunchSkill("review", parsed.mrUrl), + skill: reviewSkillForTab(config, typeof tabId === "string" ? tabId : undefined, parsed.mrUrl, resolveLaunchSkill), author, claudeCommand: config.claudeCommand, note, @@ -691,7 +692,7 @@ const httpServer = Bun.serve({ cwd: config.reviewCwd, workspaceLabel: config.reviewsWorkspace, statePath, - skill: resolveLaunchSkill("review", parsed.mrUrl), + skill: reviewSkillForTab(config, typeof tabId === "string" ? tabId : undefined, parsed.mrUrl, resolveLaunchSkill), author, claudeCommand: config.claudeCommand, note, From b4d0b5163fa927b5c1abede83be0a67ee0b0daee Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Tue, 25 Aug 2026 21:20:07 -0500 Subject: [PATCH 08/11] docs: tabs section in README and example configs --- README.md | 16 ++++++++++++++++ config.example.json | 5 ++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 9641725..756720b 100644 --- a/README.md +++ b/README.md @@ -51,6 +51,22 @@ the listen port is `$PORT` (default 7930); the server always binds `127.0.0.1` - the board lists open, non-draft MRs authored by any configured member in one of `projects`. a left sidebar switches between **All** (the whole team) and a single member; the **All** view (and each member view) can be grouped by age / author / status / pipeline and sorted by oldest / pipeline / review progress. the current member, grouping, and sort live in the URL (shareable) and are remembered across visits. +## tabs + +each tab shows a filtered view of MRs sourced from one of two kinds. tabs have a unique id, a sidebar label, and optional overrides for slack and review routing. define tabs in your team settings (`rt settings set board.tabs --scope team`) as an array of tab objects. + +**no tabs config**: the board creates a single implicit "Team" tab sourcing MRs from team members (the `authors` kind). + +**two source kinds**: +- `"authors"`: MRs authored by configured team members (the default) +- `"codeowners"`: MRs blocked on approval from a specific codeowners section. section name comes from your repo's `.gitlab/codeowners` (e.g. `Acme`, `Billing`), and excludeMembers (when true) hides MRs authored by team members so the queue shows work assigned to the team, not self-reviews. no excludeMembers = show all MRs (the section's full queue including team-authored ones) + +**per-tab overrides**: +- `slackChannel`: posts/reactions for this tab go to a different channel (instead of config.slack.channel) +- `reviewSkill`: skill binding for review launches from this tab (instead of the manifest binding or empty fallback) + +use `rt settings set` to edit tabs on the team scope -- `config.json` carries them until then, and a settings-store edit needs a board restart (settings are boot-read, not watched). + ## tokens `bun run setup` handles both. `.env` becomes optional with the daemon fallback below, not retired -- an env var still wins first when it's set, `bun run setup` and `/peer/join` both still write to it (`SWITCHBOARD_TOKEN` in particular), and it stays the simplest path for a solo/local install with no rt daemon at all. under the hood: diff --git a/config.example.json b/config.example.json index c5ff6ca..3992a06 100644 --- a/config.example.json +++ b/config.example.json @@ -54,5 +54,8 @@ }, "switchboard": { "url": "" - } + }, + "tabs": [ + { "id": "team", "label": "Team", "source": { "kind": "authors" } } + ] } From 2e12944870009a2efe16adbaf89544f8cf83392e Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Tue, 25 Aug 2026 21:34:58 -0500 Subject: [PATCH 09/11] fix: codeowner-tagged strangers reach tab 2, stay off tab 1 visibleMrsFor (server.ts's old inline visibleMrs filter, now extracted to data.ts for testability) was dropping every codeowner-tagged stranger before /data.json serialized, so a codeowners tab was always empty. It now also keeps a row whose codeownerSections is non-empty. That widened gate would leak tagged strangers onto the authors tab too (All view, author groups, summary/post selections), since filterByTab passed authors-tab rows through unfiltered. It now narrows an authors tab to roster members, same as filterByMember expects downstream. Co-Authored-By: Claude Fable 5 --- src/__tests__/board.test.ts | 31 +++++++++++++++++++++++++++++++ src/__tests__/view.test.ts | 4 ++-- src/data.ts | 10 ++++++++++ src/server.ts | 5 ++--- src/view.ts | 13 ++++++++----- 5 files changed, 53 insertions(+), 10 deletions(-) diff --git a/src/__tests__/board.test.ts b/src/__tests__/board.test.ts index 45e91a5..607bf16 100644 --- a/src/__tests__/board.test.ts +++ b/src/__tests__/board.test.ts @@ -10,6 +10,7 @@ import { projectPathFromWebUrl, reviewSkillForTab, stripDraftPrefix, + visibleMrsFor, type BoardMR, } from "../data.ts"; import { SnapshotCache, type FetchResult } from "../cache.ts"; @@ -318,6 +319,36 @@ describe("buildBoard tagged rows (codeowner tabs)", () => { }); }); +describe("visibleMrsFor (the /data.json payload gate)", () => { + test("keeps a tagged stranger's MR, drops a hidden member's own MR", () => { + const withHiddenAndTabs: BoardConfig = { + ...config, + members: [{ username: "alice" }, { username: "carol", hidden: true }], + tabs: tabsWithCodeowners, + }; + const now = Date.parse("2026-07-11T00:00:00Z"); + const hiddenMemberMr = pr({ + id: "gitlab:910", + iid: 20, + author: { id: "gitlab:110", username: "carol", name: "Carol", avatarUrl: null }, + }); + const taggedStranger = pr({ + id: "gitlab:911", + iid: 21, + author: { id: "gitlab:111", username: "outsider", name: "Outsider", avatarUrl: null }, + }); + const tags = new Map([[taggedStranger.id, ["Acme"]]]); + const snapshotMrs = buildBoard([hiddenMemberMr, taggedStranger], withHiddenAndTabs, now, tags); + // Both reach the snapshot -- buildBoard doesn't know about "hidden"; only + // visibleMrsFor (the /data.json gate) does. + expect(snapshotMrs.map((m) => m.iid)).toEqual([20, 21]); + + const visibleMembers = withHiddenAndTabs.members.filter((m) => !m.hidden); + const served = visibleMrsFor(snapshotMrs, visibleMembers); + expect(served.map((m) => m.iid)).toEqual([21]); + }); +}); + describe("buildRoster", () => { const members = [{ username: "alice" }, { username: "bob", name: "Bobby" }, { username: "carol" }]; diff --git a/src/__tests__/view.test.ts b/src/__tests__/view.test.ts index 611d07c..976a358 100644 --- a/src/__tests__/view.test.ts +++ b/src/__tests__/view.test.ts @@ -37,9 +37,9 @@ describe("filterByTab", () => { mr({ iid: 3, author: { username: "outsider" } as any, codeownerSections: [] } as any), ]; - test("authors tab passes every row through, deferring to filterByMember downstream", () => { + test("authors tab excludes a tagged stranger, keeps roster rows", () => { const team: TabConfig = { id: "t", label: "T", source: { kind: "authors" } }; - expect(filterByTab(rows, team, members)).toHaveLength(3); + expect(filterByTab(rows, team, members).map((m) => m.iid)).toEqual([1]); }); test("codeowners tab filters to the section and excludes roster authors", () => { diff --git a/src/data.ts b/src/data.ts index 98a5924..e7d7186 100644 --- a/src/data.ts +++ b/src/data.ts @@ -267,6 +267,16 @@ export function hasChangesRequested(mr: BoardMR): boolean { return mr.reviews.reviewers?.some((r) => getReviewDisplayState(r.reviewState ?? null) === "changes_requested") ?? false; } +/** Which snapshot MRs the served board keeps: a visible (non-hidden) roster + member's MR, or any MR carrying at least one codeowner tag. The tag arm is + what lets a codeowner-tagged stranger -- never a roster member -- reach a + codeowners tab; without it every tagged row from outside the roster would + be dropped here before a tab ever saw it. */ +export function visibleMrsFor(mrs: BoardMR[], visibleMembers: Member[]): BoardMR[] { + const visibleNames = new Set(visibleMembers.map((m) => m.username)); + return mrs.filter((mr) => visibleNames.has(mr.author.username) || mr.codeownerSections.length > 0); +} + export interface RosterMember { username: string; name: string | null; diff --git a/src/server.ts b/src/server.ts index e1414ac..160570d 100644 --- a/src/server.ts +++ b/src/server.ts @@ -10,7 +10,7 @@ import { loadConfig, loadGitLabToken, loadSlackToken, loadSwitchboardToken, load import { memoizeAsync } from "./memoize-async.ts"; import { resolveBoardSkill, type BoardSkillKind } from "./manifest-bindings.ts"; import { upsertEnvKeys } from "./env-file.ts"; -import { aggregateSyncScope, boardDemand, buildBoard, buildRoster, channelForMR, configuredSlackChannels, projectPathFromWebUrl, reviewSkillForTab, type BoardMR, type SyncScopeRead } from "./data.ts"; +import { aggregateSyncScope, boardDemand, buildBoard, buildRoster, channelForMR, configuredSlackChannels, projectPathFromWebUrl, reviewSkillForTab, visibleMrsFor, type BoardMR, type SyncScopeRead } from "./data.ts"; import { GitLabProvider, ReadBackFailedError, NoteMutator, parseRepoId } from "@mattstack/glance"; import { summarizeDiscussions, threadStatusCounts, unresolvedReviewerCount } from "./discussions.ts"; import { readProjectMRs, readDiscussions, subscribe } from "@mattstack/rt-client"; @@ -507,8 +507,7 @@ const httpServer = Bun.serve({ // and its counts — but stay in `allMembers` so the settings modal can // check them back in. const visible = config.members.filter((m) => !m.hidden); - const visibleNames = new Set(visible.map((m) => m.username)); - const visibleMrs = snapshot.mrs.filter((mr) => visibleNames.has(mr.author.username)); + const visibleMrs = visibleMrsFor(snapshot.mrs, visible); // Retain review/respond/doctor state for exactly as long as its MR is on // the board; prune once it merges/closes/goes stale and drops off. Gated // on a healthy, non-empty snapshot so a failed fetch (stale/empty data) diff --git a/src/view.ts b/src/view.ts index 55cd61a..bcd7022 100644 --- a/src/view.ts +++ b/src/view.ts @@ -154,12 +154,15 @@ export function filterByMember(mrs: BoardMR[], member: string): BoardMR[] { return member === "all" ? mrs : mrs.filter((m) => m.author.username === member); } -/** An authors tab passes every row through -- member filtering for it stays - downstream in filterByMember. A codeowners tab narrows to rows tagged with - its section; excludeMembers additionally drops the roster's own authors, so - the tab reads as the outside-the-team queue for that section. */ +/** An authors tab narrows to roster members -- further per-member filtering + stays downstream in filterByMember. Without this narrowing, a + codeowner-tagged stranger (never a roster member, but let through the + server's visibility gate for the codeowners tab) would leak onto the + authors tab too. A codeowners tab narrows to rows tagged with its section; + excludeMembers additionally drops the roster's own authors, so the tab + reads as the outside-the-team queue for that section. */ export function filterByTab(mrs: BoardMR[], tab: TabConfig, members: Set): BoardMR[] { - if (tab.source.kind === "authors") return mrs; + if (tab.source.kind === "authors") return mrs.filter((mr) => members.has(mr.author.username)); const { section, excludeMembers } = tab.source; return mrs.filter( (mr) => mr.codeownerSections.includes(section) && (!excludeMembers || !members.has(mr.author.username)), From 28d91144182e6d9ad39c54e3024260f65aaad030 Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Tue, 25 Aug 2026 21:52:13 -0500 Subject: [PATCH 10/11] fix: /slack/post derives the tab's channel instead of always the default Posting a tab-2 row with no explicit channel always fell back to config.slack.channel, publishing into the team channel while resolve and the sweeper look for it in the tab's channel -- the ref ended up pinned to the wrong channelId. /slack/post now derives per-MR via channelForMR: a single-MR post uses that MR's channel, a multi-MR post requires every picked MR to agree on one, and an explicit body channel (already validated against the configured set) still wins. Also: name the worktree/main mismatch in the operator handoff checklist (a daemon restart today boots main and loses the sections handler), and the minimum versions a codeowners tab needs in the README. Co-Authored-By: Claude Fable 5 --- README.md | 2 + .../server-slack-post-channel.test.ts | 229 ++++++++++++++++++ src/__tests__/slack-api-mock-preload.ts | 56 +++++ src/server.ts | 17 +- 4 files changed, 303 insertions(+), 1 deletion(-) create mode 100644 src/__tests__/server-slack-post-channel.test.ts create mode 100644 src/__tests__/slack-api-mock-preload.ts diff --git a/README.md b/README.md index 756720b..c67a1c3 100644 --- a/README.md +++ b/README.md @@ -61,6 +61,8 @@ each tab shows a filtered view of MRs sourced from one of two kinds. tabs have a - `"authors"`: MRs authored by configured team members (the default) - `"codeowners"`: MRs blocked on approval from a specific codeowners section. section name comes from your repo's `.gitlab/codeowners` (e.g. `Acme`, `Billing`), and excludeMembers (when true) hides MRs authored by team members so the queue shows work assigned to the team, not self-reviews. no excludeMembers = show all MRs (the section's full queue including team-authored ones) +a codeowners tab needs `@mattstack/rt-client` >= 0.5.0 in the board and an rt daemon running the sections-aware `project-mrs:read` handler; an older daemon reports no codeowner sections at all, so the tab just renders empty with no badge explaining why. + **per-tab overrides**: - `slackChannel`: posts/reactions for this tab go to a different channel (instead of config.slack.channel) - `reviewSkill`: skill binding for review launches from this tab (instead of the manifest binding or empty fallback) diff --git a/src/__tests__/server-slack-post-channel.test.ts b/src/__tests__/server-slack-post-channel.test.ts new file mode 100644 index 0000000..be42150 --- /dev/null +++ b/src/__tests__/server-slack-post-channel.test.ts @@ -0,0 +1,229 @@ +import { afterAll, expect, test } from "bun:test"; +import { join } from "path"; +import { mkdirSync, mkdtempSync, writeFileSync } from "fs"; +import { tmpdir } from "os"; + +// Proves /slack/post's channel derivation (server.ts's targetChannel logic, +// right after `picked` resolves): with no explicit body channel, a single +// tagged-stranger MR posts into its codeowners tab's slackChannel rather than +// the board's default channel, and a multi-MR post whose picked MRs resolve to +// different channels 400s instead of guessing one. Real (non-fixture) boot -- +// fixture mode answers /data.json from a canned file and refuses every POST +// outright, so it can't exercise this path at all. A fake rt daemon (unix +// socket, mirrors server-healthz-fast.test.ts's pattern) serves the picked +// MRs so cache.get() resolves without any real GitLab connectivity, and +// slack-api-mock-preload.ts (loaded via `bun --preload`) answers slack.com's +// API locally so resolveSlackRef's channel-name lookup never leaves the box. +// The mock gives each channel name its own channel id (see CHANNEL_IDS +// there), so the returned permalink's channel id is a fingerprint for which +// channel name the derivation actually picked. +const fakeHome = mkdtempSync(join(tmpdir(), "board-slack-post-channel-")); + +const teamDir = join(fakeHome, ".mattstack", "teams", "testteam", "mattstack"); +mkdirSync(teamDir, { recursive: true }); +writeFileSync( + join(teamDir, "settings.team.jsonc"), + JSON.stringify({ + "board.gitlabHost": "https://gitlab.example.com", + "board.projects": ["g/p"], + "board.members": [{ username: "alice" }], + "board.tabs": [ + { id: "team", label: "Team", source: { kind: "authors" } }, + { + id: "acme", + label: "Acme", + source: { kind: "codeowners", section: "Acme" }, + slackChannel: "acme-channel", + }, + { + id: "other", + label: "Other", + source: { kind: "codeowners", section: "OtherSection" }, + slackChannel: "other-channel", + }, + ], + }), +); + +// board.rtRepos is machine-scoped (registry-defs.ts), not team-scoped -- a +// team-store value for it is silently ignored, which would leave +// fetchTeamMRs with no daemon mapping for "g/p" and every MR dropped before +// buildBoard ever saw them. +writeFileSync(join(fakeHome, ".mattstack", "machine-key"), "testmachine"); +const machineDir = join(fakeHome, ".mattstack", "user", "local", "testmachine"); +mkdirSync(machineDir, { recursive: true }); +writeFileSync( + join(machineDir, "settings.local.jsonc"), + JSON.stringify({ + "board.rtRepos": [{ project: "g/p", repo: "gitlab.example.com/g/p" }], + }), +); + +const GITLAB_HOST = "https://gitlab.example.com"; +const acmeMrUrl = `${GITLAB_HOST}/g/p/-/merge_requests/501`; +const otherSectionMrUrl = `${GITLAB_HOST}/g/p/-/merge_requests/502`; + +function fakePr(overrides: Record): Record { + return { + id: overrides.id, + iid: overrides.iid, + repositoryId: "gitlab:42", + title: "An MR", + description: null, + state: "opened", + draft: false, + conflicts: false, + webUrl: overrides.webUrl, + sourceBranch: "feat/x", + targetBranch: "main", + createdAt: "2026-08-01T00:00:00Z", + updatedAt: new Date().toISOString(), + sha: null, + author: overrides.author, + assignees: [], + reviewers: [], + roles: [], + pipeline: null, + unresolvedThreadCount: 0, + approvalsLeft: 1, + approved: false, + approvedBy: [], + diffStats: null, + detailedMergeStatus: null, + autoMergeEnabled: false, + autoMergeStrategy: null, + mergeUser: null, + mergeAfter: null, + divergedCommitsCount: null, + rebaseInProgress: false, + mergeOngoing: false, + inProgressMergeCommitSha: null, + mergeError: null, + shouldBeRebased: false, + mergeabilityChecks: [], + blockingMergeRequestsCount: 0, + approvalsRequired: 1, + squash: false, + squashOnMerge: false, + mergeTrainIndex: null, + }; +} + +const acmePr = fakePr({ + id: "gitlab:501", + iid: 501, + webUrl: acmeMrUrl, + author: { id: "gitlab:901", username: "outsider1", name: "Outsider One", avatarUrl: null }, +}); +const otherSectionPr = fakePr({ + id: "gitlab:502", + iid: 502, + webUrl: otherSectionMrUrl, + author: { id: "gitlab:902", username: "outsider2", name: "Outsider Two", avatarUrl: null }, +}); + +const rtDir = join(fakeHome, ".mattstack", "rt"); +mkdirSync(rtDir, { recursive: true }); + +// Every mapped project's opened MRs, tagged the way the daemon reports a +// codeowners match -- see fetchTeamMRs' `entry.codeownerSections`. Every +// other command answers `ok:false` immediately (never hangs), which is what +// lets the token round trips (board-secrets.ts) degrade to "not configured" +// without a wedged wait. +const rtDaemon = Bun.serve({ + unix: join(rtDir, "rt.sock"), + fetch(req) { + const { pathname } = new URL(req.url); + if (pathname === "/project-mrs:read") { + return new Response( + JSON.stringify({ + ok: true, + data: { + mrs: { + "gitlab:501": { pr: acmePr, fetchedAt: Date.now(), codeownerSections: ["Acme"] }, + "gitlab:502": { pr: otherSectionPr, fetchedAt: Date.now(), codeownerSections: ["OtherSection"] }, + }, + listSyncedAt: Date.now(), + source: "poll", + syncedAt: Date.now(), + }, + }), + { headers: { "content-type": "application/json" } }, + ); + } + return new Response(JSON.stringify({ ok: false, error: "not implemented" }), { + headers: { "content-type": "application/json" }, + }); + }, +}); + +const PORT = 47946; +const proc = Bun.spawn( + [ + "bun", + "run", + "--preload", + join(import.meta.dir, "slack-api-mock-preload.ts"), + join(import.meta.dir, "..", "server.ts"), + ], + { + env: { + ...process.env, + HOME: fakeHome, + BOARD_APP_ROOT: fakeHome, + PORT: String(PORT), + GITLAB_TOKEN: "", + SLACK_TOKEN: "fake-slack-token", + SWITCHBOARD_TOKEN: "", + SWITCHBOARD_ADMIN_TOKEN: "", + // Consumed by slack-api-mock-preload.ts's conversations.history stub. + SLACK_MOCK_MR_URL: acmeMrUrl, + }, + stdout: "pipe", + stderr: "pipe", + }, +); + +afterAll(() => { + proc.kill(); + rtDaemon.stop(true); +}); + +async function ready(): Promise { + for (let i = 0; i < 100; i++) { + try { + if ((await fetch(`http://127.0.0.1:${PORT}/healthz`)).ok) return; + } catch { + // server not listening yet + } + await new Promise((r) => setTimeout(r, 100)); + } + throw new Error("server never came up"); +} + +test("/slack/post with no explicit channel derives a tagged stranger's MR to its tab's slackChannel", async () => { + await ready(); + const res = await fetch(`http://127.0.0.1:${PORT}/slack/post`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ mrUrls: [acmeMrUrl] }), + }); + expect(res.status).toBe(200); + const body = (await res.json()) as { ok: boolean; linked?: boolean; permalink?: string }; + expect(body.ok).toBe(true); + // C_ACME is "acme-channel"'s id in the mock (see CHANNEL_IDS in + // slack-api-mock-preload.ts) -- proves the post targeted the tab's channel, + // not C_DEFAULT ("code-review", config.slack.channel). + expect(body.permalink).toContain("C_ACME"); +}, 15_000); + +test("/slack/post with no explicit channel 400s when picked MRs span different channels", async () => { + await ready(); + const res = await fetch(`http://127.0.0.1:${PORT}/slack/post`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ mrUrls: [acmeMrUrl, otherSectionMrUrl] }), + }); + expect(res.status).toBe(400); + expect(await res.text()).toBe("MRs span Slack channels; post them per tab"); +}, 15_000); diff --git a/src/__tests__/slack-api-mock-preload.ts b/src/__tests__/slack-api-mock-preload.ts new file mode 100644 index 0000000..e0ee30c --- /dev/null +++ b/src/__tests__/slack-api-mock-preload.ts @@ -0,0 +1,56 @@ +// Preloaded (via `bun --preload`) into the subprocess server.ts boots by +// server-slack-post-channel.test.ts, so slack.ts's real `https://slack.com/api/*` +// calls resolve locally instead of over the network. Each mocked channel name +// maps to its own distinguishable channel id, so a test can tell which channel +// name the caller resolved purely from the id embedded in the response. +const realFetch = globalThis.fetch; + +const CHANNEL_IDS: Record = { + "code-review": "C_DEFAULT", + "acme-channel": "C_ACME", + "other-channel": "C_OTHER", +}; + +function ok(data: Record): Response { + return new Response(JSON.stringify({ ok: true, ...data }), { + status: 200, + headers: { "content-type": "application/json" }, + }); +} + +function slackApi(method: string, params: Record): Response { + switch (method) { + case "auth.test": + return ok({ url: "https://mockteam.slack.com/" }); + case "conversations.list": + return ok({ + channels: Object.entries(CHANNEL_IDS).map(([name, id]) => ({ id, name })), + response_metadata: { next_cursor: "" }, + }); + case "conversations.history": { + // SLACK_MOCK_MR_URL: the one MR url this test's channel index should + // already show a review-request message for. + const mrUrl = process.env.SLACK_MOCK_MR_URL; + return ok({ + messages: mrUrl ? [{ ts: "100.000001", user: "U1", text: `please review ${mrUrl}` }] : [], + has_more: false, + }); + } + case "reactions.get": + return ok({ message: { reactions: [] } }); + case "chat.postMessage": + return ok({ ts: "200.000001" }); + default: + return ok({}); + } +} + +globalThis.fetch = (async (input: Parameters[0], init?: Parameters[1]) => { + const url = typeof input === "string" ? input : input instanceof URL ? input.href : (input as Request).url; + if (!url.startsWith("https://slack.com/api/")) return realFetch(input as never, init); + const method = url.slice("https://slack.com/api/".length).split("?")[0]!; + const params: Record = init?.body + ? (JSON.parse(init.body as string) as Record) + : Object.fromEntries(new URL(url).searchParams); + return slackApi(method, params); +}) as typeof fetch; diff --git a/src/server.ts b/src/server.ts index 160570d..ae108c2 100644 --- a/src/server.ts +++ b/src/server.ts @@ -1223,13 +1223,28 @@ const httpServer = Bun.serve({ if (channel !== undefined && (typeof channel !== "string" || !allowedChannels.includes(channel))) { return new Response(`"channel" must be one of ${allowedChannels.join(", ")}`, { status: 400 }); } - const targetChannel = typeof channel === "string" ? channel : config.slack.channel; const snapshot = await cache.get(); const byUrl = new Map(snapshot.mrs.map((m) => [m.webUrl, m] as const)); const picked = (mrUrls as string[]).map((u) => byUrl.get(u)).filter((m): m is BoardMR => !!m); if (picked.length !== mrUrls.length) { return new Response("one or more mrUrls are not on the board", { status: 400 }); } + // An explicit body channel (already validated above) always wins. + // Otherwise derive per-MR: resolve/sweeper look in the tab's channel + // via channelForMR, so a post with no explicit channel must land + // there too, or the ref would pin the wrong channelId. A multi-MR + // post only has one channel to post to, so every picked MR must + // resolve to the same one. + let targetChannel: string; + if (typeof channel === "string") { + targetChannel = channel; + } else { + const resolved = new Set(picked.map((m) => channelForMR(config, m))); + if (resolved.size > 1) { + return new Response("MRs span Slack channels; post them per tab", { status: 400 }); + } + targetChannel = [...resolved][0]!; + } // Guard against duplicate posts: check for an existing ref file first, // and for MRs we've never resolved, sync the channel index and look for // the author's original review-request. If any MR already has a From ea4431af435b8b5327bacba53913e9b77efbf54c Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Tue, 25 Aug 2026 23:18:14 -0500 Subject: [PATCH 11/11] fix: coderabbit round on PR #3 (empty tabs, member-refresh tags, selection-clear purity) --- src/__tests__/board.test.ts | 11 +++++++++++ src/__tests__/config.test.ts | 4 ++++ src/client/board/Board.tsx | 10 ++++------ src/config.ts | 1 + src/server.ts | 7 +++++-- 5 files changed, 25 insertions(+), 8 deletions(-) diff --git a/src/__tests__/board.test.ts b/src/__tests__/board.test.ts index 607bf16..0540344 100644 --- a/src/__tests__/board.test.ts +++ b/src/__tests__/board.test.ts @@ -290,6 +290,17 @@ describe("buildBoard tagged rows (codeowner tabs)", () => { expect(out[0]!.codeownerSections).toEqual(["Acme"]); }); + test("stamps codeownerSections on a roster member's own tagged MR too", () => { + // Mirrors fetchMemberMRs' scoped refresh: the row is kept on isMember + // alone, but a tags map must still be passed for codeownerSections to + // land -- an omitted map (server.ts's prior bug) silently zeroes it. + const memberMr = pr({ id: "gitlab:904", iid: 13 }); // default author: alice, a roster member + const tags = new Map([[memberMr.id, ["Acme"]]]); + const out = buildBoard([memberMr], withTabs, now, tags); + expect(out).toHaveLength(1); + expect(out[0]!.codeownerSections).toEqual(["Acme"]); + }); + test("still drops an untagged stranger, and tag-kept rows skip the prefix filter", () => { const withPrefixes: BoardConfig = { ...withTabs, ticketPrefixes: ["CV"] }; // untagged stranger -> dropped diff --git a/src/__tests__/config.test.ts b/src/__tests__/config.test.ts index 203d962..7777e08 100644 --- a/src/__tests__/config.test.ts +++ b/src/__tests__/config.test.ts @@ -112,6 +112,10 @@ describe("parseConfig", () => { expect(cfg.tabs).toEqual([{ id: "team", label: "Team", source: { kind: "authors" } }]); }); + test("throws on an empty tabs array instead of silently producing a zero-tab board", () => { + expect(() => parseConfig(JSON.stringify({ ...base, tabs: [] }))).toThrow(/tabs.*must not be empty/); + }); + test("tabs validate: unique ids, codeowners needs a section", () => { expect(() => parseConfig(JSON.stringify({ ...base, tabs: [ { id: "a", label: "A", source: { kind: "codeowners" } }, diff --git a/src/client/board/Board.tsx b/src/client/board/Board.tsx index 993f743..5b4858c 100644 --- a/src/client/board/Board.tsx +++ b/src/client/board/Board.tsx @@ -69,12 +69,10 @@ export function Board() { }; const update = (patch: Partial) => { const clearsSelection = tabChangeClearsSelection(patch, state.tab); - setState((prev) => { - const next = { ...prev, ...patch }; - localStorage.setItem(STATE_KEY, JSON.stringify(next)); - history.replaceState(null, "", serializeViewState(next) || location.pathname); - return next; - }); + const next = { ...state, ...patch }; + localStorage.setItem(STATE_KEY, JSON.stringify(next)); + history.replaceState(null, "", serializeViewState(next) || location.pathname); + setState(next); if (clearsSelection) setSelected(new Set()); }; diff --git a/src/config.ts b/src/config.ts index 618f393..a6a9063 100644 --- a/src/config.ts +++ b/src/config.ts @@ -250,6 +250,7 @@ export function parseConfig(raw: string, source = "config.json"): BoardConfig { function parseTabs(raw: unknown, source: string): TabConfig[] { if (raw === undefined) return IMPLICIT_TABS; if (!Array.isArray(raw)) throw new Error(`${source} "tabs" must be an array`); + if (raw.length === 0) throw new Error(`${source} "tabs" must not be empty (omit "tabs" for the implicit default)`); const seenIds = new Set(); return raw.map((entry, i) => { const label = `tabs[${i}]`; diff --git a/src/server.ts b/src/server.ts index ae108c2..2b7468a 100644 --- a/src/server.ts +++ b/src/server.ts @@ -294,6 +294,7 @@ const cache = new SnapshotCache(async () => { */ async function fetchMemberMRs(username: string): Promise { const out: PullRequest[] = []; + const tags = new Map(); const errors: string[] = []; for (const projectPath of config.projects) { const repoId = daemonRepoField(config, projectPath); @@ -301,11 +302,13 @@ async function fetchMemberMRs(username: string): Promise { const res = await readProjectMRs(repoId, 20_000); if (!res.ok || !res.data) { errors.push(`${projectPath}: ${res.error ?? "empty daemon response"}`); continue; } for (const entry of Object.values(res.data.mrs)) { - if (entry.pr.state === "opened" && entry.pr.author?.username === username) out.push(entry.pr); + if (entry.pr.state !== "opened" || entry.pr.author?.username !== username) continue; + out.push(entry.pr); + if (entry.codeownerSections?.length) tags.set(entry.pr.id, entry.codeownerSections); } } if (errors.length) throw new Error(errors.join(" · ")); - const mrs = buildBoard(out, config); + const mrs = buildBoard(out, config, undefined, tags); await enrichReviewerComments(mrs); return mrs; }