From c753aad9c295760b3636f2e570ffc9a8d02a300c Mon Sep 17 00:00:00 2001 From: Gandy2025 Date: Fri, 7 Aug 2026 17:34:04 +0800 Subject: [PATCH 1/5] =?UTF-8?q?feat(web):=20give=20Settings=20=E2=86=92=20?= =?UTF-8?q?GitHub=20a=20scannable=20row=20layout?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rework the GitHub integration page around a single row shape — leading glyph, what the setting is, what it currently does, and the control that changes it right-aligned — instead of three differently-shaped stacks of label / description / full-width control. - add a shared `SettingRow` primitive for Settings sections. It draws no border, background, or radius of its own: the enclosing `Section` keeps owning the one rule above the block, so this does not reintroduce the double framing #2197 removed. - connection summary reads as one row ("GitHub App" → "Connected to github.com/") with Manage connection / Manage on GitHub on the right; the unconnected state uses the same shape, so connecting no longer reflows the section. - GitHub Task Agent moves its picker to the right at a readable minimum width instead of stretching the full page; blockers, the Context Reviewer note, and errors stay full-width under the row. - add a Repositories hand-off. Connecting GitHub is when people look for "where do I add my repos", but the catalog is provider-neutral and lives on Settings → Repositories, which this page previously offered no route to. - mirror all of it in the DEV `/preview/settings-github` gallery. Co-authored-by: multica-agent --- .../ui/__tests__/setting-row.test.tsx | 83 +++++++ .../web/src/components/ui/setting-row.tsx | 93 ++++++++ ...github-app-installation-panel-dom.test.tsx | 2 + .../pages/github-app-installation-panel.tsx | 100 ++++---- .../web/src/pages/settings-github-preview.tsx | 224 ++++++++---------- .../__tests__/github-page-dom.test.tsx | 17 ++ .../settings/github-task-agent-controls.tsx | 107 +++++---- packages/web/src/pages/settings/github.tsx | 30 ++- 8 files changed, 430 insertions(+), 226 deletions(-) create mode 100644 packages/web/src/components/ui/__tests__/setting-row.test.tsx create mode 100644 packages/web/src/components/ui/setting-row.tsx diff --git a/packages/web/src/components/ui/__tests__/setting-row.test.tsx b/packages/web/src/components/ui/__tests__/setting-row.test.tsx new file mode 100644 index 000000000..c7747753b --- /dev/null +++ b/packages/web/src/components/ui/__tests__/setting-row.test.tsx @@ -0,0 +1,83 @@ +// @vitest-environment happy-dom + +import { act, type ReactElement } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { SettingRow } from "../setting-row.js"; + +globalThis.IS_REACT_ACT_ENVIRONMENT = true; + +async function renderDom(element: ReactElement): Promise<{ container: HTMLElement; root: Root }> { + const container = document.createElement("div"); + document.body.appendChild(container); + const root = createRoot(container); + await act(async () => { + root.render(element); + }); + return { container, root }; +} + +beforeEach(() => { + document.body.innerHTML = ""; +}); + +afterEach(() => { + document.body.innerHTML = ""; +}); + +describe("SettingRow", () => { + it("renders name, effect, right-aligned control, and full-width extras", async () => { + const { container, root } = await renderDom( + Change} + > + Blocker copy + , + ); + + expect(container.textContent).toContain("GitHub Task Agent"); + expect(container.textContent).toContain("Dev Agent One handles Issue activity."); + const control = container.querySelector("button"); + expect(control?.textContent).toBe("Change"); + // The control belongs to the right-hand cluster, not the text column. + expect(control?.closest("[data-setting-row]")).not.toBeNull(); + expect(control?.parentElement?.className).toContain("sm:justify-end"); + // Extras render below the row line, outside the control cluster. + const extra = container.querySelector('[data-testid="extra"]'); + expect(extra?.parentElement?.getAttribute("data-setting-row")).toBe("true"); + + await act(async () => root.unmount()); + }); + + it("draws no chrome of its own — the enclosing Section owns the rule", async () => { + const { container, root } = await renderDom(); + + const row = container.querySelector("[data-setting-row]"); + expect(row).not.toBeNull(); + expect(row?.style.border).toBe(""); + expect(row?.style.borderRadius).toBe(""); + expect(row?.style.background).toBe(""); + + await act(async () => root.unmount()); + }); + + it("spreads call-site hooks onto the row element", async () => { + const { container, root } = await renderDom(); + + const row = container.querySelector('[data-github-task-agent-controls="admin"]'); + expect(row?.getAttribute("data-setting-row")).toBe("true"); + + await act(async () => root.unmount()); + }); + + it("omits the description and control slots when not supplied", async () => { + const { container, root } = await renderDom(); + + expect(container.querySelector("p")).toBeNull(); + expect(container.querySelector(".sm\\:justify-end")).toBeNull(); + + await act(async () => root.unmount()); + }); +}); diff --git a/packages/web/src/components/ui/setting-row.tsx b/packages/web/src/components/ui/setting-row.tsx new file mode 100644 index 000000000..f8fb8476f --- /dev/null +++ b/packages/web/src/components/ui/setting-row.tsx @@ -0,0 +1,93 @@ +import type { HTMLAttributes, ReactNode } from "react"; +import { cn } from "../../lib/utils.js"; + +/** + * One configurable thing inside a Settings `Section`: an optional leading + * glyph, the name of the setting, a one-line explanation of what it does, and + * the control that changes it — the control right-aligned on the same line. + * + * Why a row instead of the previous stack (label → description → full-width + * control): a settings section is scanned, not read. Keeping "what it is" on + * the left and "change it" on the right lets the eye run down a single control + * column, and stops a narrow `Select` from stretching across the whole page. + * + * The row draws no border, background, or radius of its own. The enclosing + * `Section` owns the single rule above the block, and card chrome stays + * reserved for genuinely selectable surfaces (DESIGN.md Pillar 5) — the leading + * glyph gets a small sunken tile so the row reads as an object, but the row + * content itself is never boxed. + * + * Extra props (`id`, `data-*`, `aria-*`) spread onto the row element so a call + * site can keep its own test/scroll hooks on the outermost node. + */ +export function SettingRow({ + icon, + title, + titleId, + description, + control, + children, + className, + style, + ...rest +}: { + /** Leading line glyph (lucide, `h-4 w-4`), rendered in a sunken tile. */ + icon?: ReactNode; + /** What this row configures. */ + title: ReactNode; + /** Set when a control in this row is named by the title (`aria-labelledby`). */ + titleId?: string; + /** One line on what the setting does / its current effect. */ + description?: ReactNode; + /** Right-aligned control — Switch, Select, Button cluster, or a status line. */ + control?: ReactNode; + /** Full-width content under the row: disclosures, blockers, inline errors. */ + children?: ReactNode; +} & Omit, "title" | "children">): ReactNode { + return ( +
+ {/* Stacked on phones so a wide control never overflows; one line with the + control right-aligned from sm up. */} +
+
+ {icon ? ( + + {icon} + + ) : null} +
+
+ {title} +
+ {description ? ( +

+ {description} +

+ ) : null} +
+
+ {control ? ( +
+ {control} +
+ ) : null} +
+ {children} +
+ ); +} diff --git a/packages/web/src/pages/__tests__/github-app-installation-panel-dom.test.tsx b/packages/web/src/pages/__tests__/github-app-installation-panel-dom.test.tsx index 246de309f..fbd4e9c3f 100644 --- a/packages/web/src/pages/__tests__/github-app-installation-panel-dom.test.tsx +++ b/packages/web/src/pages/__tests__/github-app-installation-panel-dom.test.tsx @@ -174,6 +174,8 @@ describe("GithubAppInstallationPanel", () => { const { container, root } = await renderDom(); await waitForText(container, "Connected to"); + // The connection reads as one row: what it is, then who it's bound to. + expect(container.textContent).toContain("GitHub App"); // GitHub accounts render as the full github.com path so a GitHub org is // never confusable with a First Tree team name. expect(container.textContent).toContain("github.com/octocat"); diff --git a/packages/web/src/pages/github-app-installation-panel.tsx b/packages/web/src/pages/github-app-installation-panel.tsx index e701ce6f6..c0035a897 100644 --- a/packages/web/src/pages/github-app-installation-panel.tsx +++ b/packages/web/src/pages/github-app-installation-panel.tsx @@ -1,6 +1,6 @@ import type { GithubAppConnectPanelInstallation, GithubAppInstallationOutput } from "@first-tree/shared"; import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; -import { ArrowLeft, Building2, ChevronRight, ExternalLink, PauseCircle, User } from "lucide-react"; +import { ArrowLeft, Building2, ChevronRight, ExternalLink, Github, PauseCircle, User } from "lucide-react"; import { type ReactNode, useEffect, useState } from "react"; import { ApiError } from "../api/client.js"; import { @@ -13,6 +13,7 @@ import { import { getAuthProviders, startProviderLink } from "../api/user-settings.js"; import { useAuth } from "../auth/auth-context.js"; import { Button } from "../components/ui/button.js"; +import { SettingRow } from "../components/ui/setting-row.js"; import { clearGithubAccountLinkReturn, rememberGithubAccountLinkReturn } from "../lib/github-account-link-return.js"; import { clearGithubInstallAttemptForOrganization, @@ -108,7 +109,9 @@ export function GithubAppInstallationPanel({ /** * Unbound summary: the team has no GitHub connection yet, so the whole - * surface is one prominent entry point into the connect panel. + * surface is one prominent entry point into the connect panel. Same row shape + * as the connected state, so connecting doesn't reflow the section — only the + * status line and the right-hand control change. */ function NotConnectedSummary({ disabled, @@ -120,17 +123,18 @@ function NotConnectedSummary({ onOpenPanel: () => void; }) { return ( -
-

- This team isn't connected to GitHub yet. Connect a GitHub App installation to start receiving issues, pull - requests, and reviews as routed messages. -

- {!readOnly && ( - - )} -
+ } + title="GitHub App" + description="This team isn't connected to GitHub yet. Connect a GitHub App installation to start receiving issues, pull requests, and reviews as routed messages." + control={ + readOnly ? null : ( + + ) + } + /> ); } @@ -156,49 +160,43 @@ function InstalledState({ return ( // No borderTop of its own: the page's Section frame already draws the // rule above this block. -
+
{data.suspended && } -
-
- Connected to -
-
- - - {githubAccountPath(data.accountLogin)} - - - {data.accountType} + } + title="GitHub App" + // Who's connected reads as the row's status line rather than a separate + // labelled block — one glance answers "is this wired up, and to what". + description={ + + + + Connected to {githubAccountPath(data.accountLogin)} + + {data.accountType} -
+ } + control={ + readOnly ? null : ( + <> + + + + ) + } + > {/* Expandable details sit directly under the connected account they describe, not below the action buttons. */} -
- -
-
- - {!readOnly && ( -
- - -
- )} + +
); } diff --git a/packages/web/src/pages/settings-github-preview.tsx b/packages/web/src/pages/settings-github-preview.tsx index aca14f440..3cf80485c 100644 --- a/packages/web/src/pages/settings-github-preview.tsx +++ b/packages/web/src/pages/settings-github-preview.tsx @@ -1,8 +1,20 @@ -import { Building2, ChevronRight, ExternalLink, PauseCircle, User } from "lucide-react"; +import { + ArrowRight, + Bot, + Building2, + ChevronRight, + ExternalLink, + FolderGit2, + Github, + PauseCircle, + User, +} from "lucide-react"; import { useState } from "react"; +import { Button } from "../components/ui/button.js"; import { PageHeader } from "../components/ui/page-header.js"; import { Section } from "../components/ui/section.js"; import { Select } from "../components/ui/select.js"; +import { SettingRow } from "../components/ui/setting-row.js"; /** * DEV-only visual review for Settings → GitHub (the connected GitHub App @@ -20,9 +32,10 @@ import { Select } from "../components/ui/select.js"; * - **Not installed** — the "Install on GitHub" CTA. * - **Loading** — the initial fetch state. * - * Connection and automatic handling are separate provider-owned sections. The - * markup here mirrors the real page closely enough to review their hierarchy - * without a live GitHub installation. + * Connection, automatic handling, and the Repositories hand-off are separate + * provider-owned sections, each a `SettingRow` (glyph + name + effect on the + * left, control right-aligned). The markup here mirrors the real page closely + * enough to review their hierarchy without a live GitHub installation. */ const MOCK = { @@ -149,35 +162,46 @@ function AutomaticHandlingPreview() { const agentName = agentUuid === "release-agent" ? "Release Agent" : "Dev Assistant"; return ( -
-
- - GitHub Task Agent - -
- {agentName} automatically handles Issue and pull request activity outside the Context Tree repository and - posts final replies as the First Tree GitHub App. + } + title="GitHub Task Agent" + description={`${agentName} automatically handles Issue and pull request activity outside the Context Tree repository and posts final replies as the First Tree GitHub App.`} + control={ +
+ + } + >
Context Tree activity uses Context Reviewer. These roles must use different Agents.
-
+ + ); +} + +function RepositoriesPointerPreview() { + return ( + } + title="Team code repositories" + description="Repository URLs and the agents that may clone them live in the Repositories tab — they apply to GitLab too, so they aren't tied to this connection." + control={ + + } + /> ); } @@ -194,6 +218,9 @@ function PageShell({ children }: { children: React.ReactNode }) { > +
+ +
); @@ -202,56 +229,36 @@ function PageShell({ children }: { children: React.ReactNode }) { function InstalledCard({ suspended = false, detailsOpen = false }: { suspended?: boolean; detailsOpen?: boolean }) { return ( -
+
{suspended && } -
-
- Connected to -
-
- - - {MOCK.accountLogin} - - - {MOCK.accountType} + } + title="GitHub App" + description={ + + + + Connected to github.com/{MOCK.accountLogin} + + {MOCK.accountType} -
-
- - + } + control={ + <> + + + + } + > + +
); @@ -266,53 +273,28 @@ function InstalledCard({ suspended = false, detailsOpen = false }: { suspended?: function NotInstalledCard({ waiting = false }: { waiting?: boolean }) { return ( -
-

- Install the GitHub App on your personal account or organization to start receiving issues, pull requests, and - reviews as routed messages. -

- + } + title="GitHub App" + description="This team isn't connected to GitHub yet. Connect a GitHub App installation to start receiving issues, pull requests, and reviews as routed messages." + control={ + + } + > {waiting && ( -
+
Waiting for GitHub… - +
)} -
+ ); } @@ -372,9 +354,11 @@ export function SettingsGithubPreviewPage() { Settings → GitHub — shipped layout

- Lean default: who's connected + Manage on GitHub. Permissions, subscribed events, and the installation id sit - behind a collapsed "Connection details" disclosure (click to toggle). GitHub Task Agent lives in the adjacent - Automatic handling section instead of appearing as a standalone Setup capability. + Every section is one row: glyph + what it is + what it currently does on the left, the control that changes it + right-aligned. Permissions, subscribed events, and the installation id sit behind a collapsed "Connection + details" disclosure (click to toggle). GitHub Task Agent lives in the adjacent Automatic handling section + instead of appearing as a standalone Setup capability, and Repositories hands off to the provider-neutral + catalog rather than dead-ending here.

diff --git a/packages/web/src/pages/settings/__tests__/github-page-dom.test.tsx b/packages/web/src/pages/settings/__tests__/github-page-dom.test.tsx index ae67fb8cb..77b21ff0c 100644 --- a/packages/web/src/pages/settings/__tests__/github-page-dom.test.tsx +++ b/packages/web/src/pages/settings/__tests__/github-page-dom.test.tsx @@ -344,6 +344,23 @@ describe("SettingsGithubPage — automatic handling", () => { await act(async () => root.unmount()); }); + it("hands off to the provider-neutral repository catalog below automatic handling", async () => { + const { SettingsGithubPage } = await import("../github.js"); + const { container, root } = await renderAt("/settings/integrations/github", ); + await waitForText(container, "Team code repositories"); + + const link = await waitForSelector( + container, + 'a[href="/settings/repositories#code-repositories"]', + ); + expect(link.textContent).toContain("Manage repositories"); + // The hand-off closes the page — it must not push automatic handling down. + const taskRouting = await waitForSelector(container, "#task-routing"); + expect(taskRouting.compareDocumentPosition(link) & Node.DOCUMENT_POSITION_FOLLOWING).not.toBe(0); + + await act(async () => root.unmount()); + }); + it("shows members the configured Agent without exposing assignment controls", async () => { authMock.value = { role: "member", diff --git a/packages/web/src/pages/settings/github-task-agent-controls.tsx b/packages/web/src/pages/settings/github-task-agent-controls.tsx index 01bb4ab53..0d499a7bf 100644 --- a/packages/web/src/pages/settings/github-task-agent-controls.tsx +++ b/packages/web/src/pages/settings/github-task-agent-controls.tsx @@ -1,6 +1,7 @@ import type { OrgGithubFeaturesOutput, SetupBlocker, TeamAgentCandidatesOutput } from "@first-tree/shared"; import { setupBlockerCodeSchema } from "@first-tree/shared"; import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; +import { Bot } from "lucide-react"; import { useEffect, useState } from "react"; import { Link } from "react-router"; import { ApiError } from "../../api/client.js"; @@ -9,6 +10,7 @@ import { setupCapabilitiesQueryKey } from "../../api/setup-capabilities.js"; import { getTeamAgentCandidates, putTeamAgentAssignment } from "../../api/team-agent-settings.js"; import { useAuth } from "../../auth/auth-context.js"; import { Select } from "../../components/ui/select.js"; +import { SettingRow } from "../../components/ui/setting-row.js"; import { setupBlockerCopy } from "./setup-blocker-copy.js"; export function GithubTaskAgentControls({ @@ -62,24 +64,20 @@ export function GithubTaskAgentControls({ if (!isAdmin) { const projectedAgent = settingQuery.data?.teamAgent.agent ?? null; return ( -
- - GitHub Task Agent - - - {settingQuery.isLoading + icon={} + title="GitHub Task Agent" + description={ + settingQuery.isLoading ? "Loading configured Agent…" : settingQuery.error ? "First Tree could not load the configured Agent." : projectedAgent ? `${projectedAgent.displayName} automatically handles Issue and pull request activity outside the Context Tree repository and posts final replies as the First Tree GitHub App.` - : "Not configured. An admin can choose the Agent that automatically handles Issue and pull request activity outside the Context Tree repository and posts final replies as the First Tree GitHub App."} - -
+ : "Not configured. An admin can choose the Agent that automatically handles Issue and pull request activity outside the Context Tree repository and posts final replies as the First Tree GitHub App." + } + /> ); } @@ -110,32 +108,46 @@ export function GithubTaskAgentControls({ (item) => item.resolutionOwner === "admin" && item.actionKind === "manage_github_installation", ); const error = settingQuery.error ?? candidatesQuery.error ?? assignmentMutation.error; + const loading = settingQuery.isLoading || candidatesQuery.isLoading; + const assignable = !loading && candidates.length > 0; return ( -
} + title="GitHub Task Agent" + description={ + selectedLabel + ? `${selectedLabel} automatically handles Issue and pull request activity outside the Context Tree repository and posts final replies as the First Tree GitHub App.` + : "Choose the Agent that automatically handles Issue and pull request activity outside the Context Tree repository and posts final replies as the First Tree GitHub App." + } + control={ + loading ? ( + + Loading eligible Agents… + + ) : assignable ? ( + // The picker keeps a readable minimum instead of stretching the full + // page width — it is one control in a column of controls, not a form. +
+ { - const next = agentUuid || null; - if (next === selectedAgentUuid) return; - assignmentMutation.mutate(next); - }} - disabled={assignmentMutation.isPending} - options={options} - placeholder="Select an eligible Agent" - searchable={candidates.length > 6} - /> -
- Context Tree activity uses Context Reviewer. These roles must use different Agents. -
+ ) : null} + + {assignable ? ( +
+ Context Tree activity uses Context Reviewer. These roles must use different Agents.
- )} + ) : null} {error ? (
{teamAgentMutationError(error)}
) : null} -
+ ); } diff --git a/packages/web/src/pages/settings/github.tsx b/packages/web/src/pages/settings/github.tsx index 33d1c61bb..4d2309c8e 100644 --- a/packages/web/src/pages/settings/github.tsx +++ b/packages/web/src/pages/settings/github.tsx @@ -1,11 +1,12 @@ import { useQuery } from "@tanstack/react-query"; -import { ArrowRight, Check } from "lucide-react"; +import { ArrowRight, Check, FolderGit2 } from "lucide-react"; import { useEffect, useRef, useState } from "react"; import { Link, useLocation, useSearchParams } from "react-router"; import { getGithubAppInstallation } from "../../api/github-app.js"; import { useAuth } from "../../auth/auth-context.js"; import { Button } from "../../components/ui/button.js"; import { Section } from "../../components/ui/section.js"; +import { SettingRow } from "../../components/ui/setting-row.js"; import { clearGithubAccountLinkReturn, readGithubAccountLinkReturn } from "../../lib/github-account-link-return.js"; import { clearGithubInstallAttempt, readGithubInstallAttempt } from "../../lib/github-install-attempt.js"; import { GithubAppInstallationPanel } from "../github-app-installation-panel.js"; @@ -198,10 +199,37 @@ export function SettingsGithubPage() { +
); } +/** + * Connecting GitHub is the moment people go looking for "where do I add my + * repos" — but the code catalog is provider-neutral and lives on Settings → + * Repositories (this page used to host it under `#code-access`). Close the loop + * with an explicit hand-off instead of letting the page dead-end. + */ +function RepositoriesPointer() { + return ( +
+ } + title="Team code repositories" + description="Repository URLs and the agents that may clone them live in the Repositories tab — they apply to GitLab too, so they aren't tied to this connection." + control={ + + } + /> +
+ ); +} + /** * The Context round-trip return. Before the team is connected it's a quiet line * explaining why the user was sent here; once connected it becomes the explicit From f12cd6aa491c0778f5c1fe054241c01bd58dbf22 Mon Sep 17 00:00:00 2001 From: Gandy2025 Date: Fri, 7 Aug 2026 19:03:12 +0800 Subject: [PATCH 2/5] feat(web): make Connection details answer whether the install is usable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The disclosure transcribed GitHub's `permissions` / `events` blobs verbatim, which left the reader to diff them against a requirement they had to already know — and dropped the two timestamps the API was already returning. - add `GITHUB_APP_REQUIRED_PERMISSIONS` + `githubPermissionSatisfies` to shared, and point the server's `taskReplyInstallationBlocker` at them, so "what First Tree needs" has one definition. An admin can no longer be shown a healthy connection while the task-reply gate refuses the assignment. - rewrite the disclosure as `github-connection-details.tsx`: required scopes named in prose and marked ready/blocked, other grants demoted to a secondary line, consumed events named with ignored subscriptions called out, and an Installation block with a copyable id plus `createdAt` / `updatedAt`. - a shortfall says what it costs ("Agents can't post replies on pull requests") and offers Grant on GitHub — admins only, since a member has no standing to act on it. It also marks the collapsed toggle, so the one state worth acting on isn't hidden behind a click. - blocked, not needs-you: the fix lives on GitHub, and a First Tree admin role alone doesn't establish they can grant it there (DESIGN.md §3). - the DEV gallery now renders the real component instead of a copy, and gains the two shortfall states. Side effect of routing through `githubPermissionSatisfies`: a stronger grant now satisfies a weaker requirement, so an `admin`-level scope no longer trips the gate. GitHub only issues read/write for these two scopes today, so this is correctness rather than an observable change. --- .../src/services/team-agent-settings.ts | 9 +- .../github-app-required-permissions.test.ts | 33 ++ packages/shared/src/index.ts | 2 + packages/shared/src/schemas/github-app.ts | 33 ++ ...github-app-installation-panel-dom.test.tsx | 11 +- .../github-connection-details-dom.test.tsx | 166 +++++++++ .../__tests__/preview-pages-extra.test.tsx | 10 +- .../pages/github-app-installation-panel.tsx | 88 +---- .../src/pages/github-connection-details.tsx | 322 ++++++++++++++++++ .../web/src/pages/settings-github-preview.tsx | 150 +++----- 10 files changed, 630 insertions(+), 194 deletions(-) create mode 100644 packages/shared/src/__tests__/github-app-required-permissions.test.ts create mode 100644 packages/web/src/pages/__tests__/github-connection-details-dom.test.tsx create mode 100644 packages/web/src/pages/github-connection-details.tsx diff --git a/packages/server/src/services/team-agent-settings.ts b/packages/server/src/services/team-agent-settings.ts index 806cc84f9..f400b10f5 100644 --- a/packages/server/src/services/team-agent-settings.ts +++ b/packages/server/src/services/team-agent-settings.ts @@ -1,4 +1,6 @@ import { + GITHUB_APP_REQUIRED_PERMISSIONS, + githubPermissionSatisfies, ORG_SETTINGS_NAMESPACES, type OrgGithubFeaturesOutput, type OrgGithubFeaturesStorage, @@ -74,7 +76,12 @@ async function readContextReviewerAgentUuid(db: Database, organizationId: string function taskReplyInstallationBlocker(installation: InstallationRow | null): SetupBlocker | null { if (!installation) return null; if (installation.suspendedAt) return blocker("github_app_suspended", "manage_github_installation"); - if (installation.permissions.issues !== "write" || installation.permissions.pull_requests !== "write") { + // Same requirement set the Settings → GitHub readout renders, so an admin is + // never shown a healthy connection while this gate refuses the assignment. + const unsatisfied = Object.entries(GITHUB_APP_REQUIRED_PERMISSIONS).some( + ([permission, level]) => !githubPermissionSatisfies(installation.permissions[permission], level), + ); + if (unsatisfied) { return blocker("github_app_task_reply_permission_required", "manage_github_installation"); } return null; diff --git a/packages/shared/src/__tests__/github-app-required-permissions.test.ts b/packages/shared/src/__tests__/github-app-required-permissions.test.ts new file mode 100644 index 000000000..2210e6537 --- /dev/null +++ b/packages/shared/src/__tests__/github-app-required-permissions.test.ts @@ -0,0 +1,33 @@ +import { describe, expect, it } from "vitest"; +import { GITHUB_APP_REQUIRED_PERMISSIONS, githubPermissionSatisfies } from "../schemas/github-app.js"; + +describe("GITHUB_APP_REQUIRED_PERMISSIONS", () => { + it("is the set the task-reply gate enforces", () => { + // Both the server's `taskReplyInstallationBlocker` and the Settings → + // GitHub readout derive from this. Changing it changes what an admin is + // told AND what the server will publish, so it is pinned here. + expect(GITHUB_APP_REQUIRED_PERMISSIONS).toEqual({ issues: "write", pull_requests: "write" }); + }); +}); + +describe("githubPermissionSatisfies", () => { + it("accepts an exact match", () => { + expect(githubPermissionSatisfies("write", "write")).toBe(true); + expect(githubPermissionSatisfies("read", "read")).toBe(true); + }); + + it("accepts a stronger grant — GitHub's write implies read", () => { + expect(githubPermissionSatisfies("write", "read")).toBe(true); + expect(githubPermissionSatisfies("admin", "write")).toBe(true); + expect(githubPermissionSatisfies("admin", "read")).toBe(true); + }); + + it("rejects a weaker grant", () => { + expect(githubPermissionSatisfies("read", "write")).toBe(false); + expect(githubPermissionSatisfies("write", "admin")).toBe(false); + }); + + it("treats an absent permission as unsatisfied", () => { + expect(githubPermissionSatisfies(undefined, "read")).toBe(false); + }); +}); diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 7accac3b3..0920dcd53 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -780,6 +780,7 @@ export { export { GITHUB_ACCOUNT_TYPES, GITHUB_APP_CONNECT_STATUSES, + GITHUB_APP_REQUIRED_PERMISSIONS, GITHUB_PERMISSION_LEVELS, type GithubAccountType, type GithubAppConnectBody, @@ -801,6 +802,7 @@ export { githubAppInstallationPermissionsSchema, githubAppUserTokenMetadataSchema, githubPermissionLevelSchema, + githubPermissionSatisfies, } from "./schemas/github-app.js"; export { GITHUB_TASK_REPLY_BODY_MAX_BYTES, diff --git a/packages/shared/src/schemas/github-app.ts b/packages/shared/src/schemas/github-app.ts index 771fbb6fc..dee612578 100644 --- a/packages/shared/src/schemas/github-app.ts +++ b/packages/shared/src/schemas/github-app.ts @@ -51,6 +51,39 @@ export type GithubPermissionLevel = z.infer; export const githubAppInstallationPermissionsSchema = z.record(z.string(), githubPermissionLevelSchema); export type GithubAppInstallationPermissions = z.infer; +/** + * The permission levels an installation must grant before First Tree can act + * as the App on it — `permission name -> minimum level`. + * + * This is the one definition of "is this installation good enough". The server + * gates the GitHub Task Agent on it (`taskReplyInstallationBlocker`) and the + * Settings → GitHub readout renders it, so an admin is never told the + * installation is fine while the server refuses to publish (or the reverse). + * + * `metadata: read` is deliberately absent: GitHub grants it to every App + * install and it cannot be withheld, so listing it would only ever render a + * tautological ✓. The reply publisher still requests it when minting a scoped + * token — that is a token scope, not a gate. + */ +export const GITHUB_APP_REQUIRED_PERMISSIONS = { + issues: "write", + pull_requests: "write", +} as const satisfies Record; + +/** + * True when `permissions` grants at least every level in + * `GITHUB_APP_REQUIRED_PERMISSIONS`. `write` satisfies a `read` requirement + * (GitHub's write implies read); `admin` satisfies both. + */ +export function githubPermissionSatisfies( + granted: GithubPermissionLevel | undefined, + required: GithubPermissionLevel, +): boolean { + if (granted === undefined) return false; + const rank: Record = { read: 0, write: 1, admin: 2 }; + return rank[granted] >= rank[required]; +} + /** * Subscribed event-name list, e.g. `["issues", "pull_request", "push"]`. * Free-form for the same forward-compat reason as `permissions`. diff --git a/packages/web/src/pages/__tests__/github-app-installation-panel-dom.test.tsx b/packages/web/src/pages/__tests__/github-app-installation-panel-dom.test.tsx index fbd4e9c3f..63982556b 100644 --- a/packages/web/src/pages/__tests__/github-app-installation-panel-dom.test.tsx +++ b/packages/web/src/pages/__tests__/github-app-installation-panel-dom.test.tsx @@ -192,16 +192,15 @@ describe("GithubAppInstallationPanel", () => { // the admin opens it. const detailsToggle = buttonByText(container, "Connection details"); expect(detailsToggle?.getAttribute("aria-expanded")).toBe("false"); - expect(container.textContent).not.toContain("contents:"); - expect(container.textContent).not.toContain(`Installation ${"#"}123`); + expect(container.textContent).not.toContain("Also granted"); await click(detailsToggle); expect(detailsToggle?.getAttribute("aria-expanded")).toBe("true"); - expect(container.textContent).toContain("contents:"); - expect(container.textContent).toContain("issues:"); - expect(container.textContent).toContain("pull_request"); - expect(container.textContent).toContain(`Installation ${"#"}123`); + expect(container.textContent).toContain("Required by First Tree"); + expect(container.textContent).toContain("Also granted"); + expect(container.textContent).toContain("Contents"); + expect(container.textContent).toContain("Issues"); await act(async () => root.unmount()); }); diff --git a/packages/web/src/pages/__tests__/github-connection-details-dom.test.tsx b/packages/web/src/pages/__tests__/github-connection-details-dom.test.tsx new file mode 100644 index 000000000..75c68f151 --- /dev/null +++ b/packages/web/src/pages/__tests__/github-connection-details-dom.test.tsx @@ -0,0 +1,166 @@ +// @vitest-environment happy-dom + +import type { GithubAppInstallationOutput } from "@first-tree/shared"; +import { act, type ReactElement } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { GithubConnectionDetails } from "../github-connection-details.js"; + +globalThis.IS_REACT_ACT_ENVIRONMENT = true; + +function installation(overrides: Partial = {}): GithubAppInstallationOutput { + return { + installationId: 123, + accountType: "Organization", + accountLogin: "acme", + accountGithubId: 456, + permissions: { issues: "write", pull_requests: "write", metadata: "read", contents: "write" }, + events: ["issues", "issue_comment", "pull_request", "push", "member"], + suspended: false, + manageUrl: "https://github.com/organizations/acme/settings/installations/123", + createdAt: "2026-08-03T09:12:00.000Z", + updatedAt: "2026-08-06T16:41:00.000Z", + ...overrides, + }; +} + +async function renderDom(element: ReactElement): Promise<{ container: HTMLElement; root: Root }> { + const container = document.createElement("div"); + document.body.appendChild(container); + const root = createRoot(container); + await act(async () => { + root.render(element); + }); + return { container, root }; +} + +function buttonByText(container: ParentNode, text: string): HTMLButtonElement | null { + return [...container.querySelectorAll("button")].find((button) => button.textContent?.includes(text)) ?? null; +} + +beforeEach(() => { + document.body.innerHTML = ""; +}); + +afterEach(() => { + document.body.innerHTML = ""; +}); + +describe("GithubConnectionDetails", () => { + it("keeps the detail behind a collapsed disclosure", async () => { + const { container, root } = await renderDom(); + + expect(buttonByText(container, "Connection details")?.getAttribute("aria-expanded")).toBe("false"); + expect(container.textContent).not.toContain("Required by First Tree"); + expect(container.textContent).not.toContain("131952074"); + + await act(async () => root.unmount()); + }); + + it("answers 'is this installation good enough' before listing raw grants", async () => { + const { container, root } = await renderDom(); + + // Required set is named in prose and marked satisfied. + expect(container.textContent).toContain("Required by First Tree"); + expect(container.textContent).toContain("Issues"); + expect(container.textContent).toContain("Pull requests"); + // Everything else is secondary, not mixed into the requirement list. + expect(container.textContent).toContain("Also granted"); + expect(container.textContent).toContain("Contents"); + // Nothing is blocked, so no GitHub-side call to action appears. + expect(container.querySelector('a[href*="settings/installations"]')).toBeNull(); + + await act(async () => root.unmount()); + }); + + it("marks a shortfall, says what it costs, and points at the fix", async () => { + const data = installation({ permissions: { issues: "write", pull_requests: "read", metadata: "read" } }); + const { container, root } = await renderDom(); + + expect(container.textContent).toContain("Agents can't post replies on pull requests."); + const grant = container.querySelector(`a[href="${data.manageUrl}"]`); + expect(grant?.textContent).toContain("Grant on GitHub"); + + await act(async () => root.unmount()); + }); + + it("surfaces the shortfall on the collapsed toggle so it isn't hidden", async () => { + const data = installation({ permissions: { issues: "write", pull_requests: "read" } }); + const { container, root } = await renderDom(); + + expect(container.textContent).toContain("Pull requests access is missing"); + + await act(async () => root.unmount()); + }); + + it("counts multiple shortfalls rather than naming one of them", async () => { + const { container, root } = await renderDom(); + + expect(container.textContent).toContain("2 required permissions are missing"); + + await act(async () => root.unmount()); + }); + + it("treats a higher granted level as satisfying the requirement", async () => { + const data = installation({ permissions: { issues: "admin", pull_requests: "write" } }); + const { container, root } = await renderDom(); + + expect(container.textContent).not.toContain("Agents can't post replies on issues."); + + await act(async () => root.unmount()); + }); + + it("withholds the GitHub call to action from members who can't act on it", async () => { + const data = installation({ permissions: { issues: "write", pull_requests: "read" } }); + const { container, root } = await renderDom(); + + // Members still read the same diagnosis... + expect(container.textContent).toContain("Agents can't post replies on pull requests."); + // ...without a button they have no standing to use. + expect(container.querySelector(`a[href="${data.manageUrl}"]`)).toBeNull(); + + await act(async () => root.unmount()); + }); + + it("names the events it consumes and calls out the ones it drops", async () => { + const { container, root } = await renderDom(); + + expect(container.textContent).toContain("Issue comments"); + // `push` / `member` are subscribed but never normalized into activity — + // saying so is the answer when a webhook produced nothing. + expect(container.textContent).toContain("Subscribed but unused: push, member"); + + await act(async () => root.unmount()); + }); + + it("renders the installation id and both timestamps the API already returns", async () => { + const { container, root } = await renderDom(); + + expect(container.textContent).toContain("123"); + expect(container.textContent).toContain("Connected"); + expect(container.textContent).toContain("Last updated"); + expect(container.textContent).toContain( + new Date("2026-08-03T09:12:00.000Z").toLocaleDateString(undefined, { + year: "numeric", + month: "short", + day: "numeric", + }), + ); + expect(container.querySelector('[aria-label="Copy installation id"]')).not.toBeNull(); + + await act(async () => root.unmount()); + }); + + it("degrades gracefully on permission and event names it has never seen", async () => { + const data = installation({ + permissions: { issues: "write", pull_requests: "write", dependabot_secrets: "read" }, + events: ["issues", "some_future_event"], + }); + const { container, root } = await renderDom(); + + expect(container.textContent).toContain("Dependabot secrets"); + expect(container.textContent).toContain("Subscribed but unused: some_future_event"); + + await act(async () => root.unmount()); + }); +}); diff --git a/packages/web/src/pages/__tests__/preview-pages-extra.test.tsx b/packages/web/src/pages/__tests__/preview-pages-extra.test.tsx index 8752a7a25..e36268ab8 100644 --- a/packages/web/src/pages/__tests__/preview-pages-extra.test.tsx +++ b/packages/web/src/pages/__tests__/preview-pages-extra.test.tsx @@ -349,15 +349,21 @@ describe("extra preview pages", () => { expect(text(rendered.container)).toContain("waiting"); expect(text(rendered.container)).toContain("Loading"); expect(text(rendered.container)).toContain("Waiting for GitHub"); + // The two states that exist to review the shortfall readout. + expect(text(rendered.container)).toContain("a required scope is missing"); + expect(text(rendered.container)).toContain("Missing scope — collapsed"); + expect(text(rendered.container)).toContain("Agents can't post replies on pull requests."); + expect(text(rendered.container)).toContain("Pull requests access is missing"); const detailsButtons = [...rendered.container.querySelectorAll("button")].filter((button) => button.textContent?.includes("Connection details"), ); - expect(detailsButtons.length).toBe(3); + expect(detailsButtons.length).toBe(5); expect(detailsButtons[0]?.getAttribute("aria-expanded")).toBe("false"); await click(detailsButtons[0] ?? detailsButtons[1] ?? buttonByText(rendered.container, "Connection details")); expect(detailsButtons[0]?.getAttribute("aria-expanded")).toBe("true"); - expect(text(rendered.container)).toContain("Installation #131952074"); + expect(text(rendered.container)).toContain("131952074"); + expect(text(rendered.container)).toContain("Required by First Tree"); await cleanupRendered(rendered); }); diff --git a/packages/web/src/pages/github-app-installation-panel.tsx b/packages/web/src/pages/github-app-installation-panel.tsx index c0035a897..f128b0445 100644 --- a/packages/web/src/pages/github-app-installation-panel.tsx +++ b/packages/web/src/pages/github-app-installation-panel.tsx @@ -1,6 +1,6 @@ import type { GithubAppConnectPanelInstallation, GithubAppInstallationOutput } from "@first-tree/shared"; import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; -import { ArrowLeft, Building2, ChevronRight, ExternalLink, Github, PauseCircle, User } from "lucide-react"; +import { ArrowLeft, Building2, ExternalLink, Github, PauseCircle, User } from "lucide-react"; import { type ReactNode, useEffect, useState } from "react"; import { ApiError } from "../api/client.js"; import { @@ -20,6 +20,7 @@ import { hasGithubInstallAttemptForOrganization, rememberGithubInstallAttempt, } from "../lib/github-install-attempt.js"; +import { GithubConnectionDetails } from "./github-connection-details.js"; /** * How often the open connect panel refreshes its installation list. Two @@ -195,7 +196,7 @@ function InstalledState({ > {/* Expandable details sit directly under the connected account they describe, not below the action buttons. */} - +
); @@ -721,89 +722,6 @@ function InstallationRow({ ); } -/** - * Collapsed-by-default disclosure for the developer-facing connection - * metadata: the granted permission scopes, the subscribed webhook events, - * and the installation id. Kept off the default view (most admins only need - * "who's connected" + Manage) but one click away for scope auditing. A plain - * `aria-expanded` button — there's no shared collapsible primitive in this app, - * and the controlled toggle keeps the chevron and the mounted content in - * lockstep. - */ -function ConnectionDetails({ data }: { data: GithubAppInstallationOutput }) { - const [open, setOpen] = useState(false); - const permissionEntries = Object.entries(data.permissions); - const regionId = "github-connection-details"; - - return ( -
- - - {open && ( -
- {permissionEntries.length > 0 && ( -
-
- Permissions granted -
-
    - {permissionEntries.map(([key, value]) => ( -
  • - {key}: {value} -
    • - ))} -
-
- )} - - {data.events.length > 0 && ( -
-
- Subscribed events -
-
- {data.events.join(", ")} -
-
- )} - - - Installation #{data.installationId} - -
- )} -
- ); -} - function SuspendedBanner() { return (
= { + actions: "Actions", + administration: "Administration", + checks: "Checks", + contents: "Contents", + discussions: "Discussions", + issues: "Issues", + members: "Members", + metadata: "Metadata", + organization_administration: "Organization administration", + pull_requests: "Pull requests", + workflows: "Workflows", +}; + +/** + * Webhook events First Tree actually turns into something — mirrors the + * `buildRule` switch in the server's `github-normalize.ts`. Everything else an + * installation subscribes to is delivered and dropped, which is exactly what + * you want said out loud while working out why an event produced no message. + */ +const CONSUMED_EVENT_LABELS: Record = { + commit_comment: "Commit comments", + discussion: "Discussions", + discussion_comment: "Discussion comments", + issue_comment: "Issue comments", + issues: "Issues", + pull_request: "Pull requests", + pull_request_review: "PR reviews", + pull_request_review_comment: "PR review comments", +}; + +/** What the team loses while a required permission is missing. */ +const PERMISSION_SHORTFALL: Record = { + issues: "Agents can't post replies on issues.", + pull_requests: "Agents can't post replies on pull requests.", +}; + +function humanize(value: string): string { + const spaced = value.replace(/_/g, " "); + return spaced.charAt(0).toUpperCase() + spaced.slice(1); +} + +function permissionLabel(name: string): string { + return PERMISSION_LABELS[name] ?? humanize(name); +} + +function shortfallCopy(name: string, required: GithubPermissionLevel): string { + return PERMISSION_SHORTFALL[name] ?? `First Tree needs ${required} access here.`; +} + +type RequiredPermission = { + name: string; + required: GithubPermissionLevel; + granted: GithubPermissionLevel | undefined; + satisfied: boolean; +}; + +/** Collapsed-state summary: name the single gap, or count several. */ +function blockedSummary(blocked: RequiredPermission[]): string { + const [only] = blocked; + return blocked.length === 1 && only + ? `${permissionLabel(only.name)} access is missing` + : `${blocked.length} required permissions are missing`; +} + +function readRequiredPermissions(permissions: GithubAppInstallationOutput["permissions"]): RequiredPermission[] { + return Object.entries(GITHUB_APP_REQUIRED_PERMISSIONS).map(([name, required]) => ({ + name, + required, + granted: permissions[name], + satisfied: githubPermissionSatisfies(permissions[name], required), + })); +} + +/** + * Collapsed-by-default disclosure for the connection's developer-facing + * detail. It used to transcribe GitHub's `permissions` / `events` blobs + * verbatim, which left the reader to diff them against a requirement they'd + * have to already know. It now answers the question they opened it with — + * "is this installation good enough for First Tree?" — before falling back to + * the raw grants: + * + * - **Required by First Tree** — one row per `GITHUB_APP_REQUIRED_PERMISSIONS` + * entry (the same set the server's task-reply gate gives out), each marked + * ready or blocked. + * - **Also granted** — everything else, secondary. + * - **Events** — the subscriptions First Tree consumes, named in prose, with + * ignored subscriptions called out instead of silently blended in. + * - **Installation** — id (copyable), and the two timestamps the API already + * returns, which are the first thing you want when reconstructing "when + * did this change". + * + * A shortfall also marks the collapsed toggle, so the one state worth acting + * on isn't hidden behind a closed disclosure. + * + * A plain `aria-expanded` button — there's no shared collapsible primitive in + * this app, and the controlled toggle keeps the chevron and the mounted + * content in lockstep. + */ +export function GithubConnectionDetails({ + data, + readOnly = false, + defaultOpen = false, +}: { + data: GithubAppInstallationOutput; + /** Members read the same facts but get no GitHub-side call to action. */ + readOnly?: boolean; + /** The DEV `/preview/settings-github` gallery renders the expanded state directly. */ + defaultOpen?: boolean; +}) { + const [open, setOpen] = useState(defaultOpen); + const regionId = "github-connection-details"; + const required = readRequiredPermissions(data.permissions); + const blocked = required.filter((permission) => !permission.satisfied); + const requiredNames = new Set(required.map((permission) => permission.name)); + const alsoGranted = Object.entries(data.permissions) + .filter(([name]) => !requiredNames.has(name)) + .sort(([a], [b]) => permissionLabel(a).localeCompare(permissionLabel(b))); + const consumed = data.events.filter((event) => event in CONSUMED_EVENT_LABELS); + const ignored = data.events.filter((event) => !(event in CONSUMED_EVENT_LABELS)); + + return ( +
+
+ + {/* Surface the one state worth acting on even while collapsed. */} + {blocked.length > 0 && !open ? ( + + + {blockedSummary(blocked)} + + ) : null} +
+ + {open && ( +
+ +
+ {required.map((permission) => ( + + ))} +
+ + + {alsoGranted.length > 0 && ( + +

+ {alsoGranted.map(([name, level], index) => ( + + {index > 0 ? " · " : ""} + {permissionLabel(name)} {level} + + ))} +

+ + )} + + +

+ {consumed.length > 0 + ? consumed.map((event) => CONSUMED_EVENT_LABELS[event]).join(" · ") + : "None of this installation's subscriptions produce First Tree activity."} +

+ {ignored.length > 0 && ( + // Named rather than hidden: "you subscribed to it, we drop it" is + // the answer when someone is hunting a webhook that changed nothing. +

+ Subscribed but unused: {ignored.join(", ")} +

+ )} + + + +
+ + + {data.installationId} + + + + {formatDate(data.createdAt)} + {formatDate(data.updatedAt)} +
+ +
+ )} +
+ ); +} + +function RequiredPermissionRow({ + permission, + manageUrl, + readOnly, +}: { + permission: RequiredPermission; + manageUrl: string; + readOnly: boolean; +}) { + // Blocked, not "needs you": the fix lives on GitHub's side, and a First Tree + // admin role alone doesn't establish they can grant it there (DESIGN.md §3). + const Glyph = permission.satisfied ? CircleCheck : CircleAlert; + const color = permission.satisfied ? "var(--success)" : "var(--state-blocked)"; + return ( +
+ + + + {permissionLabel(permission.name)} + + + {permission.granted ?? "not granted"} + + {!permission.satisfied && ( + + {shortfallCopy(permission.name, permission.required)} + + )} + + {!permission.satisfied && !readOnly && ( + + )} +
+ ); +} + +function CopyInstallationId({ installationId }: { installationId: number }) { + const { status, copy } = useCopyFeedback(); + const copied = status === "copied"; + return ( + + ); +} + +function DetailBlock({ label, children }: { label: string; children: ReactNode }) { + return ( +
+
+ {label} +
+ {children} +
+ ); +} + +function DetailField({ term, children }: { term: string; children: ReactNode }) { + return ( + <> +
+ {term} +
+
+ {children} +
+ + ); +} + +/** + * Absolute dates, not "3 days ago": this block is read while reconstructing a + * timeline against GitHub's own audit log, where a relative age has to be + * converted back before it's usable. Named month over an all-numeric date so + * the day/month order can't be misread across locales. + */ +function formatDate(iso: string): string { + const parsed = new Date(iso); + if (Number.isNaN(parsed.getTime())) return iso; + return parsed.toLocaleDateString(undefined, { year: "numeric", month: "short", day: "numeric" }); +} diff --git a/packages/web/src/pages/settings-github-preview.tsx b/packages/web/src/pages/settings-github-preview.tsx index 3cf80485c..d0d599abe 100644 --- a/packages/web/src/pages/settings-github-preview.tsx +++ b/packages/web/src/pages/settings-github-preview.tsx @@ -1,20 +1,12 @@ -import { - ArrowRight, - Bot, - Building2, - ChevronRight, - ExternalLink, - FolderGit2, - Github, - PauseCircle, - User, -} from "lucide-react"; +import type { GithubAppInstallationOutput } from "@first-tree/shared"; +import { ArrowRight, Bot, Building2, ExternalLink, FolderGit2, Github, PauseCircle, User } from "lucide-react"; import { useState } from "react"; import { Button } from "../components/ui/button.js"; import { PageHeader } from "../components/ui/page-header.js"; import { Section } from "../components/ui/section.js"; import { Select } from "../components/ui/select.js"; import { SettingRow } from "../components/ui/setting-row.js"; +import { GithubConnectionDetails } from "./github-connection-details.js"; /** * DEV-only visual review for Settings → GitHub (the connected GitHub App @@ -38,9 +30,11 @@ import { SettingRow } from "../components/ui/setting-row.js"; * enough to review their hierarchy without a live GitHub installation. */ -const MOCK = { +const MOCK: GithubAppInstallationOutput = { + installationId: 131952074, + accountType: "Organization", accountLogin: "agent-team-foundation", - accountType: "Organization" as const, + accountGithubId: 987654, permissions: { issues: "write", members: "read", @@ -49,10 +43,18 @@ const MOCK = { contents: "write", pull_requests: "write", administration: "write", - } as Record, + }, events: ["issues", "issue_comment", "member", "pull_request", "pull_request_review", "push"], + suspended: false, manageUrl: "https://github.com/organizations/agent-team-foundation/settings/installations/131952074", - installationId: 131952074, + createdAt: "2026-08-03T09:12:00.000Z", + updatedAt: "2026-08-06T16:41:00.000Z", +}; + +/** Same installation with a downgraded scope — the one state worth acting on. */ +const MOCK_MISSING_PERMISSION: GithubAppInstallationOutput = { + ...MOCK, + permissions: { ...MOCK.permissions, pull_requests: "read" }, }; const AccountIcon = MOCK.accountType === "Organization" ? Building2 : User; @@ -78,85 +80,6 @@ function SuspendedBanner() { ); } -function ConnectionDetails({ defaultOpen = false }: { defaultOpen?: boolean }) { - const [open, setOpen] = useState(defaultOpen); - const permissionEntries = Object.entries(MOCK.permissions); - const regionId = "github-connection-details"; - - return ( -
- - - {open && ( -
- {permissionEntries.length > 0 && ( -
-
- Permissions granted -
-
    - {permissionEntries.map(([key, value]) => ( -
  • - {key}: {value} -
    • - ))} -
-
- )} - {MOCK.events.length > 0 && ( -
-
- Subscribed events -
-
- {MOCK.events.join(", ")} -
-
- )} - - Installation #{MOCK.installationId} - -
- )} -
- ); -} - function AutomaticHandlingPreview() { const [agentUuid, setAgentUuid] = useState("dev-agent"); const agentName = agentUuid === "release-agent" ? "Release Agent" : "Dev Assistant"; @@ -226,7 +149,15 @@ function PageShell({ children }: { children: React.ReactNode }) { ); } -function InstalledCard({ suspended = false, detailsOpen = false }: { suspended?: boolean; detailsOpen?: boolean }) { +function InstalledCard({ + suspended = false, + detailsOpen = false, + data = MOCK, +}: { + suspended?: boolean; + detailsOpen?: boolean; + data?: GithubAppInstallationOutput; +}) { return (
@@ -238,9 +169,9 @@ function InstalledCard({ suspended = false, detailsOpen = false }: { suspended?: - Connected to github.com/{MOCK.accountLogin} + Connected to github.com/{data.accountLogin} - {MOCK.accountType} + {data.accountType} } control={ @@ -249,7 +180,7 @@ function InstalledCard({ suspended = false, detailsOpen = false }: { suspended?: Manage connection
@@ -366,10 +299,27 @@ export function SettingsGithubPreviewPage() { - + + + + + + + + + From a8947b631ae5e3e3b3d359e0da1219940a84f356 Mon Sep 17 00:00:00 2001 From: Gandy2025 Date: Mon, 10 Aug 2026 15:58:50 +0800 Subject: [PATCH 3/5] fix(web): hang SettingRow extras off the row's text column Anything a row rendered below its first line reset to the section's left edge while the row's own title sat indented past the glyph tile, so an expanded Connection details block read as a detached slab against a second, further-left margin. - SettingRow indents its children by the new `--setting-row-indent` (glyph tile + the gap after it) when the row has a glyph, so extras hang off the title rather than the container. - the details disclosure cancels its button's own `px-3`, putting the chevron on that same column, and indents the open blocks by chevron + gap so they line up under the toggle's label. Measured on /preview/settings-github: row title and chevron both land at x=83, every detail block label at x=103, and the block's right-hand action still ends flush with the row's own buttons at x=883. --- .../ui/__tests__/setting-row.test.tsx | 31 ++++++++++++++++++- .../web/src/components/ui/setting-row.tsx | 15 ++++++++- packages/web/src/index.css | 5 +++ .../src/pages/github-connection-details.tsx | 14 ++++++++- 4 files changed, 62 insertions(+), 3 deletions(-) diff --git a/packages/web/src/components/ui/__tests__/setting-row.test.tsx b/packages/web/src/components/ui/__tests__/setting-row.test.tsx index c7747753b..2ea838c94 100644 --- a/packages/web/src/components/ui/__tests__/setting-row.test.tsx +++ b/packages/web/src/components/ui/__tests__/setting-row.test.tsx @@ -46,7 +46,36 @@ describe("SettingRow", () => { expect(control?.parentElement?.className).toContain("sm:justify-end"); // Extras render below the row line, outside the control cluster. const extra = container.querySelector('[data-testid="extra"]'); - expect(extra?.parentElement?.getAttribute("data-setting-row")).toBe("true"); + expect(extra?.closest("[data-setting-row]")).not.toBeNull(); + expect(extra?.parentElement?.contains(control)).toBe(false); + + await act(async () => root.unmount()); + }); + + it("hangs extras off the row's text column when it has a glyph", async () => { + const { container, root } = await renderDom( + } title="GitHub App"> + Connection details + , + ); + + // Without this the block resets to the container edge while the title sits + // indented past the glyph, leaving two competing left edges. + const extraWrapper = container.querySelector('[data-testid="extra"]')?.parentElement; + expect(extraWrapper?.style.paddingLeft).toBe("var(--setting-row-indent)"); + + await act(async () => root.unmount()); + }); + + it("leaves extras flush when there is no glyph to align to", async () => { + const { container, root } = await renderDom( + + Detail + , + ); + + const extraWrapper = container.querySelector('[data-testid="extra"]')?.parentElement; + expect(extraWrapper?.style.paddingLeft).toBe(""); await act(async () => root.unmount()); }); diff --git a/packages/web/src/components/ui/setting-row.tsx b/packages/web/src/components/ui/setting-row.tsx index f8fb8476f..d3524c553 100644 --- a/packages/web/src/components/ui/setting-row.tsx +++ b/packages/web/src/components/ui/setting-row.tsx @@ -87,7 +87,20 @@ export function SettingRow({
) : null}
- {children} + + {/* Extras hang off the row's text column, not the section edge. Without + this they reset to the container's left margin while the row's own + title sits indented past the glyph, so an expanded block reads as a + detached slab with a second, further-left edge. */} + {children ?
{children}
: null}
); } + +/** + * Distance from the row's left edge to where its title starts — the glyph tile + * plus the gap after it, or nothing when the row has no glyph. + */ +function textColumnOffset(icon: ReactNode): string | undefined { + return icon ? "var(--setting-row-indent)" : undefined; +} diff --git a/packages/web/src/index.css b/packages/web/src/index.css index ecca60b1b..dffb92db7 100644 --- a/packages/web/src/index.css +++ b/packages/web/src/index.css @@ -554,6 +554,11 @@ video[data-onboarding-orientation-video]::cue { /* Shared label column for ConfigRow / IdentityField / DangerActionRow grids. */ --agent-detail-label-col: 8.25rem; + /* SettingRow's text column: the glyph tile (--sp-7) plus the gap after it + (--sp-2_5). Anything a row renders below its first line indents by this so + it hangs off the title instead of resetting to the section edge. */ + --setting-row-indent: 2.375rem; /* 38px */ + /* Default color-scheme so native form controls, scrollbars, and the canvas match the light theme (.dark flips this to dark below). */ color-scheme: light; diff --git a/packages/web/src/pages/github-connection-details.tsx b/packages/web/src/pages/github-connection-details.tsx index e01806c3b..1ec3e62bb 100644 --- a/packages/web/src/pages/github-connection-details.tsx +++ b/packages/web/src/pages/github-connection-details.tsx @@ -139,10 +139,13 @@ export function GithubConnectionDetails({ return (
+ {/* `-ml-3` cancels the button's own `px-3` so the chevron lands on this + block's left edge instead of floating one padding step inside it. */} ); } diff --git a/packages/web/src/pages/styleguide-preview.tsx b/packages/web/src/pages/styleguide-preview.tsx index f670ba57a..03e35486c 100644 --- a/packages/web/src/pages/styleguide-preview.tsx +++ b/packages/web/src/pages/styleguide-preview.tsx @@ -1,3 +1,4 @@ +import { Plug } from "lucide-react"; import { type ReactNode, useEffect, useState } from "react"; import { AgentStatusChip } from "../components/ui/agent-status-chip.js"; import { Badge } from "../components/ui/badge.js"; @@ -25,6 +26,7 @@ import { PresenceChip } from "../components/ui/presence-chip.js"; import { SectionHeader, UppercaseLabel } from "../components/ui/section-header.js"; import { SegmentedControl } from "../components/ui/segmented-control.js"; import { Select } from "../components/ui/select.js"; +import { SettingRow } from "../components/ui/setting-row.js"; import { StateChip } from "../components/ui/state-chip.js"; import { StateDot } from "../components/ui/state-dot.js"; import { StatusGlyph } from "../components/ui/status-glyph.js"; @@ -697,7 +699,7 @@ export function StyleguidePreviewPage() { {/* ─── Containers ────────────────────────────────────────────────── */} -
+
@@ -723,6 +725,31 @@ export function StyleguidePreviewPage() { + SettingRow +
+ {/* No chrome of its own — an enclosing Section owns the rule above. + Extras hang off the row's text column, not the container edge. */} + } + title="GitHub App" + description="Connected to github.com/acme" + control={ + + } + > + + Extras indent to the title, so the block reads as one column. + + + optional} + /> +
+ Tile From d700e8273edb44e869a700a3d9f3bf22699b5f24 Mon Sep 17 00:00:00 2001 From: Gandy Date: Tue, 11 Aug 2026 11:29:06 +0800 Subject: [PATCH 5/5] fix(web): scope the GitHub connection readout to what it actually proves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of this PR's diagnostics asserted more than the data behind them supports: - `GITHUB_APP_REQUIRED_PERMISSIONS` / "Required by First Tree" presented `issues: write` + `pull_requests: write` as the whole install contract, but that set only gates the GitHub Task Agent's automatic replies — Context Reviewer, repository coverage, and the capability probe each gate on their own permissions and events. An admin reading a green checklist could conclude the installation was generally usable. Renamed to `GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS`, with the heading and comments scoped to that capability. - `installation` / `installation_repositories` were filed under "Subscribed but unused". The webhook route consumes both through `handleInstallationLifecycle` before `buildRule` is ever reached, and they are what keeps the installation row and its repository coverage current — so a normally configured installation was being reported as carrying dead subscriptions. They now have their own class ("Kept in sync from"), separate from both activity events and dropped ones. - `createdAt` was labelled "Connected". The row is written by the `installation.created` webhook while still unbound and survives a disconnect/rebind, so it never records when this team connected. Labelled "First seen" — this block exists to reconstruct a timeline, which is exactly where a wrong date does the most damage. The preview fixture now subscribes to the lifecycle events a real App carries, so the gallery mirrors all three event classes. Co-Authored-By: Claude Opus 5 --- .../github-settings-connection-panel.md | 12 ++- .../src/services/team-agent-settings.ts | 4 +- ...b-task-reply-required-permissions.test.ts} | 6 +- packages/shared/src/index.ts | 2 +- packages/shared/src/schemas/github-app.ts | 21 ++++-- ...github-app-installation-panel-dom.test.tsx | 2 +- .../github-connection-details-dom.test.tsx | 34 +++++++-- .../__tests__/preview-pages-extra.test.tsx | 2 +- .../src/pages/github-connection-details.tsx | 74 ++++++++++++++----- .../web/src/pages/settings-github-preview.tsx | 15 +++- 10 files changed, 127 insertions(+), 45 deletions(-) rename packages/shared/src/__tests__/{github-app-required-permissions.test.ts => github-task-reply-required-permissions.test.ts} (82%) diff --git a/packages/qa/cases/cross-surface/github-settings-connection-panel.md b/packages/qa/cases/cross-surface/github-settings-connection-panel.md index a2bb75f23..d2682723f 100644 --- a/packages/qa/cases/cross-surface/github-settings-connection-panel.md +++ b/packages/qa/cases/cross-surface/github-settings-connection-panel.md @@ -106,16 +106,20 @@ only touches the webhook where an `installation.created` delivery records the ro Reinstall (step 1, when installed) and Disconnect (step 2). The Repositories section hands off to Settings → Repositories rather than editing repository URLs here. - Admin, not connected: a "Connect GitHub" call-to-action. -- Connection details, permissions satisfied: every entry under "Required by First Tree" reads as ready, and no - GitHub-side call to action appears. +- Connection details, permissions satisfied: every entry under "Required for automatic replies" reads as ready, and no + GitHub-side call to action appears. The heading stays scoped to that capability — it must not claim the installation + satisfies First Tree generally, since Context Reviewer and repository coverage gate on their own requirements. - Connection details, a required permission missing (downgrade `issues` or `pull_requests` to `read` on the installation row): the entry reads as blocked and names what it costs, a "Grant on GitHub" action appears for an admin, and the shortfall is also visible on the *collapsed* disclosure. This must agree with the server: the same installation makes `team-agent` assignment fail with `github_app_task_reply_permission_required`, so the readout and the gate never disagree. - Connection details, facts: subscribed events First Tree does not consume are listed as unused rather than blended into - the consumed list, and the installation id copies (a non-secure-context copy failure surfaces "Copy failed" instead of - looking inert). + the consumed list; `installation` / `installation_repositories` are *not* among them, since the webhook route consumes + both through `handleInstallationLifecycle` — a normally configured installation must never be described as carrying + unused subscriptions. The installation timestamp is labelled "First seen", not "Connected": the row is written unbound + by `installation.created` and survives disconnect/rebind, so it does not record when this team connected. The + installation id copies (a non-secure-context copy failure surfaces "Copy failed" instead of looking inert). - Google/OIDC admin without GitHub identity: after opening Connect, only the inline link-account state appears. After the identity is linked and the same Team is restored, the panel shows the linked login and Install remains a separate click. - Install preflight mismatch: the current First Tree session and selected Team remain unchanged; the recovery message diff --git a/packages/server/src/services/team-agent-settings.ts b/packages/server/src/services/team-agent-settings.ts index f400b10f5..facc08567 100644 --- a/packages/server/src/services/team-agent-settings.ts +++ b/packages/server/src/services/team-agent-settings.ts @@ -1,5 +1,5 @@ import { - GITHUB_APP_REQUIRED_PERMISSIONS, + GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS, githubPermissionSatisfies, ORG_SETTINGS_NAMESPACES, type OrgGithubFeaturesOutput, @@ -78,7 +78,7 @@ function taskReplyInstallationBlocker(installation: InstallationRow | null): Set if (installation.suspendedAt) return blocker("github_app_suspended", "manage_github_installation"); // Same requirement set the Settings → GitHub readout renders, so an admin is // never shown a healthy connection while this gate refuses the assignment. - const unsatisfied = Object.entries(GITHUB_APP_REQUIRED_PERMISSIONS).some( + const unsatisfied = Object.entries(GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS).some( ([permission, level]) => !githubPermissionSatisfies(installation.permissions[permission], level), ); if (unsatisfied) { diff --git a/packages/shared/src/__tests__/github-app-required-permissions.test.ts b/packages/shared/src/__tests__/github-task-reply-required-permissions.test.ts similarity index 82% rename from packages/shared/src/__tests__/github-app-required-permissions.test.ts rename to packages/shared/src/__tests__/github-task-reply-required-permissions.test.ts index e651f6ab0..4f402fa99 100644 --- a/packages/shared/src/__tests__/github-app-required-permissions.test.ts +++ b/packages/shared/src/__tests__/github-task-reply-required-permissions.test.ts @@ -1,12 +1,12 @@ import { describe, expect, it } from "vitest"; -import { GITHUB_APP_REQUIRED_PERMISSIONS, githubPermissionSatisfies } from "../schemas/github-app.js"; +import { GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS, githubPermissionSatisfies } from "../schemas/github-app.js"; -describe("GITHUB_APP_REQUIRED_PERMISSIONS", () => { +describe("GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS", () => { it("is the set the task-reply gate enforces", () => { // Both the server's `taskReplyInstallationBlocker` and the Settings → // GitHub readout derive from this. Changing it changes what an admin is // told AND what the server will publish, so it is pinned here. - expect(GITHUB_APP_REQUIRED_PERMISSIONS).toEqual({ issues: "write", pull_requests: "write" }); + expect(GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS).toEqual({ issues: "write", pull_requests: "write" }); }); }); diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 0920dcd53..1e49645a7 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -780,8 +780,8 @@ export { export { GITHUB_ACCOUNT_TYPES, GITHUB_APP_CONNECT_STATUSES, - GITHUB_APP_REQUIRED_PERMISSIONS, GITHUB_PERMISSION_LEVELS, + GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS, type GithubAccountType, type GithubAppConnectBody, type GithubAppConnectPanelInstallation, diff --git a/packages/shared/src/schemas/github-app.ts b/packages/shared/src/schemas/github-app.ts index 0e89ce981..f90d9a3ef 100644 --- a/packages/shared/src/schemas/github-app.ts +++ b/packages/shared/src/schemas/github-app.ts @@ -52,20 +52,27 @@ export const githubAppInstallationPermissionsSchema = z.record(z.string(), githu export type GithubAppInstallationPermissions = z.infer; /** - * The permission levels an installation must grant before First Tree can act - * as the App on it — `permission name -> minimum level`. + * The permission levels an installation must grant before the GitHub Task + * Agent can reply automatically on Issues and pull requests — + * `permission name -> required level`. * - * This is the one definition of "is this installation good enough". The server - * gates the GitHub Task Agent on it (`taskReplyInstallationBlocker`) and the - * Settings → GitHub readout renders it, so an admin is never told the - * installation is fine while the server refuses to publish (or the reverse). + * Scoped to that one capability, not to the installation as a whole. Other + * First Tree capabilities gate on their own permissions, events, and + * repository coverage (Context Reviewer, the setup-capability probe, …), so + * this set answers "can automatic replies work here", never "is this + * installation generally usable". + * + * Within that scope it is the one definition: the server gates the GitHub Task + * Agent on it (`taskReplyInstallationBlocker`) and the Settings → GitHub + * readout renders it, so an admin is never told replies are fine while the + * server refuses to publish (or the reverse). * * `metadata: read` is deliberately absent: GitHub grants it to every App * install and it cannot be withheld, so listing it would only ever render a * tautological ✓. The reply publisher still requests it when minting a scoped * token — that is a token scope, not a gate. */ -export const GITHUB_APP_REQUIRED_PERMISSIONS = { +export const GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS = { issues: "write", pull_requests: "write", } as const satisfies Record; diff --git a/packages/web/src/pages/__tests__/github-app-installation-panel-dom.test.tsx b/packages/web/src/pages/__tests__/github-app-installation-panel-dom.test.tsx index 63982556b..d6e3ad29b 100644 --- a/packages/web/src/pages/__tests__/github-app-installation-panel-dom.test.tsx +++ b/packages/web/src/pages/__tests__/github-app-installation-panel-dom.test.tsx @@ -197,7 +197,7 @@ describe("GithubAppInstallationPanel", () => { await click(detailsToggle); expect(detailsToggle?.getAttribute("aria-expanded")).toBe("true"); - expect(container.textContent).toContain("Required by First Tree"); + expect(container.textContent).toContain("Required for automatic replies"); expect(container.textContent).toContain("Also granted"); expect(container.textContent).toContain("Contents"); expect(container.textContent).toContain("Issues"); diff --git a/packages/web/src/pages/__tests__/github-connection-details-dom.test.tsx b/packages/web/src/pages/__tests__/github-connection-details-dom.test.tsx index 62f41ac9b..a24310520 100644 --- a/packages/web/src/pages/__tests__/github-connection-details-dom.test.tsx +++ b/packages/web/src/pages/__tests__/github-connection-details-dom.test.tsx @@ -51,17 +51,21 @@ describe("GithubConnectionDetails", () => { const { container, root } = await renderDom(); expect(buttonByText(container, "Connection details")?.getAttribute("aria-expanded")).toBe("false"); - expect(container.textContent).not.toContain("Required by First Tree"); + expect(container.textContent).not.toContain("Required for automatic replies"); expect(container.textContent).not.toContain("131952074"); await act(async () => root.unmount()); }); - it("answers 'is this installation good enough' before listing raw grants", async () => { + it("scopes the requirement list to the capability it actually gates", async () => { const { container, root } = await renderDom(); - // Required set is named in prose and marked satisfied. - expect(container.textContent).toContain("Required by First Tree"); + // `issues: write` / `pull_requests: write` only cover the GitHub Task + // Agent's automatic replies. Presenting them as First Tree's blanket + // install contract would tell an admin the installation is generally fine + // when only this one capability has been checked. + expect(container.textContent).toContain("Required for automatic replies"); + expect(container.textContent).not.toContain("Required by First Tree"); expect(container.textContent).toContain("Issues"); expect(container.textContent).toContain("Pull requests"); // Everything else is secondary, not mixed into the requirement list. @@ -136,11 +140,31 @@ describe("GithubConnectionDetails", () => { await act(async () => root.unmount()); }); + it("does not call handled lifecycle traffic unused", async () => { + // `installation` / `installation_repositories` never reach `buildRule`: + // the webhook route hands them to `handleInstallationLifecycle` first, and + // they are what keeps this very row and its repository coverage current. + // A normally configured installation subscribes to both, so filing them + // under "unused" reports a healthy install as misconfigured. + const data = installation({ events: ["issues", "installation", "installation_repositories", "push"] }); + const { container, root } = await renderDom(); + + expect(container.textContent).toContain("Kept in sync from: Installation lifecycle · Repository access changes"); + expect(container.textContent).toContain("Subscribed but unused: push"); + expect(container.textContent).not.toContain("Subscribed but unused: installation"); + + await act(async () => root.unmount()); + }); + it("renders the installation id and both timestamps the API already returns", async () => { const { container, root } = await renderDom(); expect(container.textContent).toContain("123"); - expect(container.textContent).toContain("Connected"); + // "First seen", not "Connected": the row is written by the + // `installation.created` webhook while still unbound and survives a + // disconnect/rebind, so it never records when this team connected. + expect(container.textContent).toContain("First seen"); + expect(container.textContent).not.toContain("Connected"); expect(container.textContent).toContain("Last updated"); expect(container.textContent).toContain( new Date("2026-08-03T09:12:00.000Z").toLocaleDateString(undefined, { diff --git a/packages/web/src/pages/__tests__/preview-pages-extra.test.tsx b/packages/web/src/pages/__tests__/preview-pages-extra.test.tsx index e36268ab8..414e40c52 100644 --- a/packages/web/src/pages/__tests__/preview-pages-extra.test.tsx +++ b/packages/web/src/pages/__tests__/preview-pages-extra.test.tsx @@ -363,7 +363,7 @@ describe("extra preview pages", () => { await click(detailsButtons[0] ?? detailsButtons[1] ?? buttonByText(rendered.container, "Connection details")); expect(detailsButtons[0]?.getAttribute("aria-expanded")).toBe("true"); expect(text(rendered.container)).toContain("131952074"); - expect(text(rendered.container)).toContain("Required by First Tree"); + expect(text(rendered.container)).toContain("Required for automatic replies"); await cleanupRendered(rendered); }); diff --git a/packages/web/src/pages/github-connection-details.tsx b/packages/web/src/pages/github-connection-details.tsx index 26654cd6b..4f13ec7d5 100644 --- a/packages/web/src/pages/github-connection-details.tsx +++ b/packages/web/src/pages/github-connection-details.tsx @@ -1,5 +1,5 @@ import { - GITHUB_APP_REQUIRED_PERMISSIONS, + GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS, type GithubAppInstallationOutput, type GithubPermissionLevel, githubPermissionSatisfies, @@ -30,12 +30,12 @@ const PERMISSION_LABELS: Record = { }; /** - * Webhook events First Tree actually turns into something — mirrors the - * `buildRule` switch in the server's `github-normalize.ts`. Everything else an - * installation subscribes to is delivered and dropped, which is exactly what - * you want said out loud while working out why an event produced no message. + * Webhook events First Tree turns into agent activity — mirrors the `buildRule` + * switch in the server's `github-normalize.ts`. These are the ones that can + * produce a message, which is what you're checking when an event seems to have + * gone nowhere. */ -const CONSUMED_EVENT_LABELS: Record = { +const ACTIVITY_EVENT_LABELS: Record = { commit_comment: "Commit comments", discussion: "Discussions", discussion_comment: "Discussion comments", @@ -46,6 +46,21 @@ const CONSUMED_EVENT_LABELS: Record = { pull_request_review_comment: "PR review comments", }; +/** + * Events the webhook route consumes before it ever reaches `buildRule` — they + * create, refresh, suspend/resume and delete the installation record and its + * repository coverage (`api/webhooks/github-app.ts`, `handleInstallationLifecycle`). + * + * Classified apart from both lists on purpose: they produce no agent activity, + * so they can't sit under the activity heading, but they are very much handled, + * so grouping them with the dropped subscriptions would tell an admin their + * normally configured installation is misconfigured. + */ +const LIFECYCLE_EVENT_LABELS: Record = { + installation: "Installation lifecycle", + installation_repositories: "Repository access changes", +}; + /** What the team loses while a required permission is missing. */ const PERMISSION_SHORTFALL: Record = { issues: "Agents can't post replies on issues.", @@ -81,7 +96,7 @@ function blockedSummary(blocked: RequiredPermission[]): string { } function readRequiredPermissions(permissions: GithubAppInstallationOutput["permissions"]): RequiredPermission[] { - return Object.entries(GITHUB_APP_REQUIRED_PERMISSIONS).map(([name, required]) => ({ + return Object.entries(GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS).map(([name, required]) => ({ name, required, granted: permissions[name], @@ -94,15 +109,18 @@ function readRequiredPermissions(permissions: GithubAppInstallationOutput["permi * detail. It used to transcribe GitHub's `permissions` / `events` blobs * verbatim, which left the reader to diff them against a requirement they'd * have to already know. It now answers the question they opened it with — - * "is this installation good enough for First Tree?" — before falling back to - * the raw grants: + * "can this installation do the thing I'm waiting on?" — before falling back + * to the raw grants: * - * - **Required by First Tree** — one row per `GITHUB_APP_REQUIRED_PERMISSIONS` - * entry (the same set the server's task-reply gate gives out), each marked - * ready or blocked. + * - **Required for automatic replies** — one row per + * `GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS` entry (the same set the server's + * task-reply gate reads), each marked ready or blocked. Scoped to that + * capability, not a verdict on the installation: other capabilities gate on + * their own permissions, events, and repository coverage. * - **Also granted** — everything else, secondary. - * - **Events** — the subscriptions First Tree consumes, named in prose, with - * ignored subscriptions called out instead of silently blended in. + * - **Events** — the subscriptions that produce agent activity, named in + * prose, with lifecycle traffic and genuinely dropped subscriptions each + * called out separately instead of blended together. * - **Installation** — id (copyable), and the two timestamps the API already * returns, which are the first thing you want when reconstructing "when * did this change". @@ -136,8 +154,11 @@ export function GithubConnectionDetails({ const alsoGranted = Object.entries(data.permissions) .filter(([name]) => !requiredNames.has(name)) .sort(([a], [b]) => permissionLabel(a).localeCompare(permissionLabel(b))); - const consumed = data.events.filter((event) => event in CONSUMED_EVENT_LABELS); - const ignored = data.events.filter((event) => !(event in CONSUMED_EVENT_LABELS)); + const activity = data.events.filter((event) => event in ACTIVITY_EVENT_LABELS); + const lifecycle = data.events.filter((event) => event in LIFECYCLE_EVENT_LABELS); + const ignored = data.events.filter( + (event) => !(event in ACTIVITY_EVENT_LABELS) && !(event in LIFECYCLE_EVENT_LABELS), + ); return (
@@ -183,7 +204,7 @@ export function GithubConnectionDetails({ paddingLeft: "var(--sp-5)", }} > - +
{required.map((permission) => (

- {consumed.length > 0 - ? consumed.map((event) => CONSUMED_EVENT_LABELS[event]).join(" · ") + {activity.length > 0 + ? activity.map((event) => ACTIVITY_EVENT_LABELS[event]).join(" · ") : "None of this installation's subscriptions produce First Tree activity."}

+ {lifecycle.length > 0 && ( + // Handled, just not by the activity path — say so, or a normally + // configured installation reads as carrying dead subscriptions. +

+ Kept in sync from: {lifecycle.map((event) => LIFECYCLE_EVENT_LABELS[event]).join(" · ")} +

+ )} {ignored.length > 0 && ( // Named rather than hidden: "you subscribed to it, we drop it" is // the answer when someone is hunting a webhook that changed nothing. @@ -232,7 +260,13 @@ export function GithubConnectionDetails({ - {formatDate(data.createdAt)} + {/* "First seen", not "Connected": this is when First Tree first + stored the installation. The row is created by the + `installation.created` webhook while still unbound, and it + survives a disconnect/rebind — so it is not the moment this + team connected. Labelling it that way would put a wrong date + into exactly the timeline this block exists to reconstruct. */} + {formatDate(data.createdAt)} {formatDate(data.updatedAt)} diff --git a/packages/web/src/pages/settings-github-preview.tsx b/packages/web/src/pages/settings-github-preview.tsx index d0d599abe..dfdf721cb 100644 --- a/packages/web/src/pages/settings-github-preview.tsx +++ b/packages/web/src/pages/settings-github-preview.tsx @@ -44,7 +44,20 @@ const MOCK: GithubAppInstallationOutput = { pull_requests: "write", administration: "write", }, - events: ["issues", "issue_comment", "member", "pull_request", "pull_request_review", "push"], + // `installation` / `installation_repositories` are in here because a real + // App subscribes to them — they are the traffic that keeps this installation + // row and its repository coverage current. The gallery has to show them so + // the three event classes (activity / lifecycle / dropped) can be reviewed. + events: [ + "installation", + "installation_repositories", + "issues", + "issue_comment", + "member", + "pull_request", + "pull_request_review", + "push", + ], suspended: false, manageUrl: "https://github.com/organizations/agent-team-foundation/settings/installations/131952074", createdAt: "2026-08-03T09:12:00.000Z",