Conversation
listner for the saved connection in the handle_events
connections that are saved
📝 WalkthroughWalkthroughAdds a saved-connections feature: new module to fetch and store Wi‑Fi connections via Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant SavedConnections
participant nmcli
participant UI
User->>App: press 's'
App->>App: handle_events() → open_saved_list()
App->>SavedConnections: fetch_saved_connections()
SavedConnections->>nmcli: execute nmcli (NAME,TYPE,TIMESTAMP-REAL)
nmcli-->>SavedConnections: return output
SavedConnections->>SavedConnections: parse & filter 802-11-wireless
SavedConnections-->>App: populate connections
User->>App: press 'j' / Down
App->>App: handle_saved() → update_selected_saved_network(+1)
App->>UI: request render of saved table
UI->>User: display saved table with highlighted row
User->>App: press Esc
App->>App: close_saved_list() (show_saved = false)
UI->>User: hide saved table
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 5
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/apps/core.rssrc/apps/core/event_handlers.rssrc/apps/core/saved_connection.rssrc/apps/core/widget.rs
🧰 Additional context used
🧬 Code graph analysis (3)
src/apps/core.rs (1)
src/apps/handlers/status.rs (1)
default(13-19)
src/apps/core/widget.rs (1)
src/apps/core.rs (1)
default(29-42)
src/apps/core/saved_connection.rs (1)
src/apps/handlers/status.rs (1)
new(23-28)
🔇 Additional comments (6)
src/apps/core/event_handlers.rs (1)
101-107: LGTM! Key binding follows existing patterns.The 's' key binding is correctly implemented and consistent with other key handlers in the file.
src/apps/core/widget.rs (1)
96-124: LGTM! Saved connections rendering follows established patterns.The rendering block correctly overlays the main UI, applies appropriate styling, and uses the correct selection index from
saved_connection.selected_index.src/apps/core.rs (2)
3-3: LGTM! Module and import declarations are correct.The new module is properly declared and the import follows the correct path structure.
Also applies to: 7-7
23-23: LGTM! Struct fields and initialization are correct.The new fields are properly typed and initialized with appropriate default values.
Also applies to: 25-25, 39-40
src/apps/core/saved_connection.rs (2)
9-19: LGTM! Struct definitions are clear and appropriate.The
ConnectionsandSavedConnectionsstructs are well-defined with appropriate fields and derive traits.
123-140: LGTM! Helper methods are correctly implemented.The navigation and list management methods properly handle bounds checking, wrap-around selection, and state management.
| pub fn fetch_saved_connections(&mut self) { | ||
| let output = Command::new("nmcli") | ||
| .args(["-t", "-f", "NAME,TYPE,TIMESTAMP-REAL", "connection", "show"]) | ||
| .output() | ||
| .expect(" Failed to execute nmcli command"); |
There was a problem hiding this comment.
Replace panic with graceful error handling.
Using .expect() will cause the application to panic if the nmcli command fails or is not installed. Consider handling the error gracefully and setting an empty connections list instead.
🔎 Proposed fix
pub fn fetch_saved_connections(&mut self) {
- let output = Command::new("nmcli")
- .args(["-t", "-f", "NAME,TYPE,TIMESTAMP-REAL", "connection", "show"])
- .output()
- .expect(" Failed to execute nmcli command");
+ let output = match Command::new("nmcli")
+ .args(["-t", "-f", "NAME,TYPE,TIMESTAMP-REAL", "connection", "show"])
+ .output() {
+ Ok(output) => output,
+ Err(_) => {
+ self.connections = Vec::new();
+ return;
+ }
+ };📝 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 fetch_saved_connections(&mut self) { | |
| let output = Command::new("nmcli") | |
| .args(["-t", "-f", "NAME,TYPE,TIMESTAMP-REAL", "connection", "show"]) | |
| .output() | |
| .expect(" Failed to execute nmcli command"); | |
| pub fn fetch_saved_connections(&mut self) { | |
| let output = match Command::new("nmcli") | |
| .args(["-t", "-f", "NAME,TYPE,TIMESTAMP-REAL", "connection", "show"]) | |
| .output() { | |
| Ok(output) => output, | |
| Err(_) => { | |
| self.connections = Vec::new(); | |
| return; | |
| } | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/apps/core/saved_connection.rs (1)
23-26: Replace panic with graceful error handling.The
.expect()call will crash the application ifnmcliis unavailable or fails to execute. This was previously flagged as a critical issue.🔎 Proposed fix
- let output = Command::new("nmcli") - .args(["-t", "-f", "NAME,TYPE,TIMESTAMP-REAL", "connection", "show"]) - .output() - .expect(" Failed to execute nmcli command"); + let output = match Command::new("nmcli") + .args(["-t", "-f", "NAME,TYPE,TIMESTAMP-REAL", "connection", "show"]) + .output() { + Ok(output) => output, + Err(_) => { + self.connections = Vec::new(); + return; + } + };
🧹 Nitpick comments (1)
src/apps/core/saved_connection.rs (1)
78-82: Clarify or implement the 'd' key handler.The empty handler for the 'd' key appears to be a placeholder. If this is intended for deleting saved connections, consider implementing it or leaving a TODO comment. If it's not needed, remove it.
Do you want me to help implement delete functionality for saved connections, or should this handler be removed?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/apps/core/run.rssrc/apps/core/saved_connection.rs
🔇 Additional comments (4)
src/apps/core/run.rs (1)
15-18: LGTM! Clean integration of saved connections popup handling.The new branch follows the established pattern for popup handling in the run loop and integrates correctly with the saved connections feature.
src/apps/core/saved_connection.rs (3)
8-18: LGTM! Well-structured data models.The
ConnectionsandSavedConnectionsstructs are clean and appropriate for managing saved Wi-Fi network state.
117-125: LGTM! Correct wrap-around navigation logic.The use of
rem_euclidprovides proper wrap-around behavior, and the empty list check prevents index errors.
126-133: LGTM! Clean popup state management.Both
open_saved_listandclose_saved_listmethods correctly manage the saved connections popup state.
| let connection_type = parts | ||
| .next() | ||
| .map(|s| { | ||
| if s.is_empty() { | ||
| "Unsecured".to_string() | ||
| } else { | ||
| s.to_string() | ||
| } | ||
| }) | ||
| .unwrap_or("".to_string()); | ||
|
|
||
| let last_used = parts.next().unwrap_or("").to_string(); | ||
|
|
||
| if !ssid.is_empty() | ||
| && !connection_type.is_empty() | ||
| && connection_type == "802-11-wireless" | ||
| { | ||
| connections.push(Connections { ssid, last_used }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove dead code: "Unsecured" mapping is never used.
Lines 43-52 map empty connection types to "Unsecured", but line 58 filters exclusively for "802-11-wireless", so these "Unsecured" entries are always filtered out. Either extend the filter to include unsecured connections or remove the unnecessary mapping logic.
🔎 Proposed fix: Remove the unused mapping
- // if the connection type is empty, it means its unsecured
- let connection_type = parts
- .next()
- .map(|s| {
- if s.is_empty() {
- "Unsecured".to_string()
- } else {
- s.to_string()
- }
- })
- .unwrap_or("".to_string());
+ let connection_type = parts.next().unwrap_or("").to_string();
let last_used = parts.next().unwrap_or("").to_string();
- if !ssid.is_empty()
- && !connection_type.is_empty()
- && connection_type == "802-11-wireless"
- {
+ if !ssid.is_empty() && connection_type == "802-11-wireless" {
connections.push(Connections { ssid, last_used });
}📝 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.
| let connection_type = parts | |
| .next() | |
| .map(|s| { | |
| if s.is_empty() { | |
| "Unsecured".to_string() | |
| } else { | |
| s.to_string() | |
| } | |
| }) | |
| .unwrap_or("".to_string()); | |
| let last_used = parts.next().unwrap_or("").to_string(); | |
| if !ssid.is_empty() | |
| && !connection_type.is_empty() | |
| && connection_type == "802-11-wireless" | |
| { | |
| connections.push(Connections { ssid, last_used }); | |
| } | |
| let connection_type = parts.next().unwrap_or("").to_string(); | |
| let last_used = parts.next().unwrap_or("").to_string(); | |
| if !ssid.is_empty() && connection_type == "802-11-wireless" { | |
| connections.push(Connections { ssid, last_used }); | |
| } |
🤖 Prompt for AI Agents
In src/apps/core/saved_connection.rs around lines 43 to 61, the branch that maps
an empty connection type to "Unsecured" is dead because later code only accepts
"802-11-wireless"; remove that unused mapping by replacing the map/if block with
a simple parts.next().unwrap_or("".to_string()) (or similar direct assignment)
so connection_type contains the raw value, and keep the existing filter that
checks for "802-11-wireless"; alternatively, if you intended to accept unsecured
entries, extend the filter to also allow "Unsecured" instead of keeping the
mapping—pick one approach and remove the unreachable mapping logic accordingly.
1 participant
list
handlers
type
that are required to view the saved connection are added with some of the refact
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.