-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
feat: sign in with passcode #7685
feat: sign in with passcode #7685
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 23 out of 43 changed files in this pull request and generated no comments.
Files not reviewed (20)
- frontend/appflowy_flutter/lib/startup/tasks/app_widget.dart: Language not supported
- frontend/appflowy_flutter/lib/startup/tasks/appflowy_cloud_task.dart: Language not supported
- frontend/appflowy_flutter/lib/user/application/auth/af_cloud_auth_service.dart: Language not supported
- frontend/appflowy_flutter/lib/user/application/auth/af_cloud_mock_auth_service.dart: Language not supported
- frontend/appflowy_flutter/lib/user/application/auth/auth_service.dart: Language not supported
- frontend/appflowy_flutter/lib/user/application/auth/backend_auth_service.dart: Language not supported
- frontend/appflowy_flutter/lib/user/application/sign_in_bloc.dart: Language not supported
- frontend/appflowy_flutter/lib/user/application/user_service.dart: Language not supported
- frontend/appflowy_flutter/lib/user/presentation/screens/sign_in_screen/desktop_sign_in_screen.dart: Language not supported
- frontend/appflowy_flutter/lib/user/presentation/screens/sign_in_screen/mobile_sign_in_screen.dart: Language not supported
- frontend/appflowy_flutter/lib/user/presentation/screens/sign_in_screen/widgets/anonymous_sign_in_button.dart: Language not supported
- frontend/appflowy_flutter/lib/user/presentation/screens/sign_in_screen/widgets/anonymous_sign_in_button/anonymous_sign_in_button.dart: Language not supported
- frontend/appflowy_flutter/lib/user/presentation/screens/sign_in_screen/widgets/continue_with/continue_with_email.dart: Language not supported
- frontend/appflowy_flutter/lib/user/presentation/screens/sign_in_screen/widgets/continue_with/continue_with_email_and_password.dart: Language not supported
- frontend/appflowy_flutter/lib/user/presentation/screens/sign_in_screen/widgets/continue_with/continue_with_magic_link_or_passcode_page.dart: Language not supported
- frontend/appflowy_flutter/lib/user/presentation/screens/sign_in_screen/widgets/continue_with/continue_with_password.dart: Language not supported
- frontend/appflowy_flutter/lib/user/presentation/screens/sign_in_screen/widgets/continue_with/continue_with_password_page.dart: Language not supported
- frontend/appflowy_flutter/lib/user/presentation/screens/sign_in_screen/widgets/logo/logo.dart: Language not supported
- frontend/appflowy_flutter/lib/user/presentation/screens/sign_in_screen/widgets/magic_link_sign_in_buttons.dart: Language not supported
- frontend/appflowy_flutter/lib/user/presentation/screens/sign_in_screen/widgets/sign_in_agreement.dart: Language not supported
Reviewer's Guide by SourceryThis pull request introduces the ability to sign in using a passcode, in addition to existing methods like email/password, OAuth, magic link, and guest access. The implementation spans across the frontend (Flutter) and backend (Rust) components, including UI changes, event handling, and service implementations. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
🥷 Ninja i18n – 🛎️ Translations need to be updatedProject
|
lint rule | new reports | level | link |
---|---|---|---|
Missing translation | 290 | warning | contribute (via Fink 🐦) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @LucasXu0 - I've reviewed your changes - here's some feedback:
Overall Comments:
- The event names in
SignInEvent
should be more descriptive, for example,signInWithEmailAndPassword
is better thansignedInWithUserEmailAndPassword
. - Consider grouping the sign-in methods and event handlers in
SignInEvent
using asealed class
or similar construct for better organization.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
void _pushContinueWithMagicLinkOrPasscodePage( | ||
BuildContext context, | ||
String email, | ||
) { | ||
if (!isEmail(email)) { | ||
return showToastNotification( | ||
context, | ||
message: LocaleKeys.signIn_invalidEmail.tr(), | ||
type: ToastificationType.error, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question (bug_risk): Navigation logic in email continuation flow.
Before pushing the ContinueWithMagicLinkOrPasscodePage, a magic link event is dispatched. It would be good to ensure that any potential asynchronous outcomes (like network delays) are handled gracefully, so users don’t get confused if the deep-link isn’t immediately processed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have skipped reviewing this pull request. It looks like we've already reviewed the commit adbf5e6 in this pull request.
b22a347
to
4840f15
Compare
4840f15
to
0344afd
Compare
250edcb
to
6cc48e6
Compare
6cc48e6
to
4341102
Compare
* feat: sign in with password or passcode * feat: add loading dialog * chore: update translations * feat: support otp on mobile
Feature Preview
PR Checklist