Skip to content

Conversation

@JohnsonEricAtSalesforce
Copy link
Contributor

🥁 Ready For Review 🎸

This catches 13.2.0 dev up with today's commits to 13.1.1 master

@github-actions
Copy link

Job Summary for Gradle

Pull Request :: test-android
Gradle Root Project Requested Tasks Gradle Version Build Outcome Build Scan®
SalesforceMobileSDK-Android libs:SalesforceSDK:convertCodeCoverage 8.14.3 Build Scan not published
SalesforceMobileSDK-Android libs:SalesforceSDK:lint 8.14.3 Build Scan not published
SalesforceMobileSDK-Android libs:SalesforceSDK:assembleAndroidTest 8.14.3 Build Scan not published

@github-actions
Copy link

Job Summary for Gradle

Pull Request :: test-android
Gradle Root Project Requested Tasks Gradle Version Build Outcome Build Scan®
SalesforceMobileSDK-Android libs:SalesforceSDK:convertCodeCoverage 8.14.3 Build Scan not published
SalesforceMobileSDK-Android libs:SalesforceSDK:lint 8.14.3 Build Scan not published
SalesforceMobileSDK-Android libs:SalesforceSDK:assembleAndroidTest 8.14.3 Build Scan not published

@codecov
Copy link

codecov bot commented Jan 21, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.49%. Comparing base (47f5b94) to head (8d769b9).
⚠️ Report is 3 commits behind head on dev.

Additional details and impacted files
@@             Coverage Diff              @@
##                dev    #2823      +/-   ##
============================================
- Coverage     63.06%   53.49%   -9.58%     
+ Complexity     2831     2428     -403     
============================================
  Files           215      216       +1     
  Lines         17036    16980      -56     
  Branches       2465     2417      -48     
============================================
- Hits          10744     9083    -1661     
- Misses         5124     6870    +1746     
+ Partials       1168     1027     -141     
Components Coverage Δ
Analytics 47.92% <ø> (ø)
SalesforceSDK 56.34% <50.00%> (+0.30%) ⬆️
Hybrid 44.91% <ø> (-14.15%) ⬇️
SmartStore 78.20% <ø> (ø)
MobileSync 36.95% <ø> (-44.73%) ⬇️
React 36.25% <ø> (-16.12%) ⬇️
Files with missing lines Coverage Δ
.../salesforce/androidsdk/app/SalesforceSDKManager.kt 58.92% <ø> (-0.15%) ⬇️
.../src/com/salesforce/androidsdk/ui/LoginActivity.kt 42.99% <100.00%> (-0.78%) ⬇️
...m/salesforce/androidsdk/ui/LoginOptionsActivity.kt 87.62% <ø> (-0.61%) ⬇️
...src/com/salesforce/androidsdk/ui/LoginViewModel.kt 89.69% <100.00%> (+0.10%) ⬆️

... and 46 files with indirect coverage changes

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

Comment on lines 575 to 577
val isNewServer = viewModel.loginUrl.value?.startsWith(value) != true
val valueUrl = value.toUri()
val loginUrl = viewModel.loginUrl.value?.toUri()
val isNewServer = loginUrl?.host != valueUrl.host || loginUrl?.path != valueUrl.path
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like an unrelated behavior change. Why is it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a fix for W-20935841 that we ran into when testing WSC and WSC/disco login. The fix was done in master in the same PR that removed supportWelcomeDiscovery.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, thank you I wasn't aware of that fix. Having both welcome.salesforce.com and welcome.salesforce.com/discovery seems like a very niche issue. And I know for a fact that I explicitly used startsWith instead of directly comparing the path to prevent bugs where login would fail from running getAuthorizationUrl when we shouldn't have as that causes the PKCE code and verifier to be set with new values. Perhaps the scenario this was intended to prevent does not exist anymore but I'm not 100% sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Circling back:

  • The startsWith is not a carry over from the old OAuthWebviewHelper.
  • Looking at the current code, LoginUrlSource is only ever called when selectedServer changes and I do not see a scenario. If it does not happen mid authentication there should not be any issues with getting new code/verifier.

Seems like whatever issue this did avoid is no longer possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow! Thanks for the detailed analysis ⭐️⭐️⭐️⭐️⭐️

@brandonpage brandonpage dismissed their stale review January 21, 2026 18:26

Unsure if issues raised is a bug or not.

Comment on lines 1600 to 1623
val intent = activity.intent
val data = intent.data
if (data == null) {
Log.i("CodeCov", "1")
}
if (data?.host?.equals(pendingServerUri.host) == true) {
Log.i("CodeCov", "2.0")
}
if (data?.host?.equals(pendingServerUri.host) == false) {
Log.i("CodeCov", "2.1")
}
if (data?.path?.equals(pendingServerUri.path) == true) {
Log.i("CodeCov", "3.0")
}
if (data?.path?.equals(pendingServerUri.path) == false) {
Log.i("CodeCov", "3.1")
}
if (intent.getStringExtra(EXTRA_KEY_LOGIN_HOST).equals(pendingServerUri.host)) {
Log.i("CodeCov", "4.0")
}
if (!(intent.getStringExtra(EXTRA_KEY_LOGIN_HOST).equals(pendingServerUri.host))) {
Log.i("CodeCov", "4.1")
}
if (((data?.host?.equals(pendingServerUri.host) == true).and(data?.path?.equals(pendingServerUri.path) == true)).or(intent.getStringExtra(EXTRA_KEY_LOGIN_HOST).equals(pendingServerUri.host))) {
Copy link
Contributor

@brandonpage brandonpage Jan 21, 2026

Choose a reason for hiding this comment

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

@JohnsonEricAtSalesforce Was this intended to be checked in?

Edit: I see the "(CodeCov Testing - Hold Reviewing Commit)" now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for looking for that. This commit will be replaced with a much simpler commit once I get full coverage. Even with Windsurf helping, getting through all the logic branches and minimizing their count is still tedious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brandonpage - Here's the finalized commit for code coverage, assuming it still produces the coverage I was looking for. 16daac0

Most of the updates were suggested by or reviewed by Windsurf. I wrote the tests by hand, though, since the generated code was overly verbose it seemed.

@JohnsonEricAtSalesforce JohnsonEricAtSalesforce force-pushed the feature/merge-13.1.1-master-to-13.2.0-dev branch 4 times, most recently from 38701a9 to 29f14a8 Compare January 21, 2026 22:34
@JohnsonEricAtSalesforce JohnsonEricAtSalesforce force-pushed the feature/merge-13.1.1-master-to-13.2.0-dev branch 2 times, most recently from 81dc9a7 to 16daac0 Compare January 21, 2026 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants