Conversation
Generated by 🚫 Danger |
|
Outdated |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
WordPress/src/main/java/org/wordpress/android/ui/newstats/NewStatsActivity.kt
Outdated
Show resolved
Hide resolved
| saveConfiguration(siteId, current.copy(visibleCards = newVisibleCards)) | ||
| } | ||
|
|
||
| @Suppress("TooGenericExceptionCaught") |
There was a problem hiding this comment.
Consider catching specific exceptions instead of generic Exception
While the @Suppress makes the linter happy, catching all exceptions here could hide important bugs during development. Consider:
private fun loadConfiguration(siteId: Long): StatsCardsConfiguration {
val json = appPrefsWrapper.getStatsCardsConfigurationJson(siteId) ?: return StatsCardsConfiguration()
return try {
val config = gson.fromJson(json, StatsCardsConfiguration::class.java)
if (isValidConfiguration(config)) {
config
} else {
AppLog.w(AppLog.T.STATS, "Stats cards configuration contains invalid card types")
resetToDefault(siteId)
}
} catch (e: JsonSyntaxException) {
AppLog.e(AppLog.T.STATS, "Failed to parse stats cards configuration", e)
resetToDefault(siteId)
}
}This way, unexpected exceptions (like NPEs from bugs) won't be silently swallowed.
There was a problem hiding this comment.
This is working as expected because we want to reset to default, no matter the error type, to avoid configuration crashes. The code is properly logging the error
WordPress/src/main/java/org/wordpress/android/ui/newstats/components/StatsCardMenu.kt
Show resolved
Hide resolved
|
|
|
|
|
Claude finished @adalpari's task in 1m 41s —— View job Code Review CompleteI've conducted a comprehensive review of this PR. Overall, the implementation is well-structured with good test coverage. Here are my findings organized by category: ✅ Strengths1. Excellent Test Coverage
2. Code Organization
3. User Experience
🔍 Issues FoundCritical: Network Status Race ConditionFile: The retry logic has a race condition. Current implementation: onRetry = {
newStatsViewModel.checkNetworkStatus()
// Flow may not have updated yet when LaunchedEffect checks it
}Recommended fix: onRetry = {
newStatsViewModel.checkNetworkStatus()
showNoConnectionScreen = false // Trust the retry and show loading states
todaysStatsViewModel.loadData()
viewsStatsViewModel.loadData()
mostViewedViewModel.loadData()
countriesViewModel.loadData()
}Option 2 - Make fun checkNetworkStatus(): Boolean {
val isAvailable = networkUtilsWrapper.isNetworkAvailable()
_isNetworkAvailable.value = isAvailable
return isAvailable
}Accessibility: Missing Content DescriptionsFile: All four move action icons are missing Icon(
imageVector = Icons.Default.KeyboardArrowUp,
contentDescription = stringResource(R.string.stats_card_move_up) // Currently null
)This applies to all four icons: KeyboardArrowUp, VerticalAlignTop, KeyboardArrowDown, VerticalAlignBottom. Fix: Add proper content descriptions to all four Icon composables in the move submenu. Code Quality: Overly Broad Exception HandlingFile: While the Current: catch (e: Exception) {
AppLog.e(AppLog.T.STATS, "Failed to parse stats cards configuration, resetting to default", e)
resetToDefault(siteId)
}Recommended: catch (e: JsonSyntaxException) {
AppLog.e(AppLog.T.STATS, "Failed to parse stats cards configuration", e)
resetToDefault(siteId)
} catch (e: JsonParseException) {
AppLog.e(AppLog.T.STATS, "Failed to parse stats cards configuration", e)
resetToDefault(siteId)
}Note: I see the PR author commented that catching all exceptions is intentional to avoid configuration crashes. This is a valid design decision, though catching specific exceptions would be more defensive during development. 📝 Minor Observations1. Network State Management (NewStatsActivity.kt:274-289) 2. Card Position Memoization (NewStatsActivity.kt:344-348) 3. Move Logic Consolidation (StatsCardsConfigurationRepository.kt:91-101) ✨ Best Practices Followed
📊 SummaryMust Fix (2):
Should Consider (1):
Overall Assessment: The PR demonstrates solid engineering with good separation of concerns, comprehensive testing, and thoughtful UX. The critical issue with the network retry logic should be addressed before merging. Tasks
|
WordPress/src/main/java/org/wordpress/android/ui/newstats/NewStatsActivity.kt
Show resolved
Hide resolved
| Icon( | ||
| imageVector = Icons.Default.KeyboardArrowUp, | ||
| contentDescription = stringResource(R.string.stats_card_move_up) |
There was a problem hiding this comment.
Accessibility: Missing content description
All four move action icons should have content descriptions for screen reader users:
Icon(
imageVector = Icons.Default.KeyboardArrowUp,
contentDescription = stringResource(R.string.stats_card_move_up)
)This applies to all four icons in this submenu (lines 109, 128, 145, 163).
There was a problem hiding this comment.
False positive, already solved
False positive. This was already fixed... |
I already worked on it here, but I took the chance to make further improvements. |
|
NP: If there are only two stats cards, we probably shouldn't show both "Move up" and "Move to top" (likewise for down/bottom). 
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #22555 +/- ##
==========================================
- Coverage 38.64% 38.57% -0.07%
==========================================
Files 2225 2225
Lines 109329 109650 +321
Branches 15327 15349 +22
==========================================
+ Hits 42248 42297 +49
- Misses 63574 63844 +270
- Partials 3507 3509 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Good point! Done here |
|
4 participants
Description
This PR adds card position management functionality to stats cards, allowing users to reorder cards via a "Move Card" sub-menu. It also adds no-connection handling and displays
the selected period in the top bar.
Changes:
moveCardToIndexhelper method in repositoryTesting instructions
Screen_recording_20260203_131420.mp4
Move card functionality:
No-connection handling:
Period display in top bar:
Single card scenario: