fix: trigger native captive portal popup for Starbucks and Komeda WiFi - #704
Conversation
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
shunkakinoki
added
the
automerge
Summary of ChangesHello @shunkakinoki, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughUpdated macOS keepalive script to detect additional SSID (Komeda_Wi-Fi) and, on failed connectivity to 1.1.1.1, restart the Wi‑Fi interface (toggle off, sleep, toggle on) instead of opening a captive portal URL. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Mesa DescriptionTL;DRTriggers the native macOS captive portal popup for Starbucks and Komeda_Wi-Fi by restarting the Wi-Fi interface when connectivity is lost. This change improves reliability with a 3-second delay and removes custom notifications in favor of the system popup. What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the keepalive script to improve handling of captive portals on specific WiFi networks on macOS by restarting the WiFi interface, which is a clever way to trigger the native system popup. The change also extends support to Komeda WiFi networks.
My main feedback is regarding the use of the hardcoded network interface en0. This is not guaranteed to be the WiFi interface on all Mac hardware, which could make the script fail on some machines. I've added a specific comment with a suggestion to dynamically detect the WiFi interface to make the script more robust.
Additionally, it appears the tests in spec/keepalive_spec.sh have not been updated to reflect these changes. The existing tests check for the old behavior and will likely fail. Please remember to update the tests to cover the new WiFi restart logic.
With these adjustments, this will be a solid improvement.
| networksetup -setairportpower en0 off | ||
| sleep 3 | ||
| networksetup -setairportpower en0 on |
There was a problem hiding this comment.
Hardcoding the network interface as en0 is not reliable, as the WiFi interface can have a different name (e.g., en1) on different Mac models. This hardcoded value is used here and also on line 16 to get the SSID. It would be more robust to dynamically determine the WiFi interface name.
Since a code suggestion can only replace the selected lines, I'm providing an example in this comment body for you to apply across the entire darwin-specific block:
if [[ $OSTYPE == "darwin"* ]]; then
WIFI_DEVICE=$(networksetup -listallhardwareports | awk '/Hardware Port: Wi-Fi/{getline; print $2}')
if [[ -z "$WIFI_DEVICE" ]]; then exit 0; fi
SSID=$(networksetup -getairportnetwork "$WIFI_DEVICE" 2>/dev/null | awk -F": " '{print $2}' || echo "")
if [[ $SSID == *"STARBUCKS"* ]] || [[ $SSID == *"Komeda_Wi-Fi"* ]]; then
if ! ping -c 1 -W 2 1.1.1.1 >/dev/null 2>&1; then
networksetup -setairportpower "$WIFI_DEVICE" off
sleep 3
networksetup -setairportpower "$WIFI_DEVICE" on
fi
fi
fiThere was a problem hiding this comment.
Pull request overview
This PR modifies the captive portal handling for WiFi networks to trigger the native macOS captive portal popup instead of opening a URL. The approach now restarts the WiFi interface when connectivity is lost on specific networks (Starbucks and Komeda WiFi).
Changes:
- Replaced URL-based captive portal trigger with WiFi interface restart (networksetup commands)
- Added support for Komeda_Wi-Fi networks alongside existing Starbucks support
- Introduced 3-second delay between WiFi power off and on
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SSID=$(networksetup -getairportnetwork en0 2>/dev/null | awk -F": " '{print $2}' || echo "") | ||
|
|
||
| # Check for Starbucks networks (e.g., at_STARBUCKS_Wi2) | ||
| if [[ $SSID == *"STARBUCKS"* ]]; then | ||
| # Check for Starbucks or Komeda networks | ||
| if [[ $SSID == *"STARBUCKS"* ]] || [[ $SSID == *"Komeda_Wi-Fi"* ]]; then | ||
| if ! ping -c 1 -W 2 1.1.1.1 >/dev/null 2>&1; then | ||
| open "http://captive.apple.com" 2>/dev/null || true | ||
| # Restart WiFi to trigger macOS captive portal popup | ||
| networksetup -setairportpower en0 off | ||
| sleep 3 | ||
| networksetup -setairportpower en0 on |
There was a problem hiding this comment.
The script hardcodes the WiFi interface as 'en0', which may not be correct on all macOS systems. Different Mac models and configurations can have WiFi on different interfaces (e.g., en1, en2). Consider detecting the active WiFi interface dynamically, or make the interface name configurable. You could use 'networksetup -listallhardwareports' to find the WiFi interface programmatically.
| open "http://captive.apple.com" 2>/dev/null || true | ||
| # Restart WiFi to trigger macOS captive portal popup | ||
| networksetup -setairportpower en0 off | ||
| sleep 3 |
There was a problem hiding this comment.
The PR description states "Increases WiFi restart delay to 3 seconds for reliability" and "increased from 1 second", but the previous code opened captive.apple.com directly without any delay. There was no previous 1-second delay to increase from. This discrepancy in the PR description is misleading.
| networksetup -setairportpower en0 off | ||
| sleep 3 | ||
| networksetup -setairportpower en0 on |
There was a problem hiding this comment.
The networksetup commands may fail due to insufficient permissions or if the WiFi interface is busy. Consider adding error handling to prevent the script from failing due to 'set -euo pipefail'. For example, you could append '|| true' to these commands or temporarily disable error handling for this block to ensure the script continues to run even if the WiFi restart fails.
| networksetup -setairportpower en0 off | |
| sleep 3 | |
| networksetup -setairportpower en0 on | |
| networksetup -setairportpower en0 off || true | |
| sleep 3 | |
| networksetup -setairportpower en0 on || true |
| # macOS-specific: Open captive portal for Starbucks WiFi if connectivity is lost | ||
| # macOS-specific: Restart WiFi to trigger captive portal popup when connectivity is lost | ||
| if [[ $OSTYPE == "darwin"* ]]; then | ||
| SSID=$(networksetup -getairportnetwork en0 2>/dev/null | awk -F": " '{print $2}' || echo "") |
There was a problem hiding this comment.
The script uses system utilities (networksetup, ping, and awk) that may not be in the PATH configured in default.nix. The launchd configuration at line 15 of default.nix only includes curl in the PATH. On macOS, networksetup is in /usr/sbin/, ping is in /sbin/, and awk is in /usr/bin/. Consider updating the PATH in default.nix to include these system directories, following the pattern used in home-manager/services/docker-postgres/default.nix line 14, which sets PATH to include /usr/local/bin:/opt/homebrew/bin:/usr/bin:/bin. You should add /usr/sbin and /sbin to ensure these commands are available.
| # Restart WiFi to trigger macOS captive portal popup | ||
| networksetup -setairportpower en0 off | ||
| sleep 3 | ||
| networksetup -setairportpower en0 on |
There was a problem hiding this comment.
The test at line 107 in spec/keepalive_spec.sh expects the script to include 'captive.apple.com', but this has been removed in favor of the WiFi restart approach. The test needs to be updated to check for the new behavior (networksetup commands and sleep 3) instead.
| # Check for Starbucks networks (e.g., at_STARBUCKS_Wi2) | ||
| if [[ $SSID == *"STARBUCKS"* ]]; then | ||
| # Check for Starbucks or Komeda networks | ||
| if [[ $SSID == *"STARBUCKS"* ]] || [[ $SSID == *"Komeda_Wi-Fi"* ]]; then |
There was a problem hiding this comment.
The test description at line 88 in spec/keepalive_spec.sh is titled 'Starbucks WiFi detection (macOS)' but the code now supports both Starbucks and Komeda networks. The test suite should be updated to reflect this broader scope and include test cases for Komeda_Wi-Fi pattern matching.
| if ! ping -c 1 -W 2 1.1.1.1 >/dev/null 2>&1; then | ||
| open "http://captive.apple.com" 2>/dev/null || true | ||
| # Restart WiFi to trigger macOS captive portal popup | ||
| networksetup -setairportpower en0 off | ||
| sleep 3 | ||
| networksetup -setairportpower en0 on |
There was a problem hiding this comment.
The script will restart WiFi every 3 seconds (the StartInterval in default.nix) when connected to Starbucks or Komeda networks without internet connectivity. This could be disruptive to the user if the captive portal is never completed or if connectivity is unavailable. Consider adding a mechanism to track the last restart time and only restart WiFi if a certain cooldown period has elapsed (e.g., 30-60 seconds), or add a file-based flag to indicate a restart was attempted recently.
![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
🤖 Fix all issues with AI agents
In `@home-manager/services/neverssl-keepalive/keepalive.sh`:
- Around line 21-24: The network setup toggles (networksetup -setairportpower
en0 off/on) are unguarded and will cause the script to exit under set -e if they
fail; update keepalive.sh to run each networksetup call with a guard (e.g.,
if/|| check) that captures non-zero exit status and logs a warning using the
same logging mechanism used earlier in the script (the existing networksetup
error handling at line 16) instead of letting the script abort, so failures due
to permissions, wrong interface, or missing device are reported but do not
terminate execution.
🧹 Nitpick comments (1)
home-manager/services/neverssl-keepalive/keepalive.sh (1)
16-24: Avoid hardcodingen0; detect the Wi‑Fi interface once.On some Macs the Wi‑Fi device is
en1, so the SSID read/toggle won’t work. A small pre‑step to detect the Wi‑Fi device makes this portable.♻️ Recommended refactor
- SSID=$(networksetup -getairportnetwork en0 2>/dev/null | awk -F": " '{print $2}' || echo "") + WIFI_IFACE=$(networksetup -listallhardwareports \ + | awk '/Wi-Fi|AirPort/{getline; print $2; exit}') + if [[ -z $WIFI_IFACE ]]; then + echo "$(date): WARN: Wi‑Fi interface not found" >&2 + exit 0 + fi + SSID=$(networksetup -getairportnetwork "$WIFI_IFACE" 2>/dev/null | awk -F": " '{print $2}' || echo "") ... - networksetup -setairportpower en0 off + networksetup -setairportpower "$WIFI_IFACE" off sleep 3 - networksetup -setairportpower en0 on + networksetup -setairportpower "$WIFI_IFACE" on
| # Restart WiFi to trigger macOS captive portal popup | ||
| networksetup -setairportpower en0 off | ||
| sleep 3 | ||
| networksetup -setairportpower en0 on |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n home-manager/services/neverssl-keepalive/keepalive.shRepository: shunkakinoki/dotfiles
Length of output: 1141
Guard networksetup commands against failures under set -e.
Lines 22–24 call networksetup without error handling. If either command fails—due to missing admin privileges, wrong interface, or unavailable device—set -e will terminate the script before the WiFi toggle completes. This is inconsistent with line 16, which already handles networksetup errors. Add guards and log warnings on failure:
🛠️ Suggested hardening
# Restart WiFi to trigger macOS captive portal popup
- networksetup -setairportpower en0 off
- sleep 3
- networksetup -setairportpower en0 on
+ if ! networksetup -setairportpower en0 off; then
+ echo "$(date): WARN: failed to power off Wi‑Fi (needs admin?)" >&2
+ else
+ sleep 3
+ if ! networksetup -setairportpower en0 on; then
+ echo "$(date): WARN: failed to power on Wi‑Fi (needs admin?)" >&2
+ fi
+ fi🤖 Prompt for AI Agents
In `@home-manager/services/neverssl-keepalive/keepalive.sh` around lines 21 - 24,
The network setup toggles (networksetup -setairportpower en0 off/on) are
unguarded and will cause the script to exit under set -e if they fail; update
keepalive.sh to run each networksetup call with a guard (e.g., if/|| check) that
captures non-zero exit status and logs a warning using the same logging
mechanism used earlier in the script (the existing networksetup error handling
at line 16) instead of letting the script abort, so failures due to permissions,
wrong interface, or missing device are reported but do not terminate execution.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="home-manager/services/neverssl-keepalive/keepalive.sh">
<violation number="1" location="home-manager/services/neverssl-keepalive/keepalive.sh:22">
P2: Unprotected `networksetup -setairportpower` calls can fail under the user LaunchAgent, and with `set -euo pipefail` will abort the script, breaking keepalive behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Restart WiFi interface when connectivity is lost on captive portals - Increases WiFi restart delay to 3 seconds for reliability - Supports both STARBUCKS and Komeda_Wi-Fi networks - No notifications - uses native macOS captive portal popup - Update shell tests to reflect new WiFi restart behavior
8d7345c to
4aa5de1
Compare
Changes Made
Technical Details
home-manager/services/neverssl-keepalive/keepalive.shnetworksetup -setairportpower en0 offnetworksetup -setairportpower en0 onTesting
🤖 Generated with pi
Summary by cubic
Triggers the native macOS captive portal popup for Starbucks and Komeda_Wi‑Fi by restarting Wi‑Fi when connectivity fails. Improves reliability with a 3s delay and removes custom notifications.
Written for commit 4aa5de1. Summary will update on new commits.