Add Quick Settings Tile service for VPN control#136
Open
Dan-Kingsley wants to merge 7 commits intonetbirdio:mainfrom
Open
Add Quick Settings Tile service for VPN control#136Dan-Kingsley wants to merge 7 commits intonetbirdio:mainfrom
Dan-Kingsley wants to merge 7 commits intonetbirdio:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a Quick Settings TileService (NetbirdTileService) and manifest entry; TileService binds to VPNService via a local binder, handles clicks and VPN permission flow, updates tile state, and VPNService exposes Changes
Sequence DiagramsequenceDiagram
actor User
participant QS as Quick Settings
participant Tile as NetbirdTileService
participant VPN as VPNService (Binder)
participant Engine as EngineRunner
participant UI as MainActivity
User->>QS: Tap Netbird tile
QS->>Tile: onClick()
alt Tile not bound
Tile->>VPN: bindService()
VPN-->>Tile: onServiceConnected (binder)
Tile->>VPN: register ServiceStateListener
end
Tile->>VPN: binder.isRunning()
VPN->>Engine: engineRunner.isRunning()
Engine-->>VPN: running? (true/false)
alt running == true
Tile->>VPN: binder.stopEngine()
VPN->>Engine: stop()
else running == false
Tile->>Tile: VpnService.prepare() -> intent?
alt permission required
Tile->>UI: startActivityAndCollapse / PendingIntent to request permission
else permission granted
Tile->>VPN: startForegroundService(INTENT_ACTION_START) + bind
VPN->>Engine: start()
end
end
Tile->>Tile: updateTile() (STATE_ACTIVE / STATE_INACTIVE)
Tile->>QS: reflect tile state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Author
|
|
![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 `@app/src/main/java/io/netbird/client/NetbirdTileService.java`:
- Around line 104-118: bindToVpnService currently starts the bind but you never
track a pending bind, so if onStopListening/unbindFromVpnService runs before
onServiceConnected the ServiceConnection remains; add a "bindingInProgress"
boolean set true in bindToVpnService, clear it in
serviceConnection.onServiceConnected and onServiceDisconnected, and modify
unbindFromVpnService to unbind when either isBound or bindingInProgress is true
(removing the serviceStateListener only if mBinder != null), then call
unbindService(serviceConnection) inside a try/catch to safely handle cases where
the system rejected the unbind, and reset isBound, bindingInProgress, and
mBinder appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/src/main/java/io/netbird/client/NetbirdTileService.java`:
- Around line 72-77: Ensure pendingClick cannot persist across failed/aborted
binds: in NetbirdTileService.onStopListening() clear/reset the pendingClick flag
immediately (and any related state) so a subsequent successful bind won't act on
an old click; additionally, in the bind flow (the methods handling bindService
success/failure—e.g., your ServiceConnection callbacks and the code paths around
bindService / onServiceConnected / onServiceDisconnected used in the bind logic
between lines ~107–124) handle bind failures by clearing pendingClick and
aborting any pending toggle actions. Make sure both unbindFromVpnService() and
any bind-failure/error branches consistently reset pendingClick to a safe
default.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/src/main/java/io/netbird/client/NetbirdTileService.java`:
- Around line 94-105: The toggle handler should avoid calling
startAndRunVpnService() when a bind is already in progress; update
handleToggle() to check the isBinding flag (or equivalent) before initiating the
service: if mBinder is null and isBinding is true, just set pendingClick = true
and return without calling startAndRunVpnService(), otherwise (when not binding)
set pendingClick and call startAndRunVpnService(); reference the handleToggle
method, mBinder, isBinding, pendingClick and startAndRunVpnService to locate and
implement this guard.
- Around line 84-88: NetbirdTileService currently calls
startActivityAndCollapse(Intent) which is deprecated and throws on Android 14+;
replace that call by building a PendingIntent for MainActivity (use the same
Intent with FLAG_ACTIVITY_NEW_TASK | FLAG_ACTIVITY_CLEAR_TOP), create the
PendingIntent with FLAG_UPDATE_CURRENT | FLAG_IMMUTABLE, and pass that
PendingIntent to startActivityAndCollapse(PendingIntent) instead of the raw
Intent so it works on SDK 35+ and avoids UnsupportedOperationException.
🧹 Nitpick comments (1)
app/src/main/java/io/netbird/client/NetbirdTileService.java (1)
39-45: Consider removing the service state listener inonServiceDisconnected.When the service disconnects unexpectedly (e.g., process killed),
mBinderis set to null without removing theserviceStateListener. While the listener reference will become stale, explicitly removing it before nulling the binder ensures cleaner lifecycle management and mirrors the pattern inunbindFromVpnService().♻️ Suggested improvement
`@Override` public void onServiceDisconnected(ComponentName name) { + if (mBinder != null) { + mBinder.removeServiceStateListener(serviceStateListener); + } mBinder = null; isBound = false; isBinding = false; updateTile(); }
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Collaborator
|
Hello @Dan-Kingsley! Thank you for the PR. It looks nice! I’ll review it soon and merge it. |
pappz
requested changes
Mar 22, 2026
| private void startAndRunVpnService() { | ||
| Intent intent = new Intent(this, VPNService.class); | ||
| intent.setAction(VPNService.INTENT_ACTION_START); | ||
| startService(intent); |
Collaborator
There was a problem hiding this comment.
startService() can throw IllegalStateException. Please use startForegroundService() and handle the INTENT_ACTION_START action in VPNService.onStartCommand() to call startForeground() immediately.
switch (action) {
case INTENT_ALWAYS_ON_START:
fgNotification.startForeground();
engineRunner.runWithoutAuth();
break;
case INTENT_ACTION_START:
fgNotification.startForeground();
break;
}
| this, 0, intent, | ||
| PendingIntent.FLAG_UPDATE_CURRENT | PendingIntent.FLAG_IMMUTABLE | ||
| ); | ||
| startActivityAndCollapse(pendingIntent); |
Collaborator
There was a problem hiding this comment.
startActivityAndCollapse(PendingIntent) requires API 34+, but minSdk is 26. Needs a version check to fall back to the deprecated startActivityAndCollapse(Intent) on older
devices.
…h for Android 14+.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/io/netbird/client/NetbirdTileService.java`:
- Around line 50-65: The callbacks in the anonymous ServiceStateListener
(onStarted, onStopped, onError) are executed from
EngineRunner.notifyServiceStateListeners' background thread but call
updateTile() which touches getQsTile() and tile.updateTile() and must run on the
main thread; change each callback to post a Runnable to the main thread (e.g.
via a Handler created with Looper.getMainLooper()) that invokes updateTile() so
all UI/tile work runs on the main thread instead of directly calling
updateTile() from the background thread.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b4e35dd-1080-409d-addb-c233c0a83a67
📒 Files selected for processing (2)
app/src/main/java/io/netbird/client/NetbirdTileService.javatool/src/main/java/io/netbird/client/tool/VPNService.java
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/io/netbird/client/NetbirdTileService.java (1)
122-124: Avoid auto-creatingVPNServicejust to paint the tile.
BIND_AUTO_CREATEhere instantiatesVPNServiceevery time Quick Settings starts listening, andVPNService.onCreate()does engine setup plus callback/receiver registration. A lightweight persisted state, or a non-auto-create bind that treats “service missing” as inactive, would keep the tile much cheaper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/io/netbird/client/NetbirdTileService.java` around lines 122 - 124, The tile currently forces creation of VPNService via bindService(intent, serviceConnection, Context.BIND_AUTO_CREATE) in bindToVpnService(), causing heavy onCreate work; change to bind without auto-create (use 0 instead of Context.BIND_AUTO_CREATE) so bindService only succeeds if VPNService is already running, set isBinding based on the boolean result, and treat a failed bind or null service in serviceConnection as “inactive” (update the tile state and avoid starting or creating the service). Keep serviceConnection and isBinding handling intact, and if you need persisted lightweight state use a simple SharedPreferences flag or existing persisted state instead of auto-creating VPNService.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/io/netbird/client/NetbirdTileService.java`:
- Around line 107-119: handleToggle currently calls mBinder.runEngine() on a
service that may be only bound (which fails on Android 12+); ensure the service
is explicitly started before invoking runEngine: when mBinder != null and
!mBinder.isRunning(), call the existing
startForegroundService/startAndRunVpnService (or a
startService/startForegroundService equivalent) to start the VPN service first,
then call mBinder.runEngine(null, false); similarly ensure any code path that
directly calls mBinder.runEngine uses startForegroundService prior to runEngine
so the service is both started and bound (refer to
NetbirdTileService.handleToggle, mBinder.runEngine and startAndRunVpnService).
---
Nitpick comments:
In `@app/src/main/java/io/netbird/client/NetbirdTileService.java`:
- Around line 122-124: The tile currently forces creation of VPNService via
bindService(intent, serviceConnection, Context.BIND_AUTO_CREATE) in
bindToVpnService(), causing heavy onCreate work; change to bind without
auto-create (use 0 instead of Context.BIND_AUTO_CREATE) so bindService only
succeeds if VPNService is already running, set isBinding based on the boolean
result, and treat a failed bind or null service in serviceConnection as
“inactive” (update the tile state and avoid starting or creating the service).
Keep serviceConnection and isBinding handling intact, and if you need persisted
lightweight state use a simple SharedPreferences flag or existing persisted
state instead of auto-creating VPNService.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64a04621-2058-42e2-9a71-a73c87d60634
📒 Files selected for processing (2)
app/src/main/java/io/netbird/client/NetbirdTileService.javatool/src/main/java/io/netbird/client/tool/VPNService.java
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/io/netbird/client/NetbirdTileService.java (1)
166-174: Optional: avoid redundant tile updates when state is unchanged.This can reduce unnecessary
tile.updateTile()calls during repeated callbacks with the same running state.♻️ Proposed micro-refactor
private void updateTile() { Tile tile = getQsTile(); if (tile == null) return; - boolean running = isVpnRunning(); - - tile.setState(running ? Tile.STATE_ACTIVE : Tile.STATE_INACTIVE); - tile.updateTile(); + int newState = isVpnRunning() ? Tile.STATE_ACTIVE : Tile.STATE_INACTIVE; + if (tile.getState() != newState) { + tile.setState(newState); + tile.updateTile(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/io/netbird/client/NetbirdTileService.java` around lines 166 - 174, The updateTile() method currently always calls tile.updateTile(); change it to only call tile.updateTile() when the state actually changes: retrieve the current Tile via getQsTile(), compute the desired state from isVpnRunning(), compare against the tile.getState() (or stored state) and only call tile.setState(...) followed by tile.updateTile() when they differ; reference functions: updateTile(), getQsTile(), isVpnRunning(), tile.getState(), tile.setState(), tile.updateTile().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/io/netbird/client/NetbirdTileService.java`:
- Around line 166-174: The updateTile() method currently always calls
tile.updateTile(); change it to only call tile.updateTile() when the state
actually changes: retrieve the current Tile via getQsTile(), compute the desired
state from isVpnRunning(), compare against the tile.getState() (or stored state)
and only call tile.setState(...) followed by tile.updateTile() when they differ;
reference functions: updateTile(), getQsTile(), isVpnRunning(), tile.getState(),
tile.setState(), tile.updateTile().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 786b25d8-5fcc-41e5-bada-b9e80938c0f7
📒 Files selected for processing (1)
app/src/main/java/io/netbird/client/NetbirdTileService.java
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
2 participants
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.
I have been using NetBird for some time now, but I find it annoying to need to into the app to turn the VPN connection on. I have created a Icon and button to exist in the Quick Settings notification panel. so that this can be easily accessed without needing to go into the NetBird app
Summary by CodeRabbit
New Features
Localization
Bug Fixes / Reliability