diff --git a/doc/connections/GITHUB-REVIEW-BOT.md b/doc/connections/GITHUB-REVIEW-BOT.md index 8458e532596..f0e9cf0f9ed 100644 --- a/doc/connections/GITHUB-REVIEW-BOT.md +++ b/doc/connections/GITHUB-REVIEW-BOT.md @@ -30,11 +30,11 @@ the GitHub Code Review Bot connector. This default-off setting is independent of 4. Paperclip reuses your verified GitHub identity when available. Otherwise, authorize the dedicated App and confirm the observed account once. Bot work uses installation credentials, never your personal GitHub token. -5. Connection verification and completion happen automatically. The connected - page shows the actual App identity and enabled repository count, with a - copyable optional review mention. Open **Bot settings** to manage the - bot; normal connection navigation is already available. Runtime and isolation - readiness remain separate prerequisites. +5. Connection verification and completion happen automatically. Paperclip opens + the bot's Settings with a connected confirmation modal showing the actual App + identity, enabled repository count, and a copyable optional review mention. + Dismiss it with **Done**, the close button, or Escape to stay in Settings. + Runtime and isolation readiness remain separate prerequisites. Installation may return before GitHub's separately delivered signed webhook ping. Paperclip keeps the same draft and credentials, shows a waiting state, diff --git a/ui/src/pages/apps/chat/ChatEndpoint.clipboard.test.tsx b/ui/src/pages/apps/chat/ChatEndpoint.clipboard.test.tsx index 5116d384116..6e8939fe1af 100644 --- a/ui/src/pages/apps/chat/ChatEndpoint.clipboard.test.tsx +++ b/ui/src/pages/apps/chat/ChatEndpoint.clipboard.test.tsx @@ -57,6 +57,7 @@ vi.mock("@/context/SidebarContext", () => ({ })); vi.mock("@/lib/router", () => ({ useNavigate: () => vi.fn(), + useLocation: () => ({ state: null }), useParams: () => ({ endpointId: "endpoint-a", tab: mocks.tab }), useSearchParams: () => [new URLSearchParams(mocks.search), mocks.setParams], Link: ({ children }: { children: React.ReactNode }) => ( diff --git a/ui/src/pages/apps/chat/ChatEndpointDetail.agentmail.test.tsx b/ui/src/pages/apps/chat/ChatEndpointDetail.agentmail.test.tsx index 16b0d1cfe9b..b20b156fe68 100644 --- a/ui/src/pages/apps/chat/ChatEndpointDetail.agentmail.test.tsx +++ b/ui/src/pages/apps/chat/ChatEndpointDetail.agentmail.test.tsx @@ -24,6 +24,7 @@ vi.mock("./EmailEndpointSetup", () => ({ EmailEndpointSettings: () =>

Email vi.mock("@/context/BreadcrumbContext", () => ({ useBreadcrumbs: () => ({ setBreadcrumbs: () => {} }) })); vi.mock("@/context/ToastContext", () => ({ useToast: () => ({ pushToast: () => {} }) })); vi.mock("@/lib/router", () => ({ + useLocation: () => ({ state: null }), useParams: () => ({ endpointId: "inbox", tab: mocks.tab }), useNavigate: () => mocks.navigate, Link: ({ children, to, ...props }: React.ComponentProps<"a"> & { to: string }) => {children}, Navigate: () => null, diff --git a/ui/src/pages/apps/chat/ChatEndpointDetail.tsx b/ui/src/pages/apps/chat/ChatEndpointDetail.tsx index 8f2f318bbbb..0596aebc902 100644 --- a/ui/src/pages/apps/chat/ChatEndpointDetail.tsx +++ b/ui/src/pages/apps/chat/ChatEndpointDetail.tsx @@ -9,11 +9,12 @@ import { AgentAvatar } from "@/components/AgentAvatar"; import { ChatConversationList } from "./ChatConversationList"; import { GitHubBotManagement, GitHubReviews } from "./GitHubBotManagement"; import { GitHubBotMention } from "./GitHubAppIdentity"; +import { GitHubConnectionComplete, type GitHubConnectionCompleteLocationState } from "./GitHubConnectionComplete"; import { EmailEndpointSettings } from "./EmailEndpointSetup"; import { EmailConnectionAccess } from "@/components/EmailConnectionAccess"; import { emailApi } from "@/api/email"; import { toolsApi } from "@/api/tools"; -import { useEffect, useMemo, useState } from "react"; +import { useEffect, useMemo, useRef, useState } from "react"; import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; import { AlertTriangle, @@ -57,7 +58,7 @@ import { useToast } from "@/context/ToastContext"; import { formatDateTime } from "@/lib/utils"; import { queryKeys } from "@/lib/queryKeys"; import { copyTextToClipboard } from "@/lib/clipboard"; -import { Link, Navigate, useNavigate, useParams } from "@/lib/router"; +import { Link, Navigate, useLocation, useNavigate, useParams } from "@/lib/router"; const tabs = ["settings", "access", "reviews", "conversations", "activity"] as const; type ChatTab = (typeof tabs)[number]; @@ -236,6 +237,9 @@ export function ChatEndpointDetail() { const tab = reviewId ? "reviews" : (routeTab ?? "settings"); const activeTab = tabs.includes(tab as ChatTab) ? (tab as ChatTab) : null; const navigate = useNavigate(); + const location = useLocation(); + const heading = useRef(null); + const completion = (location.state as GitHubConnectionCompleteLocationState | null)?.githubConnectionComplete; const { setBreadcrumbs } = useBreadcrumbs(); const endpointQuery = useQuery({ queryKey: queryKeys.chatEndpoints.detail(endpointId), @@ -304,7 +308,7 @@ export function ChatEndpointDetail() {
{endpoint.provider === "github" && }
-

+

{endpoint.provider === "github" ? {endpoint.assignedAgentName} : `${endpoint.assignedAgentName} in ${providerNames[endpoint.provider]}`}

@@ -359,6 +363,19 @@ export function ChatEndpointDetail() { {activeTab === "activity" && ( )} + {endpoint.provider === "github" && activeTab === "settings" && endpoint.status === "active" && + endpoint.setup?.step === "complete" && completion?.endpointId === endpoint.id && ( + { + const { githubConnectionComplete: _complete, ...rest } = location.state as GitHubConnectionCompleteLocationState; + navigate({ pathname: location.pathname, search: location.search, hash: location.hash }, { replace: true, state: rest }); + }} + onCloseAutoFocus={() => heading.current?.focus()} + /> + )}

); } diff --git a/ui/src/pages/apps/chat/GitHubBotManagement.test.tsx b/ui/src/pages/apps/chat/GitHubBotManagement.test.tsx index 13b0d9563b2..94a7b9c4a8c 100644 --- a/ui/src/pages/apps/chat/GitHubBotManagement.test.tsx +++ b/ui/src/pages/apps/chat/GitHubBotManagement.test.tsx @@ -69,6 +69,7 @@ vi.mock("@/lib/router", () => ({ reviewId: mocks.reviewId, }), useNavigate: () => vi.fn(), + useLocation: () => ({ state: null }), Link: ({ children, to, diff --git a/ui/src/pages/apps/chat/GitHubChatSetup.test.tsx b/ui/src/pages/apps/chat/GitHubChatSetup.test.tsx index 10e2ee0a4f1..49d9249fbb3 100644 --- a/ui/src/pages/apps/chat/GitHubChatSetup.test.tsx +++ b/ui/src/pages/apps/chat/GitHubChatSetup.test.tsx @@ -2,9 +2,12 @@ import { act } from "react"; import { createRoot, type Root } from "react-dom/client"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; -import { MemoryRouter, useLocation } from "react-router-dom"; +import { MemoryRouter, Route, Routes, useLocation } from "react-router-dom"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { GitHubChatSetup, gitHubAppManifestAction } from "./GitHubChatSetup"; +import { defaultGitHubReviewPolicy } from "@paperclipai/shared"; +import { ChatEndpointDetail } from "./ChatEndpointDetail"; +import { TooltipProvider } from "@/components/ui/tooltip"; import { agentsApi } from "@/api/agents"; import { chatEndpointsApi } from "@/api/chatEndpoints"; import { githubChatApi } from "@/api/githubChat"; @@ -71,6 +74,7 @@ vi.mock("@/api/chatEndpoints", () => ({ })); vi.mock("@/api/githubChat", () => ({ githubChatApi: { + configuration: vi.fn(), advance: vi.fn(), repositories: vi.fn(), registration: vi.fn(), @@ -161,6 +165,11 @@ describe("GitHub App wizard", () => { endpointId: "draft-1", state: "create", }); + vi.mocked(githubChatApi.configuration).mockResolvedValue({ + revision: 1, + configuration: { version: 1, toolsEnabled: true, responsibleUserId: "board", memberAccess: "selected", people: [], + defaults: defaultGitHubReviewPolicy(), repositories: {} }, + }); vi.mocked(githubChatApi.repositories).mockResolvedValue({ items: [], nextOffset: null, totalCount: 1, enabledCount: 1, availableCount: 1, }); @@ -168,7 +177,7 @@ describe("GitHub App wizard", () => { vi.mocked(githubChatApi.personalConnections).mockResolvedValue([]); vi.mocked(copyTextToClipboard).mockResolvedValue(); client = new QueryClient({ - defaultOptions: { queries: { retry: false, gcTime: 0 } }, + defaultOptions: { queries: { retry: false, gcTime: 60_000 } }, }); container = document.createElement("div"); document.body.append(container); @@ -266,13 +275,22 @@ describe("GitHub App wizard", () => { - - + + + } /> + } /> + Connectors
} /> + + + , ), ); await settle(); + // Completion mounts Settings and its own queries after the wizard redirect. + await settle(); + await settle(); } async function click(label: string) { const button = Array.from(document.querySelectorAll("button")).find( @@ -570,6 +588,8 @@ describe("GitHub App wizard", () => { it("completes automatically while reporting runtime readiness separately", async () => { fixture.endpoint = { ...fixture.endpoint, + status: "active", + setup: { step: "complete" }, botLabel: "Actual GitHub Name", botUsername: "actual-agent[bot]", resources: [{ enabled: true, label: "acme/repo" }], @@ -587,43 +607,65 @@ describe("GitHub App wizard", () => { ], }); await render("resume=draft-1"); - expect(container.querySelector("h1")?.textContent).toBe("GitHub connected"); - expect(container.textContent).toContain("Actual GitHub Name"); - expect(container.textContent).toContain("GitHub App name and logo"); - expect(container.textContent).toContain("1 repository enabled"); - expect(container.querySelector("footer a")?.getAttribute("href")).toBe("/apps/chat/draft-1/settings"); - expect(container.querySelector("details")?.hasAttribute("open")).toBe(false); - expect(container.textContent).toContain("Before your first review"); - expect(container.textContent).toContain("Configure a runtime"); - expect(container.textContent).not.toContain("Finish"); + expect(document.querySelector('[role="dialog"] h2')?.textContent).toBe("GitHub connected"); + expect(document.body.textContent).toContain("Actual GitHub Name"); + expect(document.body.textContent).toContain("GitHub App name and logo"); + expect(document.body.textContent).toContain("1 repository enabled"); + expect(container.querySelector("output")?.textContent).toBe("/apps/chat/draft-1/settings"); + expect(document.querySelector('[role="dialog"] details')?.hasAttribute("open")).toBe(false); + expect(document.body.textContent).toContain("Before your first review"); + expect(document.body.textContent).toContain("Configure a runtime"); + expect(document.body.textContent).not.toContain("Finish"); await click("Copy mention"); expect(copyTextToClipboard).toHaveBeenCalledWith( "@actual-agent review this pull request", ); }); + it.each(["Done", "Close", "Escape"])("dismisses with %s onto the same mounted Settings page", async (action) => { + fixture.endpoint = { ...fixture.endpoint, status: "active", setup: { step: "complete" } }; + vi.mocked(githubChatApi.advance).mockResolvedValue({ endpointId: "draft-1", state: "connected" }); + await render("resume=draft-1"); + const settings = container.querySelector("textarea"); + expect(settings).not.toBeNull(); + expect(document.querySelector('[role="dialog"]')).not.toBeNull(); + if (action === "Escape") { + await act(async () => { + document.dispatchEvent(new KeyboardEvent("keydown", { key: "Escape", bubbles: true })); + }); + await settle(); + } else await click(action); + expect(document.querySelector('[role="dialog"]')).toBeNull(); + expect(container.querySelector("output")?.textContent).toBe("/apps/chat/draft-1/settings"); + expect(container.querySelector("textarea")).toBe(settings); + expect(container.querySelector("h1")?.textContent).toBe("Reviewer"); + expect(document.activeElement).toBe(container.querySelector("h1")); + expect(githubChatApi.saveDraft).not.toHaveBeenCalled(); + expect(chatEndpointsApi.create).not.toHaveBeenCalled(); + }); it("reports copy failure without undoing the completed connection", async () => { - fixture.endpoint = { ...fixture.endpoint, status: "active", botUsername: "actual-agent[bot]" }; + fixture.endpoint = { ...fixture.endpoint, status: "active", setup: { step: "complete" }, botUsername: "actual-agent[bot]" }; vi.mocked(githubChatApi.advance).mockResolvedValue({ endpointId: "draft-1", state: "connected" }); vi.mocked(copyTextToClipboard).mockRejectedValueOnce(new Error("Clipboard unavailable")); await render("resume=draft-1"); await click("Copy mention"); - expect(container.textContent).toContain("Copy failed"); - expect(container.querySelector("h1")?.textContent).toBe("GitHub connected"); + expect(document.body.textContent).toContain("Copy failed"); + expect(document.querySelector('[role="dialog"] h2')?.textContent).toBe("GitHub connected"); expect(githubChatApi.registration).not.toHaveBeenCalled(); }); it("shows a useful next step when a connected bot has no enabled repositories", async () => { + fixture.endpoint = { ...fixture.endpoint, status: "active", setup: { step: "complete" } }; vi.mocked(githubChatApi.advance).mockResolvedValue({ endpointId: "draft-1", state: "connected" }); vi.mocked(githubChatApi.repositories).mockResolvedValue({ items: [], nextOffset: null, totalCount: 2, enabledCount: 0, availableCount: 2, }); await render("resume=draft-1"); - expect(container.textContent).toContain("No repositories enabled"); - expect(container.textContent).toContain("Enable a repository in Access to try your bot"); - expect(container.querySelector('a[href="/apps/chat/draft-1/access"]')).not.toBeNull(); + expect(document.body.textContent).toContain("No repositories enabled"); + expect(document.body.textContent).toContain("Enable a repository in Access to try your bot"); + expect(document.querySelector('[role="dialog"] a[href="/apps/chat/draft-1/access"]')).not.toBeNull(); }); it("does not flash App creation controls while checking a resumed connection", async () => { - fixture.endpoint = { ...fixture.endpoint, status: "active", botUsername: "actual-agent[bot]" }; + fixture.endpoint = { ...fixture.endpoint, status: "active", setup: { step: "complete" }, botUsername: "actual-agent[bot]" }; vi.mocked(githubChatApi.advance).mockReturnValue(new Promise(() => {})); await render("resume=draft-1"); expect(container.textContent).toContain("Checking your GitHub connection"); diff --git a/ui/src/pages/apps/chat/GitHubChatSetup.tsx b/ui/src/pages/apps/chat/GitHubChatSetup.tsx index 57c3bc8bf6a..873b670510d 100644 --- a/ui/src/pages/apps/chat/GitHubChatSetup.tsx +++ b/ui/src/pages/apps/chat/GitHubChatSetup.tsx @@ -21,7 +21,8 @@ import { useCompany } from "@/context/CompanyContext"; import { useBreadcrumbs } from "@/context/BreadcrumbContext"; import { useNavigate, useSearchParams, Link } from "@/lib/router"; import { buildPermissionsForTrustPreset } from "@/lib/trust-policy-ui"; -import { GitHubConnectionComplete } from "./GitHubConnectionComplete"; +import { queryKeys } from "@/lib/queryKeys"; +import type { GitHubConnectionCompleteLocationState } from "./GitHubConnectionComplete"; /** Restrict native manifest submission to GitHub registration endpoints. */ export function gitHubAppManifestAction( @@ -142,6 +143,17 @@ export function GitHubChatSetup() { const existingIdentityMethod = identityOnly || state?.identityMethod === "existing_connection"; const connected = state?.state === "connected" && !existing; + useEffect(() => { + if (!connected || !bot) return; + queryClient.setQueryData(queryKeys.chatEndpoints.detail(bot.id), bot); + if (selectedAgent) queryClient.setQueryData(queryKeys.agents.detail(selectedAgent.id), selectedAgent); + navigate(`/apps/chat/${bot.id}/settings`, { + replace: true, + state: { + githubConnectionComplete: { endpointId: bot.id, runtimeChecks: state.runtimeChecks }, + } satisfies GitHubConnectionCompleteLocationState, + }); + }, [connected, bot, selectedAgent, state, queryClient, navigate]); useEffect(() => { setBreadcrumbs([ { label: "Connectors", href: "/apps" }, @@ -238,11 +250,10 @@ export function GitHubChatSetup() { })} ); + if (connected) return null; return ( -
- {!identityOnly && !connected && ( +
+ {!identityOnly && ( )} - {!connected &&

+

{!bot ? "Choose agent" : identityOnly || state?.state === "identity" ? "Connect your account" : "Connect GitHub"} -

} +

{(error || (!identityOnly && progress.error) || accounts.error) && (

{error || @@ -331,14 +342,6 @@ export function GitHubChatSetup() {

{footer()} - ) : connected ? ( - ) : existing ? ( <>

diff --git a/ui/src/pages/apps/chat/GitHubConnectionComplete.tsx b/ui/src/pages/apps/chat/GitHubConnectionComplete.tsx index 97fb088f0f5..7b0c4a60c2a 100644 --- a/ui/src/pages/apps/chat/GitHubConnectionComplete.tsx +++ b/ui/src/pages/apps/chat/GitHubConnectionComplete.tsx @@ -6,24 +6,31 @@ import { githubChatApi } from "@/api/githubChat"; import { AgentAvatar, type AvatarAgent } from "@/components/AgentAvatar"; import { AppLogo } from "../AppLogo"; import { CopyText } from "@/components/CopyText"; -import { SetupWizardFooter } from "@/components/SetupWizard"; import { Button } from "@/components/ui/button"; +import { Dialog, DialogContent, DialogFooter, DialogTitle } from "@/components/ui/dialog"; import { agentAvatarUrl } from "@/lib/agent-avatar-url"; import { Link } from "@/lib/router"; import { GitHubAppBranding, gitHubBotMention } from "./GitHubAppIdentity"; +export type GitHubConnectionCompleteLocationState = { + githubConnectionComplete?: { + endpointId: string; + runtimeChecks?: GitHubAppWizardState["runtimeChecks"]; + }; +}; + export function GitHubConnectionComplete({ endpoint, agent, runtimeChecks, - onExit, - pending = false, + onClose, + onCloseAutoFocus, }: { endpoint: ChatEndpoint; agent?: AvatarAgent; runtimeChecks?: GitHubAppWizardState["runtimeChecks"]; - onExit: () => void; - pending?: boolean; + onClose: () => void; + onCloseAutoFocus?: () => void; }) { const repositories = useQuery({ queryKey: ["github-connected-repositories", endpoint.id], @@ -34,73 +41,82 @@ export function GitHubConnectionComplete({ const count = repositories.data?.enabledCount; const incomplete = runtimeChecks?.filter((check) => !check.ok) ?? []; return ( -

-
-
- -

GitHub connected

+ { if (!open) onClose(); }}> + { + event.preventDefault(); + onCloseAutoFocus(); + } : undefined} + > +
+
+
+ + GitHub connected +
+
+ +
+

+ {endpoint.botLabel ?? endpoint.botUsername ?? endpoint.assignedAgentName} +

+

+ + {endpoint.assignedAgentName} + + {endpoint.providerAccountLabel && <> + + + + {endpoint.providerAccountLabel} + + } +

+ + {repositories.isError ? "Repository access unavailable" : count === undefined ? "Repository access" : count === 0 ? "No repositories enabled" : `${count} ${count === 1 ? "repository" : "repositories"} enabled`} + +
+
+
+ {incomplete.length > 0 && ( +
+

Before your first review

+ {incomplete.map((check) => ( +

{check.detail}

+ ))} +
+ )} +
+
+

Try your bot

+

+ {count === 0 + ? "Enable a repository in Access to try your bot." + : "Post this on a pull request in an enabled repository."} +

+
+ + {mention} + + +
+ + + +
-
- -
-

- {endpoint.botLabel ?? endpoint.botUsername ?? endpoint.assignedAgentName} -

-

- - {endpoint.assignedAgentName} - - {endpoint.providerAccountLabel && <> - - - - {endpoint.providerAccountLabel} - - } -

- - {repositories.isError ? "Repository access unavailable" : count === undefined ? "Repository access" : count === 0 ? "No repositories enabled" : `${count} ${count === 1 ? "repository" : "repositories"} enabled`} - -
-
-
- {incomplete.length > 0 && ( -
-

Before your first review

- {incomplete.map((check) => ( -

{check.detail}

- ))} -
- )} -
-
-

Try your bot

-

- {count === 0 - ? "Enable a repository in Access to try your bot." - : "Post this on a pull request in an enabled repository."} -

-
- - {mention} - - -
- - - - -
+ + ); } diff --git a/ui/storybook/stories/github-bot-management/management.stories.tsx b/ui/storybook/stories/github-bot-management/management.stories.tsx index 033f9bb27d0..caef58955b8 100644 --- a/ui/storybook/stories/github-bot-management/management.stories.tsx +++ b/ui/storybook/stories/github-bot-management/management.stories.tsx @@ -1,4 +1,5 @@ -import { useState } from "react"; +import { useState, type ComponentProps } from "react"; +import { Button } from "@/components/ui/button"; import { useQueryClient } from "@tanstack/react-query"; import { githubChatApi } from "@/api/githubChat"; import { chatEndpointsApi } from "@/api/chatEndpoints"; @@ -42,7 +43,7 @@ const meta = { docs: { description: { component: - "Production pages and components. Start with **01 Journey / Settings**, then use the actual contextual sidebar to visit Access, Reviews, and Conversations. Try editing instructions, switch tabs, and save or discard the shared draft. **01 Journey / Connected** uses the production completion page, then opens the same bot Settings. **02 States** covers empty, loading, recoverable error, long content, and mobile. **03 Components** isolates the policy editor, people access, repository access, review rows and detail, pending review, and conversations. Provider operations use isolated fixture APIs; these stories are not live GitHub evidence.", + "Production pages and components. Start with **01 Journey / Settings**, then use the actual contextual sidebar to visit Access, Reviews, and Conversations. Try editing instructions, switch tabs, and save or discard the shared draft. **01 Journey / Connected** opens the production confirmation modal over the same bot Settings. Close it with Done, the ×, or Escape and continue editing Settings. **02 States** covers empty, loading, recoverable error, long content, and mobile. **03 Components** isolates the policy editor, people access, repository access, review rows and detail, pending review, and conversations. Provider operations use isolated fixture APIs; these stories are not live GitHub evidence.", }, }, }, @@ -79,7 +80,7 @@ const route = (tab: string) => ({ initialEntries: [`/PAP/apps/chat/${endpoint.id}/${tab}`], }); export const ConnectedJourney: Story = { - name: "01 Journey / Connected — copy a mention or open settings", + name: "01 Journey / Connected — dismiss to settings", parameters: { initialEntries: [`/PAP/apps/chat/connect?provider=github&resume=${endpoint.id}`] }, }; export const ConnectedMobile: Story = { @@ -205,20 +206,27 @@ export const RepositoryAccess: Story = { name: "03 Components / Repository access", render: () => component(), }; +function ConnectedExample(props: Omit, "onClose">) { + const [open, setOpen] = useState(true); + return <> + + {open && setOpen(false)} />} + ; +} export const ConnectedSummary: Story = { name: "03 Components / Connected summary", - render: () => component( {}} />), + render: () => component(), }; export const ConnectedRuntimeNotReady: Story = { name: "03 Components / Connected — runtime needs setup", - render: () => component( {}} + render: () => component(), }; export const ConnectedLongName: Story = { name: "03 Components / Connected — long App name", - render: () => component( component( {}} />), + providerAccountLabel: "acme-platform-engineering" }} agent={agent} />), }; export const AppIdentityAndLogo: Story = { name: "03 Components / App identity and logo",