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.
This commit is contained in:
Nik Clayton 2024-03-24 18:36:28 +01:00 committed by GitHub
parent 2a4126a542
commit d3c7c7c89a
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 34 additions and 21 deletions

View File

@ -20,7 +20,6 @@ import androidx.paging.ExperimentalPagingApi
import androidx.paging.LoadType import androidx.paging.LoadType
import androidx.paging.PagingState import androidx.paging.PagingState
import androidx.paging.RemoteMediator import androidx.paging.RemoteMediator
import app.pachli.components.timeline.util.ifExpected
import app.pachli.core.database.model.AccountEntity import app.pachli.core.database.model.AccountEntity
import app.pachli.core.navigation.AttachmentViewData import app.pachli.core.navigation.AttachmentViewData
import app.pachli.core.network.retrofit.MastodonApi import app.pachli.core.network.retrofit.MastodonApi
@ -72,9 +71,7 @@ class AccountMediaRemoteMediator(
viewModel.currentSource?.invalidate() viewModel.currentSource?.invalidate()
return MediatorResult.Success(endOfPaginationReached = statuses.isEmpty()) return MediatorResult.Success(endOfPaginationReached = statuses.isEmpty())
} catch (e: Exception) { } catch (e: Exception) {
return ifExpected(e) { return MediatorResult.Error(e)
MediatorResult.Error(e)
}
} }
} }
} }

View File

@ -32,7 +32,6 @@ import app.pachli.appstore.MuteConversationEvent
import app.pachli.appstore.MuteEvent import app.pachli.appstore.MuteEvent
import app.pachli.components.timeline.FilterKind import app.pachli.components.timeline.FilterKind
import app.pachli.components.timeline.FiltersRepository import app.pachli.components.timeline.FiltersRepository
import app.pachli.components.timeline.util.ifExpected
import app.pachli.core.accounts.AccountManager import app.pachli.core.accounts.AccountManager
import app.pachli.core.network.model.Filter import app.pachli.core.network.model.Filter
import app.pachli.core.network.model.FilterContext import app.pachli.core.network.model.FilterContext
@ -409,7 +408,7 @@ class NotificationsViewModel @Inject constructor(
} }
} }
} catch (e: Exception) { } 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)) uiSuccess.emit(NotificationActionSuccess.from(action))
} catch (e: Exception) { } catch (e: Exception) {
ifExpected(e) { _uiErrorChannel.send(UiError.make(e, action)) } _uiErrorChannel.send(UiError.make(e, action))
} }
} }
} }

View File

@ -41,6 +41,7 @@ import androidx.recyclerview.widget.SimpleItemAnimator
import androidx.swiperefreshlayout.widget.SwipeRefreshLayout.OnRefreshListener import androidx.swiperefreshlayout.widget.SwipeRefreshLayout.OnRefreshListener
import app.pachli.R import app.pachli.R
import app.pachli.adapter.StatusBaseViewHolder 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.CachedTimelineViewModel
import app.pachli.components.timeline.viewmodel.InfallibleUiAction import app.pachli.components.timeline.viewmodel.InfallibleUiAction
import app.pachli.components.timeline.viewmodel.NetworkTimelineViewModel 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 // Show errors as a snackbar if there is existing content to show
// (either cached, or in the adapter), or as a full screen error // (either cached, or in the adapter), or as a full screen error
// otherwise. // otherwise.
//
// Expected errors can be retried, unexpected ones cannot
if (adapter.itemCount > 0) { if (adapter.itemCount > 0) {
snackbar = Snackbar.make( snackbar = Snackbar.make(
(activity as ActionButtonActivity).actionButton (activity as ActionButtonActivity).actionButton
?: binding.root, ?: binding.root,
message, message,
Snackbar.LENGTH_INDEFINITE, Snackbar.LENGTH_INDEFINITE,
) ).apply {
.setAction(app.pachli.core.ui.R.string.action_retry) { adapter.retry() } if (error.isExpected()) {
setAction(app.pachli.core.ui.R.string.action_retry) { adapter.retry() }
}
}
snackbar!!.show() snackbar!!.show()
} else { } else {
binding.statusView.setup(error) { val callback: ((v: View) -> Unit)? = if (error.isExpected()) {
{
snackbar?.dismiss() snackbar?.dismiss()
adapter.retry() adapter.retry()
} }
} else {
null
}
binding.statusView.setup(error, callback)
binding.statusView.show() binding.statusView.show()
binding.recyclerView.hide() binding.recyclerView.hide()
} }

View File

@ -37,7 +37,6 @@ import app.pachli.core.network.model.Links
import app.pachli.core.network.model.Status import app.pachli.core.network.model.Status
import app.pachli.core.network.retrofit.MastodonApi import app.pachli.core.network.retrofit.MastodonApi
import com.squareup.moshi.Moshi import com.squareup.moshi.Moshi
import java.io.IOException
import kotlinx.coroutines.async import kotlinx.coroutines.async
import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.coroutineScope
import okhttp3.Headers import okhttp3.Headers
@ -78,6 +77,7 @@ class CachedTimelineRemoteMediator(
Timber.d("Loading from item: %s", statusId) Timber.d("Loading from item: %s", statusId)
getInitialPage(statusId, state.config.pageSize) getInitialPage(statusId, state.config.pageSize)
} }
LoadType.APPEND -> { LoadType.APPEND -> {
val rke = remoteKeyDao.remoteKeyForKind( val rke = remoteKeyDao.remoteKeyForKind(
activeAccount.id, activeAccount.id,
@ -87,6 +87,7 @@ class CachedTimelineRemoteMediator(
Timber.d("Loading from remoteKey: %s", rke) Timber.d("Loading from remoteKey: %s", rke)
api.homeTimeline(maxId = rke.key, limit = state.config.pageSize) api.homeTimeline(maxId = rke.key, limit = state.config.pageSize)
} }
LoadType.PREPEND -> { LoadType.PREPEND -> {
val rke = remoteKeyDao.remoteKeyForKind( val rke = remoteKeyDao.remoteKeyForKind(
activeAccount.id, activeAccount.id,
@ -168,9 +169,8 @@ class CachedTimelineRemoteMediator(
} }
return MediatorResult.Success(endOfPaginationReached = false) return MediatorResult.Success(endOfPaginationReached = false)
} catch (e: IOException) { } catch (e: Exception) {
MediatorResult.Error(e) Timber.e(e, "Error loading, LoadType = %s", loadType)
} catch (e: HttpException) {
MediatorResult.Error(e) MediatorResult.Error(e)
} }
} }

View File

@ -126,9 +126,8 @@ class NetworkTimelineRemoteMediator(
} }
return MediatorResult.Success(endOfPaginationReached = endOfPaginationReached) return MediatorResult.Success(endOfPaginationReached = endOfPaginationReached)
} catch (e: IOException) { } catch (e: Exception) {
MediatorResult.Error(e) Timber.e(e, "Error loading, LoadType = %s", loadType)
} catch (e: HttpException) {
MediatorResult.Error(e) MediatorResult.Error(e)
} }
} }

View File

@ -43,7 +43,6 @@ import app.pachli.appstore.StatusEditedEvent
import app.pachli.appstore.UnfollowEvent import app.pachli.appstore.UnfollowEvent
import app.pachli.components.timeline.FilterKind import app.pachli.components.timeline.FilterKind
import app.pachli.components.timeline.FiltersRepository import app.pachli.components.timeline.FiltersRepository
import app.pachli.components.timeline.util.ifExpected
import app.pachli.core.accounts.AccountManager import app.pachli.core.accounts.AccountManager
import app.pachli.core.network.model.Filter import app.pachli.core.network.model.Filter
import app.pachli.core.network.model.FilterContext import app.pachli.core.network.model.FilterContext
@ -361,7 +360,7 @@ abstract class TimelineViewModel(
}.getOrThrow() }.getOrThrow()
uiSuccess.emit(StatusActionSuccess.from(action)) uiSuccess.emit(StatusActionSuccess.from(action))
} catch (e: Exception) { } catch (e: Exception) {
ifExpected(e) { _uiErrorChannel.send(UiError.make(e, action)) } _uiErrorChannel.send(UiError.make(e, action))
} }
} }
} }

View File

@ -38,6 +38,9 @@ dependencies {
// Uses HttpException from Retrofit // Uses HttpException from Retrofit
implementation(projects.core.network) implementation(projects.core.network)
// Uses JsonDataException from Moshi
implementation(libs.moshi)
// Some views inherit from AndroidX views // Some views inherit from AndroidX views
implementation(libs.bundles.androidx) implementation(libs.bundles.androidx)

View File

@ -20,6 +20,7 @@ package app.pachli.core.ui.extensions
import android.content.Context import android.content.Context
import app.pachli.core.network.extensions.getServerErrorMessage import app.pachli.core.network.extensions.getServerErrorMessage
import app.pachli.core.ui.R import app.pachli.core.ui.R
import com.squareup.moshi.JsonDataException
import java.io.IOException import java.io.IOException
import retrofit2.HttpException import retrofit2.HttpException
@ -40,5 +41,6 @@ fun Throwable.getDrawableRes(): Int = when (this) {
fun Throwable.getErrorString(context: Context): String = getServerErrorMessage() ?: when (this) { fun Throwable.getErrorString(context: Context): String = getServerErrorMessage() ?: when (this) {
is IOException -> context.getString(R.string.error_network_fmt, this.message) 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 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) else -> context.getString(R.string.error_generic_fmt, this.message)
} }

View File

@ -4,6 +4,7 @@
<string name="error_network_fmt">A network error occurred: %s</string> <string name="error_network_fmt">A network error occurred: %s</string>
<string name="error_generic_fmt">An error occurred: %s</string> <string name="error_generic_fmt">An error occurred: %s</string>
<string name="error_404_not_found_fmt">Your server does not support this feature: %1$s</string> <string name="error_404_not_found_fmt">Your server does not support this feature: %1$s</string>
<string name="error_json_data_fmt">Your server returned an invalid response: %1$s</string>
<string name="error_generic">An error occurred.</string> <string name="error_generic">An error occurred.</string>
<string name="message_empty">Nothing here.</string> <string name="message_empty">Nothing here.</string>
<string name="action_retry">Retry</string> <string name="action_retry">Retry</string>