[774] fix: Improve phone number validation error messages - #209
Conversation
WalkthroughPhone-number validation was refactored: the PHONE_NUMBER schema now trims input, rejects alphabetic characters early, enforces digits count between 10 and 15 with explicit "Phone number is too short"/"Phone number is too long" errors, and delegates formatting/validation to an imported formatPhoneNumber function. Tests in formValidator.test.js, phoneNumberValidator.test.js, and validationSchemas.test.js were updated to expect simplified or more specific error messages and some tests now call validation directly rather than using a helper. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
VitalyyP
changed the title
🔍 Vulnerabilities of
|
| digest | sha256:a21a3d5a0b18fa56ae00491dba7b68ac957ec6c06ca0e013306079ef1aaa2b7a |
| vulnerabilities |     |
| platform | linux/amd64 |
| size | 291 MB |
| packages | 984 |
📦 Base Image node:23-alpine
| also known as |
|
| digest | sha256:b9d38d589853406ff0d4364f21969840c3e0397087643aef8eede40edbb6c7cd |
| vulnerabilities |     |
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
|
![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 (3)
website/modules/asset/ui/src/js/formValidator.test.js (1)
17-18: Avoid hard-coding the new error string in multiple placesYou updated
PHONE_INVALID, but the tests below embed'Phone number is too short'inline. Expose aPHONE_TOO_SHORTconstant (similar to the other message constants) and reuse it here:+ PHONE_TOO_SHORT: 'Phone number is too short',and replace the inline literals with the constant. This keeps future copy edits to a single change.
Also applies to: 202-208
website/modules/asset/ui/src/js/validationSchemas.test.js (1)
110-113: Duplicate literal strings across test suitesThe literal
'Phone number is too short'and'Enter a valid phone number'are now repeated across three separate test files. Consider importing the same message constants used by the validator so that tests stay in sync with the schema wording.Also applies to: 124-126, 130-132
website/modules/asset/ui/src/js/phoneNumberValidator.test.js (1)
8-9: Keep message sources and helpers consistent
You introduce
ERROR_MESSAGEbut still hard-code the same string in later assertions. Re-use the constant to avoid accidental drift.The bespoke assertions in the two middle tests duplicate the logic already encapsulated in
expectInvalidPhone. Using the helper keeps the suite DRY and aligned with future message tweaks.Refactor example:
- it('rejects invalid international format', async () => { - phoneInput.value = '+123'; - const result = await validateField(phoneInput); - expect(result).toEqual({ - isValid: false, - message: ERROR_MESSAGE, - }); - }); + it('rejects invalid international format', async () => + expectInvalidPhone('+123'));Also applies to: 35-42, 45-51
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
website/modules/asset/ui/src/js/formValidator.test.js(2 hunks)website/modules/asset/ui/src/js/phoneNumberValidator.test.js(2 hunks)website/modules/asset/ui/src/js/validationSchemas.js(2 hunks)website/modules/asset/ui/src/js/validationSchemas.test.js(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: yuramax
PR: speedandfunction/website#120
File: website/modules/asset/ui/src/index.js:43-67
Timestamp: 2025-05-26T06:19:32.209Z
Learning: When reviewing PRs focused on styling, defer functional JavaScript improvements to separate tasks to maintain clear scope separation.
Learnt from: yuramax
PR: speedandfunction/website#139
File: website/jest.config.cjs:4-4
Timestamp: 2025-06-03T16:27:46.172Z
Learning: When reviewing PRs, focus suggestions on the main objectives stated in the PR description. Skip suggestions for incidental changes that are unrelated to the PR's primary purpose, especially when the user explicitly requests to stay focused on the main feature/improvement.
📚 Learning: 2025-07-01T07:44:33.034Z
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/apostrophecms/form/index.js : modules/apostrophecms/form/index.js: relaxed maximum lines rule
Applied to files:
website/modules/asset/ui/src/js/formValidator.test.jswebsite/modules/asset/ui/src/js/validationSchemas.test.jswebsite/modules/asset/ui/src/js/validationSchemas.jswebsite/modules/asset/ui/src/js/phoneNumberValidator.test.js
📚 Learning: 2025-07-01T07:44:33.034Z
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to **/*.test.{js,jsx} : Test files have relaxed rules for function length, statements, extraneous requires, and destructuring
Applied to files:
website/modules/asset/ui/src/js/phoneNumberValidator.test.js
📚 Learning: 2025-07-01T07:44:33.034Z
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to **/*.e2e.{js,jsx} : E2E test files: relaxed import, ternary, and sorting rules
Applied to files:
website/modules/asset/ui/src/js/phoneNumberValidator.test.js
🧬 Code Graph Analysis (2)
website/modules/asset/ui/src/js/validationSchemas.js (1)
website/modules/asset/ui/src/js/phoneFormat.js (1)
formatPhoneNumber(163-174)
website/modules/asset/ui/src/js/phoneNumberValidator.test.js (1)
website/modules/asset/ui/src/js/formValidator.js (1)
validateField(39-74)
⏰ 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). (4)
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: security-scan
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
website/modules/asset/ui/src/js/validationSchemas.js (1)
43-56: Resolved prior concern: length checks now use digit count, with specific messagesMoving min/max into the custom test and counting digits fixes the “raw length vs. digit length” issue and ensures users see “too short/too long.” This also addresses the previous review’s feedback, and the .trim() addition is a plus.
🧹 Nitpick comments (3)
website/modules/asset/ui/src/js/validationSchemas.test.js (1)
81-134: Add trimming and boundary tests for phone numbersSince .trim() was added to the phone number schema, add a test to confirm whitespace is ignored. Also consider boundary tests for exactly 10 and exactly 15 digits.
Would you like me to open a follow-up PR adding:
- accepts phone number with surrounding whitespace
- accepts exactly 10 digits
- accepts exactly 15 digits
Example tests to add:
test('trims whitespace around phone number', async () => { await expect(schema.validate(' (234) 567-8900 ')).resolves.toBe('(234) 567-8900'); }); test('accepts exactly 10 digits', async () => { await expect(schema.validate('2345678900')).resolves.toBe('2345678900'); }); test('accepts exactly 15 digits', async () => { await expect(schema.validate('+123456789012345')).resolves.toBe('+123456789012345'); });website/modules/asset/ui/src/js/validationSchemas.js (2)
41-42: Broaden letter detection to all Unicode letters (optional)/[A-Za-z]/ matches only Latin letters. If you want to block any script’s letters (e.g., Cyrillic, Greek), prefer Unicode properties. Node 12+ supports this.
- if (/[A-Za-z]/u.test(value)) return false; + // Disallow any letter from any script + if (/\p{L}/u.test(value)) return false;If older runtimes must be supported, consider a negative char class allow-list instead (digits and common punctuation only).
58-60: Delegation to formatPhoneNumber is clearReturning Boolean(formatted) makes the generic “Enter a valid phone number” the fallback for non-digit-length issues. Consider adding unit tests for exactly-10 and exactly-15 digits to cover boundaries (see test-file note).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
website/modules/asset/ui/src/js/validationSchemas.js(2 hunks)website/modules/asset/ui/src/js/validationSchemas.test.js(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
website/modules/asset/ui/src/js/validationSchemas.test.js (1)
website/modules/asset/ui/src/js/formValidator.js (2)
schema(25-25)schema(53-53)
⏰ 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). (4)
- GitHub Check: unit-tests
- GitHub Check: e2e-tests
- GitHub Check: security-scan
- GitHub Check: lint
🔇 Additional comments (6)
website/modules/asset/ui/src/js/validationSchemas.test.js (4)
110-113: Updated message matches new schema behaviorSwitch to "Phone number is too short" is consistent with digit-length validation. LGTM.
123-127: Renamed test and simplified message are appropriateRenaming to “rejects phone number with only letters” and asserting "Enter a valid phone number" aligns with the formatter-first flow.
129-133: Consistent invalid-format messageUsing "Enter a valid phone number" for mixed alphanumerics matches the schema’s fallback. Good.
123-127: Ignore trailing-period suggestion for phone number messageOur grep search found no instances of
'Enter a valid phone number.'(with a period). All occurrences use the message without a trailing period:
- website/modules/asset/ui/src/js/validationSchemas.js (line 39)
- website/modules/asset/ui/src/js/phoneNumberValidator.test.js (line 49)
- website/modules/asset/ui/src/js/validationSchemas.test.js (lines 125, 131)
- website/modules/asset/ui/src/js/formValidator.test.js (line 17)
You can safely ignore this review comment.
Likely an incorrect or invalid review comment.
website/modules/asset/ui/src/js/validationSchemas.js (2)
37-37: Good addition: .trim()Trimming ensures empty/space-only inputs hit the required check consistently. LGTM.
1-1: No issues with formatPhoneNumber importThe file
website/modules/asset/ui/src/js/phoneFormat.jsexists and correctly exports a namedformatPhoneNumberfunction alongsideinitPhoneFormatting. The destructuring import invalidationSchemas.jsis safe and will not cause runtime errors.
|
@coderabbitai re-run |
|
@VitalyyP I'll re-run the review for this PR. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
website/modules/asset/ui/src/js/validationSchemas.js (1)
39-59: Don’t override “required” error and support non-ASCII letters in the checkTwo targeted tweaks:
- Let .required() surface for empty values to avoid showing “too short” on empty input if abortEarly is disabled upstream.
- Use Unicode property escapes to catch letters beyond A–Z (e.g., “тест”, “ábč”).
- .test('phone-format', 'Enter a valid phone number', (value, context) => { - if (/[A-Za-z]/u.test(value)) return false; + .test('phone-format', 'Enter a valid phone number', (value, context) => { + // Defer empty/undefined handling to .required() above + if (!value) return true; + if (/\p{L}/u.test(value)) return false;No change needed below for the digit-length checks and format validation.
🧹 Nitpick comments (5)
website/modules/asset/ui/src/js/formValidator.test.js (2)
200-208: Avoid magic strings: reuse a constant for “too short”The literal "Phone number is too short" is duplicated in this suite. Prefer a constant for consistency and easier future changes.
Apply this diff to use a constant here:
{ description: 'rejects too short phone number', value: TEST_CONSTANTS.INVALID_SAMPLES.PHONE_SHORT, - message: 'Phone number is too short', + message: TEST_CONSTANTS.MESSAGES.PHONE_TOO_SHORT, }, { description: 'rejects invalid international format', value: TEST_CONSTANTS.INVALID_SAMPLES.PHONE_INVALID, - message: 'Phone number is too short', + message: TEST_CONSTANTS.MESSAGES.PHONE_TOO_SHORT, },And define the constants in the MESSAGES block (outside the selected range):
// Add to TEST_CONSTANTS.MESSAGES PHONE_TOO_SHORT: 'Phone number is too short', PHONE_TOO_LONG: 'Phone number is too long',
198-210: Add a “too long” phone validation case to cover the upper boundWe test “too short” here; adding a “too long” case strengthens integration coverage at form level to mirror schema tests.
additionalTests: [ { description: 'rejects too short phone number', value: TEST_CONSTANTS.INVALID_SAMPLES.PHONE_SHORT, message: TEST_CONSTANTS.MESSAGES.PHONE_TOO_SHORT, }, { description: 'rejects invalid international format', value: TEST_CONSTANTS.INVALID_SAMPLES.PHONE_INVALID, message: TEST_CONSTANTS.MESSAGES.PHONE_TOO_SHORT, }, + { + description: 'rejects too long phone number', + value: '1'.repeat(16), // 16 digits + message: TEST_CONSTANTS.MESSAGES.PHONE_TOO_LONG, + }, ],website/modules/asset/ui/src/js/validationSchemas.test.js (1)
123-127: Broaden letter-only rejection to cover non-ASCII letters; add a testThe schema currently checks
[A-Za-z]; this misses non-ASCII letters (e.g., “тест”, “ábč”). Consider adding a test to document expected behavior and updating the schema to use Unicode properties.Add this test (outside the selected range):
test('rejects phone number with only non-ASCII letters', async () => { await expect(schema.validate('тест')).rejects.toThrow('Enter a valid phone number'); await expect(schema.validate('ábč')).rejects.toThrow('Enter a valid phone number'); });Then update the schema regex to
\p{L}(see comment in validationSchemas.js).website/modules/asset/ui/src/js/phoneNumberValidator.test.js (2)
35-42: Reuse the ERROR_MESSAGE constant to avoid duplicationYou already defined ERROR_MESSAGE; use it here too.
it('rejects invalid international format', async () => { phoneInput.value = '+123'; const result = await validateField(phoneInput); expect(result).toEqual({ isValid: false, - message: 'Phone number is too short', + message: ERROR_MESSAGE, }); });
44-51: Extract the “invalid” message into a constant for consistencyThis keeps message strings centralized in tests.
Apply this change within this block:
it('rejects phone number with letters', async () => { phoneInput.value = '+1 (234) ABC-1234'; const result = await validateField(phoneInput); expect(result).toEqual({ isValid: false, - message: 'Enter a valid phone number', + message: INVALID_PHONE_MESSAGE, }); });And define the constant near ERROR_MESSAGE (outside the selected range):
const INVALID_PHONE_MESSAGE = 'Enter a valid phone number';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (4)
website/modules/asset/ui/src/js/formValidator.test.js(2 hunks)website/modules/asset/ui/src/js/phoneNumberValidator.test.js(2 hunks)website/modules/asset/ui/src/js/validationSchemas.js(2 hunks)website/modules/asset/ui/src/js/validationSchemas.test.js(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
website/modules/asset/ui/src/js/phoneNumberValidator.test.js (3)
website/modules/asset/ui/src/js/formValidator.test.js (5)
result(55-55)result(68-68)result(77-77)result(87-87)result(93-93)website/modules/asset/ui/src/js/formValidation.test.js (1)
validateField(23-23)website/modules/asset/ui/src/js/formValidator.js (1)
validateField(39-74)
website/modules/asset/ui/src/js/validationSchemas.test.js (1)
website/modules/asset/ui/src/js/formValidator.js (2)
schema(25-25)schema(53-53)
website/modules/asset/ui/src/js/validationSchemas.js (1)
website/modules/asset/ui/src/js/phoneFormat.js (1)
formatPhoneNumber(163-174)
🔇 Additional comments (3)
website/modules/asset/ui/src/js/formValidator.test.js (1)
17-17: Message update aligns with new validation UX — looks goodSwitching PHONE_INVALID to the shorter "Enter a valid phone number" matches the updated schema and other tests.
website/modules/asset/ui/src/js/validationSchemas.test.js (2)
110-114: Specific “too short” message assertion is correctThis matches the schema’s digit-count path and ensures the user gets the more actionable error.
129-133: Letters mixed with digits: expectation matches the generic invalid messageGood coverage for non-digit contamination.
|
3 participants
Uh oh!
There was an error while loading. Please reload this page.