Feature/complete onboarding form #1216
Conversation
|
@John-Paul-Larkin is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request introduces several enhancements to the onboarding process within the application. Key changes include the integration of a Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
John-Paul-Larkin
temporarily deployed
to
production
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (11)
drizzle/0011_years-experience-user-table.sql (1)
1-3: Consider adding indexes for performanceGiven that these columns will be used in login-time checks and redirects, consider adding indexes to optimize query performance:
onboardingCompletewill be frequently queried during session validation- Combined index might be beneficial if querying patterns commonly include both new columns
Consider adding these indexes in a follow-up migration:
CREATE INDEX idx_user_onboarding ON "user" ("onboardingComplete"); CREATE INDEX idx_user_onboarding_combined ON "user" ("onboardingComplete", "yearsOfExperience");types/next-auth.d.ts (2)
Line range hint
18-18: Fix the nullable type syntaxThe syntax
?stringis incorrect for TypeScript. To make the username property nullable, use eitherstring | nullorstring | undefineddepending on your requirements.Apply this fix:
- username: ?string; + username: string | null; // or string | undefined
Line range hint
18-21: Add missing yearsOfExperience field to User interfaceThe PR adds a 'yearsOfExperience' field to the user table, but this isn't reflected in the User interface. For type safety, this field should be added to the interface.
Apply this addition:
interface User extends DefaultUser { username: string | null; role: Role; id: string; + yearsOfExperience: string | null; }context/AuthProvider.tsx (2)
22-23: Remove debug console.log statementDebug logging should be removed before merging to production.
- console.log("-------------");
30-30: Consider adding a loading state to the component renderThe component currently renders children regardless of the authentication/onboarding state, which could lead to flash of incorrect content.
- return <>{children}</>; + if (status === "loading") { + return null; // or a loading spinner + } + + return <>{children}</>;schema/additionalUserDetails.ts (1)
73-82: Improve validation error message and maintainabilityWhile the validation logic is correct, consider these improvements:
- Use a more descriptive error message
- Consider extracting common validation patterns
if ( val.professionalOrStudent === "Working professional" && val.yearsOfExperience === "" ) { ctx.addIssue({ path: ["yearsOfExperience"], code: "custom", - message: "required", + message: "Please select your years of experience", }); }Consider extracting the validation logic into a helper function to reduce repetition:
const validateRequiredField = ( ctx: z.RefinementCtx, condition: boolean, field: string, message: string ) => { if (condition) { ctx.addIssue({ path: [field], code: "custom", message, }); } };app/(app)/onboarding/_actions.ts (1)
Line range hint
18-119: Consider refactoring to reduce code duplication across slide actionsAll three slide submission actions share identical patterns for session validation, error handling, and database updates. Consider extracting these common patterns into a higher-order function.
Here's a suggested approach:
async function createSlideAction<T>( schema: z.ZodSchema<T>, updateFields: (data: T) => Partial<typeof user.$inferSelect> ) { return async (dataInput: T) => { const session = await getServerAuthSession(); if (!session?.user) redirect("/get-started"); try { const data = schema.parse(dataInput); await db .update(user) .set(updateFields(data)) .where(eq(user.id, session.user.id)); return true; } catch (error) { if (error instanceof z.ZodError) { console.error("Validation error:", error.errors); } else { console.error("Error updating the User model:", error); } return false; } }; } // Usage example: export const slideThreeSubmitAction = createSlideAction( slideThreeSchema, (data) => ({ ...data, onboardingComplete: new Date().toISOString(), }) );server/auth.ts (1)
72-73: Improve type safety and documentation for onboarding status.The conversion of onboardingComplete to a boolean could be more explicit and type-safe.
- // onboardingComplete is an iso date string - if it exists then it is complete - session.user.isOnboardingComplete = !!userStatus?.onboardingComplete; + // onboardingComplete is a timestamp indicating when onboarding was completed + // null/undefined values are converted to false, indicating incomplete onboarding + session.user.isOnboardingComplete = userStatus?.onboardingComplete instanceof Date;Also, consider extending the session type to include the new field:
declare module "next-auth" { interface Session { user: { isOnboardingComplete: boolean; // ... other fields }; } }server/db/schema.ts (1)
Line range hint
228-228: Remove redundant index on usernameThe
usernameIndexappears to be redundant as theusernamefield is already covered by:
- A unique index
usernameKey- A compound index
usernameIdIdxHaving multiple indices on the same column can:
- Slow down write operations
- Increase storage space
- Make the query planner's job more complex
return { usernameKey: uniqueIndex("User_username_key").on(table.username), emailKey: uniqueIndex("User_email_key").on(table.email), usernameIdIdx: index("User_username_id_idx").on(table.id, table.username), - usernameIndex: index("User_username_index").on(table.username), // Add this line };app/(app)/onboarding/_client.tsx (2)
438-438: Consider improving the years of experience rangesThe current implementation has some potential issues:
- There's a gap in the ranges (8-12 years is not covered)
- The "12+" format differs from other ranges, which might be confusing
- These options could be reused elsewhere in the application
Consider these improvements:
- const yearsOfExperienceOptions = ["0-1", "1-3", "3-5", "5-8", "12+"] as const; + export const yearsOfExperienceOptions = [ + "0-1", + "1-3", + "3-5", + "5-8", + "8-12", + "12-15", + "15+" + ] as const;Also applies to: 460-461
526-546: LGTM with a minor suggestion for the placeholder textThe implementation follows the established patterns and properly handles all states. Consider making the placeholder more specific for better user experience.
- <option value="" disabled> - Select range - </option> + <option value="" disabled> + Select years of experience + </option>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
drizzle/meta/0011_snapshot.jsonis excluded by!**/*.jsondrizzle/meta/_journal.jsonis excluded by!**/*.json
📒 Files selected for processing (10)
app/(app)/onboarding/_actions.ts(2 hunks)app/(app)/onboarding/_client.tsx(7 hunks)app/(app)/onboarding/page.tsx(2 hunks)context/AuthProvider.tsx(1 hunks)drizzle/0011_years-experience-user-table.sql(1 hunks)e2e/setup.ts(1 hunks)schema/additionalUserDetails.ts(2 hunks)server/auth.ts(1 hunks)server/db/schema.ts(1 hunks)types/next-auth.d.ts(1 hunks)
🔇 Additional comments (14)
drizzle/0011_years-experience-user-table.sql (1)
1-1: Verify existing session data before applying PRIMARY KEY constraint
Adding a PRIMARY KEY constraint could fail if duplicate session tokens or NULL values exist in the database.
✅ Verification successful
PRIMARY KEY constraint addition verified: no duplicate or NULL sessionTokens found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential issues before adding PRIMARY KEY constraint
# Check for duplicate sessionTokens
echo "Checking for duplicate sessionTokens that could break the migration..."
ast-grep --pattern 'SELECT "sessionToken", COUNT(*) as count
FROM "session"
GROUP BY "sessionToken"
HAVING COUNT(*) > 1;'
# Check for NULL sessionTokens
echo "Checking for NULL sessionTokens that could break the migration..."
ast-grep --pattern 'SELECT COUNT(*)
FROM "session"
WHERE "sessionToken" IS NULL;'
Length of output: 504
types/next-auth.d.ts (1)
14-14: LGTM: Session interface enhancement
The addition of isOnboardingComplete boolean flag to the Session interface aligns well with the PR objectives and properly types the onboarding status tracking within the session.
context/AuthProvider.tsx (2)
1-16: LGTM! Clean implementation of the AuthProvider component.
The component follows React best practices with proper provider nesting and type definitions.
18-30: Verify the onboarding redirect logic across the application
The client-side onboarding check might need to be complemented with server-side validation to ensure security and consistency.
app/(app)/onboarding/page.tsx (2)
41-41: Consider handling null yearsOfExperience differently for working professionals.
While using an empty string as a default value matches the pattern of other fields, for working professionals this field should likely have a meaningful value. Consider adding validation or a different default for this specific user type.
Let's check how this field is validated:
#!/bin/bash
# Description: Check validation logic for yearsOfExperience
# Expected: Find validation schema and form handling
# Look for validation schema
rg -A 5 "yearsOfExperience.*schema"
# Check form submission handling
ast-grep --pattern 'function $FUNC($_, $_) {
$$$
yearsOfExperience
$$$
}'25-25: LGTM! Database query includes the new field.
The addition of yearsOfExperience to the query columns is consistent with the PR objectives.
Let's verify the database schema for this field:
✅ Verification successful
Database schema correctly includes the yearsOfExperience field.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the database schema migration for yearsOfExperience field
# Expected: Find migration file and verify field definition
# Look for migration files containing years-experience
fd -e sql years-experience
# Check field definition in migration files
rg -A 5 "yearsOfExperience"
Length of output: 214
Script:
#!/bin/bash
# Description: Check the database schema migration for yearsOfExperience field
# Expected: Find migration file and verify field definition
# Look for migration files containing years-experience
fd -e sql years-experience
# Check field definition in migration files
rg -A 5 "yearsOfExperience"
Length of output: 6182
schema/additionalUserDetails.ts (1)
Line range hint 33-82: Verify schema usage in the frontend components
The schema changes look good and align with the PR objectives. Let's verify the frontend implementation:
✅ Verification successful
Schema integration in frontend verified successfully. No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using slideThreeSchema and yearsOfExperience field
rg -l "slideThreeSchema|yearsOfExperience" --type tsx
# Check for any hardcoded years of experience values that should match the schema
rg -A 2 -B 2 "yearsOfExperience.*select|select.*yearsOfExperience" --type tsx
Length of output: 191
Script:
#!/bin/bash
# Search for components using slideThreeSchema and yearsOfExperience field
rg "slideThreeSchema|yearsOfExperience" --glob "*.tsx"
# Check for any hardcoded years of experience values that should match the schema
rg -A 2 -B 2 "yearsOfExperience.*select|select.*yearsOfExperience" --glob "*.tsx"
Length of output: 1184
app/(app)/onboarding/_actions.ts (1)
83-90: Verify schema validation for yearsOfExperience
Ensure that the slideThreeSchema properly validates the yearsOfExperience field based on business requirements.
server/auth.ts (1)
65-73: Verify session callback timing and race conditions.
The implementation checks onboarding status in the session callback, but we should verify:
- That the session is refreshed appropriately when onboarding status changes
- No race conditions exist between onboarding completion and session updates
e2e/setup.ts (1)
124-124: LGTM! Verify schema compatibility.
The addition of onboardingComplete with ISO timestamp format is correct and aligns with the PR's objective of tracking onboarding completion status. This ensures E2E test users bypass the onboarding flow appropriately.
Let's verify the database schema matches this implementation:
✅ Verification successful
Schema compatibility verified.
The onboardingComplete field in the database schema is correctly defined as a timestamp with time zone, matching the ISO string format used in the test setup. This ensures consistency between the test data and the database.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the database schema has the onboardingComplete field with matching type
# Expected: Should find the field definition in the migration file
# Look for the field definition in migration files
fd -e sql . | xargs rg -A 3 'onboardingComplete'
Length of output: 170
server/db/schema.ts (1)
220-224: Verify migration for schema changes
These schema changes require a migration. Let's verify its existence and content.
✅ Verification successful
Migration for schema changes exists and includes the necessary additions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for migration file adding yearsOfExperience and onboardingComplete fields
# Look for migration files
fd -e sql . | while read -r file; do
echo "Checking $file for schema changes..."
rg -A 5 "ALTER TABLE.*user" "$file"
rg -A 5 "yearsOfExperience|onboardingComplete" "$file"
done
Length of output: 21897
app/(app)/onboarding/_client.tsx (3)
24-24: LGTM: Type definitions and imports are properly updated
The changes correctly integrate the new yearsOfExperience field into the type system and maintain proper import organization.
Also applies to: 54-54
403-410: LGTM: Form initialization properly includes yearsOfExperience
The changes correctly integrate the new field into the form's initialization and props handling.
Also applies to: 427-427
Line range hint 446-458: Verify onboarding completion tracking
The form submission successfully saves the data, but we should verify that it properly updates the onboardingComplete timestamp mentioned in the PR objectives.
| ALTER TABLE "user" ADD COLUMN "yearsOfExperience" text;--> statement-breakpoint | ||
| ALTER TABLE "user" ADD COLUMN "onboardingComplete" timestamp with time zone; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider strengthening column definitions
The current column definitions could be improved:
-
yearsOfExperience:- Consider using a numeric type with constraints instead of text
- Add CHECK constraint to validate valid ranges
- Consider NOT NULL with a default value
-
onboardingComplete:- Consider NOT NULL with a default value for existing users
Consider this alternative implementation:
-ALTER TABLE "user" ADD COLUMN "yearsOfExperience" text;
-ALTER TABLE "user" ADD COLUMN "onboardingComplete" timestamp with time zone;
+ALTER TABLE "user" ADD COLUMN "yearsOfExperience" integer CHECK ("yearsOfExperience" >= 0 AND "yearsOfExperience" <= 50);
+ALTER TABLE "user" ADD COLUMN "onboardingComplete" timestamp with time zone NOT NULL DEFAULT CURRENT_TIMESTAMP;📝 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.
| ALTER TABLE "user" ADD COLUMN "yearsOfExperience" text;--> statement-breakpoint | |
| ALTER TABLE "user" ADD COLUMN "onboardingComplete" timestamp with time zone; | |
| ALTER TABLE "user" ADD COLUMN "yearsOfExperience" integer CHECK ("yearsOfExperience" >= 0 AND "yearsOfExperience" <= 50);--> statement-breakpoint | |
| ALTER TABLE "user" ADD COLUMN "onboardingComplete" timestamp with time zone NOT NULL DEFAULT CURRENT_TIMESTAMP; |
schema/additionalUserDetails.ts
Outdated
| @@ -30,6 +30,7 @@ export const slideThreeSchema = z | |||
| jobTitle: z.string().max(30, "Max length is 30 characters."), | |||
| levelOfStudy: z.string(), | |||
| course: z.string().max(30, "Max name length is 30 characters."), | |||
| yearsOfExperience: z.string(), | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance validation for yearsOfExperience field
The field definition could be improved with additional constraints and proper typing:
- yearsOfExperience: z.string(),
+ yearsOfExperience: z.string()
+ .max(30, "Max length is 30 characters.")
+ .refine(
+ (val) => !val || ["0-2", "3-5", "5-10", "10+"].includes(val),
+ "Invalid years of experience value"
+ ),📝 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.
| yearsOfExperience: z.string(), | |
| yearsOfExperience: z.string() | |
| .max(30, "Max length is 30 characters.") | |
| .refine( | |
| (val) => !val || ["0-2", "3-5", "5-10", "10+"].includes(val), | |
| "Invalid years of experience value" | |
| ), |
| yearsOfExperience, | ||
| } = slideThreeSchema.parse(dataInput); | ||
|
|
||
| const onboardingComplete = new Date().toISOString(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving timestamp generation to database level
Instead of generating the timestamp in the application code, consider letting the database handle it for better consistency and reliability across all onboarding completions.
- const onboardingComplete = new Date().toISOString();
+ // Let database handle the timestamp
await db
.update(user)
.set({
professionalOrStudent,
course,
jobTitle,
levelOfStudy,
workplace,
yearsOfExperience,
- onboardingComplete,
+ onboardingComplete: db.fn.now(), // Use database function
})Committable suggestion skipped: line range outside the PR's diff.
| const userStatus = await db.query.user.findFirst({ | ||
| where: (users, { eq }) => eq(users.id, user.id), | ||
| columns: { onboardingComplete: true }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and consider performance optimization.
The database query lacks error handling and runs on every session callback. Consider:
- Adding try-catch block to handle database errors gracefully
- Combining this query with other user data fetches to reduce database calls
- const userStatus = await db.query.user.findFirst({
- where: (users, { eq }) => eq(users.id, user.id),
- columns: { onboardingComplete: true },
- });
+ try {
+ const userStatus = await db.query.user.findFirst({
+ where: (users, { eq }) => eq(users.id, user.id),
+ columns: {
+ onboardingComplete: true,
+ role: true,
+ // Include other frequently needed user fields
+ },
+ });
+
+ session.user.id = user.id;
+ session.user.role = userStatus?.role ?? user.role;
+ session.user.isOnboardingComplete = !!userStatus?.onboardingComplete;
+ } catch (error) {
+ console.error('Failed to fetch user status:', error);
+ Sentry.captureException(error);
+ // Fail safe: assume onboarding is incomplete
+ session.user.isOnboardingComplete = false;
+ }Committable suggestion skipped: line range outside the PR's diff.
server/db/schema.ts
Outdated
| yearsOfExperience: text("yearsOfExperience"), | ||
| onboardingComplete: timestamp("onboardingComplete", { | ||
| mode: "string", | ||
| withTimezone: true, | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider strengthening data type constraints for better data integrity
-
The
yearsOfExperiencefield is currently defined astext, but since it represents years of experience, consider:- Using an enum for predefined ranges (e.g., "0-2", "3-5", "5-10", "10+")
- Or using an integer with check constraints for valid ranges
-
The
onboardingCompletefield might benefit from a default value ofnullto make the incomplete state explicit.
- yearsOfExperience: text("yearsOfExperience"),
+ yearsOfExperience: varchar("yearsOfExperience", { length: 10 }).$type<"0-2" | "3-5" | "5-10" | "10+">(),
onboardingComplete: timestamp("onboardingComplete", {
mode: "string",
withTimezone: true,
- }),
+ }).default(null),📝 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.
| yearsOfExperience: text("yearsOfExperience"), | |
| onboardingComplete: timestamp("onboardingComplete", { | |
| mode: "string", | |
| withTimezone: true, | |
| }), | |
| yearsOfExperience: varchar("yearsOfExperience", { length: 10 }).$type<"0-2" | "3-5" | "5-10" | "10+">(), | |
| onboardingComplete: timestamp("onboardingComplete", { | |
| mode: "string", | |
| withTimezone: true, | |
| }).default(null), |
context/AuthProvider.tsx
Outdated
| const { data: session, status } = useSession(); | ||
| const router = useRouter(); | ||
|
|
||
| console.log("-------------"); |
server/db/schema.ts
Outdated
| @@ -217,6 +217,11 @@ export const user = pgTable( | |||
| levelOfStudy: text("levelOfStudy"), | |||
| course: text("course"), | |||
| role: role("role").default("USER").notNull(), | |||
| yearsOfExperience: text("yearsOfExperience"), | |||
There was a problem hiding this comment.
We should probably use ENUMs here.
CREATE TYPE experience_range AS ENUM ('0-1', '1-3', '3-5', '5-8', '12+');
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
drizzle/0011_years-experience-user-table.sql (1)
1-5: Consider addressing the gap in experience rangesThe current ENUM values ('0-1', '1-3', '3-5', '5-8', '12+') have a gap between 8-12 years. This could lead to confusion for users with experience in this range.
Consider adding an '8-12' range:
-CREATE TYPE "public"."experience_range" AS ENUM('0-1', '1-3', '3-5', '5-8', '12+'); +CREATE TYPE "public"."experience_range" AS ENUM('0-1', '1-3', '3-5', '5-8', '8-12', '12+');schema/additionalUserDetails.ts (1)
43-79: Consider refactoring validation logic for better maintainabilityThe current validation logic, while functional, could be improved for better maintainability and testability.
Consider refactoring the validation logic like this:
+ const validateStudent = (val: z.infer<typeof slideThreeSchema>, ctx: z.RefinementCtx) => { + if (val.levelOfStudy === "") { + ctx.addIssue({ + path: ["levelOfStudy"], + code: "custom", + message: "Please select your level of study", + }); + } + if (val.course === "") { + ctx.addIssue({ + path: ["course"], + code: "custom", + message: "Please enter your course name", + }); + } + }; + + const validateProfessional = (val: z.infer<typeof slideThreeSchema>, ctx: z.RefinementCtx) => { + if (val.workplace === "") { + ctx.addIssue({ + path: ["workplace"], + code: "custom", + message: "Please enter your workplace", + }); + } + if (val.jobTitle === "") { + ctx.addIssue({ + path: ["jobTitle"], + code: "custom", + message: "Please enter your job title", + }); + } + if (val.yearsOfExperience === undefined) { + ctx.addIssue({ + path: ["yearsOfExperience"], + code: "custom", + message: "Please select your years of experience", + }); + } + }; export const slideThreeSchema = z .object({ // ... existing fields ... }) .superRefine((val, ctx) => { if (val.professionalOrStudent === "Current student") { - if (val.levelOfStudy === "") { - ctx.addIssue({ - path: ["levelOfStudy"], - code: "custom", - message: "required", - }); - } - // ... other student validations + validateStudent(val, ctx); } else if (val.professionalOrStudent === "Working professional") { - // ... professional validations + validateProfessional(val, ctx); } });Benefits:
- Separate functions for each validation scenario improve readability
- More descriptive error messages
- Easier to test individual validation rules
- Reduced code duplication
server/db/schema.ts (1)
173-179: Consider adjusting experience ranges for better coverageThe current ranges have a gap between 8-12 years and an inconsistent pattern with "12+". Consider adjusting the ranges for better continuity:
export const experienceRangeEnum = pgEnum("experience_range", [ "0-1", "1-3", "3-5", "5-8", - "12+", + "8-10", + "10+" ]);app/(app)/onboarding/_client.tsx (2)
Line range hint
441-449: Consider consolidating form validation logic.The current implementation uses separate
triggercalls which could lead to inconsistent validation states. Consider consolidating the validation logic into a single call.- let isError = await trigger(["professionalOrStudent"]); - const professionalOrStudent = getValues("professionalOrStudent"); - - if (isError && professionalOrStudent === "Working professional") { - isError = await trigger(["workplace", "jobTitle", "yearsOfExperience"]); - } - - if (isError && professionalOrStudent === "Current student") { - isError = await trigger(["levelOfStudy", "course"]); - } + const professionalOrStudent = getValues("professionalOrStudent"); + const fieldsToValidate = ["professionalOrStudent"]; + + if (professionalOrStudent === "Working professional") { + fieldsToValidate.push("workplace", "jobTitle", "yearsOfExperience"); + } else if (professionalOrStudent === "Current student") { + fieldsToValidate.push("levelOfStudy", "course"); + } + + const isError = await trigger(fieldsToValidate);
531-552: Enhance field labels and placeholders for better UX.While the implementation is solid, consider these UX improvements:
- Make the label more descriptive, e.g., "Years of professional experience"
- Add context to the placeholder, e.g., "Select your years of professional experience"
- <Label>Years of experience:</Label> + <Label>Years of professional experience</Label> <Select id="years-of-experience" {...register("yearsOfExperience")} defaultValue="" > <option value="" disabled> - Select years of experience + Select your years of professional experience </option>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
drizzle/meta/0011_snapshot.jsonis excluded by!**/*.jsondrizzle/meta/_journal.jsonis excluded by!**/*.json
📒 Files selected for processing (6)
app/(app)/onboarding/_client.tsx(8 hunks)app/(app)/onboarding/page.tsx(2 hunks)context/AuthProvider.tsx(1 hunks)drizzle/0011_years-experience-user-table.sql(1 hunks)schema/additionalUserDetails.ts(2 hunks)server/db/schema.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/(app)/onboarding/page.tsx
- context/AuthProvider.tsx
🔇 Additional comments (7)
drizzle/0011_years-experience-user-table.sql (1)
7-7: Verify handling of existing session data
Adding a primary key constraint could fail if there are duplicate sessionToken values in existing records.
Run this script to check for potential duplicates:
✅ Verification successful
No risk of duplicate sessionToken values
The codebase shows that the session table has always had a unique constraint on sessionToken through the index "Session_sessionToken_key", so there's no risk of existing duplicate values when adding the primary key constraint. The change is safe to apply.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for duplicate sessionToken values in the session table
# that might prevent primary key creation
ast-grep --pattern 'CREATE TABLE "session" {
$$$
"sessionToken" $_ $_,
$$$
}'
rg -A 10 'CREATE TABLE.*session'
Length of output: 120
Script:
#!/bin/bash
# Let's try to find the session table creation and any related migrations
fd -e sql . | xargs cat
# Also search for any session-related migrations or schema files
rg -i "session.*table|create.*session" -g "*.sql"
Length of output: 43012
schema/additionalUserDetails.ts (2)
1-1: LGTM! Good practice using DB enum values directly
The approach of importing and using enum values directly from the database schema ensures type consistency between the database and application layers.
Also applies to: 4-4
36-40: Enhance field validation as per previous suggestion
The current implementation is functional but could benefit from the previously suggested validation enhancements.
server/db/schema.ts (2)
Line range hint 238-238: LGTM: Index additions will improve query performance
The new indices on userId, username, and postId fields are well-placed and will enhance query performance for:
- User lookups during onboarding
- Post retrieval by user
- Comment retrieval by post
Also applies to: 238-238, 341-341, 341-341
228-232: 🛠️ Refactor suggestion
Enhance field definitions with defaults and constraints
The new fields would benefit from:
- A default value for
onboardingCompleteto explicitly track incomplete status - A constraint on
yearsOfExperienceto ensure it's set whenprofessionalOrStudentis 'Working professional'
- yearsOfExperience: experienceRangeEnum("yearsOfExperience"),
+ yearsOfExperience: experienceRangeEnum("yearsOfExperience").$type<experienceRangeEnum>(),
onboardingComplete: timestamp("onboardingComplete", {
mode: "string",
withTimezone: true,
- }),
+ }).default(null),Also, consider adding a check constraint to ensure yearsOfExperience is set when professionalOrStudent is 'Working professional':
app/(app)/onboarding/_client.tsx (2)
24-24: LGTM! Type definitions and imports are well structured.
The changes properly integrate the new years of experience field with:
- Centralized select options
- Schema-driven enumeration
- Type-safe field definition
Also applies to: 42-42, 55-55
541-545: Verify schema validation matches UI constraints.
Ensure that the experienceRangeEnum in the schema matches the options rendered in the UI.
✅ Verification successful
Schema validation matches UI constraints perfectly ✅
The experienceRangeEnum in server/db/schema.ts defines exactly the same values that are being rendered in the UI:
"0-1""1-3""3-5""5-8""12+"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the schema definition matches the UI constraints
# Search for experienceRangeEnum definition in schema files
rg -A 5 "experienceRangeEnum.*=.*" --type typescript
# Search for any other usage of these specific year ranges to ensure consistency
rg -A 2 '"(0-1|1-3|3-5|5-8|12\+)"' --type typescript
Length of output: 179
Script:
#!/bin/bash
# Search for experienceRangeEnum definition and usage without file type restriction
rg -A 5 "experienceRangeEnum.*=.*"
# Search for enum or array definitions containing year ranges
rg -A 5 "(enum|array).*years?"
# Search for option elements with year ranges
rg "option.*value.*years?"
Length of output: 764
| ALTER TABLE "user" ADD COLUMN "yearsOfExperience" "experience_range";--> statement-breakpoint | ||
| ALTER TABLE "user" ADD COLUMN "onboardingComplete" timestamp with time zone; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen column definitions for data integrity
The current column definitions could be improved to ensure data consistency:
yearsOfExperienceshould be NOT NULL for new users going through onboardingonboardingCompleteshould have a default value for existing users
Consider this alternative implementation:
-ALTER TABLE "user" ADD COLUMN "yearsOfExperience" "experience_range";
-ALTER TABLE "user" ADD COLUMN "onboardingComplete" timestamp with time zone;
+ALTER TABLE "user" ADD COLUMN "yearsOfExperience" "experience_range" NOT NULL DEFAULT '0-1';
+ALTER TABLE "user" ADD COLUMN "onboardingComplete" timestamp with time zone NOT NULL DEFAULT CURRENT_TIMESTAMP;Note: The default value for yearsOfExperience should align with your business logic for existing users.
📝 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.
| ALTER TABLE "user" ADD COLUMN "yearsOfExperience" "experience_range";--> statement-breakpoint | |
| ALTER TABLE "user" ADD COLUMN "onboardingComplete" timestamp with time zone; | |
| ALTER TABLE "user" ADD COLUMN "yearsOfExperience" "experience_range" NOT NULL DEFAULT '0-1';--> statement-breakpoint | |
| ALTER TABLE "user" ADD COLUMN "onboardingComplete" timestamp with time zone NOT NULL DEFAULT CURRENT_TIMESTAMP; |
✨ Codu Pull Request 💻
Fixes #(issue)
Pull Request details
This PR completes the onboarding form
Add a ‘years of experience’ select question when the user selects ‘Working professional’
Create migration to add two fields to the user table
yearsOfExperience text field and onboardingComplete timestamp field
Adds a check for completion and redirect on Login
Perform isOnboardingComplete database check in the session callback
Add isOnboardingComplete boolean to the session for onboarding status tracking.
Add component within AuthProvider to verify if the user has completed onboarding and redirect if not.
Attempts to handle the onboarding redirect - Open to suggestions
Middleware: Attempted to use middleware, but it can’t access the session or database.
JWT Strategy: Tried adding an isOnboardingComplete field to the JWT, but strategy: "jwt" doesn’t integrate well with our session setup.
Root Layout Check: Added a server-side check in the RootLayout, but this led to repeated redirects.
In the end I added the check client side to the AuthProvider. Not ideal, but it works.
Any Breaking changes
[Optional] What gif best describes this PR or how it makes you feel