Compare commits
2
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
943c83b9e3 | ||
|
|
60188610ee |
@@ -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());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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`
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -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", () => {
|
||||
|
||||
@@ -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."}
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
|
||||
@@ -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: [] });
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user