Skip to content
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

refactor(frontend): Auth pages update #9124

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from

Conversation

kcze
Copy link
Contributor

@kcze kcze commented Dec 23, 2024

There are UX and design issues with current auth pages; login, signup and reset_password (including change password).

Changes 🏗️

auth
*Missing s on the login's password error is fixed.

Important changes in bold.

All auth pages

  • Split /login into /signup
  • UI Redesign that adheres to Figma designs
  • General code cleanup and improvements
  • Fix feedback: it's now shown when needed and clear (e.g. "String Password must be...")
  • All action functions use Sentry.withServerActionInstrumentation
  • PasswordInput "eye button" shows password only when mouse button is hold and doesn't capture tab

Login page

  • Removed agree to terms checkbox (it's only on signup now)
  • Move provider login function to actions.ts

Signup page

  • Requires to type password twice
  • Shows waitlist information on any database error

Reset password page

  • Password update requires to type password twice
  • When request to send email is processed then the feedback is: Password reset email sent if user exists. Please check your email.
  • Email sent feedback is black, error is red
  • Move send email and update password functions to actions.ts
  • Disable button when email is sent

Other

  • Update zod schema objects and move them to types/auth
  • Move components/PasswordInput.tsx to /components/auth
  • Make common UI elements separate components in components/auth
  • Update yarn.lock (supabase packages)
  • Remove redundant letter in client.ts
  • Don't log error when user auth is missing in useSupabase; user is simply not logged in

Checklist 📋

For code changes:

  • I have clearly listed my changes in the PR description
  • I have made a test plan
  • I have tested my changes according to the test plan:
    • Form feedback:
      • Login works
      • Signup works
      • Reset email works
      • Change password works
    • Login works
    • Signup works
    • Reset email is sent
    • Reset email logs user in and redirects to /reset_password
    • Change password works
    • Logout works
    • All links across auth pages work

Note: OAuth login providers are disabled and so untested.

Example test plan
  • Create from scratch and execute an agent with at least 3 blocks
  • Import an agent from file upload, and confirm it executes correctly
  • Upload agent to marketplace
  • Import an agent from marketplace and confirm it executes correctly
  • Edit an agent from monitor, and confirm it executes correctly

For configuration changes:

  • .env.example is updated or already compatible with my changes
  • docker-compose.yml is updated or already compatible with my changes
  • I have included a list of my configuration changes in the PR description (under Changes)
Examples of configuration changes
  • Changing ports
  • Adding new services that need to communicate with each other
  • Secrets or environment variable changes
  • New or infrastructure changes such as databases

@github-actions github-actions bot added platform/frontend AutoGPT Platform - Front end platform/backend AutoGPT Platform - Back end labels Dec 23, 2024
Copy link

netlify bot commented Dec 23, 2024

Deploy Preview for auto-gpt-docs-dev canceled.

Name Link
🔨 Latest commit 3693f79
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs-dev/deploys/676fc9f9826730000889809c

Copy link

netlify bot commented Dec 23, 2024

Deploy Preview for auto-gpt-docs ready!

Name Link
🔨 Latest commit 3693f79
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/676fc9f909e7bc000848275d
😎 Deploy Preview https://deploy-preview-9124--auto-gpt-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.


if (error) {
console.error("Error signing up", error);
// FIXME: supabase doesn't return the correct error message for this case
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't make supabase return actual error on database trigger, it returns unexpected_failure instead, so any error triggers waitlist prompt in signup/page.tsx.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we propagate the error either way or say something like "Internal/unknown error" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried fixing it but couldn't, supabase returns a generic database error instead of the custom one from the function (set in the supabase dashboard). This is just to show waitlist information (see screenshot).

@kcze kcze marked this pull request as ready for review December 23, 2024 18:05
@kcze kcze requested a review from a team as a code owner December 23, 2024 18:05
majdyz
majdyz previously approved these changes Dec 27, 2024
},
});

// const onProviderLogin = useCallback(async (
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is valid code that should be uncommented when we enable social login, I added a TODO.

{/* <div className="mb-6 space-y-2">
<AuthCard>
<AuthHeader>Login to your account</AuthHeader>
{/* <div className="mb-6 space-y-2">
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this since it's most likely going to look different anyway.

const headersList = headers();
const host = headersList.get("host");
const protocol =
process.env.NODE_ENV === "development" ? "http" : "https";
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we have a constant value for this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do on the backend, I don't see anything on the frontend but let me know if you have any idea.


if (error) {
console.error("Error signing up", error);
// FIXME: supabase doesn't return the correct error message for this case
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we propagate the error either way or say something like "Internal/unknown error" ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform/backend AutoGPT Platform - Back end platform/frontend AutoGPT Platform - Front end size/xl
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants