CMM-1145 create stats countries card #22546
Conversation
…-stats-Countries-card
|
OUTDATED |
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/countries/CountriesCard.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/newstats/countries/CountriesCard.kt
Outdated
Show resolved
Hide resolved
| ReferrersDataResult.Error("Unknown error") | ||
| } | ||
| } | ||
| return ReferrersDataResult.Error("Referrers API not available") |
There was a problem hiding this comment.
Unreachable code: This return statement is unreachable because the when expression above (lines 204-222) already returns in all branches. This line should be removed.
| val maxViews = state.countries.maxOfOrNull { it.views } ?: 1L | ||
| state.countries.forEachIndexed { index, country -> | ||
| val percentage = if (maxViews > 0) country.views.toFloat() / maxViews.toFloat() else 0f |
There was a problem hiding this comment.
Performance: maxOfOrNull is called inside the composable on every recomposition, scanning through all countries. Consider calculating maxViews in the ViewModel and including it in the UI state instead:
data class Loaded(
val countries: List<CountryItem>,
val maxCountryViews: Long, // Pre-calculated in ViewModel
// ...
)Note: The division by zero check is also redundant since maxViews is initialized with ?: 1L.
WordPress/src/main/java/org/wordpress/android/ui/newstats/countries/CountriesDetailActivity.kt
Outdated
Show resolved
Hide resolved
Project manifest changes for WordPressThe following changes in the --- ./build/reports/diff_manifest/WordPress/wordpressVanillaRelease/base_manifest.txt 2026-01-30 15:34:18.553867749 +0000
+++ ./build/reports/diff_manifest/WordPress/wordpressVanillaRelease/head_manifest.txt 2026-01-30 15:34:29.343894594 +0000
@@ -195,6 +195,10 @@
<activity
android:name="org.wordpress.android.ui.newstats.mostviewed.MostViewedDetailActivity"
android:exported="false"
+ android:theme="@style/WordPress.NoActionBar" />
+ <activity
+ android:name="org.wordpress.android.ui.newstats.countries.CountriesDetailActivity"
+ android:exported="false"
android:theme="@style/WordPress.NoActionBar" /> <!-- Account activities -->
<activity
android:name="org.wordpress.android.ui.main.MeActivity"Go to https://buildkite.com/automattic/wordpress-android/builds/24724/canvas?sid=019c0f84-4483-45fb-8732-af60f3e0109b, click on the |
Project manifest changes for WordPressThe following changes in the --- ./build/reports/diff_manifest/WordPress/jetpackVanillaRelease/base_manifest.txt 2026-01-30 15:34:05.262294710 +0000
+++ ./build/reports/diff_manifest/WordPress/jetpackVanillaRelease/head_manifest.txt 2026-01-30 15:34:15.572412575 +0000
@@ -389,6 +389,10 @@
<activity
android:name="org.wordpress.android.ui.newstats.mostviewed.MostViewedDetailActivity"
android:exported="false"
+ android:theme="@style/WordPress.NoActionBar" />
+ <activity
+ android:name="org.wordpress.android.ui.newstats.countries.CountriesDetailActivity"
+ android:exported="false"
android:theme="@style/WordPress.NoActionBar" /> <!-- Account activities -->
<activity
android:name="org.wordpress.android.ui.main.MeActivity"Go to https://buildkite.com/automattic/wordpress-android/builds/24724/canvas?sid=019c0f84-4484-4d67-bca8-0944accf9e75, click on the |
Project dependencies changeslist! Upgraded Dependencies
rs.wordpress.api:android:1134-6cdddac9b99642742ae89bb5833bbe2ed1c7cdd1, (changed from 1119-c0b6b2248a27094599f6411ee52e68e7baafc469)
rs.wordpress.api:kotlin:1134-6cdddac9b99642742ae89bb5833bbe2ed1c7cdd1, (changed from 1119-c0b6b2248a27094599f6411ee52e68e7baafc469)tree +--- project :libs:fluxc
-| \--- rs.wordpress.api:android:1119-c0b6b2248a27094599f6411ee52e68e7baafc469
-| +--- com.squareup.okhttp3:okhttp:4.12.0 -> 5.3.2 (*)
-| +--- com.squareup.okhttp3:okhttp-tls:4.12.0
-| | +--- com.squareup.okhttp3:okhttp:4.12.0 -> 5.3.2 (*)
-| | +--- com.squareup.okio:okio:3.6.0 -> 3.16.4 (*)
-| | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.8.21 -> 1.9.24 (*)
-| +--- net.java.dev.jna:jna:5.18.1
-| +--- rs.wordpress.api:kotlin:1119-c0b6b2248a27094599f6411ee52e68e7baafc469
-| | +--- com.squareup.okhttp3:okhttp:4.12.0 -> 5.3.2 (*)
-| | +--- com.squareup.okhttp3:okhttp-tls:4.12.0 (*)
-| | +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.10.2 (*)
-| | \--- org.jetbrains.kotlin:kotlin-stdlib:2.1.21 -> 2.2.21 (*)
-| \--- org.jetbrains.kotlin:kotlin-stdlib:2.1.21 -> 2.2.21 (*)
+| \--- rs.wordpress.api:android:1134-6cdddac9b99642742ae89bb5833bbe2ed1c7cdd1
+| +--- com.squareup.okhttp3:okhttp:4.12.0 -> 5.3.2 (*)
+| +--- com.squareup.okhttp3:okhttp-tls:4.12.0
+| | +--- com.squareup.okhttp3:okhttp:4.12.0 -> 5.3.2 (*)
+| | +--- com.squareup.okio:okio:3.6.0 -> 3.16.4 (*)
+| | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.8.21 -> 1.9.24 (*)
+| +--- net.java.dev.jna:jna:5.18.1
+| +--- rs.wordpress.api:kotlin:1134-6cdddac9b99642742ae89bb5833bbe2ed1c7cdd1
+| | +--- com.squareup.okhttp3:okhttp:4.12.0 -> 5.3.2 (*)
+| | +--- com.squareup.okhttp3:okhttp-tls:4.12.0 (*)
+| | +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.10.2 (*)
+| | \--- org.jetbrains.kotlin:kotlin-stdlib:2.1.21 -> 2.2.21 (*)
+| \--- org.jetbrains.kotlin:kotlin-stdlib:2.1.21 -> 2.2.21 (*)
-\--- rs.wordpress.api:android:1119-c0b6b2248a27094599f6411ee52e68e7baafc469 (*)
+\--- rs.wordpress.api:android:1134-6cdddac9b99642742ae89bb5833bbe2ed1c7cdd1 (*) |
|
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
WordPress/src/main/java/org/wordpress/android/ui/newstats/components/StatsSummaryCard.kt
Fixed
Show fixed
Hide fixed
|
|
|
Claude finished @adalpari's task in 1m 39s —— View job Code Review Complete ✓I've reviewed the Countries card implementation. Here's my comprehensive feedback: SummaryThis PR implements a well-structured Countries stats feature with good separation of concerns, proper state management, and comprehensive testing. However, there are several important issues to address related to security, code duplication, and architecture. Critical Issues1. WebView Security RiskLocation: The implementation enables JavaScript and loads content from an external CDN (
Recommendations:
High Priority Issues2. Code Duplication - Shared ComponentsLocation: Multiple files The map-related composables (
Recommendation: Confirm that other stats cards will use maps. If not, consider moving these components into the 3. Inefficient State CalculationLocation: The val percentage = if (state.maxViewsForBar > 0) {
country.views.toFloat() / state.maxViewsForBar.toFloat()
} else 0fImpact: This recalculates for each country row on every recomposition. Recommendation: Pre-calculate 4. Parcelable vs Serializable PerformanceLocation: The code uses Status: ✓ Already correct Medium Priority Issues5. Unnecessary Division-by-Zero CheckLocation: The check
Recommendation: Remove the conditional or document why it's needed. 6. Unreachable Code in Data SourceLocation: The return statement after the 7. Empty State Handling InconsistencyLocation: When Code Quality ObservationsStrengths
Areas for Improvement
Security Review✓ Input Validation: Country codes and names from API are handled safely Performance Review✓ Lazy Loading: List only shows 10 items in card, full list in detail screen Testing ReviewTest Coverage: Excellent ✓
Missing Tests
Architecture ReviewDesign Pattern Adherence ✓
Suggestions
Overall AssessmentCode Quality: 8/10 This is a solid implementation with good engineering practices. The main concerns are:
The comprehensive test coverage and clean architecture demonstrate good software craftsmanship. Once the security concerns are addressed, this will be production-ready. Recommendations Priority
|
This has already been fixed... |
This has already been fixed... |
This is also already done: 0fda461 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #22546 +/- ##
==========================================
- Coverage 38.73% 38.62% -0.11%
==========================================
Files 2211 2219 +8
Lines 108268 109055 +787
Branches 15228 15293 +65
==========================================
+ Hits 41937 42125 +188
- Misses 62842 63433 +591
- Partials 3489 3497 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
adalpari
requested review from
nbradbury
and removed request for
a team and
nbradbury
|
@adalpari When viewing country stats for a date that has no views, I see "Unknown error": 
Logcat shows "Error fetching country views: Unknown error," but the request succeeds with HTTP 200 and this response: |
Ahhhgggg, I forgot about that case! It will probably need a change in the RS library. Will work on it and ping you back. |
|
@nbradbury The bug should have been fixed now :) 
|
|
Description
Implements a new Countries stats card for the new stats feature. The card displays visitor statistics by country with an interactive map visualization and a list of top countries. Users can tap "Show All" to see the complete list of countries in a detail screen.
Key components added:
CountriesCard- Jetpack Compose UI with map visualization (WebView-based), country list with flags, loading shimmer, and error statesCountriesViewModel- Manages UI state, fetches country data via StatsRepository, handles period changes and refreshCountriesDetailActivity- Full-screen detail view showing all countries with the mapCountriesCardUiState- Sealed class representing Loading, Loaded, and Error statesStatsDataSourceandStatsRepositoryScreen_recording_20260130_114034.mp4
Testing instructions
Countries card loading and display:
Countries detail screen:
Error handling:
NOTE: I know I need to work in the per-card retry button :)
Period changes: