feat: add country code filtering for SMS workflows#23021
feat: add country code filtering for SMS workflows#23021joeauyeung wants to merge 50 commits intomainfrom
Conversation
- Add allowedCountryCodes field to WorkflowStep model in database schema - Update workflow step configuration UI to allow country code selection for SMS actions - Implement SMS validation to check phone numbers against allowed country codes - Modify PhoneInput component to support country code restrictions and prevent deletion when only one code allowed - Update booking form integration to pass country code restrictions from workflows - Add translation strings for new UI elements - Update all TypeScript types to support new allowedCountryCodes field This allows users to restrict SMS workflows to specific country codes and prevents bookers from deleting country code prefixes when only one is allowed (e.g., +1 for US/Canada). Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
WalkthroughThis change removes the isOrgTeamEvent flag across API, services, and booking field builders, updating method signatures and call sites accordingly. Booking field generation now relies solely on eventType data. It introduces allowedCountryCodes support end-to-end: form builder fields, phone input components (web and mobile), and booking response schema (auto-prepending calling codes when a single country is allowed). A new countryCodeUtils module provides country options and labels. Public group events now pass workflows: [] into booking field generation. Misc updates include translations for country code restrictions, memoization in WorkflowStepContainer, a schema tweak (senderName nullable; agentId removed), and a security rel attribute. Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 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 |
keithwillcode
added
core
|
This PR is being marked as stale due to inactivity. |
…uration - Replace hardcoded 20 countries with dynamic generation of all 245 supported countries - Use libphonenumber-js.isSupportedCountry() and getCountryCallingCode() for consistency - Use Intl.DisplayNames for proper country name localization - Ensures workflow country selection matches PhoneInput component options exactly Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
- Update WorkflowStepContainer to show country code UI only for SMS_ATTENDEE - Add allowedCountryCodes field to FormValues type and form schema - Ensures country code filtering is specific to attendee SMS workflows Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
github-actions
bot
added
❗️ migrations
…filtering Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
…/git-manager.devin.ai/proxy/github.com/calcom/cal.com into devin/sms-country-code-filtering-1754928743
Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
![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 (2)
packages/features/components/phone-input/PhoneInput.tsx (2)
80-117: Enforce allowed countries on typed input (cannot bypass by pasting another E.164).Users can still enter numbers from disallowed countries; add client-side validation with libphonenumber-js.
return ( <PhoneInput {...rest} value={value ? value.trim().replace(/^\+?/, "+") : undefined} enableSearch disableSearchIcon + isValid={(val: string) => { + if (!allowedCountryCodes || allowedCountryCodes.length === 0) return true; + try { + const parsed = parsePhoneNumberFromString(`+${val}`); + if (!parsed?.country) return true; // allow partial input + const allowed = new Set(allowedCountryCodes.map((c) => c.toUpperCase())); + return allowed.has(parsed.country); + } catch { + return true; + } + }} country={effectiveDefaultCountry} onlyCountries={onlyCountries} disableCountryCode={disableCountryCode}Add import:
-import { isSupportedCountry } from "libphonenumber-js"; +import { isSupportedCountry, parsePhoneNumberFromString } from "libphonenumber-js";
142-179: Add isValid here too to prevent bypass on web.Keep behavior consistent across both render paths.
return ( <PhoneInput {...rest} value={value ? value.trim().replace(/^\+?/, "+") : undefined} country={value ? undefined : effectiveDefaultCountry} enableSearch disableSearchIcon + isValid={(val: string) => { + if (!allowedCountryCodes || allowedCountryCodes.length === 0) return true; + try { + const parsed = parsePhoneNumberFromString(`+${val}`); + if (!parsed?.country) return true; + const allowed = new Set(allowedCountryCodes.map((c) => c.toUpperCase())); + return allowed.has(parsed.country); + } catch { + return true; + } + }} onlyCountries={onlyCountries} disableCountryCode={disableCountryCode}
🧹 Nitpick comments (3)
packages/features/components/phone-input/PhoneInput.tsx (3)
23-24: Clarify prop semantics (empty list = unrestricted; ISO codes).Document expected format and behavior to avoid misuse downstream.
export type PhoneInputProps = { @@ - allowedCountryCodes?: string[]; - preventCountryCodeDeletion?: boolean; + /** ISO 3166-1 alpha‑2 codes (e.g., "US", "AT"). undefined or [] = no restriction. */ + allowedCountryCodes?: string[]; + /** When true and all allowed countries share the same dial code, lock the prefix (non‑editable). */ + preventCountryCodeDeletion?: boolean;
136-139: Prefer viewer’s default when allowed; otherwise fallback to first allowed.Improves UX: keep user’s locale when it’s within the restriction.
- const effectiveDefaultCountry = allowedCountryCodes?.length - ? allowedCountryCodes[0].toLowerCase() - : defaultCountry; + const effectiveDefaultCountry = + allowedCountryCodes && allowedCountryCodes.length > 0 + ? (allowedCountryCodes.includes(defaultCountry.toUpperCase()) + ? defaultCountry + : allowedCountryCodes[0].toLowerCase()) + : defaultCountry;
210-210: Prefer named export over default export.Aligns with tree‑shaking and refactorability guidelines.
-export default BasePhoneInput; +export { BasePhoneInput };
📜 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 (2)
packages/features/components/phone-input/PhoneInput.tsx(5 hunks)packages/features/form-builder/FormBuilder.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/features/form-builder/FormBuilder.tsx
🧰 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
🧠 Learnings (2)
📓 Common learnings
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
📚 Learning: 2025-08-08T10:26:13.362Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts:212-220
Timestamp: 2025-08-08T10:26:13.362Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts should include the phone number in client-facing HttpError messages (e.g., in updatePhoneNumber/getPhoneNumber catch blocks). Do not suggest redacting the phone number from these errors unless requirements change (per maintainer: Udit-takkar).
Applied to 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). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (4)
packages/features/components/phone-input/PhoneInput.tsx (4)
145-145: Good: don’t override country when a value exists.Avoids flicker and preserves the parsed country from the existing value.
198-202: LGTM: sensible default-country fallback.Uses server hint when supported; otherwise falls back to navigator region or “us”.
72-79: Bug: onlyCountries=[] disables all countries; treat [] as “no restriction”. Also lock prefix when all allowed share dial code.Current logic breaks the picker when allowedCountryCodes is empty and doesn’t handle shared dial codes (+1).
Apply this diff:
- const onlyCountries = allowedCountryCodes?.map((code) => code.toLowerCase()); - const disableCountryCode = preventCountryCodeDeletion && allowedCountryCodes?.length === 1; + const onlyCountries = + allowedCountryCodes && allowedCountryCodes.length > 0 + ? allowedCountryCodes.map((code) => code.toLowerCase()) + : undefined; + const disableCountryCode = + !!preventCountryCodeDeletion && + !!allowedCountryCodes && + allowedCountryCodes.length > 0 && + (() => { + try { + const first = getCountryCallingCode(allowedCountryCodes[0] as any); + return allowedCountryCodes.every( + (c) => getCountryCallingCode(c as any) === first + ); + } catch { + return false; + } + })();And add the import:
-import { isSupportedCountry } from "libphonenumber-js"; +import { isSupportedCountry, getCountryCallingCode } from "libphonenumber-js";Also mirror the same change in the web path (Lines 133-140).
126-131: Same fixes needed in web path (onlyCountries empty → undefined; shared dial code lock).Mirror the bug fix and locking logic from the platform path here.
- const onlyCountries = allowedCountryCodes?.map((code) => code.toLowerCase()); - const disableCountryCode = preventCountryCodeDeletion && allowedCountryCodes?.length === 1; + const onlyCountries = + allowedCountryCodes && allowedCountryCodes.length > 0 + ? allowedCountryCodes.map((code) => code.toLowerCase()) + : undefined; + const disableCountryCode = + !!preventCountryCodeDeletion && + !!allowedCountryCodes && + allowedCountryCodes.length > 0 && + (() => { + try { + const first = getCountryCallingCode(allowedCountryCodes[0] as any); + return allowedCountryCodes.every( + (c) => getCountryCallingCode(c as any) === first + ); + } catch { + return false; + } + })();Also applies to: 133-140
| country={effectiveDefaultCountry} | ||
| onlyCountries={onlyCountries} | ||
| disableCountryCode={disableCountryCode} | ||
| inputProps={{ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Confirm react-phone-input-2 prop name: disableCountryCode vs countryCodeEditable.
If the library expects countryCodeEditable={false} (not disableCountryCode), the current prop will be ignored.
Also applies to: 148-149
🌐 Web query:
Does react-phone-input-2 support a `disableCountryCode` prop, or should `countryCodeEditable={false}` be used to lock the dial code prefix?
💡 Result:
Short answer: Use countryCodeEditable={false} to lock the dial-code prefix. The component also exposes disableCountryCode (which tries to keep the dial code out of the input value), but it does not prevent the dial code/auto-guessing behavior reliably — users have reported unexpected behavior and issues. See the prop docs and related GitHub issues. [1][2]
Sources:
- react-phone-input-2 props documentation (shows disableCountryCode and countryCodeEditable). [1]
- GitHub issues discussing disableCountryCode bugs and unexpected country-guess behavior. [2]
Use countryCodeEditable={false} instead of disableCountryCode – disableCountryCode only omits the dial code from the value and doesn’t reliably lock the prefix. Update the props in packages/features/components/phone-input/PhoneInput.tsx (lines 86-89, 148-149).
🤖 Prompt for AI Agents
In packages/features/components/phone-input/PhoneInput.tsx around lines 86-89
and 148-149, the component currently passes disableCountryCode which only omits
the dial code and doesn’t lock the prefix; replace disableCountryCode with
countryCodeEditable={false} in the props passed to the phone input so the
country dial code is rendered but not editable, and ensure any related logic or
prop typings are updated accordingly to accept countryCodeEditable.
- Fix phone validation failure when allowedCountryCodes has single country - Auto-prepend country code in onChange when disableCountryCode is true - Maintain UI behavior (hide country code) while fixing form submission - Apply fix to both BasePhoneInput and BasePhoneInputWeb functions - Remove allowedCountryCodes references from workflow update handler - Use proper CountryCode type from libphonenumber-js - Fix ESLint warnings with proper disable comments Fixes booking submission failures when phone fields have single allowed country code by ensuring E.164 format validation passes while keeping the clean UI experience for users. Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
…ictions" This reverts commit a9e0938.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/features/components/phone-input/PhoneInput.tsx (5)
23-24: Clarify prop semantics inlineDocument that empty/undefined means “no restriction,” and the expected format.
- allowedCountryCodes?: string[]; - preventCountryCodeDeletion?: boolean; + /** ISO-3166-1 alpha-2 codes (e.g., "US"). Empty/undefined = no restriction */ + allowedCountryCodes?: string[]; + /** When true, lock the dial-code prefix if it can be deterministically derived */ + preventCountryCodeDeletion?: boolean;
75-79: Derive default from normalized listPrefer onlyCountries[0] (already lowercased/validated) to raw allowedCountryCodes.
- const effectiveDefaultCountry = allowedCountryCodes?.length - ? allowedCountryCodes[0].toLowerCase() - : defaultCountry; + const effectiveDefaultCountry = onlyCountries?.[0] ?? defaultCountry;
136-139: Derive default from normalized list (web path)Use the normalized list for consistency.
- const effectiveDefaultCountry = allowedCountryCodes?.length - ? allowedCountryCodes[0].toLowerCase() - : defaultCountry; + const effectiveDefaultCountry = onlyCountries?.[0] ?? defaultCountry;
198-202: Harden navigator fallback (unsupported or UK alias)Normalize navigator code and ensure it’s supported; fall back to “us”.
- } else { - setDefaultCountry(navigator.language.split("-")[1]?.toLowerCase() || "us"); - } + } else { + const nav = navigator.language.split("-")[1]?.toLowerCase(); + const normalized = nav === "uk" ? "gb" : nav; + setDefaultCountry( + normalized && isSupportedCountry(normalized.toUpperCase()) ? normalized : "us" + ); + }
210-210: Prefer named exports for componentsSwitching from default to named export improves tree-shaking and refactors. This is a broad change; consider as a follow-up.
If you proceed, update import sites:
-export default BasePhoneInput; +export { BasePhoneInput as PhoneInput };Then replace default imports with:
import { PhoneInput } from "packages/features/components/phone-input/PhoneInput";
📜 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 (2)
packages/features/components/phone-input/PhoneInput.tsx(5 hunks)packages/trpc/server/routers/viewer/workflows/update.handler.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/trpc/server/routers/viewer/workflows/update.handler.ts
🧰 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
🧠 Learnings (2)
📓 Common learnings
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Learnt from: supalarry
PR: calcom/cal.com#23217
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts:93-94
Timestamp: 2025-08-21T13:44:06.805Z
Learning: In apps/api/v2/src/ee/event-types/event-types_2024_06_14/event-types.repository.ts, repository functions that use explicit Prisma select clauses (like getEventTypeWithSeats) are used for specific purposes and don't need to include all EventType fields like bookingRequiresAuthentication. These methods don't feed into the general OutputEventTypesService_2024_06_14 flow.
📚 Learning: 2025-08-08T10:26:13.362Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts:212-220
Timestamp: 2025-08-08T10:26:13.362Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts should include the phone number in client-facing HttpError messages (e.g., in updatePhoneNumber/getPhoneNumber catch blocks). Do not suggest redacting the phone number from these errors unless requirements change (per maintainer: Udit-takkar).
Applied to files:
packages/features/components/phone-input/PhoneInput.tsx
🔇 Additional comments (6)
packages/features/components/phone-input/PhoneInput.tsx (6)
60-68: Props plumb-through LGTM
Forwarding the new props into BasePhoneInputWeb looks correct.
72-79: Optional: lock dial code when all allowed share the same calling codeIf multiple allowed countries share a dial code (e.g., US/CA = +1), you can still lock the prefix. Requires getCountryCallingCode.
+import { getCountryCallingCode } from "libphonenumber-js"; @@ - const disableCountryCode = - !!preventCountryCodeDeletion && !!allowedCountryCodes && allowedCountryCodes.length === 1; + const disableCountryCode = + !!preventCountryCodeDeletion && + !!allowedCountryCodes && + allowedCountryCodes.length > 0 && + (() => { + try { + const first = getCountryCallingCode(allowedCountryCodes[0] as any); + return allowedCountryCodes.every( + (c) => getCountryCallingCode(c as any) === first + ); + } catch { + return false; + } + })();Also applies to: 126-139
72-74: Bug: [] should mean “no restriction”, not “disable all countries”Passing onlyCountries=[] blocks selection entirely. Use undefined when the list is empty.
- const onlyCountries = allowedCountryCodes?.map((code) => code.toLowerCase()); - const disableCountryCode = preventCountryCodeDeletion && allowedCountryCodes?.length === 1; + const onlyCountries = + allowedCountryCodes && allowedCountryCodes.length > 0 + ? allowedCountryCodes.map((code) => code.toLowerCase()) + : undefined; + const disableCountryCode = + !!preventCountryCodeDeletion && !!allowedCountryCodes && allowedCountryCodes.length === 1;
86-89: Lock dial code with countryCodeEditable, and don’t override once value existsUse countryCodeEditable to prevent entering disallowed dial codes; avoid forcing country when value is present. This addresses the UX bug reported by Carina.
- country={effectiveDefaultCountry} - onlyCountries={onlyCountries} - disableCountryCode={disableCountryCode} + country={value ? undefined : effectiveDefaultCountry} + onlyCountries={onlyCountries} + countryCodeEditable={disableCountryCode ? false : undefined}
126-135: Repeat fix: [] → undefined and robust booleanMirror the non-web path: undefined means unrestricted; tighten boolean checks.
- const onlyCountries = allowedCountryCodes?.map((code) => code.toLowerCase()); - const disableCountryCode = preventCountryCodeDeletion && allowedCountryCodes?.length === 1; + const onlyCountries = + allowedCountryCodes && allowedCountryCodes.length > 0 + ? allowedCountryCodes.map((code) => code.toLowerCase()) + : undefined; + const disableCountryCode = + !!preventCountryCodeDeletion && !!allowedCountryCodes && allowedCountryCodes.length === 1;
145-149: Lock dial code with countryCodeEditable (web path)Use the correct prop to actually prevent dial-code edits; keeps user from entering disallowed countries.
- country={value ? undefined : effectiveDefaultCountry} + country={value ? undefined : effectiveDefaultCountry} enableSearch disableSearchIcon - onlyCountries={onlyCountries} - disableCountryCode={disableCountryCode} + onlyCountries={onlyCountries} + countryCodeEditable={disableCountryCode ? false : undefined}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
packages/features/bookings/lib/getBookingResponsesSchema.ts (5)
1-1: Import parsePhoneNumberFromString to enable country enforcementYou’ll need this for server-side enforcement of allowedCountryCodes in validation.
-import { isValidPhoneNumber, getCountryCallingCode, type CountryCode } from "libphonenumber-js"; +import { isValidPhoneNumber, getCountryCallingCode, parsePhoneNumberFromString, type CountryCode } from "libphonenumber-js";
303-314: Enforce allowedCountryCodes at validation time (not only via UI)Add a server-side check so numbers must belong to the allowed set (covers multi-country restrictions and UI bypass).
if (bookingField.type === "phone") { // Determine if the phone field needs validation const needsValidation = isRequired || (value && value.trim() !== ""); // Validate phone number if the field is not hidden and requires validation if (!bookingField.hidden && needsValidation) { if (!(await phoneSchema.safeParseAsync(value)).success) { ctx.addIssue({ code: z.ZodIssueCode.custom, message: m("invalid_number") }); } + // Enforce allowed country codes if configured + if ( + Array.isArray(bookingField.allowedCountryCodes) && + bookingField.allowedCountryCodes.length > 0 + ) { + const parsed = parsePhoneNumberFromString(value); + const allowed = bookingField.allowedCountryCodes.map((c) => c.toUpperCase()); + const country = parsed?.country?.toUpperCase(); + if (country && !allowed.includes(country)) { + ctx.addIssue({ code: z.ZodIssueCode.custom, message: m("invalid_number") }); + } + } } continue; }
337-344: Also enforce allowedCountryCodes for radioInput phone option valuesParity with the plain phone field so radio-driven phone inputs can’t bypass the restriction.
if (typeOfOptionInput === "phone") { if (!(await phoneSchema.safeParseAsync(optionValue)).success) { ctx.addIssue({ code: z.ZodIssueCode.custom, message: m("invalid_number") }); } + // Enforce allowed country codes if configured on the parent field + if ( + Array.isArray(bookingField.allowedCountryCodes) && + bookingField.allowedCountryCodes.length > 0 + ) { + const parsed = parsePhoneNumberFromString(optionValue); + const allowed = bookingField.allowedCountryCodes.map((c) => c.toUpperCase()); + const country = parsed?.country?.toUpperCase(); + if (country && !allowed.includes(country)) { + ctx.addIssue({ code: z.ZodIssueCode.custom, message: m("invalid_number") }); + } + } }
111-124: Reduce duplication: extract a small helper for “single allowed country” auto-prependKeeps logic consistent between radioInput and plain phone fields.
Example helper (place near ensureValidPhoneNumber):
function prependDialCodeIfSingleAllowedCountry( raw: string, allowed?: string[] ): string { if (!raw || !allowed?.length || allowed.length !== 1) return raw; const cc = allowed[0]?.toUpperCase(); try { const dial = getCountryCallingCode(cc as CountryCode); return raw.startsWith("+") ? raw : `+${dial}${raw}`; } catch { return raw; } }Then replace both blocks with:
- radioInput: phoneValue = prependDialCodeIfSingleAllowedCountry(phoneValue, field.allowedCountryCodes);
- phone: phoneValue = prependDialCodeIfSingleAllowedCountry(value, field.allowedCountryCodes);
Also applies to: 127-140
33-36: Prefer named exports over defaultAligns with internal guidelines for clearer imports and better tree-shaking. Can be a follow-up, not blocking this PR.
📜 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 (3)
packages/features/bookings/lib/getBookingResponsesSchema.ts(2 hunks)packages/features/components/phone-input/PhoneInput.tsx(5 hunks)packages/features/form-builder/Components.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/features/components/phone-input/PhoneInput.tsx
- packages/features/form-builder/Components.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/bookings/lib/getBookingResponsesSchema.ts
**/*.{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/bookings/lib/getBookingResponsesSchema.ts
**/*.{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/bookings/lib/getBookingResponsesSchema.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Learnt from: supalarry
PR: calcom/cal.com#23217
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts:93-94
Timestamp: 2025-08-21T13:44:06.805Z
Learning: In apps/api/v2/src/ee/event-types/event-types_2024_06_14/event-types.repository.ts, repository functions that use explicit Prisma select clauses (like getEventTypeWithSeats) are used for specific purposes and don't need to include all EventType fields like bookingRequiresAuthentication. These methods don't feed into the general OutputEventTypesService_2024_06_14 flow.
📚 Learning: 2025-08-08T10:26:13.362Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts:212-220
Timestamp: 2025-08-08T10:26:13.362Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts should include the phone number in client-facing HttpError messages (e.g., in updatePhoneNumber/getPhoneNumber catch blocks). Do not suggest redacting the phone number from these errors unless requirements change (per maintainer: Udit-takkar).
Applied to files:
packages/features/bookings/lib/getBookingResponsesSchema.ts
📚 Learning: 2025-08-27T13:32:46.887Z
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Applied to files:
packages/features/bookings/lib/getBookingResponsesSchema.ts
🔇 Additional comments (1)
packages/features/bookings/lib/getBookingResponsesSchema.ts (1)
105-107: LGTM on safe JSON parseGraceful handling is good; avoids crashing preprocess on malformed radioInput payloads.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/features/components/phone-input/PhoneInput.tsx (5)
23-26: Type allowedCountryCodes precisely (CountryCode[]) and document semanticsUse the library’s CountryCode type to prevent invalid inputs and make intent clear (empty array = no restriction).
-import type { CSSProperties } from "react"; +import type { CSSProperties } from "react"; +import type { CountryCode } from "libphonenumber-js/core"; @@ - allowedCountryCodes?: string[]; + allowedCountryCodes?: CountryCode[];Note: You’ll need the type-only import above.
69-76: Normalize/validate allowedCountryCodes and precompute dial-code lockingFilter out unsupported codes and compute a lockDialCode flag when all allowed countries share the same calling code (e.g., US/CA). This improves robustness and UX.
- const singleCountry = allowedCountryCodes?.length === 1; - const onlyCountries = allowedCountryCodes?.map((code) => code.toLowerCase()); - - // If country codes are restricted, use the first one as the default - const effectiveDefaultCountry = allowedCountryCodes?.length - ? allowedCountryCodes[0].toLowerCase() - : defaultCountry; + const normalizedAllowed = + Array.isArray(allowedCountryCodes) && allowedCountryCodes.length > 0 + ? allowedCountryCodes + .map((c) => c?.toUpperCase()) + .filter((c): c is string => !!c && isSupportedCountry(c as any)) + : undefined; + + const onlyCountries = normalizedAllowed?.map((c) => c.toLowerCase()); + + const effectiveDefaultCountry = + onlyCountries && onlyCountries.length > 0 ? onlyCountries[0] : defaultCountry; + + // Lock dial-code if all allowed countries share the same calling code + const lockDialCode = + !!normalizedAllowed && + normalizedAllowed.length > 0 && + (() => { + try { + const first = getCountryCallingCode(normalizedAllowed[0] as any); + return normalizedAllowed.every( + (c) => getCountryCallingCode(c as any) === first + ); + } catch { + return false; + } + })();Add import:
import { isSupportedCountry, getCountryCallingCode } from "libphonenumber-js";
129-136: Mirror normalization and dial-code locking on web pathKeep behavior consistent between platform and web.
- const onlyCountries = allowedCountryCodes?.map((code) => code.toLowerCase()); - const singleCountry = allowedCountryCodes?.length === 1; - - // If country codes are restricted, use the first one as the default - const effectiveDefaultCountry = allowedCountryCodes?.length - ? allowedCountryCodes[0].toLowerCase() - : defaultCountry; + const normalizedAllowed = + Array.isArray(allowedCountryCodes) && allowedCountryCodes.length > 0 + ? allowedCountryCodes + .map((c) => c?.toUpperCase()) + .filter((c): c is string => !!c && isSupportedCountry(c as any)) + : undefined; + const onlyCountries = normalizedAllowed?.map((c) => c.toLowerCase()); + const effectiveDefaultCountry = + onlyCountries && onlyCountries.length > 0 ? onlyCountries[0] : defaultCountry; + const lockDialCode = + !!normalizedAllowed && + normalizedAllowed.length > 0 && + (() => { + try { + const first = getCountryCallingCode(normalizedAllowed[0] as any); + return normalizedAllowed.every( + (c) => getCountryCallingCode(c as any) === first + ); + } catch { + return false; + } + })();Add import if not added already:
import { isSupportedCountry, getCountryCallingCode } from "libphonenumber-js";
194-198: Harden default-country fallbackValidate the navigator-derived region before using it to avoid unsupported values like “001”.
- if (isSupportedCountry(data?.countryCode)) { - setDefaultCountry(data.countryCode.toLowerCase()); - } else { - setDefaultCountry(navigator.language.split("-")[1]?.toLowerCase() || "us"); - } + if (isSupportedCountry((data.countryCode as any))) { + setDefaultCountry(data.countryCode.toLowerCase()); + } else { + const navRegion = navigator.language.split("-")[1]?.toUpperCase(); + setDefaultCountry( + navRegion && isSupportedCountry(navRegion as any) + ? navRegion.toLowerCase() + : "us" + ); + }
39-54: Sanitization effect runs only once; confirm intended behaviorComment says “trigger validation on prefill value changes” but the effect ignores value changes. If the prop can change after mount, consider adding value to deps.
📜 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 (2)
packages/features/bookings/lib/getBookingFields.ts(2 hunks)packages/features/components/phone-input/PhoneInput.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/features/bookings/lib/getBookingFields.ts
🧰 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
🧠 Learnings (2)
📓 Common learnings
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Learnt from: supalarry
PR: calcom/cal.com#23217
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts:93-94
Timestamp: 2025-08-21T13:44:06.805Z
Learning: In apps/api/v2/src/ee/event-types/event-types_2024_06_14/event-types.repository.ts, repository functions that use explicit Prisma select clauses (like getEventTypeWithSeats) are used for specific purposes and don't need to include all EventType fields like bookingRequiresAuthentication. These methods don't feed into the general OutputEventTypesService_2024_06_14 flow.
📚 Learning: 2025-08-08T10:26:13.362Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts:212-220
Timestamp: 2025-08-08T10:26:13.362Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts should include the phone number in client-facing HttpError messages (e.g., in updatePhoneNumber/getPhoneNumber catch blocks). Do not suggest redacting the phone number from these errors unless requirements change (per maintainer: Udit-takkar).
Applied to 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). (8)
- GitHub Check: Production builds / Build API v1
- GitHub Check: Production builds / Build Web App
- GitHub Check: Production builds / Build Docs
- GitHub Check: Tests / Unit
- GitHub Check: Production builds / Build Atoms
- GitHub Check: Production builds / Build API v2
- GitHub Check: Type check / check-types
- GitHub Check: Linters / lint
🔇 Additional comments (4)
packages/features/components/phone-input/PhoneInput.tsx (4)
58-66: LGTM: Prop plumbing to web variantForwarding allowedCountryCodes into BasePhoneInputWeb looks correct.
118-127: LGTM: Web variant signatureallowedCountryCodes threading into BasePhoneInputWeb is correct.
83-86: Replace disableCountryCode with countryCodeEditable and enforce restriction via isValiddisableCountryCode doesn’t reliably lock the prefix. Use countryCodeEditable and add isValid to prevent disallowed countries at input time.
- country={effectiveDefaultCountry} - onlyCountries={onlyCountries && onlyCountries.length > 0 ? onlyCountries : undefined} - disableCountryCode={singleCountry} + country={effectiveDefaultCountry} + onlyCountries={onlyCountries && onlyCountries.length > 0 ? onlyCountries : undefined} + countryCodeEditable={!lockDialCode} + disableDropdown={!!onlyCountries && onlyCountries.length === 1} + isValid={(_, country) => + !onlyCountries || + onlyCountries.includes(country?.countryCode?.toLowerCase() || "") + }
141-146: Lock prefix and validate allowed countries in web pathSame as platform path; this also addresses QA’s “I can still add any phone number” concern.
- country={value ? undefined : effectiveDefaultCountry} + country={value ? undefined : effectiveDefaultCountry} @@ - onlyCountries={onlyCountries && onlyCountries.length > 0 ? onlyCountries : undefined} - disableCountryCode={singleCountry} + onlyCountries={onlyCountries && onlyCountries.length > 0 ? onlyCountries : undefined} + countryCodeEditable={!lockDialCode} + disableDropdown={!!onlyCountries && onlyCountries.length === 1} + isValid={(_, country) => + !onlyCountries || + onlyCountries.includes(country?.countryCode?.toLowerCase() || "") + }
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 (2)
packages/features/ee/workflows/components/WorkflowStepContainer.tsx (2)
437-449: Wrong toast severity on failed phone verificationWhen isVerified is false, you’re still using a “success” toast. Use error for wrong code.
- showToast(isVerified ? t("verified_successfully") : t("wrong_code"), "success"); + showToast(isVerified ? t("verified_successfully") : t("wrong_code"), isVerified ? "success" : "error");
475-487: Bug: email verification flag set to true even on wrong code; wrong toast severitysetEmailVerified(true) should respect isVerified. Also use “error” toast when code is wrong.
- onSuccess: (isVerified) => { - showToast(isVerified ? t("verified_successfully") : t("wrong_code"), "success"); - setEmailVerified(true); + onSuccess: (isVerified) => { + showToast(isVerified ? t("verified_successfully") : t("wrong_code"), isVerified ? "success" : "error"); + setEmailVerified(isVerified); if ( step && form?.formState?.errors?.steps && form.formState.errors.steps[step.stepNumber - 1]?.sendTo && isVerified ) { form.clearErrors(`steps.${step.stepNumber - 1}.sendTo`); }
🧹 Nitpick comments (5)
packages/features/ee/workflows/components/WorkflowStepContainer.tsx (5)
97-107: Minor TS cleanup: avoid non-null assertionYou already guard triggerMap[trigger]; the non-null assertion is unnecessary. Slightly clearer:
- return t(triggerMap[trigger]!); + const key = triggerMap[trigger]; + if (!key) return null; + return t(key);
239-244: Normalize and precompute sets for membership checksCurrent arrays cause repeated linear scans and inconsistent normalization across call sites. Normalize once and use Sets for O(1) lookups.
- const verifiedNumbers = useMemo( - () => _verifiedNumbers?.map((number) => number.phoneNumber) || [], - [_verifiedNumbers] - ); - const verifiedEmails = useMemo(() => _verifiedEmails || [], [_verifiedEmails]); + const verifiedNumbers = useMemo( + () => (_verifiedNumbers?.map((n) => n.phoneNumber) ?? []), + [_verifiedNumbers] + ); + const verifiedEmails = useMemo(() => (_verifiedEmails ?? []), [_verifiedEmails]); + const normalizePhone = useCallback((s?: string) => s?.replace(/[^\d+]/g, "") ?? "", []); + const verifiedNumberSet = useMemo( + () => new Set(verifiedNumbers.map(normalizePhone)), + [verifiedNumbers, normalizePhone] + ); + const verifiedEmailSet = useMemo( + () => new Set(verifiedEmails.map((e) => e.toLowerCase().trim())), + [verifiedEmails] + );
401-408: Email verification status: compare case-insensitivelyEmail addresses are case-insensitive; current strict equality can misreport.
- const getEmailVerificationStatus = useCallback( - () => - !!step && - !!verifiedEmails.find( - (email: string) => email === form.getValues(`steps.${step.stepNumber - 1}.sendTo`) - ), - [step, verifiedEmails, form] - ); + const getEmailVerificationStatus = useCallback(() => { + if (!step) return false; + const sendTo = (form.getValues(`steps.${step.stepNumber - 1}.sendTo`) as string | undefined) + ?.toLowerCase() + .trim(); + return !!sendTo && verifiedEmailSet.has(sendTo); + }, [step, verifiedEmailSet, form]);
413-415: Keep verification flags in sync on programmatic value changesIf sendTo changes via form.setValue (e.g., action switch), effects depending only on callbacks won’t re-run. Subscribe to form changes:
useEffect(() => { const sub = form.watch((_, { name }) => { if (!name?.endsWith(".sendTo")) return; setNumberVerified(getNumberVerificationStatus()); setEmailVerified(getEmailVerificationStatus()); }); return () => sub.unsubscribe(); }, [form, getNumberVerificationStatus, getEmailVerificationStatus]);Verify this is necessary in your flows (e.g., switching actions sets sendTo programmatically).
239-244: Consistency: remove unnecessary array clone in onChange checksElsewhere you use .concat([]) before .find(). It’s not needed; .find() is non-mutating. After adopting Sets as suggested, this also becomes moot.
Also applies to: 392-399, 401-408
📜 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/ee/workflows/components/WorkflowStepContainer.tsx(4 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/ee/workflows/components/WorkflowStepContainer.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/ee/workflows/components/WorkflowStepContainer.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/ee/workflows/components/WorkflowStepContainer.tsx
🧠 Learnings (5)
📓 Common learnings
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Learnt from: supalarry
PR: calcom/cal.com#23217
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts:93-94
Timestamp: 2025-08-21T13:44:06.805Z
Learning: In apps/api/v2/src/ee/event-types/event-types_2024_06_14/event-types.repository.ts, repository functions that use explicit Prisma select clauses (like getEventTypeWithSeats) are used for specific purposes and don't need to include all EventType fields like bookingRequiresAuthentication. These methods don't feed into the general OutputEventTypesService_2024_06_14 flow.
📚 Learning: 2025-08-26T20:09:17.089Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/features/ee/workflows/components/WorkflowStepContainer.tsx:641-649
Timestamp: 2025-08-26T20:09:17.089Z
Learning: In packages/features/ee/workflows/components/WorkflowStepContainer.tsx, Cal.AI actions are intentionally filtered out/hidden for organization workflows when props.isOrganization is true. This restriction is by design per maintainer Udit-takkar in PR #22995, despite the broader goal of enabling Cal.AI self-serve.
Applied to files:
packages/features/ee/workflows/components/WorkflowStepContainer.tsx
📚 Learning: 2025-09-08T09:34:46.661Z
Learnt from: CarinaWolli
PR: calcom/cal.com#23421
File: packages/features/ee/workflows/components/AddActionDialog.tsx:171-174
Timestamp: 2025-09-08T09:34:46.661Z
Learning: In packages/features/ee/workflows/components/AddActionDialog.tsx, the actionOptions prop is guaranteed to never be empty according to maintainer CarinaWolli, so defensive checks for empty arrays are not necessary.
Applied to files:
packages/features/ee/workflows/components/WorkflowStepContainer.tsx
📚 Learning: 2025-08-15T00:27:33.280Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/features/ee/workflows/components/AgentConfigurationSheet.tsx:348-371
Timestamp: 2025-08-15T00:27:33.280Z
Learning: In calcom/cal.com workflows and AI agent components, variable insertion follows a consistent pattern of directly transforming the input variable with toUpperCase() and replace(/ /g, "_") to create tokens like {VARIABLE_NAME}. The AgentConfigurationSheet.tsx implementation correctly follows this same pattern as WorkflowStepContainer.tsx, per maintainer Udit-takkar in PR #22995.
Applied to files:
packages/features/ee/workflows/components/WorkflowStepContainer.tsx
📚 Learning: 2025-08-15T00:27:33.280Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/features/ee/workflows/components/AgentConfigurationSheet.tsx:348-371
Timestamp: 2025-08-15T00:27:33.280Z
Learning: In calcom/cal.com, variable insertion in both WorkflowStepContainer.tsx (addVariableEmailSubject) and AgentConfigurationSheet.tsx (addVariableToGeneralPrompt) follows the identical pattern: `{${variable.toUpperCase().replace(/ /g, "_")}}`. This is the established, consistent approach across the codebase for transforming variable labels into tokens, per maintainer Udit-takkar in PR #22995.
Applied to files:
packages/features/ee/workflows/components/WorkflowStepContainer.tsx
🔇 Additional comments (1)
packages/features/ee/workflows/components/WorkflowStepContainer.tsx (1)
4-4: LGTM: added React hooks importThe added useCallback/useEffect/useMemo/useRef/useState imports align with later memoization/callback usage.
| const getNumberVerificationStatus = useCallback( | ||
| () => | ||
| !!step && | ||
| !!verifiedNumbers.find( | ||
| (number: string) => number === form.getValues(`steps.${step.stepNumber - 1}.sendTo`) | ||
| ), | ||
| [step, verifiedNumbers, form] | ||
| ); |
There was a problem hiding this comment.
Phone verification status: normalize and use Set
Direct string equality will false-negative due to formatting (spaces, dashes). Also prefer Set lookup for performance.
- const getNumberVerificationStatus = useCallback(
- () =>
- !!step &&
- !!verifiedNumbers.find(
- (number: string) => number === form.getValues(`steps.${step.stepNumber - 1}.sendTo`)
- ),
- [step, verifiedNumbers, form]
- );
+ const getNumberVerificationStatus = useCallback(() => {
+ if (!step) return false;
+ const sendTo = form.getValues(`steps.${step.stepNumber - 1}.sendTo`) as string | undefined;
+ return verifiedNumberSet.has(normalizePhone(sendTo));
+ }, [step, verifiedNumberSet, normalizePhone, form]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/features/ee/workflows/components/WorkflowStepContainer.tsx around
lines 392-399, the current phone verification check uses raw string equality
which can false-negative due to formatting; replace it by normalizing numbers
and using a Set lookup: create a memoized Set (useMemo) of verifiedNumbers
normalized to digits only (remove non-digit chars) and useCallback to normalize
the form value for steps.{step.stepNumber - 1}.sendTo to digits-only, then
return !!step && normalizedSet.has(normalizedValue); update the hook
dependencies to include step and the memoized Set (and form if needed) so
lookups are correct and performant.
| return plainToClass( | ||
| EventTypeOutput_2024_06_14, | ||
| this.outputEventTypesService.getResponseEventType(eventType.ownerId, eventType, false), | ||
| this.outputEventTypesService.getResponseEventType(eventType.ownerId, eventType), |
There was a problem hiding this comment.
Platform changes are eslint fixes. Since we removed the unused param from getResponseEventType
| /> | ||
| )} | ||
|
|
||
| {formFieldType === "phone" && ( |
There was a problem hiding this comment.
Move selecting allowed countries for phone inputs to the booking field level
CarinaWolli
left a comment
CarinaWolli
left a comment
There was a problem hiding this comment.
It still let's me submit a different country code. See here:
--> Allowed Country codes --> US & Germany:
--> Shows correct in the dropdown, but I can still type another country code:
--> No error message, booking is successful with a country code that is not supported:
| }: { | ||
| workflowId: Workflow["id"]; | ||
| isSmsReminderNumberRequired: boolean; | ||
| allowedCountryCodes?: string[]; |
There was a problem hiding this comment.
| allowedCountryCodes?: string[]; | |
| allowedCountryCodes?: string[]; |
This is not used
| newResponses[field.name] = parsedValue; | ||
| } else if (field.type === "phone") { | ||
| newResponses[field.name] = ensureValidPhoneNumber(value); | ||
| // Auto-prepend country code for single country restrictions |
There was a problem hiding this comment.
I think the single country restriction causes issues for many countries. For example, in Austria if I don’t see +43, I naturally enter my number with a leading 0:
But then we change it to: +430680123456
Which is the wrong international format, as it should be +43 680123456
|
Deprio for now |
3 participants
What does this PR do?
This PR implements country code filtering for SMS workflows in Cal.com, allowing users to restrict SMS notifications to specific countries. It also includes performance optimizations for country code generation.
Key Features:
getCountries()callDatabase Changes:
allowedCountryCodesfield toWorkflowStepmodel (defaults to empty array = all countries allowed)Visual Demo
https://www.loom.com/share/090dd55a92764e95bdc0bbdaa3145611
Technical Implementation
Frontend Changes:
Backend Changes:
Performance Improvements:
getCountries()callHow should this be tested?
Environment Setup:
Test Scenarios:
Basic Functionality:
SMS Filtering:
Backward Compatibility:
allowedCountryCodesshould allow all countriesEdge Cases:
Mandatory Tasks
Important Review Areas
allowedCountryCodesfield migration works correctlysmsReminderManager.tsfor invalid phone numbersFiles Requiring Extra Attention:
packages/features/bookings/lib/getBookingFields.ts- Complex type manipulationspackages/features/ee/workflows/lib/reminders/smsReminderManager.ts- Phone validation logicpackages/prisma/schema.prisma- Database schema changesLink to Devin run: https://app.devin.ai/sessions/5d47ef9c555c4aaca9c657e6cf713f1e
Requested by: @joeauyeung