Conversation
|
Warning Rate limit exceeded@santoshxshrestha has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 2 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughPressing 'x' triggers a disconnect flow: the event handler calls Changes
Sequence DiagramsequenceDiagram
autonumber
participant User
participant EventHandler as Event Handler
participant Utils as disconnect_connected_network()
participant WifiList as WiFi List\n(Arc<Mutex<Vec<WifiNetwork>>>)
participant Nmcli as nmcli (system)
participant Status as Status
User->>EventHandler: press 'x'
EventHandler->>Utils: disconnect_connected_network(wifi_list.clone())
Utils->>WifiList: lock & iterate networks
alt active network found
Utils->>Nmcli: execute disconnect command
Nmcli-->>Utils: exit code + stdout/stderr
Utils->>Status: build Status (stdout/stderr + exit status)
else no active network
Utils->>Status: build Status("No connection found")
end
Utils-->>EventHandler: return Status
EventHandler->>EventHandler: set wifi_credentials.status, show_status_popup = true
EventHandler->>EventHandler: scan_networks(wifi_list.clone())
EventHandler-->>User: display status popup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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: 4
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/apps/core/event_handlers.rssrc/utils.rssrc/utils/disconnect_connection.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/event_handlers.rs
🧬 Code graph analysis (2)
src/utils/disconnect_connection.rs (1)
src/apps/handlers/status.rs (1)
new(23-28)
src/apps/core/event_handlers.rs (1)
src/utils/disconnect_connection.rs (1)
disconnect_connected_network(7-37)
🪛 GitHub Actions: Rust
src/utils/disconnect_connection.rs
[error] 36-36: unneeded return statement. Replace with Status::new(format!("No connection found"), ExitStatus::default())
[error] 36-36: useless use of format!. Consider using .to_string() instead: Status::new("No connection found".to_string(), ExitStatus::default())
🔇 Additional comments (3)
src/utils.rs (1)
3-3: LGTM!The module declaration follows the existing pattern and integrates cleanly with the other utility modules.
src/apps/core/event_handlers.rs (1)
2-2: LGTM!The import is correctly structured and follows Rust conventions.
src/utils/disconnect_connection.rs (1)
1-5: LGTM!All imports are necessary and correctly structured.
src/utils/disconnect_connection.rs
Outdated
| pub fn disconnect_connected_network(wifi_list: Arc<Mutex<Vec<WifiNetwork>>>) -> Status { | ||
| let list = wifi_list.lock().unwrap(); |
There was a problem hiding this comment.
Handle potential mutex poisoning error.
Using .unwrap() on lock() will panic if the mutex is poisoned (i.e., a thread panicked while holding the lock). This can cause unexpected crashes.
🔎 Suggested fix to handle poisoned mutex
pub fn disconnect_connected_network(wifi_list: Arc<Mutex<Vec<WifiNetwork>>>) -> Status {
- let list = wifi_list.lock().unwrap();
+ let list = match wifi_list.lock() {
+ Ok(guard) => guard,
+ Err(poisoned) => poisoned.into_inner(), // or return an error Status
+ };📝 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.
| pub fn disconnect_connected_network(wifi_list: Arc<Mutex<Vec<WifiNetwork>>>) -> Status { | |
| let list = wifi_list.lock().unwrap(); | |
| pub fn disconnect_connected_network(wifi_list: Arc<Mutex<Vec<WifiNetwork>>>) -> Status { | |
| let list = match wifi_list.lock() { | |
| Ok(guard) => guard, | |
| Err(poisoned) => poisoned.into_inner(), // or return an error Status | |
| }; |
🤖 Prompt for AI Agents
In src/utils/disconnect_connection.rs around lines 7-8, avoid calling .unwrap()
on wifi_list.lock() which will panic on a poisoned mutex; instead handle the
Result from lock(): either recover the inner data with
lock().unwrap_or_else(|poisoned| poisoned.into_inner()) if safe to continue, or
match on Err and return a proper Status::internal with a clear message (and
optional logging) so the function returns an error instead of panicking.
| for network in list.iter() { | ||
| if network.in_use { | ||
| let ssid = &network.ssid; | ||
| let output = Command::new("nmtui") | ||
| .args(["connection", "down", ssid]) | ||
| .output(); | ||
| match output { | ||
| Ok(output) => { | ||
| let status = output.status; | ||
| if status.success() { | ||
| let stdout = String::from_utf8_lossy(&output.stdout); | ||
| return Status::new(stdout.into(), status); | ||
| } else { | ||
| let stderr = String::from_utf8_lossy(&output.stderr); | ||
| return Status::new(stderr.into(), status); | ||
| } | ||
| } | ||
| Err(e) => { | ||
| return Status::new( | ||
| format!("Failed to execute nmcli: {}", e), | ||
| ExitStatus::default(), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Use nmcli instead of nmtui and fix the inconsistent error message.
There are two critical issues:
-
Wrong command tool: Line 13 uses
nmtui(NetworkManager Text User Interface), which is an interactive TUI tool not designed for programmatic use. The correct tool for scripting isnmcli(NetworkManager Command Line Interface). -
Inconsistent error message: Line 29 mentions "nmcli" in the error message, but the actual command being executed is "nmtui". This is confusing.
🔎 Proposed fix to use the correct command and fix error message
for network in list.iter() {
if network.in_use {
let ssid = &network.ssid;
- let output = Command::new("nmtui")
+ let output = Command::new("nmcli")
.args(["connection", "down", ssid])
.output();
match output {
Ok(output) => {
let status = output.status;
if status.success() {
let stdout = String::from_utf8_lossy(&output.stdout);
return Status::new(stdout.into(), status);
} else {
let stderr = String::from_utf8_lossy(&output.stderr);
return Status::new(stderr.into(), status);
}
}
Err(e) => {
return Status::new(
format!("Failed to execute nmcli: {}", e),
ExitStatus::default(),
);
}
}
}
}📝 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.
| for network in list.iter() { | |
| if network.in_use { | |
| let ssid = &network.ssid; | |
| let output = Command::new("nmtui") | |
| .args(["connection", "down", ssid]) | |
| .output(); | |
| match output { | |
| Ok(output) => { | |
| let status = output.status; | |
| if status.success() { | |
| let stdout = String::from_utf8_lossy(&output.stdout); | |
| return Status::new(stdout.into(), status); | |
| } else { | |
| let stderr = String::from_utf8_lossy(&output.stderr); | |
| return Status::new(stderr.into(), status); | |
| } | |
| } | |
| Err(e) => { | |
| return Status::new( | |
| format!("Failed to execute nmcli: {}", e), | |
| ExitStatus::default(), | |
| ); | |
| } | |
| } | |
| for network in list.iter() { | |
| if network.in_use { | |
| let ssid = &network.ssid; | |
| let output = Command::new("nmcli") | |
| .args(["connection", "down", ssid]) | |
| .output(); | |
| match output { | |
| Ok(output) => { | |
| let status = output.status; | |
| if status.success() { | |
| let stdout = String::from_utf8_lossy(&output.stdout); | |
| return Status::new(stdout.into(), status); | |
| } else { | |
| let stderr = String::from_utf8_lossy(&output.stderr); | |
| return Status::new(stderr.into(), status); | |
| } | |
| } | |
| Err(e) => { | |
| return Status::new( | |
| format!("Failed to execute nmcli: {}", e), | |
| ExitStatus::default(), | |
| ); | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/utils/disconnect_connection.rs around lines 10 to 33, the code invokes
the interactive tool "nmtui" instead of the programmatic "nmcli" and the error
message incorrectly mentions "nmcli" while executing "nmtui"; change
Command::new("nmtui") to Command::new("nmcli") so the script uses the proper
CLI, and update the Err branch error message to accurately reflect the executed
command (include "nmcli" and the SSID/connection name in the formatted error) so
logging consistently matches the actual command run.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/apps/core/event_handlers.rssrc/utils/disconnect_connection.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/disconnect_connection.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/event_handlers.rs
🧬 Code graph analysis (1)
src/apps/core/event_handlers.rs (1)
src/utils/disconnect_connection.rs (1)
disconnect_connected_network(7-37)
🔇 Additional comments (2)
src/apps/core/event_handlers.rs (2)
2-2: LGTM: Import is correctly added.The import statement is properly formatted and necessary for the new disconnect functionality.
109-117: Good implementation of status capture and user feedback.The handler correctly captures the
Statusreturn value and displays it via popup, addressing the previous review concern. The implementation follows the same pattern as other key handlers in the file.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.