CMM-1952: Skip data fetching for hidden Insights cards and polish detail screens

[adalpari]

Description

Improves the Insights tab by skipping unnecessary network calls for hidden cards and polishing the Tags & Categories detail screen.

Changes:

• Conditional data fetching — InsightsViewModel.fetchData() now only calls summary/insights endpoints when visible cards need them, avoiding wasted network calls for hidden cards
• Re-fetch on card re-add — tracks which endpoint groups have been fetched (summaryFetched/insightsFetched) so re-adding a hidden card triggers a fetch for its missing data
• Tags detail screen — fetches data independently instead of using a static field, with its own ViewModel and max 100 items
• Shared mapper & detail VM tests — extracts TagsAndCategoriesMapper, adds TagsAndCategoriesDetailViewModelTest, guards against double fetch calls, shows all 10 card items
• StatsTagsUseCase — centralizes token validation and repository init, removing duplication between Tags ViewModels
• Shimmer cleanup — replaces duplicate shimmer animation in YearInReviewCard with shared rememberShimmerBrush()
• Locale-safe tests — fixes locale-dependent assertions in MostPopularDayViewModelTest
• Card reordering tests — adds tests for moveCardUp/moveCardDown/moveCardToTop/moveCardToBottom on middle elements

Testing instructions

Conditional fetching:

1. Navigate to Stats → Insights tab with all cards visible

• All cards load data correctly

2. Remove all cards except Tags & Categories (via three-dot menu → "Remove card")

• No summary or insights endpoint calls in Logcat

3. Re-add "All Time Stats" via "Add card" button

• Summary endpoint is called and the card loads data

4. Pull to refresh

• All visible cards reload their data

Tags & Categories detail screen:

1. Tap "Show all" on Tags & Categories card

• Detail screen loads data independently (up to 100 items)
• Items show position numbers
• Loading shimmer appears while fetching

Card reordering:

1. Tap three-dot menu on a middle card → "Move up" / "Move down"

• Card order changes correctly

2. Use "Move to top" / "Move to bottom"

• Card moves to the expected position

[dangermattic]

	2 Errors
🚫	Please add tests for class BaseTagsAndCategoriesViewModel (or add unit-tests-exemption label to ignore this).
🚫	This PR includes a PR version of wordpress-rs (1230-d4c9317af369c7231b414998986fd2e773eebf7d). Please merge the corresponding wordpress-rs PR and update to a trunk version before merging.

	1 Warning
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

	1 Message
📖	This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

[adalpari]

@claude

[claude]

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

---

Code Review

• Gather context and read changed files
• Review InsightsViewModel.kt
• Review TagsAndCategoriesDetailViewModel.kt (new file)
• Review TagsAndCategoriesDetailActivity.kt
• Review TagsAndCategoriesViewModel.kt
• Review TagsAndCategoriesMapper.kt (new file)
• Review StatsTagsUseCase.kt (new file)
• Review test files
• Review remaining changed files
• Compile and post final review

---

Overall this is a well-structured PR. The architecture is clean, the conditional fetching logic is a nice optimization, and test coverage is solid. I found a few issues worth addressing before merging.

---

Bugs

1. Process-death / back-stack restore leaves detail screen stuck in Loading

TagsAndCategoriesDetailActivity.kt:60-62:

if (savedInstanceState == null) {
    viewModel.loadData()
}

This guard is correct for configuration changes (rotation), but after a system-initiated process death and restore, the activity is recreated with a non-null savedInstanceState. At that point the ViewModel is freshly instantiated (isLoaded = false, isLoading = false), starts in Loading state, but loadData() is never called — leaving the user on a permanent loading screen. Consider calling loadData() unconditionally in onCreate, or checking the ViewModel's current state (uiState.value) to decide whether to trigger a fetch.

Fix this →

---

2. StatsTagsUseCase has no caching — inconsistent with StatsSummaryUseCase / StatsInsightsUseCase

StatsTagsUseCase.kt fetches fresh data on every call, while StatsSummaryUseCase and StatsInsightsUseCase both use a Mutex-protected in-memory cache keyed by (siteId, data). If the detail screen and card screen both call StatsTagsUseCase (e.g., via back/forward navigation), two redundant network requests will be made. The pattern used in the other two use cases should be applied here for consistency and efficiency.

Fix this →

---

3. Tags card is excluded from InsightsViewModel.refreshData() coordination

InsightsViewModel.kt:322-334 — needsSummary() and needsInsights() do not include TAGS_AND_CATEGORIES. When the user pulls to refresh on the Insights tab, refreshData() re-fetches summary and insights data, but TagsAndCategoriesViewModel.refresh() is a separate call. If the Insights screen composable doesn't explicitly propagate refreshData() to the tags card ViewModel, the tags card will not refresh. Please verify (or add a test) that the pull-to-refresh path calls TagsAndCategoriesViewModel.refresh() in addition to InsightsViewModel.refreshData().

---

Code Quality

4. @Volatile flags in InsightsViewModel should use AtomicBoolean

InsightsViewModel.kt:71-81:

@Volatile private var isDataLoaded = false
@Volatile private var isDataLoading = false

The check-then-act in loadDataIfNeeded() is not atomic:

if (isDataLoaded || isDataLoading) return  // read
isDataLoading = true                        // write

@Volatile guarantees visibility but not atomicity of compound operations. The sibling ViewModels (TagsAndCategoriesViewModel, TagsAndCategoriesDetailViewModel) correctly use AtomicBoolean.compareAndSet() for exactly this reason. For consistency and correctness, use AtomicBoolean with compareAndSet here as well — the existing pattern in those ViewModels is a good reference.

Fix this →

---

5. Raw exception messages exposed to users

TagsAndCategoriesViewModel.kt:81-85 and TagsAndCategoriesDetailViewModel.kt:74-80:

} catch (e: Exception) {
    _uiState.value = TagsAndCategoriesCardUiState.Error(
        e.message ?: resourceProvider.getString(R.string.stats_error_unknown)
    )

e.message can contain raw technical strings (e.g. "Unable to resolve host 'public-api.wordpress.com'") that are not user-friendly. Consider always using the localized stats_error_unknown string here, keeping the raw message only for logging.

---

6. Significant code duplication between the two Tags ViewModels

TagsAndCategoriesViewModel.kt and TagsAndCategoriesDetailViewModel.kt share almost identical loadData(), fetchData(), handleResult() logic and the same AtomicBoolean + fetchJob pattern. The only meaningful differences are the max parameter and the presence of refresh() in the card VM. A common base class or a delegated helper would reduce this duplication and keep the two in sync going forward.

---

Testing Gaps

7. Missing test: only TAGS_AND_CATEGORIES visible → no summary/insights endpoints called

InsightsViewModelTest.kt — There is a test for "all cards hidden → no endpoints called", but no analogous test for the case where only TAGS_AND_CATEGORIES is visible (which also produces shouldFetchSummary = false and shouldFetchInsights = false). Adding this case would explicitly document the intended behavior and guard against regressions.

8. No test for loadDataIfNeeded() after the early-return path sets isDataLoaded to false

When fetchData() early-returns because no cards need data, isDataLoaded stays false and isDataLoading is reset to false. Every subsequent call to loadDataIfNeeded() will invoke fetchData() again (harmlessly), but this is an infinite re-entry potential if the card list changes. A test documenting this behavior (calling loadDataIfNeeded() multiple times with a tags-only config) would be useful.

---

Minor

• InsightsCardsConfigurationRepository.kt:283-288: The @Suppress("USELESS_CAST") + null-check pattern to detect Gson deserializing unknown enum values as null is fragile. Since EnumWithFallbackValueTypeAdapterFactory is already registered, it might be worth documenting why the extra null check is still needed (or whether the factory makes it redundant).

• TagsAndCategoriesDetailActivity.kt — the start(context: Context) companion factory doesn't accept any parameters. If the detail screen ever needs a site ID or other arguments (e.g., for deep-link support), this will require a breaking change. Consider passing siteId now for future-proofing, even if the ViewModel currently retrieves it from SelectedSiteRepository.

[wpmobilebot]

Project dependencies changes

list

! Upgraded Dependencies
rs.wordpress.api:android:1230-d4c9317af369c7231b414998986fd2e773eebf7d, (changed from trunk-4fac42b1a262e93adcbc0cd0353aafa0369671d2)
rs.wordpress.api:kotlin:1230-d4c9317af369c7231b414998986fd2e773eebf7d, (changed from trunk-4fac42b1a262e93adcbc0cd0353aafa0369671d2)

tree

 +--- project :libs:fluxc
-|    \--- rs.wordpress.api:android:trunk-4fac42b1a262e93adcbc0cd0353aafa0369671d2
-|         +--- 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-4fac42b1a262e93adcbc0cd0353aafa0369671d2
-|         |    +--- 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:1230-d4c9317af369c7231b414998986fd2e773eebf7d
+|         +--- 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:1230-d4c9317af369c7231b414998986fd2e773eebf7d
+|         |    +--- 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-4fac42b1a262e93adcbc0cd0353aafa0369671d2 (*)
+\--- rs.wordpress.api:android:1230-d4c9317af369c7231b414998986fd2e773eebf7d (*)

[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
	Build Type	Debug
	Version	pr22688-ae608d8
	Build Number	1487
	Application ID	org.wordpress.android.prealpha
	Commit	ae608d8
	Installation URL	4kp60dl2lonlg

Note: Google Login is not supported on these builds.

[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
	Build Type	Debug
	Version	pr22688-ae608d8
	Build Number	1487
	Application ID	com.jetpack.android.prealpha
	Commit	ae608d8
	Installation URL	5qteag7ovl18g

Note: Google Login is not supported on these builds.

[wpmobilebot]

🤖 Build Failure Analysis

This build has failures. Claude has analyzed them - check the build annotations for details.

[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