Add StatsDataSource and refactor StatsRepository

[adalpari]

Description

This PR introduces a StatsDataSource abstraction layer to decouple StatsRepository from the
wordpress-rs uniffi objects, enabling proper unit testing. The refactoring extracts API communication
into a separate data source interface that can be mocked in tests.

Key changes:

• Add StatsDataSource interface with data models (StatsVisitsData, StatsUnit, etc.)
• Add StatsDataSourceImpl that wraps the wordpress-rs API client
• Refactor StatsRepository to use the new StatsDataSource interface
• Add StatsModule Dagger Hilt module to bind the implementation
• Add comprehensive unit tests for StatsRepository (25 test cases)
• Rename package todaysstat → todaysstats for correct plural naming

Testing instructions

Unit tests:

• Verify stats screen works.

[dangermattic]

	3 Warnings
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️	Class StatsDataSourceImpl is missing tests, but unit-tests-exemption label was set to ignore this.
⚠️	PR is not assigned to a milestone.

Generated by 🚫 Danger

[wpmobilebot]

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr22513-d93b3f7
	Commit	d93b3f7
	Direct Download	jetpack-prototype-build-pr22513-d93b3f7.apk

Note: Google Login is not supported on these builds.

[wpmobilebot]

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr22513-d93b3f7
	Commit	d93b3f7
	Direct Download	wordpress-prototype-build-pr22513-d93b3f7.apk

Note: Google Login is not supported on these builds.

[adalpari]

@claude

[claude]

Claude finished @adalpari's task in 2m 15s —— View job

---

Code Review Complete ✅

I've completed a comprehensive review of this PR. Overall, this is a well-structured refactoring that successfully introduces a testable abstraction layer. The code quality is high, with excellent documentation and comprehensive test coverage. Here are my findings:

---

✨ Strengths

1. Excellent Separation of Concerns: The StatsDataSource abstraction cleanly decouples the repository from the uniffi implementation, making the code much more testable.

2. Comprehensive Test Coverage: 25 well-organized test cases covering all major scenarios (success paths, error handling, edge cases, and parameter variations).

3. Clear Documentation: Every public interface, function, and complex logic is well-documented with KDoc comments.

4. Type Safety: Good use of sealed classes for result types, making error handling explicit and type-safe.

5. Proper Thread Safety: @Volatile annotation on accessToken with good documentation explaining the threading concerns.

---

🐛 Issues Found

1. Line Length Violation (Minor)

File: StatsRepositoryTest.kt:249

The test function name exceeds 120 characters:

fun `given successful response, when fetchDailyViewsForWeek is called, then data source is called with DAY unit`() =

Recommendation: Shorten to fit within 120 characters:

fun `given successful response, when fetchDailyViewsForWeek called, then uses DAY unit`() =

---

2. Incomplete Error Handling in StatsDataSourceImpl (Medium)

File: StatsDataSourceImpl.kt:69-71

else -> {
    StatsVisitsDataResult.Error("Unknown error")
}

The WpRequestResult sealed class likely has more cases than Success and WpError (e.g., NetworkError, HttpError). The else branch provides a generic "Unknown error" message that loses important error context.

Recommendation: Handle all WpRequestResult cases explicitly:

when (result) {
    is WpRequestResult.Success -> { /* ... */ }
    is WpRequestResult.WpError -> {
        StatsVisitsDataResult.Error(result.errorMessage)
    }
    is WpRequestResult.NetworkError -> {
        StatsVisitsDataResult.Error("Network error: ${result.message}")
    }
    is WpRequestResult.HttpError -> {
        StatsVisitsDataResult.Error("HTTP error ${result.code}: ${result.message}")
    }
    // Handle other specific cases
}

This provides better error messages for debugging and user feedback.

---

3. Potential Race Condition in Lazy Initialization (Low)

File: StatsDataSourceImpl.kt:30-33

private val wpComApiClient: WpComApiClient by lazy {
    check(accessToken != null) { "DataSource not initialized" }
    wpComApiClientProvider.getWpComApiClient(accessToken!!)
}

While accessToken is marked @Volatile, the lazy initialization itself is not thread-safe by default. If two threads call fetchStatsVisits simultaneously before initialization, there's a potential race condition.

Recommendation: Use lazy(LazyThreadSafetyMode.SYNCHRONIZED) explicitly:

private val wpComApiClient: WpComApiClient by lazy(LazyThreadSafetyMode.SYNCHRONIZED) {
    check(accessToken != null) { "DataSource not initialized" }
    wpComApiClientProvider.getWpComApiClient(accessToken!!)
}

Or alternatively, make wpComApiClient nullable and initialize it in init():

private var wpComApiClient: WpComApiClient? = null

override fun init(accessToken: String) {
    this.accessToken = accessToken
    this.wpComApiClient = wpComApiClientProvider.getWpComApiClient(accessToken)
}

---

4. Missing Test Coverage for StatsDataSourceImpl (Medium)

File: StatsDataSourceImpl.kt

As noted by Danger, StatsDataSourceImpl has no unit tests. While the unit-tests-exemption label was applied, this is a critical component that interfaces with external APIs and contains error handling logic.

Recommendation: Add tests for StatsDataSourceImpl covering:

• Successful API responses with data mapping
• Different error types from WpRequestResult
• Uninitialized data source handling
• Unit enum mapping (StatsUnit.HOUR ↔ StatsVisitsUnit.HOUR)

---

5. Inconsistent Null Checking (Low)

File: StatsDataSourceImpl.kt:45-47 and 30-33

The code checks accessToken == null in fetchStatsVisits() but also relies on the lazy wpComApiClient to check initialization. This creates redundant checks and potential confusion.

Recommendation: Choose one approach:

• Either check in fetchStatsVisits() and remove the check from lazy initialization
• Or rely solely on the lazy initialization check and remove the check from fetchStatsVisits()

I recommend the second approach for cleaner code:

override suspend fun fetchStatsVisits(...): StatsVisitsDataResult {
    // Remove this check since wpComApiClient.lazy will check it
    val params = StatsVisitsParams(...)
    val result = wpComApiClient.request { ... }
    // ...
}

---

💡 Suggestions for Improvement

1. Consider Adding Metrics for Empty Data (Minor)

File: StatsRepository.kt:66

When firstOrNull() returns null, you default to 0L. Consider logging or tracking metrics when this happens, as it might indicate API issues or data anomalies.

2. ThreadLocal DateFormat Documentation (Documentation)

File: StatsRepository.kt:36-38

The comment explains why ThreadLocal is used, which is excellent. Consider also mentioning that Locale.ROOT is used for consistent, locale-independent date formatting (important for API calls).

3. Test Organization (Minor)

File: StatsRepositoryTest.kt

The tests are well-organized with region comments. Consider extracting common test data into a TestDataFactory or similar pattern if you add more test classes that need similar data.

4. Package Naming Consistency (Positive Feedback)

The rename from todaysstat to todaysstats is a good catch for proper plural naming!

---

🔒 Security Review

✅ No security concerns identified:

• Input validation is delegated to the API client
• No SQL injection risks (using type-safe API calls)
• No XSS risks (backend data processing)
• Access tokens are properly scoped to the data source

---

⚡ Performance Review

✅ Performance looks good:

• Proper use of coroutines and IO dispatcher
• Efficient data aggregation (single API call for combined data)
• No N+1 query patterns
• ThreadLocal for date formatting reduces object allocation

One note: The calculateWeekDateRange() clones calendars. This is fine for current usage, but if called frequently, consider caching or reusing calendar instances.

---

📋 Testing Review

Test Coverage: Excellent ✅

• All public methods tested
• Success and error paths covered
• Edge cases (empty data) handled
• Parameter variations tested

Test Quality: High ✅

• Clear naming using backtick syntax
• Well-organized with region comments
• Proper use of AssertJ for readable assertions
• Good use of helper functions to reduce duplication

Missing: Unit tests for StatsDataSourceImpl (noted above)

---

📝 Summary

Category	Status	Notes
Code Quality	✅ Excellent	Clean architecture, well-documented
Security	✅ Good	No vulnerabilities found
Performance	✅ Good	Efficient use of coroutines and caching
Testing	⚠️ Good	Excellent repository tests, missing data source tests
Documentation	✅ Excellent	Comprehensive KDoc comments

Recommendation: Approve with minor improvements

The refactoring successfully achieves its goals. The identified issues are mostly minor and can be addressed in follow-up commits. The only medium-priority items are:

1. Adding tests for StatsDataSourceImpl
2. Improving error handling to avoid the generic "Unknown error" case

Great work on this refactoring! The abstraction layer will make future testing much easier.

---

[sonarqubecloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
6.1% Duplication on New Code

See analysis details on SonarQube Cloud

[codecov]

Codecov Report

❌ Patch coverage is 72.80000% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.83%. Comparing base (ad31d1f) to head (d93b3f7).
⚠️ Report is 10 commits behind head on trunk.

Files with missing lines	Patch %	Lines
...roid/ui/newstats/datasource/StatsDataSourceImpl.kt	0.00%	34 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk   #22513      +/-   ##
==========================================
+ Coverage   38.67%   38.83%   +0.15%     
==========================================
  Files        2199     2201       +2     
  Lines      106874   106879       +5     
  Branches    15062    15055       -7     
==========================================
+ Hits        41337    41507     +170     
+ Misses      62094    61929     -165     
  Partials     3443     3443              

☔ 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.

[adalpari]

@nbradbury I forgot to set this PR to "Ready to review" 😓

[nbradbury]

@adalpari This looks good and I'll approve it, but I'm not sure if you want to address anything mentioned by Claude before merging

[adalpari]

@adalpari This looks good and I'll approve it, but I'm not sure if you want to address anything mentioned by Claude before merging

Thank you. Yes, I did achieve some of its concerns, and I keep working on the code in the next iteration PR