feat: Implement variable pricing calculation engine #23737 #23853
Conversation
- Add comprehensive TypeScript types for variable pricing rules and configurations - Implement core pricing calculator with support for: - Duration-based absolute pricing and modifiers - Time-of-day based surcharges/discounts - Day-of-week based pricing rules - Percentage and fixed-amount modifiers - Rule priority and conditions - Add utility functions for config management and validation - Implement comprehensive test suite with 35 passing tests - Support for multiple currencies and price formatting - Fallback to legacy pricing for backward compatibility This establishes the foundation for dynamic pricing in Cal.com bookings.
|
@dharanidharansr is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds a variable pricing subsystem across library, API, Stripe integration, and web UI. New files introduce pricing types, utils, a caching-enabled calculator (with bulk/preview/enhanced helpers and cache management), TRPC routers and procedures for event-type pricing and Stripe price creation, Stripe metadata/mapping helpers, admin and booking React components, demo/playground/test scripts, and an index barrel export. Also updates eventTypes router composition to include the pricing router, adjusts an apps-store ESLint override, updates VSCode spell settings, and modifies turbo.json environment variables for Stripe. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
github-actions
bot
added
✨ feature
|
|
|
Hi team, the automated checks all passed and the changes add support for multiple pricing options per event slot as discussed. Please review the PR and let me know if any adjustments or clarifications are needed. Thanks! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (21)
packages/lib/pricing/types.ts (4)
5-14: Consider makingpriorityrequired for consistent rule ordering.The
priorityfield is optional but is critical for determining rule evaluation order. Making it required would prevent potential issues with undefined priority values defaulting to 0 and ensure explicit prioritization.Apply this diff to make priority required:
export interface PricingRule { id: string; type: PricingRuleType; description: string; enabled: boolean; - priority?: number; + priority: number; // 0-100, higher values = higher priority condition: DurationCondition | TimeOfDayCondition | DayOfWeekCondition | CustomCondition; price?: number; // Absolute price in cents priceModifier?: PriceModifier; // For surcharges/discounts }
21-24: Add validation for time format in TimeOfDayCondition.The comment indicates "HH:mm format" but there's no type-level enforcement. Consider using a branded type or documenting the validation requirement more prominently.
Consider adding a type alias for clarity:
+// Time in 24-hour format "HH:mm" (e.g., "09:00", "17:30") +export type TimeString = string; + export interface TimeOfDayCondition { - startTime: string; // HH:mm format - endTime: string; // HH:mm format + startTime: TimeString; + endTime: TimeString; }
26-28: Consider using an enum for day names to prevent typos.Using string literals for day names is error-prone. An enum or union type would provide better type safety.
Apply this diff to add type safety for day names:
+export type DayOfWeek = "sunday" | "monday" | "tuesday" | "wednesday" | "thursday" | "friday" | "saturday"; + export interface DayOfWeekCondition { - days: string[]; // ["monday", "tuesday", etc.] + days: DayOfWeek[]; }
35-39: Clarify the relationship betweenvalueandpercentagefields.The PriceModifier interface has both
valueandpercentagefields, but their interaction isn't clear. Consider adding validation rules or making them mutually exclusive.Consider using a discriminated union for clearer semantics:
-export interface PriceModifier { - type: PriceModifierType; - value: number; // Amount in cents - percentage?: number; // For percentage-based modifiers -} +export type PriceModifier = + | { + type: "surcharge" | "discount"; + value: number; // Fixed amount in cents + } + | { + type: "surcharge" | "discount"; + percentage: number; // Percentage (0-100) + } + | { + type: "absolute"; + value: number; // Absolute price in cents + };packages/lib/pricing/index.ts (1)
1-21: Prefer named exports over wildcard re-exports for types.Using wildcard exports (
export *) can lead to namespace pollution and makes it harder to track what's being exposed. Consider explicitly listing the exported types.Apply this diff for more explicit exports:
// Types -export * from "./types"; +export type { + PricingRuleType, + PriceModifierType, + PricingRule, + DurationCondition, + TimeOfDayCondition, + DayOfWeekCondition, + CustomCondition, + PriceModifier, + VariablePricingConfig, + PricingContext, + PriceBreakdownItem, + PriceCalculationResult, + EventTypeMetadataStripe, +} from "./types"; // Calculator functions export { calculateVariablePrice, formatPrice, createPricingContext, } from "./calculator"; // Utility functions export { getVariablePricingConfig, setVariablePricingConfig, createPricingRule, createPriceModifier, validateVariablePricingConfig, isVariablePricingEnabled, getPricingRulesSummary, DEFAULT_VARIABLE_PRICING_CONFIG, } from "./utils";packages/lib/pricing/__tests__/calculator.test.ts (1)
399-405: Consider testing additional edge cases for formatPrice.While the current tests cover basic formatting, consider adding tests for edge cases like very large amounts, negative amounts (if applicable), and invalid currency codes.
Add these test cases:
it("should handle edge cases in price formatting", () => { // Large amounts expect(formatPrice(999999999, "usd")).toBe("$9,999,999.99"); // Zero expect(formatPrice(0, "usd")).toBe("$0.00"); // Single cent expect(formatPrice(1, "usd")).toBe("$0.01"); // Maximum safe integer expect(formatPrice(Number.MAX_SAFE_INTEGER, "usd")).toBeDefined(); }); it("should handle invalid currency codes gracefully", () => { // This might throw or return a fallback - define expected behavior expect(() => formatPrice(5000, "invalid")).toThrow(); });packages/lib/pricing/calculator.ts (5)
51-61: Potential issue with type order sorting.The type order object assumes all rule types are present. If a new rule type is added or an unexpected type appears, this will cause a runtime error.
Apply this diff to handle unknown types safely:
const enabledRules = config.rules .filter((rule) => rule.enabled) .sort((a, b) => { const priorityDiff = (b.priority || 0) - (a.priority || 0); if (priorityDiff !== 0) return priorityDiff; // Secondary sort by type order: duration > timeOfDay > dayOfWeek > custom const typeOrder = { duration: 0, timeOfDay: 1, dayOfWeek: 2, custom: 3 }; - return typeOrder[a.type] - typeOrder[b.type]; + return (typeOrder[a.type] ?? 999) - (typeOrder[b.type] ?? 999); });
105-119: Type assertions could hide type errors.Multiple type assertions are used when accessing rule conditions. Consider using type guards for safer type narrowing.
Apply this diff to use type guards instead of assertions:
+// Type guards +function isDurationRule(rule: PricingRule): rule is PricingRule & { condition: DurationCondition } { + return rule.type === "duration"; +} + +function isTimeOfDayRule(rule: PricingRule): rule is PricingRule & { condition: TimeOfDayCondition } { + return rule.type === "timeOfDay"; +} + +function isDayOfWeekRule(rule: PricingRule): rule is PricingRule & { condition: DayOfWeekCondition } { + return rule.type === "dayOfWeek"; +} + function isRuleApplicable(rule: PricingRule, context: PricingContext): boolean { - switch (rule.type) { - case "duration": - return isDurationRuleApplicable(rule.condition as DurationCondition, context); - case "timeOfDay": - return isTimeOfDayRuleApplicable(rule.condition as TimeOfDayCondition, context); - case "dayOfWeek": - return isDayOfWeekRuleApplicable(rule.condition as DayOfWeekCondition, context); - case "custom": - // For now, custom rules are not implemented - return false; - default: - return false; - } + if (isDurationRule(rule)) { + return isDurationRuleApplicable(rule.condition, context); + } + if (isTimeOfDayRule(rule)) { + return isTimeOfDayRuleApplicable(rule.condition, context); + } + if (isDayOfWeekRule(rule)) { + return isDayOfWeekRuleApplicable(rule.condition, context); + } + if (rule.type === "custom") { + // For now, custom rules are not implemented + return false; + } + return false; }
151-162: Time comparison logic doesn't account for booking end time.The time-of-day rule only checks the start time, but for accurate pricing, you might want to consider if the entire booking duration falls within the time range.
Consider whether the rule should apply if:
- Only the start time is in range (current implementation)
- The entire booking is within the range
- Any part of the booking overlaps the range
This should be documented clearly. If you want to check the entire duration:
function isTimeOfDayRuleApplicable(condition: TimeOfDayCondition, context: PricingContext): boolean { const { startTime, endTime } = condition; - const contextTime = format(context.startTime, "HH:mm"); + const contextStartTime = format(context.startTime, "HH:mm"); + const contextEndTime = format(context.endTime, "HH:mm"); // Handle same-day time range (e.g., 09:00 to 17:00) if (startTime <= endTime) { - return contextTime >= startTime && contextTime <= endTime; + // Check if booking is entirely within the time range + return contextStartTime >= startTime && contextEndTime <= endTime; } // Handle overnight time range (e.g., 22:00 to 06:00) - return contextTime >= startTime || contextTime <= endTime; + // This case becomes more complex for full duration checking + return contextStartTime >= startTime || contextEndTime <= endTime; }
189-214: Complex modifier calculation logic could be simplified.The modifier calculation has overlapping logic paths that could lead to confusion. The percentage handling overrides the initial amount calculation for surcharge/discount types.
Apply this diff to clarify the logic:
function calculateModifier(rule: PricingRule, currentPrice: number, context: PricingContext): PriceBreakdownItem { const { priceModifier } = rule; if (!priceModifier) { return { description: rule.description, amount: 0, type: "surcharge", ruleId: rule.id, }; } let amount = 0; let type: "surcharge" | "discount" = "surcharge"; - switch (priceModifier.type) { - case "absolute": - amount = priceModifier.value - currentPrice; - type = amount >= 0 ? "surcharge" : "discount"; - break; - case "surcharge": - amount = priceModifier.value; - type = "surcharge"; - break; - case "discount": - amount = -Math.abs(priceModifier.value); - type = "discount"; - break; - } - - // Handle percentage-based modifiers - if (priceModifier.percentage !== undefined) { - const percentageAmount = Math.round((currentPrice * priceModifier.percentage) / 100); - if (priceModifier.type === "discount") { - amount = -percentageAmount; - type = "discount"; - } else { - amount = percentageAmount; - type = "surcharge"; + // Calculate amount based on modifier type and whether it's percentage-based + if (priceModifier.percentage !== undefined) { + // Percentage-based modifier + const percentageAmount = Math.round((currentPrice * priceModifier.percentage) / 100); + switch (priceModifier.type) { + case "surcharge": + amount = percentageAmount; + type = "surcharge"; + break; + case "discount": + amount = -percentageAmount; + type = "discount"; + break; + case "absolute": + // Absolute type doesn't support percentage + console.warn(`Absolute modifier type doesn't support percentage`); + amount = priceModifier.value - currentPrice; + type = amount >= 0 ? "surcharge" : "discount"; + break; + } + } else { + // Fixed amount modifier + switch (priceModifier.type) { + case "absolute": + amount = priceModifier.value - currentPrice; + type = amount >= 0 ? "surcharge" : "discount"; + break; + case "surcharge": + amount = priceModifier.value; + type = "surcharge"; + break; + case "discount": + amount = -Math.abs(priceModifier.value); + type = "discount"; + break; } } return { description: rule.description, amount, type, ruleId: rule.id, }; }
227-232: Consider caching NumberFormat instances for performance.Creating new Intl.NumberFormat instances on every call can be expensive in hot paths. Consider memoizing them.
Apply this diff to cache formatter instances:
+// Cache for NumberFormat instances +const formatCache = new Map<string, Intl.NumberFormat>(); + +function getFormatter(currency: string, locale: string): Intl.NumberFormat { + const key = `${locale}:${currency}`; + if (!formatCache.has(key)) { + formatCache.set(key, new Intl.NumberFormat(locale, { + style: "currency", + currency: currency.toUpperCase(), + })); + } + return formatCache.get(key)!; +} + export function formatPrice(amount: number, currency: string, locale = "en-US"): string { - return new Intl.NumberFormat(locale, { - style: "currency", - currency: currency.toUpperCase(), - }).format(amount / 100); + return getFormatter(currency, locale).format(amount / 100); }packages/lib/pricing/utils.ts (10)
1-10: Consider importing only the necessary Prisma typesInstead of importing the entire
Prismanamespace, consider importing onlyJsonObjectdirectly to reduce bundle size and improve tree-shaking.-import type { EventType, Prisma } from "@prisma/client"; +import type { EventType } from "@prisma/client"; +import type { JsonObject } from "@prisma/client/runtime/library";Note: Verify that
JsonObjectis exported from the correct path in your Prisma version.
25-58: Add proper type checking and improve error handlingThe function has several issues with type safety and error handling:
- Line 31: The condition
pricingConfig === nullis redundant sincetypeof null === 'object'is true- The type assertion on line 28 is unsafe and could mask runtime errors
- Missing validation for negative legacy prices
export function getVariablePricingConfig(eventType: EventType): VariablePricingConfig { try { const metadata = eventType.metadata as Prisma.JsonObject; - const pricingConfig = metadata?.variablePricing as VariablePricingConfig; + const pricingConfig = metadata?.variablePricing; // Check if pricingConfig is a valid object with required properties - if (!pricingConfig || typeof pricingConfig !== 'object' || pricingConfig === null) { + if (!pricingConfig || typeof pricingConfig !== 'object') { // Check for legacy price field as fallback - const legacyPrice = eventType.price || 0; + const legacyPrice = Math.max(0, eventType.price || 0); return { ...DEFAULT_VARIABLE_PRICING_CONFIG, basePrice: legacyPrice, currency: eventType.currency || "usd", }; } + // Type guard to ensure pricingConfig has the expected shape + const config = pricingConfig as Record<string, unknown>; + // Validate and return the pricing config return { - enabled: Boolean(pricingConfig.enabled), - basePrice: Number(pricingConfig.basePrice) || 0, - currency: pricingConfig.currency || "usd", - rules: Array.isArray(pricingConfig.rules) ? pricingConfig.rules : [], + enabled: Boolean(config.enabled), + basePrice: Math.max(0, Number(config.basePrice) || 0), + currency: String(config.currency || "usd"), + rules: Array.isArray(config.rules) ? config.rules : [], }; } catch (error) { console.error("Error parsing variable pricing config:", error); // Return default config with legacy price as fallback - const legacyPrice = eventType.price || 0; + const legacyPrice = Math.max(0, eventType.price || 0); return { ...DEFAULT_VARIABLE_PRICING_CONFIG, basePrice: legacyPrice, currency: eventType.currency || "usd", }; } }
133-135: Improve ID generation for better uniqueness and readabilityThe current ID generation using
substris deprecated and the random component might be too short for guaranteed uniqueness in high-throughput scenarios.function generateRuleId(): string { - return `rule_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`; + return `rule_${Date.now()}_${Math.random().toString(36).substring(2, 11)}`; }Consider using a proper UUID library like
crypto.randomUUID()if available in your runtime environment for better uniqueness guarantees.
167-169: Improve currency validation to be more robustThe current validation only checks for 3-letter codes but doesn't verify if it's a valid ISO 4217 currency code.
// Validate currency - if (!config.currency || config.currency.length !== 3) { - errors.push("Currency must be a valid 3-letter ISO code"); + if (!config.currency || !/^[A-Z]{3}$/i.test(config.currency)) { + errors.push("Currency must be a valid 3-letter ISO 4217 code"); }Consider maintaining a list of valid ISO 4217 currency codes for more accurate validation, or use a library like
currency-codesfor comprehensive validation.
206-208: Priority validation range seems arbitraryThe priority range of 0-100 is validated here, but there's no documentation explaining why this specific range was chosen. Consider documenting this business rule or making it configurable.
+ // Priority determines the order of rule application (0 = lowest, 100 = highest) if (rule.priority !== undefined && (rule.priority < 0 || rule.priority > 100)) { errors.push("Priority must be between 0 and 100"); }
300-300: Case-insensitive day comparison might cause issuesThe validation converts days to lowercase for comparison but doesn't normalize the stored values, which could lead to inconsistent data.
- const invalidDays = dayCondition.days.filter((day) => !validDays.includes(day.toLowerCase())); + // Normalize days to lowercase for consistent storage + const normalizedDays = dayCondition.days.map(day => day.toLowerCase()); + const invalidDays = normalizedDays.filter((day) => !validDays.includes(day)); + + if (invalidDays.length === 0) { + // Update the condition with normalized days + dayCondition.days = normalizedDays; + }
319-322: Time format validation is too permissiveThe regex allows formats like "9:30" instead of enforcing "09:30", which might lead to inconsistent data storage.
function isValidTimeFormat(time: string): boolean { - const timeRegex = /^([01]?[0-9]|2[0-3]):[0-5][0-9]$/; + // Enforce HH:mm format (always 2 digits for hours and minutes) + const timeRegex = /^([01][0-9]|2[0-3]):[0-5][0-9]$/; return timeRegex.test(time); }This ensures consistent "HH:mm" format (e.g., "09:30" instead of "9:30").
327-330: Consider caching the configuration for performanceThe
isVariablePricingEnabledfunction callsgetVariablePricingConfigwhich parses metadata every time. For frequently called code paths, this could impact performance.Consider implementing a caching mechanism if this function is called frequently:
// Example with a simple WeakMap cache const configCache = new WeakMap<EventType, VariablePricingConfig>(); export function isVariablePricingEnabled(eventType: EventType): boolean { let config = configCache.get(eventType); if (!config) { config = getVariablePricingConfig(eventType); configCache.set(eventType, config); } return config.enabled && config.rules.length > 0; }
335-350: Summary text could be more informativeThe summary only shows rule counts and types but doesn't indicate the actual pricing impact or conditions.
Consider providing more detailed summaries:
export function getPricingRulesSummary(config: VariablePricingConfig): string { if (!config.enabled || !config.rules || config.rules.length === 0) { - return "Variable pricing disabled"; + return config.enabled ? "No pricing rules configured" : "Variable pricing disabled"; } const enabledRules = config.rules.filter((rule) => rule.enabled); if (enabledRules.length === 0) { return "No active pricing rules"; } const ruleTypes = enabledRules.map((rule) => rule.type); const uniqueTypes = [...new Set(ruleTypes)]; - return `${enabledRules.length} active rules (${uniqueTypes.join(", ")})`; + const typeCount = uniqueTypes.map(type => + `${ruleTypes.filter(t => t === type).length} ${type}` + ).join(", "); + + return `${enabledRules.length} active rule${enabledRules.length > 1 ? 's' : ''} (${typeCount})`; }
103-128: Add TypeScript strict type checking for condition creationThe function returns inconsistent types and uses unsafe type assertions. Consider using a discriminated union pattern for better type safety.
-function createDefaultCondition(type: PricingRule["type"]): PricingRule["condition"] { +function createDefaultCondition(type: PricingRule["type"]): DurationCondition | TimeOfDayCondition | DayOfWeekCondition | Record<string, unknown> { switch (type) { case "duration": - return { + return { minDuration: 30, maxDuration: 120, - } as DurationCondition; + } satisfies DurationCondition; case "timeOfDay": - return { + return { startTime: "09:00", endTime: "17:00", - } as TimeOfDayCondition; + } satisfies TimeOfDayCondition; case "dayOfWeek": - return { + return { days: ["monday", "tuesday", "wednesday", "thursday", "friday"], - } as DayOfWeekCondition; + } satisfies DayOfWeekCondition; case "custom": - return {} as Record<string, unknown>; + return {}; default: + // This should never happen with proper TypeScript types + const exhaustiveCheck: never = type; return {}; } }
📜 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 (6)
packages/lib/pricing/__tests__/calculator.test.ts(1 hunks)packages/lib/pricing/__tests__/utils.test.ts(1 hunks)packages/lib/pricing/calculator.ts(1 hunks)packages/lib/pricing/index.ts(1 hunks)packages/lib/pricing/types.ts(1 hunks)packages/lib/pricing/utils.ts(1 hunks)
🧰 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/lib/pricing/index.tspackages/lib/pricing/__tests__/calculator.test.tspackages/lib/pricing/types.tspackages/lib/pricing/calculator.tspackages/lib/pricing/__tests__/utils.test.tspackages/lib/pricing/utils.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/lib/pricing/index.tspackages/lib/pricing/__tests__/calculator.test.tspackages/lib/pricing/types.tspackages/lib/pricing/calculator.tspackages/lib/pricing/__tests__/utils.test.tspackages/lib/pricing/utils.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/lib/pricing/index.tspackages/lib/pricing/__tests__/calculator.test.tspackages/lib/pricing/types.tspackages/lib/pricing/calculator.tspackages/lib/pricing/__tests__/utils.test.tspackages/lib/pricing/utils.ts
🔇 Additional comments (12)
packages/lib/pricing/types.ts (2)
1-4: LGTM! Well-structured type definitions.The union types for pricing rules and modifiers provide clear, type-safe categorization for the pricing system.
72-78: Good backward compatibility approach with deprecated field.The inclusion of the deprecated
pricefield maintains backward compatibility while transitioning to the newvariablePricingstructure.packages/lib/pricing/__tests__/utils.test.ts (3)
17-60: LGTM! Comprehensive mock EventType factory.The mock factory provides all necessary fields for testing and supports overrides for flexibility.
289-292: Good validation for currency format.The test correctly validates the 3-letter ISO currency code requirement.
315-319: Excellent comprehensive validation testing.The test thoroughly validates multiple error conditions in a single rule, ensuring proper error aggregation.
packages/lib/pricing/__tests__/calculator.test.ts (3)
85-119: LGTM! Comprehensive test coverage for pricing rules.The tests effectively validate weekend surcharge application and percentage-based calculations.
232-283: Excellent overnight time range testing.The test thoroughly validates the edge case of time ranges that cross midnight boundaries, testing both late night and early morning scenarios.
285-313: Good defensive programming with price floor at zero.The test validates that heavy discounts don't result in negative prices, which is crucial for preventing billing errors.
packages/lib/pricing/calculator.ts (2)
237-253: LGTM! Clean helper function for context creation.The function correctly calculates duration in minutes and extracts the day of week.
1-2: Flag excessive Day.js use - prefer native Date in this module.According to coding guidelines, excessive Day.js use should be flagged in performance-critical code. Since this pricing calculator will be called frequently, consider using native Date methods or Day.js
.utc()for better performance. However, I see you're using date-fns which is fine - just ensure it's necessary.Let's verify if date-fns is already used elsewhere in the codebase and check the performance implications:
#!/bin/bash # Check date-fns usage in the codebase echo "Checking date-fns usage patterns..." rg -n --type=ts --type=tsx "from ['\"]date-fns" -C2 # Check if Day.js is the standard in this codebase echo -e "\nChecking Day.js usage patterns..." rg -n --type=ts --type=tsx "from ['\"]dayjs" -C2 | head -20 # Check package.json for both libraries echo -e "\nChecking package dependencies..." fd -t f "package.json" --exec grep -E "(date-fns|dayjs)" {} \;packages/lib/pricing/utils.ts (2)
242-245: Percentage validation for price modifiers needs clarificationThe validation allows percentages from 0-100, but it's unclear if percentages over 100% should be allowed for surcharges (e.g., 150% surcharge).
Should the system support surcharge percentages greater than 100%? This would allow for scenarios like "150% surcharge" which would add 1.5x the base price. The current validation limits it to 100%.
15-20: Excellent use of strongly-typed default configurationThe default configuration provides a clear, type-safe baseline for the pricing system with sensible defaults. This is a good practice for configuration management.
|
Hi @calcom/maintainers 👋 This PR implements the variable pricing calculation engine (Fixes #23737 / CAL-6395).
Testing notes:
Could you please review/approve when you have a chance? 🙏 Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (27)
test-variable-pricing.mjs (3)
8-9: Import stability: avoid depending on TS source paths in an .mjs runner.This will fail unless the library is pre-built to .js at those paths. Either (a) run via tsx/ts-node and import TS, or (b) import from the package’s built entry (e.g., dist/index.js).
-import { calculateVariablePrice, createPricingContext, formatPrice } from "../packages/lib/pricing/calculator.js"; -import { createPricingRule, createPriceModifier } from "../packages/lib/pricing/utils.js"; +// Option A: run with `pnpm dlx tsx test-variable-pricing.ts` after renaming to .ts and using TS imports +// Option B: after building the lib, import the built entry: +import { calculateVariablePrice, createPricingContext, formatPrice, createPricingRule, createPriceModifier } from "../packages/lib/pricing/dist/index.js";
116-121: Cross‑midnight time windows may be mishandled.
timeOfDay { startTime: "20:00", endTime: "06:00" }implies wrap-around. Verify the engine supports overnight ranges; otherwise split into two windows or update matcher logic.
142-143: Add assertions instead of a success banner.Log output is not a safeguard. Add basic asserts for each scenario to prevent silent regressions.
-console.log("\n✅ All tests completed successfully!"); +import assert from "node:assert/strict"; +// Example: weekend 25% on $100 -> $125 +assert.equal(weekendResult.totalPrice, 12500, "Weekend surcharge mismatch"); +console.log("\n✅ All checks passed!");packages/lib/pricing/demo.ts (2)
127-137: Use explicit timeZone in toLocaleString to match the pricing context.Without
timeZone, displayed timestamps vary by host settings and may not match the"America/New_York"context.- ` 📅 ${scenario.startTime.toLocaleString("en-US", { + ` 📅 ${scenario.startTime.toLocaleString("en-US", { weekday: "long", year: "numeric", month: "short", day: "numeric", hour: "2-digit", minute: "2-digit", - })}` + timeZone: "America/New_York", + })}`
77-109: Make scenarios timezone‑robust.Bare ISO strings (no offset/Z) are parsed as local time, causing cross‑machine differences. Prefer explicit offsets or UTC constructors.
- startTime: new Date("2024-02-17T14:00:00"), - endTime: new Date("2024-02-17T15:00:00"), + startTime: new Date("2024-02-17T14:00:00-05:00"), // EST + endTime: new Date("2024-02-17T15:00:00-05:00"),apps/web/components/booking/VariablePricingPayment.tsx (3)
23-24: Remove duplicate loading state.Rely on React Query’s isLoading; drop isCalculating and its setters.
- const [isCalculating, setIsCalculating] = useState(true); + // rely on React Query's isLoading - setIsCalculating(false); + // no-op - setIsCalculating(false); + // no-op - if (isCalculating || isLoading) { + if (isLoading) {Also applies to: 59-63, 106-114
81-95: Surface error when Stripe account is missing.Silent return leaves the user stranded. Fail fast and inform the caller.
- const handlePayClick = () => { - if (!calculatedPrice || !stripeAccountId) return; + const handlePayClick = () => { + if (!calculatedPrice) return; + if (!stripeAccountId) { + onError(new Error(t("stripe_not_connected"))); + return; + }
98-104: Respect user locale when formatting currency.- const formatPrice = (amount: number, currency: string) => { - return new Intl.NumberFormat("en-US", { + const formatPrice = (amount: number, currency: string) => { + return new Intl.NumberFormat(undefined, { style: "currency", currency: currency, minimumFractionDigits: 2, }).format(amount / 100); };packages/lib/pricing/playground.ts (1)
1-9: Dev-only script: exclude from production builds.Mark this as a non-bundled dev tool (e.g., move under scripts/ or add package.json "files" whitelist) to avoid accidental publishing.
apps/web/components/eventtype/VariablePricingModal.tsx (3)
165-169: Localize loading state and use Spinner.- {isLoading ? ( - <div className="flex justify-center py-8"> - <div className="spinner">Loading...</div> - </div> - ) : ( + {isLoading ? ( + <div className="flex justify-center py-8"> + <Spinner /> + </div> + ) : (Add import:
- showToast, + showToast, + Spinner,
102-111: Normalize currency casing to match options and server validation.Back-end may return "usd"; the select expects uppercase. Normalize on read.
- form.setValue("variablePricing.currency", config.currency); + form.setValue("variablePricing.currency", config.currency?.toUpperCase?.() || "USD");Ensure the server persists currency as uppercase ISO-4217; otherwise, force uppercasing before update as well.
35-41: Localize all user-facing strings per guidelines.Labels in PRICE_MODIFIER_ACTIONS, CONDITION_TYPES, COMPARISON_OPERATORS, DAYS_OF_WEEK and inline rule summaries ("Duration …", "Time between …", "Days: …") should use t(). Keep the value stable; only translate label.
Example:
- { value: "add", label: "Add (Fixed Amount)" }, + { value: "add", label: t("add_fixed_amount") },And for summaries:
- ? `Duration ${rule.condition.minDuration || 0} - ${rule.condition.maxDuration || "∞"} min` + ? t("duration_range_min_max_min_label", { min: rule.condition.minDuration || 0, max: rule.condition.maxDuration ?? "∞" })Also applies to: 43-49, 50-59, 61-69, 217-229, 241-269
packages/app-store/stripepayment/lib/variablePricing.ts (3)
64-66: Make cache/lookup key collision-safe across options.Include recurring and productId in the generated ID; otherwise one-time vs recurring can collide.
-export function generatePriceId(calculation: PriceCalculationResult, eventTypeId: number): string { - return `price_${eventTypeId}_${calculation.totalPrice}_${calculation.currency}`; -} +export function generatePriceId( + calculation: PriceCalculationResult, + eventTypeId: number, + opts?: { recurring?: boolean; productId?: string } +): string { + const recur = opts?.recurring ? "recurring" : "one_time"; + const prod = opts?.productId ?? "no_product"; + return `price_${eventTypeId}_${calculation.totalPrice}_${calculation.currency}_${recur}_${prod}`; +}
82-96: Use lookup_key on creation to enable idempotent reuse later.We compute a key but never use it. Attach it to the Price so future flows can reuse it instead of creating duplicates.
- const priceParams = mapCalculationToStripePrice(calculation, eventTypeId, options); - const _cacheKey = options.cacheKey || generatePriceId(calculation, eventTypeId); + const priceParams = mapCalculationToStripePrice(calculation, eventTypeId, options); + const _cacheKey = options.cacheKey || generatePriceId(calculation, eventTypeId, { recurring: options.recurring, productId: options.productId }); @@ - const price = await stripe.prices.create(priceParams, { + const price = await stripe.prices.create({ ...priceParams, lookup_key: _cacheKey }, { stripeAccount: stripeAccountId, });If your Stripe account supports Price Search, consider first attempting:
await stripe.prices.search({query: `active:'true' AND lookup_key:'${_cacheKey}'`, limit: 1}, {stripeAccount: stripeAccountId})and reuse the found price if present.
35-39: Naming nit: ruleTypes actually stores descriptions.Optional: rename to ruleDescriptions to avoid confusion with rule kind (duration/timeOfDay/…).
- const ruleTypes = hasRules + const ruleDescriptions = hasRules ? Array.from(new Set(breakdown.filter((item) => item.ruleId).map((item) => item.description))) : []; @@ - ruleTypes: ruleTypes.join(","), + ruleDescriptions: ruleDescriptions.join(","),Please update any downstream readers if you adopt this rename.
Also applies to: 53-55
packages/lib/pricing/calculator.ts (3)
438-476: Float-step loop can produce imprecise minutes (e.g., 45-min intervals).Use integer minute increments to avoid rounding issues.
- for (let hour = startHour; hour <= endHour; hour += intervalMinutes / 60) { - const startTime = new Date(date); - startTime.setHours(Math.floor(hour), (hour % 1) * 60, 0, 0); + const startMinutes = startHour * 60; + const endMinutes = endHour * 60; + for (let m = startMinutes; m <= endMinutes; m += intervalMinutes) { + const startTime = new Date(date); + const h = Math.floor(m / 60); + const min = m % 60; + startTime.setHours(h, min, 0, 0);
34-60: Cache key stability: normalize rule order to reduce cache misses.Reordering the same enabled rules yields different JSON; sort rules deterministically in the key.
const configHash = JSON.stringify({ enabled: config.enabled, basePrice: config.basePrice, currency: config.currency, - rules: config.rules + rules: [...config.rules] + .sort((a, b) => (a.id > b.id ? 1 : -1)) .filter((r) => r.enabled) .map((r) => ({ id: r.id, type: r.type, priority: r.priority, condition: r.condition, price: r.price, priceModifier: r.priceModifier, })), });
19-25: Operational note: in-memory cache is per-process and non-durable.Fine for perf, but won’t help across serverless invocations or multi-instance deployments. Consider an optional shared cache (Redis) or feature-flag the cache.
Also applies to: 82-95, 99-101
packages/lib/pricing/utils.ts (7)
325-328: Enablement check should consider active rules, not just rule count.If all rules are disabled, this returns true.
-export function isVariablePricingEnabled(eventType: EventType): boolean { - const config = getVariablePricingConfig(eventType); - return config.enabled && config.rules.length > 0; -} +export function isVariablePricingEnabled(eventType: EventType): boolean { + const config = getVariablePricingConfig(eventType); + return config.enabled && (config.rules?.some((r) => r.enabled) ?? false); +}
543-556: Overnight range conflict detection always returns true.This causes false positives. Implement precise overlap for ranges crossing midnight.
-function timeRangesOverlap(cond1: TimeOfDayCondition, cond2: TimeOfDayCondition): boolean { - const time1Start = timeToMinutes(cond1.startTime); - const time1End = timeToMinutes(cond1.endTime); - const time2Start = timeToMinutes(cond2.startTime); - const time2End = timeToMinutes(cond2.endTime); - - // Handle overnight ranges - if (time1End < time1Start || time2End < time2Start) { - // Complex overnight logic - simplified for now - return true; // Assume overlap for overnight ranges - } - - return !(time1End < time2Start || time2End < time1Start); -} +function timeRangesOverlap(cond1: TimeOfDayCondition, cond2: TimeOfDayCondition): boolean { + const s1 = timeToMinutes(cond1.startTime); + const e1 = timeToMinutes(cond1.endTime); + const s2 = timeToMinutes(cond2.startTime); + const e2 = timeToMinutes(cond2.endTime); + + const ranges1 = e1 >= s1 ? [[s1, e1]] : [[s1, 1440], [0, e1]]; + const ranges2 = e2 >= s2 ? [[s2, e2]] : [[s2, 1440], [0, e2]]; + + for (const [a1, b1] of ranges1) { + for (const [a2, b2] of ranges2) { + if (!(b1 <= a2 || b2 <= a1)) return true; + } + } + return false; +}
569-571: Case-insensitive comparison for day strings.Defensive normalization avoids surprises.
-function daysOverlap(days1: string[], days2: string[]): boolean { - return days1.some((day) => days2.includes(day)); -} +function daysOverlap(days1: string[], days2: string[]): boolean { + const d1 = days1.map((d) => d.toLowerCase()); + const d2 = days2.map((d) => d.toLowerCase()); + return d1.some((day) => d2.includes(day)); +}
357-373: Normalize currency codes before lookup in placeholder convertPrice.Prevents misses when callers pass lowercase codes.
-export function convertPrice(amount: number, fromCurrency: string, toCurrency: string): number { +export function convertPrice(amount: number, fromCurrency: string, toCurrency: string): number { // This is a placeholder - in production, you'd integrate with an exchange rate service // For now, return the same amount - if (fromCurrency === toCurrency) { + if (fromCurrency.toUpperCase() === toCurrency.toUpperCase()) { return amount; } @@ - const rate = conversionRates[fromCurrency]?.[toCurrency]; + const fc = fromCurrency.toUpperCase(); + const tc = toCurrency.toUpperCase(); + const rate = conversionRates[fc]?.[tc]; return rate ? Math.round(amount * rate) : amount; }
651-657: Median calculation for even counts should average the two middle values.Current implementation picks the lower middle element.
- const medianPrice = prices[Math.floor(prices.length / 2)]; + const mid = Math.floor(prices.length / 2); + const medianPrice = prices.length % 2 === 0 ? Math.round((prices[mid - 1] + prices[mid]) / 2) : prices[mid];
153-185: Doc vs validation mismatch for discount value sign.Docs suggest negative values for discounts; validator forbids negatives. Prefer “value is non‑negative; sign implied by type” and keep the validator.
Would you like me to update the docs in types.ts accordingly?
Also applies to: 234-247
49-59: Use logger consistently.Prefer shared logger over console.error for consistency and structured logs.
packages/lib/pricing/types.ts (2)
237-244: Clarify sign semantics of PriceModifier.value.Align docs with validator and calculator: value should be non‑negative; the type determines sign.
- /** Fixed amount in cents (positive for surcharge, negative for discount) */ + /** Fixed amount in cents (non‑negative; sign is implied by type) */ value: number;
304-308: Narrow timezone type for clarity.Use the provided TimezoneId alias to communicate intent without behavior change.
- /** Timezone for time-based calculations */ - timezone: string; + /** Timezone for time-based calculations */ + timezone: TimezoneId;
📜 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 (16)
.eslintrc.js(0 hunks).vscode/settings.json(1 hunks)apps/web/components/booking/VariablePricingPayment.tsx(1 hunks)apps/web/components/eventtype/VariablePricingModal.tsx(1 hunks)packages/app-store/stripepayment/lib/variablePricing.ts(1 hunks)packages/lib/pricing/calculator.ts(1 hunks)packages/lib/pricing/demo.ts(1 hunks)packages/lib/pricing/playground.ts(1 hunks)packages/lib/pricing/types.ts(1 hunks)packages/lib/pricing/utils.ts(1 hunks)packages/trpc/server/routers/viewer/eventTypes/_router.ts(3 hunks)packages/trpc/server/routers/viewer/eventTypes/variablePricing.ts(1 hunks)packages/trpc/server/routers/viewer/payments/_router.ts(2 hunks)packages/trpc/server/routers/viewer/payments/stripeVariablePricing.ts(1 hunks)test-variable-pricing.mjs(1 hunks)turbo.json(1 hunks)
💤 Files with no reviewable changes (1)
- .eslintrc.js
✅ Files skipped from review due to trivial changes (1)
- .vscode/settings.json
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/trpc/server/routers/viewer/eventTypes/variablePricing.tspackages/lib/pricing/demo.tspackages/trpc/server/routers/viewer/payments/_router.tspackages/lib/pricing/playground.tspackages/app-store/stripepayment/lib/variablePricing.tspackages/trpc/server/routers/viewer/payments/stripeVariablePricing.tspackages/trpc/server/routers/viewer/eventTypes/_router.tspackages/lib/pricing/calculator.tspackages/lib/pricing/utils.tspackages/lib/pricing/types.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/trpc/server/routers/viewer/eventTypes/variablePricing.tspackages/lib/pricing/demo.tspackages/trpc/server/routers/viewer/payments/_router.tspackages/lib/pricing/playground.tsapps/web/components/booking/VariablePricingPayment.tsxpackages/app-store/stripepayment/lib/variablePricing.tspackages/trpc/server/routers/viewer/payments/stripeVariablePricing.tspackages/trpc/server/routers/viewer/eventTypes/_router.tsapps/web/components/eventtype/VariablePricingModal.tsxpackages/lib/pricing/calculator.tspackages/lib/pricing/utils.tspackages/lib/pricing/types.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/trpc/server/routers/viewer/eventTypes/variablePricing.tspackages/lib/pricing/demo.tspackages/trpc/server/routers/viewer/payments/_router.tspackages/lib/pricing/playground.tsapps/web/components/booking/VariablePricingPayment.tsxpackages/app-store/stripepayment/lib/variablePricing.tspackages/trpc/server/routers/viewer/payments/stripeVariablePricing.tspackages/trpc/server/routers/viewer/eventTypes/_router.tsapps/web/components/eventtype/VariablePricingModal.tsxpackages/lib/pricing/calculator.tspackages/lib/pricing/utils.tspackages/lib/pricing/types.ts
**/*.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:
apps/web/components/booking/VariablePricingPayment.tsxapps/web/components/eventtype/VariablePricingModal.tsx
🧬 Code graph analysis (11)
packages/trpc/server/routers/viewer/eventTypes/variablePricing.ts (4)
packages/trpc/server/trpc.ts (1)
router(13-13)packages/lib/pricing/utils.ts (3)
getVariablePricingConfig(26-59)validateVariablePricingConfig(153-185)setVariablePricingConfig(64-79)packages/lib/pricing/types.ts (1)
PriceModifier(234-244)packages/lib/pricing/calculator.ts (2)
createPricingContext(353-369)calculateVariablePrice(107-126)
packages/lib/pricing/demo.ts (1)
packages/lib/pricing/types.ts (1)
VariablePricingConfig(280-289)
packages/trpc/server/routers/viewer/payments/_router.ts (1)
packages/trpc/server/routers/viewer/payments/stripeVariablePricing.ts (1)
stripeVariablePricingRouter(31-116)
packages/lib/pricing/playground.ts (1)
packages/lib/pricing/types.ts (1)
VariablePricingConfig(280-289)
apps/web/components/booking/VariablePricingPayment.tsx (2)
packages/trpc/react/trpc.ts (1)
trpc(54-138)packages/ui/components/button/Button.tsx (1)
Button(221-349)
packages/app-store/stripepayment/lib/variablePricing.ts (1)
packages/lib/pricing/types.ts (1)
PriceCalculationResult(350-361)
packages/trpc/server/routers/viewer/payments/stripeVariablePricing.ts (5)
packages/trpc/server/trpc.ts (1)
router(13-13)packages/lib/pricing/utils.ts (1)
getVariablePricingConfig(26-59)packages/lib/pricing/types.ts (1)
PricingContext(295-308)packages/lib/pricing/calculator.ts (1)
calculateVariablePrice(107-126)packages/app-store/stripepayment/lib/variablePricing.ts (2)
getOrCreateStripePrice(71-101)generatePaymentMetadata(106-122)
packages/trpc/server/routers/viewer/eventTypes/_router.ts (3)
packages/trpc/server/trpc.ts (1)
router(13-13)packages/trpc/server/routers/viewer/eventTypes/heavy/_router.ts (1)
eventTypesRouter(20-45)packages/trpc/server/routers/viewer/eventTypes/variablePricing.ts (1)
variablePricingRouter(171-332)
apps/web/components/eventtype/VariablePricingModal.tsx (2)
packages/lib/pricing/types.ts (1)
PricingRule(92-119)packages/trpc/react/trpc.ts (1)
trpc(54-138)
packages/lib/pricing/calculator.ts (1)
packages/lib/pricing/types.ts (11)
PriceCalculationResult(350-361)VariablePricingConfig(280-289)PricingContext(295-308)PriceBreakdownItem(314-323)PricingRule(92-119)DurationCondition(131-136)TimeOfDayCondition(158-163)DayOfWeekCondition(182-185)BulkPricingRequest(429-440)BulkPricingResult(445-455)EnhancedPricingContext(413-424)
packages/lib/pricing/utils.ts (2)
packages/lib/pricing/index.ts (8)
DEFAULT_VARIABLE_PRICING_CONFIG(20-20)getVariablePricingConfig(13-13)setVariablePricingConfig(14-14)createPricingRule(15-15)createPriceModifier(16-16)validateVariablePricingConfig(17-17)isVariablePricingEnabled(18-18)getPricingRulesSummary(19-19)packages/lib/pricing/types.ts (7)
VariablePricingConfig(280-289)PricingRule(92-119)DurationCondition(131-136)TimeOfDayCondition(158-163)DayOfWeekCondition(182-185)PriceModifier(234-244)PriceCalculationResult(350-361)
⏰ 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). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (7)
packages/trpc/server/routers/viewer/payments/_router.ts (2)
4-4: LGTM: modular Stripe variable pricing router import.
30-31: Stripe price creation: ensure idempotency and up-to-date API version.Confirm
getOrCreateStripePriceguarantees de-duplication under concurrent calls and uses idempotency keys where creation is unavoidable. Also ensure the Stripe client in stripeVariablePricing.ts targets a current API version to avoid behavioral drift.packages/trpc/server/routers/viewer/eventTypes/_router.ts (2)
32-164: Router split looks good.The base/router merge keeps concerns separated and is consistent with existing patterns.
14-15: Consistent exposure point for variable pricing.Importing
variablePricingRouterhere centralizes event-type pricing ops. Ensure its Prisma calls continue usingselect(notinclude) and never exposecredential.keyfields. Current snippets adhere to this.turbo.json (1)
185-186: Guard STRIPE_SECRET_KEY from accidental client exposure.turbo.json lists STRIPE_SECRET_KEY and STRIPE_WEBHOOK_SECRET in globalEnv — globalEnv is provided to all Turbo tasks (including web builds). Move server-only secrets to task-scoped envs and ensure no client bundle references them.
Run this locally to confirm no client usage:
rg -n --hidden -S \ -g '!.git' -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/.next/**' -g '!**/build/**' \ "process\.env\.(STRIPE_SECRET_KEY|STRIPE_WEBHOOK_SECRET)\b|process\.env\[(?:'STRIPE_SECRET_KEY'|'STRIPE_WEBHOOK_SECRET'|\"STRIPE_SECRET_KEY\"|\"STRIPE_WEBHOOK_SECRET\")\]|import\.meta\.env.*(STRIPE_SECRET_KEY|STRIPE_WEBHOOK_SECRET)" || trueapps/web/components/booking/VariablePricingPayment.tsx (1)
34-45: No secret leakage — eventTypes.get only returns parsed metadata (no credential.key).trpc.viewer.eventTypes.get routes to packages/trpc/server/routers/viewer/eventTypes/get.handler.ts which calls getEventTypeById (packages/lib/event-types/getEventTypeById.ts). getEventTypeById parses and returns eventType.metadata.apps (e.g. stripe_user_id) but does not surface credential.key. credential.key is only referenced in credentials/app handlers (e.g. packages/trpc/server/routers/viewer/apps/updateAppCredentials.handler.ts).
packages/lib/pricing/types.ts (1)
142-156: Example consistency with validator (HH:mm).Examples already show zero‑padded times; keep enforcing HH:mm (paired with utils.ts regex fix).
Please ensure any UI inputs also enforce HH:mm to avoid invalid configs slipping through.
Also applies to: 150-156
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
turbo.json (1)
90-91: Deduplicate env keys to reduce noise and cache churn.Duplicates increase maintenance cost and can cause confusing diffs.
Apply this cleanup:
- "INTERCOM_SECRET", - "INTERCOM_SECRET", + "INTERCOM_SECRET", - "ORG_MONTHLY_CREDITS", ... - "ORG_MONTHLY_CREDITS", + "ORG_MONTHLY_CREDITS", - "VAPID_PRIVATE_KEY", ... - "VAPID_PRIVATE_KEY", + "VAPID_PRIVATE_KEY", "NEXT_PUBLIC_SENTRY_DSN_CLIENT", ... - "NEXT_PUBLIC_SENTRY_DSN_CLIENT",Also applies to: 123-123, 188-188, 250-250, 256-256, 312-312, 338-338
📜 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)
turbo.json(2 hunks)
🔇 Additional comments (1)
turbo.json (1)
366-369: Limit Stripe/NEXTAUTH secrets to runtime, not build.rg returned "No such file or directory" for apps/api-v2 and packages/api-v2, so I couldn't verify whether the build reads these env vars.
File: turbo.json Lines: 366-369
If build does not depend on them, remove:
"LOG_LEVEL", - "NEXTAUTH_SECRET", - "STRIPE_SECRET_KEY", - "STRIPE_WEBHOOK_SECRET" + "LOG_LEVEL"Verify by running from the repo root:
rg -nP -C3 '(process\.env\.(STRIPE_SECRET_KEY|STRIPE_WEBHOOK_SECRET|NEXTAUTH_SECRET))' -S
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/trpc/server/routers/viewer/eventTypes/variablePricing.ts (1)
1-360: Remove unused duplicate router: packages/trpc/server/routers/viewer/eventTypes/variablePricing.tsVerified: variablePricingRouter is not merged into eventTypesRouter and has no imports; pricing.router.ts + pricing.procedures.ts are the canonical implementation. Delete the file to avoid API/type drift — it diverges in schemas (priceModifier vs modifier), return shape (modifiers vs appliedRules/breakdown.label), and validation/access handling.
If kept temporarily, at minimum:
- Use the validation result from validateVariablePricingConfig (don’t ignore it).
- Make the calculate endpoint public if price calculation should be callable without auth.
- Align response shape and schema names to the canonical PriceCalculationResult (appliedRules + breakdown.label) and the existing modifier schema.
♻️ Duplicate comments (6)
turbo.json (2)
343-346: Remove Stripe server secrets from the web build env.Leaking STRIPE_SECRET_KEY/STRIPE_WEBHOOK_SECRET into the Next.js web build is a high‑risk footgun. Keep these server‑only on API/server tasks. This was previously flagged.
Apply:
"NEXT_PUBLIC_PLAIN_CHAT_EXCLUDED_PATHS", - "STRIPE_SECRET_KEY", - "STRIPE_WEBHOOK_SECRET" + // Server-only; do not expose in web build + // "STRIPE_SECRET_KEY", + // "STRIPE_WEBHOOK_SECRET"
277-279: Do not include STRIPE_SECRET_KEY in globalEnv.globalEnv influences cache keys and increases secret sprawl. Restrict secrets to server tasks only.
- "NEXT_PUBLIC_INTERCOM_APP_ID", - "STRIPE_SECRET_KEY" + "NEXT_PUBLIC_INTERCOM_APP_ID"packages/trpc/server/routers/viewer/eventTypes/_router.ts (1)
166-172: Make price calculation public for unauthenticated bookers.The merged pricing.calculate currently uses authedProcedure in pricing.procedures.ts, blocking public booking flows. Switch that procedure to publicProcedure and drop access checks for read‑only fields. Previously flagged.
#!/bin/bash # Verify calculate is public after changes rg -nP 'export\s+const\s+calculate\s*=\s*publicProcedure' packages/trpc/server/routers/viewer/eventTypes/pricing.procedures.tspackages/trpc/server/routers/viewer/eventTypes/variablePricing.ts (2)
237-243: Use validation result; it doesn’t throw.Currently ignored; throw when invalid.
- // Validate the pricing config - validateVariablePricingConfig(input.pricingConfig); + const validation = validateVariablePricingConfig(input.pricingConfig); + if (!validation.isValid) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: validation.errors.join("; "), + }); + }
289-353: Make calculatePrice a public procedure and align response shape.This endpoint is read‑only and should be callable by unauthenticated bookers. Also align fallback return with the shared shape.
-import authedProcedure from "../../../procedures/authedProcedure"; +import authedProcedure from "../../../procedures/authedProcedure"; +import publicProcedure from "../../../procedures/publicProcedure"; @@ - calculatePrice: authedProcedure.input(calculatePriceInputSchema).query(async ({ ctx, input }) => { + calculatePrice: publicProcedure.input(calculatePriceInputSchema).query(async ({ ctx, input }) => { @@ - return { - basePrice: eventType.price, - currency: eventType.currency, - totalPrice: eventType.price, - modifiers: [] as PriceModifier[], - breakdown: [ - { - description: "Base price", - amount: eventType.price, - type: "base" as const, - }, - ], - }; + return { + basePrice: variablePricing.basePrice, + totalPrice: variablePricing.basePrice, + currency: variablePricing.currency, + appliedRules: [], + breakdown: [ + { label: "Base price", type: "base" as const, amount: variablePricing.basePrice }, + ], + };packages/lib/pricing/utils.ts (1)
325-328: Enforce zero‑padded HH:mm.Regex currently allows “9:00”; breaks string comparisons.
-function isValidTimeFormat(time: string): boolean { - const timeRegex = /^([01]?[0-9]|2[0-3]):[0-5][0-9]$/; +function isValidTimeFormat(time: string): boolean { + const timeRegex = /^(0[0-9]|1[0-9]|2[0-3]):[0-5][0-9]$/; return timeRegex.test(time); }
🧹 Nitpick comments (10)
turbo.json (1)
89-90: Deduplicate env entries to avoid confusion and drift.INTERCOM_SECRET, ORG_MONTHLY_CREDITS, and VAPID_PRIVATE_KEY appear twice.
- "INTERCOM_SECRET", - "INTERCOM_SECRET", + "INTERCOM_SECRET", @@ - "ORG_MONTHLY_CREDITS", + "ORG_MONTHLY_CREDITS", @@ - "ORG_MONTHLY_CREDITS", + // (duplicate removed) @@ - "VAPID_PRIVATE_KEY", + "VAPID_PRIVATE_KEY", @@ - "VAPID_PRIVATE_KEY", + // (duplicate removed)Also applies to: 123-124, 188-189, 249-257
packages/trpc/server/routers/viewer/eventTypes/pricing.router.ts (1)
1-8: Consolidate to a single canonical pricing router — keep pricing.router.ts + pricing.procedures.ts; remove duplicates.pricing.ts and variablePricing.ts define overlapping routers/procedure exports; remove/merge them and update callers to import from the canonical files.
Key locations to act on:
- packages/trpc/server/routers/viewer/eventTypes/pricing.ts
- packages/trpc/server/routers/viewer/eventTypes/variablePricing.ts — remove variablePricingRouter and the procedure proxy exports at lines 357–359 (getPricingRules / updatePricingRules / calculatePrice).
packages/trpc/server/routers/viewer/eventTypes/pricing.ts (3)
72-91: Trim unused selected fields.
roleisn’t used. Remove to cut payload/CPU in hot paths.team: { select: { members: { select: { userId: true, - role: true, }, where: { userId: ctx.user.id, }, }, }, },
157-169: Type safety gap:input.pricingConfigstill may not beVariablePricingConfig.Even with Zod, keep a defensive cast or adapter before validation to avoid structural drift across modules.
-const validationResult = validateVariablePricingConfig(input.pricingConfig); +// Optionally adapt unknown client shapes to internal VariablePricingConfig here. +const validationResult = validateVariablePricingConfig(input.pricingConfig as unknown as VariablePricingConfig);Or better: introduce a shared Zod schema in
packages/lib/pricingand import it here to eliminate divergence.
188-212: Potential metadata overwrite race.Two concurrent updates could stomp unrelated metadata keys. Prefer atomic JSON merge at field path if supported, or re-read/merge-with-latest inside a transaction.
packages/lib/pricing/utils.ts (5)
365-381: Currency conversion should case-normalize codes.Indexing
conversionRateswith lowercase inputs returns undefined.-export function convertPrice(amount: number, fromCurrency: string, toCurrency: string): number { +export function convertPrice(amount: number, fromCurrency: string, toCurrency: string): number { // ... - if (fromCurrency === toCurrency) { + const from = fromCurrency.toUpperCase(); + const to = toCurrency.toUpperCase(); + if (from === to) { return amount; } // Placeholder conversion rates (would come from live service) const conversionRates: Record<string, Record<string, number>> = { USD: { EUR: 0.85, GBP: 0.75, CAD: 1.25, AUD: 1.35 }, EUR: { USD: 1.18, GBP: 0.88, CAD: 1.47, AUD: 1.59 }, GBP: { USD: 1.33, EUR: 1.14, CAD: 1.67, AUD: 1.8 }, }; - - const rate = conversionRates[fromCurrency]?.[toCurrency]; + const rate = conversionRates[from]?.[to]; return rate ? Math.round(amount * rate) : amount; }
551-561: Overnight ranges always “overlap”.Returning
truefor any overnight input inflates conflict signals and can hide real non-overlaps.Implement normalized 0–1440 minute ranges with wrap-around comparison, or split overnight into two segments and test both.
569-572: Guard against malformed time strings.
Number("foo")yieldsNaN.function timeToMinutes(time: string): number { - const [hours, minutes] = time.split(":").map(Number); - return hours * 60 + minutes; + const [h, m] = time.split(":"); + const hours = Number(h); + const minutes = Number(m); + if (!Number.isFinite(hours) || !Number.isFinite(minutes)) return NaN; + return hours * 60 + minutes; }
638-683: Median calculation for even N is off.Currently picks lower middle; average the two middles.
- const medianPrice = prices[Math.floor(prices.length / 2)]; + const mid = Math.floor(prices.length / 2); + const medianPrice = + prices.length % 2 === 0 ? Math.round((prices[mid - 1] + prices[mid]) / 2) : prices[mid];
482-534: Conflict detection heuristics are helpful, butJSON.stringifyfor condition equality is brittle.Equivalent objects with different key orders won’t match.
Use a stable key order or structural/semantic comparison per condition type.
📜 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 (9)
packages/app-store/stripepayment/lib/variablePricing.ts(1 hunks)packages/lib/pricing/utils.ts(1 hunks)packages/trpc/server/routers/viewer/eventTypes/_router.ts(3 hunks)packages/trpc/server/routers/viewer/eventTypes/pricing.procedures.ts(1 hunks)packages/trpc/server/routers/viewer/eventTypes/pricing.router.ts(1 hunks)packages/trpc/server/routers/viewer/eventTypes/pricing.ts(1 hunks)packages/trpc/server/routers/viewer/eventTypes/variablePricing.ts(1 hunks)packages/trpc/server/routers/viewer/payments/stripeVariablePricing.ts(1 hunks)turbo.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/app-store/stripepayment/lib/variablePricing.ts
- packages/trpc/server/routers/viewer/payments/stripeVariablePricing.ts
🧰 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/trpc/server/routers/viewer/eventTypes/pricing.router.tspackages/trpc/server/routers/viewer/eventTypes/pricing.procedures.tspackages/trpc/server/routers/viewer/eventTypes/pricing.tspackages/trpc/server/routers/viewer/eventTypes/variablePricing.tspackages/trpc/server/routers/viewer/eventTypes/_router.tspackages/lib/pricing/utils.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/trpc/server/routers/viewer/eventTypes/pricing.router.tspackages/trpc/server/routers/viewer/eventTypes/pricing.procedures.tspackages/trpc/server/routers/viewer/eventTypes/pricing.tspackages/trpc/server/routers/viewer/eventTypes/variablePricing.tspackages/trpc/server/routers/viewer/eventTypes/_router.tspackages/lib/pricing/utils.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/trpc/server/routers/viewer/eventTypes/pricing.router.tspackages/trpc/server/routers/viewer/eventTypes/pricing.procedures.tspackages/trpc/server/routers/viewer/eventTypes/pricing.tspackages/trpc/server/routers/viewer/eventTypes/variablePricing.tspackages/trpc/server/routers/viewer/eventTypes/_router.tspackages/lib/pricing/utils.ts
🧠 Learnings (1)
📚 Learning: 2025-08-26T08:08:23.395Z
Learnt from: SinghaAnirban005
PR: calcom/cal.com#23343
File: packages/features/insights/server/trpc-router.ts:1080-1101
Timestamp: 2025-08-26T08:08:23.395Z
Learning: In packages/features/insights/server/trpc-router.ts, when filtering personal event types (userId provided, no teamId, not isAll), the query correctly uses user.id (authenticated user) instead of the input userId parameter for security reasons. This prevents users from accessing other users' personal event types by passing arbitrary user IDs.
Applied to files:
packages/trpc/server/routers/viewer/eventTypes/pricing.ts
🧬 Code graph analysis (6)
packages/trpc/server/routers/viewer/eventTypes/pricing.router.ts (2)
packages/trpc/server/routers/viewer/eventTypes/pricing.ts (1)
pricingRouter(113-304)packages/trpc/server/trpc.ts (1)
router(13-13)
packages/trpc/server/routers/viewer/eventTypes/pricing.procedures.ts (1)
packages/lib/pricing/utils.ts (2)
getVariablePricingConfig(34-67)validateVariablePricingConfig(161-193)
packages/trpc/server/routers/viewer/eventTypes/pricing.ts (1)
packages/lib/pricing/utils.ts (2)
getVariablePricingConfig(34-67)validateVariablePricingConfig(161-193)
packages/trpc/server/routers/viewer/eventTypes/variablePricing.ts (3)
packages/lib/pricing/utils.ts (2)
getVariablePricingConfig(34-67)validateVariablePricingConfig(161-193)packages/lib/pricing/types.ts (1)
PriceModifier(234-244)packages/lib/pricing/calculator.ts (2)
createPricingContext(353-369)calculateVariablePrice(107-126)
packages/trpc/server/routers/viewer/eventTypes/_router.ts (2)
packages/trpc/server/trpc.ts (2)
router(13-13)mergeRouters(14-14)packages/trpc/server/routers/viewer/eventTypes/pricing.router.ts (1)
pricingRouter(4-8)
packages/lib/pricing/utils.ts (1)
packages/lib/pricing/types.ts (7)
VariablePricingConfig(280-289)PricingRule(92-119)DurationCondition(131-136)TimeOfDayCondition(158-163)DayOfWeekCondition(182-185)PriceModifier(234-244)PriceCalculationResult(350-361)
🔇 Additional comments (11)
packages/trpc/server/routers/viewer/eventTypes/pricing.router.ts (1)
1-8: Router wiring looks good.Straightforward aggregation of procedures.
packages/trpc/server/routers/viewer/eventTypes/pricing.procedures.ts (3)
117-149: Good: select-only Prisma reads and minimal fields.Adheres to our “select-only” guideline; no credential fields exposed.
154-234: Validation handled correctly.Using validateVariablePricingConfig and returning BAD_REQUEST on failure is correct.
243-301: Return shape consistency and caching usage look good.Matches PriceCalculationResult pattern and relies on calculator/cache.
turbo.json (1)
367-370: Server tasks: keep Stripe secrets here (OK).Verified — ripgrep output shows usages only in server-side code and tests (e.g. apps/web/pages/api/integrations/subscriptions/webhook.ts; packages/features/ee/payments/api/webhook.ts; packages/trpc/server/routers/viewer/payments/stripeVariablePricing.ts; apps/web/playwright/fixtures/users.ts; apps/web/test/utils/bookingScenario/setupAndTeardown.ts). No client-bundle references outside pages/api detected.
packages/trpc/server/routers/viewer/eventTypes/pricing.ts (2)
291-298: Timezone/Date parsing gotcha.
new Date(iso)treats naive strings as local time. Ensure inputs include an offset orZ, or parse in the provided IANA timezone insidecreatePricingContext.Would you confirm
createPricingContextparses usingtimezone(not JS local time) so time-of-day rules evaluate correctly across DST?
112-153: Router structure is good overall.
- Uses
select(notinclude) and returns only the needed pricing config/calculation.- Errors map to appropriate TRPC codes.
Please add tests for:
- dayOfWeek rules with multiple days
- timeOfDay rules around midnight and DST boundaries
- conflicting rules priority resolution
Also applies to: 237-304
packages/lib/pricing/utils.ts (4)
31-67: getVariablePricingConfig looks good; sensible fallback to legacy fields.
409-421: Supported currencies utilities are fine, but consider sourcing from a single authoritative list.[ suggest_optional_refactor ]
Also applies to: 426-429
434-445: Config cloning/merging utilities look solid.Also applies to: 450-461, 466-477
237-252: Centralize PriceModifier Zod schema in packages/lib/pricingRouter code is validating priceModifier with a local schema while the canonical PriceModifier type and helpers live in packages/lib/pricing — export a single Zod schema from packages/lib/pricing and import it in the router/UI to avoid contract drift.
Relevant locations: packages/lib/pricing/types.ts, packages/lib/pricing/utils.ts, packages/trpc/server/routers/viewer/eventTypes/variablePricing.ts, apps/web/components/eventtype/VariablePricingModal.tsx
| function validatePriceModifier(modifier: PriceModifier): string[] { | ||
| const errors: string[] = []; | ||
|
|
||
| if (!["absolute", "surcharge", "discount"].includes(modifier.type)) { | ||
| errors.push("Invalid modifier type"); | ||
| } | ||
|
|
||
| if (modifier.value < 0) { | ||
| errors.push("Modifier value must be non-negative"); | ||
| } | ||
|
|
||
| if (modifier.percentage !== undefined) { | ||
| if (modifier.percentage < 0 || modifier.percentage > 100) { | ||
| errors.push("Modifier percentage must be between 0 and 100"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Modifier type mismatch vs router/docs; align PriceModifier.type.
Validator only accepts absolute/surcharge/discount, while router schema and PR objectives mention percentage/fixed modifiers. Unify across types, calculator, validators, and API.
Option A (prefer): keep priceModifier: { type: "fixed" | "percentage"; ... } and map "percentage" precedence over value.
-function validatePriceModifier(modifier: PriceModifier): string[] {
+function validatePriceModifier(modifier: PriceModifier): string[] {
const errors: string[] = [];
-
- if (!["absolute", "surcharge", "discount"].includes(modifier.type)) {
+ if (!["fixed", "percentage"].includes(modifier.type)) {
errors.push("Invalid modifier type");
}Option B: accept both for backward compat and normalize internally.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function validatePriceModifier(modifier: PriceModifier): string[] { | |
| const errors: string[] = []; | |
| if (!["absolute", "surcharge", "discount"].includes(modifier.type)) { | |
| errors.push("Invalid modifier type"); | |
| } | |
| if (modifier.value < 0) { | |
| errors.push("Modifier value must be non-negative"); | |
| } | |
| if (modifier.percentage !== undefined) { | |
| if (modifier.percentage < 0 || modifier.percentage > 100) { | |
| errors.push("Modifier percentage must be between 0 and 100"); | |
| } | |
| } | |
| function validatePriceModifier(modifier: PriceModifier): string[] { | |
| const errors: string[] = []; | |
| if (!["fixed", "percentage"].includes(modifier.type)) { | |
| errors.push("Invalid modifier type"); | |
| } | |
| if (modifier.value < 0) { | |
| errors.push("Modifier value must be non-negative"); | |
| } | |
| if (modifier.percentage !== undefined) { | |
| if (modifier.percentage < 0 || modifier.percentage > 100) { | |
| errors.push("Modifier percentage must be between 0 and 100"); | |
| } | |
| } | |
| return errors; | |
| } |
🤖 Prompt for AI Agents
In packages/lib/pricing/utils.ts around lines 237-252, the validator currently
only accepts modifier.type values "absolute", "surcharge", "discount" which
conflicts with the router/schema and PR intent to use "fixed" and "percentage".
Update the validator to accept type = "fixed" | "percentage" (preferred) and
enforce semantic rules: when type === "percentage" validate percentage between 0
and 100 and ignore/optional value, when type === "fixed" validate value is
non-negative and ignore/optional percentage; adjust error messages accordingly.
Also add a normalization step (or note callers) to map or normalize legacy types
if present (backwards-compatibility optional) so the calculator and API receive
the unified shape.
| export const calculate = authedProcedure.input(calculatePriceInputSchema).query(async ({ ctx, input }) => { | ||
| await verifyEventTypeAccess(ctx, input.eventTypeId); | ||
|
|
||
| // Get the event type |
There was a problem hiding this comment.
Make calculate public; remove auth check (read‑only access).
Booking UIs must price without login. Limit selection to public fields (already done) and drop access verification.
-import authedProcedure from "../../../procedures/authedProcedure";
+import authedProcedure from "../../../procedures/authedProcedure";
+import publicProcedure from "../../../procedures/publicProcedure";
@@
-export const calculate = authedProcedure.input(calculatePriceInputSchema).query(async ({ ctx, input }) => {
- await verifyEventTypeAccess(ctx, input.eventTypeId);
+export const calculate = publicProcedure.input(calculatePriceInputSchema).query(async ({ ctx, input }) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const calculate = authedProcedure.input(calculatePriceInputSchema).query(async ({ ctx, input }) => { | |
| await verifyEventTypeAccess(ctx, input.eventTypeId); | |
| // Get the event type | |
| import authedProcedure from "../../../procedures/authedProcedure"; | |
| import publicProcedure from "../../../procedures/publicProcedure"; | |
| export const calculate = publicProcedure.input(calculatePriceInputSchema).query(async ({ ctx, input }) => { | |
| // Get the event type |
🤖 Prompt for AI Agents
In packages/trpc/server/routers/viewer/eventTypes/pricing.procedures.ts around
lines 239-242, the calculate procedure is currently protected and calls
verifyEventTypeAccess; change it to be public/read‑only by replacing
authedProcedure with the public (unauthenticated) procedure factory and remove
the await verifyEventTypeAccess(ctx, input.eventTypeId) line; ensure any imports
are updated (remove unused verifyEventTypeAccess import and add/import
publicProcedure if not present) and keep the existing input schema and
public-field restrictions intact.
|
|
||
| import { calculateVariablePrice, createPricingContext } from "@calcom/lib/pricing/calculator"; | ||
| import { getVariablePricingConfig, validateVariablePricingConfig } from "@calcom/lib/pricing/utils"; | ||
| import type { prisma } from "@calcom/prisma"; |
There was a problem hiding this comment.
Fix Prisma typing: typeof prisma used with a type-only import will not compile.
Import PrismaClient type and use that for ctx.prisma.
-import type { prisma } from "@calcom/prisma";
+import type { PrismaClient } from "@prisma/client";
...
-async function verifyEventTypeAccess(
- ctx: { prisma: typeof prisma; user: TrpcSessionUser },
+async function verifyEventTypeAccess(
+ ctx: { prisma: PrismaClient; user: TrpcSessionUser },
eventTypeId: number
) {Also applies to: 61-64
🤖 Prompt for AI Agents
In packages/trpc/server/routers/viewer/eventTypes/pricing.ts around lines 5 and
again near 61-64, the code imports "prisma" as a type (import type { prisma }
from "@calcom/prisma") and then uses typeof prisma which won't compile because
type-only imports cannot be used with typeof; replace that import with the
PrismaClient type (import type { PrismaClient } from "@prisma/client" or the
project's Prisma client type) and update any ctx.prisma typings to use
PrismaClient (or the correct exported client type) instead of typeof prisma;
ensure all occurrences (including lines 61-64) reference PrismaClient in
parameter and variable type annotations.
| const updateVariablePricingInputSchema = z.object({ | ||
| eventTypeId: z.number().int(), | ||
| pricingConfig: z.object({ | ||
| enabled: z.boolean(), | ||
| basePrice: z.number().min(0), | ||
| currency: z.string(), | ||
| rules: z.array( | ||
| z.object({ | ||
| id: z.string(), | ||
| type: z.enum(["duration", "timeOfDay", "dayOfWeek", "custom"]), | ||
| enabled: z.boolean(), | ||
| priority: z.number().int(), | ||
| modifier: z.object({ | ||
| type: z.enum(["percentage", "fixed"]), | ||
| value: z.number(), | ||
| }), | ||
| description: z.string(), | ||
| condition: z.object({ | ||
| // Duration condition | ||
| minDuration: z.number().int().min(1).optional(), | ||
| maxDuration: z.number().int().min(1).optional(), | ||
| // Time of day condition | ||
| startTime: z.string().optional(), | ||
| endTime: z.string().optional(), | ||
| // Day of week condition | ||
| days: z.array(z.number().int().min(0).max(6)).optional(), | ||
| }), | ||
| }) | ||
| ), | ||
| }), | ||
| }); |
There was a problem hiding this comment.
Schema doesn’t match domain types; will throw at runtime and/or fail validation.
- Uses
modifierinstead ofpriceModifier. dayOfWeek.condition.daysisnumber[]but utils/types expect string day names;validateRuleConditioncallstoLowerCase()and will crash on numbers.timeOfDay.conditionfields are optional but validator expects HH:mm strings.currencyis unconstrained.
Fix the schema to align with PricingRule/PriceModifier and validators.
-const updateVariablePricingInputSchema = z.object({
+const updateVariablePricingInputSchema = z.object({
eventTypeId: z.number().int(),
pricingConfig: z.object({
enabled: z.boolean(),
basePrice: z.number().min(0),
- currency: z.string(),
- rules: z.array(
- z.object({
- id: z.string(),
- type: z.enum(["duration", "timeOfDay", "dayOfWeek", "custom"]),
- enabled: z.boolean(),
- priority: z.number().int(),
- modifier: z.object({
- type: z.enum(["percentage", "fixed"]),
- value: z.number(),
- }),
- description: z.string(),
- condition: z.object({
- // Duration condition
- minDuration: z.number().int().min(1).optional(),
- maxDuration: z.number().int().min(1).optional(),
- // Time of day condition
- startTime: z.string().optional(),
- endTime: z.string().optional(),
- // Day of week condition
- days: z.array(z.number().int().min(0).max(6)).optional(),
- }),
- })
- ),
+ currency: z.string().transform((s) => s.toUpperCase()).regex(/^[A-Z]{3}$/),
+ rules: z.array(
+ z.discriminatedUnion("type", [
+ z.object({
+ id: z.string(),
+ type: z.literal("duration"),
+ enabled: z.boolean(),
+ priority: z.number().int().min(0).max(100).optional(),
+ description: z.string(),
+ condition: z.object({
+ minDuration: z.number().int().min(1).optional(),
+ maxDuration: z.number().int().min(1).optional(),
+ }),
+ price: z.number().int().min(0).optional(),
+ priceModifier: z.object({
+ type: z.enum(["absolute","surcharge","discount"]),
+ value: z.number().int().min(0),
+ percentage: z.number().min(0).max(100).optional(),
+ }).optional(),
+ }),
+ z.object({
+ id: z.string(),
+ type: z.literal("timeOfDay"),
+ enabled: z.boolean(),
+ priority: z.number().int().min(0).max(100).optional(),
+ description: z.string(),
+ condition: z.object({
+ startTime: z.string().regex(/^(0[0-9]|1[0-9]|2[0-3]):[0-5][0-9]$/),
+ endTime: z.string().regex(/^(0[0-9]|1[0-9]|2[0-3]):[0-5][0-9]$/),
+ }),
+ price: z.number().int().min(0).optional(),
+ priceModifier: z.object({
+ type: z.enum(["absolute","surcharge","discount"]),
+ value: z.number().int().min(0),
+ percentage: z.number().min(0).max(100).optional(),
+ }).optional(),
+ }),
+ z.object({
+ id: z.string(),
+ type: z.literal("dayOfWeek"),
+ enabled: z.boolean(),
+ priority: z.number().int().min(0).max(100).optional(),
+ description: z.string(),
+ condition: z.object({
+ days: z.array(
+ z.enum(["monday","tuesday","wednesday","thursday","friday","saturday","sunday"])
+ ).min(1),
+ }),
+ price: z.number().int().min(0).optional(),
+ priceModifier: z.object({
+ type: z.enum(["absolute","surcharge","discount"]),
+ value: z.number().int().min(0),
+ percentage: z.number().min(0).max(100).optional(),
+ }).optional(),
+ }),
+ z.object({
+ id: z.string(),
+ type: z.literal("custom"),
+ enabled: z.boolean(),
+ priority: z.number().int().min(0).max(100).optional(),
+ description: z.string(),
+ condition: z.record(z.unknown()),
+ price: z.number().int().min(0).optional(),
+ priceModifier: z.object({
+ type: z.enum(["absolute","surcharge","discount"]),
+ value: z.number().int().min(0),
+ percentage: z.number().min(0).max(100).optional(),
+ }).optional(),
+ }),
+ ])
+ ),
}),
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const updateVariablePricingInputSchema = z.object({ | |
| eventTypeId: z.number().int(), | |
| pricingConfig: z.object({ | |
| enabled: z.boolean(), | |
| basePrice: z.number().min(0), | |
| currency: z.string(), | |
| rules: z.array( | |
| z.object({ | |
| id: z.string(), | |
| type: z.enum(["duration", "timeOfDay", "dayOfWeek", "custom"]), | |
| enabled: z.boolean(), | |
| priority: z.number().int(), | |
| modifier: z.object({ | |
| type: z.enum(["percentage", "fixed"]), | |
| value: z.number(), | |
| }), | |
| description: z.string(), | |
| condition: z.object({ | |
| // Duration condition | |
| minDuration: z.number().int().min(1).optional(), | |
| maxDuration: z.number().int().min(1).optional(), | |
| // Time of day condition | |
| startTime: z.string().optional(), | |
| endTime: z.string().optional(), | |
| // Day of week condition | |
| days: z.array(z.number().int().min(0).max(6)).optional(), | |
| }), | |
| }) | |
| ), | |
| }), | |
| }); | |
| const updateVariablePricingInputSchema = z.object({ | |
| eventTypeId: z.number().int(), | |
| pricingConfig: z.object({ | |
| enabled: z.boolean(), | |
| basePrice: z.number().min(0), | |
| currency: z.string().transform((s) => s.toUpperCase()).regex(/^[A-Z]{3}$/), | |
| rules: z.array( | |
| z.discriminatedUnion("type", [ | |
| z.object({ | |
| id: z.string(), | |
| type: z.literal("duration"), | |
| enabled: z.boolean(), | |
| priority: z.number().int().min(0).max(100).optional(), | |
| description: z.string(), | |
| condition: z.object({ | |
| minDuration: z.number().int().min(1).optional(), | |
| maxDuration: z.number().int().min(1).optional(), | |
| }), | |
| price: z.number().int().min(0).optional(), | |
| priceModifier: z | |
| .object({ | |
| type: z.enum(["absolute", "surcharge", "discount"]), | |
| value: z.number().int().min(0), | |
| percentage: z.number().min(0).max(100).optional(), | |
| }) | |
| .optional(), | |
| }), | |
| z.object({ | |
| id: z.string(), | |
| type: z.literal("timeOfDay"), | |
| enabled: z.boolean(), | |
| priority: z.number().int().min(0).max(100).optional(), | |
| description: z.string(), | |
| condition: z.object({ | |
| startTime: z.string().regex(/^(0[0-9]|1[0-9]|2[0-3]):[0-5][0-9]$/), | |
| endTime: z.string().regex(/^(0[0-9]|1[0-9]|2[0-3]):[0-5][0-9]$/), | |
| }), | |
| price: z.number().int().min(0).optional(), | |
| priceModifier: z | |
| .object({ | |
| type: z.enum(["absolute", "surcharge", "discount"]), | |
| value: z.number().int().min(0), | |
| percentage: z.number().min(0).max(100).optional(), | |
| }) | |
| .optional(), | |
| }), | |
| z.object({ | |
| id: z.string(), | |
| type: z.literal("dayOfWeek"), | |
| enabled: z.boolean(), | |
| priority: z.number().int().min(0).max(100).optional(), | |
| description: z.string(), | |
| condition: z.object({ | |
| days: z | |
| .array( | |
| z.enum([ | |
| "monday", | |
| "tuesday", | |
| "wednesday", | |
| "thursday", | |
| "friday", | |
| "saturday", | |
| "sunday", | |
| ]) | |
| ) | |
| .min(1), | |
| }), | |
| price: z.number().int().min(0).optional(), | |
| priceModifier: z | |
| .object({ | |
| type: z.enum(["absolute", "surcharge", "discount"]), | |
| value: z.number().int().min(0), | |
| percentage: z.number().min(0).max(100).optional(), | |
| }) | |
| .optional(), | |
| }), | |
| z.object({ | |
| id: z.string(), | |
| type: z.literal("custom"), | |
| enabled: z.boolean(), | |
| priority: z.number().int().min(0).max(100).optional(), | |
| description: z.string(), | |
| condition: z.record(z.unknown()), | |
| price: z.number().int().min(0).optional(), | |
| priceModifier: z | |
| .object({ | |
| type: z.enum(["absolute", "surcharge", "discount"]), | |
| value: z.number().int().min(0), | |
| percentage: z.number().min(0).max(100).optional(), | |
| }) | |
| .optional(), | |
| }), | |
| ]) | |
| ), | |
| }), | |
| }); |
🤖 Prompt for AI Agents
In packages/trpc/server/routers/viewer/eventTypes/pricing.ts around lines 19–49,
the Zod schema must match the domain PricingRule/PriceModifier shape: rename the
"modifier" field to "priceModifier" to match the domain, change day-of-week
"days" to an array of strings (day names) not numbers and validate them as
lowercase weekday names, enforce startTime/endTime to match HH:mm format (or
make them required only if validator expects them) using a regex or refined
string check, and constrain "currency" to a valid ISO currency code pattern
(e.g. 3-letter uppercase) so validators won’t fail at runtime.
| const calculatePriceInputSchema = z.object({ | ||
| eventTypeId: z.number().int(), | ||
| duration: z.number().int().min(1), | ||
| startTime: z.string(), // ISO date string | ||
| endTime: z.string(), // ISO date string | ||
| timezone: z.string(), | ||
| }); |
There was a problem hiding this comment.
duration is unused; either remove from input or validate against derived duration.
Mismatches cause incorrect rule application around DST.
-const calculatePriceInputSchema = z.object({
+const calculatePriceInputSchema = z.object({
eventTypeId: z.number().int(),
- duration: z.number().int().min(1),
+ duration: z.number().int().min(1),
startTime: z.string(), // ISO date string
endTime: z.string(), // ISO date string
timezone: z.string(),
});
...
-const pricingContext = createPricingContext(
+// Validate duration matches the provided times (in minutes)
+const derivedDuration = Math.round(
+ (new Date(input.endTime).getTime() - new Date(input.startTime).getTime()) / 60000
+);
+if (derivedDuration > 0 && Math.abs(derivedDuration - input.duration) > 1) {
+ throw new TRPCError({ code: "BAD_REQUEST", message: "Duration does not match start/end time" });
+}
+const pricingContext = createPricingContext(
input.eventTypeId,
new Date(input.startTime),
new Date(input.endTime),
input.timezone
);Also applies to: 291-301
| const priceModifierSchema = z.object({ | ||
| type: z.enum(["surcharge", "discount", "absolute"]), | ||
| value: z.number().int().min(0), | ||
| percentage: z.number().min(0).max(100).optional(), | ||
| }); |
There was a problem hiding this comment.
Schema mismatch with shared PriceModifier.
types.ts uses a PriceModifier with precedence rules; elsewhere procedures expect modifier.type ∈ {"percentage","fixed"}. Here you use {"surcharge","discount","absolute"} with non‑negative value. This will drift from calculator logic.
Align to the canonical schema used in pricing.procedures.ts or remove this file.
🤖 Prompt for AI Agents
In packages/trpc/server/routers/viewer/eventTypes/variablePricing.ts around
lines 54 to 58, the local priceModifierSchema uses type values
{"surcharge","discount","absolute"} and non‑negative value semantics which
diverge from the canonical PriceModifier used in pricing.procedures.ts (which
expects types like "percentage" and "fixed" and has precedence rules); replace
this local schema with the canonical one by importing and reusing the shared
PriceModifier schema/type from the central types/pricing module (or mirror it
exactly: type enum ["percentage","fixed"], percentage constrained 0–100 when
applicable, numeric value semantics matching the shared definition and any
precedence/optional fields), or remove this file if unused. Ensure the schema
names and constraints match the shared definition so callers and calculator
logic remain consistent.
…RIPE_SECRET_KEY to turbo.json
2 participants
This establishes the foundation for dynamic pricing in Cal.com bookings.
What does this PR do?
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist