Add user registration UI #738
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces several updates to the Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (9)
ui/src/pages/join/components/SuccessStep.tsx (1)
8-17: Consider enhancing user feedback and accessibilityWhile the implementation is functional, consider these improvements for better user experience:
- Add a success message/icon to confirm registration completion
- Include aria-label for screen readers
- Add loading state handling during navigation
<Box sx={{ textAlign: "center" }}> + <Box sx={{ mb: 3 }}> + <Typography variant="h5" component="h1"> + Registration Complete! + </Typography> + <Typography color="text.secondary" sx={{ mt: 1 }}> + Your account has been successfully created. + </Typography> + </Box> <Button variant="contained" color="primary" onClick={() => router.push("/")} size="large" + aria-label="Start listening to orca sounds" > Start listening </Button> </Box>ui/src/graphql/mutations/registerWithPassword.graphql (1)
Line range hint
18-29: Consider enhancing error response structureThe error response structure looks good but consider adding:
- Field-specific validation rules in the error response
- Localization keys for error messages
errors { message code fields shortMessage vars + validationRules + i18nKey }ui/src/pages/join/components/FormActions.tsx (1)
3-7: Consider extending props interfaceThe type definition could be enhanced to:
- Support disabled states
- Allow custom button styling
- Include loading states
type FormActionsProps = { onSkip?: () => void; submitText?: string; skipText?: string; + isSubmitting?: boolean; + isDisabled?: boolean; + buttonProps?: Partial<ButtonProps>; };ui/src/pages/join/components/StepLayout.tsx (1)
15-36: Enhance accessibility and semantic structure.While the layout is clean, it could benefit from improved accessibility.
Consider these enhancements:
return ( <> <Head> <title>{pageTitle} | Orcasound</title> </Head> - <Container maxWidth="sm"> + <Container maxWidth="sm" component="main"> <Box sx={{ mt: 4, mb: 6 }}> - <Typography variant="h4" gutterBottom> + <Typography variant="h4" component="h1" gutterBottom> {title} </Typography> {description && ( - <Typography variant="body1" color="text.secondary"> + <Typography + variant="body1" + color="text.secondary" + component="p" + role="doc-subtitle"> {description} </Typography> )} </Box> - {children} + <div role="main"> + {children} + </div> </Container> </> );ui/src/pages/join/utils.tsx (1)
20-36: Consider enhancing type safety for field names.The
getFieldErrorPropsfunction accepts any string as a field name, which could lead to runtime errors if mistyped.Consider making the field parameter type-safe:
export const getFieldErrorProps = < + T extends object, + K extends keyof T >( - field: string, + field: K, formError?: FieldError, mutationErrors?: MutationError[], ) => { const hasError = !!formError || mutationErrors?.some((e) => e.fields?.includes(field as string)); const errorMessage = formError?.message || mutationErrors?.find((e) => e.fields?.includes(field as string))?.message || ""; return { error: hasError, helperText: errorMessage || "", }; };ui/src/pages/join/components/PreferencesStep.tsx (1)
9-14: Consider adding constraints to the preference schemaWhile the boolean defaults are good, consider adding
.describe()to each field to document their purpose in the schema itself. This would improve self-documentation and could be used to generate API documentation.const preferencesSchema = z.object({ - liveNotifications: z.boolean().default(false), + liveNotifications: z.boolean().default(false).describe('Receive notifications for live orca detections'), - volunteering: z.boolean().default(false), + volunteering: z.boolean().default(false).describe('Interest in Orcasound volunteering opportunities'), - userTesting: z.boolean().default(false), + userTesting: z.boolean().default(false).describe('Willingness to participate in website testing'), - newsletter: z.boolean().default(false), + newsletter: z.boolean().default(false).describe('Opt-in for Orcasound newsletter'), });ui/src/pages/join/components/ProfileStep.tsx (1)
61-68: Add loading state for organization fieldThe conditional rendering of the organization field should handle loading state to prevent flicker.
-{isMarineScientist && ( +{isMarineScientist && ( <TextField {...register("organization")} label="Organization" fullWidth helperText="Your professional affiliation" + error={!!errors.organization} + helperText={errors.organization?.message || "Your professional affiliation"} + inputProps={{ + 'aria-label': 'Organization', + }} /> )}ui/src/pages/join/index.tsx (1)
17-41: Consider extracting step configurationsThe step configurations object is quite large and could be moved to a separate file for better maintainability.
Consider creating a new file
stepConfigs.tsto house these configurations.ui/src/components/Header.tsx (1)
110-128: Consider adding aria-label for accessibilityThe Join button could benefit from improved accessibility attributes.
Add aria-label to both mobile and desktop Join buttons:
<Button variant="contained" startIcon={<Person />} color="primary" + aria-label="Create an account" sx={{ borderRadius: 8, }} > Join </Button>Also applies to: 266-287
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ui/package-lock.jsonis excluded by!**/package-lock.jsonui/src/graphql/generated/index.tsis excluded by!**/generated/**
📒 Files selected for processing (12)
ui/package.json(2 hunks)ui/src/components/Header.tsx(5 hunks)ui/src/graphql/mutations/registerWithPassword.graphql(1 hunks)ui/src/pages/join/components/AccountStep.tsx(1 hunks)ui/src/pages/join/components/FormActions.tsx(1 hunks)ui/src/pages/join/components/PreferencesStep.tsx(1 hunks)ui/src/pages/join/components/ProfileStep.tsx(1 hunks)ui/src/pages/join/components/StepLayout.tsx(1 hunks)ui/src/pages/join/components/SuccessStep.tsx(1 hunks)ui/src/pages/join/hooks/useSteps.ts(1 hunks)ui/src/pages/join/index.tsx(1 hunks)ui/src/pages/join/utils.tsx(1 hunks)
🔇 Additional comments (5)
ui/src/pages/join/components/SuccessStep.tsx (1)
12-12: Consider handling navigation errors
The navigation callback should handle potential errors to ensure users aren't left in a broken state.
ui/src/graphql/mutations/registerWithPassword.graphql (1)
5-5: Verify impact of optional username
Making username optional is a significant change that could affect existing code paths. Please ensure:
- Client-side validation is updated accordingly
- Database constraints are aligned
- Username generation logic is in place if none provided
✅ Verification successful
Let me gather more information about the recent database migrations and related tests to understand the username changes better.
Let me check the GraphQL schema and related code to understand how username handling is implemented.
Based on the gathered information, I can now provide a final response:
Optional username change is properly implemented
The codebase shows proper handling of optional usernames:
- Database migration
20240327003711_allow_username_nil.exsexplicitly allows null usernames - User schema in
accounts/user.exhasallow_nil?: truewith appropriate constraints - Username uniqueness is still enforced through
identity :unique_usernamewhen present - Tests demonstrate the registration flow remains functional
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for username validation patterns in the codebase
rg "username.*required|required.*username" --type typescript
# Look for username-related database migrations
fd -e exs -e sql | xargs rg -l "username"
Length of output: 580
Script:
#!/bin/bash
# Check the content of the recent username-related migrations
cat server/priv/repo/migrations/20240327003711_allow_username_nil.exs
cat server/priv/repo/migrations/20240122204313_make_username_non_null.exs
# Check username-related test cases
cat server/test/orcasite_web/graphql/accounts_test.exs
Length of output: 3507
Script:
#!/bin/bash
# Check GraphQL schema definitions and resolvers
fd -e ex | xargs rg -l "username"
# Look for username-related validation or generation logic
rg -A 5 "username" server/lib/
Length of output: 3877
ui/src/pages/join/hooks/useSteps.ts (1)
3-10: LGTM! Well-structured step definition using const assertions.
The use of const assertions provides excellent type safety for the step values.
ui/src/pages/join/components/StepLayout.tsx (1)
4-14: LGTM! Well-structured props interface.
The props are well-typed and provide good flexibility for different step content.
ui/package.json (1)
25-25: Verify version compatibility and check for security vulnerabilities
The new dependencies use caret ranges which could lead to compatibility issues:
@hookform/resolvers: ^3.9.1react-hook-form: ^7.54.0zod: ^3.24.1
Consider pinning these versions for better stability.
Also applies to: 41-41, 48-48
| <Box sx={{ display: "flex", gap: 2, mt: 2 }}> | ||
| {onSkip && ( | ||
| <Button variant="outlined" onClick={onSkip} fullWidth> | ||
| {skipText} | ||
| </Button> | ||
| )} | ||
| <Button type="submit" variant="contained" color="primary" fullWidth> | ||
| {submitText} | ||
| </Button> | ||
| </Box> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and user feedback
The implementation could be improved with:
- Loading states during form submission
- Disabled states to prevent double submission
- ARIA attributes for better screen reader support
<Box sx={{ display: "flex", gap: 2, mt: 2 }}>
{onSkip && (
<Button
variant="outlined"
onClick={onSkip}
fullWidth
+ disabled={isSubmitting}
+ aria-label={skipText}
>
{skipText}
</Button>
)}
<Button
type="submit"
variant="contained"
color="primary"
fullWidth
+ disabled={isSubmitting || isDisabled}
+ aria-label={submitText}
>
- {submitText}
+ {isSubmitting ? <CircularProgress size={24} /> : submitText}
</Button>
</Box>Committable suggestion skipped: line range outside the PR's diff.
ui/src/pages/join/hooks/useSteps.ts
Outdated
| export const useSteps = () => { | ||
| const [currentStep, setCurrentStep] = useState<Step>(steps.account); | ||
|
|
||
| const goToNextStep = () => { | ||
| setCurrentStep((prev) => | ||
| prev < steps.success ? ((prev + 1) as Step) : prev, | ||
| ); | ||
| }; | ||
|
|
||
| const skipToNextStep = () => { | ||
| goToNextStep(); | ||
| }; | ||
|
|
||
| return { | ||
| currentStep, | ||
| goToNextStep, | ||
| skipToNextStep, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing step navigation capabilities.
The current implementation has a few limitations:
skipToNextStepis identical togoToNextStep, making it redundant- No ability to navigate to previous steps
- Missing step validation before progression
Consider this enhanced implementation:
export const useSteps = () => {
const [currentStep, setCurrentStep] = useState<Step>(steps.account);
const goToNextStep = () => {
setCurrentStep((prev) =>
prev < steps.success ? ((prev + 1) as Step) : prev,
);
};
- const skipToNextStep = () => {
- goToNextStep();
- };
+ const goToPreviousStep = () => {
+ setCurrentStep((prev) =>
+ prev > steps.account ? ((prev - 1) as Step) : prev,
+ );
+ };
+
+ const skipToNextStep = () => {
+ setCurrentStep((prev) =>
+ prev < steps.success ? ((prev + 2) as Step) : prev,
+ );
+ };
return {
currentStep,
goToNextStep,
+ goToPreviousStep,
skipToNextStep,
};
};📝 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.
| export const useSteps = () => { | |
| const [currentStep, setCurrentStep] = useState<Step>(steps.account); | |
| const goToNextStep = () => { | |
| setCurrentStep((prev) => | |
| prev < steps.success ? ((prev + 1) as Step) : prev, | |
| ); | |
| }; | |
| const skipToNextStep = () => { | |
| goToNextStep(); | |
| }; | |
| return { | |
| currentStep, | |
| goToNextStep, | |
| skipToNextStep, | |
| }; | |
| }; | |
| export const useSteps = () => { | |
| const [currentStep, setCurrentStep] = useState<Step>(steps.account); | |
| const goToNextStep = () => { | |
| setCurrentStep((prev) => | |
| prev < steps.success ? ((prev + 1) as Step) : prev, | |
| ); | |
| }; | |
| const goToPreviousStep = () => { | |
| setCurrentStep((prev) => | |
| prev > steps.account ? ((prev - 1) as Step) : prev, | |
| ); | |
| }; | |
| const skipToNextStep = () => { | |
| setCurrentStep((prev) => | |
| prev < steps.success ? ((prev + 2) as Step) : prev, | |
| ); | |
| }; | |
| return { | |
| currentStep, | |
| goToNextStep, | |
| goToPreviousStep, | |
| skipToNextStep, | |
| }; | |
| }; |
ui/src/pages/join/utils.tsx
Outdated
| export const createFormSubmitHandler = <T extends object>( | ||
| form: UseFormReturn<T>, | ||
| onSubmit: (data: T) => void, | ||
| ) => { | ||
| return form.handleSubmit((data) => { | ||
| console.log("Form data:", data); | ||
| onSubmit(data); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Remove console.log from production code.
The form submission handler includes a console.log statement that should not be in production code.
Apply this change:
export const createFormSubmitHandler = <T extends object>(
form: UseFormReturn<T>,
onSubmit: (data: T) => void,
) => {
return form.handleSubmit((data) => {
- console.log("Form data:", data);
onSubmit(data);
});
};📝 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.
| export const createFormSubmitHandler = <T extends object>( | |
| form: UseFormReturn<T>, | |
| onSubmit: (data: T) => void, | |
| ) => { | |
| return form.handleSubmit((data) => { | |
| console.log("Form data:", data); | |
| onSubmit(data); | |
| }); | |
| }; | |
| export const createFormSubmitHandler = <T extends object>( | |
| form: UseFormReturn<T>, | |
| onSubmit: (data: T) => void, | |
| ) => { | |
| return form.handleSubmit((data) => { | |
| onSubmit(data); | |
| }); | |
| }; |
| const onSubmit = (data: PreferencesFormInputs) => { | ||
| // TODO: Implement preferences update mutation | ||
| onSuccess(); | ||
| }; |
There was a problem hiding this comment.
Implement the TODO for preferences update mutation
The preferences update mutation needs to be implemented. This is a critical piece for persisting user preferences.
Would you like me to help generate the GraphQL mutation implementation for updating user preferences?
| return ( | ||
| <form onSubmit={createFormSubmitHandler(form, onSubmit)} name="preferences"> | ||
| <Box sx={{ display: "flex", flexDirection: "column", gap: 3 }}> | ||
| <FormControlLabel | ||
| control={<Switch {...register("liveNotifications")} />} | ||
| label="Notify me when orcas are detected live" | ||
| /> | ||
|
|
||
| <FormControlLabel | ||
| control={<Switch {...register("volunteering")} />} | ||
| label="I'm interested in volunteering with Orcasound" | ||
| /> | ||
|
|
||
| <FormControlLabel | ||
| control={<Switch {...register("userTesting")} />} | ||
| label="I'd like to participate in website user testing" | ||
| /> | ||
|
|
||
| <FormControlLabel | ||
| control={<Switch {...register("newsletter")} />} | ||
| label="Subscribe to the Orcasound newsletter" | ||
| /> | ||
|
|
||
| <FormActions onSkip={onSkip} submitText="Complete setup" /> | ||
| </Box> | ||
| </form> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding loading state and error handling
The form submission lacks loading state and error handling. This could lead to a poor user experience if the mutation fails.
export const PreferencesStep = ({ onSuccess, onSkip }: StepFormProps) => {
- const { form, onSubmit } = usePreferencesForm(onSuccess);
- const { register } = form;
+ const { form, onSubmit } = usePreferencesForm(onSuccess);
+ const { register, formState: { isSubmitting, errors } } = form;
return (
<form onSubmit={createFormSubmitHandler(form, onSubmit)} name="preferences">
<Box sx={{ display: "flex", flexDirection: "column", gap: 3 }}>
{/* ... existing form controls ... */}
- <FormActions onSkip={onSkip} submitText="Complete setup" />
+ <FormActions
+ onSkip={onSkip}
+ submitText="Complete setup"
+ disabled={isSubmitting}
+ />
+ {errors.root && (
+ <Typography color="error">
+ {errors.root.message}
+ </Typography>
+ )}
</Box>
</form>
);
};Committable suggestion skipped: line range outside the PR's diff.
ui/src/pages/join/index.tsx
Outdated
| {currentStep === steps.account && ( | ||
| <AccountStep onSuccess={goToNextStep} /> | ||
| )} | ||
| {currentStep === steps.profile && ( | ||
| <ProfileStep onSuccess={goToNextStep} onSkip={skipToNextStep} /> | ||
| )} | ||
| {currentStep === steps.preferences && ( | ||
| <PreferencesStep onSuccess={goToNextStep} onSkip={skipToNextStep} /> | ||
| )} | ||
| {currentStep === steps.success && <SuccessStep />} | ||
| </StepLayout> |
There was a problem hiding this comment.
Consider handling invalid step states
The component doesn't handle cases where currentStep might be invalid.
Add error boundaries or fallback UI for invalid states:
+ if (currentStep < 1 || currentStep > Object.keys(steps).length) {
+ return <div>Invalid step</div>;
+ }
{currentStep === steps.account && (
<AccountStep onSuccess={goToNextStep} />
)}
// ... rest of the stepsCommittable suggestion skipped: line range outside the PR's diff.
| const onSubmit = (data: AccountFormInputs) => { | ||
| submitRegister.mutate({ | ||
| email: data.email, | ||
| password: data.password, | ||
| passwordConfirmation: data.password, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add loading state handling
The form submission doesn't handle loading states, which could lead to multiple submissions.
const onSubmit = (data: AccountFormInputs) => {
+ if (submitRegister.isLoading) return;
submitRegister.mutate({
email: data.email,
password: data.password,
passwordConfirmation: data.password,
});
};📝 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.
| const onSubmit = (data: AccountFormInputs) => { | |
| submitRegister.mutate({ | |
| email: data.email, | |
| password: data.password, | |
| passwordConfirmation: data.password, | |
| }); | |
| }; | |
| const onSubmit = (data: AccountFormInputs) => { | |
| if (submitRegister.isLoading) return; | |
| submitRegister.mutate({ | |
| email: data.email, | |
| password: data.password, | |
| passwordConfirmation: data.password, | |
| }); | |
| }; |
| onError: (error) => { | ||
| console.log("Register error", error); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error logging
The error logging could be more informative for debugging purposes.
- console.log("Register error", error);
+ console.error("Registration failed:", {
+ error,
+ timestamp: new Date().toISOString(),
+ context: "AccountStep registration"
+ });📝 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.
| onError: (error) => { | |
| console.log("Register error", error); | |
| }, | |
| }); | |
| onError: (error) => { | |
| console.error("Registration failed:", { | |
| error, | |
| timestamp: new Date().toISOString(), | |
| context: "AccountStep registration" | |
| }); | |
| }, | |
| }); |
| const accountSchema = z.object({ | ||
| email: z.string().min(1, "Email is required").email("Invalid email address"), | ||
| password: z | ||
| .string() | ||
| .min(1, "Password is required") | ||
| .min(8, "Password must be at least 8 characters") | ||
| .regex( | ||
| /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)/, | ||
| "Password must contain at least one uppercase letter, one lowercase letter, and one number", | ||
| ), | ||
| }); |
There was a problem hiding this comment.
Enhance password validation requirements
The current password validation might not be sufficient for security best practices.
Consider strengthening the password requirements:
.regex(
- /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)/,
- "Password must contain at least one uppercase letter, one lowercase letter, and one number",
+ /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[!@#$%^&*])/,
+ "Password must contain at least one uppercase letter, one lowercase letter, one number, and one special character",
)
+.max(128, "Password must not exceed 128 characters")📝 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.
| const accountSchema = z.object({ | |
| email: z.string().min(1, "Email is required").email("Invalid email address"), | |
| password: z | |
| .string() | |
| .min(1, "Password is required") | |
| .min(8, "Password must be at least 8 characters") | |
| .regex( | |
| /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)/, | |
| "Password must contain at least one uppercase letter, one lowercase letter, and one number", | |
| ), | |
| }); | |
| const accountSchema = z.object({ | |
| email: z.string().min(1, "Email is required").email("Invalid email address"), | |
| password: z | |
| .string() | |
| .min(1, "Password is required") | |
| .min(8, "Password must be at least 8 characters") | |
| .regex( | |
| /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[!@#$%^&*])/, | |
| "Password must contain at least one uppercase letter, one lowercase letter, one number, and one special character", | |
| ) | |
| .max(128, "Password must not exceed 128 characters"), | |
| }); |
ui/src/components/Header.tsx
Outdated
| // TODO(@paulcretu): Add analytics event | ||
| // onClick={() => analytics.nav.joinClicked()} |
There was a problem hiding this comment.
Implement analytics tracking
The TODO comment indicates missing analytics tracking for the Join button.
Implement the analytics tracking as indicated in the TODO comment:
-// TODO(@paulcretu): Add analytics event
-// onClick={() => analytics.nav.joinClicked()}
+onClick={() => analytics.nav.joinClicked()}📝 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.
| // TODO(@paulcretu): Add analytics event | |
| // onClick={() => analytics.nav.joinClicked()} | |
| onClick={() => analytics.nav.joinClicked()} |
Summary by CodeRabbit
Release Notes
New Features
react-hook-formandzod.Bug Fixes
registerWithPasswordmutation to allow the username to be optional.Documentation
Chores