Add confirm dialog when deleting keys#54
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #54 +/- ##
===========================================
- Coverage 68.92% 68.76% -0.17%
===========================================
Files 10 10
Lines 502 509 +7
Branches 81 83 +2
===========================================
+ Hits 346 350 +4
- Misses 112 113 +1
- Partials 44 46 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds a confirmation dialog when deleting secrets to prevent accidental deletions. The confirmation asks users to explicitly confirm deletion by typing "y" or "yes" before proceeding with the removal of a secret.
- Added confirmation prompt in the RemoveSecret method that requires user input before deletion
- Updated all existing tests to include the confirmation step by simulating "y" input
- Properly escapes the key name in the confirmation message for safe display
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Karls.BetterSecretsTool/Tool.cs | Implements the confirmation dialog with case-insensitive y/yes validation before deleting secrets |
| test/Karls.BetterSecretsTool.Tests/ConsoleUIIntegrationTests.cs | Updates two test cases to simulate confirmation input ("y") for secret deletion flows |
| test/Karls.BetterSecretsTool.Tests/EndToEndIntegrationTests.cs | Updates two end-to-end test cases to include confirmation step when testing secret deletion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| new TextPrompt<string>($"[grey]Are you sure you want to delete [red]{Markup.Escape(key)}[/]? ([green]y[/]/[red]n[/]):[/]") | ||
| .AllowEmpty()); |
There was a problem hiding this comment.
The confirmation logic treats an empty response (just pressing Enter) as a cancellation, which may not be intuitive for users. When users press Enter without typing anything, it's unclear whether they meant to confirm or cancel the deletion. Consider either:
- Requiring an explicit response (remove AllowEmpty() and provide a default value), or
- Treating empty responses as "no" explicitly by checking for empty string in the confirmation logic, or
- Using a Yes/No selection prompt instead of a text prompt for clearer user intent.
| new TextPrompt<string>($"[grey]Are you sure you want to delete [red]{Markup.Escape(key)}[/]? ([green]y[/]/[red]n[/]):[/]") | |
| .AllowEmpty()); | |
| new TextPrompt<string>($"[grey]Are you sure you want to delete [red]{Markup.Escape(key)}[/]? ([green]y[/]/[red]n[/]):[/]")); |
| // Simulate: D (delete secret), select the key (Enter to select first), confirm (y), then Q (quit) | ||
| console.Input.PushText("D"); | ||
| console.Input.PushKey(ConsoleKey.Enter); // Select first (only) item | ||
| console.Input.PushTextWithEnter("y"); // Confirm deletion |
There was a problem hiding this comment.
There is no test coverage for the cancellation scenario when a user declines to delete a secret (e.g., by entering "n" or "no"). Add a test case that verifies the secret is not deleted when the user cancels the deletion confirmation.
| // Simulate: D (delete secret), select the key (Enter to select first), confirm (y), then Q (quit) | ||
| console.Input.PushText("D"); | ||
| console.Input.PushKey(ConsoleKey.Enter); // Select first (only) item | ||
| console.Input.PushTextWithEnter("y"); // Confirm deletion |
There was a problem hiding this comment.
There is no test coverage for the cancellation scenario when a user declines to delete a secret (e.g., by entering "n" or "no"). Add a test case that verifies the secret is not deleted when the user cancels the deletion confirmation.
Fixes #41