fix: improve country code detection for phone input#28175
fix: improve country code detection for phone input#28175yuvrajangadsingh wants to merge 9 commits into
Conversation
github-actions
Bot
added
foundation
High priority
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Graphite Automations"Send notification to Community team when High Priority PR opened" took an action on this PR • (02/25/26)2 teammates were notified to this PR based on Keith Williams's automation. |
yuvrajangadsingh
force-pushed
the
fix/country-code-detection
branch
3 times, most recently
from
3d492f5 to
92c8f0d
Compare
|
hey, this is a small fix for country code detection in the phone input. no reviewer assigned yet, would appreciate someone taking a look. pretty straightforward change. |
yuvrajangadsingh
force-pushed
the
fix/country-code-detection
branch
7 times, most recently
from
c263743 to
1455c76
Compare
yuvrajangadsingh
force-pushed
the
fix/country-code-detection
branch
2 times, most recently
from
8f26af3 to
cd07bb1
Compare
|
rebased onto latest main. this fixes a high priority bug with country code detection for phone inputs. would appreciate a review if anyone on the team has bandwidth. |
yuvrajangadsingh
force-pushed
the
fix/country-code-detection
branch
3 times, most recently
from
032576f to
61faff1
Compare
|
This PR has been marked as stale due to inactivity. If you're still working on it or need any help, please let us know or update the PR to keep it active. |
|
still active, waiting on review. |
|
bump, anything needed from my side on this one? |
|
rebased on latest main via merge commit 4f25ea0 (no conflicts). should clear the 'required' check now. @keithwillcode would appreciate a look. high priority bug, small fix scoped to PhoneInput country-code detection across 3 files. |
|
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:
📝 WalkthroughWalkthroughThree files were updated. The geolocation API route and public viewer handler now prefer the 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/components/phone-input/PhoneInput.tsx (1)
257-269:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
getCountryFromTimezone()is unreachable — this is the consumer-side manifestation of the handler bug.When the server returns
""(no CF/Vercel headers),!data?.countryCodeistrueat line 257, so the effect returns early and lines 264-269 are never executed.If you prefer to fix this here instead of (or in addition to) the handler, restructure the guard to distinguish "query still in-flight" from "query resolved with no usable country":
🐛 Proposed consumer-side fix
const data = query.data; - if (!data?.countryCode) { - return; - } - - if (isSupportedCountry(data?.countryCode)) { + if (!query.isSuccess && !query.isError) { + return; + } + + if (data?.countryCode && isSupportedCountry(data.countryCode)) { setDefaultCountry(data.countryCode.toLowerCase() as CountryCode); } else { const tzCountry = getCountryFromTimezone();Also add
query.isSuccess,query.isErrorto the dependency array:- [query.data, defaultPhoneCountryFromStore] + [query.data, query.isSuccess, query.isError, defaultPhoneCountryFromStore]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/components/phone-input/PhoneInput.tsx` around lines 257 - 269, The effect returns early when data?.countryCode is falsy, making getCountryFromTimezone() unreachable; change the guard to only return early when the query is still loading or unsettled (use the query's status flags) so you can handle the case where data resolved but countryCode is empty — if data?.countryCode exists and isSupportedCountry(data.countryCode) call setDefaultCountry(data.countryCode.toLowerCase()), else if the query has succeeded check getCountryFromTimezone() and isSupportedCountry(tzCountry.toUpperCase()) to setDefaultCountry(tzCountry), otherwise fall back to "us"; also include query.isSuccess and query.isError in the effect dependency array and reference functions/vars like getCountryFromTimezone, isSupportedCountry, setDefaultCountry, and data so the logic runs correctly.
🧹 Nitpick comments (1)
apps/web/components/phone-input/PhoneInput.tsx (1)
161-229: ⚡ Quick winPrefer
Record<string, CountryCode>overRecord<string, string>to eliminate theas CountryCodecast.Typing the map more precisely removes the unsafe cast at line 266 and lets the compiler catch any invalid country code values added to the map in the future.
♻️ Proposed refactor
-const TIMEZONE_COUNTRY_MAP: Record<string, string> = { +const TIMEZONE_COUNTRY_MAP: Record<string, CountryCode> = {- setDefaultCountry(tzCountry as CountryCode); + setDefaultCountry(tzCountry);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/components/phone-input/PhoneInput.tsx` around lines 161 - 229, TIMEZONE_COUNTRY_MAP is typed as Record<string, string>; change its type to Record<string, CountryCode> and remove the unsafe "as CountryCode" cast where the map is read; ensure you import or reference the CountryCode type used elsewhere (e.g., from libphonenumber-js or your project types) so the map values are validated by the compiler and any invalid country codes are flagged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/trpc/server/routers/publicViewer/countryCode.handler.ts`:
- Around line 10-11: The handler currently falls back to an empty string which
is falsy and prevents the timezone-detection branch in PhoneInput.tsx from
running; change the fallback in the countryCode extraction to the same sentinel
used elsewhere (e.g. "Unknown") so the value is truthy and allows
isSupportedCountry/getCountryFromTimezone to behave as intended—update the line
that defines countryCode in countryCode.handler (the const countryCode: string |
string[] = ...) to use "Unknown" instead of "" as the final fallback.
---
Outside diff comments:
In `@apps/web/components/phone-input/PhoneInput.tsx`:
- Around line 257-269: The effect returns early when data?.countryCode is falsy,
making getCountryFromTimezone() unreachable; change the guard to only return
early when the query is still loading or unsettled (use the query's status
flags) so you can handle the case where data resolved but countryCode is empty —
if data?.countryCode exists and isSupportedCountry(data.countryCode) call
setDefaultCountry(data.countryCode.toLowerCase()), else if the query has
succeeded check getCountryFromTimezone() and
isSupportedCountry(tzCountry.toUpperCase()) to setDefaultCountry(tzCountry),
otherwise fall back to "us"; also include query.isSuccess and query.isError in
the effect dependency array and reference functions/vars like
getCountryFromTimezone, isSupportedCountry, setDefaultCountry, and data so the
logic runs correctly.
---
Nitpick comments:
In `@apps/web/components/phone-input/PhoneInput.tsx`:
- Around line 161-229: TIMEZONE_COUNTRY_MAP is typed as Record<string, string>;
change its type to Record<string, CountryCode> and remove the unsafe "as
CountryCode" cast where the map is read; ensure you import or reference the
CountryCode type used elsewhere (e.g., from libphonenumber-js or your project
types) so the map values are validated by the compiler and any invalid country
codes are flagged.
🪄 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: 60f18eb2-98b6-4758-bd93-52b0dd33d916
📒 Files selected for processing (3)
apps/web/app/api/geolocation/route.tsapps/web/components/phone-input/PhoneInput.tsxpackages/trpc/server/routers/publicViewer/countryCode.handler.ts
| const countryCode: string | string[] = | ||
| req?.headers?.["cf-ipcountry"] || req?.headers?.["x-vercel-ip-country"] || ""; |
There was a problem hiding this comment.
Empty-string fallback makes the timezone detection unreachable in PhoneInput.tsx.
The countryCodeHandler returns "" when neither header is present. In PhoneInput.tsx (context snippet 1, line 257), the consumer guard is:
if (!data?.countryCode) {
return; // empty string is falsy → returns here
}
// …
} else {
const tzCountry = getCountryFromTimezone(); // ← never reached!"" is true, so the effect returns early and the timezone-detection branch (getCountryFromTimezone()) is never reached for self-hosted deployments without either header. That is precisely the use-case this PR is intended to fix.
geolocation/route.ts still returns "Unknown" as its no-header fallback; using the same sentinel here restores the intended flow (truthy → isSupportedCountry("Unknown") is false → else branch executes):
🐛 Proposed fix
const countryCode: string | string[] =
- req?.headers?.["cf-ipcountry"] || req?.headers?.["x-vercel-ip-country"] || "";
+ req?.headers?.["cf-ipcountry"] || req?.headers?.["x-vercel-ip-country"] || "Unknown";Note: Cloudflare uses
"XX"for clients without country-code data and"T1"for Tor-network clients. Both are already handled correctly (truthy,isSupportedCountryreturnsfalse→ triggers the timezone fallback), so no extra handling is needed for those values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/trpc/server/routers/publicViewer/countryCode.handler.ts` around
lines 10 - 11, The handler currently falls back to an empty string which is
falsy and prevents the timezone-detection branch in PhoneInput.tsx from running;
change the fallback in the countryCode extraction to the same sentinel used
elsewhere (e.g. "Unknown") so the value is truthy and allows
isSupportedCountry/getCountryFromTimezone to behave as intended—update the line
that defines countryCode in countryCode.handler (the const countryCode: string |
string[] = ...) to use "Unknown" instead of "" as the final fallback.
Address coderabbit review on PR calcom#28175: 1. The early return on `!data?.countryCode` made the timezone fallback unreachable when the server returned "" (no CF/Vercel headers — the exact case this PR was meant to handle). Restructured the guard to distinguish loading from resolved-with-no-country. 2. Tightened TIMEZONE_COUNTRY_MAP type from Record<string, string> to Record<string, CountryCode>, removing the unsafe `as CountryCode` cast.
|
thanks @coderabbitai, both valid. fixed in f11d9be:
|
|
Tip For best results, initiate chat on the files or code changes.
The logic flow now correctly handles all three cases (CF header → Vercel header → timezone → If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/components/phone-input/PhoneInput.tsx (1)
161-229: ⚡ Quick win
TIMEZONE_COUNTRY_MAPhas significant coverage gaps that produce incorrect "us" defaultsFor users whose IANA timezone isn't in the map — and who are behind neither a Cloudflare nor Vercel edge — the code falls through to
setDefaultCountry("us")(line 275). This is correct for missing US timezone variants likeAmerica/Phoenix, but it's wrong for the many non-US timezones absent from the map. For a global scheduling product, the most impactful gaps are:
Timezone Country Europe/Kyiv/Europe/Kiev"ua"(Ukraine)Europe/Budapest"hu"Europe/Sofia"bg"Europe/Vilnius"lt"Europe/Riga"lv"Europe/Tallinn"ee"Europe/Zagreb"hr"Europe/Belgrade"rs"Europe/Bratislava"sk"Europe/Minsk"by"America/Caracas"ve"America/Montevideo"uy"America/La_Paz"bo"Asia/Kabul"af"Asia/Almaty"kz"Asia/Tashkent"uz"Asia/Yangon"mm"Asia/Muscat"om"Asia/Kuwait"kw"Asia/Baku"az"Asia/Tbilisi"ge"Asia/Yerevan"am"Africa/Tunis"tn"Africa/Algiers"dz"Africa/Accra"gh"Africa/Addis_Ababa"et"Africa/Dar_es_Salaam"tz"Africa/Harare"zw"Africa/Kampala"ug"Pacific/Fiji"fj"✏️ Proposed additions to `TIMEZONE_COUNTRY_MAP`
"Asia/Tehran": "ir", + // Europe — Eastern / Northern / Southern + "Europe/Kyiv": "ua", + "Europe/Kiev": "ua", + "Europe/Minsk": "by", + "Europe/Budapest": "hu", + "Europe/Sofia": "bg", + "Europe/Bucharest": "ro", // already present + "Europe/Vilnius": "lt", + "Europe/Riga": "lv", + "Europe/Tallinn": "ee", + "Europe/Zagreb": "hr", + "Europe/Belgrade": "rs", + "Europe/Bratislava": "sk", + "Europe/Ljubljana": "si", + "Europe/Sarajevo": "ba", + "Europe/Skopje": "mk", + "Europe/Podgorica": "me", + "Europe/Tirane": "al", + "Europe/Nicosia": "cy", + "Europe/Valletta": "mt", + "Europe/Riga": "lv", + // Asia — Central / South-East / Middle East + "Asia/Kabul": "af", + "Asia/Almaty": "kz", + "Asia/Tashkent": "uz", + "Asia/Bishkek": "kg", + "Asia/Dushanbe": "tj", + "Asia/Ashgabat": "tm", + "Asia/Baku": "az", + "Asia/Tbilisi": "ge", + "Asia/Yerevan": "am", + "Asia/Yangon": "mm", + "Asia/Rangoon": "mm", + "Asia/Muscat": "om", + "Asia/Kuwait": "kw", + "Asia/Qatar": "qa", + "Asia/Bahrain": "bh", + // Americas + "America/Caracas": "ve", + "America/Montevideo": "uy", + "America/La_Paz": "bo", + "America/Guayaquil": "ec", + "America/Asuncion": "py", + "America/Panama": "pa", + "America/Tegucigalpa": "hn", + "America/Managua": "ni", + "America/Costa_Rica": "cr", + "America/Guatemala": "gt", + "America/El_Salvador": "sv", + "America/Halifax": "ca", + "America/Winnipeg": "ca", + "America/Edmonton": "ca", + "America/St_Johns": "ca", + // Africa + "Africa/Tunis": "tn", + "Africa/Algiers": "dz", + "Africa/Accra": "gh", + "Africa/Addis_Ababa": "et", + "Africa/Dar_es_Salaam": "tz", + "Africa/Harare": "zw", + "Africa/Lusaka": "zm", + "Africa/Kampala": "ug", + "Africa/Kigali": "rw", + "Africa/Abidjan": "ci", + "Africa/Dakar": "sn", + "Africa/Maputo": "mz", + "Africa/Khartoum": "sd", + "Africa/Tripoli": "ly", + // Pacific / Oceania + "Pacific/Fiji": "fj", + "Pacific/Guam": "gu", + "Pacific/Port_Moresby": "pg",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/components/phone-input/PhoneInput.tsx` around lines 161 - 229, TIMEZONE_COUNTRY_MAP currently omits many IANA zones so code falls back to setDefaultCountry("us") incorrectly; add the missing mappings (e.g. "Europe/Kyiv"|"Europe/Kiev" -> "ua", "Europe/Budapest" -> "hu", "Europe/Sofia" -> "bg", "Europe/Vilnius" -> "lt", "Europe/Riga" -> "lv", "Europe/Tallinn" -> "ee", "Europe/Zagreb" -> "hr", "Europe/Belgrade" -> "rs", "Europe/Bratislava" -> "sk", "Europe/Minsk" -> "by", "America/Caracas" -> "ve", "America/Montevideo" -> "uy", "America/La_Paz" -> "bo", "Asia/Kabul" -> "af", "Asia/Almaty" -> "kz", "Asia/Tashkent" -> "uz", "Asia/Yangon" -> "mm", "Asia/Muscat" -> "om", "Asia/Kuwait" -> "kw", "Asia/Baku" -> "az", "Asia/Tbilisi" -> "ge", "Asia/Yerevan" -> "am", "Africa/Tunis" -> "tn", "Africa/Algiers" -> "dz", "Africa/Accra" -> "gh", "Africa/Addis_Ababa" -> "et", "Africa/Dar_es_Salaam" -> "tz", "Africa/Harare" -> "zw", "Africa/Kampala" -> "ug", "Pacific/Fiji" -> "fj") into TIMEZONE_COUNTRY_MAP and then change the fallback around the setDefaultCountry call to only default to "us" for timezones that clearly map to US regions (e.g. America/* variants) otherwise use the mapped value if present or skip setting a hardcoded country so callers can fall back to other signals (Cloudflare/Vercel or geolocation); reference TIMEZONE_COUNTRY_MAP and the setDefaultCountry(...) invocation when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/components/phone-input/PhoneInput.tsx`:
- Around line 161-229: TIMEZONE_COUNTRY_MAP currently omits many IANA zones so
code falls back to setDefaultCountry("us") incorrectly; add the missing mappings
(e.g. "Europe/Kyiv"|"Europe/Kiev" -> "ua", "Europe/Budapest" -> "hu",
"Europe/Sofia" -> "bg", "Europe/Vilnius" -> "lt", "Europe/Riga" -> "lv",
"Europe/Tallinn" -> "ee", "Europe/Zagreb" -> "hr", "Europe/Belgrade" -> "rs",
"Europe/Bratislava" -> "sk", "Europe/Minsk" -> "by", "America/Caracas" -> "ve",
"America/Montevideo" -> "uy", "America/La_Paz" -> "bo", "Asia/Kabul" -> "af",
"Asia/Almaty" -> "kz", "Asia/Tashkent" -> "uz", "Asia/Yangon" -> "mm",
"Asia/Muscat" -> "om", "Asia/Kuwait" -> "kw", "Asia/Baku" -> "az",
"Asia/Tbilisi" -> "ge", "Asia/Yerevan" -> "am", "Africa/Tunis" -> "tn",
"Africa/Algiers" -> "dz", "Africa/Accra" -> "gh", "Africa/Addis_Ababa" -> "et",
"Africa/Dar_es_Salaam" -> "tz", "Africa/Harare" -> "zw", "Africa/Kampala" ->
"ug", "Pacific/Fiji" -> "fj") into TIMEZONE_COUNTRY_MAP and then change the
fallback around the setDefaultCountry call to only default to "us" for timezones
that clearly map to US regions (e.g. America/* variants) otherwise use the
mapped value if present or skip setting a hardcoded country so callers can fall
back to other signals (Cloudflare/Vercel or geolocation); reference
TIMEZONE_COUNTRY_MAP and the setDefaultCountry(...) invocation when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b462f000-e9e3-4147-8c9b-b1133c76711e
📒 Files selected for processing (1)
apps/web/components/phone-input/PhoneInput.tsx
Address coderabbit follow-up nitpick on PR calcom#28175: The map covers high-traffic timezones but isn't exhaustive. Hardcoding 'us' for any unmapped zone produces wrong defaults for users in countries the map doesn't cover (Ukraine, parts of Africa, Central Asia, etc.). Now only force 'us' when the resolved IANA zone starts with 'America/'. For other unmapped zones, keep the initial default state rather than overriding with a wrong guess. Refactored TZ resolution into getResolvedTimezone() to avoid duplicating the Intl call and try/catch.
|
thanks for the follow-up @coderabbitai. fixed the fallback logic in f6df6cd — now only forces 'us' when the resolved IANA zone starts with 'America/'. for other unmapped zones the initial default (or whatever came from the store) sticks rather than overriding with a wrong guess. extracted getResolvedTimezone() so we don't duplicate the Intl call. skipping the 50-entry map expansion in this pr to keep the diff focused. the current ~70 entries cover high-traffic zones, and with the fallback fix, unmapped non-american users now get the initial default instead of being forced to 'us'. happy to follow up with a coverage pr backed by traffic data later. |
|
Tip For best results, initiate chat on the files or code changes.
One thing worth keeping in mind for the follow-up PR: the Extracting Overall the incremental approach (fix the critical fallback logic now, expand coverage with data later) is exactly the right call for a focused, reviewable diff. The follow-up coverage PR backed by traffic data sounds solid. ✏️ Learnings added
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/components/phone-input/PhoneInput.tsx (1)
273-286: 💤 Low valueConsider computing the timezone once to avoid duplicate
Intlcalls.
getResolvedTimezone()is called insidegetCountryFromTimezone()(line 240) and again at line 283. While the performance impact is negligible, you could refactor to compute the timezone once at the start of the fallback path.♻️ Suggested refactor
- const tzCountry = getCountryFromTimezone(); - if (tzCountry && isSupportedCountry(tzCountry.toUpperCase())) { - setDefaultCountry(tzCountry); - return; - } - - // The map covers high-traffic timezones, not every IANA zone. - // Only force "us" when the resolved zone is clearly North American. - // Otherwise keep the initial default ("us" or whatever came from the - // store) rather than overriding with a wrong guess. - const tz = getResolvedTimezone(); - if (tz?.startsWith("America/")) { + const tz = getResolvedTimezone(); + const tzCountry = tz ? TIMEZONE_COUNTRY_MAP[tz] : undefined; + if (tzCountry && isSupportedCountry(tzCountry.toUpperCase())) { + setDefaultCountry(tzCountry); + return; + } + + // The map covers high-traffic timezones, not every IANA zone. + // Only force "us" when the resolved zone is clearly North American. + // Otherwise keep the initial default ("us" or whatever came from the + // store) rather than overriding with a wrong guess. + if (tz?.startsWith("America/")) { setDefaultCountry("us"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/components/phone-input/PhoneInput.tsx` around lines 273 - 286, getResolvedTimezone() is invoked twice via getCountryFromTimezone() and again directly, so compute the resolved timezone once and reuse it: call getResolvedTimezone() once at the start of the fallback path, store it (e.g., resolvedTz), pass that value into getCountryFromTimezone(resolvedTz) or modify getCountryFromTimezone to accept an optional timezone parameter, then use the same resolvedTz for the tz?.startsWith("America/") check before calling setDefaultCountry("us"); this avoids duplicate Intl calls while preserving the existing logic that sets tzCountry and the "America/" fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/components/phone-input/PhoneInput.tsx`:
- Around line 273-286: getResolvedTimezone() is invoked twice via
getCountryFromTimezone() and again directly, so compute the resolved timezone
once and reuse it: call getResolvedTimezone() once at the start of the fallback
path, store it (e.g., resolvedTz), pass that value into
getCountryFromTimezone(resolvedTz) or modify getCountryFromTimezone to accept an
optional timezone parameter, then use the same resolvedTz for the
tz?.startsWith("America/") check before calling setDefaultCountry("us"); this
avoids duplicate Intl calls while preserving the existing logic that sets
tzCountry and the "America/" fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3954a46-1c28-407d-ac26-b969fae3990b
📒 Files selected for processing (1)
apps/web/components/phone-input/PhoneInput.tsx
Address coderabbit nitpick on PR calcom#28175 — was calling getResolvedTimezone() twice (once via getCountryFromTimezone, once for the America/* check). Resolve once in the fallback path, look up the map inline. Drops the now-unused getCountryFromTimezone() helper.
|
applied in 4d1b2ac — resolve the tz once at the top of the fallback path and inline the map lookup. dropped getCountryFromTimezone() since nothing uses it anymore. |
|
Hi, Severity: action required | Category: correctness How to fix: Whitelist US timezones only Agent prompt to fix - you can give this to your LLM of choice:
We noticed a couple of other issues in this PR as well - happy to share if helpful. Qodo code review - free for open-source. |
|
Hi, Severity: remediation recommended | Category: security How to fix: Add Vary header Agent prompt to fix - you can give this to your LLM of choice:
We noticed a couple of other issues in this PR as well - happy to share if helpful. Found by Qodo code review |
…rowser Address Qodo bot review on PR calcom#28175: 1. PhoneInput: removed the `tz.startsWith("America/")` -> "us" fallback. The IANA `America/*` namespace covers Latin America and the Caribbean, so this was mis-detecting unmapped LATAM users as US. The block was also redundant — the React initial state is already "us", so deleting it changes nothing for unmapped zones in the no-store case. 2. /api/geolocation: changed Cache-Control from `public, max-age=3600, s-maxage=3600, stale-while-revalidate=86400` to `private, max-age=3600`. The previous header allowed shared CDN caching of a response that varies by request headers (no Vary key), so one user's country could be served to other users. The only consumer (GTM.tsx) already does 24h localStorage + react-query caching, so per-user browser caching is sufficient.
|
thanks @Qodo-Free-For-OSS, both valid. fixed in 8cb2901:
yes please, go ahead and post them. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/app/api/geolocation/route.ts`:
- Line 7: The country value derived from headersList.get("cf-ipcountry") /
"x-vercel-ip-country" may be lower/mixed-case and should be normalized before
returning; update the logic in the route handler (the line that sets the country
variable in apps/web/app/api/geolocation/route.ts) to trim and convert any
header value to uppercase (e.g., use .trim().toUpperCase()) and fall back to
"Unknown" only if there's no header or the trimmed value is empty, so downstream
checks like apps/web/components/GTM.tsx comparing to "US" work reliably.
In `@apps/web/components/phone-input/PhoneInput.tsx`:
- Around line 262-265: Normalize the header-derived country code before checking
support: read query.data.countryCode into a local variable, coerce to a string
and call .toUpperCase() for the isSupportedCountry check (e.g., let cc =
String(query.data.countryCode || '').toUpperCase()), then if
isSupportedCountry(cc) call setDefaultCountry(cc.toLowerCase() as CountryCode);
update references to query.data.countryCode, isSupportedCountry, and
setDefaultCountry accordingly and guard empty/null values.
🪄 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: 2e1154c9-50e3-4cc6-8dfb-71d337477bab
📒 Files selected for processing (2)
apps/web/app/api/geolocation/route.tsapps/web/components/phone-input/PhoneInput.tsx
Address coderabbit round 4 on PR calcom#28175. Headers should already be uppercase ISO 3166-1 alpha-2 codes, but normalize defensively at every boundary so consumers can rely on stable casing for strict equality checks like `data.country === "US"` (apps/web/components/GTM.tsx). Three sites: - apps/web/app/api/geolocation/route.ts (REST) - packages/trpc/server/routers/publicViewer/countryCode.handler.ts (tRPC) - apps/web/components/phone-input/PhoneInput.tsx (consumer)
|
thanks @coderabbitai. fixed at all 3 boundaries in f2218ab:
defense in depth at every boundary so casing stays consistent end to end. |
|
Tip For best results, initiate chat on the files or code changes.
A couple of quick observations on the three boundaries:
The whole chain is consistent and clearly reasoned. ✏️ Learnings added
|
|
Hi, Severity: remediation recommended | Category: correctness How to fix: Use || or trim-check Agent prompt to fix - you can give this to your LLM of choice:
Found by Qodo. Free code review for open-source maintainers. |
Per qodo bot review: `??` only falls back on null/undefined, so an empty cf-ipcountry header would prevent fallback to x-vercel-ip-country and return "Unknown" even when Vercel had a valid country code. In practice cloudflare returns `XX` for unknown country and `T1` for Tor (not empty string), so the runtime impact is speculative. But the defensive fix is correct and matches the trpc handler at packages/trpc/server/routers/publicViewer/countryCode.handler.ts which already uses ||.
|
thanks @Qodo-Free-For-OSS, real bug. fixed in 74a1fa3c76 — reverted |
2 participants
What does this PR do?
the phone number field's country auto-detection only checks
x-vercel-ip-countryheader. self-hosted deployments behind Cloudflare get no country detection, falling back tonavigator.languagewhich gives browser locale (language preference), not physical location.this adds
cf-ipcountryheader support (matching the pattern already inapp/layout.tsx) and replaces the unreliablenavigator.languagefallback with timezone-based country detection usingIntl.DateTimeFormat().resolvedOptions().timeZone.Visual Demo (For contributors especially)
N/A — no UI changes, only the pre-selected country code in the phone input changes based on better detection logic. the phone input component itself is unchanged.
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
cf-ipcountryheader manually via dev proxy)for the timezone fallback:
cf-ipcountryandx-vercel-ip-countryheaders (self-hosted without reverse proxy geolocation)Asia/KolkataChecklist