fix(profile): improve full name validation with clearer error messages#1396
fix(profile): improve full name validation with clearer error messages#1396rohilsurana wants to merge 4 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughName validation in the user update form was refactored into multiple constraints: names must start and end with a letter, allow Unicode letters and periods, disallow consecutive spaces, and require spaces/hyphens/apostrophes to be followed by a letter; per-constraint messages were added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
Pull Request Test Coverage Report for Build 22171604098Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/lib/react/components/organization/user/update.tsx (1)
35-38: Simplify the "end with letter" regex — the single-char alternative|^[a-zA-Z]$is unreachable dead code.
.min(2)is enforced before this.matches(), so a single-character string never reaches it. The entire two-alternative regex can be reduced to a simpler end-anchor check (the start constraint is already covered by the preceding rule):♻️ Proposed simplification
- .matches( - /^[a-zA-Z][a-zA-Z\s.'\-]*[a-zA-Z]$|^[a-zA-Z]$/, - 'Name must end with a letter' - ) + .matches( + /[a-zA-Z]$/, + 'Name must end with a letter' + )
There was a problem hiding this comment.
Period validation gap — consecutive periods (e.g., "John..Doe") pass all checks.
The PR description states that periods, like spaces/hyphens/apostrophes, must be followed by a letter, but no .matches() constraint enforces this for periods. As a result:
"John..Doe"— two consecutive periods — is accepted as valid."Dr..Smith"— same issue.
Note: "Dr. Smith" (period → space → letter) is correctly valid per the current code because the existing space-follow check handles the space after the period. However, any period followed directly by another non-letter special character (e.g., another period) slips through.
If the intent is to match the stated requirement, add:
🛡️ Proposed fix to enforce letter-after-period
.matches(
/^(?!.*'[^\p{L}]).*$/u,
'Apostrophes must be followed by a letter'
- ),
+ )
+ .matches(
+ /^(?!.*\.[^\p{L}\s]).*$/u,
+ 'Periods must be followed by a letter or space'
+ ),If the stricter rule (period must be followed only by a letter, not a space) is desired — which would invalidate
"Dr. Smith"— use/^(?!.*\.[^\p{L}]).*$/uinstead. Confirm the intended behaviour against the listed valid test cases before applying.
🧰 Tools
🪛 GitHub Check: JS SDK Lint
[failure] 33-33:
Unnecessary escape character: -
[failure] 28-28:
Unnecessary escape character: -
| /^[\p{L}\s.'-]+$/u, | ||
| 'Name can only contain letters, spaces, periods, hyphens, and apostrophes' | ||
| ) | ||
| .matches(/^\p{L}/u, 'Name must start with a letter') | ||
| .matches( | ||
| /^\p{L}[\p{L}\s.'-]*\p{L}$|^\p{L}$/u, | ||
| 'Name must end with a letter' | ||
| ) | ||
| .matches(/^(?!.*\s\s).*$/, 'Name cannot have consecutive spaces') | ||
| .matches(/^(?!.*\s[^\p{L}]).*$/u, 'Spaces must be followed by a letter') |
There was a problem hiding this comment.
\s permits tabs, newlines, and other whitespace — use a literal space instead.
\s matches \t, \n, \r, \f, \v, and (with the u flag) additional Unicode whitespace characters. A value like "John\tDoe" would pass every check in this chain. The error messages and PR description consistently refer to "spaces," so the intent is clearly to allow only the ASCII space character.
🔧 Proposed fix — replace \s with literal space in all four regexes
.matches(
- /^[\p{L}\s.'-]+$/u,
+ /^[\p{L} .'-]+$/u,
'Name can only contain letters, spaces, periods, hyphens, and apostrophes'
)
.matches(/^\p{L}/u, 'Name must start with a letter')
.matches(
- /^\p{L}[\p{L}\s.'-]*\p{L}$|^\p{L}$/u,
+ /^\p{L}[\p{L} .'-]*\p{L}$|^\p{L}$/u,
'Name must end with a letter'
)
- .matches(/^(?!.*\s\s).*$/, 'Name cannot have consecutive spaces')
- .matches(/^(?!.*\s[^\p{L}]).*$/u, 'Spaces must be followed by a letter')
+ .matches(/^(?!.* ).*$/, 'Name cannot have consecutive spaces')
+ .matches(/^(?!.* [^\p{L}]).*$/u, 'Spaces must be followed by a letter')📝 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.
| /^[\p{L}\s.'-]+$/u, | |
| 'Name can only contain letters, spaces, periods, hyphens, and apostrophes' | |
| ) | |
| .matches(/^\p{L}/u, 'Name must start with a letter') | |
| .matches( | |
| /^\p{L}[\p{L}\s.'-]*\p{L}$|^\p{L}$/u, | |
| 'Name must end with a letter' | |
| ) | |
| .matches(/^(?!.*\s\s).*$/, 'Name cannot have consecutive spaces') | |
| .matches(/^(?!.*\s[^\p{L}]).*$/u, 'Spaces must be followed by a letter') | |
| /^[\p{L} .'-]+$/u, | |
| 'Name can only contain letters, spaces, periods, hyphens, and apostrophes' | |
| ) | |
| .matches(/^\p{L}/u, 'Name must start with a letter') | |
| .matches( | |
| /^\p{L}[\p{L} .'-]*\p{L}$|^\p{L}$/u, | |
| 'Name must end with a letter' | |
| ) | |
| .matches(/^(?!.* ).*$/, 'Name cannot have consecutive spaces') | |
| .matches(/^(?!.* [^\p{L}]).*$/u, 'Spaces must be followed by a letter') |
2 participants
Summary
Changes
Test plan