Conversation
There was a problem hiding this comment.
Pull request overview
Adds a dynamic “Select Location” submenu to the desktop system tray menu so Pro users can pick a VPN server location directly from the tray, with menu contents updating when Pro status or available server locations change.
Changes:
- Listen to Pro status and available server list changes to refresh the tray menu in real time.
- Add a “Select Location” tray submenu for Pro users, listing locations sorted by country/city and connecting on selection.
- Add the
select_locationlocalization string.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| lib/features/system_tray/provider/system_tray_notifier.dart | Adds Pro/location state tracking, listeners to refresh the tray menu, and a location-selection submenu with connect behavior. |
| assets/locales/en.po | Adds English translation for the new “Select Location” tray label. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _isUserPro = ref.read(isUserProProvider); | ||
| ref.listen<bool>( | ||
| isUserProProvider, | ||
| (previous, next) async { | ||
| _isUserPro = next; | ||
| await updateTrayMenu(); | ||
| }, | ||
| ); | ||
|
|
||
| ref.listen<AsyncValue<AvailableServers>>( | ||
| availableServersProvider, | ||
| (previous, next) async { | ||
| final data = next.value; | ||
| _locations = data?.lantern.locations.values.toList() ?? []; | ||
| await updateTrayMenu(); | ||
| }, |
There was a problem hiding this comment.
ref.listen callbacks are declared async and await updateTrayMenu(), but ref.listen expects a synchronous callback; the returned Future is ignored and any exceptions become unhandled. Consider making the listener callback synchronous and triggering the async work explicitly (e.g., fire-and-forget with centralized error handling/logging inside updateTrayMenu).
| result.fold( | ||
| (failure) => appLogger | ||
| .error('Failed to connect: ${failure.localizedErrorMessage}'), | ||
| (success) async { | ||
| final savedServerLocation = | ||
| sl<LocalStorageService>().getSavedServerLocations(); | ||
| final serverLocation = savedServerLocation.lanternLocation( | ||
| server: location, | ||
| autoSelect: false, | ||
| ); | ||
| await ref | ||
| .read(serverLocationProvider.notifier) | ||
| .updateServerLocation(serverLocation); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
result.fold mixes a synchronous left handler with an async right handler; the Future returned from the success branch is not awaited, so updateServerLocation may run later (or fail) without being observed. Prefer making both branches async and awaiting the fold result (or rewrite to an explicit if (result.isLeft/Right) flow) so errors are handled deterministically.
| Future<void> _onLocationSelected(Location_ location) async { | ||
| final result = await ref.read(vpnProvider.notifier).connectToServer( | ||
| ServerLocationType.lanternLocation, | ||
| location.tag, | ||
| ); | ||
| result.fold( | ||
| (failure) => appLogger | ||
| .error('Failed to connect: ${failure.localizedErrorMessage}'), | ||
| (success) async { | ||
| final savedServerLocation = | ||
| sl<LocalStorageService>().getSavedServerLocations(); | ||
| final serverLocation = savedServerLocation.lanternLocation( | ||
| server: location, | ||
| autoSelect: false, | ||
| ); | ||
| await ref | ||
| .read(serverLocationProvider.notifier) | ||
| .updateServerLocation(serverLocation); | ||
| }, |
There was a problem hiding this comment.
The tray path updates the saved/selected server location immediately on connectToServer success. Elsewhere (e.g. server selection UI) the app waits until the VPN status actually becomes connected before persisting the chosen location, to avoid showing/saving a location if the connection ultimately fails. Consider aligning this flow by updating serverLocationProvider only after the VPN reports connected (or after verifying the switch completed).
| return MenuItem( | ||
| key: 'location_${location.tag}', | ||
| label: displayName, | ||
| onClick: (_) => _onLocationSelected(location), |
There was a problem hiding this comment.
MenuItem.onClick is invoked from a synchronous tray event handler; returning a Future from (_) => _onLocationSelected(location) means errors can surface as unhandled async exceptions. Consider invoking the async method without returning its future and handling/logging errors within _onLocationSelected (or via an explicit fire-and-forget helper).
| onClick: (_) => _onLocationSelected(location), | |
| onClick: (_) { | |
| _onLocationSelected(location).catchError((error, stackTrace) { | |
| stderr.writeln('Error selecting location $displayName: $error'); | |
| stderr.writeln(stackTrace); | |
| }); | |
| }, |
* Select server location from menu bar * Improve readability and check extension status. * Merge main --------- Co-authored-by: Jigar-f <jigar@getlantern.org>
2 participants
This pull request enhances the system tray functionality by adding a dynamic server location selection menu for Pro users. It introduces logic to listen for changes in user status and available server locations, updating the tray menu accordingly. Additionally, it implements the ability for users to connect to a selected server location directly from the tray menu.
System tray enhancements:
Localization:
Internal state management:
_isUserProand_locationsfields inSystemTrayNotifierto track user status and available locations._onLocationSelectedmethod to handle connecting to a selected server and updating the current server location in the app state.Dependency management: