Replace VPNServiceRepository with ServiceAccessor pattern #114
Conversation
VPNServiceRepository created its own service binding which caused conflicts when unbinding - it would disconnect other components relying on the service. Now NetworksFragmentViewModel uses ServiceAccessor from MainActivity, which maintains a single shared service binding across all components.
WalkthroughThis pull request refactors the VPN service access pattern by removing VPNServiceRepository and VPNServiceBindListener, replacing them with a ServiceAccessor interface. Route management and listener capabilities are now exposed through MainActivity and integrated into NetworksFragmentViewModel, while data transformation logic migrates from the repository to the ViewModel. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/src/main/java/io/netbird/client/ServiceAccessor.java (1)
15-16: Consider using a more specific exception type.The methods throw a generic
Exception, which forces callers to handle all exception types. If the underlyingmBindermethods throw specific exceptions, consider propagating those types or creating a custom exception class for route operations.app/src/main/java/io/netbird/client/ui/home/NetworksFragmentViewModel.java (1)
54-60: Consider graceful error handling instead of RuntimeException.Wrapping exceptions in
RuntimeExceptionwill crash the app if the underlying gomobile library throws. Consider logging the error and returning an empty list, or propagating errors through a UI-visible error state.try { for (int i = 0; i < peerRoutes.size(); i++) { routes.add(peerRoutes.get(i)); } } catch (Exception e) { - throw new RuntimeException(e); + Log.e("NetworksFragmentVM", "Failed to get peer routes", e); + // Return partial results collected so far } return routes;Also applies to: 86-88
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/src/main/java/io/netbird/client/MainActivity.java(2 hunks)app/src/main/java/io/netbird/client/MyApplication.java(0 hunks)app/src/main/java/io/netbird/client/ServiceAccessor.java(2 hunks)app/src/main/java/io/netbird/client/repository/VPNServiceBindListener.java(0 hunks)app/src/main/java/io/netbird/client/repository/VPNServiceRepository.java(0 hunks)app/src/main/java/io/netbird/client/ui/home/NetworksFragment.java(4 hunks)app/src/main/java/io/netbird/client/ui/home/NetworksFragmentViewModel.java(2 hunks)
💤 Files with no reviewable changes (3)
- app/src/main/java/io/netbird/client/repository/VPNServiceBindListener.java
- app/src/main/java/io/netbird/client/MyApplication.java
- app/src/main/java/io/netbird/client/repository/VPNServiceRepository.java
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/io/netbird/client/ui/home/NetworksFragment.java (1)
app/src/main/java/io/netbird/client/ui/home/NetworksFragmentViewModel.java (1)
NetworksFragmentViewModel(18-190)
⏰ 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). (2)
- GitHub Check: build-debug
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (7)
app/src/main/java/io/netbird/client/ui/home/NetworksFragment.java (2)
41-56: LGTM - Consistent dependency injection pattern.The
ServiceAccessorcheck follows the same pattern as the existingStateListenerRegistrycheck, ensuring the hosting activity implements the required interface. SinceonAttachis called beforeonViewCreated,serviceAccessorwill be properly initialized when used.
69-70: LGTM - Factory-based ViewModel instantiation.Using
NetworksFragmentViewModel.getFactory(serviceAccessor)properly injects the dependency while adhering to the ViewModel lifecycle.app/src/main/java/io/netbird/client/ServiceAccessor.java (1)
18-19: LGTM - Standard listener registration pattern.The add/remove listener methods follow the standard observer pattern for route change notifications.
app/src/main/java/io/netbird/client/ui/home/NetworksFragmentViewModel.java (4)
28-39: LGTM - Standard ViewModel factory pattern.The factory correctly creates the ViewModel with the injected
ServiceAccessordependency. The unchecked cast warning is unavoidable with ViewModelProvider's generic API.
41-45: LGTM - Proper cleanup in onCleared.The ViewModel correctly unregisters itself as a listener when cleared, preventing memory leaks and dangling callbacks.
128-134: LGTM - Correct use of postValue for background thread updates.Using
postValueinstead ofsetValueis appropriate since this method may be called from background threads viaonRouteChangedandonPeersListChangedcallbacks.
23-26: I'll help you verify the timing of listener registration in the NetworksFragmentViewModel constructor. Let me investigate the service binding lifecycle and how it relates to ViewModel creation.
<function_calls>
#!/bin/bashFind MainActivity and check service binding lifecycle
fd -t f "MainActivity.java"
</function_calls>
<function_calls>
#!/bin/bashFind ServiceAccessor to check listener registration logic
fd -t f "ServiceAccessor.java"
</function_calls>
<function_calls>
#!/bin/bashSearch for onServiceConnected and service binding patterns
rg -n "onServiceConnected|onServiceDisconnected|mBinder" --type java -B2 -A5
</function_calls>
| @Override | ||
| public void selectRoute(String route) throws Exception { | ||
| if (mBinder == null) { | ||
| Log.w(LOGTAG, "VPN binder is null"); | ||
| return; | ||
| } | ||
|
|
||
| mBinder.selectRoute(route); | ||
| } |
There was a problem hiding this comment.
Silent failure on null binder may cause unexpected behavior.
When mBinder is null, the method returns silently without throwing or indicating failure. Callers expect either success or an exception (per the method signature). Consider throwing an exception or returning a status to indicate the operation couldn't be performed.
@Override
public void selectRoute(String route) throws Exception {
if (mBinder == null) {
Log.w(LOGTAG, "VPN binder is null");
- return;
+ throw new IllegalStateException("VPN service not bound");
}
mBinder.selectRoute(route);
}🤖 Prompt for AI Agents
In app/src/main/java/io/netbird/client/MainActivity.java around lines 369 to
377, the method selectRoute currently returns silently when mBinder is null even
though its signature declares it throws Exception; change the null-handling to
log the condition and then throw a suitable exception (e.g.,
IllegalStateException or a checked Exception consistent with the signature) so
callers receive an error instead of silent failure; keep the existing log
warning, then throw the exception with a clear message like "VPN binder is null"
to signal the operation couldn't be performed.
VPNServiceRepository created its own service binding which caused conflicts when unbinding. It would disconnect other components relying on the service.
Now NetworksFragmentViewModel uses ServiceAccessor from MainActivity, which maintains a single shared service binding across all components.
Summary by CodeRabbit
Release Notes
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.