Skip to content

fix: login flow properly shows invalid email#7802

Open
lorenzo-ranciaffi wants to merge 5 commits intodevfrom
fix/6971-invalid-email-error-on-login
Open

fix: login flow properly shows invalid email#7802
lorenzo-ranciaffi wants to merge 5 commits intodevfrom
fix/6971-invalid-email-error-on-login

Conversation

@lorenzo-ranciaffi
Copy link
Copy Markdown
Contributor

@lorenzo-ranciaffi lorenzo-ranciaffi commented Mar 25, 2026

Pull Request Description

Fixes #6971

Figma

What does this PR change?

Adds the management of the otp code send so that the UI can properly react to failures caused by invalid emails.

The main idea is to handle the invalid email exception + have an event fired after a successful otp send so that the UI can react to that and proceed with the OTP validation

Test Instructions

Test Steps

  1. Verify that at login, invalid emails are properly recognized and shown as described in the attached Figma doc
  2. Only the login flow should be affected (credits menu shouldn't)

Quality Checklist

  • Changes have been tested locally
  • Documentation has been updated (if required)
  • Performance impact has been considered
  • For SDK features: Test scene is included

Code Review Reference

Please review our Code Review Standards before submitting.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 25, 2026

@lorenzo-ranciaffi lorenzo-ranciaffi marked this pull request as ready for review March 26, 2026 08:37
@lorenzo-ranciaffi lorenzo-ranciaffi requested review from a team as code owners March 26, 2026 08:37
@lorenzo-ranciaffi lorenzo-ranciaffi moved this from In Progress to With QA / Awaiting Review in Explorer Alpha Mar 26, 2026
Copy link
Copy Markdown
Contributor

@RominaMarchetti RominaMarchetti left a comment

Choose a reason for hiding this comment

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

The flow is now working as expected: email validation happens before proceeding to the verification code view. No connection modal appearing anymore but the email error when it's invalid.

Evidence
https://github.com/user-attachments/assets/4eb9d695-1d2c-4f36-be3b-baf38183f4de

Copy link
Copy Markdown

@DafGreco DafGreco left a comment

Choose a reason for hiding this comment

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

✔️ PR reviewed and approved by QA on both platforms following instructions playing both happy and un-happy path

Regressions for this ticket had been performed in order to verify that the normal flow is working as expected:

  • [✔️ ] Log In/Log Out
  • [ ✔️] Backpack and wearables in world
  • [ ✔️] Emotes in world and in backpack
  • [ ✔️] Teleport with map/coordinates/Jump In
  • [ ✔️] Chat and multiplayer
  • [ ✔️] Profile card
  • [ ✔️] Camera
  • [ ✔️] Skybox
  • [ ✔️] Settings

Evidence:

Screen.Recording.2026-03-26.at.10.18.57.mov
20260326-1016-41.2819788.mp4

@github-project-automation github-project-automation bot moved this from With QA / Awaiting Review to In Progress in Explorer Alpha Mar 26, 2026
@popuz
Copy link
Copy Markdown
Collaborator

popuz commented Mar 26, 2026

✔️ PR reviewed and approved by QA on both platforms following instructions playing both happy and un-happy path

Regressions for this ticket had been performed in order to verify that the normal flow is working as expected:

  • [✔️ ] Log In/Log Out
  • [ ✔️] Backpack and wearables in world
  • [ ✔️] Emotes in world and in backpack
  • [ ✔️] Teleport with map/coordinates/Jump In
  • [ ✔️] Chat and multiplayer
  • [ ✔️] Profile card
  • [ ✔️] Camera
  • [ ✔️] Skybox
  • [ ✔️] Settings

Evidence:

Screen.Recording.2026-03-26.at.10.18.57.mov
20260326-1016-41.2819788.mp4

@DafGreco , did you try to disconnect in this specific time window when loading spinner is shown? So the test is

  1. you enter correct email
  2. during loading spinner you break the connection (you can slow down your internet to have bigger time window for disconnection actoin)
  3. I expect it will show that email is incorrect in this case (even that you enter a correct one and problem was in the internet connection)

@popuz
Copy link
Copy Markdown
Collaborator

popuz commented Mar 26, 2026

@claude review

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 26, 2026

Claude encountered an error —— View job


I'll analyze this and get back to you.

Copy link
Copy Markdown

@decentraland-bot decentraland-bot left a comment

Choose a reason for hiding this comment

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

Review Summary

Clean refactoring that splits the OTP login flow into two phases — SendOtpAsync (validates email, creates wallet, sends code) and the existing login flow (reuses pendingWallet if already created). The approach is sound. A few issues to address:

Issues

  1. AuthStatus not reset on error (Medium) — In OTPLoginAsync, when SendOtpAsync throws (non-cancellation), the method sets the error state on the input field and returns, but currentState.Value remains AuthStatus.LoginRequested. This may leave the UI in an inconsistent state (e.g. other buttons disabled). Consider resetting it back to LoginSelectionScreen in the catch blocks.

  2. Caught exception e is unused (Low) — In the generic catch (Exception e) block in OTPLoginAsync, the exception is silently swallowed. Consider logging it via ReportHub so that failures (network errors, ThirdWeb SDK errors, etc.) are traceable in production.

  3. pendingWallet lifetime / reuse concern (Medium)SendOtpAsync sets this.pendingWallet on ThirdWebLoginService. If the user tries to send OTP, it fails with a non-cancellation exception, and then the user tries again with a different email — the old pendingWallet is overwritten, which is fine. But if the user cancels (OperationCanceledException), pendingWallet could be left in a partially-initialized state (wallet created but OTP not sent). On a subsequent call, OTPLoginFlowAsync would see pendingWallet != null and skip SendOtpAsync, proceeding to wait for an OTP that was never sent. Consider nullifying pendingWallet on cancellation in SendOtpAsync, or always checking wallet state before reuse.

  4. Minor: email! null-forgiving in OTPLoginFlowAsync — The email! on line await SendOtpAsync(email!, ct) relies on the caller having validated the email. SendOtpAsync already checks for null/empty and throws, so this is safe at runtime, but it might be clearer to validate earlier or document the contract.

@AnsisMalins
Copy link
Copy Markdown
Collaborator

PR 7802

PR number: 7802
Windows change build
Windows baseline build
macOS change build
macOS baseline build

Intel Core i3

Change samples: 2668
Baseline samples: 2670

Metric Change Baseline Delta Improvement
CPU average 33.7 ms 33.7 ms 0.0 ms -0.0% 🔴
CPU 1% worst 56.3 ms 55.5 ms 0.8 ms -1.5% 🔴
CPU 0.1% worst 150.0 ms 125.5 ms 24.5 ms -19.5% 🔴
GPU average 16.4 ms 16.1 ms 0.3 ms -1.6% 🔴
GPU 1% worst 21.8 ms 21.8 ms 0.0 ms -0.0% 🔴
GPU 0.1% worst 25.4 ms 25.3 ms 0.1 ms -0.5% 🔴

@lorenzo-ranciaffi lorenzo-ranciaffi requested a review from popuz March 27, 2026 10:06
@lorenzo-ranciaffi lorenzo-ranciaffi moved this from In Progress to With QA / Awaiting Review in Explorer Alpha Mar 27, 2026
@lorenzo-ranciaffi
Copy link
Copy Markdown
Contributor Author

@claude re-review

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 27, 2026

Claude finished @lorenzo-ranciaffi's task in 3m 57s —— View job


Re-review of PR #7802

  • Gather context and read changed files
  • Check if previous review issues were addressed
  • Assess code quality per CLAUDE.md standards
  • Post final review

Previous issues addressed ✅

All main concerns from the prior review have been resolved:

Concern Status
Move InvalidEmailException handling to IdentityVerificationOTPAuthState (mirror dApp pattern) ✅ Done
Specific catch (InvalidEmailException) added ✅ Done
Use ErrorType.INVALID_EMAIL for machine transition ✅ Done
AuthStatus not reset after error ✅ Fixed — Enter(ErrorType.INVALID_EMAIL)Enter() resets to LoginSelectionScreen
Exception silently swallowed without logging ✅ Fixed — Exit() logs via ReportHub.LogException for non-cancellation exceptions
pendingWallet stale on cancellation ✅ Not an issue — OTPLoginFlowAsync always overwrites pendingWallet at the top; no reuse check

Network disconnect scenario (@popuz concern)

The concern about a broken connection showing "invalid email" instead of a connection error is correctly handled: ContainsInvalidEmailError checks for the exact ThirdWeb message "Invalid email." (traversing inner exceptions). A network error will have a different message and falls through to the general catch (Exception e)ErrorType.CONNECTION_ERROR. ✅

One minor nit

Left an inline suggestion on line 191 — viewInstance.LoginSelectionAuthView.EmailInputField.Text should use the cached view field instead.


REVIEW_RESULT: PASS ✅

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 27, 2026

Claude finished @lorenzo-ranciaffi's task in 4m 26s —— View job


PR #7802 Review

  • Gather context
  • Read changed files
  • Analyze for issues
  • Post review

Review

All previous concerns have been addressed. One minor issue remains:

pendingWallet not nullified after InvalidEmailException (ThirdWebLoginService.cs)

In OTPLoginFlowAsync, after InvalidEmailException is thrown, pendingWallet holds a reference to a wallet created by InAppWallet.Create but which never sent an OTP. LogoutAsync doesn't clear pendingWallet. On the next login attempt this is overwritten safely, and SubmitOtpAsync/ResendOtpAsync can't be reached without a new OTP send, so there's no active bug. However, the orphaned wallet instance leaks between the error and the next login attempt.

Consider adding pendingWallet = null; in the InvalidEmailException catch inside OTPLoginFlowAsync, or in LogoutAsync:

catch (Exception ex) when (ContainsInvalidEmailError(ex))
{
    pendingWallet = null;
    throw new InvalidEmailException(ex.Message, ex);
}

This is low severity given the overwrite-on-next-call guarantee, but it makes the invariant explicit: pendingWallet is only non-null when an OTP has been successfully sent and is awaiting submission.


REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX

@lorenzo-ranciaffi
Copy link
Copy Markdown
Contributor Author

@claude re-review and approve if passes

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 27, 2026

Claude finished @lorenzo-ranciaffi's task in 1m 17s —— View job


Re-review of PR #7802

  • Gather context from previous reviews
  • Read changed files
  • Check if prior issues were addressed
  • Post final review

Previous issues addressed ✅

All concerns from the prior reviews have been resolved:

Concern Status
Move InvalidEmailException handling to IdentityVerificationOTPAuthState ✅ Done — specific catch (InvalidEmailException e) at line 124
Use ErrorType.INVALID_EMAIL for machine transition ✅ Done — machine.Enter<LoginSelectionAuthState, ErrorType>(ErrorType.INVALID_EMAIL)
AuthStatus not reset after error ✅ Fixed — Enter(ErrorType.INVALID_EMAIL) calls Enter() which sets currentState.Value = AuthStatus.LoginSelectionScreen and clears the spinner
Exception silently swallowed without logging ✅ Fixed — Exit() in IdentityVerificationOTPAuthState calls ReportHub.LogException for all non-cancellation exceptions
pendingWallet stale on cancellation ✅ Not an issue — OTPLoginFlowAsync always overwrites pendingWallet = await InAppWallet.Create(...) at the top; stale state is impossible
viewInstance.LoginSelectionAuthView.EmailInputField.Text nit ✅ Fixed — OTPLogin() now correctly uses view.EmailInputField.Text
Network disconnect showing "invalid email" (@popuz concern) ✅ Correct — ContainsInvalidEmailError checks for the exact ThirdWeb string "Invalid email." via inner exception traversal; network errors will have a different message and fall through to ErrorType.CONNECTION_ERROR

No blocking issues found.

REVIEW_RESULT: PASS ✅

@github-actions
Copy link
Copy Markdown
Contributor

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: With QA / Awaiting Review

Development

Successfully merging this pull request may close these issues.

[QA] Email+ OTP | Connection modal is appearing when adding an invalid email

6 participants