Conversation
|
Claude finished @adalpari's task in 2m 28s —— View job Code Review
Overall this is a solid implementation that follows the existing architecture patterns well. The code is clean and the test coverage is reasonable. A few items worth addressing: Issues1. Unnecessary computation in repository — previous date range discarded In suspend fun fetchDevicesScreensize(siteId: Long, period: StatsPeriod): DevicesResult =
withContext(ioDispatcher) {
val (currentDateRange, _) = calculateComparisonDateRanges(period) // _ is wasted
fetchDevicesData { statsDataSource.fetchDevicesScreensize(siteId, currentDateRange) }
}
2. Double item cap at 10 Items are capped at 10 in two separate places:
The 3. ViewModel fetch methods are nearly identical — code duplication
private suspend fun fetchAndUpdate(
stateFlow: MutableStateFlow<DevicesCardUiState>,
setPeriod: () -> Unit,
fetch: suspend () -> DevicesResult
) {
try {
when (val result = fetch()) {
is DevicesResult.Success -> {
setPeriod()
stateFlow.value = mapToLoadedState(result)
}
is DevicesResult.Error -> stateFlow.value = DevicesCardUiState.Error(...)
}
} catch (e: Exception) { ... }
}4. Missing test: exception path in ViewModel The three 5. Missing test: item truncation at There is no test verifying that more than 10 items are truncated to 10 in Observations / Minor Points6.
7. Accessibility description lacks units In val description = entries.joinToString(", ") {
"${it.label}: ${it.value.toLong()}"
}This produces e.g. 8.
val maxViewsForBar = cardItems.firstOrNull()?.views ?: 0LThis implicitly assumes items arrive sorted descending. The sort is done in the repository, which is correct, but this assumption is unguarded and undocumented. If the sorting in the repository were ever changed or bypassed (e.g., in tests), this would silently produce incorrect bar sizing. A comment or an explicit 9. In SummaryThe implementation is well-structured and consistent with the codebase. The main actionable items are:
|
Generated by 🚫 Danger |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
WordPress/src/main/java/org/wordpress/android/ui/newstats/repository/StatsRepository.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/newstats/devices/DevicesViewModel.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/newstats/devices/DevicesViewModel.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/newstats/devices/DevicesViewModel.kt
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/newstats/devices/StatsRingChart.kt
Outdated
Show resolved
Hide resolved
Project dependencies changeslist! Upgraded Dependencies
rs.wordpress.api:android:1179-5909d11d98d0438c6ee896a45e3fab3720b395a1, (changed from trunk-9edcee430afd18d7d440baf497a763f9b1bb83d9)
rs.wordpress.api:kotlin:1179-5909d11d98d0438c6ee896a45e3fab3720b395a1, (changed from trunk-9edcee430afd18d7d440baf497a763f9b1bb83d9)tree +--- project :libs:fluxc
-| \--- rs.wordpress.api:android:trunk-9edcee430afd18d7d440baf497a763f9b1bb83d9
-| +--- com.squareup.okhttp3:okhttp:5.3.2 (*)
-| +--- com.squareup.okhttp3:okhttp-tls:5.3.2
-| | +--- com.squareup.okhttp3:okhttp:5.3.2 (*)
-| | +--- com.squareup.okio:okio:3.16.4 (*)
-| | \--- org.jetbrains.kotlin:kotlin-stdlib:2.2.21 -> 2.3.10 (*)
-| +--- net.java.dev.jna:jna:5.18.1
-| +--- rs.wordpress.api:kotlin:trunk-9edcee430afd18d7d440baf497a763f9b1bb83d9
-| | +--- com.squareup.okhttp3:okhttp:5.3.2 (*)
-| | +--- com.squareup.okhttp3:okhttp-tls:5.3.2 (*)
-| | +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.10.2 (*)
-| | \--- org.jetbrains.kotlin:kotlin-stdlib:2.1.21 -> 2.3.10 (*)
-| \--- org.jetbrains.kotlin:kotlin-stdlib:2.1.21 -> 2.3.10 (*)
+| \--- rs.wordpress.api:android:1179-5909d11d98d0438c6ee896a45e3fab3720b395a1
+| +--- com.squareup.okhttp3:okhttp:5.3.2 (*)
+| +--- com.squareup.okhttp3:okhttp-tls:5.3.2
+| | +--- com.squareup.okhttp3:okhttp:5.3.2 (*)
+| | +--- com.squareup.okio:okio:3.16.4 (*)
+| | \--- org.jetbrains.kotlin:kotlin-stdlib:2.2.21 -> 2.3.10 (*)
+| +--- net.java.dev.jna:jna:5.18.1
+| +--- rs.wordpress.api:kotlin:1179-5909d11d98d0438c6ee896a45e3fab3720b395a1
+| | +--- com.squareup.okhttp3:okhttp:5.3.2 (*)
+| | +--- com.squareup.okhttp3:okhttp-tls:5.3.2 (*)
+| | +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.10.2 (*)
+| | \--- org.jetbrains.kotlin:kotlin-stdlib:2.1.21 -> 2.3.10 (*)
+| \--- org.jetbrains.kotlin:kotlin-stdlib:2.1.21 -> 2.3.10 (*)
-\--- rs.wordpress.api:android:trunk-9edcee430afd18d7d440baf497a763f9b1bb83d9 (*)
+\--- rs.wordpress.api:android:1179-5909d11d98d0438c6ee896a45e3fab3720b395a1 (*) |
|
|
|
|
The API is not always following the 10 items cap. So, this is necessary |
gradle/libs.versions.toml
Outdated
| wordpress-lint = '2.2.0' | ||
| wordpress-persistent-edittext = '1.0.2' | ||
| wordpress-rs = 'trunk-9edcee430afd18d7d440baf497a763f9b1bb83d9' | ||
| wordpress-rs = '1179-e4582399b9d0c8735d2ddb16894e94668781927f' |
There was a problem hiding this comment.
Note: The PR won't pass until this is reverted to trunk
|
@adalpari When there's no data in the response the devices cards shows an error message. 
|
|
@adalpari The UI is showing only two items for the screen sizes, but the response contains three items. 
|
|
@adalpari When using a larger font size, "Screen size" looks a little too tight. 
Maybe just use "Size" like we do on the web? 
|
|
@nbradbury thank you for the checks!
|
|
Fixed the empty results case So, @nbradbury, can you take another look? In other notes, I noticed that the webapp is not always skipping the empty values. So, I've made the Android app to always show them to be consiostent. Two different sites in the webapp: 
|
Adds screen size, browser, and platform device stats cards with ring chart visualization. Also includes clicks, search terms, video plays, and file downloads stats cards.
adalpari
force-pushed
the
feat/CMM-1267-stats-devices-card
branch
from
e8703ef to
a5ca3b3
Compare
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #22620 +/- ##
==========================================
- Coverage 38.24% 38.20% -0.05%
==========================================
Files 2259 2263 +4
Lines 114979 115584 +605
Branches 15980 16041 +61
==========================================
+ Hits 43978 44157 +179
- Misses 67384 67802 +418
- Partials 3617 3625 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4 participants
Description
Add a new "Devices" card to the new stats screen that displays device statistics (screen size, browser, and platform) using a ring (donut) chart and a sorted list. The card
includes a segmented selector to switch between the three device categories. Items with zero views are filtered out.
The implementation follows the same architecture as the Locations card but is simpler — no detail screen ("Show All") and no previous-period comparison.
Changes
fetchDevicesScreensize,fetchDevicesBrowser,fetchDevicesPlatformtoStatsDataSourceinterface andStatsDataSourceImpl, using the newStatsDevicesParams/StatsDevicesPerioduniffi typesDevicesCardUiState(Loading/Loaded/Error),DeviceTypeenum,DeviceItemdata classDevicesViewModelwith per-type lazy loading, period tracking, and refresh supportStatsRingChart— Canvas-based donut chart with proportional arcs and accessibility supportDevicesCardcomposable with segmented type selector, ring chart, color-coded device list, shimmer loading state, and card management actionsDEVICEStoStatsCardTypeenum and integrated intoNewStatsActivityTesting instructions
Devices card on new stats screen:
Verify the card displays with the "Screen size" tab selected by default
Verify a ring (donut) chart is shown with color-coded segments
Verify a list of devices is shown below the chart with color dots, names, and view counts
Verify Browser and Platform tab works as well
Screen_recording_20260223_145007.mp4