🎨 Palette: [UX improvement] - Fix TUI keyboard trap and prompt errors#138
🎨 Palette: [UX improvement] - Fix TUI keyboard trap and prompt errors#138haseeb-heaven wants to merge 1 commit into
Conversation
|
👋 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. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe PR adds a reference snapshot of ChangesTerminalUI keyboard interaction fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/terminal_ui.py (1)
42-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winESC cancel can still hang on POSIX due blocking read.
At Line 43,
sys.stdin.read(2)blocks when the user presses plainEsc(no trailing bytes), so cancel may not trigger and the selector can appear stuck.💡 Proposed fix
import termios + import select import tty @@ if key == '\x1b': - next_chars = sys.stdin.read(2) - mapping = {'[A': 'up', '[B': 'down', '[D': 'left', '[C': 'right'} - return mapping.get(next_chars, 'escape') + if select.select([sys.stdin], [], [], 0.03)[0]: + next_char = sys.stdin.read(1) + if next_char == '[' and select.select([sys.stdin], [], [], 0.03)[0]: + arrow = sys.stdin.read(1) + mapping = {'A': 'up', 'B': 'down', 'D': 'left', 'C': 'right'} + return mapping.get(arrow, 'escape') + return 'escape'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/terminal_ui.py` around lines 42 - 45, The issue is that when the user presses plain ESC (the '\x1b' character), the code at sys.stdin.read(2) blocks indefinitely waiting for 2 additional characters that may never arrive, causing the selector to hang. Fix this by implementing non-blocking I/O for the stdin read operation when handling the ESC key, or use a timeout mechanism (such as select with a very short timeout) to read available bytes without blocking. This ensures that pressing plain ESC will return 'escape' promptly instead of hanging the application.
🧹 Nitpick comments (1)
libs/terminal_ui.py.orig (1)
13-199: ⚡ Quick winMove the
.origclass snapshot out oflibs/to avoid divergence.This file is a full duplicate of
TerminalUIand will drift fromlibs/terminal_ui.py(it already misses the Ctrl-C / prompt fixes). Keeping executable-style snapshots in source paths increases maintenance risk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/terminal_ui.py.orig` around lines 13 - 199, The `libs/terminal_ui.py.orig` file is a snapshot duplicate of the TerminalUI class that creates maintenance risk by diverging from the actual implementation in `libs/terminal_ui.py`. Move the `.orig` file out of the `libs/` directory to prevent this snapshot from becoming stale and causing confusion. Executable-style snapshots should not be stored in source directories where they can cause divergence and increased maintenance burden.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/terminal_ui.py`:
- Line 84: The `Prompt.ask()` call at line 84 passes unsanitized `title` and
`options` parameters that could contain Rich markup characters (like `[`, `]`,
`{`, `}`) which will trigger MarkupError in non-TTY environments. Wrap both the
`title` parameter and the result of `'|'.join(options)` with
`rich.markup.escape()` before passing them into the f-string for the
`Prompt.ask()` call to safely escape any markup characters in the dynamic
content.
---
Outside diff comments:
In `@libs/terminal_ui.py`:
- Around line 42-45: The issue is that when the user presses plain ESC (the
'\x1b' character), the code at sys.stdin.read(2) blocks indefinitely waiting for
2 additional characters that may never arrive, causing the selector to hang. Fix
this by implementing non-blocking I/O for the stdin read operation when handling
the ESC key, or use a timeout mechanism (such as select with a very short
timeout) to read available bytes without blocking. This ensures that pressing
plain ESC will return 'escape' promptly instead of hanging the application.
---
Nitpick comments:
In `@libs/terminal_ui.py.orig`:
- Around line 13-199: The `libs/terminal_ui.py.orig` file is a snapshot
duplicate of the TerminalUI class that creates maintenance risk by diverging
from the actual implementation in `libs/terminal_ui.py`. Move the `.orig` file
out of the `libs/` directory to prevent this snapshot from becoming stale and
causing confusion. Executable-style snapshots should not be stored in source
directories where they can cause divergence and increased maintenance burden.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01df28af-5f85-47af-a913-6eab4b4dd997
📒 Files selected for processing (3)
.jules/palette.mdlibs/terminal_ui.pylibs/terminal_ui.py.orig
| if not sys.stdin.isatty(): | ||
| default_choice = default if default in options else options[0] | ||
| answer = Prompt.ask(f"{title}", default=default_choice).strip() | ||
| answer = Prompt.ask(f"{title} \\[{'|'.join(options)}]", default=default_choice).strip() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current prompt construction does not escape dynamic options.
rg -n -C2 "Prompt\.ask|join\(options\)|rich\.markup|escape\(" libs/terminal_ui.pyRepository: haseeb-heaven/code-interpreter
Length of output: 363
🏁 Script executed:
cat -n libs/terminal_ui.py | head -100Repository: haseeb-heaven/code-interpreter
Length of output: 4413
🏁 Script executed:
rg -n "def.*title|def.*options" libs/terminal_ui.py -A5Repository: haseeb-heaven/code-interpreter
Length of output: 1656
🏁 Script executed:
rg "from rich|import rich" libs/terminal_ui.pyRepository: haseeb-heaven/code-interpreter
Length of output: 196
🏁 Script executed:
rg "escape" --type pyRepository: haseeb-heaven/code-interpreter
Length of output: 257
🌐 Web query:
Rich library Prompt.ask markup escaping documentation
💡 Result:
In the Rich library, Prompt.ask accepts strings that are interpreted as Console Markup [1][2]. To prevent Rich from interpreting parts of your prompt string as markup tags (e.g., [bold], [red]), you must escape them [3][4]. The primary methods for escaping are: 1. Using the escape function: The recommended approach when dealing with dynamic content or strings that may contain square brackets is to use the escape function from rich.markup [3][4]. This function automatically escapes all markup-sensitive characters in the string [5]. from rich.prompt import Prompt from rich.markup import escape user_input = "[malicious-tag]" # Escapes the brackets so they are rendered as literal text prompt_text = f"Enter value (ignore {escape(user_input)}): " value = Prompt.ask(prompt_text) 2. Using backslashes: You can manually escape a specific tag by placing a backslash before the opening square bracket [3]. from rich.prompt import Prompt # Renders as literal [bold] Prompt.ask("Enter value [bold]: ") If you need to output a literal backslash followed by a tag, use two backslashes [3]. Note that Prompt.ask internally uses Text.from_markup when receiving a string, which is why it parses markup by default [6][7]. If you have full control over the prompt text and do not require any markup features, you can also pass a rich.text.Text object directly to the prompt argument instead of a string, which bypasses markup parsing entirely [1][2].
Citations:
- 1: https://rich.readthedocs.io/en/stable/prompt.html
- 2: https://rich.readthedocs.io/en/stable/reference/prompt.html
- 3: https://rich.readthedocs.io/en/latest/markup.html
- 4: https://stackoverflow.com/questions/69955974/python-make-rich-not-style-a-string
- 5: https://rich.readthedocs.io/en/latest/%5Fmodules/rich/markup.html
- 6: https://github.com/Textualize/rich/blob/fc41075a/rich/prompt.py
- 7: https://github.com/Textualize/rich/blob/master/rich/prompt.py
Escape dynamic content in non-TTY prompts to prevent Rich markup parsing errors.
The title and options parameters are passed unsanitized to Prompt.ask() at line 84. Rich's Prompt.ask() parses Console Markup by default, so if either parameter contains characters like [, ], {, or }, it will trigger a MarkupError in non-TTY environments.
While the literal opening bracket is escaped with \\[, this does not protect against markup characters in the dynamic content. Use rich.markup.escape() to safely handle user-controlled text:
Recommended fix
from rich.console import Console
+from rich.markup import escape
from rich.panel import Panel
from rich.prompt import Prompt
@@
- answer = Prompt.ask(f"{title} \\[{'|'.join(options)}]", default=default_choice).strip()
+ safe_title = escape(title)
+ safe_options = "|".join(escape(option) for option in options)
+ answer = Prompt.ask(f"{safe_title} \\[{safe_options}]", default=default_choice).strip()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| answer = Prompt.ask(f"{title} \\[{'|'.join(options)}]", default=default_choice).strip() | |
| from rich.console import Console | |
| from rich.markup import escape | |
| from rich.panel import Panel | |
| from rich.prompt import Prompt | |
| ... | |
| safe_title = escape(title) | |
| safe_options = "|".join(escape(option) for option in options) | |
| answer = Prompt.ask(f"{safe_title} \\[{safe_options}]", default=default_choice).strip() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/terminal_ui.py` at line 84, The `Prompt.ask()` call at line 84 passes
unsanitized `title` and `options` parameters that could contain Rich markup
characters (like `[`, `]`, `{`, `}`) which will trigger MarkupError in non-TTY
environments. Wrap both the `title` parameter and the result of
`'|'.join(options)` with `rich.markup.escape()` before passing them into the
f-string for the `Prompt.ask()` call to safely escape any markup characters in
the dynamic content.
1 participant
🎨 Palette: [UX improvement] - Fix TUI keyboard trap and prompt errors
💡 What
I fixed a frustrating keyboard trap where users pressing
Ctrl-C(\x03) in the raw terminal UI were unable to exit, feeling stuck. I also improved the UI feedback by adding a hint to the footer (Esc/Ctrl-C to cancel) and fixed an issue whererichprompts would throw errors when displaying bracketed fallback choices.🎯 Why
When users trigger standard keyboard interrupts like
Ctrl-C, they expect the application to respond immediately. Swallowing these events in a raw terminal read loop creates a terrible UX. Additionally, clear visual hints empower users to navigate the TUI confidently.📸 Before/After
Before:
Ctrl-Cdid nothing in the TUI selector.Use Up/Down arrows and Enter to select.[code|script]torich.Promptwithout escaping raised aMarkupError.After:
Ctrl-Ccorrectly raises aKeyboardInterrupt.Use Up/Down arrows and Enter to select. (Esc/Ctrl-C to cancel)\\[code|script]inrich.Prompt.♿ Accessibility
Improves keyboard accessibility by ensuring standard keyboard exit patterns (
Ctrl-C) work as expected in the raw terminal interface, preventing users who rely exclusively on keyboard navigation from getting trapped.PR created automatically by Jules for task 15696170430766334392 started by @haseeb-heaven
Summary by CodeRabbit
New Features
Bug Fixes