-
Notifications
You must be signed in to change notification settings - Fork 106
Now on SignUp/SignIn shows correct message #214
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Pranjali Bhardwaj <pranjalisharma6543@gmail.com>
Signed-off-by: Pranjali Bhardwaj <pranjalisharma6543@gmail.com>
WalkthroughBackend database utilities show no functional changes. The frontend slider component now supports multi-thumb rendering based on array values. Signup page implements improved user feedback with messages for existing accounts and verification instructions for new registrations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Frontend/src/pages/Signup.tsx (1)
52-77: Good handling of Supabase's existing-user quirk, but type safety could be improved.The logic correctly handles both the explicit error case and the identities-empty quirk. However,
as anybypasses type safety. Supabase'sUsertype already includesidentities?: UserIdentity[].- const identities = (data.user as any)?.identities ?? []; + const identities = data.user?.identities ?? [];Also note that relying on
error.message.toLowerCase().includes(...)is fragile if Supabase changes their error messages. Consider adding a comment documenting this dependency for future maintainers.Frontend/src/components/ui/slider.tsx (2)
8-12: Multi-thumb calculation logic is correct.The calculation correctly determines thumb count from array values. Minor simplification possible:
- const thumbCount = - Array.isArray(props.value ?? props.defaultValue) - ? (props.value ?? props.defaultValue ?? []).length || 1 - : 1 + const valueArray = props.value ?? props.defaultValue; + const thumbCount = Array.isArray(valueArray) ? valueArray.length || 1 : 1;
31-31: Consider adding displayName for better DevTools debugging.ForwardRef components commonly set
displayNamefor React DevTools clarity.}) +Slider.displayName = SliderPrimitive.Root.displayName export { Slider };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Backend/app/db/db.py(1 hunks)Frontend/src/components/ui/slider.tsx(1 hunks)Frontend/src/pages/Signup.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Saahi30
Repo: AOSSIE-Org/InPactAI PR: 98
File: Frontend/src/pages/CollaborationDetails.tsx:519-521
Timestamp: 2025-07-12T20:28:05.017Z
Learning: In frontend-only commits that use mock data, hardcoded progress values and similar placeholder data are acceptable and intentional. These values serve as placeholders for UI development and will be replaced with dynamic calculations once backend integration occurs.
🧬 Code graph analysis (2)
Frontend/src/pages/Signup.tsx (1)
Frontend/src/utils/supabase.tsx (1)
supabase(11-11)
Frontend/src/components/ui/slider.tsx (1)
Frontend/src/lib/utils.ts (1)
cn(4-6)
🔇 Additional comments (5)
Backend/app/db/db.py (1)
38-40: No functional changes detected.This appears to be a whitespace or formatting-only change. The
get_db()async generator implementation is correct and follows standard SQLAlchemy async session management patterns.Note: This file seems unrelated to the PR's objective of improving SignUp/SignIn messages. If this change was unintentional, consider excluding it from the PR to keep the changeset focused.
Frontend/src/pages/Signup.tsx (2)
20-20: LGTM!Adding the
successMessagestate to provide user feedback after successful signup attempts is a clean approach.
170-175: Nice success message UI with appropriate visual styling.The green info box with the checkmark icon provides clear visual feedback. The styling is consistent with the error message block above, and the
flex-shrink-0on the icon prevents layout issues with longer messages.Frontend/src/components/ui/slider.tsx (2)
23-28: LGTM on multi-thumb rendering.Using
Array.fromwith index-based keys is appropriate here since thumb order corresponds to stable array positions.
5-31: Verify: Is this slider change intentionally part of this PR?This PR's objectives focus on SignUp/SignIn messaging improvements. The slider multi-thumb support appears unrelated. Please confirm this change was intentionally included, or if it should be a separate PR.
| } | ||
| setIsLoading(true); | ||
| setError(""); | ||
| setSuccessMessage(""); |
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.
Minor indentation inconsistency.
There's an extra leading space before setSuccessMessage("").
- setSuccessMessage("");
+ setSuccessMessage("");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setSuccessMessage(""); | |
| setSuccessMessage(""); |
🤖 Prompt for AI Agents
In Frontend/src/pages/Signup.tsx around line 43, there's an indentation
inconsistency: the line "setSuccessMessage(\"\");" has an extra leading space.
Remove the extra leading space so the call aligns with the surrounding code's
indentation (match the same indent level as adjacent statements) to keep
formatting consistent.
|
When I committed these changes into this PR, Slider changes were pushed too, but now I have removes all the changes of previous pr, now it is only about this one. |
Closes #213
🔧 Changes Made
after a successful supabase.auth.signUp, the Signup page now shows a green info box saying:
“Verification link sent. Please check your email to verify your account before signing in.”
if Supabase returns an error indicating the email is already registered / user exists, the page now shows:
“An account with this email already exists. Please sign in instead.”
📷 Screenshots or Visual Changes (if applicable)
✅ Checklist
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.