feat: add isValidPhoneNumber and formatPhoneNumberOrThrow to phone-number #210
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds phone-number validation and types, exposes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Formatter as formatPhoneNumber
participant ThrowVariant as formatPhoneNumberOrThrow
Caller->>ThrowVariant: formatPhoneNumberOrThrow(params)
activate ThrowVariant
ThrowVariant->>Formatter: formatPhoneNumber(params)
activate Formatter
note right of Formatter: Returns ServiceResult<string>
Formatter-->>ThrowVariant: Ok | Err(error)
deactivate Formatter
alt Ok
ThrowVariant-->>Caller: string
else Err
note right of ThrowVariant: Convert/propagate error and throw
ThrowVariant-->>Caller: throws Error
end
deactivate ThrowVariant
sequenceDiagram
autonumber
actor Caller
participant Validator as isValidPhoneNumber
participant Lib as libphonenumber-js
Caller->>Validator: isValidPhoneNumber({ phoneNumber })
activate Validator
Validator->>Validator: trim(phoneNumber)
Validator->>Lib: isValidPhoneNumber(trimmed, { defaultCountry: "US" })
Lib-->>Validator: boolean
Validator-->>Caller: boolean
deactivate Validator
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Summary
This PR enhances the phone-number package by adding two new utility functions and improving error handling. The main changes include adding `isValidPhoneNumber` for boolean phone number validation and `formatPhoneNumberOrThrow` which provides an exception-throwing alternative to the existing `formatPhoneNumber` function. The implementation introduces a new `WithPhoneNumber` interface to establish consistent typing across phone number utilities, and improves error handling by preserving original error messages from the underlying libphonenumber-js library using the `toError` utility.The changes align with the broader goal of centralizing phone number operations in this utility package, which will enable removing the direct libphonenumber-js dependency from cbh-core. The new functions follow established patterns in the codebase: isValidPhoneNumber uses the same default country (US) and trimming behavior as the existing formatter, while formatPhoneNumberOrThrow wraps the ServiceResult-based formatter to provide exception-based error handling for consumers who prefer that approach.
The test suite has been refactored to use a DRY approach with shared constants and parameterized testing, reducing code duplication while maintaining comprehensive coverage for both success and error cases across different phone number formats.
Important Files Changed
Changed Files
| Filename | Score | Overview |
|---|---|---|
| packages/phone-number/src/lib/formatPhoneNumber.spec.ts | 1/5 | Tests expect specific error messages (NOT_A_NUMBER, TOO_LONG) but implementation returns generic libphonenumber-js errors |
| packages/phone-number/src/lib/formatPhoneNumber.ts | 4/5 | Added formatPhoneNumberOrThrow function and improved error handling with toError utility |
| packages/phone-number/src/lib/isValidPhoneNumber.ts | 5/5 | New boolean validation function using libphonenumber-js with US default country |
| packages/phone-number/src/lib/isValidPhoneNumber.spec.ts | 5/5 | Comprehensive test coverage for phone number validation with parameterized testing |
| packages/phone-number/src/lib/types.ts | 5/5 | Added WithPhoneNumber interface for consistent typing across phone utilities |
| packages/phone-number/src/index.ts | 5/5 | Added export for new isValidPhoneNumber module to public API |
Confidence score: 2/5
- This PR has a critical test failure that will prevent it from passing CI due to mismatched error message expectations
- Score lowered due to test assertions expecting specific error messages that the implementation doesn't return
- Pay close attention to packages/phone-number/src/lib/formatPhoneNumber.spec.ts which needs error message alignment
Sequence Diagram
sequenceDiagram
participant User
participant App
participant isValidPhoneNumber
participant formatPhoneNumber
participant formatPhoneNumberOrThrow
participant libphonenumber-js
User->>App: "Input phone number"
App->>isValidPhoneNumber: "isValidPhoneNumber({ phoneNumber })"
isValidPhoneNumber->>libphonenumber-js: "isValidPhoneNumberF(phoneNumber.trim(), { defaultCountry: 'US' })"
libphonenumber-js-->>isValidPhoneNumber: "boolean result"
isValidPhoneNumber-->>App: "true/false"
alt Phone number is valid
App->>formatPhoneNumber: "formatPhoneNumber({ phoneNumber, format })"
formatPhoneNumber->>libphonenumber-js: "parsePhoneNumberWithError(phoneNumber.trim(), { defaultCountry: 'US' })"
libphonenumber-js-->>formatPhoneNumber: "ParsedPhoneNumber object"
formatPhoneNumber->>formatPhoneNumber: "parsedPhoneNumber.format(E.164 or NATIONAL)"
formatPhoneNumber-->>App: "ServiceResult<string> with formatted number"
else Phone number invalid
formatPhoneNumber->>libphonenumber-js: "parsePhoneNumberWithError(phoneNumber.trim(), { defaultCountry: 'US' })"
libphonenumber-js-->>formatPhoneNumber: "throws error"
formatPhoneNumber-->>App: "ServiceResult<failure> with error details"
end
alt User prefers throwing errors
App->>formatPhoneNumberOrThrow: "formatPhoneNumberOrThrow({ phoneNumber, format })"
formatPhoneNumberOrThrow->>formatPhoneNumber: "formatPhoneNumber({ phoneNumber, format })"
formatPhoneNumber-->>formatPhoneNumberOrThrow: "ServiceResult"
alt ServiceResult is failure
formatPhoneNumberOrThrow->>formatPhoneNumberOrThrow: "throw new Error(result.error.issues[0].message)"
formatPhoneNumberOrThrow-->>App: "throws Error"
else ServiceResult is success
formatPhoneNumberOrThrow-->>App: "formatted phone number string"
end
end
6 files reviewed, 1 comment
|
View your CI Pipeline Execution ↗ for commit 99db828
☁️ Nx Cloud last updated this comment at |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
packages/phone-number/src/index.ts(1 hunks)packages/phone-number/src/lib/formatPhoneNumber.spec.ts(1 hunks)packages/phone-number/src/lib/formatPhoneNumber.ts(1 hunks)packages/phone-number/src/lib/isValidPhoneNumber.spec.ts(1 hunks)packages/phone-number/src/lib/isValidPhoneNumber.ts(1 hunks)packages/phone-number/src/lib/types.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{js,ts,py,go,rb}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,py,go,rb}: Use environment variables for secrets and configuration, never hardcode secrets in code
Always validate and sanitize user input on the server side
Use parameterized queries or ORM query builders to prevent SQL injection
Return appropriate HTTP status codes for all API responses
Log errors with sufficient context but avoid logging sensitive information
Use consistent error handling patterns (e.g., try/catch, error middleware)
Do not expose internal implementation details in API responses
Use dependency injection for services and repositories
Files:
packages/phone-number/src/index.tspackages/phone-number/src/lib/formatPhoneNumber.spec.tspackages/phone-number/src/lib/isValidPhoneNumber.spec.tspackages/phone-number/src/lib/formatPhoneNumber.tspackages/phone-number/src/lib/types.tspackages/phone-number/src/lib/isValidPhoneNumber.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts}: Use async/await for asynchronous operations in Node.js backends
Prefer named exports over default exports in backend modules
Files:
packages/phone-number/src/index.tspackages/phone-number/src/lib/formatPhoneNumber.spec.tspackages/phone-number/src/lib/isValidPhoneNumber.spec.tspackages/phone-number/src/lib/formatPhoneNumber.tspackages/phone-number/src/lib/types.tspackages/phone-number/src/lib/isValidPhoneNumber.ts
packages/**/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
packages/**/src/**/*.ts: Use the function keyword instead of const for function declarations in TypeScript source files
Favor @clipboard-health/util-ts Either (ServiceResult) over try/catch for expected errors
Structure files with sections in order: constants, types, exported functions, non-exported functions
Use descriptive variable names with auxiliary verbs (e.g., isLoading, hasError)
Files:
packages/phone-number/src/index.tspackages/phone-number/src/lib/formatPhoneNumber.spec.tspackages/phone-number/src/lib/isValidPhoneNumber.spec.tspackages/phone-number/src/lib/formatPhoneNumber.tspackages/phone-number/src/lib/types.tspackages/phone-number/src/lib/isValidPhoneNumber.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Avoid the any type in TypeScript
Prefer interfaces over type aliases for object shapes
Avoid enums; use const maps (as const objects) instead
Files:
packages/phone-number/src/index.tspackages/phone-number/src/lib/formatPhoneNumber.spec.tspackages/phone-number/src/lib/isValidPhoneNumber.spec.tspackages/phone-number/src/lib/formatPhoneNumber.tspackages/phone-number/src/lib/types.tspackages/phone-number/src/lib/isValidPhoneNumber.ts
⚙️ CodeRabbit configuration file
**/*.ts:
- URLs and request and response bodies must adhere to jsonapi.org's JSON:API specification.
- The links, version, ext, profile, and meta members are not required.
- Use only the data resource linkage for relationships.
- The type member should be singular, not plural.
Files:
packages/phone-number/src/index.tspackages/phone-number/src/lib/formatPhoneNumber.spec.tspackages/phone-number/src/lib/isValidPhoneNumber.spec.tspackages/phone-number/src/lib/formatPhoneNumber.tspackages/phone-number/src/lib/types.tspackages/phone-number/src/lib/isValidPhoneNumber.ts
packages/**
📄 CodeRabbit inference engine (CLAUDE.md)
Use camelCase for files and directories
Files:
packages/phone-number/src/index.tspackages/phone-number/src/lib/formatPhoneNumber.spec.tspackages/phone-number/src/lib/isValidPhoneNumber.spec.tspackages/phone-number/src/lib/formatPhoneNumber.tspackages/phone-number/src/lib/types.tspackages/phone-number/src/lib/isValidPhoneNumber.ts
packages/*/src/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Each package must expose a main export file at src/index.ts
Files:
packages/phone-number/src/index.ts
**/*.{test,spec}.{js,ts,py,go,rb}
📄 CodeRabbit inference engine (.cursorrules)
Write unit tests for all business logic
Files:
packages/phone-number/src/lib/formatPhoneNumber.spec.tspackages/phone-number/src/lib/isValidPhoneNumber.spec.ts
packages/**/src/**/*.@(spec|test).ts
📄 CodeRabbit inference engine (CLAUDE.md)
packages/**/src/**/*.@(spec|test).ts: Follow Arrange-Act-Assert convention in tests with blank lines between sections
Use naming like mockX, input, expected, actual in tests
Files:
packages/phone-number/src/lib/formatPhoneNumber.spec.tspackages/phone-number/src/lib/isValidPhoneNumber.spec.ts
🧬 Code graph analysis (4)
packages/phone-number/src/lib/formatPhoneNumber.spec.ts (3)
packages/phone-number/src/lib/formatPhoneNumber.ts (2)
formatPhoneNumber(32-43)formatPhoneNumberOrThrow(68-77)packages/testing-core/src/lib/expectToBeSuccess.ts (1)
expectToBeSuccess(15-20)packages/testing-core/src/lib/expectToBeFailure.ts (1)
expectToBeFailure(16-21)
packages/phone-number/src/lib/isValidPhoneNumber.spec.ts (1)
packages/phone-number/src/lib/isValidPhoneNumber.ts (1)
isValidPhoneNumber(7-13)
packages/phone-number/src/lib/formatPhoneNumber.ts (3)
packages/phone-number/src/lib/types.ts (1)
WithPhoneNumber(1-3)packages/util-ts/src/lib/functional/serviceResult.ts (4)
ServiceResult(26-26)success(31-38)failure(43-51)isFailure(56-60)packages/util-ts/src/lib/errors/toError.ts (1)
toError(23-57)
packages/phone-number/src/lib/isValidPhoneNumber.ts (1)
packages/phone-number/src/lib/types.ts (1)
WithPhoneNumber(1-3)
⏰ 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). (2)
- GitHub Check: pull-request
- GitHub Check: cubic · AI code reviewer
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary
Sometimes you want to throw, so I added
formatPhoneNumberOrThrowto make that easier. Also addedisValidPhoneNumberso we can removelibphonenumber-jsfromcbh-core.Summary by cubic
Added isValidPhoneNumber and formatPhoneNumberOrThrow to make phone number validation and formatting simpler, and help remove libphonenumber-js usage in cbh-core. formatPhoneNumber now surfaces specific parse errors (e.g., NOT_A_NUMBER, TOO_LONG) for clearer failures.