An attempt to fix a crash in the Dashboard Performance card

[hichamboushaba]

Closes: #12548

Please don't merge yet, I'll add a release notes entry later.

Description

This PR is an attempt to fix a crash in the performance card. I'm not 100% certain this is the situation that the users face, but it's the only way I found where I was able to reproduce the crash.
And this is what happens according to my theory:

1. The user has a cached revenue stats that contain some data.
2. When the app is launched, the app will start by showing the cached data, then start fetching fresh data.
3. If the updated data is empty, and given these lines, we'll keep displaying the cached data.
4. If the user taps on a line, then we'll get the crash.

Using the cached data followed up with a fetch is a new behavior that we added recently (precisely as part of 19.6), and I think this could explain why the crash started happening just now.

@JorgeMucientes given your experience with the stats, I'm assigning you this for review 🙏

Steps to reproduce the crash

1. Use trunk
2. Apply the patch from below to simulate an empty revenue result after a non-empty one.
3. Open the app, and make sure the performance card is shown.
4. Switch to a tab that has some sales.
5. Tap on one of the entries.
6. Notice the crash.

Confirm the fix

1. Use this branch
2. Repeat 2-5 from above.
3. Confirm the performance card is correctly showing "No revenue this period" and that you can't tap a line.

Patch

Index: WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/stats/DashboardStatsCard.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/stats/DashboardStatsCard.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/stats/DashboardStatsCard.kt
--- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/stats/DashboardStatsCard.kt	(revision 328f85127b189b99eaae501c8457aaeeeb4a515f)
+++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/stats/DashboardStatsCard.kt	(date 1726221143819)
@@ -36,6 +36,8 @@
 import com.woocommerce.android.util.CurrencyFormatter
 import com.woocommerce.android.util.DateUtils
 import com.woocommerce.android.viewmodel.MultiLiveEvent
+import kotlinx.coroutines.delay
+import kotlinx.coroutines.launch
 import java.util.Date
 
 @Composable
@@ -209,6 +211,16 @@
                 statsView.showErrorView(false)
                 statsView.showSkeleton(false)
                 statsView.updateView(revenueStatsState.revenueStats)
+                launch {
+                    delay(100)
+                    statsView.updateView(
+                        revenueStatsState.revenueStats?.copy(
+                            intervalList = emptyList(),
+                            totalSales = 0.0,
+                            totalOrdersCount = 0
+                        )
+                    )
+                }
             }
 
             DashboardStatsViewModel.RevenueStatsViewState.GenericError -> {

Images/gif

• I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

• The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
• Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
• Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

[dangermattic]

	1 Warning
⚠️	View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.

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

Generated by 🚫 Danger

[wpmobilebot]

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

	App Name	WooCommerce-Wear Android
	Platform	⌚️ Wear OS
	Flavor	Jalapeno
	Build Type	Debug
	Commit	40b4638
	Direct Download	woocommerce-wear-prototype-build-pr12605-40b4638.apk

[wpmobilebot]

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

	App Name	WooCommerce Android
	Platform	📱 Mobile
	Flavor	Jalapeno
	Build Type	Debug
	Commit	40b4638
	Direct Download	woocommerce-prototype-build-pr12605-40b4638.apk

[hichamboushaba]

This change is not related to the fix, but it's something that I noticed and that confused me and lost me a lot of time while checking if it could be the cause of the issue: we were assigning chartRevenueStats to itself here, and while investigating the issue I was looking for any race modifications to the chartRevenueStats that could explain the issue.

To avoid this confusion in the future, I'm simplifying the logic here, it's still the same just without the useless assignment.

[JorgeMucientes]

Hey @hichamboushaba, thanks for investigating this.

The explanation for the possible cause of the crash makes sense. However, I can't think of a scenario where given a date range, there's cached revenue data for it, but when requesting fresh data for the same range, the revenue data is empty.

I suspect that the logic enabled here might be messing around with the cached values, leading to this empty data. The reason I think this is because:

• Crashes started happening from 19.8 which is when this feature was enabled
• Upon checking the app logs for several of the users impacted by the crash I can see that always leading to the crash before opening the app there's one of these 2 logs:

🔵 Tracked: background_data_sync_error, Properties: {"error_context":"UpdateDataOnBackgroundWorker","error_description":"Dashboard stats refresh failed." ...

🔵 Tracked: background_data_synced, Properties: {"time_taken":25409,"blog_id":236595758, ...

Thinking out loud here. Could it be that this is happening?

• User loads successfully revenue data. Chart is displayed.
• User sends app to the background
• A background stats refresh is triggered. This clears the cached data because it fails or whatever reason.
• User brings the app to the foreground, ViewModel still has the viewState in memory, but the cache for the selected date range is empty now. Chart will be displayed, and then empty revenue data will be passedto DashboardStatsView

Aside from that hypothesis. And looking at the proposed way to mitigate the crash I was thinking of a different way of resolving it, without clearing the chart, and keeping the cached data visible. Just avoid updating the view completely if the reneue stats model is empty.

Subject: [PATCH] Avoid updating the view
---
Index: WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/stats/DashboardStatsView.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/stats/DashboardStatsView.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/stats/DashboardStatsView.kt
--- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/stats/DashboardStatsView.kt	(revision 40b46384d60a12e8beffa7f0fae562239b6c6917)
+++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/stats/DashboardStatsView.kt	(date 1726593414406)
@@ -378,6 +378,11 @@
     }
 
     fun updateView(revenueStatsModel: RevenueStatsUiModel?) {
+        if (revenueStatsModel?.rangeId == this.revenueStatsModel?.rangeId &&
+            revenueStatsModel?.intervalList.isNullOrEmpty()
+        ) {
+            return
+        }
         this.revenueStatsModel = revenueStatsModel
 
         // There are times when the stats v4 api returns no grossRevenue or ordersCount for a site
@@ -552,7 +557,6 @@
         fadeInLabelValue(ordersValue, orders)
 
         if (chartRevenueStats.isEmpty() || revenueStatsModel?.totalSales == 0.toDouble()) {
-            binding.chart.clear()
             isRequestingStats = false
             return
         }
@@ -726,6 +730,7 @@
                     dateString,
                     statsTimeRangeSelection.revenueStatsGranularity
                 )
+
                 else -> error("Unsupported range value used in my store tab: ${statsTimeRangeSelection.selectionType}")
             }.also { result -> trackUnexpectedFormat(result, dateString) }
         }

It feel like a workaround, but at least the user won't be seeing empty results for an interval the previously did show some revenue results.
Wdyt?

[hichamboushaba]

Thanks @JorgeMucientes for the great analysis, let me try to respond to some points below, and we'll take it from there.

However, I can't think of a scenario where given a date range, there's cached revenue data for it, but when requesting fresh data for the same range, the revenue data is empty.

It can happen if an order gets deleted.

This clears the cached data because it fails or whatever reason.

Looking at the code, I don't think that failures could trigger clearing the data, only a success would.

And looking at the proposed way to mitigate the crash I was thinking of a different way of resolving it, without clearing the chart, and keeping the cached data visible

My issue with this is that if our theory is right, we are actually displaying outdated data here, so clearing the chart is still the correct thing to do to avoid displaying wrong data.

Please let me know what you think.

[JorgeMucientes]

Thanks for the clarifications @hichamboushaba

It can happen if an order gets deleted.

True! Had not considered that case.

My issue with this is that if our theory is right, we are actually displaying outdated data here, so clearing the chart is still the correct thing to do to avoid displaying wrong data.

Fair enough. Let's keep the current approach, then.