perf: update frontend components to use paginated hosts and delta-based state management#28155
Conversation
…ed state management - Lazy tab rendering: tabs are functions called only when active - Paginated host fetching hooks: usePaginatedAssignmentHosts, usePaginatedAvailabilityHosts, usePaginatedAssignmentChildren - HostsProvider context with delta tracking (add/update/remove) - Virtualized lists with @tanstack/react-virtual for smooth scrolling - Async team member search with useSearchTeamMembers hook - Updated assignment, availability, and setup tabs to use paginated data - EditWeightsForAllTeamMembers refactored to use tRPC directly - checkForEmptyAssignment updated to use hostCount - Removed teamMembers dependency from components - E2E tests for team event type assignment - Fix: tab persistence after save (revalidate moved to onSettled) - Delete unused useTeamMembersWithSegment hooks Co-Authored-By: unknown <>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
…devin/1771970971-improve-event-type-page-frontend
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
12 issues found across 36 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/web/modules/event-types/components/locations/HostLocations.tsx">
<violation number="1" location="apps/web/modules/event-types/components/locations/HostLocations.tsx:741">
P2: Disabling per-host locations now only clears locations for hosts that have been paginated in. Hosts not loaded yet keep their previous location data, so re-enabling or saving can persist stale per-host locations. Consider clearing locations for all hosts (e.g., backend bulk clear or a delta mechanism that doesn’t depend on loaded pages).</violation>
</file>
<file name="packages/features/eventtypes/lib/useHostsForEventType.ts">
<violation number="1" location="packages/features/eventtypes/lib/useHostsForEventType.ts:169">
P3: Guard against duplicate userIds in hostsToRemove so repeated remove actions don’t bloat the delta or issue redundant deleteMany conditions.</violation>
</file>
<file name="apps/web/modules/event-types/components/EventTypeWebWrapper.tsx">
<violation number="1" location="apps/web/modules/event-types/components/EventTypeWebWrapper.tsx:163">
P2: Reset pendingHostChanges on successful save; otherwise stale host deltas can be resent on later saves and reapply prior host updates.</violation>
</file>
<file name="apps/web/modules/event-types/components/tabs/assignment/EventTeamAssignmentTab.tsx">
<violation number="1" location="apps/web/modules/event-types/components/tabs/assignment/EventTeamAssignmentTab.tsx:517">
P2: Hardcoded "Group" text is not localized. Use `t()` instead of embedding English strings directly.</violation>
<violation number="2" location="apps/web/modules/event-types/components/tabs/assignment/EventTeamAssignmentTab.tsx:721">
P2: `pendingChanges` is read via `getValues()` (non-reactive) and passed to `usePaginatedAssignmentChildren`. Use `useWatch({ name: "pendingChildrenChanges" })` so the hook re-runs when the form value changes reactively.</violation>
</file>
<file name="apps/web/modules/event-types/components/AddMembersWithSwitch.tsx">
<violation number="1" location="apps/web/modules/event-types/components/AddMembersWithSwitch.tsx:319">
P2: `useSearchTeamMembers` is always enabled, so it will fetch paginated team members even when the assignment state hides the member selector (e.g., assign-all or segment modes). This adds unnecessary network requests and processing. Consider enabling the query only when the selector is rendered.</violation>
</file>
<file name="apps/web/modules/event-types/components/EditWeightsForAllTeamMembers.tsx">
<violation number="1" location="apps/web/modules/event-types/components/EditWeightsForAllTeamMembers.tsx:155">
P2: `useEffect` re-initializes `localWeights` whenever `value` changes while the sheet is open, silently discarding unsaved edits. Use a ref to capture the initial weights only at open time.</violation>
<violation number="2" location="apps/web/modules/event-types/components/EditWeightsForAllTeamMembers.tsx:217">
P2: `handleDownloadCsv` has `try/finally` but no `catch`. A failed fetch silently swallows the error with no user feedback. Add a `catch` to show an error toast.</violation>
<violation number="3" location="apps/web/modules/event-types/components/EditWeightsForAllTeamMembers.tsx:273">
P2: `allMembers.find(...)` inside the CSV parsing loop is O(n×m). Build an email→member `Map` before the loop for O(n+m) complexity.</violation>
</file>
<file name="packages/features/eventtypes/components/CheckedTeamSelect.tsx">
<violation number="1" location="packages/features/eventtypes/components/CheckedTeamSelect.tsx:143">
P2: The scroll listener won’t attach if the selected-host list starts empty because the container ref is only mounted after `useFetchMoreOnScroll` has already returned. This breaks infinite scrolling once hosts are added later.</violation>
</file>
<file name="apps/web/playwright/team-event-type-assignment.e2e.ts">
<violation number="1" location="apps/web/playwright/team-event-type-assignment.e2e.ts:45">
P2: Custom agent: **E2E Tests Best Practices**
Add an explicit expect(page).toHaveURL() after navigation to satisfy the E2E best practice for fast redirection detection.</violation>
<violation number="2" location="apps/web/playwright/team-event-type-assignment.e2e.ts:85">
P2: Custom agent: **E2E Tests Best Practices**
Replace text-based locators with data-testid-based selectors. The E2E best practices require avoiding text locators unless scoped under a stable test-id container.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| { shouldDirty: true } | ||
| ); | ||
| // Use setHosts from context instead of form setValue for performance | ||
| setHosts(serverHosts, hosts.map((host) => ({ ...host, location: null }))); |
There was a problem hiding this comment.
P2: Disabling per-host locations now only clears locations for hosts that have been paginated in. Hosts not loaded yet keep their previous location data, so re-enabling or saving can persist stale per-host locations. Consider clearing locations for all hosts (e.g., backend bulk clear or a delta mechanism that doesn’t depend on loaded pages).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/modules/event-types/components/locations/HostLocations.tsx, line 741:
<comment>Disabling per-host locations now only clears locations for hosts that have been paginated in. Hosts not loaded yet keep their previous location data, so re-enabling or saving can persist stale per-host locations. Consider clearing locations for all hosts (e.g., backend bulk clear or a delta mechanism that doesn’t depend on loaded pages).</comment>
<file context>
@@ -751,34 +730,32 @@ const normalizeHostLocation = (host: Host, eventTypeId: number): Host => {
- { shouldDirty: true }
- );
+ // Use setHosts from context instead of form setValue for performance
+ setHosts(serverHosts, hosts.map((host) => ({ ...host, location: null })));
}
};
</file context>
| created: true, | ||
| })); | ||
| // Reset pending children changes after successful save | ||
| currentValues.pendingChildrenChanges = { |
There was a problem hiding this comment.
P2: Reset pendingHostChanges on successful save; otherwise stale host deltas can be resent on later saves and reapply prior host updates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/modules/event-types/components/EventTypeWebWrapper.tsx, line 163:
<comment>Reset pendingHostChanges on successful save; otherwise stale host deltas can be resent on later saves and reapply prior host updates.</comment>
<file context>
@@ -159,15 +159,16 @@ const EventTypeWeb = ({
- created: true,
- }));
+ // Reset pending children changes after successful save
+ currentValues.pendingChildrenChanges = {
+ childrenToAdd: [],
+ childrenToRemove: [],
</file context>
apps/web/modules/event-types/components/tabs/assignment/EventTeamAssignmentTab.tsx
Outdated
Show resolved
Hide resolved
| <div className="-mb-4 flex items-center justify-between"> | ||
| <div className="flex items-center gap-1"> | ||
| <span className="text-default text-sm font-medium">{`Group ${hostGroups.length + 1}`}</span> | ||
| <span className="text-default text-sm font-medium">{`Group ${ |
There was a problem hiding this comment.
P2: Hardcoded "Group" text is not localized. Use t() instead of embedding English strings directly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/modules/event-types/components/tabs/assignment/EventTeamAssignmentTab.tsx, line 517:
<comment>Hardcoded "Group" text is not localized. Use `t()` instead of embedding English strings directly.</comment>
<file context>
@@ -450,10 +514,12 @@ const RoundRobinHosts = ({
<div className="-mb-4 flex items-center justify-between">
<div className="flex items-center gap-1">
- <span className="text-default text-sm font-medium">{`Group ${hostGroups.length + 1}`}</span>
+ <span className="text-default text-sm font-medium">{`Group ${
+ hostGroups.length + 1
+ }`}</span>
</file context>
| <span className="text-default text-sm font-medium">{`Group ${ | |
| + <span className="text-default text-sm font-medium">{`${t("group")} ${ |
| } = useSearchTeamMembers({ | ||
| teamId, | ||
| search: debouncedSearch, | ||
| enabled: true, |
There was a problem hiding this comment.
P2: useSearchTeamMembers is always enabled, so it will fetch paginated team members even when the assignment state hides the member selector (e.g., assign-all or segment modes). This adds unnecessary network requests and processing. Consider enabling the query only when the selector is rendered.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/modules/event-types/components/AddMembersWithSwitch.tsx, line 319:
<comment>`useSearchTeamMembers` is always enabled, so it will fetch paginated team members even when the assignment state hides the member selector (e.g., assign-all or segment modes). This adds unnecessary network requests and processing. Consider enabling the query only when the selector is rendered.</comment>
<file context>
@@ -260,10 +297,27 @@ export function AddMembersWithSwitch({
+ } = useSearchTeamMembers({
+ teamId,
+ search: debouncedSearch,
+ enabled: true,
+ });
const {
</file context>
| const utils = trpc.useUtils(); | ||
| const [isDownloading, setIsDownloading] = useState(false); | ||
|
|
||
| const handleDownloadCsv = useCallback(async () => { |
There was a problem hiding this comment.
P2: handleDownloadCsv has try/finally but no catch. A failed fetch silently swallows the error with no user feedback. Add a catch to show an error toast.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/modules/event-types/components/EditWeightsForAllTeamMembers.tsx, line 217:
<comment>`handleDownloadCsv` has `try/finally` but no `catch`. A failed fetch silently swallows the error with no user feedback. Add a `catch` to show an error toast.</comment>
<file context>
@@ -94,81 +101,152 @@ const TeamMemberItem = ({ member, onWeightChange }: TeamMemberItemProps) => {
+ const utils = trpc.useUtils();
+ const [isDownloading, setIsDownloading] = useState(false);
+
+ const handleDownloadCsv = useCallback(async () => {
+ setIsDownloading(true);
+ try {
</file context>
| {isPlatform && ( | ||
| <Icon | ||
| name="user" | ||
| {valueFromGroup.length >= 1 && ( |
There was a problem hiding this comment.
P2: The scroll listener won’t attach if the selected-host list starts empty because the container ref is only mounted after useFetchMoreOnScroll has already returned. This breaks infinite scrolling once hosts are added later.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/eventtypes/components/CheckedTeamSelect.tsx, line 143:
<comment>The scroll listener won’t attach if the selected-host list starts empty because the container ref is only mounted after `useFetchMoreOnScroll` has already returned. This breaks infinite scrolling once hosts are added later.</comment>
<file context>
@@ -98,93 +129,118 @@ export const CheckedTeamSelect = ({
- {isPlatform && (
- <Icon
- name="user"
+ {valueFromGroup.length >= 1 && (
+ <div
+ ref={scrollContainerRef}
</file context>
| setPendingChanges( | ||
| { | ||
| ...current, | ||
| hostsToRemove: [...current.hostsToRemove, userId], |
There was a problem hiding this comment.
P3: Guard against duplicate userIds in hostsToRemove so repeated remove actions don’t bloat the delta or issue redundant deleteMany conditions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/eventtypes/lib/useHostsForEventType.ts, line 169:
<comment>Guard against duplicate userIds in hostsToRemove so repeated remove actions don’t bloat the delta or issue redundant deleteMany conditions.</comment>
<file context>
@@ -0,0 +1,258 @@
+ setPendingChanges(
+ {
+ ...current,
+ hostsToRemove: [...current.hostsToRemove, userId],
+ hostsToUpdate: current.hostsToUpdate.filter((u) => u.userId !== userId),
+ },
</file context>
Devin AI is addressing Cubic AI's review feedbackNew feedback has been sent to the existing Devin session. ✅ Pushed commit |
…devin/1771970971-improve-event-type-page-frontend
- useEffect: only initialize weights on sheet open transition, not on value changes - CSV upload: use email-to-member Map for O(n+m) lookup instead of O(n*m) - useWatch for pendingChildrenChanges instead of non-reactive getValues - Add data-testid attributes to Fixed/RR hosts labels - E2E: add URL assertion after navigation, use data-testid selectors Co-Authored-By: unknown <>
Review annotations from original PR #27371These are the review annotations from @joeauyeung on the original PR, carried over to this split PR for reviewer context.
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| import type { FormValues, Host, HostLocation } from "@calcom/features/eventtypes/lib/types"; | ||
| import { useHosts } from "@calcom/features/eventtypes/lib/HostsContext"; | ||
| import { usePaginatedAssignmentHosts } from "@calcom/features/eventtypes/lib/usePaginatedAssignmentHosts"; | ||
| import { useFetchMoreOnScroll } from "@calcom/features/eventtypes/lib/useFetchMoreOnScroll"; |
There was a problem hiding this comment.
Extracted to be used in other places
| const useHostLocationHandlers = ( | ||
| formMethods: ReturnType<typeof useFormContext<FormValues>>, | ||
| hosts: Host[], | ||
| serverHosts: Host[], |
There was a problem hiding this comment.
One of the main changes in this PR is instead of writing all hosts to the react hook form, we instead track changed host values and write that to the form.
| setAssignAllTeamMembers, | ||
| isRoundRobinEvent = false, | ||
| customClassNames, | ||
| serverHosts, |
There was a problem hiding this comment.
Instead of the whole hosts array being passed, these are the hosts from the paginated results
| // No-op: when "assign all" is toggled ON, the assignAllTeamMembers flag | ||
| // is set by the AssignAllTeamMembers component. The booking system resolves | ||
| // all members at booking time, so we don't need to create individual host entries. | ||
| const handleFixedHostsActivation = useCallback(() => {}, []); |
There was a problem hiding this comment.
When "Assign all members" is selected, we don't need to iterate through the team's members and add them to the react hook form. We can just send this flag to the update endpoint and handle this logic there
| usePaginatedAssignmentChildren, | ||
| assignmentChildToChildrenEventType, | ||
| } from "@calcom/features/eventtypes/lib/usePaginatedAssignmentChildren"; | ||
| import { usePaginatedAssignmentHosts } from "@calcom/features/eventtypes/lib/usePaginatedAssignmentHosts"; |
There was a problem hiding this comment.
Rather than writing all hosts to the react hook form, we just track the changed hosts
| Host, | ||
| SelectClassNames, | ||
| } from "@calcom/features/eventtypes/lib/types"; | ||
| import { useHosts } from "@calcom/features/eventtypes/lib/HostsContext"; |
There was a problem hiding this comment.
We don't need the controller component now that we're not relying on the form's hosts array
| const formMethods = useFormContext<FormValues>(); | ||
| const eventTypeId = formMethods.getValues("id"); | ||
|
|
||
| const [search, setSearch] = useState(""); |
There was a problem hiding this comment.
Add ability to search for a specific host. If an admin has to make a change to a host's schedule on the event type, they usually have a person in mind.
| import { useHosts } from "@calcom/features/eventtypes/lib/HostsContext"; | ||
| import { useFetchMoreOnScroll } from "@calcom/features/eventtypes/lib/useFetchMoreOnScroll"; | ||
| import { | ||
| usePaginatedAvailabilityHosts, |
There was a problem hiding this comment.
Implement pagination in the availability tab
| export type EventSetupTabProps = Pick< | ||
| EventTypeSetupProps, | ||
| "eventType" | "locationOptions" | "team" | "teamMembers" | "destinationCalendar" | ||
| "eventType" | "locationOptions" | "team" | "destinationCalendar" |
There was a problem hiding this comment.
Remove the dependency to pass all team members
| const debouncedSearch = useDebounce(search, 300); | ||
|
|
||
| const { | ||
| options: searchOptions, |
There was a problem hiding this comment.
Instead of mapping through all team members we just map through the displayed options
| customClassNames, | ||
| groupId, | ||
| hosts = [], | ||
| onSearchChange, |
There was a problem hiding this comment.
Implement pagination and searching for host assignment
| // This prevents all tab hooks (including watch("hosts") with 700 hosts) from running on every render | ||
| const tabMap = { | ||
| setup: ( | ||
| setup: () => ( |
There was a problem hiding this comment.
Remove dependency on expecting all team members. Most of the time, a user doesn't need to access all tabs of the event type settings.
| * Provider that manages hosts state efficiently using delta tracking. | ||
| * Wrap your event type form with this provider to enable efficient host management. | ||
| */ | ||
| export function HostsProvider({ children }: { children: ReactNode }) { |
There was a problem hiding this comment.
Now instead of storing all host data in the react hook form, we can use this context to track the changed hosts.
| import type React from "react"; | ||
| import { useCallback, useEffect } from "react"; | ||
|
|
||
| export const useFetchMoreOnScroll = ( |
There was a problem hiding this comment.
Used for the paginated hosts components
| }; | ||
| }; | ||
|
|
||
| export function usePaginatedAssignmentChildren({ |
There was a problem hiding this comment.
This hook is used when assigning users to a managed event type.
| avatarUrl: string | null; | ||
| }; | ||
|
|
||
| export function usePaginatedAssignmentHosts({ |
There was a problem hiding this comment.
This hook is used when assigning hosts to a round robin or collective event
| avatarUrl: string | null; | ||
| }; | ||
|
|
||
| export function usePaginatedAvailabilityHosts({ |
There was a problem hiding this comment.
This hook is used on the availability tab
| role: MembershipRole; | ||
| }; | ||
|
|
||
| export function useSearchTeamMembers({ |
There was a problem hiding this comment.
This hook is used to search for team members to add to the event type. This searches for the specific member in the DB rather than loading all members to the page.
| * | ||
| * This dramatically improves performance for event types with many hosts. | ||
| */ | ||
| export function useHostsForEventType() { |
There was a problem hiding this comment.
I specifically left comments in this file to help explain the hooks used when determining changed hosts and whether to write them to the form.
(@eunjae-lee's review comment on the field comparison in setHosts: what if we refactor this part a bit like ["isFixed", "priority", ...].forEach ?)
| metadata: eventType.metadata, | ||
| hosts: eventType.hosts.sort((a, b) => sortHosts(a, b, eventType.isRRWeightsEnabled)), | ||
| // Delta-based host changes for performance - only track changes, not all 700+ hosts | ||
| pendingHostChanges: { |
There was a problem hiding this comment.
Instead of adding all hosts to the react hook form, we just track the changed hosts
| isTeamEventType, | ||
| assignedUsers, | ||
| hosts, | ||
| childrenCount, |
There was a problem hiding this comment.
Instead of relying on the whole array of hosts and assigned users to the managed event type, we just pass the count
| "data-testid": "webhooks", | ||
| }); | ||
| return navigation; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps -- formMethods is excluded intentionally to avoid recalculating on every form change |
There was a problem hiding this comment.
Since the form methods are changing constantly with tracking changed hosts this is going to cause re-renders. Instead this should rely on specific variables like the ones already in this array.
| </div> | ||
| <div className="mt-4 text-sm"> | ||
| <MatchingTeamMembers teamId={teamId} queryValue={queryValue} filterMemberIds={filterMemberIds} /> | ||
| <MatchingTeamMembers teamId={teamId} queryValue={queryValue} /> |
There was a problem hiding this comment.
Remove relying on the fully loaded team members to display matching members to the attribute filter
| import { useTeamMembers } from "../../hooks/teams/useTeamMembers"; | ||
| import { useAtomHostSchedules } from "../hooks/useAtomHostSchedules"; | ||
|
|
||
| type EventAvailabilityTabPlatformWrapperProps = { |
There was a problem hiding this comment.
The availability tab no longer relies on the team members array
…devin/1771970971-improve-event-type-page-frontend
Move pendingHostChanges/pendingChildrenChanges processing to frontend PR. This includes: - Delta type definitions (PendingHostChanges, PendingChildrenChanges, etc.) - Zod schemas for delta input validation - Delta processing logic in update.handler.ts - Legacy host path preserved for backward compatibility Co-Authored-By: unknown <>
| eventTypeColor, | ||
| users, | ||
| children, | ||
| pendingChildrenChanges, |
There was a problem hiding this comment.
@eunjae-lee's review comment from PR #27371: can we use a repository instead of direct prisma usage
(Moved from backend PR #28156 since delta-based saving logic was relocated to this frontend PR)
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/features/eventtypes/lib/types.ts">
<violation number="1" location="packages/features/eventtypes/lib/types.ts:62">
P2: `HostUpdate` (frontend state) and `HostUpdateInput` (API input) have inconsistent nullability for `priority` and `weight`: `HostUpdate` uses `number | undefined` while `HostUpdateInput` uses `number | null | undefined`. This misalignment means null values from the backend can't be round-tripped through `HostUpdate`, potentially causing lost updates or unexpected `undefined` vs `null` semantics.</violation>
<violation number="2" location="packages/features/eventtypes/lib/types.ts:311">
P2: `PendingChildrenChangesInput.childrenToAdd` is missing the `eventTypeSlugs: string[]` field that exists in the analogous `ChildInput` type. If the backend handler requires `eventTypeSlugs` when creating child event types (as the existing `ChildInput` contract implies), children added via the delta path will silently omit this data.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| userId: number; | ||
| isFixed?: boolean; | ||
| priority?: number; | ||
| weight?: number; |
There was a problem hiding this comment.
P2: HostUpdate (frontend state) and HostUpdateInput (API input) have inconsistent nullability for priority and weight: HostUpdate uses number | undefined while HostUpdateInput uses number | null | undefined. This misalignment means null values from the backend can't be round-tripped through HostUpdate, potentially causing lost updates or unexpected undefined vs null semantics.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/eventtypes/lib/types.ts, line 62:
<comment>`HostUpdate` (frontend state) and `HostUpdateInput` (API input) have inconsistent nullability for `priority` and `weight`: `HostUpdate` uses `number | undefined` while `HostUpdateInput` uses `number | null | undefined`. This misalignment means null values from the backend can't be round-tripped through `HostUpdate`, potentially causing lost updates or unexpected `undefined` vs `null` semantics.</comment>
<file context>
@@ -53,6 +53,32 @@ export type Host = {
+ userId: number;
+ isFixed?: boolean;
+ priority?: number;
+ weight?: number;
+ scheduleId?: number | null;
+ groupId?: string | null;
</file context>
| }; | ||
|
|
||
| export type PendingChildrenChangesInput = { | ||
| childrenToAdd: { owner: { id: number; name: string; email: string }; hidden: boolean }[]; |
There was a problem hiding this comment.
P2: PendingChildrenChangesInput.childrenToAdd is missing the eventTypeSlugs: string[] field that exists in the analogous ChildInput type. If the backend handler requires eventTypeSlugs when creating child event types (as the existing ChildInput contract implies), children added via the delta path will silently omit this data.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/eventtypes/lib/types.ts, line 311:
<comment>`PendingChildrenChangesInput.childrenToAdd` is missing the `eventTypeSlugs: string[]` field that exists in the analogous `ChildInput` type. If the backend handler requires `eventTypeSlugs` when creating child event types (as the existing `ChildInput` contract implies), children added via the delta path will silently omit this data.</comment>
<file context>
@@ -278,6 +290,30 @@ export type HostInput = {
+};
+
+export type PendingChildrenChangesInput = {
+ childrenToAdd: { owner: { id: number; name: string; email: string }; hidden: boolean }[];
+ childrenToRemove: number[];
+ childrenToUpdate: { userId: number; hidden?: boolean }[];
</file context>
| childrenToAdd: { owner: { id: number; name: string; email: string }; hidden: boolean }[]; | |
| childrenToAdd: { owner: { id: number; name: string; email: string; eventTypeSlugs: string[] }; hidden: boolean }[]; |
Devin AI is addressing Cubic AI's review feedbackNew feedback has been sent to the existing Devin session. View Devin Session✅ No changes pushed |
…devin/1771970971-improve-event-type-page-frontend
…tend PR The getEventTypeById changes (removing hosts/children/teamMembers from initial load, using _count, MembershipRepository for currentUserMembership, getEventTypeByIdWithTeamMembers wrapper) now live in the frontend PR. This keeps the backend PR purely additive with no return type changes. Co-Authored-By: unknown <>
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/features/eventtypes/lib/getEventTypeById.ts">
<violation number="1" location="packages/features/eventtypes/lib/getEventTypeById.ts:254">
P1: N+1 query pattern: `enrichUserWithItsProfile` is called once per team member inside `Promise.all`, firing N parallel DB queries. A bulk variant already exists in `UserRepository` (~line 695) that fetches all profiles in a single `ProfileRepository.findManyForUsers(userIds)` call. Use that instead to keep this O(1) database round-trips regardless of team size.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Devin AI is addressing Cubic AI's review feedbackNew feedback has been sent to the existing Devin session. |
…devin/1771970971-improve-event-type-page-frontend
…ation to findTeamMembersMatchingAttributeLogic Co-Authored-By: unknown <>
|
Fixed the N+1 query pattern in ✅ Pushed commit |
1 participant
What does this PR do?
This is the frontend half of PR #27371, split for easier review. It depends on the backend PR #28156 (paginated endpoints & new repository methods) being merged first.
Note: Several pieces were moved here from the backend PR to keep that PR purely additive (no existing return type changes):
pendingHostChanges/pendingChildrenChangesprocessing inupdate.handler.ts)getEventTypeByIdoptimizations (removinghosts,children,teamMembersfrom initial load)findTeamMembersMatchingAttributeLogicpagination supporteventTypeRepositoryquery optimizations (removingmembers,hosts,childrenfrom select, adding_count)Key changes
Frontend:
usePaginatedAssignmentHosts,usePaginatedAvailabilityHosts,useSearchTeamMembers) fetch hosts on-demand with infinite scrollHostsProvidercontext tracks add/update/remove deltas instead of full host arrays@tanstack/react-virtualfor smooth scrolling with large host liststeamMembersprop: No longer passed to components; fetched on-demand via paginated searchEditWeightsForAllTeamMembersrefactored: Uses tRPCexportHostsForWeightsendpoint directly andupdateHostfromHostsContextfor weight changesrevalidateEventTypeEditPagemoved toonSettledto prevent tab reset after saveteam-event-type-assignment.e2e.tsBackend (query optimizations, moved from #28156):
getEventTypeById: Removedhosts,children,team.membersfrom initial queries; replaced with_count; usesMembershipRepositoryforcurrentUserMembershipgetEventTypeByIdWithTeamMembers: New wrapper function that fetches team members separately using bulkenrichUsersWithTheirProfiles(single DB round-trip viaProfileRepository.findManyForUsers)eventTypeRepository: OptimizedfindById/findByIdForOrgAdminqueries to excludemembers,hosts,childrenselects; added_countfor childrenfindTeamMembersMatchingAttributeLogic: Addedcursor,limit,searchinput fields and paginated response (nextCursor,total)Backend (delta saving logic):
update.handler.ts: NewpendingHostChangespath processes host add/update/remove deltas andclearAllHosts; legacy fullhostsarray path preserved for Platform API backward compatibilitypendingChildrenChanges: Reconstructs children array from deltas for managed event types (clearAllChildren,childrenToAdd,childrenToRemove,childrenToUpdate)pendingHostChangesSchema,pendingChildrenChangesSchema,hostUpdateSchemaadded to tRPC typesPendingHostChanges,PendingChildrenChanges,HostUpdate,HostUpdateInputadded topackages/features/eventtypes/lib/types.ts;FormValues.hostsreplaced withpendingHostChanges/pendingChildrenChanges;TabMapvalues changed to() => React.ReactNodeUpdates since last revision
getEventTypeByIdWithTeamMembers: Replaced per-memberenrichUserWithItsProfilecalls (N parallel DB queries) with bulkenrichUsersWithTheirProfiles(singleProfileRepository.findManyForUsersquery)isOrgEventTypevariable ingetEventTypeById(only used in the new wrapper function)findTeamMembersMatchingAttributeLogicto main, then re-applied pagination here)Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Prerequisites:
Test scenarios:
hostsarray (backward compatibility)Run E2E tests locally:
Human Review Checklist
Frontend:
hostAnytype assertion inAddMembersWithSwitch.tsx: Line ~120 castshost as Host & { name?: string | null; email?: string; avatarUrl?: string | null }. Confirm backend actually returns these fields from paginated endpoints, or the fallback label logic will break.effectiveHostCountcomputation: InEventTypePlatformWrapper.tsx, the calculation handlesclearAllHosts,hostsToAdd, andhostsToRemove. Check edge cases like: clearing all then adding some, removing more than exist, etc.useTeamMembersWithSegmentPlatformhook is deleted,teamMembersprop removed from several components. Confirm no platform consumers are broken. The atom service now usesgetEventTypeByIdWithTeamMembersinstead ofgetEventTypeById.updateHost: InEditWeightsForAllTeamMembers.handleSave, weight changes useupdateHostfromHostsContext. Verify this correctly tracks changes inpendingHostChangesfor hosts beyond page 1.useFetchMoreOnScrolluses IntersectionObserver. Test in nested scrollable containers (e.g., modal inside modal) to ensure it triggers correctly.revalidateEventTypeEditPagefromonSuccesstoonSettleddoesn't cause stale data issues for other sessions.Backend (query optimizations):
getEventTypeByIdWithTeamMemberstype safety: TheenrichedUserMap.get()fallback tomembership.userwon't have theprofileproperty, but downstream code accessesuser.profile.id. In practice the map should always contain every user (bulk enrichment processes all), but verify TypeScript doesn't complain.getEventTypeByIdWithTeamMembersinstead ofgetEventTypeById. TheteamMembersfield is only returned by the wrapper. Verify this doesn't break any platform consumers.enrichUsersWithTheirProfilesactually does a singleProfileRepository.findManyForUsers(userIds)call instead of N queries.Backend (delta saving logic):
clearAllHostsdelta computation: Inupdate.handler.tslines ~640-714, verify the logic correctly handles: hosts to delete (existing but NOT inhostsToAdd), hosts to update (in BOTH), hosts to add (only inhostsToAdd). Test edge case: clear all, then add some.resolvedChildrenlogic: Lines ~996-1062 reconstruct children frompendingChildrenChanges. Verify edge cases: no changes (preserve all),clearAllChildren(replace withchildrenToAdd), partial changes (apply deltas).hostspath preserved: Verify Platform API can still save with fullhostsarray (backward compatibility). Theif (pendingHostChanges)vselse if (hosts)branching should handle both paths.hostLocationDeletionsarray is built for both delta and legacy paths. Verify locations are correctly deleted whenhost.location === null.teamMemberIds.includes(host.userId). With 700+ members, this is O(n×m). Consider converting toSetfor O(1) lookups (Cubic AI flagged this).requiresCancellationReasonfield removed: This field was removed from the Zod schema. Verify no callers send this field or handle the removal gracefully.Checklist
Link to Devin run: https://app.devin.ai/sessions/91be6db4b8794a4e8f11e2fa2649a2d5
Requested by: @joeauyeung