Feat/multi host round robin #4
Conversation
Add maxRoundRobinHosts config to assign up to N hosts per RR booking. Includes schema, API v2, booking logic, UI, and tRPC integration. Default (null/1) maintains existing single-host behavior feat: add multi-host assignment for round robin bookings Add maxRoundRobinHosts config to assign up to N hosts per RR booking. Includes schema, API v2, booking logic, UI, and tRPC integration. Default (null/1) maintains existing single-host behavior
- Add 4 unit tests covering multi-host assignment scenarios - Update event-types-hooks.mdx with maxRoundRobinHosts documentation - Improve API field description in DTOs for better OpenAPI docs
- Test create round robin event type with maxRoundRobinHosts - Test default value (null) when not provided - Test update maxRoundRobinHosts value
- Use Set for faster user lookups - Move userIdsSet outside the loop - Move async calls (firstUserOrgId, enrichedHosts) outside loops - Filter group hosts once per group instead of every iteration
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
@refacto-visz |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis pull request introduces support for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
Summary of ChangesHello @visz11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to Round Robin event types by allowing the configuration of a maximum number of hosts to be assigned per booking. Previously, Round Robin events would typically assign a single host. With this change, users can now specify Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
codeant-ai
bot
added
the
size:XL
|
@refacto-visz |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (10)
docs/platform/event-types-hooks.mdx (1)
141-148: Align wording with the public API: hook “options object” vs mutation “payload object”.
The narrative says the “mutation function accepts an object…”, but your PR summary indicates the hook now accepts an options object (and the mutation receives the payload). Consider explicitly separating:
useCreateTeamEventType(hookOptions)createTeamEventType(payload)(includingmaxRoundRobinHosts)Also applies to: 155-164
docs/api-reference/v2/openapi.json (2)
13844-13844: Tighten the 200-response description formatting for readabilityLine 13844 has a long, indented, multi-sentence string; consider reflowing to avoid embedded extra whitespace (the
\n) so rendered docs don’t show odd indentation.
19663-19671: Use integer + explicit OpenAPI constraints formaxRoundRobinHosts
maxRoundRobinHostsis described as min 1, default 1, but is declared as"type": "number"(Lines 19668, 21686, 22154), which permits floats and doesn’t encode the default/min. Prefer:
"type": "integer"(optionally"format": "int32")"minimum": 1"default": 1Suggested diff (apply in all three locations):
"maxRoundRobinHosts": { - "type": "number", + "type": "integer", + "format": "int32", + "minimum": 1, + "default": 1, "description": "Only relevant for round robin event types. Specifies the maximum number of hosts to automatically assign per booking. When a booking is created, the system assigns up to this number of available hosts. If fewer hosts are available than the configured maximum, all available hosts are assigned. Minimum value is 1, defaults to 1.", "example": 1 }Also applies to: 21681-21689, 22149-22157
packages/features/eventtypes/components/tabs/assignment/EventTeamAssignmentTab.tsx (2)
517-541: LGTM! New maxRoundRobinHosts input field.The implementation correctly handles the nullable integer field with appropriate min constraint and value parsing. The
nullfallback for empty string ensures the field can be cleared.One minor observation: consider adding a
maxattribute or validation to prevent unreasonably large values (e.g.,max={100}or matching the team size).
281-282: Unused parameterorgId.The
orgIdparameter is aliased to_orgIdbut never used withinRoundRobinHosts. If this is intentional for API compatibility, the underscore prefix is appropriate. Otherwise, consider removing it from the props interface.packages/prisma/migrations/20251208104340_add_max_round_robin_hosts_optional/migration.sql (1)
1-2: Migration looks correct.The nullable
INTEGERcolumn aligns with the application logic that defaults to1whennull.Consider adding a
CHECKconstraint to enforce positive values at the database level:ALTER TABLE "public"."EventType" ADD COLUMN "maxRoundRobinHosts" INTEGER CHECK ("maxRoundRobinHosts" IS NULL OR "maxRoundRobinHosts" >= 1);This would prevent invalid data from bypassing application validation.
packages/prisma/schema.prisma (1)
249-249: Consider adding a database-level default value.The field is defined as
Int?without a@default()directive. This means:
- New
EventTyperecords will havemaxRoundRobinHosts = null- The application layer must handle the
nullcaseIf the intended behavior is to default to
1(as suggested by the test builder and API docs), consider adding:- maxRoundRobinHosts Int? + maxRoundRobinHosts Int? @default(1)This would provide consistency and reduce the need for null-checking throughout the application. If
nullis intentional to represent "not configured" or "no limit", the current schema is appropriate.packages/platform/types/event-types/event-types_2024_06_14/inputs/update-event-type.input.ts (1)
555-563: Consider conditional validation + a sane upper bound formaxRoundRobinHosts.
Right now the DTO allows this value regardless ofschedulingType, and it also allows arbitrarily large ints. If the field is intended to apply only to round-robin, consider enforcing that here (or explicitly rejecting it for COLLECTIVE) and bounding the max to prevent pathological inputs.@@ -import { +import { IsString, IsInt, IsBoolean, IsOptional, Min, + ValidateIf, + Max, ValidateNested, IsArray, ArrayNotEmpty, ArrayUnique, IsUrl, IsIn, } from "class-validator"; @@ @IsInt() @Min(1) @IsOptional() + @ValidateIf((o: UpdateTeamEventTypeInput_2024_06_14) => o.schedulingType === SchedulingType.ROUND_ROBIN) + @Max(50) @DocsPropertyOptional({ description: "Only relevant for round robin event types. Specifies the maximum number of hosts to automatically assign per booking. When a booking is created, the system assigns up to this number of available hosts. If fewer hosts are available than the configured maximum, all available hosts are assigned. Minimum value is 1, defaults to 1.", example: 1, + default: 1, }) maxRoundRobinHosts?: number;apps/api/v2/src/modules/organizations/event-types/max-round-robin-hosts.e2e-spec.ts (2)
77-100: Consider removing the@ts-ignoreby fixing the type definition.The
@ts-ignoreon line 84 indicates a type mismatch between the API's expected input (string"roundRobin") and theCreateTeamEventTypeInput_2024_06_14type definition. The input type should accept the string literal union if that's what the API expects.const createRoundRobinEventType = async ( overrides: Partial<CreateTeamEventTypeInput_2024_06_14> = {} ): Promise<TeamEventTypeOutput_2024_06_14> => { const body: CreateTeamEventTypeInput_2024_06_14 = { title: "Round Robin Event", slug: `max-rr-hosts-${randomString()}`, lengthInMinutes: 30, - // @ts-ignore - schedulingType accepts string - schedulingType: "roundRobin", + schedulingType: "roundRobin" as const, assignAllTeamMembers: true, ...overrides, };Alternatively, verify that
CreateTeamEventTypeInput_2024_06_14.schedulingTypeaccepts the string literal"roundRobin"in the type definition.
199-205: Consider sequential cleanup to improve error visibility.Using
Promise.allfor cleanup could mask individual deletion failures. For test reliability, consider sequential cleanup or wrapping in try-catch.afterAll(async () => { - await Promise.all(managedUsers.map((u) => userRepositoryFixture.delete(u.user.id))); + for (const u of managedUsers) { + await userRepositoryFixture.delete(u.user.id); + } await userRepositoryFixture.delete(platformAdmin.id); await oauthClientRepositoryFixture.delete(oAuthClient.id); await teamRepositoryFixture.delete(organization.id); await app.close(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts(1 hunks)apps/api/v2/src/modules/organizations/event-types/max-round-robin-hosts.e2e-spec.ts(1 hunks)apps/api/v2/src/modules/organizations/event-types/services/output.service.ts(3 hunks)apps/web/public/static/locales/en/common.json(1 hunks)docs/api-reference/v2/openapi.json(7 hunks)docs/platform/event-types-hooks.mdx(2 hunks)packages/features/bookings/lib/handleNewBooking/getEventTypesFromDB.ts(1 hunks)packages/features/bookings/lib/handleNewBooking/test/team-bookings/round-robin.test.ts(1 hunks)packages/features/bookings/lib/service/RegularBookingService.ts(11 hunks)packages/features/eventtypes/components/tabs/assignment/EventTeamAssignmentTab.tsx(7 hunks)packages/features/eventtypes/lib/defaultEvents.ts(1 hunks)packages/features/eventtypes/lib/types.ts(1 hunks)packages/features/eventtypes/repositories/eventTypeRepository.ts(3 hunks)packages/lib/test/builder.ts(1 hunks)packages/platform/atoms/event-types/hooks/useEventTypeForm.ts(1 hunks)packages/platform/types/event-types/event-types_2024_06_14/inputs/create-event-type.input.ts(1 hunks)packages/platform/types/event-types/event-types_2024_06_14/inputs/update-event-type.input.ts(1 hunks)packages/platform/types/event-types/event-types_2024_06_14/outputs/event-type.output.ts(1 hunks)packages/prisma/migrations/20251208104340_add_max_round_robin_hosts_optional/migration.sql(1 hunks)packages/prisma/schema.prisma(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/features/eventtypes/components/tabs/assignment/EventTeamAssignmentTab.tsx (2)
packages/features/eventtypes/lib/types.ts (1)
FormValues(84-182)packages/ui/components/form/inputs/Input.tsx (1)
NumberInput(170-182)
🔇 Additional comments (20)
docs/platform/event-types-hooks.mdx (1)
133-134: Verify/qualify “defaults to 1” (and where that default is enforced).
If the default is enforced server-side, call that out explicitly; if it’s only a UI/form default, the docs should avoid implying an API default.docs/api-reference/v2/openapi.json (1)
29460-29463: Addingmessageto only 2 output envelopes: confirm consistency/contractAdding a top-level
messagefield is generally backward compatible, but please confirm whether this “informational/deprecation” field should be part of the standard response envelope across endpoints (for consistency), or intentionally limited to these two outputs (and why).Also applies to: 29491-29494
packages/features/eventtypes/components/tabs/assignment/EventTeamAssignmentTab.tsx (1)
104-110: Good defensive guard for optional callback.The null check before invoking
onChangeprevents potential runtime errors when the callback is not provided.packages/features/bookings/lib/service/RegularBookingService.ts (3)
1047-1070: Multi-host round robin selection logic looks correct.The implementation properly:
- Defaults
maxHostsPerGroupto 1 whenmaxRoundRobinHostsis null- Pre-computes
enrichedHostsanduserIdsSetoutside the loop for efficiency- Filters
groupRRHoststo the current group contextOne consideration: if
eventType.maxRoundRobinHostsis explicitly set to0, the while loop conditionhostsFoundInGroup < maxHostsPerGroup(i.e.,0 < 0) will never be true, effectively disabling host selection. Verify this is the intended behavior or add validation upstream.
1072-1136: Loop termination and state management are correct.The while loop properly handles:
- Finding multiple hosts per group up to
maxHostsPerGroup- Excluding already-selected users via
excludedUserIds- Correctly differentiating between successful selection (increment counter) and availability failure (exclude but don't increment)
The
breakon line 1088 whennewLuckyUseris null ensures the loop terminates if no suitable host is found.
1156-1161: Availability guard handles multi-host scenario correctly.The condition
luckyUsers.length < (Object.keys(nonEmptyHostGroups).length || 1)ensures at least one host is selected per non-empty group. SincemaxRoundRobinHostsis a maximum (not a required minimum), finding fewer hosts than the max is acceptable when the pool is exhausted.packages/platform/atoms/event-types/hooks/useEventTypeForm.ts (2)
122-122: Form default value correctly initialized.The
maxRoundRobinHostsfield is properly added to the form's default values. Since the...inputspread inhandleSubmit(line 378) passes through fields not explicitly destructured,maxRoundRobinHostswill be included in the payload when dirty.
302-331: Field correctly flows through submit payload.The
maxRoundRobinHostsfield is not explicitly destructured fromdirtyValues, so it correctly remains in theinputobject, then flows throughrestto the finalpayload. This ensures it's submitted when the field is dirty.packages/features/eventtypes/lib/types.ts (1)
171-171: LGTM!The type definition for
maxRoundRobinHostsis consistent with the Prisma schema and follows the established pattern for optional numeric fields inFormValues.apps/web/public/static/locales/en/common.json (1)
4108-4109: Translation keys look good; verify key usage matches UI exactly.
Since these are new keys, the main risk is a mismatch between the UI lookup key and the JSON key (typos / different naming convention).packages/features/eventtypes/lib/defaultEvents.ts (1)
128-130:maxRoundRobinHosts: nullis fine, but confirm downstream treats null/undefined as “use default (1)”.
Because this value is now present on default event shapes, anything that reads it should handle null safely (especially if it expects a number).packages/features/bookings/lib/handleNewBooking/getEventTypesFromDB.ts (1)
18-185: Good: Prisma select now hydratesmaxRoundRobinHosts.
This ensures the field actually makes it into the returned event type object via...restEventType.packages/platform/types/event-types/event-types_2024_06_14/inputs/create-event-type.input.ts (1)
641-649: LGTM! Well-documented optional field for round robin host limits.The validation decorators (
@IsInt(),@Min(1),@IsOptional()) are appropriate, and the documentation clearly explains the field's purpose and behavior when fewer hosts are available than the configured maximum.packages/features/bookings/lib/handleNewBooking/test/team-bookings/round-robin.test.ts (2)
1089-1170: LGTM! Well-structured test helpers and scenario setup.The helper functions effectively reduce duplication and the
setupScenariofunction correctly handles the optionalmaxRoundRobinHostsparameter with a conditional spread.
1191-1237: Comprehensive test coverage for maxRoundRobinHosts scenarios.The four test cases cover the essential scenarios:
- Multiple hosts assignment when
maxRoundRobinHosts > 1- Graceful degradation when fewer hosts are available than configured
- Default behavior (single host) when undefined
- Explicit single host when set to 1
apps/api/v2/src/modules/organizations/event-types/services/output.service.ts (1)
107-108: LGTM! Consistent propagation of maxRoundRobinHosts through the output service.The field is correctly destructured and included in the response object, following the same pattern as
rescheduleWithSameRoundRobinHost.Also applies to: 143-143
packages/features/eventtypes/repositories/eventTypeRepository.ts (3)
606-606: LGTM! Correctly added to CompleteEventTypeSelect in findById.The field is properly positioned alongside related round robin configuration fields.
905-905: LGTM! Consistently added to findByIdForOrgAdmin.Maintains parity with
findByIdfor organization admin access patterns.
1265-1265: Critical addition for slot calculation logic.Including
maxRoundRobinHostsinfindForSlotsensures the booking service has access to this configuration when determining host assignment during slot availability calculations.apps/api/v2/src/modules/organizations/event-types/max-round-robin-hosts.e2e-spec.ts (1)
207-230: Good test coverage for maxRoundRobinHosts CRUD operations.The tests effectively validate:
- Setting
maxRoundRobinHostsduring creation- Default
nullvalue when not provided (backward compatibility)- Updating the value on an existing event type
| | "maxRoundRobinHosts" | ||
| >; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the correct file and verify it exists
find . -name "output-event-types.service.ts" -type fRepository: coderabbit-test/cal.com
Length of output: 161
🏁 Script executed:
#!/bin/bash
# List the entire file to understand its structure
cat -n apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts | head -250Repository: coderabbit-test/cal.com
Length of output: 9831
🏁 Script executed:
#!/bin/bash
# Search for maxRoundRobinHosts in the file
rg -n "maxRoundRobinHosts" apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts -B 3 -A 3Repository: coderabbit-test/cal.com
Length of output: 247
maxRoundRobinHosts is accepted in the Input type but not exposed in the API response.
The field is added to the Input type (line 97) but neither extracted in the destructuring block (lines 107-138) nor included in the return object (lines 173-218). Unlike maxActiveBookingsPerBooker and maxActiveBookingPerBookerOfferReschedule, which have a corresponding transformation method, maxRoundRobinHosts has no processing logic. Either add it to the response output or remove it from the Input type if intentionally unsupported.
🤖 Prompt for AI Agents
In
apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts
around lines 97 to 218, the Input type includes maxRoundRobinHosts but it is
neither destructured from input (lines ~107-138) nor included in the returned
API response (lines ~173-218) and there is no transformation method; update the
service to handle this field by (1) adding maxRoundRobinHosts to the
destructuring where other Input fields are extracted, (2) applying the same
transformation/validation pattern used for maxActiveBookingsPerBooker (create a
small transform method if necessary) or pass through the value consistently, and
(3) include the transformed/maxRoundRobinHosts value in the returned object so
the API exposes it; alternatively, if this field is intentionally unsupported,
remove it from the Input type to keep types and output consistent.
| "summary": "Get team event types", | ||
| "description": "Use the optional `sortCreatedAt` query parameter to order results by creation date (by ID). Accepts \"asc\" (oldest first) or \"desc\" (newest first). When not provided, no explicit ordering is applied.", |
There was a problem hiding this comment.
“sortCreatedAt” description mentions “by ID” — confirm that’s accurate
Line 14456 says ordering is “by creation date (by ID)”; if the actual sort is createdAt, drop the “(by ID)” phrasing (or rename the param / clarify the implementation) to avoid misleading consumers.
🤖 Prompt for AI Agents
In docs/api-reference/v2/openapi.json around lines 14455 to 14456, the
summary/description for the sortCreatedAt query parameter incorrectly says "by
creation date (by ID)"; verify the actual implementation and then update the
description to match: if the API sorts by the createdAt timestamp, remove the
parenthetical "(by ID)" and state "order results by creation date (createdAt)";
if the API actually sorts by ID instead of createdAt, either change the
parameter name to reflect ID-based sorting or update the description to say
"order results by ID" to avoid misleading consumers.
| The useCreateTeamEventType hook allows you to create a new team event type. This hook returns a mutation function that handles the event type creation process. The mutation function accepts an object with the following properties: ***lengthInMinutes*** which is the length of the event in minutes, ***title*** which is the title of the event, ***slug*** which is the slug of the event, ***description*** which is the description of the event, schedulingType which can be either ***COLLECTIVE***, ***ROUND_ROBIN*** or ***MANAGED***, ***hosts*** which is an array of hosts for the event and the ***teamId*** which is the id of the team, and optionally ***maxRoundRobinHosts*** which specifies the maximum number of hosts to assign per booking for Round Robin events (defaults to 1). | ||
There was a problem hiding this comment.
Clarify when maxRoundRobinHosts applies + what happens for non-ROUND_ROBIN scheduling types.
Right now it reads Round Robin-specific, but the example below uses schedulingType: "COLLECTIVE". Document whether the API ignores it, rejects it, or only accepts it for ROUND_ROBIN (and ideally show a ROUND_ROBIN example if that’s the intended use).
🤖 Prompt for AI Agents
In docs/platform/event-types-hooks.mdx around lines 133 to 134, the docstring
for useCreateTeamEventType is ambiguous about maxRoundRobinHosts; update the
text to state explicitly that maxRoundRobinHosts only applies when
schedulingType is "ROUND_ROBIN", explain whether the API ignores or rejects it
for other scheduling types (e.g., "ignored" or "validated" per implementation),
and replace or add an example showing schedulingType: "ROUND_ROBIN" with
maxRoundRobinHosts set (or note that the field is optional and defaults to 1) so
readers know intended usage and behavior for non-ROUND_ROBIN types.
| durationLimits: null, | ||
| assignAllTeamMembers: false, | ||
| rescheduleWithSameRoundRobinHost: false, | ||
| maxRoundRobinHosts: 1, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -nP --type=ts -C5 'maxRoundRobinHosts' --glob '!**/test/**' --glob '!**/builder.ts'Repository: coderabbit-test/cal.com
Length of output: 23937
🏁 Script executed:
# Check test builder to understand the context of the hardcoded value
head -150 packages/lib/test/builder.ts | tail -20Repository: coderabbit-test/cal.com
Length of output: 687
Use null instead of 1 for maxRoundRobinHosts in test builder to match form defaults.
The test builder hardcodes maxRoundRobinHosts: 1, but the form defaults use null (see packages/features/eventtypes/lib/defaultEvents.ts:129). While the booking logic correctly handles this via nullish coalescing (?? 1 in RegularBookingService.ts:1047), the test builder should match the actual form defaults for consistency.
🤖 Prompt for AI Agents
In packages/lib/test/builder.ts at line 143, the test builder currently sets
maxRoundRobinHosts: 1 which differs from the form default of null; change that
property to maxRoundRobinHosts: null so the test data matches form defaults (and
update any tests that assert on the numeric value to expect null or use the
service's nullish defaulting).
| @IsInt() | ||
| @Min(1) | ||
| @IsOptional() | ||
| @ApiPropertyOptional({ | ||
| description: "Only relevant for round robin event types. Specifies the maximum number of hosts to automatically assign per booking. When a booking is created, the system assigns up to this number of available hosts. If fewer hosts are available than the configured maximum, all available hosts are assigned. Minimum value is 1, defaults to 1.", | ||
| example: 1, | ||
| }) | ||
| maxRoundRobinHosts?: number; |
There was a problem hiding this comment.
Clarify default value behavior in API documentation.
The description states "defaults to 1", but the field is marked @IsOptional() and the schema allows null. This could confuse API consumers about the actual behavior:
- If
null/undefinedmeans "use default of 1", the description is correct but the API should document thatnull→1 - If
nullmeans "no limit" or "not configured", the description should be updated to clarify this
Please update the description to accurately reflect when/where the default value of 1 is applied (e.g., "When not specified, defaults to 1" vs "Defaults to 1 when null").
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature allowing users to specify maxRoundRobinHosts for Round Robin event types, controlling the maximum number of hosts assigned per booking. The changes include adding the maxRoundRobinHosts field to the database schema, API input/output types, and internal data structures. The core logic in RegularBookingService.ts was updated to respect this new limit during host assignment, ensuring that bookings assign up to the specified number of hosts per group, or all available hosts if fewer than the maximum. A new E2E test suite was added to verify the functionality of maxRoundRobinHosts during event type creation and updates, covering scenarios where the value is set, undefined, or equal to one. The UI was updated with a new number input field for this setting, and documentation (API reference and platform hooks) was updated to reflect the new property. A review comment highlighted that the existing tests for maxRoundRobinHosts only cover single-group scenarios and suggested adding tests for multiple round-robin groups to ensure the limit is applied correctly as a total, not per-group. Another comment requested correcting a misleading example in the documentation where maxRoundRobinHosts was shown with a COLLECTIVE scheduling type instead of ROUND_ROBIN.
| } from "@calcom/platform-types"; | ||
| import type { PlatformOAuthClient, Team, User } from "@calcom/prisma/client"; | ||
|
|
||
| describe("maxRoundRobinHosts for Round Robin event types", () => { |
There was a problem hiding this comment.
This is a great test suite for the maxRoundRobinHosts feature. However, it currently only covers scenarios with a single team/group.
The implementation in RegularBookingService.ts has a potential issue with how it handles multiple round-robin groups, where it might assign more hosts than the maxRoundRobinHosts limit.
To ensure robust testing, I recommend adding test cases that involve multiple round-robin groups to verify that maxRoundRobinHosts is correctly applied as a total limit for the booking, not as a per-group limit.
| title: "Daily standup", | ||
| slug: "daily-standup", | ||
| description: "Daily standup for the team", | ||
| schedulingType: "COLLECTIVE", |
There was a problem hiding this comment.
The example for useCreateTeamEventType is a bit misleading. It shows schedulingType: "COLLECTIVE" while also setting maxRoundRobinHosts. However, maxRoundRobinHosts is only applicable for ROUND_ROBIN event types, as stated in the description.
To avoid confusion, please update the example to use schedulingType: "ROUND_ROBIN".
schedulingType: "ROUND_ROBIN",
|
@refacto-visz |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
|
|
||
| const response = await request(app.getHttpServer()) | ||
| .post(`/api/v2/oauth-clients/${oAuthClient.id}/users`) | ||
| .set(X_CAL_SECRET_KEY, oAuthClient.secret) |
There was a problem hiding this comment.
Suggestion: The managed user creation request only sets the secret header and omits the client ID header, which is required by other OAuth-client-authenticated endpoints in this codebase; this can cause the call to /api/v2/oauth-clients/:id/users to be rejected as unauthorized even though a valid client is configured. [possible bug]
Severity Level: Critical 🚨
| .set(X_CAL_SECRET_KEY, oAuthClient.secret) | |
| .set(X_CAL_CLIENT_ID, oAuthClient.id) |
Why it matters? ⭐
This test file uses the OAuth client id + secret as request headers elsewhere (see other requests in the same file where both X_CAL_CLIENT_ID and X_CAL_SECRET_KEY are set). The managed-user creation call only sets the secret header which could plausibly be rejected by auth middleware that expects both headers. Adding the missing X_CAL_CLIENT_ID header is a small, correct change that fixes a real authorization mismatch rather than a stylistic nit.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** apps/api/v2/src/modules/organizations/event-types/max-round-robin-hosts.e2e-spec.ts
**Line:** 60:60
**Comment:**
*Possible Bug: The managed user creation request only sets the secret header and omits the client ID header, which is required by other OAuth-client-authenticated endpoints in this codebase; this can cause the call to `/api/v2/oauth-clients/:id/users` to be rejected as unauthorized even though a valid client is configured.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| const { setValue, getValues, control } = useFormContext<FormValues>(); | ||
| const assignRRMembersUsingSegment = getValues("assignRRMembersUsingSegment"); | ||
| const isRRWeightsEnabled = useWatch({ | ||
| control, |
There was a problem hiding this comment.
Suggestion: The value for assignRRMembersUsingSegment is read once via getValues and never subscribed, so if that form field is toggled elsewhere the RoundRobinHosts component (and EditWeightsForAllTeamMembers) will keep using a stale value, which is a logic bug; you should use useWatch for this field just like the others so the UI reacts to changes correctly. [logic error]
Severity Level: Minor
| control, | |
| const assignRRMembersUsingSegment = useWatch({ | |
| control, | |
| name: "assignRRMembersUsingSegment", | |
| }); |
Why it matters? ⭐
The current code uses getValues to read assignRRMembersUsingSegment once, so it won't react to updates made elsewhere in the form. Replacing this with useWatch (like the other fields) fixes a real logic bug: the component and any dependent child components (e.g. EditWeightsForAllTeamMembers) will correctly re-render when that field changes. This is not a cosmetic change — it fixes stale-state behavior.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/features/eventtypes/components/tabs/assignment/EventTeamAssignmentTab.tsx
**Line:** 307:307
**Comment:**
*Logic Error: The value for `assignRRMembersUsingSegment` is read once via `getValues` and never subscribed, so if that form field is toggled elsewhere the `RoundRobinHosts` component (and `EditWeightsForAllTeamMembers`) will keep using a stale value, which is a logic bug; you should use `useWatch` for this field just like the others so the UI reacts to changes correctly.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| schedulingType: "COLLECTIVE", | ||
| hosts: [{"userId": 1456}, {"userId": 2357}], | ||
| teamId: 1234, | ||
| maxRoundRobinHosts: 2, |
There was a problem hiding this comment.
Suggestion: The example sets maxRoundRobinHosts on an event configured with a COLLECTIVE scheduling type even though the text explains this option only affects Round Robin events, which can mislead developers into expecting this field to change behaviour for collective events when it will be ignored; adding an inline clarification avoids this incorrect assumption while keeping the example runnable. [possible bug]
Severity Level: Critical 🚨
| maxRoundRobinHosts: 2, | |
| maxRoundRobinHosts: 2, // Only used when schedulingType is "ROUND_ROBIN" |
Why it matters? ⭐
The PR documentation text explicitly states maxRoundRobinHosts only applies to Round Robin events, but the example object sets that field while using schedulingType: "COLLECTIVE". Adding a short inline comment prevents readers from mistakenly thinking the field has an effect for COLLECTIVE events. This is a small, clear clarification in a doc example (no runtime change) and helps prevent confusion when copying the snippet.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** docs/platform/event-types-hooks.mdx
**Line:** 163:163
**Comment:**
*Possible Bug: The example sets `maxRoundRobinHosts` on an event configured with a `COLLECTIVE` scheduling type even though the text explains this option only affects Round Robin events, which can mislead developers into expecting this field to change behaviour for collective events when it will be ignored; adding an inline clarification avoids this incorrect assumption while keeping the example runnable.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| durationLimits: null, | ||
| assignAllTeamMembers: false, | ||
| rescheduleWithSameRoundRobinHost: false, | ||
| maxRoundRobinHosts: 1, |
There was a problem hiding this comment.
Suggestion: The test builder sets maxRoundRobinHosts to 1 by default, while the Prisma schema declares this field as optional (Int?) and the shared default event configuration uses null; this mismatch means tests using buildEventType() will create event types with an explicit max-hosts limit even when the feature is conceptually "not configured", diverging from real runtime defaults and potentially hiding bugs around the distinction between null and an explicit 1. [logic error]
Severity Level: Minor
| maxRoundRobinHosts: 1, | |
| maxRoundRobinHosts: null, |
Why it matters? ⭐
The suggestion highlights a real mismatch between the test fixture and the production model: an optional Prisma Int? field semantically differs from an explicit value of 1. Using 1 in the test builder will mask behavior where the field is unset (null) in real data and can hide bugs that depend on distinguishing "not configured" vs "explicitly limited to 1". Changing the fixture to null aligns tests with runtime defaults and improves fidelity.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/lib/test/builder.ts
**Line:** 143:143
**Comment:**
*Logic Error: The test builder sets `maxRoundRobinHosts` to `1` by default, while the Prisma schema declares this field as optional (`Int?`) and the shared default event configuration uses `null`; this mismatch means tests using `buildEventType()` will create event types with an explicit max-hosts limit even when the feature is conceptually "not configured", diverging from real runtime defaults and potentially hiding bugs around the distinction between `null` and an explicit `1`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| }) | ||
| maxRoundRobinHosts?: number; |
There was a problem hiding this comment.
Suggestion: The new output field is typed and documented as a non-nullable number, but the database and services can return null for this value; this mismatch (number vs number|null and missing nullable: true in the Swagger decorator) breaks the API contract and can cause consumers to treat a possibly-null value as always numeric, leading to runtime errors when they perform numeric operations. [type error]
Severity Level: Minor
| }) | |
| maxRoundRobinHosts?: number; | |
| nullable: true, | |
| }) | |
| maxRoundRobinHosts?: number | null; |
Why it matters? ⭐
The suggestion fixes a real typing/contract inconsistency. Other optional numeric fields in this file that can be null (see parentEventTypeId and ownerId patterns in the same output) include nullable: true in the ApiPropertyOptional and use a union type (number | null). The current code marks maxRoundRobinHosts as optional but not nullable in the schema and as a plain number type, which could mislead consumers and swagger clients into treating it as always numeric. Making it nullable in the decorator and typing it number | null aligns runtime behavior, the OpenAPI contract, and the project's existing patterns.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/platform/types/event-types/event-types_2024_06_14/outputs/event-type.output.ts
**Line:** 560:561
**Comment:**
*Type Error: The new output field is typed and documented as a non-nullable number, but the database and services can return null for this value; this mismatch (number vs number|null and missing `nullable: true` in the Swagger decorator) breaks the API contract and can cause consumers to treat a possibly-null value as always numeric, leading to runtime errors when they perform numeric operations.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Nitpicks 🔍
|
Code Review: Multi-Host Round Robin Booking FeaturePR Confidence Score: 🟥 3 / 5👍 Well Done
📁 Selected files for review (20)
📝 Additional Comments
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/in/1889637?s=60&v=4){kind=small}
| for (const [groupId, luckyUserPool] of Object.entries(luckyUserPools)) { | ||
| let luckUserFound = false; | ||
| while (luckyUserPool.length > 0 && !luckUserFound) { | ||
| const freeUsers = luckyUserPool.filter( | ||
| (user) => !luckyUsers.concat(notAvailableLuckyUsers).find((existing) => existing.id === user.id) | ||
| ); | ||
| let hostsFoundInGroup = 0; |
There was a problem hiding this comment.
Incorrect Multi-Host Selection Logic
The logic for selecting multiple hosts for a round-robin booking is flawed when host groups are configured. The counter hostsFoundInGroup is re-initialized for each host group, causing the system to select up to maxRoundRobinHosts from each group, rather than a total for the entire booking. This results in assigning num_groups * maxRoundRobinHosts hosts, which contradicts the feature's intent and the user-facing description of "Maximum hosts per booking".
let totalHostsFound = 0;
for (const [groupId, luckyUserPool] of Object.entries(luckyUserPools)) {
let hostsFoundInGroup = 0;
const remainingHostsNeeded = maxHostsPerGroup - totalHostsFound;
const groupRRHosts = enrichedHosts.filter(
(host) =>
!host.isFixed &&
userIdsSet.has(host.user.id) &&
(host.groupId === groupId || (!host.groupId && groupId === DEFAULT_GROUP_ID))
);
while (luckyUserPool.length > 0 && hostsFoundInGroup < remainingHostsNeeded && totalHostsFound < maxHostsPerGroup) {
Commitable Suggestion
| for (const [groupId, luckyUserPool] of Object.entries(luckyUserPools)) { | |
| let luckUserFound = false; | |
| while (luckyUserPool.length > 0 && !luckUserFound) { | |
| const freeUsers = luckyUserPool.filter( | |
| (user) => !luckyUsers.concat(notAvailableLuckyUsers).find((existing) => existing.id === user.id) | |
| ); | |
| let hostsFoundInGroup = 0; | |
| let totalHostsFound = 0; | |
| for (const [groupId, luckyUserPool] of Object.entries(luckyUserPools)) { | |
| let hostsFoundInGroup = 0; | |
| const remainingHostsNeeded = maxHostsPerGroup - totalHostsFound; | |
| const groupRRHosts = enrichedHosts.filter( | |
| (host) => | |
| !host.isFixed && | |
| userIdsSet.has(host.user.id) && | |
| (host.groupId === groupId || (!host.groupId && groupId === DEFAULT_GROUP_ID)) | |
| ); | |
| while (luckyUserPool.length > 0 && hostsFoundInGroup < remainingHostsNeeded && totalHostsFound < maxHostsPerGroup) { |
Standards
- Algorithm-Correctness-Loop-Logic
- Business-Rule-State-Consistency
| @IsInt() | ||
| @Min(1) | ||
| @IsOptional() | ||
| @DocsPropertyOptional({ | ||
| description: "Only relevant for round robin event types. Specifies the maximum number of hosts to automatically assign per booking. When a booking is created, the system assigns up to this number of available hosts. If fewer hosts are available than the configured maximum, all available hosts are assigned. Minimum value is 1, defaults to 1.", | ||
| example: 1, | ||
| }) | ||
| maxRoundRobinHosts?: number; |
There was a problem hiding this comment.
Missing Maximum Validation
Input validation only enforces minimum value but lacks maximum boundary validation. Attackers could submit extremely large values causing resource exhaustion through excessive host assignment operations. System performance degradation possible through unbounded resource allocation in booking processing.
Standards
- CWE-770
- OWASP-A04
- NIST-SSDF-PW.1
| @IsInt() | ||
| @Min(1) | ||
| @IsOptional() | ||
| @DocsPropertyOptional({ | ||
| description: "Only relevant for round robin event types. Specifies the maximum number of hosts to automatically assign per booking. When a booking is created, the system assigns up to this number of available hosts. If fewer hosts are available than the configured maximum, all available hosts are assigned. Minimum value is 1, defaults to 1.", | ||
| example: 1, | ||
| }) | ||
| maxRoundRobinHosts?: number; |
There was a problem hiding this comment.
Missing Maximum Validation
Update endpoint validation mirrors create endpoint vulnerability with missing maximum boundary validation. Attackers could modify existing event types with excessive host counts causing resource exhaustion. System performance impact through unbounded host assignment operations in booking workflows.
Standards
- CWE-770
- OWASP-A04
- NIST-SSDF-PW.1
| @IsInt() | ||
| @Min(1) | ||
| @IsOptional() | ||
| @ApiPropertyOptional({ | ||
| description: "Only relevant for round robin event types. Specifies the maximum number of hosts to automatically assign per booking. When a booking is created, the system assigns up to this number of available hosts. If fewer hosts are available than the configured maximum, all available hosts are assigned. Minimum value is 1, defaults to 1.", | ||
| example: 1, | ||
| }) | ||
| maxRoundRobinHosts?: number; |
There was a problem hiding this comment.
Missing Maximum Validation
Output validation lacks maximum boundary constraints allowing dangerous values to persist in API responses. While output validation is less critical than input, consistent validation prevents data integrity issues and ensures safe client-side processing. Resource exhaustion possible if clients process unbounded values.
Standards
- CWE-770
- OWASP-A04
- NIST-SSDF-PW.1
User description
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
CodeAnt-AI Description
Add configurable multi-host assignment for Round Robin bookings
What Changed
Impact
✅ Assign up to N hosts per round-robin booking✅ Fewer failed bookings when multiple hosts are available✅ Clearer API + UI control for host count💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.