diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 518e41b4f..4ededf72c 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -88,6 +88,18 @@ settings") are composition on top of this tenant and its settings rows, not a separate object; the surface lives in `packages/chat-ui`'s `workbench-settings`. +**In-room onboarding scene.** A named template's first-minute walkthrough +is one timeline card posted in the room's own voice, not a member's +message and not a side effect of hosting an agent. The card keeps a +stable header — the job, the promise, the ordered steps — and flips its +body in place through connect, pick-repos, and reviewing. Change-repos +returns the body to the picker without rewriting what the room already +recorded. An empty step list is omitted, not rendered as an empty rail. +The current step is named in words; colour is additive, never the only +signal. Consecutive agent-joined events collapse into one line so the +scene and the reviewers' own introductions are what a person reads +first. + **Streaming a reply.** An agent's live reply reaches the timeline through one path, deltas to pixels: diff --git a/DESIGN.md b/DESIGN.md index cee28133a..0dbb6d538 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -63,11 +63,16 @@ real, never a 404. not home. The primary act is a prompt: say what the channel should do, or pick a named-template shortcut underneath. Blank `+` / prompt mint an empty channel and invite nobody. Named templates mint that same -empty channel, then invite existing agents (including Myra as a -participant, never as the mint host). The sidebar `+` opens this -route. First-run after credential does not: `/` hops to Myra's one -DM (`openAgentDm` / find-or-reopen). There is no parallel Myra home -route and no Describe door. +empty channel with no host, then instantiate the picked Workbench +Definition — the agents, block workflows, and pending plugins it names +(Myra joins only when the definition names her; Code review's three +reviewers do not) — and run its ordered onboarding walkthrough as an +in-room card the room itself posts, never a side effect of hosting an +agent. The card reads live connection state and flips straight to the +repo pick, so there is one walkthrough, not a separate already-connected +dialog. The sidebar `+` opens this route. First-run after credential +does not: `/` hops to Myra's one DM (`openAgentDm` / find-or-reopen). +There is no parallel Myra home route and no Describe door. **`/inbox` is gone as a page** (CL-6151). The path stays as a redirect home so old links still resolve; it is not a live groups inbox. @@ -283,6 +288,15 @@ personal-access-token paste, then the same card flips to pick repositories. A GitHub App / hosted OAuth Connect as the welcome mat is CL-6343 (out of scope), not the shipped card. +The room's own onboarding card renders as a scene, not a member's +message: no author row, the job as its title with the promise beneath, +and the walkthrough's steps listed with the current one marked in +words. Once repos are recorded the card shows the Reviewing state — +what it's reviewing now — with a change-repos link back to the picker, +never still offering Connect. Consecutive agent-joined rows collapse +into one line naming everyone, so a template room opens on the scene +and the reviewers' own introductions, never a join dump. + ## State Pills Status indicators (ok / warn / error / running) use semantic colors that diff --git a/IMPLEMENTATION.md b/IMPLEMENTATION.md index e485dee54..1d3abd7a8 100644 --- a/IMPLEMENTATION.md +++ b/IMPLEMENTATION.md @@ -139,6 +139,23 @@ the current product path — do not document OAuth as the first Connect step, and do not treat a PAT paste as a defect against an OAuth-first welcome mat that has not shipped. +The room posts that card from `system@` +(`POST /workbenches/:id/onboarding` in `packages/chat/src/routes.ts`). +`packages/chat-ui` renders a system-sender `connect-github` block as a +scene: no author row, no avatar, no "Member" label; the job title and +optional promise stay put while the body flips. Step labels come from +the block's `steps` array — an empty or omitted array does not draw a +step list. The current-step marker ("You're here") is applied only when +there are exactly three steps (connect / pick / review); `data-state` on +the step is colour, never the only signal. After the server has recorded +repos, the body is the reviewing state (repo names plus a `change repos` +control). Clicking `change repos` is client-local in +`connect-github-block-container` and does not mutate the recorded +selection. `onReviewingStarted` posts canned introductions from +`packages/code-review/src/introductions.ts` under each reviewer's own +address in roster order. Consecutive `workbench.agent-joined` rows +collapse to one line in `packages/chat-ui/src/timeline.tsx`. + Optional `GITHUB_APP_CLIENT_ID` / `GITHUB_APP_CLIENT_SECRET` exist for that future hosted path; leaving them unset is normal. See `docs/connect-cards.md` and PRODUCT.md's Code review first minute. diff --git a/PRODUCT.md b/PRODUCT.md index 3f27815a0..5e33328ec 100644 --- a/PRODUCT.md +++ b/PRODUCT.md @@ -92,11 +92,23 @@ put there. Code review is the product scene for the definition-driven path: minting the Code review workbench opens an empty room with no host — its Workbench Definition names three reviewers and no Myra. The room -itself posts the onboarding card: Connect GitHub with a personal access -token (the shipped path today). The same in-room card reads live -connection state and flips in place to pick repositories — already -connected GitHub is that card, not a `/new` dialog. A GitHub App / -hosted OAuth welcome mat is future work (CL-6343), not current product. +itself posts the onboarding card as a scene, not a member's message: +no author row, the job as its title, the promise beneath, and the +walkthrough's steps listed with the current one marked in words. A +walkthrough with no steps hides the step list rather than drawing an +empty one. Connect GitHub with a personal access token (the shipped +path today). The same in-room card reads live connection state and +flips in place to pick repositories — already connected GitHub is +that card, not a `/new` dialog — then Start reviewing. Once repos are +recorded the same card shows what it is reviewing, with a change-repos +link back to the picker — a reviewing card never still says Connect. +Once reviewing starts, each reviewer posts its own canned introduction +under its own address, in roster order — the first thing a person +reads is who is reviewing and what for, never a join dump. Consecutive +agent-joined rows collapse into one line naming everyone. A GitHub App +/ hosted OAuth welcome mat is future work (CL-6343), not current +product. Inviting teammates into the room is a later slice, not part +of this first minute. Settle for a template-key-only wait posts from a system sender and does not wake an agent. Generic `connections/pending` still wakes the diff --git a/apps/hub/src/index.ts b/apps/hub/src/index.ts index 5529ff05a..aae856550 100644 --- a/apps/hub/src/index.ts +++ b/apps/hub/src/index.ts @@ -94,6 +94,9 @@ import { isWorkbenchHostDefinitionName, listConnectedProviders, listDefaultInferencePreferences, + localPartOf, + parseParticipants, + postRoomMessage, startWorkflowCommand, sendWorkbenchMessage, settleConnectedService, @@ -2266,6 +2269,33 @@ export async function createHub(config: HubConfig) { data: { updatedBy: principalId, settings: row.settings }, }); }, + onReviewingStarted: async ( + tenantId, + workbenchId, + _principalId, + introductions, + ) => { + const row = await chatStore.getWorkbenchSettings(tenantId, workbenchId); + const participants = parseParticipants( + row?.settings["chat/participants"], + ); + for (const introduction of introductions) { + const participant = participants.find( + (candidate) => candidate.handle === introduction.handle, + ); + if (participant === undefined) continue; + await postRoomMessage( + { roomMessages, publish: workbenchSubscribers.publish }, + { + tenantId, + workbenchId, + sender: { name: null, address: participant.address }, + runId: localPartOf(participant.address), + parts: [{ kind: "text", text: introduction.text }], + }, + ); + } + }, }), ); // Template block workflows (CL-6405): the instantiate path's diff --git a/bun.lock b/bun.lock index 9bc93fec9..4e0948eb1 100644 --- a/bun.lock +++ b/bun.lock @@ -1455,6 +1455,7 @@ "dependencies": { "@corbits/code-review": "workspace:*", "@corbits/code-review-workflow": "workspace:*", + "@corbits/error-sink": "workspace:*", "@corbits/github-tools": "workspace:*", "@corbits/jimmy-agent": "workspace:*", "@corbits/scout-agent": "workspace:*", diff --git a/docs/connect-cards.md b/docs/connect-cards.md index 4713539b1..9a57743d2 100644 --- a/docs/connect-cards.md +++ b/docs/connect-cards.md @@ -54,9 +54,13 @@ the walkthrough. Connect opens a guided personal-access-token paste (create a token with the `repo` scope, paste it, store encrypted). After Connect succeeds — or when GitHub is already connected — the same in-room card flips in place to pick repositories; there is no -`/new` already-connected dialog. Settling a credential a template room -is waiting on posts the connected notice from the system address and -never wakes an agent (`packages/chat/src/connect-pending.ts`). +`/new` already-connected dialog. Code review needs a repo pick before +reviewers are watching, then Start reviewing. Settling a credential a +template room is waiting on posts the connected notice from the system +address and never wakes an agent (`packages/chat/src/connect-pending.ts`) +— a reviewer roster has no host to answer. Once reviewing starts, each +reviewer posts its own canned introduction under its own address, in +roster order (`packages/code-review/src/introductions.ts`). A GitHub App / hosted OAuth Connect as the welcome mat is CL-6343, out of scope for the shipped card — do not document OAuth-first GitHub connect diff --git a/packages/chat-ui/src/blocks/connect-github-block-container.tsx b/packages/chat-ui/src/blocks/connect-github-block-container.tsx index 9089cb372..177a309e3 100644 --- a/packages/chat-ui/src/blocks/connect-github-block-container.tsx +++ b/packages/chat-ui/src/blocks/connect-github-block-container.tsx @@ -22,13 +22,25 @@ // snapshot so a real disconnect still shows Connect. import { useCallback, useEffect, useRef, useState } from "react"; import type { ConnectGithubBlockData } from "@corbits/chat/blocks"; +import { reportError } from "@corbits/error-sink"; +import { CHAT_STRINGS } from "../strings"; import type { ConnectGithubActions, ConnectGithubQuery, } from "./connect-github-actions"; +import type { OnboardingScene } from "./connect-github-block"; import { ConnectGithubBlockView } from "./connect-github-block"; +/** Positions in the room's three-step walkthrough. Which one is current + * is read off the live connect state, never off the card's own data: + * disconnected means connect, connected with nothing recorded (or a + * person who pressed "change repos") means pick, and repos the server + * actually recorded means reviewing. */ +const STEP_CONNECT = 0; +const STEP_PICK = 1; +const STEP_REVIEWING = 2; + type ConnectedGithubQuery = Extract; /** Survives container remounts so a post-connect loading flash never @@ -60,6 +72,7 @@ function displayQueryOf( } export function ConnectGithubBlockContainer({ + data, messageId, actions, }: { @@ -73,7 +86,16 @@ export function ConnectGithubBlockContainer({ const [selectedRepoIds, setSelectedRepoIds] = useState( () => lastConnectedOf(messageId)?.selectedRepoIds ?? [], ); + /** A person who pressed "change repos" on the done state gets the + * picker back without the server's recorded selection changing — only + * pressing "Start reviewing" again writes anything. */ + const [repickRequested, setRepickRequested] = useState(false); + const [startReviewingError, setStartReviewingError] = useState< + string | undefined + >(undefined); const mountedRef = useRef(true); + const hadPickerRef = useRef(false); + const hadConnectRef = useRef(false); const applyQuery = useCallback( (result: ConnectGithubQuery) => { @@ -123,10 +145,39 @@ export function ConnectGithubBlockContainer({ ); const displayQuery = displayQueryOf(messageId, query); + const recordedRepoIds = + displayQuery.kind === "connected" ? displayQuery.selectedRepoIds : []; + const currentStepIndex = + displayQuery.kind !== "connected" + ? STEP_CONNECT + : recordedRepoIds.length === 0 || repickRequested + ? STEP_PICK + : STEP_REVIEWING; + if (currentStepIndex === STEP_CONNECT) hadConnectRef.current = true; + if (currentStepIndex === STEP_PICK) hadPickerRef.current = true; + const scene: OnboardingScene = { + title: data.requiredForTemplate, + currentStepIndex, + ...(data.promise !== undefined ? { promise: data.promise } : {}), + ...(data.steps !== undefined ? { steps: data.steps } : {}), + }; + + if (displayQuery.kind === "error") { + return ( + actions?.requestConnect()} + onSubmitAccessToken={submitAccessTokenAndRefresh} + /> + ); + } if (actions === undefined || displayQuery.kind !== "connected") { return ( actions?.requestConnect()} onSubmitAccessToken={submitAccessTokenAndRefresh} @@ -134,6 +185,22 @@ export function ConnectGithubBlockContainer({ ); } + if (currentStepIndex === STEP_REVIEWING) { + return ( + recordedRepoIds.includes(repo.id)) + .map((repo) => repo.name)} + onChangeRepos={() => setRepickRequested(true)} + {...(hadPickerRef.current ? { autoFocus: true } : {})} + /> + ); + } + + const connectedActions = actions; + function toggleRepo(repoId: string) { setSelectedRepoIds((current) => current.includes(repoId) @@ -142,8 +209,24 @@ export function ConnectGithubBlockContainer({ ); } + async function startReviewing(repoIds: readonly string[]) { + try { + setStartReviewingError(undefined); + await connectedActions.startReviewing(repoIds); + if (mountedRef.current) setRepickRequested(false); + } catch (cause) { + reportError(cause, { operation: "connect-github.startReviewing" }); + if (mountedRef.current) { + setStartReviewingError( + CHAT_STRINGS.blockConnectGithubStartReviewingError, + ); + } + } + } + return ( setSelectedRepoIds(displayQuery.repos.map((repo) => repo.id)) } - onChangeConnection={actions.requestConnect} + onChangeConnection={connectedActions.requestConnect} onStartReviewing={(repoIds) => { - void actions.startReviewing(repoIds); + void startReviewing(repoIds); }} onSkip={() => { - void actions.skip(); + void connectedActions.skip(); }} + {...(startReviewingError !== undefined + ? { error: startReviewingError } + : {})} + {...(hadConnectRef.current ? { autoFocus: true } : {})} /> ); } diff --git a/packages/chat-ui/src/blocks/connect-github-block.tsx b/packages/chat-ui/src/blocks/connect-github-block.tsx index a8b25af6c..786fcc440 100644 --- a/packages/chat-ui/src/blocks/connect-github-block.tsx +++ b/packages/chat-ui/src/blocks/connect-github-block.tsx @@ -1,6 +1,7 @@ -// The GitHub connect card renders both states of the first-run connect flow -// (CL-6342 screen 2) inline in the room, using the same `BlockCard` frame -// every other block uses -- there is no settings page or dialog for this. +// The room's first-minute scene card: one `BlockCard` that names the job, +// carries the walkthrough the workbench's own onboarding copy wrote, and +// flips its body between connecting, picking repos, and reviewing -- +// inline in the room, never a settings page or a dialog. // Selection is controlled: like `PollBlockView` never keeps its own tally, // this view never owns which repos are picked -- it renders what it's given // and reports toggles upward. It stays pure and props-driven so it can be @@ -16,7 +17,7 @@ // layout around it (name left, open-PR count right) is workbench-specific // composition and stays local. -import { useState } from "react"; +import { useEffect, useRef, useState } from "react"; import { Button, Checkbox, Input } from "@corbits/react-ui"; import { Check } from "@corbits/icons"; @@ -29,7 +30,34 @@ export type ConnectGithubRepo = { readonly openPullRequestCount: number; }; -export type ConnectGithubCardProps = +/** One labelled step of the room's walkthrough, as the workbench's own + * onboarding copy wrote it — the card never holds step text of its own. */ +export type OnboardingSceneStep = { + readonly title: string; + readonly why: string; +}; + +/** The framing every state of this card keeps: the job the room is here + * to do, the one-line promise under it, and the ordered walkthrough. The + * card flips its body between states while this header stays put. */ +export type OnboardingScene = { + readonly title: string; + readonly promise?: string; + readonly steps?: readonly OnboardingSceneStep[]; + /** Which step of `steps` the person is on now, by position. */ + readonly currentStepIndex: number; +}; + +/** The walkthrough this card knows how to mark: connect, pick, review. + * A workbench whose steps read differently still gets its labels + * rendered — just without a "you're here" marker the positions would + * only be guessing at. */ +const MARKABLE_STEP_COUNT = 3; + +/** What the card's body is doing right now, without the scene framing + * every state shares — the shape a host names when it only cares about + * the body variant. */ +export type ConnectGithubCardBody = | { readonly kind: "disconnected"; readonly onConnect: () => void; @@ -45,6 +73,16 @@ export type ConnectGithubCardProps = { readonly ok: true } | { readonly ok: false; readonly message: string } >; } + | { + readonly kind: "error"; + readonly message: string; + readonly onConnect: () => void; + readonly onSubmitAccessToken: ( + token: string, + ) => Promise< + { readonly ok: true } | { readonly ok: false; readonly message: string } + >; + } | { readonly kind: "connected"; readonly orgName: string; @@ -55,17 +93,140 @@ export type ConnectGithubCardProps = readonly onChangeConnection: () => void; readonly onStartReviewing: (repoIds: readonly string[]) => void; readonly onSkip: () => void; + /** Set when `onStartReviewing` rejected — the picker stays up and + * this is the card's own alert, never a toast-only failure. */ + readonly error?: string; + /** Move focus onto the pick-repos heading after Connect succeeded, + * so it does not stay on a control that unmounted. */ + readonly autoFocus?: boolean; + } + | { + readonly kind: "reviewing"; + readonly repoNames: readonly string[]; + readonly onChangeRepos: () => void; + /** Move focus onto the reviewing scene after Start reviewing + * succeeded, so it does not stay on a control that unmounted. */ + readonly autoFocus?: boolean; }; +export type ConnectGithubCardProps = ConnectGithubCardBody & { + readonly scene: OnboardingScene; +}; + function repoMetaLabel(openPullRequestCount: number): string { return openPullRequestCount === 0 ? CHAT_STRINGS.blockConnectGithubNoOpenPulls : CHAT_STRINGS.blockConnectGithubOpenPulls(openPullRequestCount); } +type StepState = "done" | "current" | "upcoming"; + +function stepStateAt(index: number, currentStepIndex: number): StepState { + if (index < currentStepIndex) return "done"; + if (index === currentStepIndex) return "current"; + return "upcoming"; +} + +function stepMarkLabel(state: StepState): string | undefined { + if (state === "done") return CHAT_STRINGS.blockConnectGithubStepDone; + if (state === "current") return CHAT_STRINGS.blockConnectGithubStepCurrent; + return undefined; +} + +/** The card's constant framing: what this room is for, the promise under + * it, and where the person is in the walkthrough. Every label here is the + * workbench's own onboarding copy, never this package's. */ +function SceneHeader({ scene }: { readonly scene: OnboardingScene }) { + const steps = scene.steps; + const marked = steps !== undefined && steps.length === MARKABLE_STEP_COUNT; + const currentWhy = marked ? steps[scene.currentStepIndex]?.why : undefined; + return ( + <> + {scene.promise !== undefined ? ( +

{scene.promise}

+ ) : null} + {steps !== undefined && steps.length > 0 ? ( +
    + {steps.map((step, index) => { + const state = stepStateAt(index, scene.currentStepIndex); + const mark = marked ? stepMarkLabel(state) : undefined; + return ( +
  1. + + {step.title} + + {mark !== undefined ? ( + {mark} + ) : null} +
    2. + ); + })} +
+ ) : null} + {currentWhy !== undefined ? ( +

{currentWhy}

+ ) : null} + + ); +} + +/** The walkthrough's done state: the repos under review, what happens in + * them now, and one quiet way back to the picker. Deliberately says + * nothing about connecting — this card is past that. */ +function ReviewingBody({ + repoNames, + onChangeRepos, + autoFocus, +}: { + readonly repoNames: readonly string[]; + readonly onChangeRepos: () => void; + readonly autoFocus?: boolean; +}) { + const sceneRef = useRef(null); + useEffect(() => { + if (autoFocus === true) sceneRef.current?.focus(); + }, [autoFocus]); + return ( +
+

+ + {CHAT_STRINGS.blockConnectGithubReviewingHeadline} +

+
    + {repoNames.map((name) => ( +
  • {name}
    • + ))} +
+

+ {CHAT_STRINGS.blockConnectGithubReviewingLine} +

+
+ +
+
+ ); +} + function DisconnectedBody({ onConnect, onSubmitAccessToken, + error: stateError, + actionLabel, }: { readonly onConnect: () => void; readonly onSubmitAccessToken: ( @@ -73,11 +234,13 @@ function DisconnectedBody({ ) => Promise< { readonly ok: true } | { readonly ok: false; readonly message: string } >; + readonly error?: string; + readonly actionLabel?: string; }) { const [fieldOpen, setFieldOpen] = useState(false); const [token, setToken] = useState(""); const [submitting, setSubmitting] = useState(false); - const [error, setError] = useState(undefined); + const [error, setError] = useState(stateError); function openField() { onConnect(); @@ -133,9 +296,13 @@ function DisconnectedBody({ setToken(event.target.value); }} disabled={submitting} + {...(error !== undefined ? { "aria-invalid": true } : {})} /> {error !== undefined ? ( -

+

{error}

) : null} @@ -156,7 +323,7 @@ function DisconnectedBody({ onClick={() => { setFieldOpen(false); setToken(""); - setError(undefined); + setError(stateError); }} > {CHAT_STRINGS.blockConnectGithubTokenCancel} @@ -168,10 +335,18 @@ function DisconnectedBody({ return ( <> + {error !== undefined ? ( +

+ {error} +

+ ) : null}

{CHAT_STRINGS.blockConnectGithubIntro}

@@ -190,10 +365,24 @@ function ConnectedBody({ onChangeConnection, onStartReviewing, onSkip, + error, + autoFocus, }: Extract) { + const headingRef = useRef(null); + useEffect(() => { + if (autoFocus === true) headingRef.current?.focus(); + }, [autoFocus]); const pickedCount = selectedRepoIds.length; return ( <> +

+ {CHAT_STRINGS.blockConnectGithubPickHeadline} +

{repos.map((repo) => { const selected = selectedRepoIds.includes(repo.id); @@ -248,11 +437,18 @@ function ConnectedBody({ {CHAT_STRINGS.blockConnectGithubPermissionHelper}

+ {error !== undefined ? ( +

+ {error} +

+ ) : null} +
@@ -265,19 +461,42 @@ function ConnectedBody({ } export function ConnectGithubBlockView(props: ConnectGithubCardProps) { - if (props.kind === "disconnected") { - return ( - - - - ); - } + const currentStepTitle = + props.scene.steps?.[props.scene.currentStepIndex]?.title; return ( - - + + +
+ {currentStepTitle ?? null} +
+
+ {props.kind === "disconnected" ? ( + + ) : null} + {props.kind === "error" ? ( + + ) : null} + {props.kind === "connected" ? : null} + {props.kind === "reviewing" ? ( + + ) : null} +
); } diff --git a/packages/chat-ui/src/index.ts b/packages/chat-ui/src/index.ts index 1c8b4c5a2..50e1aff40 100644 --- a/packages/chat-ui/src/index.ts +++ b/packages/chat-ui/src/index.ts @@ -95,8 +95,11 @@ export { BlockCard } from "./blocks/block-card"; export { ConnectGithubBlockView } from "./blocks/connect-github-block"; export type { + ConnectGithubCardBody, ConnectGithubCardProps, ConnectGithubRepo, + OnboardingScene, + OnboardingSceneStep, } from "./blocks/connect-github-block"; export type { ApprovalActions, diff --git a/packages/chat-ui/src/strings.ts b/packages/chat-ui/src/strings.ts index a2a42543a..f6e6760ce 100644 --- a/packages/chat-ui/src/strings.ts +++ b/packages/chat-ui/src/strings.ts @@ -91,6 +91,8 @@ export const CHAT_STRINGS = { legacyBadgeLabel: "Legacy", eventAgentJoined: (displayName: string) => `${displayName} joined`, eventAgentJoinedUnknown: "An agent joined", + eventAgentsJoined: (displayNames: readonly string[]) => + `${joinWithAnd(displayNames)} joined`, eventMembershipChanged: "Membership updated", eventSettingsChanged: "Settings updated", eventWorkbenchRenamed: (from: string, to: string): string => @@ -212,11 +214,17 @@ export const CHAT_STRINGS = { blockQuestionSubmitting: "Sending…", blockQuestionAnswerError: "Couldn't send your answer — try again.", blockQuestionAnsweredLabel: "Your answer", - blockConnectGithubHeadline: "Connect GitHub", blockConnectGithubPickHeadline: "Pick your repos", + blockConnectGithubStepDone: "Done", + blockConnectGithubStepCurrent: "You're here", + blockConnectGithubReviewingHeadline: "Reviewing", + blockConnectGithubReviewingLine: + "Every new pull request in these gets a review posted right here.", + blockConnectGithubChangeRepos: "change repos", blockConnectGithubIntro: "Connect GitHub with a personal access token — three quick steps, about a minute.", blockConnectGithubAction: "Connect GitHub", + blockConnectGithubReconnect: "Reconnect", blockConnectGithubTokenSteps: [ "Open github.com/settings/tokens and generate a new token.", "Give it the repo scope — that lets agents read code, issues, and pull requests.", @@ -240,6 +248,8 @@ export const CHAT_STRINGS = { blockConnectGithubStartReviewing: (count: number) => `Start reviewing ${count} repo${count === 1 ? "" : "s"}`, blockConnectGithubSkip: "skip for now", + blockConnectGithubStartReviewingError: + "Couldn't start reviewing — try again.", blockConnectGithubTokenFieldLabel: "Personal access token", blockConnectGithubTokenFieldPlaceholder: "ghp_...", blockConnectGithubTokenSubmit: "Connect", diff --git a/packages/chat-ui/src/styles.css b/packages/chat-ui/src/styles.css index c42bb8b84..57df5848b 100644 --- a/packages/chat-ui/src/styles.css +++ b/packages/chat-ui/src/styles.css @@ -1247,7 +1247,12 @@ height: 0.4rem; border-radius: 0; background: currentColor; - animation: chat-block-pulse 1.6s ease infinite; +} + +@media (prefers-reduced-motion: no-preference) { + .chat-block-pulse { + animation: chat-block-pulse 1.6s ease infinite; + } } @keyframes chat-block-pulse { @@ -1703,6 +1708,97 @@ color: var(--destructive); } +/* The room's onboarding scene card: one constant header (what this room + is for, the promise under it, where you are in the walkthrough) above a + body that flips between connect, pick, and reviewing. Step state is + carried in words inside each row, never by color alone — DESIGN.md + State Pills. */ +.chat-block-scene-promise { + margin: 0 0 0.6rem; + font-size: 0.9rem; + line-height: 1.45; + color: var(--foreground); +} + +.chat-block-scene-steps { + margin: 0 0 0.55rem; + padding: 0; + list-style: none; + border: 1px solid var(--border); + background: var(--background); +} + +.chat-block-scene-step { + display: flex; + align-items: baseline; + gap: 0.5rem; + padding: 0.4rem 0.7rem; + border-bottom: 1px solid var(--border); + font-size: 0.8125rem; + color: var(--muted-foreground); +} + +.chat-block-scene-step:last-child { + border-bottom: 0; +} + +.chat-block-scene-step[data-state="current"] { + color: var(--foreground); + font-weight: 650; +} + +.chat-block-scene-step-mark { + margin-left: auto; + font-size: 0.6875rem; + font-weight: 650; + letter-spacing: 0.02em; + text-transform: uppercase; + color: var(--muted-foreground); +} + +.chat-block-scene-step[data-state="current"] .chat-block-scene-step-mark { + color: var(--chat-agent-accent, var(--primary)); +} + +.chat-block-scene-step[data-state="done"] .chat-block-scene-step-mark { + color: var(--ok, #37904a); +} + +.chat-block-scene-why { + margin: 0 0 0.65rem; +} + +.chat-block-scene-status { + position: absolute; + width: 1px; + height: 1px; + padding: 0; + margin: -1px; + overflow: hidden; + clip: rect(0, 0, 0, 0); + white-space: nowrap; + border: 0; +} + +.chat-block-scene-pick-heading { + margin: 0 0 0.7rem; + font-size: 0.8125rem; + font-weight: 650; + line-height: 1.45; +} + +.chat-block-scene-repo-names { + margin: 0 0 0.55rem; + padding: 0; + list-style: none; + display: flex; + flex-wrap: wrap; + gap: 0.35rem 0.6rem; + font-size: 0.875rem; + font-weight: 600; + color: var(--foreground); +} + /* GitHub connect card (CL-6342 screen 2). The row's checkbox control is `@corbits/react-ui`'s own `Checkbox`; only the row layout around it (name left, open-PR count right) is workbench-specific. */ diff --git a/packages/chat-ui/src/timeline.tsx b/packages/chat-ui/src/timeline.tsx index e5dc43866..e06703b59 100644 --- a/packages/chat-ui/src/timeline.tsx +++ b/packages/chat-ui/src/timeline.tsx @@ -578,10 +578,17 @@ function TextBubble({ * anything else falls back to the event name with its separators turned * into spaces. */ -export function friendlyEventText( +/** + * The display name a "workbench.agent-joined" event carries, resolved the + * friendly way: the roster's own handle for that address when the roster + * knows it, else the address's own local part — never the raw address, and + * never the generic "An agent joined", which hides a name the event + * already carries. Undefined only when the event names nobody. + */ +function joinedAgentName( part: Part & { kind: "event" }, participants: readonly ParticipantRecord[], -): string { +): string | undefined { const data = typeof part.data === "object" && part.data !== null ? (part.data as Record) @@ -590,22 +597,28 @@ export function friendlyEventText( data !== undefined && typeof data.address === "string" ? data.address : undefined; - // The participant record's own handle is the friendly, settings-held - // name (see `packages/chat/src/participants.ts`); when the roster - // hasn't caught up with this address yet, the address's own local - // part (CL-6594) is still a real identifier — never the generic "An - // agent joined", which hides a name the event already carries. + if (address === undefined) return undefined; const handle = - address !== undefined - ? (participants.find((participant) => participant.address === address) - ?.handle ?? localPartOf(address)) - : undefined; + participants.find((participant) => participant.address === address) + ?.handle ?? localPartOf(address); + return displayNameFromHandle(handle); +} +export function friendlyEventText( + part: Part & { kind: "event" }, + participants: readonly ParticipantRecord[], +): string { + const data = + typeof part.data === "object" && part.data !== null + ? (part.data as Record) + : undefined; switch (part.event) { - case "workbench.agent-joined": - return handle !== undefined - ? CHAT_STRINGS.eventAgentJoined(displayNameFromHandle(handle)) + case "workbench.agent-joined": { + const name = joinedAgentName(part, participants); + return name !== undefined + ? CHAT_STRINGS.eventAgentJoined(name) : CHAT_STRINGS.eventAgentJoinedUnknown; + } case "workbench.membership-changed": return CHAT_STRINGS.eventMembershipChanged; case "workbench.settings-changed": { @@ -658,10 +671,14 @@ function EventLine({ part, createdAt, participants, + collapsedText, }: { part: Part & { kind: "event" }; createdAt: string; participants: readonly ParticipantRecord[]; + /** The one line that stands in for a whole run of consecutive joins — + * see `collapseAgentJoinRuns`. Undefined on every other event row. */ + collapsedText?: string; }) { const data = typeof part.data === "object" && part.data !== null @@ -677,7 +694,9 @@ function EventLine({ return (
- {connectedDisplayName !== undefined ? ( + {collapsedText !== undefined ? ( + collapsedText + ) : connectedDisplayName !== undefined ? ( <> {CHAT_STRINGS.eventConnectionConnectedBeforePlugins( connectedDisplayName, @@ -965,6 +984,81 @@ export function isSystemNoticeItem(item: MessageItem): boolean { ); } +/** + * A message posted in the room's own voice rather than by any member — + * the `system@` sender the room's onboarding card arrives under. Such a + * row is never "own" for any viewer and never carries author chrome: it + * is the room talking, not a person. + */ +export function isSystemSenderItem(item: MessageItem): boolean { + return ( + item.sender !== undefined && localPartOf(item.sender.address) === "system" + ); +} + +/** The display name a lone agent-joined row names, or undefined when the + * item is anything else. */ +function agentJoinName( + item: TimelineMessageItem, + participants: readonly ParticipantRecord[], +): string | undefined { + if (item.parts.length !== 1) return undefined; + const part = item.parts[0]; + if ( + part === undefined || + part.kind !== "event" || + part.event !== "workbench.agent-joined" + ) { + return undefined; + } + return joinedAgentName(part, participants); +} + +/** + * A room whose whole team arrives at once used to open on a stack of + * "X joined / Y joined / Z joined" — the first thing a person read was a + * membership log. Consecutive joins with nothing between them collapse + * into a single line naming everyone; joins separated by a real message + * stay their own rows, because there the sequence is the point. + * + * Returns the line to render on each run's first item, and the ids of the + * items that line already accounts for. + */ +export function collapseAgentJoinRuns( + items: readonly TimelineMessageItem[], + participants: readonly ParticipantRecord[], +): { + readonly textByLeadId: ReadonlyMap; + readonly absorbedIds: ReadonlySet; +} { + const textByLeadId = new Map(); + const absorbedIds = new Set(); + let index = 0; + while (index < items.length) { + const names: string[] = []; + let end = index; + while (end < items.length) { + const item = items[end]; + if (item === undefined) break; + const name = agentJoinName(item, participants); + if (name === undefined) break; + names.push(name); + end += 1; + } + if (names.length > 1) { + const lead = items[index]; + if (lead !== undefined) { + textByLeadId.set(lead.id, CHAT_STRINGS.eventAgentsJoined(names)); + for (const absorbed of items.slice(index + 1, end)) { + absorbedIds.add(absorbed.id); + } + } + } + index = end > index ? end : index + 1; + } + return { textByLeadId, absorbedIds }; +} + /** * A failed send's inline recovery row (CL-6251/CL-5879): appended below * the bubble text of the exact same message group a confirmed message @@ -1339,6 +1433,7 @@ function MessagePartsInner({ currentUser, showDayDivider, showHeader, + collapsedJoinText, threadMeta, threadAffordanceMode = "reply", onOpenThread, @@ -1367,6 +1462,9 @@ function MessagePartsInner({ /** `false` when this message continues an unbroken run from the same * author as the item directly above it — see `isGroupedWithPrevious`. */ readonly showHeader: boolean; + /** Set only on the first item of a collapsed run of consecutive agent + * joins — see `collapseAgentJoinRuns`. */ + readonly collapsedJoinText?: string; readonly threadMeta?: ThreadAffordanceMeta | undefined; readonly threadAffordanceMode?: ThreadAffordanceMode; readonly onOpenThread?: (messageId: string) => void; @@ -1421,6 +1519,7 @@ function MessagePartsInner({ currentUser !== undefined && item.sender !== undefined && !isSystemNoticeItem(item) && + !isSystemSenderItem(item) && localPartOf(item.sender.address) === currentUser.principalId; const contextMenu = useContextMenuState(); const menu = offersSocialChrome @@ -1512,6 +1611,9 @@ function MessagePartsInner({ part={part} createdAt={item.createdAt} participants={participants} + {...(collapsedJoinText !== undefined + ? { collapsedText: collapsedJoinText } + : {})} /> ); } @@ -1919,6 +2021,8 @@ export function WorkbenchTimeline({ return () => observer.disconnect(); }, []); + const joinRuns = collapseAgentJoinRuns(items, participants); + if (items.length === 0) { // A freshly created agent chat answers before it finishes setting up // in the background: the agent participant streams in seconds later. @@ -1965,6 +2069,7 @@ export function WorkbenchTimeline({ return (
{items.map((item, index) => { + if (joinRuns.absorbedIds.has(item.id)) return null; const previous = index > 0 ? items[index - 1] : undefined; const showDayDivider = previous === undefined || @@ -1996,6 +2101,7 @@ export function WorkbenchTimeline({ previous, showDayDivider, ); + const collapsedJoinText = joinRuns.textByLeadId.get(item.id); return ( { root = null; }); -async function mount(actions: ConnectGithubActions) { +async function mount(actions: ConnectGithubActions, messageId = "m1") { container = document.createElement("div"); document.body.appendChild(container); root = createRoot(container); @@ -47,7 +47,7 @@ async function mount(actions: ConnectGithubActions) { root?.render( , ); @@ -140,6 +140,41 @@ describe("ConnectGithubBlockContainer post-submit refresh (CL-6463)", () => { ); }); + test("a successful PAT submit moves focus onto the pick-repos heading", async () => { + const harness = buildNeverNotifiesHarness(); + const el = await mount(harness.actions, "m_pat_focus"); + + const connectButton = [...el.querySelectorAll("button")].find( + (button) => button.textContent === "Connect GitHub", + ) as HTMLButtonElement; + await act(async () => { + connectButton.click(); + }); + const tokenField = el.querySelector( + "#connect-github-token", + ) as HTMLInputElement; + await act(async () => { + typeInto(tokenField, "ghp_test123"); + }); + const submitButton = [...el.querySelectorAll("button")].find( + (button) => button.textContent === "Connect", + ) as HTMLButtonElement; + await act(async () => { + submitButton.focus(); + submitButton.click(); + }); + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); + + const heading = el.querySelector(".chat-block-scene-pick-heading"); + expect(heading?.textContent).toBe("Pick your repos"); + expect(heading?.getAttribute("tabindex")).toBe("-1"); + expect(document.activeElement).toBe(heading); + expect(el.querySelector("#connect-github-token")).toBeNull(); + }); + test("a rejected token shows what went wrong and leaves a working submit button, never a dead card", async () => { const harness = buildNeverNotifiesHarness({ submitResult: { ok: false, message: "That token looks expired." }, @@ -259,3 +294,32 @@ describe("ConnectGithubBlockContainer keeps connected across loading (CL-6741)", expect(el.textContent).not.toContain("Connected to GitHub as"); }); }); + +describe("ConnectGithubBlockContainer names a kind:error state (PR 422)", () => { + test("query kind error shows the state message as an alert, not a silent Connect GitHub primary", async () => { + const message = "Couldn't read your GitHub repositories. Try reconnecting."; + const actions: ConnectGithubActions = { + getConnectState: () => Promise.resolve({ kind: "error", message }), + subscribeConnectState: () => () => {}, + requestConnect: () => {}, + submitAccessToken: async () => ({ ok: true as const }), + startReviewing: async () => ({ startedTriggerCount: 0 }), + skip: async () => {}, + }; + + const el = await mount(actions, "m_state_error"); + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); + + const alert = el.querySelector('[role="alert"]'); + expect(alert).not.toBeNull(); + expect(alert?.textContent).toBe(message); + + const connectPrimary = [...el.querySelectorAll("button")].find( + (button) => button.textContent === "Connect GitHub", + ); + expect(connectPrimary).toBeUndefined(); + }); +}); diff --git a/packages/chat-ui/test/connect-github-block.test.tsx b/packages/chat-ui/test/connect-github-block.test.tsx index f60816fe7..1e020a023 100644 --- a/packages/chat-ui/test/connect-github-block.test.tsx +++ b/packages/chat-ui/test/connect-github-block.test.tsx @@ -17,8 +17,9 @@ import { createRoot } from "react-dom/client"; import type { Root } from "react-dom/client"; import type { - ConnectGithubCardProps, + ConnectGithubCardBody, ConnectGithubRepo, + OnboardingScene, } from "../src/blocks/connect-github-block"; import { ConnectGithubBlockView } from "../src/blocks/connect-github-block"; @@ -31,6 +32,20 @@ const REPOS: readonly ConnectGithubRepo[] = [ { id: "handbook", name: "acme/handbook", openPullRequestCount: 0 }, ]; +/** The framing the room's onboarding card always carries. Individual + * tests override `currentStepIndex` where the marker is what's under + * test. */ +const SCENE: OnboardingScene = { + title: "Code review", + promise: "Every new pull request gets reviewed before you merge it.", + steps: [ + { title: "Connect GitHub", why: "Reviewers need to read your code." }, + { title: "Pick your repos", why: "Only the repos you pick are watched." }, + { title: "Start reviewing", why: "Reviews land right here in this room." }, + ], + currentStepIndex: 0, +}; + let container: HTMLDivElement | null = null; let root: Root | null = null; @@ -51,8 +66,8 @@ async function mountElement(element: ReactElement) { return container; } -async function mount(props: ConnectGithubCardProps) { - return mountElement(); +async function mount(props: ConnectGithubCardBody) { + return mountElement(); } function typeInto(element: HTMLInputElement, text: string) { @@ -80,6 +95,7 @@ function PickReposHarness({ useState(initiallySelected); return ( { connect?.click(); }); - const steps = [...el.querySelectorAll("ol li")].map( + const steps = [...el.querySelectorAll(".chat-block-connect-steps li")].map( (item) => item.textContent, ); expect(steps).toHaveLength(3); @@ -233,6 +249,10 @@ describe("connect GitHub card — 2a disconnected", () => { expect(el.textContent).toContain("Bad token."); expect(el.querySelector("#connect-github-token")).not.toBeNull(); + expect( + el.querySelector(".chat-block-connect-token-error")?.getAttribute("role"), + ).toBe("alert"); + expect(field.getAttribute("aria-invalid")).toBe("true"); }); }); @@ -339,6 +359,11 @@ describe("connect GitHub card — 2b pick your repos", () => { button.textContent?.startsWith("Start reviewing"), ) as HTMLButtonElement; expect(start.textContent).toBe("Start reviewing 0 repos"); + expect(start.disabled).toBe(true); + const skip = [...el.querySelectorAll("button")].find( + (button) => button.textContent === "skip for now", + ) as HTMLButtonElement; + expect(skip.disabled).toBe(false); const selectAll = [...el.querySelectorAll("button")].find( (button) => button.textContent === "Select all", @@ -352,6 +377,7 @@ describe("connect GitHub card — 2b pick your repos", () => { button.textContent?.startsWith("Start reviewing"), ) as HTMLButtonElement; expect(startAfter.textContent).toBe("Start reviewing 6 repos"); + expect(startAfter.disabled).toBe(false); }); test("skip fires its own quiet callback, distinct from starting", async () => { @@ -406,7 +432,12 @@ describe("connect GitHub card — accessibility", () => { } const group = el.querySelector('[role="group"]'); - expect(group?.getAttribute("aria-label")).toBe("Pick your repos"); + expect( + el.querySelector(".chat-block-scene-pick-heading")?.textContent, + ).toBe("Pick your repos"); + expect(group?.getAttribute("aria-labelledby")).toBe( + "connect-github-pick-heading", + ); const firstCheckbox = checkboxes[0]; if (firstCheckbox === undefined) { @@ -416,6 +447,144 @@ describe("connect GitHub card — accessibility", () => { expect(document.activeElement).toBe(firstCheckbox); }); + test("the current walkthrough step is marked aria-current=step and a sibling status live region names it", async () => { + const el = await mount({ + kind: "disconnected", + onConnect: () => undefined, + onSubmitAccessToken: () => Promise.resolve({ ok: true }), + }); + + const current = [...el.querySelectorAll(".chat-block-scene-step")].find( + (row) => row.getAttribute("data-state") === "current", + ); + expect(current?.getAttribute("aria-current")).toBe("step"); + expect( + [...el.querySelectorAll(".chat-block-scene-step")].filter( + (row) => row.getAttribute("aria-current") === "step", + ), + ).toHaveLength(1); + const status = el.querySelector(".chat-block-scene-status"); + const body = el.querySelector(".chat-block-scene-body"); + expect(body?.getAttribute("aria-live")).toBeNull(); + expect(status?.getAttribute("aria-live")).toBe("polite"); + expect(status?.getAttribute("aria-atomic")).toBe("true"); + expect(status?.textContent).toBe("Connect GitHub"); + expect(body?.contains(status)).toBe(false); + expect( + el.querySelector("#connect-github-token") ?? + el.querySelector('input[type="password"]'), + ).toBeNull(); + }); + + test("a token error is an alert beside the status live region, not nested inside it", async () => { + const el = await mount({ + kind: "disconnected", + onConnect: () => undefined, + onSubmitAccessToken: () => + Promise.resolve({ ok: false, message: "Bad token." }), + }); + + const openLink = [...el.querySelectorAll("button")].find( + (button) => button.textContent === "Connect GitHub", + ) as HTMLButtonElement; + await act(async () => { + openLink.click(); + }); + + const field = el.querySelector("#connect-github-token") as HTMLInputElement; + await act(async () => { + typeInto(field, "ghp_bad"); + }); + const submit = [...el.querySelectorAll("button")].find( + (button) => button.textContent === "Connect", + ) as HTMLButtonElement; + await act(async () => { + submit.click(); + }); + await act(async () => { + await Promise.resolve(); + }); + + const status = el.querySelector(".chat-block-scene-status"); + const alert = el.querySelector('[role="alert"]'); + expect(alert?.textContent).toContain("Bad token."); + expect(status?.contains(alert)).toBe(false); + expect(status?.textContent).toBe("Connect GitHub"); + expect(el.querySelector(".chat-block-scene-body")?.contains(field)).toBe( + true, + ); + }); + + test("a start-reviewing error is an alert beside the status live region, not nested inside it", async () => { + const el = await mountElement( + undefined} + onSelectAll={() => undefined} + onChangeConnection={() => undefined} + onStartReviewing={() => undefined} + onSkip={() => undefined} + error="Couldn't start reviewing — try again." + />, + ); + + const status = el.querySelector(".chat-block-scene-status"); + const alert = el.querySelector('[role="alert"]'); + expect(status?.textContent).toBe("Pick your repos"); + expect(alert?.textContent).toContain("Couldn't start reviewing"); + expect(status?.contains(alert)).toBe(false); + expect( + el + .querySelector(".chat-block-scene-body") + ?.contains(el.querySelector('input[type="checkbox"]')), + ).toBe(true); + }); + + test("toggling a repo checkbox does not change the status live region", async () => { + const el = await mountElement( + undefined} + />, + ); + const status = el.querySelector(".chat-block-scene-status"); + expect(status?.textContent).toBe("Connect GitHub"); + + const mobileCheckbox = [ + ...el.querySelectorAll('input[type="checkbox"]'), + ][3]; + await act(async () => { + mobileCheckbox?.click(); + }); + + expect(status?.textContent).toBe("Connect GitHub"); + expect(el.textContent).toContain("6 repos found · 2 picked"); + }); + + test("autoFocus on the pick-repos scene moves focus onto the pick heading", async () => { + const el = await mount({ + kind: "connected", + orgName: "acme", + repos: REPOS, + selectedRepoIds: [], + onToggleRepo: () => undefined, + onSelectAll: () => undefined, + onChangeConnection: () => undefined, + onStartReviewing: () => undefined, + onSkip: () => undefined, + autoFocus: true, + }); + + const heading = el.querySelector(".chat-block-scene-pick-heading"); + expect(heading?.textContent).toBe("Pick your repos"); + expect(heading?.getAttribute("tabindex")).toBe("-1"); + expect(document.activeElement).toBe(heading); + }); + test("the disconnected state's actions are real buttons, not divs", async () => { const el = await mount({ kind: "disconnected", diff --git a/packages/chat-ui/test/connect-github-flow.test.tsx b/packages/chat-ui/test/connect-github-flow.test.tsx index 14175c4a8..665e539ab 100644 --- a/packages/chat-ui/test/connect-github-flow.test.tsx +++ b/packages/chat-ui/test/connect-github-flow.test.tsx @@ -250,7 +250,12 @@ describe("connect-github round trip (CL-6345)", () => { expect(harness.getConnectStateCallCount()).toBe( getConnectStateCallCountBeforeStart, ); - expect(el.textContent).toContain("Connected to GitHub as octocat"); - expect(el.textContent).toContain("4 repos found · 3 picked"); + // Reviewing, not "pick your repos" again and never "Connect": the + // server recorded the selection, so the same card now names what is + // under review. + expect(el.textContent).toContain("Reviewing"); + expect(el.textContent).toContain("acme/checkout"); + expect(el.textContent).toContain("acme/web"); + expect(el.textContent).not.toContain("Connect"); }); }); diff --git a/packages/chat-ui/test/onboarding-scene-card.test.tsx b/packages/chat-ui/test/onboarding-scene-card.test.tsx new file mode 100644 index 000000000..f0c8fc5a3 --- /dev/null +++ b/packages/chat-ui/test/onboarding-scene-card.test.tsx @@ -0,0 +1,389 @@ +// The room's first minute: one designed card posted in the room's own +// voice that names the job, walks the person through connect → pick → +// reviewing, and lands on a real done state. Covers the card's chrome (no +// author row, no "Member", no avatar), the header the card keeps across +// every state, and the walkthrough marker each live connect state puts on +// it. + +import { afterEach, describe, expect, mock, spyOn, test } from "bun:test"; +import { act } from "react"; +import { createRoot } from "react-dom/client"; +import type { Root } from "react-dom/client"; +import * as errorSink from "@corbits/error-sink"; + +import type { MessageItem } from "../src/api"; +import type { + ConnectGithubActions, + ConnectGithubQuery, + ConnectGithubRepo, +} from "../src/blocks/connect-github-actions"; +import { ConnectGithubBlockContainer } from "../src/blocks/connect-github-block-container"; +import { WorkbenchTimeline } from "../src/timeline"; + +const STEPS: { title: string; why: string }[] = [ + { title: "Connect GitHub", why: "Reviewers need to read your code." }, + { title: "Pick your repos", why: "Only the repos you pick are watched." }, + { title: "Start reviewing", why: "Reviews land right here in this room." }, +]; + +const PROMISE = "Every new pull request gets reviewed before you merge it."; + +const REPOS: readonly ConnectGithubRepo[] = [ + { id: "1", name: "acme/checkout", openPullRequestCount: 2 }, + { id: "2", name: "acme/web", openPullRequestCount: 0 }, +]; + +let container: HTMLDivElement | null = null; +let root: Root | null = null; + +afterEach(() => { + if (root !== null) act(() => root?.unmount()); + container?.remove(); + container = null; + root = null; + mock.restore(); +}); + +async function mount(element: React.ReactElement) { + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + await act(async () => { + root?.render(element); + }); + return container; +} + +function onboardingCardItem(data: unknown): readonly MessageItem[] { + return [ + { + id: "m_onboarding", + createdAt: "2026-01-01T00:00:00.000Z", + sender: { name: null, address: "system@wb_1" }, + parts: [{ kind: "block", block: { type: "connect-github", data } }], + }, + ]; +} + +function stepRows(el: HTMLElement) { + return [...el.querySelectorAll(".chat-block-scene-step")]; +} + +function stepTitles(el: HTMLElement) { + return stepRows(el).map( + (row) => + row.querySelector(".chat-block-scene-step-title")?.textContent ?? "", + ); +} + +function currentStepTitle(el: HTMLElement): string | undefined { + const current = stepRows(el).find( + (row) => row.getAttribute("data-state") === "current", + ); + return ( + current?.querySelector(".chat-block-scene-step-title")?.textContent ?? + undefined + ); +} + +function currentStepAria(el: HTMLElement): string | null { + const current = stepRows(el).find( + (row) => row.getAttribute("aria-current") === "step", + ); + return ( + current?.querySelector(".chat-block-scene-step-title")?.textContent ?? null + ); +} + +/** A host whose live state is whatever the test says it is, pushed once + * on mount — the same read-then-subscribe contract the real + * `createChatConnectGithubActions` binds. */ +function fixedStateActions(state: ConnectGithubQuery): ConnectGithubActions { + return { + getConnectState: () => Promise.resolve(state), + subscribeConnectState: () => () => undefined, + requestConnect: () => undefined, + submitAccessToken: () => Promise.resolve({ ok: true as const }), + startReviewing: () => Promise.resolve({ startedTriggerCount: 0 }), + skip: () => Promise.resolve(), + }; +} + +describe("the room's onboarding card is a scene, not a member's message", () => { + test("a card posted in the room's own voice carries no author row, no avatar, and no Member label", async () => { + const el = await mount( + , + ); + expect(el.querySelector(".chat-block")).not.toBeNull(); + expect(el.querySelector(".chat-sender-avatar-wrap")).toBeNull(); + expect(el.querySelector(".chat-bubble-row")).toBeNull(); + expect(el.textContent).not.toContain("Member"); + expect(el.textContent).not.toContain("system"); + expect( + el.querySelector(".chat-message-group")?.getAttribute("data-own"), + ).toBe("false"); + }); + + test("the card names the job, promises the outcome, and lists the room's own step labels in order", async () => { + const el = await mount( + , + ); + expect(el.querySelector(".chat-block-title")?.textContent).toBe( + "Code review", + ); + expect(el.querySelector(".chat-block-scene-promise")?.textContent).toBe( + PROMISE, + ); + expect(stepTitles(el)).toEqual([ + "Connect GitHub", + "Pick your repos", + "Start reviewing", + ]); + }); + + test("a card persisted without a promise or steps renders the card without either — never invented copy", async () => { + const el = await mount( + , + ); + expect(el.querySelector(".chat-block-title")?.textContent).toBe( + "Code review", + ); + expect(el.querySelector(".chat-block-scene-promise")).toBeNull(); + expect(el.querySelector(".chat-block-scene-steps")).toBeNull(); + expect(el.querySelector(".chat-block-scene-why")).toBeNull(); + }); + + test("an empty steps list does not render the step list", async () => { + const el = await mount( + , + ); + expect(el.querySelector(".chat-block-scene-promise")?.textContent).toBe( + PROMISE, + ); + expect(el.querySelector(".chat-block-scene-steps")).toBeNull(); + }); + + test("a walkthrough of some other length still shows its labels, but marks no step", async () => { + const el = await mount( + , + ); + expect(stepTitles(el)).toEqual(["Connect GitHub", "Pick your repos"]); + expect(currentStepTitle(el)).toBeUndefined(); + expect(el.querySelector(".chat-block-scene-why")).toBeNull(); + }); +}); + +describe("the walkthrough marker follows the live connect state", () => { + const DATA = { + requiredForTemplate: "Code review", + state: "disconnected" as const, + promise: PROMISE, + steps: STEPS, + }; + + async function mountAt(messageId: string, state: ConnectGithubQuery) { + return mount( + , + ); + } + + test("nothing connected yet marks Connect GitHub and explains why that step matters", async () => { + const el = await mountAt("m_step1", { kind: "disconnected" }); + expect(currentStepTitle(el)).toBe("Connect GitHub"); + expect(currentStepAria(el)).toBe("Connect GitHub"); + expect(el.querySelector(".chat-block-scene-why")?.textContent).toBe( + STEPS[0]?.why, + ); + const status = el.querySelector(".chat-block-scene-status"); + expect(status?.getAttribute("aria-live")).toBe("polite"); + expect(status?.textContent).toBe("Connect GitHub"); + expect( + el.querySelector(".chat-block-scene-body")?.getAttribute("aria-live"), + ).toBeNull(); + expect(el.querySelector(".chat-block-scene-body")?.contains(status)).toBe( + false, + ); + }); + + test("connected with nothing recorded yet marks Pick your repos", async () => { + const el = await mountAt("m_step2", { + kind: "connected", + orgName: "acme", + repos: REPOS, + selectedRepoIds: [], + }); + expect(currentStepTitle(el)).toBe("Pick your repos"); + expect(currentStepAria(el)).toBe("Pick your repos"); + expect(el.querySelector(".chat-block-scene-why")?.textContent).toBe( + STEPS[1]?.why, + ); + expect(el.textContent).toContain("acme/checkout"); + }); + + test("repos the server actually recorded mark Start reviewing and show the done state, never Connect", async () => { + const el = await mountAt("m_step3", { + kind: "connected", + orgName: "acme", + repos: REPOS, + selectedRepoIds: ["1", "2"], + }); + expect(currentStepTitle(el)).toBe("Start reviewing"); + expect(currentStepAria(el)).toBe("Start reviewing"); + const done = el.querySelector(".chat-block-scene-reviewing"); + expect(done?.textContent).toContain("Reviewing"); + const names = [ + ...el.querySelectorAll(".chat-block-scene-repo-names li"), + ].map((item) => item.textContent); + expect(names).toEqual(["acme/checkout", "acme/web"]); + expect(done?.textContent).toContain("change repos"); + // The done state never re-offers connecting — the card is past it. + expect(done?.textContent).not.toContain("Connect"); + }); + + test("change repos hands the picker back without touching what the server recorded", async () => { + const el = await mountAt("m_step4", { + kind: "connected", + orgName: "acme", + repos: REPOS, + selectedRepoIds: ["1"], + }); + const changeRepos = [...el.querySelectorAll("button")].find( + (button) => button.textContent === "change repos", + ); + expect(changeRepos).not.toBeUndefined(); + await act(async () => { + changeRepos?.click(); + }); + expect(el.textContent).toContain("2 repos found · 1 picked"); + expect(el.querySelector(".chat-block-title")?.textContent).toBe( + "Code review", + ); + expect(currentStepTitle(el)).toBe("Pick your repos"); + expect(el.querySelector(".chat-block-scene-why")?.textContent).toBe( + STEPS[1]?.why, + ); + }); + + test("a rejected start reviewing after change repos keeps the picker and reports the error", async () => { + const report = spyOn(errorSink, "reportError").mockReturnValue("ref_test"); + const actions: ConnectGithubActions = { + ...fixedStateActions({ + kind: "connected", + orgName: "acme", + repos: REPOS, + selectedRepoIds: ["1"], + }), + startReviewing: () => Promise.reject(new Error("could not start")), + }; + const el = await mount( + , + ); + const changeRepos = [...el.querySelectorAll("button")].find( + (button) => button.textContent === "change repos", + ); + await act(async () => { + changeRepos?.click(); + }); + expect(el.textContent).toContain("2 repos found · 1 picked"); + + const start = [...el.querySelectorAll("button")].find((button) => + button.textContent?.startsWith("Start reviewing"), + ); + await act(async () => { + start?.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + + expect(el.querySelector(".chat-block-scene-reviewing")).toBeNull(); + expect(el.textContent).toContain("2 repos found · 1 picked"); + expect(currentStepTitle(el)).toBe("Pick your repos"); + const alert = el.querySelector('[role="alert"]'); + expect(alert).not.toBeNull(); + expect(alert?.textContent).toContain("Couldn't start reviewing"); + const status = el.querySelector(".chat-block-scene-status"); + expect(status?.textContent).toBe("Pick your repos"); + expect(status?.contains(alert)).toBe(false); + expect(report).toHaveBeenCalled(); + expect(report.mock.calls[0]?.[1]).toMatchObject({ + operation: "connect-github.startReviewing", + }); + }); + + test("a successful start reviewing after change repos moves focus onto the reviewing scene", async () => { + const el = await mount( + , + ); + const changeRepos = [...el.querySelectorAll("button")].find( + (button) => button.textContent === "change repos", + ); + await act(async () => { + changeRepos?.click(); + }); + const start = [...el.querySelectorAll("button")].find((button) => + button.textContent?.startsWith("Start reviewing"), + ); + await act(async () => { + start?.focus(); + start?.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + + const reviewing = el.querySelector(".chat-block-scene-reviewing"); + expect(reviewing).not.toBeNull(); + expect(currentStepAria(el)).toBe("Start reviewing"); + expect(reviewing?.contains(document.activeElement)).toBe(true); + }); +}); diff --git a/packages/chat-ui/test/system-join-edge.test.tsx b/packages/chat-ui/test/system-join-edge.test.tsx index 6281d1689..c61ec6da7 100644 --- a/packages/chat-ui/test/system-join-edge.test.tsx +++ b/packages/chat-ui/test/system-join-edge.test.tsx @@ -50,6 +50,30 @@ async function mount(items: readonly MessageItem[], currentUser?: CurrentUser) { return container; } +function joinItemFor(id: string, agentAddress: string): MessageItem { + return { + id, + createdAt: "2026-01-01T00:00:00.000Z", + sender: { name: null, address: "sawyer@agents.example" }, + parts: [ + { + kind: "event", + event: "workbench.agent-joined", + data: { address: agentAddress }, + }, + ], + }; +} + +function textItem(id: string): MessageItem { + return { + id, + createdAt: "2026-01-01T00:00:00.000Z", + sender: { name: "Sawyer", address: "sawyer@agents.example" }, + parts: [{ kind: "text", text: "Morning" }], + }; +} + function joinItem(senderAddress: string): MessageItem { return { id: "join_1", @@ -101,3 +125,32 @@ describe("CL-6772: join / system notices stay on the left edge", () => { expect(block?.[0]).toMatch(/2\.9rem/); }); }); + +describe("a whole team arriving reads as one line, not a join dump", () => { + test("three consecutive joins collapse into one line naming everyone", async () => { + const el = await mount([ + joinItemFor("j1", "correctness-reviewer@agents.example"), + joinItemFor("j2", "architecture-reviewer@agents.example"), + joinItemFor("j3", "release-risk-reviewer@agents.example"), + ]); + const lines = [...el.querySelectorAll(".chat-event-line")]; + expect(lines).toHaveLength(1); + const text = lines[0]?.textContent ?? ""; + expect(text).toContain("Correctness Reviewer"); + expect(text).toContain("Architecture Reviewer"); + expect(text).toContain("Release Risk Reviewer"); + expect(text.match(/joined/g)).toHaveLength(1); + }); + + test("joins separated by a real message stay their own rows", async () => { + const el = await mount([ + joinItemFor("j1", "correctness-reviewer@agents.example"), + textItem("t1"), + joinItemFor("j2", "architecture-reviewer@agents.example"), + ]); + const lines = [...el.querySelectorAll(".chat-event-line")]; + expect(lines).toHaveLength(2); + expect(lines[0]?.textContent).toContain("Correctness Reviewer joined"); + expect(lines[1]?.textContent).toContain("Architecture Reviewer joined"); + }); +}); diff --git a/packages/code-review/README.md b/packages/code-review/README.md index 73b9f15bc..6045a1be3 100644 --- a/packages/code-review/README.md +++ b/packages/code-review/README.md @@ -18,6 +18,13 @@ Three reviewers, each a lens rather than a whole opinion: requests, so the roster a review fans out to is the roster a person can see and edit in the workbench. +Each reviewer also carries a canned `introduction` — one or two sentences +in its own voice, naming the job and the repos it will watch. +`reviewerIntroductions(repoNames)` (`src/introductions.ts`, +`@corbits/code-review/introductions`) renders all three in roster order; +a host posts them once a person starts reviewing, so the room shows who +just came online without running any inference. + ## The report contract Every reviewer replies under `REVIEWER_REPORT_CONTRACT` diff --git a/packages/code-review/package.json b/packages/code-review/package.json index 98b3eeedd..d711a9bb0 100644 --- a/packages/code-review/package.json +++ b/packages/code-review/package.json @@ -8,7 +8,8 @@ "exports": { ".": "./src/index.ts", "./reviewers": "./src/reviewers.ts", - "./agent-requests": "./src/agent-requests.ts" + "./agent-requests": "./src/agent-requests.ts", + "./introductions": "./src/introductions.ts" }, "scripts": { "typecheck": "tsc --noEmit", diff --git a/packages/code-review/src/index.ts b/packages/code-review/src/index.ts index f8f16cc86..79b02612c 100644 --- a/packages/code-review/src/index.ts +++ b/packages/code-review/src/index.ts @@ -23,6 +23,10 @@ export { reviewerById, type ReviewerDefinition, } from "./reviewers"; +export { + reviewerIntroductions, + type ReviewerIntroduction, +} from "./introductions"; export { runPullRequestReview, type CodeReviewGitHub, diff --git a/packages/code-review/src/introductions.test.ts b/packages/code-review/src/introductions.test.ts new file mode 100644 index 000000000..2130d94f4 --- /dev/null +++ b/packages/code-review/src/introductions.test.ts @@ -0,0 +1,32 @@ +import { describe, expect, test } from "bun:test"; + +import { CODE_REVIEW_REVIEWERS } from "./reviewers"; +import { reviewerIntroductions } from "./introductions"; + +const REPO_NAMES = ["acme/widgets", "acme/gadgets"]; + +describe("reviewerIntroductions", () => { + test("returns one introduction per reviewer, in roster order", () => { + const introductions = reviewerIntroductions(REPO_NAMES); + expect(introductions.map((introduction) => introduction.handle)).toEqual( + CODE_REVIEW_REVIEWERS.map((reviewer) => reviewer.handle), + ); + }); + + test("every introduction names a picked repo and carries no internal detail", () => { + const introductions = reviewerIntroductions(REPO_NAMES); + for (const { handle, text } of introductions) { + expect(REPO_NAMES.some((name) => text.includes(name))).toBe(true); + expect(text).not.toContain("@"); + expect(text.toLowerCase()).not.toContain("http"); + expect(text).not.toContain(handle); + } + }); + + test("names the single picked repo when only one is selected", () => { + const introductions = reviewerIntroductions(["acme/widgets"]); + for (const { text } of introductions) { + expect(text).toContain("acme/widgets"); + } + }); +}); diff --git a/packages/code-review/src/introductions.ts b/packages/code-review/src/introductions.ts new file mode 100644 index 000000000..134ead986 --- /dev/null +++ b/packages/code-review/src/introductions.ts @@ -0,0 +1,21 @@ +// The canned copy each reviewer posts once repos are picked and it +// starts reviewing — the identity beat of the first minute. Pure +// formatting over `./reviewers.ts`'s own `introduction` templates; no +// inference runs to produce these, exactly like `@corbits/chat`'s +// `postCannedGreeting`. +import { CODE_REVIEW_REVIEWERS } from "./reviewers"; + +export interface ReviewerIntroduction { + readonly handle: string; + readonly text: string; +} + +/** One introduction per reviewer, in `CODE_REVIEW_REVIEWERS` order. */ +export function reviewerIntroductions( + repoNames: readonly string[], +): readonly ReviewerIntroduction[] { + return CODE_REVIEW_REVIEWERS.map((reviewer) => ({ + handle: reviewer.handle, + text: reviewer.introduction(repoNames), + })); +} diff --git a/packages/code-review/src/reviewers.ts b/packages/code-review/src/reviewers.ts index 25d1bc8d3..1acac760e 100644 --- a/packages/code-review/src/reviewers.ts +++ b/packages/code-review/src/reviewers.ts @@ -52,6 +52,24 @@ export interface ReviewerDefinition { readonly displayName: string; readonly description: string; readonly systemPrompt: string; + /** The canned message this reviewer posts, in its own voice, once + * repos are picked and it starts reviewing — who it is and what it + * will do for these repos. Never run through inference; see + * `./introductions.ts`'s `reviewerIntroductions`. */ + readonly introduction: (repoNames: readonly string[]) => string; +} + +/** Renders a list of repo names the way a sentence names them: + * "widgets", "widgets and gadgets", or "widgets, gadgets, and sprockets". */ +function namedRepos(repoNames: readonly string[]): string { + if (repoNames.length === 0) return "these repos"; + const [first, ...rest] = repoNames; + if (first === undefined) return "these repos"; + if (rest.length === 0) return first; + const last = rest[rest.length - 1]; + if (rest.length === 1) return `${first} and ${last}`; + const middle = [first, ...rest.slice(0, -1)].join(", "); + return `${middle}, and ${last}`; } const ARCHITECTURE_REVIEWER: ReviewerDefinition = { @@ -75,6 +93,11 @@ const ARCHITECTURE_REVIEWER: ReviewerDefinition = { "Out of lane: style-only nitpicking, and speculative redesigns of " + "code this change did not touch.\n\n" + REVIEWER_REPORT_CONTRACT, + introduction: (repoNames) => + `I'm the architecture reviewer. I'll read every pull request on ` + + `${namedRepos(repoNames)} for whether the shape holds up: the ` + + `invariants, where a constraint should live, and what it costs to ` + + `maintain later.`, }; const CORRECTNESS_REVIEWER: ReviewerDefinition = { @@ -102,6 +125,10 @@ const CORRECTNESS_REVIEWER: ReviewerDefinition = { "became a promise, a changed parameter order, a return type that " + "narrowed — is blocking.\n\n" + REVIEWER_REPORT_CONTRACT, + introduction: (repoNames) => + `I'm the correctness reviewer. I'll read every pull request opened ` + + `on ${namedRepos(repoNames)} for defects, with the file, the line, ` + + `and the input that trips them.`, }; const RELEASE_RISK_REVIEWER: ReviewerDefinition = { @@ -122,6 +149,10 @@ const RELEASE_RISK_REVIEWER: ReviewerDefinition = { "no is worth more than a late surprise. Sequencing, rollout order, " + "and what has to be true before this lands are yours to raise.\n\n" + REVIEWER_REPORT_CONTRACT, + introduction: (repoNames) => + `I'm the release-risk reviewer. I'll weigh in on pull requests to ` + + `${namedRepos(repoNames)}, saying plainly what actually blocks ` + + `shipping, what ships with a note, and what can wait.`, }; /** The roster a review fans out to, in the order findings are reported. */ diff --git a/packages/workflow-catalog/package.json b/packages/workflow-catalog/package.json index 5a5b36974..46331677f 100644 --- a/packages/workflow-catalog/package.json +++ b/packages/workflow-catalog/package.json @@ -17,6 +17,7 @@ "dependencies": { "@corbits/code-review": "workspace:*", "@corbits/code-review-workflow": "workspace:*", + "@corbits/error-sink": "workspace:*", "@corbits/github-tools": "workspace:*", "@corbits/jimmy-agent": "workspace:*", "@corbits/scout-agent": "workspace:*", diff --git a/packages/workflow-catalog/src/connect-github-credential-link.test.ts b/packages/workflow-catalog/src/connect-github-credential-link.test.ts index 940ca780f..1a4cb8da4 100644 --- a/packages/workflow-catalog/src/connect-github-credential-link.test.ts +++ b/packages/workflow-catalog/src/connect-github-credential-link.test.ts @@ -146,6 +146,7 @@ describe("the room GitHub connect card reads what its own submit writes", () => selectedRepos: [], }), persistSelectedRepos: async () => {}, + onReviewingStarted: async () => {}, listReposFn: async () => [], fetchAuthenticatedLoginFn: async () => "octocat", }); @@ -188,6 +189,7 @@ describe("the room GitHub connect card reads what its own submit writes", () => selectedRepos: [], }), persistSelectedRepos: async () => {}, + onReviewingStarted: async () => {}, listReposFn: async () => [], fetchAuthenticatedLoginFn: async () => "octocat", }); diff --git a/packages/workflow-catalog/src/connect-github-routes.test.ts b/packages/workflow-catalog/src/connect-github-routes.test.ts index b2e8ba34e..255e0e73c 100644 --- a/packages/workflow-catalog/src/connect-github-routes.test.ts +++ b/packages/workflow-catalog/src/connect-github-routes.test.ts @@ -3,10 +3,11 @@ // mounted the same way `packages/connections/src/routes.test.ts` mounts // its own routes: a bare `Hono` with a tenant-injecting middleware, no // real network, no real database — every port is a plain fake. -import { describe, expect, test } from "bun:test"; +import { afterEach, describe, expect, mock, spyOn, test } from "bun:test"; import { Hono } from "hono"; import type { MiddlewareHandler } from "hono"; import type { RequireGrant, TenantEnv } from "@intx/hub-api"; +import * as errorSink from "@corbits/error-sink"; import type { GitHubClientConfig, GitHubRepoSummary, @@ -62,6 +63,12 @@ function mountAs(routes: Hono): Hono { function buildApp(overrides: Partial = {}) { const grants: { tenantId: string; repo: GitHubRepoSummary }[] = []; const triggers: { tenantId: string; repo: GitHubRepoSummary }[] = []; + const introductionCalls: { + tenantId: string; + workbenchId: string; + principalId: string; + introductions: readonly { handle: string; text: string }[]; + }[] = []; let settings: { pendingConnections: readonly string[]; selectedRepos: readonly string[]; @@ -97,6 +104,19 @@ function buildApp(overrides: Partial = {}) { selectedRepos: patch["template/selectedRepos"], }; }, + onReviewingStarted: async ( + tenantId, + workbenchId, + principalId, + introductions, + ) => { + introductionCalls.push({ + tenantId, + workbenchId, + principalId, + introductions, + }); + }, listReposFn: async () => REPOS, fetchAuthenticatedLoginFn: async () => "octocat", ...overrides, @@ -107,10 +127,15 @@ function buildApp(overrides: Partial = {}) { app: mountAs(routes), grants, triggers, + introductionCalls, settingsNow: () => settings, }; } +afterEach(() => { + mock.restore(); +}); + describe("GET /:workbenchId/github/state", () => { test("reports disconnected with no github credential", async () => { const { app } = buildApp({ resolveGithubConfig: async () => undefined }); @@ -172,6 +197,68 @@ describe("POST /:workbenchId/github/start-reviewing", () => { }); }); + test("posts each reviewer's introduction once, naming the selected repos, after success", async () => { + const harness = buildApp(); + const response = await harness.app.request("/wb_1/github/start-reviewing", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ repoIds: ["1", "2"] }), + }); + expect(response.status).toBe(200); + expect(harness.introductionCalls.length).toBe(1); + const [call] = harness.introductionCalls; + if (call === undefined) throw new Error("expected one introduction call"); + expect(call.tenantId).toBe("tnt_1"); + expect(call.workbenchId).toBe("wb_1"); + expect(call.principalId).toBe("prn_alice"); + expect(call.introductions.length).toBe(3); + for (const introduction of call.introductions) { + expect( + ["acme/widgets", "acme/gadgets"].some((name) => + introduction.text.includes(name), + ), + ).toBe(true); + } + }); + + test("a rejecting introduction port still yields 200 and reports the failure", async () => { + const report = spyOn(errorSink, "reportError").mockReturnValue("ref_test"); + const harness = buildApp({ + onReviewingStarted: async () => { + throw new Error("boom"); + }, + }); + const response = await harness.app.request("/wb_1/github/start-reviewing", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ repoIds: ["1"] }), + }); + expect(response.status).toBe(200); + expect(report).toHaveBeenCalled(); + expect(report.mock.calls[0]?.[0]).toBeInstanceOf(Error); + expect(report.mock.calls[0]?.[1]).toMatchObject({ + operation: "connect-github.reviewerIntroductions", + tenantId: "tnt_1", + roomId: "wb_1", + }); + }); + + test("a second start-reviewing does not re-post introductions already landed for the room", async () => { + const harness = buildApp(); + const startReviewing = (repoIds: readonly string[]) => + harness.app.request("/wb_1/github/start-reviewing", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ repoIds }), + }); + const first = await startReviewing(["1"]); + expect(first.status).toBe(200); + expect(harness.introductionCalls).toHaveLength(1); + const second = await startReviewing(["1", "2"]); + expect(second.status).toBe(200); + expect(harness.introductionCalls).toHaveLength(1); + }); + test("400s on a malformed body without leaking raw parser text", async () => { const harness = buildApp(); const response = await harness.app.request("/wb_1/github/start-reviewing", { @@ -182,6 +269,7 @@ describe("POST /:workbenchId/github/start-reviewing", () => { expect(response.status).toBe(400); const body = (await response.json()) as { error: { code: string } }; expect(body.error.code).toBe("bad_request"); + expect(harness.introductionCalls).toEqual([]); }); test("409s when the tenant has no github credential yet", async () => { @@ -193,6 +281,24 @@ describe("POST /:workbenchId/github/start-reviewing", () => { }); expect(response.status).toBe(409); expect(harness.grants).toEqual([]); + expect(harness.introductionCalls).toEqual([]); + }); + + test("502s when GitHub cannot be read, without posting introductions", async () => { + const harness = buildApp({ + listReposFn: async () => { + throw new Error( + "GitHub request to /user/repos failed: 401 Bad credentials", + ); + }, + }); + const response = await harness.app.request("/wb_1/github/start-reviewing", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ repoIds: ["1"] }), + }); + expect(response.status).toBe(502); + expect(harness.introductionCalls).toEqual([]); }); test("404s when the code-review workflow isn't deployed in this tenant", async () => { @@ -206,5 +312,6 @@ describe("POST /:workbenchId/github/start-reviewing", () => { }); expect(response.status).toBe(404); expect(harness.grants).toEqual([]); + expect(harness.introductionCalls).toEqual([]); }); }); diff --git a/packages/workflow-catalog/src/connect-github-routes.ts b/packages/workflow-catalog/src/connect-github-routes.ts index 3eb02a59c..d62e04a0d 100644 --- a/packages/workflow-catalog/src/connect-github-routes.ts +++ b/packages/workflow-catalog/src/connect-github-routes.ts @@ -26,6 +26,7 @@ import { Hono } from "hono"; import { type } from "arktype"; import type { RequireGrant, TenantEnv } from "@intx/hub-api"; import { makeErrorEnvelope } from "@workbench/hub-client"; +import { reportError } from "@corbits/error-sink"; import { fetchAuthenticatedLogin, @@ -33,6 +34,10 @@ import { type GitHubClientConfig, type GitHubRepoSummary, } from "@corbits/github-tools"; +import { + reviewerIntroductions, + type ReviewerIntroduction, +} from "@corbits/code-review/introductions"; import { startReviewingRepos } from "./connect-github-setup"; import { templateReposSettingsPatch } from "./settings"; @@ -110,6 +115,20 @@ export type ConnectGithubRoutesDeps = { principalId: string, patch: ReturnType, ): Promise; + /** Posts each reviewer's canned introduction to the room once + * `startReviewingRepos` succeeds — the identity beat of the first + * minute a person sees after picking repos. Called once, after the + * repos are recorded and before the 200 response; a later + * `start-reviewing` (change repos) does not call this again. A + * rejection is reported through `reportError` and never fails the + * response — the webhook triggers already exist, so a person must + * not see an error for a missed introduction. */ + onReviewingStarted( + tenantId: string, + workbenchId: string, + principalId: string, + introductions: readonly ReviewerIntroduction[], + ): Promise; /** Test-only override, defaulting to `@corbits/github-tools`' real * `listRepos` — lets `connect-github-routes.test.ts` stub the GitHub * client without reaching for module mocking or the real network. */ @@ -246,6 +265,12 @@ export function createConnectGithubRoutes( } try { + const settingsBefore = await deps.getTemplateSettings( + tenant.id, + workbenchId, + ); + const introductionsAlreadyPosted = + settingsBefore.selectedRepos.length > 0; const result = await startReviewingRepos(body.repoIds, state.repos, { mintRepoGrant: (repo) => deps.mintRepoGrant(tenant.id, repo), createWebhookTrigger: (repo) => @@ -271,6 +296,30 @@ export function createConnectGithubRoutes( ); }, }); + + const reposById = new Map( + state.repos.map((repo) => [repo.id, repo] as const), + ); + const repoNames = body.repoIds + .map((repoId) => reposById.get(repoId)?.name) + .filter((name): name is string => name !== undefined); + if (!introductionsAlreadyPosted) { + try { + await deps.onReviewingStarted( + tenant.id, + workbenchId, + principal.id, + reviewerIntroductions(repoNames), + ); + } catch (cause) { + reportError(cause, { + operation: "connect-github.reviewerIntroductions", + tenantId: tenant.id, + roomId: workbenchId, + }); + } + } + return c.json( { startedTriggerCount: result.createdTriggerIds.length }, 200, diff --git a/packages/workflow-catalog/src/connect-github-setup.ts b/packages/workflow-catalog/src/connect-github-setup.ts index b32c4d9e8..ad49fda96 100644 --- a/packages/workflow-catalog/src/connect-github-setup.ts +++ b/packages/workflow-catalog/src/connect-github-setup.ts @@ -1,11 +1,10 @@ // What runs once a person has actually picked repos on a room's GitHub -// connect card (CL-6345 — the slice `./instantiate.ts`'s -// `webhookTriggerTodos` named as still open). `instantiateWorkbenchTemplate` +// connect card and starts reviewing. `instantiateWorkbenchTemplate` // resolves a template at create time, before any repo is known; this // module resolves the repo-scoped half once the person answers the -// connect card, mirroring `instantiate.ts`'s own shape exactly: pure -// orchestration over injected async ports, no HTTP, no store, testable -// with plain fakes. +// connect card and creates the live webhook trigger per repo, mirroring +// `instantiate.ts`'s own shape exactly: pure orchestration over injected +// async ports, no HTTP, no store, testable with plain fakes. import type { GitHubRepoSummary } from "@corbits/github-tools"; export interface ConnectGithubSetupPorts { @@ -20,9 +19,9 @@ export interface ConnectGithubSetupPorts { mintRepoGrant(repo: GitHubRepoSummary): Promise; /** * Creates the live `webhook_trigger` row this repo's pull-request-opened - * events fire — the resolution of `instantiate.ts`'s own honest - * `webhookTriggerTodo` for this repo. A host binds this to - * `@corbits/webhook-triggers`' `WebhookTriggerStore.create`. + * events fire — the onboarding card's start-reviewing step is what + * creates this trigger, for each repo the person picked. A host binds + * this to `@corbits/webhook-triggers`' `WebhookTriggerStore.create`. */ createWebhookTrigger( repo: GitHubRepoSummary,