From bf8094dbc47f972b8d1cd3cf2ac3088489ff0161 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Thu, 17 Sep 2026 10:07:50 -0700 Subject: [PATCH] fix: route sign-in links by what can actually catch the callback `isAnthropicSignInUrl` made the container the default action for every Anthropic sign-in link, justified by "the host has nothing to catch it with". That was wrong in both directions. The host does have something -- the auth bridge -- and the container side is not a general browser at all but Playwright's dashboard, whose packages and chromium are deliberately not baked into the image. So the default pointed at the one path that is uninstalled on a fresh project, on every platform, while the path that works sat behind a switch. The decision now lives in `useSignInOpenTarget`: a live auth bridge picks the host, otherwise a container that can actually launch a browser picks the container, otherwise the host. It resolves at mount rather than when a URL arrives, so the buttons do not swap under a moving mouse, and it re-decides on `auth-bridge-changed` so flipping the switch during a hanging login takes effect. A bridge with port conflicts reads as not live; an empty `active_ports` does not, since there is nothing to bridge until the CLI binds its listener and that races the URL. Both buttons still render either way -- this changes which one leads. `sanitizeRelayUrl` is byte-for-byte unchanged, so the embedded copy in web_terminal/terminal.html needs no matching edit. The host "Open" path also failed silently: `dismissUrlPrompt()` ran before `openUrl`, so the toast vanished and a rejected promise reached only the devtools console. Dismissal now happens on success only, leaving "In container" one click away after a failure, and the error surfaces through the same toast the container path already used. On Linux this catch will not fire for the common case -- `xdg-open` routinely exits 0 having done nothing -- so it complements the AppImage environment fix rather than replacing it. Co-Authored-By: Claude Opus 5 (1M context) --- .../components/projects/home/BrowserTab.tsx | 10 +- .../components/terminal/TerminalView.test.tsx | 236 ++++++++++++++++++ app/src/components/terminal/TerminalView.tsx | 57 ++++- app/src/components/terminal/UrlToast.test.tsx | 56 ++++- app/src/components/terminal/UrlToast.tsx | 56 +++-- app/src/hooks/useSignInOpenTarget.ts | 176 +++++++++++++ app/src/lib/browserViewSupport.ts | 53 ++++ app/src/lib/urlRelay.test.ts | 51 ++++ app/src/lib/urlRelay.ts | 21 +- 9 files changed, 677 insertions(+), 39 deletions(-) create mode 100644 app/src/hooks/useSignInOpenTarget.ts create mode 100644 app/src/lib/browserViewSupport.ts diff --git a/app/src/components/projects/home/BrowserTab.tsx b/app/src/components/projects/home/BrowserTab.tsx index 8e5aac3..1fe5a80 100644 --- a/app/src/components/projects/home/BrowserTab.tsx +++ b/app/src/components/projects/home/BrowserTab.tsx @@ -23,6 +23,7 @@ import { setBrowserViewMatchWindow, setBrowserViewPopoutAlwaysOnTop, } from "../../../lib/tauri-commands"; +import { isBrowserViewUsable } from "../../../lib/browserViewSupport"; import { useAppState } from "../../../store/appState"; import OpenPageDialog from "./OpenPageDialog"; import AccordionSection from "../../ui/AccordionSection"; @@ -338,7 +339,7 @@ export default function BrowserTab({ project, active }: Props) { // Prefer the probe: it is the fresher of the two, and it is the one that // reflects an install that just finished. const probed = detection ?? status.detection; - const ready = isUsable(probed); + const ready = isBrowserViewUsable(probed); // Mirrors Rust `PlaywrightDetection::needs_browser`: the Chrome channel is an // apt package, so it never shows up in `browsers`, and a container that has // it is not missing a browser. @@ -539,11 +540,6 @@ export default function BrowserTab({ project, active }: Props) { ); } -/** Mirrors Rust `PlaywrightDetection::is_usable`. */ -function isUsable(d: PlaywrightDetection | null): boolean { - return d !== null && d.playwright_version !== null && d.has_bind && d.cli_entry !== null; -} - /** * Mirrors Rust `PlaywrightDetection::revision_skew`. * @@ -627,7 +623,7 @@ function Setup({ onInstall: (which: Exclude) => void; }) { const busy = job !== null; - const havePackages = isUsable(detection); + const havePackages = isBrowserViewUsable(detection); const missing = missingParts(detection); const browsers = detection?.browsers ?? []; const chrome = detection?.chrome_channel ?? null; diff --git a/app/src/components/terminal/TerminalView.test.tsx b/app/src/components/terminal/TerminalView.test.tsx index 5f71ad0..093d471 100644 --- a/app/src/components/terminal/TerminalView.test.tsx +++ b/app/src/components/terminal/TerminalView.test.tsx @@ -3,6 +3,12 @@ import { render, fireEvent, cleanup, act } from "@testing-library/react"; import TerminalView, { supersedes } from "./TerminalView"; import { useAppState } from "../../store/appState"; import { uploadHostFileToTerminal } from "../../lib/tauri-commands"; +import { openUrl } from "@tauri-apps/plugin-opener"; +import { + chooseSignInTarget, + resetBrowserSupportCache, +} from "../../hooks/useSignInOpenTarget"; +import type { AuthBridgeStatus, PlaywrightDetection } from "../../lib/types"; import { URL_TOAST_SELECTOR } from "./UrlToast"; /** @@ -16,6 +22,18 @@ const dragDrop = vi.hoisted(() => ({ handler: null as null | ((event: unknown) => unknown), })); +/** + * What the project's container answers about itself. + * + * `TerminalView` asks two questions on mount — is the auth bridge live, and is + * there a browser inside to open a page in — because together they decide which + * of the URL toast's two buttons leads for a sign-in link. + */ +const containerEnv = vi.hoisted(() => ({ + bridge: { enabled: false, active_ports: [], conflicts: [] } as unknown, + detection: null as unknown, +})); + /** The `terminal-output-{id}` listeners, so a test can be the PTY. */ const ptyOutput = vi.hoisted(() => ({ listeners: new Map void>(), @@ -45,6 +63,8 @@ vi.mock("../../lib/tauri-commands", () => ({ awsSsoRefresh: vi.fn(async () => {}), openPageInContainerBrowser: vi.fn(async () => ({ error: null })), uploadHostFileToTerminal: vi.fn(async () => ""), + getAuthBridgeStatus: vi.fn(async () => containerEnv.bridge), + checkBrowserViewSupport: vi.fn(async () => containerEnv.detection), })); vi.mock("@tauri-apps/api/event", () => ({ @@ -128,6 +148,14 @@ beforeEach(() => { vi.mocked(uploadHostFileToTerminal).mockResolvedValue("/workspace/api/dropped.txt"); dragDrop.handler = null; ptyOutput.listeners.clear(); + vi.mocked(openUrl).mockReset(); + vi.mocked(openUrl).mockResolvedValue(undefined); + containerEnv.bridge = { enabled: false, active_ports: [], conflicts: [] }; + containerEnv.detection = null; + // The Playwright probe is memoized across mounts (it is a container exec), so + // a case that changes the answer has to drop what an earlier one cached. + resetBrowserSupportCache(); + useAppState.setState({ toasts: [] }); document.body.innerHTML = ""; useAppState.setState({ sessions: [] }); }); @@ -559,6 +587,214 @@ describe("TerminalView — reaching the URL prompt without a mouse", () => { }); }); +/** + * A container with Playwright *and* a browser in the cache — i.e. one where + * "In container" would actually open something. + */ +function usableDetection( + over: Partial = {}, +): PlaywrightDetection { + return { + node_version: "v22.11.0", + playwright_version: "1.56.0", + playwright_path: "/workspace/node_modules/playwright", + playwright_cli: "/workspace/node_modules/playwright/cli.js", + has_bind: true, + cli_version: "1.56.0", + cli_entry: "/workspace/node_modules/@playwright/cli/index.js", + browsers: ["chromium-1200"], + chrome_channel: null, + chromium_executable: "/home/claude/.cache/ms-playwright/chromium-1200/chrome", + chromium_executable_exists: true, + script_playwright_version: "1.56.0", + script_chromium_executable: null, + script_chromium_executable_exists: false, + searched: [], + ...over, + }; +} + +const LIVE_BRIDGE: AuthBridgeStatus = { + enabled: true, + active_ports: [], + conflicts: [], +}; + +describe("chooseSignInTarget — which action leads for a sign-in link", () => { + // The rule this replaced was "container, always", justified by the callback + // listener living inside the container. Both halves of that justification + // stopped being true: the auth bridge mirrors that listener onto the host, + // 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"); + }); + + it("does not call a bridge live while it is holding a port conflict", () => { + // Enabled and unable to catch the callback anyway — the one state where + // "on" must not read as "will work". + const conflicted: AuthBridgeStatus = { + enabled: true, + active_ports: [], + conflicts: [{ port: 54545, reason: "already in use on the host" }], + }; + expect(chooseSignInTarget(conflicted, usableDetection())).toBe("container"); + }); + + it("does not wait for a bridged port before trusting an enabled bridge", () => { + // 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"); + }); + + 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"); + // Packages installed, cache empty — the fresh-project state, and the one + // that used to be the silent default. + expect( + chooseSignInTarget( + off, + usableDetection({ browsers: [], chromium_executable_exists: false }), + ), + ).toBe("host"); + // Playwright too old to bind: the pane cannot show it either. + expect(chooseSignInTarget(off, usableDetection({ has_bind: false }))).toBe("host"); + }); + + it("answers host when nothing is known at all", () => { + expect(chooseSignInTarget(null, null)).toBe("host"); + }); +}); + +describe("TerminalView — the sign-in default follows the project", () => { + const SIGN_IN = + "https://claude.ai/oauth/authorize?code=true&client_id=abc&response_type=code"; + + function relaySequence(url: string): number[] { + return Array.from( + new TextEncoder().encode(`\x1b]7777;open;${btoa(url)}\x07`), + ); + } + + async function mountWithPrompt() { + const view = mountSession("claude"); + await act(async () => {}); + const emit = ptyOutput.listeners.get("terminal-output-s1"); + if (!emit) throw new Error("no terminal-output listener registered"); + await act(async () => { + emit({ payload: relaySequence(SIGN_IN) }); + await new Promise((r) => setTimeout(r, 0)); + await new Promise((r) => setTimeout(r, 0)); + }); + return view; + } + + function primaryLabel(): string | null { + return document.querySelector( + '[data-url-toast-primary="true"]', + )?.textContent ?? null; + } + + function actionOrder(): (string | null)[] { + return Array.from(document.querySelectorAll("button")) + .map((b) => b.textContent) + .filter((t) => t === "Open" || t === "In container"); + } + + it("leads with the host browser when the auth bridge is on", async () => { + containerEnv.bridge = LIVE_BRIDGE; + containerEnv.detection = usableDetection(); + await mountWithPrompt(); + expect(primaryLabel()).toBe("Open"); + // Both are still offered — this changes which leads, never which exist. + expect(actionOrder()).toEqual(["Open", "In container"]); + }); + + it("leads with the container when the bridge is off and a browser is there", async () => { + containerEnv.detection = usableDetection(); + await mountWithPrompt(); + expect(primaryLabel()).toBe("In container"); + expect(actionOrder()).toEqual(["In container", "Open"]); + }); + + it("leads with the host on a fresh project, where neither is set up", async () => { + // Playwright is deliberately not baked into the image, so this is what a + // project looks like until someone presses install — and pointing the + // default at it failed on every platform, silently. + await mountWithPrompt(); + expect(primaryLabel()).toBe("Open"); + }); +}); + +describe("TerminalView — a host open that fails says so", () => { + const URL = "https://github.com/login/device?code=ABCD-EFGH"; + + function relaySequence(url: string): number[] { + return Array.from( + new TextEncoder().encode(`\x1b]7777;open;${btoa(url)}\x07`), + ); + } + + async function mountWithPrompt() { + const view = mountSession("claude"); + await act(async () => {}); + 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)); + }); + return view; + } + + 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; + } + + it("pushes a toast instead of a console line nobody reads", async () => { + vi.mocked(openUrl).mockRejectedValueOnce(new Error("no opener")); + await mountWithPrompt(); + await act(async () => { + fireEvent.click(openButton()); + await Promise.resolve(); + }); + const toasts = useAppState.getState().toasts; + expect(toasts).toHaveLength(1); + expect(toasts[0].kind).toBe("error"); + expect(toasts[0].detail).toContain("no opener"); + }); + + it("keeps the prompt on screen, so the other route is still one click away", async () => { + // Dismissing first is what this replaced: the toast vanished, nothing + // opened, and the URL only existed in the container's transcript. + vi.mocked(openUrl).mockRejectedValueOnce(new Error("no opener")); + await mountWithPrompt(); + await act(async () => { + fireEvent.click(openButton()); + await Promise.resolve(); + }); + expect(document.querySelector(URL_TOAST_SELECTOR)).not.toBeNull(); + }); + + it("dismisses the prompt once the handoff actually succeeded", async () => { + await mountWithPrompt(); + await act(async () => { + fireEvent.click(openButton()); + await Promise.resolve(); + }); + expect(openUrl).toHaveBeenCalledWith(URL); + 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 ff17972..7f5cba2 100644 --- a/app/src/components/terminal/TerminalView.tsx +++ b/app/src/components/terminal/TerminalView.tsx @@ -23,6 +23,7 @@ import { sanitizeRelayUrl, } from "../../lib/urlRelay"; import { classifyDrop, DROP_BLOCKED_TOAST } from "../../lib/dropTarget"; +import { useSignInOpenTarget } from "../../hooks/useSignInOpenTarget"; import UrlToast, { URL_TOAST_PRIMARY_SELECTOR, URL_TOAST_SELECTOR, @@ -409,7 +410,18 @@ export default function TerminalView({ sessionId, active }: Props) { console.warn("Refusing to open a link that failed validation"); return; } - openUrl(safe).catch((e) => console.error("Failed to open URL:", e)); + // Same failure reporting as the toast's Open button — see the long note + // on `handleOpenUrl`, including what this catch does *not* catch on + // Linux. A click that appears to do nothing is the complaint either way. + openUrl(safe).catch((e) => + useAppState.getState().pushToast({ + kind: "error", + message: "Could not open that link in your browser", + detail: String(e), + // A dead opener fails for every link in the buffer. One card. + dedupeKey: "host-open-failed", + }), + ); }, { urlRegex }); term.loadAddon(webLinksAddon); @@ -786,20 +798,58 @@ export default function TerminalView({ sessionId, active }: Props) { return () => clearTimeout(timer); }, [imagePasteMsg]); + /** + * Hand the prompted URL to the host's browser. + * + * 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 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. + * + * What this does *not* cover, and must not be described as covering: on Linux + * `xdg-open` routinely exits 0 having done nothing useful, so the most common + * Linux failure resolves this promise and reports success. Stripping the + * leaked AppImage environment before the browser is spawned is what addresses + * that; this is the complement that catches everything which does report. + */ const handleOpenUrl = useCallback(() => { if (!urlPrompt) return; // Validated again at the sink. `promptUrl` is the only writer and already // sanitizes, so this can only fail if that invariant is broken — which is // precisely when it matters that the last thing before `openUrl` checks. const safe = sanitizeRelayUrl(urlPrompt.url); - dismissUrlPrompt(); if (!safe) { console.warn("Refusing to open a URL that failed validation"); + dismissUrlPrompt(); return; } - openUrl(safe).catch((e) => console.error("Failed to open URL:", e)); + openUrl(safe) + .then(() => dismissUrlPrompt()) + .catch((e) => + useAppState.getState().pushToast({ + kind: "error", + message: "Could not open it in your browser", + detail: String(e), + dedupeKey: "host-open-failed", + }), + ); }, [urlPrompt, dismissUrlPrompt]); + /** + * Which action leads when the prompt is holding an Anthropic sign-in link. + * + * Resolved per project, not per URL — see `useSignInOpenTarget`. The toast + * offers both regardless; this is only which one is filled in and reachable + * with {@link URL_TOAST_SHORTCUT}. + */ + const signInDefault = useSignInOpenTarget(projectId); + /** * Open the prompted URL in the container's own browser instead of the host's. * @@ -896,6 +946,7 @@ export default function TerminalView({ sessionId, active }: Props) { label={urlPrompt.label} onOpen={handleOpenUrl} onOpenInContainer={handleOpenUrlInContainer} + signInDefault={signInDefault} onDismiss={dismissUrlPrompt} /> )} diff --git a/app/src/components/terminal/UrlToast.test.tsx b/app/src/components/terminal/UrlToast.test.tsx index ad705d0..60e68d1 100644 --- a/app/src/components/terminal/UrlToast.test.tsx +++ b/app/src/components/terminal/UrlToast.test.tsx @@ -105,6 +105,7 @@ describe("UrlToast", () => { url={SIGN_IN} onOpen={noop} onOpenInContainer={noop} + signInDefault="container" onDismiss={noop} />, ); @@ -150,6 +151,7 @@ describe("UrlToast", () => { url={SIGN_IN} onOpen={noop} onOpenInContainer={noop} + signInDefault="container" onDismiss={noop} />, ); @@ -166,10 +168,11 @@ describe("UrlToast", () => { describe("Anthropic sign-in links", () => { // The callback listener a `claude login` is waiting on is *inside* the - // container. Sending the user to their host browser completes the sign-in - // and then posts the result where nothing is listening, and the terminal - // hangs to its timeout — so for these, and only these, the container-side - // browser leads. + // container, so a sign-in is the one case where the host browser may be the + // wrong lead. Whether it actually is depends on the project — a live auth + // bridge carries the callback back, and the container-side alternative is + // not installed on a fresh project — so the owner decides and passes + // `signInDefault`. This component only renders the decision. const SIGN_IN = "https://claude.ai/oauth/authorize?code=true&client_id=abc&response_type=code"; @@ -180,12 +183,13 @@ describe("UrlToast", () => { .filter((t) => t === "Open" || t === "In container"); } - it("puts the container browser first", () => { + it("puts the container browser first when the caller asks for it", () => { render( , ); @@ -195,6 +199,42 @@ 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. + render( + , + ); + expect(actions()).toEqual(["Open", "In container"]); + expect( + document.querySelector(URL_TOAST_PRIMARY_SELECTOR), + ).toHaveTextContent("Open"); + // Still recognised as a sign-in, so the explanation stays. + expect(screen.getByTestId("url-toast-signin-hint")).toHaveTextContent( + /auth bridge/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. + render( + , + ); + expect(actions()).toEqual(["Open", "In container"]); + }); + it("keeps the host browser available as a fallback", () => { const onOpen = vi.fn(); render( @@ -202,6 +242,7 @@ describe("UrlToast", () => { url={SIGN_IN} onOpen={onOpen} onOpenInContainer={noop} + signInDefault="container" onDismiss={noop} />, ); @@ -211,12 +252,14 @@ describe("UrlToast", () => { it("leaves an ordinary URL alone", () => { // A `gh auth login` device code, a docs page, a preview build — the host - // browser is the right answer for all of them and stays the default. + // browser is the right answer for all of them and stays the default, + // whatever the project's sign-in preference happens to be. render( , ); @@ -232,6 +275,7 @@ describe("UrlToast", () => { url="https://claude.ai.evil.tld/oauth/authorize?x=1" onOpen={noop} onOpenInContainer={noop} + signInDefault="container" onDismiss={noop} />, ); diff --git a/app/src/components/terminal/UrlToast.tsx b/app/src/components/terminal/UrlToast.tsx index ddad469..148c723 100644 --- a/app/src/components/terminal/UrlToast.tsx +++ b/app/src/components/terminal/UrlToast.tsx @@ -37,6 +37,18 @@ interface Props { /** Open it in the container's own browser instead of the host's. Omitted when * 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. + * + * 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. + */ + signInDefault?: "host" | "container"; onDismiss: () => void; } @@ -57,17 +69,20 @@ interface Props { * text swaps with no animation, and a user reading URL A can click Open on URL * B that arrived a second later. * - * ## Anthropic sign-in links default to the container's browser + * ## Anthropic sign-in links get their default from the caller * * For an ordinary URL the host browser is the right answer and stays the - * default. For a sign-in it is the *wrong* one: the callback listener the CLI - * is waiting on is inside the container, so a host browser completes the sign-in - * and then posts the result somewhere nothing is listening, and the terminal - * hangs until it times out. Making the host button primary there was quietly - * steering every user into that. The container-side browser closes the loop - * with no host round trip and no auth bridge, so it leads — and the host button - * stays, because a user who has the auth bridge on, or who wants their existing - * browser session, still needs it. + * default, unconditionally. A sign-in is the one case where it might not be: + * the callback listener the CLI is waiting on is inside the container, so a + * host browser can complete the sign-in and then post the result where nothing + * is listening, leaving the terminal to hang to its timeout. + * + * *Can*, not *does* — which is why this is no longer decided from the URL. The + * auth bridge mirrors that container listener onto the same host port, and the + * container-side alternative is Playwright's dashboard pane, which a fresh + * project has not installed. Both of those are project facts, so the owner + * passes {@link Props.signInDefault} and this only renders it: the leading + * button is filled and comes first, the other keeps its place beside it. * * ## Reachable without a mouse, and it does not take focus to manage it * @@ -96,6 +111,7 @@ export default function UrlToast({ label = "Long URL detected", onOpen, onOpenInContainer, + signInDefault = "host", onDismiss, }: Props) { const origin = urlOrigin(url); @@ -103,18 +119,22 @@ export default function UrlToast({ // Only when there is somewhere to send it: without `onOpenInContainer` the // host button is the only action there is, so it stays primary. const signIn = !!onOpenInContainer && isAnthropicSignInUrl(url); + // A sign-in link the caller has decided is better completed inside the + // 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"; // `Button` already owns the filled/outlined variants — including the rule // that filled uses `--accent-emphasis` and never `--accent`, which is the // foreground/link accent and fails WCAG AA behind white text. const hostButton = (