android: add selection of included/excluded apps in split tunneling #621
Conversation
Updates tailscale/tailscale#14660 Signed-off-by: davfsa <[email protected]>
davfsa
force-pushed
the
task/improve-app-split-tunneling-feature
branch
from
91cf3af to
fbcd071
Compare
davfsa
changed the title
davfsa
changed the title
Pull Request Revisions
✅ AI review completed for r5 HelpReact with emojis to give feedback on AI-generated reviews:
We'd love to hear from you—reach out anytime at [email protected]. |
![review-ai-agent[bot]](https://avatars.githubusercontent.com/in/1104228?s=60&v=4){kind=small}
android/src/main/java/com/tailscale/ipn/ui/view/SplitTunnelAppPickerView.kt
Show resolved
Hide resolved
android/src/main/java/com/tailscale/ipn/ui/view/SplitTunnelAppPickerView.kt
Show resolved
Hide resolved
davfsa
force-pushed
the
task/improve-app-split-tunneling-feature
branch
from
944d0f8 to
6b210ba
Compare
Signed-off-by: davfsa <[email protected]>
|
Hey @barnstar, sorry for the ping, but its been a little while, would it be possible to get a review? Thanks! And again, sorry for the ping! |
Signed-off-by: davfsa <[email protected]>
|
Ability to merge seems stuck, so closing and re-opening to try and get it fixed :) |
| onConfirm = { | ||
| model.showSwitchDialog.set(false) | ||
| model.performSelectionSwitch() | ||
| }, |
There was a problem hiding this comment.
In SplitTunnelAppPickerView.kt, the dialog implementation (SwitchAlertDialog) uses a nested function call pattern that could be simplified. The onConfirm callback calls model.showSwitchDialog.set(false) followed by model.performSelectionSwitch(). Consider handling this in the ViewModel to maintain separation of concerns.
| @@ -109,3 +147,40 @@ fun SplitTunnelAppPickerView( | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| @Composable | |||
| fun FusMenu(viewModel: SplitTunnelAppPickerViewModel, onSwitchClick: (() -> Unit)) { | |||
There was a problem hiding this comment.
The FusMenu name in SplitTunnelAppPickerView.kt is not descriptive of its purpose. Consider renaming it to something more meaningful like 'FilterSelectionMenu' to better indicate its role in switching between app inclusion and exclusion modes.
| installedApps.set(installedAppsManager.fetchInstalledApps()) | ||
| excludedPackageNames.set( | ||
| initSelectedPackageNames() | ||
| } | ||
|
|
||
| private fun initSelectedPackageNames() { | ||
| allowSelected.set(App.get().allowSelectedPackages()) | ||
| selectedPackageNames.set( | ||
| App.get() | ||
| .disallowedPackageNames() | ||
| .selectedPackageNames() | ||
| .let { | ||
| if (!allowSelected.value) { | ||
| it.union(App.get().builtInDisallowedPackageNames) | ||
| } else { | ||
| it | ||
| } | ||
| } | ||
| .intersect(installedApps.value.map { it.packageName }.toSet()) | ||
| .toList()) |
There was a problem hiding this comment.
In SplitTunnelAppPickerViewModel.kt, you're using .set() and .value properties to update state flows. This pattern mixes direct mutation with property access in a way that's prone to errors. Consider a more consistent pattern like only using value = or consistently using set().
| IconButton(onClick = { model.showHeaderMenu.set(!showHeaderMenu) }) { | ||
| Icon(Icons.Default.MoreVert, "menu") |
There was a problem hiding this comment.
In SplitTunnelAppPickerView.kt line 72-73, the IconButton has meaningful functionality but its icon's contentDescription is just 'menu' which is too generic. Consider using a more descriptive content description like 'Split tunneling options' to improve accessibility.
Hello!
I took a stab at adding the ability to select included/excluded apps from App Split Tunneling instead of only allowing selecting excluded applications.
All images are 1280x2856, so you can open them in a new tab to view them better
A couple of points:
DISALLOWED_APPS_KEY(nowSELECTED_APPS_KEY) is in a bit of a weird situation as the value no longer represents the variable name. I havent opted to add a migration flow for the key, but can be done if needed. For now I have just added a comment above explaining the the reasonswitchUserSelectedPackagesis not the best of names, but I couldn't come up with any better, will update with any suggestions or if I come up with a better oneCloses tailscale/tailscale#14660