From d3c7c7c89a34b05887c4dfbebe69fde9f16a50f2 Mon Sep 17 00:00:00 2001 From: Nik Clayton Date: Sun, 24 Mar 2024 18:36:28 +0100 Subject: [PATCH] fix: Surface all exceptions to the user instead of crashing (#565) Previous code would handle some expected exceptions (IO, HTTP) when fetching a timeline, and show them to the user. Any other exception would crash. Now, surface all exceptions. Treat IO and HTTP exceptions as retryable and show the "Retry" option, all others are considered non-retryable. Provide a specific error string for exceptions caused by bad JSON. --- .../media/AccountMediaRemoteMediator.kt | 5 +--- .../notifications/NotificationsViewModel.kt | 5 ++-- .../components/timeline/TimelineFragment.kt | 23 +++++++++++++++---- .../viewmodel/CachedTimelineRemoteMediator.kt | 8 +++---- .../NetworkTimelineRemoteMediator.kt | 5 ++-- .../timeline/viewmodel/TimelineViewModel.kt | 3 +-- core/ui/build.gradle.kts | 3 +++ .../core/ui/extensions/ThrowableExtensions.kt | 2 ++ core/ui/src/main/res/values/strings.xml | 1 + 9 files changed, 34 insertions(+), 21 deletions(-) diff --git a/app/src/main/java/app/pachli/components/account/media/AccountMediaRemoteMediator.kt b/app/src/main/java/app/pachli/components/account/media/AccountMediaRemoteMediator.kt index dc3604f74..226de8ee5 100644 --- a/app/src/main/java/app/pachli/components/account/media/AccountMediaRemoteMediator.kt +++ b/app/src/main/java/app/pachli/components/account/media/AccountMediaRemoteMediator.kt @@ -20,7 +20,6 @@ import androidx.paging.ExperimentalPagingApi import androidx.paging.LoadType import androidx.paging.PagingState import androidx.paging.RemoteMediator -import app.pachli.components.timeline.util.ifExpected import app.pachli.core.database.model.AccountEntity import app.pachli.core.navigation.AttachmentViewData import app.pachli.core.network.retrofit.MastodonApi @@ -72,9 +71,7 @@ class AccountMediaRemoteMediator( viewModel.currentSource?.invalidate() return MediatorResult.Success(endOfPaginationReached = statuses.isEmpty()) } catch (e: Exception) { - return ifExpected(e) { - MediatorResult.Error(e) - } + return MediatorResult.Error(e) } } } diff --git a/app/src/main/java/app/pachli/components/notifications/NotificationsViewModel.kt b/app/src/main/java/app/pachli/components/notifications/NotificationsViewModel.kt index 985982cd2..2fbc604a2 100644 --- a/app/src/main/java/app/pachli/components/notifications/NotificationsViewModel.kt +++ b/app/src/main/java/app/pachli/components/notifications/NotificationsViewModel.kt @@ -32,7 +32,6 @@ import app.pachli.appstore.MuteConversationEvent import app.pachli.appstore.MuteEvent import app.pachli.components.timeline.FilterKind import app.pachli.components.timeline.FiltersRepository -import app.pachli.components.timeline.util.ifExpected import app.pachli.core.accounts.AccountManager import app.pachli.core.network.model.Filter import app.pachli.core.network.model.FilterContext @@ -409,7 +408,7 @@ class NotificationsViewModel @Inject constructor( } } } catch (e: Exception) { - ifExpected(e) { _uiErrorChannel.send(UiError.make(e, it)) } + _uiErrorChannel.send(UiError.make(e, it)) } } } @@ -428,7 +427,7 @@ class NotificationsViewModel @Inject constructor( } uiSuccess.emit(NotificationActionSuccess.from(action)) } catch (e: Exception) { - ifExpected(e) { _uiErrorChannel.send(UiError.make(e, action)) } + _uiErrorChannel.send(UiError.make(e, action)) } } } diff --git a/app/src/main/java/app/pachli/components/timeline/TimelineFragment.kt b/app/src/main/java/app/pachli/components/timeline/TimelineFragment.kt index 368ca897b..bae17dc62 100644 --- a/app/src/main/java/app/pachli/components/timeline/TimelineFragment.kt +++ b/app/src/main/java/app/pachli/components/timeline/TimelineFragment.kt @@ -41,6 +41,7 @@ import androidx.recyclerview.widget.SimpleItemAnimator import androidx.swiperefreshlayout.widget.SwipeRefreshLayout.OnRefreshListener import app.pachli.R import app.pachli.adapter.StatusBaseViewHolder +import app.pachli.components.timeline.util.isExpected import app.pachli.components.timeline.viewmodel.CachedTimelineViewModel import app.pachli.components.timeline.viewmodel.InfallibleUiAction import app.pachli.components.timeline.viewmodel.NetworkTimelineViewModel @@ -431,20 +432,32 @@ class TimelineFragment : // Show errors as a snackbar if there is existing content to show // (either cached, or in the adapter), or as a full screen error // otherwise. + // + // Expected errors can be retried, unexpected ones cannot if (adapter.itemCount > 0) { snackbar = Snackbar.make( (activity as ActionButtonActivity).actionButton ?: binding.root, message, Snackbar.LENGTH_INDEFINITE, - ) - .setAction(app.pachli.core.ui.R.string.action_retry) { adapter.retry() } + ).apply { + if (error.isExpected()) { + setAction(app.pachli.core.ui.R.string.action_retry) { adapter.retry() } + } + } + snackbar!!.show() } else { - binding.statusView.setup(error) { - snackbar?.dismiss() - adapter.retry() + val callback: ((v: View) -> Unit)? = if (error.isExpected()) { + { + snackbar?.dismiss() + adapter.retry() + } + } else { + null } + + binding.statusView.setup(error, callback) binding.statusView.show() binding.recyclerView.hide() } diff --git a/app/src/main/java/app/pachli/components/timeline/viewmodel/CachedTimelineRemoteMediator.kt b/app/src/main/java/app/pachli/components/timeline/viewmodel/CachedTimelineRemoteMediator.kt index ac18abc16..2eed3b32f 100644 --- a/app/src/main/java/app/pachli/components/timeline/viewmodel/CachedTimelineRemoteMediator.kt +++ b/app/src/main/java/app/pachli/components/timeline/viewmodel/CachedTimelineRemoteMediator.kt @@ -37,7 +37,6 @@ import app.pachli.core.network.model.Links import app.pachli.core.network.model.Status import app.pachli.core.network.retrofit.MastodonApi import com.squareup.moshi.Moshi -import java.io.IOException import kotlinx.coroutines.async import kotlinx.coroutines.coroutineScope import okhttp3.Headers @@ -78,6 +77,7 @@ class CachedTimelineRemoteMediator( Timber.d("Loading from item: %s", statusId) getInitialPage(statusId, state.config.pageSize) } + LoadType.APPEND -> { val rke = remoteKeyDao.remoteKeyForKind( activeAccount.id, @@ -87,6 +87,7 @@ class CachedTimelineRemoteMediator( Timber.d("Loading from remoteKey: %s", rke) api.homeTimeline(maxId = rke.key, limit = state.config.pageSize) } + LoadType.PREPEND -> { val rke = remoteKeyDao.remoteKeyForKind( activeAccount.id, @@ -168,9 +169,8 @@ class CachedTimelineRemoteMediator( } return MediatorResult.Success(endOfPaginationReached = false) - } catch (e: IOException) { - MediatorResult.Error(e) - } catch (e: HttpException) { + } catch (e: Exception) { + Timber.e(e, "Error loading, LoadType = %s", loadType) MediatorResult.Error(e) } } diff --git a/app/src/main/java/app/pachli/components/timeline/viewmodel/NetworkTimelineRemoteMediator.kt b/app/src/main/java/app/pachli/components/timeline/viewmodel/NetworkTimelineRemoteMediator.kt index 15d6fc320..0f01af729 100644 --- a/app/src/main/java/app/pachli/components/timeline/viewmodel/NetworkTimelineRemoteMediator.kt +++ b/app/src/main/java/app/pachli/components/timeline/viewmodel/NetworkTimelineRemoteMediator.kt @@ -126,9 +126,8 @@ class NetworkTimelineRemoteMediator( } return MediatorResult.Success(endOfPaginationReached = endOfPaginationReached) - } catch (e: IOException) { - MediatorResult.Error(e) - } catch (e: HttpException) { + } catch (e: Exception) { + Timber.e(e, "Error loading, LoadType = %s", loadType) MediatorResult.Error(e) } } diff --git a/app/src/main/java/app/pachli/components/timeline/viewmodel/TimelineViewModel.kt b/app/src/main/java/app/pachli/components/timeline/viewmodel/TimelineViewModel.kt index 0b7b792ad..0976fa58d 100644 --- a/app/src/main/java/app/pachli/components/timeline/viewmodel/TimelineViewModel.kt +++ b/app/src/main/java/app/pachli/components/timeline/viewmodel/TimelineViewModel.kt @@ -43,7 +43,6 @@ import app.pachli.appstore.StatusEditedEvent import app.pachli.appstore.UnfollowEvent import app.pachli.components.timeline.FilterKind import app.pachli.components.timeline.FiltersRepository -import app.pachli.components.timeline.util.ifExpected import app.pachli.core.accounts.AccountManager import app.pachli.core.network.model.Filter import app.pachli.core.network.model.FilterContext @@ -361,7 +360,7 @@ abstract class TimelineViewModel( }.getOrThrow() uiSuccess.emit(StatusActionSuccess.from(action)) } catch (e: Exception) { - ifExpected(e) { _uiErrorChannel.send(UiError.make(e, action)) } + _uiErrorChannel.send(UiError.make(e, action)) } } } diff --git a/core/ui/build.gradle.kts b/core/ui/build.gradle.kts index 5bfd0614e..88b2e5bba 100644 --- a/core/ui/build.gradle.kts +++ b/core/ui/build.gradle.kts @@ -38,6 +38,9 @@ dependencies { // Uses HttpException from Retrofit implementation(projects.core.network) + // Uses JsonDataException from Moshi + implementation(libs.moshi) + // Some views inherit from AndroidX views implementation(libs.bundles.androidx) diff --git a/core/ui/src/main/kotlin/app/pachli/core/ui/extensions/ThrowableExtensions.kt b/core/ui/src/main/kotlin/app/pachli/core/ui/extensions/ThrowableExtensions.kt index e5cc02795..2e739edcd 100644 --- a/core/ui/src/main/kotlin/app/pachli/core/ui/extensions/ThrowableExtensions.kt +++ b/core/ui/src/main/kotlin/app/pachli/core/ui/extensions/ThrowableExtensions.kt @@ -20,6 +20,7 @@ package app.pachli.core.ui.extensions import android.content.Context import app.pachli.core.network.extensions.getServerErrorMessage import app.pachli.core.ui.R +import com.squareup.moshi.JsonDataException import java.io.IOException import retrofit2.HttpException @@ -40,5 +41,6 @@ fun Throwable.getDrawableRes(): Int = when (this) { fun Throwable.getErrorString(context: Context): String = getServerErrorMessage() ?: when (this) { is IOException -> context.getString(R.string.error_network_fmt, this.message) is HttpException -> if (this.code() == 404) context.getString(R.string.error_404_not_found_fmt, this.message) else context.getString(R.string.error_generic_fmt, this.message) + is JsonDataException -> context.getString(R.string.error_json_data_fmt, this.message) else -> context.getString(R.string.error_generic_fmt, this.message) } diff --git a/core/ui/src/main/res/values/strings.xml b/core/ui/src/main/res/values/strings.xml index 8673000f8..0c606e435 100644 --- a/core/ui/src/main/res/values/strings.xml +++ b/core/ui/src/main/res/values/strings.xml @@ -4,6 +4,7 @@ A network error occurred: %s An error occurred: %s Your server does not support this feature: %1$s + Your server returned an invalid response: %1$s An error occurred. Nothing here. Retry