Conversation
contents mutably
regarding the appearance of the delete conformation for the unsaved connection from the main list
📝 WalkthroughWalkthroughThis PR moves WiFi list synchronization from Mutex to RwLock, adds an Arc scanning flag into Flags/App, updates scan_networks signature to accept the flag, changes scan update/read locking, and renders a spinner while scanning; several call sites and error messages were adjusted. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as TUI / Widget
participant App as App (flags, wifi_list)
participant Scan as scan_networks (worker)
participant Storage as wifi_list (Arc<RwLock>)
UI->>App: User triggers scan (Ctrl+R) or App init
App->>App: flags.is_scanning.store(true)
App->>Scan: scan_networks(wifi_list.clone(), is_scanning.clone())
Note right of Scan `#a3be8c`: Scan sets is_scanning true and runs
Scan->>Storage: acquire write() and replace networks
Note right of Scan `#f6c177`: adds "Connect to Hidden network"
Scan->>App: flags.is_scanning.store(false) (scan complete)
UI->>App: render loop reads flags.is_scanning
alt is_scanning == true
UI->>UI: display spinner row ("<spinner> Scanning...")
else
UI->>Storage: read() wifi_list
UI->>UI: render network rows
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/utils/scan.rs (2)
16-18:is_scanningnot reset on early return, leaving UI stuck in scanning state.If
nmclifails (non-success status), the function returns early without settingis_scanningtofalse. This will leave the UI permanently showing a scanning indicator.🔎 Proposed fix
if !output.status.success() { + is_scanning.store(false, Ordering::SeqCst); return; }
8-14: Setis_scanningtotruebefore each call toscan_networks.The function sets
is_scanningtofalsewhen the scan completes but does not set it totrueat the start. Only the initial call inApp::default()sets the flag totrue; all subsequent calls at lines 133, 155, 184,delete_handler.rs:129, andevent_handlers.rs:68pass the flag without setting it first. This means the scanning indicator will not appear on refreshes after the initial scan.src/apps/core.rs (1)
44-48: Update stale doc comment.The example on line 47 still references
.lock().unwrap()which is the old Mutex API. It should be updated to reflect the new RwLock usage (.read().unwrap()or.write().unwrap()).🔎 Proposed fix
/// # Examples /// /// ``` /// let app = App::default(); /// assert_eq!(app.selected, 0); - /// assert!(app.wifi_list.lock().unwrap().is_empty()); + /// assert!(app.wifi_list.read().unwrap().is_empty()); /// ```
🧹 Nitpick comments (5)
src/utils/tui.rs (1)
5-5: Consider propagating the error instead of panicking.Since
tui()returns aResult, you could propagate the restoration error using?instead of panicking withexpect(). This would allow the caller to handle the failure gracefully.🔎 Suggested improvement
- ratatui::try_restore().expect("Failed to restore terminal"); + ratatui::try_restore()?;src/utils/disconnect_connection.rs (1)
8-35: Read lock held during external command execution.The
RwLockread guard is held while executing thenmclicommand, which could block writers (likescan_networks) for the duration of the command. Consider cloning the SSID and releasing the lock before executing the command.🔎 Suggested improvement
pub fn disconnect_connected_network(wifi_list: Arc<RwLock<Vec<WifiNetwork>>>) -> Status { - let list = wifi_list.read().expect("WifiNetworks lock poisoned"); - - for network in list.iter() { - if network.in_use { - let ssid = &network.ssid; + let ssid_to_disconnect = { + let list = wifi_list.read().expect("WifiNetworks lock poisoned"); + list.iter() + .find(|network| network.in_use) + .map(|network| network.ssid.clone()) + }; + + if let Some(ssid) = ssid_to_disconnect { - let output = Command::new("nmcli") - .args(["connection", "down", ssid]) + let output = Command::new("nmcli") + .args(["connection", "down", &ssid]) .output(); - match output { + match output { // ... rest of match arms unchanged - } } - } + } else { Status::new( "No connected network found".to_string(), ExitStatus::default(), ) + } }src/main.rs (1)
21-21: Consider propagating the error for better diagnostics.Since
main()returns aResultand usescolor_eyre, propagating the error with?would provide a richer backtrace and error context instead of a raw panic.🔎 Suggested improvement
- tui().expect("Failed to run TUI application"); + tui()?;src/apps/core/widget.rs (1)
69-89: Spinner animation implementation looks good, with a minor typo.The time-based spinner approach is a clean way to animate without additional state. However, there's a typo on line 74: "teh" should be "the".
Also, consider using
Ordering::Acquireinstead ofSeqCstfor the atomic load—it's sufficient for reading a flag and has lower overhead.🔎 Proposed fix
- if self.flags.is_scanning.load(Ordering::SeqCst) { + if self.flags.is_scanning.load(Ordering::Acquire) { let spinner = ["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"]; // Use some time-based value for the index // this will select a different spinner character every 100ms - // and push it to teh rows vector + // and push it to the rows vectorsrc/apps/core.rs (1)
120-167: Consider dropping the write lock before spawning the scan thread.
prepare_to_connectholds the write lock onwifi_listwhen callingscan_networks. Whilescan_networksspawns a thread that will eventually wait for the write lock (no deadlock), the lock is held longer than necessary. Consider restructuring to release the lock before triggering the scan.🔎 Sketch of the refactor
Extract the decision logic to determine what action to take, drop the lock, then execute the action (including
scan_networks):fn prepare_to_connect(&mut self) { enum Action { None, ConnectUnsecured(String), ShowHiddenSsidPopup, ConnectSaved(String), ShowPasswordPopup(String), } let action = { let wifi_list = self.wifi_list.read().expect("..."); // determine action based on wifi_list[self.selected] // ... }; // lock released here match action { // handle each case, call scan_networks outside the lock } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.mdsrc/apps/core.rssrc/apps/core/delete_handler.rssrc/apps/core/event_handlers.rssrc/apps/core/widget.rssrc/apps/handlers/flags.rssrc/main.rssrc/utils/disconnect_connection.rssrc/utils/scan.rssrc/utils/tui.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/delete_handler.rssrc/apps/core.rssrc/apps/core/event_handlers.rs
🧬 Code graph analysis (5)
src/main.rs (1)
src/utils/tui.rs (1)
tui(2-7)
src/apps/core/widget.rs (1)
src/apps/core.rs (1)
default(49-68)
src/apps/core/delete_handler.rs (1)
src/utils/scan.rs (1)
scan_networks(8-63)
src/apps/core.rs (1)
src/utils/scan.rs (1)
scan_networks(8-63)
src/apps/core/event_handlers.rs (1)
src/utils/scan.rs (1)
scan_networks(8-63)
🪛 LanguageTool
CHANGELOG.md
[style] ~11-~11: Consider using a different verb for a more formal wording.
Context: ... app is scanning the networks or not. - Fixed the issue of pop-up for pressing 'd' ke...
(FIX_RESOLVE)
[style] ~16-~16: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ed type rather than Arc wrapped type. - Changed the render method to render the scannin...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~34-~34: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...r the deletion of a saved connection. - Fixed the out of bound issue when the list ge...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (5)
src/apps/handlers/flags.rs (1)
1-14: LGTM!The addition of
is_scanning: Arc<AtomicBool>is well-structured. TheDefaultderive correctly initializes it toArc::new(AtomicBool::new(false)), which is the appropriate initial state.src/apps/core/widget.rs (2)
11-12: LGTM on new imports.The imports for
OrderingandSystemTimeare appropriate for the new scanning state management and spinner animation.
90-119: LGTM on the wifi_list read path.The switch to
read()lock for populating the table rows is correct and aligns with the RwLock concurrency model. The logic for highlighting the selected row and marking in-use networks is preserved correctly.src/apps/core.rs (2)
20-24: LGTM on the type change to RwLock.The migration from
MutextoRwLockforwifi_listis correctly implemented, enabling concurrent reads while maintaining exclusive write access.Also applies to: 29-29
49-67: LGTM on initialization logic.The initialization correctly sets up the
is_scanningflag totruebefore triggering the initial scan, and properly wires it into theFlagsstruct.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/scan.rs (1)
12-15: Replaceexpectwith graceful error handling.Using
expecthere will panic the thread ifnmcliis not installed, not in PATH, or encounters permission issues. This compounds the stuck flag issue by making the panic more likely.🔎 Suggested change
let output = Command::new("nmcli") .args(["-t", "-f", "IN-USE,SSID,SECURITY", "device", "wifi", "list"]) - .output() - .expect("Failed to execute nmcli command"); + .output(); + + let output = match output { + Ok(o) => o, + Err(_) => return, // nmcli not available or execution failed + };Note: This still requires fixing the flag cleanup issue identified earlier to ensure
is_scanningis reset even when returning here.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/scan.rs
🔇 Additional comments (1)
src/utils/scan.rs (1)
4-5: LGTM: Appropriate imports for the new synchronization primitives.The imports correctly include
AtomicBool,Ordering, andRwLockto support the migration fromMutextoRwLockand the addition of the scanning flag.
| pub fn scan_networks(wifi_list: Arc<RwLock<Vec<WifiNetwork>>>, is_scanning: Arc<AtomicBool>) { | ||
| is_scanning.store(true, Ordering::SeqCst); |
There was a problem hiding this comment.
Critical: Scanning flag can be stuck at true indefinitely.
The is_scanning flag is set to true here but only reset to false at Line 62 in the happy path. If the spawned thread panics (lines 15 or 60) or returns early (Line 18), the flag remains true forever, causing the UI to display a spinner indefinitely.
🔎 Recommended fix using a scope guard pattern
pub fn scan_networks(wifi_list: Arc<RwLock<Vec<WifiNetwork>>>, is_scanning: Arc<AtomicBool>) {
- is_scanning.store(true, Ordering::SeqCst);
- // // nmcli -t -f IN-USE,SSID,SECURITY device wifi list
thread::spawn(move || {
+ is_scanning.store(true, Ordering::SeqCst);
+ // Ensure the flag is reset when the thread exits (success, error, or panic)
+ let _guard = scopeguard::guard((), |_| {
+ is_scanning.store(false, Ordering::SeqCst);
+ });
+
let output = Command::new("nmcli")
.args(["-t", "-f", "IN-USE,SSID,SECURITY", "device", "wifi", "list"])
.output()
- .expect("Failed to execute nmcli command");
+ .ok()?;
if !output.status.success() {
- return;
+ return None;
}
let stdout = String::from_utf8_lossy(&output.stdout);
let mut networks: Vec<WifiNetwork> = Vec::new();
let saved_networks = saved_connections();
for line in stdout.lines() {
let mut parts = line.splitn(3, ':');
let in_use = parts.next() == Some("*");
let ssid = parts.next().unwrap_or("").to_string();
let security = parts
.next()
.map(|s| {
if s.is_empty() {
"Unsecured".to_string()
} else {
s.to_string()
}
})
.unwrap_or("?".to_string());
let is_saved = saved_networks.contains(&ssid);
if !ssid.is_empty() {
networks.push(WifiNetwork {
is_saved,
in_use,
ssid,
security,
})
}
}
networks.push(WifiNetwork {
is_saved: false,
in_use: false,
ssid: "Connect to Hidden network".to_string(),
security: "?".to_string(),
});
- let mut wifi_list_lock = wifi_list.write().expect("WifiNetworks lock poisoned");
+ let mut wifi_list_lock = wifi_list.write().ok()?;
*wifi_list_lock = networks;
- is_scanning.store(false, Ordering::SeqCst);
+ Some(())
});
}Note: This requires adding the scopeguard crate. Alternatively, you can use a custom Drop implementation or wrap the thread body in a closure that always resets the flag.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
can you create issue related to that I will fix it later
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the scanning flag bug
gh issue create \
--title "Critical: is_scanning flag can remain stuck at true indefinitely" \
--body "## Description
The \`is_scanning\` flag in \`src/utils/scan.rs\` can become permanently stuck at \`true\`, causing the UI to display a spinner indefinitely.
## Problem
In the \`scan_networks\` function, the \`is_scanning\` flag is set to \`true\` at the start of the function, but it is only reset to \`false\` at the end of the spawned thread's happy path. If the thread panics (e.g., at the \`expect()\` calls) or returns early (e.g., when the command fails), the flag remains \`true\` forever.
## Impact
When this occurs, users will see a continuous scanning animation and will be unable to interact with the network list, effectively breaking the application's functionality.
## Related PR and Comment
- PR: https://github.com/santoshxshrestha/nmtui/pull/30
- Original comment: https://github.com/santoshxshrestha/nmtui/pull/30#discussion_r2659084183
- Reported by: @coderabbitai" \
--assignee santoshxshrestha
Length of output: 51
@santoshxshrestha, I've created issue #31 to track this bug. The issue describes the problem where the is_scanning flag can get stuck at true indefinitely, along with the impact on users. You can address it in a future PR. 👍
🧬 Code Graph Analysis Results
[src/utils/saved_connection.rs] lines 3-28
pub fn saved_connections() -> Vec<String> {
// nmcli -t -f NAME,TYPE connection show
let output = Command::new("nmcli")
.args(["-t", "-f", "NAME,TYPE", "connection", "show"])
.output()
.expect(" Failed to execute nmcli command");
let mut ssids: Vec<String> = Vec::new();
if !output.status.success() {
return ssids;
}
let stdout = String::from_utf8_lossy(&output.stdout);
// header is already handled by nmcli with -t flag
for line in stdout.lines() {
let mut parts = line.splitn(2, ':');
let ssid = parts.next().unwrap_or("").to_string();
let connection_type = parts.next().unwrap_or("").to_string();
// we are only interested in wifi saved connections
if !ssid.is_empty() && !connection_type.is_empty() && connection_type == "802-11-wireless" {
ssids.push(ssid);
}
}
ssids
}
[src/utils/scan.rs] (function: scan_networks)
- Summary: scan_networks takes an Arc<RwLock<Vec>> and an Arc is_scanning. It marks scanning as true, spawns a thread to execute the system command nmcli to list wifi networks, parses each line of output (IN-USE:SSID:SECURITY) into WifiNetwork entries (computing in_use, ssid, security, and is_saved by checking saved_connections()). It adds a synthetic "Connect to Hidden network" entry, writes the resulting vector into wifi_list, and then marks is_scanning as false. The function returns immediately after spawning the thread.
1 participant
Summary by CodeRabbit
New Features
Changed
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.