Skip to content

Conversation

@itsmeichigo
Copy link
Contributor

@itsmeichigo itsmeichigo commented Jul 28, 2025

Closes WOOMOB-902

Description

We got crash reports in the LoginViewController.showLoginEpilogue step of the login flow. This method can fail in two cases:

  • When the navigationController of the view controller cannot be found.
  • When the authenticationDelegate is nil.

This PR mitigates the issue with some enhancements:

  • Updates the initialization of the authenticator to allow initializing only once upon app start. Reasons behind this fix:
    • It's not necessary to initialize more than once.
    • The previous implementation allows the private instance to be recreated, breaking the delegate reference.
  • Add messages to the fatalError in the login flow to better understand the issue if the above doesn't fix the crash.

Note: I'm targeting 23.0 for this PR as the crash is likely an edge case. Let me know if you think we should target the beta build 22.9 instead.

Testing steps

Test login using different scenarios:

  • Log out of the app and try logging back in => confirm that login succeeds.
  • Test login with magic link when the app is forced closed (detailed steps here).
  • Test that connecting to existing site flow still works (detailed steps here)

Testing information

Tested the above test cases using simulator iPhone 16 iOS 18.4.

Screenshots

No UI changes.


  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@itsmeichigo itsmeichigo added this to the 23.0 milestone Jul 28, 2025
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jul 28, 2025

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

App NameWooCommerce iOS Prototype
Build Numberpr15953-df0a0df
Version22.9
Bundle IDcom.automattic.alpha.woocommerce
Commitdf0a0df
Installation URL2m12qvrelr2o8
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@itsmeichigo itsmeichigo changed the title Jetpack Setup: Fix initialization of authenticator Fix initialization of authenticator Jul 29, 2025
@itsmeichigo itsmeichigo changed the base branch from trunk to release/22.9 July 29, 2025 07:21
@itsmeichigo itsmeichigo changed the base branch from release/22.9 to trunk July 29, 2025 07:21
@itsmeichigo itsmeichigo added feature: login Related to any part of the log in or sign in flow, or authentication. type: crash The worst kind of bug. labels Jul 29, 2025
@itsmeichigo itsmeichigo marked this pull request as ready for review July 29, 2025 07:22
Copy link
Contributor

@RafaelKayumov RafaelKayumov left a comment

Choose a reason for hiding this comment

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

LGTM, pre-approving. Left a couple of comments.


/// the authenticator needs to be initialized with configs
/// to be used for requesting authentication link and handle login later.
WordPressAuthenticator.initializeWithCustomConfigs(dotcomAuthScheme: dotcomAuthScheme)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the dotcomAuthScheme argument of initializeWithCustomConfigs method is no longer being passed and the ApiCredentials.dotcomAuthScheme default value is always used there. May be it's worth removing the method argument at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in 40282f6.

}

/// Configures the WPAuthenticator for usage in both logged-in and logged-out states.
func configureAuthenticator() {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is another configureAuthenticator call on line 292. Should it removed as well?

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 catching this! Removed in b4f38e6.

@itsmeichigo itsmeichigo enabled auto-merge July 30, 2025 07:07
@itsmeichigo itsmeichigo merged commit 1954eec into trunk Jul 30, 2025
13 checks passed
@itsmeichigo itsmeichigo deleted the woomob-902-crash-on-certain-flows-that-use-loginviewcontroller branch July 30, 2025 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: login Related to any part of the log in or sign in flow, or authentication. type: crash The worst kind of bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants