SANC-48-When-the-user-completes-registration-they-should-be-able-to-input-their-name #36
Conversation
- Completed SANC-48 - Added a loading screen while the Google Maps API loads (feedback from SANC-39). Found in \src\app\_components\agencycomponents\agency-interactive-area.tsx
jason-duong4509
requested review from
JustinTan-1,
Lujarios,
Yemyam,
burtonjong,
promatty,
tanvimahal,
themaxboucher and
wesleylui
as code owners
📝 WalkthroughWalkthroughAdds a name-collection page and API to set a user's name, updates dashboard redirect logic to guard against empty names and validate agency slugs, and replaces an empty Google Maps loading placeholder with a styled Mantine Paper containing a centered "Loading..." title. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client as FillOutName Page
participant API as organizations.router
participant DB as Database
participant Router as Next Router
User->>Client: Submit name
Client->>API: changeName(newName)
API->>API: Validate nameRegex
API->>DB: UPDATE user SET name = newName WHERE id = ctx.session.user.id
DB-->>API: Success/Error
API-->>Client: changeName result
Client->>API: redirectToDashboard()
API->>API: Check ctx.session.user.name, role, slug
API->>DB: Query organization by slug (if needed)
DB-->>API: Organization / Not found
API-->>Client: redirectUrl or error
Client->>Router: router.push(redirectUrl)
Router-->>User: Navigate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/app/login/page.tsx (1)
12-15: Document the RedirectLink type.The custom type declaration is necessary but lacks context. Consider adding a comment explaining that this matches the return value from the better-auth sign-in callback hook defined in
src/lib/auth.ts.🔎 Suggested documentation
-//Custom type declaration fo result.data after logging in. +// Custom type declaration for result.data after logging in. -//Needed as the better auth hook will return one of two URLs -//to redirect to. +// The better-auth sign-in hook returns a redirect URL via the after middleware +// defined in src/lib/auth.ts (either /fill-out-name or /dashboard). type RedirectLink = { newLink: string };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app/_components/agencycomponents/agency-interactive-area.tsxsrc/app/fill-out-name/page.tsxsrc/app/login/page.tsxsrc/lib/auth.tssrc/server/api/routers/organizations.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-23T05:44:35.017Z
Learnt from: tanvimahal
Repo: Code-the-Change-YYC/salvationarmy PR: 25
File: src/server/api/routers/organizations.ts:38-38
Timestamp: 2025-12-23T05:44:35.017Z
Learning: In this codebase's TRPC protectedProcedure context, ctx.session has a nested structure containing both a session object and a user object. To access activeOrganizationId, use ctx.session.session.activeOrganizationId (not ctx.session.activeOrganizationId).
Applied to files:
src/server/api/routers/organizations.ts
🧬 Code graph analysis (3)
src/app/fill-out-name/page.tsx (4)
src/app/login/page.tsx (1)
LoginPage(17-96)src/trpc/react.tsx (1)
api(25-25)src/lib/notifications.ts (1)
notify(13-58)src/app/_components/common/button/Button.tsx (1)
Button(41-80)
src/server/api/routers/organizations.ts (2)
src/server/api/trpc.ts (1)
protectedProcedure(122-132)src/server/db/auth-schema.ts (1)
user(5-19)
src/app/login/page.tsx (1)
src/lib/auth-client.ts (2)
authClient(4-7)authClient(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci
🔇 Additional comments (3)
src/app/_components/agencycomponents/agency-interactive-area.tsx (1)
3-3: LGTM! Nice loading UI improvement.The addition of a styled loading card using Mantine's Paper and Title components is a good UX enhancement over the previous empty div. The implementation is clean and consistent with the existing Mantine design system usage.
Also applies to: 169-176
src/app/login/page.tsx (2)
50-56: Good error handling with proper cleanup.The error handling correctly shows user-facing messages via
alert()and thefinallyblock ensures the loading state is always reset regardless of success or failure.
40-49: The type assertion is safe but consider formalizing the custom type.The double assertion pattern
(result.data as unknown as RedirectLink)is a valid workaround for better-auth's type limitations. The server-side middleware (insrc/lib/auth.tslines 21 and 24) explicitly returns{ newLink: ... }for/sign-in/emailrequests, which correctly flows back to the client inresult.data. However, since this custom property is not in better-auth's type definitions, the assertion is necessary.To improve type safety without the workaround, consider creating a type guard or a safer wrapper around
authClient.signIn.email()that validates thenewLinkproperty exists before using it.
| <Stack gap="md"> | ||
| <TextInput | ||
| label="Name" | ||
| placeholder="Alphanumeric, 3+ Characters" |
There was a problem hiding this comment.
Update placeholder to match actual validation.
The placeholder says "3+ Characters" but the regex pattern actually requires 5+ characters (though it should be corrected to 3+ as per the intended design).
- placeholder="Alphanumeric, 3+ Characters"
+ placeholder="Your full name"📝 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.
| placeholder="Alphanumeric, 3+ Characters" | |
| placeholder="Your full name" |
🤖 Prompt for AI Agents
In src/app/fill-out-name/page.tsx around line 61, the input placeholder
currently reads "Alphanumeric, 3+ Characters" which mismatches the actual regex
validation requiring 5+ characters; update the code so the placeholder matches
the intended validation by either changing the regex to require 3+ characters or
changing the placeholder to state "Alphanumeric, 5+ Characters" — choose the
intended design (preferably correct the regex to 3+ if that was intended) and
ensure placeholder text and the validation pattern are consistent.
src/lib/auth.ts
Outdated
| hooks: { | ||
| after: createAuthMiddleware(async (ctx) => { | ||
| if (ctx.path.startsWith("/sign-in/email")) { | ||
| const accountName = ctx.context.newSession?.user.name; //Get the account name | ||
| if (accountName === "") { | ||
| //Account name not set | ||
| return { newLink: "/fill-out-name" }; | ||
| } | ||
| //TODO V: change later when we know where to send the user | ||
| return { newLink: "/dashboard" }; | ||
| } | ||
| }), | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find the database schema file(s)
find . -type f -name "*schema*" -o -name "*db*" | grep -E "\.(ts|js)$" | head -20Repository: Code-the-Change-YYC/salvationarmy
Length of output: 167
🏁 Script executed:
#!/bin/bash
# Look for pgTable definitions related to users
rg "pgTable.*user" -A 10 -B 2Repository: Code-the-Change-YYC/salvationarmy
Length of output: 917
🏁 Script executed:
#!/bin/bash
# Search for user table/schema definition
rg "name.*:" -t ts | grep -i "user\|account" | head -20Repository: Code-the-Change-YYC/salvationarmy
Length of output: 631
🏁 Script executed:
#!/bin/bash
# Find better-auth configuration and type definitions
find . -type f \( -name "auth.ts" -o -name "auth.config.*" -o -name "*auth*" \) | grep -v node_modules | head -20Repository: Code-the-Change-YYC/salvationarmy
Length of output: 229
🏁 Script executed:
#!/bin/bash
# Check the actual import and usage of user.name in the auth.ts file
cat -n src/lib/auth.ts | head -50Repository: Code-the-Change-YYC/salvationarmy
Length of output: 2107
Handle null/undefined values in the name check.
The after middleware correctly implements conditional routing based on user profile completion. However, the name check on line 19 only handles empty strings. Since ctx.context.newSession?.user.name uses optional chaining, accountName can be undefined if either newSession or user is falsy. Update the condition to catch both cases:
if (!accountName || accountName === "") {
// Account name not set
return { newLink: "/fill-out-name" };
}🤖 Prompt for AI Agents
In src/lib/auth.ts around lines 15 to 27, the middleware only checks for an
empty string when determining if the account name is set, but accountName can be
undefined/null due to optional chaining; update the condition to treat falsy
values as unset (e.g., check if accountName is falsy or empty) so the branch
returns { newLink: "/fill-out-name" } when name is undefined, null, or an empty
string.
- Changed the function name in /src/app/fill-out-name from LoginPage to FillOutNamePage because that was missed earlier - Added a comment in /src/app/fill-out-name because that was missed earlier - Changed an HTTP error code from 500 to 400 because of the coderabbitai suggestion
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/app/fill-out-name/page.tsx (2)
34-48: Validation issues remain unresolved.The regex validation logic still has the problems identified in the previous review:
- Incorrect minimum length: The regex pattern requires 5 characters minimum, contradicting the comment and placeholder claiming ">=3 characters."
- Code duplication: This exact regex and validation logic is duplicated in
src/server/api/routers/organizations.ts(lines 235-243).- Suboptimal UX: Client-side validation should be in the
useFormhook'svalidateproperty for inline error feedback, not in the submit handler.Please refer to the detailed refactoring suggestions in the previous review comment on lines 36-42, which proposed creating a shared validation utility in
src/lib/validation.tsto eliminate duplication and fix the minimum length requirement.
50-74: UI structure is clean, but placeholder text remains inconsistent.The form UI is well-structured with appropriate Mantine components. However, the placeholder text issue from the previous review persists:
The placeholder states "Alphanumeric, 3+ Characters" but the actual regex validation requires 5+ characters minimum. This misleads users about the actual requirements.
src/server/api/routers/organizations.ts (1)
230-243: Validation error code fixed, but duplication remains.Good fix changing the validation error to
BAD_REQUEST(line 240) as recommended in the previous review. However, the core validation issues remain unresolved:
- Regex duplication: This exact regex pattern and validation logic is duplicated from
src/app/fill-out-name/page.tsx(line 37), violating DRY principles.- Incorrect minimum length: The regex requires 5 characters minimum, not 3 as documented in the comment (line 235).
As suggested in the previous review, please create a shared validation utility (e.g.,
src/lib/validation.ts) with avalidateNamefunction and constants that both the client and server can import. This will ensure consistency and make it easier to fix the minimum length requirement in one place.
🧹 Nitpick comments (1)
src/app/fill-out-name/page.tsx (1)
11-19: Component setup is functional.The function naming issue from the previous review has been resolved. However, consider adding validation to the
useFormconfiguration (in thevalidateproperty) rather than performing it inhandleSubmitfor better UX with inline error messages.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/fill-out-name/page.tsxsrc/server/api/routers/organizations.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-23T05:44:35.017Z
Learnt from: tanvimahal
Repo: Code-the-Change-YYC/salvationarmy PR: 25
File: src/server/api/routers/organizations.ts:38-38
Timestamp: 2025-12-23T05:44:35.017Z
Learning: In this codebase's TRPC protectedProcedure context, ctx.session has a nested structure containing both a session object and a user object. To access activeOrganizationId, use ctx.session.session.activeOrganizationId (not ctx.session.activeOrganizationId).
Applied to files:
src/server/api/routers/organizations.ts
🧬 Code graph analysis (2)
src/app/fill-out-name/page.tsx (3)
src/trpc/react.tsx (1)
api(25-25)src/lib/notifications.ts (1)
notify(13-58)src/app/_components/common/button/Button.tsx (1)
Button(41-80)
src/server/api/routers/organizations.ts (2)
src/server/api/trpc.ts (1)
protectedProcedure(122-132)src/server/db/auth-schema.ts (1)
user(5-19)
🔇 Additional comments (4)
src/app/fill-out-name/page.tsx (2)
1-9: LGTM!Imports are appropriate for a client-side form component with TRPC integration.
21-32: LGTM!The mutation setup follows TRPC best practices with proper success/error handling and loading state management. The TODO comment is reasonable for tracking future routing decisions.
src/server/api/routers/organizations.ts (2)
2-2: LGTM!The new imports (
eqanduser) are necessary for the database update operation in thechangeNameprocedure.Also applies to: 12-12
245-258: LGTM!The database update logic is correct:
- Properly scopes the update to the authenticated user (
ctx.session.user.id)- Uses
.returning()to verify the update succeeded- Appropriate error handling with
INTERNAL_SERVER_ERRORfor the unexpected case where no rows are updated (sinceprotectedProcedureguarantees the user exists)
- Moved front-end redirect logic from the back-end to the front-end - Replaced the loading useState - Moved the name regex to validation.ts - Simplified form validation in \fill-out-name\page.tsx to streamline code
…-completes-registration-they-should-be-able-to-input-their-name # Conflicts: # src/app/_components/agencycomponents/agency-interactive-area.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @src/types/validation.ts:
- Around line 12-13: Replace the current export const nameRegex with a regex
that (1) requires at least two characters, (2) ensures the string contains at
least one letter (so numeric-only names fail), (3) allows letters, single
spaces, hyphens and apostrophes as separators, (4) forbids consecutive spaces,
and (5) ensures any separator is followed by a letter (no trailing separators);
update the nameRegex definition accordingly to implement these rules (target the
export const nameRegex symbol).
🧹 Nitpick comments (1)
src/app/_components/agencycomponents/agency-interactive-area.tsx (1)
177-183: Consider using the existing styles module for consistency.The inline styles work correctly, but the file already imports and uses a CSS module (
stylesfrom./agency-interactive-area.module.scss) for other layout concerns. For consistency and maintainability, consider extracting these styles into the module.🔎 Optional refactor to use CSS module
In
agency-interactive-area.module.scss, add:.loadingCardContainer { max-width: 250px; margin: 4rem auto; padding: 0 1rem; }Then update the component:
- return ( - <div style={{ maxWidth: "250px", margin: "4rem auto", padding: "0 1rem" }}> - <Paper shadow="sm" p="xl" radius="md"> - <Title>Loading...</Title> - </Paper> - </div> - ); + return ( + <div className={styles.loadingCardContainer}> + <Paper shadow="sm" p="xl" radius="md"> + <Title>Loading...</Title> + </Paper> + </div> + );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/_components/agencycomponents/agency-interactive-area.tsxsrc/app/fill-out-name/page.tsxsrc/app/login/page.tsxsrc/types/validation.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/fill-out-name/page.tsx
- src/app/login/page.tsx
🔇 Additional comments (1)
src/app/_components/agencycomponents/agency-interactive-area.tsx (1)
3-3: LGTM!The additional Mantine imports support the enhanced loading UI.
| export const nameRegex = | ||
| /^(?!.*\s{2})[A-Za-z0-9][A-Za-z0-9\s]*[A-Za-z0-9][A-Za-z0-9\s]*[A-Za-z0-9]$/; |
There was a problem hiding this comment.
Fix the name validation regex to support valid names and reject invalid ones.
The current regex has several issues that will impact user experience and data integrity:
- Rejects valid short names: The pattern requires a minimum of 3 characters, rejecting legitimate names like "Jo", "Li", "Wu", "Bo", etc.
- Accepts numeric-only input: Names like "123" or "007" would pass validation, which is likely unintended.
- Overly complex pattern: The repeating
[A-Za-z0-9][A-Za-z0-9\s]*[A-Za-z0-9]structure appears redundant. - Missing common name characters: Doesn't support hyphens or apostrophes needed for names like "O'Brien" or "Mary-Jane".
🔎 Proposed fix for the name validation regex
-export const nameRegex =
- /^(?!.*\s{2})[A-Za-z0-9][A-Za-z0-9\s]*[A-Za-z0-9][A-Za-z0-9\s]*[A-Za-z0-9]$/;
+export const nameRegex = /^(?!.*\s{2})[A-Za-z]+(?:[ '-][A-Za-z]+)*$/;This improved pattern:
- Allows names of any length (2+ characters)
- Requires at least one letter at the start and after each separator
- Prevents consecutive spaces (maintains your original intent)
- Supports hyphens and apostrophes for compound names
- Rejects numeric-only input
- Simplified and more maintainable
Alternatively, if you want to enforce a minimum length or allow numbers in names, consider:
// Minimum 2 characters, letters only with optional spaces/hyphens/apostrophes
export const nameRegex = /^(?!.*\s{2})[A-Za-z]{2,}(?:[ '-][A-Za-z]+)*$/;🤖 Prompt for AI Agents
In @src/types/validation.ts around lines 12-13, Replace the current export const
nameRegex with a regex that (1) requires at least two characters, (2) ensures
the string contains at least one letter (so numeric-only names fail), (3) allows
letters, single spaces, hyphens and apostrophes as separators, (4) forbids
consecutive spaces, and (5) ensures any separator is followed by a letter (no
trailing separators); update the nameRegex definition accordingly to implement
these rules (target the export const nameRegex symbol).
…-completes-registration-they-should-be-able-to-input-their-name # Conflicts: # src/app/login/page.tsx # src/server/api/routers/organizations.ts
- Changed the way a user is redirected from /login to /fill-out-name to accommodate changes made to main - Changed the way a user is redirected from /fill-out-name to accommodate changes made to main
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/server/api/routers/organizations.ts (1)
306-334: Inconsistency: Regex is defined locally, not imported.The AI summary states that
nameRegexis imported fromsrc/types/validation.ts, but the code shows the regex is defined locally at line 312. The validation logic itself is correct and the error codes have been properly addressed from the previous review.
Optional: Consider returning a success indicator.
The mutation completes successfully but returns
void. Consider returning the updated user object or a success status to provide feedback to the caller.💡 Optional enhancement
if (listOfUpdatedUsers.length === 0) { //Drizzle updated no users (something went wrong) throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "Failed to update user's name", }); } + + return { + success: true, + user: listOfUpdatedUsers[0], + }; }),
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/fill-out-name/page.tsxsrc/server/api/routers/organizations.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/fill-out-name/page.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-23T05:44:37.925Z
Learnt from: tanvimahal
Repo: Code-the-Change-YYC/salvationarmy PR: 25
File: src/server/api/routers/organizations.ts:38-38
Timestamp: 2025-12-23T05:44:37.925Z
Learning: In this codebase's TRPC protectedProcedure context, ctx.session has a nested structure containing both a session object and a user object. To access activeOrganizationId, use ctx.session.session.activeOrganizationId (not ctx.session.activeOrganizationId).
Applied to files:
src/server/api/routers/organizations.ts
🧬 Code graph analysis (1)
src/server/api/routers/organizations.ts (2)
src/server/db/auth-schema.ts (2)
organization(72-79)user(6-20)src/server/api/trpc.ts (1)
protectedProcedure(122-132)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci
🔇 Additional comments (2)
src/server/api/routers/organizations.ts (2)
2-2: LGTM!The new imports support the
changeNamemutation correctly. Theeqfunction andusertable reference are properly utilized in the database update operation.Also applies to: 12-12
34-82: Good defensive programming with proper error handling.The refactored redirect logic correctly guards against users with empty names and includes thorough validation for the agency role:
- The organization lookup ensures the slug exists before constructing the redirect URL
- Error codes are appropriate (NOT_FOUND for missing entities, INTERNAL_SERVER_ERROR for data integrity issues)
- Properly uses
ctx.session.session.activeOrganizationIdas per the codebase's session structureBased on learnings about the session structure in this codebase.
- Changed a function name for better clarity - Added additional text to error messages to provide more information
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/server/api/routers/organizations.ts:
- Around line 313-321: Replace the duplicated local regex with the shared
exported nameRegex from the validation module: import nameRegex (from the module
that exports it) at the top of this file and remove the local const regex; then
use nameRegex.test(newName) in place of regex.test(newName) in the organizations
update/name validation block so server-side validation matches the client-side
rule and avoids duplication.
🧹 Nitpick comments (2)
src/server/api/routers/organizations.ts (2)
34-36: Consider handlingundefinedin addition to empty string for name check.The check
ctx.session.user.name === ""only catches empty strings. If the name field could beundefinedornullin some authentication scenarios, this condition might not redirect appropriately.♻️ Suggested fix
- if (ctx.session.user.name === "") { + if (!ctx.session.user.name) { redirectUrl = "/fill-out-name"; } else {
309-309: Consider adding maximum length constraint to input validation.The input schema only validates that
newNameis a string. Adding a maximum length constraint provides defense-in-depth validation before the regex check and protects against excessively long inputs.♻️ Suggested fix
changeName: protectedProcedure - .input(z.object({ newName: z.string() })) + .input(z.object({ newName: z.string().max(100) })) .mutation(async ({ input, ctx }) => {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/fill-out-name/page.tsxsrc/server/api/routers/organizations.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/fill-out-name/page.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-23T05:44:37.925Z
Learnt from: tanvimahal
Repo: Code-the-Change-YYC/salvationarmy PR: 25
File: src/server/api/routers/organizations.ts:38-38
Timestamp: 2025-12-23T05:44:37.925Z
Learning: In this codebase's TRPC protectedProcedure context, ctx.session has a nested structure containing both a session object and a user object. To access activeOrganizationId, use ctx.session.session.activeOrganizationId (not ctx.session.activeOrganizationId).
Applied to files:
src/server/api/routers/organizations.ts
🧬 Code graph analysis (1)
src/server/api/routers/organizations.ts (2)
src/server/db/auth-schema.ts (2)
organization(72-79)user(6-20)src/server/api/trpc.ts (1)
protectedProcedure(122-132)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci
🔇 Additional comments (3)
src/server/api/routers/organizations.ts (3)
2-2: LGTM!The imports are correctly added to support the new
changeNameprocedure's database update operation using Drizzle ORM.Also applies to: 12-12
42-74: LGTM!The agency role handling is well-implemented with proper organization lookup and comprehensive error handling. The access pattern
ctx.session.session.activeOrganizationIdcorrectly follows the nested session structure. Based on learnings, this is the correct pattern for accessing activeOrganizationId in this codebase.
323-336: LGTM!The database update logic correctly uses Drizzle ORM's update pattern with proper error handling for the edge case where no rows are updated.
- Changed how the back-end validated user name input by relying on an imported regex rather than a hardcoded one in-file. Missed this issue in previous commits
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/server/api/routers/organizations.ts:
- Around line 321-334: The DB update in the mutation updates the user's name
(result in listOfUpdatedUsers) but does not refresh ctx.session.user.name so
downstream logic sees a stale session name; instead of trying to refresh session
here, update redirectToDashboard to read the authoritative name from the
database record (query the users table by ctx.session.user.id and use the
returned user.name) and ensure any post-update logic uses that DB value (refer
to listOfUpdatedUsers, ctx.session.user.name, and redirectToDashboard) so
session staleness is avoided.
- Around line 34-36: The redirect check is using the session value
(ctx.session.user.name) instead of the database user fetched earlier, causing
stale-session redirects; update the condition to use the DB user (the `user`
variable returned from the lookup used before the check) e.g. replace references
to `ctx.session.user.name` with `user.name`, and add a null-safe guard (ensure
`user` exists) so `redirectUrl = "/fill-out-name"` only when the persisted
`user.name` is empty.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/server/api/routers/organizations.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-23T05:44:37.925Z
Learnt from: tanvimahal
Repo: Code-the-Change-YYC/salvationarmy PR: 25
File: src/server/api/routers/organizations.ts:38-38
Timestamp: 2025-12-23T05:44:37.925Z
Learning: In this codebase's TRPC protectedProcedure context, ctx.session has a nested structure containing both a session object and a user object. To access activeOrganizationId, use ctx.session.session.activeOrganizationId (not ctx.session.activeOrganizationId).
Applied to files:
src/server/api/routers/organizations.ts
🧬 Code graph analysis (1)
src/server/api/routers/organizations.ts (3)
src/server/db/auth-schema.ts (2)
organization(72-79)user(6-20)src/server/api/trpc.ts (1)
protectedProcedure(122-132)src/types/validation.ts (1)
nameRegex(12-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci
🔇 Additional comments (2)
src/server/api/routers/organizations.ts (2)
2-2: LGTM! Imports are correctly added.The new imports (
eq,user,nameRegex) are all used appropriately in the newchangeNameprocedure and support the required functionality.Also applies to: 12-12, 14-14
43-74: Good enhancement: Robust organization slug validation.The AGENCY role handling now includes comprehensive validation for activeOrganizationId, organization existence, and slug presence with clear error messages. The code correctly accesses
ctx.session.session.activeOrganizationIdper the codebase's nested session structure.Based on learnings, the nested session access pattern is correct for this codebase.
| if (!nameRegex.test(newName)) { | ||
| //User inputted name is not proper | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: "Invalid Name", | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the validation file and examine nameRegex definition
find . -type f -name "validation.ts" -o -name "validation.js" | head -20Repository: Code-the-Change-YYC/salvationarmy
Length of output: 102
🏁 Script executed:
# Also look for the organizations router file
find . -type f -path "*/api/routers/organizations.ts" | head -5Repository: Code-the-Change-YYC/salvationarmy
Length of output: 118
🏁 Script executed:
cat -n src/types/validation.ts | head -100Repository: Code-the-Change-YYC/salvationarmy
Length of output: 662
🏁 Script executed:
cat -n src/server/api/routers/organizations.ts | sed -n '305,335p'Repository: Code-the-Change-YYC/salvationarmy
Length of output: 1170
🏁 Script executed:
# Run the test script to verify regex behavior
cat << 'EOF' > test_names.py
import re
# Pattern from validation.ts (exactly as defined)
nameRegex = r"^(?!.*\s{2})[A-Za-z0-9][A-Za-z0-9\s]*[A-Za-z0-9][A-Za-z0-9\s]*[A-Za-z0-9]$"
test_cases = [
("Al", "short 2-char name"),
("X", "single character"),
("John Doe", "standard name"),
("John Smith", "2-word name"),
("Mary-Jane", "hyphenated"),
("O'Brien", "apostrophe"),
("José", "accented"),
("François", "accented"),
("李明", "non-Latin"),
("John Doe", "double space"),
("123", "numeric only"),
("John123", "alphanumeric"),
("A B C", "single letters with spaces"),
]
print("Testing nameRegex pattern:\n")
for name, desc in test_cases:
matches = bool(re.match(nameRegex, name))
status = "✓ PASS" if matches else "✗ FAIL"
print(f"{status} | {name:20} | {desc}")
EOF
python test_names.pyRepository: Code-the-Change-YYC/salvationarmy
Length of output: 163
🏁 Script executed:
# Check if ctx.session is updated after the DB update in changeName
rg -A 20 "changeName:" src/server/api/routers/organizations.ts | head -40Repository: Code-the-Change-YYC/salvationarmy
Length of output: 782
🏁 Script executed:
# Test regex pattern with a simpler approach using Node.js
node << 'EOF'
const nameRegex = /^(?!.*\s{2})[A-Za-z0-9][A-Za-z0-9\s]*[A-Za-z0-9][A-Za-z0-9\s]*[A-Za-z0-9]$/;
const testCases = [
["Al", "short 2-char name"],
["X", "single character"],
["John Doe", "standard name"],
["Mary-Jane", "hyphenated"],
["O'Brien", "apostrophe"],
["José", "accented"],
["123", "numeric only"],
["John123", "alphanumeric"],
];
console.log("Testing nameRegex pattern:\n");
testCases.forEach(([name, desc]) => {
const matches = nameRegex.test(name);
const status = matches ? "✓ PASS" : "✗ FAIL";
console.log(`${status} | ${name.padEnd(20)} | ${desc}`);
});
EOFRepository: Code-the-Change-YYC/salvationarmy
Length of output: 466
🏁 Script executed:
# Check if there's any session update logic after the DB update in changeName or related functions
rg -B 5 -A 15 "ctx.session" src/server/api/routers/organizations.ts | grep -A 15 "changeName"Repository: Code-the-Change-YYC/salvationarmy
Length of output: 59
🏁 Script executed:
# Search for any session refresh or revalidation logic in the codebase related to user updates
rg -l "session.*refresh|revalidateSession|unstable_getServerSession" src/ | head -10Repository: Code-the-Change-YYC/salvationarmy
Length of output: 59
Name validation regex is too restrictive and rejects legitimate names.
The nameRegex pattern enforces requirements that block common, valid name formats:
- Minimum 3 characters required: rejects short names like "Al", "Bo", or single-letter names
- Hyphens rejected: "Mary-Jane" fails validation
- Apostrophes rejected: "O'Brien" fails validation
- Accented characters rejected: "José", "François" fail validation
- Only allows alphanumeric + spaces: no special characters supported
This creates a poor user experience for people with legitimate names that don't fit this narrow pattern. Additionally, after the database update on line 321–325, ctx.session.user.name is not refreshed, meaning subsequent operations in the same session will reference stale user data.
| const listOfUpdatedUsers = await ctx.db | ||
| .update(user) | ||
| .set({ name: newName }) | ||
| .where(eq(user.id, ctx.session.user.id)) | ||
| .returning(); //Make DB update command to change the user's name | ||
|
|
||
| if (listOfUpdatedUsers.length === 0) { | ||
| //Drizzle updated no users (something went wrong) | ||
| throw new TRPCError({ | ||
| code: "INTERNAL_SERVER_ERROR", | ||
| message: "Failed to update user's name", | ||
| }); | ||
| } | ||
| }), |
There was a problem hiding this comment.
Session not refreshed after database update.
After updating the user's name in the database, ctx.session.user.name remains unchanged until the session is refreshed. This means subsequent calls within the same session will see the old name value.
Impact: This contributes to the critical issue flagged in redirectToDashboard (line 34), where the session's stale name causes incorrect redirect logic. Once that issue is fixed by using the database user's name instead of the session name, this concern is mitigated.
Note: The recommended fix is to use the database user record consistently in redirectToDashboard rather than attempting to refresh the session here.
🤖 Prompt for AI Agents
In @src/server/api/routers/organizations.ts around lines 321 - 334, The DB
update in the mutation updates the user's name (result in listOfUpdatedUsers)
but does not refresh ctx.session.user.name so downstream logic sees a stale
session name; instead of trying to refresh session here, update
redirectToDashboard to read the authoritative name from the database record
(query the users table by ctx.session.user.id and use the returned user.name)
and ensure any post-update logic uses that DB value (refer to
listOfUpdatedUsers, ctx.session.user.name, and redirectToDashboard) so session
staleness is avoided.
Attached is a video demonstrating the login process, starting when the user has a name (directs them to /dashboard) and then when the user has no name (directs them to /fill-out-name).
In the video, /dashboard was a copy-paste of the agency-interative-area.tsx file. It was only added since the endpoint does not yet exist. It is not included in the final branch (/dashboard remains non-existent)
SANC-48.Video.mp4
Summary by CodeRabbit
New Features
Improvements
Validation
✏️ Tip: You can customize this high-level summary in your review settings.