feat: booker atom automatic phone country code detection #24224
Conversation
|
@Anshumancanrock is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughReplaces platform-specific rendering in PhoneInput.tsx with a single unified BasePhoneInput that always renders one PhoneInput. Removes useIsPlatform branching and BasePhoneInputWeb/platform variants. Derives defaultCountry via a new useDefaultCountry hook and updates the signature to Omit<PhoneInputProps, "defaultCountry">. Merges inputStyle and flagButtonStyle into the underlying component’s style/ button props and preserves value normalization and onChange (including prefix handling). Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml 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)
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/features/components/phone-input/PhoneInput.tsx (2)
71-120: Consider eliminating duplication between BasePhoneInputPlatform and BasePhoneInputWeb.Both components share identical implementations (PhoneInput configuration, styling, value normalization, onChange handling). The conditional in
BasePhoneInputcould be removed entirely since both paths now useuseDefaultCountry()the same way.Consider this refactor to eliminate ~100 lines of duplication:
function BasePhoneInput({ name, className = "", onChange, value, - defaultCountry = "us", + inputStyle, + flagButtonStyle, ...rest }: PhoneInputProps) { - const isPlatform = useIsPlatform(); + const defaultCountry = useDefaultCountry(); // This is to trigger validation on prefill value changes useEffect(() => { if (!value) return; const sanitized = value .trim() .replace(/[^\d+]/g, "") .replace(/^\+?/, "+"); if (sanitized === "+" || sanitized === "") return; if (value !== sanitized) { onChange(sanitized); } // eslint-disable-next-line react-hooks/exhaustive-deps }, []); - if (!isPlatform) { - return ( - <BasePhoneInputWeb name={name} className={className} onChange={onChange} value={value} {...rest} /> - ); - } - - return ( - <BasePhoneInputPlatform - name={name} - className={className} - onChange={onChange} - value={value} - {...rest} - /> - ); -} - -function BasePhoneInputPlatform({ - name, - className = "", - onChange, - value, - inputStyle, - flagButtonStyle, - ...rest -}: Omit<PhoneInputProps, "defaultCountry">) { - const defaultCountry = useDefaultCountry(); - return ( <PhoneInput {...rest} value={value ? value.trim().replace(/^\+?/, "+") : undefined} country={value ? undefined : defaultCountry} enableSearch disableSearchIcon inputProps={{ name, required: rest.required, placeholder: rest.placeholder, }} onChange={(val: string) => { onChange(`+${val}`); }} containerClass={classNames( "hover:border-emphasis dark:focus:border-emphasis border-default !bg-default rounded-md border focus-within:outline-none focus-within:ring-2 focus-within:ring-brand-default disabled:cursor-not-allowed", className )} inputClass="text-sm focus:ring-0 !bg-default text-default placeholder:text-muted" buttonClass="text-emphasis !bg-default hover:!bg-emphasis" buttonStyle={{ ...flagButtonStyle }} searchClass="!text-default !bg-default hover:!bg-emphasis" dropdownClass="!text-default !bg-default" inputStyle={{ width: "inherit", border: 0, ...inputStyle }} searchStyle={{ display: "flex", flexDirection: "row", alignItems: "center", padding: "6px 12px", gap: "8px", width: "296px", height: "28px", marginLeft: "-4px", }} dropdownStyle={{ width: "max-content" }} /> ); } - -function BasePhoneInputWeb({ - name, - className = "", - onChange, - value, - inputStyle, - flagButtonStyle, - ...rest -}: Omit<PhoneInputProps, "defaultCountry">) { - const defaultCountry = useDefaultCountry(); - - return ( - <PhoneInput - {...rest} - value={value ? value.trim().replace(/^\+?/, "+") : undefined} - country={value ? undefined : defaultCountry} - enableSearch - disableSearchIcon - inputProps={{ - name, - required: rest.required, - placeholder: rest.placeholder, - }} - onChange={(val: string) => { - onChange(`+${val}`); - }} - containerClass={classNames( - "hover:border-emphasis dark:focus:border-emphasis border-default !bg-default rounded-md border focus-within:outline-none focus-within:ring-2 focus-within:ring-brand-default disabled:cursor-not-allowed", - className - )} - inputClass="text-sm focus:ring-0 !bg-default text-default placeholder:text-muted" - buttonClass="text-emphasis !bg-default hover:!bg-emphasis" - buttonStyle={{ ...flagButtonStyle }} - searchClass="!text-default !bg-default hover:!bg-emphasis" - dropdownClass="!text-default !bg-default" - inputStyle={{ width: "inherit", border: 0, ...inputStyle }} - searchStyle={{ - display: "flex", - flexDirection: "row", - alignItems: "center", - padding: "6px 12px", - gap: "8px", - width: "296px", - height: "28px", - marginLeft: "-4px", - }} - dropdownStyle={{ width: "max-content" }} - /> - ); -}
198-198: Prefer named export over default export.As per coding guidelines, named exports provide better tree-shaking, easier refactoring, and clearer imports. While default exports are acceptable for pages and layouts, this shared component should use a named export.
Apply this diff:
-export default BasePhoneInput; +export { BasePhoneInput };Then update imports across the codebase from:
import BasePhoneInput from "...";to:
import { BasePhoneInput } from "...";As per coding guidelines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
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 (1)
packages/features/components/phone-input/PhoneInput.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/components/phone-input/PhoneInput.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/components/phone-input/PhoneInput.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/components/phone-input/PhoneInput.tsx
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Type check / check-types
- GitHub Check: Tests / Unit
- GitHub Check: Linters / lint
🔇 Additional comments (2)
packages/features/components/phone-input/PhoneInput.tsx (2)
60-69: LGTM!The conditional rendering cleanly separates platform and web paths, with appropriate prop forwarding to the new
BasePhoneInputPlatformcomponent.
173-196: LGTM!The country detection logic correctly falls back from API response → navigator.language → "us", meeting the PR's objective to unify country detection between web and platform.
dhairyashiil
left a comment
There was a problem hiding this comment.
The two functions BasePhoneInputWeb and BasePhoneInputPlatform do the same thing. Please remove the web/platform split and keep only one function called BasePhoneInput
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); | ||
|
|
||
| if (!isPlatform) { |
There was a problem hiding this comment.
Combine them into a single version that works for both web and platform, and remove the isPlatform check.
There was a problem hiding this comment.
@dhairyashiil Thanks for the feedback! I’ve made the changes as requested, removed the extra isPlatform check and the duplicate BasePhoneInputWeb / BasePhoneInputPlatform functions. Now everything’s handled in a single BasePhoneInput component that uses useDefaultCountry for both web and platform modes.
24f13ee to
d95f760
Compare
|
@dhairyashiil I’ve made the changes as requested. Please review now. Thanks ! |
E2E results are ready! |
|
Hi @supalarry , Could you please review this PR? Thank you ! |
1 similar comment
|
Hi @supalarry , Could you please review this PR? Thank you ! |
dhairyashiil
left a comment
There was a problem hiding this comment.
@Anshumancanrock , please resolve merge conflicts, I will ask team member for the review on monday, and sorry for delay 🙏🏼
There was a problem hiding this comment.
2 issues found across 8 files
Prompt for AI agents (all 2 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/features/components/phone-input/PhoneInputPlatformWrapper.tsx">
<violation number="1" location="packages/features/components/phone-input/PhoneInputPlatformWrapper.tsx:22">
Locale fallback grabs the second locale segment, so locales with a script subtag (e.g., zh-Hant-TW) resolve to an invalid country code. Use the last hyphen-delimited segment (or another region-aware parser) when deriving the fallback country.</violation>
</file>
<file name="packages/features/components/phone-input/PhoneInputWebWrapper.tsx">
<violation number="1" location="packages/features/components/phone-input/PhoneInputWebWrapper.tsx:28">
Validate the browser-derived fallback country before passing it to the phone input; `navigator.language` can contain non-country tokens (e.g., scripts or UN regions), so currently an invalid `defaultCountry` is set and the phone field loses its default flag.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
packages/features/components/phone-input/PhoneInputPlatformWrapper.tsx
Outdated
Show resolved
Hide resolved
packages/features/components/phone-input/PhoneInputWebWrapper.tsx
Outdated
Show resolved
Hide resolved
|
Hey guys, Thanks again! |
|
Hi everyone, This change would unblock real usage of the phone field on platform and aligns it with the web behaviour. Thanks a lot for taking another look. |
Devin AI is resolving merge conflictsThis PR has merge conflicts with the Devin will:
If you prefer to resolve conflicts manually, you can close the Devin session and handle it yourself. |
Devin AI is resolving merge conflictsThis PR has merge conflicts with the Devin will:
If you prefer to resolve conflicts manually, you can close the Devin session and handle it yourself. |
|
hi @TwanBox and @TeleCalendar i'll try to finish the remaning work in this PR and get it past the line |
Devin AI is resolving merge conflictsThis PR has merge conflicts with the Devin will:
If you prefer to resolve conflicts manually, you can close the Devin session and handle it yourself. |
Co-Authored-By: unknown <>
implemented feedback
| @@ -1,8 +0,0 @@ | |||
| import dynamic from "next/dynamic"; | |||
There was a problem hiding this comment.
this is causing lazy loading for both atoms and web wrapper, for some reason api calls were not being made correctly with vite, hence shifted lazy loading inside web wrapper. this way it only lazy loads for web and not for atoms.
| import { PhoneInputPlatformWrapper } from "./PhoneInputPlatformWrapper"; | ||
|
|
||
| /** These are like 40kb that not every user needs */ | ||
| const PhoneInputWebWrapper: ComponentType<Omit<PhoneInputProps, "defaultCountry">> = dynamic(() => |
There was a problem hiding this comment.
we only wanna lazy loading for web wrapper
Ryukemeister
left a comment
There was a problem hiding this comment.
looks good on atoms side, tested with web app as well and looking good. @Udit-takkar needs an extra pair of eyes from you here
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/features/components/phone-input/PhoneInputPlatformWrapper.tsx">
<violation number="1" location="packages/features/components/phone-input/PhoneInputPlatformWrapper.tsx:23">
P2: Browser locale fallback always fails country support check due to lowercased code passed to case-sensitive isSupportedCountry, forcing default to US.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| setDefaultCountry(countryCode.toLowerCase()); | ||
| } else { | ||
| const browserCountry = navigator.language.split("-").pop()?.toLowerCase(); | ||
| if (browserCountry && isSupportedCountry(browserCountry)) { |
There was a problem hiding this comment.
P2: Browser locale fallback always fails country support check due to lowercased code passed to case-sensitive isSupportedCountry, forcing default to US.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/components/phone-input/PhoneInputPlatformWrapper.tsx, line 23:
<comment>Browser locale fallback always fails country support check due to lowercased code passed to case-sensitive isSupportedCountry, forcing default to US.</comment>
<file context>
@@ -21,7 +20,11 @@ export function PhoneInputPlatformWrapper(props: Omit<PhoneInputProps, "defaultC
} else {
const browserCountry = navigator.language.split("-").pop()?.toLowerCase();
- setDefaultCountry(browserCountry && isSupportedCountry(browserCountry) ? browserCountry : "us");
+ if (browserCountry && isSupportedCountry(browserCountry)) {
+ setDefaultCountry(browserCountry);
+ } else {
</file context>
What does this PR do?
This PR fixes the phone field in Booker Atom (Platform mode) to auto-detect the user's country, matching the behavior of the main web app. Previously, the phone field always defaulted to US (+1) regardless of the user's actual location.
Before:
After:
BasePhoneInputPlatformcomponent that uses the sameuseDefaultCountry()hook asBasePhoneInputWebVisual Demo (For contributors especially)
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist