feat: captive portal auto-login - #67
Open
aashish-thapa wants to merge 9 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds captive-portal detection and auto-launch: new Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant PortalWatcher
participant NMClient
participant Notif as NotificationQueue
participant Browser
App->>PortalWatcher: poll(nm_client, device_path)
PortalWatcher->>NMClient: get_connectivity_state()
NMClient-->>PortalWatcher: Connectivity state
alt Transition to PORTAL
PortalWatcher->>NMClient: get_current_ssid(device_path)
NMClient-->>PortalWatcher: SSID
PortalWatcher->>NMClient: get_connectivity_check_uri()
NMClient-->>PortalWatcher: probe URL
PortalWatcher-->>App: PortalDetected{ssid, url}
App->>App: check config.captive_portal.auto_open
alt auto_open enabled
App->>Notif: enqueue info (ttl=5)
App->>Browser: launch_browser(url)
Browser-->>App: Ok / Err
alt Browser Err
App->>Notif: enqueue error (ttl=5)
end
end
else No portal / unchanged
PortalWatcher-->>App: None
end
sequenceDiagram
participant User
participant CLI as wlctl portal
participant Portal as portal::run_once()
participant NMClient
participant Browser
User->>CLI: invoke `wlctl portal`
CLI->>Portal: run_once()
Portal->>NMClient: check connectivity
NMClient-->>Portal: Connectivity result
Portal->>User: print status
alt Connectivity == PORTAL
Portal->>NMClient: get_connectivity_check_uri()
NMClient-->>Portal: probe URL
alt URL non-empty
Portal->>Browser: launch(url)
Browser-->>Portal: Ok / Err
end
end
Portal-->>CLI: return
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
src/app.rs (1)
165-173: Don’t silently drop portal polling errors.Line 172 discards all poll failures, which makes auto-open regressions hard to debug. Consider logging at debug/warn while keeping this path non-fatal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 165 - 173, The match on portal_watcher.poll currently swallows all errors (Err(_)) silently; change the Err branch to log the error (including the error value) at debug or warn level rather than returning silently so failures in portal_watcher.poll(&self.client, &self.device.device_path).await are visible; keep the non-fatal behavior (still return after logging) and include context (e.g., the device path and that polling failed) to aid debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/portal/browser.rs`:
- Around line 15-18: The current env::var("BROWSER") handling treats the whole
BROWSER string as a single executable which fails if it contains multiple
candidates, colons or embedded args; change the logic that builds (program,
args) so it (1) splits BROWSER on ':' into candidates and tries them in order,
(2) for a chosen candidate splits on whitespace to treat the first token as the
executable and the rest as initial args, and (3) appends the URL to those args;
if none of the candidates spawn successfully fall back to "xdg-open" with the
URL. Update the same parsing used later around the other BROWSER handling (lines
20-27) so both places use this candidate-and-token-splitting behavior instead of
passing the entire BROWSER string as the program.
In `@src/portal/mod.rs`:
- Around line 49-70: previous_state and opened_for are being mutated before
confirming the SSID and connectivity URI, which can permanently suppress portal
handling on transient lookup failures; move the mutation logic so you only call
self.previous_state.replace(state) and self.opened_for.insert(ssid.clone())
after nm.get_connected_ssid(...) and nm.get_connectivity_check_uri() both
succeed and url is non-empty. Concretely: call
nm.get_connected_ssid(device_path).await and
nm.get_connectivity_check_uri().await first, bail early on failures or empty
URI, then perform the previous_state update (replace) and the opened_for.insert
only when you know you have a valid ssid and url, using the same identifiers
previous_state and opened_for to find the correct spots to move these calls.
---
Nitpick comments:
In `@src/app.rs`:
- Around line 165-173: The match on portal_watcher.poll currently swallows all
errors (Err(_)) silently; change the Err branch to log the error (including the
error value) at debug or warn level rather than returning silently so failures
in portal_watcher.poll(&self.client, &self.device.device_path).await are
visible; keep the non-fatal behavior (still return after logging) and include
context (e.g., the device path and that polling failed) to aid debugging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: abf41b02-4412-4bc5-843d-83d15cd8b465
📒 Files selected for processing (8)
src/app.rssrc/cli.rssrc/config.rssrc/lib.rssrc/main.rssrc/nm/mod.rssrc/portal/browser.rssrc/portal/mod.rs
…transient errors - resolve_command splits $BROWSER on colons and takes the first non-empty trimmed candidate, then whitespace-splits for program + args. Handles BROWSER="firefox --private-window" and BROWSER="firefox:chromium" correctly; previously the whole string was treated as a single executable name, breaking auto-open silently - PortalWatcher::poll now commits previous_state and opened_for only after SSID + URL lookups succeed. A transient D-Bus failure previously marked the portal transition as already handled, making auto-open never fire again for that session - URL lookup uses ? instead of unwrap_or_default() so genuine errors surface rather than masquerading as "no URL configured" - 6 unit tests cover resolve_command edge cases (unset, empty, whitespace, plain, args, colon fallbacks, leading empties)
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/portal/mod.rs (2)
71-74: Differentiate SSID absence from SSID lookup failure.Line 71 currently treats
Ok(None)andErr(_)the same (Ok(None)), which can hide persistent lookup failures and make the feature fail silently.Proposed adjustment
- let ssid = match nm.get_connected_ssid(device_path).await { - Ok(Some(s)) => s, - _ => return Ok(None), - }; + let ssid = match nm.get_connected_ssid(device_path).await { + Ok(Some(s)) => s, + Ok(None) => return Ok(None), + Err(e) => return Err(e.into()), + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/portal/mod.rs` around lines 71 - 74, The current match on nm.get_connected_ssid(device_path).await conflates Ok(None) and Err(_) by returning Ok(None); change it so Ok(Some(s)) assigns ssid, Ok(None) still returns Ok(None) (SSID not present), but Err(e) is propagated as an error (return Err(e) or map the error into the function's error type) so lookup failures are not swallowed; ensure the surrounding function's Result error type supports returning that error from get_connected_ssid.
151-167: Tests should coverPortalWatcher::poll()behavior, not just data structures.Current tests are useful but don’t guard the transition/error semantics that this PR depends on (portal transition, transient lookup failure retry, per-SSID suppression, empty URL handling).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/portal/mod.rs` around lines 151 - 167, Add unit tests that exercise PortalWatcher::poll() behavior rather than only structs: write tests that use the existing watcher_with helper (or create a similar mocked PortalWatcher) to simulate (1) a successful portal detection sequence and verify state transition and emitted PortalDetected (ssid and url) instead of just PortalDetected construction, (2) a transient DNS/lookup failure followed by success to ensure poll() retries and ultimately emits the portal event, (3) per-SSID suppression by calling poll() twice for the same ssid and asserting only the first poll emits PortalDetected (use opened_for to verify suppression), and (4) handling of an empty URL response by asserting poll() does not emit a PortalDetected and updates state appropriately; target functions/types PortalWatcher::poll, watcher_with, opened_for, and PortalDetected when adding these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/portal/mod.rs`:
- Around line 71-74: The current match on
nm.get_connected_ssid(device_path).await conflates Ok(None) and Err(_) by
returning Ok(None); change it so Ok(Some(s)) assigns ssid, Ok(None) still
returns Ok(None) (SSID not present), but Err(e) is propagated as an error
(return Err(e) or map the error into the function's error type) so lookup
failures are not swallowed; ensure the surrounding function's Result error type
supports returning that error from get_connected_ssid.
- Around line 151-167: Add unit tests that exercise PortalWatcher::poll()
behavior rather than only structs: write tests that use the existing
watcher_with helper (or create a similar mocked PortalWatcher) to simulate (1) a
successful portal detection sequence and verify state transition and emitted
PortalDetected (ssid and url) instead of just PortalDetected construction, (2) a
transient DNS/lookup failure followed by success to ensure poll() retries and
ultimately emits the portal event, (3) per-SSID suppression by calling poll()
twice for the same ssid and asserting only the first poll emits PortalDetected
(use opened_for to verify suppression), and (4) handling of an empty URL
response by asserting poll() does not emit a PortalDetected and updates state
appropriately; target functions/types PortalWatcher::poll, watcher_with,
opened_for, and PortalDetected when adding these tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 57c0d5c8-fff6-4ab9-8049-90fbc85cd2eb
📒 Files selected for processing (2)
src/portal/browser.rssrc/portal/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/portal/browser.rs
get_connected_ssid returns Result<Option<String>>. The previous match collapsed both `Err(_)` (D-Bus failure) and `Ok(None)` (no associated SSID) into a silent early-return, hiding transient lookup failures. Use `?` to propagate errors (consistent with the sibling URL lookup three lines below) and `let-else` on the `Option` to keep the "no SSID currently" no-op explicit. Caller behaviour in app.rs is unchanged — both paths still result in no notification — but the error now exists at the API boundary for future logging.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/portal/mod.rs (2)
90-93:opened_foris recorded before launch success is known.At Line 92, the SSID is marked handled before the caller performs browser launch. A transient launch failure can suppress further auto-open attempts for that SSID during the session.
💡 Suggested direction
pub struct PortalWatcher { previous_state: Option<Connectivity>, opened_for: HashSet<String>, } impl PortalWatcher { + pub fn mark_opened(&mut self, ssid: &str) { + self.opened_for.insert(ssid.to_string()); + } + pub async fn poll( &mut self, nm: &Arc<NMClient>, device_path: &str, ) -> Result<Option<PortalDetected>> { @@ - self.opened_for.insert(ssid.clone()); Ok(Some(PortalDetected { ssid, url })) } }Then call
mark_opened()only afterlaunch_browser()succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/portal/mod.rs` around lines 90 - 93, The SSID is being recorded as handled too early — move the mutation of self.opened_for (or call to mark_opened()) so it only happens after a successful browser launch; specifically, after launch_browser() returns without error, set self.previous_state = Some(Connectivity::Portal) and then insert the SSID into opened_for (or call mark_opened(ssid)) so transient launch failures won't suppress future auto-open attempts for that SSID during the session.
152-168: Tests currently misspoll()transition regressions.These tests validate data structures but not watcher behavior (transition gating, SSID suppression, transient lookup failures). Adding
poll()-focused tests would guard the core portal logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/portal/mod.rs` around lines 152 - 168, Add unit tests that exercise the watcher's poll() transition logic rather than only struct behavior: create tests using watcher_with(...) and PortalDetected to simulate detection sequences and call w.poll() between events to assert state transitions (e.g., gating from Connectivity states, that opened_for set suppresses repeated PortalDetected for the same SSID, and that transient lookup failures (simulate lookup error) do not permanently mark SSIDs as opened). Specifically add tests that (1) send repeated PortalDetected("work") events with intermediate w.poll() and assert only the first triggers an "open" path (opened_for behavior), (2) simulate a lookup failure then a success around poll() to verify the watcher retries/transient behavior, and (3) verify connectivity-based gating by toggling Connectivity variants and calling poll() to ensure transitions occur as expected; reference watcher_with, poll(), opened_for, and PortalDetected when locating where to add these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/portal/mod.rs`:
- Around line 63-66: The early return skips SSID checks whenever
self.previous_state == Some(Connectivity::Portal), causing missed Portal→Portal
SSID changes; update the logic in the function containing the return so it only
short-circuits when the prior SSID is the same as the current one (e.g., check
self.previous_ssid == current_ssid) or remove the return and let downstream SSID
comparison logic run; reference self.previous_state, self.previous_ssid and
Connectivity::Portal to locate and adjust the condition so Portal→Portal
transitions with different SSIDs are detected.
---
Nitpick comments:
In `@src/portal/mod.rs`:
- Around line 90-93: The SSID is being recorded as handled too early — move the
mutation of self.opened_for (or call to mark_opened()) so it only happens after
a successful browser launch; specifically, after launch_browser() returns
without error, set self.previous_state = Some(Connectivity::Portal) and then
insert the SSID into opened_for (or call mark_opened(ssid)) so transient launch
failures won't suppress future auto-open attempts for that SSID during the
session.
- Around line 152-168: Add unit tests that exercise the watcher's poll()
transition logic rather than only struct behavior: create tests using
watcher_with(...) and PortalDetected to simulate detection sequences and call
w.poll() between events to assert state transitions (e.g., gating from
Connectivity states, that opened_for set suppresses repeated PortalDetected for
the same SSID, and that transient lookup failures (simulate lookup error) do not
permanently mark SSIDs as opened). Specifically add tests that (1) send repeated
PortalDetected("work") events with intermediate w.poll() and assert only the
first triggers an "open" path (opened_for behavior), (2) simulate a lookup
failure then a success around poll() to verify the watcher retries/transient
behavior, and (3) verify connectivity-based gating by toggling Connectivity
variants and calling poll() to ensure transitions occur as expected; reference
watcher_with, poll(), opened_for, and PortalDetected when locating where to add
these tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+63
to
+66
| // Already in Portal on the previous tick — no transition to report. | ||
| if self.previous_state == Some(Connectivity::Portal) { | ||
| return Ok(None); | ||
| } |
There was a problem hiding this comment.
Portal→Portal SSID switches can be missed.
The early return at Line 64 skips SSID evaluation whenever the prior tick was Portal. If a user moves from one captive SSID to another without an observed non-Portal tick, the second network may never trigger detection.
💡 Suggested fix
- // Already in Portal on the previous tick — no transition to report.
- if self.previous_state == Some(Connectivity::Portal) {
- return Ok(None);
- }
+ // Even if we were already in Portal, continue to SSID evaluation so
+ // portal→portal SSID switches are not missed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/portal/mod.rs` around lines 63 - 66, The early return skips SSID checks
whenever self.previous_state == Some(Connectivity::Portal), causing missed
Portal→Portal SSID changes; update the logic in the function containing the
return so it only short-circuits when the prior SSID is the same as the current
one (e.g., check self.previous_ssid == current_ssid) or remove the return and
let downstream SSID comparison logic run; reference self.previous_state,
self.previous_ssid and Connectivity::Portal to locate and adjust the condition
so Portal→Portal transitions with different SSIDs are detected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #61.
Summary
When NetworkManager reports
Connectivity=PORTAL,wlctlnow opens the probe URL in the user's browser automatically. The portal's HTTP redirect sends them to the login page.What's included
PortalWatcher— tracks state transitions and a per-session SSID ignore-list so we never spamlaunch_browser— resolves$BROWSERthen falls back toxdg-open; child is fully detachedApp::tick()polls the watcher and pushes a notification when a new portal is detectedwlctl portalsubcommand — one-shot manual trigger for scriptingcaptive_portal.auto_openconfig toggle (defaulttrue) to disable the background behaviorFollow-ups (not in this PR)
Summary by CodeRabbit
New Features
Configuration