Skip to content

Commit 05e0989

Browse files
adalpariclaude
adalpari
and
claude
committed
Fix bugs, thread safety issues, and performance in Subscribers tab
- Bug #1: Add error state with retry to detail Activities (blank screen on error) - Bug #2: Cancel previous load job on tab switch to prevent race conditions - Bug #3: Reset isLoadedSuccessfully in refresh() to allow data reload - Bug #4: Add duplicate guard in addCard() to prevent duplicate cards - Bug #9: Use resource string instead of raw e.message for error display - Thread safety #7: Use AtomicBoolean for isInitialLoad in SubscribersTabViewModel - Performance #11: Pre-compute formatted dates in ViewModel, memoize in card composable - Update all tests to match new error handling and API signatures Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent f67d459 commit 05e0989

File tree

15 files changed

+212
-53
lines changed

15 files changed

+212
-53
lines changed

‎WordPress/src/main/java/org/wordpress/android/ui/newstats/repository/SubscribersCardsConfigurationRepository.kt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ class SubscribersCardsConfigurationRepository @Inject constructor(
7272
cardType: SubscribersCardType
7373
): Unit = withContext(ioDispatcher) {
7474
val current = getConfiguration(siteId)
75+
if (cardType in current.visibleCards) return@withContext
7576
val newVisibleCards =
7677
current.visibleCards + cardType
7778
saveConfiguration(

‎WordPress/src/main/java/org/wordpress/android/ui/newstats/subscribers/BaseSubscribersCardViewModel.kt‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package org.wordpress.android.ui.newstats.subscribers
22

33
import androidx.lifecycle.ViewModel
44
import androidx.lifecycle.viewModelScope
5+
import kotlinx.coroutines.Job
56
import kotlinx.coroutines.flow.MutableStateFlow
67
import kotlinx.coroutines.flow.StateFlow
78
import kotlinx.coroutines.flow.asStateFlow
@@ -10,6 +11,7 @@ import org.wordpress.android.R
1011
import org.wordpress.android.fluxc.store.AccountStore
1112
import org.wordpress.android.ui.mysite.SelectedSiteRepository
1213
import org.wordpress.android.ui.newstats.repository.StatsRepository
14+
import org.wordpress.android.util.AppLog
1315
import org.wordpress.android.viewmodel.ResourceProvider
1416
import java.util.concurrent.atomic.AtomicBoolean
1517

@@ -25,17 +27,21 @@ abstract class BaseSubscribersCardViewModel<UiState : Any>(
2527
val uiState: StateFlow<UiState> = _uiState.asStateFlow()
2628

2729
private val _isRefreshing = MutableStateFlow(false)
28-
val isRefreshing: StateFlow<Boolean> = _isRefreshing.asStateFlow()
30+
val isRefreshing: StateFlow<Boolean> =
31+
_isRefreshing.asStateFlow()
2932

3033
private val isLoading = AtomicBoolean(false)
3134
private val isLoadedSuccessfully = AtomicBoolean(false)
35+
private var loadJob: Job? = null
3236

3337
protected abstract val loadingState: UiState
3438
protected abstract fun errorState(
3539
message: String,
3640
isAuthError: Boolean = false
3741
): UiState
38-
protected abstract suspend fun loadDataInternal(siteId: Long)
42+
protected abstract suspend fun loadDataInternal(
43+
siteId: Long
44+
)
3945

4046
fun loadDataIfNeeded() {
4147
if (isLoadedSuccessfully.get() ||
@@ -50,6 +56,7 @@ abstract class BaseSubscribersCardViewModel<UiState : Any>(
5056
val accessToken = accountStore.accessToken
5157
if (accessToken.isNullOrEmpty()) return
5258
statsRepository.init(accessToken)
59+
resetLoadedSuccessfully()
5360
viewModelScope.launch {
5461
try {
5562
_isRefreshing.value = true
@@ -90,7 +97,8 @@ abstract class BaseSubscribersCardViewModel<UiState : Any>(
9097
statsRepository.init(accessToken)
9198
updateState(loadingState)
9299

93-
viewModelScope.launch {
100+
loadJob?.cancel()
101+
loadJob = viewModelScope.launch {
94102
try {
95103
fetchData(site.siteId)
96104
} finally {
@@ -103,9 +111,14 @@ abstract class BaseSubscribersCardViewModel<UiState : Any>(
103111
try {
104112
loadDataInternal(siteId)
105113
} catch (e: Exception) {
114+
AppLog.e(
115+
AppLog.T.STATS,
116+
"Error loading stats data",
117+
e
118+
)
106119
updateState(
107120
errorState(
108-
e.message ?: resourceProvider.getString(
121+
resourceProvider.getString(
109122
R.string.stats_error_unknown
110123
)
111124
)

‎WordPress/src/main/java/org/wordpress/android/ui/newstats/subscribers/SubscribersTabViewModel.kt‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import kotlinx.coroutines.flow.StateFlow
88
import kotlinx.coroutines.flow.asStateFlow
99
import kotlinx.coroutines.launch
1010
import org.wordpress.android.ui.mysite.SelectedSiteRepository
11+
import java.util.concurrent.atomic.AtomicBoolean
1112
import org.wordpress.android.ui.newstats.repository.SubscribersCardsConfigurationRepository
1213
import org.wordpress.android.util.NetworkUtilsWrapper
1314
import javax.inject.Inject
@@ -41,7 +42,7 @@ class SubscribersTabViewModel @Inject constructor(
4142
val cardsToLoad: StateFlow<List<SubscribersCardType>> =
4243
_cardsToLoad.asStateFlow()
4344

44-
private var isInitialLoad = true
45+
private val isInitialLoad = AtomicBoolean(true)
4546

4647
private val siteId: Long
4748
get() = selectedSiteRepository
@@ -91,9 +92,8 @@ class SubscribersTabViewModel @Inject constructor(
9192
) {
9293
_visibleCards.value = config.visibleCards
9394
_hiddenCards.value = config.hiddenCards()
94-
if (isInitialLoad) {
95+
if (isInitialLoad.compareAndSet(true, false)) {
9596
_cardsToLoad.value = config.visibleCards
96-
isInitialLoad = false
9797
}
9898
}
9999

‎WordPress/src/main/java/org/wordpress/android/ui/newstats/subscribers/emails/EmailsDetailActivity.kt‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import android.content.Intent
55
import android.os.Bundle
66
import androidx.activity.compose.setContent
77
import androidx.compose.foundation.layout.Box
8+
import androidx.compose.foundation.layout.Column
89
import androidx.compose.foundation.layout.Row
910
import androidx.compose.foundation.layout.Spacer
1011
import androidx.compose.foundation.layout.fillMaxSize
@@ -18,6 +19,7 @@ import androidx.compose.foundation.lazy.itemsIndexed
1819
import androidx.compose.foundation.lazy.rememberLazyListState
1920
import androidx.compose.material.icons.Icons
2021
import androidx.compose.material.icons.automirrored.filled.ArrowBack
22+
import androidx.compose.material3.Button
2123
import androidx.compose.material3.CircularProgressIndicator
2224
import androidx.compose.material3.ExperimentalMaterial3Api
2325
import androidx.compose.material3.HorizontalDivider
@@ -86,6 +88,7 @@ private fun EmailsDetailScreen(
8688
viewModel.isLoadingMore.collectAsState()
8789
val canLoadMore by
8890
viewModel.canLoadMore.collectAsState()
91+
val hasError by viewModel.hasError.collectAsState()
8992

9093
val listState = rememberLazyListState()
9194

@@ -143,6 +146,13 @@ private fun EmailsDetailScreen(
143146
) {
144147
CircularProgressIndicator()
145148
}
149+
} else if (hasError) {
150+
ErrorContent(
151+
modifier = Modifier
152+
.fillMaxSize()
153+
.padding(contentPadding),
154+
onRetry = { viewModel.loadInitialPage() }
155+
)
146156
} else {
147157
LazyColumn(
148158
state = listState,
@@ -295,3 +305,37 @@ private fun DetailEmailRow(item: EmailListItem) {
295305
)
296306
}
297307
}
308+
309+
@Composable
310+
private fun ErrorContent(
311+
modifier: Modifier = Modifier,
312+
onRetry: () -> Unit
313+
) {
314+
Box(
315+
modifier = modifier,
316+
contentAlignment = Alignment.Center
317+
) {
318+
Column(
319+
horizontalAlignment =
320+
Alignment.CenterHorizontally
321+
) {
322+
Text(
323+
text = stringResource(
324+
R.string.stats_error_api
325+
),
326+
style = MaterialTheme
327+
.typography.bodyMedium,
328+
color = MaterialTheme
329+
.colorScheme.onSurfaceVariant
330+
)
331+
Spacer(modifier = Modifier.height(16.dp))
332+
Button(onClick = onRetry) {
333+
Text(
334+
text = stringResource(
335+
R.string.retry
336+
)
337+
)
338+
}
339+
}
340+
}
341+
}

‎WordPress/src/main/java/org/wordpress/android/ui/newstats/subscribers/emails/EmailsDetailViewModel.kt‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,10 @@ class EmailsDetailViewModel @Inject constructor(
4141
val canLoadMore: StateFlow<Boolean> =
4242
_canLoadMore.asStateFlow()
4343

44+
private val _hasError = MutableStateFlow(false)
45+
val hasError: StateFlow<Boolean> =
46+
_hasError.asStateFlow()
47+
4448
private var currentQuantity = 0
4549
private val paginationMutex = Mutex()
4650

@@ -50,6 +54,7 @@ class EmailsDetailViewModel @Inject constructor(
5054
if (_items.value.isNotEmpty()) return@launch
5155
currentQuantity = PAGE_SIZE
5256
_isLoading.value = true
57+
_hasError.value = false
5358
_canLoadMore.value = true
5459
fetchEmails(currentQuantity, isInitial = true)
5560
_isLoading.value = false
@@ -104,14 +109,20 @@ class EmailsDetailViewModel @Inject constructor(
104109
}
105110
is EmailsStatsResult.Error -> {
106111
if (isInitial) {
112+
_hasError.value = true
107113
_canLoadMore.value = false
108114
} else {
109115
currentQuantity -= PAGE_SIZE
110116
}
111117
}
112118
}
113119
} catch (_: Exception) {
114-
if (!isInitial) currentQuantity -= PAGE_SIZE
120+
if (isInitial) {
121+
_hasError.value = true
122+
_canLoadMore.value = false
123+
} else {
124+
currentQuantity -= PAGE_SIZE
125+
}
115126
}
116127
}
117128
}

‎WordPress/src/main/java/org/wordpress/android/ui/newstats/subscribers/subscriberslist/SubscribersListCard.kt‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import androidx.compose.foundation.layout.width
1111
import androidx.compose.material3.MaterialTheme
1212
import androidx.compose.material3.Text
1313
import androidx.compose.runtime.Composable
14+
import androidx.compose.runtime.remember
1415
import androidx.compose.ui.Alignment
1516
import androidx.compose.ui.platform.LocalContext
1617
import androidx.compose.ui.Modifier
@@ -229,11 +230,16 @@ private fun SubscriberItemRow(
229230
modifier = Modifier.weight(1f)
230231
)
231232
Spacer(modifier = Modifier.width(12.dp))
233+
val resources = LocalContext.current.resources
234+
val formattedDate = remember(
235+
item.subscribedSince
236+
) {
237+
formatSubscriberDate(
238+
item.subscribedSince, resources
239+
)
240+
}
232241
Text(
233-
text = formatSubscriberDate(
234-
item.subscribedSince,
235-
LocalContext.current.resources
236-
),
242+
text = formattedDate,
237243
style = MaterialTheme
238244
.typography.bodySmall,
239245
color = MaterialTheme

‎WordPress/src/main/java/org/wordpress/android/ui/newstats/subscribers/subscriberslist/SubscribersListDetailActivity.kt‎

Lines changed: 50 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import android.os.Bundle
66
import androidx.activity.compose.setContent
77
import androidx.compose.foundation.layout.Arrangement
88
import androidx.compose.foundation.layout.Box
9+
import androidx.compose.foundation.layout.Column
910
import androidx.compose.foundation.layout.Row
1011
import androidx.compose.foundation.layout.Spacer
1112
import androidx.compose.foundation.layout.fillMaxSize
@@ -19,6 +20,7 @@ import androidx.compose.foundation.lazy.itemsIndexed
1920
import androidx.compose.foundation.lazy.rememberLazyListState
2021
import androidx.compose.material.icons.Icons
2122
import androidx.compose.material.icons.automirrored.filled.ArrowBack
23+
import androidx.compose.material3.Button
2224
import androidx.compose.material3.CircularProgressIndicator
2325
import androidx.compose.material3.ExperimentalMaterial3Api
2426
import androidx.compose.material3.Icon
@@ -86,11 +88,13 @@ private fun SubscribersListDetailScreen(
8688
viewModel.isLoadingMore.collectAsState()
8789
val canLoadMore by
8890
viewModel.canLoadMore.collectAsState()
91+
val hasError by viewModel.hasError.collectAsState()
8992

93+
val resources = LocalContext.current.resources
9094
val listState = rememberLazyListState()
9195

9296
LaunchedEffect(Unit) {
93-
viewModel.loadInitialPage()
97+
viewModel.loadInitialPage(resources)
9498
}
9599

96100
val shouldLoadMore by remember {
@@ -108,7 +112,7 @@ private fun SubscribersListDetailScreen(
108112
}
109113

110114
LaunchedEffect(shouldLoadMore) {
111-
if (shouldLoadMore) viewModel.loadMore()
115+
if (shouldLoadMore) viewModel.loadMore(resources)
112116
}
113117

114118
val title = stringResource(
@@ -143,6 +147,15 @@ private fun SubscribersListDetailScreen(
143147
) {
144148
CircularProgressIndicator()
145149
}
150+
} else if (hasError) {
151+
ErrorContent(
152+
modifier = Modifier
153+
.fillMaxSize()
154+
.padding(contentPadding),
155+
onRetry = {
156+
viewModel.loadInitialPage(resources)
157+
}
158+
)
146159
} else {
147160
LazyColumn(
148161
state = listState,
@@ -252,14 +265,45 @@ private fun DetailSubscriberRow(
252265
)
253266
Spacer(modifier = Modifier.width(12.dp))
254267
Text(
255-
text = formatSubscriberDate(
256-
item.subscribedSince,
257-
LocalContext.current.resources
258-
),
268+
text = item.formattedDate,
259269
style = MaterialTheme
260270
.typography.bodySmall,
261271
color = MaterialTheme
262272
.colorScheme.onSurfaceVariant
263273
)
264274
}
265275
}
276+
277+
@Composable
278+
private fun ErrorContent(
279+
modifier: Modifier = Modifier,
280+
onRetry: () -> Unit
281+
) {
282+
Box(
283+
modifier = modifier,
284+
contentAlignment = Alignment.Center
285+
) {
286+
Column(
287+
horizontalAlignment =
288+
Alignment.CenterHorizontally
289+
) {
290+
Text(
291+
text = stringResource(
292+
R.string.stats_error_api
293+
),
294+
style = MaterialTheme
295+
.typography.bodyMedium,
296+
color = MaterialTheme
297+
.colorScheme.onSurfaceVariant
298+
)
299+
Spacer(modifier = Modifier.height(16.dp))
300+
Button(onClick = onRetry) {
301+
Text(
302+
text = stringResource(
303+
R.string.retry
304+
)
305+
)
306+
}
307+
}
308+
}
309+
}

0 commit comments

Comments
 (0)
⚡