Repository navigation
Improve TUI usability, quit behavior, and resource safety - #11
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Output capture remains unbounded for newline-free streams, and several cancellation and log-refresh paths contain concurrency races.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves TUI navigation, progress feedback, cancellation, lifecycle cleanup, and resource limits.
Changes:
- Adds contextual titles, global quit handling, terminal-size protection, and inline spinners.
- Cancels stale asynchronous work and bounds caches, logs, and subprocess output.
- Expands unit and integration coverage and documents lifecycle invariants.
File summaries
| File | Description |
|---|---|
AGENTS.md |
Records new lifecycle and resource rules. |
README.md |
Documents revised quit behavior. |
agent_docs/tui-pty-testing.md |
Updates PTY keyboard guidance. |
agent_docs/ui-lifecycle-and-resource-bounds.md |
Adds hardening guidance. |
src/SkillView.Core/Bootstrapping/EntryPoint.cs |
Disposes CLI logging subscription. |
src/SkillView.Core/Logging/FileLogSink.cs |
Manages logger subscription lifetime. |
src/SkillView.Core/Logging/Logger.cs |
Adds disposable subscriptions. |
src/SkillView.Core/Subprocess/ProcessRunner.cs |
Introduces captured-output limits. |
src/SkillView.Core/Ui/ChangesTabView.cs |
Adds cancellable loading and spinner. |
src/SkillView.Core/Ui/ContextBarView.cs |
Displays workspace titles. |
src/SkillView.Core/Ui/DoctorTabView.cs |
Revises quit-key handling. |
src/SkillView.Core/Ui/HelpOverlay.cs |
Updates keyboard help. |
src/SkillView.Core/Ui/InstallConfirmModal.cs |
Adds operation cancellation and locking. |
src/SkillView.Core/Ui/InstalledScreen.cs |
Defers q to global routing. |
src/SkillView.Core/Ui/InstallScreen.cs |
Repositions inline progress. |
src/SkillView.Core/Ui/LatestRequestGate.cs |
Adds latest-request cancellation. |
src/SkillView.Core/Ui/RepoSkillPickerModal.cs |
Hardens install-dialog lifetime. |
src/SkillView.Core/Ui/SearchAgentMetadataCache.cs |
Implements bounded thread-safe LRU storage. |
src/SkillView.Core/Ui/SkillViewApp.cs |
Integrates quit, guard, cancellation, and log bounds. |
src/SkillView.Core/Ui/StatusStripView.cs |
Embeds the main spinner. |
src/SkillView.Core/Ui/TabBarView.cs |
Removes duplicate branding. |
src/SkillView.Core/Ui/Tabs/ChangesTabView.cs |
Adds cancellable inventory loading. |
src/SkillView.Core/Ui/Tabs/DoctorTabView.cs |
Aligns Doctor navigation semantics. |
src/SkillView.Core/Ui/Tabs/InstalledTabView.cs |
Adds cancellation, progress, and clearer guidance. |
src/SkillView.Core/Ui/Tabs/UpdatesTabView.cs |
Adds cancellation and operation locking. |
src/SkillView.Core/Ui/TerminalSizeGuardView.cs |
Adds minimum-terminal protection. |
src/SkillView.Core/Ui/TuiHelpers.cs |
Hides Markdown heading prefixes. |
tests/SkillView.IntegrationTests/Ui/SkillViewAppIntegrationTests.cs |
Covers top-level quit paths. |
tests/SkillView.Tests/Logging/LoggerTests.cs |
Tests subscription disposal. |
tests/SkillView.Tests/Subprocess/ProcessRunnerTests.cs |
Tests output truncation. |
tests/SkillView.Tests/Ui/ChangesTabViewTests.cs |
Updates cancellation-aware loaders. |
tests/SkillView.Tests/Ui/ContextBarViewTests.cs |
Verifies workspace titles. |
tests/SkillView.Tests/Ui/InstalledScreenTests.cs |
Verifies global q routing. |
tests/SkillView.Tests/Ui/InstalledTabViewTests.cs |
Covers revised guidance and loaders. |
tests/SkillView.Tests/Ui/LatestRequestGateTests.cs |
Tests supersession and cancellation. |
tests/SkillView.Tests/Ui/SearchAgentMetadataCacheTests.cs |
Tests capacity eviction. |
tests/SkillView.Tests/Ui/SkillViewAppTests.cs |
Covers titles, hints, and quit detection. |
tests/SkillView.Tests/Ui/StatusStripViewTests.cs |
Tests spinner state. |
tests/SkillView.Tests/Ui/TerminalSizeGuardViewTests.cs |
Tests size thresholds. |
tests/SkillView.Tests/Ui/TuiHelpersTests.cs |
Tests Markdown configuration. |
tests/SkillView.Tests/Ui/UpdatesTabViewTests.cs |
Tests superseded inventory cancellation. |
Review details
- Files reviewed: 39/39 changed files
- Comments generated: 7
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Shutdown deadlock, focus-routing, and overlapping update lifecycle issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/SkillView.Core/Ui/SkillViewApp.cs:603
- Esc does not actually leave every Discover input as advertised. The query field has its own Esc handler, but the owner/agent fields (and potentially the limit control) reach this branch; it only updates the status and consumes the key, so focus remains in the editor and the next
qis still typed. Move focus to the results table before reporting that the field was left.
SetStatus(TextInputHasFocus()
? "Esc leaves the field · Ctrl+Q quits"
: "Press q or Ctrl+Q to quit");
tests/SkillView.Tests/Ui/SearchAgentMetadataCacheTests.cs:100
- This test verifies capacity/FIFO eviction, but not least-recently-used behavior: no entry is accessed between insertion and eviction, so a FIFO cache would pass. Touch
firstbefore storingthird, then assert thatsecondwas evicted.
- Files reviewed: 41/41 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Log initialization contains a race that can permanently omit concurrently emitted entries.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/SkillView.Core/Ui/SkillViewApp.cs:1941
- A log entry can be lost when logs are opened: an entry emitted after
Logger.Snapshot()but before this lock is enqueued byOnLogEntry, then immediately removed by_visibleLogLines.Clear(). Take the logger snapshot while holding_visibleLogGate; callbacks for entries already added to the logger will then either be included in that snapshot or enqueue after initialization, preserving every wakeup.
lock (_visibleLogGate)
{
_visibleLogLines.Clear();
foreach (var line in lines) _visibleLogLines.Enqueue(line);
- Files reviewed: 42/42 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Summary
This release improves SkillView's visual hierarchy, keyboard behavior, progress feedback, Markdown previews, and resilience during long-running or repeated operations. It also adds explicit resource bounds and lifecycle cleanup so extended TUI sessions remain responsive and memory usage stays predictable.
User-facing changes
Clearer navigation and screen identity
Skill Viewlabel beneath the application header.Better progress and feedback
Cleaner Markdown previews
##markers.Predictable keyboard behavior
Ctrl+Qworks even when focus is inside a text field or nested modal.qas a convenient quit shortcut in top-level read-only views without hijacking text entry.Escas the back/close action and makes the help/status guidance match the actual behavior.Small-terminal protection
Reliability and resource hardening
Validation
dotnet build --no-restore— passed with 0 warnings and 0 errors.dotnet test --no-build— 556 tests passed, 0 failed.osx-arm64— passed withIL2026,IL3050, andIL3053promoted to errors.git diff --check— passed.Release notes
SkillView now has clearer screen titles, cleaner search guidance, inline progress indicators, improved Markdown previews, and reliable quit shortcuts across views and dialogs. Long-running sessions are safer and lighter thanks to bounded subprocess output, log history, and metadata caching, plus stronger cancellation and cleanup for asynchronous UI work. Terminals below 80×24 now receive an explicit size warning instead of attempting to render unusable controls.