feat(user): add phone number to user#3230
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds ChangesUser phone number support
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant UserSettingsRoute
participant UserEntity
participant Database
Client->>UserSettingsRoute: Submit phoneNumber
UserSettingsRoute->>UserSettingsRoute: Validate phoneNumber
UserSettingsRoute->>UserEntity: Set user.phoneNumber
UserEntity->>Database: Persist user
Database-->>UserSettingsRoute: Return saved user
UserSettingsRoute-->>Client: Return phoneNumber
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/UserProfile/UserSettings/UserGeneralSettings/index.tsx (1)
158-172: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
phoneNumberinitial value missing fallback, inconsistent with sibling fields.
displayNameand''when the loaded value is empty/undefined (Lines 159-160), butphoneNumberis set directly fromdata?.phoneNumber(Line 161). Since the DB column is nullable,data?.phoneNumbercan beundefined, which makes thetelinput start uncontrolled and switch to controlled once populated — triggering React's "component is changing an uncontrolled input to be controlled" warning.🐛 Proposed fix
- phoneNumber: data?.phoneNumber, + phoneNumber: data?.phoneNumber ?? '',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/UserProfile/UserSettings/UserGeneralSettings/index.tsx` around lines 158 - 172, The UserGeneralSettings form is passing phoneNumber directly into the initialValues object, unlike displayName and email, so it can start as undefined and make the tel input switch from uncontrolled to controlled. Update the initialValues in UserGeneralSettings to give phoneNumber a safe empty-string fallback when data?.phoneNumber is missing, matching the pattern used for the other fields and keeping the input controlled from the start.
🧹 Nitpick comments (2)
docs/using-seerr/notifications/webhook.md (1)
138-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWording says "requesting" instead of "reporting" user.
For the
reportedBy_phoneNumberrow (under the Issue section), the description should reference the "reporting" user, not "requesting" user. Note this mirrors an existing wording issue on the neighboring unchangedreportedBy_username/reportedBy_emailrows (Lines 136-137), so this new row simply follows that same pre-existing pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/using-seerr/notifications/webhook.md` at line 138, Update the `reportedBy_phoneNumber` row in the webhook notifications table so its description matches the surrounding `reportedBy_username` and `reportedBy_email` rows and refers to the “reporting” user instead of the “requesting” user. Keep the wording consistent within the Issue section by editing the table entry in the markdown content only.server/routes/user/index.ts (1)
206-215: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider validating
phoneNumberformat server-side.
body.phoneNumberis persisted without any format validation, relying solely on frontendPHONE_REGEX. Since this value now feeds webhook payloads for external SMS/WhatsApp integrations, an unvalidated/malformed number here could silently break those notifications for admin-created users.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/routes/user/index.ts` around lines 206 - 215, The user creation flow in the User constructor is persisting body.phoneNumber without any server-side validation. Add a phone number format check in the server route before constructing the User, ideally using the same validation rules enforced elsewhere so malformed numbers are rejected consistently. Update the user creation handler to validate body.phoneNumber alongside the existing email/username fields and only assign it when it passes validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@seerr-api.yml`:
- Around line 4267-4268: The shared User schema is missing the phoneNumber
field, so update the schema definition used by /user, /auth/me, and
/user/{userId} to include phoneNumber alongside the existing user properties.
Reuse the same schema component where the User object is defined rather than
only adding it to the POST /user request shape, so the OpenAPI response contract
stays consistent.
In `@server/entity/User.ts`:
- Around line 80-82: The nullable phoneNumber field in User should be typed to
match the DB nullability and sibling columns. Update the phoneNumber property in
the User entity to include null in its TypeScript type, consistent with
plexUsername and jellyfinUsername. Keep the `@Column`({ nullable: true }) mapping
unchanged and ensure the property reflects both undefined and null cases if
needed.
---
Outside diff comments:
In `@src/components/UserProfile/UserSettings/UserGeneralSettings/index.tsx`:
- Around line 158-172: The UserGeneralSettings form is passing phoneNumber
directly into the initialValues object, unlike displayName and email, so it can
start as undefined and make the tel input switch from uncontrolled to
controlled. Update the initialValues in UserGeneralSettings to give phoneNumber
a safe empty-string fallback when data?.phoneNumber is missing, matching the
pattern used for the other fields and keeping the input controlled from the
start.
---
Nitpick comments:
In `@docs/using-seerr/notifications/webhook.md`:
- Line 138: Update the `reportedBy_phoneNumber` row in the webhook notifications
table so its description matches the surrounding `reportedBy_username` and
`reportedBy_email` rows and refers to the “reporting” user instead of the
“requesting” user. Keep the wording consistent within the Issue section by
editing the table entry in the markdown content only.
In `@server/routes/user/index.ts`:
- Around line 206-215: The user creation flow in the User constructor is
persisting body.phoneNumber without any server-side validation. Add a phone
number format check in the server route before constructing the User, ideally
using the same validation rules enforced elsewhere so malformed numbers are
rejected consistently. Update the user creation handler to validate
body.phoneNumber alongside the existing email/username fields and only assign it
when it passes validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fdf887c7-c0e5-4f0c-b79d-7f64124bf20f
📒 Files selected for processing (16)
docs/using-seerr/notifications/webhook.mdseerr-api.ymlserver/entity/User.tsserver/interfaces/api/userSettingsInterfaces.tsserver/lib/notifications/agents/webhook.tsserver/migration/postgres/1783161650264-AddPhoneNumberToUser.tsserver/migration/sqlite/1783161620407-AddPhoneNumberToUser.tsserver/routes/user/index.tsserver/routes/user/usersettings.tssrc/components/Settings/Notifications/NotificationsWebhook/index.tsxsrc/components/UserList/index.tsxsrc/components/UserProfile/UserSettings/UserGeneralSettings/index.tsxsrc/i18n/locale/de.jsonsrc/i18n/locale/en.jsonsrc/styles/globals.csssrc/utils/numberHelpers.ts
modarah
force-pushed
the
feature/add-phone-number-to-users
branch
from
2689893 to
6ac5f64
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/routes/user/usersettings.ts (1)
115-120: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake phone-number validation unconditional.
user.phoneNumber && !result.successonly rejects bad input once a phone number already exists, so users with a phone number can’t clear it, and invalid input for users without one is ignored instead of returning 400. Allow an explicit empty value to clear the field and fail fast on any invalid format.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/routes/user/usersettings.ts` around lines 115 - 120, Update the phone-number validation in the user settings handler so any unsuccessful phoneNumberSchema.safeParse result returns ApiError 400 InvalidPhoneNumber, regardless of the existing user.phoneNumber value. Preserve explicit empty-value support so a valid empty input clears the field, then assign the parsed value to user.phoneNumber.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server/routes/user/usersettings.ts`:
- Around line 115-120: Update the phone-number validation in the user settings
handler so any unsuccessful phoneNumberSchema.safeParse result returns ApiError
400 InvalidPhoneNumber, regardless of the existing user.phoneNumber value.
Preserve explicit empty-value support so a valid empty input clears the field,
then assign the parsed value to user.phoneNumber.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86a17f57-fbf1-4ad0-a044-322a8ddd728a
📒 Files selected for processing (2)
server/entity/User.tsserver/routes/user/usersettings.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- server/entity/User.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/routes/user/usersettings.ts (2)
30-30: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider requiring the
+prefix and verify frontend regex parity.The regex
/^\+?[0-9]{1,15}$/makes the+optional. For international SMS/WhatsApp delivery (the stated use case per PR objectives), E.164 numbers typically require the+prefix. The screenshot shows"123456453"stored without+, which may cause delivery failures depending on the downstream service.Additionally, the frontend defines its own
PHONE_REGEXinsrc/utils/numberHelpers.ts(per the stack context). If the two regexes diverge, a number that passes frontend validation could be rejected by the backend (or vice versa), causing a confusing UX. Verify they match.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/routes/user/usersettings.ts` at line 30, Update phoneNumberSchema to require the international “+” prefix for E.164-style numbers, and verify that the frontend PHONE_REGEX in numberHelpers.ts uses the same validation rule. Keep frontend and backend acceptance criteria identical so valid numbers are handled consistently.
115-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRefactor for cleaner type narrowing on
safeParseresult.
result.datais accessed on line 120 without first narrowing onresult.success. The throw conditionreq.body.phoneNumber && !result.successdoesn't give TypeScript enough information to narrow the discriminated union, soresult.datamay beundefinedon the failure branch. The?? ''fallback handles this at runtime, but the pattern is fragile and non-idiomatic.Restructuring so
result.datais only accessed after a successful parse makes the type narrowing explicit and eliminates the reliance on??:♻️ Proposed refactor
- const result = phoneNumberSchema.safeParse(req.body.phoneNumber); - if (req.body.phoneNumber && !result.success) { - throw new ApiError(400, ApiErrorCode.InvalidPhoneNumber); - } - - user.phoneNumber = result.data ?? ''; + if (req.body.phoneNumber) { + const result = phoneNumberSchema.safeParse(req.body.phoneNumber); + if (!result.success) { + throw new ApiError(400, ApiErrorCode.InvalidPhoneNumber); + } + user.phoneNumber = result.data; + } else { + user.phoneNumber = ''; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/routes/user/usersettings.ts` around lines 115 - 121, Refactor the phone number parsing flow around phoneNumberSchema.safeParse so result.data is accessed only within an explicit result.success branch. Preserve the existing InvalidPhoneNumber ApiError for provided invalid values, and assign user.phoneNumber to the successfully parsed value or an empty string when no phone number is provided, without relying on result.data ?? ''.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/routes/user/usersettings.ts`:
- Line 30: Update phoneNumberSchema to require the international “+” prefix for
E.164-style numbers, and verify that the frontend PHONE_REGEX in
numberHelpers.ts uses the same validation rule. Keep frontend and backend
acceptance criteria identical so valid numbers are handled consistently.
- Around line 115-121: Refactor the phone number parsing flow around
phoneNumberSchema.safeParse so result.data is accessed only within an explicit
result.success branch. Preserve the existing InvalidPhoneNumber ApiError for
provided invalid values, and assign user.phoneNumber to the successfully parsed
value or an empty string when no phone number is provided, without relying on
result.data ?? ''.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e771c7c-25bf-4a08-bd7b-fa42df7814d3
📒 Files selected for processing (1)
server/routes/user/usersettings.ts
modarah
force-pushed
the
feature/add-phone-number-to-users
branch
from
3a19174 to
5bace0c
Compare
1 participant
Description
Add new column to user table
Add phone number field to user and settings
Support phone number in webhook
Closes New attribute on users for saving phone number #751
How Has This Been Tested?
pnpm testandpnpm buildAI
Screenshots / Logs (if applicable)
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit
+plus 1–15 digits).telinputs to match other form fields.