Skip to content

Conversation

@adalpari
Copy link
Contributor

@adalpari adalpari commented Jan 20, 2026

Description

Second part of #22496, this PR uses the new wordpress-rs stats endpoint to fetch today stats. NO UI CHANGES

Screenshot 2026-01-20 at 15 59 49

Testing instructions

Today's Stats Card displays correctly:

  1. Log in with a WordPress.com account that has a site with stats
  2. Navigate to the new stats screen
  • Verify the card loads with graph, views, visitors, likes, and comments counts

@adalpari adalpari requested a review from a team as a code owner January 20, 2026 14:45
@adalpari adalpari requested review from nbradbury and removed request for a team January 20, 2026 14:45
@dangermattic
Copy link
Collaborator

dangermattic commented Jan 20, 2026

2 Warnings
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

wordpress-lint = '2.2.0'
wordpress-persistent-edittext = '1.0.2'
wordpress-rs = 'trunk-21200d63735de310067d88c7dfb6080a2200f18e'
wordpress-rs = '1106-8dad0111fb97082cc2ab4280835d4e1e3167210a'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pointing a PR on the wp repo. But, the idea is to merge it today and point to trunk again

@wpmobilebot
Copy link
Contributor

Project dependencies changes

list
! Upgraded Dependencies
rs.wordpress.api:android:1106-8dad0111fb97082cc2ab4280835d4e1e3167210a, (changed from trunk-21200d63735de310067d88c7dfb6080a2200f18e)
rs.wordpress.api:kotlin:1106-8dad0111fb97082cc2ab4280835d4e1e3167210a, (changed from trunk-21200d63735de310067d88c7dfb6080a2200f18e)
tree
 +--- project :libs:fluxc
-|    \--- rs.wordpress.api:android:trunk-21200d63735de310067d88c7dfb6080a2200f18e
-|         +--- 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:trunk-21200d63735de310067d88c7dfb6080a2200f18e
-|         |    +--- 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:1106-8dad0111fb97082cc2ab4280835d4e1e3167210a
+|         +--- 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:1106-8dad0111fb97082cc2ab4280835d4e1e3167210a
+|         |    +--- 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:trunk-21200d63735de310067d88c7dfb6080a2200f18e (*)
+\--- rs.wordpress.api:android:1106-8dad0111fb97082cc2ab4280835d4e1e3167210a (*)

@nbradbury nbradbury self-assigned this Jan 20, 2026
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 20, 2026

App Icon📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress
FlavorJalapeno
Build TypeDebug
Versionpr22502-abefb77
Commitabefb77
Direct Downloadwordpress-prototype-build-pr22502-abefb77.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 20, 2026

App Icon📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack
FlavorJalapeno
Build TypeDebug
Versionpr22502-abefb77
Commitabefb77
Direct Downloadjetpack-prototype-build-pr22502-abefb77.apk
Note: Google Login is not supported on these builds.

@adalpari
Copy link
Contributor Author

@claude

@claude
Copy link
Contributor

claude bot commented Jan 20, 2026

Claude finished @adalpari's task in 1m 37s —— View job


Code Review Complete ✅

I've completed a comprehensive review of PR #22502. Overall, the implementation is solid with good test coverage. Below are my findings organized by category.

Todo List

  • Read and analyze all changed files
  • Review StatsRepository.kt (new file)
  • Review TodaysStatsViewModel.kt changes
  • Review StatsRepositoryTest.kt (new test file)
  • Review TodaysStatsViewModelTest.kt changes
  • Review dependency changes in libs.versions.toml
  • Provide comprehensive feedback

✅ Strengths

  1. Excellent Test Coverage: Both StatsRepositoryTest.kt and TodaysStatsViewModelTest.kt have comprehensive test cases covering success, error, and edge cases
  2. Clean Architecture: Good separation of concerns with Repository pattern and MVVM
  3. Proper Error Handling: Well-structured sealed classes for result types
  4. Thread Safety: Correct use of @Volatile for accessToken (StatsRepository.kt:44)
  5. Documentation: Good KDoc comments explaining functionality

🔴 Critical Issues

1. Potential Data Loss in Long-to-Int Conversion

📍 TodaysStatsViewModel.kt:116-119

TodayStatsData(
    views = result.aggregates.views.toInt(),
    visitors = result.aggregates.visitors.toInt(),
    likes = result.aggregates.likes.toInt(),
    comments = result.aggregates.comments.toInt()
)

Issue: Converting Long to Int can cause overflow if values exceed Int.MAX_VALUE (2,147,483,647). For high-traffic sites, views could exceed this.

Recommendation: Either keep as Long throughout or use .coerceIn(0, Int.MAX_VALUE) to prevent silent overflow.

Fix this →


⚠️ Medium Priority Issues

2. Hardcoded Date Format Locale

📍 StatsRepository.kt:52

private val dateFormat = SimpleDateFormat("yyyy-MM-dd", Locale.ROOT)

Issue: While Locale.ROOT is correct for API communication, this is a class-level property. If SimpleDateFormat instances are reused across threads (unlikely here due to coroutines, but architecturally fragile), this could cause issues.

Recommendation: Consider moving to a thread-safe alternative or document thread-safety assumptions.

3. Calendar Offset Logic Could Be Clearer

📍 StatsRepository.kt:127

calendar.add(Calendar.DAY_OF_YEAR, 1 - offsetDays)

Issue: The formula 1 - offsetDays is confusing:

  • offsetDays=0 → adds 1 day (tomorrow)
  • offsetDays=1 → adds 0 days (today)

Recommendation: Add a comment explaining why this adds 1 day, or refactor for clarity:

// API expects endDate to be one day after the target date for hourly queries
calendar.add(Calendar.DAY_OF_YEAR, 1 - offsetDays)

4. Inconsistent Error Handling Pattern

📍 TodaysStatsViewModel.kt:103-107

} catch (e: Exception) {
    _uiState.value = TodaysStatsCardUiState.Error(
        message = e.message ?: resourceProvider.getString(R.string.stats_todays_stats_unknown_error),
        onRetry = { loadData(forced = true) }
    )
}

Issue: Catching Exception is too broad. If there's an unexpected OutOfMemoryError or other critical error, it shouldn't be handled as a retryable stats error.

Recommendation: Catch specific exceptions (IOException, HttpException, etc.) or at minimum catch RuntimeException instead of Exception.

5. Missing Index Constant

📍 StatsRepository.kt:23-28

private const val INDEX_VIEWS = 1
private const val INDEX_VISITORS = 2
private const val INDEX_LIKES = 3
private const val INDEX_COMMENTS = 5  // Missing INDEX_REBLOGS = 4

Issue: INDEX_REBLOGS = 4 is not defined but referenced in comments. While not used in code, this creates confusion.

Recommendation: Add private const val INDEX_REBLOGS = 4 for completeness and documentation purposes.


💡 Minor Improvements

6. Suppress Annotation Could Be More Specific

📍 TodaysStatsViewModel.kt:73

@Suppress("TooGenericExceptionCaught", "UnusedParameter")

Issue: UnusedParameter is suppressed for forced parameter which is indeed unused.

Recommendation: Either use the forced parameter for cache invalidation logic, or remove it and document why it's not needed.

7. Fallback Date Parsing Could Be Simplified

📍 TodaysStatsViewModel.kt:158-177

The nested try-catch for date parsing is comprehensive but complex. Consider extracting to a separate function for testability.

8. Test Constants Could Be More Descriptive

📍 StatsRepositoryTest.kt:394-405

private const val TEST_VIEWS = 500L
private const val TEST_VISITORS = 100L

Suggestion: Consider using more realistic test values or add comments explaining why these specific values were chosen.

9. Unused Return Value

📍 TodaysStatsViewModel.kt:179-181

private fun onCardClicked() {
    // Navigation will be handled by the parent
}

Issue: This is a placeholder. Consider adding a TODO or removing if not yet implemented.


🔍 Security & Performance

10. Access Token Storage

The @Volatile annotation on accessToken is correct for thread visibility. The lazy initialization of wpComApiClient with null-check is appropriate.

11. Input Validation

Good defensive programming with:

  • Null checks for accessToken
  • Safe list access with getOrNull()
  • Type checking with is operator
  • Default values (0L) for invalid data

📊 Test Quality

Excellent Coverage in Both Test Files:

  • ✅ Success and error paths
  • ✅ Edge cases (null, empty, invalid data)
  • ✅ State transitions
  • ✅ Refresh behavior
  • ✅ Multiple offsetDays scenarios

Minor: Consider adding test for overflow scenario (Long to Int conversion mentioned above).


📦 Dependency Changes

gradle/libs.versions.toml:105

wordpress-rs = '1106-8dad0111fb97082cc2ab4280835d4e1e3167210a'

Note: As mentioned in your review comment, this points to a PR branch. ✅ Confirmed this will be updated to trunk before merge.


📝 Summary

Overall Assessment: Strong implementation with minor issues 🟢

The code demonstrates:

  • Good architectural patterns (Repository, MVVM, sealed classes)
  • Comprehensive test coverage
  • Proper error handling and logging
  • Clean, readable code

Recommended Actions Before Merge:

  1. Must Fix: Address Long-to-Int overflow issue (Menu Drawer Item refactor #1)
  2. Should Fix: Update wordpress-rs dependency to trunk (already planned)
  3. 💡 Consider: Improve exception handling specificity (Fix for bug #403 #4)
  4. 💡 Consider: Add clarity comment for calendar offset logic (WordPress.com Notifications in Android #3)

Great work on this implementation! The migration to wordpress-rs is clean and well-tested.

--- | Branch: feat/CMM-1142-stats-Create-Todays-card-rust

@codecov
Copy link

codecov bot commented Jan 20, 2026

Codecov Report

❌ Patch coverage is 90.20979% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.89%. Comparing base (5a02b7b) to head (abefb77).
⚠️ Report is 3 commits behind head on trunk.

Files with missing lines Patch % Lines
.../android/ui/newstats/todaysstat/StatsRepository.kt 87.50% 9 Missing and 4 partials ⚠️
...oid/ui/newstats/todaysstat/TodaysStatsViewModel.kt 97.14% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            trunk   #22502      +/-   ##
==========================================
+ Coverage   38.84%   38.89%   +0.04%     
==========================================
  Files        2193     2194       +1     
  Lines      105921   106024     +103     
  Branches    14982    14995      +13     
==========================================
+ Hits        41148    41240      +92     
- Misses      61333    61341       +8     
- Partials     3440     3443       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sonarqubecloud
Copy link

Copy link
Contributor

@nbradbury nbradbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! :shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants