Skip to content

Fix issues introduced by login lib removal#22703

Merged
nbradbury merged 2 commits intotrunkfrom
remove/login-lib-part2
Mar 18, 2026
Merged

Fix issues introduced by login lib removal#22703
nbradbury merged 2 commits intotrunkfrom
remove/login-lib-part2

Conversation

@nbradbury
Copy link
Contributor

@nbradbury nbradbury commented Mar 17, 2026

Summary

This PR addresses the remaining issues introduced in #22564.

  • Fix multiple setContentView() calls in LoginActivity that orphan fragments by using a single root layout with a loading overlay and toggling visibility
  • Wrap unbindService() in try-catch to prevent IllegalArgumentException crashes
  • Reset cached mLoginFlow in onNewIntent() to prevent stale login flow when activity is reused via singleTop
  • Remove custom get() on loadingStateFlow that recreated the StateFlow wrapper on every access
  • Remove redundant e.printStackTrace() (already logged via appLogWrapper)
  • Require a dot in site URL validation to reject single-word inputs like example
  • Make the site address loading dialog dismissible so users aren't stuck if the request hangs

Test plan

Test the various login flows and verify everything works as expected.

@dangermattic
Copy link
Collaborator

dangermattic commented Mar 17, 2026

1 Warning
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 17, 2026

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

App NameJetpack Android
Build TypeDebug
Versionpr22703-f02231d
Build Number1487
Application IDcom.jetpack.android.prealpha
Commitf02231d
Installation URL32b9bf8siftig
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 17, 2026

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

App NameWordPress Android
Build TypeDebug
Versionpr22703-f02231d
Build Number1487
Application IDorg.wordpress.android.prealpha
Commitf02231d
Installation URL1c8hp21levsc8
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@nbradbury nbradbury force-pushed the remove/login-lib-part2 branch from afb65f5 to 2f8c6d3 Compare March 17, 2026 23:13
@codecov
Copy link

codecov bot commented Mar 17, 2026

Codecov Report

❌ Patch coverage is 22.22222% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 37.49%. Comparing base (cc26b8d) to head (f02231d).
⚠️ Report is 1 commits behind head on trunk.

Files with missing lines Patch % Lines
...tionpassword/LoginSiteApplicationPasswordScreen.kt 0.00% 10 Missing ⚠️
...npassword/LoginSiteApplicationPasswordViewModel.kt 57.14% 3 Missing ⚠️
...ress/android/ui/accounts/login/WPcomLoginHelper.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            trunk   #22703      +/-   ##
==========================================
- Coverage   37.49%   37.49%   -0.01%     
==========================================
  Files        2286     2286              
  Lines      120484   120491       +7     
  Branches    16501    16505       +4     
==========================================
  Hits        45173    45173              
- Misses      71637    71644       +7     
  Partials     3674     3674              

☔ 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 nbradbury marked this pull request as ready for review March 17, 2026 23:39
@nbradbury nbradbury requested a review from adalpari March 17, 2026 23:39
- Use single root layout with loading overlay in LoginActivity to avoid
  multiple setContentView() calls that orphan fragments
- Wrap unbindService() in try-catch for IllegalArgumentException
- Reset cached mLoginFlow in onNewIntent() to prevent stale values
- Remove custom get() on loadingStateFlow to avoid recreating wrapper
- Remove redundant e.printStackTrace() already logged via appLogWrapper
- Require dot in site URL validation to reject single-word inputs
- Make loading dialog dismissible by adding cancel discovery callback

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@nbradbury nbradbury force-pushed the remove/login-lib-part2 branch from 2f8c6d3 to 6d41710 Compare March 17, 2026 23:46
@adalpari
Copy link
Contributor

Just saw a couple of finding could be good to review:

  1. LoginActivity.java — Loading overlay approach (Good, one concern)

The single-root-layout + overlay toggle is a solid fix for the fragment orphaning problem.

Concern — onNewIntent fallback path:

if (mLoadingOverlay != null) {
showLoadingOverlay();
} else {
setContentView(R.layout.login_loading);
}

mLoadingOverlay is initialized in onCreate, which always runs before onNewIntent (since the activity must exist to receive a new intent). The else branch with setContentView
reintroduces the exact problem this PR fixes. If this is truly unreachable, remove the fallback entirely rather than keeping dead code that could cause the original bug if ever
reached. If there's a legitimate scenario, it should be documented.

  1. LoginActivity.java — showLoadingOverlay doesn't have a reverse

There's showLoadingOverlay() but no hideLoadingOverlay() to restore the fragment container visibility. If the loading state is cleared (e.g., an error occurs after showing the
overlay), there's no path to make the fragment container visible again. The mIsWaitingForSitesToLoad codepath and the OAuth callback path both show the overlay but rely on
loggedInAndFinish() to exit. Consider what happens if the login fails after the overlay is shown.

  1. LoginSiteApplicationPasswordViewModel.kt — cancelDiscovery race condition

fun cancelDiscovery() {
discoveryJob?.cancel()
discoveryJob = null
_loadingStateFlow.value = false
}

If cancellation happens just as runApiDiscovery completes, the coroutine's _loadingStateFlow.value = false will run after cancelDiscovery sets it to false — benign in this
case. But _discoveryURL.send(discoveryUrl) could still execute before cancellation takes effect, potentially navigating the user away after they dismissed the dialog. Consider
adding an ensureActive() check before _discoveryURL.send():

discoveryJob = viewModelScope.launch {
val discoveryUrl = applicationPasswordLoginHelper
.getAuthorizationUrlComplete(siteUrl)
ensureActive() // Don't navigate if cancelled
_discoveryURL.send(discoveryUrl)
_loadingStateFlow.value = false
}

Copy link
Contributor

@adalpari adalpari left a comment

Choose a reason for hiding this comment

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

Testing the app looks good to me, so I'm approving it.
I left some comments from Claude because they look important, but I'm not sure they are actually useful. So, feel free to use them or not.

Adds the inverse of showLoadingOverlay() so future code paths
can transition back from the loading state to the fragment UI.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@nbradbury
Copy link
Contributor Author

@adalpari #1 and #2 are valid concerns and have been fixed. Thanks for the review!

@nbradbury nbradbury enabled auto-merge (squash) March 18, 2026 21:08
@sonarqubecloud
Copy link

@nbradbury nbradbury merged commit 49d8dcd into trunk Mar 18, 2026
21 of 22 checks passed
@nbradbury nbradbury deleted the remove/login-lib-part2 branch March 18, 2026 21:30
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.

4 participants