diff --git a/AGENTS.md b/AGENTS.md index 0479695..46ed8fa 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -150,6 +150,11 @@ the terminal, with both a full-screen TUI and scriptable CLI commands. paste through Terminal.Gui's default `Command.Paste` pipeline, while read-only panes ignore paste events. `TerminalEscapeSanitizer` still applies to rendered remote content and is separate from Terminal.Gui's pasted-input sanitization. +- Keep the main shell's contextual header, inline busy indicators, quit routing, + cancellation ownership, and memory bounds aligned with + `agent_docs/ui-lifecycle-and-resource-bounds.md`. In particular, `Ctrl+Q` + must quit from text fields, subprocess capture and caches must stay bounded, + and superseded preview/inventory work must be canceled. - If Copilot-specific, Claude-specific, or other agent-platform guidance turns out to matter for this repo, capture the repo-relevant part here so future agents do not need to rediscover it from external docs. @@ -164,3 +169,5 @@ the terminal, with both a full-screen TUI and scriptable CLI commands. and the current Homebrew / WinGet dark-launch artifacts. - `agent_docs/tui-pty-testing.md` — sandboxed PTY workflow, synchronization strategy, verification scripts, and known pitfalls for terminal UI testing. +- `agent_docs/ui-lifecycle-and-resource-bounds.md` — shell conventions, + cancellation ownership, and subprocess/cache/log memory limits. diff --git a/README.md b/README.md index 0754ce1..0072c0c 100644 --- a/README.md +++ b/README.md @@ -182,8 +182,9 @@ Navigation: | `Tab` / `Shift+Tab` | Move focus between list and detail | | `/` | Jump to Discover and focus the search box | | `?` or `F1` | Open the help overlay | -| `Esc` | Back out of the current sub-view / modal | -| `q` | Quit | +| `Esc` | Leave a field or back out of the current sub-view / modal | +| `q` | Quit from a top-level list or preview | +| `Ctrl+Q` | Quit from anywhere, including while typing | Discover tab: diff --git a/agent_docs/tui-pty-testing.md b/agent_docs/tui-pty-testing.md index ca88df0..35a2a61 100644 --- a/agent_docs/tui-pty-testing.md +++ b/agent_docs/tui-pty-testing.md @@ -92,8 +92,10 @@ immediately. ### 1. Startup focus is in the query field -At startup, plain letters go into the query field. If you want global shortcuts -like `d`, `u`, `c`, `I`, or `q`, first send `Esc`. +At startup, plain letters go into the query field. If you want single-letter +global shortcuts like `d`, `u`, `c`, or `I`, first send `Esc`. `Ctrl+Q` is the +unconditional quit path and works without leaving the field; plain `q` quits +only when a top-level read-only view has focus. ### 2. Search works even when naive detectors say it failed @@ -213,7 +215,7 @@ These are current repo-specific pitfalls, not generic PTY problems: 2. Create a temp sandbox (`HOME_DIR`, `WORK_DIR`, `SCAN_ROOT`). 3. Pick a query and row index from `gh skill search --json ...`. 4. Launch the built `skillview` binary in a PTY with `SKILLVIEW_LOG=debug`. -5. Send `Esc` before global shortcuts. +5. Send `Esc` before single-letter global shortcuts; use `Ctrl+Q` to quit from any focus state. 6. Wait on log conditions for search/preview milestones. 7. Verify install/remove with CLI shell scripts after each mutation. 8. Save cleaned PTY snapshots for UX review. diff --git a/agent_docs/ui-lifecycle-and-resource-bounds.md b/agent_docs/ui-lifecycle-and-resource-bounds.md new file mode 100644 index 0000000..153a0d4 --- /dev/null +++ b/agent_docs/ui-lifecycle-and-resource-bounds.md @@ -0,0 +1,52 @@ +# UI lifecycle and resource bounds + +Use these rules when changing the main shell, asynchronous tab work, previews, +logging, or subprocess adapters. + +## Shell and interaction conventions + +- The window border owns the `SkillView — skillview` / `gh skillview` brand. + Do not add another persistent logo. `ContextBarView` owns the active workspace + title and its optional context chips. +- Keep search scope beside the relevant search field. Put actions in the bottom + status strip and keep table abbreviations in a separate legend. +- A `SpinnerView` must be immediately before its status label on the same row. + The main spinner is owned by `StatusStripView`; tab and modal spinners use the + same inline layout. +- Configure rendered Markdown through `TuiHelpers.ConfigureMarkdownPane`, which + hides literal heading prefixes while retaining semantic heading styles. +- `Ctrl+Q` is unconditional and must be handled before printable-key routing. + Plain `q` is a top-level quit shortcut; `Esc` means leave field, back, or close. +- Layouts below 80×24 show `TerminalSizeGuardView` instead of overlapping panes. + +## Lifetime and memory rules + +- `LatestRequestGate` owns Discover preview cancellation. A new preview cancels + the previous request, and every completion checks `Lease.IsCurrent` before + updating UI. +- Installed, Changes, and Updates cancel superseded inventory loads. Pass their + cancellation token through to inventory capture; do not add generation-only + refreshes that leave obsolete scans running. Use `CancellationTokenSourceSlot` + so source replacement, cancellation, and lease disposal share one ownership + boundary; never cancel a source that another path can independently dispose. +- Async update/install controls stay disabled while their operation is active. + Continue to pass cancellation into subprocess-backed services. +- `ProcessRunner` retains at most 1 MiB of child output per stdout/stderr stream, + plus a small truncation marker. It drains both streams in fixed-size chunks; + do not use line-based process events because a newline-free child can make the + framework retain an unbounded line before SkillView sees it. Increase the + capture limit only with evidence that a supported command needs more output. +- `SearchAgentMetadataCache` is a thread-safe 512-entry LRU. Do not replace it + with an unbounded dictionary. +- Logger subscriptions are disposable. Every long-lived subscriber must retain + and dispose its subscription. Disposal deactivates registrations that were + already snapshotted and waits for an in-flight callback, so no callback can + begin after disposal returns. The visible log pane retains at most 512 + formatted lines and coalesces burst refreshes without lost wakeups. + +## Focused verification + +Run `dotnet build`, then `dotnet test --no-build`. Resource-focused coverage +includes large subprocess output, LRU eviction, subscription disposal, +overlapping preview cancellation, superseded inventory scans, inline busy +state, and layout guards at 80×24, 100×30, and 140×42. diff --git a/src/SkillView.Core/Bootstrapping/EntryPoint.cs b/src/SkillView.Core/Bootstrapping/EntryPoint.cs index d7d670e..5146fbd 100644 --- a/src/SkillView.Core/Bootstrapping/EntryPoint.cs +++ b/src/SkillView.Core/Bootstrapping/EntryPoint.cs @@ -51,9 +51,10 @@ public static async Task RunAsync(string[] args) // In CLI mode, `--debug` streams structured log lines to stderr for // immediate visibility. + IDisposable? consoleSubscription = null; if (options.Debug && options.DispatchMode == DispatchMode.Cli) { - logger.Subscribe(entry => Console.Error.WriteLine(Logger.Format(entry))); + consoleSubscription = logger.Subscribe(entry => Console.Error.WriteLine(Logger.Format(entry))); } var services = TuiServices.Build(logger, fileSink, logDir); @@ -73,6 +74,7 @@ public static async Task RunAsync(string[] args) } finally { + consoleSubscription?.Dispose(); fileSink?.Dispose(); } } diff --git a/src/SkillView.Core/Logging/FileLogSink.cs b/src/SkillView.Core/Logging/FileLogSink.cs index 932a407..0c2e87d 100644 --- a/src/SkillView.Core/Logging/FileLogSink.cs +++ b/src/SkillView.Core/Logging/FileLogSink.cs @@ -22,30 +22,44 @@ public sealed class FileLogSink : IDisposable private DateOnly _currentDay; private bool _disposed; private bool _trimPending; + private IDisposable? _subscription; + private readonly Action? _beforeAppendLockForTests; public FileLogSink(string directory, Func? clock = null) + : this(directory, clock, beforeAppendLockForTests: null) + { + } + + internal FileLogSink( + string directory, + Func? clock, + Action? beforeAppendLockForTests) { _directory = directory; _clock = clock ?? (() => DateTimeOffset.Now); + _beforeAppendLockForTests = beforeAppendLockForTests; } public string Directory => _directory; public void Attach(Logger logger) { + ObjectDisposedException.ThrowIf(_disposed, this); + _subscription?.Dispose(); // Emit the ring-buffer snapshot first so the file reflects pre-attach entries too. foreach (var entry in logger.Snapshot()) { Append(entry); } - logger.Subscribe(Append); + _subscription = logger.Subscribe(Append); } public void Append(LogEntry entry) { - if (_disposed) return; + _beforeAppendLockForTests?.Invoke(); lock (_gate) { + if (_disposed) return; try { var today = DateOnly.FromDateTime(entry.Timestamp.ToLocalTime().DateTime); @@ -193,11 +207,26 @@ private static void TrySetPosixMode(string path) public void Dispose() { + IDisposable? subscription; lock (_gate) { if (_disposed) return; _disposed = true; + subscription = _subscription; + _subscription = null; + } + + // Logger subscription disposal waits for an in-flight callback to + // finish. Never wait while holding _gate: Append takes the logger's + // registration lock first and then this lock, so the reverse order + // here would deadlock shutdown. + subscription?.Dispose(); + + lock (_gate) + { CloseWriterLocked(); } } + + internal bool IsDisposedForTests => Volatile.Read(ref _disposed); } diff --git a/src/SkillView.Core/Logging/Logger.cs b/src/SkillView.Core/Logging/Logger.cs index c222fd3..08ef71e 100644 --- a/src/SkillView.Core/Logging/Logger.cs +++ b/src/SkillView.Core/Logging/Logger.cs @@ -1,4 +1,3 @@ -using System.Collections.Concurrent; using System.Globalization; namespace SkillView.Logging; @@ -8,9 +7,11 @@ namespace SkillView.Logging; public sealed class Logger { private readonly object _gate = new(); + private readonly object _observerGate = new(); private readonly LinkedList _ring = new(); private readonly int _capacity; - private readonly ConcurrentBag> _observers = new(); + private readonly Dictionary _observers = new(); + private long _nextObserverId; public Logger(LogLevel minimumLevel = LogLevel.Info, int capacity = 2048) { @@ -20,7 +21,17 @@ public Logger(LogLevel minimumLevel = LogLevel.Info, int capacity = 2048) public LogLevel MinimumLevel { get; set; } - public void Subscribe(Action observer) => _observers.Add(observer); + public IDisposable Subscribe(Action observer) + { + ArgumentNullException.ThrowIfNull(observer); + lock (_observerGate) + { + var id = ++_nextObserverId; + var registration = new ObserverRegistration(observer); + _observers.Add(id, registration); + return new Subscription(this, id, registration); + } + } public void Log(LogLevel level, string category, string message) { @@ -44,9 +55,14 @@ public void Log(LogLevel level, string category, string message) } } - foreach (var observer in _observers) + ObserverRegistration[] observers; + lock (_observerGate) { - try { observer(entry); } + observers = _observers.Values.ToArray(); + } + foreach (var observer in observers) + { + try { observer.Invoke(entry); } catch { /* observer faults must not kill the logger */ } } } @@ -74,4 +90,65 @@ public static string Format(LogEntry entry) entry.Category, entry.Message); } + + private void Unsubscribe(long id, ObserverRegistration registration) + { + lock (_observerGate) + { + _observers.Remove(id); + } + + // Do not hold the collection lock while waiting for an in-flight + // callback. This avoids lock inversion if an observer subscribes or + // disposes a subscription from inside its callback. + registration.Deactivate(); + } + + private sealed class ObserverRegistration + { + private readonly object _gate = new(); + private readonly Action _observer; + private bool _active = true; + + internal ObserverRegistration(Action observer) + { + _observer = observer; + } + + internal void Invoke(LogEntry entry) + { + lock (_gate) + { + if (_active) + { + _observer(entry); + } + } + } + + internal void Deactivate() + { + lock (_gate) + { + _active = false; + } + } + } + + private sealed class Subscription : IDisposable + { + private Logger? _owner; + private readonly long _id; + private readonly ObserverRegistration _registration; + + internal Subscription(Logger owner, long id, ObserverRegistration registration) + { + _owner = owner; + _id = id; + _registration = registration; + } + + public void Dispose() => + Interlocked.Exchange(ref _owner, null)?.Unsubscribe(_id, _registration); + } } diff --git a/src/SkillView.Core/Subprocess/ProcessRunner.cs b/src/SkillView.Core/Subprocess/ProcessRunner.cs index 8c95a93..0dbdaad 100644 --- a/src/SkillView.Core/Subprocess/ProcessRunner.cs +++ b/src/SkillView.Core/Subprocess/ProcessRunner.cs @@ -7,11 +7,15 @@ namespace SkillView.Subprocess; /// argv-array subprocess invoker — never shell composition. public sealed class ProcessRunner { + public const int DefaultMaxCapturedCharsPerStream = 1024 * 1024; private readonly Logger _logger; + private readonly int _maxCapturedCharsPerStream; - public ProcessRunner(Logger logger) + public ProcessRunner(Logger logger, int maxCapturedCharsPerStream = DefaultMaxCapturedCharsPerStream) { + ArgumentOutOfRangeException.ThrowIfLessThan(maxCapturedCharsPerStream, 1); _logger = logger; + _maxCapturedCharsPerStream = maxCapturedCharsPerStream; } public async Task RunAsync( @@ -38,11 +42,8 @@ public async Task RunAsync( _logger.Debug("subprocess", $"exec: {executable} {string.Join(' ', arguments)}"); using var process = new Process { StartInfo = psi, EnableRaisingEvents = true }; - var stdout = new StringBuilder(); - var stderr = new StringBuilder(); - - process.OutputDataReceived += (_, e) => { if (e.Data is not null) stdout.AppendLine(e.Data); }; - process.ErrorDataReceived += (_, e) => { if (e.Data is not null) stderr.AppendLine(e.Data); }; + var stdout = new BoundedTextBuffer(_maxCapturedCharsPerStream); + var stderr = new BoundedTextBuffer(_maxCapturedCharsPerStream); var sw = Stopwatch.StartNew(); try @@ -55,18 +56,27 @@ public async Task RunAsync( return new ProcessResult(executable, arguments, -1, string.Empty, ex.Message, sw.Elapsed); } - process.BeginOutputReadLine(); - process.BeginErrorReadLine(); process.StandardInput.Close(); + var stdoutDrain = DrainAsync(process.StandardOutput, stdout, cancellationToken); + var stderrDrain = DrainAsync(process.StandardError, stderr, cancellationToken); try { await process.WaitForExitAsync(cancellationToken).ConfigureAwait(false); + await Task.WhenAll(stdoutDrain, stderrDrain).ConfigureAwait(false); } catch (OperationCanceledException) { try { process.Kill(entireProcessTree: true); } catch { /* best-effort */ } + + // Observe both readers. They use the same cancellation token, so + // cancellation cannot leave background reads attached to a process + // that this method is about to dispose. + try { await Task.WhenAll(stdoutDrain, stderrDrain).ConfigureAwait(false); } + catch (OperationCanceledException) { } + catch (ObjectDisposedException) { } + catch (IOException) { } throw; } @@ -84,4 +94,74 @@ public async Task RunAsync( $"exit={result.ExitCode} dur={result.Duration.TotalMilliseconds:F0}ms {executable}"); return result; } + + private static async Task DrainAsync( + StreamReader reader, + BoundedTextBuffer destination, + CancellationToken cancellationToken) + { + // StreamReader's line-oriented APIs retain a whole unterminated line. + // Fixed-size reads keep memory bounded even for a child that never + // emits a newline, while continuing to drain bytes after capture fills. + var chunk = new char[4096]; + while (true) + { + var read = await reader + .ReadAsync(chunk.AsMemory(), cancellationToken) + .ConfigureAwait(false); + if (read == 0) + { + return; + } + + destination.Append(chunk.AsSpan(0, read)); + } + } + + /// Retains only the leading portion of a stream. Process output is + /// untrusted and can be arbitrarily large, so callers get a clear marker + /// instead of allowing a noisy child process to grow the app indefinitely. + private sealed class BoundedTextBuffer + { + private readonly object _gate = new(); + private readonly int _limit; + private readonly StringBuilder _buffer; + private bool _truncated; + + internal BoundedTextBuffer(int limit) + { + _limit = limit; + _buffer = new StringBuilder(Math.Min(limit, 16 * 1024)); + } + + internal void Append(ReadOnlySpan text) + { + lock (_gate) + { + if (_truncated) return; + var remaining = _limit - _buffer.Length; + if (remaining <= 0) + { + _truncated = true; + return; + } + + var take = Math.Min(text.Length, remaining); + _buffer.Append(text[..take]); + if (take < text.Length) + { + _truncated = true; + } + } + } + + public override string ToString() + { + lock (_gate) + { + if (!_truncated) return _buffer.ToString(); + return $"{_buffer}\n… output truncated after {_limit} characters …\n"; + } + } + } } diff --git a/src/SkillView.Core/Ui/CancellationTokenSourceSlot.cs b/src/SkillView.Core/Ui/CancellationTokenSourceSlot.cs new file mode 100644 index 0000000..fedbdf7 --- /dev/null +++ b/src/SkillView.Core/Ui/CancellationTokenSourceSlot.cs @@ -0,0 +1,96 @@ +namespace SkillView.Ui; + +/// Coordinates one owned cancellation source across replacement, cancellation, +/// and lease disposal. Every operation that can touch the source itself runs +/// under the same gate, preventing Cancel from racing Dispose. +internal sealed class CancellationTokenSourceSlot +{ + private readonly object _gate = new(); + private CancellationTokenSource? _active; + + internal bool HasActive + { + get + { + lock (_gate) + { + return _active is not null; + } + } + } + + internal Lease Replace(CancellationToken lifetime) + { + var next = CancellationTokenSource.CreateLinkedTokenSource(lifetime); + lock (_gate) + { + _active?.Cancel(); + _active = next; + } + + return new Lease(this, next); + } + + internal Lease? TryBegin(CancellationToken lifetime) + { + var next = CancellationTokenSource.CreateLinkedTokenSource(lifetime); + lock (_gate) + { + if (_active is not null) + { + next.Dispose(); + return null; + } + + _active = next; + } + + return new Lease(this, next); + } + + internal bool Cancel() + { + lock (_gate) + { + if (_active is null) + { + return false; + } + + _active.Cancel(); + _active = null; + return true; + } + } + + private void Release(CancellationTokenSource source) + { + lock (_gate) + { + if (ReferenceEquals(_active, source)) + { + _active = null; + } + + source.Dispose(); + } + } + + internal sealed class Lease : IDisposable + { + private CancellationTokenSourceSlot? _owner; + private readonly CancellationTokenSource _source; + + internal Lease(CancellationTokenSourceSlot owner, CancellationTokenSource source) + { + _owner = owner; + _source = source; + Token = source.Token; + } + + internal CancellationToken Token { get; } + + public void Dispose() => + Interlocked.Exchange(ref _owner, null)?.Release(_source); + } +} diff --git a/src/SkillView.Core/Ui/ContextBarView.cs b/src/SkillView.Core/Ui/ContextBarView.cs index 492132d..f64d098 100644 --- a/src/SkillView.Core/Ui/ContextBarView.cs +++ b/src/SkillView.Core/Ui/ContextBarView.cs @@ -13,9 +13,9 @@ internal readonly record struct ContextBarState( string? HealthLabel, string? FilterLabel); -/// One-line bar rendered directly below the tab strip that surfaces the active -/// agent, location, provenance, health, and quick-filter state as compact -/// labelled chips. +/// One-line bar rendered directly below the tab strip. It starts with the +/// active workspace title, then surfaces agent, location, provenance, health, +/// and quick-filter state as compact labelled chips. /// /// Rendering uses "Location" / "Install location" wording (never "roots"). /// "roots" is reserved for doctor-grade diagnostics only (§ spec §50). @@ -52,6 +52,7 @@ internal static string FormatForTests(ContextBarState state) { var sb = new StringBuilder(); + AppendChip(sb, state.Workspace); AppendChip(sb, state.AgentLabel); AppendLabeledValue(sb, "Location", state.LocationLabel); AppendLabeledValue(sb, "Source", state.ProvenanceLabel); diff --git a/src/SkillView.Core/Ui/HelpOverlay.cs b/src/SkillView.Core/Ui/HelpOverlay.cs index be78e3e..faac7f8 100644 --- a/src/SkillView.Core/Ui/HelpOverlay.cs +++ b/src/SkillView.Core/Ui/HelpOverlay.cs @@ -48,7 +48,9 @@ internal static string BuildMarkdown() => """ - **e** — toggle raw / rendered preview - **l** — show / hide logs - **r** — refresh the active tab -- **q / Esc** — quit at root · close modal otherwise +- **Ctrl+Q** — quit from anywhere, including text fields +- **q** — quit from a top-level list or preview +- **Esc** — leave a field, go back, or close a modal _Press **Esc**, **Enter**, or **?** to close._ """; diff --git a/src/SkillView.Core/Ui/InstallConfirmModal.cs b/src/SkillView.Core/Ui/InstallConfirmModal.cs index 7460e4a..32d8d67 100644 --- a/src/SkillView.Core/Ui/InstallConfirmModal.cs +++ b/src/SkillView.Core/Ui/InstallConfirmModal.cs @@ -70,6 +70,7 @@ internal InstallConfirmModal( internal Result Show() { + using var lifetime = new CancellationTokenSource(); var outcome = Outcome.Cancelled; InstallResult? installResult = null; @@ -139,14 +140,14 @@ internal Result Show() var status = new Label { - X = 1, + X = 3, Y = Pos.AnchorEnd(3), Width = Dim.Fill(2), Text = " ready", }; var spinner = new SpinnerView { - X = Pos.AnchorEnd(2), + X = 1, Y = Pos.AnchorEnd(3), Width = 1, Height = 1, @@ -201,6 +202,7 @@ internal Result Show() spinner.AutoSpin = true; installButton.Enabled = false; advancedButton.Enabled = false; + cancelButton.Enabled = false; status.Text = $" installing {_request.Repo}…"; try @@ -215,8 +217,10 @@ internal Result Show() _ghPath, _request.Repo, _request.SkillName, - options).ConfigureAwait(false); + options, + lifetime.Token).ConfigureAwait(false); + lifetime.Token.ThrowIfCancellationRequested(); _app.Invoke(() => { spinner.AutoSpin = false; @@ -235,9 +239,14 @@ internal Result Show() : $" install failed (exit {installResult.ExitCode}) — see logs"; installButton.Enabled = CurrentValidationError() is null; advancedButton.Enabled = true; + cancelButton.Enabled = true; } }); } + catch (OperationCanceledException) when (lifetime.IsCancellationRequested) + { + _logger.Debug("install.compact", "install canceled because the dialog closed"); + } catch (Exception ex) { _logger.Error("install.compact", ex.Message); @@ -249,6 +258,7 @@ internal Result Show() status.Text = $" install failed: {TuiHelpers.ErrorSnippet(ex.Message)}"; installButton.Enabled = CurrentValidationError() is null; advancedButton.Enabled = true; + cancelButton.Enabled = true; }); } }; @@ -256,12 +266,14 @@ internal Result Show() advancedButton.Accepting += (_, ev) => { ev.Handled = true; + lifetime.Cancel(); outcome = Outcome.EscalateToAdvanced; _app.RequestStop(); }; cancelButton.Accepting += (_, ev) => { ev.Handled = true; + lifetime.Cancel(); outcome = Outcome.Cancelled; _app.RequestStop(); }; @@ -271,6 +283,12 @@ internal Result Show() if (key.KeyCode == KeyCode.Esc) { key.Handled = true; + if (spinner.Visible) + { + status.Text = " install in progress — wait for completion"; + return; + } + lifetime.Cancel(); outcome = Outcome.Cancelled; _app.RequestStop(); } @@ -290,8 +308,15 @@ internal Result Show() TuiHelpers.ApplyScheme(SkillViewStyling.DialogSchemeName, box); } - _app.Run(dialog); - dialog.Dispose(); + try + { + _app.Run(dialog); + } + finally + { + lifetime.Cancel(); + dialog.Dispose(); + } return new Result(outcome, installResult); } diff --git a/src/SkillView.Core/Ui/InstallScreen.cs b/src/SkillView.Core/Ui/InstallScreen.cs index f4d3134..d3c27a3 100644 --- a/src/SkillView.Core/Ui/InstallScreen.cs +++ b/src/SkillView.Core/Ui/InstallScreen.cs @@ -279,14 +279,14 @@ void InvokeIfActive(Action action) var status = new Label { - X = 0, + X = 2, Y = Pos.AnchorEnd(3), - Width = Dim.Fill(10), + Width = Dim.Fill(2), Text = " ready — review the options, then press Install", }; var spinner = new SpinnerView { - X = Pos.AnchorEnd(10), + X = 0, Y = Pos.AnchorEnd(3), Width = 1, Height = 1, diff --git a/src/SkillView.Core/Ui/InstalledScreen.cs b/src/SkillView.Core/Ui/InstalledScreen.cs index 86c495d..946cc1b 100644 --- a/src/SkillView.Core/Ui/InstalledScreen.cs +++ b/src/SkillView.Core/Ui/InstalledScreen.cs @@ -49,7 +49,7 @@ internal static ShortcutDecision DecideShortcut(Key key, bool filterHasFocus, bo : default; } - if (key.KeyCode == KeyCode.Esc || key.AsRune.Value == 'q' || key.AsRune.Value == 'Q') + if (key.KeyCode == KeyCode.Esc) { return new ShortcutDecision(ShortcutCommand.Close, RequestStop: true); } diff --git a/src/SkillView.Core/Ui/LatestRequestGate.cs b/src/SkillView.Core/Ui/LatestRequestGate.cs new file mode 100644 index 0000000..9460fcb --- /dev/null +++ b/src/SkillView.Core/Ui/LatestRequestGate.cs @@ -0,0 +1,82 @@ +namespace SkillView.Ui; + +/// Owns one cancellable request at a time. Beginning a newer request cancels +/// the previous one, and each lease can cheaply tell whether its result is +/// still current before touching UI state. +internal sealed class LatestRequestGate : IDisposable +{ + private readonly object _gate = new(); + private CancellationTokenSource? _active; + private long _generation; + + internal Lease Begin(CancellationToken lifetime, TimeSpan timeout) + { + var cancellation = CancellationTokenSource.CreateLinkedTokenSource(lifetime); + cancellation.CancelAfter(timeout); + + long generation; + lock (_gate) + { + generation = ++_generation; + _active?.Cancel(); + _active = cancellation; + } + return new Lease(this, generation, cancellation); + } + + internal bool Cancel() + { + lock (_gate) + { + _generation++; + var active = _active; + active?.Cancel(); + _active = null; + return active is not null; + } + } + + public void Dispose() => _ = Cancel(); + + private bool IsCurrent(long generation, CancellationTokenSource cancellation) + { + lock (_gate) + { + return generation == _generation && ReferenceEquals(_active, cancellation); + } + } + + private void Release(long generation, CancellationTokenSource cancellation) + { + lock (_gate) + { + if (generation == _generation && ReferenceEquals(_active, cancellation)) + { + _active = null; + } + + cancellation.Dispose(); + } + } + + internal sealed class Lease : IDisposable + { + private LatestRequestGate? _owner; + private readonly long _generation; + private readonly CancellationTokenSource _cancellation; + + internal Lease(LatestRequestGate owner, long generation, CancellationTokenSource cancellation) + { + _owner = owner; + _generation = generation; + _cancellation = cancellation; + } + + internal CancellationToken Token => _cancellation.Token; + + internal bool IsCurrent => _owner?.IsCurrent(_generation, _cancellation) == true; + + public void Dispose() => + Interlocked.Exchange(ref _owner, null)?.Release(_generation, _cancellation); + } +} diff --git a/src/SkillView.Core/Ui/RepoSkillPickerModal.cs b/src/SkillView.Core/Ui/RepoSkillPickerModal.cs index 45ab88e..9e2cb65 100644 --- a/src/SkillView.Core/Ui/RepoSkillPickerModal.cs +++ b/src/SkillView.Core/Ui/RepoSkillPickerModal.cs @@ -63,6 +63,7 @@ internal RepoSkillPickerModal( internal Result Show() { + using var lifetime = new CancellationTokenSource(); var outcome = Outcome.Cancelled; var installedCount = 0; var failedCount = 0; @@ -161,14 +162,14 @@ internal Result Show() var status = new Label { - X = 1, + X = 3, Y = Pos.AnchorEnd(3), Width = Dim.Fill(2), Text = " ready", }; var spinner = new SpinnerView { - X = Pos.AnchorEnd(2), + X = 1, Y = Pos.AnchorEnd(3), Width = 1, Height = 1, @@ -235,6 +236,16 @@ void SetAll(CheckState state) RefreshValidity(); } + void SetOperationControlsEnabled(bool enabled) + { + installButton.Enabled = enabled && CurrentValidationError() is null; + cancelButton.Enabled = enabled; + scopeSelector.Enabled = enabled; + customPathField.Enabled = enabled; + listFrame.Enabled = enabled; + agentsView.Enabled = enabled; + } + installButton.Accepting += async (_, ev) => { ev.Handled = true; @@ -265,7 +276,7 @@ void SetAll(CheckState state) spinner.Visible = true; spinner.AutoSpin = true; - installButton.Enabled = false; + SetOperationControlsEnabled(false); var selectedAgentIds = new List(); for (var i = 0; i < entries.Length; i++) @@ -290,7 +301,8 @@ void SetAll(CheckState state) if (plan.UseAll) { status.Text = $" installing all {_skills.Length} skills…"; - var r = await _install.InstallAsync(_ghPath, _request.Repo, null, baseOptions).ConfigureAwait(false); + var r = await _install.InstallAsync( + _ghPath, _request.Repo, null, baseOptions, lifetime.Token).ConfigureAwait(false); if (r.Succeeded) installedCount = _skills.Length; else { failedCount = _skills.Length; firstError ??= r.ErrorMessage; } } @@ -298,15 +310,22 @@ void SetAll(CheckState state) { for (var i = 0; i < plan.SkillNames.Length; i++) { + lifetime.Token.ThrowIfCancellationRequested(); var name = plan.SkillNames[i]; var idx = i; - _app.Invoke(() => status.Text = $" installing {idx + 1}/{plan.SkillNames.Length}: {name}…"); - var r = await _install.InstallAsync(_ghPath, _request.Repo, name, baseOptions).ConfigureAwait(false); + _app.Invoke(() => + { + if (!lifetime.IsCancellationRequested) + status.Text = $" installing {idx + 1}/{plan.SkillNames.Length}: {name}…"; + }); + var r = await _install.InstallAsync( + _ghPath, _request.Repo, name, baseOptions, lifetime.Token).ConfigureAwait(false); if (r.Succeeded) installedCount++; else { failedCount++; firstError ??= r.ErrorMessage; } } } + lifetime.Token.ThrowIfCancellationRequested(); _app.Invoke(() => { spinner.AutoSpin = false; @@ -322,6 +341,7 @@ void SetAll(CheckState state) outcome = Outcome.Installed; status.Text = $" installed {installedCount}, {failedCount} failed — see logs (l)"; installButton.Enabled = false; + cancelButton.Enabled = true; } else { @@ -331,9 +351,14 @@ void SetAll(CheckState state) ? $" install failed: {snippet}" : " install failed — see logs (l)"; RefreshValidity(); + SetOperationControlsEnabled(true); } }); } + catch (OperationCanceledException) when (lifetime.IsCancellationRequested) + { + _logger.Debug("install.picker", "install canceled because the dialog closed"); + } catch (Exception ex) { _logger.Error("install.picker", ex.Message); @@ -344,6 +369,7 @@ void SetAll(CheckState state) outcome = installedCount > 0 ? Outcome.Installed : Outcome.Failed; status.Text = $" install failed: {TuiHelpers.ErrorSnippet(ex.Message)}"; RefreshValidity(); + SetOperationControlsEnabled(true); }); } }; @@ -351,6 +377,8 @@ void SetAll(CheckState state) cancelButton.Accepting += (_, ev) => { ev.Handled = true; + if (spinner.Visible) return; + lifetime.Cancel(); outcome = Outcome.Cancelled; _app.RequestStop(); }; @@ -360,15 +388,21 @@ void SetAll(CheckState state) if (key.KeyCode == KeyCode.Esc) { key.Handled = true; + if (spinner.Visible) + { + status.Text = " install in progress — wait for completion"; + return; + } + lifetime.Cancel(); outcome = Outcome.Cancelled; _app.RequestStop(); } - else if (!customPathField.HasFocus && key.AsRune.Value is 'a' or 'A') + else if (!spinner.Visible && !customPathField.HasFocus && key.AsRune.Value is 'a' or 'A') { key.Handled = true; SetAll(CheckState.Checked); } - else if (!customPathField.HasFocus && key.AsRune.Value is 'n' or 'N') + else if (!spinner.Visible && !customPathField.HasFocus && key.AsRune.Value is 'n' or 'N') { key.Handled = true; SetAll(CheckState.UnChecked); @@ -387,8 +421,15 @@ void SetAll(CheckState state) foreach (var box in agentBoxes) TuiHelpers.ApplyScheme(SkillViewStyling.DialogSchemeName, box); RefreshValidity(); - _app.Run(dialog); - dialog.Dispose(); + try + { + _app.Run(dialog); + } + finally + { + lifetime.Cancel(); + dialog.Dispose(); + } return new Result(outcome, installedCount, failedCount, firstError); } diff --git a/src/SkillView.Core/Ui/SearchAgentMetadataCache.cs b/src/SkillView.Core/Ui/SearchAgentMetadataCache.cs index 14f78fa..3d20b4a 100644 --- a/src/SkillView.Core/Ui/SearchAgentMetadataCache.cs +++ b/src/SkillView.Core/Ui/SearchAgentMetadataCache.cs @@ -6,8 +6,19 @@ namespace SkillView.Ui; internal sealed class SearchAgentMetadataCache { - private readonly Dictionary> _agentsByResult = + internal const int DefaultCapacity = 512; + + private readonly object _gate = new(); + private readonly int _capacity; + private readonly Dictionary> _agentsByResult = new(StringComparer.Ordinal); + private readonly LinkedList _lru = new(); + + internal SearchAgentMetadataCache(int capacity = DefaultCapacity) + { + ArgumentOutOfRangeException.ThrowIfLessThan(capacity, 1); + _capacity = capacity; + } internal static string? NormalizeAgent(string? value) { @@ -54,10 +65,37 @@ internal static ImmutableArray ExtractAgentsFromMarkdown(string markdown return builder.ToImmutable(); } - internal bool Has(SearchResultSkill result) => _agentsByResult.ContainsKey(BuildKey(result)); + internal bool Has(SearchResultSkill result) + { + lock (_gate) + { + if (!_agentsByResult.TryGetValue(BuildKey(result), out var node)) return false; + Touch(node); + return true; + } + } + + internal void Store(SearchResultSkill result, ImmutableArray agents) + { + var key = BuildKey(result); + lock (_gate) + { + if (_agentsByResult.TryGetValue(key, out var existing)) + { + existing.Value = existing.Value with { Agents = agents }; + Touch(existing); + return; + } - internal void Store(SearchResultSkill result, ImmutableArray agents) => - _agentsByResult[BuildKey(result)] = agents; + var node = _lru.AddFirst(new CacheEntry(key, agents)); + _agentsByResult.Add(key, node); + if (_agentsByResult.Count <= _capacity) return; + + var expired = _lru.Last!; + _lru.RemoveLast(); + _agentsByResult.Remove(expired.Value.Key); + } + } internal IReadOnlyList Filter( IReadOnlyList results, @@ -69,13 +107,29 @@ internal IReadOnlyList Filter( return results; } - return results - .Where(result => - _agentsByResult.TryGetValue(BuildKey(result), out var agents) - && agents.Any(agent => string.Equals(agent, normalized, StringComparison.OrdinalIgnoreCase))) - .ToArray(); + lock (_gate) + { + return results + .Where(result => + _agentsByResult.TryGetValue(BuildKey(result), out var node) + && node.Value.Agents.Any(agent => string.Equals(agent, normalized, StringComparison.OrdinalIgnoreCase))) + .ToArray(); + } + } + + internal int CountForTests + { + get { lock (_gate) return _agentsByResult.Count; } + } + + private void Touch(LinkedListNode node) + { + _lru.Remove(node); + _lru.AddFirst(node); } private static string BuildKey(SearchResultSkill result) => $"{result.Repo ?? string.Empty}\n{result.SkillName ?? string.Empty}\n{result.Path ?? string.Empty}"; + + private sealed record CacheEntry(string Key, ImmutableArray Agents); } diff --git a/src/SkillView.Core/Ui/SkillViewApp.cs b/src/SkillView.Core/Ui/SkillViewApp.cs index 4c70a94..86e19d5 100644 --- a/src/SkillView.Core/Ui/SkillViewApp.cs +++ b/src/SkillView.Core/Ui/SkillViewApp.cs @@ -35,9 +35,13 @@ public sealed class SkillViewApp private readonly bool _probeOnRun; private readonly SearchAgentMetadataCache _searchAgentMetadata = new(); private readonly SkillViewWorkflowCoordinator _workflows; + private const int MaxVisibleLogLines = 512; private IApplication? _app; + private Window? _mainWindow; + private IDisposable? _logSubscription; private CancellationTokenSource? _runLifetime; + private readonly LatestRequestGate _previewRequests = new(); private bool _hasRunLifetime; private TextField? _queryField; private TextField? _ownerField; @@ -50,8 +54,8 @@ public sealed class SkillViewApp private Editor? _logPane; private ContextBarView? _contextBar; private StatusStripView? _statusStrip; - private SpinnerView? _spinner; private TabBarView? _tabBar; + private TerminalSizeGuardView? _sizeGuard; private SkillViewTab _activeTab = SkillViewTab.Discover; private FrameView? _leftFrame; private SkillDetailPaneView? _detailPane; @@ -94,6 +98,9 @@ public sealed class SkillViewApp private bool _contextBarShown = true; private View? _lastDiscoverFocus; private string? _loadedPreviewKey; + private readonly object _visibleLogGate = new(); + private readonly Queue _visibleLogLines = new(MaxVisibleLogLines); + private int _logRefreshQueued; /// Sort modes for the Search tab results table. Mirrors winget-tui's /// app.sort_field cycle in src/app.rs. Off restores the natural ordering @@ -209,6 +216,9 @@ public async Task RunAsync(CancellationToken cancellationToken = default) AppDomain.CurrentDomain.UnhandledException -= onUnhandledException; CancelStatusAutoClear(); runLifetime.Cancel(); + CancelCurrentPreview(); + DisposeLogSubscription(); + DetachApplicationKeyHandler(); _hasRunLifetime = false; _runLifetime = null; _app = null; @@ -242,6 +252,14 @@ private Window BuildUi() Width = Dim.Fill(), Height = Dim.Fill(), }; + _mainWindow = window; + if (_app is not null) + { + // Keyboard.KeyDown is raised before focused views. That makes it + // the reliable place for app-wide quit keys even when TableView or + // a text editor would otherwise consume the key first. + _app.Keyboard.KeyDown += OnApplicationKeyDown; + } // Top header strip: tabs on the right, "Skill View" wordmark on the // left. Stays present across all tab views, mirroring winget-tui's @@ -309,6 +327,7 @@ private Window BuildUi() }; _resultsTable.ValueChanged += (_, _) => { + CancelCurrentPreview(); UpdatePreviewPlaceholder(); UpdateMetadataPane(); }; @@ -338,40 +357,48 @@ private Window BuildUi() Y = Pos.AnchorEnd(1), Width = Dim.Fill(), }; - _spinner = new SpinnerView - { - X = Pos.AnchorEnd(10), - Y = Pos.AnchorEnd(2), - Width = 1, - Height = 1, - Visible = false, - AutoSpin = false, - Style = new SpinnerStyle.Dots(), - }; - - TuiHelpers.ApplyScheme(SkillViewStyling.BaseSchemeName, window, _spinner); + _sizeGuard = new TerminalSizeGuardView(); RefreshHiddenDirUi(); - Func runOnUi = action => + Func runOnUi = async action => { - var tcs = new TaskCompletionSource(); - Invoke(() => + var cancellationToken = GetRunLifetimeToken(); + if (cancellationToken.IsCancellationRequested) return; + var app = _app; + if (app is null) return; + + var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + using var registration = cancellationToken.Register(() => tcs.TrySetCanceled(cancellationToken)); + try { - try { action(); tcs.TrySetResult(); } - catch (Exception ex) { tcs.TrySetException(ex); } - }); - return tcs.Task; + app.Invoke(() => + { + if (cancellationToken.IsCancellationRequested) + { + tcs.TrySetCanceled(cancellationToken); + return; + } + try { action(); tcs.TrySetResult(); } + catch (Exception ex) { tcs.TrySetException(ex); } + }); + } + catch when (cancellationToken.IsCancellationRequested) + { + return; + } + await tcs.Task.ConfigureAwait(false); }; _installedTab = new SkillView.Ui.Tabs.InstalledTabView( runOnUi: runOnUi, - snapshotLoader: () => _workflows.CaptureInventorySnapshotAsync(GetRunLifetimeToken()), + snapshotLoader: token => _workflows.CaptureInventorySnapshotAsync(token), onRemove: (skill, snap) => _workflows.OpenRemoveDialog(skill, snap), onLeaveTab: () => ActivateTab(SkillViewTab.Discover), onGoToSearch: () => { ActivateTab(SkillViewTab.Discover); FocusSearchFromInstalled(); }, onStateChange: RefreshShellChrome, // Scope cycle (`G`) pushes `--scope` down to `gh skill list`. - scopedSnapshotLoader: scope => _workflows.CaptureInventorySnapshotAsync(scope, GetRunLifetimeToken())) + scopedSnapshotLoader: (scope, token) => _workflows.CaptureInventorySnapshotAsync(scope, token), + lifetimeToken: GetRunLifetimeToken()) { X = 0, Y = 2, @@ -382,12 +409,13 @@ private Window BuildUi() _updatesTab = new SkillView.Ui.Tabs.UpdatesTabView( runOnUi: runOnUi, - snapshotLoader: () => _workflows.CaptureInventorySnapshotAsync(GetRunLifetimeToken()), - updateServiceFactory: () => _services.UpdateService, + snapshotLoader: token => _workflows.CaptureInventorySnapshotAsync(token), + updateRunner: (ghPath, options, token) => _services.UpdateService.UpdateAsync(ghPath, options, token), ghPathProvider: () => _ghPath, logger: _services.Logger, onLeaveTab: LeaveUpdates, - onUpdateApplied: () => _services.ListAdapter.Invalidate()) + onUpdateApplied: () => _services.ListAdapter.Invalidate(), + lifetimeToken: GetRunLifetimeToken()) { X = 0, Y = 2, @@ -408,10 +436,11 @@ private Window BuildUi() _changesTab = new SkillView.Ui.Tabs.ChangesTabView( runOnUi: runOnUi, - snapshotLoader: () => _workflows.CaptureInventorySnapshotAsync(GetRunLifetimeToken()), + snapshotLoader: token => _workflows.CaptureInventorySnapshotAsync(token), onActivateUpdates: () => { _openedUpdatesFromChanges = true; + _changesTab?.CancelPendingLoad(); _workflows.OpenUpdatesFromChanges( hideChanges: () => _changesTab!.Visible = false, activateUpdates: hideChanges => _updatesTab!.ActivateFromChanges(hideChanges)); @@ -421,7 +450,8 @@ private Window BuildUi() hideChanges: () => { if (_changesTab is not null) _changesTab.Visible = false; }, enterDoctor: EnterDoctor), onLeaveTab: () => ActivateTab(SkillViewTab.Discover), - onStateChange: UpdateContextBar) + onStateChange: UpdateContextBar, + lifetimeToken: GetRunLifetimeToken()) { X = 0, Y = 2, @@ -431,7 +461,9 @@ private Window BuildUi() window.Add(_tabBar, _contextBar, _discoverTab, _installedTab, _updatesTab, _doctorTab, _changesTab, - _spinner, _statusStrip); + _statusStrip, _sizeGuard); + window.FrameChanged += (_, _) => + _sizeGuard.UpdateForSize(window.Frame.Width, window.Frame.Height); window.KeyDown += OnWindowKeyDown; AttachStartupPointerAndKeyTracking( window, @@ -452,7 +484,13 @@ private Window BuildUi() _resultsTable); RefreshResultsTable(); - _services.Logger.Subscribe(OnLogEntry); + DisposeLogSubscription(); + _logSubscription = _services.Logger.Subscribe(OnLogEntry); + window.Disposing += (_, _) => + { + DisposeLogSubscription(); + DetachApplicationKeyHandler(); + }; UpdateContextBar(); if (TuiHelpers.IsWarpTerminal) @@ -473,6 +511,42 @@ private void OnWindowKeyDown(object? sender, Key key) } } + private void OnApplicationKeyDown(object? sender, Key key) + { + if (key.Handled) return; + var app = _app; + var mainWindow = _mainWindow; + if (app is null || mainWindow is null) return; + + if (IsUnconditionalQuitKey(key)) + { + var top = app.TopRunnable; + if (top is not null && !ReferenceEquals(top, mainWindow)) + { + app.RequestStop(top); + } + app.RequestStop(mainWindow); + key.Handled = true; + return; + } + + if (_sizeGuard?.Visible == true) + { + // Do not let hidden controls mutate state while the resize guard + // is covering them. Ctrl+Q above remains available. + key.Handled = true; + return; + } + + if (ReferenceEquals(app.TopRunnableView, mainWindow) + && !TextInputHasFocus() + && key.AsRune.Value is 'q' or 'Q') + { + app.RequestStop(mainWindow); + key.Handled = true; + } + } + private void RememberStickyFocus(View view) { if (ReferenceEquals(view, _queryField) @@ -501,6 +575,14 @@ private void RestoreDiscoverFocus() /// Returns true if the key was consumed. private bool OnWindowShortcut(Key key) { + // Ctrl+Q is the unconditional escape hatch. Handle it before the + // printable-input guard so quitting still works while typing. + if (IsUnconditionalQuitKey(key)) + { + _app?.RequestStop(); + return true; + } + // Let printable text keep going to the focused input, but keep global // navigation (tab arrows, help, slash, etc.) available everywhere. if (TextInputHasFocus() && IsPrintableTextInputKey(key)) @@ -516,7 +598,9 @@ private bool OnWindowShortcut(Key key) // top-level list where Esc previously meant "lose your session". if (key.KeyCode == KeyCode.Esc) { - SetStatus("Press q to quit"); + SetStatus(TextInputHasFocus() + ? "Esc leaves the field · Ctrl+Q quits" + : "Press q or Ctrl+Q to quit"); return true; } @@ -579,11 +663,20 @@ private bool OnWindowShortcut(Key key) return false; } - private bool TextInputHasFocus() => - _queryField?.HasFocus == true - || _ownerField?.HasFocus == true - || _agentField?.HasFocus == true - || _limitUpDown?.HasFocus == true; + private bool TextInputHasFocus() + { + if (_inDoctor) return false; + if (_activeTab == SkillViewTab.Installed) + { + return _installedTab?.FilterHasFocus == true; + } + return _activeTab == SkillViewTab.Discover + && _discoverTab?.Visible == true + && (_queryField?.HasFocus == true + || _ownerField?.HasFocus == true + || _agentField?.HasFocus == true + || _limitUpDown?.HasFocus == true); + } private static bool IsPrintableTextInputKey(Key key) { @@ -598,12 +691,16 @@ private static bool IsPrintableTextInputKey(Key key) && key.KeyCode != KeyCode.CursorRight; } + internal static bool IsUnconditionalQuitKey(Key key) => + key.KeyCode == (KeyCode.Q | KeyCode.CtrlMask); + /// Switch active tab. All three (Discover / Installed / Changes) are /// embedded views — flipping the Visible flags swaps them in-place /// without re-running the app loop. private void ActivateTab(SkillViewTab tab) { if (tab == _activeTab) return; + CancelPendingTabWork(); _activeTab = tab; _tabBar?.SetActiveTab(tab); @@ -669,6 +766,7 @@ private void EnterDoctor() if (_inDoctor || _doctorTab is null) return; _tabBeforeDoctor = _activeTab; _inDoctor = true; + CancelPendingTabWork(); ShowSearchPanes(false); if (_installedTab is not null) _installedTab.Visible = false; if (_updatesTab is not null) _updatesTab.Visible = false; @@ -701,6 +799,13 @@ private void EnterDoctor() }, "doctor"); } + private void CancelPendingTabWork() + { + _installedTab?.CancelPendingLoad(); + _changesTab?.CancelPendingLoad(); + _updatesTab?.CancelPendingWork(); + } + private void LeaveDoctor() { if (!_inDoctor || _doctorTab is null) return; @@ -1017,12 +1122,11 @@ private async Task PreviewSelectedAsync() return; } - SetBusy($"preview {repo}/{pick.SkillName}…"); var runCancellationToken = GetRunLifetimeToken(); + using var request = _previewRequests.Begin(runCancellationToken, PreviewTimeout); + SetBusy($"preview {repo}/{pick.SkillName}…"); try { - using var cts = CancellationTokenSource.CreateLinkedTokenSource(runCancellationToken); - cts.CancelAfter(PreviewTimeout); _services.Logger.Info("preview", $"loading {repo}/{pick.SkillName}…"); var preview = await _services.PreviewService .PreviewAsync( @@ -1030,11 +1134,12 @@ private async Task PreviewSelectedAsync() repo, pick.SkillName, allowHiddenDirs: ShouldAllowHiddenDirs(pick, HiddenDirsEnabled), - cancellationToken: cts.Token) + cancellationToken: request.Token) .ConfigureAwait(false); _services.Logger.Debug("preview", $"PreviewAsync returned: succeeded={preview.Succeeded} exit={preview.ExitCode} bodyLen={preview.Body?.Length ?? 0}"); Invoke(() => { + if (!request.IsCurrent) return; _loadedPreviewKey = preview.Succeeded ? BuildPreviewSelectionKey(pick) : null; SetPreviewText(preview.Succeeded ? preview.MarkdownBody ?? preview.Body ?? "(empty preview)" @@ -1063,9 +1168,15 @@ private async Task PreviewSelectedAsync() } catch (OperationCanceledException) { + if (!request.IsCurrent) + { + _services.Logger.Debug("preview", "preview superseded by a newer selection"); + return; + } _services.Logger.Warn("preview", "preview timed out"); Invoke(() => { + if (!request.IsCurrent) return; _loadedPreviewKey = null; SetPreviewText("(preview timed out)\n\nThe gh subprocess did not respond within 30 seconds."); SetStatus("preview timed out", TuiHelpers.NotificationLevel.Error); @@ -1077,6 +1188,7 @@ private async Task PreviewSelectedAsync() var snippet = TuiHelpers.ErrorSnippet(ex.Message); Invoke(() => { + if (!request.IsCurrent) return; _loadedPreviewKey = null; SetPreviewText(snippet.Length > 0 ? $"(preview failed)\n\n{snippet}" @@ -1090,7 +1202,10 @@ private async Task PreviewSelectedAsync() } finally { - Invoke(ClearBusy); + if (request.IsCurrent) + { + Invoke(ClearBusy); + } } } @@ -1454,10 +1569,10 @@ private void UpdateContextBar() var workspaceName = _activeTab switch { - _ when _inDoctor => "Doctor", - SkillViewTab.Discover => "Discover", - SkillViewTab.Installed => "Installed", - SkillViewTab.Changes => "Changes", + _ when _inDoctor => "Doctor — Environment diagnostics", + SkillViewTab.Discover => "Discover skills", + SkillViewTab.Installed => "Installed skills", + SkillViewTab.Changes => "Review changes", _ => null }; @@ -1607,13 +1722,8 @@ private void ToggleRightPane() else { ShowLogPane(); - var log = string.Join('\n', _services.Logger.Snapshot().Select(Logger.Format)); - if (_logPane is not null) - { - _logPane.Text = log.Length > 0 - ? TerminalEscapeSanitizer.Sanitize(log) ?? string.Empty - : "(no log entries yet)"; - } + InitializeVisibleLogLines(); + FlushVisibleLogLines(); } } @@ -1797,16 +1907,78 @@ private void FocusSearchFromInstalled() RestoreDefaultStatus(); } - private void OnLogEntry(LogEntry _) + private void OnLogEntry(LogEntry entry) { if (!_showingLogs) return; - Invoke(() => + var line = TerminalEscapeSanitizer.Sanitize(Logger.Format(entry)) ?? string.Empty; + lock (_visibleLogGate) { - if (_logPane is not null) + _visibleLogLines.Enqueue(line); + while (_visibleLogLines.Count > MaxVisibleLogLines) { - _logPane.Text = string.Join('\n', _services.Logger.Snapshot().Select(Logger.Format)); + _visibleLogLines.Dequeue(); } - }); + } + + // Coalesce bursts into one UI refresh. This avoids rebuilding and + // redrawing the whole log pane once per individual entry. + if (Interlocked.Exchange(ref _logRefreshQueued, 1) == 0) + { + Invoke(FlushVisibleLogLines); + } + } + + private void InitializeVisibleLogLines() + { + var lines = _services.Logger.Snapshot() + .TakeLast(MaxVisibleLogLines) + .Select(entry => TerminalEscapeSanitizer.Sanitize(Logger.Format(entry)) ?? string.Empty); + lock (_visibleLogGate) + { + _visibleLogLines.Clear(); + foreach (var line in lines) _visibleLogLines.Enqueue(line); + } + } + + private void FlushVisibleLogLines() + { + string text; + lock (_visibleLogGate) + { + text = _visibleLogLines.Count == 0 + ? "(no log entries yet)" + : string.Join('\n', _visibleLogLines); + + // Reset while the queue snapshot is protected. An entry enqueued + // after this point must observe zero and schedule another flush. + Interlocked.Exchange(ref _logRefreshQueued, 0); + } + if (_showingLogs && _logPane is not null) + { + _logPane.Text = text; + } + } + + private void CancelCurrentPreview() + { + if (_previewRequests.Cancel()) + { + Invoke(ClearBusy); + } + } + + private void DisposeLogSubscription() + { + Interlocked.Exchange(ref _logSubscription, null)?.Dispose(); + } + + private void DetachApplicationKeyHandler() + { + if (_app is not null) + { + _app.Keyboard.KeyDown -= OnApplicationKeyDown; + } + _mainWindow = null; } private void SetStatus(string text) => SetStatus(text, TuiHelpers.NotificationLevel.Info); @@ -1850,6 +2022,7 @@ private List GetCurrentHints() return [ new StatusHint("l", "Preview"), new StatusHint("?", "Help"), + new StatusHint("Ctrl+Q", "Quit"), ]; } @@ -1858,6 +2031,7 @@ private List GetCurrentHints() return [ new StatusHint("Esc", "Back"), new StatusHint("?", "Help"), + new StatusHint("Ctrl+Q", "Quit"), ]; } @@ -1865,23 +2039,29 @@ private List GetCurrentHints() { SkillViewTab.Discover => [ new StatusHint("f", "Filters"), - new StatusHint("1/2/3", "Tabs"), - new StatusHint("?", "Help"), + new StatusHint("1/2/3", "Tabs"), + new StatusHint("?", "Help"), + new StatusHint("Ctrl+Q", "Quit"), ], SkillViewTab.Installed => [ new StatusHint("f", "Filter"), + new StatusHint("s", "Sort"), + new StatusHint("P", "Pins"), + new StatusHint("G", "Scope"), new StatusHint("x", "Remove"), - new StatusHint("1/2/3", "Tabs"), new StatusHint("?", "Help"), + new StatusHint("Ctrl+Q", "Quit"), ], SkillViewTab.Changes => [ new StatusHint("Enter", "Open"), new StatusHint("c", "Cleanup"), new StatusHint("d", "Doctor"), new StatusHint("?", "Help"), + new StatusHint("Ctrl+Q", "Quit"), ], _ => [ new StatusHint("?", "Help"), + new StatusHint("Ctrl+Q", "Quit"), ], }; } @@ -1928,21 +2108,13 @@ private void CancelStatusAutoClear() private void SetBusy(string text) => Invoke(() => { - if (_spinner is not null) - { - _spinner.Visible = true; - _spinner.AutoSpin = true; - } + _statusStrip?.SetBusy(true); UpdateStatusStrip(text, TuiHelpers.NotificationLevel.Info); }); private void ClearBusy() { - if (_spinner is not null) - { - _spinner.AutoSpin = false; - _spinner.Visible = false; - } + _statusStrip?.SetBusy(false); } private void Invoke(Action action) diff --git a/src/SkillView.Core/Ui/StatusStripView.cs b/src/SkillView.Core/Ui/StatusStripView.cs index 26627c9..b5615a2 100644 --- a/src/SkillView.Core/Ui/StatusStripView.cs +++ b/src/SkillView.Core/Ui/StatusStripView.cs @@ -3,6 +3,7 @@ using SkillView.Ui.Theming; using Terminal.Gui.Drawing; using Terminal.Gui.ViewBase; +using Terminal.Gui.Views; namespace SkillView.Ui; @@ -20,6 +21,8 @@ internal sealed class StatusStripView : View private string _statusText = string.Empty; private ImmutableArray _hints = []; private string _leftBadges = string.Empty; + private readonly SpinnerView _spinner; + private bool _isBusy; internal StatusStripView() { @@ -27,6 +30,19 @@ internal StatusStripView() Height = 1; Width = Dim.Fill(); SchemeName = SchemeNames.Base; + + _spinner = new SpinnerView + { + X = 0, + Y = 0, + Width = 1, + Height = 1, + Visible = false, + AutoSpin = false, + Style = new SpinnerStyle.Dots(), + }; + _spinner.SetScheme(TuiHelpers.CreateStatusScheme(TuiHelpers.NotificationLevel.Info)); + Add(_spinner); } internal void Update(string statusText, IEnumerable hints, string leftBadges = "") @@ -37,6 +53,16 @@ internal void Update(string statusText, IEnumerable hints, string le SetNeedsDraw(); } + internal void SetBusy(bool isBusy) + { + _isBusy = isBusy; + _spinner.Visible = isBusy; + _spinner.AutoSpin = isBusy; + SetNeedsDraw(); + } + + internal bool IsBusyForTests => _isBusy; + internal string LeftBadgesForTests => _leftBadges; protected override bool OnDrawingContent(DrawContext? context) @@ -64,14 +90,17 @@ protected override bool OnDrawingContent(DrawContext? context) if (_statusText.Length > 0) { var centerX = Math.Max(x, width / 4); - Move(centerX, 0); + _spinner.X = centerX; + var textX = centerX + (_isBusy ? 2 : 0); + Move(textX, 0); SetAttribute(statusText); - AddStr(TuiHelpers.Truncate(_statusText, width / 2)); + AddStr(TuiHelpers.Truncate(_statusText, Math.Max(0, width / 2 - (_isBusy ? 2 : 0)))); } // Right hints — budget excludes left badges AND the center-status // region so hints can never overwrite the status text (Bug 1). - var leftEdge = ComputeHintLeftEdge(width, x, _statusText); + var measuredStatus = _isBusy ? " " + _statusText : _statusText; + var leftEdge = ComputeHintLeftEdge(width, x, measuredStatus); var visibleHints = TruncateHintsForTests(_hints, width - leftEdge); if (visibleHints.Count > 0) { diff --git a/src/SkillView.Core/Ui/TabBarView.cs b/src/SkillView.Core/Ui/TabBarView.cs index 0107291..2c2e420 100644 --- a/src/SkillView.Core/Ui/TabBarView.cs +++ b/src/SkillView.Core/Ui/TabBarView.cs @@ -70,9 +70,8 @@ protected override bool OnDrawingContent(DrawContext? context) var width = viewport.Width; if (width <= 0) return true; - // Build right-anchored tabs: " Skill View ◇ Discover ▣ Installed △ Changes " - // Logo on the left, tabs flush right with a 2-cell gap between pills. - const string logo = " Skill View"; + // Branding already lives in the window title. Keep this row focused on + // navigation and leave the line below it for the active workspace title. var inactiveFg = WingetTuiTheme.TextSecondary; var activeFg = WingetTuiTheme.Accent; var background = WingetTuiTheme.Background; @@ -82,16 +81,12 @@ protected override bool OnDrawingContent(DrawContext? context) SetAttribute(new Attribute(WingetTuiTheme.TextPrimary, background)); AddStr(new string(' ', width)); - Move(0, 0); - SetAttribute(new Attribute(WingetTuiTheme.Accent, background, TextStyle.Bold)); - AddStr(logo); - // Compute pill total width so we can right-align. const int gap = 3; var pillWidths = Tabs.Select(t => Pill(t.Icon, t.Label).Length).ToArray(); var pillsTotal = pillWidths.Sum() + gap * (Tabs.Length - 1); - var x = Math.Max(logo.Length + 4, width - pillsTotal - 2); + var x = Math.Max(0, width - pillsTotal - 2); _tabRegions.Clear(); for (var i = 0; i < Tabs.Length; i++) { diff --git a/src/SkillView.Core/Ui/Tabs/ChangesTabView.cs b/src/SkillView.Core/Ui/Tabs/ChangesTabView.cs index 87c3e86..23d36b4 100644 --- a/src/SkillView.Core/Ui/Tabs/ChangesTabView.cs +++ b/src/SkillView.Core/Ui/Tabs/ChangesTabView.cs @@ -21,27 +21,31 @@ internal sealed class ChangesTabView : FrameView { private const int KindColumnWidth = 11; private readonly Func _runOnUi; - private readonly Func> _snapshotLoader; + private readonly Func> _snapshotLoader; private readonly Action _onActivateUpdates; private readonly Action _onActivateCleanup; private readonly Action _onActivateDoctor; private readonly Action _onLeaveTab; private readonly Action? _onStateChange; + private readonly CancellationToken _lifetimeToken; private readonly TableView _table; private readonly Markdown _detail; private readonly Label _status; + private readonly SpinnerView _spinner; private IReadOnlyList _rows = Array.Empty(); private long _loadGeneration; + private readonly CancellationTokenSourceSlot _loadCancellations = new(); internal ChangesTabView( Func runOnUi, - Func> snapshotLoader, + Func> snapshotLoader, Action onActivateUpdates, Action onActivateCleanup, Action onActivateDoctor, Action onLeaveTab, - Action? onStateChange = null) + Action? onStateChange = null, + CancellationToken lifetimeToken = default) { _runOnUi = runOnUi; _snapshotLoader = snapshotLoader; @@ -50,6 +54,7 @@ internal ChangesTabView( _onActivateDoctor = onActivateDoctor; _onLeaveTab = onLeaveTab; _onStateChange = onStateChange; + _lifetimeToken = lifetimeToken; BorderStyle = LineStyle.None; SchemeName = SchemeNames.Base; @@ -80,29 +85,42 @@ internal ChangesTabView( _status = new Label { - X = 0, + X = 2, Y = Pos.AnchorEnd(1), - Width = Dim.Fill(), + Width = Dim.Fill(2), Text = " loading…", }; + _spinner = new SpinnerView + { + X = 0, + Y = Pos.AnchorEnd(1), + Width = 1, + Height = 1, + Visible = true, + AutoSpin = true, + Style = new SpinnerStyle.Dots(), + }; - TuiHelpers.ApplyScheme(SchemeNames.Base, this, _table, _detail, _status); + TuiHelpers.ApplyScheme(SchemeNames.Base, this, _table, _detail, _spinner, _status); _table.KeyDown += OnTableKeyDown; - Add(_table, _detail, _status); + Add(_table, _detail, _spinner, _status); + Disposing += (_, _) => CancelPendingLoad(); } /// Load the maintenance queue from the inventory snapshot. internal async Task LoadAsync() { var gen = Interlocked.Increment(ref _loadGeneration); + using var loadCancellation = _loadCancellations.Replace(_lifetimeToken); Visible = true; + SetLoading(true); _status.Text = " loading inventory…"; try { - var snapshot = await _snapshotLoader().ConfigureAwait(false); + var snapshot = await _snapshotLoader(loadCancellation.Token).ConfigureAwait(false); // Only queue items backed by real pending state. // Update availability is unknown until the user runs a dry-run; Doctor // is always accessible via 'd'. Neither belongs in the pending queue. @@ -125,12 +143,17 @@ await _runOnUi(() => summary); }).ConfigureAwait(false); } + catch (OperationCanceledException) when (loadCancellation.Token.IsCancellationRequested) + { + // A newer load or tab deactivation superseded this one. + } catch (Exception ex) { await _runOnUi(() => { if (Interlocked.Read(ref _loadGeneration) != gen) return; _status.Text = $" load failed: {TuiHelpers.ErrorSnippet(ex.Message)}"; + SetLoading(false); }).ConfigureAwait(false); } } @@ -177,6 +200,7 @@ private void LoadRows( } _onStateChange?.Invoke(); + SetLoading(false); } // Tests only ----------------------------------------------------------- @@ -313,4 +337,17 @@ private static string DescribeWorkspaceSummary(InventorySnapshot snapshot, bool return "No cleanup items"; } + + internal void CancelPendingLoad() + { + Interlocked.Increment(ref _loadGeneration); + _loadCancellations.Cancel(); + SetLoading(false); + } + + private void SetLoading(bool loading) + { + _spinner.Visible = loading; + _spinner.AutoSpin = loading; + } } diff --git a/src/SkillView.Core/Ui/Tabs/DoctorTabView.cs b/src/SkillView.Core/Ui/Tabs/DoctorTabView.cs index 9340b17..7722fee 100644 --- a/src/SkillView.Core/Ui/Tabs/DoctorTabView.cs +++ b/src/SkillView.Core/Ui/Tabs/DoctorTabView.cs @@ -14,7 +14,7 @@ namespace SkillView.Ui.Tabs; /// so the 3 DoctorScreenTests stay green unchanged. /// /// Doctor isn't a primary tab (no pill in TabBarView). It's reached via -/// `d` and dismissed via Esc / q — which restores the previously-active +/// `d` and dismissed via Esc — which restores the previously-active /// primary tab through onLeaveTab. internal sealed class DoctorTabView : FrameView { @@ -43,7 +43,7 @@ internal DoctorTabView(Action onLeaveTab) KeyDown += (_, key) => { if (key.Handled) return; - if (key.KeyCode == KeyCode.Esc || key.AsRune.Value == 'q' || key.AsRune.Value == 'Q') + if (key.KeyCode == KeyCode.Esc) { key.Handled = true; _onLeaveTab(); diff --git a/src/SkillView.Core/Ui/Tabs/InstalledTabView.cs b/src/SkillView.Core/Ui/Tabs/InstalledTabView.cs index 4468f81..15884b2 100644 --- a/src/SkillView.Core/Ui/Tabs/InstalledTabView.cs +++ b/src/SkillView.Core/Ui/Tabs/InstalledTabView.cs @@ -33,17 +33,19 @@ internal enum PinFilter { All, PinnedOnly, UnpinnedOnly } internal enum ScopeFilter { All, User, Project, Custom } private readonly Func _runOnUi; - private readonly Func> _snapshotLoader; - private readonly Func>? _scopedSnapshotLoader; + private readonly Func> _snapshotLoader; + private readonly Func>? _scopedSnapshotLoader; private readonly Action _onRemove; private readonly Action _onLeaveTab; private readonly Action _onGoToSearch; private readonly Action? _onStateChange; + private readonly CancellationToken _lifetimeToken; private readonly TextField _filterField; private readonly TableView _table; private readonly Markdown _detail; private readonly Label _footer; + private readonly SpinnerView _spinner; private InventorySnapshot? _snapshot; private IReadOnlyList _rows = Array.Empty(); private IReadOnlyList _all = Array.Empty(); @@ -58,15 +60,17 @@ internal enum ScopeFilter { All, User, Project, Custom } private int _agentsW = 8; private FocusTarget _lastFocusedTarget = FocusTarget.Table; private long _loadGeneration; + private readonly CancellationTokenSourceSlot _loadCancellations = new(); internal InstalledTabView( Func runOnUi, - Func> snapshotLoader, + Func> snapshotLoader, Action onRemove, Action onLeaveTab, Action onGoToSearch, Action? onStateChange = null, - Func>? scopedSnapshotLoader = null) + Func>? scopedSnapshotLoader = null, + CancellationToken lifetimeToken = default) { _runOnUi = runOnUi; _snapshotLoader = snapshotLoader; @@ -75,19 +79,13 @@ internal InstalledTabView( _onLeaveTab = onLeaveTab; _onGoToSearch = onGoToSearch; _onStateChange = onStateChange; + _lifetimeToken = lifetimeToken; BorderStyle = LineStyle.None; SchemeName = SchemeNames.Base; Visible = false; var filterLabel = new Label { Text = "Filter:", X = 0, Y = 0 }; - var guideLabel = new Label - { - X = 0, - Y = 1, - Width = Dim.Fill(), - Text = BuildGuideText(), - }; _filterField = new TextField { X = 8, @@ -95,6 +93,22 @@ internal InstalledTabView( Width = Dim.Percent(60) - 8, Text = string.Empty, }; + var searchScopeLabel = new Label + { + X = Pos.Right(_filterField) + 1, + Y = 0, + Width = Dim.Fill(), + Text = BuildSearchScopeText(), + Enabled = false, + }; + var legendLabel = new Label + { + X = 0, + Y = 1, + Width = Dim.Fill(), + Text = BuildLegendText(), + Enabled = false, + }; TuiHelpers.ConfigureTextInput(_filterField, SchemeNames.Base); _filterField.HasFocusChanged += (_, _) => { @@ -135,11 +149,21 @@ internal InstalledTabView( _footer = new Label { - X = 0, + X = 2, Y = Pos.AnchorEnd(1), - Width = Dim.Fill(), + Width = Dim.Fill(2), Text = " loading inventory…", }; + _spinner = new SpinnerView + { + X = 0, + Y = Pos.AnchorEnd(1), + Width = 1, + Height = 1, + Visible = true, + AutoSpin = true, + Style = new SpinnerStyle.Dots(), + }; _filterField.TextChanged += (_, _) => RefreshAll(); _filterField.KeyDown += (_, key) => @@ -178,9 +202,10 @@ internal InstalledTabView( KeyDown += OnKeyDown; TuiHelpers.ApplyScheme(SchemeNames.Base, - this, filterLabel, guideLabel, _filterField, _table, _detail, _footer); + this, filterLabel, searchScopeLabel, legendLabel, _filterField, _table, _detail, _footer, _spinner); - Add(filterLabel, guideLabel, _filterField, _table, _detail, _footer); + Add(filterLabel, searchScopeLabel, legendLabel, _filterField, _table, _detail, _spinner, _footer); + Disposing += (_, _) => CancelPendingLoad(); } /// Kick off a snapshot load and reveal the tab. Safe to call repeatedly — @@ -188,11 +213,13 @@ internal InstalledTabView( internal async Task LoadAsync() { var loadGeneration = Interlocked.Increment(ref _loadGeneration); + using var loadCancellation = _loadCancellations.Replace(_lifetimeToken); Visible = true; + SetLoading(true); _footer.Text = " loading inventory…"; try { - var snapshot = await CaptureSnapshotAsync().ConfigureAwait(false); + var snapshot = await CaptureSnapshotAsync(loadCancellation.Token).ConfigureAwait(false); await _runOnUi(() => { if (!IsCurrentLoad(loadGeneration)) @@ -203,6 +230,10 @@ await _runOnUi(() => Populate(snapshot); }).ConfigureAwait(false); } + catch (OperationCanceledException) when (loadCancellation.Token.IsCancellationRequested) + { + // A newer load or tab deactivation superseded this one. + } catch (Exception ex) { await _runOnUi(() => @@ -213,6 +244,7 @@ await _runOnUi(() => } _footer.Text = $" inventory load failed: {TuiHelpers.ErrorSnippet(ex.Message)}"; + SetLoading(false); }).ConfigureAwait(false); } } @@ -221,6 +253,7 @@ await _runOnUi(() => /// another async load. Used by startup auto-open so we don't double-scan. internal void LoadSeeded(InventorySnapshot snapshot) { + CancelPendingLoad(); Interlocked.Increment(ref _loadGeneration); Visible = true; Populate(snapshot); @@ -246,6 +279,7 @@ private void Populate(InventorySnapshot snapshot) _detail.Text = _rows.Count == 0 ? "(no matches)" : RenderDetail(_rows[0]); RestoreFocus(); _onStateChange?.Invoke(); + SetLoading(false); } private void ApplyFilter() @@ -280,10 +314,10 @@ private void ApplyFilter() /// is wired and the filter is user/project, the capture pushes `--scope` /// down to `gh skill list`; otherwise it captures everything and the /// in-process filter narrows the view. - private Task CaptureSnapshotAsync() => + private Task CaptureSnapshotAsync(CancellationToken cancellationToken) => _scopedSnapshotLoader is not null - ? _scopedSnapshotLoader(ToGhScope(_scopeFilter)) - : _snapshotLoader(); + ? _scopedSnapshotLoader(ToGhScope(_scopeFilter), cancellationToken) + : _snapshotLoader(cancellationToken); internal static PinFilter CyclePin(PinFilter current) => current switch { @@ -442,8 +476,10 @@ private static int ResolveWidthForLayout(int viewportWidth, int frameWidth, int return fallbackWidth; } - private static string BuildGuideText() => - "Matches name/path/agent/package · P pin · G scope · USR user · PRJ project · CUS custom · OK valid · SYM symlink · REV review"; + internal static string BuildSearchScopeText() => "name · path · agent · package"; + + internal static string BuildLegendText() => + "Legend: USR user · PRJ project · CUS custom · OK valid · SYM symlink · REV review"; private void ScheduleDeferredWidthStabilization() { @@ -626,6 +662,21 @@ private static string RenderDetail(InstalledSkill skill) private bool IsCurrentLoad(long loadGeneration) => Interlocked.Read(ref _loadGeneration) == loadGeneration; + internal void CancelPendingLoad() + { + Interlocked.Increment(ref _loadGeneration); + if (_loadCancellations.Cancel()) + { + SetLoading(false); + } + } + + private void SetLoading(bool loading) + { + _spinner.Visible = loading; + _spinner.AutoSpin = loading; + } + internal string GetFilterText() => _filterField.Text.Trim(); internal string? GetPinFilterState() => _pinFilter switch @@ -647,7 +698,9 @@ private bool IsCurrentLoad(long loadGeneration) => internal void FocusFilterForTests() => _filterField.SetFocus(); - internal bool FilterHasFocusForTests => _filterField.HasFocus; + internal bool FilterHasFocus => _filterField.HasFocus; + + internal bool FilterHasFocusForTests => FilterHasFocus; internal bool TableHasFocusForTests => _table.HasFocus; } diff --git a/src/SkillView.Core/Ui/Tabs/UpdatesTabView.cs b/src/SkillView.Core/Ui/Tabs/UpdatesTabView.cs index 8e4f7eb..79693e6 100644 --- a/src/SkillView.Core/Ui/Tabs/UpdatesTabView.cs +++ b/src/SkillView.Core/Ui/Tabs/UpdatesTabView.cs @@ -25,12 +25,13 @@ namespace SkillView.Ui.Tabs; internal sealed class UpdatesTabView : FrameView { private readonly Func _runOnUi; - private readonly Func> _snapshotLoader; - private readonly Func _updateServiceFactory; + private readonly Func> _snapshotLoader; + private readonly Func> _updateRunner; private readonly Func _ghPathProvider; private readonly Logger _logger; private readonly Action _onLeaveTab; private readonly Action _onUpdateApplied; + private readonly CancellationToken _lifetimeToken; private readonly Label _tableLabel; private readonly TableView _table; @@ -48,23 +49,27 @@ internal sealed class UpdatesTabView : FrameView private IReadOnlyList _skills = Array.Empty(); private int _nameW = 12; private long _loadGeneration; + private readonly CancellationTokenSourceSlot _loadCancellations = new(); + private readonly CancellationTokenSourceSlot _operationCancellations = new(); internal UpdatesTabView( Func runOnUi, - Func> snapshotLoader, - Func updateServiceFactory, + Func> snapshotLoader, + Func> updateRunner, Func ghPathProvider, Logger logger, Action onLeaveTab, - Action onUpdateApplied) + Action onUpdateApplied, + CancellationToken lifetimeToken = default) { _runOnUi = runOnUi; _snapshotLoader = snapshotLoader; - _updateServiceFactory = updateServiceFactory; + _updateRunner = updateRunner; _ghPathProvider = ghPathProvider; _logger = logger; _onLeaveTab = onLeaveTab; _onUpdateApplied = onUpdateApplied; + _lifetimeToken = lifetimeToken; BorderStyle = LineStyle.None; SchemeName = SchemeNames.Base; @@ -118,14 +123,14 @@ internal UpdatesTabView( _status = new Label { - X = 0, + X = 2, Y = Pos.AnchorEnd(3), - Width = Dim.Fill(10), + Width = Dim.Fill(2), Text = " loading inventory…", }; _spinner = new SpinnerView { - X = Pos.AnchorEnd(10), + X = 0, Y = Pos.AnchorEnd(3), Width = 1, Height = 1, @@ -179,16 +184,19 @@ internal UpdatesTabView( Add(_tableLabel, _table, _preview, _allBox, _forceBox, _unpinBox, _status, _spinner, _dryRunButton, _updateButton, _statusBar); + Disposing += (_, _) => CancelPendingWork(); } internal async Task LoadAsync() { var loadGeneration = Interlocked.Increment(ref _loadGeneration); + using var loadCancellation = _loadCancellations.Replace(_lifetimeToken); Visible = true; + SetBusy(true); _status.Text = " loading inventory…"; try { - var snapshot = await _snapshotLoader().ConfigureAwait(false); + var snapshot = await _snapshotLoader(loadCancellation.Token).ConfigureAwait(false); await _runOnUi(() => { if (!IsCurrentLoad(loadGeneration)) @@ -199,6 +207,10 @@ await _runOnUi(() => Populate(snapshot); }).ConfigureAwait(false); } + catch (OperationCanceledException) when (loadCancellation.Token.IsCancellationRequested) + { + // A newer load or teardown superseded this one. + } catch (Exception ex) { await _runOnUi(() => @@ -209,6 +221,7 @@ await _runOnUi(() => } _status.Text = $" inventory load failed: {TuiHelpers.ErrorSnippet(ex.Message)}"; + SetBusy(false); }).ConfigureAwait(false); } } @@ -235,6 +248,7 @@ private void Populate(InventorySnapshot snapshot) _status.Text = $" {_skills.Count} installed skill(s) — Space to toggle, U to update marked"; _preview.Text = "## Updates\n\n_Press **Dry-run** to preview, or mark rows and **U** to update all marked._"; _table.SetFocus(); + SetBusy(false); } @@ -245,6 +259,8 @@ private bool IsCurrentLoad(long loadGeneration) => internal Button DryRunButtonForTests => _dryRunButton; internal string StatusTextForTests => _status.Text.ToString(); internal IReadOnlyList LoadedSkillNamesForTests => _skills.Select(s => s.Name).ToArray(); + internal bool BusyForTests => _spinner.Visible || _spinner.AutoSpin; + internal Task RunForTestsAsync(bool dryRun, bool batchOnly) => RunAsync(dryRun, batchOnly); /// Activate this view as a drill-in from the Changes workspace. /// Hides the Changes tab via , then shows and loads this view. @@ -351,7 +367,7 @@ private void UpdateCurrentRow() private async Task RunAsync(bool dryRun, bool batchOnly) { - if (_spinner.Visible) return; + if (_operationCancellations.HasActive || _loadCancellations.HasActive) return; var ghPath = _ghPathProvider(); if (ghPath is null) { @@ -374,8 +390,13 @@ private async Task RunAsync(bool dryRun, bool batchOnly) return; } - _spinner.Visible = true; - _spinner.AutoSpin = true; + using var operationCancellation = _operationCancellations.TryBegin(_lifetimeToken); + if (operationCancellation is null) + { + return; + } + SetBusy(true); + SetOperationControlsEnabled(false); _status.Text = dryRun ? $" dry-running {(allChecked ? "all" : marked.Count + " skill(s)")}…" : $" updating {(allChecked ? "all" : marked.Count + " skill(s)")}…"; @@ -389,12 +410,16 @@ private async Task RunAsync(bool dryRun, bool batchOnly) try { - var result = await _updateServiceFactory() - .UpdateAsync(ghPath, options).ConfigureAwait(false); + var result = await _updateRunner(ghPath, options, operationCancellation.Token).ConfigureAwait(false); await _runOnUi(() => { - _spinner.AutoSpin = false; - _spinner.Visible = false; + if (operationCancellation.Token.IsCancellationRequested) + { + return; + } + + SetBusy(false); + SetOperationControlsEnabled(true); _preview.Text = UpdateScreen.RenderResult(result, dryRun, allChecked, marked); if (dryRun) { @@ -423,15 +448,58 @@ await _runOnUi(() => } }).ConfigureAwait(false); } + catch (OperationCanceledException) when (operationCancellation.Token.IsCancellationRequested) + { + _logger.Debug("update.tab", "update canceled during deactivation or teardown"); + } catch (Exception ex) { _logger.Error("update.tab", ex.Message); await _runOnUi(() => { - _spinner.AutoSpin = false; - _spinner.Visible = false; + if (operationCancellation.Token.IsCancellationRequested) + { + return; + } + + SetBusy(false); + SetOperationControlsEnabled(true); _status.Text = $" update failed: {TuiHelpers.ErrorSnippet(ex.Message)}"; }).ConfigureAwait(false); } } + + internal void CancelPendingLoad() + { + Interlocked.Increment(ref _loadGeneration); + _loadCancellations.Cancel(); + if (!_operationCancellations.HasActive) + { + SetBusy(false); + } + } + + internal void CancelPendingWork() + { + CancelPendingLoad(); + _operationCancellations.Cancel(); + SetBusy(false); + SetOperationControlsEnabled(true); + } + + private void SetBusy(bool busy) + { + _spinner.Visible = busy; + _spinner.AutoSpin = busy; + } + + private void SetOperationControlsEnabled(bool enabled) + { + _dryRunButton.Enabled = enabled; + _updateButton.Enabled = enabled; + _allBox.Enabled = enabled; + _forceBox.Enabled = enabled; + _unpinBox.Enabled = enabled; + _table.Enabled = enabled; + } } diff --git a/src/SkillView.Core/Ui/TerminalSizeGuardView.cs b/src/SkillView.Core/Ui/TerminalSizeGuardView.cs new file mode 100644 index 0000000..f1cc7b9 --- /dev/null +++ b/src/SkillView.Core/Ui/TerminalSizeGuardView.cs @@ -0,0 +1,55 @@ +using SkillView.Ui.Theming; +using Terminal.Gui.Drawing; +using Terminal.Gui.ViewBase; +using Terminal.Gui.Views; + +namespace SkillView.Ui; + +/// Covers the workspace with a clear resize message when the terminal is too +/// small for the two-pane layouts. Keeping this as one lightweight view avoids +/// every tab attempting its own cramped fallback layout. +internal sealed class TerminalSizeGuardView : FrameView +{ + internal const int MinimumWidth = 80; + internal const int MinimumHeight = 24; + + private readonly Label _message; + + internal TerminalSizeGuardView() + { + X = 0; + Y = 0; + Width = Dim.Fill(); + Height = Dim.Fill(); + BorderStyle = LineStyle.None; + SchemeName = SchemeNames.Base; + CanFocus = false; + Visible = false; + + _message = new Label + { + X = Pos.Center(), + Y = Pos.Center(), + TextAlignment = Alignment.Center, + Text = BuildMessage(MinimumWidth, MinimumHeight), + }; + TuiHelpers.ApplyScheme(SchemeNames.Base, this, _message); + Add(_message); + } + + internal void UpdateForSize(int width, int height) + { + Visible = IsTooSmall(width, height); + if (Visible) + { + _message.Text = BuildMessage(width, height); + SetNeedsDraw(); + } + } + + internal static bool IsTooSmall(int width, int height) => + width > 0 && height > 0 && (width < MinimumWidth || height < MinimumHeight); + + internal static string BuildMessage(int width, int height) => + $"Terminal too small ({width}×{height})\nResize to at least {MinimumWidth}×{MinimumHeight}\nCtrl+Q quits"; +} diff --git a/src/SkillView.Core/Ui/TuiHelpers.cs b/src/SkillView.Core/Ui/TuiHelpers.cs index f024325..c07c4b7 100644 --- a/src/SkillView.Core/Ui/TuiHelpers.cs +++ b/src/SkillView.Core/Ui/TuiHelpers.cs @@ -264,6 +264,9 @@ internal static void ConfigureReadOnlyPane(Editor view, string schemeName, bool /// blocks, tables, links, and lists with a built-in vertical scrollbar. internal static void ConfigureMarkdownPane(Markdown view, string schemeName, Func? openTarget = null) { + // Preserve semantic heading levels and styling without showing literal + // Markdown prefixes ("#", "##") in the rendered UI. + view.ShowHeadingPrefix = false; view.SchemeName = schemeName; view.SetScheme(CreateReadOnlyPaneScheme()); openTarget ??= OpenInDefaultHandler; diff --git a/tests/SkillView.IntegrationTests/Ui/SkillViewAppIntegrationTests.cs b/tests/SkillView.IntegrationTests/Ui/SkillViewAppIntegrationTests.cs index 61db282..547853a 100644 --- a/tests/SkillView.IntegrationTests/Ui/SkillViewAppIntegrationTests.cs +++ b/tests/SkillView.IntegrationTests/Ui/SkillViewAppIntegrationTests.cs @@ -2,6 +2,8 @@ using SkillView.Logging; using SkillView.Ui; using Terminal.Gui.App; +using Terminal.Gui.Drivers; +using Terminal.Gui.Input; using Xunit; namespace SkillView.IntegrationTests.Ui; @@ -28,6 +30,46 @@ public async Task RunAsync_WithAnsiDriverAndSingleTick_ReturnsSuccess() Assert.Equal(ExitCodes.Success, exitCode); } + [Theory] + [InlineData("ctrl-q-from-query")] + [InlineData("q-from-discover-table")] + [InlineData("q-from-installed")] + [InlineData("q-from-doctor")] + [InlineData("ctrl-q-from-help")] + public async Task RunAsync_QuitPaths_StopFromEveryTopLevelWorkspace(string scenario) + { + var services = TuiServices.Build(new Logger(LogLevel.Debug)); + var options = new AppOptions( + InvocationMode.Standalone, + DispatchMode.Tui, + Debug: false, + Theme: AppTheme.Default, + ScanRoots: [], + SubcommandName: null, + SubcommandArgs: []); + Key[] keys = scenario switch + { + "ctrl-q-from-query" => [new Key(KeyCode.Q | KeyCode.CtrlMask)], + "q-from-discover-table" => [new Key(KeyCode.Esc), new Key('q')], + "q-from-installed" => [new Key(KeyCode.Esc), new Key('2'), new Key('q')], + "q-from-doctor" => [new Key(KeyCode.Esc), new Key('d'), new Key('q')], + "ctrl-q-from-help" => + [new Key(KeyCode.Esc), new Key('?'), new Key(KeyCode.Q | KeyCode.CtrlMask)], + _ => throw new ArgumentOutOfRangeException(nameof(scenario)), + }; + using var timeout = new CancellationTokenSource(TimeSpan.FromSeconds(5)); + var app = new SkillViewApp( + services, + options, + () => CreateAnsiAppWithInput(keys), + probeOnRun: false); + + var exitCode = await app.RunAsync(timeout.Token); + + Assert.False(timeout.IsCancellationRequested); + Assert.Equal(ExitCodes.Success, exitCode); + } + private static IApplication CreateAnsiApp() { var app = Application.Create(); @@ -35,4 +77,20 @@ private static IApplication CreateAnsiApp() app.StopAfterFirstIteration = true; return app; } + + private static IApplication CreateAnsiAppWithInput(IReadOnlyList keys) + { + var app = Application.Create(); + app.Init("ansi"); + for (var i = 0; i < keys.Count; i++) + { + var key = keys[i]; + app.AddTimeout(TimeSpan.FromMilliseconds(10 * (i + 1)), () => + { + app.Keyboard.RaiseKeyDownEvent(key); + return false; + }); + } + return app; + } } diff --git a/tests/SkillView.Tests/Logging/FileLogSinkTests.cs b/tests/SkillView.Tests/Logging/FileLogSinkTests.cs index daeaed6..e75eec5 100644 --- a/tests/SkillView.Tests/Logging/FileLogSinkTests.cs +++ b/tests/SkillView.Tests/Logging/FileLogSinkTests.cs @@ -99,4 +99,44 @@ public void ClearAll_removes_every_log_file() } finally { Directory.Delete(dir, true); } } + + [Fact] + public async Task Dispose_does_not_deadlock_with_callback_waiting_for_sink_lock() + { + var dir = NewTempDir(); + var appendReached = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var releaseAppend = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + try + { + var logger = new Logger(LogLevel.Info); + var sink = new FileLogSink( + dir, + clock: null, + beforeAppendLockForTests: () => + { + appendReached.TrySetResult(); + releaseAppend.Task.GetAwaiter().GetResult(); + }); + sink.Attach(logger); + + var logTask = Task.Run(() => logger.Info("test", "concurrent append"), TestContext.Current.CancellationToken); + await appendReached.Task.WaitAsync(TestContext.Current.CancellationToken); + + var disposeTask = Task.Run(sink.Dispose, TestContext.Current.CancellationToken); + Assert.True(SpinWait.SpinUntil( + () => sink.IsDisposedForTests, + TimeSpan.FromSeconds(2))); + + releaseAppend.TrySetResult(); + await Task.WhenAll(logTask, disposeTask) + .WaitAsync(TimeSpan.FromSeconds(2), TestContext.Current.CancellationToken); + + Assert.True(sink.IsDisposedForTests); + } + finally + { + releaseAppend.TrySetResult(); + Directory.Delete(dir, true); + } + } } diff --git a/tests/SkillView.Tests/Logging/LoggerTests.cs b/tests/SkillView.Tests/Logging/LoggerTests.cs index 7e2533f..bec2a56 100644 --- a/tests/SkillView.Tests/Logging/LoggerTests.cs +++ b/tests/SkillView.Tests/Logging/LoggerTests.cs @@ -49,4 +49,50 @@ public void SubscriberReceivesEntries() Assert.Single(received); Assert.Equal("hello", received[0].Message); } + + [Fact] + public void DisposedSubscriptionStopsReceivingEntries() + { + var logger = new Logger(); + var received = new List(); + var subscription = logger.Subscribe(received.Add); + + logger.Info("cat", "before"); + subscription.Dispose(); + logger.Info("cat", "after"); + + var entry = Assert.Single(received); + Assert.Equal("before", entry.Message); + } + + [Fact] + public async Task DisposedSubscriptionIsSkippedWhenAConcurrentLogAlreadySnapshottedIt() + { + var logger = new Logger(); + var firstObserverStarted = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var releaseFirstObserver = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var targetCallCount = 0; + using var blockingSubscription = logger.Subscribe(_ => + { + firstObserverStarted.SetResult(); + releaseFirstObserver.Task.GetAwaiter().GetResult(); + }); + var targetSubscription = logger.Subscribe(_ => Interlocked.Increment(ref targetCallCount)); + + var cancellationToken = TestContext.Current.CancellationToken; + var logTask = Task.Run(() => logger.Info("cat", "concurrent"), cancellationToken); + await firstObserverStarted.Task.WaitAsync(cancellationToken); + + try + { + targetSubscription.Dispose(); + } + finally + { + releaseFirstObserver.SetResult(); + } + await logTask; + + Assert.Equal(0, targetCallCount); + } } diff --git a/tests/SkillView.Tests/Subprocess/ProcessRunnerTests.cs b/tests/SkillView.Tests/Subprocess/ProcessRunnerTests.cs index 7098b2a..33db419 100644 --- a/tests/SkillView.Tests/Subprocess/ProcessRunnerTests.cs +++ b/tests/SkillView.Tests/Subprocess/ProcessRunnerTests.cs @@ -20,6 +20,22 @@ public async Task RunAsync_ClosesStandardInput_ForCommandsThatWaitForEof() Assert.Contains("done", result.StdOut); } + [Fact] + public async Task RunAsync_BoundsCapturedOutputAndMarksTruncation() + { + const int limit = 128; + var runner = new ProcessRunner(new Logger(LogLevel.Debug), limit); + var (executable, arguments) = CreateLargeOutputCommand(); + + var result = await runner.RunAsync(executable, arguments, cancellationToken: TestContext.Current.CancellationToken); + + Assert.True(result.Succeeded); + Assert.Contains("output truncated after 128 characters", result.StdOut); + Assert.InRange(result.StdOut.Length, limit, limit + 80); + Assert.Contains("output truncated after 128 characters", result.StdErr); + Assert.InRange(result.StdErr.Length, limit, limit + 80); + } + private static (string Executable, string[] Arguments) CreateWaitForEofCommand() { if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) @@ -38,4 +54,23 @@ private static (string Executable, string[] Arguments) CreateWaitForEofCommand() "cat >/dev/null; printf done" }); } + + private static (string Executable, string[] Arguments) CreateLargeOutputCommand() + { + if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { + return ("pwsh", new[] + { + "-NoProfile", + "-Command", + "[Console]::Out.Write('x' * 4096); [Console]::Error.Write('e' * 4096)" + }); + } + + return ("/bin/sh", new[] + { + "-c", + "i=0; while [ $i -lt 512 ]; do printf 0123456789; printf abcdefghij >&2; i=$((i+1)); done" + }); + } } diff --git a/tests/SkillView.Tests/Ui/CancellationTokenSourceSlotTests.cs b/tests/SkillView.Tests/Ui/CancellationTokenSourceSlotTests.cs new file mode 100644 index 0000000..0710897 --- /dev/null +++ b/tests/SkillView.Tests/Ui/CancellationTokenSourceSlotTests.cs @@ -0,0 +1,65 @@ +using SkillView.Ui; +using Xunit; + +namespace SkillView.Tests.Ui; + +public sealed class CancellationTokenSourceSlotTests +{ + [Fact] + public void Replace_CancelsPreviousLeaseAndKeepsReplacementActive() + { + var slot = new CancellationTokenSourceSlot(); + using var first = slot.Replace(CancellationToken.None); + + using var second = slot.Replace(CancellationToken.None); + + Assert.True(first.Token.IsCancellationRequested); + Assert.False(second.Token.IsCancellationRequested); + Assert.True(slot.HasActive); + } + + [Fact] + public void Cancel_CanRaceLeaseReleaseWithoutThrowing() + { + for (var i = 0; i < 1_000; i++) + { + var slot = new CancellationTokenSourceSlot(); + var lease = slot.Replace(CancellationToken.None); + + Parallel.Invoke(() => slot.Cancel(), lease.Dispose); + + Assert.False(slot.HasActive); + } + } + + [Fact] + public void Replace_CanRaceSupersededLeaseReleaseWithoutThrowing() + { + for (var i = 0; i < 1_000; i++) + { + var slot = new CancellationTokenSourceSlot(); + var first = slot.Replace(CancellationToken.None); + + Parallel.Invoke( + first.Dispose, + () => + { + using var replacement = slot.Replace(CancellationToken.None); + }); + + Assert.False(slot.HasActive); + } + } + + [Fact] + public void TryBegin_AllowsOnlyOneActiveOperation() + { + var slot = new CancellationTokenSourceSlot(); + using var first = slot.TryBegin(CancellationToken.None); + + using var rejected = slot.TryBegin(CancellationToken.None); + + Assert.NotNull(first); + Assert.Null(rejected); + } +} diff --git a/tests/SkillView.Tests/Ui/ChangesTabViewTests.cs b/tests/SkillView.Tests/Ui/ChangesTabViewTests.cs index 9d90ffa..6925f80 100644 --- a/tests/SkillView.Tests/Ui/ChangesTabViewTests.cs +++ b/tests/SkillView.Tests/Ui/ChangesTabViewTests.cs @@ -18,7 +18,7 @@ public void Load_ShowsDetailForTheFirstPendingItemWithoutNeedingEnter() action(); return Task.CompletedTask; }, - snapshotLoader: static () => Task.FromResult(InventorySnapshot.Empty), + snapshotLoader: static _ => Task.FromResult(InventorySnapshot.Empty), onActivateUpdates: static () => { }, onActivateCleanup: static () => { }, onActivateDoctor: static () => { }, @@ -45,7 +45,7 @@ public void Load_AssignsExplicitWidthToTheTitleColumn() action(); return Task.CompletedTask; }, - snapshotLoader: static () => Task.FromResult(InventorySnapshot.Empty), + snapshotLoader: static _ => Task.FromResult(InventorySnapshot.Empty), onActivateUpdates: static () => { }, onActivateCleanup: static () => { }, onActivateDoctor: static () => { }, @@ -75,7 +75,7 @@ public async Task LoadAsync_ShowsCleanupCandidateDetailOnSelectionWithoutOpening action(); return Task.CompletedTask; }, - snapshotLoader: static () => Task.FromResult(SnapshotWithMalformedSkill()), + snapshotLoader: static _ => Task.FromResult(SnapshotWithMalformedSkill()), onActivateUpdates: static () => { }, onActivateCleanup: static () => { }, onActivateDoctor: static () => { }, @@ -100,7 +100,7 @@ public async Task LoadAsync_CleanupOnlyQueue_UsesCleanupCandidateCopyInsteadOfGe action(); return Task.CompletedTask; }, - snapshotLoader: static () => Task.FromResult(SnapshotWithMalformedSkill()), + snapshotLoader: static _ => Task.FromResult(SnapshotWithMalformedSkill()), onActivateUpdates: static () => { }, onActivateCleanup: static () => { }, onActivateDoctor: static () => { }, diff --git a/tests/SkillView.Tests/Ui/ContextBarViewTests.cs b/tests/SkillView.Tests/Ui/ContextBarViewTests.cs index e30d626..9722b05 100644 --- a/tests/SkillView.Tests/Ui/ContextBarViewTests.cs +++ b/tests/SkillView.Tests/Ui/ContextBarViewTests.cs @@ -88,7 +88,7 @@ public void FormatForTests_FilterLabel_AppendedWhenPresent() } [Fact] - public void FormatForTests_WorkspaceOnly_OmitsLowSignalChrome() + public void FormatForTests_WorkspaceOnly_RendersContextTitle() { var state = new ContextBarState( Workspace: "Installed", @@ -100,6 +100,6 @@ public void FormatForTests_WorkspaceOnly_OmitsLowSignalChrome() var text = ContextBarView.FormatForTests(state); - Assert.Equal(string.Empty, text); + Assert.Equal("Installed", text); } } diff --git a/tests/SkillView.Tests/Ui/InstalledScreenTests.cs b/tests/SkillView.Tests/Ui/InstalledScreenTests.cs index 1c2b94c..ba4eeaa 100644 --- a/tests/SkillView.Tests/Ui/InstalledScreenTests.cs +++ b/tests/SkillView.Tests/Ui/InstalledScreenTests.cs @@ -45,6 +45,15 @@ public void DecideShortcut_EscapeFromFilterFocusesTable() Assert.False(decision.RequestStop); } + [Fact] + public void DecideShortcut_QIsLeftForTheGlobalQuitHandler() + { + var decision = InstalledScreen.DecideShortcut(new Key('q'), filterHasFocus: false, canRemove: true); + + Assert.Equal(InstalledScreen.ShortcutCommand.None, decision.Command); + Assert.False(decision.RequestStop); + } + [Fact] public void RenderDetail_FormatsSummaryAndPackageMetadataAsMarkdownLists() { diff --git a/tests/SkillView.Tests/Ui/InstalledTabViewTests.cs b/tests/SkillView.Tests/Ui/InstalledTabViewTests.cs index 1c91161..1b3cfdf 100644 --- a/tests/SkillView.Tests/Ui/InstalledTabViewTests.cs +++ b/tests/SkillView.Tests/Ui/InstalledTabViewTests.cs @@ -97,7 +97,7 @@ public void LoadSeeded_RequestsMultipleDeferredUiPassesToStabilizeInitialColumnW action(); return Task.CompletedTask; }, - snapshotLoader: static () => Task.FromResult(InventorySnapshot.Empty), + snapshotLoader: static _ => Task.FromResult(InventorySnapshot.Empty), onRemove: static (_, _) => { }, onLeaveTab: static () => { }, onGoToSearch: static () => { }); @@ -146,6 +146,8 @@ public void Constructor_ShowsInstalledFilterGuideAndCompactLegend() && text.Contains("PRJ", StringComparison.Ordinal) && text.Contains("OK", StringComparison.Ordinal) && text.Contains("REV", StringComparison.Ordinal)); + Assert.DoesNotContain(labels, text => text.Contains("P pin", StringComparison.Ordinal) + || text.Contains("G scope", StringComparison.Ordinal)); } private static InstalledTabView CreateSubject() => new( @@ -154,7 +156,7 @@ public void Constructor_ShowsInstalledFilterGuideAndCompactLegend() action(); return Task.CompletedTask; }, - snapshotLoader: static () => Task.FromResult(InventorySnapshot.Empty), + snapshotLoader: static _ => Task.FromResult(InventorySnapshot.Empty), onRemove: static (_, _) => { }, onLeaveTab: static () => { }, onGoToSearch: static () => { }); diff --git a/tests/SkillView.Tests/Ui/LatestRequestGateTests.cs b/tests/SkillView.Tests/Ui/LatestRequestGateTests.cs new file mode 100644 index 0000000..b437aac --- /dev/null +++ b/tests/SkillView.Tests/Ui/LatestRequestGateTests.cs @@ -0,0 +1,51 @@ +using SkillView.Ui; +using Xunit; + +namespace SkillView.Tests.Ui; + +public sealed class LatestRequestGateTests +{ + [Fact] + public void Begin_CancelsOverlappingPreviewAndRejectsItsStaleResult() + { + using var gate = new LatestRequestGate(); + using var first = gate.Begin(CancellationToken.None, TimeSpan.FromMinutes(1)); + + using var second = gate.Begin(CancellationToken.None, TimeSpan.FromMinutes(1)); + + Assert.True(first.Token.IsCancellationRequested); + Assert.False(first.IsCurrent); + Assert.False(second.Token.IsCancellationRequested); + Assert.True(second.IsCurrent); + } + + [Fact] + public void Cancel_CancelsCurrentPreviewLease() + { + using var gate = new LatestRequestGate(); + using var request = gate.Begin(CancellationToken.None, TimeSpan.FromMinutes(1)); + + gate.Cancel(); + + Assert.True(request.Token.IsCancellationRequested); + Assert.False(request.IsCurrent); + } + + [Fact] + public void BeginAndCancel_CanRaceSupersededLeaseReleaseWithoutThrowing() + { + for (var i = 0; i < 1_000; i++) + { + using var gate = new LatestRequestGate(); + var first = gate.Begin(CancellationToken.None, TimeSpan.FromMinutes(1)); + + Parallel.Invoke( + first.Dispose, + () => + { + using var replacement = gate.Begin(CancellationToken.None, TimeSpan.FromMinutes(1)); + gate.Cancel(); + }); + } + } +} diff --git a/tests/SkillView.Tests/Ui/SearchAgentMetadataCacheTests.cs b/tests/SkillView.Tests/Ui/SearchAgentMetadataCacheTests.cs index 873f3c7..dcefb62 100644 --- a/tests/SkillView.Tests/Ui/SearchAgentMetadataCacheTests.cs +++ b/tests/SkillView.Tests/Ui/SearchAgentMetadataCacheTests.cs @@ -82,6 +82,24 @@ public void MissingMetadata_DoesNotMatchAgentFilter() only => Assert.Equal("alpha", only.SkillName)); } + [Fact] + public void Store_EvictsLeastRecentlyUsedMetadataAtCapacity() + { + var cache = new SearchAgentMetadataCache(capacity: 2); + var first = Skill("owner/one", "alpha"); + var second = Skill("owner/two", "beta"); + var third = Skill("owner/three", "gamma"); + + cache.Store(first, ImmutableArray.Create("claude-code")); + cache.Store(second, ImmutableArray.Create("github-copilot")); + cache.Store(third, ImmutableArray.Create("gemini-cli")); + + Assert.Equal(2, cache.CountForTests); + Assert.False(cache.Has(first)); + Assert.True(cache.Has(second)); + Assert.True(cache.Has(third)); + } + private static SearchResultSkill Skill(string repo, string skillName) => new( Description: null, diff --git a/tests/SkillView.Tests/Ui/SkillViewAppTests.cs b/tests/SkillView.Tests/Ui/SkillViewAppTests.cs index 3021963..98fc6c7 100644 --- a/tests/SkillView.Tests/Ui/SkillViewAppTests.cs +++ b/tests/SkillView.Tests/Ui/SkillViewAppTests.cs @@ -285,10 +285,11 @@ public void BuildUi_DiscoverFooterAvoidsRepeatingDetailActions() var hints = app.CurrentHintsForTests; - Assert.Equal(3, hints.Count); + Assert.Equal(4, hints.Count); Assert.Contains(hints, hint => hint.Key == "f" && hint.Label == "Filters"); Assert.Contains(hints, hint => hint.Key == "1/2/3" && hint.Label == "Tabs"); Assert.Contains(hints, hint => hint.Key == "?" && hint.Label == "Help"); + Assert.Contains(hints, hint => hint.Key == "Ctrl+Q" && hint.Label == "Quit"); Assert.DoesNotContain(hints, hint => hint.Key == "/" && hint.Label == "Search"); Assert.DoesNotContain(hints, hint => hint.Key == "Enter" && hint.Label == "Preview"); Assert.DoesNotContain(hints, hint => hint.Key == "i" && hint.Label == "Install"); @@ -305,25 +306,28 @@ public void BuildUi_InstalledHints_ShowOnlyPrimaryActions() app.ForceActiveTabForTests(SkillViewTab.Installed); var hints = app.CurrentHintsForTests; - Assert.Equal(4, hints.Count); + Assert.Equal(7, hints.Count); Assert.Contains(hints, hint => hint.Key == "f" && hint.Label == "Filter"); + Assert.Contains(hints, hint => hint.Key == "s" && hint.Label == "Sort"); + Assert.Contains(hints, hint => hint.Key == "P" && hint.Label == "Pins"); + Assert.Contains(hints, hint => hint.Key == "G" && hint.Label == "Scope"); Assert.Contains(hints, hint => hint.Key == "x" && hint.Label == "Remove"); - Assert.Contains(hints, hint => hint.Key == "1/2/3" && hint.Label == "Tabs"); Assert.Contains(hints, hint => hint.Key == "?" && hint.Label == "Help"); + Assert.Contains(hints, hint => hint.Key == "Ctrl+Q" && hint.Label == "Quit"); Assert.DoesNotContain(hints, hint => hint.Key == "/" && hint.Label == "Search"); - Assert.DoesNotContain(hints, hint => hint.Key == "s" && hint.Label == "Sort"); - Assert.DoesNotContain(hints, hint => hint.Key == "P" && hint.Label == "Pins"); Assert.DoesNotContain(hints, hint => hint.Key == "o" && hint.Label == "Open"); Assert.DoesNotContain(hints, hint => hint.Key == "q" && hint.Label == "Quit"); } [Fact] - public void BuildUi_HidesContextBarWhenItHasNoMeaningfulContent() + public void BuildUi_ShowsContextualWorkspaceTitle() { var app = CreateApp(); using var window = app.BuildUiForTests(); - Assert.False(app.ContextBarForTests!.Visible); + Assert.True(app.ContextBarForTests!.Visible); + Assert.Contains("Discover skills", ContextBarView.FormatForTests( + app.ContextBarForTests.CurrentStateForTests)); } [Fact] @@ -530,7 +534,7 @@ public void BuildUi_DiscoverContextBar_DoesNotRepeatFilterSummary() var contextBar = app.ContextBarForTests!.CurrentStateForTests; - Assert.Equal("Discover", contextBar.Workspace); + Assert.Equal("Discover skills", contextBar.Workspace); Assert.Null(contextBar.FilterLabel); } @@ -763,7 +767,7 @@ public void InstalledSelectionChange_KeepsShellChromeFocusedOnFiltersInsteadOfRe } [Fact] - public void ContextBar_FormatOmitsWorkspaceOnlyCopy() + public void ContextBar_FormatIncludesWorkspaceTitle() { var state = new ContextBarState( Workspace: "Discover", @@ -774,7 +778,7 @@ public void ContextBar_FormatOmitsWorkspaceOnlyCopy() FilterLabel: null); var rendered = ContextBarView.FormatForTests(state); - Assert.Equal(string.Empty, rendered); + Assert.Equal("Discover", rendered); } [Fact] @@ -811,4 +815,12 @@ public void ContextBar_FormatIncludesFilterLabelWhenPresent() Assert.Contains("Filters:", rendered); Assert.Contains("hidden dirs on", rendered); } + + [Fact] + public void CtrlQ_IsAnUnconditionalQuitKey() + { + Assert.True(SkillViewApp.IsUnconditionalQuitKey( + new Key(KeyCode.Q | KeyCode.CtrlMask))); + Assert.False(SkillViewApp.IsUnconditionalQuitKey(new Key('q'))); + } } diff --git a/tests/SkillView.Tests/Ui/StatusStripViewTests.cs b/tests/SkillView.Tests/Ui/StatusStripViewTests.cs index 94918c1..16c532d 100644 --- a/tests/SkillView.Tests/Ui/StatusStripViewTests.cs +++ b/tests/SkillView.Tests/Ui/StatusStripViewTests.cs @@ -9,6 +9,18 @@ namespace SkillView.Tests.Ui; public sealed class StatusStripViewTests { + [Fact] + public void SetBusy_ControlsInlineSpinnerState() + { + var view = new StatusStripView(); + + view.SetBusy(true); + Assert.True(view.IsBusyForTests); + + view.SetBusy(false); + Assert.False(view.IsBusyForTests); + } + [Fact] public void TruncateHintsForTests_PreservesRightmostPairs_WhenSpaceIsTight() { diff --git a/tests/SkillView.Tests/Ui/TerminalSizeGuardViewTests.cs b/tests/SkillView.Tests/Ui/TerminalSizeGuardViewTests.cs new file mode 100644 index 0000000..75d8149 --- /dev/null +++ b/tests/SkillView.Tests/Ui/TerminalSizeGuardViewTests.cs @@ -0,0 +1,26 @@ +using SkillView.Ui; +using Xunit; + +namespace SkillView.Tests.Ui; + +public sealed class TerminalSizeGuardViewTests +{ + [Theory] + [InlineData(80, 24)] + [InlineData(100, 30)] + [InlineData(140, 42)] + public void SupportedLayouts_DoNotShowResizeGuard(int width, int height) + { + Assert.False(TerminalSizeGuardView.IsTooSmall(width, height)); + } + + [Theory] + [InlineData(79, 24)] + [InlineData(80, 23)] + [InlineData(60, 18)] + public void CrampedLayouts_ShowResizeGuard(int width, int height) + { + Assert.True(TerminalSizeGuardView.IsTooSmall(width, height)); + Assert.Contains("Ctrl+Q quits", TerminalSizeGuardView.BuildMessage(width, height)); + } +} diff --git a/tests/SkillView.Tests/Ui/TuiHelpersTests.cs b/tests/SkillView.Tests/Ui/TuiHelpersTests.cs index 9915a68..0fbdc72 100644 --- a/tests/SkillView.Tests/Ui/TuiHelpersTests.cs +++ b/tests/SkillView.Tests/Ui/TuiHelpersTests.cs @@ -306,6 +306,16 @@ public void ConfigureMarkdownPane_RoutesLinkClicksThroughProvidedOpener() Assert.Equal(["https://example.test/docs"], openedTargets); } + [Fact] + public void ConfigureMarkdownPane_HidesLiteralHeadingPrefixes() + { + var view = new Markdown(); + + TuiHelpers.ConfigureMarkdownPane(view, SkillViewStyling.DialogSchemeName); + + Assert.False(view.ShowHeadingPrefix); + } + [Fact] public void ConfigureMarkdownPane_MarksNonAnchorLinksHandledWhenProvidedOpenerFails() { diff --git a/tests/SkillView.Tests/Ui/UpdatesTabViewTests.cs b/tests/SkillView.Tests/Ui/UpdatesTabViewTests.cs index 4927c9a..d875643 100644 --- a/tests/SkillView.Tests/Ui/UpdatesTabViewTests.cs +++ b/tests/SkillView.Tests/Ui/UpdatesTabViewTests.cs @@ -1,9 +1,11 @@ using System.Collections.Immutable; using System.Threading.Tasks; using SkillView.Gh; +using SkillView.Gh.Models; using SkillView.Inventory.Models; using SkillView.Logging; using SkillView.Ui.Tabs; +using Terminal.Gui.Views; using Xunit; namespace SkillView.Tests.Ui; @@ -43,6 +45,67 @@ public async Task LoadAsync_IgnoresStaleEarlierSnapshot() Assert.Equal(["newer"], view.LoadedSkillNamesForTests); } + [Fact] + public async Task CancelPendingWork_CancelsActiveUpdateAndRestoresControls() + { + var updateStarted = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + CancellationToken updateToken = default; + var view = CreateUpdatesTab( + () => Task.FromResult(SnapshotWithSkill("installed")), + async (_, _, token) => + { + updateToken = token; + updateStarted.SetResult(); + await Task.Delay(Timeout.InfiniteTimeSpan, token); + throw new InvalidOperationException("The canceled delay should not complete normally."); + }); + await view.LoadAsync(); + view.AllBoxForTests.Value = CheckState.Checked; + + var update = view.RunForTestsAsync(dryRun: true, batchOnly: false); + await updateStarted.Task.WaitAsync(TestContext.Current.CancellationToken); + Assert.True(view.BusyForTests); + Assert.False(view.DryRunButtonForTests.Enabled); + + view.CancelPendingWork(); + await update.WaitAsync(TestContext.Current.CancellationToken); + + Assert.True(updateToken.IsCancellationRequested); + Assert.False(view.BusyForTests); + Assert.True(view.DryRunButtonForTests.Enabled); + } + + [Fact] + public async Task CanceledUpdateCompletion_DoesNotOverwriteReloadedState() + { + var updateStarted = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var updateCompleted = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var snapshotName = "initial"; + var view = CreateUpdatesTab( + () => Task.FromResult(SnapshotWithSkill(snapshotName)), + (_, _, _) => + { + updateStarted.SetResult(); + return updateCompleted.Task; + }); + await view.LoadAsync(); + view.AllBoxForTests.Value = CheckState.Checked; + + var update = view.RunForTestsAsync(dryRun: true, batchOnly: false); + await updateStarted.Task.WaitAsync(TestContext.Current.CancellationToken); + view.CancelPendingWork(); + + snapshotName = "reloaded"; + await view.LoadAsync(); + var reloadedStatus = view.StatusTextForTests; + updateCompleted.SetResult(SuccessfulDryRun()); + await update.WaitAsync(TestContext.Current.CancellationToken); + + Assert.Equal(["reloaded"], view.LoadedSkillNamesForTests); + Assert.Equal(reloadedStatus, view.StatusTextForTests); + Assert.DoesNotContain("dry-run complete", view.StatusTextForTests, StringComparison.Ordinal); + } + [Fact] public async Task InstalledTab_LoadAsync_IgnoresStaleEarlierSnapshot() { @@ -55,7 +118,7 @@ public async Task InstalledTab_LoadAsync_IgnoresStaleEarlierSnapshot() action(); return Task.CompletedTask; }, - snapshotLoader: () => ++loadCount == 1 ? first.Task : second.Task, + snapshotLoader: _ => ++loadCount == 1 ? first.Task : second.Task, onRemove: static (_, _) => { }, onLeaveTab: static () => { }, onGoToSearch: static () => { }); @@ -72,6 +135,42 @@ public async Task InstalledTab_LoadAsync_IgnoresStaleEarlierSnapshot() Assert.Equal(["newer"], view.VisibleSkillNamesForTests); } + [Fact] + public async Task InstalledTab_LoadAsync_CancelsSupersededInventoryScan() + { + var loadCount = 0; + CancellationToken firstToken = default; + var firstStarted = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var view = new InstalledTabView( + runOnUi: action => + { + action(); + return Task.CompletedTask; + }, + snapshotLoader: async token => + { + if (Interlocked.Increment(ref loadCount) == 1) + { + firstToken = token; + firstStarted.SetResult(); + await Task.Delay(Timeout.InfiniteTimeSpan, token); + } + return SnapshotWithSkill("newer"); + }, + onRemove: static (_, _) => { }, + onLeaveTab: static () => { }, + onGoToSearch: static () => { }); + + var firstLoad = view.LoadAsync(); + await firstStarted.Task; + var replacementLoad = view.LoadAsync(); + + await Task.WhenAll(firstLoad, replacementLoad); + + Assert.True(firstToken.IsCancellationRequested); + Assert.Equal(["newer"], view.VisibleSkillNamesForTests); + } + [Fact] public async Task InstalledTab_LoadAsync_NotifiesStateChangeAfterPopulate() { @@ -85,7 +184,7 @@ public async Task InstalledTab_LoadAsync_NotifiesStateChangeAfterPopulate() action(); return Task.CompletedTask; }, - snapshotLoader: () => Task.FromResult(SnapshotWithSkill("loaded")), + snapshotLoader: _ => Task.FromResult(SnapshotWithSkill("loaded")), onRemove: static (_, _) => { }, onLeaveTab: static () => { }, onGoToSearch: static () => { }, @@ -105,7 +204,8 @@ public async Task InstalledTab_LoadAsync_NotifiesStateChangeAfterPopulate() } private static UpdatesTabView CreateUpdatesTab( - Func> snapshotLoader) + Func> snapshotLoader, + Func>? updateRunner = null) { var logger = new Logger(LogLevel.Debug); return new UpdatesTabView( @@ -114,14 +214,26 @@ private static UpdatesTabView CreateUpdatesTab( action(); return Task.CompletedTask; }, - snapshotLoader: snapshotLoader, - updateServiceFactory: static () => throw new NotSupportedException(), + snapshotLoader: _ => snapshotLoader(), + updateRunner: updateRunner ?? ((_, _, _) => throw new NotSupportedException()), ghPathProvider: static () => "/usr/bin/gh", logger: logger, onLeaveTab: static () => { }, onUpdateApplied: static () => { }); } + private static UpdateResult SuccessfulDryRun() => new() + { + DryRun = true, + Succeeded = true, + ExitCode = 0, + StdOut = "would update initial", + StdErr = string.Empty, + ErrorMessage = null, + CommandLine = ["skill", "update", "--dry-run", "--all"], + Entries = ImmutableArray.Empty, + }; + private static InventorySnapshot SnapshotWithSkill(string name) => InventorySnapshot.Empty with { Skills = ImmutableArray.Create(new InstalledSkill