🎨 Palette: Improve CLI UX with graceful exit and cleaner help#139
🎨 Palette: Improve CLI UX with graceful exit and cleaner help#139
Conversation
Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Merging to
|
Summary of ChangesHello @abhimehro, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the user experience and maintainability of the Palette CLI tool. It introduces graceful handling for user-initiated cancellations (Ctrl+C), provides a more informative and user-friendly help message, and cleans up redundant code. Additionally, it establishes a new journal for documenting UX and accessibility insights. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR improves the CLI user experience by adding graceful exit handling for keyboard interrupts and enhancing the help message with an example. It also removes duplicate code for rendering progress bars.
Changes:
- Added top-level KeyboardInterrupt handler to prevent traceback on Ctrl+C
- Enhanced argparse help with example usage in epilog
- Removed duplicate
render_progress_barfunction definition
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| main.py | Added KeyboardInterrupt handler at module level, improved argparse with epilog example, removed duplicate progress bar function |
| .Jules/palette.md | Created new journal file to track UX/accessibility learnings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| main() | ||
| except KeyboardInterrupt: | ||
| sys.stderr.write( | ||
| f"\n{Colors.WARNING}⚠️ Sync cancelled by user.{Colors.ENDC}\n" | ||
| ) | ||
| sys.exit(130) |
There was a problem hiding this comment.
This outer KeyboardInterrupt handler will only catch interrupts that occur outside the try/except block within main() (lines 1282-1355). The main sync loop already has its own KeyboardInterrupt handler at line 1335 that does not re-raise the exception. This means interrupts during the sync will be handled by the inner handler and will continue execution to print the summary table, never reaching this outer handler.
While the outer handler is useful for catching interrupts during initialization (lines 1217-1282) or during the summary printing (lines 1357-1438), it creates potentially confusing behavior where the same type of interrupt is handled differently depending on when it occurs. Consider documenting this behavior or consolidating the handling strategy to be more consistent.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the command-line user experience by adding a graceful exit on Ctrl+C, providing a helpful example in the help message, and cleaning up duplicated code. The changes are well-intentioned and generally look good. However, I've identified an inconsistency in how KeyboardInterrupt is handled. The new top-level handler exits with code 130, but an existing handler within the main function results in an exit code of 1. This could be confusing for users and for scripts that rely on the exit code. My review includes a comment detailing this issue.
| try: | ||
| main() | ||
| except KeyboardInterrupt: | ||
| sys.stderr.write( | ||
| f"\n{Colors.WARNING}⚠️ Sync cancelled by user.{Colors.ENDC}\n" | ||
| ) | ||
| sys.exit(130) |
There was a problem hiding this comment.
This new handler is a great improvement for gracefully exiting on user cancellation.
However, there's an existing try...except KeyboardInterrupt block inside the main() function (around the profile processing loop, lines 1335-1355) which is handled differently. When an interrupt is caught by that inner handler, the script continues execution, prints a summary, and then exits with code 1 (as success_count will not equal total).
This new handler exits with code 130. This creates an inconsistency: the exit code depends on when the user presses Ctrl+C.
For a consistent user experience, the script should exit with 130 whenever it's cancelled by the user. You might consider modifying the logic at the end of main() to check if cancellation occurred and exit with 130 in that case. This would allow the summary table to still be printed before exiting.
…itization, add dry-run plan details Incorporates the best changes from 36 Jules PRs, addressing review feedback: Bolt (Performance) - from PR #173: - Pre-compile PROFILE_ID_PATTERN and RULE_PATTERN at module level - Use compiled patterns in is_valid_profile_id_format, validate_profile_id, and is_valid_rule - Supersedes PRs: #140, #143, #152, #155, #158, #161, #167, #170, #173 Sentinel (Security) - from PR #172 with review feedback: - Enhance sanitize_for_log to redact Basic Auth credentials in URLs - Redact sensitive query parameters (token, key, secret, password, etc.) - Handle fragment separators (#) per Gemini Code Assist review - Use [^&#\s]* pattern per Copilot reviewer suggestion - Update docstring per reviewer suggestion - Supersedes PRs: #142, #145, #148, #151, #154, #157, #160, #169, #172 Palette (UX) - from PR #174 with lint fixes: - Add print_plan_details function for dry-run visibility - Fix duplicate render_progress_bar definition bug - Supersedes PRs: #139, #141, #144, #147, #150, #153, #156, #159, #162, #165, #168, #171, #174 Also: #146, #149, #164 (parallel folder deletion) and #166 (auto-fix .env perms) are independent features not consolidated here. Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
…itization, add dry-run plan details Incorporates the best changes from 36 Jules PRs, addressing review feedback: Bolt (Performance) - from PR #173: - Pre-compile PROFILE_ID_PATTERN and RULE_PATTERN at module level - Use compiled patterns in is_valid_profile_id_format, validate_profile_id, and is_valid_rule - Supersedes PRs: #140, #143, #152, #155, #158, #161, #167, #170, #173 Sentinel (Security) - from PR #172 with review feedback: - Enhance sanitize_for_log to redact Basic Auth credentials in URLs - Redact sensitive query parameters (token, key, secret, password, etc.) - Handle fragment separators (#) per Gemini Code Assist review - Use [^&#\s]* pattern per Copilot reviewer suggestion - Update docstring per reviewer suggestion - Supersedes PRs: #142, #145, #148, #151, #154, #157, #160, #169, #172 Palette (UX) - from PR #174 with lint fixes: - Add print_plan_details function for dry-run visibility - Fix duplicate render_progress_bar definition bug - Supersedes PRs: #139, #141, #144, #147, #150, #153, #156, #159, #162, #165, #168, #171, #174 Also: #146, #149, #164 (parallel folder deletion) and #166 (auto-fix .env perms) are independent features not consolidated here. Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
💡 What: Added graceful exit handling for Ctrl+C and improved help message.
🎯 Why: Users shouldn't see a traceback when they cancel the script. Help message should provide an example.
🛠️ Changes: Wrapped main execution in try/except KeyboardInterrupt, added epilog to argparse, and removed duplicate render_progress_bar code.
♿ Accessibility: Improved error output clarity.
PR created automatically by Jules for task 1507048143857036051 started by @abhimehro