From 60188610eee206d7266ba76e38c9085a94202169 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Thu, 17 Sep 2026 11:12:22 -0700 Subject: [PATCH] fix: do not let an in-flight open blank a newer prompt, or promise a bridge that is off Two findings from review of this branch. Awaiting the open instead of dismissing up front bought a window: on Linux it is at least OPENER_GRACE, doubled when xdg-open fails and gio is tried. If the container relays a second URL inside that window, the first open's resolution blanked the second prompt -- losing a link that exists only in the container's transcript, which is the failure "dismiss on success only" was made to prevent. The slot already carried a `seq` for exactly this reason; dismissal is now conditional on it. `urlPromptRef` is written eagerly by the two functions that change the slot rather than synced by an effect. That is load-bearing: an effect-synced mirror lags state by a commit, and a promise microtask can resolve between `setUrlPrompt` and React flushing passive effects -- so it answers "did a newer prompt land?" wrong in precisely the window the guard exists for. Dropping the functional updater also fixes `promptSeqRef.current += 1` being mutated inside a state updater React is free to invoke twice. The guard is a sibling function rather than an optional argument on `dismissUrlPrompt`, because that function is passed by reference as UrlToast's `onDismiss` and React would hand it a MouseEvent as its first argument -- the seq check would fail and the close button would silently stop working, with the types still assignable. Separately, the sign-in hint was binary on which button leads, but "host leads" covers both a live bridge and a fallback where nothing is set up to catch the callback at all. In the second case the toast promised the bridge would carry it and the login hung to its timeout. The target is now three-state, the hint tells the truth in the fallback case and names the control that fixes it, and the hook starts at `host-fallback` rather than assuming a bridge it has not confirmed. Co-Authored-By: Claude Opus 5 (1M context) --- .../components/terminal/TerminalView.test.tsx | 154 +++++++++++++++++- app/src/components/terminal/TerminalView.tsx | 116 ++++++++++--- app/src/components/terminal/UrlToast.test.tsx | 51 +++++- app/src/components/terminal/UrlToast.tsx | 36 +++- app/src/hooks/useSignInOpenTarget.ts | 64 ++++++-- 5 files changed, 356 insertions(+), 65 deletions(-) diff --git a/app/src/components/terminal/TerminalView.test.tsx b/app/src/components/terminal/TerminalView.test.tsx index 56aa70d..b59da3b 100644 --- a/app/src/components/terminal/TerminalView.test.tsx +++ b/app/src/components/terminal/TerminalView.test.tsx @@ -626,7 +626,7 @@ describe("chooseSignInTarget — which action leads for a sign-in link", () => { // and the container-side target is Playwright's pane, whose browsers are not // in the image. it("prefers the host browser whenever the bridge is live", () => { - expect(chooseSignInTarget(LIVE_BRIDGE, usableDetection())).toBe("host"); + expect(chooseSignInTarget(LIVE_BRIDGE, usableDetection())).toBe("host-bridged"); }); it("does not call a bridge live while it is holding a port conflict", () => { @@ -644,13 +644,17 @@ describe("chooseSignInTarget — which action leads for a sign-in link", () => { // There is nothing to bridge until the CLI binds its listener, and that // races the URL reaching the transcript. Requiring a port would make the // default flip between two identical sign-ins. - expect(chooseSignInTarget(LIVE_BRIDGE, null)).toBe("host"); + expect(chooseSignInTarget(LIVE_BRIDGE, null)).toBe("host-bridged"); }); it("falls to the container only when it has a browser to open", () => { const off: AuthBridgeStatus = { enabled: false, active_ports: [], conflicts: [] }; expect(chooseSignInTarget(off, usableDetection())).toBe("container"); - expect(chooseSignInTarget(off, null)).toBe("host"); + // Not plain "host": with the bridge off and no browser inside, nothing is + // carrying the callback, and the toast's hint has to say so rather than + // promising a bridge. That distinction is the whole reason this answer is + // three-valued. + expect(chooseSignInTarget(off, null)).toBe("host-fallback"); // Packages installed, cache empty — the fresh-project state, and the one // that used to be the silent default. expect( @@ -658,13 +662,27 @@ describe("chooseSignInTarget — which action leads for a sign-in link", () => { off, usableDetection({ browsers: [], chromium_executable_exists: false }), ), - ).toBe("host"); + ).toBe("host-fallback"); // Playwright too old to bind: the pane cannot show it either. - expect(chooseSignInTarget(off, usableDetection({ has_bind: false }))).toBe("host"); + expect(chooseSignInTarget(off, usableDetection({ has_bind: false }))).toBe( + "host-fallback", + ); }); - it("answers host when nothing is known at all", () => { - expect(chooseSignInTarget(null, null)).toBe("host"); + it("answers the host *fallback* when nothing is known at all", () => { + // "Unknown" must not read as "bridged". A status call that never answered + // is not evidence that something will carry the callback home. + expect(chooseSignInTarget(null, null)).toBe("host-fallback"); + }); + + it("separates a live bridge from the least-bad answer, though both lead with the host", () => { + const off: AuthBridgeStatus = { enabled: false, active_ports: [], conflicts: [] }; + // The two states the old two-valued answer collapsed together. Folding them + // back into one is what let the toast tell a user with the bridge disabled + // that the bridge would carry their callback. + expect(chooseSignInTarget(LIVE_BRIDGE, null)).not.toBe( + chooseSignInTarget(off, null), + ); }); }); @@ -726,6 +744,26 @@ describe("TerminalView — the sign-in default follows the project", () => { await mountWithPrompt(); expect(primaryLabel()).toBe("Open"); }); + + it("does not promise the auth bridge on a project that has it switched off", async () => { + // The end-to-end version of the three-state answer: bridge off, no browser + // inside. The host still leads, because it is the least bad of two answers + // that can both fail — but the hint must not tell the user the bridge is + // bringing their callback home, because there is no bridge. That hint is + // what sent people to a host browser and a login that hung to its timeout. + await mountWithPrompt(); + const hint = document.querySelector('[data-testid="url-toast-signin-hint"]'); + expect(hint?.textContent).toMatch(/nothing is set up/i); + expect(hint?.textContent).not.toMatch(/what carries the callback/i); + }); + + it("does promise it when the bridge is actually live", async () => { + containerEnv.bridge = LIVE_BRIDGE; + await mountWithPrompt(); + const hint = document.querySelector('[data-testid="url-toast-signin-hint"]'); + expect(hint?.textContent).toMatch(/auth bridge/i); + expect(hint?.textContent).not.toMatch(/nothing is set up/i); + }); }); describe("TerminalView — a host open that fails says so", () => { @@ -794,6 +832,108 @@ describe("TerminalView — a host open that fails says so", () => { }); }); +describe("TerminalView — an open in flight must not blank a newer prompt", () => { + // The window is real and is measured in hundreds of milliseconds, not in + // microtasks: on Linux the opener sleeps `OPENER_GRACE` (400 ms, doubled when + // `xdg-open` fails and `gio` is tried) before resolving. The container is free + // to relay a second URL inside it — a `gh auth login` right after a + // `claude login` is the ordinary way that happens — and the toast slot is + // shared, so by the time the first open answers the slot may be holding a + // prompt the user has never seen. Blanking it loses that URL for good: it + // exists nowhere but the container's transcript. + const URL_A = "https://github.com/login/device?code=AAAA-1111"; + const URL_B = "https://claude.ai/oauth/authorize?code=true&client_id=b"; + + function relaySequence(url: string): number[] { + return Array.from( + new TextEncoder().encode(`\x1b]7777;open;${btoa(url)}\x07`), + ); + } + + async function emitRelay(url: string) { + const emit = ptyOutput.listeners.get("terminal-output-s1"); + if (!emit) throw new Error("no terminal-output listener registered"); + await act(async () => { + emit({ payload: relaySequence(url) }); + await new Promise((r) => setTimeout(r, 0)); + await new Promise((r) => setTimeout(r, 0)); + }); + } + + function openButton(): HTMLElement { + const el = Array.from(document.querySelectorAll("button")).find( + (b) => b.textContent === "Open", + ); + if (!el) throw new Error("Open button not found"); + return el as HTMLElement; + } + + function promptedUrl(): string | null { + return ( + document + .querySelector('[data-testid="url-toast-url"]') + ?.getAttribute("title") ?? null + ); + } + + /** An `openUrlExternal` that hangs until the test lets it finish. */ + function deferredOpen(): () => void { + let finish: () => void = () => {}; + vi.mocked(openUrlExternal).mockReturnValueOnce( + new Promise((resolve) => { + finish = () => resolve(); + }), + ); + return () => finish(); + } + + it("keeps URL B's prompt when A's open resolves after B arrived", async () => { + const finishOpen = deferredOpen(); + mountSession("claude"); + await act(async () => {}); + await emitRelay(URL_A); + + await act(async () => { + fireEvent.click(openButton()); + }); + expect(openUrlExternal).toHaveBeenCalledWith(URL_A); + + // The container supersedes it while the opener is still inside its grace. + await emitRelay(URL_B); + expect(promptedUrl()).toBe(URL_B); + + await act(async () => { + finishOpen(); + await Promise.resolve(); + await Promise.resolve(); + }); + + expect(document.querySelector(URL_TOAST_SELECTOR)).not.toBeNull(); + expect(promptedUrl()).toBe(URL_B); + }); + + it("still dismisses when the slot is holding the prompt that was opened", async () => { + // The other half of the guard: it must not turn "dismiss on success" into + // "never dismiss". Same deferred open, nothing superseding it. + const finishOpen = deferredOpen(); + mountSession("claude"); + await act(async () => {}); + await emitRelay(URL_A); + + await act(async () => { + fireEvent.click(openButton()); + }); + expect(document.querySelector(URL_TOAST_SELECTOR)).not.toBeNull(); + + await act(async () => { + finishOpen(); + await Promise.resolve(); + await Promise.resolve(); + }); + expect(document.querySelector(URL_TOAST_SELECTOR)).toBeNull(); + }); +}); + describe("TerminalView — focus on request", () => { /** Mount, then deliberately give focus away, so what the assertions below * observe is the *request* taking effect and never the focus `active` diff --git a/app/src/components/terminal/TerminalView.tsx b/app/src/components/terminal/TerminalView.tsx index 0ff1f4a..888e1f6 100644 --- a/app/src/components/terminal/TerminalView.tsx +++ b/app/src/components/terminal/TerminalView.tsx @@ -49,6 +49,22 @@ interface Props { */ export type PromptSource = "relay" | UrlSource; +/** + * What the shared prompt slot holds. + * + * `seq` is identity: the slot is one long-lived place that several prompts pass + * through, so "is this still the prompt I acted on?" cannot be answered by the + * URL (the same link can legitimately be relayed twice) and must not be + * answered by "is anything there?". It keys the toast for remounting *and* + * guards the deferred dismissal — see `dismissUrlPromptIfCurrent`. + */ +interface UrlPrompt { + url: string; + label: string; + source: PromptSource; + seq: number; +} + /** Higher wins. Provenance, not recency. */ const SOURCE_RANK: Record = { heuristic: 0, @@ -132,17 +148,22 @@ export default function TerminalView({ sessionId, active }: Props) { // replacing a first would otherwise mutate the toast in place, swapping the // text under a user who is mid-read and mid-click. Keying the toast on it // remounts the component, so a new URL is unmistakably a new prompt. - const [urlPrompt, setUrlPrompt] = useState<{ - url: string; - label: string; - source: PromptSource; - seq: number; - } | null>(null); + const [urlPrompt, setUrlPrompt] = useState(null); const promptSeqRef = useRef(0); const relayLimiterRef = useRef(new RelayRateLimiter()); - // Read by the long-lived keyboard listener below, which is registered once - // and would otherwise close over the prompt as it was at mount. - const urlPromptRef = useRef<{ url: string } | null>(null); + /** + * A mirror of the prompt slot, written *eagerly* by the two functions that + * change it. + * + * Read by the long-lived keyboard listener below, which is registered once + * and would otherwise close over the prompt as it was at mount — and by + * {@link dismissUrlPromptIfCurrent}, which is the reason it is written on the + * spot rather than from an effect. An effect-synced mirror lags the state it + * mirrors by a commit, and the whole question that identity check answers is + * "did a new prompt land while I was awaiting?" — a mirror that has not + * caught up yet answers it wrong in exactly the window that matters. + */ + const urlPromptRef = useRef(null); /** * Empty the prompt slot, and put focus somewhere real if it was inside the @@ -157,10 +178,40 @@ export default function TerminalView({ sessionId, active }: Props) { */ const dismissUrlPrompt = useCallback(() => { const wasInside = !!document.activeElement?.closest(URL_TOAST_SELECTOR); + urlPromptRef.current = null; setUrlPrompt(null); if (wasInside) termRef.current?.focus(); }, []); + /** + * Dismiss, but only if the slot is still holding the prompt the caller + * acted on. + * + * For anything that dismisses *after* awaiting. `openUrlExternal` takes at + * least `OPENER_GRACE` (400 ms, doubled when `xdg-open` fails and `gio` is + * tried) on Linux by construction, and the container can relay a second, + * superseding URL inside that window — at which point the slot has been + * remounted with prompt B and an unconditional `setUrlPrompt(null)` blanks + * it. The user never sees B, and B exists nowhere but the container's + * transcript, which is the exact failure "dismiss on success only" was + * introduced to prevent. + * + * This is a sibling of {@link dismissUrlPrompt} rather than an optional + * `expectedSeq` parameter on it, because `dismissUrlPrompt` is handed + * straight to `onClick`/`onDismiss`: React would call it with a `MouseEvent` + * as its first argument, that event would land in `expectedSeq`, and the ✕ + * button would silently stop dismissing anything. A parameter that is only + * ever correct when nobody passes it by reference is not a safe signature + * here. + */ + const dismissUrlPromptIfCurrent = useCallback( + (seq: number) => { + if (urlPromptRef.current?.seq !== seq) return; + dismissUrlPrompt(); + }, + [dismissUrlPrompt], + ); + /** * The only writer of the prompt slot. Re-validates whatever the caller * found: the OSC relay branch has already been through `parseUrlRelayOsc`, @@ -179,17 +230,19 @@ export default function TerminalView({ sessionId, active }: Props) { console.warn("Refusing to prompt for a URL that failed validation"); return; } - setUrlPrompt((current) => { - if (!supersedes({ url, source }, current)) return current; - promptSeqRef.current += 1; - return { url, label, source, seq: promptSeqRef.current }; - }); + // Read and written through the ref rather than a functional update, so + // the mirror is current the instant this returns. Two prompts arriving in + // one tick still see each other — that is what the ref being the eager + // copy buys — and the seq counter no longer advances inside a state + // updater, which React is free to run twice. + if (!supersedes({ url, source }, urlPromptRef.current)) return; + promptSeqRef.current += 1; + const next: UrlPrompt = { url, label, source, seq: promptSeqRef.current }; + urlPromptRef.current = next; + setUrlPrompt(next); }, [], ); - useEffect(() => { - urlPromptRef.current = urlPrompt; - }, [urlPrompt]); /** * The keyboard route into the toast. @@ -803,11 +856,16 @@ export default function TerminalView({ sessionId, active }: Props) { * * Two things here are ordering, not decoration: * - * - **The toast is dismissed on success only.** It used to go first, so a - * failed open left the user with an empty screen and no way back to a URL - * that only exists in the container's transcript. Now a failure keeps the - * prompt exactly where it was, which also leaves "In container" one click - * away — the fallback this failure is the argument for. + * - **The toast is dismissed on success only, and only if it is still the + * same toast.** Dismissing first is what this replaced: a failed open left + * the user with an empty screen and no way back to a URL that only exists + * in the container's transcript. Now a failure keeps the prompt exactly + * where it was, which also leaves "In container" one click away — the + * fallback this failure is the argument for. Waiting to dismiss opens a + * second window, though: the open is awaited, the container can relay a + * superseding URL while it is in flight, and blanking the slot on success + * would then throw away a prompt the user has never seen. Hence the seq + * check in `dismissUrlPromptIfCurrent` rather than a bare dismissal. * - **The failure is a toast, not a `console.error`.** Same `pushToast` the * container-browser branch below uses, because from the user's side the * two actions fail identically: nothing happens. @@ -829,8 +887,11 @@ export default function TerminalView({ sessionId, active }: Props) { dismissUrlPrompt(); return; } + // The prompt this click was for. Captured before the await, because the + // slot may be holding a different one by the time the opener answers. + const openedSeq = urlPrompt.seq; openUrlExternal(safe) - .then(() => dismissUrlPrompt()) + .then(() => dismissUrlPromptIfCurrent(openedSeq)) .catch((e) => useAppState.getState().pushToast({ kind: "error", @@ -839,7 +900,7 @@ export default function TerminalView({ sessionId, active }: Props) { dedupeKey: "host-open-failed", }), ); - }, [urlPrompt, dismissUrlPrompt]); + }, [urlPrompt, dismissUrlPrompt, dismissUrlPromptIfCurrent]); /** * Which action leads when the prompt is holding an Anthropic sign-in link. @@ -861,6 +922,13 @@ export default function TerminalView({ sessionId, active }: Props) { const handleOpenUrlInContainer = useCallback(() => { if (!urlPrompt) return; const safe = sanitizeRelayUrl(urlPrompt.url); + // Unconditional, and it needs no seq guard, because it happens *before* the + // first await: nothing else can have touched the slot between the click and + // this line. The success and failure reports below are toasts rather than + // this prompt coming back, so there is nothing here that has to survive the + // round trip — which is what makes dismissing up front correct here and + // wrong in `handleOpenUrl`. Anything that moves this dismissal after the + // `openPageInContainerBrowser` call has to take the seq with it. dismissUrlPrompt(); if (!safe) { console.warn("Refusing to open a URL that failed validation"); diff --git a/app/src/components/terminal/UrlToast.test.tsx b/app/src/components/terminal/UrlToast.test.tsx index 60e68d1..514a84e 100644 --- a/app/src/components/terminal/UrlToast.test.tsx +++ b/app/src/components/terminal/UrlToast.test.tsx @@ -199,15 +199,14 @@ describe("UrlToast", () => { ); }); - it("leads with the host when the caller says so, without hiding the other", () => { - // A live auth bridge, or a container with no browser installed. The pair - // is unchanged; only the order and which one is filled. + it("leads with the host, and promises the bridge, when the bridge is live", () => { + // The pair is unchanged; only the order and which one is filled. render( , ); @@ -215,15 +214,46 @@ describe("UrlToast", () => { expect( document.querySelector(URL_TOAST_PRIMARY_SELECTOR), ).toHaveTextContent("Open"); - // Still recognised as a sign-in, so the explanation stays. + // Still recognised as a sign-in, so the explanation stays — and here the + // explanation is true, which is the only state in which it may be given. expect(screen.getByTestId("url-toast-signin-hint")).toHaveTextContent( - /auth bridge/i, + /the auth bridge is what carries the callback/i, ); }); - it("defaults to the host when the caller passes nothing", () => { - // The safe fallback: the answer more likely to work, and the one that - // reports its own failure. + it("says the callback has nothing carrying it when the host is the last resort", () => { + // `host-fallback`: bridge off or unknown *and* no browser in the + // container. The old two-state hint said the auth bridge would carry the + // callback here too, which is a false promise — the user opens the link + // in their own browser and `claude login` hangs to its timeout with + // nothing on screen explaining why. + render( + , + ); + // Which button leads does not change — only what the hint claims. + expect(actions()).toEqual(["Open", "In container"]); + expect( + document.querySelector(URL_TOAST_PRIMARY_SELECTOR), + ).toHaveTextContent("Open"); + const hint = screen.getByTestId("url-toast-signin-hint"); + expect(hint).toHaveTextContent(/nothing is set up to reach it/i); + // And it points at the two things that would fix it, since a warning + // with no next step is only a nicer way to fail. + expect(hint).toHaveTextContent(/Auth bridge/); + expect(hint).toHaveTextContent(/install browser support/i); + expect(hint).not.toHaveTextContent(/the auth bridge is what carries the callback/i); + }); + + it("defaults to the least-bad reading when the caller passes nothing", () => { + // A caller that says nothing has not told us a bridge is live, so the + // hint must not invent one. The host still leads: it is the answer more + // likely to work, and the one that reports its own failure. render( { />, ); expect(actions()).toEqual(["Open", "In container"]); + expect(screen.getByTestId("url-toast-signin-hint")).toHaveTextContent( + /nothing is set up to reach it/i, + ); }); it("keeps the host browser available as a fallback", () => { diff --git a/app/src/components/terminal/UrlToast.tsx b/app/src/components/terminal/UrlToast.tsx index 148c723..3a440e4 100644 --- a/app/src/components/terminal/UrlToast.tsx +++ b/app/src/components/terminal/UrlToast.tsx @@ -1,5 +1,6 @@ import type { KeyboardEvent } from "react"; import { isAnthropicSignInUrl, urlOrigin } from "../../lib/urlRelay"; +import type { SignInOpenTarget } from "../../hooks/useSignInOpenTarget"; import Button from "../ui/Button"; /** @@ -38,17 +39,23 @@ interface Props { * the project has no browser to open it in. */ onOpenInContainer?: () => void; /** - * Which action leads for a *sign-in* link (see the note below). Nothing else - * in the toast moves: both buttons are offered either way, in either order. + * Which action leads for a *sign-in* link, and why (see the note below). + * Nothing else in the toast moves: both buttons are offered in all three + * states, in one of two orders. * * This component does not work it out, because the answer depends on the * project's auth bridge and on what is installed inside its container — * neither of which a presentational component should be reaching for. - * `hooks/useSignInOpenTarget.ts` owns the rule. `"host"` is the default here - * for the same reason it is the fallback there: it is the answer that is more - * likely to work, and the one that reports its own failure. + * `hooks/useSignInOpenTarget.ts` owns the rule. + * + * Two of the three lead with the host button and differ only in the hint, + * which is the whole point of carrying three: `"host-bridged"` may promise + * that the auth bridge brings the callback home, `"host-fallback"` may not, + * because in that state nothing does. `"host-fallback"` is the default for + * that reason — a caller that says nothing has not told us a bridge is live, + * and the hint must not invent one. */ - signInDefault?: "host" | "container"; + signInDefault?: SignInOpenTarget; onDismiss: () => void; } @@ -84,6 +91,12 @@ interface Props { * passes {@link Props.signInDefault} and this only renders it: the leading * button is filled and comes first, the other keeps its place beside it. * + * The hint below the URL renders all *three* states, not the two orderings. + * "Neither is set up" also leads with the host, but it is not the same claim: + * there the callback has nothing carrying it, so the hint names what would fix + * that instead of describing a bridge that is off. A two-way hint keyed on + * which button leads is exactly how that false promise got shipped. + * * ## Reachable without a mouse, and it does not take focus to manage it * * This toast is the only route to completing a sign-in started in a terminal, @@ -111,7 +124,7 @@ export default function UrlToast({ label = "Long URL detected", onOpen, onOpenInContainer, - signInDefault = "host", + signInDefault = "host-fallback", onDismiss, }: Props) { const origin = urlOrigin(url); @@ -123,6 +136,11 @@ export default function UrlToast({ // container. Everything below keys off this rather than off `signIn`, so the // two orderings differ only in which of the pair leads. const containerLeads = signIn && signInDefault === "container"; + // The third state. Both host states put the same button first, so this is + // read by the hint alone: no bridge and no container browser means nothing is + // carrying the callback back, and saying "the auth bridge is what carries it" + // here is a promise the project cannot keep. + const hostIsLastResort = signIn && signInDefault === "host-fallback"; // `Button` already owns the filled/outlined variants — including the rule // that filled uses `--accent-emphasis` and never `--accent`, which is the @@ -257,7 +275,9 @@ export default function UrlToast({ > {containerLeads ? "Sign-in link — the callback listener is inside the container. Opening it there closes the loop; the host browser needs the auth bridge." - : "Sign-in link — the callback listener is inside the container. The auth bridge is what carries the callback back to it from your own browser."} + : hostIsLastResort + ? "Sign-in link — the callback listener is inside the container and nothing is set up to reach it. Turn on Auth bridge in the project’s Config tab, or install browser support to sign in inside the container." + : "Sign-in link — the callback listener is inside the container. The auth bridge is what carries the callback back to it from your own browser."} )} diff --git a/app/src/hooks/useSignInOpenTarget.ts b/app/src/hooks/useSignInOpenTarget.ts index f863c87..ffb2bdc 100644 --- a/app/src/hooks/useSignInOpenTarget.ts +++ b/app/src/hooks/useSignInOpenTarget.ts @@ -14,8 +14,28 @@ import type { /** Emitted by `auth_bridge/mod.rs` whenever the port or conflict set changes. */ const AUTH_BRIDGE_EVENT = "auth-bridge-changed"; -/** Which of the URL toast's two buttons should lead for a sign-in link. */ -export type SignInOpenTarget = "host" | "container"; +/** + * Which of the URL toast's two buttons should lead for a sign-in link — and, + * for the host, *why*. + * + * Three states rather than two because "host" covers two worlds that are not + * the same promise to the user: + * + * - `host-bridged` — the auth bridge is live, so a sign-in completed in the + * user's own browser has its callback carried back to the listener inside + * the container. The host is genuinely the better answer here. + * - `container` — no bridge, but the container has a browser to open, which + * closes the loop locally with nothing crossing to the host. + * - `host-fallback` — neither. The host is the *least bad* of two answers + * that can both fail, and the toast has to say so: a hint claiming the + * bridge will carry the callback is a false promise in this state, and the + * user's `claude login` hangs to its timeout with nothing explaining why. + * + * Only `container` changes which button leads; the split between the two host + * states exists so the toast's hint can tell the truth. Keep it that way — the + * consumer that folds them back together is the bug this replaced. + */ +export type SignInOpenTarget = "host-bridged" | "container" | "host-fallback"; /** * Whether the auth bridge can be relied on to catch a callback for this @@ -42,16 +62,19 @@ export function authBridgeIsLive(status: AuthBridgeStatus | null): boolean { /** * The rule, as a pure function of the two things it depends on. * - * Both fallbacks land on the host, for different reasons: + * Both host answers land on the same button, for different reasons — and they + * are deliberately *not* the same value: * - * - With the bridge live, the host browser is strictly better — it is the - * user's own signed-in profile, and the callback still reaches the container. - * - With neither available, the host is the *more likely to work* of two - * imperfect answers, and it is the one that reports its own failure (see - * `handleOpenUrl` in `TerminalView`). The container-side target is - * Playwright's dashboard pane, and Playwright's browsers are not baked into - * the image, so on a fresh project pointing there fails on every platform - * after a several-second wait. + * - With the bridge live (`host-bridged`), the host browser is strictly + * better — it is the user's own signed-in profile, and the callback still + * reaches the container. + * - With neither available (`host-fallback`), the host is the *more likely to + * work* of two imperfect answers, and it is the one that reports its own + * failure (see `handleOpenUrl` in `TerminalView`). The container-side target + * is Playwright's dashboard pane, and Playwright's browsers are not baked + * into the image, so on a fresh project pointing there fails on every + * platform after a several-second wait. Nothing carries the callback back in + * this state, so the toast says so rather than promising the bridge. * * Whichever way it goes, both buttons stay in the toast. This chooses which one * leads, never which ones exist. @@ -60,9 +83,9 @@ export function chooseSignInTarget( bridge: AuthBridgeStatus | null, detection: PlaywrightDetection | null, ): SignInOpenTarget { - if (authBridgeIsLive(bridge)) return "host"; + if (authBridgeIsLive(bridge)) return "host-bridged"; if (canOpenPageInContainerBrowser(detection)) return "container"; - return "host"; + return "host-fallback"; } /** @@ -113,11 +136,16 @@ export function resetBrowserSupportCache(): void { * bridge now on by default, is the ordinary case. */ export function useSignInOpenTarget(projectId: string | undefined): SignInOpenTarget { - const [target, setTarget] = useState("host"); + // `host-fallback` is the honest starting point, not `host-bridged`: before + // the status call answers, nothing is known to be carrying the callback, and + // the hint that claims one is the failure this three-state answer exists to + // prevent. Over-warning for the moment before the answer arrives costs a line + // of hedged text; under-warning costs a login that hangs to its timeout. + const [target, setTarget] = useState("host-fallback"); useEffect(() => { if (!projectId) { - setTarget("host"); + setTarget("host-fallback"); return; } @@ -145,8 +173,10 @@ export function useSignInOpenTarget(projectId: string | undefined): SignInOpenTa .then((s) => { if (!cancelled) consider(s); }) - // Nothing to say to the user here: this only picks which button is - // filled in, and the fallback is the one that reports its own failures. + // Nothing to say to the user here: an unanswered status call is fed + // through as a bridge that is off, which lands on `container` or + // `host-fallback` — and `host-fallback`'s hint is the one that tells the + // user the callback has nothing carrying it. .catch(() => { if (!cancelled) consider({ enabled: false, active_ports: [], conflicts: [] }); });