From 56d7be331e727662dfd6014a14a9c7bd2ba5b5a8 Mon Sep 17 00:00:00 2001 From: Andrea Gottardo Date: Wed, 29 Jan 2025 10:03:23 -0800 Subject: [PATCH] ui: display error dialog when saving subnet routes fails (#604) Fixes tailscale/corp#26175 When setting subnet routing settings, for a variety of reasons the Tailscale backend may reject an entered value with a 400 error. Here we handle such errors in a user-facing fashion: - We display an ErrorDialog with title 'Failed to save' and whatever error message the backend request returned. To do so, we introduce a new initializer for ErrorDialog that accepts a runtime-generated String instead of a fixed string resource. - We ask the backend to provide an updated value of AdvertiseRoutes whenever the error dialog is dismissed by the user, and set it as the UI state, to ensure consistency between UI and backend upon a failed save. Signed-off-by: Andrea Gottardo --- .../com/tailscale/ipn/ui/view/ErrorDialog.kt | 41 ++++++++++++++----- .../ipn/ui/view/SubnetRoutingView.kt | 8 ++++ .../ui/viewModel/SubnetRoutingViewModel.kt | 22 ++++++++++ android/src/main/res/values/strings.xml | 1 + 4 files changed, 61 insertions(+), 11 deletions(-) diff --git a/android/src/main/java/com/tailscale/ipn/ui/view/ErrorDialog.kt b/android/src/main/java/com/tailscale/ipn/ui/view/ErrorDialog.kt index 2e2750a..36b7596 100644 --- a/android/src/main/java/com/tailscale/ipn/ui/view/ErrorDialog.kt +++ b/android/src/main/java/com/tailscale/ipn/ui/view/ErrorDialog.kt @@ -53,8 +53,12 @@ enum class ErrorDialogType { @Composable fun ErrorDialog(type: ErrorDialogType, action: () -> Unit = {}) { - ErrorDialog( - title = type.title, message = type.message, buttonText = type.buttonText, onDismiss = action) + ErrorDialog( + title = type.title, + message = stringResource(id = type.message), + buttonText = type.buttonText, + onDismiss = action + ) } @Composable @@ -64,15 +68,30 @@ fun ErrorDialog( @StringRes buttonText: Int = R.string.ok, onDismiss: () -> Unit = {} ) { - AppTheme { - AlertDialog( - onDismissRequest = onDismiss, - title = { Text(text = stringResource(id = title)) }, - text = { Text(text = stringResource(id = message)) }, - confirmButton = { - PrimaryActionButton(onClick = onDismiss) { Text(text = stringResource(id = buttonText)) } - }) - } + ErrorDialog( + title = title, + message = stringResource(id = message), + buttonText = buttonText, + onDismiss = onDismiss + ) +} + +@Composable +fun ErrorDialog( + @StringRes title: Int = R.string.error, + message: String, + @StringRes buttonText: Int = R.string.ok, + onDismiss: () -> Unit = {} +) { + AppTheme { + AlertDialog( + onDismissRequest = onDismiss, + title = { Text(text = stringResource(id = title)) }, + text = { Text(text = message) }, + confirmButton = { + PrimaryActionButton(onClick = onDismiss) { Text(text = stringResource(id = buttonText)) } + }) + } } @Preview diff --git a/android/src/main/java/com/tailscale/ipn/ui/view/SubnetRoutingView.kt b/android/src/main/java/com/tailscale/ipn/ui/view/SubnetRoutingView.kt index 357d609..ffab356 100644 --- a/android/src/main/java/com/tailscale/ipn/ui/view/SubnetRoutingView.kt +++ b/android/src/main/java/com/tailscale/ipn/ui/view/SubnetRoutingView.kt @@ -40,6 +40,7 @@ fun SubnetRoutingView(backToSettings: BackNavigation, model: SubnetRoutingViewMo val uriHandler = LocalUriHandler.current val isPresentingDialog by model.isPresentingDialog.collectAsState() val useSubnets by model.routeAll.collectAsState() + val currentError by model.currentError.collectAsState() Scaffold(topBar = { Header(R.string.subnet_routes, onBack = backToSettings, actions = { @@ -56,6 +57,13 @@ fun SubnetRoutingView(backToSettings: BackNavigation, model: SubnetRoutingViewMo }) { innerPadding -> LoadingIndicator.Wrap { LazyColumn(modifier = Modifier.padding(innerPadding)) { + currentError?.let { + item("error") { + ErrorDialog(title = R.string.failed_to_save, message = it, onDismiss = { + model.onErrorDismissed() + }) + } + } item("subnetsToggle") { Setting.Switch(R.string.use_tailscale_subnets, isOn = useSubnets, onToggle = { LoadingIndicator.start() diff --git a/android/src/main/java/com/tailscale/ipn/ui/viewModel/SubnetRoutingViewModel.kt b/android/src/main/java/com/tailscale/ipn/ui/viewModel/SubnetRoutingViewModel.kt index 73269df..0bee1bb 100644 --- a/android/src/main/java/com/tailscale/ipn/ui/viewModel/SubnetRoutingViewModel.kt +++ b/android/src/main/java/com/tailscale/ipn/ui/viewModel/SubnetRoutingViewModel.kt @@ -63,6 +63,12 @@ class SubnetRoutingViewModel : ViewModel() { */ val isTextFieldValueValid: StateFlow = MutableStateFlow(true) + /** + * If an error occurred while saving the ipn.Prefs to the backend this value is + * non-null. Subsequent successful attempts to save will clear it. + */ + val currentError: MutableStateFlow = MutableStateFlow(null) + init { viewModelScope.launch { // Any time the value entered by the user in the add/edit dialog changes, we determine @@ -113,12 +119,14 @@ class SubnetRoutingViewModel : ViewModel() { Client(viewModelScope).editPrefs(prefsOut, responseHandler = { result -> if (result.isFailure) { Log.e(TAG, "Error saving RouteAll: ${result.exceptionOrNull()}") + currentError.set(result.exceptionOrNull()?.localizedMessage) return@editPrefs } else { Log.d( TAG, "RouteAll set in backend. New value: ${result.getOrNull()?.RouteAll}" ) + currentError.set(null) } }) } @@ -221,16 +229,30 @@ class SubnetRoutingViewModel : ViewModel() { Client(viewModelScope).editPrefs(prefsOut, responseHandler = { result -> if (result.isFailure) { Log.e(TAG, "Error saving AdvertiseRoutes: ${result.exceptionOrNull()}") + currentError.set(result.exceptionOrNull()?.localizedMessage) return@editPrefs } else { Log.d( TAG, "AdvertiseRoutes set in backend. New value: ${result.getOrNull()?.AdvertiseRoutes}" ) + currentError.set(null) } }) } + /** + * Clears the current error message and reloads the routes currently saved in the backend + * to the UI. We call this when dismissing an error upon saving the routes. + */ + fun onErrorDismissed() { + currentError.set(null) + Client(viewModelScope).prefs { response -> + Log.d(TAG, "Reloading routes from backend due to failed save: $response") + this.advertisedRoutes.set(response.getOrNull()?.AdvertiseRoutes ?: emptyList()) + } + } + companion object RouteValidation { /** * Returns true if the given String is a valid IPv4 or IPv6 CIDR range, false otherwise. diff --git a/android/src/main/res/values/strings.xml b/android/src/main/res/values/strings.xml index 01b122c..c1c539a 100644 --- a/android/src/main/res/values/strings.xml +++ b/android/src/main/res/values/strings.xml @@ -321,5 +321,6 @@ Subnet routing Specifies a device name to be used instead of the automatic default. Hostname + Failed to save