Fix/stats cards and networking

[adalpari]

Description

Prior to this PR, all card ViewModels loaded data unconditionally on creation regardless of the user's card configuration, wasting network bandwidth. This PR gates all API-triggering calls (period changes, network reconnection, retry, pull-to-refresh) with visibility checks so hidden cards never fetch data.

In addition a duplication call issue has been solved. There was a race condition where te cards were calling the endpoints twice (2x" because of default config loading. Now we are only calling each endpoint once: one for the current period, and one for the previous one to compate data.

Key changes:

• Removed init { loadData() } from card ViewModels (TodaysStats, ViewsStats, MostViewed, Countries, Authors) — data loading is now driven by the composable
• Added cardsToLoad flow to NewStatsViewModel that starts empty and only populates after the real configuration loads, preventing premature fetches with the default card
  set
• Added isPeriodInitialized to ViewsStatsViewModel to prevent double-fetching when the persisted period is restored asynchronously
• Added loadDataIfNeeded() idempotent methods to TodaysStatsViewModel and ViewsStatsViewModel
• Extracted dispatchToVisibleCards helper and loadVisibleCards lambda to deduplicate the repeated visibility-gated dispatch logic across 4 call sites

Testing instructions

Only visible cards should trigger API calls:

1. Open the new stats screen with all cards visible

• Verify all cards load data normally

2. Remove a few card
3. Monitor network calls

• Verify only the visible cards endpoints are called (current + previous period = 2 calls)

• Verify no other card endpoints are called

• some test card changes and verify they work as expected

[adalpari]

@claude

[claude]

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

---

Code Review Complete

I've completed a comprehensive review of this PR. Overall, the implementation effectively solves the duplicate API calls and unnecessary data fetching issues. The approach is sound, but I've identified several areas for improvement.

---

✅ Strengths

1. Clean Architecture: The cardsToLoad flow pattern elegantly prevents premature fetching before configuration loads
2. Idempotent Loading: loadDataIfNeeded() methods prevent duplicate loads effectively
3. Comprehensive Testing: Good test coverage for the new behavior, including edge cases
4. Clear Separation: Visibility-gated dispatch logic is well-extracted and reusable

---

🔍 Issues Found

1. Critical: Race Condition in ViewsStatsViewModel (ViewsStatsViewModel.kt:101-105)

The loadDataIfNeeded() implementation has a subtle race condition:

fun loadDataIfNeeded() {
    if (loadedPeriod == currentPeriod) return
    loadedPeriod = currentPeriod
    loadData()
}

Problem: If loadData() is called before the previous call completes, loadedPeriod is set immediately but the actual data load is asynchronous. This means:

• Call 1: loadedPeriod = Last7Days, starts async load
• User changes period to Last30Days
• Call 2: loadedPeriod = Last30Days, starts async load
• User changes back to Last7Days
• Call 3: Sees loadedPeriod == Last7Days, returns early (skips load even though it never completed)

Solution: Track loading state properly:

private var loadingPeriod: StatsPeriod? = null
private var loadedPeriod: StatsPeriod? = null

fun loadDataIfNeeded() {
    val targetPeriod = currentPeriod
    if (loadedPeriod == targetPeriod || loadingPeriod == targetPeriod) return
    loadingPeriod = targetPeriod
    loadData()
}

private suspend fun loadDataInternal(site: SiteModel) {
    try {
        // ... existing code ...
        loadedPeriod = currentPeriod
    } finally {
        loadingPeriod = null
    }
}

Fix this →

---

2. Bug: TodaysStatsViewModel Doesn't Update on Config Changes (TodaysStatsViewModel.kt:42-46)

Unlike other ViewModels, TodaysStatsViewModel has no dependency on period changes, yet uses the same isInitialLoadDone flag pattern. This means:

fun loadDataIfNeeded() {
    if (isInitialLoadDone) return  // ❌ Always returns after first load
    isInitialLoadDone = true
    loadData()
}

Problem: If a user hides the "Today's Stats" card and then shows it again later, loadDataIfNeeded() will be called but will return early because isInitialLoadDone = true. The card won't refresh with new data.

Solution: Either:

1. Track whether data is currently loaded and reset the flag when the card is hidden, OR
2. Make loadDataIfNeeded() always load if no data exists:

private var hasLoadedData = false

fun loadDataIfNeeded() {
    if (hasLoadedData && _uiState.value is TodaysStatsCardUiState.Loaded) return
    hasLoadedData = true
    loadData()
}

Fix this →

---

3. Potential Memory Leak: Unconstrained Flow Collection (NewStatsViewModel.kt:66-74)

The observeConfigurationChanges() function launches a coroutine that collects a flow indefinitely:

private fun observeConfigurationChanges() {
    viewModelScope.launch {
        cardConfigurationRepository.configurationFlow.collect { pair ->
            if (pair != null && pair.first == siteId) {
                updateFromConfiguration(pair.second)
            }
        }
    }
}

Problem: siteId is a computed property that can change, but the flow collection captures it at launch time. If the user switches sites:

1. The collector keeps running with the old site ID
2. A new collector is NOT started (this is only called in init)
3. The ViewModel won't respond to configuration changes for the new site

Solution: Use distinctUntilChangedBy or restart collection on site changes, OR filter dynamically:

private fun observeConfigurationChanges() {
    viewModelScope.launch {
        cardConfigurationRepository.configurationFlow.collect { pair ->
            val currentSiteId = selectedSiteRepository.getSelectedSite()?.siteId ?: 0L
            if (pair != null && pair.first == currentSiteId) {
                updateFromConfiguration(pair.second)
            }
        }
    }
}

Fix this →

---

4. Code Quality: Inconsistent loadDataIfNeeded() Implementations

Three different patterns are used across ViewModels:

Pattern A (TodaysStatsViewModel):

private var isInitialLoadDone = false
fun loadDataIfNeeded() {
    if (isInitialLoadDone) return
    isInitialLoadDone = true
    loadData()
}

Pattern B (ViewsStatsViewModel):

private var loadedPeriod: StatsPeriod? = null
fun loadDataIfNeeded() {
    if (loadedPeriod == currentPeriod) return
    loadedPeriod = currentPeriod
    loadData()
}

Pattern C (MostViewedViewModel, CountriesViewModel, AuthorsViewModel):

private var isInitialLoadDone = false
fun onPeriodChanged(period: StatsPeriod) {
    if (isInitialLoadDone && currentPeriod == period) return
    isInitialLoadDone = true
    currentPeriod = period
    loadData()
}

Recommendation: Standardize on a single pattern. Pattern B is most correct for period-based cards, but needs the race condition fix. Pattern A needs to be fixed per issue #2.

---

5. Performance: Multiple Parallel Loads in MostViewedViewModel (MostViewedViewModel.kt:171-178)

private suspend fun loadDataInternal(siteId: Long) {
    // Load both data sources in parallel
    viewModelScope.launch {
        loadDataForSourceInternal(siteId, MostViewedDataSource.POSTS_AND_PAGES)
    }
    viewModelScope.launch {
        loadDataForSourceInternal(siteId, MostViewedDataSource.REFERRERS)
    }
}

Issue: Launching with viewModelScope inside a suspend function is unusual. The function returns immediately without waiting for completion, which could cause issues:

• isRefreshing flag is set to false before loads complete
• No error propagation from the inner coroutines

Better approach:

private suspend fun loadDataInternal(siteId: Long) = coroutineScope {
    launch { loadDataForSourceInternal(siteId, MostViewedDataSource.POSTS_AND_PAGES) }
    launch { loadDataForSourceInternal(siteId, MostViewedDataSource.REFERRERS) }
}

Fix this →

---

💡 Suggestions

6. Testing Gap: Missing Test for cardsToLoad Preventing Premature Fetch

The core bug fix (preventing default card fetches before config loads) lacks a specific test. Add:

@Test
fun `when ViewModel is created, cardsToLoad starts empty`() = test {
    // Don't call initViewModel() to avoid the config load
    viewModel = NewStatsViewModel(
        selectedSiteRepository,
        cardConfigurationRepository,
        networkUtilsWrapper
    )
    
    // Should start empty to prevent premature fetches
    assertThat(viewModel.cardsToLoad.value).isEmpty()
}

7. Consider: Extract Visibility Dispatch Pattern

The dispatchToVisibleCards extension function is excellent, but it's used in 4 places with different lambda bodies. Consider a more reusable approach:

sealed class CardAction {
    object Load : CardAction()
    object Refresh : CardAction()
    data class PeriodChanged(val period: StatsPeriod) : CardAction()
}

fun List<StatsCardType>.dispatchToVisibleCards(
    action: CardAction,
    todaysStatsViewModel: TodaysStatsViewModel,
    viewsStatsViewModel: ViewsStatsViewModel,
    // ... other ViewModels
) {
    when (action) {
        is CardAction.Load -> {
            if (TODAYS_STATS in this) todaysStatsViewModel.loadDataIfNeeded()
            // ...
        }
        is CardAction.Refresh -> {
            if (TODAYS_STATS in this) todaysStatsViewModel.refresh()
            // ...
        }
        is CardAction.PeriodChanged -> {
            // Only period-sensitive cards
            if (VIEWS_STATS in this) viewsStatsViewModel.loadDataIfNeeded()
            // ...
        }
    }
}

---

📊 Summary

Critical Issues: 3 (race condition, reload bug, flow collection bug)
Performance Issues: 1 (coroutine scope usage)
Code Quality: 2 (inconsistent patterns, missing test)

Recommendation: Address critical issues #1-3 before merging. Issues #4-5 can be addressed in a follow-up if time-constrained.

---

[dangermattic]

	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

[adalpari]

2. Bug: TodaysStatsViewModel Doesn't Update on Config Changes (TodaysStatsViewModel.kt:42-46)

This is correct. Today's stats card will always show Today's period. So, no need to refresh

[wpmobilebot]

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

	App Name	Jetpack Android
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr22586-296eb23
	Build Number	1484
	Application ID	com.jetpack.android.prealpha
	Commit	296eb23
	Installation URL	3c95ama6fd2t8

Note: Google Login is not supported on these builds.

[wpmobilebot]

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

	App Name	WordPress Android
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr22586-296eb23
	Build Number	1484
	Application ID	org.wordpress.android.prealpha
	Commit	296eb23
	Installation URL	234sdmnjlkhg0

Note: Google Login is not supported on these builds.

[adalpari]

@claude

[claude]

Claude finished @adalpari's task in 3m 58s —— View job

---

Code Review

• Gather context and understand the PR
• Read changed files
• Analyze code for issues
• Post comprehensive review

---

Overview

The PR's core approach is sound: gating all data-fetching calls behind visibility checks and a cardsToLoad flow that starts empty prevents both premature fetches and duplicate API calls. The previous review flagged several issues; most are now addressed. A few smaller items remain.

---

✅ Improvements Since Previous Review

1. Race condition in ViewsStatsViewModel fixed — loadDataIfNeeded() now correctly uses dual loadingPeriod/loadedPeriod tracking (ViewsStatsViewModel.kt:103-108), with loadingPeriod reset in the finally block and loadedPeriod only set on success.

2. coroutineScope in MostViewedViewModel fixed — loadDataInternal now correctly uses coroutineScope { launch {...} launch {...} } (MostViewedViewModel.kt:183-190), so the function waits for both child coroutines before returning.

3. observeConfigurationChanges siteId capture fixed — NewStatsViewModel.kt:69 now fetches siteId dynamically inside the collector, not at launch time.

4. TodaysStatsViewModel idempotent loading — The isLoading/isLoadedSuccessfully dual-flag pattern (TodaysStatsViewModel.kt:40-47) is cleaner than the previous isInitialLoadDone approach and correctly handles the "don't re-fetch if already loaded" requirement.

---

🔍 Issues Found

1. MostViewedViewModel.loadData() marks period as loaded even when one source fails

In MostViewedViewModel.kt:137-144:

viewModelScope.launch {
    try {
        loadDataInternal(site.siteId)
        loadedPeriod = currentPeriod  // set after both coroutines complete
    } finally {
        loadingPeriod = null
    }
}

loadDataForSourceInternal swallows all exceptions internally and transitions to an error state without propagating. So loadDataInternal always completes successfully from the try/catch perspective, meaning loadedPeriod = currentPeriod is set even if REFERRERS loaded successfully but POSTS_AND_PAGES failed.

A subsequent onPeriodChanged(samePeriod) will skip loading because loadedPeriod == period, leaving the POSTS_AND_PAGES card stuck in the error state until the user manually taps retry.

Individual retry (onRetryPosts/onRetryReferrers) works around this, but it's still worth being aware of.

Fix this →

---

2. Potential injection in buildMapData via country code (CountriesViewModel.kt:199-202)

private fun buildMapData(countries: List<CountryItem>): String {
    return countries.joinToString(",") { country ->
        "['${country.countryCode}',${country.views}]"
    }
}

The countryCode value is directly interpolated into a string that appears to be used as JavaScript data (Google GeoChart format). ISO 3166-1 alpha-2 codes are normally safe (e.g. "US", "GB"), but if the API ever returns an unexpected value containing a single quote or escape character, this could produce malformed or injectable JS when rendered in a WebView.

A simple guard would be to validate that countryCode matches [A-Z]{2} before interpolating, or use escaping.

Fix this →

---

3. dispatchToVisibleCards loads both MostViewed sources even when only one card is visible (NewStatsActivity.kt:541-543)

if (StatsCardType.MOST_VIEWED_POSTS_AND_PAGES in this ||
    StatsCardType.MOST_VIEWED_REFERRERS in this
) onMostViewed()

onMostViewed() triggers MostViewedViewModel.onPeriodChanged() which always loads both POSTS_AND_PAGES and REFERRERS (both calls inside loadDataInternal). So hiding one of the two MostViewed cards still causes both network calls. This is a design limitation of the single MostViewedViewModel managing both sources, not newly introduced here, but the dispatchToVisibleCards gate doesn't fully solve the problem for these two card types.

If saving bandwidth for individual card visibility matters, MostViewedViewModel would need to accept which sources to fetch.

---

💡 Suggestions

4. refresh() bypasses isLoadedSuccessfully in TodaysStatsViewModel — intentionally, but undocumented

refresh() calls loadDataInternal(site) directly (TodaysStatsViewModel.kt:51-56), bypassing isLoadedSuccessfully. This is correct behavior (pull-to-refresh should always re-fetch), but since loadDataInternal in refresh() also doesn't reset isLoading, there's a brief window where both a loadDataIfNeeded() call and a refresh() could concurrently call loadDataInternal. Both run under viewModelScope on the main dispatcher so in practice they're serialized, but a short comment noting that refresh() intentionally bypasses idempotency would help future readers.

5. Missing test: re-adding a card doesn't trigger a re-fetch

There's no test verifying that when a card is removed and re-added (same period), onPeriodChanged is correctly skipped because loadedPeriod matches. The existing loadDataIfNeeded multiple times tests cover TodaysStats and ViewsStats, but not the Countries/Authors onPeriodChanged path. Not blocking, but worth adding for completeness.

---

📊 Summary

Remaining Issues: 1 medium (MostViewed partial failure leaves period marked loaded), 1 low (countryCode injection surface), 1 minor (MostViewed card visibility granularity)
Suggestions: 2 (documentation, test gap)

The core changes are solid and the previous critical issues are resolved. Issue #1 (MostViewed loadedPeriod on partial failure) is worth addressing before merge since it can silently leave a card stuck in error state across period changes.

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 60 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.18%. Comparing base (18ad789) to head (296eb23).
⚠️ Report is 2 commits behind head on trunk.

Files with missing lines	Patch %	Lines
.../wordpress/android/ui/newstats/NewStatsActivity.kt	0.00%	46 Missing ⚠️
...ss/android/ui/newstats/authors/AuthorsViewModel.kt	69.23%	3 Missing and 1 partial ⚠️
...ndroid/ui/newstats/countries/CountriesViewModel.kt	76.47%	3 Missing and 1 partial ⚠️
...roid/ui/newstats/mostviewed/MostViewedViewModel.kt	93.84%	0 Missing and 4 partials ⚠️
...id/ui/newstats/todaysstats/TodaysStatsViewModel.kt	90.90%	0 Missing and 1 partial ⚠️
...roid/ui/newstats/viewsstats/ViewsStatsViewModel.kt	95.65%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk   #22586      +/-   ##
==========================================
+ Coverage   38.16%   38.18%   +0.02%     
==========================================
  Files        2254     2254              
  Lines      113362   113442      +80     
  Branches    15791    15802      +11     
==========================================
+ Hits        43259    43315      +56     
- Misses      66516    66534      +18     
- Partials     3587     3593       +6     

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

[nbradbury]

@adalpari I removed all cards except "Posts & Pages":

I then did a pull-to-refresh, and the inspector showed network requests for both top posts and referrers:

Is this expected?

[nbradbury]

@adalpari I did a Claude review locally and a few issues were reported (see attached MD). Should any of these be addressed?

stats-cards-review.md

[adalpari]

@adalpari I did a Claude review locally and a few issues were reported (see attached MD). Should any of these be addressed?

This is odd, the review is talking about a totally different PR:

This branch adds Authors and Countries stats cards (with detail screens) to the new stats UI, plus various fixes.

And it's reporting issues about code or classes (StatsRepositoryAuthorsTest) that haven't been changed. So it seems a bit off.

I've checked the reports anyway, and there might be a couple of points that make sense. I'll take it into account in a different PR.

[adalpari]

@adalpari I removed all cards except "Posts & Pages":

I then did a pull-to-refresh, and the inspector showed network requests for both top posts and referrers:

Is this expected?

Thanks for noting it! No, it's not expected. It's because they started being the same cads with just a different source. When I split them into two different cards, I missed this callback. I'll fix it.

[adalpari]

@nbradbury Fixed here: 296eb23

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[nbradbury]

This is odd, the review is talking about a totally different PR:

That's odd, but not the first time I've seen it. I wonder if Claude gets confused by the multiple worktree branches?

[nbradbury]

@nbradbury Fixed here: 296eb23

That fix is good! I'll approve this so you can merge when ready.

[nbradbury]