diff --git a/CLAUDE.md b/CLAUDE.md index e7b96c7..03fd2f1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -67,7 +67,7 @@ PTY (ConPTY) → PseudoTerminal → TerminalBridge → WebView2 (xterm.js) | `ImportExportService` | Read/write a full `AppState` to a JSON file (settings + sessions backup) | | `SessionConfigEditor` | Diffs/applies a `SessionConfigDraft` onto a `ShellSession`; decides whether the change needs a PTY restart | | `DbGate` | Serializes every use of the shared `output.db` `SqliteConnection`. That one connection is handed to `SearchService` *and* to every `OutputIndexer`, and it is not thread-safe — concurrent create/dispose corrupts its internal command list. Acquire as `using var _ = await DbGate.AcquireAsync();` at the top of anything touching it. See issue #102 | -| `PwshLocator` | Single answer to "pwsh or powershell?", shared by `RunInstance` (run commands) and `PseudoTerminal.BuildCmdLine` (session wrapper) so they can't disagree. Resolves via `where.exe`, then rejects zero-length/reparse-point hits — a Microsoft Store App Execution Alias stub resolves on PATH but fails to execute (#104) | +| `PwshLocator` | Single answer to "pwsh or powershell?", shared by `RunInstance` (run commands) and `PseudoTerminal.BuildCmdLine` (session wrapper) so they can't disagree. Resolves via `where.exe`. An ordinary executable is accepted from metadata alone (no spawn). A **zero-byte reparse point is ambiguous, not bad**: that shape is a Store App Execution Alias, and a *working* Store install of PowerShell 7 looks identical to a stub left by an uninstalled app — so only that case is settled by actually probing execution. Rejecting the shape outright silently downgraded Store-PS7 users to 5.1 on every session launch | | `ClaudeConfigGate` | Watches Claude's config file settle. **Only used at shutdown.** The same mechanism was tried on the launch path and reverted (#111) — see the Claude launch stagger note below | | `ToastHelper` | Tray balloon notifications | | `SessionRunner` | Per-session owner of `RunInstance` dictionary (run commands runtime) | @@ -160,6 +160,10 @@ The page-side `mousedown` handler also calls `fitAddon.fit()`, and the initial f - **Sleep** (`SleepSession(vm)`) → `vm.Dispose()` + remove from `Sessions` but **keep** the `ShellSession` in `SessionManager` with `IsDormant = true`. A muted dormant sidebar entry replaces the active one. - **Wake** (`WakeSessionAsync(session)`) → re-runs `LaunchSessionAsync(session, restoring: true)` — same path as restore-on-startup. - **Restart** (`RestartSessionAsync(vm)`) → sleep-style teardown *without* the dormant bookkeeping, then `LaunchSessionAsync(session, restoring: true, removeOnFailure: false)`. The `ShellSession` stays in `SessionManager` (so Id, group, run commands and sidebar slot survive) and never enters the recently-closed ring. Used by "Edit session…" — see below. Both non-default arguments matter: `restoring: true` makes a Claude session resume rather than start a fresh conversation (matching Wake — a restart tears down the same way, so it must recover the same way), and `removeOnFailure: false` stops a bad edit from *deleting* the session, since `LaunchSessionAsync`'s PTY-failure path calls `SessionManager.RemoveSession` — correct for a session that never started, destructive for a relaunch. If the relaunch fails either way, the session falls back to dormant so the row stays visible and fixable instead of leaving a launching placeholder that never resolves. + + A Claude restart also **waits for the old process to actually exit** (`DisposeAndWaitForExitAsync` + a config quiesce) before relaunching. Without that it recreates the concurrent-config-writer race the launch stagger and the shutdown loop both exist to prevent, and `--resume` can read a session index the outgoing process hasn't finalised. Non-Claude sessions skip the wait — they don't touch that file. + +**Waiting for a PTY to exit.** Check `PseudoTerminal.HasExited`, never `IsRunning`. `IsRunning` is `_hProcess != IntPtr.Zero` and the handle is only released in `Dispose`, so it stays true for a child that exited on its own — subscribing to `Exited` for one of those waits out the full timeout for an event that already fired. `HasExited` is latched immediately before `Exited` is raised. This is not academic: combined with `ClaudeShutdownBudgetMs`, two stale panes consumed the entire shutdown budget and every remaining *live* Claude session was then force-disposed with no exit wait — the exact opposite of what the budget was for. 6. On app close: `_vm.SaveStateAsync()` flushes `_sessionManager.Sessions` (live + dormant) to `state.json` (unless `--clean`). ## Editing a Session's Configuration diff --git a/src/CodeShellManager/Assets/terminal-init.js b/src/CodeShellManager/Assets/terminal-init.js index 858d989..93ee266 100644 --- a/src/CodeShellManager/Assets/terminal-init.js +++ b/src/CodeShellManager/Assets/terminal-init.js @@ -108,15 +108,20 @@ if (opts.padding !== undefined) document.getElementById('terminal').style.padding = opts.padding; if (opts.retro !== undefined) document.body.classList.toggle('retro', !!opts.retro); fitAddon.fit(); - // A profile override can switch fontFamily/fontSize to a face that isn't loaded - // yet, so the fit above measures the wrong metrics for the same reason the - // initial one can. Re-fit once the new face is ready. + // A profile override can switch fontFamily/fontSize, so the fit above measures the + // old metrics. Re-fit on the next frame, once the new ones are in effect. + // + // Neither fonts API helps here. document.fonts.ready resolves once at page load + // and stays resolved, so a .then() attached now runs synchronously with the stale + // metrics — the bug this replaced. document.fonts.load() only matches + // CSS-connected FontFace objects (@font-face rules); the families used here are + // OS-installed and there are no such rules, so it resolves on the next microtask + // having matched nothing. requestAnimationFrame is the honest signal: it fires + // after the style change has been applied and measured. if (opts.fontFamily !== undefined || opts.fontSize !== undefined) { - if (document.fonts && document.fonts.ready) { - document.fonts.ready.then(function () { - try { fitAddon.fit(); } catch (e) {} - }); - } + requestAnimationFrame(function () { + try { fitAddon.fit(); } catch (e) {} + }); } } else if (msg.type === 'dropOverlayClear') overlay.classList.remove('active'); diff --git a/src/CodeShellManager/MainWindow.xaml.cs b/src/CodeShellManager/MainWindow.xaml.cs index 6ae291e..1ba65a7 100644 --- a/src/CodeShellManager/MainWindow.xaml.cs +++ b/src/CodeShellManager/MainWindow.xaml.cs @@ -202,8 +202,23 @@ private void SaveWindowBounds() // ── Startup ─────────────────────────────────────────────────────────────── + /// Resolves PwshLocator.Executable off the UI thread; awaited before restore. + private Task _pwshWarmup = Task.CompletedTask; + private async void OnLoaded(object sender, RoutedEventArgs e) { + // Start resolving pwsh-vs-powershell off the UI thread, as early as possible. + // + // PwshLocator.Executable is a Lazy first forced from PseudoTerminal.BuildCmdLine + // inside Start(), which LaunchSessionAsync calls ON THE UI THREAD. That costs a + // where.exe spawn and — since the Store-alias disambiguation — possibly a full + // PowerShell cold start behind it: up to ~7s of frozen window, exactly the class + // of stall #107 and #110 were undoing. + // + // The task is awaited before the restore loop rather than fire-and-forget; see + // there for why starting it here is necessary but not sufficient. + _pwshWarmup = Task.Run(() => _ = Services.PwshLocator.Executable); + await InitDatabaseAsync(); await _vm.LoadStateAsync(); RestoreWindowState(); @@ -272,6 +287,20 @@ private async void OnLoaded(object sender, RoutedEventArgs e) // applies the active group filter so off-group placeholders are hidden. RebuildSidebarOrder(); + // Make sure the pwsh/powershell decision is finished before the first launch. + // + // Starting the warm task in OnLoaded is not sufficient on its own: PwshLocator's + // Lazy uses ExecutionAndPublication, so a UI thread that reaches .Value while + // the pool thread is still inside the factory takes the Lazy's monitor and + // blocks for the REMAINING factory duration — a raced stall rather than no + // stall. PublicationOnly would not help either; the UI thread would simply run + // its own copy of the factory and pay the same cost. + // + // Awaiting here yields instead of blocking, so the window stays responsive. + // WhenAny with a ceiling above Resolve's own 7s bound (2s where.exe + 5s alias + // probe) so a wedged probe delays restore rather than preventing it. + await Task.WhenAny(_pwshWarmup, Task.Delay(8000)); + // Launch live sessions sequentially. Stagger consecutive claude launches: // claude's CLI does an unlocked read-modify-write on ~/.claude.json at startup, // so simultaneous boots can corrupt the user's profile. @@ -4257,6 +4286,12 @@ private async Task EditSessionAsync(SessionViewModel vm) if (!change.AnyChange) return; bool wasRemote = session.IsRemote; + // Apply mutates session.Command in place, so capture what the RUNNING process was + // launched with before that happens. RestartSessionAsync needs the OLD command to + // decide whether the outgoing process is a Claude that has to be waited out — + // reading session.Command there would see the new one and skip the wait on exactly + // the claude -> non-claude edit that needs it. + string launchedCommand = session.Command; Services.SessionConfigEditor.Apply(session, draft); vm.NotifyConfigChanged(); @@ -4283,7 +4318,7 @@ private async Task EditSessionAsync(SessionViewModel vm) + "this session starts.", "Restart session?", MessageBoxButton.YesNo, MessageBoxImage.Question, MessageBoxResult.Yes); - if (answer == MessageBoxResult.Yes) await RestartSessionAsync(vm); + if (answer == MessageBoxResult.Yes) await RestartSessionAsync(vm, launchedCommand); } /// @@ -4315,7 +4350,13 @@ private void EditDormantSession(ShellSession session) /// ( re-inserts at the SessionManager index), /// and it never enters the recently-closed ring. /// - private async Task RestartSessionAsync(SessionViewModel vm) + /// + /// The command the RUNNING process was started with. Callers that have already mutated + /// session.Command (the edit flow applies the draft before restarting) must pass + /// the old value, or a claude → non-claude edit skips the exit wait the outgoing + /// process needs. Null means "use the session's current command". + /// + private async Task RestartSessionAsync(SessionViewModel vm, string? launchedCommand = null) { var session = vm.Session; @@ -4327,6 +4368,12 @@ private async Task RestartSessionAsync(SessionViewModel vm) TerminalGrid.Children.Remove(ui.terminalWrapper); SidebarSessionList.Children.Remove(ui.sidebarItem); _sessionUi.Remove(vm.Id); + // The layout signature is keyed on session ids plus this counter; ids alone + // don't identify visual objects, and this method builds a NEW wrapper for the + // SAME id. Safe today only because LaunchSessionAsync bumps on re-add, but a + // future path that removes without re-adding would leave the signature stale + // and make RefreshTerminalLayout skip a rebuild it needed. + _sessionUiVersion++; } _runControls.Remove(vm.Id); _drawerItemBySession.Remove(vm.Id); @@ -4338,12 +4385,36 @@ private async Task RestartSessionAsync(SessionViewModel vm) _vm.Sessions.Remove(vm); if (_vm.ActiveSession == vm) _vm.ActiveSession = _vm.Sessions.LastOrDefault(); - vm.Dispose(); - // Placeholder so the row doesn't blink out of the sidebar while WebView2 boots. + // Placeholder BEFORE the teardown wait, not after: the sidebar row was removed + // above, and a Claude restart can now wait up to 11s. Without this the row is + // simply missing for that whole time. AddLaunchingSidebarItem(session); RebuildSidebarOrder(); + // Wait for the old process to actually exit before starting its replacement. + // + // For a Claude session this is the same concurrent-config-writer race the launch + // stagger and the shutdown loop both exist to prevent: the outgoing claude.exe can + // still be flushing its config while the new one reads and rewrites it. It also + // makes --resume reliable, since GetLastSessionId is read on the relaunch path and + // the outgoing process may not have finalised its session index yet. + // + // Keyed on the command the running process was LAUNCHED with — see the parameter. + // + // Non-Claude sessions don't touch that file, so they keep the cheap teardown. + if (ClaudeSessionService.IsClaudeCommand(launchedCommand ?? session.Command)) + { + DateTime? cfgBefore = ClaudeConfigGate.LastWriteUtcOrNull(_claudeConfigPath); + await DisposeAndWaitForExitAsync(vm, timeoutMs: 10000); + await WaitForClaudeConfigQuiesceAsync( + cfgBefore, Math.Min(_vm.Settings.ClaudeLaunchStaggerMs, 1000)); + } + else + { + vm.Dispose(); + } + try { // restoring: true so a Claude session resumes its conversation instead of @@ -5377,7 +5448,17 @@ private Task WaitForClaudeConfigQuiesceAsync(DateTime? baseline, int capMs) => private static async Task DisposeAndWaitForExitAsync(SessionViewModel vm, int timeoutMs) { var pty = vm.Pty; - if (pty == null || !pty.IsRunning) + + // HasExited, not IsRunning. IsRunning only reports "we still hold a handle", and + // that handle is released in Dispose — so it stays true for a child that exited + // earlier in the run (user typed `exit`, or claude crashed). Waiting on Exited for + // one of those burns the full timeout for an event that already fired. + // + // That mattered more than it looks: with the shutdown budget above, two such stale + // panes consume the entire allowance, and every remaining LIVE Claude session is + // then force-disposed with no exit wait — losing exactly the ~/.claude.json + // serialization this loop exists to provide. + if (pty == null || pty.HasExited) { vm.Dispose(); return; @@ -5388,6 +5469,10 @@ private static async Task DisposeAndWaitForExitAsync(SessionViewModel vm, int ti pty.Exited += OnExit; try { + // Re-check after subscribing: the child can exit in the window between the + // guard above and this line, and that firing would otherwise be missed. + if (pty.HasExited) { vm.Dispose(); return; } + // Dispose triggers ClosePseudoConsole, which signals the child to shut down. // MonitorExitAsync (already running) will fire Exited once the process exits. vm.Dispose(); diff --git a/src/CodeShellManager/Services/PwshLocator.cs b/src/CodeShellManager/Services/PwshLocator.cs index dc891e4..1135290 100644 --- a/src/CodeShellManager/Services/PwshLocator.cs +++ b/src/CodeShellManager/Services/PwshLocator.cs @@ -15,12 +15,18 @@ namespace CodeShellManager.Services; /// Prefer pwsh because that is where modern users keep their profile functions — /// wrapping in 5.1 loads a different profile and won't see them. /// -/// We only pick a *name*; CreateProcess resolves PATH. So a PATH lookup is the whole -/// question, and where.exe answers it in ~10ms. The earlier RunInstance version -/// spawned pwsh -Command "exit 0" instead, which additionally proved pwsh could -/// actually run — but at the cost of a full PowerShell startup (hundreds of ms) on a -/// path that runs during session restore. Not worth it for the rare broken install: -/// that case now surfaces as a failed session rather than a slower launch for everyone. +/// We only pick a *name*; CreateProcess resolves PATH. So a PATH lookup is most of the +/// question, and where.exe answers it in ~10ms. That alone is enough for an +/// ordinary executable. +/// +/// It is NOT enough for a Microsoft Store App Execution Alias, which is a zero-byte +/// reparse point whether the app behind it is installed or not — so a working Store +/// PowerShell 7 and a stub left by an uninstalled one are identical on disk. Only that +/// ambiguous case pays an execution probe; see . +/// +/// is warmed off the UI thread in MainWindow.OnLoaded, because +/// the Lazy is otherwise first forced from PseudoTerminal.Start on the UI thread and the +/// probe would freeze the window. /// internal static class PwshLocator { @@ -78,12 +84,12 @@ private static string Resolve() } /// - /// True when looks like a real executable rather than a Store - /// App Execution Alias stub. + /// True when can actually be executed. /// - /// The stubs live under WindowsApps, are zero bytes on disk, and are reparse points. - /// Length is the cheap discriminator and needs no extra API; the reparse-point check - /// is the belt-and-braces one. An unreadable path is treated as not runnable, because + /// Ordinary executables are decided from metadata alone — non-zero length and not a + /// reparse point — so the common install costs nothing. A zero-byte reparse point is + /// a Store App Execution Alias, which is *ambiguous* rather than bad, and only that + /// case is settled by probing. An unreadable path is treated as not runnable, because /// falling back to powershell.exe is always safe and picking a broken pwsh is not. /// internal static bool IsRunnable(string path) @@ -92,9 +98,59 @@ internal static bool IsRunnable(string path) try { var info = new System.IO.FileInfo(path); - if (!info.Exists || info.Length == 0) return false; - return (info.Attributes & System.IO.FileAttributes.ReparsePoint) == 0; + if (!info.Exists) return false; + + // A real executable — decided, no probe needed. + if (info.Length > 0 && + (info.Attributes & System.IO.FileAttributes.ReparsePoint) == 0) + return true; + + // Zero-byte and/or a reparse point: a Microsoft Store App Execution Alias. + // + // The earlier version rejected these outright to skip stubs left behind for + // uninstalled apps. That was wrong: a WORKING Store install of PowerShell 7 is + // exactly the same shape — a zero-byte AppExecLink at + // %LOCALAPPDATA%\Microsoft\WindowsApps\pwsh.exe. The two are indistinguishable + // on disk, so rejecting the shape silently downgraded Store-PowerShell users to + // 5.1 — losing the PS7 profile functions that are the whole reason for + // preferring pwsh, on every session launch since the locators merged. + // + // Neither answer is safe from metadata alone, so ask the alias to run. Only + // reached for the alias shape, so the common MSI install still costs nothing. + return CanExecute(path); } catch { return false; } } + + /// + /// Runs with a trivial no-op and reports whether it exited + /// cleanly. Used only to disambiguate a Store App Execution Alias, where the on-disk + /// metadata cannot tell a live alias from a dead stub. + /// + private static bool CanExecute(string path) + { + Process? probe = null; + try + { + probe = Process.Start(new ProcessStartInfo + { + FileName = path, + Arguments = "-NoLogo -NoProfile -Command \"exit 0\"", + UseShellExecute = false, + CreateNoWindow = true, + RedirectStandardOutput = true, + RedirectStandardError = true, + }); + // A dead alias fails fast (Win32Exception). A live one still pays a PowerShell + // startup, hence the generous ceiling — but a hang must not become a hang here. + return probe != null && probe.WaitForExit(5000) && probe.ExitCode == 0; + } + catch { return false; } + finally + { + try { if (probe is { HasExited: false }) probe.Kill(entireProcessTree: true); } + catch { /* best effort */ } + probe?.Dispose(); + } + } } diff --git a/src/CodeShellManager/Terminal/PseudoTerminal.cs b/src/CodeShellManager/Terminal/PseudoTerminal.cs index 8c184d5..1beee6a 100644 --- a/src/CodeShellManager/Terminal/PseudoTerminal.cs +++ b/src/CodeShellManager/Terminal/PseudoTerminal.cs @@ -168,8 +168,31 @@ private struct JOBOBJECT_EXTENDED_LIMIT_INFORMATION public event Action? DataReceived; public event Action? Exited; + /// + /// True while we still hold a process handle. NOT the same as "the child is alive" — + /// the handle is only released in , so this stays true after the + /// child has exited on its own. Use to ask that question. + /// public bool IsRunning => _hProcess != IntPtr.Zero; + /// + /// Latched once the child has actually exited and has been raised. + /// + /// Callers that wait on must check this, because the event may + /// already have fired before they subscribed — a session whose child exited earlier in + /// the run (user typed `exit`, or the process crashed) would otherwise wait out its + /// whole timeout for an event that will never come again. See + /// MainWindow.DisposeAndWaitForExitAsync. + /// + // volatile-equivalent: written on the monitor thread, read by shutdown on the UI + // thread. Today the Exited subscribe/unsubscribe accessors are Interlocked and supply + // the fence, but that is a subtle thing to depend on — an explicit field keeps it true + // for any future reader that polls without one. + private volatile bool _hasExited; + + /// + public bool HasExited => _hasExited; + // ── Public API ──────────────────────────────────────────────────────────── // Resolved once per process. pwsh (PowerShell 7+) is preferred because that's @@ -374,19 +397,43 @@ private static Task WaitForHandleAsync(IntPtr handle) var safe = new SafeWaitHandle(handle, ownsHandle: false); // caller closes the handle var waitHandle = new ManualResetEvent(false) { SafeWaitHandle = safe }; + // The registration is published through a gate rather than a bare local. + // + // If the handle is ALREADY signalled — a run-command child that exits immediately — + // the callback can run on a pool thread before RegisterWaitForSingleObject has even + // returned, so a plain `registration` local is still null when the callback reads + // it. The wait would then never be unregistered, and the ManualResetEvent would be + // disposed while the pool still held a live registration on it, which is documented + // as unsafe. + // + // The lock makes the callback wait for the assignment; TryUnregister runs exactly + // once either way, so a callback that arrives late is a no-op rather than a + // double-release. + var gate = new object(); RegisteredWaitHandle? registration = null; - registration = ThreadPool.RegisterWaitForSingleObject( - waitHandle, - (_, _) => + bool unregistered = false; + + void Release() + { + lock (gate) { - // Unregister first so the entry is released even if a continuation throws. + if (unregistered) return; + unregistered = true; registration?.Unregister(null); - waitHandle.Dispose(); - tcs.TrySetResult(); - }, - state: null, - millisecondsTimeOutInterval: Timeout.Infinite, - executeOnlyOnce: true); + } + waitHandle.Dispose(); + tcs.TrySetResult(); + } + + lock (gate) + { + registration = ThreadPool.RegisterWaitForSingleObject( + waitHandle, + (_, _) => Release(), + state: null, + millisecondsTimeOutInterval: Timeout.Infinite, + executeOnlyOnce: true); + } return tcs.Task; } @@ -442,6 +489,10 @@ private async Task MonitorExitAsync() } finally { + // Latch BEFORE raising, so a subscriber that checks HasExited from inside the + // handler — or one that subscribes concurrently — never sees "not exited yet" + // for a process that has already gone. + _hasExited = true; Exited?.Invoke(); } } diff --git a/src/CodeShellManager/Terminal/TerminalBridge.cs b/src/CodeShellManager/Terminal/TerminalBridge.cs index 71a7c24..fa3e001 100644 --- a/src/CodeShellManager/Terminal/TerminalBridge.cs +++ b/src/CodeShellManager/Terminal/TerminalBridge.cs @@ -456,8 +456,19 @@ public void ApplyProfileOverrides(ShellSession session) if (session.ProfileCursorBlink != null) opts["cursorBlink"] = session.ProfileCursorBlink; if (session.ProfilePadding != null) opts["padding"] = session.ProfilePadding; if (session.ProfileRetroEffect != null) opts["retro"] = session.ProfileRetroEffect; + // Malformed JSON here must not take the session down. This value is normally + // produced by SchemeMapper, but ImportExportService will deserialize a whole + // AppState from any file the user opens, so it can be arbitrary — and an + // unhandled throw on this path aborts the launch of an otherwise fine session. + // Dropping the theme degrades to the default palette, which is survivable. if (!string.IsNullOrEmpty(session.ProfileColorSchemeJson)) - opts["theme"] = JsonSerializer.Deserialize(session.ProfileColorSchemeJson); + { + try { opts["theme"] = JsonSerializer.Deserialize(session.ProfileColorSchemeJson); } + catch (JsonException ex) + { + Log($"ignoring malformed ProfileColorSchemeJson for '{session.Name}': {ex.Message}"); + } + } string json = JsonSerializer.Serialize(new { type = "setOptions", options = opts }); WpfApplication.Current?.Dispatcher.BeginInvoke(() => diff --git a/src/CodeShellManager/ViewModels/MainViewModel.cs b/src/CodeShellManager/ViewModels/MainViewModel.cs index 6240e5c..62fb204 100644 --- a/src/CodeShellManager/ViewModels/MainViewModel.cs +++ b/src/CodeShellManager/ViewModels/MainViewModel.cs @@ -284,14 +284,19 @@ public void RegisterSession(SessionViewModel vm) // there), so it must stay cheap. NotifyUserInteracted already fires AlertCleared // unconditionally, whose handler below raises AlertCount — so this deliberately // does not raise it a second time (issue #70). - // Typing into a pane makes it the active session, so its output flushes at - // foreground dispatcher priority (#70) instead of behind every other pane. + // Typing or clicking in a pane makes it the active session, so its output + // flushes at foreground dispatcher priority (#70) instead of behind every + // other pane. + // + // Both signals come from the PAGE, and must: + // - KeyboardInput <- xterm's onKey. NOT onData: that also carries the + // terminal's own replies (device attributes, cursor-position reports, OSC + // colour replies, focus in/out) and mouse reports, none of which are + // distinguishable from typing by inspecting the bytes. An earlier attempt + // filtered them and turned this into hover-to-focus (#106). + // - PaneActivated <- a capture-phase mousedown. WebView2 is an HwndHost, so + // a click on the terminal raises no WPF routed event at all (#108). // - // MUST NOT fire on mouse movement. terminal-init.js forwards xterm's onData, - // and onData carries mouse reports as well as keystrokes whenever the running - // app enables mouse tracking — which Claude Code does. Promoting on those - // turned this into hover-to-focus and repainted every pane's border on every - // mouse move. Hence the mouse-report filter rather than promoting on any input. // Guarded on reference equality: these run per keystroke / per click, and the // assign fans out to UpdateActiveTerminalHighlight across every session. void Promote() diff --git a/src/CodeShellManager/Views/NewSessionDialog.xaml.cs b/src/CodeShellManager/Views/NewSessionDialog.xaml.cs index ec84b8b..5150776 100644 --- a/src/CodeShellManager/Views/NewSessionDialog.xaml.cs +++ b/src/CodeShellManager/Views/NewSessionDialog.xaml.cs @@ -177,8 +177,9 @@ private void ApplyEditMode(ShellSession s) if (s.IsRemote) { - // Checking the radio runs SessionType_Changed, which swaps the panels and - // blanks NameBox — so the name is filled back in afterwards. + // Checking the radio runs SessionType_Changed, which swaps the panels. It no + // longer blanks NameBox — that handler returns early in edit mode — so the + // assignment below is the only thing setting the name, not a repair. RemoteRadio.IsChecked = true; SshHostBox.Text = string.IsNullOrWhiteSpace(s.SshUser) ? s.SshHost @@ -377,6 +378,14 @@ private void SessionType_Changed(object sender, RoutedEventArgs e) _lastProbedFolder = null; } CommandLabel.Text = IsRemoteMode ? "Remote Shell" : "Command"; + + // Never clear an existing session's name. In create mode the box holds an + // auto-filled suggestion and re-deriving it on a mode flip is the point; in edit + // mode it holds the user's actual name, and blanking it means SessionConfigEditor + // writes Name = "" — silently, because AutoFillName has nothing to refill from + // (ForEdit passes no default folder for a remote session). + if (IsEditMode) return; + NameBox.Text = ""; AutoFillName(); } @@ -530,6 +539,36 @@ private void Start_Click(object sender, RoutedEventArgs e) { SelectedFolder = FolderBox.Text.Trim(); + // Validate the folder in EDIT mode. + // + // Create mode deliberately tolerates a blank folder — LaunchSessionAsync falls + // back to %USERPROFILE% and a brand-new session in your home directory is a + // reasonable default. Editing an existing one is different: the same fallback + // silently relocates a configured session to the home folder, persists the + // empty path, and leaves git info and the accent colour keyed off nothing. + // Flipping Remote -> Local hits this every time, because a remote session has + // no local folder to pre-fill from. + if (IsEditMode) + { + if (string.IsNullOrWhiteSpace(SelectedFolder)) + { + System.Windows.MessageBox.Show( + "Please choose a working folder for this session.", + "Working folder required", MessageBoxButton.OK, MessageBoxImage.Warning); + FolderBox.Focus(); + return; + } + if (!System.IO.Directory.Exists(SelectedFolder)) + { + System.Windows.MessageBox.Show( + $"That folder doesn't exist:\n\n{SelectedFolder}\n\n" + + "Pick a folder that exists, or the session will fail to start.", + "Folder not found", MessageBoxButton.OK, MessageBoxImage.Warning); + FolderBox.Focus(); + return; + } + } + var selectedTag = (CommandCombo.SelectedItem as ComboBoxItem)?.Tag?.ToString() ?? "claude"; if (selectedTag == "custom") { diff --git a/tests/CodeShellManager.Tests/PwshLocatorTests.cs b/tests/CodeShellManager.Tests/PwshLocatorTests.cs index 67538f3..2fb97e0 100644 --- a/tests/CodeShellManager.Tests/PwshLocatorTests.cs +++ b/tests/CodeShellManager.Tests/PwshLocatorTests.cs @@ -77,4 +77,28 @@ public void Executable_ResolvesToOneOfTheTwoKnownNames() // Whatever this machine has, the answer must be a name ConPTY can resolve. Assert.Contains(PwshLocator.Executable, new[] { "pwsh.exe", "powershell.exe" }); } + + // ── Store App Execution Alias (issue #104 follow-up) ────────────────── + + [Fact] + public void IsRunnable_ZeroByteStub_DoesNotShortCircuitToFalse() + { + // Regression: an earlier version rejected the alias SHAPE outright. A working + // Store install of PowerShell 7 is the same shape as a dead stub — a zero-byte + // AppExecLink — so shape alone must not decide. A plain zero-byte file here is + // not executable, so the probe correctly says no; the point is that it is the + // PROBE saying no, not the length check. + Assert.False(PwshLocator.IsRunnable(Make("pwsh.exe", Array.Empty()))); + } + + [Fact] + public void IsRunnable_RealExecutableWithContent_SkipsTheProbeEntirely() + { + // The common MSI install must stay on the fast path — non-zero length and not a + // reparse point is decided from metadata, with no process spawned. + var sw = System.Diagnostics.Stopwatch.StartNew(); + Assert.True(PwshLocator.IsRunnable(Make("real.exe", new byte[] { 0x4D, 0x5A, 0x90, 0x00 }))); + Assert.True(sw.ElapsedMilliseconds < 500, + $"metadata path should not spawn anything, took {sw.ElapsedMilliseconds}ms"); + } }