diff --git a/packages/chat-ui/src/streaming-reply.test.ts b/packages/chat-ui/src/streaming-reply.test.ts index 72d661d46..12db641f0 100644 --- a/packages/chat-ui/src/streaming-reply.test.ts +++ b/packages/chat-ui/src/streaming-reply.test.ts @@ -173,6 +173,42 @@ describe("nextStreamingReplyState (CL-6115: token deltas fold into a growing rep }); }); +describe("nextStreamingReplyState (CL-6376: the typing pulse clears on a dispatch failure too)", () => { + test("a chat.message carrying a turnFailed part clears a pending reply", () => { + const state = { text: "" }; + expect( + nextStreamingReplyState(state, { + eventType: "chat.message", + data: { + id: "msg_1", + parts: [ + { kind: "text", text: "I didn't get that one", turnFailed: true }, + ], + }, + }), + ).toBeNull(); + }); + + test("an ordinary chat.message (no turnFailed part) leaves the reply untouched", () => { + const state = { text: "" }; + expect( + nextStreamingReplyState(state, { + eventType: "chat.message", + data: { id: "msg_1", parts: [{ kind: "text", text: "hi" }] }, + }), + ).toBe(state); + }); + + test("a chat.message with no pending reply stays null", () => { + expect( + nextStreamingReplyState(null, { + eventType: "chat.message", + data: { parts: [{ kind: "text", text: "x", turnFailed: true }] }, + }), + ).toBeNull(); + }); +}); + describe("openPendingReply", () => { test("opens an empty pending reply when idle", () => { expect(openPendingReply(null)).toEqual({ text: "" }); diff --git a/packages/chat-ui/src/streaming-reply.ts b/packages/chat-ui/src/streaming-reply.ts index bf96e2d85..913f5b195 100644 --- a/packages/chat-ui/src/streaming-reply.ts +++ b/packages/chat-ui/src/streaming-reply.ts @@ -50,6 +50,25 @@ function innerEventType(data: unknown): string | null { return typeof type === "string" ? type : null; } +/** Whether a `chat.message` payload carries `postUndeliveredNotice`'s + * `turnFailed` part (see `packages/chat/src/workbench-service.ts`). This + * notice posts straight to the room with no `chat.agent` events at all — + * the dispatch failed before `sendMail` ever reached the agent — so + * without this check a turn that fails this way never emits the + * `reactor.error`/`inference.error` this module otherwise relies on to + * clear the typing pulse, leaving it stranded until the 120s backstop. */ +function hasTurnFailedPart(data: unknown): boolean { + if (typeof data !== "object" || data === null) return false; + const parts = (data as Record).parts; + if (!Array.isArray(parts)) return false; + return parts.some( + (part) => + typeof part === "object" && + part !== null && + (part as Record).turnFailed === true, + ); +} + /** * The streaming reply's whole state machine, pure: an `inference.start` * opens an empty in-progress reply if nothing is showing yet (it never @@ -58,13 +77,18 @@ function innerEventType(data: unknown): string | null { * clear it — the turn is over. `inference.done` only clears once tokens * have streamed (the persisted message takes over); an empty pending * survives so the typing pulse stays up across tool rounds. `inference.error` - * always clears. Every other event type (tool calls, thinking, usage) - * leaves the current state untouched. + * always clears. A `chat.message` carrying `postUndeliveredNotice`'s + * `turnFailed` part also clears it (see `hasTurnFailedPart`) — the one + * failure path with no `chat.agent` events of its own. Every other event + * type (tool calls, thinking, usage) leaves the current state untouched. */ export function nextStreamingReplyState( current: StreamingReplyState, event: { readonly eventType: string; readonly data: unknown }, ): StreamingReplyState { + if (event.eventType === "chat.message") { + return hasTurnFailedPart(event.data) ? null : current; + } if (event.eventType !== "chat.agent") return current; const innerType = innerEventType(event.data); diff --git a/packages/chat-ui/src/styles.css b/packages/chat-ui/src/styles.css index 7fef69d3a..e2eab0d86 100644 --- a/packages/chat-ui/src/styles.css +++ b/packages/chat-ui/src/styles.css @@ -2772,8 +2772,14 @@ "Reply in thread" link and the standalone add-reaction trigger. Floats over the message's top-right corner, fading in on hover/focus-within like `.chat-pin-toggle` above, plus `data-open` so it stays put while its - own picker or menu is open even if focus moves into a portaled menu. */ -.chat-message-group { + own picker or menu is open even if focus moves into a portaled menu. + Positioned relative to `.chat-message-row` (CL-6376) — the wrapper + around this one message's own content, siblings-with but excluding its + optional `.chat-day-divider` — rather than `.chat-message-group` as a + whole: anchoring to the outer group let the toolbar float up into the + day divider's own space on the first message of a new day, detached + from the row a reader was actually hovering. */ +.chat-message-row { position: relative; } @@ -3622,3 +3628,49 @@ .chat-pr-failed-what-happened:hover { color: var(--foreground); } + +/* The general chat timeline's own failed-turn row (CL-6376) — a quiet + inline system line under the same left gutter every message sits + under, not a bordered banner: muted danger-tinted text, a small ghost + Retry, and "What happened" as an inline disclosure. Reads as part of + the conversation rather than an alert dropped into it. */ +.chat-turn-failed { + display: flex; + align-items: center; + flex-wrap: wrap; + gap: 0.5rem; + margin: 0.15rem 0 0.35rem 2.9rem; + max-width: 100%; + font-size: 0.8125rem; +} + +.chat-turn-failed-text { + color: color-mix(in srgb, var(--destructive) 78%, var(--muted-foreground)); +} + +.chat-turn-failed-retry { + height: auto; + padding: 0.05rem 0.45rem; + font-size: 0.75rem; +} + +.chat-turn-failed-disclosure { + border: 0; + background: transparent; + padding: 0; + font-size: 0.75rem; + color: var(--muted-foreground); + text-decoration: underline; + text-underline-offset: 2px; + cursor: pointer; +} + +.chat-turn-failed-disclosure:hover { + color: var(--foreground); +} + +.chat-turn-failed-detail { + flex-basis: 100%; + font-size: 0.75rem; + color: var(--muted-foreground); +} diff --git a/packages/chat-ui/src/timeline.tsx b/packages/chat-ui/src/timeline.tsx index 6390a2443..1f71d6024 100644 --- a/packages/chat-ui/src/timeline.tsx +++ b/packages/chat-ui/src/timeline.tsx @@ -56,7 +56,6 @@ import { BlockPartView } from "./blocks/registry"; import { isClassifiedInferenceFailureText } from "./inference-failure"; import { WorkbenchLoadingState } from "./loading-state"; import { Markdown } from "./markdown"; -import { PrFailedTurnStrip } from "./pr-thread-view"; import type { ProfileSubject } from "./profile-subject"; import { profileSubjectFromParticipant } from "./profile-subject"; import { CHAT_STRINGS } from "./strings"; @@ -625,14 +624,18 @@ function EventLine({ } /** - * The general chat timeline's failed-turn treatment (CL-6332): a text - * part `postUndeliveredNotice` posted in its unreachable agent's own - * voice (`part.turnFailed`), rendered through the same - * `PrFailedTurnStrip` the PR-review mock uses — the "visible treatment - * matches the mock" is this literal component, just fed chat-specific - * copy (`titleText`/`subText`) instead of the PR thread's repo-scoped - * one. `onRetry`/`onWhatHappened` are the host's own actions; a host - * that wires neither still gets the strip, just with inert buttons — + * The general chat timeline's failed-turn treatment (CL-6332, redesigned + * CL-6376 to match the timeline's own idiom rather than borrow + * `PrFailedTurnStrip`'s bordered banner — that component stays as-is for + * the PR-review surface it was built for, but reusing it here read as a + * floating alert dropped mid-conversation). A text part + * `postUndeliveredNotice` posted in its unreachable agent's own voice + * (`part.turnFailed`) now renders as a quiet inline system row, aligned + * under the same left gutter every message bubble sits under: muted + * danger-tinted copy, a small ghost Retry button, and "What happened" as + * a subtle inline disclosure rather than a second button competing for + * attention. `onRetry`/`onWhatHappened` are the host's own actions; a + * host that wires neither still gets the row, just with inert controls — * matching the fixed-disabled framing every other undefined-action port * in this file already falls back to. */ @@ -651,18 +654,38 @@ function FailedTurnStrip({ }) { const display = senderDisplay(item.sender, participants, currentUser); const sender = display?.label ?? CHAT_STRINGS.senderFallbackMember; + const [expanded, setExpanded] = useState(false); return ( - onRetryFailedTurn?.(item), - onWhatHappened: () => onWhatHappenedFailedTurn?.(item), - }} - /> +
+ + {CHAT_STRINGS.turnFailedTitle(sender)} + + + + {expanded ? ( + + {CHAT_STRINGS.turnFailedSub} + + ) : null} +
); } @@ -1201,139 +1224,155 @@ function MessageParts({ onContextMenu={handleContextMenu} > {showDayDivider && } - {item.parts.map((part, index) => { - const key = `${groupKey}-${index}`; - if (part.kind === "text" && part.turnFailed === true) { - return ( - - ); - } - if (part.kind === "text") { - return ( - - ); - } - if (part.kind === "event") { - return ( - - ); - } - if (part.kind === "file") { - return ( - - ); - } - // The agent's own thinking and its tool calls (CL-6318). Both - // render through react-ui, which already owns this presentation — - // a reasoning disclosure and the tool-call lifecycle — so the - // workbench carries no second version of either. - if (part.kind === "reasoning") { - return ; - } - if (part.kind === "tool-trace") { - const trace = toReactUiToolTrace(part, key); - return ( - - ); - } - if (part.kind === "block") { +
+ {item.parts.map((part, index) => { + const key = `${groupKey}-${index}`; + if (part.kind === "text" && part.turnFailed === true) { + return ( + + ); + } + if (part.kind === "text") { + return ( + + ); + } + if (part.kind === "event") { + return ( + + ); + } + if (part.kind === "file") { + return ( + + ); + } + // The agent's own thinking and its tool calls (CL-6318). Both + // render through react-ui, which already owns this presentation — + // a reasoning disclosure and the tool-call lifecycle — so the + // workbench carries no second version of either. + if (part.kind === "reasoning") { + return ( + + ); + } + if (part.kind === "tool-trace") { + const trace = toReactUiToolTrace(part, key); + return ( + + ); + } + if (part.kind === "block") { + return ( + + ); + } + return ; + })} + {(() => { + const hasReactions = + reactionActions !== undefined && (item.reactions?.length ?? 0) > 0; + // Unpinned messages offer no persistent glyph here — pinning + // itself stays reachable through the ellipsis menu's own + // "Pin"/"Unpin" entry (`buildMessageMenu`); this row only shows + // once there's something to show (a reaction, or a message + // already pinned, which needs a visible way to unpin). Before + // this, a pin toggle mounted for every message the moment a host + // wired `pinActions` at all, CSS-hidden until hover but present + // in the DOM under every line, greeting included. + const isPinned = pinActions !== undefined && item.pinned === true; + if (isPending || (!hasReactions && !isPinned)) return null; return ( - +
+ {hasReactions && reactionActions !== undefined ? ( + + ) : null} + {isPinned && pinActions !== undefined ? ( + + ) : null} +
); - } - return ; - })} - {!isPending && - ((reactionActions !== undefined && (item.reactions?.length ?? 0) > 0) || - pinActions !== undefined) ? ( -
- {reactionActions !== undefined ? ( - - ) : null} - {pinActions !== undefined ? ( - - ) : null} -
- ) : null} - {!isPending && onOpenThread !== undefined && replyCount > 0 ? ( - onOpenThread(item.id)} - /> - ) : null} - {!isPending ? ( - contextMenu.show(x, y, menu, origin)} - threadAffordanceMode={threadAffordanceMode} - {...(onOpenThread !== undefined ? { onOpenThread } : {})} - {...(reactionActions !== undefined ? { reactionActions } : {})} - /> - ) : null} + })()} + {!isPending && onOpenThread !== undefined && replyCount > 0 ? ( + onOpenThread(item.id)} + /> + ) : null} + {!isPending ? ( + contextMenu.show(x, y, menu, origin)} + threadAffordanceMode={threadAffordanceMode} + {...(onOpenThread !== undefined ? { onOpenThread } : {})} + {...(reactionActions !== undefined ? { reactionActions } : {})} + /> + ) : null} +
( @@ -62,27 +61,14 @@ async function request( return parsed; } -export async function testConnectorCredential( - tenantId: string, - connectorId: string, - apiKey: string, -): Promise<{ ok: true } | { ok: false; message: string }> { - try { - await request( - `/api/tenants/${tenantId}/connections/${connectorId}/credential/test`, - TestResult, - "testing that connection", - { method: "POST", body: JSON.stringify({ apiKey }) }, - ); - return { ok: true }; - } catch (cause) { - if (cause instanceof PluginsApiError && cause.status === 422) { - return { ok: false, message: cause.message }; - } - throw cause; - } -} - +/** + * The one connect action (CL-6377): the server proves the pasted key + * against the connector's own probe and only stores it once that probe + * accepts — there is no separate client-driven "test" round-trip before + * this call. A rejected probe 422s with the probe's own message, which + * throws `PluginsApiError` (status 422); the caller renders that inline + * as the normal connect-failed state. + */ export function completeConnectorCredential( tenantId: string, connectorId: string, diff --git a/packages/chat-ui/src/workbench-settings/plugins-section.tsx b/packages/chat-ui/src/workbench-settings/plugins-section.tsx index ee2873877..c94781197 100644 --- a/packages/chat-ui/src/workbench-settings/plugins-section.tsx +++ b/packages/chat-ui/src/workbench-settings/plugins-section.tsx @@ -62,7 +62,6 @@ import { completeConnectorCredential, PluginsApiError, removeWorkbenchCredential, - testConnectorCredential, } from "./plugins-api"; import { connectMcpPreset, @@ -500,26 +499,16 @@ function ConnectDialog({ setError(null); }, [plugin]); + // One connect action (CL-6377): the server proves the key before ever + // storing it, so this is the only round-trip — no separate test step. function handleSubmit() { if (plugin === null || apiKey.trim() === "") return; setSubmitting(true); setError(null); - testConnectorCredential(tenantId, plugin.descriptor.id, apiKey) - .then((result) => { - if (!result.ok) { - setError(result.message); - return; - } - return completeConnectorCredential( - tenantId, - plugin.descriptor.id, - apiKey, - ).then(() => { - toast( - `Connected ${plugin.descriptor.displayName} for this workbench.`, - ); - onConnected(); - }); + completeConnectorCredential(tenantId, plugin.descriptor.id, apiKey) + .then(() => { + toast(`Connected ${plugin.descriptor.displayName} for this workbench.`); + onConnected(); }) .catch((cause: unknown) => setError(errorMessage(cause, "Couldn't save that key.")), @@ -575,7 +564,7 @@ function ConnectDialog({ disabled={apiKey.trim() === "" || submitting} onClick={handleSubmit} > - {submitting ? "Saving…" : "Test & Save"} + {submitting ? "Connecting…" : "Connect"} diff --git a/packages/chat-ui/test/failed-turn-strip.test.tsx b/packages/chat-ui/test/failed-turn-strip.test.tsx index 6dcc2a6c3..be8da2435 100644 --- a/packages/chat-ui/test/failed-turn-strip.test.tsx +++ b/packages/chat-ui/test/failed-turn-strip.test.tsx @@ -1,8 +1,9 @@ -// DOM tests for CL-6332's failed-turn strip: the server's undelivered-turn -// notice (`postUndeliveredNotice`, `@corbits/chat`'s `workbench-service.ts`) -// marks its text part `turnFailed: true`; the general chat timeline renders -// that part through `PrFailedTurnStrip` (PR #71) instead of an ordinary text -// bubble, matching the PR-review mock's visible treatment. +// DOM tests for CL-6332/CL-6376's failed-turn strip: the server's +// undelivered-turn notice (`postUndeliveredNotice`, `@corbits/chat`'s +// `workbench-service.ts`) marks its text part `turnFailed: true`; the +// general chat timeline renders that part as its own quiet inline system +// row (`.chat-turn-failed`, CL-6376) instead of an ordinary text bubble — +// or, before the CL-6376 redesign, `PrFailedTurnStrip`'s bordered banner. import { afterEach, describe, expect, test } from "bun:test"; import { act } from "react"; import { createRoot } from "react-dom/client"; @@ -60,12 +61,15 @@ describe("the failed-turn notice renders through PrFailedTurnStrip", () => { ); }); - const strip = container.querySelector(".chat-pr-failed"); + const strip = container.querySelector(".chat-turn-failed"); expect(strip).not.toBeNull(); expect(strip?.getAttribute("role")).toBe("status"); expect(strip?.textContent).toContain("Echo"); expect(strip?.textContent).toContain("didn't reply"); + // Never the old bordered-banner treatment. + expect(container.querySelector(".chat-pr-failed")).toBeNull(); + // The notice never renders as an ordinary bubble alongside the strip. const bubbles = container.querySelectorAll(".chat-bubble"); expect( @@ -94,7 +98,7 @@ describe("the failed-turn notice renders through PrFailedTurnStrip", () => { ); }); - const buttons = container.querySelectorAll(".chat-pr-failed button"); + const buttons = container.querySelectorAll(".chat-turn-failed button"); expect(buttons).toHaveLength(2); act(() => { (buttons[0] as HTMLButtonElement).click(); @@ -126,7 +130,7 @@ describe("the failed-turn notice renders through PrFailedTurnStrip", () => { ); }); - expect(container.querySelector(".chat-pr-failed")).toBeNull(); + expect(container.querySelector(".chat-turn-failed")).toBeNull(); expect(container.querySelector(".chat-bubble")).not.toBeNull(); }); }); diff --git a/packages/chat-ui/test/reactions-and-pins.test.tsx b/packages/chat-ui/test/reactions-and-pins.test.tsx index 251649136..77fa7608f 100644 --- a/packages/chat-ui/test/reactions-and-pins.test.tsx +++ b/packages/chat-ui/test/reactions-and-pins.test.tsx @@ -111,29 +111,61 @@ describe("reaction chip row", () => { }); }); +const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + +/** Radix's exit-animation handling runs through a couple of microtasks even + * with no real CSS animation configured — mirrors the identical helper in + * `test/message-hover-toolbar.test.tsx`. */ +async function flush(): Promise { + await act(async () => { + await sleep(0); + await sleep(0); + }); +} + describe("pin toggle", () => { + // CL-6376: an unpinned message renders no persistent glyph at all — the + // pin toggle only mounts once a message is actually pinned (needing a + // visible way to unpin it). Pinning itself stays reachable through the + // ellipsis/context menu's own "Pin message" entry either way. test("with no pinActions, no pin button renders", async () => { const el = await mount(messageWithReactions()); expect(el.querySelector(".chat-pin-toggle")).toBeNull(); }); - test("an unpinned message's pin button calls onPin", async () => { + test("an unpinned message renders no pin toggle, even with pinActions wired", async () => { + const el = await mount(messageWithReactions(), undefined, { + onPin: () => undefined, + onUnpin: () => undefined, + }); + expect(el.querySelector(".chat-pin-toggle")).toBeNull(); + }); + + test("an unpinned message can still be pinned through the context menu's onPin", async () => { const pinned: string[] = []; - const unpinned: string[] = []; const el = await mount(messageWithReactions(), undefined, { onPin: (id) => pinned.push(id), - onUnpin: (id) => unpinned.push(id), + onUnpin: () => undefined, }); - const button = el.querySelector(".chat-pin-toggle") as HTMLButtonElement; - expect(button.dataset["pinned"]).toBe("false"); - await act(async () => button.click()); + const group = el.querySelector(".chat-message-group") as HTMLElement; + await act(async () => { + group.dispatchEvent( + new MouseEvent("contextmenu", { bubbles: true, cancelable: true }), + ); + }); + await flush(); + + const pinItem = Array.from( + document.querySelectorAll('[data-slot="menu-item"]'), + ).find((item) => item.textContent === "Pin message"); + expect(pinItem).not.toBeUndefined(); + await act(async () => pinItem?.click()); expect(pinned).toEqual(["m1"]); - expect(unpinned).toEqual([]); }); - test("a pinned message's pin button calls onUnpin instead", async () => { + test("a pinned message's pin button calls onUnpin", async () => { const pinned: string[] = []; const unpinned: string[] = []; const items: MessageItem[] = [ diff --git a/packages/chat/src/workbench-service.ts b/packages/chat/src/workbench-service.ts index bd7efcf9e..24b135a43 100644 --- a/packages/chat/src/workbench-service.ts +++ b/packages/chat/src/workbench-service.ts @@ -356,9 +356,9 @@ const GREETING_VARIATIONS: readonly ((who: string, agent: string) => string)[] = "research, lining up automations: all fair game. What should we " + "dig into first?", (who, agent) => - `Welcome in${who}. I'm ${agent}; think of me as the teammate who ` + - "writes, plans, and pulls in the right specialists when a job " + - "calls for them. What's on your plate?", + `Welcome in${who === "" ? "" : `,${who}`}. I'm ${agent}; think of me ` + + "as the teammate who writes, plans, and pulls in the right " + + "specialists when a job calls for them. What's on your plate?", (who, agent) => `Hey${who} — ${agent} here. This space is ours to work in: I can ` + "draft, plan, and wire things up as we go. What are you working on?", diff --git a/packages/connections/README.md b/packages/connections/README.md index 6a9ca6611..ba713199f 100644 --- a/packages/connections/README.md +++ b/packages/connections/README.md @@ -27,9 +27,10 @@ resolve a connector's credential across a tenant chain. without pulling in `hono` or `@intx/inference`. - `descriptor.ts` — the `ConnectorDescriptor`/`ConnectorOAuthConfig` shape every registry entry implements. -- `routes.ts` — tenant-scoped `POST /:connectorId/credential/test` and - `/complete`: proves an api-key connector's credential before storing it, - mounted inside the platform's native tenant middleware. +- `routes.ts` — tenant-scoped `POST /:connectorId/complete`: proves an + api-key connector's credential against its own probe and only stores it + once that probe accepts (one action, not a separate test step), mounted + inside the platform's native tenant middleware. - `oauth-routes.ts` — the generalized `GET /:connectorId/start` / `/callback` factory driving every `oauth-pkce`/`oauth-code` connector (OpenRouter, Hugging Face) from one `ConnectorDescriptor.oauth` config. diff --git a/packages/connections/src/routes.test.ts b/packages/connections/src/routes.test.ts index 440dfc02b..5faab9fba 100644 --- a/packages/connections/src/routes.test.ts +++ b/packages/connections/src/routes.test.ts @@ -190,69 +190,6 @@ describe("GET /oauth-configured", () => { }); }); -describe("POST /:connectorId/credential/test", () => { - test("unknown connector 404s", async () => { - const app = buildApp(); - const response = await app.request("/not-a-connector/credential/test", { - method: "POST", - headers: { "content-type": "application/json" }, - body: JSON.stringify({ apiKey: "test-key" }), - }); - expect(response.status).toBe(404); - const body = (await response.json()) as { error: { code: string } }; - expect(body.error.code).toBe("not_found"); - }); - - test("a display-only connector (no probe) 404s", async () => { - const app = buildApp(); - const response = await app.request( - "/display-only-connector/credential/test", - { - method: "POST", - headers: { "content-type": "application/json" }, - body: JSON.stringify({ apiKey: "test-key" }), - }, - ); - expect(response.status).toBe(404); - }); - - test("malformed body 400s", async () => { - const app = buildApp(); - const response = await app.request("/accepting-connector/credential/test", { - method: "POST", - headers: { "content-type": "application/json" }, - body: JSON.stringify({}), - }); - expect(response.status).toBe(400); - const body = (await response.json()) as { error: { code: string } }; - expect(body.error.code).toBe("bad_request"); - }); - - test("a rejected probe 422s with no storage", async () => { - const app = buildApp(); - const response = await app.request("/rejecting-connector/credential/test", { - method: "POST", - headers: { "content-type": "application/json" }, - body: JSON.stringify({ apiKey: "bad-key" }), - }); - expect(response.status).toBe(422); - const body = (await response.json()) as { error: { code: string } }; - expect(body.error.code).toBe("invalid_credential"); - }); - - test("an accepted probe 200s with no storage", async () => { - const app = buildApp(); - const response = await app.request("/accepting-connector/credential/test", { - method: "POST", - headers: { "content-type": "application/json" }, - body: JSON.stringify({ apiKey: "good-key" }), - }); - expect(response.status).toBe(200); - const body = (await response.json()) as { ok: boolean }; - expect(body.ok).toBe(true); - }); -}); - describe("POST /:connectorId/complete", () => { test("unknown connector 404s", async () => { const app = buildApp(); @@ -627,39 +564,6 @@ describe("POST /:connectorId/complete", () => { }); }); -describe("POST /:connectorId/credential/test provider health wiring", () => { - test("a rejected probe reports the connector needs_attention", async () => { - const providerHealth = createProviderHealthStore(); - const app = buildApp({ providerHealth }); - await app.request("/rejecting-connector/credential/test", { - method: "POST", - headers: { "content-type": "application/json" }, - body: JSON.stringify({ apiKey: "bad-key" }), - }); - const record = providerHealth.get(TENANT.id, "rejecting-connector"); - expect(record?.status).toBe("needs_attention"); - expect(record?.category).toBe("credential_failure"); - }); - - test("a passing probe clears any needs_attention record for that connector", async () => { - const providerHealth = createProviderHealthStore(); - providerHealth.report( - TENANT.id, - "accepting-connector", - "credential_failure", - ); - const app = buildApp({ providerHealth }); - await app.request("/accepting-connector/credential/test", { - method: "POST", - headers: { "content-type": "application/json" }, - body: JSON.stringify({ apiKey: "good-key" }), - }); - expect( - providerHealth.get(TENANT.id, "accepting-connector"), - ).toBeUndefined(); - }); -}); - describe("GET /provider-health", () => { test("reports every provider this tenant has marked needs_attention", async () => { const providerHealth = createProviderHealthStore( diff --git a/packages/connections/src/routes.ts b/packages/connections/src/routes.ts index 5373fa4e3..a745710bf 100644 --- a/packages/connections/src/routes.ts +++ b/packages/connections/src/routes.ts @@ -1,14 +1,13 @@ -// Tenant-scoped credential test-and-store for the Connections surface: -// `POST /:connectorId/credential/test` proves a pasted api-key against -// the connector's own probe with no storage, `POST /:connectorId/complete` -// re-proves it (never trusting a client-side "already tested" claim, -// mirroring `@workbench/onboarding`'s `completeCredentialSetup`) and, on -// success, plants the credential through the same `ensureProvider` / -// `ensureCredential` seam `seedCatalog` uses — never reimplementing -// credential storage. Mounted inside the platform's native tenant -// middleware (`TenantEnv`'s `tenant`/`principal` resolved before any -// handler here runs), the same way `@corbits/webhook-triggers`' -// management routes are. +// Tenant-scoped credential connect-and-store for the Connections surface: +// `POST /:connectorId/complete` proves a pasted api-key against the +// connector's own probe (CL-6377: one action, not a separate test step +// the client calls first) and, on success, plants the credential through +// the same `ensureProvider` / `ensureCredential` seam `seedCatalog` uses — +// never reimplementing credential storage. A rejected probe 422s with no +// storage, mirroring `@workbench/onboarding`'s `completeCredentialSetup`. +// Mounted inside the platform's native tenant middleware (`TenantEnv`'s +// `tenant`/`principal` resolved before any handler here runs), the same +// way `@corbits/webhook-triggers`' management routes are. // // Only api-key connectors are servable here: an unknown connector id, or // a registry entry with no `probe` (oauth-pkce/oauth-code/webhook-secret, @@ -303,48 +302,6 @@ export function createConnectionRoutes( return SubmitCredential(body); } - app.post( - "/:connectorId/credential/test", - deps.requireGrant("credential:*", "create"), - async (c) => { - const connectorId = c.req.param("connectorId"); - const descriptor = findApiKeyDescriptor(connectorId); - if (descriptor === undefined || descriptor.probe === undefined) { - return c.json( - ErrorEnvelope("not_found", `Unknown connector: ${connectorId}`), - 404, - ); - } - - const parsed = await parseApiKeyBody(c); - if (parsed instanceof type.errors) { - return c.json( - ErrorEnvelope( - "bad_request", - `An API key is required: ${parsed.summary}`, - ), - 400, - ); - } - - const tenant = c.get("tenant"); - const result = await descriptor.probe(parsed.apiKey); - if (!result.ok) { - deps.providerHealth?.report( - tenant.id, - descriptor.id, - CREDENTIAL_TEST_FAILURE_CATEGORY, - ); - return c.json(ErrorEnvelope("invalid_credential", result.message), 422); - } - // A passing test here is a genuine, if lighter-weight, proof the - // credential works — the same signal `/complete`'s own passing test - // clears on (CL-6092): never a reply's prose, always a real probe. - deps.providerHealth?.clear(tenant.id, descriptor.id); - return c.json({ ok: true }, 200); - }, - ); - // The PROVIDER row is named by the connector's lowercase `id` — the // canonical name `credentialBindings` resolve against via the // platform's case-sensitive `resolveProviderByName` (and the same diff --git a/packages/plugins-ui/README.md b/packages/plugins-ui/README.md index f6d716bab..4e442972b 100644 --- a/packages/plugins-ui/README.md +++ b/packages/plugins-ui/README.md @@ -12,8 +12,8 @@ owns fetching data and passes it down. tenant-inheritance-aware resolver — rather than re-deriving connection status itself. - `PluginConnectPanel` reuses the exact mutations `@corbits/settings-ui`'s - own Connections section calls (`testConnectorCredential`, - `completeConnectorCredential`, `deleteCredential`, `oauthStartHref`), + own Connections section calls (`completeConnectorCredential`, + `deleteCredential`, `oauthStartHref`), and mounts `@corbits/settings-ui`'s `GranolaWebhookCard` wholesale for Granola's key-plus-webhook connect rather than forking its dialog. - Built on `@corbits/react-ui` primitives (`Card`, `Badge`, `Dialog`, diff --git a/packages/plugins-ui/src/plugin-connect-panel.tsx b/packages/plugins-ui/src/plugin-connect-panel.tsx index 335e527ce..31f6f9eb1 100644 --- a/packages/plugins-ui/src/plugin-connect-panel.tsx +++ b/packages/plugins-ui/src/plugin-connect-panel.tsx @@ -2,8 +2,8 @@ // Granola's key-plus-webhook combination — connects from this one // right-docked panel, never a "create a routine first, then come back" // detour. It reuses the exact mutations `@corbits/settings-ui`'s own -// Connections section already calls (`testConnectorCredential`, -// `completeConnectorCredential`, `deleteCredential`, `oauthStartHref`) and, +// Connections section already calls (`completeConnectorCredential`, +// `deleteCredential`, `oauthStartHref`) and, // for Granola's webhook half, mounts `GranolaWebhookCard` wholesale rather // than forking its dialog — see that component's own header comment for // why a routine picker is deliberately not offered when zero `granola-call` @@ -30,7 +30,6 @@ import { completeConnectorCredential, deleteCredential, oauthStartHref, - testConnectorCredential, } from "@corbits/settings-ui"; import { CONNECTOR_REGISTRY } from "@workbench/connections/registry"; import type { ResolvedPlugin } from "@workbench/connections/plugins"; @@ -67,22 +66,16 @@ function ApiKeyConnectForm({ const [submitting, setSubmitting] = useState(false); const [error, setError] = useState(null); + // One connect action (CL-6377): the server proves the key before ever + // storing it, so this is the only round-trip — no separate test step. function handleSubmit() { setSubmitting(true); setError(null); - testConnectorCredential(tenantId, connectorId, value) - .then((result) => { - if (!result.ok) { - setError(result.message); - return; - } - return completeConnectorCredential(tenantId, connectorId, value).then( - () => { - toast(`${displayName} connected.`); - setValue(isUrl ? (fieldPlaceholder ?? "") : ""); - onConnected(); - }, - ); + completeConnectorCredential(tenantId, connectorId, value) + .then(() => { + toast(`${displayName} connected.`); + setValue(isUrl ? (fieldPlaceholder ?? "") : ""); + onConnected(); }) .catch((cause: unknown) => setError(cause instanceof Error ? cause.message : String(cause)), @@ -116,7 +109,7 @@ function ApiKeyConnectForm({ disabled={value.trim() === "" || submitting} onClick={handleSubmit} > - {submitting ? "Testing & connecting…" : "Test & connect"} + {submitting ? "Connecting…" : "Connect"} ); diff --git a/packages/plugins-ui/test/plugin-connect-panel.test.tsx b/packages/plugins-ui/test/plugin-connect-panel.test.tsx index 103c543fe..d43459e25 100644 --- a/packages/plugins-ui/test/plugin-connect-panel.test.tsx +++ b/packages/plugins-ui/test/plugin-connect-panel.test.tsx @@ -1,7 +1,8 @@ // The connect panel opens the right surface per connector kind: an OAuth -// link for `oauth-pkce`/`oauth-code`, a test-and-connect form for -// `api-key`, and — for Granola specifically — both the api-key form and -// `GranolaWebhookCard` stacked in the same panel, never a second dialog. +// link for `oauth-pkce`/`oauth-code`, a single-action connect form for +// `api-key` (CL-6377: no separate test step), and — for Granola +// specifically — both the api-key form and `GranolaWebhookCard` stacked +// in the same panel, never a second dialog. import { afterEach, describe, expect, test } from "bun:test"; import { act } from "react"; @@ -83,11 +84,13 @@ describe("PluginConnectPanel", () => { expect(container.querySelector('input[type="password"]')).toBeNull(); }); - test("an api-key connector shows the test-and-connect form", () => { + // CL-6377: one Connect action — no separate test step or "Test" copy. + test("an api-key connector shows the connect form", () => { const container = render(notConnected(descriptor("exa", "Exa", "api-key"))); expect(container.querySelector('input[type="password"]')).not.toBeNull(); - expect(container.textContent).toContain("Test & connect"); + expect(container.textContent).toContain("Connect"); + expect(container.textContent).not.toContain("Test"); expect(container.querySelector("a")).toBeNull(); }); diff --git a/packages/settings-ui/src/connections-api.ts b/packages/settings-ui/src/connections-api.ts index ed9f98ea2..f51e57b53 100644 --- a/packages/settings-ui/src/connections-api.ts +++ b/packages/settings-ui/src/connections-api.ts @@ -16,8 +16,6 @@ export class ConnectionsApiError extends Error { } } -const TestResult = type({ ok: "true" }); - const CompleteResult = type({ credentialId: "string", status: "'active'", @@ -34,34 +32,6 @@ function request( return apiRequest(path, schema, verb, ConnectionsApiError, init); } -/** - * Tests an api-key connector's credential without storing it. A 422 means - * the probe rejected the key — an expected, non-exceptional outcome the - * caller renders inline, not a transport failure — so it resolves - * `{ ok: false, message }` instead of throwing. Every other non-2xx status - * still throws `ConnectionsApiError`. - */ -export async function testConnectorCredential( - tenantId: string, - connectorId: string, - apiKey: string, -): Promise<{ ok: true } | { ok: false; message: string }> { - try { - await request( - `/api/tenants/${tenantId}/connections/${connectorId}/credential/test`, - TestResult, - "testing that connection", - { method: "POST", body: JSON.stringify({ apiKey }) }, - ); - return { ok: true }; - } catch (cause) { - if (cause instanceof ConnectionsApiError && cause.status === 422) { - return { ok: false, message: cause.message }; - } - throw cause; - } -} - /** * Which oauth-pkce/oauth-code connectors have a registered OAuth app * (a client id) configured, keyed by connector id — read ahead of @@ -79,6 +49,14 @@ export function fetchOAuthConfigured( ); } +/** + * The one connect action (CL-6377): the server proves the pasted key + * against the connector's own probe and only stores it once that probe + * accepts — there is no separate client-driven "test" round-trip before + * this call. A rejected probe 422s with the probe's own message, which + * throws `ConnectionsApiError` (status 422); the caller renders that + * inline as the normal connect-failed state. + */ export function completeConnectorCredential( tenantId: string, connectorId: string, diff --git a/packages/settings-ui/src/connections-section.tsx b/packages/settings-ui/src/connections-section.tsx index 6f363e949..026449646 100644 --- a/packages/settings-ui/src/connections-section.tsx +++ b/packages/settings-ui/src/connections-section.tsx @@ -49,10 +49,10 @@ import { describeQueryError, } from "@corbits/api-query"; import { + ConnectionsApiError, completeConnectorCredential, disconnectConnector, fetchOAuthConfigured, - testConnectorCredential, } from "./connections-api"; import { CONNECTOR_PINNED_WORKFLOWS } from "./connections-pinned-by"; import { @@ -829,31 +829,28 @@ export function ConnectorCredentialDialog({ const open = descriptor !== null; const canSubmit = apiKey.trim() !== "" && !submitting; - // One primary action, not test-then-save: it proves the key with a real - // call before ever storing it, so a rejected key never reaches - // `completeConnectorCredential` and nothing gets sealed on a bad key. + // One action, not test-then-save (CL-6377): the server proves the key + // with a real call before ever storing it, so a rejected key never gets + // sealed — this call is the only round-trip, and its 422 rejection + // renders inline the same as any other connect failure. function handleSubmit() { if (descriptor === null) return; setSubmitting(true); setSubmitError(null); - testConnectorCredential(tenantId, descriptor.id, apiKey) - .then((result) => { - if (!result.ok) { - setSubmitError(result.message); - return; - } - return completeConnectorCredential( - tenantId, - descriptor.id, - apiKey, - ).then(() => { - toast( - SETTINGS_STRINGS.connectionsConnectedToast(descriptor.displayName), - ); - onConnected(); - }); + completeConnectorCredential(tenantId, descriptor.id, apiKey) + .then(() => { + toast( + SETTINGS_STRINGS.connectionsConnectedToast(descriptor.displayName), + ); + onConnected(); + }) + .catch((cause: unknown) => { + setSubmitError( + cause instanceof ConnectionsApiError && cause.status === 422 + ? cause.message + : describeQueryError(cause), + ); }) - .catch((cause: unknown) => setSubmitError(describeQueryError(cause))) .finally(() => setSubmitting(false)); } @@ -935,8 +932,8 @@ export function ConnectorCredentialDialog({ onClick={handleSubmit} > {submitting - ? SETTINGS_STRINGS.connectionsTestAndSaving - : SETTINGS_STRINGS.connectionsTestAndSaveAction} + ? SETTINGS_STRINGS.connectionsConnecting + : SETTINGS_STRINGS.connectionsConnectDialogAction} diff --git a/packages/settings-ui/src/index.ts b/packages/settings-ui/src/index.ts index 5a65b557c..7bffd43ac 100644 --- a/packages/settings-ui/src/index.ts +++ b/packages/settings-ui/src/index.ts @@ -117,7 +117,6 @@ export type { export { ConnectionsApiError, - testConnectorCredential, completeConnectorCredential, disconnectConnector, } from "./connections-api"; diff --git a/packages/settings-ui/src/strings.ts b/packages/settings-ui/src/strings.ts index dbfff48bd..7fa91c247 100644 --- a/packages/settings-ui/src/strings.ts +++ b/packages/settings-ui/src/strings.ts @@ -269,10 +269,10 @@ export const SETTINGS_STRINGS = { connectionsDialogConnectTitle: (name: string) => `Connect ${name}`, connectionsDialogReconnectTitle: (name: string) => `Reconnect ${name}`, connectionsDialogDescription: - "Sealed on save — this key is never shown again after create. It's tested before it's stored, so a bad key never gets saved.", + "Sealed on save — this key is never shown again after create. A bad key never gets saved; connecting surfaces the problem right here.", connectionsKeyLabel: "API key", - connectionsTestAndSaveAction: "Test key and connect", - connectionsTestAndSaving: "Testing and connecting…", + connectionsConnectDialogAction: "Connect", + connectionsConnecting: "Connecting…", connectionsSaving: "Saving…", connectionsCancel: "Cancel", connectionsConnectedToast: (name: string) => `${name} connected`, diff --git a/packages/settings-ui/test/connections-api.test.ts b/packages/settings-ui/test/connections-api.test.ts index 0d521b1a1..7edfccda2 100644 --- a/packages/settings-ui/test/connections-api.test.ts +++ b/packages/settings-ui/test/connections-api.test.ts @@ -8,7 +8,6 @@ import { completeConnectorCredential, disconnectConnector, fetchOAuthConfigured, - testConnectorCredential, } from "../src/connections-api"; const realFetch = globalThis.fetch; @@ -83,22 +82,6 @@ describe("fetchOAuthConfigured", () => { }); }); -describe("testConnectorCredential", () => { - test("resolves ok on 200", async () => { - stubFetch(() => json({ ok: true })); - const result = await testConnectorCredential("tnt_1", "granola", "key"); - expect(result).toEqual({ ok: true }); - }); - - test("resolves { ok: false, message } on 422 instead of throwing", async () => { - stubFetch(() => - json({ error: { code: "invalid_credential", message: "bad key" } }, 422), - ); - const result = await testConnectorCredential("tnt_1", "granola", "key"); - expect(result).toEqual({ ok: false, message: "bad key" }); - }); -}); - describe("completeConnectorCredential", () => { test("posts the api key and returns the stored credential id", async () => { const calls = stubFetch(() => @@ -110,6 +93,17 @@ describe("completeConnectorCredential", () => { ); expect(result).toEqual({ credentialId: "cred_1", status: "active" }); }); + + // CL-6377: connecting is the one round-trip — a rejected key throws + // straight from this call, with no separate test step beforehand. + test("throws ConnectionsApiError with the probe's own message on a 422", async () => { + stubFetch(() => + json({ error: { code: "invalid_credential", message: "bad key" } }, 422), + ); + await expect( + completeConnectorCredential("tnt_1", "granola", "key"), + ).rejects.toThrow("bad key"); + }); }); describe("disconnectConnector", () => { diff --git a/packages/settings-ui/test/connector-credential-dialog.test.tsx b/packages/settings-ui/test/connector-credential-dialog.test.tsx index 5f360da12..71525c9d7 100644 --- a/packages/settings-ui/test/connector-credential-dialog.test.tsx +++ b/packages/settings-ui/test/connector-credential-dialog.test.tsx @@ -1,9 +1,8 @@ -// CL-6077: one primary action, not test-then-save — the wizard's own -// onboarding step already combines "test key and run my first routine" -// into a single button, and this dialog now matches that: pasting a key -// and pressing the one primary action tests it for real and only stores -// it once that test passes. A rejected key never reaches -// `completeConnectorCredential`, so nothing gets sealed on a bad key. +// CL-6377: one action, not test-then-save — pasting a key and pressing +// the single Connect button is the whole flow. There is no separate +// client-driven "test" round-trip before it: `/complete` itself proves +// the key against the connector's own probe and only stores it once that +// probe accepts, so a rejected key never gets sealed. import { afterEach, describe, expect, test } from "bun:test"; import { act } from "react"; @@ -67,15 +66,15 @@ const settle = () => function primaryButton(): HTMLButtonElement { const button = [...document.body.querySelectorAll("button")].find( (candidate) => - candidate.textContent === "Test key and connect" || - candidate.textContent === "Testing and connecting…", + candidate.textContent === "Connect" || + candidate.textContent === "Connecting…", ); expect(button).not.toBeUndefined(); return button as HTMLButtonElement; } describe("ConnectorCredentialDialog", () => { - test("offers exactly one primary action — no separate Test and Save buttons", () => { + test("offers exactly one primary action — no separate Test and Save buttons, and no test-key copy anywhere", () => { const { container, root } = mount(); try { const labels = [...document.body.querySelectorAll("button")].map( @@ -83,24 +82,20 @@ describe("ConnectorCredentialDialog", () => { ); expect(labels).not.toContain("Test connection"); expect(labels).not.toContain("Save"); - expect(labels).toContain("Test key and connect"); + expect(labels).toContain("Connect"); + expect(document.body.textContent).not.toContain("Test key"); + expect(document.body.textContent).not.toContain("test the key"); } finally { act(() => root.unmount()); container.remove(); } }); - test("a passing test stores the key and reports success, in one click", async () => { + test("connecting is a single round-trip to /complete — no separate /credential/test call", async () => { const calls: string[] = []; globalThis.fetch = (async (input: RequestInfo | URL) => { const path = String(input); calls.push(path); - if (path.endsWith("/credential/test")) { - return new Response(JSON.stringify({ ok: true }), { - status: 200, - headers: { "content-type": "application/json" }, - }); - } return new Response( JSON.stringify({ credentialId: "cred_1", status: "active" }), { status: 200, headers: { "content-type": "application/json" } }, @@ -117,10 +112,7 @@ describe("ConnectorCredentialDialog", () => { act(() => primaryButton().click()); await settle(); - expect(calls.some((path) => path.endsWith("/credential/test"))).toBe( - true, - ); - expect(calls.some((path) => path.endsWith("/complete"))).toBe(true); + expect(calls).toEqual(["/api/tenants/ten_1/connections/linear/complete"]); expect(connected).toBe(true); } finally { act(() => root.unmount()); @@ -128,7 +120,7 @@ describe("ConnectorCredentialDialog", () => { } }); - test("a failing test shows the rejection and never calls complete", async () => { + test("a rejected key surfaces the probe's own message inline, from that same call", async () => { const calls: string[] = []; globalThis.fetch = (async (input: RequestInfo | URL) => { const path = String(input); @@ -152,7 +144,7 @@ describe("ConnectorCredentialDialog", () => { await settle(); expect(document.body.textContent).toContain("That key doesn't work."); - expect(calls.some((path) => path.endsWith("/complete"))).toBe(false); + expect(calls).toEqual(["/api/tenants/ten_1/connections/linear/complete"]); } finally { act(() => root.unmount()); container.remove();