From 383b746db25e085a75b2ab0e052023b455185bcc Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Thu, 20 Aug 2026 22:55:16 -0700 Subject: [PATCH 1/2] Add tests proving the connect-github card must flip itself on submit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The container currently has no wiring that can flip it to connected as a consequence of its own successful PAT submit: it fetches state once on mount, then only folds `subscribeConnectState` updates a host chooses to publish. These tests use a host that resolves the token submit ok but never notifies the subscription, matching real-world hosts that only re-fetch rather than fan out — proving the flip has to come from the container itself, not from trusting the host. Fixes CL-6463 --- .../connect-github-block-container.test.tsx | 164 ++++++++++++++++++ 1 file changed, 164 insertions(+) create mode 100644 packages/chat-ui/test/connect-github-block-container.test.tsx diff --git a/packages/chat-ui/test/connect-github-block-container.test.tsx b/packages/chat-ui/test/connect-github-block-container.test.tsx new file mode 100644 index 000000000..1a75e261c --- /dev/null +++ b/packages/chat-ui/test/connect-github-block-container.test.tsx @@ -0,0 +1,164 @@ +// CL-6463: a successful PAT submit must flip the connect-github card to +// connected on its own — never leaning on a host that happens to fan the +// change out through `subscribeConnectState` (real hosts vary, and +// `chat.settings` never carries the credential-save path at all; see +// `connect-github-stream.ts`'s own header). These fakes deliberately never +// call the subscriber from `submitAccessToken`, so a pass here proves the +// container drove its own state from the submit's own result — not from a +// side channel a differently-wired host might forget. +import { afterEach, describe, expect, test } from "bun:test"; +import { act } from "react"; +import { createRoot } from "react-dom/client"; +import type { Root } from "react-dom/client"; + +import type { ConnectGithubBlockData } from "@corbits/chat/blocks"; + +import type { + ConnectGithubActions, + ConnectGithubQuery, + ConnectGithubRepo, +} from "../src/blocks/connect-github-actions"; +import { ConnectGithubBlockContainer } from "../src/blocks/connect-github-block-container"; + +const DATA: ConnectGithubBlockData = { + requiredForTemplate: "github", + state: "disconnected", +}; + +const REPOS: readonly ConnectGithubRepo[] = [ + { id: "1", name: "acme/widgets", openPullRequestCount: 2 }, +]; + +let container: HTMLDivElement | null = null; +let root: Root | null = null; + +afterEach(() => { + if (root !== null) act(() => root?.unmount()); + container?.remove(); + container = null; + root = null; +}); + +async function mount(actions: ConnectGithubActions) { + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + await act(async () => { + root?.render( + , + ); + }); + return container; +} + +function typeInto(element: HTMLInputElement, text: string) { + const setter = Object.getOwnPropertyDescriptor( + globalThis.HTMLInputElement.prototype, + "value", + )?.set; + setter?.call(element, text); + element.dispatchEvent(new Event("input", { bubbles: true })); +} + +/** A host that never notifies `subscribeConnectState` from + * `submitAccessToken` — the exact shape of the real gap this ticket fixes: + * the credential save succeeds, but nothing about it ever reaches the + * fold. `getConnectState` is the only thing that reports the new + * connected fact, mirroring the real `/github/state` route reading the + * just-written credential. */ +function buildNeverNotifiesHarness(options?: { + readonly submitResult?: + { readonly ok: true } | { readonly ok: false; readonly message: string }; +}) { + let connected = false; + return { + actions: { + getConnectState: () => + Promise.resolve( + connected + ? { + kind: "connected", + orgName: "octocat", + repos: REPOS, + selectedRepoIds: [], + } + : { kind: "disconnected" }, + ), + subscribeConnectState: () => () => {}, + requestConnect: () => {}, + submitAccessToken: async (_token: string) => { + const result = options?.submitResult ?? { ok: true as const }; + if (result.ok) connected = true; + return result; + }, + startReviewing: async () => ({ startedTriggerCount: 0 }), + skip: async () => {}, + } satisfies ConnectGithubActions, + }; +} + +async function openFieldAndSubmit(el: HTMLElement, token: string) { + const connectButton = [...el.querySelectorAll("button")].find( + (button) => button.textContent === "Connect GitHub", + ) as HTMLButtonElement; + await act(async () => { + connectButton.click(); + }); + const tokenField = el.querySelector( + "#connect-github-token", + ) as HTMLInputElement; + await act(async () => { + typeInto(tokenField, token); + }); + const submitButton = [...el.querySelectorAll("button")].find( + (button) => button.textContent === "Connect", + ) as HTMLButtonElement; + await act(async () => { + submitButton.click(); + }); + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); +} + +describe("ConnectGithubBlockContainer post-submit refresh (CL-6463)", () => { + test("a successful PAT submit flips the card to connected on its own, even when the host never fans the change out through subscribeConnectState", async () => { + const harness = buildNeverNotifiesHarness(); + const el = await mount(harness.actions); + + expect(el.textContent).toContain("Connect GitHub"); + await openFieldAndSubmit(el, "ghp_test123"); + + expect(el.textContent).toContain("Connected to GitHub as octocat"); + expect(el.querySelectorAll(".chat-block-connect-repo-row")).toHaveLength( + REPOS.length, + ); + }); + + test("a rejected token shows what went wrong and leaves a working submit button, never a dead card", async () => { + const harness = buildNeverNotifiesHarness({ + submitResult: { ok: false, message: "That token looks expired." }, + }); + const el = await mount(harness.actions); + + await openFieldAndSubmit(el, "ghp_bad"); + + expect(el.textContent).toContain("That token looks expired."); + expect(el.textContent).not.toContain("Connected to GitHub as"); + + const submitButton = [...el.querySelectorAll("button")].find( + (button) => button.textContent === "Connect", + ) as HTMLButtonElement; + expect(submitButton.disabled).toBe(false); + + const tokenField = el.querySelector( + "#connect-github-token", + ) as HTMLInputElement; + expect(tokenField.disabled).toBe(false); + }); +}); From 0e74d3e36c4ebb1b375fc0d088b30c753b72b2a4 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Thu, 20 Aug 2026 22:55:26 -0700 Subject: [PATCH 2/2] Connect-github card: refetch its own state after a successful PAT submit A successful `submitAccessToken` was invisible to the card: the container only ever read state once on mount and otherwise folded whatever `subscribeConnectState` happened to publish, and the room's `chat.settings` event (the only thing the stream fold reacts to) is written by the later repo-review PATCH, not by the credential save. The container now runs its own `getConnectState` refetch as the direct consequence of its own submit succeeding, so the flip to connected never depends on a host also fanning the change out on its own. This is the only refetch outside the mount effect; every other update still rides the existing subscription fold. Fixes CL-6463 --- .../blocks/connect-github-block-container.tsx | 74 +++++++++++++------ 1 file changed, 50 insertions(+), 24 deletions(-) diff --git a/packages/chat-ui/src/blocks/connect-github-block-container.tsx b/packages/chat-ui/src/blocks/connect-github-block-container.tsx index b04c214cc..6a1547538 100644 --- a/packages/chat-ui/src/blocks/connect-github-block-container.tsx +++ b/packages/chat-ui/src/blocks/connect-github-block-container.tsx @@ -1,11 +1,20 @@ // Wires the presentational `ConnectGithubBlockView` to a live // `ConnectGithubActions` port (CL-6345) — mirroring `PollBlockView`'s // own container shape: an initial `getConnectState` read on mount, plus -// a live `subscribeConnectState` fold for every update after, never a -// second fetch once mounted. With no port at all, the card renders the -// same fixed-disabled disconnected framing every other block's "no -// port, no feature" fallback uses. -import { useEffect, useState } from "react"; +// a live `subscribeConnectState` fold for every update after. With no +// port at all, the card renders the same fixed-disabled disconnected +// framing every other block's "no port, no feature" fallback uses. +// +// CL-6463: a card's own successful PAT submit is the one change this +// container never waits on a fold for. `subscribeConnectState` folds +// whatever a host chooses to publish, and the room's `chat.settings` +// event (the only thing `connect-github-stream.ts` can fold) is written +// by the later, unrelated repo-review PATCH — never by the credential +// save itself. So `submitAccessToken` gets its own explicit +// `getConnectState` refetch here, run once as the direct consequence of +// that one submit — not a poll, and not a second source of truth +// alongside the fold; the fold keeps handling every other update. +import { useCallback, useEffect, useRef, useState } from "react"; import type { ConnectGithubBlockData } from "@corbits/chat/blocks"; import type { @@ -24,36 +33,53 @@ export function ConnectGithubBlockContainer({ }) { const [query, setQuery] = useState({ kind: "loading" }); const [selectedRepoIds, setSelectedRepoIds] = useState([]); + const mountedRef = useRef(true); - useEffect(() => { - if (actions === undefined) return; - let cancelled = false; + const applyQuery = useCallback((result: ConnectGithubQuery) => { + if (!mountedRef.current) return; + setQuery(result); + if (result.kind === "connected") setSelectedRepoIds(result.selectedRepoIds); + }, []); - function applyQuery(result: ConnectGithubQuery) { - if (cancelled) return; - setQuery(result); - if (result.kind === "connected") - setSelectedRepoIds(result.selectedRepoIds); - } + useEffect(() => { + mountedRef.current = true; + return () => { + mountedRef.current = false; + }; + }, []); + useEffect(() => { + if (actions === undefined) return; actions.getConnectState(messageId).then(applyQuery); const unsubscribe = actions.subscribeConnectState(messageId, applyQuery); - return () => { - cancelled = true; - unsubscribe(); - }; - }, [actions, messageId]); + return unsubscribe; + }, [actions, messageId, applyQuery]); + + // The one refetch this container ever runs outside its mount effect: + // a submit this card itself just made succeeded, so re-reading the + // card's own state is a direct consequence of that submit — never a + // poll, and it runs whether or not the host's `subscribeConnectState` + // happens to fan the change out on its own. + const submitAccessTokenAndRefresh = useCallback( + async (token: string) => { + if (actions === undefined) { + return { ok: false as const, message: "Not available." }; + } + const result = await actions.submitAccessToken(token); + if (result.ok) { + applyQuery(await actions.getConnectState(messageId)); + } + return result; + }, + [actions, messageId, applyQuery], + ); if (actions === undefined || query.kind !== "connected") { return ( actions?.requestConnect()} - onSubmitAccessToken={(token) => - actions !== undefined - ? actions.submitAccessToken(token) - : Promise.resolve({ ok: false, message: "Not available." }) - } + onSubmitAccessToken={submitAccessTokenAndRefresh} /> ); }