Update peer list when peers tab opened #126
Conversation
Updates the UI when the peers list change via StateListener
📝 WalkthroughWalkthroughAdds listener-driven peer-state plumbing: new PeersStateListener and adapters, a StateListenerAdapter base class, a PeersFragmentViewModel exposing LiveData, and refactors PeersFragment to observe the ViewModel and register/unregister state listeners. Changes
Sequence Diagram(s)sequenceDiagram
participant Fragment as PeersFragment
participant ViewModel as PeersFragmentViewModel
participant Adapter as PeersStateListenerAdapter
participant Registry as StateListenerRegistry
participant Service as ServiceAccessor
Fragment->>ViewModel: obtain via factory(getFactory(service))
Fragment->>Registry: register(ViewModel.getStateListener())
Service->>Registry: notify peer-state change (peer list update)
Registry->>Adapter: onPeersListChanged(totalPeers)
Adapter->>ViewModel: onPeersChanged(totalPeers)
ViewModel->>ViewModel: fetch PeerInfoArray, map -> List<Peer>
ViewModel->>Fragment: post LiveData(PeersFragmentUiState)
Fragment->>Fragment: update UI (peers list, counter, adapter, filters)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
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: 3
🤖 Fix all issues with AI agents
In @app/src/main/java/io/netbird/client/ui/home/PeersFragment.java:
- Around line 150-156: PeersFragment.onDestroyView can NPE if model is null;
guard the unregister call by checking model (and stateListenerRegistry) before
calling
stateListenerRegistry.unregisterServiceStateListener(model.getStateListener()).
Update onDestroyView to only call unregisterServiceStateListener when model !=
null (and stateListenerRegistry != null), then proceed to null out binding,
stateListenerRegistry, serviceAccessor and call super.onDestroyView().
In @app/src/main/java/io/netbird/client/ui/home/PeersFragmentUiState.java:
- Around line 5-15: PeersFragmentUiState currently exposes a mutable list; make
the state truly immutable by changing the constructor to defensively copy the
incoming list (e.g., assign this.peers = List.copyOf(peers) or new
ArrayList<>(peers) then wrap with Collections.unmodifiableList) and ensure
getPeers() returns the unmodifiable/immutable list (or a defensive copy) instead
of the original mutable reference; update the PeersFragmentUiState constructor
and getPeers method to implement this defensive-copy/unmodifiable-wrapper
approach.
In @app/src/main/java/io/netbird/client/ui/home/PeersFragmentViewModel.java:
- Around line 45-53: getPeers currently calls
Status.fromString(peerInfo.getConnStatus()) without null/validation and also
assumes peerInfo entries are non-null; update getPeers to skip null PeerInfo
items and to guard against a null or unexpected connStatus by checking peerInfo
!= null, reading peerInfo.getConnStatus() into a local variable, and if that
string is null or Status.fromString throws, fall back to a safe default (e.g.,
Status.UNKNOWN or Status.DISCONNECTED) before constructing new Peer(peerStatus,
peerInfo.getIP(), peerInfo.getFQDN()); reference getPeers, PeerInfoArray,
PeerInfo, peerInfo.getConnStatus(), Status.fromString, and Peer to locate the
change.
🧹 Nitpick comments (2)
app/src/main/java/io/netbird/client/PeersStateListenerAdapter.java (1)
8-10: Consider clarifying the nullability contract.The
@NotNullannotation suggests the listener should never be null, butclearListener()allows setting it to null. This creates ambiguity about the intended contract. Consider either:
- Removing
@NotNullto acknowledge the listener can be cleared- Adding explicit null validation in the constructor if initial null should be prevented
app/src/main/java/io/netbird/client/ui/home/PeersFragment.java (1)
93-102: Remove redundantnotifyDataSetChanged()call on line 100.The
filterBySearchQuery()method on line 101 internally callsapplyFilters(), which already invokesnotifyDataSetChanged(). CallingnotifyDataSetChanged()beforefilterBySearchQuery()causes two consecutive adapter refresh notifications, with the first one immediately becoming stale. Remove the explicitnotifyDataSetChanged()call on line 100 and letfilterBySearchQuery()handle the adapter notification.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/main/java/io/netbird/client/PeersStateListener.javaapp/src/main/java/io/netbird/client/PeersStateListenerAdapter.javaapp/src/main/java/io/netbird/client/StateListenerAdapter.javaapp/src/main/java/io/netbird/client/ui/home/PeersFragment.javaapp/src/main/java/io/netbird/client/ui/home/PeersFragmentUiState.javaapp/src/main/java/io/netbird/client/ui/home/PeersFragmentViewModel.java
🧰 Additional context used
🧬 Code graph analysis (2)
app/src/main/java/io/netbird/client/ui/home/PeersFragmentViewModel.java (1)
app/src/main/java/io/netbird/client/PeersStateListenerAdapter.java (1)
PeersStateListenerAdapter(5-22)
app/src/main/java/io/netbird/client/ui/home/PeersFragment.java (2)
app/src/main/java/io/netbird/client/ui/home/PeersFragmentViewModel.java (1)
PeersFragmentViewModel(19-74)app/src/main/java/io/netbird/client/ui/home/ZeroPeerView.java (1)
ZeroPeerView(10-28)
⏰ 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). (4)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (java)
- GitHub Check: build-debug
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (12)
app/src/main/java/io/netbird/client/PeersStateListener.java (1)
3-5: LGTM!Clean interface design following the listener pattern. The method signature is clear and appropriately scoped.
app/src/main/java/io/netbird/client/StateListenerAdapter.java (1)
3-44: LGTM!Classic adapter pattern implementation that provides no-op defaults for all StateListener methods, allowing subclasses to override only the callbacks they need.
app/src/main/java/io/netbird/client/PeersStateListenerAdapter.java (1)
16-21: LGTM!Proper null-safe delegation pattern. The guard clause prevents NPE when the listener has been cleared.
app/src/main/java/io/netbird/client/ui/home/PeersFragmentViewModel.java (4)
19-29: LGTM!Clean architecture with proper initialization. The ViewModel implements
PeersStateListenerand creates the adapter, establishing the delegation chain correctly.
31-43: LGTM!Standard ViewModelProvider.Factory implementation following Android architecture best practices.
55-67: LGTM!Proper lifecycle management with appropriate cleanup in
onCleared(). The getters correctly expose the UI state and state listener.
69-73: The code is correct as written. ThetotalPeersparameter is part of thePeersStateListenerinterface contract and cannot be removed. This implementation intentionally ignores it in favor of fetching fresh peer data fromserviceAccessor.getPeersList(), which is a valid architectural choice. The use ofpostValue()is also correct—it's the appropriate method for updating LiveData from background thread callbacks (such as native layer notifications), whereassetValue()can only be called from the main thread.If clarification is desired, a brief comment explaining why the implementation fetches full data rather than using the count parameter would be optional documentation, but the code requires no changes.
app/src/main/java/io/netbird/client/ui/home/PeersFragment.java (5)
35-37: LGTM!The new fields for
StateListenerRegistry,PeersFragmentViewModel, and the sharedpeerslist are appropriate for the reactive pattern being implemented.
48-53: LGTM!The
StateListenerRegistryinterface check follows the same established pattern as theServiceAccessorcheck, ensuring the host activity implements the required contract.
66-68: LGTM!The ViewModel is correctly instantiated using the factory pattern with the
serviceAccessordependency. Registering the state listener inonViewCreatedpairs well with unregistering inonDestroyView.
158-173: LGTM!The
updatePeersCountermethod correctly iterates through peers to count connected ones and formats the display string. Usingpost()for the UI update is safe, andHtml.fromHtmlwithFROM_HTML_MODE_LEGACYensures proper API compatibility.
104-141: LGTM!The search text filtering, focus-based icon toggling, and popup menu for status filtering are well-integrated with the new
PeersAdapter.
There was a problem hiding this comment.
Pull request overview
This PR implements reactive updates for the PeersFragment by introducing a ViewModel-based architecture with state listeners. When the peers tab is opened, it automatically updates the displayed peer list when peer availability changes.
Key changes:
- Introduced
PeersFragmentViewModelwith LiveData-based UI state management - Created adapter pattern classes (
StateListenerAdapter,PeersStateListenerAdapter) to bridge between the service'sStateListenercallbacks and the newPeersStateListenerinterface - Refactored
PeersFragmentto observe ViewModel state changes and update the UI reactively
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| StateListenerAdapter.java | Abstract adapter providing empty implementations of StateListener methods, enabling selective override pattern |
| PeersStateListener.java | Simple interface defining peer change callback, decoupling peer state handling from full StateListener implementation |
| PeersStateListenerAdapter.java | Adapter bridging StateListener to PeersStateListener, forwarding onPeersListChanged events with proper null safety |
| PeersFragmentViewModel.java | ViewModel managing peers data state, transforming PeerInfoArray to Peer list, and publishing updates via LiveData |
| PeersFragmentUiState.java | Immutable state holder for peers list, following existing UI state patterns |
| PeersFragment.java | Refactored to use ViewModel pattern with LiveData observation, replacing synchronous peer fetching with reactive updates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This code is throwing "Unknown status: Idle" without reason
Some locales might be turning lowercase i to dotless i (ı)
When parsing PeerInfoArray to List<Peer>
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/ui/home/Status.java:
- Line 38: The thrown IllegalArgumentException uses status.toLowerCase()
inconsistently with earlier comparisons; change the message to use
status.toLowerCase(Locale.ROOT) (i.e., "Unknown status: " +
status.toLowerCase(Locale.ROOT)) to match the comparison, and add/import
java.util.Locale if not already imported so the code compiles.
🧹 Nitpick comments (3)
app/src/main/java/io/netbird/client/ui/home/PeersFragmentUiState.java (1)
10-12: Consider adding null check for defensive programming.If
peersisnull,new ArrayList<>(peers)will throw aNullPointerException. While the current callers (e.g.,PeersFragmentViewModel) always pass non-null lists, a defensive null check would make the class more robust.Proposed fix
public PeersFragmentUiState(List<Peer> peers) { - this.peers = new ArrayList<>(peers); + this.peers = peers != null ? new ArrayList<>(peers) : new ArrayList<>(); }app/src/main/java/io/netbird/client/ui/home/PeersFragment.java (2)
93-102: Consider reordering adapter notifications.Calling
notifyDataSetChanged()immediately followed byfilterBySearchQuery()may trigger two adapter updates in quick succession. IffilterBySearchQueryinternally calls its own notify method, consider removing the explicitnotifyDataSetChanged()call or deferring the filter until after the data change is processed.
156-158: Add null check forstateListenerRegistryfor defensive programming.While in normal lifecycle flow
stateListenerRegistryshould be non-null at this point, adding a null check would make the cleanup more robust against edge cases (e.g., ifonDestroyViewis called due to a configuration change before proper attachment).Proposed fix
- if (model != null) { - stateListenerRegistry.unregisterServiceStateListener(model.getStateListener()); + if (model != null && stateListenerRegistry != null) { + stateListenerRegistry.unregisterServiceStateListener(model.getStateListener()); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/src/main/java/io/netbird/client/ui/home/PeersFragment.javaapp/src/main/java/io/netbird/client/ui/home/PeersFragmentUiState.javaapp/src/main/java/io/netbird/client/ui/home/PeersFragmentViewModel.javaapp/src/main/java/io/netbird/client/ui/home/Status.java
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/io/netbird/client/ui/home/PeersFragmentViewModel.java
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/io/netbird/client/ui/home/PeersFragment.java (2)
app/src/main/java/io/netbird/client/ui/home/PeersFragmentViewModel.java (1)
PeersFragmentViewModel(19-88)app/src/main/java/io/netbird/client/ui/home/ZeroPeerView.java (1)
ZeroPeerView(10-28)
⏰ 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 (6)
app/src/main/java/io/netbird/client/ui/home/Status.java (1)
4-4: Good practice: UsingLocale.ROOTfor consistent case normalization.Using
Locale.ROOTprevents locale-specific issues where certain locales have non-standard case mappings (e.g., Turkish locale where "I".toLowerCase() produces "ı" instead of "i"). This ensures consistent behavior across all locales.Also applies to: 30-30
app/src/main/java/io/netbird/client/ui/home/PeersFragmentUiState.java (1)
14-16: LGTM!Returning
Collections.unmodifiableList(peers)correctly ensures immutability from the caller's perspective while keeping the internal copy.app/src/main/java/io/netbird/client/ui/home/PeersFragment.java (4)
48-53: LGTM!The pattern is consistent with the existing
ServiceAccessorcheck and appropriately enforces the host contract.
66-68: LGTM!The ViewModel is correctly obtained using the factory pattern, and the state listener is registered in
onViewCreatedto pair with the unregistration inonDestroyView.
163-178: LGTM!The method correctly counts connected peers and updates the UI. Using
post()ensures thread-safe UI updates, and the observer is bound togetViewLifecycleOwner(), ensuring this won't be called after the view is destroyed.
31-37: LGTM on the MVVM refactoring.The introduction of
PeersFragmentViewModeland the reactiveuiStatepattern cleanly separates UI state management from the fragment. The sharedpeerslist works correctly with the adapter notification pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/main/java/io/netbird/client/ui/home/PeersFragment.java (1)
93-102: Solid reactive UI update logic.The observer correctly:
- Synchronizes the local peers list with the ViewModel state
- Updates the connected peers counter
- Toggles zero-state visibility
- Reapplies the search filter to maintain filter state across updates
The use of
notifyDataSetChanged()is acceptable for a peers list. If performance becomes a concern with larger datasets, consider usingDiffUtilfor more granular updates.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/io/netbird/client/ui/home/PeersFragment.javaapp/src/main/java/io/netbird/client/ui/home/Status.java
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/io/netbird/client/ui/home/PeersFragment.java (2)
app/src/main/java/io/netbird/client/ui/home/PeersFragmentViewModel.java (1)
PeersFragmentViewModel(19-88)app/src/main/java/io/netbird/client/ui/home/ZeroPeerView.java (1)
ZeroPeerView(10-28)
⏰ 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/Status.java (1)
4-4: LGTM! Good use of Locale.ROOT for case-insensitive comparisons.Using
Locale.ROOTensures consistent behavior across different system locales and avoids potential bugs with locale-sensitive case conversions.Also applies to: 30-30, 38-38
app/src/main/java/io/netbird/client/ui/home/PeersFragment.java (6)
6-8: LGTM! Appropriate imports and fields for MVVM pattern.The new imports and fields support the reactive architecture with proper lifecycle-aware components.
Also applies to: 19-19, 28-28, 35-37
48-53: LGTM! Consistent interface requirement pattern.The StateListenerRegistry requirement check follows the same pattern as ServiceAccessor, ensuring proper architecture.
87-91: LGTM! Clean RecyclerView setup.The adapter and layout manager are properly initialized with the local peers list.
145-150: LGTM! Proper lifecycle cleanup order.The cleanup sequence is correct:
onDestroyView(): Unregisters the listener (with defensive null check) and cleans up view-related resources before callingsuper.onDestroyView()onDetach(): Nullifies context-related references before callingsuper.onDetach()The null check on
modelat line 154 is good defensive programming.Also applies to: 153-159
161-176: LGTM! Cleaner method signature.Passing the peers list as a parameter makes the method more explicit and easier to test compared to accessing a field directly.
66-68: No changes needed. The StateListenerRegistry implementation in MainActivity explicitly prevents duplicate listener registrations via acontains()check inregisterServiceStateListener(), which returns early if the listener is already registered. The listener registration pattern in onViewCreated/onDestroyView is correct and handles configuration changes properly without causing duplicate callbacks.Likely an incorrect or invalid review comment.
This PR makes PeersFragment aware of changes in the available peers list to update the displayed peers list.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.