CyberCoveLLC/Triple-C
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 44 Wiki Activity

fix: do not let an in-flight open blank a newer prompt, or promise a bridge that is off

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
shadowdao
2026-09-17 11:12:22 -07:00
co-authored by Claude Opus 5
parent db648230ee
commit 60188610ee
5 changed files with 356 additions and 65 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/src/components/terminal/TerminalView.test.tsx
+147 -7
View File
@@ -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<void>((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`