fix: add minimize gaps between bookings option to event types#28797
Open
sudoKrishna wants to merge 2 commits intocalcom:mainfrom
Open
fix: add minimize gaps between bookings option to event types#28797sudoKrishna wants to merge 2 commits intocalcom:mainfrom
sudoKrishna wants to merge 2 commits intocalcom:mainfrom
Conversation
github-actions
bot
added
event-types
Contributor
📝 WalkthroughWalkthroughThe changes add support for a new "minimize gaps" feature for event types and implement credit transfer functionality for team downgrades. The minimize gaps feature includes UI components, database schema changes to store the setting, type definitions, and slot calculation logic that filters available times based on existing bookings when the feature is enabled. Additionally, a new credit service method enables transferring team credits to the team owner, which is integrated into the team downgrade process to handle credit migration. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/features/schedules/lib/slots.ts (1)
256-285:⚠️ Potential issue | 🔴 CriticalForward the new inputs through
getSlots().
minimizeGapsandexistingBookingswere added toGetSlots, but the exported wrapper never destructures or passes them tobuildSlotsWithDateRanges(). Right now the new behavior is dead code.Suggested fix
const getSlots = ({ inviteeDate, frequency, minimumBookingNotice, dateRanges, eventLength, offsetStart = 0, datesOutOfOffice, showOptimizedSlots, + minimizeGaps, + existingBookings, datesOutOfOfficeTimeZone, }: GetSlots): { time: Dayjs; userIds?: number[]; away?: boolean; @@ minimumBookingNotice, offsetStart, datesOutOfOffice, showOptimizedSlots, + minimizeGaps, + existingBookings, datesOutOfOfficeTimeZone, }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/features/schedules/lib/slots.ts` around lines 256 - 285, getSlots is now typed to accept minimizeGaps and existingBookings on GetSlots but the wrapper does not destructure or forward them to buildSlotsWithDateRanges, so the new inputs are unused; update the parameter destructuring in getSlots to include minimizeGaps and existingBookings and pass both as arguments in the call to buildSlotsWithDateRanges({ ... }) (keep existing timeZone: getTimeZone(inviteeDate) and other props) so buildSlotsWithDateRanges receives the new options.packages/trpc/server/routers/viewer/slots/util.ts (1)
1256-1275:⚠️ Potential issue | 🟠 MajorRefresh
currentBookingsAllUsersin the fallback-host recalculation path.When
diff > 0, availability is recomputed with fallback hosts, butcurrentBookingsAllUsersis not reassigned.getSlots({ minimizeGaps, existingBookings })then uses stale bookings from the previous host set, which can incorrectly filter slots.💡 Proposed fix
- ({ allUsersAvailability, usersWithCredentials, currentSeats } = + ({ allUsersAvailability, usersWithCredentials, currentSeats, currentBookingsAllUsers } = await this.calculateHostsAndAvailabilities({ input, eventType, hosts: [...eligibleFallbackRRHosts, ...eligibleFixedHosts], loggerWithEventDetails, startTime, endTime, bypassBusyCalendarTimes, silentCalendarFailures, mode, }));Also applies to: 1327-1339, 1358-1362
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/server/routers/viewer/slots/util.ts` around lines 1256 - 1275, The fallback-host recalculation path calls this.calculateHostsAndAvailabilities(...) but fails to update currentBookingsAllUsers, causing getSlots({ minimizeGaps, existingBookings }) to use stale bookings; update the code so that after each recalculation (the call to calculateHostsAndAvailabilities in the fallback flow) you reassign currentBookingsAllUsers from the returned tuple (alongside allUsersAvailability and usersWithCredentials/currentSeats) before calling getSlots, and apply the same fix to the other recalculation sites that mirror this logic (the other places where calculateHostsAndAvailabilities is invoked in the fallback path).
🧹 Nitpick comments (1)
packages/features/ee/billing/service/teams/TeamBillingService.ts (1)
147-149: UseErrorWithCodefor the missing-owner path.This introduces a plain
Errorin a service class. If this is meant to abort the downgrade, please throwErrorWithCode; if it's only local control flow, log and return early instead.As per coding guidelines,
Use ErrorWithCode for errors in non-tRPC files (services, repositories, utilities); use TRPCError only in tRPC routers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/features/ee/billing/service/teams/TeamBillingService.ts` around lines 147 - 149, Replace the plain Error thrown in TeamBillingService when owner is missing with an ErrorWithCode so the service returns a typed error (e.g., throw new ErrorWithCode with an appropriate error code like "NO_OWNER" and a message including this.team.id); ensure you import ErrorWithCode at the top of the file and keep the same control flow (or, if this is not intended to abort, log and return early from the method instead of throwing). Target the missing-owner branch that checks `if (!owner)` inside the TeamBillingService method and update the throw to use ErrorWithCode with a clear code and message referencing this.team.id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/features/ee/billing/credit-service.ts`:
- Around line 795-839: The current moveCreditsFromTeamToUser reads
teamCreditBalance.additionalCredits then clears it, which allows races; instead
perform an atomic debit on the team row first and capture the previous amount,
then increment the user. Modify moveCreditsFromTeamToUser to replace the
read-then-later update with a single atomic operation (using CreditsRepository
via tx or a tx.$queryRaw SQL UPDATE ... RETURNING previous additionalCredits /
use UPDATE ... WHERE teamId = ? AND additionalCredits > 0 RETURNING
additionalCredits) to set additionalCredits = 0 and return the amount actually
debited, then only if that returned amount > 0 perform the
CreditsRepository.updateCreditBalance call to increment the user by that
returned amount; keep everything inside the same prisma.$transaction.
In `@packages/features/ee/billing/service/teams/TeamBillingService.ts`:
- Around line 138-154: The owner lookup (prisma.membership.findFirst), the
credit transfer (CreditService.moveCreditsFromTeamToUser) and the team billing
metadata update (prisma.team.update) must be executed in a single DB transaction
to avoid partial commits; refactor this code to use a Prisma transaction
(prisma.$transaction) or accept a transactional client: fetch the owner inside
the transaction, call moveCreditsFromTeamToUser with the same transaction
context (or convert that method to accept a Prisma transaction/client), then
perform prisma.team.update within the same transaction so either all succeed or
all roll back; if changing moveCreditsFromTeamToUser is infeasible, implement a
compensating rollback step invoked on prisma.team.update failure to revert the
credit move.
In `@packages/features/eventtypes/lib/types.ts`:
- Line 174: The EventTypeUpdateInput type is missing the minimizeGaps flag
present in FormValues, so update the shared update contract by adding
minimizeGaps?: boolean | null to the EventTypeUpdateInput definition; locate
EventTypeUpdateInput in types.ts and mirror the FormValues property signature to
ensure the write path preserves the setting when saving.
In `@packages/features/schedules/lib/slots.ts`:
- Around line 235-251: The filter branch should convert the Map iterator to an
array before filtering and preserve the minimizeGaps intent, and the public API
must accept/forward the new params: wrap slots.values() with Array.from(...)
before calling .filter() (use slot.time.toISOString() vs validStartTimes), and
if minimizeGaps is true return the filtered array even when empty (don’t fall
back to full slots). Also add minimizeGaps and existingBookings to the exported
getSlots signature and forward them into buildSlotsWithDateRanges so callers can
enable the feature; update references to eventLength, slots,
buildSlotsWithDateRanges, getSlots, minimizeGaps, and existingBookings
accordingly.
---
Outside diff comments:
In `@packages/features/schedules/lib/slots.ts`:
- Around line 256-285: getSlots is now typed to accept minimizeGaps and
existingBookings on GetSlots but the wrapper does not destructure or forward
them to buildSlotsWithDateRanges, so the new inputs are unused; update the
parameter destructuring in getSlots to include minimizeGaps and existingBookings
and pass both as arguments in the call to buildSlotsWithDateRanges({ ... })
(keep existing timeZone: getTimeZone(inviteeDate) and other props) so
buildSlotsWithDateRanges receives the new options.
In `@packages/trpc/server/routers/viewer/slots/util.ts`:
- Around line 1256-1275: The fallback-host recalculation path calls
this.calculateHostsAndAvailabilities(...) but fails to update
currentBookingsAllUsers, causing getSlots({ minimizeGaps, existingBookings }) to
use stale bookings; update the code so that after each recalculation (the call
to calculateHostsAndAvailabilities in the fallback flow) you reassign
currentBookingsAllUsers from the returned tuple (alongside allUsersAvailability
and usersWithCredentials/currentSeats) before calling getSlots, and apply the
same fix to the other recalculation sites that mirror this logic (the other
places where calculateHostsAndAvailabilities is invoked in the fallback path).
---
Nitpick comments:
In `@packages/features/ee/billing/service/teams/TeamBillingService.ts`:
- Around line 147-149: Replace the plain Error thrown in TeamBillingService when
owner is missing with an ErrorWithCode so the service returns a typed error
(e.g., throw new ErrorWithCode with an appropriate error code like "NO_OWNER"
and a message including this.team.id); ensure you import ErrorWithCode at the
top of the file and keep the same control flow (or, if this is not intended to
abort, log and return early from the method instead of throwing). Target the
missing-owner branch that checks `if (!owner)` inside the TeamBillingService
method and update the throw to use ErrorWithCode with a clear code and message
referencing this.team.id.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a388a29-2d9c-4ac3-bafd-28d314cbe054
📒 Files selected for processing (8)
apps/web/modules/event-types/components/tabs/advanced/EventAdvancedTab.tsxpackages/features/ee/billing/credit-service.tspackages/features/ee/billing/service/teams/TeamBillingService.tspackages/features/eventtypes/lib/types.tspackages/features/eventtypes/repositories/eventTypeRepository.tspackages/features/schedules/lib/slots.tspackages/prisma/schema.prismapackages/trpc/server/routers/viewer/slots/util.ts
Comment on lines
+795
to
+839
| async moveCreditsFromTeamToUser({teamId , userId} : {teamId : number; userId : number}) { | ||
| return await prisma.$transaction(async (tx) => { | ||
| const teamCreditBalance = await CreditsRepository.findCreditBalance({teamId}, tx); | ||
|
|
||
| if(!teamCreditBalance || teamCreditBalance.additionalCredits <= 0) { | ||
| return ; | ||
| } | ||
|
|
||
| let userCreditBalance = await CreditsRepository.findCreditBalance({userId}, tx); | ||
|
|
||
| if(!userCreditBalance) { | ||
| userCreditBalance = await CreditsRepository.createCreditBalance( | ||
| {userId}, | ||
| tx | ||
| ); | ||
| } | ||
|
|
||
| const creditsToTransfer = teamCreditBalance.additionalCredits; | ||
|
|
||
| logger.info("Moving credits from team to owner" , { | ||
| teamId, | ||
| userId, | ||
| creditsToTransfer, | ||
| }) | ||
|
|
||
| await CreditsRepository.updateCreditBalance ( | ||
| { | ||
| teamId, | ||
| data : {additionalCredits : 0}, | ||
| }, | ||
| tx | ||
| ); | ||
|
|
||
| await CreditsRepository.updateCreditBalance( | ||
| { | ||
| userId , | ||
| data : { | ||
| additionalCredits : { | ||
| increment : creditsToTransfer, | ||
| }, | ||
| }, | ||
| }, | ||
| tx | ||
| ); | ||
| return { creditsTransferred : creditsToTransfer }; |
Contributor
There was a problem hiding this comment.
Prevent double-transferring the same balance.
This captures teamCreditBalance.additionalCredits before the source balance is cleared, so two concurrent downgrade calls can both read the same amount and both increment the owner's balance. Please make the debit atomic on the source row first, then apply the increment to the user balance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/features/ee/billing/credit-service.ts` around lines 795 - 839, The
current moveCreditsFromTeamToUser reads teamCreditBalance.additionalCredits then
clears it, which allows races; instead perform an atomic debit on the team row
first and capture the previous amount, then increment the user. Modify
moveCreditsFromTeamToUser to replace the read-then-later update with a single
atomic operation (using CreditsRepository via tx or a tx.$queryRaw SQL UPDATE
... RETURNING previous additionalCredits / use UPDATE ... WHERE teamId = ? AND
additionalCredits > 0 RETURNING additionalCredits) to set additionalCredits = 0
and return the amount actually debited, then only if that returned amount > 0
perform the CreditsRepository.updateCreditBalance call to increment the user by
that returned amount; keep everything inside the same prisma.$transaction.
Comment on lines
+138
to
+154
| const creditService = new CreditService(); | ||
|
|
||
| const owner = await prisma.membership.findFirst({ | ||
| where : { | ||
| teamId : this.team.id, | ||
| role : MembershipRole.OWNER | ||
| }, | ||
| }); | ||
|
|
||
| if(!owner) { | ||
| throw new Error(`No owner found for team ${this.team.id}`); | ||
| } | ||
|
|
||
| await creditService.moveCreditsFromTeamToUser({ | ||
| teamId : this.team.id, | ||
| userId : owner.userId, | ||
| }); |
Contributor
There was a problem hiding this comment.
Keep the downgrade writes in one transaction.
moveCreditsFromTeamToUser() commits before the billing metadata is cleared. If the prisma.team.update at Line 163 fails, the owner keeps the transferred credits while the team still looks subscribed, and this method only logs the failure. Please run the owner lookup, credit transfer, and metadata update in the same DB transaction or add compensation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/features/ee/billing/service/teams/TeamBillingService.ts` around
lines 138 - 154, The owner lookup (prisma.membership.findFirst), the credit
transfer (CreditService.moveCreditsFromTeamToUser) and the team billing metadata
update (prisma.team.update) must be executed in a single DB transaction to avoid
partial commits; refactor this code to use a Prisma transaction
(prisma.$transaction) or accept a transactional client: fetch the owner inside
the transaction, call moveCreditsFromTeamToUser with the same transaction
context (or convert that method to accept a Prisma transaction/client), then
perform prisma.team.update within the same transaction so either all succeed or
all roll back; if changing moveCreditsFromTeamToUser is infeasible, implement a
compensating rollback step invoked on prisma.team.update failure to revert the
credit move.
| bookingLimits?: IntervalLimit; | ||
| onlyShowFirstAvailableSlot: boolean; | ||
| showOptimizedSlots: boolean; | ||
| minimizeGaps?: boolean | null; |
Contributor
There was a problem hiding this comment.
Add minimizeGaps to the shared update contract.
This file now carries the flag in FormValues, but EventTypeUpdateInput below still omits it. Since that type is documented as the shared update shape, the write path is still incomplete and can drop the setting on save.
Suggested fix
onlyShowFirstAvailableSlot?: boolean;
showOptimizedSlots?: boolean | null;
+ minimizeGaps?: boolean | null;
disableCancelling?: boolean | null;
disableRescheduling?: boolean | null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/features/eventtypes/lib/types.ts` at line 174, The
EventTypeUpdateInput type is missing the minimizeGaps flag present in
FormValues, so update the shared update contract by adding minimizeGaps?:
boolean | null to the EventTypeUpdateInput definition; locate
EventTypeUpdateInput in types.ts and mirror the FormValues property signature to
ensure the write path preserves the setting when saving.
Comment on lines
+235
to
+251
| if(minimizeGaps && existingBookings && existingBookings.length > 0) { | ||
| const validStartTimes = new Set<string>(); | ||
|
|
||
| for(const booking of existingBookings) { | ||
| validStartTimes.add(booking.endTime.toISOString()); | ||
|
|
||
| validStartTimes.add( | ||
| booking.startTime.subtract(eventLength, "minutes").toISOString() | ||
| ); | ||
| } | ||
|
|
||
| const filtered = Array.from(slots.values().filter((slot) => | ||
| validStartTimes.has(slot.time.toISOString()) | ||
| )) | ||
|
|
||
| return filtered.length > 0 ? filtered : Array.from(slots.values()); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '235,251p' packages/features/schedules/lib/slots.ts
rg -n '\.values\(\)\.filter\(' packages/features/schedules/lib/slots.tsRepository: calcom/cal.com
Length of output: 664
🏁 Script executed:
sed -n '256,285p' packages/features/schedules/lib/slots.tsRepository: calcom/cal.com
Length of output: 657
🏁 Script executed:
rg -A 15 'const buildSlotsWithDateRanges = \(' packages/features/schedules/lib/slots.ts | head -30Repository: calcom/cal.com
Length of output: 40
🏁 Script executed:
rg -n "buildSlotsWithDateRanges" packages/features/schedules/lib/slots.ts | head -20Repository: calcom/cal.com
Length of output: 136
🏁 Script executed:
sed -n '73,120p' packages/features/schedules/lib/slots.tsRepository: calcom/cal.com
Length of output: 1431
This filter branch won't work as written, and the feature is disconnected from the public API.
Two critical issues:
-
Invalid Iterator call:
slots.values()returns a map iterator, which does not have a.filter()method. This must be wrapped inArray.from()before filtering. -
Missing wrapper parameters: The exported
getSlotsfunction (lines 256-285) does not acceptminimizeGapsorexistingBookingsparameters and does not forward them tobuildSlotsWithDateRanges. Even if the first bug is fixed, the feature remains completely non-functional because callers cannot enable it.
The fallback filtered.length > 0 ? filtered : Array.from(slots.values()) also silently disables minimizeGaps in the absence of valid slots, defeating the intended behavior.
Suggested fix for issue 1
if (minimizeGaps && existingBookings && existingBookings.length > 0) {
const validStartTimes = new Set<string>();
for (const booking of existingBookings) {
validStartTimes.add(booking.endTime.toISOString());
validStartTimes.add(booking.startTime.subtract(eventLength, "minutes").toISOString());
}
- const filtered = Array.from(slots.values().filter((slot) =>
- validStartTimes.has(slot.time.toISOString())
- ))
-
- return filtered.length > 0 ? filtered : Array.from(slots.values());
+ const filtered = Array.from(slots.values()).filter((slot) =>
+ validStartTimes.has(slot.time.toISOString())
+ );
+
+ return filtered;
}Fix issue 2 by adding minimizeGaps and existingBookings to the getSlots function signature (lines 256-268) and forwarding them to buildSlotsWithDateRanges (lines 275-283).
📝 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.
Suggested change
| if(minimizeGaps && existingBookings && existingBookings.length > 0) { | |
| const validStartTimes = new Set<string>(); | |
| for(const booking of existingBookings) { | |
| validStartTimes.add(booking.endTime.toISOString()); | |
| validStartTimes.add( | |
| booking.startTime.subtract(eventLength, "minutes").toISOString() | |
| ); | |
| } | |
| const filtered = Array.from(slots.values().filter((slot) => | |
| validStartTimes.has(slot.time.toISOString()) | |
| )) | |
| return filtered.length > 0 ? filtered : Array.from(slots.values()); | |
| } | |
| if (minimizeGaps && existingBookings && existingBookings.length > 0) { | |
| const validStartTimes = new Set<string>(); | |
| for (const booking of existingBookings) { | |
| validStartTimes.add(booking.endTime.toISOString()); | |
| validStartTimes.add( | |
| booking.startTime.subtract(eventLength, "minutes").toISOString() | |
| ); | |
| } | |
| const filtered = Array.from(slots.values()).filter((slot) => | |
| validStartTimes.has(slot.time.toISOString()) | |
| ); | |
| return filtered; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/features/schedules/lib/slots.ts` around lines 235 - 251, The filter
branch should convert the Map iterator to an array before filtering and preserve
the minimizeGaps intent, and the public API must accept/forward the new params:
wrap slots.values() with Array.from(...) before calling .filter() (use
slot.time.toISOString() vs validStartTimes), and if minimizeGaps is true return
the filtered array even when empty (don’t fall back to full slots). Also add
minimizeGaps and existingBookings to the exported getSlots signature and forward
them into buildSlotsWithDateRanges so callers can enable the feature; update
references to eventLength, slots, buildSlotsWithDateRanges, getSlots,
minimizeGaps, and existingBookings accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Closes #6084
Adds a new "Minimize gaps between bookings" toggle to the event type
advanced settings. When enabled, only time slots directly before or
after an existing booking are shown to new customers — preventing idle
downtime for the host.
This mirrors how Acuity Scheduling handles this feature, as referenced
in the issue.
Changes
1.
packages/prisma/schema.prismaAdded
minimizeGaps Boolean @default(false)to theEventTypemodelso the setting is persisted per event type.
2.
packages/features/eventtypes/repositories/eventTypeRepository.tsAdded
minimizeGaps: trueto thefindForSlotsselect query so thefield is returned when slots are being calculated.
3.
packages/trpc/server/routers/viewer/slots/util.tsPassed
minimizeGapsfrom the event type andexistingBookings(reconstructed from
allUsersAvailability) into thegetSlotscall.4.
packages/features/eventtypes/lib/types.tsAdded
minimizeGaps?: boolean | nullto theFormValuestype so theform is aware of the new field.
5.
packages/features/eventtypes/components/EventAdvancedTab.tsxAdded a
SettingsToggleUI component forminimizeGapsin theadvanced tab, directly below the existing
showOptimizedSlotstoggle.Follows the exact same pattern.
How it works
Pattern followed
This follows the same implementation pattern as
showOptimizedSlotsthroughout the entire codebase.