CMM-1146 stats create cards handling#22552
Conversation
|
Outdated |
Generated by 🚫 Danger |
The user can only change the site by closing the screen. So, there's no sync problem. |
We don't want to test/handle those cases, because they are perfectly fine. |
It shows the card name, which is expected. It was previously a selector. We may just remove it. |
I've removed the cache now since it's not adding much value. |
|
Claude finished @adalpari's task in 2m 4s —— View job Code Review Complete ✅I've reviewed this PR implementing stats cards configuration handling. Overall, this is a well-structured implementation with good test coverage. Here are my findings: 🎯 Architecture & DesignStrengths:
Observations:
🔒 SecurityNo critical security issues found. The implementation properly:
⚡ PerformanceMinor Considerations:
🧪 Test CoverageExcellent test coverage! The test files cover:
📝 Code Quality Issues1. Potential Site ID Synchronization IssueFile: private val siteId: Long
get() = selectedSiteRepository.getSelectedSite()?.siteId ?: 0LIssue: While you mentioned users can only change sites by closing the screen, the Recommendation: Consider capturing siteId at operation start: fun removeCard(cardType: StatsCardType) {
viewModelScope.launch {
val currentSiteId = siteId
cardConfigurationRepository.removeCard(currentSiteId, cardType)
}
}Impact: Low - Current implementation is likely safe given usage patterns, but defensive coding would prevent future issues. 2. Missing Input ValidationFile: suspend fun addCard(siteId: Long, cardType: StatsCardType): Unit = withContext(ioDispatcher) {
val current = getConfiguration(siteId)
val newVisibleCards = current.visibleCards + cardType
saveConfiguration(siteId, current.copy(visibleCards = newVisibleCards))
}Issue: No check for duplicate cards. If Recommendation: Add duplicate check: suspend fun addCard(siteId: Long, cardType: StatsCardType): Unit = withContext(ioDispatcher) {
val current = getConfiguration(siteId)
if (cardType in current.visibleCards) return@withContext
val newVisibleCards = current.visibleCards + cardType
saveConfiguration(siteId, current.copy(visibleCards = newVisibleCards))
}Impact: Medium - Could lead to duplicate cards in the UI if the method is called multiple times. 3. Inconsistent Error HandlingFiles: Issue: The repository's Current flow: Recommendation: Since the repository already handles errors gracefully, the ViewModel's try-catch could be simplified or removed. The current approach works but is redundant. Impact: Low - No functional issue, just unnecessary complexity. 4. Card Order Not Preserved on AddFile: val newVisibleCards = current.visibleCards + cardTypeIssue: When a card is added back, it's appended to the end rather than being inserted at its original position (based on Recommendation: Consider inserting cards at their proper position: suspend fun addCard(siteId: Long, cardType: StatsCardType): Unit = withContext(ioDispatcher) {
val current = getConfiguration(siteId)
if (cardType in current.visibleCards) return@withContext
// Insert at proper position based on defaultOrder
val newVisibleCards = (current.visibleCards + cardType)
.sortedBy { it.defaultOrder }
saveConfiguration(siteId, current.copy(visibleCards = newVisibleCards))
}Impact: Low - UX preference; current behavior might be intentional to let users customize order. 5. Empty State AccessibilityFile: if (visibleCards.isEmpty()) {
Box(
modifier = Modifier
.fillMaxWidth()
.padding(32.dp),
contentAlignment = Alignment.Center
) {
Text(
text = stringResource(R.string.stats_no_cards_message),
style = MaterialTheme.typography.bodyLarge,
color = MaterialTheme.colorScheme.onSurfaceVariant
)
}
}Issue: The empty state text doesn't have a semantic role or content description for accessibility. Recommendation: Consider adding semantic properties: Text(
text = stringResource(R.string.stats_no_cards_message),
style = MaterialTheme.typography.bodyLarge,
color = MaterialTheme.colorScheme.onSurfaceVariant,
modifier = Modifier.semantics {
heading()
}
)Impact: Low - Accessibility improvement for screen readers. 6. Unused Configuration FieldsFile: val selectedPeriodType: String? = null,
val customPeriodStartDate: Long? = null,
val customPeriodEndDate: Long? = nullIssue: These fields are defined but never used in the current PR. They appear to be for future functionality. Recommendation: Either:
Impact: Very Low - Just code cleanliness; no functional issue. 🎨 UI/UX Observations
📦 DocumentationStrengths:
Suggestions:
✅ SummaryThis is a high-quality implementation with:
Priority Fixes:
Optional Improvements:
Great work on this feature! The testing and migration handling are particularly well done. |
This is expected |
|
I've now achieved some of the Claude's suggestions, and the PR is ready to be reviewed |
|
@adalpari This is a nitpick, but how about changing this message to simply, "No cards to display" (removing the part about tapping the button, since that seems unnecessary)? 
|
|
|
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #22552 +/- ##
==========================================
+ Coverage 38.62% 38.64% +0.01%
==========================================
Files 2219 2225 +6
Lines 109055 109361 +306
Branches 15293 15327 +34
==========================================
+ Hits 42125 42262 +137
- Misses 63433 63592 +159
- Partials 3497 3507 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sounds reasonable. This is how it looks now: 
|
|
|
Great progress! |
5 participants
Description
Implements stats cards configuration handling for the new stats screen. Users can now customize which
stats cards are visible by removing cards via menu and adding them back through a bottom sheet.
Configuration is persisted per-site using SharedPreferences.
NEXT: allow user to move cards and fix the individual "Retry" button
Key changes:
StatsCardsConfigurationRepositoryto manage per-site card visibility preferencesMOST_VIEWED_POSTS_AND_PAGES,MOST_VIEWED_REFERRERS) instead ofusing a single
MOST_VIEWEDtype with subtypes, simplifying the architectureAddStatsCardBottomSheetfor re-adding hidden cardsinvalid/removed card types
Testing instructions
Screen_recording_20260202_162331.mp4
Card removal:
Adding cards back:
Empty state:
Configuration persistence:
Multi-site configuration: