diff --git a/shell/src-tauri/permissions/default.toml b/shell/src-tauri/permissions/default.toml index b564a846..3bbf6e97 100644 --- a/shell/src-tauri/permissions/default.toml +++ b/shell/src-tauri/permissions/default.toml @@ -17,6 +17,7 @@ permissions = [ "allow-open-dashboard", "allow-reveal-config-dir", "allow-reveal-logs-dir", + "allow-restart-sidecar", "allow-get-shell-api-key", "allow-subscribe-token-usage", ] @@ -41,6 +42,11 @@ identifier = "allow-reveal-logs-dir" description = "Reveal the maximal logs directory in Finder/Explorer." commands.allow = ["reveal_logs_dir"] +[[permission]] +identifier = "allow-restart-sidecar" +description = "Reboot the proxy sidecar into the on-disk config — the final step of an account switch / sign-in / sign-out, invoked from the Settings webview (a 'remote' loopback origin, so it needs an explicit permission)." +commands.allow = ["restart_sidecar"] + [[permission]] identifier = "allow-get-shell-api-key" description = "Return the per-launch shell API key so the webview can authenticate against the sidecar even when 'Block unknown connections' is on." diff --git a/shell/src-tauri/src/lib.rs b/shell/src-tauri/src/lib.rs index fca0ce79..8aa51704 100644 --- a/shell/src-tauri/src/lib.rs +++ b/shell/src-tauri/src/lib.rs @@ -317,6 +317,30 @@ impl SplashDismissed { } } +/// Set while `respawn_sidecar` is intentionally cycling the sidecar (the +/// account-switch / sign-in / sign-out reboot). The old sidecar exits cleanly +/// from our SIGTERM, which the Terminated handler would otherwise read as a +/// user-initiated quit and bring the WHOLE app down — stranding the tray and +/// killing the reboot before the replacement spawns. The handler consumes this +/// flag and keeps the app alive for the respawn instead. +struct SidecarRestarting(std::sync::atomic::AtomicBool); + +impl SidecarRestarting { + fn new() -> Self { + Self(std::sync::atomic::AtomicBool::new(false)) + } + + /// Mark that the next sidecar exit is an intentional restart, not a quit. + fn begin(&self) { + self.0.store(true, std::sync::atomic::Ordering::SeqCst); + } + + /// Returns true (and clears the flag) if a restart was in progress. + fn consume(&self) -> bool { + self.0.swap(false, std::sync::atomic::Ordering::SeqCst) + } +} + /// Random per-launch key the shell shares with its sidecar so the /// webview can authenticate against /settings/api/* even when the user /// has enabled "Block unknown connections." Injected as an env var when @@ -493,6 +517,7 @@ pub fn run() { .manage(SetupPromptShown::new()) .manage(StartupAnnounced::new()) .manage(SplashDismissed::new()) + .manage(SidecarRestarting::new()) .manage(LastRejection::new()) .manage(LastSidecarError::new()) .invoke_handler(tauri::generate_handler![ @@ -737,6 +762,16 @@ fn spawn_sidecar(app: &AppHandle) -> tauri::Result<()> { } CommandEvent::Terminated(payload) => { eprintln!("[maximal] sidecar exited: {:?}", payload); + // Intentional restart (account switch / sign-in / sign-out + // reboot)? respawn_sidecar set this flag before SIGTERMing + // the old child, and is already spawning the replacement. + // Keep the app alive — DON'T treat this exit as a quit. + if handle.state::().consume() { + eprintln!( + "[maximal] (intentional restart — keeping app alive for the respawn)" + ); + break; + } // Clean exit signals an intentional shutdown — either // we just sent SIGTERM via kill_sidecar (Quit flow), // or an external caller hit /_internal/shutdown @@ -880,24 +915,19 @@ fn kill_sidecar(app: &AppHandle) { libc::kill(pid, libc::SIGTERM); } - // Stash the child back so the escalation task can decide - // whether it still needs SIGKILL. If the sidecar exits - // cleanly within 3s, the child handle goes out of scope here - // and that's fine — killing an already-exited child is a - // harmless no-op. - app.state::().set(child); - - // Plain OS thread for the 3s grace timer rather than pulling - // a tokio runtime in for this single sleep. AppHandle::clone() - // is Arc-cheap. - let app_handle = app.clone(); + // MOVE the old child into the escalation thread — do NOT put it back + // in the shared Sidecar slot. respawn_sidecar calls spawn_sidecar + // immediately after us, which set()s the REPLACEMENT child into that + // slot; if this escalation re-read the slot it would SIGKILL the FRESH + // sidecar ~3s after a restart (the proxy would vanish from :4141 and + // the whole UI would "Load failed" — the account-switch/sign-in/ + // sign-out reboot bug). Holding the specific old child here SIGKILLs + // only it. Dropping a CommandChild does NOT kill its process, so + // leaving the slot empty until spawn_sidecar fills it is safe. std::thread::spawn(move || { std::thread::sleep(std::time::Duration::from_secs(3)); - if let Some(child) = app_handle.state::().take() { - // Still alive (or at least, still in our slot) after - // the grace period — escalate to SIGKILL. - let _ = child.kill(); - } + // No-op if the SIGTERM above already made it exit. + let _ = child.kill(); }); } @@ -1611,6 +1641,11 @@ fn respawn_sidecar(app: &AppHandle) { app.state::().set(None); dismiss_splash(app); + // Mark this as an intentional restart BEFORE we SIGTERM the child, so the + // old sidecar's clean exit isn't mistaken for a user quit (which would + // bring the whole app down — see the Terminated handler in spawn_sidecar). + app.state::().begin(); + // Reap the current child (healthy, hung, or mid-crash) so the respawn binds // cleanly. No-op if already gone. spawn_sidecar also passes --replace as a // backstop against a not-yet-released port. diff --git a/shell/src/api.ts b/shell/src/api.ts index 79f57e18..25052b75 100644 --- a/shell/src/api.ts +++ b/shell/src/api.ts @@ -308,14 +308,18 @@ type ApiResult = function baseUrl(): string { // Vite injects import.meta.env.DEV at build time. In production the shell // loads from the sidecar's own origin via the Tauri window URL, so a - // relative path resolves to the same origin — correct regardless of port. - // In dev (`app:ui`), Vite serves the UI on its own port while the proxy - // runs separately, so we absolute-prefix. Default to :4142 to match - // CLAUDE.md's fast-iteration workflow (`bun run dev start --port 4142`, - // the sidecar's port); override with VITE_API_BASE to point elsewhere. + // relative path resolves to the same origin. + // + // In dev the webview is the Vite server on its own port, so we must + // absolute-prefix the proxy. Default to :4141 — the canonical Maximal / + // SIDECAR_PORT (shell/src-tauri/src/lib.rs). `bun run app:dev` runs the real + // sidecar there, so the default MUST be :4141 or the whole settings UI hits + // a dead port. For `app:ui` against a proxy on a different port (e.g. a + // second `bun run dev start --port 4142` alongside a running app:dev), set + // VITE_API_BASE to override. if (import.meta.env.DEV) { const override = import.meta.env.VITE_API_BASE as string | undefined - return override ?? "http://localhost:4142" + return override ?? "http://localhost:4141" } return "" } diff --git a/shell/src/main.ts b/shell/src/main.ts index 4c27833d..f56e46b9 100644 --- a/shell/src/main.ts +++ b/shell/src/main.ts @@ -81,12 +81,17 @@ function wireNav(): void { } } -async function safeInvoke(cmd: string): Promise { +async function safeInvoke(cmd: string): Promise { try { await invoke(cmd); + return true; } catch (err) { - // Tauri command unavailable (e.g. running in plain browser). Log and continue. + // Tauri command unavailable (e.g. plain-browser app:ui) or the IPC + // rejected. Returns false so callers that DEPEND on the command (e.g. the + // restart_sidecar reboot that completes a sign-in) can surface a visible + // error instead of silently stranding the user. console.warn(`invoke(${cmd}) failed:`, err); + return false; } } @@ -633,6 +638,9 @@ function showGhError(message: string): void { if (!el) return; el.textContent = message; el.hidden = false; + // The account section can be tall (remembered + gh lists), so an error on a + // row near the bottom can land off-screen. Bring it into view. + el.scrollIntoView({ block: "nearest" }); } function hideGhError(): void { @@ -682,9 +690,31 @@ async function loadGhAccounts(): Promise { return; } + // Dedup against the registry: a gh account that's already a remembered + // (persisted) account is offered in the "Switch to a remembered account" + // list above, so don't ALSO list it here as a fresh sign-in — that + // double-listing is what reads as confusing. Best-effort: if the accounts + // fetch fails, fall back to showing every gh account. + const remembered = await apiCall({ + kind: "accounts-list", + method: "GET", + path: "/settings/api/accounts", + }); + const rememberedKeys = new Set( + remembered.ok ? remembered.data.accounts.map((a) => a.key) : [], + ); + const ghAccounts = result.data.accounts.filter( + (a) => !rememberedKeys.has(`${a.login}@${a.host}`), + ); + if (ghAccounts.length === 0) { + wrapper.hidden = true; + list.replaceChildren(); + return; + } + list.replaceChildren(); hideGhError(); - for (const account of result.data.accounts) { + for (const account of ghAccounts) { const seed = template.content.firstElementChild; if (!seed) continue; const row = seed.cloneNode(true) as HTMLElement; @@ -796,7 +826,15 @@ async function rebootAndAwaitAuth( onSuccess: (status: AuthStatus) => void, onFailure: (sawDown: boolean) => void, ): Promise { - await safeInvoke("restart_sidecar"); + // If the restart IPC itself didn't fire (invoke rejected / unavailable), + // the proxy will never reboot — fail fast and visibly rather than polling a + // never-changing status for 20s. `sawDown: false` → the "didn't restart" + // branch of the caller's message. + const restarted = await safeInvoke("restart_sidecar"); + if (!restarted) { + onFailure(false); + return; + } const deadlineMs = Date.now() + 20_000; let sawDown = false; while (Date.now() < deadlineMs) { @@ -853,6 +891,7 @@ function showRosterError(mode: AccountRosterMode, message: string): void { if (el) { el.textContent = message; el.hidden = false; + el.scrollIntoView({ block: "nearest" }); } }