Skip to content

Ask for confirmation before clearing chat history #1180

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 14, 2025

Conversation

GoodluckH
Copy link
Collaborator

Sometimes users may habitually type /clear to clean the console output but in q chat, this actually erase chat history.

Adding a confirmation dialog to prevent mistakes.

Screen.Recording.2025-04-10.at.4.10.38.PM.mov

@GoodluckH GoodluckH requested a review from a team as a code owner April 10, 2025 23:12
@GoodluckH GoodluckH requested a review from brandonskiser April 10, 2025 23:12
@GoodluckH GoodluckH self-assigned this Apr 10, 2025
@GoodluckH GoodluckH linked an issue Apr 10, 2025 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Apr 10, 2025

Codecov Report

Attention: Patch coverage is 70.45455% with 13 lines in your changes missing coverage. Please review.

Project coverage is 13.98%. Comparing base (43f666f) to head (63f853d).

Files with missing lines Patch % Lines
crates/q_cli/src/cli/chat/mod.rs 70.45% 13 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1180      +/-   ##
==========================================
+ Coverage   13.46%   13.98%   +0.51%     
==========================================
  Files        2346     2364      +18     
  Lines      201210   204636    +3426     
  Branches   181574   185000    +3426     
==========================================
+ Hits        27091    28611    +1520     
- Misses     172822   174620    +1798     
- Partials     1297     1405     +108     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

execute!(
self.output,
style::Print(format!(
"\n(To exit, press Ctrl+C or Ctrl+D again or type {})\n\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely shouldn't duplicate this - if anything, add an extra parameter to the ChatState::HandleInput enum stating we are in the middle of clearing state.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored

@GoodluckH GoodluckH force-pushed the xipu/clear-confirmation-dialog branch from 28ac80f to 37cf67a Compare April 11, 2025 19:09
execute!(
self.output,
style::SetForegroundColor(Color::Red),
style::Print(format!("\nError using tools: {}\n\n", e)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error message doesn't make much sense imo, this is just prompting the user regardless of whether or not tools are being presented.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@@ -2141,6 +2162,41 @@ where
Ok(())
}

/// Helper function to read user input with a prompt and Ctrl+C handling
fn read_user_input(&mut self, prompt: &str) -> Result<String, ChatError> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously this logic would just bubble up errors or return an exit state, so now we have to handle errors when instead it would be nicer to just have:

let input = match self.read_user_input(...)? {
Some(input) => input,
None => return ChatState::Exit
}

Prevents us from now handling errors verbosely in multiple locations.

Essentially change this to return a Result<Option<String>, ChatError> instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right! This is much cleaner now

@GoodluckH GoodluckH force-pushed the xipu/clear-confirmation-dialog branch 2 times, most recently from 14c84ec to a9283a7 Compare April 11, 2025 20:06
@GoodluckH GoodluckH requested a review from brandonskiser April 11, 2025 20:06
@GoodluckH GoodluckH force-pushed the xipu/clear-confirmation-dialog branch 2 times, most recently from 123414a to 02939ea Compare April 12, 2025 17:42
@GoodluckH GoodluckH force-pushed the xipu/clear-confirmation-dialog branch from 02939ea to 63f853d Compare April 12, 2025 19:09
@GoodluckH GoodluckH merged commit 06c2e21 into main Apr 14, 2025
20 checks passed
@GoodluckH GoodluckH deleted the xipu/clear-confirmation-dialog branch April 14, 2025 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[UX] Confirmation dialog for the/clear command
3 participants