feat(cli): add y/n confirmation prompt to bs down command#395
Open
shallowtensr wants to merge 2 commits intoone-covenant:mainfrom
Open
feat(cli): add y/n confirmation prompt to bs down command#395shallowtensr wants to merge 2 commits intoone-covenant:mainfrom
bs down command#395shallowtensr wants to merge 2 commits intoone-covenant:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdded a Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as "basilica-cli\nArgs -> Handler"
participant Prompt as "dialoguer::Confirm\n(spawn_blocking)"
participant API as "GPU Rental API"
User->>CLI: run `down [--all] [--yes]` / `down <id> [--yes]`
alt skip_confirm == false
CLI->>Prompt: spawn_blocking(Confirm)
Prompt->>User: "Stop all X rentals?" / "Stop rental {id}?"
User-->>Prompt: confirm / deny
Prompt->>CLI: bool
alt confirmed
CLI->>API: call stop_rental(s)...
API-->>CLI: success/fail
else denied
CLI->>User: "Cancelled."
end
else skip_confirm == true
CLI->>API: call stop_rental(s) without prompting
API-->>CLI: success/fail
end
CLI->>User: completion status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/basilica-cli/src/cli/handlers/gpu_rental.rs (1)
2069-2089: Consider extracting duplicated confirmation flow into one helper.Both branches repeat the same
spawn_blocking + Confirm + error mappinglogic. A shared helper would reduce drift risk.Refactor sketch
+async fn confirm_down_action(prompt: String) -> Result<bool, CliError> { + tokio::task::spawn_blocking(move || { + let theme = dialoguer::theme::ColorfulTheme::default(); + Confirm::with_theme(&theme) + .with_prompt(prompt) + .default(false) + .interact() + }) + .await + .map_err(|e| CliError::Internal(eyre!("Task join error: {}", e)))? + .map_err(|e| CliError::Internal(e.into())) +} ... - let confirmed = tokio::task::spawn_blocking(move || { ... }).await ... ?; + let confirmed = confirm_down_action(format!( + "Are you sure you want to stop all {} active rental{}?", + all_count, + if all_count == 1 { "" } else { "s" } + )) + .await?; ... - let confirmed = tokio::task::spawn_blocking(move || { ... }).await ... ?; + let confirmed = confirm_down_action(format!("Stop rental {}?", confirm_id)).await?;Also applies to: 2217-2234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-cli/src/cli/handlers/gpu_rental.rs` around lines 2069 - 2089, Extract the repeated confirmation logic (tokio::task::spawn_blocking + dialoguer::Confirm::with_theme + the current error mapping to CliError::Internal) into a single async helper function (e.g., async fn prompt_confirm(prompt: impl Into<String>) -> Result<bool, CliError>) and replace both occurrences (the block using skip_confirm and the similar block at 2217-2234) to call this helper; ensure the helper encapsulates creating ColorfulTheme, building the prompt string, running interact() inside spawn_blocking, and mapping both spawn join errors and interact errors into CliError::Internal so callers only need to check the returned bool.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/basilica-cli/src/cli/handlers/gpu_rental.rs`:
- Around line 2069-2089: Extract the repeated confirmation logic
(tokio::task::spawn_blocking + dialoguer::Confirm::with_theme + the current
error mapping to CliError::Internal) into a single async helper function (e.g.,
async fn prompt_confirm(prompt: impl Into<String>) -> Result<bool, CliError>)
and replace both occurrences (the block using skip_confirm and the similar block
at 2217-2234) to call this helper; ensure the helper encapsulates creating
ColorfulTheme, building the prompt string, running interact() inside
spawn_blocking, and mapping both spawn join errors and interact errors into
CliError::Internal so callers only need to check the returned bool.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96244adb-6aee-414c-aacb-6660fc433d58
📒 Files selected for processing (3)
crates/basilica-cli/src/cli/args.rscrates/basilica-cli/src/cli/commands.rscrates/basilica-cli/src/cli/handlers/gpu_rental.rs
itzlambda
reviewed
Mar 27, 2026
Contributor
itzlambda
left a comment
itzlambda
left a comment
There was a problem hiding this comment.
thanks for the contribution! just one nitpick that i noticed, not a blocker.
itzlambda
approved these changes
Mar 30, 2026
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
2 participants
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
Add y/n confirmation prompt to
bs downcommand to prevent accidental instance termination. Adds--yes/-yflag to skip confirmation for scripting.Related Issues
User (my) request from Discord — accidental double-enter on interactive selector killed a running instance.
Type of Change
Changes Made
List the main changes in this PR:
--yes/-yflag toDowncommand definitionStop rental <id>? [y/N]--all:Are you sure you want to stop all N active rental(s)? [y/N]spawn_blocking+dialoguer::Confirmwithdefault(false), matching existing patterns involumes.rs,ssh_keys.rs, andtokens.rsTesting
How Has This Been Tested?
Describe the tests you ran to verify your changes.
cargo test)Test Configuration
macOS Darwin 25.3.0rustc 1.93.1 (Homebrew)Checklist
cargo fmtto format my codecargo clippyand addressed all warningsAdditional Context
Manual testing proof :
down_all.mov
down_help.mov
down_id_yes.mov
down_with_id.mov
down.mov
Summary by CodeRabbit
--yes/-yto the terminate instance command to skip confirmation prompts.--yesis used; cancelling prints "Cancelled." and aborts the operation.