Repository navigation
Move remove preflight off the UI thread - #18
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Concurrent inventory loading can incorrectly clear the active removal preflight spinner and footer state.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Moves removal preflight and safety inspection off the Terminal.Gui thread while preserving modal ownership and cancellation behavior.
Changes:
- Adds cancellable background remove planning and reuses evaluations.
- Introduces start-aware modal dispatch ownership.
- Adds regression tests and lifecycle documentation.
File summaries
| File | Description |
|---|---|
AGENTS.md |
Documents removal preflight and modal ownership rules. |
agent_docs/ui-lifecycle-and-resource-bounds.md |
Records lifecycle and cancellation behavior. |
docs/reviews/adversarial-concurrency-resource-cancellation-audit-2026-08-28.md |
Documents the finding and resolution. |
src/SkillView.Core/Inventory/RemoveTargetResolver.cs |
Adds cancellation-aware target building and evaluation. |
src/SkillView.Core/Ui/InstalledTabView.cs |
Owns and gates asynchronous remove preflight. |
src/SkillView.Core/Ui/RemoveScreen.cs |
Reuses precomputed targets and evaluations. |
src/SkillView.Core/Ui/SkillViewApp.cs |
Adds start-aware owned UI dispatch. |
src/SkillView.Core/Ui/SkillViewWorkflowCoordinator.cs |
Builds removal plans off-thread and dispatches modals. |
tests/SkillView.Tests/Ui/InstalledTabViewTests.cs |
Tests gating, refresh, and cancellation. |
tests/SkillView.Tests/Ui/SkillViewAppTests.cs |
Tests owned-dispatch cancellation races. |
tests/SkillView.Tests/Ui/SkillViewWorkflowCoordinatorTests.cs |
Tests worker-thread planning and pre-cancellation. |
tests/SkillView.Tests/Ui/UpdatesTabViewTests.cs |
Updates callback signatures. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- 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.
🔵 Needs a closer look
Removal safety, nested modal ownership, and cancellation timing remain sufficiently concurrency-sensitive to warrant final human review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Moves Installed-tab remove preflight completely off the Terminal.Gui thread and closes an adjacent modal-dispatch ownership race.
This addresses the final
Needs a closer lookfinding from PR #17 (review 5071638437). The review was correct: compact eligibility still reached native filesystem capture synchronously throughOpenRemoveDialogInternal, so slow or disconnected roots could freeze the TUI before the advanced wizard's background evaluation began.User-facing changes
checking removal safety…spinner while preflight runs.xpresses cannot launch overlapping remove preflights or dialogs.Implementation and hardening
RemoveDialogPlanboundary that builds targets and evaluates compact eligibility on the thread pool.CancellationTokenSourceSlot.RemoveScreen; display caching does not weaken finish-time authority revalidation.AGENTS.md, and the UI lifecycle guide.Regression coverage
Validation
dotnet format --verify-no-changes --no-restoreIL2026,IL3050, andIL3053promoted to errorsRelease notes
Remove dialogs now open responsively even when an installed skill is located on a slow or disconnected filesystem. SkillView performs removal safety checks in the background, prevents duplicate remove requests while checking, cancels stale work on navigation/shutdown, and preserves compact versus advanced confirmation behavior without duplicate native inspection.