Compare commits

...
Author SHA1 Message Date
shadowdaoandClaude Opus 5 943c83b9e3 fix: stop a stale payload re-enabling a bridge the user turned off
Secret Scan / scan (push) Successful in 3s
Build App (Preview) / compute-version (pull_request) Successful in 7s
Secret Scan / scan (pull_request) Successful in 5s
Build App (Preview) / create-release (pull_request) Successful in 3s
Build App (Preview) / build-macos (pull_request) Successful in 2m48s
Build App (Preview) / build-windows (pull_request) Successful in 4m55s
Build App (Preview) / build-linux (pull_request) Successful in 8m39s
Build App (Preview) / prune-previews (pull_request) Successful in 2s
Review of this branch found that `update_project` restored
`browser_view_enabled` from the store but took `auth_bridge_enabled` from
the IPC payload, on a comment claiming the Config tab edits it through that
save. The comment was wrong. `AuthBridgeRow` is the only writer, it calls
`set_auth_bridge_enabled` out of band precisely so the switch works while a
login is hanging, and it never writes the value back into frontend state --
so a payload's copy of that flag is always a stale snapshot.

The consequence was not cosmetic: turn the bridge off, then close a renamed
terminal tab, and `useTerminal` round-trips the stale `true` and the
reconcile block restarts a bridge whose own UI warns that a bridged port is
unauthenticated and reachable by any local process. Defaulting the flag to
true earlier in this branch made it worse, since the stale value is now
true for every pre-existing project.

Both flags are now restored from the store by `restore_store_owned_fields`,
and the reconcile block is gone rather than corrected: with the value
always restored it could only re-assert what was already true, and every
writer already owns its own side effect -- the setter starts and stops
synchronously, container start arms the bridge, launch reconcile re-arms
it, and the poller re-reads the flag each tick and self-terminates.
Re-adding a start path to the one function that no longer owns the flag is
what caused this.

Turning the browser view off also stopped tearing the session down when the
project record had vanished, because the persist used `?` and returned
early -- the supervisor's own `store.get()` check exists because records do
vanish mid-session. Teardown is now unconditional and the write error still
surfaces afterwards, since the stored flag saying "enabled" means the view
returns on next launch and that is worth reporting.

Finally, the opener no longer falls through to `gio` on any non-zero exit.
xdg-open's 1, 2 and 3 assert no handler ran; 4 also covers a handler that
was launched and then failed, which would have opened the link twice --
two authorize requests for one click in an OAuth flow. Reasoned from
documented exit codes rather than an observed double-open, and the cost is
stated: a genuine code-4 failure no longer reaches gio.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-17 11:13:57 -07:00
shadowdaoandClaude Opus 5 60188610ee 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) <noreply@anthropic.com>
2026-09-17 11:12:22 -07:00
8 changed files with 653 additions and 106 deletions
+74 -3
View File
@@ -32,11 +32,28 @@ pub async fn set_browser_view_enabled(
// Persist first, then tear down: the supervisor's own teardown emit
// reads this flag back out of the store, and reading it mid-stop would
// announce a view that is going away as still enabled.
state
//
// But the write's outcome is a *value*, not a branch. A `?` here meant
// that a store with no such project record returned early and
// `manager().stop()` never ran, leaving the supervisor, the proxy and
// the host port up for a project that, as far as the user is concerned,
// just had its view switched off. That state is not hypothetical while
// a session is live — the supervisor's own `store.get()` check in
// [`crate::browser_view`] exists because a record can go away
// underneath it — and before the flag was persisted at all, turning the
// view off always tore the session down.
let persisted = state
.projects_store
.set_browser_view_enabled(&project_id, false)?;
.set_browser_view_enabled(&project_id, false);
// Awaits the supervisor, so the host port is released before we return.
manager().stop(&project_id).await;
//
// A failed write is still reported rather than logged and swallowed.
// The resources are gone either way by this point, so surfacing it
// costs nothing that matters, and the failure it describes is one the
// user needs: the stored flag still says *enabled*, so the view comes
// back by itself on the next launch. Returning `Ok` would be a claim
// about persistence that isn't true.
tear_down_then_report(persisted, manager().stop(&project_id)).await?;
return Ok(manager().status(&project_id, false).await);
}
@@ -52,6 +69,20 @@ pub async fn set_browser_view_enabled(
.await
}
/// Await `teardown`, then report `persisted`.
///
/// Trivial on purpose, and split out for one reason: it is the whole rule the
/// disable path of [`set_browser_view_enabled`] has to obey — the teardown is
/// unconditional, and a failed persist surfaces only after it has run — and as
/// a free function that rule can be tested without a live `AppState`.
async fn tear_down_then_report(
persisted: Result<(), String>,
teardown: impl std::future::Future<Output = ()>,
) -> Result<(), String> {
teardown.await;
persisted
}
/// Current status. Cheap: the session map in this process plus the stored flag,
/// never the container.
///
@@ -383,3 +414,43 @@ async fn running_container(
}
Ok(container_id)
}
#[cfg(test)]
mod tests {
use super::*;
use std::sync::atomic::{AtomicBool, Ordering};
/// The regression: turning the view off must not leave the supervisor, the
/// proxy and the host port running just because the project record could
/// not be written — which is exactly what a missing record did.
#[tokio::test]
async fn a_failed_persist_does_not_skip_the_teardown() {
let torn_down = AtomicBool::new(false);
let result = tear_down_then_report(Err("Project x not found".to_string()), async {
torn_down.store(true, Ordering::SeqCst);
})
.await;
assert!(
torn_down.load(Ordering::SeqCst),
"the session must be torn down even when the store write failed"
);
assert_eq!(
result.err().as_deref(),
Some("Project x not found"),
"and the write failure must still reach the caller, not be swallowed"
);
}
#[tokio::test]
async fn a_successful_persist_reports_success_after_the_teardown() {
let torn_down = AtomicBool::new(false);
let result = tear_down_then_report(Ok(()), async {
torn_down.store(true, Ordering::SeqCst);
})
.await;
assert!(torn_down.load(Ordering::SeqCst));
assert!(result.is_ok());
}
}
+131 -36
View File
@@ -1036,7 +1036,6 @@ fn pending_cleanup_is_stale(recorded_at: &str, now: chrono::DateTime<chrono::Utc
#[tauri::command]
pub async fn update_project(
project: serde_json::Value,
app_handle: tauri::AppHandle,
state: State<'_, AppState>,
) -> Result<Project, String> {
// Taken as raw JSON, then deserialised, for one reason: a secret field that
@@ -1098,46 +1097,57 @@ pub async fn update_project(
// [`crate::models::validate_env_vars_update`].
crate::models::validate_env_vars_update(&stored.custom_env_vars, &project.custom_env_vars)?;
project.container_id = stored.container_id;
project.status = stored.status;
// `browser_view_enabled` is owned by `set_browser_view_enabled` and is
// restored here rather than taken from the payload, exactly like
// `container_id` and `status` above. The Config tab has no control for it
// — the Browser tab's toggle is the only way it ever changes — so the
// project object the frontend round-trips carries whatever it was told at
// load time and would silently undo a toggle made since. `auth_bridge_enabled`
// is different and does arrive through this save: the Config tab edits it,
// which is why the reconcile below follows whatever was just persisted.
project.browser_view_enabled = stored.browser_view_enabled;
project.created_at = stored.created_at;
restore_store_owned_fields(&mut project, &stored);
project.updated_at = chrono::Utc::now().to_rfc3339();
store_secrets_for_project(&project, &explicitly_cleared)?;
let updated = state.projects_store.update(project)?;
// `auth_bridge_enabled` can arrive through this generic save as well as
// through `set_auth_bridge_enabled`, so reconcile the running bridge with
// whatever was just persisted. `start` is idempotent and `stop` is a no-op
// when nothing is running, so this is safe on every project save.
if updated.auth_bridge_enabled {
if let Some(ref container_id) = updated.container_id {
if docker::is_container_running(container_id).await.unwrap_or(false) {
state
.auth_bridge
.start(
updated.id.clone(),
container_id.clone(),
app_handle,
state.projects_store.clone(),
)
.await;
}
}
} else {
state.auth_bridge.stop(&updated.id).await;
}
// Nothing reconciles the *running* auth bridge here any more, and there is
// nothing left for such a step to do. This command can no longer change
// `auth_bridge_enabled` at all (see [`restore_store_owned_fields`]), so a
// reconcile could only ever re-assert what was already true. The paths that
// do change it each own their own side effect: `set_auth_bridge_enabled`
// starts or stops the bridge itself, [`start_project_container`] arms it
// when the container comes up, and `reconcile_project_statuses` re-arms it
// for every already-running container at launch. The version of this that
// re-asserted on every save is what turned a stale flag in a payload into a
// restarted bridge.
state.projects_store.update(project)
}
Ok(updated)
/// Restore onto `project` the fields whose value belongs to the store rather
/// than to whoever is saving the project. See the comment above `stored` in
/// [`update_project`] for `container_id`, `status` and `created_at`.
///
/// **Both feature flags are in here, for one reason that covers them equally:
/// neither ever arrives through this command as an edit.** Each has a
/// dedicated setter — [`crate::browser_view::commands::set_browser_view_enabled`]
/// and [`crate::commands::auth_bridge_commands::set_auth_bridge_enabled`] —
/// and that setter is the only control the UI offers for it. Neither is wired
/// into the Config tab's `save`: the browser view's toggle lives in the Browser
/// tab, and `AuthBridgeRow`'s switch calls `set_auth_bridge_enabled` directly
/// even though it is rendered *in* the Config tab, because that tab's editors
/// are disabled while the container runs and the bridge is precisely the thing
/// a user needs to flip while a login is hanging.
///
/// So the flags in an incoming payload are never a choice — they are whatever
/// the frontend was told when it loaded the project, and the setters do not
/// write their new value back into frontend app state. Every unrelated save
/// (a renamed session, an env var, a mount name) carries that snapshot back.
/// Taking it would silently undo a toggle made since.
///
/// This restored only `browser_view_enabled` before, on the stated belief that
/// the Config tab edited `auth_bridge_enabled` through this save. It does not.
/// The consequence was specific: a user turns the bridge off — having been told
/// a bridged port is unauthenticated and reachable by any local process — then
/// closes a renamed terminal tab, and the stale `true` in that save re-persisted
/// and restarted the bridge.
fn restore_store_owned_fields(project: &mut Project, stored: &Project) {
project.container_id = stored.container_id.clone();
project.status = stored.status.clone();
project.browser_view_enabled = stored.browser_view_enabled;
project.auth_bridge_enabled = stored.auth_bridge_enabled;
project.created_at = stored.created_at.clone();
}
#[tauri::command]
@@ -2195,4 +2205,89 @@ mod tests {
// Changing it to a different root is a change, and refused.
assert!(validate_mounted_host_path("x", Some("/"), Some("C:\\")).is_err());
}
// ── Fields a generic save does not get to write ───────────────────────
/// A project as the store holds it, plus the copy the frontend is about to
/// save back: same record, one unrelated edit, and the flags as they were
/// when the frontend last loaded it.
fn stored_and_stale_payload() -> (Project, Project) {
let mut stored = Project::new("demo".to_string(), Vec::new());
stored.container_id = Some("abc123".to_string());
stored.status = ProjectStatus::Running;
let mut payload = stored.clone();
payload.container_id = None;
payload.status = ProjectStatus::Stopped;
payload
.renamed_session_names
.insert("s1".to_string(), "build".to_string());
(stored, payload)
}
/// The regression. The user turns the auth bridge off — the switch calls
/// `set_auth_bridge_enabled`, which persists `false` and stops the bridge,
/// and writes nothing back into the frontend's copy of the project. Every
/// holder of that copy still has `auth_bridge_enabled: true`, and the next
/// unrelated save (closing a renamed terminal tab) posts it back. That save
/// must not re-enable the bridge.
#[test]
fn a_stale_auth_bridge_flag_in_a_save_cannot_re_enable_a_disabled_bridge() {
let (mut stored, mut payload) = stored_and_stale_payload();
stored.auth_bridge_enabled = false;
payload.auth_bridge_enabled = true;
restore_store_owned_fields(&mut payload, &stored);
assert!(
!payload.auth_bridge_enabled,
"a save must not be able to turn the bridge back on: the stored value is the user's"
);
// The edit the save was actually for still goes through.
assert_eq!(
payload.renamed_session_names.get("s1").map(String::as_str),
Some("build")
);
}
/// The mirror image, and the reason the serde default going to `true`
/// made this worse: a pre-existing record with no `auth_bridge_enabled`
/// key reads as enabled, so the stale payload is `true` for every project
/// that predates the field. A user who has *not* turned the bridge off is
/// equally entitled to have the store's answer win.
#[test]
fn an_enabled_bridge_is_left_enabled_by_the_same_rule() {
let (mut stored, mut payload) = stored_and_stale_payload();
stored.auth_bridge_enabled = true;
payload.auth_bridge_enabled = false;
restore_store_owned_fields(&mut payload, &stored);
assert!(payload.auth_bridge_enabled);
}
/// The flag that was already restored, kept under test beside the one that
/// was not — the two are owned by their setters for the same reason and
/// must not drift apart again.
#[test]
fn a_stale_browser_view_flag_cannot_undo_the_panes_toggle_either() {
let (mut stored, mut payload) = stored_and_stale_payload();
stored.browser_view_enabled = true;
payload.browser_view_enabled = false;
restore_store_owned_fields(&mut payload, &stored);
assert!(payload.browser_view_enabled);
}
#[test]
fn the_container_handle_status_and_creation_time_still_come_from_the_store() {
let (stored, mut payload) = stored_and_stale_payload();
restore_store_owned_fields(&mut payload, &stored);
assert_eq!(payload.container_id.as_deref(), Some("abc123"));
assert_eq!(payload.status, ProjectStatus::Running);
assert_eq!(payload.created_at, stored.created_at);
}
}
+92 -2
View File
@@ -346,11 +346,47 @@ const OPENERS: &[(&str, &[&str])] = &[("xdg-open", &[]), ("gio", &["open"])];
/// `xdg-open` usually returns immediately (it hands the URL to a running
/// browser and exits), but in its generic fallback mode it *is* the browser's
/// parent and stays alive for the session. So "still running" cannot be read
/// as failure, and "exited non-zero quickly" is the only reliable signal
/// there is.
/// as failure, and "exited non-zero quickly" is the only negative signal there
/// is — though not, on its own, a trustworthy one. See
/// [`exit_code_means_nothing_was_launched`].
#[cfg(target_os = "linux")]
const OPENER_GRACE: std::time::Duration = std::time::Duration::from_millis(400);
/// Whether a non-zero exit says the opener certainly launched nothing, and so
/// that the next candidate can be tried without risking a second tab.
///
/// The loop used to treat every quick non-zero exit as "it did nothing" and
/// fall through. That is safe for most of `xdg-open`'s documented codes — 1
/// (syntax), 2 (file not found) and 3 (a required tool could not be found) are
/// all statements that it never got as far as launching a handler, and 3 is the
/// missing-association case `gio open` is in [`OPENERS`] for. 127 is the same
/// statement made by a shell, which is how a `$BROWSER` or `x-www-browser`
/// wrapper naming a program that does not exist comes back.
///
/// Code 4 is the one that cannot be read that way, and it is the catch-all:
/// "the action failed" also covers a handler that *was* launched and then
/// returned non-zero. A browser that takes the URL, opens the tab in an already
/// running instance and exits non-zero for its own reasons ends up here, as
/// does a wrapper script that does its job and then returns the exit status of
/// something else. Falling through on that hands the same URL to a second
/// opener: two tabs for one click, and for an OAuth link two authorize
/// requests.
///
/// So anything not recognised below — 4, an unfamiliar code, or a death by
/// signal (`code()` is `None`) — ends the loop rather than continuing it. The
/// caller is told the opener failed, which is the honest report of an
/// ambiguous outcome, and no second request is made on the user's behalf. Note
/// what this costs: an opener that genuinely failed with code 4 no longer falls
/// through to `gio`, so a user whose `xdg-open` fails that way sees an error
/// where they previously might have got a tab.
///
/// This is reasoning from `xdg-open`'s documented exit codes, not from an
/// observed double-open in this app.
#[cfg(target_os = "linux")]
fn exit_code_means_nothing_was_launched(code: Option<i32>) -> bool {
matches!(code, Some(1 | 2 | 3 | 127))
}
/// Spawn `url` with an opener, under a sanitized environment.
#[cfg(target_os = "linux")]
fn spawn_with_clean_env(url: &str) -> Result<(), String> {
@@ -383,6 +419,10 @@ fn spawn_with_clean_env(url: &str) -> Result<(), String> {
.stdout(std::process::Stdio::null())
.stderr(std::process::Stdio::null());
// A spawn failure — `ErrorKind::NotFound` for an opener that is not
// installed, `PermissionDenied` for one that cannot be executed — is
// the unambiguous case: nothing ran, so nothing was opened, and the
// next candidate is free to try.
let mut child = match command.spawn() {
Ok(child) => child,
Err(err) => {
@@ -395,6 +435,14 @@ fn spawn_with_clean_env(url: &str) -> Result<(), String> {
match child.try_wait() {
Ok(Some(status)) if !status.success() => {
failures.push(format!("{program} exited with {status}"));
// A program that *ran* is not a program that did nothing.
if !exit_code_means_nothing_was_launched(status.code()) {
return Err(format!(
"Could not confirm the link opened. Tried: {}. It may have opened anyway \
— check your browser before trying again.",
failures.join("; ")
));
}
continue;
}
Ok(_) => {}
@@ -699,3 +747,45 @@ mod tests {
assert_eq!(changes, vec![("GTK_PATH".to_string(), None)]);
}
}
#[cfg(all(test, target_os = "linux"))]
mod opener_fallback_tests {
use super::*;
/// The codes `xdg-open` documents as "nothing was launched". Falling
/// through to the next opener on these is what keeps `gio open` reachable
/// for the case it was added for: no usable `x-scheme-handler/https`
/// association.
#[test]
fn the_codes_that_mean_no_handler_ran_fall_through() {
for code in [1, 2, 3, 127] {
assert!(
exit_code_means_nothing_was_launched(Some(code)),
"exit {code} means the opener never launched anything"
);
}
}
/// The regression this guards: `xdg-open` returns 4 both when it could not
/// act and when the handler it launched returned non-zero — including a
/// browser that had already opened the tab. Trying `gio open` next would
/// open it a second time, which for an OAuth URL is a second authorize
/// request.
#[test]
fn an_exit_that_may_follow_a_successful_open_does_not_fall_through() {
assert!(!exit_code_means_nothing_was_launched(Some(4)));
for code in [5, 7, 126, 255] {
assert!(
!exit_code_means_nothing_was_launched(Some(code)),
"exit {code} is not a documented 'did nothing', so it must not be assumed to be one"
);
}
}
/// Killed by a signal: `code()` is `None` and the outcome is unknowable,
/// so it is treated like any other unrecognised exit.
#[test]
fn a_death_by_signal_does_not_fall_through() {
assert!(!exit_code_means_nothing_was_launched(None));
}
}
@@ -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`
+92 -24
View File
@@ -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<PromptSource, number> = {
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<UrlPrompt | null>(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<UrlPrompt | null>(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");
+42 -9
View File
@@ -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(
<UrlToast
url={SIGN_IN}
onOpen={noop}
onOpenInContainer={noop}
signInDefault="host"
signInDefault="host-bridged"
onDismiss={noop}
/>,
);
@@ -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(
<UrlToast
url={SIGN_IN}
onOpen={noop}
onOpenInContainer={noop}
signInDefault="host-fallback"
onDismiss={noop}
/>,
);
// 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(
<UrlToast
url={SIGN_IN}
@@ -233,6 +263,9 @@ describe("UrlToast", () => {
/>,
);
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", () => {
+28 -8
View File
@@ -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 projects 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."}
</div>
)}
</div>
+47 -17
View File
@@ -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<SignInOpenTarget>("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<SignInOpenTarget>("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: [] });
});