diff --git a/ui/src/components/AdapterLoginChrome.tsx b/ui/src/components/AdapterLoginChrome.tsx index cf7a89cf64..b2d8416daa 100644 --- a/ui/src/components/AdapterLoginChrome.tsx +++ b/ui/src/components/AdapterLoginChrome.tsx @@ -57,8 +57,8 @@ export function connectSourceName(adapterType: string): string { } /** - * The connect step's login card: an instruction with a Cancel beside it, then - * the rows the customer works through. + * The connect step's login card: an instruction, then the rows the customer + * works through. * * The rows are the caller's, because the two login modes genuinely differ in * the last one — Claude takes a code back, OpenAI hands one out — while @@ -67,7 +67,6 @@ export function connectSourceName(adapterType: string): string { */ export function OnboardingLoginCard({ instruction, - onCancel, loading = false, children, }: { @@ -77,7 +76,6 @@ export function OnboardingLoginCard({ * be mono to read as a name rather than as prose. */ instruction: ReactNode; - onCancel?: () => void; /** * Show a spinner instead of the contents, at the same height. * @@ -104,41 +102,25 @@ export function OnboardingLoginCard({ return (
- {/* No `gap`: `justify-between` already holds the two apart, and the eight - pixels a gap reserves are eight the instruction does not have. The - longest of these strings needs the full width between the inset and - Cancel to stay on one line, which is how the design draws it — a gap - here wrapped it onto a second. + {/* The row is the instruction alone. It shared this line with a Cancel + until that button went — it repeated the footer's Back, which is the + step's one way out. - It is still allowed to wrap rather than being pinned to one line: a - translation longer than the English will not fit however the row is - divided, and two lines is a better failure than an overflow. */} - {/* The instruction is a step down from Cancel, which is the hierarchy the - design draws — the label describes, the button acts. + 12px, not 14. The frame's own label measures 281px and this string at + 12px Inter measures 280px, where at 14px it needs 327px in a row that + has 327px to give and wraps on the rounding. Matching the width the + design actually renders is the closer reading of it than matching a + nominal size in a font it was not drawn in. - It is also what keeps the longest of these strings on one line. The - frame's own label measures 281px, and this string at 12px Inter - measures 280px, where at 14px it needs 327px in a row that has 327px - to give and wraps on the rounding. Matching the width the design - actually renders is the closer reading of it than matching a nominal - size in a font it was not drawn in. */} + Still allowed to wrap: a translation longer than the English will not + fit however the row is divided, and two lines is a better failure + than an overflow. */} {instruction} - {onCancel && ( - - )} {/* A beat behind the sentence above it, so the card reads as one thing unfolding and the instruction has been read by the time the field is diff --git a/ui/src/components/AgentConfigForm.render.test.tsx b/ui/src/components/AgentConfigForm.render.test.tsx index f9fa9dcea5..c3da862e35 100644 --- a/ui/src/components/AgentConfigForm.render.test.tsx +++ b/ui/src/components/AgentConfigForm.render.test.tsx @@ -1640,10 +1640,13 @@ describe("AgentConfigForm environment selector", () => { expect(mockAgentsApi.cancelAdapterAuthLogin).not.toHaveBeenCalled(); }); - it("shows a reachable Cancel control in the onboarding chrome and cancels the session", async () => { - mockAgentsApi.testEnvironment.mockResolvedValue(AUTH_MISSING_RESULT); - const onCancel = vi.fn(); - + it("offers no Cancel in the onboarding chrome", async () => { + // The card carried a Cancel beside its instruction, directly above the + // step's own Back. Two ways out of one screen is one too many, so the + // button went — and with it the only explicit release, since unmounting + // deliberately keeps the session alive for a later resume. An abandoned + // login is now collected by the server deadline, the same as one abandoned + // by closing the tab. const container = document.createElement("div"); document.body.appendChild(container); const root = createRoot(container); @@ -1663,25 +1666,20 @@ describe("AgentConfigForm environment selector", () => { environmentId="sandbox-1" chrome="onboarding" autoStart - onCancel={onCancel} /> , ); }); - await flushUntil(() => Boolean(findButton(container, "Cancel"))); + // Wait for the card itself, then assert it is actually there: an absence + // check over an empty render passes for the wrong reason. + await flushUntil(() => container.textContent?.includes("authorization code") ?? false); + expect(container.textContent).toContain("authorization code"); - await clickByText(container, "Cancel"); - - expect(mockAgentsApi.cancelAdapterAuthLogin).toHaveBeenCalledWith( - "company-1", - "codex_local", - "session-1", - ); - expect(onCancel).toHaveBeenCalledTimes(1); + expect(findButton(container, "Cancel")).toBeFalsy(); + expect(mockAgentsApi.cancelAdapterAuthLogin).not.toHaveBeenCalled(); }); - it("resumes an active login session on mount, adopting its session id and prompt", async () => { // A page reload loses every piece of local state, so the panel must read // the caller's active session and adopt it instead of starting a new one. @@ -2191,9 +2189,13 @@ describe("AgentConfigForm environment selector", () => { expect(onStored).toHaveBeenCalledWith("stored-session-1"); }); - it("shows a reachable Cancel control in the onboarding chrome and cancels the session", async () => { - const onCancel = vi.fn(); - + it("offers no Cancel in the onboarding chrome", async () => { + // The card carried a Cancel beside its instruction, directly above the + // step's own Back. Two ways out of one screen is one too many, so the + // button went — and with it the only explicit release, since unmounting + // deliberately keeps the session alive for a later resume. An abandoned + // login is now collected by the server deadline, the same as one abandoned + // by closing the tab. const container = document.createElement("div"); document.body.appendChild(container); const root = createRoot(container); @@ -2213,24 +2215,20 @@ describe("AgentConfigForm environment selector", () => { environmentId="sandbox-1" chrome="onboarding" autoStart - onCancel={onCancel} /> , ); }); - await flushUntil(() => Boolean(findButton(container, "Cancel"))); + // Wait for the card itself, then assert it is actually there: an absence + // check over an empty render passes for the wrong reason. + await flushUntil(() => container.textContent?.includes("authorization code") ?? false); + expect(container.textContent).toContain("authorization code"); - await clickByText(container, "Cancel"); - - expect(mockAgentsApi.cancelClaudeSetupTokenLogin).toHaveBeenCalledWith( - "company-1", - "claude-session-1", - ); - expect(onCancel).toHaveBeenCalledTimes(1); + expect(findButton(container, "Cancel")).toBeFalsy(); + expect(mockAgentsApi.cancelClaudeSetupTokenLogin).not.toHaveBeenCalled(); }); - it("offers an apply-existing affordance when the status route reports a stored value", async () => { mockAgentsApi.getClaudeOAuthTokenStatus.mockResolvedValue({ secretId: "secret-1", diff --git a/ui/src/components/AgentConfigForm.tsx b/ui/src/components/AgentConfigForm.tsx index cedc57ad55..790c127734 100644 --- a/ui/src/components/AgentConfigForm.tsx +++ b/ui/src/components/AgentConfigForm.tsx @@ -2031,10 +2031,6 @@ export type AdapterLoginPanelProps = AdapterLoginDescriptor & { // footer button is the press — by the time the panel is rendered there, the // customer has already asked for this. autoStart?: boolean; - // The customer abandoned the login from inside the card. The panel has - // already cancelled the server session by the time this fires; the caller - // uses it to put its own control back to the state it started in. - onCancel?: () => void; // The login reached its success state. Onboarding advances on this, which is // why the `onboarding` chrome draws no success state of its own — the screen // it would appear on is already gone. @@ -2091,7 +2087,6 @@ function DisplayedCodeLoginPanel({ adapterType, environmentId, autoStart, - onCancel, onConnected, chrome = "panel", onPromptReady, @@ -2155,6 +2150,13 @@ function DisplayedCodeLoginPanel({ } }, retry: false, + // Never answered from cache. This read decides whether to adopt a running + // session or start a new one, and a cached "none" from an earlier mount is + // exactly wrong after Back: the panel would read `isFetched` immediately, + // see the stale null, and start a second login while the refetch was still + // in flight — which the per-owner cap then rejects. + gcTime: 0, + staleTime: 0, }); // While the panel releases a resumed session it cannot recover (see below), @@ -2291,17 +2293,11 @@ function DisplayedCodeLoginPanel({ onPromptReadyRef.current?.(prompt?.url ?? null); }, [prompt]); - const handleCancel = () => { - cancelLogin.mutate(); - onCancel?.(); - }; - if (chrome === "onboarding") { const failed = isTerminal && status && status !== "authenticated"; return ( {/* The same destination as the step's own button. Two ways to one @@ -2495,7 +2491,6 @@ function SubmittedBrowserCodeLoginPanel({ onStored, onApplyStored, autoStart, - onCancel, onConnected, chrome = "panel", onPromptReady, @@ -2664,6 +2659,13 @@ function SubmittedBrowserCodeLoginPanel({ } }, retry: false, + // Never answered from cache. This read decides whether to adopt a running + // session or start a new one, and a cached "none" from an earlier mount is + // exactly wrong after Back: the panel would read `isFetched` immediately, + // see the stale null, and start a second login while the refetch was still + // in flight — which the per-owner cap then rejects. + gcTime: 0, + staleTime: 0, }); // While the panel releases a resumed session it cannot recover (see below), @@ -2958,11 +2960,6 @@ function SubmittedBrowserCodeLoginPanel({ onConnectedRef.current?.(); }, [isStored]); - const handleCancel = () => { - cancelLogin.mutate(); - onCancel?.(); - }; - const onPromptReadyRef = useRef(onPromptReady); onPromptReadyRef.current = onPromptReady; useEffect(() => { @@ -2974,7 +2971,6 @@ function SubmittedBrowserCodeLoginPanel({ return ( root.unmount()); }); + it("resumes the same claude_local session after Back, rather than starting a second", async () => { + // This is the behaviour that makes the card's Cancel removable. Back only + // hides the card — it deliberately does not release the session — so + // coming back has to adopt the one already running. If it started a + // fresh one instead, the removed Cancel would have been the only way out + // of a login the customer could no longer reach, and the per-owner cap + // would reject the second start. + const session = { + sessionId: "claude-session-1", + status: "pending", + expiresAt: new Date(Date.now() + 600_000).toISOString(), + }; + let started = false; + mockAgentsApi.startClaudeSetupTokenLogin.mockImplementation(async () => { + started = true; + return session; + }); + mockAgentsApi.getActiveClaudeSetupTokenLoginSession.mockReset(); + mockAgentsApi.getActiveClaudeSetupTokenLoginSession.mockImplementation(async () => + started ? session : null, + ); + mockAgentsApi.getAdapterAuthSignal.mockResolvedValue({ status: "absent" }); + const { root } = await openStep4({ adapterType: "claude_local" }); + + await pickSource(/Claude/); + // The login is genuinely running: without this the assertion below holds + // for the wrong reason. + expect(mockAgentsApi.startClaudeSetupTokenLogin).toHaveBeenCalledTimes(1); + + const back = [...document.body.querySelectorAll("button")].find((b) => + b.textContent?.trim().startsWith("Back"), + ); + await act(async () => { + back!.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + for (let i = 0; i < 12; i++) await flushReact(); + + await pickSource(/Claude/); + for (let i = 0; i < 8; i++) await flushReact(); + + expect(mockAgentsApi.startClaudeSetupTokenLogin).toHaveBeenCalledTimes(1); + expect(mockAgentsApi.getActiveClaudeSetupTokenLoginSession).toHaveBeenCalled(); + + await act(async () => root.unmount()); + }); + + it("resumes the same codex_local session after Back, rather than starting a second", async () => { + const session = { sessionId: "codex-session-1", status: "pending" }; + let started = false; + mockAgentsApi.startAdapterAuthLogin.mockImplementation(async () => { + started = true; + return session; + }); + mockAgentsApi.getActiveAdapterAuthLoginSession.mockReset(); + mockAgentsApi.getActiveAdapterAuthLoginSession.mockImplementation(async () => + started ? session : null, + ); + mockAgentsApi.getAdapterAuthSignal.mockResolvedValue({ status: "unknown" }); + const { root } = await openStep4({ adapterType: "claude_local" }); + + await pickSource(/OpenAI/); + expect(mockAgentsApi.startAdapterAuthLogin).toHaveBeenCalledTimes(1); + + const back = [...document.body.querySelectorAll("button")].find((b) => + b.textContent?.trim().startsWith("Back"), + ); + await act(async () => { + back!.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + for (let i = 0; i < 12; i++) await flushReact(); + + await pickSource(/OpenAI/); + for (let i = 0; i < 8; i++) await flushReact(); + + expect(mockAgentsApi.startAdapterAuthLogin).toHaveBeenCalledTimes(1); + expect(mockAgentsApi.getActiveAdapterAuthLoginSession).toHaveBeenCalled(); + + await act(async () => root.unmount()); + }); + + it("starts the other source's login after backing out of the first", async () => { + // The abandonment case, raised in review against removing the card's + // Cancel: with no explicit release, does a source switch still get a + // login? It does. The server's lease is keyed on the adapter type as + // well as the company and environment, so the abandoned Claude session + // does not stand in the way of a Codex one — and it is collected on its + // own five-minute timer regardless (DEVICE_LOGIN_TIMEOUT_MS), with the + // reaper as the restart-safe backstop. + mockAgentsApi.getAdapterAuthSignal.mockResolvedValue({ status: "absent" }); + const { root } = await openStep4({ adapterType: "claude_local" }); + + await pickSource(/Claude/); + expect(mockAgentsApi.startClaudeSetupTokenLogin).toHaveBeenCalledTimes(1); + + const back = [...document.body.querySelectorAll("button")].find((b) => + b.textContent?.trim().startsWith("Back"), + ); + await act(async () => { + back!.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + for (let i = 0; i < 12; i++) await flushReact(); + + await pickSource(/OpenAI/); + for (let i = 0; i < 8; i++) await flushReact(); + + expect(mockAgentsApi.startAdapterAuthLogin).toHaveBeenCalledTimes(1); + expect(mockAgentsApi.startClaudeSetupTokenLogin).toHaveBeenCalledTimes(1); + + await act(async () => root.unmount()); + }); + it("hires on Connect, with no sign-in, when the signal reports a ready credential", async () => { mockAgentsApi.getAdapterAuthSignal.mockResolvedValue({ status: "present" }); const { root } = await openStep4({ adapterType: "claude_local" }); diff --git a/ui/src/components/OnboardingWizard.tsx b/ui/src/components/OnboardingWizard.tsx index b976feb495..a40e517bce 100644 --- a/ui/src/components/OnboardingWizard.tsx +++ b/ui/src/components/OnboardingWizard.tsx @@ -1393,12 +1393,16 @@ function OnboardingWizardInner({ /** * Back, on the connect step, unwinds the sign-in before it leaves the step. * - * This hides the card; it does not cancel the login. Unmounting the panel no - * longer releases the server session — the session stays reachable for a + * This hides the card; it does not cancel the login. Unmounting the panel + * does not release the server session — the session stays reachable for a * later resume, the same read that restores it after a reload — so backing - * out and returning shows the sign-in still running, not a fresh one. The - * card's own Cancel button is the only explicit release; `onCancel` below - * puts the step back here when it fires. + * out and returning shows the sign-in still running, not a fresh one. + * + * Nothing releases it explicitly any more. The card carried a Cancel that + * did, sitting beside an instruction and directly above this step's own + * Back, and two ways out of one screen is one too many — the button went and + * the release went with it. What is left is the server deadline, which is + * the same thing that collects a session abandoned by closing the tab. */ function unwindConnectStep() { setConnectAuthUrl(null); @@ -3171,11 +3175,10 @@ function OnboardingWizardInner({ in the connect step's chrome. It owns the session; the step owns the sequence around it. - Unmounting it is no longer the cancel: the session - stays reachable for a later resume, so Back and a - source switch only hide the card. `onCancel` fires - from the card's own Cancel button, the one explicit - release, and puts the step back where Back would. + Unmounting it is not the cancel: the session stays + reachable for a later resume, so Back and a source + switch only hide the card, and nothing here releases + the session early — see `unwindConnectStep`. No "Use saved login" control: the hire step already applies a stored login on its own. */ @@ -3186,7 +3189,6 @@ function OnboardingWizardInner({ environmentId={resolvedLoginEnvironmentId} chrome="onboarding" autoStart - onCancel={unwindConnectStep} onPromptReady={(url) => { setConnectAuthUrl(url); // The prompt arriving is what ends the waiting beat.