feat(keepalive): add Starbucks WiFi captive portal detection for macOS - #436
Conversation
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds macOS-specific functionality to detect Starbucks WiFi networks and automatically trigger the captive portal if connectivity fails via ping. Includes corresponding test suite covering OSTYPE detection, SSID identification, and portal-opening behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
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 enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
Mesa DescriptionTL;DRAdded macOS-specific Starbucks WiFi captive portal detection to the 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 introduces a helpful feature for handling Starbucks captive portals on macOS. The overall approach is good, but I've found a few issues in the implementation. There's a critical bug in the ping command due to platform differences in timeout flags, which would cause it to fail incorrectly. Additionally, the script uses a hardcoded network interface en0, which isn't guaranteed to be the Wi-Fi interface, and the SSID matching pattern is incorrect. I've also left a comment on the new tests, suggesting a more robust approach using mocks to align with the existing test patterns in the project. My suggestions aim to fix these issues and improve the robustness of the new functionality.
|
|
||
| # Check for Starbucks networks (e.g., at_STARBUCKS_Wi2) | ||
| if [[ $SSID == *"STARBUCKS"* ]]; then | ||
| if ! ping -c 1 -W 2 1.1.1.1 >/dev/null 2>&1; then |
There was a problem hiding this comment.
The ping command's -W flag behaves differently on macOS compared to Linux. On macOS, it specifies the timeout in milliseconds, not seconds. A timeout of 2 milliseconds is far too short and will likely cause the ping to fail even with a good connection, leading to the captive portal opening unnecessarily on every script execution.
You should use the -t flag for a timeout in seconds on macOS, or increase the value for -W to 2000 for a 2-second timeout.
| if ! ping -c 1 -W 2 1.1.1.1 >/dev/null 2>&1; then | |
| if ! ping -c 1 -t 2 1.1.1.1 >/dev/null 2>&1; then |
|
|
||
| # macOS-specific: Open captive portal for Starbucks WiFi if 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 Wi-Fi network interface is hardcoded as en0. While this is common on many Macs, it's not guaranteed. Other interfaces like en1 could be used, especially on older hardware or systems with multiple network adapters. To make this script more robust, you should dynamically determine the Wi-Fi interface device name.
The suggestion below also refactors the awk command to use -v FS=... which is a bit cleaner and avoids potential shell quoting issues.
| SSID=$(networksetup -getairportnetwork en0 2>/dev/null | awk -F": " '{print $2}' || echo "") | |
| SSID=$(networksetup -getairportnetwork "$(networksetup -listallhardwareports | awk '/Hardware Port: (Wi-Fi|AirPort)/{getline; print $2}')" 2>/dev/null | awk -v FS=": " '{print $2}' || echo "") |
| 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 |
There was a problem hiding this comment.
The pattern *"STARBUCKS"* includes literal double quotes, so it would only match an SSID that contains "STARBUCKS" including the quotes, which is unlikely. The quotes should be removed from the pattern.
Additionally, the comparison is case-sensitive. A case-insensitive match would be more robust and catch variations like Starbucks or starbucks.
The suggested change addresses both issues.
| if [[ $SSID == *"STARBUCKS"* ]]; then | |
| if [[ ${SSID,,} == *starbucks* ]]; then |
| Describe 'Starbucks WiFi detection (macOS)' | ||
| It 'checks for macOS via OSTYPE' | ||
| When run bash -c "cat '$SCRIPT'" | ||
| The output should include 'OSTYPE' | ||
| The output should include 'darwin' | ||
| End | ||
|
|
||
| It 'uses networksetup to get SSID' | ||
| When run bash -c "cat '$SCRIPT'" | ||
| The output should include 'networksetup -getairportnetwork' | ||
| End | ||
|
|
||
| It 'checks for STARBUCKS SSID pattern' | ||
| When run bash -c "cat '$SCRIPT'" | ||
| The output should include '*"STARBUCKS"*' | ||
| End | ||
|
|
||
| It 'opens captive portal when connectivity fails' | ||
| When run bash -c "cat '$SCRIPT'" | ||
| The output should include 'captive.apple.com' | ||
| End | ||
| End |
There was a problem hiding this comment.
The new tests for Starbucks WiFi detection only check the script's source code for specific strings. This makes them brittle and tightly coupled to implementation details. For example, if the string matching for "STARBUCKS" is improved (e.g., to be case-insensitive as I suggested elsewhere), these tests would fail.
The existing tests in this file for curl behavior use mocking (mock_bin_setup) to test the script's actual behavior under different conditions. It would be better to follow that pattern for the new tests as well.
For example, you could:
- Mock
networksetupto return a Starbucks SSID. - Mock
pingto fail. - Assert that
open "http://captive.apple.com"is called.
And another test where ping succeeds, and assert that open is not called. This would provide much more confidence in the correctness of the logic.
![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 2 files
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:16">
P1: On macOS, `ping -W` expects time in **milliseconds**, not seconds. `-W 2` sets a 2ms timeout, which is too short for any realistic network response and will almost always fail, causing the captive portal to open unnecessarily even when connectivity is working.
Use `-W 2000` for a 2-second timeout, or use `-t 2` which specifies timeout in seconds.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
|
|
||
| # Check for Starbucks networks (e.g., at_STARBUCKS_Wi2) | ||
| if [[ $SSID == *"STARBUCKS"* ]]; then | ||
| if ! ping -c 1 -W 2 1.1.1.1 >/dev/null 2>&1; then |
There was a problem hiding this comment.
P1: On macOS, ping -W expects time in milliseconds, not seconds. -W 2 sets a 2ms timeout, which is too short for any realistic network response and will almost always fail, causing the captive portal to open unnecessarily even when connectivity is working.
Use -W 2000 for a 2-second timeout, or use -t 2 which specifies timeout in seconds.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/services/neverssl-keepalive/keepalive.sh, line 16:
<comment>On macOS, `ping -W` expects time in **milliseconds**, not seconds. `-W 2` sets a 2ms timeout, which is too short for any realistic network response and will almost always fail, causing the captive portal to open unnecessarily even when connectivity is working.
Use `-W 2000` for a 2-second timeout, or use `-t 2` which specifies timeout in seconds.</comment>
<file context>
@@ -6,3 +6,15 @@ set -euo pipefail
+
+ # Check for Starbucks networks (e.g., at_STARBUCKS_Wi2)
+ if [[ $SSID == *"STARBUCKS"* ]]; 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
+ fi
</file context>
| if ! ping -c 1 -W 2 1.1.1.1 >/dev/null 2>&1; then | |
| if ! ping -c 1 -W 2000 1.1.1.1 >/dev/null 2>&1; then |
![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
🧹 Nitpick comments (2)
home-manager/services/neverssl-keepalive/keepalive.sh (1)
15-15: Quote the SSID variable for safety.The
$SSIDvariable should be quoted to handle empty values safely. While the current code works due to the[[...]]construct, quoting is a shell scripting best practice that prevents potential issues.🔎 Proposed fix
- if [[ $SSID == *"STARBUCKS"* ]]; then + if [[ "$SSID" == *"STARBUCKS"* ]]; thenspec/keepalive_spec.sh (1)
86-107: Improve test coverage with behavioral tests.The current tests only verify that certain strings exist in the script file. They don't test actual behavior or execution paths, and would pass even if the logic were completely broken or commented out.
Consider adding behavioral tests that:
- Mock the macOS environment (
OSTYPE,networksetup,ping,open)- Test the actual control flow (e.g., captive portal opens only on Starbucks WiFi when connectivity fails)
- Verify commands are called with correct arguments in different scenarios
Example behavioral test structure
Describe 'Starbucks WiFi detection behavior (macOS)' setup() { MOCK_BIN=$(mktemp -d) export PATH="$MOCK_BIN:$PATH" export OSTYPE="darwin22.0" # Mock networksetup to return Starbucks SSID cat >"$MOCK_BIN/networksetup" <<'EOF' #!/usr/bin/env bash echo "Current Wi-Fi Network: at_STARBUCKS_Wi2" EOF chmod +x "$MOCK_BIN/networksetup" # Mock ping to fail (no connectivity) cat >"$MOCK_BIN/ping" <<'EOF' #!/usr/bin/env bash exit 1 EOF chmod +x "$MOCK_BIN/ping" # Mock open command to log calls cat >"$MOCK_BIN/open" <<'EOF' #!/usr/bin/env bash echo "open $@" >> /tmp/open_log EOF chmod +x "$MOCK_BIN/open" } cleanup() { rm -rf "$MOCK_BIN" rm -f /tmp/open_log } Before 'setup' After 'cleanup' It 'opens captive portal on Starbucks WiFi when connectivity fails' When run bash "$SCRIPT" The contents of file /tmp/open_log should include 'captive.apple.com' The status should be success End End
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
home-manager/services/neverssl-keepalive/keepalive.sh(1 hunks)spec/keepalive_spec.sh(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{sh,bash}
📄 CodeRabbit inference engine (CLAUDE.md)
Use shfmt with 2-space indentation for shell scripts
**/*.{sh,bash}: Use 2 spaces for indentation in shell scripts
Add proper shebang lines to shell scripts
Follow shellcheck recommendations in shell scripts
Document complex commands in shell scripts
Use consistent variable naming in shell scripts
Files:
home-manager/services/neverssl-keepalive/keepalive.shspec/keepalive_spec.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Agent
- GitHub Check: cubic · AI code reviewer
- GitHub Check: shellcheck
- GitHub Check: nix-linux
- GitHub Check: docker-build-push (linux/arm64, arm64, ubuntu-24.04-arm)
- GitHub Check: e2e-run (NixOS, ubuntu-latest)
- GitHub Check: shellspec
- GitHub Check: e2e-run (MacOS, macos-latest)
- GitHub Check: docker-build-push (linux/amd64, amd64, ubuntu-latest)
- GitHub Check: nix-darwin
- GitHub Check: e2e-run (Ubuntu, ubuntu-latest)
- GitHub Check: nix-nixos
- GitHub Check: lua-hammerspoon
- GitHub Check: lua-neovim
- GitHub Check: lua-neovim-test
|
|
||
| # macOS-specific: Open captive portal for Starbucks WiFi if 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.
Hardcode the WiFi interface name or detect it dynamically.
The hardcoded en0 interface may not work on all macOS systems; the device name could be airport, en0, en1, etc, depending on the Mac hardware and the version of OS X. Consider detecting the WiFi interface dynamically using networksetup -listallhardwareports or making it configurable.
🤖 Prompt for AI Agents
In home-manager/services/neverssl-keepalive/keepalive.sh around line 12 the WiFi
interface is hardcoded as en0 which fails on machines where the WiFi device uses
a different name; change the script to determine the active WiFi interface
dynamically or make it configurable: run networksetup -listallhardwareports and
parse the "Wi-Fi"/"AirPort"/"WiFi" Hardware Port entry to get the Device value
(fall back to common names like en0/en1), assign that to a variable (e.g.
WIFI_IFACE) and use it instead of en0, and allow overriding via an environment
variable or config option so the script works across macOS variants.
There was a problem hiding this comment.
Pull request overview
This PR adds macOS-specific functionality to automatically detect and handle Starbucks WiFi captive portals by checking the network SSID and opening Apple's captive portal page when connectivity is lost.
Key Changes:
- Added macOS detection using
OSTYPEenvironment variable - Implemented SSID retrieval via
networksetupcommand to identify Starbucks networks - Integrated connectivity checking with fallback to open captive portal on Starbucks WiFi
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| home-manager/services/neverssl-keepalive/keepalive.sh | Added macOS-specific logic to detect Starbucks WiFi networks and automatically open captive portal when connectivity fails |
| spec/keepalive_spec.sh | Added four test cases to verify the presence of Starbucks WiFi detection code in the script |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # macOS-specific: Open captive portal for Starbucks WiFi if 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 network interface name is hardcoded to en0, but macOS systems may have different primary WiFi interface names (e.g., en1, en2) depending on hardware configuration. Consider making this more robust by detecting the active WiFi interface dynamically, or document this assumption if en0 is guaranteed to be the WiFi interface in your target environment.
| SSID=$(networksetup -getairportnetwork en0 2>/dev/null | awk -F": " '{print $2}' || echo "") | |
| WIFI_DEVICE=$(networksetup -listallhardwareports 2>/dev/null | awk '/Wi-Fi/{getline; if ($1=="Device:") {print $2; exit}}') | |
| : "${WIFI_DEVICE:=en0}" | |
| SSID=$(networksetup -getairportnetwork "$WIFI_DEVICE" 2>/dev/null | awk -F": " '{print $2}' || echo "") |
| It 'checks for macOS via OSTYPE' | ||
| When run bash -c "cat '$SCRIPT'" | ||
| The output should include 'OSTYPE' | ||
| The output should include 'darwin' | ||
| End | ||
|
|
||
| It 'uses networksetup to get SSID' | ||
| When run bash -c "cat '$SCRIPT'" | ||
| The output should include 'networksetup -getairportnetwork' | ||
| End | ||
|
|
||
| It 'checks for STARBUCKS SSID pattern' | ||
| When run bash -c "cat '$SCRIPT'" | ||
| The output should include '*"STARBUCKS"*' | ||
| End | ||
|
|
||
| It 'opens captive portal when connectivity fails' | ||
| When run bash -c "cat '$SCRIPT'" | ||
| The output should include 'captive.apple.com' | ||
| End |
There was a problem hiding this comment.
These tests only verify that certain strings exist in the script source code, but they don't validate the actual behavior or logic flow. Consider adding behavioral tests that mock the commands (networksetup, ping, open) and verify the script executes correctly under different conditions, similar to the existing "curl behavior" and "error handling" test suites. For example, test scenarios where: (1) SSID contains "STARBUCKS" and ping fails, (2) SSID doesn't contain "STARBUCKS", (3) SSID contains "STARBUCKS" but ping succeeds, and (4) networksetup command fails.
Summary
Changes
Technical Details
networksetup -getairportnetwork en0to get SSID on macOSping -c 1 -W 2 1.1.1.1open "http://captive.apple.com"|| trueSummary by cubic
Adds macOS Starbucks WiFi detection to neverssl-keepalive. On Starbucks networks, if connectivity fails, it auto-opens the captive portal to restore access.
Written for commit 4304368. Summary will update automatically on new commits.