Skip to content

Comments

perf: Improve event type page speed by paginating hosts#27371

Closed
joeauyeung wants to merge 51 commits intomainfrom
improve-event-type-page
Closed

perf: Improve event type page speed by paginating hosts#27371
joeauyeung wants to merge 51 commits intomainfrom
improve-event-type-page

Conversation

@joeauyeung
Copy link
Contributor

@joeauyeung joeauyeung commented Jan 29, 2026

What does this PR do?

This PR significantly improves the performance of the event type settings page for teams with many hosts (tested with 700 hosts). The changes address slow form initialization and tab switching by implementing lazy rendering, paginated data fetching, and delta-based state management.

https://www.loom.com/share/28bd91df784f41b5af00dc1f80a8d276

Performance Benchmark Results (700 Hosts)

Metric Before After Improvement
Tab initialization 2.16ms 0.16ms 13.7x faster
Initial payload 211.78 KB 60.66 KB 71.4% smaller
Host lookup operations 2.05ms (O(n²)) 0.30ms (O(n)) 7x faster

Key Changes

1. Lazy Tab Rendering

  • Tabs are now functions that return JSX, only called when the tab is active
  • Prevents all tab hooks from running on every render

2. Removal of teamMembers from Initial Load

  • The teamMembers array (~151 KB for 700 members) is no longer loaded upfront
  • Team members are fetched on-demand via paginated search endpoint

3. Paginated Host Fetching

  • New cursor-paginated tRPC endpoints: getAssignmentHosts, getAvailabilityHosts, searchTeamMembers
  • Frontend hooks with infinite scroll fetch hosts on demand
  • Virtualized lists using @tanstack/react-virtual for smooth scrolling

4. Delta-Based Host Updates

  • New HostsProvider context tracks add/update/remove deltas via pendingHostChanges
  • Update handler accepts deltas while maintaining backward compatibility with full hosts array for Platform API

Updates since last revision (Eunjae's review feedback)

Refactor: Repetitive field comparison in setHosts replaced with loop (Comment 23)

  • Replaced ~20 lines of repetitive if (newHost.X !== serverHost.X) blocks with a COMPARABLE_FIELDS array and for...of loop
  • location still handled separately since it uses areLocationsEqual rather than strict equality
  • File: packages/features/eventtypes/lib/useHostsForEventType.ts

Refactor: Direct Prisma query in update.handler.ts moved to repository (Comment 24)

  • Extracted ctx.prisma.eventType.findMany({ where: { parentId } }) into EventTypeRepository.findChildrenByParentId()
  • File: packages/features/eventtypes/repositories/eventTypeRepository.ts, packages/trpc/server/routers/viewer/eventTypes/heavy/update.handler.ts

Refactor: searchTeamMembers handler Prisma calls moved to MembershipRepository (Comment 25)

  • Added searchMembers() method to MembershipRepository for paginated team member search
  • Handler now uses membershipRepo.hasMembership() (existing method) for auth check and membershipRepo.searchMembers() for the query
  • File: packages/features/membership/repositories/MembershipRepository.ts, packages/trpc/server/routers/viewer/eventTypes/searchTeamMembers.handler.ts

Refactor: Added paginated search to UserRepository (Comment 26)

  • New findByIdsWithPagination() method applies search/cursor/limit at DB level instead of loading all users into memory
  • Replaces JS-side filtering in findTeamMembersMatchingAttributeLogic.handler.ts
  • File: packages/features/users/repositories/UserRepository.ts, packages/trpc/server/routers/viewer/attributes/findTeamMembersMatchingAttributeLogic.handler.ts

Previous updates (managed event type fixes)

Fix: Managed event children deleted when admin saves without children changes

  • Root cause: When pendingChildrenChanges is undefined, resolvedChildren stayed undefined and handleChildrenEventTypes interpreted this as "delete all children"
  • Fix: Always fetch existing children from DB when children param is undefined, then apply any pending deltas on top
  • Files: packages/trpc/server/routers/viewer/eventTypes/heavy/update.handler.ts

Fix: Tab resets to "Basics" after saving on a non-default tab

  • Root cause: revalidateEventTypeEditPage in onSuccess calls revalidatePath which temporarily empties useSearchParams, causing useTypedQuery to reset tabName to the default ("setup")
  • Fix: Move revalidateEventTypeEditPage from onSuccess to onSettled (after tRPC query invalidations complete), so server cache revalidation doesn't race with tab navigation
  • Files: apps/web/modules/event-types/components/EventTypeWebWrapper.tsx

Both fixes resolve the 2 failing E2E tests in managed-event-types.e2e.ts — all 16 tests now pass locally.

Previous updates (E2E tests & weight fix)

Added: E2E tests for team event type assignment (apps/web/playwright/team-event-type-assignment.e2e.ts)

  • 14 tests across 4 suites: Assignment Tab, Round Robin Weights, Host Assignment/Removal, Managed Event Assignment
  • Tests create 40-member teams to exercise pagination (limit is 20 per page)
  • Added scrollToHost() helper that scrolls overflow-y-auto containers to trigger infinite scroll loading for hosts beyond page 1
  • All 14 tests pass locally

Fix: Weight changes via "Edit weights" sheet not persisting for paginated hosts

  • Root cause: EditWeightsForAllTeamMembers.handleSave only processed hosts from the paginated value prop, losing changes for hosts on page 2+
  • Fix: Modified handleSave to use updateHost from HostsContext for individual weight changes, which properly tracks them in pendingHostChanges regardless of pagination state
  • Refactored component to use tRPC directly instead of useTeamMembersWithSegment hooks

Previous updates (cubic's review feedback - 9 issues)

Fix: clearAllHosts flag not checked in hasHostChanges (#9)

  • hasHostChanges() now returns true when clearAllHosts is set

Fix: Redundant DB query when clearing all hosts (#8)

  • Reuses eventType.hosts already fetched earlier instead of making a separate findMany query

Fix: Sequential await in loop replaced with Promise.all (#5)

  • Parallelized host deletion operations in clearAllHosts path using Promise.all

Fix: Duplicate hosts in hostsToAdd (#6)

  • Added Set-based deduplication check when adding hosts via addHost()

Fix: O(n²) host lookup in setHosts replaced with Map (#7)

  • Changed from Array.find() to Map lookup for O(n) complexity

Fix: Hardcoded fallback label bypasses localization (#4)

  • Changed "Team Member" to t("team_member") in CheckedHostField

Fix: Missing error handling in Segment.tsx (#3)

  • Added isError state handling to display error message when query fails

Fix: Toggle off fixed hosts only removes loaded pages (#1 - P1)

  • Added clearAllHosts flag to PendingHostChanges type
  • Backend computes delta: keeps hosts in hostsToAdd, removes all others from DB

Fix: assignedOwnerIds only tracks children from loaded pages (#2)

  • Added pendingChildrenChanges with clearAllChildren flag for managed event types

Type definitions centralized

  • Added HostUpdateInput, PendingHostChangesInput, PendingChildrenChangesInput to packages/features/eventtypes/lib/types.ts

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. N/A - internal performance optimization
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  1. Create or use an event type with a large number of hosts (ideally 700+)
  2. Navigate to the event type settings page
  3. Verify the page loads quickly without freezing
  4. Switch between tabs and verify smooth transitions
  5. In the Team tab:
    • Scroll through the host list and verify virtualization works
    • Search for team members and verify pagination loads more results
    • Add/remove hosts and verify changes are saved correctly
  6. Test scheduling type change:
    • With many hosts assigned, change scheduling type (e.g., COLLECTIVE → ROUND_ROBIN)
    • Verify all hosts are cleared (not just the first page)
    • Add new hosts and save
    • Verify only the newly added hosts are assigned
  7. Test weight dialog (important for new fix):
    • Open "Edit weights" sheet on a Round Robin event with 30+ hosts
    • Scroll down to find a host beyond position 20 (e.g., teammate-30)
    • Change their weight and save
    • Reload the page and verify the weight persisted
  8. Test managed event types:
    • With many children assigned, toggle "assign all team members" off
    • Verify all children are removed (not just loaded pages)
  9. Test tab persistence (new fix):
    • Navigate to a managed event type's Assignment tab
    • Make a change and save
    • Verify you stay on the Assignment tab (not reset to Basics)
  10. Verify existing functionality still works for event types with few hosts

Run E2E tests locally:

PLAYWRIGHT_HEADLESS=1 yarn e2e team-event-type-assignment.e2e.ts
PLAYWRIGHT_HEADLESS=1 yarn e2e managed-event-types.e2e.ts

Human Review Checklist

  • Verify hasMembership behavioral change: The refactored searchTeamMembers handler now uses hasMembership() which checks accepted: true, whereas the original handler didn't check membership acceptance status. Confirm this is the intended behavior (more secure - only accepted members can search).
  • Verify Prisma in + gt combination: In MembershipRepository.searchMembers, when both memberUserIds and cursor are provided, it uses { in: memberUserIds, gt: cursor } on the same field. Confirm Prisma handles this correctly (should filter to IDs in the array AND greater than cursor).
  • Verify findByIdsWithPagination performance: The new method does an extra count query when limit is provided. For large datasets, this could be slow. Consider if the total count is actually needed by callers.
  • Verify moving revalidateEventTypeEditPage from onSuccess to onSettled doesn't cause stale data issues for other sessions
  • Verify children preservation logic in update.handler.ts (lines 996-1062) correctly handles: no changes (preserve all), clearAllChildren (replace with childrenToAdd), partial changes (apply deltas)
  • Verify EditWeightsForAllTeamMembers.handleSave correctly tracks weight changes via updateHost from HostsContext
  • Verify the scrollToHost() E2E helper reliably triggers pagination loading (scroll containers with overflow-y-auto)
  • Verify clearAllHosts delta computation in update.handler.ts correctly handles: hosts to delete (existing but NOT in hostsToAdd), hosts to update (in BOTH), hosts to add (only in hostsToAdd)
  • Check backward compatibility: Platform API should still work with full hosts array
  • Verify virtualized host lists maintain accessibility (keyboard navigation, screen readers)
  • Check pagination edge cases: empty results, single page, cursor boundaries

Checklist

  • I have read the contributing guide
  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have checked if my changes generate no new warnings
  • My PR is large (>500 lines) - consider splitting if reviewers request it

Link to Devin run: https://app.devin.ai/sessions/01bf278940f745cdbd4dc6c8af2038d8
Link to Devin run (merge conflict resolution): https://app.devin.ai/sessions/24a418ad1a3b4f2c9c7ecbcc62d350ae
Link to Devin run (cubic review fixes): https://app.devin.ai/sessions/df4d6e11f27c47beac0715750900622d
Link to Devin run (E2E tests & weight fix): https://app.devin.ai/sessions/c5b9a3599c1f403dbb5d08e02b5ea8b4
Link to Devin run (Eunjae review refactors): https://app.devin.ai/sessions/39f5fe04887743f1a0c7655094e807f5
Requested by: joe@cal.com (@joeauyeung)

joeauyeung and others added 7 commits January 28, 2026 16:58
Instead of storing all 700+ hosts in React Hook Form state, track only
changes via pendingHostChanges (hostsToAdd, hostsToUpdate, hostsToRemove).
The backend update handler accepts deltas directly while maintaining
backward compatibility with the full hosts array for Platform API consumers.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add cursor-based paginated tRPC endpoints to fetch hosts without loading
all of them upfront. Includes repository methods with keyset pagination
and user name/avatar data for direct display.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add usePaginatedAvailabilityHosts, usePaginatedAssignmentHosts, and
useFetchMoreOnScroll utility for cursor-based infinite scrolling of
hosts in the event type editor tabs.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…hosts

Refactor EventTeamAssignmentTab, HostLocations, HostEditDialogs, and
EventType to consume paginated host data from context instead of form
state. Use hostCount instead of hosts array for empty assignment checks.
Change tab rendering to lazy functions to avoid mounting all tab hooks.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Use host name/avatarUrl from paginated responses directly instead of
looking up team members by userId. Remove teamMembers prop from
EventAvailabilityTab and its web/platform wrappers.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add searchTeamMembers tRPC endpoint with cursor-based pagination and
name/email search. Update CheckedTeamSelect to support server-side
filtering and infinite scroll. Replace static teamMembers prop in
AddMembersWithSwitch with the new useSearchTeamMembers hook.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Stop loading all 700+ team members in the initial getEventTypeById
query. Remove members select from repository queries, replace with
targeted queries for current user membership and child owner roles.
Remove teamMembers prop threading from web and platform wrappers.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@devin-ai-integration
Copy link
Contributor

Performance Benchmark Results (700 Hosts)

I ran benchmarks comparing form initialization and tab switching performance between main and this branch with 700 hosts. Here are the findings:

Key Performance Improvements

Metric Main Branch This Branch Improvement
Tab initialization 2.16ms 0.16ms 13.7x faster
Initial payload 211.78 KB 60.66 KB 71.4% smaller
Host lookup operations 2.05ms (O(n²)) 0.30ms (O(n)) 7x faster

How the Optimizations Achieve These Results

1. Lazy Tab Rendering (13.7x faster initialization)

Before (main branch):

const tabMap = {
  setup: <EventSetupTab eventType={eventType} teamMembers={teamMembers} />,
  availability: <EventAvailabilityTab eventType={eventType} teamMembers={teamMembers} />,
  team: <EventTeamAssignmentTab teamMembers={teamMembers} />,
  // All tabs instantiated upfront
};

After (this branch):

const tabMap = {
  setup: () => <EventSetupTab eventType={eventType} />,
  availability: () => <EventAvailabilityTab eventType={eventType} />,
  team: () => <EventTeamAssignmentTab teamMembers={[]} />,
  // Tabs are functions, only called when active
};

// In render:
{tabMap[tabName]?.()}

2. Removal of teamMembers from Initial Load (71.4% payload reduction)

  • The 700-member teamMembers array (~151 KB) is no longer passed to tabs on initial load
  • Team members are now fetched on-demand via paginated endpoints
  • This significantly reduces memory usage and initial render time

3. Paginated Host Fetching

  • Hosts are fetched via cursor-paginated tRPC endpoints instead of loading all upfront
  • Frontend hooks with infinite scroll and virtualization fetch hosts on demand
  • Reduces initial page load time and memory footprint

4. HostsProvider Context with Delta Tracking

  • Dedicated context for efficient host state management
  • Tracks add/update/remove deltas instead of full host arrays
  • More efficient state updates that don't trigger unnecessary re-renders

Benchmark Script

A benchmark script was created to measure these improvements:

npx tsx apps/web/modules/event-types/components/__benchmarks__/runBenchmark.ts

Summary

For event types with 700 hosts, users should experience:

  • Faster initial page load
  • Smoother tab switching
  • Reduced memory usage
  • Better responsiveness when editing hosts

Copy link
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 3 potential issues.

View issues and 12 additional flags in Devin Review.

Open in Devin Review

devin-ai-integration bot and others added 20 commits January 30, 2026 02:47
Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
When scheduling type changes, all hosts need to be cleared. Previously,
only paginated hosts were removed which caused a bug when there were
more hosts than fit in one page.

This adds a clearAllHosts flag to PendingHostChanges that:
- Signals the backend to compute the delta between existing hosts and hostsToAdd
- Keeps hosts that are in both sets (updates their properties)
- Removes hosts that are not in hostsToAdd
- Adds hosts that are only in hostsToAdd

The frontend now calls clearAllHosts() when scheduling type changes,
and the usePaginatedAssignmentHosts hook properly shows only hostsToAdd
when clearAllHosts is true.

Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
…teration

With paginated host loading, not all hosts are available client-side.
Query hasFixedHosts on the first page from the DB and pass it through
to FixedHosts so it can determine fixed host state from server data
combined with pending changes.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
hostCount was using the static eventType._count.hosts from the initial
query, so adding/removing hosts without saving would not trigger the
empty assignment warning on navigation. Derive effectiveHostCount from
pendingHostChanges to account for unsaved additions and removals.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
team.members was fetched with take: 0 (always empty), so teamMembers
was always []. Remove the vestigial data flow: repository members
select, enrichment loop, teamMembers mapping, and all downstream
prop threading. Replace team.members usages (child owner roles,
currentUserMembership) with targeted queries.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add a wrapper function that calls getEventTypeById then fetches and
enriches team members separately. The atoms service uses this to
restore teamMembers with platform-managed user filtering, while the
web tRPC handler continues using the leaner getEventTypeById.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Include membership role in the searchTeamMembers response (was already
selected from DB but dropped). Expose raw members from the hook so
consumers can access role data. Remove the childOwnerMemberships batch
query from getEventTypeById since role will come from the paginated
search instead. Make ChildrenEventType.profile optional for new
children added from search results.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Wire up useSearchTeamMembers to the ChildrenEventTypes component with
debounced search and infinite scroll pagination. Transform search
results into ChildrenEventType options with role from the paginated
query. Add server-side search and scroll-to-load props to
ChildrenEventTypeSelect, matching the pattern used by CheckedTeamSelect
for host assignment.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove the two sequential enrichUserWithItsProfile loops (children
owners and event type users) from getEventTypeById. No consumer uses
the enriched profile on event type users, and children enrichment
moves to pagination. Replace children select with _count in the
repository. Return childrenCount instead of enriched children array.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…checks

Replace assignedUsers array with childrenCount number in
checkForEmptyAssignment and useHandleRouteChange. Remove the
EventTypeAssignedUsers type. Add PendingChildrenChanges type and
pendingChildrenChanges to FormValues. Update both web and platform
wrappers to pass childrenCount and reset pending changes on save.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
… event types

Add getChildrenForAssignment paginated tRPC endpoint with repository
method. Create usePaginatedAssignmentChildren hook following the same
pattern as hosts. Update ChildrenEventTypes component to load existing
children via pagination and track changes via pendingChildrenChanges
delta. Add pendingChildrenChanges to the update schema and reconstruct
the full children array on the backend from DB + delta before passing
to handleChildrenEventTypes.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9 issues found across 59 files

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<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:215">
P1: Bug: toggling off fixed hosts only removes fixed hosts from loaded pages, not all pages. With 700+ hosts and pagination (limit=20), only the first page's fixed hosts are removed. The scheduling type change correctly uses `clearAllHosts()` to handle this scenario — a similar flag-based approach should be used here so the backend can compute the full delta across all hosts.</violation>

<violation number="2" location="apps/web/modules/event-types/components/tabs/assignment/EventTeamAssignmentTab.tsx:750">
P2: Potential duplicate adds: `assignedOwnerIds` only tracks children from loaded pages (pagination). Already-assigned children on unloaded pages will still appear as dropdown options, potentially causing duplicate additions. Consider adding a server-side deduplication check in the add handler, or use a separate lightweight query to get all assigned user IDs.</violation>
</file>

<file name="packages/features/Segment.tsx">

<violation number="1" location="packages/features/Segment.tsx:148">
P2: Missing error handling after switching to useInfiniteQuery can mask query failures by rendering an empty list instead of an error state. Add a guard so that when the query errors (data undefined and not pending), the UI shows the error message.</violation>
</file>

<file name="apps/web/modules/event-types/components/AddMembersWithSwitch.tsx">

<violation number="1" location="apps/web/modules/event-types/components/AddMembersWithSwitch.tsx:126">
P3: Hardcoded fallback label `User ${host.userId}` bypasses localization. Use `t("user")` (or another existing key) for the fallback label instead of a raw English string.</violation>
</file>

<file name="packages/features/eventtypes/lib/getEventTypeById.ts">

<violation number="1" location="packages/features/eventtypes/lib/getEventTypeById.ts:255">
P2: Sequential `await` in loop causes O(n) round-trips for profile enrichment. For the 700-host scenario this PR targets, this means 700 sequential async calls in `getEventTypeByIdWithTeamMembers`. Use `Promise.all` to parallelize.</violation>
</file>

<file name="packages/features/eventtypes/lib/useHostsForEventType.ts">

<violation number="1" location="packages/features/eventtypes/lib/useHostsForEventType.ts:86">
P2: `addHost` does not check for duplicates in `hostsToAdd`. If called twice with the same `userId` (e.g., double-click, re-render), the host will be added twice. Consider guarding with an existence check before appending.</violation>

<violation number="2" location="packages/features/eventtypes/lib/useHostsForEventType.ts:208">
P2: O(n²) host lookup in `setHosts`: `serverHosts.find()` inside the `for` loop is O(n) per iteration, making the whole block O(n²). With 700 hosts this means ~490k comparisons. Build a `Map` from `serverHosts` for O(1) lookups, consistent with the PR's own stated goal of eliminating O(n²) operations.</violation>
</file>

<file name="packages/trpc/server/routers/viewer/eventTypes/heavy/update.handler.ts">

<violation number="1" location="packages/trpc/server/routers/viewer/eventTypes/heavy/update.handler.ts:492">
P2: Redundant database query — `eventType.hosts` (loaded at the top of the handler) already contains all host `userId`s for this event type. In a PR optimizing for 700+ hosts, this extra round-trip is counter-productive. Reuse the data you already have.</violation>
</file>

<file name="packages/platform/atoms/event-types/hooks/useEventTypeForm.ts">

<violation number="1" location="packages/platform/atoms/event-types/hooks/useEventTypeForm.ts:385">
P2: `hasHostChanges` ignores the `clearAllHosts` flag, so a “clear all hosts” action with empty delta arrays won’t be sent to the backend.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

devin-ai-integration bot and others added 3 commits February 11, 2026 20:52
Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
…est mocks

Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
@github-actions
Copy link
Contributor

Devin AI is resolving merge conflicts

This PR has merge conflicts with the main branch. A Devin session has been created to automatically resolve them.

View Devin Session

Devin will:

  1. Merge the latest main into this branch
  2. Resolve any conflicts intelligently
  3. Run lint/type checks to ensure validity
  4. Push the resolved changes

If you prefer to resolve conflicts manually, you can close the Devin session and handle it yourself.

…ence for paginated hosts

Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 2 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.


<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: Rule violated: **E2E Tests Best Practices**

Add expect(page).toHaveURL() after page.goto() to comply with the E2E best-practice requirement for fast-fail on unexpected redirects.</violation>

<violation number="2" location="apps/web/playwright/team-event-type-assignment.e2e.ts:85">
P2: Rule violated: **E2E Tests Best Practices**

Avoid text locators in E2E tests per E2E Tests Best Practices; use data-testid and page.getByTestId for these elements to prevent brittle tests.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

}

async function navigateToAssignmentTab(page: Page, eventId: number) {
await page.goto(`/event-types/${eventId}?tabName=team`);
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Rule violated: E2E Tests Best Practices

Add expect(page).toHaveURL() after page.goto() to comply with the E2E best-practice requirement for fast-fail on unexpected redirects.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/playwright/team-event-type-assignment.e2e.ts, line 45:

<comment>Add expect(page).toHaveURL() after page.goto() to comply with the E2E best-practice requirement for fast-fail on unexpected redirects.</comment>

<file context>
@@ -0,0 +1,408 @@
+}
+
+async function navigateToAssignmentTab(page: Page, eventId: number) {
+  await page.goto(`/event-types/${eventId}?tabName=team`);
+  await page.waitForLoadState("networkidle");
+  const form = page.locator("#event-type-form");
</file context>
Fix with Cubic


const form = await navigateToAssignmentTab(page, teamEvent.id);

await expect(form.getByText("Fixed hosts").first()).toBeVisible();
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Rule violated: E2E Tests Best Practices

Avoid text locators in E2E tests per E2E Tests Best Practices; use data-testid and page.getByTestId for these elements to prevent brittle tests.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/playwright/team-event-type-assignment.e2e.ts, line 85:

<comment>Avoid text locators in E2E tests per E2E Tests Best Practices; use data-testid and page.getByTestId for these elements to prevent brittle tests.</comment>

<file context>
@@ -0,0 +1,408 @@
+
+    const form = await navigateToAssignmentTab(page, teamEvent.id);
+
+    await expect(form.getByText("Fixed hosts").first()).toBeVisible();
+    await scrollToHost(form, TARGET_HOST, page);
+  });
</file context>
Fix with Cubic

@github-actions
Copy link
Contributor

Devin AI is addressing Cubic AI's review feedback

A Devin session has been created to address the issues identified by Cubic AI.

View Devin Session

@devin-ai-integration
Copy link
Contributor

Reviewed the two Cubic AI comments on apps/web/playwright/team-event-type-assignment.e2e.ts:

  1. Violation 1 (line 45) — Add expect(page).toHaveURL() after page.goto() → Confidence 8/10
  2. Violation 2 (line 85) — Avoid text locators, use data-testid → Confidence 8/10

Both are below the 9/10 confidence threshold, so no changes were made.

…ring revalidation

- Move revalidateEventTypeEditPage from onSuccess to onSettled to prevent
  revalidatePath from resetting useSearchParams during tab navigation
- Always fetch existing children from DB when pendingChildrenChanges is
  undefined, preventing accidental deletion of child managed event types

Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
@joeauyeung joeauyeung marked this pull request as ready for review February 18, 2026 15:00
@joeauyeung joeauyeung requested review from a team as code owners February 18, 2026 15:00
@graphite-app graphite-app bot added the enterprise area: enterprise, audit log, organisation, SAML, SSO label Feb 18, 2026
@graphite-app graphite-app bot requested a review from a team February 18, 2026 15:00
Copy link
Contributor

@eunjae-lee eunjae-lee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

massive job ! overall the code looks good. left some comments. let me test it and will leave more review.

Comment on lines 216 to 235
if (newHost.isFixed !== serverHost.isFixed) {
changes.isFixed = newHost.isFixed;
hasChanges = true;
}
if (newHost.priority !== serverHost.priority) {
changes.priority = newHost.priority;
hasChanges = true;
}
if (newHost.weight !== serverHost.weight) {
changes.weight = newHost.weight;
hasChanges = true;
}
if (newHost.scheduleId !== serverHost.scheduleId) {
changes.scheduleId = newHost.scheduleId;
hasChanges = true;
}
if (newHost.groupId !== serverHost.groupId) {
changes.groupId = newHost.groupId;
hasChanges = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if we refactor this part a bit like

["ixFixed", "priority", ...].forEach ?

// Reconstruct children array from pendingChildrenChanges or preserve existing children
let resolvedChildren = children;
if (!children) {
const existingChildren = await ctx.prisma.eventType.findMany({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use a repository instead of direct prisma usage

const { teamId, cursor, limit, search, memberUserIds } = input;

// Verify the requesting user is a member of this team
const callerMembership = await ctx.prisma.membership.findFirst({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's move this to a repository

fallbackWarnings,
troubleshooter: troubleshooter,
result: matchingTeamMembers.map((user) => ({
const sortedMembers = matchingTeamMembers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we update findByIds or create a new method that supports search & limit & cursor out of the box, so that we don't fetch everything and filter at the app level?

Copy link
Contributor

@Udit-takkar Udit-takkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it be possible that we make these perf improvements incrementally in multiple PRs?

@joeauyeung
Copy link
Contributor Author

@Udit-takkar I thought about this but since all of the areas this PR touches is deeply dependent on the initial query for hosts and members it's hard to break this into multiple PRs without causing double network calls.

I already discussed this with @volnei and @eunjae-lee

@Udit-takkar
Copy link
Contributor

@joeauyeung there are some new trpc endpoints with tests. We can reivew and merge them first and then everything together

- Comment 23: Refactor repetitive field comparison in setHosts to use
  COMPARABLE_FIELDS loop instead of individual if blocks
- Comment 24: Move direct Prisma query in update.handler.ts to
  EventTypeRepository.findChildrenByParentId method
- Comment 25: Move searchTeamMembers handler Prisma calls to
  MembershipRepository (hasMembership + searchMembers methods)
- Comment 26: Add UserRepository.findByIdsWithPagination for DB-level
  search/cursor/limit instead of JS-side filtering

Co-Authored-By: unknown <>
);
};

const useFetchMoreOnScroll = (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted to be used in other places

Comment on lines -712 to 717
formMethods.setValue(
"hosts",
hosts.map((host) => ({ ...host, location: null })),
{ shouldDirty: true }
);
// Use setHosts from context instead of form setValue for performance
setHosts(serverHosts, hosts.map((host) => ({ ...host, location: null })));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines -565 to -571
const userTimeZone = extractHostTimezone({
userId: eventType.userId,
teamId: eventType.teamId,
hosts: eventType.hosts,
owner: eventType.owner,
team: eventType.team,
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used to display the expiry date of private links. Since we're moving away from relying on the whole hosts and team members in the event type pages, we can just use the owner for personal event types or the perspective of the logged in user to display these times.

customClassNames,
teamId,
isSegmentApplicable,
serverHosts,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of the whole hosts array being passed, these are the hosts from the paginated results

Comment on lines -166 to +171
const handleFixedHostsActivation = useCallback(() => {
const currentHosts = getValues("hosts");
setValue(
"hosts",
teamMembers.map((teamMember) => {
const host = currentHosts.find((host) => host.userId === parseInt(teamMember.value, 10));
return {
isFixed: true,
userId: parseInt(teamMember.value, 10),
priority: host?.priority ?? 2,
weight: host?.weight ?? 100,
// if host was already added, retain scheduleId and groupId
scheduleId: host?.scheduleId || teamMember.defaultScheduleId,
groupId: host?.groupId || null,
};
}),
{ shouldDirty: true }
);
}, [getValues, setValue, teamMembers]);
// 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(() => {}, []);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines -222 to +221
min={0}
min={1}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a weight is set to 0, should just remove the host

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dumb component for searching for hosts

innerClassNames={customClassNames?.assignToSelect?.innerClassNames}
value={value}
isMulti
{...(onSearchChange
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implement pagination and searching for specific children event types

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@volnei 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.


const matchingTeamMembers = filterMemberIds
? allMatchingTeamMembers?.filter((member) => filterMemberIds.includes(member.id))
: allMatchingTeamMembers;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove relying on the fully loaded team members to display matching members to the attribute filter

@joeauyeung
Copy link
Contributor Author

@volnei @eunjae-lee what are your thoughts on @Udit-takkar's comment here?
#27371 (comment)

@eunjae-lee
Copy link
Contributor

@joeauyeung

@volnei @eunjae-lee what are your thoughts on @Udit-takkar's comment here? #27371 (comment)

Yeah I think that's doable. Also something I tried in the past as well. Pure additions to the backend that are not used anywhere first, and then the second part of the project that uses those backend elements. Can you try Devin this?

@joeauyeung
Copy link
Contributor Author

@joeauyeung joeauyeung closed this Feb 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core area: core, team members only enterprise area: enterprise, audit log, organisation, SAML, SSO ready-for-e2e size/XXL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants