Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughCentralizes terminal cursor control by removing cursor side-effects from widget render paths and adding cursor positioning and blinking management to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Comment |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @santoshxshrestha. * #28 (comment) The following files were modified: * `src/apps/core.rs` * `src/apps/core/widget.rs`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
CHANGELOG.md (1)
9-9: Minor grammar correction.The phrase "placing it to the draw method" should be "placing it in the draw method" for proper grammar.
🔎 Suggested fix
-- Fixed the rerendering issue of the cursor by removing it from the render method and placing it to the draw method. +- Fixed the rerendering issue of the cursor by removing it from the render method and placing it in the draw method.src/apps/core.rs (1)
73-78: Simplify the overly verbose comment.The comment explaining
io::stdout()usage spans 6 lines and covers basic concepts. This level of detail is unnecessary for developers familiar with the codebase.🔎 Suggested simplification
- // The reason for using io::stdout() here is that the crossterm execute! macro needs a writable output target to send - // terminal commands to. io::stdout() provides a handle to the standard output (the terminal), so the commands (Show, MoveTo, EnableBlinking) - // are sent to the terminal for immediate effect. - // - // If you remove io::stdout(), execute! won't know where to send the commands, resulting in a compilation error. - // You must provide a valid output stream (like io::stdout()) for terminal control commands to work. - // EnableBlinking + // Use io::stdout() as the output target for crossterm terminal commands let _ = execute!(io::stdout(), cursor::Show, EnableBlinking);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.mdsrc/apps/core.rssrc/apps/core/widget.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-29T17:44:05.883Z
Learnt from: santoshxshrestha
Repo: santoshxshrestha/nmtui PR: 0
File: :0-0
Timestamp: 2025-12-29T17:44:05.883Z
Learning: In the santoshxshrestha/nmtui project, input field handlers (SSID and password handlers in src/apps/handlers/ssid_handler.rs and src/apps/handlers/password_handler.rs) intentionally do not include Ctrl+C or global help handlers to avoid the performance overhead of sharing app state and additional polling that would make the program laggy.
Applied to files:
src/apps/core/widget.rssrc/apps/core.rs
🔇 Additional comments (3)
src/apps/core/widget.rs (2)
17-30: LGTM! Good refactoring.The renaming from
HELP_TESTtoHELP_TEXTimproves clarity, and the addition of the disconnect command to the help text provides better user guidance.
175-175: LGTM! Consistent with constant rename.The usage has been correctly updated to reference the renamed
HELP_TEXTconstant.src/apps/core.rs (1)
13-19: LGTM! Necessary imports for cursor control.The new imports support the centralized cursor handling logic in the
drawmethod.
| // Set cursor position for SSID or Password popups | ||
| if self.wifi_credentials.flags.show_ssid_popup | ||
| || self.wifi_credentials.flags.show_password_popup | ||
| { | ||
| frame.set_cursor_position(Position::new( | ||
| frame.area().x + frame.area().width / 4 + self.wifi_credentials.cursor_pos + 1, | ||
| frame.area().y + frame.area().height / 4 + 1, | ||
| )); | ||
| // The reason for using io::stdout() here is that the crossterm execute! macro needs a writable output target to send | ||
| // terminal commands to. io::stdout() provides a handle to the standard output (the terminal), so the commands (Show, MoveTo, EnableBlinking) | ||
| // are sent to the terminal for immediate effect. | ||
| // | ||
| // If you remove io::stdout(), execute! won’t know where to send the commands, resulting in a compilation error. | ||
| // You must provide a valid output stream (like io::stdout()) for terminal control commands to work. | ||
| // EnableBlinking | ||
| let _ = execute!(io::stdout(), cursor::Show, EnableBlinking); | ||
| } else { | ||
| let _ = execute!(io::stdout(), cursor::Hide, DisableBlinking); | ||
| } |
There was a problem hiding this comment.
Silently ignoring terminal control errors could cause UX issues.
The let _ = execute!(...) pattern on lines 80 and 82 silently discards any errors from cursor visibility/blinking commands. If these operations fail, the cursor could be left in an inconsistent state, degrading the user experience.
Consider logging errors or handling them gracefully to ensure cursor state problems are detectable.
🔎 Proposed fix to handle errors
- let _ = execute!(io::stdout(), cursor::Show, EnableBlinking);
+ if let Err(e) = execute!(io::stdout(), cursor::Show, EnableBlinking) {
+ eprintln!("Failed to show cursor: {}", e);
+ }
} else {
- let _ = execute!(io::stdout(), cursor::Hide, DisableBlinking);
+ if let Err(e) = execute!(io::stdout(), cursor::Hide, DisableBlinking) {
+ eprintln!("Failed to hide cursor: {}", e);
+ }
}📝 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.
| // Set cursor position for SSID or Password popups | |
| if self.wifi_credentials.flags.show_ssid_popup | |
| || self.wifi_credentials.flags.show_password_popup | |
| { | |
| frame.set_cursor_position(Position::new( | |
| frame.area().x + frame.area().width / 4 + self.wifi_credentials.cursor_pos + 1, | |
| frame.area().y + frame.area().height / 4 + 1, | |
| )); | |
| // The reason for using io::stdout() here is that the crossterm execute! macro needs a writable output target to send | |
| // terminal commands to. io::stdout() provides a handle to the standard output (the terminal), so the commands (Show, MoveTo, EnableBlinking) | |
| // are sent to the terminal for immediate effect. | |
| // | |
| // If you remove io::stdout(), execute! won’t know where to send the commands, resulting in a compilation error. | |
| // You must provide a valid output stream (like io::stdout()) for terminal control commands to work. | |
| // EnableBlinking | |
| let _ = execute!(io::stdout(), cursor::Show, EnableBlinking); | |
| } else { | |
| let _ = execute!(io::stdout(), cursor::Hide, DisableBlinking); | |
| } | |
| // Set cursor position for SSID or Password popups | |
| if self.wifi_credentials.flags.show_ssid_popup | |
| || self.wifi_credentials.flags.show_password_popup | |
| { | |
| frame.set_cursor_position(Position::new( | |
| frame.area().x + frame.area().width / 4 + self.wifi_credentials.cursor_pos + 1, | |
| frame.area().y + frame.area().height / 4 + 1, | |
| )); | |
| // The reason for using io::stdout() here is that the crossterm execute! macro needs a writable output target to send | |
| // terminal commands to. io::stdout() provides a handle to the standard output (the terminal), so the commands (Show, MoveTo, EnableBlinking) | |
| // are sent to the terminal for immediate effect. | |
| // | |
| // If you remove io::stdout(), execute! won't know where to send the commands, resulting in a compilation error. | |
| // You must provide a valid output stream (like io::stdout()) for terminal control commands to work. | |
| // EnableBlinking | |
| if let Err(e) = execute!(io::stdout(), cursor::Show, EnableBlinking) { | |
| eprintln!("Failed to show cursor: {}", e); | |
| } | |
| } else { | |
| if let Err(e) = execute!(io::stdout(), cursor::Hide, DisableBlinking) { | |
| eprintln!("Failed to hide cursor: {}", e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/apps/core.rs around lines 65 to 83, the execute!(io::stdout(), ...) calls
currently use `let _ =` which discards errors; change these to handle the
Result: call execute! and match or use if let Err(e) = ... to log the error
(using the existing logger) and attempt a safe fallback to restore cursor state
(e.g., try the opposite command or ensure cursor is hidden/shown on error).
Ensure both the Show/EnableBlinking and Hide/DisableBlinking paths handle Err by
logging the error with context and performing a best-effort cleanup to keep
terminal cursor state consistent.
| frame.set_cursor_position(Position::new( | ||
| frame.area().x + frame.area().width / 4 + self.wifi_credentials.cursor_pos + 1, | ||
| frame.area().y + frame.area().height / 4 + 1, | ||
| )); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for cursor_pos assignments and modifications in the codebase
rg -nP --type=rust 'cursor_pos\s*[=+\-]' -B3 -A3Repository: santoshxshrestha/nmtui
Length of output: 2774
🏁 Script executed:
#!/bin/bash
# Find the popup rendering code in core.rs
rg -nP --type=rust 'width\s*/\s*2' src/apps/core.rs -B5 -A5Repository: santoshxshrestha/nmtui
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Search for popup rendering code
rg -nP --type=rust 'Block|Paragraph|show_ssid_popup|show_password_popup' src/apps/core.rs -B2 -A2 | head -80Repository: santoshxshrestha/nmtui
Length of output: 1267
🏁 Script executed:
#!/bin/bash
# Search for render functions related to SSID/password
rg -nP --type=rust 'render.*ssid|render.*password|fn.*ssid|fn.*password' src/apps/core.rs -B2 -A8 | head -100Repository: santoshxshrestha/nmtui
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Get the size of core.rs and read relevant sections
wc -l src/apps/core.rs
echo "---"
# Look for widget/block creation around the cursor position code
sed -n '50,150p' src/apps/core.rs | cat -nRepository: santoshxshrestha/nmtui
Length of output: 5497
🏁 Script executed:
#!/bin/bash
# Read the entire core.rs file
cat -n src/apps/core.rsRepository: santoshxshrestha/nmtui
Length of output: 7739
🏁 Script executed:
#!/bin/bash
# Find widget implementation
fd widget src/apps/ -type f
echo "---"
# If widget.rs exists, show its size
if [ -f src/apps/widget.rs ]; then
wc -l src/apps/widget.rs
fi
if [ -d src/apps/widget ]; then
ls -la src/apps/widget/
fiRepository: santoshxshrestha/nmtui
Length of output: 257
🏁 Script executed:
#!/bin/bash
# Find widget implementation
find src/apps -name "widget*" -o -name "Widget*"
echo "---"
# Look for widget in imports or module declarations
rg -n "mod widget" src/apps/Repository: santoshxshrestha/nmtui
Length of output: 124
🏁 Script executed:
#!/bin/bash
# Check size and read widget.rs
wc -l src/apps/core/widget.rs
echo "---"
cat -n src/apps/core/widget.rsRepository: santoshxshrestha/nmtui
Length of output: 13033
Bound cursor position to popup width, not just input string length.
The cursor position calculation (frame.area().x + frame.area().width / 4 + self.wifi_credentials.cursor_pos + 1) has no constraint to keep it within the popup boundaries. While cursor_pos is bounded to the input string's character count in the handlers, it is not bounded to the popup width (area.width / 2). If an input string is very long (longer than the popup can display), the cursor would render outside the visible popup area.
Update the move_cursor_right function in src/apps/handlers/utils.rs to also cap cursor_pos to the popup width (accounting for borders and padding).
🤖 Prompt for AI Agents
In src/apps/core.rs around lines 69-72 the cursor is positioned using
self.wifi_credentials.cursor_pos without constraining it to the popup width;
update src/apps/handlers/utils.rs (move_cursor_right) to clamp cursor_pos to the
visible input area width of the popup: compute popup_width = frame.area().width
/ 2 (as used for the popup), subtract any borders/padding (e.g. 2 for borders
and any left padding) to get visible_input_width, then set cursor_pos =
min(cursor_pos, visible_input_width.saturating_sub(1)) when moving right so the
cursor can never be placed beyond the popup's drawable area; ensure the same
clamping logic is applied wherever cursor_pos is modified.
…c3050b0 📝 Add docstrings to `cursor`
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.