diff --git a/ui/src/components/AdapterLoginChrome.test.tsx b/ui/src/components/AdapterLoginChrome.test.tsx index df572a6040..8a2008e7f1 100644 --- a/ui/src/components/AdapterLoginChrome.test.tsx +++ b/ui/src/components/AdapterLoginChrome.test.tsx @@ -94,9 +94,11 @@ describe("the connect step's cards", () => { expect(key!.className).toBe(code!.className); }); - it("masks a key and does not mask a one-time code", () => { - // A provider key is a credential that goes on living; a browser code is - // single-use and about to be pasted somewhere the customer can see. + it("masks only when asked", () => { + // The primitive leaves the choice to each card rather than guessing from + // the label. The key card asks, and so does the Claude card for its code — + // that call site is pinned by the wizard's paste test. What this pins is + // that asking is what does it, and that not asking shows the value. render( <> {}} onSubmit={() => {}} /> diff --git a/ui/src/components/AdapterLoginChrome.tsx b/ui/src/components/AdapterLoginChrome.tsx index b2d8416daa..a8028cf950 100644 --- a/ui/src/components/AdapterLoginChrome.tsx +++ b/ui/src/components/AdapterLoginChrome.tsx @@ -360,7 +360,12 @@ export function OnboardingCardField({ disabled?: boolean; label?: string; placeholder?: string; - /** A provider key is a credential; a one-time browser code is not. */ + /** + * Dots instead of the value. The key card asks for it because a provider key + * is a credential that goes on living. The Claude card asks too: its code + * stays in the field after the paste so the customer can see something + * landed, and that is all they need to see of it. + */ masked?: boolean; /** * Take focus when the card opens. diff --git a/ui/src/components/AgentConfigForm.tsx b/ui/src/components/AgentConfigForm.tsx index 4bb03b9759..1e22442fcc 100644 --- a/ui/src/components/AgentConfigForm.tsx +++ b/ui/src/components/AgentConfigForm.tsx @@ -2261,6 +2261,15 @@ export type AdapterLoginPanelProps = AdapterLoginDescriptor & { // why the `onboarding` chrome draws no success state of its own — the screen // it would appear on is already gone. onConnected?: () => void; + // The pasted code went to the server. Fires as the submit starts rather than + // when the login finishes, so a caller can show the work the moment the + // customer has done their part: the round trip to `onConnected` is a poll + // and a completion read, long enough to read as nothing having happened. + onCodeSubmitted?: () => void; + // A submitted code did not become a stored login — the submit was refused, + // the completion failed, or the session failed or ran out of time. The pair + // of `onCodeSubmitted`, so a caller that showed work can stop showing it. + onSubmitFailed?: () => void; chrome?: AdapterLoginChrome; /** * The address the customer has to open, once the server has produced one. @@ -2268,9 +2277,9 @@ export type AdapterLoginPanelProps = AdapterLoginDescriptor & { * The one fact about a running login that the step needs outside the card: * its own button is what sends the customer there, and a prompt arriving is * what moves the step from waiting to ready. Everything else it needs the - * panel already does — the paste submits itself, success is reported through - * `onConnected`, and the customer's own Cancel press is reported through - * `onCancel` — so this stays a single value rather than a whole session + * panel already does — the paste submits itself, and the submit and how it + * ended are reported through `onCodeSubmitted`, `onSubmitFailed` and + * `onConnected` — so this stays a single value rather than a whole session * handed upward. */ onPromptReady?: (authorizationUrl: string | null) => void; @@ -2804,6 +2813,8 @@ function SubmittedBrowserCodeLoginPanel({ onApplyStored, autoStart, onConnected, + onCodeSubmitted, + onSubmitFailed, chrome = "panel", onPromptReady, }: AdapterLoginPanelProps) { @@ -2827,6 +2838,10 @@ function SubmittedBrowserCodeLoginPanel({ // True after the client wall-clock cap passes for the active login. The panel // stops both polls and shows the timed-out state. const [timedOut, setTimedOut] = useState(false); + // A code has gone to the server and has not yet come back as a stored login + // or a failure. The field is locked for that stretch: the step's button is + // saying "Connecting" above it, and a second paste would submit again. + const [codeSubmitted, setCodeSubmitted] = useState(false); // True after the status poll returns 404. The server removes the row and the // in-memory session at once on any non-stored terminal state, so a status 404 // means the login failed and the server cleaned up. The panel stops both @@ -2855,6 +2870,7 @@ function SubmittedBrowserCodeLoginPanel({ setCompletionFailed(false); setTimedOut(false); setStatusGone(false); + setCodeSubmitted(false); completionStartedRef.current = false; }; @@ -3171,11 +3187,26 @@ function SubmittedBrowserCodeLoginPanel({ Boolean(authorizationUrl) && !isCompleting && isValidBrowserCode(trimmedCode) && - !submitCode.isPending; + !submitCode.isPending && + !codeSubmitted; + + const onCodeSubmittedRef = useRef(onCodeSubmitted); + onCodeSubmittedRef.current = onCodeSubmitted; + const onSubmitFailedRef = useRef(onSubmitFailed); + onSubmitFailedRef.current = onSubmitFailed; const handleSubmit = () => { if (!canSubmit) return; + // A new attempt supersedes the last attempt's error, and has to: the + // failure report below watches for an error after a submit, and one left + // over from before it would end this attempt the moment it began. + setStartError(null); submitCode.mutate(trimmedCode); + // Reported now, not when the login finishes. A stored login is a poll and a + // completion read away, long enough that a button still offering "Waiting + // for code" after the paste read as the paste not having registered. + setCodeSubmitted(true); + onCodeSubmittedRef.current?.(); // Onboarding keeps the code on screen; the panel still clears it. // // Clearing emptied the input in the same frame the paste landed, so on the @@ -3272,6 +3303,18 @@ function SubmittedBrowserCodeLoginPanel({ onConnectedRef.current?.(); }, [isStored]); + // The other end of `onCodeSubmitted`. Any of these after a submit means the + // code is not going to become a stored login, and a caller still showing + // "Connecting" would otherwise spin for good. Once per submit; the field + // unlocks with it. Not reset on success: the field stays locked through the + // hold that follows, rather than reopening under a button saying Connecting. + useEffect(() => { + if (!codeSubmitted) return; + if (!startError && !isFailure && !timedOut) return; + setCodeSubmitted(false); + onSubmitFailedRef.current?.(); + }, [codeSubmitted, startError, isFailure, timedOut]); + const onPromptReadyRef = useRef(onPromptReady); onPromptReadyRef.current = onPromptReady; useEffect(() => { @@ -3324,7 +3367,11 @@ function SubmittedBrowserCodeLoginPanel({ onPaste={() => { pastedRef.current = true; }} - disabled={submitCode.isPending || isCompleting} + // Dots, not the code. It stays in the field after the paste so the + // customer can see something landed, and that is all they need to + // see of it. + masked + disabled={submitCode.isPending || isCompleting || codeSubmitted} /> )} diff --git a/ui/src/components/OnboardingWizard.test.tsx b/ui/src/components/OnboardingWizard.test.tsx index 15d35e13d5..01ef763576 100644 --- a/ui/src/components/OnboardingWizard.test.tsx +++ b/ui/src/components/OnboardingWizard.test.tsx @@ -2306,10 +2306,271 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( // followed was an input that had just gone blank — reported from staging // as the paste looking dropped, or the step looking stuck. expect(field!.value).toBe("Q2RJ-E1YIF-authorization-code"); + // As dots. The code is kept so the customer can see the paste landed, + // and that is all the field needs to show of it. + expect(field!.type).toBe("password"); + // And the button answers the paste itself. The status here never reaches + // authenticated, so this is "Connecting" before any server confirmation — + // waiting for that left about a second of a button still reading + // "Waiting for code" after the code had gone in. + expect( + [...document.body.querySelectorAll("button")].pop()?.textContent?.trim(), + ).toBe("Connecting"); await act(async () => root.unmount()); }); + it("does not hire on the paste alone, before the login is stored", async () => { + // "Connecting" appears at the paste now, ahead of the server confirming + // anything. The two-second hold used to start at that same moment, so + // moving one without the other would hire at the paste plus two seconds + // whether or not a credential existed. The status here stays pending, so + // the login is never stored. + mockAgentsApi.getAdapterAuthSignal.mockResolvedValue({ status: "absent" }); + const { root } = await openStep4({ adapterType: "claude_local" }); + await pickSource(/Claude/); + + const field = document.body.querySelector( + 'input[aria-label="Authorization code"]', + ) as HTMLInputElement; + await act(async () => { + field.dispatchEvent(new Event("paste", { bubbles: true })); + setControlledValue(field, "Q2RJ-E1YIF-authorization-code"); + }); + for (let i = 0; i < 4; i++) await flushReact(); + + const cta = () => + [...document.body.querySelectorAll("button")].pop()?.textContent?.trim(); + // The paste really did start Connecting; without this the assertion + // below would hold for a flow that never got that far. + expect(cta(), "the paste should have started Connecting").toBe("Connecting"); + + await act(async () => { + await new Promise((resolve) => window.setTimeout(resolve, CONNECTED_HOLD_MS + 400)); + }); + for (let i = 0; i < 4; i++) await flushReact(); + + expect(mockAgentsApi.hire).not.toHaveBeenCalled(); + expect(cta()).toBe("Connecting"); + + await act(async () => root.unmount()); + }); + + it("gives the button back when the pasted code is refused", async () => { + // The other half of answering the paste early: a button that says + // "Connecting" before the server answers has to stop saying it when the + // answer is no, or it spins on a login that is not coming. + mockAgentsApi.getAdapterAuthSignal.mockResolvedValue({ status: "absent" }); + mockAgentsApi.submitClaudeSetupTokenBrowserCode.mockRejectedValueOnce( + new Error("That authorization code was not accepted."), + ); + const { root } = await openStep4({ adapterType: "claude_local" }); + await pickSource(/Claude/); + + const cta = () => + [...document.body.querySelectorAll("button")].pop()?.textContent?.trim(); + expect(cta()).toBe("Sign in to Claude"); + + const field = document.body.querySelector( + 'input[aria-label="Authorization code"]', + ) as HTMLInputElement; + await act(async () => { + field.dispatchEvent(new Event("paste", { bubbles: true })); + setControlledValue(field, "Q2RJ-E1YIF-authorization-code"); + }); + for (let i = 0; i < 8; i++) await flushReact(); + + expect(mockAgentsApi.submitClaudeSetupTokenBrowserCode).toHaveBeenCalledTimes(1); + expect(document.body.textContent).toContain("That authorization code was not accepted."); + expect(cta()).toBe("Sign in to Claude"); + expect(mockAgentsApi.hire).not.toHaveBeenCalled(); + + await act(async () => root.unmount()); + }); + + it("does not reopen the card when a pasted code fails after Back", async () => { + // The panel stays mounted through Back's exit, so its report of a failed + // submit can land mid-exit. Restoring the button there reopened the card + // the customer was leaving, without the address Back had cleared. + mockAgentsApi.getAdapterAuthSignal.mockResolvedValue({ status: "absent" }); + let refuse: (error: Error) => void = () => {}; + mockAgentsApi.submitClaudeSetupTokenBrowserCode.mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + refuse = reject; + }), + ); + const { root } = await openStep4({ adapterType: "claude_local" }); + await pickSource(/Claude/); + + const field = document.body.querySelector( + 'input[aria-label="Authorization code"]', + ) as HTMLInputElement; + await act(async () => { + field.dispatchEvent(new Event("paste", { bubbles: true })); + setControlledValue(field, "Q2RJ-E1YIF-authorization-code"); + }); + for (let i = 0; i < 4; i++) await flushReact(); + + const cta = () => + [...document.body.querySelectorAll("button")].pop()?.textContent?.trim(); + expect(mockAgentsApi.submitClaudeSetupTokenBrowserCode).toHaveBeenCalledTimes(1); + expect(cta(), "the paste should have started Connecting").toBe("Connecting"); + + // Hold the exit open so the refusal lands inside it. Without a + // `matchMedia` to ask, every beat collapses to zero and the exit would be + // over before the refusal arrived — which would pass for the wrong reason. + const realMatchMedia = window.matchMedia; + Object.defineProperty(window, "matchMedia", { + configurable: true, + writable: true, + value: (query: string) => ({ + matches: false, + media: query, + onchange: null, + addListener: () => {}, + removeListener: () => {}, + addEventListener: () => {}, + removeEventListener: () => {}, + dispatchEvent: () => false, + }), + }); + try { + const back = [...document.body.querySelectorAll("button")].find((b) => + b.textContent?.trim().startsWith("Back"), + ); + await act(async () => { + back!.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + await act(async () => { + refuse(new Error("That authorization code was not accepted.")); + }); + for (let i = 0; i < 4; i++) await flushReact(); + + // Still leaving: the button shows the step's resting face, not the + // sign-in it would have reopened. + expect(cta()).toBe("Next"); + + // And the exit finishes — the row is a question again. Waited in short + // slices, each its own `act`. One long `act` defers React's commits to + // its end, so a beat's timer fires on time but its phase only commits + // when the wait is over — and the next beat is scheduled only then. The + // exit crawls one step per wait and never gets back to the question. + for (let i = 0; i < 30; i++) { + await act(async () => { + await new Promise((resolve) => window.setTimeout(resolve, 50)); + }); + } + expect( + document.body + .querySelector('[role="radiogroup"]')! + .className.includes("justify-center"), + ).toBe(false); + expect(mockAgentsApi.hire).not.toHaveBeenCalled(); + } finally { + Object.defineProperty(window, "matchMedia", { + configurable: true, + writable: true, + value: realMatchMedia, + }); + } + + await act(async () => root.unmount()); + }); + + it("does not hire when the login finishes after Back", async () => { + // The same window from the other side. A login can complete while Back's + // exit is still running, and reporting that success pulled the step back + // into "Connecting" and on into a hire the customer had backed away from. + // No paste needed: here the server has already authenticated, and the + // completion read is simply slow. + mockAgentsApi.getAdapterAuthSignal.mockResolvedValue({ status: "absent" }); + mockAgentsApi.getClaudeSetupTokenLoginStatus.mockResolvedValue({ + sessionId: "claude-session-1", + status: "authenticated", + expiresAt: new Date(Date.now() + 600_000).toISOString(), + }); + let finishCompletion: (value: { storedSessionId: string }) => void = () => {}; + mockAgentsApi.completeClaudeSetupTokenLogin.mockImplementationOnce( + () => + new Promise((resolve) => { + finishCompletion = resolve; + }), + ); + const realMatchMedia = window.matchMedia; + try { + const { root } = await openStep4({ adapterType: "claude_local" }); + await pickSource(/Claude/); + for (let i = 0; i < 6; i++) await flushReact(); + + // The completion read is out and has not answered, and the card is up. + expect(mockAgentsApi.completeClaudeSetupTokenLogin).toHaveBeenCalledTimes(1); + const cta = () => + [...document.body.querySelectorAll("button")].pop()?.textContent?.trim(); + expect(cta()).toBe("Sign in to Claude"); + + // Hold the exit open, as above, so the success lands inside it. + Object.defineProperty(window, "matchMedia", { + configurable: true, + writable: true, + value: (query: string) => ({ + matches: false, + media: query, + onchange: null, + addListener: () => {}, + removeListener: () => {}, + addEventListener: () => {}, + removeEventListener: () => {}, + dispatchEvent: () => false, + }), + }); + const back = [...document.body.querySelectorAll("button")].find((b) => + b.textContent?.trim().startsWith("Back"), + ); + await act(async () => { + back!.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + await act(async () => { + finishCompletion({ storedSessionId: "stored-1" }); + }); + for (let i = 0; i < 4; i++) await flushReact(); + + expect(cta()).toBe("Next"); + + // Past the exit, and past the full hold a late success would have + // started. In short slices, each its own `act`, for the reason given in + // the test above — and here it matters twice: a hire scheduled by a late + // "Connecting" is only scheduled once that phase commits, so one long + // `act` would hide the very hire this is looking for. + const slices = Math.ceil((CONNECTED_HOLD_MS + 1200) / 50); + for (let i = 0; i < slices; i++) { + await act(async () => { + await new Promise((resolve) => window.setTimeout(resolve, 50)); + }); + } + + expect(mockAgentsApi.hire).not.toHaveBeenCalled(); + expect( + document.body + .querySelector('[role="radiogroup"]')! + .className.includes("justify-center"), + ).toBe(false); + + await act(async () => root.unmount()); + } finally { + Object.defineProperty(window, "matchMedia", { + configurable: true, + writable: true, + value: realMatchMedia, + }); + mockAgentsApi.getClaudeSetupTokenLoginStatus.mockResolvedValue({ + sessionId: "claude-session-1", + status: "pending", + expiresAt: new Date(Date.now() + 600_000).toISOString(), + }); + } + }, 15_000); + it("starts the sign-in on the first press, even when it changes the adapter", async () => { // The regression this is here for. Picking a source sets the phase *and* // the adapter, and a reset keyed on the adapter then put the phase diff --git a/ui/src/components/OnboardingWizard.tsx b/ui/src/components/OnboardingWizard.tsx index 2b7af85e04..6df9ace26d 100644 --- a/ui/src/components/OnboardingWizard.tsx +++ b/ui/src/components/OnboardingWizard.tsx @@ -1200,6 +1200,21 @@ function OnboardingWizardInner({ connectPhase === "unwindRow"; const connectLinkVisible = connectPhase === "idle" || connectPhase === "unwindRow"; + /** + * When "Connecting" started, and whether the login behind it has finished. + * + * Two facts because they now arrive at different times. The button says + * "Connecting" the moment a code is pasted, but the credential only exists + * once the server confirms it, a poll and a completion read later. The hire + * waits for both: the stored login, and two seconds of "Connecting" counted + * from the paste — so a fast server still shows the state, and a slow one + * does not have the hold added on top of its own wait. + */ + const connectingSinceRef = useRef(null); + const [connectCredentialStored, setConnectCredentialStored] = useState(false); + /** What the button was offering before a paste, for when the paste is refused. */ + const phaseBeforeSubmitRef = useRef("waiting"); + /** A sign-in is running and has not succeeded. */ const connectStepLoggingIn = connectStepNeedsLogin && connectPhase !== "idle" && connectPhase !== "connecting"; @@ -1228,17 +1243,27 @@ function OnboardingWizardInner({ return () => clearTimeout(t); } if (connectPhase === "connecting") { + // Not before the login is stored. "Connecting" starts at the paste now, + // ahead of the server confirming anything, so a hire from here would go + // out against a source with no credential to run on. + if (!connectCredentialStored) return; // No success state: the step advances. The hold is so "Connecting" is // legible as a state rather than a flicker on the way out — a step that // left the instant a paste landed would read as the paste having gone - // wrong. + // wrong. Counted from when "Connecting" appeared, so the time the server + // spent confirming counts toward it instead of being added to it. // // A beat rather than a bare timer because Back stays live through it. A // dropped handle hired two seconds after the customer had backed out, // landing them on Review having asked for the opposite; `handleGiveHeartbeat` // has no notion of the phase and could not refuse it. Leaving the phase — // Back, the step changing, unmount — now cancels the hire with it. - const t = setTimeout(() => void handleGiveHeartbeat(), CONNECTED_HOLD_MS); + const shownFor = + connectingSinceRef.current === null ? 0 : Date.now() - connectingSinceRef.current; + const t = setTimeout( + () => void handleGiveHeartbeat(), + Math.max(0, CONNECTED_HOLD_MS - shownFor), + ); return () => clearTimeout(t); } if (connectPhase === "unwindCard") { @@ -1260,7 +1285,7 @@ function OnboardingWizardInner({ return () => clearTimeout(t); } return; - }, [step, connectPhase, credentialMode, connectStepNeedsLogin]); + }, [step, connectPhase, credentialMode, connectStepNeedsLogin, connectCredentialStored]); /** * The button's four faces, and which of them can be pressed. @@ -1309,6 +1334,8 @@ function OnboardingWizardInner({ */ function unwindConnectStep() { setConnectAuthUrl(null); + connectingSinceRef.current = null; + setConnectCredentialStored(false); // Where the reverse starts depends on how far the sequence got. Backing out // during the collapse has no card to close and no room to give back. // With no card open, the row is the whole of the unwind. @@ -1331,6 +1358,10 @@ function OnboardingWizardInner({ setConnectPhase("waiting"); return; } + // The hold owns the hire once "Connecting" is showing. That now starts at + // the paste, before the credential exists, so Cmd+Enter here would hire + // against a source with nothing to run on. + if (connectPhase === "connecting") return; if (connectStepLoggingIn) return; void handleGiveHeartbeat(); } @@ -1456,6 +1487,8 @@ function OnboardingWizardInner({ setConnectPhase("idle"); setConnectAuthUrl(null); setSourcePicked(false); + connectingSinceRef.current = null; + setConnectCredentialStored(false); }, [step]); const selectedModel = (adapterModels ?? []).find((m) => m.id === model); @@ -2654,10 +2687,58 @@ function OnboardingWizardInner({ // The prompt arriving is what ends the waiting beat. if (url) setConnectPhase((p) => (p === "loading" ? "ready" : p)); }} + onCodeSubmitted={() => { + // The button reacts to the paste, not to the server. + // Waiting for the login to be stored left about a + // second of a button still reading "Waiting for code" + // after the code had already gone in. + phaseBeforeSubmitRef.current = connectPhase; + connectingSinceRef.current = Date.now(); + setConnectCredentialStored(false); + setConnectPhase("connecting"); + }} + onSubmitFailed={() => { + // Only while the button still says "Connecting". The + // panel stays mounted through Back's exit, so a failure + // that landed after Back restored the button and + // reopened the card the customer was leaving — without + // the address Back had cleared, so its sign-in could + // not even be pressed. + if (connectPhase !== "connecting") return; + // Refused, failed or timed out — the card says which. + // The button goes back to what it was offering rather + // than spinning on a login that is not coming. + connectingSinceRef.current = null; + setConnectCredentialStored(false); + setConnectPhase( + phaseBeforeSubmitRef.current === "ready" ? "ready" : "waiting", + ); + }} onConnected={() => { + // Not into a card the customer has left. The panel is + // still mounted through Back's exit, and a login that + // finished there pulled the step back into "Connecting" + // and on into a hire they had just backed away from. + // The login is stored either way; what this refuses is + // only the step moving forward after they chose to go. + if ( + connectPhase !== "loading" && + connectPhase !== "ready" && + connectPhase !== "waiting" && + connectPhase !== "connecting" + ) { + return; + } // The hold before the step advances is the phase's own // beat, above, so that backing out during it cancels - // the hire. + // the hire. It counts from the paste when there was + // one, and from here for a login that finished without + // one — a resumed session, or a code handed out rather + // than pasted back. + if (connectingSinceRef.current === null) { + connectingSinceRef.current = Date.now(); + } + setConnectCredentialStored(true); setConnectPhase("connecting"); }} onStored={() => { diff --git a/ui/src/connect-flow-preview-main.tsx b/ui/src/connect-flow-preview-main.tsx index 5d045213c5..9b6aa7510d 100644 --- a/ui/src/connect-flow-preview-main.tsx +++ b/ui/src/connect-flow-preview-main.tsx @@ -434,6 +434,7 @@ function ConnectFlowPreview({ { if (isValidBrowserCode(code.trim())) {