Repository navigation
fix: stabilize Changes shortcuts and remove flow - #24
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Detail-pane updates can overwrite or indefinitely retain the removal-preflight message in supported interaction and failure paths.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Stabilizes tab shortcuts and asynchronous modal removal workflows.
Changes:
- Refreshes status shortcuts when tabs change.
- Stages background-prepared modals outside timer callbacks.
- Improves removal preflight visibility and avoids redundant evaluations.
File summaries
| File | Description |
|---|---|
AGENTS.md |
Documents removal and modal lifecycle rules. |
agent_docs/ui-lifecycle-and-resource-bounds.md |
Expands UI lifecycle guidance. |
src/SkillView.Core/Ui/InstalledTabView.cs |
Shows removal preflight in the detail pane. |
src/SkillView.Core/Ui/RemoveScreen.cs |
Compacts wizard and reuses seeded evaluation. |
src/SkillView.Core/Ui/SkillViewApp.cs |
Refreshes shell chrome and stages modal dispatch. |
src/SkillView.Core/Ui/SkillViewWorkflowCoordinator.cs |
Uses owned dispatch for prepared modals. |
src/SkillView.Core/Ui/StatusStripView.cs |
Exposes hints for tests. |
tests/SkillView.Tests/Ui/InstalledTabViewTests.cs |
Verifies prominent preflight feedback. |
tests/SkillView.Tests/Ui/RemoveScreenTests.cs |
Tests sizing and seeded evaluation. |
tests/SkillView.Tests/Ui/SkillViewAppTests.cs |
Tests shortcuts and staged dispatch. |
Review details
Suppressed comments (1)
src/SkillView.Core/Ui/Tabs/InstalledTabView.cs:775
- This restoration is after the
_idleStatusMessageearly return. If an inventory load fails while removal preflight is active, the failure path keeps this preflight text visible; when preflight ends,RefreshOperationStatusreturns after updating only the footer, leaving the stale “Checking removal safety” detail indefinitely. Restore the selected-row detail before handling the idle error footer.
_detail.Text = _rows.Count == 0
? "(no matches)"
: RenderDetail(_rows[Math.Clamp(_table.GetSelectedRow(), 0, _rows.Count - 1)]);
- Files reviewed: 10/10 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.
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
Investigation
Installed removal crash
The macOS crash report captured while the remove wizard was open shows a native
SIGSEGV/EXC_BAD_ACCESSon a.NET TP Worker. Because the process ended in a native fault, managed teardown could not restore Terminal.Gui's alternate-screen and mouse-reporting modes, which explains the unusable terminal and raw mouse character codes after the crash.This change removes redundant eager safety evaluation after the seeded Installed-tab preflight opens the wizard. It does not claim that managed code alone can establish the native fault's exact root cause, but it eliminates the unnecessary concurrent work present in that path.
Cleanup removal hang
A managed stack report was captured from the still-running hung process. The UI thread was running
CleanupScreen.Show/ the nested modal loop fromTimedEvents.RunTimersImpl, while the completed removal worker was blocked inBatchProgressAdapter.CompleteBatchtrying to callIApplication.Invoke, which needed the sameTimedEventslock. Escape changed the status to canceling, but cancellation could not publish completion through that deadlocked path.Background-completed modal launches now use a two-stage owned dispatch:
IApplication.Invokestages the work, returns to release the timer lock, and the synchronous modal starts on the nextIApplication.Iteration. Cleanup, Installed removal, and repo discovery use this shared path.The logs confirmed both selected cleanup targets were refused by validation and remained on disk; the hang did not delete either target.
Changes shortcuts
The Changes tab intentionally supports:
Enter— open the selected queue itemc— Cleanupd— Doctor?— HelpCtrl+Q— QuitThe previously displayed
f,s,P,G, andxhints belong to Installed and were stale after switching tabs.Validation
dotnet builddotnet test --no-build— 819 passedTimedEventslock regression reproducing background progress admission while modal work runsdotnet format --verify-no-changes --no-restoreSkillView.AppandSkillView.GhExtensiononosx-arm64