Feat: Feature signup page (Create SignUp Page & Integrate Backend API) [Issue #57] #81
Conversation
Summary of ChangesHello @Rohanrathod7, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a complete signup feature, enabling new user registration for the application. It encompasses a modern UI design, robust client-side and server-side validation using Zod, secure password hashing, and seamless API integration. The changes are thoroughly tested with both unit and integration tests, and comprehensive documentation is provided to explain the data flow and architecture. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive signup feature, including frontend UI, backend API integration, and extensive testing. The implementation is solid, with good separation of concerns and thorough test coverage. My feedback focuses on improving security, consistency, and test reliability. Key points include tightening the CORS policy, aligning validation logic between tests and implementation, and improving the robustness of integration tests. Overall, this is a great contribution that significantly advances the project's user authentication capabilities.
| password: z | ||
| .string() | ||
| .min(8, 'Password must be at least 8 characters') | ||
| .max(128, 'Password must be less than 128 characters') | ||
| .regex(/[A-Z]/, 'Must contain uppercase letter') | ||
| .regex(/[a-z]/, 'Must contain lowercase letter') | ||
| .regex(/[0-9]/, 'Must contain a number') | ||
| .regex(/[@$!%*?&]/, 'Must contain special character'), |
There was a problem hiding this comment.
The password validation regex in this test file is different from the one used in the SignUp.tsx component and the backend validator. This test uses /[@$!%*?&]/, while the component and backend use the more comprehensive /[!@#$%^&*(),.?":{}|<>]/. This discrepancy means your tests are not accurately validating the same password policy that is enforced in the application, which could lead to valid passwords being rejected by tests or vice-versa. Please update the regex in this test file to match the one in SignUp.tsx.
| password: z | |
| .string() | |
| .min(8, 'Password must be at least 8 characters') | |
| .max(128, 'Password must be less than 128 characters') | |
| .regex(/[A-Z]/, 'Must contain uppercase letter') | |
| .regex(/[a-z]/, 'Must contain lowercase letter') | |
| .regex(/[0-9]/, 'Must contain a number') | |
| .regex(/[@$!%*?&]/, 'Must contain special character'), | |
| password: z | |
| .string() | |
| .min(8, 'Password must be at least 8 characters') | |
| .max(128, 'Password must be less than 128 characters') | |
| .regex(/[A-Z]/, 'Must contain uppercase letter') | |
| .regex(/[a-z]/, 'Must contain lowercase letter') | |
| .regex(/[0-9]/, 'Must contain a number') | |
| .regex(/[!@#$%^&*(),.?":{}|<>]/, 'Must contain special character'), |
| logger.token('time', () => new Date().toLocaleString()) | ||
| app.use(logger(':time :method :url :status')) | ||
|
|
||
| app.use(cors({ origin: true, credentials: true })) |
There was a problem hiding this comment.
The current CORS configuration is too permissive for a production environment. Using origin: true reflects the request's origin, which can introduce security risks by allowing requests from any domain. It is highly recommended to restrict this to a specific list of allowed origins, preferably loaded from an environment variable.
| app.use(cors({ origin: true, credentials: true })) | |
| app.use(cors({ origin: [env.FRONTEND_URL], credentials: true })) |
| it('should successfully login with valid credentials', async () => { | ||
| try { | ||
| const res = await axios.post(`${API_URL}/user/login`, { | ||
| email: loginTestEmail, | ||
| password: validPassword, | ||
| }) | ||
|
|
||
| expect(res.status).toBe(200) | ||
| expect(res.data).toBeDefined() | ||
| expect(res.data.message).toBeDefined() | ||
| } catch (error: any) { | ||
| // User might not exist in test environment | ||
| if (error.response?.status === 401 || error.response?.status === 404) { | ||
| console.log('Test user not found, skipping login success test') | ||
| expect(true).toBe(true) | ||
| } else { | ||
| throw error | ||
| } | ||
| } | ||
| }, 10000) |
There was a problem hiding this comment.
This test for a successful login is a bit fragile. It includes conditional logic and a console.log to handle the case where the test user might not exist. A better practice for integration tests is to ensure a consistent state before each test run.
Consider using a beforeAll or beforeEach hook to create the necessary test user. This will make your test more deterministic and remove the need for conditional logic and logging within the test itself.
For example:
describe('Login Endpoint Tests', () => {
const loginTestEmail = env.YOUR_EMAIL || 'test@example.com';
const validPassword = 'Test@1234';
beforeAll(async () => {
// Ensure the test user exists, create if not.
await UserUtils.ensureTestUserExists(loginTestEmail, validPassword);
});
it('should successfully login with valid credentials', async () => {
const res = await axios.post(`${API_URL}/user/login`, {
email: loginTestEmail,
password: validPassword,
});
expect(res.status).toBe(200);
expect(res.data).toBeDefined();
expect(res.data.message).toBeDefined();
}, 10000);
// ... other tests
});| "data": { | ||
| "userId": "abc123", | ||
| "username": "john_doe", | ||
| "email": "john@example.com", | ||
| "user": { | ||
| "firstName": "John", | ||
| "email": "john@example.com", | ||
| "role": "user", | ||
| "createdAt": "2024-01-15T10:30:00Z" | ||
| }, | ||
| "token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9..." | ||
| } |
There was a problem hiding this comment.
The response example for the "Register User" endpoint shows the user data under a user key. However, the corresponding backend test (user.test.ts) asserts that this data is under a userObj key. Please ensure the documentation matches the actual API response for consistency.
| "data": { | |
| "userId": "abc123", | |
| "username": "john_doe", | |
| "email": "john@example.com", | |
| "user": { | |
| "firstName": "John", | |
| "email": "john@example.com", | |
| "role": "user", | |
| "createdAt": "2024-01-15T10:30:00Z" | |
| }, | |
| "token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9..." | |
| } | |
| "data": { | |
| "userObj": { | |
| "firstName": "John", | |
| "email": "john@example.com", | |
| "role": "user", | |
| "createdAt": "2024-01-15T10:30:00Z" | |
| }, | |
| "token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9..." | |
| } |
| <Input | ||
| label="Role" | ||
| name="role" | ||
| type="text" | ||
| placeholder="e.g., Product Manager" | ||
| value={formData.role} | ||
| onChange={handleChange} | ||
| error={errors.role} | ||
| disabled={loading} | ||
| /> |
There was a problem hiding this comment.
The form includes an input field for role, but the handleSubmit function hardcodes the role to 'user' in the payload sent to the backend. This is confusing for the user, as their input in the role field is ignored. If the role is not meant to be user-configurable during signup, this input field should be removed from the form to avoid confusion. After removing it, you may also want to adjust the grid layout for the 'Full Name' field to span the full width.
| .regex(/[0-9]/, 'Must contain a number') | ||
| .regex(/[@$!%*?&]/, 'Must contain special character'), | ||
| portfolioUrl: z.string().url('Please enter a valid URL').or(z.literal('')), | ||
| bio: z.string().max(50, 'Bio must be less than 50 characters'), |
There was a problem hiding this comment.
The Zod schema for bio in this test file is missing the .min(5, ...) validation that is present in the SignUp.tsx component. This can lead to tests passing for inputs that would fail in the actual component. Please ensure the validation schemas are identical to maintain consistency between your tests and the implementation.
| bio: z.string().max(50, 'Bio must be less than 50 characters'), | |
| bio: z.string().min(5, 'Bio must be at least 5 characters').max(50, 'Bio must be less than 50 characters'), |
|
Create SignUp page Similar to Sign In page make sure the signup page match withLogin page 
|
|
|
Just Updated Sign up page it also includes a little Password Reset functionality code in the same branch. Technically, the code works perfectly and I've resolved the merge conflicts with the latest master. I actually have the full Password Reset feature ready in a separate branch with updated UI for forgot reset page #89 |
|
@Rohanrathod7 add margin on top of the form and make sure the website is Fully Responsive in all screen size |
…nsiveness across screen sizes
|
updated top margin and responsiveness |
2 participants
Signup
Closes #57
Feature Implementation - Walkthrough
Key Changes
Frontend (LocalMind-Frontend)
sign**: Implemented a modern, split-screen split layout with a promotional image and a clean form interface using SignUp.tsx.
Zodschema validation:Rolefield added.POST /api/v1/auth/signupBackend (
LocalMind-Backend)POST /api/v1/auth/signup.user.validator.tsmatches frontend rules.firstNameis used consistently (replacedfullName).Documentation
🧪 Verification Results
Automatic Tests
Manual Verification
PR Checklist