Home screen widget#172
Open
NoxelFoxel wants to merge 4 commits intonetbirdio:mainfrom
Open
Conversation
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new home-screen widget: manifest registration, AppWidgetProvider and updater, widget UI/layout/drawables/strings, Preferences persistence, VPNService widget intent handling and polling, notification refactor, and supporting profile validation. Changes
Sequence DiagramsequenceDiagram
actor User
participant Widget as Widget (AppWidgetProvider)
participant VPNService as VPNService
participant Preferences as Preferences
participant Engine as Engine
User->>Widget: Tap connection switch
Widget->>VPNService: PendingIntent ACTION_WIDGET_TOGGLE_CONNECTION
VPNService->>Preferences: read state / setWidgetState(...)
VPNService->>Engine: start/stop (may call VpnService.prepare)
Engine-->>VPNService: state change / routes
VPNService->>Preferences: setWidgetState(...), setLastExitNodeRoute(...)
VPNService->>Widget: send ACTION_WIDGET_REFRESH broadcast
Widget->>Preferences: read widget state
Widget->>User: update RemoteViews UI
User->>Widget: Tap exit-node switch
Widget->>VPNService: PendingIntent ACTION_WIDGET_TOGGLE_EXIT_NODE
VPNService->>Engine: ensure running, retry fetch routes
Engine-->>VPNService: routes available / selection changed
VPNService->>Preferences: setWidgetState(...), setLastExitNodeRoute(...)
VPNService->>Widget: send ACTION_WIDGET_REFRESH broadcast
Widget->>Preferences: read exit-node state
Widget->>User: update exit-node UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
app/src/main/res/values/strings.xml (1)
13-13: Consider reusingmain_status_disconnectedinstead of defining a duplicate.
widget_status_disconnectedhas the same English value as the pre-existingmain_status_disconnectedon Line 23. Two separate keys for the same concept will need to be translated and kept in sync independently. If the wording is intentionally meant to diverge across surfaces, please ignore; otherwise consider referencing the existing string from the widget layout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/res/values/strings.xml` at line 13, The file defines a duplicate string key widget_status_disconnected with the same text as main_status_disconnected; remove the duplicate and update all usages of widget_status_disconnected (e.g., in the widget layout or code) to reference the existing main_status_disconnected string instead (use `@string/main_status_disconnected`), unless the widget intentionally needs different wording—in that case keep widget_status_disconnected and document the divergence.app/src/main/java/io/netbird/client/NetbirdWidgetUpdater.java (1)
51-82: Optional: don’t wire the exit-node click when no exit node is available.When
exitNodeAvailableis false you still attachexitNodeIntentto all three exit-side views. A tap will start (or keep) the foreground service, forceengineRunner.runWithoutAuth(), and spin a background thread that polls for exit nodes for ~6s before giving up silently. Consider only wiring those click handlers when an exit node is actually known, or visibly indicate the disabled state to avoid confusing taps.♻️ Sketch
- views.setOnClickPendingIntent(R.id.widget_exit_switch, - exitNodeIntent); - views.setOnClickPendingIntent(R.id.widget_exit_icon, - exitNodeIntent); - views.setOnClickPendingIntent(R.id.widget_exit_status, - exitNodeIntent); + if (exitNodeAvailable) { + views.setOnClickPendingIntent(R.id.widget_exit_switch, exitNodeIntent); + views.setOnClickPendingIntent(R.id.widget_exit_icon, exitNodeIntent); + views.setOnClickPendingIntent(R.id.widget_exit_status, exitNodeIntent); + }🤖 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/NetbirdWidgetUpdater.java` around lines 51 - 82, The exit-side click handlers are attached even when exitNodeAvailable is false; update NetbirdWidgetUpdater to only call servicePendingIntent(...) and views.setOnClickPendingIntent(...) for the exit-side (exitNodeIntent, widget_exit_switch, widget_exit_icon, widget_exit_status) when exitNodeAvailable is true (use the existing exitNodeAvailable boolean), otherwise skip wiring those handlers and optionally set the exit views to a disabled/visually inactive state (you can reuse exitColor logic and the setBackgroundResource call) so taps do not start the foreground service or trigger engineRunner.runWithoutAuth().
🤖 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/AndroidManifest.xml`:
- Around line 59-70: The receiver declaration for NetbirdWidgetProvider
currently exports a custom action "io.netbird.client.widget.action.REFRESH",
allowing any app to trigger widget refreshes; remove that action from the
exported intent-filter and instead have the component targeted explicitly from
your code or protect it with a signature permission. Update the AndroidManifest
receiver for NetbirdWidgetProvider to not advertise the REFRESH action (keep
APPWIDGET_UPDATE), and either (A) change the code that sends the refresh
broadcast (e.g., VPNService) to use an explicit ComponentName or
Intent.setClassName to target NetbirdWidgetProvider, or (B) add
android:permission="io.netbird.permission.NOTIFICATION" to the receiver so only
apps signed with your key can call it; choose one approach and apply it
consistently.
In `@app/src/main/java/io/netbird/client/ui/home/NetworksFragment.java`:
- Around line 142-158: The persisted LastExitNodeRoute isn't cleared when a user
toggles an exit node off; update routeSwitchToggleHandler so the else branch
(where model.deselectRoute(route) is called) also clears or updates the stored
preference. Specifically, in routeSwitchToggleHandler, after calling
model.deselectRoute(route) check whether the deselected route matches any
resource that isExitNode() and if so call new
Preferences(requireContext()).setLastExitNodeRoute("") or null to clear it (or,
alternatively, when selecting a different exit node ensure
rememberExitNodeSelection overwrites the previous value). Use the existing
methods rememberExitNodeSelection, model.deselectRoute,
Preferences.setLastExitNodeRoute and the resources/resource.isExitNode checks to
implement this behavior.
In `@app/src/main/res/drawable/widget_switch_on.xml`:
- Line 8: The on-state drawable file widget_switch_on.xml incorrectly uses
`@color/track_off_color` for its <solid> track color (copy-paste bug); update the
track color reference in widget_switch_on.xml from `@color/track_off_color` to
`@color/track_on_color` so the track appearance differs between the ON and OFF
drawables.
In `@app/src/main/res/layout/widget_netbird.xml`:
- Around line 48-54: Add meaningful contentDescription attributes to the empty
"switch" TextViews so screen readers announce them (e.g., for the connection
switch with id widget_connection_switch and the exit-node switch at the other
block), and update NetbirdWidgetUpdater.createRemoteViews to call
views.setContentDescription(viewId, description) to toggle the description based
on on/off state so TalkBack can announce current state.
In `@tool/src/main/java/io/netbird/client/tool/VPNService.java`:
- Around line 332-350: handleWidgetExitNodeToggle and scheduleWidgetStateRefresh
currently spawn unbounded new Thread(...) instances causing concurrent mutation
of engineRunner, prefs (setLastExitNodeRoute/setWidgetState) and repeated
broadcasts; replace those ad-hoc threads with a single-threaded ExecutorService
(e.g., widgetExecutor) or an AtomicBoolean "in-flight" guard to serialize
executions for the widget paths (invocations that call
toggleExitNodeWhenAvailable, deselectRoute/selectRoute, and prefs writes),
ensure scheduleWidgetStateRefresh uses the same serialized executor so polls
don't overlap, and shut down the executor in onDestroy()
(widgetExecutor.shutdownNow()) to avoid leaked threads.
- Around line 442-467: The method updateWidgetStateAndBroadcast writes
preferences in two separate transactions (preferences.setLastExitNodeRoute(...)
and preferences.setWidgetState(...)), allowing concurrent callers
(serviceStateListener.onStarted/onStopped/onError, RouteChangeListener, widget
toggle handlers, scheduleWidgetStateRefresh) to interleave and cause
inconsistent widget reads in NetbirdWidgetUpdater.createRemoteViews; fix by
replacing the two separate preference calls with a single atomic update: add a
new Preferences API like setWidgetStateAndLastExitNodeRoute(boolean isRunning,
boolean hasSelectedExitNode, String exitNodeName) that performs one
Editor.apply() (or commit) for both the route and widget flags, and call that
from updateWidgetStateAndBroadcast (or alternatively route all callers through
the same single-thread executor used by the toggle/refresh threads to serialize
invocations) so updates cannot interleave.
- Around line 352-374: The service must not start an Activity from the
background (openMainActivity/startActivity) because Android 12+ will block it;
instead modify ensureVpnPermissionFromWidget() to stop calling
openMainActivity() and to post/update a foreground/clickable notification (use
fgNotification/NotificationManager) that contains a PendingIntent created from
getPackageManager().getLaunchIntentForPackage(getPackageName()) so the user can
tap to grant VPN permission; remove or deprecate openMainActivity() usage in the
service and ensure engineRunner/stopSelf behavior remains the same after showing
the notification.
- Around line 317-330: handleWidgetConnectionToggle currently calls
engineRunner.runWithoutAuth() unconditionally which can throw when there's no
cached profile; before calling runWithoutAuth() check for an active/cached
profile via profileManager.getActiveProfile() (or a dedicated
profileManager.hasCachedProfile()/isAuthenticated() if available) and only call
engineRunner.runWithoutAuth() when a profile exists; if no profile is present,
do not attempt runWithoutAuth(), do not flip widget state, and instead launch
MainActivity (or send an intent to open the app) to prompt first-time
authentication; keep existing ensureVpnPermissionFromWidget() behavior and
updateWidgetStateAndBroadcast() only after a successful start attempt or when
stopping.
---
Nitpick comments:
In `@app/src/main/java/io/netbird/client/NetbirdWidgetUpdater.java`:
- Around line 51-82: The exit-side click handlers are attached even when
exitNodeAvailable is false; update NetbirdWidgetUpdater to only call
servicePendingIntent(...) and views.setOnClickPendingIntent(...) for the
exit-side (exitNodeIntent, widget_exit_switch, widget_exit_icon,
widget_exit_status) when exitNodeAvailable is true (use the existing
exitNodeAvailable boolean), otherwise skip wiring those handlers and optionally
set the exit views to a disabled/visually inactive state (you can reuse
exitColor logic and the setBackgroundResource call) so taps do not start the
foreground service or trigger engineRunner.runWithoutAuth().
In `@app/src/main/res/values/strings.xml`:
- Line 13: The file defines a duplicate string key widget_status_disconnected
with the same text as main_status_disconnected; remove the duplicate and update
all usages of widget_status_disconnected (e.g., in the widget layout or code) to
reference the existing main_status_disconnected string instead (use
`@string/main_status_disconnected`), unless the widget intentionally needs
different wording—in that case keep widget_status_disconnected and document the
divergence.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a469a81-0380-4420-8a96-415ba6846f91
📒 Files selected for processing (12)
app/src/main/AndroidManifest.xmlapp/src/main/java/io/netbird/client/NetbirdWidgetProvider.javaapp/src/main/java/io/netbird/client/NetbirdWidgetUpdater.javaapp/src/main/java/io/netbird/client/ui/home/NetworksFragment.javaapp/src/main/res/drawable/widget_background.xmlapp/src/main/res/drawable/widget_switch_off.xmlapp/src/main/res/drawable/widget_switch_on.xmlapp/src/main/res/layout/widget_netbird.xmlapp/src/main/res/values/strings.xmlapp/src/main/res/xml/netbird_widget_info.xmltool/src/main/java/io/netbird/client/tool/Preferences.javatool/src/main/java/io/netbird/client/tool/VPNService.java
Comment on lines
+48
to
+54
| <TextView | ||
| android:id="@+id/widget_connection_switch" | ||
| android:layout_width="52dp" | ||
| android:layout_height="32dp" | ||
| android:layout_marginStart="12dp" | ||
| android:background="@drawable/widget_switch_off" | ||
| android:text="" /> |
Contributor
There was a problem hiding this comment.
Add contentDescription to the “switch” TextViews for accessibility.
The connection and exit-node "switches" are clickable TextViews with empty text and only a background drawable, so screen readers have nothing to announce for these touch targets. Setting a meaningful contentDescription (and updating it from NetbirdWidgetUpdater based on the on/off state) makes the widget usable with TalkBack.
♿ Suggested layout change
<TextView
android:id="@+id/widget_connection_switch"
android:layout_width="52dp"
android:layout_height="32dp"
android:layout_marginStart="12dp"
android:background="@drawable/widget_switch_off"
+ android:contentDescription="@string/widget_status_disconnected"
android:text="" /> <TextView
android:id="@+id/widget_exit_switch"
android:layout_width="52dp"
android:layout_height="32dp"
android:layout_marginStart="12dp"
android:background="@drawable/widget_switch_off"
+ android:contentDescription="@string/widget_exit_node_unavailable"
android:text="" />You can then keep these descriptions in sync from NetbirdWidgetUpdater.createRemoteViews(...) via views.setContentDescription(...).
Also applies to: 93-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/res/layout/widget_netbird.xml` around lines 48 - 54, Add
meaningful contentDescription attributes to the empty "switch" TextViews so
screen readers announce them (e.g., for the connection switch with id
widget_connection_switch and the exit-node switch at the other block), and
update NetbirdWidgetUpdater.createRemoteViews to call
views.setContentDescription(viewId, description) to toggle the description based
on on/off state so TalkBack can announce current state.
Collaborator
|
Hello @NoxelFoxel, thank you for the PR! Could you review the CodeRabbit feedback? |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tool/src/main/java/io/netbird/client/tool/ForegroundNotification.java (1)
39-40: Channel creation on everybuildNotificationcall.
ensureNotificationChannel()is invoked on every notification build, which re-instantiates aNotificationChanneland callscreateNotificationChannelevery time. The platform call is idempotent so it's safe, but it's wasted work on eachshowNotification/startForeground. Consider running this once from the constructor (or lazily with a boolean guard) since the channel parameters are static.♻️ Suggested refactor
public ForegroundNotification(android.net.VpnService vpnService) { this.service = vpnService; + ensureNotificationChannel(); } @@ private Notification buildNotification(String text, boolean ongoing) { - ensureNotificationChannel(); - NotificationCompat.Builder builder = new NotificationCompat.Builder(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tool/src/main/java/io/netbird/client/tool/ForegroundNotification.java` around lines 39 - 40, ensureNotificationChannel() is being called on every buildNotification(String text, boolean ongoing) which reconstructs the NotificationChannel repeatedly; move the channel creation to run once (either call ensureNotificationChannel() from the ForegroundNotification constructor or add a lazy boolean guard field like channelCreated and set it inside ensureNotificationChannel to avoid re-creating) so buildNotification, showNotification, and startForeground no longer perform redundant channel creation while preserving the idempotent createNotificationChannel call.tool/src/main/java/io/netbird/client/tool/VPNService.java (1)
476-506: Avoid holding the monitor during the engine query.
updateWidgetStateAndBroadcastis invoked from many sites (state listener, route change listener, both toggle handlers, scheduled refresh). Holding the intrinsic lock acrossengineRunner.networks()(viagetExitNodes()on line 487) means a slow gomobile call serializes every widget update path — including those triggered from the engine's own callback threads. The atomic prefs write you added (good fix) is the only piece that strictly needs serialization.Optional refactor: gather engine state outside the synchronized region, then guard only the prefs-write + broadcast.
♻️ Sketch
- private synchronized void updateWidgetStateAndBroadcast() { + private void updateWidgetStateAndBroadcast() { if (preferences == null || engineRunner == null) { return; } boolean isRunning = engineRunner.isRunning(); boolean hasSelectedExitNode = false; String lastExitNodeRoute = preferences.getLastExitNodeRoute(); String exitNodeName = lastExitNodeRoute; if (isRunning) { for (ExitNode exitNode : getExitNodes()) { if (exitNode.selected) { hasSelectedExitNode = true; exitNodeName = exitNode.name; lastExitNodeRoute = exitNode.name; break; } } } - preferences.setWidgetStateAndLastExitNodeRoute( - lastExitNodeRoute, - isRunning, - hasSelectedExitNode, - exitNodeName); - - Intent refreshIntent = new Intent(ACTION_WIDGET_REFRESH); - refreshIntent.setClassName(getPackageName(), WIDGET_PROVIDER_CLASS_NAME); - sendBroadcast(refreshIntent); + synchronized (this) { + preferences.setWidgetStateAndLastExitNodeRoute( + lastExitNodeRoute, + isRunning, + hasSelectedExitNode, + exitNodeName); + + Intent refreshIntent = new Intent(ACTION_WIDGET_REFRESH); + refreshIntent.setClassName(getPackageName(), WIDGET_PROVIDER_CLASS_NAME); + sendBroadcast(refreshIntent); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tool/src/main/java/io/netbird/client/tool/VPNService.java` around lines 476 - 506, updateWidgetStateAndBroadcast currently holds the object's monitor while calling into engine/IO via getExitNodes()/engineRunner (which may block), causing unnecessary serialization; refactor so you first read the volatile/readonly state from engineRunner (isRunning and exit node selection info via getExitNodes()) outside the synchronized block, then enter a small synchronized(this) region only to call preferences.setWidgetStateAndLastExitNodeRoute(...) and sendBroadcast(refreshIntent) (keep Intent creation local to the synchronized region if it must be protected), ensuring the heavy engineRunner.getExitNodes()/isRunning calls are moved before synchronization to avoid blocking other callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tool/src/main/java/io/netbird/client/tool/ForegroundNotification.java`:
- Around line 35-37: showNotification currently reuses the foreground
notification ID (NOTIFICATION_ID) which can replace an ongoing foreground
notification; change showNotification to use a distinct transient ID (e.g.
PROMPT_NOTIFICATION_ID) when calling getNotificationManager().notify(...) so it
doesn't overwrite the startForeground notification, and update references in
buildNotification if needed (leave startForeground and NOTIFICATION_ID for the
ongoing service notification); alternatively, if you prefer the precondition
approach, document in VPNService.promptUserToOpenApp that stopForeground() must
be called before invoking ForegroundNotification.showNotification.
- Around line 72-79: The fallback in createLaunchAppPendingIntent builds a
MainActivity FQCN using service.getPackageName() which can include build-time
suffixes and may not resolve; instead, replace that blind setClassName approach
by verifying the constructed Intent resolves (use
PackageManager.queryIntentActivities/resolveActivity) and if it does not, use a
known hardcoded FQCN (e.g., "io.netbird.client.MainActivity") or construct a
launch Intent via ACTION_MAIN/LAUNCHER targeted to the package; update
createLaunchAppPendingIntent to perform the resolution check after creating the
fallback Intent and only use setClassName when it actually resolves.
In `@tool/src/main/java/io/netbird/client/tool/VPNService.java`:
- Around line 412-427: The current toggleExitNodeWhenAvailable loop
(toggleExitNodeWhenAvailable -> getExitNodes, governed by EXIT_NODE_RETRY_COUNT
and EXIT_NODE_RETRY_DELAY_MS) may expire too quickly on cold starts; either
increase the retry budget (e.g., multiply EXIT_NODE_RETRY_COUNT or align it with
WIDGET_STATE_REFRESH_COUNT) so EXIT_NODE_RETRY_COUNT * EXIT_NODE_RETRY_DELAY_MS
covers slower startups, or, preferably, remove polling and subscribe to the
network-ready signal (e.g., add a RouteChangeListener or networks-ready
callback), and in that listener call toggleExitNode(exitNodes) when
getExitNodes() returns non-empty, then unregister the listener; also ensure
widgetExitToggleInFlight is only cleared after a successful toggle or a
controlled timeout so the UI doesn’t silently reset.
- Around line 459-474: getExitNodes() assumes engineRunner.networks() never null
or throwing; wrap the call to engineRunner.networks() in a try-catch and
defensively handle a null return: if the call throws (or returns null) catch the
exception, log a warning/error, and return an empty List so
updateWidgetStateAndBroadcast() can continue and persist state; also check
networks == null before iterating and only loop when networks is non-null to
avoid NPEs.
- Around line 403-410: Add a runtime POST_NOTIFICATIONS permission check before
attempting to post notifications from promptUserToOpenApp (or inside
ForegroundNotification.showNotification): use ContextCompat.checkSelfPermission
for Manifest.permission.POST_NOTIFICATIONS on API >= 33 and, if not granted,
avoid calling fgNotification.showNotification() and instead provide fallback UX
(e.g., launch an activity/Toast/dialog to inform the user or trigger a
permission request flow); keep the existing stopSelf() call path unchanged when
engineRunner.isRunning() is false. Ensure the check references
POST_NOTIFICATIONS, ContextCompat.checkSelfPermission, promptUserToOpenApp, and
ForegroundNotification.showNotification so the change is easy to locate.
---
Nitpick comments:
In `@tool/src/main/java/io/netbird/client/tool/ForegroundNotification.java`:
- Around line 39-40: ensureNotificationChannel() is being called on every
buildNotification(String text, boolean ongoing) which reconstructs the
NotificationChannel repeatedly; move the channel creation to run once (either
call ensureNotificationChannel() from the ForegroundNotification constructor or
add a lazy boolean guard field like channelCreated and set it inside
ensureNotificationChannel to avoid re-creating) so buildNotification,
showNotification, and startForeground no longer perform redundant channel
creation while preserving the idempotent createNotificationChannel call.
In `@tool/src/main/java/io/netbird/client/tool/VPNService.java`:
- Around line 476-506: updateWidgetStateAndBroadcast currently holds the
object's monitor while calling into engine/IO via getExitNodes()/engineRunner
(which may block), causing unnecessary serialization; refactor so you first read
the volatile/readonly state from engineRunner (isRunning and exit node selection
info via getExitNodes()) outside the synchronized block, then enter a small
synchronized(this) region only to call
preferences.setWidgetStateAndLastExitNodeRoute(...) and
sendBroadcast(refreshIntent) (keep Intent creation local to the synchronized
region if it must be protected), ensuring the heavy
engineRunner.getExitNodes()/isRunning calls are moved before synchronization to
avoid blocking other callers.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e5fbe77-f8d8-4b7d-96ee-f9fe3806ee4c
📒 Files selected for processing (11)
app/src/main/AndroidManifest.xmlapp/src/main/java/io/netbird/client/NetbirdWidgetUpdater.javaapp/src/main/java/io/netbird/client/ui/home/NetworksFragment.javaapp/src/main/res/drawable/widget_switch_on.xmlapp/src/main/res/layout/widget_netbird.xmlapp/src/main/res/values/strings.xmltool/src/main/java/io/netbird/client/tool/ForegroundNotification.javatool/src/main/java/io/netbird/client/tool/Preferences.javatool/src/main/java/io/netbird/client/tool/ProfileManagerWrapper.javatool/src/main/java/io/netbird/client/tool/VPNService.javatool/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (5)
- tool/src/main/res/values/strings.xml
- app/src/main/res/values/strings.xml
- app/src/main/res/drawable/widget_switch_on.xml
- app/src/main/res/layout/widget_netbird.xml
- app/src/main/java/io/netbird/client/ui/home/NetworksFragment.java
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/AndroidManifest.xml
- app/src/main/java/io/netbird/client/NetbirdWidgetUpdater.java
- tool/src/main/java/io/netbird/client/tool/Preferences.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
tool/src/main/java/io/netbird/client/tool/ForegroundNotification.java (4)
60-66: Minor: using the application package name as the notification channel ID is unconventional.Channel IDs are normally a stable, descriptive constant (e.g.,
"netbird_foreground_service"), independent ofapplicationId. Tying it toservice.getPackageName()means build variants withapplicationIdSuffix(e.g.,.debug) get a different channel ID even though the channel name string resource is shared, which can fragment user channel settings if they have multiple variants installed. Consider extracting aprivate static final String CHANNEL_ID = "netbird_foreground_service"(or similar) and reusing it for both the channel and the builder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tool/src/main/java/io/netbird/client/tool/ForegroundNotification.java` around lines 60 - 66, The current ensureNotificationChannel() uses service.getPackageName() as the NotificationChannel ID which varies across build variants; define a stable constant like private static final String CHANNEL_ID = "netbird_foreground_service" and replace service.getPackageName() with CHANNEL_ID when creating the NotificationChannel in ensureNotificationChannel(), and also use CHANNEL_ID when building the Notification via Notification.Builder/NotificationCompat.Builder (and anywhere else the channel id is referenced) so the channel id remains stable across variants; keep the existing channel name resource and importance as-is.
72-102: Fallback chain is defensible but partly redundant.
PackageManager.getLaunchIntentForPackage(pkg)already returns anACTION_MAIN/CATEGORY_LAUNCHERintent constrained to that package, so the secondary fallback at Lines 75-81 is essentially the same query and is unlikely to succeed if the first call returnednull. The third fallback (Lines 83-90) using a hardcoded FQCN is the meaningful safety net. Consider dropping the middle block to reduce noise:♻️ Optional simplification
Intent notificationIntent = service.getPackageManager().getLaunchIntentForPackage(service.getPackageName()); - if (notificationIntent == null) { - Intent launcherIntent = new Intent(Intent.ACTION_MAIN); - launcherIntent.addCategory(Intent.CATEGORY_LAUNCHER); - launcherIntent.setPackage(service.getPackageName()); - if (launcherIntent.resolveActivity(service.getPackageManager()) != null) { - notificationIntent = launcherIntent; - } - } - if (notificationIntent == null) { Intent mainActivityIntent = new Intent(Intent.ACTION_MAIN); mainActivityIntent.addCategory(Intent.CATEGORY_LAUNCHER); mainActivityIntent.setClassName(service.getPackageName(), "io.netbird.client.MainActivity"); if (mainActivityIntent.resolveActivity(service.getPackageManager()) != null) { notificationIntent = mainActivityIntent; } }Otherwise the logic correctly resolves the activity before constructing the
PendingIntentand usesFLAG_IMMUTABLE | FLAG_UPDATE_CURRENT(required on API 31+).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tool/src/main/java/io/netbird/client/tool/ForegroundNotification.java` around lines 72 - 102, In createLaunchAppPendingIntent(), remove the middle fallback that builds launcherIntent (the Intent with ACTION_MAIN/CATEGORY_LAUNCHER and setPackage) because PackageManager.getLaunchIntentForPackage(service.getPackageName()) already returns an ACTION_MAIN/CATEGORY_LAUNCHER intent scoped to the package; keep the initial notificationIntent = getLaunchIntentForPackage(...) check and the final hardcoded mainActivityIntent fallback (resolveActivity(...) then assign to notificationIntent), preserve the resolveActivity checks and the FLAG_IMMUTABLE | FLAG_UPDATE_CURRENT use when returning the PendingIntent.
68-70: Optional: guard againstgetSystemServicereturning null.
getSystemService(Context.NOTIFICATION_SERVICE)is documented to potentially returnnullon uncommon contexts. In practice it is non-null for aService, but the unchecked cast plus immediatenotify(...)at Line 38 will NPE if it ever returns null. Either assert non-null or short-circuit gracefully:♻️ Suggested change
- private NotificationManager getNotificationManager() { - return (NotificationManager) service.getSystemService(Context.NOTIFICATION_SERVICE); - } + private NotificationManager getNotificationManager() { + NotificationManager nm = (NotificationManager) service.getSystemService(Context.NOTIFICATION_SERVICE); + if (nm == null) { + throw new IllegalStateException("NotificationManager unavailable"); + } + return nm; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tool/src/main/java/io/netbird/client/tool/ForegroundNotification.java` around lines 68 - 70, The getNotificationManager method in ForegroundNotification currently casts service.getSystemService(Context.NOTIFICATION_SERVICE) without guarding for null; update getNotificationManager() to handle a null return (either by asserting/non-null with a clear IllegalStateException or by returning null and updating callers) and modify the code that calls getNotificationManager() (the notify/notifyCompat usage) to short-circuit gracefully if null (e.g., log and skip notify) so you avoid an NPE when getSystemService returns null.
33-35:stopForeground(boolean)is deprecated since API 33; refactor to use the int-based overload.
stopForeground(true)at line 34 uses the boolean overload deprecated in Android 13 (API 33). Refactor tostopForeground(Service.STOP_FOREGROUND_REMOVE)for explicit clarity, or useandroidx.core.app.ServiceCompat.stopForeground(service, ServiceCompat.STOP_FOREGROUND_REMOVE)for automatic backward compatibility across API levels.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tool/src/main/java/io/netbird/client/tool/ForegroundNotification.java` around lines 33 - 35, The ForegroundNotification.stopForeground() method calls the deprecated service.stopForeground(true); update this call to the int-based overload by replacing service.stopForeground(true) with service.stopForeground(Service.STOP_FOREGROUND_REMOVE) or, for backward compatibility, call androidx.core.app.ServiceCompat.stopForeground(service, androidx.core.app.ServiceCompat.STOP_FOREGROUND_REMOVE); ensure you import android.app.Service or androidx.core.app.ServiceCompat as appropriate and keep the method name ForegroundNotification.stopForeground() unchanged.tool/src/main/java/io/netbird/client/tool/VPNService.java (4)
541-565: Optional: dual reference (scheduledTaskRef+widgetRefreshTask) is hard to follow.The local
AtomicReference<ScheduledFuture<?>> scheduledTaskRefat Line 547 exists only so the runnable can callstopWidgetStateRefresh(scheduledTaskRef.get())for self-cancellation; the fieldwidgetRefreshTaskat Line 564 is the canonical "current task" handle used bystopWidgetStateRefresh()(no-arg). This works, but is a bit confusing — a future maintainer may not realize why both exist. A short comment explaining the self-cancellation pattern would help.Also, since
scheduleWithFixedDelayissynchronizedand writes towidgetRefreshTask, but the no-argstopWidgetStateRefresh()and the(expectedTask)overload are alsosynchronized, the locking is consistent here. Just consider documenting the contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tool/src/main/java/io/netbird/client/tool/VPNService.java` around lines 541 - 565, Add a short clarifying comment in scheduleWidgetStateRefresh() explaining why both the local AtomicReference<ScheduledFuture<?>> scheduledTaskRef and the field widgetRefreshTask are used: scheduledTaskRef is a local handle captured by the Runnable so it can call stopWidgetStateRefresh(scheduledTaskRef.get()) to self-cancel, while widgetRefreshTask is the canonical current task stored for external cancellation and status checks; also note that scheduleWidgetStateRefresh(), stopWidgetStateRefresh() (no-arg) and stopWidgetStateRefresh(ScheduledFuture<?>) are synchronized so callers can rely on that locking contract when reading/modifying widgetRefreshTask.
432-447: Retry budget doubled to ~12s — better, but still polls; consider event-driven trigger.
EXIT_NODE_RETRY_COUNT = WIDGET_STATE_REFRESH_COUNT * 2 = 24× 500ms = ~12s, which is safer for cold starts than the prior 6s window. As suggested previously, listening for a "networks ready" /RouteChangeListenerevent would be strictly better than polling and would eliminate the silent-no-op edge case on slow startups, but the doubled budget is an acceptable interim mitigation.Also note:
Thread.sleep(EXIT_NODE_RETRY_DELAY_MS)onwidgetActionExecutor(a single-thread pool) blocks all subsequent widget actions for up to 12s. A user who toggles the connection switch right after the exit-node switch will see the connection toggle stall behind the exit-node retry loop. AScheduledExecutorService.schedule(...)chain (similar toscheduleWidgetStateRefresh) would avoid head-of-line blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tool/src/main/java/io/netbird/client/tool/VPNService.java` around lines 432 - 447, The toggleExitNodeWhenAvailable method currently blocks the widgetActionExecutor with a synchronous retry loop using EXIT_NODE_RETRY_COUNT and Thread.sleep(EXIT_NODE_RETRY_DELAY_MS); change it to a non-blocking scheduled retry: remove the sleep/for-loop and use a ScheduledExecutorService to schedule repeated attempts (or a single delayed reschedule) that calls getExitNodes() and then toggleExitNode(exitNodes) when available, cancelling further schedules once successful; alternatively hook this flow into the existing network-ready/RouteChangeListener event to trigger a single attempt instead of polling. Ensure scheduled tasks run off the widgetActionExecutor so widget actions are not head-of-line blocked and keep the same retry budget semantics while cancelling/rescheduling correctly on success or interrupt.
152-161: Minor: brief foreground-notification flash on early-return paths.
fgNotification.startForeground()is invoked unconditionally beforehandleWidgetConnectionToggle()/handleWidgetExitNodeToggle(). When those handlers immediately bail out viapromptUserToOpenApp()(no usable profile, missing VPN permission), the flow becomes: post foreground notification →stopForeground()insidepromptUserToOpenApp→ post prompt notification. That's an extra short-lived notification that can flash on screen. Consider gatingstartForeground()so it's only called once the handler has decided to actually run the engine, e.g.:♻️ Suggested restructure
- if (ACTION_WIDGET_TOGGLE_CONNECTION.equals(intent.getAction())) { - fgNotification.startForeground(); - handleWidgetConnectionToggle(); - return START_NOT_STICKY; - } - if (ACTION_WIDGET_TOGGLE_EXIT_NODE.equals(intent.getAction())) { - fgNotification.startForeground(); - handleWidgetExitNodeToggle(); - return START_NOT_STICKY; - } + if (ACTION_WIDGET_TOGGLE_CONNECTION.equals(intent.getAction()) + || ACTION_WIDGET_TOGGLE_EXIT_NODE.equals(intent.getAction())) { + // Promote to foreground immediately to satisfy startForegroundService contract, + // but rely on the handler to call stopForeground/stopSelf when it bails out. + fgNotification.startForeground(); + if (ACTION_WIDGET_TOGGLE_CONNECTION.equals(intent.getAction())) { + handleWidgetConnectionToggle(); + } else { + handleWidgetExitNodeToggle(); + } + return START_NOT_STICKY; + }Note: if these intents arrived via
startForegroundService, you must callstartForegroundwithin 5s regardless, so the trade-off is acceptable. Just be aware of the visual flash.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tool/src/main/java/io/netbird/client/tool/VPNService.java` around lines 152 - 161, The handlers call fgNotification.startForeground() unconditionally causing a brief flash when handlers immediately bail (promptUserToOpenApp()); modify the flow so startForeground() is only invoked when the handler decides to actually proceed: update handleWidgetConnectionToggle() and handleWidgetExitNodeToggle() to return a boolean (e.g., willStartEngine) or expose a method like shouldStartForeground(), have the intent branch call fgNotification.startForeground() only if the handler indicates it will run the engine (or move the startForeground() call into the code path inside those handlers after permission/profile checks), and ensure promptUserToOpenApp() remains able to stopForeground() without the intermediate flash.
364-392: Concurrency hardening looks good; minor: no widget feedback while engine starts.
widgetExitToggleInFlightcorrectly serializes overlapping toggles, and offloadingtoggleExitNodeWhenAvailabletowidgetActionExecutorprevents UI work on the binder thread.One residual UX gap: when
engineRunner.isRunning()isfalseat Line 374, the handler kicks offrunWithoutAuth()and immediately enters the polling loop, but does not broadcast an interim widget refresh. The user has tapped the exit-node switch and may see the widget appear stuck (still showing "Disconnected") until eitherserviceStateListener.onStartedfires the polling loop or the toggle finally lands ~12s later. Consider an immediateupdateWidgetStateAndBroadcast()after kicking off the engine so the widget reflects the in-progress state right away.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tool/src/main/java/io/netbird/client/tool/VPNService.java` around lines 364 - 392, In handleWidgetExitNodeToggle: when engineRunner.isRunning() is false we call engineRunner.runWithoutAuth() but do not update the widget UI; add an immediate call to updateWidgetStateAndBroadcast() right after engineRunner.runWithoutAuth() (before proceeding to set widgetExitToggleInFlight and scheduling toggleExitNodeWhenAvailable) so the widget reflects the "starting/in-progress" state while serviceStateListener.onStarted and the polling loop complete; keep existing serialization via widgetExitToggleInFlight and the widgetActionExecutor unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tool/src/main/java/io/netbird/client/tool/VPNService.java`:
- Around line 505-539: The snapshot of engine state and exit nodes in
updateWidgetStateAndBroadcast is taken outside the synchronized(this) block,
allowing races between isRunning()/getExitNodes() reads and the pref write; fix
by moving the entire computation of isRunning, hasSelectedExitNode,
lastExitNodeRoute and exitNodeName (the logic that iterates getExitNodes())
inside the synchronized(this) block so the read-modify-write is atomic with
respect to other callers, or alternatively ensure updateWidgetStateAndBroadcast
is always invoked on the single-threaded widgetActionExecutor to serialize
calls; reference updateWidgetStateAndBroadcast, getExitNodes, isRunning,
synchronized(this) and widgetActionExecutor when applying the change.
---
Nitpick comments:
In `@tool/src/main/java/io/netbird/client/tool/ForegroundNotification.java`:
- Around line 60-66: The current ensureNotificationChannel() uses
service.getPackageName() as the NotificationChannel ID which varies across build
variants; define a stable constant like private static final String CHANNEL_ID =
"netbird_foreground_service" and replace service.getPackageName() with
CHANNEL_ID when creating the NotificationChannel in ensureNotificationChannel(),
and also use CHANNEL_ID when building the Notification via
Notification.Builder/NotificationCompat.Builder (and anywhere else the channel
id is referenced) so the channel id remains stable across variants; keep the
existing channel name resource and importance as-is.
- Around line 72-102: In createLaunchAppPendingIntent(), remove the middle
fallback that builds launcherIntent (the Intent with
ACTION_MAIN/CATEGORY_LAUNCHER and setPackage) because
PackageManager.getLaunchIntentForPackage(service.getPackageName()) already
returns an ACTION_MAIN/CATEGORY_LAUNCHER intent scoped to the package; keep the
initial notificationIntent = getLaunchIntentForPackage(...) check and the final
hardcoded mainActivityIntent fallback (resolveActivity(...) then assign to
notificationIntent), preserve the resolveActivity checks and the FLAG_IMMUTABLE
| FLAG_UPDATE_CURRENT use when returning the PendingIntent.
- Around line 68-70: The getNotificationManager method in ForegroundNotification
currently casts service.getSystemService(Context.NOTIFICATION_SERVICE) without
guarding for null; update getNotificationManager() to handle a null return
(either by asserting/non-null with a clear IllegalStateException or by returning
null and updating callers) and modify the code that calls
getNotificationManager() (the notify/notifyCompat usage) to short-circuit
gracefully if null (e.g., log and skip notify) so you avoid an NPE when
getSystemService returns null.
- Around line 33-35: The ForegroundNotification.stopForeground() method calls
the deprecated service.stopForeground(true); update this call to the int-based
overload by replacing service.stopForeground(true) with
service.stopForeground(Service.STOP_FOREGROUND_REMOVE) or, for backward
compatibility, call androidx.core.app.ServiceCompat.stopForeground(service,
androidx.core.app.ServiceCompat.STOP_FOREGROUND_REMOVE); ensure you import
android.app.Service or androidx.core.app.ServiceCompat as appropriate and keep
the method name ForegroundNotification.stopForeground() unchanged.
In `@tool/src/main/java/io/netbird/client/tool/VPNService.java`:
- Around line 541-565: Add a short clarifying comment in
scheduleWidgetStateRefresh() explaining why both the local
AtomicReference<ScheduledFuture<?>> scheduledTaskRef and the field
widgetRefreshTask are used: scheduledTaskRef is a local handle captured by the
Runnable so it can call stopWidgetStateRefresh(scheduledTaskRef.get()) to
self-cancel, while widgetRefreshTask is the canonical current task stored for
external cancellation and status checks; also note that
scheduleWidgetStateRefresh(), stopWidgetStateRefresh() (no-arg) and
stopWidgetStateRefresh(ScheduledFuture<?>) are synchronized so callers can rely
on that locking contract when reading/modifying widgetRefreshTask.
- Around line 432-447: The toggleExitNodeWhenAvailable method currently blocks
the widgetActionExecutor with a synchronous retry loop using
EXIT_NODE_RETRY_COUNT and Thread.sleep(EXIT_NODE_RETRY_DELAY_MS); change it to a
non-blocking scheduled retry: remove the sleep/for-loop and use a
ScheduledExecutorService to schedule repeated attempts (or a single delayed
reschedule) that calls getExitNodes() and then toggleExitNode(exitNodes) when
available, cancelling further schedules once successful; alternatively hook this
flow into the existing network-ready/RouteChangeListener event to trigger a
single attempt instead of polling. Ensure scheduled tasks run off the
widgetActionExecutor so widget actions are not head-of-line blocked and keep the
same retry budget semantics while cancelling/rescheduling correctly on success
or interrupt.
- Around line 152-161: The handlers call fgNotification.startForeground()
unconditionally causing a brief flash when handlers immediately bail
(promptUserToOpenApp()); modify the flow so startForeground() is only invoked
when the handler decides to actually proceed: update
handleWidgetConnectionToggle() and handleWidgetExitNodeToggle() to return a
boolean (e.g., willStartEngine) or expose a method like shouldStartForeground(),
have the intent branch call fgNotification.startForeground() only if the handler
indicates it will run the engine (or move the startForeground() call into the
code path inside those handlers after permission/profile checks), and ensure
promptUserToOpenApp() remains able to stopForeground() without the intermediate
flash.
- Around line 364-392: In handleWidgetExitNodeToggle: when
engineRunner.isRunning() is false we call engineRunner.runWithoutAuth() but do
not update the widget UI; add an immediate call to
updateWidgetStateAndBroadcast() right after engineRunner.runWithoutAuth()
(before proceeding to set widgetExitToggleInFlight and scheduling
toggleExitNodeWhenAvailable) so the widget reflects the "starting/in-progress"
state while serviceStateListener.onStarted and the polling loop complete; keep
existing serialization via widgetExitToggleInFlight and the widgetActionExecutor
unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 486f28ba-64ea-4c80-9ef4-f961afbb9cac
📒 Files selected for processing (2)
tool/src/main/java/io/netbird/client/tool/ForegroundNotification.javatool/src/main/java/io/netbird/client/tool/VPNService.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.
This PR adds a NetBird home screen widget for Android.
The widget shows the current connection state and allows toggling:
Full disclosure: I am not a Java/Android developer, and this implementation was written with the help of an LLM coding agent. I would appreciate a careful review from someone more experienced with this codebase. If the approach is not ideal, I would be very happy if someone could adjust or complete it. I would really like to see this feature available in the official app.
Summary by CodeRabbit