fix: render error UI for TopicUiState.Error instead of crashing#2109
fix: render error UI for TopicUiState.Error instead of crashing#2109Noahs212 wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request implements the error state for the Topic screen by adding a new error message string, a TopicErrorBody composable, and updating the UI logic to handle TopicUiState.Error. It also includes a new UI test to verify the error message display. Feedback highlights that the topicItemsSize calculation for the success state may be incomplete, potentially affecting scrollbar accuracy, and suggests adding a modifier parameter to the TopicErrorBody composable to follow Compose best practices.
| newsUiState: NewsUiState, | ||
| ) = when (topicUiState) { | ||
| TopicUiState.Error -> 0 // Nothing | ||
| TopicUiState.Error -> 1 // Error message |
There was a problem hiding this comment.
While updating this to 1 is correct for the new error state, the existing logic for TopicUiState.Success (lines 185-186) appears to be incorrect as it doesn't account for the Toolbar and Header items which are always rendered in that state. This discrepancy will cause the scrollbar to be incorrectly sized or hidden when news is loading or has an error. Consider updating the entire function to ensure the scrollbar accurately reflects the number of items in the LazyColumn.
| private fun TopicErrorBody() { | ||
| Column( | ||
| horizontalAlignment = Alignment.CenterHorizontally, | ||
| modifier = Modifier |
There was a problem hiding this comment.
It is a best practice in Compose to have a modifier parameter for all composables to allow for layout adjustments from the caller. This also ensures that the internal Column uses the passed-in modifier as its base.
| private fun TopicErrorBody() { | |
| Column( | |
| horizontalAlignment = Alignment.CenterHorizontally, | |
| modifier = Modifier | |
| private fun TopicErrorBody(modifier: Modifier = Modifier) { | |
| Column( | |
| horizontalAlignment = Alignment.CenterHorizontally, | |
| modifier = modifier |
8d57c33 to
45cb357
Compare
45cb357 to
c160d18
Compare
Summary
TopicUiState.Error -> TODO()(TopicScreen.kt:132) with a localized error message.TODO()throwsNotImplementedErrorat runtime, so the topic detail screen currently crashes whenever the upstream topic flow emits an error (Result.ErrorfromasResult).feature_topic_api_error_headerstring and a privateTopicErrorBodycomposable modeled onSearchNotReadyBody.topicItemsSizeso the scrollbar item count matches the rendered error item (0 -> 1).errorMessage_whenTopicIsError_isShowntoTopicScreenTest. This test fails onmainwithNotImplementedErrorand passes after the fix.The other two TODOs in the same file (hardcoded
"Loading news"at L249 andText("Error")at L253) are in theNewsUiStatebranch and intentionally out of scope to keep this diff small.Fixes #2108
Test plan
./gradlew :feature:topic:impl:connectedDemoDebugAndroidTest --tests "*TopicScreenTest*"./gradlew :feature:topic:impl:testDemoDebugUnitTest./gradlew spotlessApply./gradlew :feature:topic:impl:lintDemoDebug :feature:topic:api:lintDemoDebug