ggfix: no need to have the escape there in the time and some listhers change#20
ggfix: no need to have the escape there in the time and some listhers change#20
Conversation
📝 WalkthroughWalkthroughAdds and standardizes keyboard shortcuts across UI popups (Ctrl+C to exit, 'q' to close/cancel, 'd' to delete, 'h'/'?' for help), extends delete logic to handle saved vs scanned networks, tweaks saved-timestamp parsing, updates help text, and reorders popup input priority to ensure delete confirmation is handled first. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai are there any missing listhers |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8183 🏁 Script executed: Length of output: 23307 @santoshxshrestha Yes, there are missing listeners in some of the popup handlers! Based on the PR changes, you've added Missing Listeners:1.
|
![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
🧹 Nitpick comments (2)
src/apps/core/widget.rs (1)
22-34: Consider consolidating duplicate help entries and fixing the constant name typo.Two observations:
- Lines 31-32 both show help shortcuts but as separate entries. Consider merging them:
"(h|?) help".- The constant is named
HELP_TEST— likely a typo forHELP_TEXT.🔎 Proposed fix
-const HELP_TEST: [&str; 11] = [ +const HELP_TEXT: [&str; 10] = [ "[Esc] quit", "(Ctrl+c) force quit", "(Ctrl+R) scan for networks", "(Enter) connect to network", "(o) connect to network", "(d) delete saved network", "(↑|k) move up", "(↓|j) move down", - "(h) help", - "(?) help", + "(h|?) help", "(s) view saved networks", ];Note: Update the reference on line 163 from
HELP_TESTtoHELP_TEXTas well.src/apps/core/saved_connection.rs (1)
123-136: Consider consolidating duplicate match arms for 'h' and '?'.Both key handlers perform the same action. You can combine them using an OR pattern.
🔎 Proposed fix
- Event::Key(KeyEvent { - code: event::KeyCode::Char('h'), - kind: event::KeyEventKind::Press, - .. - }) => { - self.show_help = true; - } - Event::Key(KeyEvent { - code: event::KeyCode::Char('?'), - kind: event::KeyEventKind::Press, - .. - }) => { - self.show_help = true; - } + Event::Key(KeyEvent { + code: event::KeyCode::Char('h') | event::KeyCode::Char('?'), + kind: event::KeyEventKind::Press, + .. + }) => { + self.show_help = true; + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/apps/core/delete_handler.rssrc/apps/core/help_handlers.rssrc/apps/core/run.rssrc/apps/core/saved_connection.rssrc/apps/core/widget.rs
🔇 Additional comments (5)
src/apps/core/help_handlers.rs (1)
25-39: LGTM!The added shortcuts for 'q' to close help and Ctrl+C to force quit are consistent with the behavior in other popup handlers across the PR.
src/apps/core/run.rs (1)
11-23: LGTM! The popup priority reordering is correct.The delete confirmation popup now takes precedence over the saved connections popup, which matches the intended layering behavior.
Minor: Typos in comments — "preceeding" → "preceding" (line 12), "becuase" → "because" (line 19).
src/apps/core/saved_connection.rs (2)
45-50: LGTM!Filtering out backslash characters from the timestamp is a reasonable fix for escaped colons in the nmcli output.
64-93: LGTM!The new shortcuts ('q' to close, Ctrl+C to exit, 'd' to delete) are consistent with the other popup handlers in this PR.
src/apps/core/delete_handler.rs (1)
49-70: LGTM!The Ctrl+C and 'q' shortcuts for exiting and canceling the delete confirmation are consistent with the other popup handlers.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7914 🏁 Script executed: Length of output: 13941 🏁 Script executed: Length of output: 3742 🏁 Script executed: Length of output: 5715 @santoshxshrestha Good catch! After analyzing all the event handlers in the codebase, I found that the SSID and password input handlers are missing the Here's what I found: Current Handler CoverageHandlers WITH
Handlers MISSING
IssueCurrently, when users are in the SSID or password input popups, they cannot use Suggested FixAdd For Event::Key(KeyEvent {
code: KeyCode::Char('c'),
modifiers: KeyModifiers::CONTROL,
kind: Press,
..
}) => {
// Force exit the application
// You'll need to pass the app reference or have access to exit()
}For Event::Key(KeyEvent {
code: KeyCode::Char('c'),
modifiers: KeyModifiers::CONTROL,
kind: Press,
..
}) => {
// Force exit the application
// You'll need to pass the app reference or have access to exit()
}Note: The input handlers are implemented on
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/apps/handlers/status.rs (1)
44-52: LGTM! 'q' handler added for consistency.The addition of the 'q' key handler achieves the PR's goal of standardizing close behavior across popups. The logic correctly mirrors the Esc and Enter handlers.
Optional: reduce duplication by extracting common closure
Since Esc, 'q', and Enter all execute identical logic, you could optionally extract a closure to reduce duplication:
impl WifiInputState { pub fn handle_status_message(&mut self) -> io::Result<()> { if poll(Duration::from_micros(1))? { + let close_status = || { + self.flags.show_status_popup = false; + self.status.status_message.clear(); + self.status.status_code = ExitStatus::default(); + }; + match event::read()? { Event::Key(KeyEvent { code: KeyCode::Esc, kind: Press, .. - }) => { - self.flags.show_status_popup = false; - self.status.status_message.clear(); - self.status.status_code = ExitStatus::default(); - } + }) => close_status(), Event::Key(KeyEvent { code: KeyCode::Char('q'), kind: Press, .. - }) => { - self.flags.show_status_popup = false; - self.status.status_message.clear(); - self.status.status_code = ExitStatus::default(); - } + }) => close_status(), Event::Key(KeyEvent { code: KeyCode::Enter, kind: Press, .. - }) => { - self.flags.show_status_popup = false; - self.status.status_message.clear(); - self.status.status_code = ExitStatus::default(); - } + }) => close_status(), _ => {} } }; Ok(()) } }Note: The absence of a
Ctrl+Chandler is understood given the architectural constraint thatWifiInputStatedoes not have direct access toApp::exit(), as noted in the PR objectives.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/apps/handlers/status.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/apps/handlers/status.rs (1)
src/apps/core.rs (1)
default(29-42)
|
@coderabbitai the share of the app will be heavy again and it will make the condition of the program more worst rn so will not add the ctrl+ c and other one there for the input fields also there will be no help global as it will make the call to polling so will consume more resources or it will be laggy as more polling |
|
Tip For best results, initiate chat on the files or code changes. @santoshxshrestha Thank you for the clarification! I completely understand your concern about the architectural overhead and performance implications. You're right that passing the app reference to the input field handlers would add complexity and potentially impact performance, especially with the polling considerations for a global help feature. It makes sense to keep the input handlers lightweight and focused on their core functionality. The current implementation where input fields only use ✏️ Learnings added
|
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.