RomanNum3ral
/
paperclip
mirror of https://github.com/paperclipai/paperclip.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix(onboarding): drop the sign-in card's Cancel, and resume after Back (#12958) 

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.

Removing it was only safe if returning genuinely resumed the running session, and writing the test for that disproved the premise: coming back started a second login rather than adopting the first, which the server's per-owner lease rejects. Cancel was not duplicative — it was the only way out of a login the customer could no longer reach.

The cause was the cache. The active-session read is a useQuery with no staleTime, so on the second mount it answered instantly from the first mount's cached null: isFetched was true, the resume effect concluded there was nothing to adopt, and autoStart began a new login while the refetch was still in flight. Both panels did it, and it was pre-existing on master. Both reads now refuse the cache, so the resume works and the removal is safe because of it.

Removing the button also stranded the chain behind it: handleCancel in both panels, the onCancel prop on AdapterLoginPanel, and the wizard's wiring. The panel chrome's own Cancel is untouched.

An abandoned login is collected on a five-minute timer (DEVICE_LOGIN_TIMEOUT_MS) with the reaper as the restart-safe backstop, and the lease is keyed on adapterType, so a source switch does not contend. Both covered by tests.
This commit is contained in:
Tonio 2026-09-07 00:20:31 -07:00 committed by GitHub
parent 856813ba3a
commit ae03465ad4
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 178 additions and 89 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

46
ui/src/components/AdapterLoginChrome.tsx
View File

@ -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 (
<div className="flex min-h-(--sz-108px) flex-col gap-4 rounded-xl bg-muted/40 p-4">
{/* 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. */}
<motion.div
className="flex items-center justify-between pl-2"
className="flex items-center pl-2"
initial={{ opacity: 0, y: CARD_REVEAL_TRAVEL }}
animate={{ opacity: 1, y: 0, transition: CARD_REVEAL_INSTRUCTION }}
>
<span className="text-xs text-muted-foreground">{instruction}</span>
{onCancel && (
<Button
type="button"
variant="ghost"
size="sm"
className="h-7 shrink-0 px-2.5 text-sm font-medium"
onClick={onCancel}
>
Cancel
</Button>
)}
</motion.div>
{/* 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

54
ui/src/components/AgentConfigForm.render.test.tsx
View File

@ -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}
/>
</TooltipProvider>
</ToastProvider>
</QueryClientProvider>,
);
});
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}
/>
</TooltipProvider>
</ToastProvider>
</QueryClientProvider>,
);
});
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",

32
ui/src/components/AgentConfigForm.tsx
View File

@ -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 (
<OnboardingLoginCard
loading={!prompt && !startError && !failed}
onCancel={handleCancel}
instruction={
<>
{/* 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 (
<OnboardingLoginCard
loading={!authorizationUrl && !startError && !failedNow}
onCancel={handleCancel}
instruction={
<>
<a

111
ui/src/components/OnboardingWizard.test.tsx
View File

@ -2533,6 +2533,117 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", (
await act(async () => 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" });

24
ui/src/components/OnboardingWizard.tsx
View File

@ -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.