Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes CustomerCenter state not refreshing when reopening #2202

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

vegaro
Copy link
Contributor

@vegaro vegaro commented Feb 28, 2025

We noticed that the customer center wasn't reloading when reopening it, only after switching to another tab in the Paywalls Tester.

I moved the loadCustomerCenter to LaunchedEffect next to when we track the impression to fix it

Copy link

emerge-tools bot commented Feb 28, 2025

📸 Snapshot Test

260 unchanged

Name Added Removed Modified Renamed Unchanged Errored Approval
TestPurchasesUIAndroidCompatibility
com.revenuecat.testpurchasesuiandroidcompatibility
0 0 0 0 260 0 N/A

🛸 Powered by Emerge Tools

@vegaro vegaro requested a review from a team February 28, 2025 13:50
LaunchedEffect(Unit) {
viewModel.loadCustomerCenter()
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that loadCustomerCenter does not cancel previous calls, or at least doesn't seem to.
Is it worth checking something like ... ?

    override suspend fun loadCustomerCenter() {
        if (_state.value is CustomerCenterState.Loading) {.  <--- early exit if its already loading
            return
        }
        if (_state.value !is CustomerCenterState.Loading) {
            _state.value = CustomerCenterState.Loading
        }
        try {
            val customerCenterConfigData = purchases.awaitCustomerCenterConfigData()
            val purchaseInformation = loadPurchaseInformation(dateFormatter, locale)
            _state.value = CustomerCenterState.Success(
                customerCenterConfigData,
                purchaseInformation,
                supportedPathsForManagementScreen = customerCenterConfigData.getManagementScreen()?.let {
                    supportedPaths(purchaseInformation, it)
                },
            )
        } catch (e: PurchasesException) {
            _state.value = CustomerCenterState.Error(e.error)
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm that's a good point. Actually we shouldn't be loading the initial state here. Let me rethink it

Copy link
Contributor

@facumenzella facumenzella Feb 28, 2025

Choose a reason for hiding this comment

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

What about a refreshIfNeeded kind of thing?

Copy link
Contributor

@facumenzella facumenzella left a comment

Choose a reason for hiding this comment

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

Just one question

Copy link

codecov bot commented Feb 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.54%. Comparing base (2c525b9) to head (6448edc).
Report is 5 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2202   +/-   ##
=======================================
  Coverage   80.54%   80.54%           
=======================================
  Files         277      277           
  Lines        9456     9456           
  Branches     1334     1334           
=======================================
  Hits         7616     7616           
  Misses       1280     1280           
  Partials      560      560           

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

@vegaro
Copy link
Contributor Author

vegaro commented Feb 28, 2025

I found the root cause. It was a regression from #2171

@vegaro vegaro requested a review from facumenzella February 28, 2025 16:12
@vegaro
Copy link
Contributor Author

vegaro commented Feb 28, 2025

I would love to add a test for this, but will do in a separate PR, it might be tricky

@vegaro
Copy link
Contributor Author

vegaro commented Mar 3, 2025

Added a test

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.

2 participants