feat: implement system filter segments for data tables#22939
feat: implement system filter segments for data tables#22939eunjae-lee merged 42 commits intomainfrom
Conversation
- Add defaultSegmentId column to UserFilterSegmentPreference table - Support mixed segment ID types (number for user segments, string for default segments) - Add DefaultFilterSegment and CombinedFilterSegment types - Update useSegments hook to handle default segments with date range recalculation - Modify FilterSegmentSelect to group and display default segments separately - Add default segments to bookings view (My Bookings, Upcoming Bookings) - Prevent editing/deleting of default segments in SaveFilterSegmentButton - Update DataTableProvider to support defaultSegments prop - Update parsers and repository to handle mixed segment ID types Implements frontend-only default segments as specified in the requirements. Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
- Update eslint-config-next and @typescript-eslint packages to latest versions - Fix dependency compatibility issues that were blocking pre-commit hooks Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
🤖 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:
|
WalkthroughThis PR implements typed system vs user segments across the data-table feature (new types, system segment IDs, provider and hooks changes), adds a "My Bookings" system segment (locale key and server-side session -> userId propagation to bookings), updates repository and Prisma schema to persist either system or user segment preferences, and adds Playwright helpers and e2e tests covering system segments and bookings behavior. Assessment against linked issues
Out-of-scope changes
Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
- Update SegmentIdentifier to use discriminated union with 'custom' vs 'default' types
- Refactor setSegmentId to accept object parameters: { id: string; type: 'default' } | { id: number; type: 'custom' }
- Update type definitions with DefaultFilterSegment, CustomFilterSegment, and CombinedFilterSegment
- Modify useSegments hook to handle new segment type structure
- Update FilterSegmentSelect component to work with discriminated unions
- Refactor database preference handling to store segment type alongside ID
- Add proper type safety throughout the data flow
- Remove description property from DefaultFilterSegment type
Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
…redSegmentId
The test was expecting preferredSegmentId to be a number, but after the discriminated union refactor it now returns { id: number, type: 'custom' } for custom segments. Updated the assertion to use toEqual() for deep object comparison.
Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
- Remove null from SegmentIdentifier type definition since null/undefined indicate absence of identifier - Update DataTableProvider and useSegments to handle SegmentIdentifier | null properly - Fix type safety while maintaining discriminated union functionality - Ensure setAndPersistSegmentId accepts null values for clearing segments Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
…ntification - Update 'my_bookings' to 'default-my_bookings' - Update 'upcoming-bookings' to 'default-upcoming-bookings' - Makes it easier to verify segment type by looking at the ID string - Maintains all existing discriminated union functionality Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
…refix for segment IDs Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
… better semantics Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
…scriminated union Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
… all component files Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
…type naming Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
…:calcom/cal.com into devin/1754492880-default-filter-segments
|
Related Documentation No published documentation to review for changes on this repository. How did I do? Any feedback? |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/20/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add foundation team as reviewer" took an action on this PR • (08/20/25)1 reviewer was added to this PR based on Keith Williams's automation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/web/playwright/filter-helpers.ts (1)
143-156: Missing await can race submenu open/interaction and cause flakiness.openSegmentSubmenu is async but not awaited. Subsequent clicks may happen before the submenu appears.
Apply this diff:
export async function deleteSegment(page: Page, segmentName: string) { - openSegmentSubmenu(page, segmentName); + await openSegmentSubmenu(page, segmentName); await page.getByTestId("filter-segment-select-submenu-content").getByText("Delete").click();packages/lib/server/repository/filterSegment.type.ts (1)
1-14: Align FilterSegmentsListResponse.preferredSegmentId with the new identifier typeThe public type still declares number | null while tests and repository intend a discriminated union. Update it to SegmentIdentifier | null to avoid drift across layers.
Outside this hunk, replace:
preferredSegmentId: number | null;with:
preferredSegmentId: SegmentIdentifier | null;packages/features/data-table/components/segment/SaveFilterSegmentButton.tsx (1)
98-101: Team selection handling: avoid 0 sentinel; make the guard null-safeUsing
0as a fallback conflates “no selection” with a valid numeric value and trips the falsy check on submit. Preferundefinedfor “no team selected”, a null-safe guard, and parse only when an option exists.Apply these diffs:
- if (isTeamSegment && !selectedTeamId) { + if (isTeamSegment && selectedTeamId == null) { showToast(t("please_select_team"), "error"); return; }- teamId: selectedTeamId || 0, + teamId: selectedTeamId as number,- onChange={(option) => setSelectedTeamId(parseInt(option?.value || "0"))} + onChange={(option) => + setSelectedTeamId(option ? parseInt(option.value, 10) : undefined) + }Also applies to: 133-139, 220-226
packages/lib/server/repository/filterSegment.ts (1)
347-394: DB-Level Exclusivity Constraint NeededThe unique composite on
(userId, tableIdentifier)is already in place, but there’s no database‐level check to prevent bothsegmentIdandsystemSegmentIdfrom being non‐NULL simultaneously. Please add a CHECK constraint to enforce that exactly one of these columns is set.– In
packages/prisma/migrations/20250812084523_add_system_segment_id/migration.sql, after adding the column, append:ALTER TABLE "UserFilterSegmentPreference" ADD CONSTRAINT user_filter_segment_preference_exclusive_segments CHECK ( (segmentId IS NULL)::boolean <> (systemSegmentId IS NULL)::boolean );– (Optional) If using Prisma’s
@@checkpreview feature, updatepackages/prisma/schema.prisma:model UserFilterSegmentPreference { id Int @id @default(autoincrement()) userId Int tableIdentifier String segmentId Int? systemSegmentId String? createdAt DateTime @default(now()) updatedAt DateTime @updatedAt @@unique([userId, tableIdentifier]) @@index([userId]) @@index([segmentId]) + @@check([ "(segmentId IS NULL)::boolean <> (systemSegmentId IS NULL)::boolean" ]) }Ensure this constraint is applied via a new migration so the DB enforces the exclusivity.
packages/features/data-table/lib/types.ts (1)
290-301: SegmentStorage still assumes numeric IDs — breaks with system segmentsWith system segments introducing string IDs,
SegmentStorageandZSegmentStorageshould support string IDs. As-is, it will drop or reject system segment preferences.Apply this minimal compatible change:
export type SegmentStorage = { [tableIdentifier: string]: { - segmentId: number; + segmentId: string | number; }; }; export const ZSegmentStorage = z.record( z.string(), z.object({ - segmentId: z.number(), + segmentId: z.union([z.string(), z.number()]), }) ) satisfies z.ZodType<SegmentStorage>;If you prefer stronger typing, you could store the full SegmentIdentifier object in storage, but the above is the least invasive change.
packages/features/data-table/DataTableProvider.tsx (1)
287-290: Bug: setPageIndexWrapper treats 0 as nullUsing
||will coerce 0 to null. Use nullish coalescing to preserve 0.Apply:
- const setPageIndexWrapper = useCallback( - (newPageIndex: number | null) => setPageIndex(newPageIndex || null), + const setPageIndexWrapper = useCallback( + (newPageIndex: number | null) => setPageIndex(newPageIndex ?? null), [setPageIndex] );
🧹 Nitpick comments (26)
packages/features/data-table/lib/parsers.ts (1)
21-21: All clear: no legacy sentinel or numeric conversions remainWe searched the entire data-table stack and found:
- No
segmentId === -1or-1 === segmentIdcomparisons- No
Number(segmentId),parseInt(segmentId), orsegmentId as numbercasts- No
segmentId: numbertypings
parseAsString.withDefault("")is consistently applied in:
• packages/features/data-table/DataTableProvider.tsx (line 20)
• packages/features/data-table/lib/parsers.ts (line 21)
• packages/features/data-table/lib/serializers.ts (lines 8, 24)Optional refactor: centralize URL‐to‐SegmentIdentifier logic in one helper (e.g.,
toSegmentIdentifier(raw: string): SegmentIdentifier | undefined) that enforces:function toSegmentIdentifier(raw: string): SegmentIdentifier | undefined { const v = (raw ?? "").trim(); if (!v || v === "-1") return undefined; // legacy “no selection” if (v.startsWith(SYSTEM_SEGMENT_PREFIX)) { return { type: "system", id: v }; } if (/^\d+$/.test(v)) { return { type: "user", id: Number(v) }; } return { type: "system", id: v }; // forward‐compatible }Then replace direct
segmentIdParserusages with this single normalizer to avoid future drift.packages/lib/hooks/useElementByClassName.ts (1)
8-15: Optionally re-resolve when DOM changes (MutationObserver) to avoid stale refs.Right now, the ref is only set when className changes. If the target element mounts/unmounts dynamically under the same class, elementRef can become stale. Consider observing DOM mutations to keep the ref up to date.
Apply this diff to make the ref resilient to dynamic DOM changes and add a defensive document check:
useEffect(() => { - if (className) { - const element = document.getElementsByClassName(className)[0] as T; - elementRef.current = element || null; - } else { - elementRef.current = null; - } + if (!className) { + elementRef.current = null; + return; + } + if (typeof document === "undefined") { + elementRef.current = null; + return; + } + const update = () => { + const element = document.getElementsByClassName(className)[0] as T | undefined; + elementRef.current = element ?? null; + }; + // Initial resolve + update(); + // Keep ref fresh on DOM changes + const observer = new MutationObserver(update); + observer.observe(document.body, { childList: true, subtree: true }); + return () => observer.disconnect(); }, [className]);packages/features/data-table/hooks/useSegmentsNoop.ts (1)
1-2: Nit: avoid importing lodash root for a noop.No need to bypass the restricted import rule here. Inline a tiny noop to avoid pulling lodash at all.
Outside the selected lines, consider replacing the import with:
// Remove the lodash import and ESLint disable const noop = () => {};apps/web/playwright/filter-helpers.ts (2)
200-205: Use locator assertion for “cleared” state to reduce text parsing fragility.Directly reading textContent and trimming is brittle. Prefer Playwright’s toHaveText with a regex to normalize whitespace.
Apply this diff:
export async function expectSegmentCleared(page: Page) { - // Check that no segment is selected (button shows default text) - const segmentSelect = page.getByTestId("filter-segment-select"); - const buttonText = await segmentSelect.textContent(); - expect(buttonText?.trim()).toEqual("Segment"); + // Check that no segment is selected (button shows default text) + await expect(page.getByTestId("filter-segment-select")).toHaveText(/^\s*Segment\s*$/); }
184-195: Consider asserting by testid for group headers to avoid i18n brittleness.Hard-coding "Default" assumes English. If possible, expose a data-testid for the group header and assert against it instead of visible text.
apps/web/playwright/insights.e2e.ts (1)
205-206: Guard against empty usernames to prevent selection timeouts.If member.username is falsy, applySelectFilter will wait for a blank option and stall.
Apply this diff:
- await applySelectFilter(page, "userId", member.username || ""); + expect(member.username, "Expected created member to have a username").toBeTruthy(); + await applySelectFilter(page, "userId", member.username!);apps/web/playwright/system-segments.e2e.ts (6)
7-11: Parallel mode + global cleanup can cause cross-test interferenceRunning the suite in parallel while calling users.deleteAll() after each test risks deleting users created by other concurrently running tests (depending on fixture scoping). This can lead to flaky tests.
Consider one of:
- Scope cleanup to only the user(s) created within the current test.
- Run this describe block serially if the cleanup is necessarily global.
- Ensure the users fixture isolates per-worker DB/state so deleteAll() cannot affect other workers. Verify by running with 2+ workers and stressing user creation across tests.
16-23: Harden navigation wait logic to avoid race conditionsThe current pattern can miss the intended response or wait on an unrelated one if multiple matching requests occur. Tighten the matcher and coordinate navigation/response waits atomically.
Apply this diff to make the wait more explicit and robust:
async function navigateToBookings(page: Page, status = "upcoming") { - // Wait for the bookings API response like existing tests do - const bookingsGetResponse = page.waitForResponse((response) => - /\/api\/trpc\/bookings\/get.*/.test(response.url()) - ); - await page.goto(`/bookings/${status}`, { waitUntil: "domcontentloaded" }); - await bookingsGetResponse; + // Wait for the bookings API GET response and URL change atomically + await Promise.all([ + page.waitForResponse((resp) => { + const url = resp.url(); + return resp.request().method() === "GET" && + /\/api\/trpc\/bookings\/get(\?.*)?$/.test(url) && + resp.ok(); + }), + page.goto(`/bookings/${status}`, { waitUntil: "domcontentloaded" }), + page.waitForURL(new RegExp(`/bookings/${status}(\\?.*)?$`)), + ]); }
38-41: Strengthen assertion: verify the actual user filter, not just presence of any filters in URLChecking only for “activeFilters” is weak and may pass even if the wrong filter is applied. Assert that the userId filter is actually present (or use a UI indicator test-id).
For example (either option):
- Parse the query string and verify the filter payload includes userId/self.
- Or assert on a UI test-id if available, e.g., page.getByTestId("filter-popover-trigger-userId").
If you prefer URL parsing, you could do:
const url = new URL(page.url()); const params = url.searchParams.get("activeFilters"); expect(params).toBeTruthy(); const activeFilters = JSON.parse(decodeURIComponent(params!)); expect(activeFilters).toEqual(expect.arrayContaining([expect.objectContaining({ f: "userId" })]));
49-60: Use count-based assertions for missing items; avoid brittle text selectors
- Using toBeHidden() for items that should not exist can be flaky when the element is removed from the DOM (0 matches). Prefer toHaveCount(0).
- Also, text-based assertions (“Duplicate”, “Rename”, “Delete”) are brittle with i18n. Prefer data-testid if available.
Apply this diff to stabilize the assertions:
- const submenu = page.getByTestId("filter-segment-select-submenu-content"); - await expect(submenu.getByText("Duplicate")).toBeVisible(); - await expect(submenu.getByText("Rename")).toBeHidden(); - await expect(submenu.getByText("Delete")).toBeHidden(); + const submenu = page.getByTestId("filter-segment-select-submenu-content"); + await expect(submenu.getByText("Duplicate")).toBeVisible(); + await expect(submenu.getByText("Rename")).toHaveCount(0); + await expect(submenu.getByText("Delete")).toHaveCount(0);If feasible, expose stable test-ids such as:
- filter-segment-action-duplicate
- filter-segment-action-rename
- filter-segment-action-delete
Then assert via submenu.getByTestId(...), avoiding locale dependencies.
78-97: Optional: assert immediate unselection on toggle before revisitingYou rely on a revisit to detect cleared state. A quick sanity check right after the second select() call can catch UI state regressions earlier.
Example:
await selectSegment(page, "My Bookings"); // toggles off await expectSegmentCleared(page); // assert immediately, then revisit below
111-118: Close overlay deterministically instead of using EscapeKeyboard Escape may close the wrong layer if multiple overlays are present. Prefer clicking outside or using a close button if exposed via test-id.
For example:
await page.locator("body").click({ position: { x: 0, y: 0 } }); // or a dedicated backdrop locator/test-idpackages/lib/server/repository/__tests__/filterSegments/get.test.ts (1)
252-266: Add system-segment preference test coverageGreat coverage for user-segment preference. Add a test where userFilterSegmentPreference stores systemSegmentId and assert repository.get() returns preferredSegmentId: { id: "", type: "system" }.
I can draft the test if you confirm the canonical system segment id values used in the repository.
packages/lib/server/repository/filterSegment.type.ts (1)
3-10: Avoid duplicating types across packagesFilterSegmentOutput is imported from features/data-table types, but SegmentIdentifier is redefined here. If there's a shared type already defined client-side, consider centralizing SegmentIdentifier in a shared package to avoid divergence.
If centralization isn’t practical now, ensure strong unit tests cover the discriminant and id types at the boundary to catch drift early.
apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx (1)
34-35: Drop unnecessary awaits on headers() and cookies()next/headers’ headers() and cookies() are synchronous. The extra await is harmless but unnecessary.
Apply this diff:
- const session = await getServerSession({ req: buildLegacyRequest(await headers(), await cookies()) }); + const session = await getServerSession({ req: buildLegacyRequest(headers(), cookies()) });apps/web/modules/bookings/views/bookings-listing-view.tsx (1)
62-88: System segment (“My Bookings”) shape looks correct; tiny cleanup possible
- The provided SystemFilterSegment is valid and localized.
- Minor: perPage is redundantly set here; useSegments defaults system segments to perPage: 10 already.
Apply this diff to reduce duplication:
return [ { id: "my_bookings", name: t("my_bookings"), type: "system", activeFilters: [ { f: "userId", v: { type: ColumnFilterType.MULTI_SELECT, data: [userId], }, }, ], - perPage: 10, }, ];packages/features/data-table/components/segment/SaveFilterSegmentButton.tsx (1)
113-121: Avoid using 0 as a sentinel for teamId in updatesDefaulting
teamIdto0can cause confusing permission failures downstream. For TEAM segments,teamIdshould always be a valid number.Apply this diff:
- teamId: selectedSegment.teamId || 0, + teamId: selectedSegment.teamId as number,packages/features/data-table/components/segment/DuplicateSegmentDialog.tsx (2)
55-61: Avoid 0 sentinel for teamId when duplicating TEAM segmentsTEAM segments should carry a valid teamId. Using
0invites accidental permission failures.- teamId: teamId ?? 0, + teamId: teamId as number,
6-6: Unify trpc import path for React hooksThis file imports from
@calcom/trpcwhile others (e.g., SaveFilterSegmentButton) use@calcom/trpc/react. For consistency and to avoid ambiguity, prefer the React entry point.-import { trpc } from "@calcom/trpc"; +import { trpc } from "@calcom/trpc/react";packages/features/data-table/components/segment/FilterSegmentSelect.tsx (2)
165-170: Use stable keys for groups instead of indexKeys based on array index can cause unnecessary re-renders and subtle state issues if the groups reorder.
- {segmentGroups.map((group, index) => ( - <div key={index}> + {segmentGroups.map((group, index) => ( + <div key={group.isSystem ? "system" : group.label}> {group.label && ( - <DropdownMenuLabel className={index === 0 ? "" : "mt-2"}>{group.label}</DropdownMenuLabel> + <DropdownMenuLabel className={index === 0 ? "" : "mt-2"}>{group.label}</DropdownMenuLabel> )}If duplicate team names across orgs are possible in this view, consider
key={team:${group.label}}for team groups.
170-183: Inline submenu-item filtering logic could be extractedThe branching on segment type/scope is repeated and adds noise to rendering.
Extract to a helper for readability:
function getSubmenuItemsFor(segment: CombinedFilterSegment, base: SubmenuItem[], isAdminOrOwner: boolean) { if (segment.type === "system") return base.filter((i) => i.enabledForSystemSegment); if (segment.scope === "USER") return base; return base.filter((i) => isAdminOrOwner || !i.adminOnly); }Then:
- let items: SubmenuItem[] = []; - if (segment.type === "system") { - items = submenuItems.filter((item) => item.enabledForSystemSegment); - } else if (segment.type === "user" && segment.scope === "USER") { - items = submenuItems; - } else if (segment.type === "user" && segment.scope === "TEAM") { - items = submenuItems.filter( - (item) => - // Team segments: show if not admin-only or if user is admin/owner - isAdminOrOwner || !item.adminOnly - ); - } + const items = getSubmenuItemsFor(segment, submenuItems, isAdminOrOwner);packages/lib/server/repository/filterSegment.ts (1)
220-251: Don’t rely on client-provided scope for update permission checksThe current permission query branches based on
input.scope. A malicious client could send an incorrect scope causing misleading errors. Safer pattern: fetch by id first, inspect the actual scope/teamId, then run a permission check branch based on stored values.High-level approach:
- SELECT the segment by id with scope, teamId, userId (and team membership as needed).
- If TEAM: ensure user is ADMIN/OWNER of that team.
- If USER: ensure
segment.userId === userId.- Proceed to update with the allowed fields.
packages/features/data-table/DataTableProvider.tsx (4)
6-8: Prefer a smaller deep-equality utility over lodash/isEqualisEqual is fine functionally, but lodash may be restricted for bundle/treeshaking reasons (suppressed here). Consider react-fast-compare or fast-deep-equal to avoid the lint suppression and reduce footprint.
Apply:
-// eslint-disable-next-line no-restricted-imports -import isEqual from "lodash/isEqual"; +import isEqual from "react-fast-compare";
104-109: Guarded tableIdentifier is good; consider localizing or standardizing error messagesThrowing if tableIdentifier is missing is correct. Given this is frontend code, our guidelines prefer localization; if this error can surface to users, wrap with t(). If it’s purely internal, consider a standardized internal error helper instead of a literal string.
- if (!tableIdentifier) { - throw new Error("tableIdentifier is required"); - } + if (!tableIdentifier) { + // Internal developer error; if this can surface to users, use t("table_identifier_required") + throw new Error("tableIdentifier is required"); + }
270-273: Add cleanup for debounced setter to avoid setState on unmountedCancel the debounced function on unmount to prevent leaks.
Add:
useEffect(() => setDebouncedSearchTerm.cancel, [setDebouncedSearchTerm]);
361-385: Guard against nullish activeFilters in canSaveSegmentAlthough nuqs typically returns defaults, we set null to clear the param in some places. Safer to coalesce length.
Apply:
- activeFilters.length > 0 || + (activeFilters?.length ?? 0) > 0 ||
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (23)
apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx(2 hunks)apps/web/modules/bookings/views/bookings-listing-view.tsx(3 hunks)apps/web/modules/insights/insights-view.tsx(2 hunks)apps/web/playwright/filter-helpers.ts(2 hunks)apps/web/playwright/insights.e2e.ts(2 hunks)apps/web/playwright/system-segments.e2e.ts(1 hunks)apps/web/public/static/locales/en/common.json(1 hunks)packages/features/data-table/DataTableProvider.tsx(8 hunks)packages/features/data-table/components/segment/DeleteSegmentDialog.tsx(1 hunks)packages/features/data-table/components/segment/DuplicateSegmentDialog.tsx(5 hunks)packages/features/data-table/components/segment/FilterSegmentSelect.tsx(4 hunks)packages/features/data-table/components/segment/RenameSegmentDialog.tsx(1 hunks)packages/features/data-table/components/segment/SaveFilterSegmentButton.tsx(5 hunks)packages/features/data-table/hooks/useSegments.ts(2 hunks)packages/features/data-table/hooks/useSegmentsNoop.ts(1 hunks)packages/features/data-table/lib/parsers.ts(1 hunks)packages/features/data-table/lib/types.ts(3 hunks)packages/lib/hooks/useElementByClassName.ts(1 hunks)packages/lib/server/repository/__tests__/filterSegments/get.test.ts(1 hunks)packages/lib/server/repository/filterSegment.ts(8 hunks)packages/lib/server/repository/filterSegment.type.ts(1 hunks)packages/prisma/migrations/20250812084523_add_system_segment_id/migration.sql(1 hunks)packages/prisma/schema.prisma(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/data-table/components/segment/RenameSegmentDialog.tsxapps/web/modules/insights/insights-view.tsxpackages/features/data-table/components/segment/DeleteSegmentDialog.tsxpackages/features/data-table/components/segment/SaveFilterSegmentButton.tsxapps/web/modules/bookings/views/bookings-listing-view.tsxapps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsxpackages/features/data-table/components/segment/FilterSegmentSelect.tsxpackages/features/data-table/DataTableProvider.tsxpackages/features/data-table/components/segment/DuplicateSegmentDialog.tsx
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/data-table/components/segment/RenameSegmentDialog.tsxapps/web/modules/insights/insights-view.tsxpackages/features/data-table/lib/parsers.tspackages/features/data-table/components/segment/DeleteSegmentDialog.tsxpackages/lib/hooks/useElementByClassName.tspackages/features/data-table/components/segment/SaveFilterSegmentButton.tsxapps/web/playwright/system-segments.e2e.tsapps/web/playwright/filter-helpers.tspackages/features/data-table/hooks/useSegmentsNoop.tsapps/web/modules/bookings/views/bookings-listing-view.tsxapps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsxpackages/lib/server/repository/__tests__/filterSegments/get.test.tspackages/lib/server/repository/filterSegment.type.tspackages/features/data-table/lib/types.tsapps/web/playwright/insights.e2e.tspackages/features/data-table/components/segment/FilterSegmentSelect.tsxpackages/features/data-table/DataTableProvider.tsxpackages/features/data-table/hooks/useSegments.tspackages/features/data-table/components/segment/DuplicateSegmentDialog.tsxpackages/lib/server/repository/filterSegment.ts
**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/data-table/lib/parsers.tspackages/lib/hooks/useElementByClassName.tsapps/web/playwright/system-segments.e2e.tsapps/web/playwright/filter-helpers.tspackages/features/data-table/hooks/useSegmentsNoop.tspackages/lib/server/repository/__tests__/filterSegments/get.test.tspackages/lib/server/repository/filterSegment.type.tspackages/features/data-table/lib/types.tsapps/web/playwright/insights.e2e.tspackages/features/data-table/hooks/useSegments.tspackages/lib/server/repository/filterSegment.ts
🧠 Learnings (1)
📚 Learning: 2025-07-15T12:59:34.389Z
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Applied to files:
apps/web/modules/bookings/views/bookings-listing-view.tsx
🧬 Code Graph Analysis (12)
apps/web/modules/insights/insights-view.tsx (2)
packages/features/data-table/DataTableProvider.tsx (1)
DataTableProvider(93-424)packages/features/data-table/hooks/useSegments.ts (1)
useSegments(9-66)
packages/features/data-table/components/segment/SaveFilterSegmentButton.tsx (1)
apps/web/app/_trpc/trpc.ts (1)
trpc(7-7)
apps/web/playwright/system-segments.e2e.ts (1)
apps/web/playwright/filter-helpers.ts (4)
selectSegment(118-125)expectSegmentSelected(210-212)expectSegmentCleared(200-205)addFilter(11-14)
apps/web/modules/bookings/views/bookings-listing-view.tsx (3)
packages/features/data-table/lib/types.ts (1)
SystemFilterSegment(260-270)packages/features/data-table/DataTableProvider.tsx (1)
DataTableProvider(93-424)packages/features/data-table/hooks/useSegments.ts (1)
useSegments(9-66)
apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx (2)
packages/features/auth/lib/getServerSession.ts (1)
getServerSession(32-136)apps/web/lib/buildLegacyCtx.tsx (1)
buildLegacyRequest(47-49)
packages/features/data-table/lib/types.ts (1)
packages/lib/server/repository/filterSegment.type.ts (1)
FilterSegmentsListResponse(11-14)
apps/web/playwright/insights.e2e.ts (1)
apps/web/playwright/filter-helpers.ts (2)
applySelectFilter(23-31)clearFilters(83-85)
packages/features/data-table/components/segment/FilterSegmentSelect.tsx (2)
packages/features/data-table/lib/types.ts (4)
CombinedFilterSegment(281-281)FilterSegmentOutput(242-258)SystemFilterSegmentInternal(272-275)UserFilterSegment(277-279)packages/features/data-table/hooks/useDataTable.ts (1)
useDataTable(5-11)
packages/features/data-table/DataTableProvider.tsx (4)
packages/features/data-table/lib/types.ts (4)
CombinedFilterSegment(281-281)SegmentIdentifier(283-283)SystemFilterSegment(260-270)SYSTEM_SEGMENT_PREFIX(8-8)packages/features/data-table/lib/parsers.ts (8)
pageIndexParser(22-22)pageSizeParser(23-23)searchTermParser(24-24)activeFiltersParser(13-15)sortingParser(16-16)columnVisibilityParser(17-19)columnSizingParser(20-20)segmentIdParser(21-21)packages/features/data-table/hooks/useSegments.ts (1)
useSegments(9-66)packages/lib/hooks/useElementByClassName.ts (1)
useElementByClassName(3-18)
packages/features/data-table/hooks/useSegments.ts (4)
packages/features/data-table/lib/types.ts (2)
UseSegments(303-303)SYSTEM_SEGMENT_PREFIX(8-8)packages/lib/server/repository/filterSegment.ts (1)
setPreference(347-394)packages/features/data-table/lib/utils.ts (1)
isDateRangeFilterValue(131-133)packages/features/data-table/lib/dateRange.ts (1)
recalculateDateRange(71-88)
packages/features/data-table/components/segment/DuplicateSegmentDialog.tsx (1)
packages/features/data-table/lib/types.ts (1)
CombinedFilterSegment(281-281)
packages/lib/server/repository/filterSegment.ts (1)
packages/features/data-table/lib/types.ts (6)
FilterSegmentOutput(242-258)SegmentIdentifier(283-283)ZActiveFilters(240-240)ZSortingState(216-216)ZColumnVisibility(220-220)ZColumnSizing(218-218)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Detect changes
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (41)
packages/lib/hooks/useElementByClassName.ts (1)
3-5: Hook API and SSR safety look good.Generic typing and useEffect-based DOM access keep SSR safe. Defaulting T to HTMLDivElement is sensible for general usage.
packages/features/data-table/hooks/useSegmentsNoop.ts (1)
9-13: No stale UseSegments API fields detected
TheuseSegmentsNoopreturn shape correctly matches the new API (segments,preferredSegmentId,isSuccess,setPreference,isSegmentEnabled), and our search found no remaining references to the old properties (setAndPersistSegmentId, etc.). Approving these changes.apps/web/playwright/filter-helpers.ts (1)
34-36: Good stabilization: wait for option before click.The added waitFor reduces flakiness when options render asynchronously.
apps/web/playwright/insights.e2e.ts (1)
6-6: Import cleanup looks good.Switching to the consolidated helpers keeps the test succinct.
packages/features/data-table/components/segment/RenameSegmentDialog.tsx (1)
74-80: LGTM: test-id addition is appropriateAdding data-testid="rename-segment-name" improves testability without affecting behavior. Labels use t(), satisfying i18n guidelines.
packages/lib/server/repository/filterSegment.type.ts (1)
66-69: Good: preference input schema now supports system/user identifiersThis aligns with the new segmentation model and repository.setPreference behavior.
packages/features/data-table/components/segment/DeleteSegmentDialog.tsx (1)
24-26: Correctly clear selection only when deleting the active user segment — LGTMThe guard against clearing system segment selection is correct and aligns with the new typed SegmentIdentifier.
apps/web/modules/insights/insights-view.tsx (2)
10-10: Wiring DataTableProvider with useSegments — LGTMThis matches the new provider API and keeps the segmentation logic centralized.
35-35: Passing useSegments into DataTableProvider — LGTMNo issues. Provider will fall back to pathname for tableIdentifier in this client component.
apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx (1)
38-39: Propagating userId to BookingsList — LGTMType aligns with BookingsProps (number) and the downstream system segment filter.
apps/web/modules/bookings/views/bookings-listing-view.tsx (1)
91-98: Provider integration with systemSegments and tableIdentifier — LGTMExplicit tableIdentifier via pathname is consistent with the new preference scoping; fallback to undefined is safe.
packages/features/data-table/components/segment/SaveFilterSegmentButton.tsx (2)
59-61: Update flow correctly restricted to user segments — LGTMGating update mode and the update/create radio UI behind
selectedSegment?.type === "user"is the right call. This prevents any edit affordances on system segments.Also applies to: 69-70, 154-155, 182-196
74-81: No changes needed:setSegmentIdsupports an optionalCombinedFilterSegmentVerified in packages/features/data-table/DataTableProvider.tsx (line 69):
setSegmentId: (id: SegmentIdentifier | null, providedSegment?: CombinedFilterSegment) => void;Passing both the
{ id: segment.id, type: "user" }identifier and the{ type: "user", …segment }object aligns with this overload.packages/features/data-table/components/segment/DuplicateSegmentDialog.tsx (2)
13-14: Type upgrade to CombinedFilterSegment — LGTMSwitching to
CombinedFilterSegmentfor the dialog prop aligns the component with system/user segment handling.Also applies to: 23-24
37-43: Correctly setting selected segment after duplication — LGTMUsing a typed identifier
{ id, type: "user" }after successful create is consistent with the new model.packages/features/data-table/components/segment/FilterSegmentSelect.tsx (3)
18-24: Type-aware submenu and actions — LGTM
- Strong typing via
CombinedFilterSegmentacross props and callbacks.- Submenu actions correctly disallow rename/delete for system segments while allowing duplicate.
Also applies to: 32-36, 238-239, 270-279
85-139: Grouping and visibility rules look solid
- System segments grouped under “default” and listed first.
- Personal segments separated from team segments, which are grouped by team name.
- Sorting and labeling are consistent with expectations.
184-205: Selection toggle with typed SegmentIdentifier — LGTMThe equality check considers both id and type, and toggling off by setting
nullis correct.packages/features/data-table/hooks/useSegments.ts (2)
25-41: System segment construction and date-range normalization — LGTM
- Stable system IDs via
SYSTEM_SEGMENT_PREFIXalign with the new identifier model.- Date ranges are recomputed on render for non-custom presets, as intended.
42-54: User segment typing and date-range normalization — LGTMAdding
type: "user"and normalizing date-range filters ensures uniform downstream handling.packages/lib/server/repository/filterSegment.ts (2)
1-8: Unified preferredSegmentId with SegmentIdentifier — LGTM
- Public return now includes
preferredSegmentIdas a typed union (system/user).- Selecting only needed fields and parsing with Zod conforms to the repo guidelines.
Also applies to: 14-17, 118-136
64-108: Prisma selects and Zod parsing — LGTMYou consistently use
select(noinclude) and parse untrusted JSON fields. This keeps the repo outputs stable and typed.Also applies to: 176-206, 269-291
packages/features/data-table/lib/types.ts (8)
1-1: Re-export/import of SortingState looks fineThe import and re-export align with tanstack types and keep the public API consistent.
260-271: SystemFilterSegment shape is reasonable; optional fields match provider usageOptional fields for system segments align with the provider’s conditional application logic.
272-276: Internal system segment guarantees perPage/tableIdentifier — good boundaryMaking perPage/tableIdentifier required internally avoids downstream undefined checks.
277-279: UserFilterSegment tagging is correctTagging user segments with type:"user" on top of FilterSegmentOutput is the right disambiguation.
281-284: CombinedFilterSegment and SegmentIdentifier unions look correctThe union types will make downstream narrowing straightforward.
305-309: UseSegmentsProps extension LGTMAllowing both provided user segments and system segments via props aligns with the new flow.
311-322: UseSegmentsReturn API looks coherentReturning typed segments, preferredSegmentId, and a
setPreferencecallback matches DataTableProvider’s needs.
8-8: SYSTEM_SEGMENT_PREFIX usage verified across TS/TSX filesNo literal “system_” was found outside of the
export const SYSTEM_SEGMENT_PREFIX = "system_";definition in packages/features/data-table/lib/types.ts. All system IDs in the TypeScript/TSX code reference the centralized constant—no drift detected.packages/features/data-table/DataTableProvider.tsx (11)
121-129: Initializing query state from preferredSegmentId: LGTMUsing the fetched/pre-provided preferredSegmentId to seed the query param default is correct and keeps URL/state in sync.
141-158: findSelectedSegment logic is fine; handles system/user IDsCorrectly distinguishes system (prefixed strings) and user (numeric) IDs.
160-177: segmentIdObject correctly maps query param to typed SegmentIdentifierThis normalization is useful for downstream consumers.
250-263: Auto-select preferred segment effect works; scope is intentionalThis effect is narrowly scoped to initial preferred selection; fine as-is once the deep-link sync issue is fixed (see comment above).
275-285: Clearing system segment on user edits is a nice touchGood UX: changing filters implicitly clears the system selection.
292-312: Update/remove filter flows look correct and clear system selectionState updates flow through nuqs query state parsers; looks good.
326-333: Page size change clears system selection — consistent with filter updatesBehavior is consistent across state-changing actions.
335-345: clearAll clears segment and preserves excluded filters — goodThis provides a sensible reset while allowing targeted persistence.
347-360: hasStateChanged deep compares are fine; be mindful of optional fieldsUsing equality against possibly undefined fields is intentional and acceptable here.
413-415: Exposing typed segmentId/setSegmentId in context: LGTMThis will simplify downstream consumers dealing with user/system IDs.
129-140: Confirm TRPC filterSegments list & setPreference typesThe
useSegmentsintegration inDataTableProvider.tsxlooks correct and matches the expected return shape. Before final approval, please manually verify that the server-side TRPC router definitions for these endpoints align with yourSegmentIdentifiertype:
viewer.filterSegments.listreturns apreferredSegmentIdof typeSegmentIdentifier(seetypes.tscomment).viewer.filterSegments.setPreferenceaccepts and returns the correctSegmentIdentifier.Inspect the corresponding router definitions and input/output schemas in your server code to ensure full type safety.
packages/features/data-table/components/segment/DuplicateSegmentDialog.tsx
Show resolved
Hide resolved
packages/prisma/migrations/20250812084523_add_system_segment_id/migration.sql
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
apps/web/playwright/insights.e2e.ts (1)
209-209: Good fix: use toHaveURL with auto-wait to deflakeReplacing the raw string check with Playwright’s URL assertion is the right call; it auto-waits for navigation/search params to settle.
packages/lib/server/repository/filterSegment.type.ts (1)
55-58: Export and harden SegmentIdentifier schema (non-empty string, positive int)Make the discriminated union stricter and exportable for reuse to avoid invalid identifiers and drift across layers.
Apply this diff:
-const ZSegmentIdentifier = z.discriminatedUnion("type", [ - z.object({ id: z.string(), type: z.literal("system") }), - z.object({ id: z.number(), type: z.literal("user") }), -]); +export const ZSegmentIdentifier = z.discriminatedUnion("type", [ + z.object({ id: z.string().min(1), type: z.literal("system") }), + z.object({ id: z.number().int().positive(), type: z.literal("user") }), +]); +export type SegmentIdentifier = z.infer<typeof ZSegmentIdentifier>;packages/features/data-table/components/segment/DuplicateSegmentDialog.tsx (1)
69-75: Bug: Duplicating a system segment leaks id into create payload and ignores schema strictnessThe system branch spreads id and any system-only fields into create. Server schemas typically strip/deny unknowns; sending id on create is incorrect and can fail validation.
Apply this diff:
- } else if (segment.type === "system") { - const { type: _type, ...rest } = segment; - createSegment({ - ...rest, - scope: "USER", - name: data.name, - }); - } + } else if (segment.type === "system") { + const { type: _type, id: _id, name: _name, ...rest } = segment; + createSegment({ + ...rest, + scope: "USER", + name: data.name, + }); + }packages/features/data-table/DataTableProvider.tsx (1)
178-181: Fix: keep segment selection in sync with URL/segments (handles deep links and late loads)selectedSegment can remain undefined or stale when segments load after mount or when the segment query param changes via navigation. Add a syncing effect that reacts to [segments, segmentId] and applies/clears selection accordingly. This addresses the earlier concern about missed application of deep-linked segments.
Apply:
@@ }, [isSegmentFetchedSuccessfully]); + // Keep selectedSegment in sync with the URL param and freshly loaded segments + // Intentionally omit `selectedSegment` from deps to avoid loops + useEffect(() => { + if (!segmentId) { + setSelectedSegment(undefined); + return; + } + const found = findSelectedSegment(segmentId); + if (!found) { + // Clear invalid segment ids from the URL/preferences + setSegmentId(null); + return; + } + if (!selectedSegment || selectedSegment.id !== found.id || selectedSegment.type !== found.type) { + const idObj = + found.type === "system" + ? ({ id: found.id, type: "system" } as const) + : ({ id: found.id, type: "user" } as const); + setSegmentId(idObj, found); + } + }, [segments, segmentId]);Also applies to: 250-265
🧹 Nitpick comments (11)
apps/web/playwright/insights.e2e.ts (1)
205-205: Ensure non-empty username & assert the filter is appliedSince
selectOptionValuematches the visible option label (viagetByRole("option", { name: value })), falling back tomember.idwould never find the option. Instead, explicitly assert thatmember.usernameis set, use it for the filter, and then verify the URL:
- Assert
member.usernameis truthy to avoid an empty fallback- Pass the actual username to
applySelectFilter- Add a positive URL assertion to confirm the filter was applied
Apply this diff:
- await applySelectFilter(page, "userId", member.username || ""); + const username = member.username; + expect(username, "member.username must be set to apply the filter").toBeTruthy(); + await applySelectFilter(page, "userId", username!); + await expect(page).toHaveURL(/[?&]userId=/);packages/prisma/migrations/20250812084523_add_system_segment_id/migration.sql (1)
6-13: Good XOR constraint; tighten non-empty check to reject whitespace-only valuesXOR correctly enforces exactly one of segmentId/systemSegmentId. Consider trimming whitespace in the non-empty check to prevent storing " " as a valid systemSegmentId.
Apply this diff:
ALTER TABLE "UserFilterSegmentPreference" ADD CONSTRAINT "UserFilterSegmentPreference_segment_xor_system_chk" CHECK ( (("segmentId" IS NOT NULL AND "systemSegmentId" IS NULL) OR ("segmentId" IS NULL AND "systemSegmentId" IS NOT NULL)) ), ADD CONSTRAINT "UserFilterSegmentPreference_systemSegmentId_nonempty_chk" - CHECK ("systemSegmentId" IS NULL OR length("systemSegmentId") > 0); + CHECK ("systemSegmentId" IS NULL OR length(btrim("systemSegmentId")) > 0);packages/features/data-table/components/segment/DuplicateSegmentDialog.tsx (1)
54-68: Tighten payload: drop server-managed fields and avoid invalid teamId fallback
- Avoid sending createdAt, updatedAt, userId, etc. Keep payload minimal.
- Using teamId ?? 0 risks failing z.number().positive() when teamId is absent. If scope is TEAM, teamId should already be defined; don’t coerce to 0.
Apply this diff:
- const { type: _type, id: _id, name: _name, team: _team, teamId, ...rest } = segment; + const { + type: _type, + id: _id, + name: _name, + team: _team, + teamId, + createdAt: _createdAt, + updatedAt: _updatedAt, + userId: _userId, + ...rest + } = segment; @@ - createSegment({ - ...rest, - teamId: teamId ?? 0, - scope: "TEAM", - name: data.name, - }); + createSegment({ + ...rest, + teamId, // must be defined for TEAM scope + scope: "TEAM", + name: data.name, + });packages/features/data-table/hooks/useSegments.ts (1)
25-31: System segments: stable ID prefix and date-range recalculation look correct
- Prefixing with SYSTEM_SEGMENT_PREFIX avoids collisions in UI state/URLs.
- Recalculating relative date ranges on render keeps “Upcoming” segments current.
If systemSegmentId storage expects the prefixed value (as implied elsewhere), ensure all callers pass the prefixed id inside SegmentIdentifier when persisting preferences.
packages/features/data-table/lib/types.ts (1)
281-284: Revise SegmentStorage to support string-based system segment IDs or deprecateSegmentStorage today is defined as
export type SegmentStorage = Record<string, { segmentId: number }>; export const ZSegmentStorage = z.record( z.string(), z.object({ segmentId: z.number() }) ) satisfies z.ZodType<SegmentStorage>;while SegmentIdentifier allows
{ id: string; type: "system" }. Any persisted “system” segments (string IDs) won’t round-trip.Recommended options:
- Migrate SegmentStorage to use SegmentIdentifier:
- Change to
export type SegmentStorage = Record<string, { segmentId: SegmentIdentifier }>; export const ZSegmentStorage = z.record( z.string(), z.object({ segmentId: z.discriminatedUnion("type", [ z.object({ type: z.literal("system"), id: z.string() }), z.object({ type: z.literal("user"), id: z.number() }), ]), }) ) satisfies z.ZodType<SegmentStorage>;- Or mark SegmentStorage/ZSegmentStorage as legacy (with a clear deprecation comment) and remove in a follow-up if it’s been superseded by server-backed preferences.
packages/features/data-table/DataTableProvider.tsx (6)
363-387: Make canSaveSegment null-safe and consistentMinor hardening: activeFilters can be cleared to default via null when updating query state; use a null-safe length check to avoid edge cases.
Apply:
- return ( - activeFilters.length > 0 || + return ( + (activeFilters?.length ?? 0) > 0 || sorting.length > 0 || Object.keys(columnVisibility).length > 0 || Object.keys(columnSizing).length > 0 || pageSize !== defaultPageSize || searchTerm?.length > 0 );
272-275: Clear system selection when search term changesSearch term is part of segment state. To keep UX consistent, clear a selected system segment when users modify the search term (similar to filters and page size).
Apply:
- const setDebouncedSearchTerm = useMemo( - () => debounce((value: string | null) => setSearchTerm(value ? value.trim() : null), 500), - [setSearchTerm] - ); + const setDebouncedSearchTerm = useMemo( + () => + debounce((value: string | null) => { + clearSystemSegmentSelectionIfExists(); + setSearchTerm(value ? value.trim() : null); + }, 500), + [setSearchTerm, clearSystemSegmentSelectionIfExists] + );
401-406: Optionally clear system selection on sorting/column changesIf the intent is to clear system selection whenever users diverge from the preset, wrap the setters for sorting/visibility/sizing to call clearSystemSegmentSelectionIfExists first.
Apply:
+ const setSortingWithClear: OnChangeFn<SortingState> = useCallback( + (updater) => { + clearSystemSegmentSelectionIfExists(); + setSorting(updater); + }, + [setSorting, clearSystemSegmentSelectionIfExists] + ); + + const setColumnVisibilityWithClear: OnChangeFn<VisibilityState> = useCallback( + (updater) => { + clearSystemSegmentSelectionIfExists(); + setColumnVisibility(updater); + }, + [setColumnVisibility, clearSystemSegmentSelectionIfExists] + ); + + const setColumnSizingWithClear: OnChangeFn<ColumnSizingState> = useCallback( + (updater) => { + clearSystemSegmentSelectionIfExists(); + setColumnSizing(updater); + }, + [setColumnSizing, clearSystemSegmentSelectionIfExists] + ); @@ - setSorting, + setSorting: setSortingWithClear, @@ - setColumnVisibility, + setColumnVisibility: setColumnVisibilityWithClear, @@ - setColumnSizing, + setColumnSizing: setColumnSizingWithClear,
226-228: Nit: avoid writing default page size to the URL when applying a segmentFor consistency with manual changes (where defaults are nulled), avoid setting size to defaultPageSize explicitly when applying a segment equal to the default.
Apply:
- if (segment.perPage !== undefined) { - setPageSize(segment.perPage); - } + if (segment.perPage !== undefined) { + setPageSize(segment.perPage === defaultPageSize ? null : segment.perPage); + } @@ - ] + defaultPageSize, + ]Also applies to: 234-248
141-158: Nit: simplify findSelectedSegment and handle invalid IDs earlyThe SYSTEM_SEGMENT_PREFIX check is redundant if you compare id equality for system segments; also return early on empty/invalid ids for readability.
Apply:
- const findSelectedSegment = useCallback( - (segmentId: string) => { - return segments.find((segment) => { - if ( - segment.type === "system" && - segmentId && - segmentId.startsWith(SYSTEM_SEGMENT_PREFIX) && - segment.id === segmentId - ) { - return true; - } else if (segment.type === "user") { - const segmentIdNumber = parseInt(segmentId, 10); - return segment.id === segmentIdNumber; - } - }); - }, - [segments] - ); + const findSelectedSegment = useCallback( + (rawId: string) => { + if (!rawId) return undefined; + if (rawId.startsWith(SYSTEM_SEGMENT_PREFIX)) { + return segments.find((s) => s.type === "system" && s.id === rawId); + } + const idNum = Number.parseInt(rawId, 10); + if (Number.isNaN(idNum)) return undefined; + return segments.find((s) => s.type === "user" && s.id === idNum); + }, + [segments] + );
106-108: Localization nit: avoid hard-coded text in TSXPer guidelines for .tsx, prefer t() for user-visible strings. If this error is strictly dev-only, consider a short code comment to indicate it's intentionally not localized.
[coding_guidelines]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
apps/web/playwright/insights.e2e.ts(2 hunks)packages/features/data-table/DataTableProvider.tsx(8 hunks)packages/features/data-table/components/segment/DuplicateSegmentDialog.tsx(5 hunks)packages/features/data-table/hooks/useSegments.ts(2 hunks)packages/features/data-table/lib/types.ts(3 hunks)packages/lib/server/repository/filterSegment.type.ts(1 hunks)packages/prisma/migrations/20250812084523_add_system_segment_id/migration.sql(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
apps/web/playwright/insights.e2e.tspackages/features/data-table/lib/types.tspackages/features/data-table/hooks/useSegments.tspackages/lib/server/repository/filterSegment.type.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/playwright/insights.e2e.tspackages/features/data-table/DataTableProvider.tsxpackages/features/data-table/components/segment/DuplicateSegmentDialog.tsxpackages/features/data-table/lib/types.tspackages/features/data-table/hooks/useSegments.tspackages/lib/server/repository/filterSegment.type.ts
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/data-table/DataTableProvider.tsxpackages/features/data-table/components/segment/DuplicateSegmentDialog.tsx
🧬 Code Graph Analysis (4)
apps/web/playwright/insights.e2e.ts (1)
apps/web/playwright/filter-helpers.ts (2)
applySelectFilter(23-31)clearFilters(83-85)
packages/features/data-table/DataTableProvider.tsx (4)
packages/features/data-table/lib/types.ts (4)
CombinedFilterSegment(281-281)SegmentIdentifier(283-283)SystemFilterSegment(260-270)SYSTEM_SEGMENT_PREFIX(8-8)packages/features/data-table/lib/parsers.ts (8)
pageIndexParser(22-22)pageSizeParser(23-23)searchTermParser(24-24)activeFiltersParser(13-15)sortingParser(16-16)columnVisibilityParser(17-19)columnSizingParser(20-20)segmentIdParser(21-21)packages/features/data-table/hooks/useSegments.ts (1)
useSegments(9-66)packages/lib/hooks/useElementByClassName.ts (1)
useElementByClassName(3-18)
packages/features/data-table/components/segment/DuplicateSegmentDialog.tsx (1)
packages/features/data-table/lib/types.ts (1)
CombinedFilterSegment(281-281)
packages/features/data-table/hooks/useSegments.ts (4)
packages/features/data-table/lib/types.ts (2)
UseSegments(298-298)SYSTEM_SEGMENT_PREFIX(8-8)packages/lib/server/repository/filterSegment.ts (1)
setPreference(347-394)packages/features/data-table/lib/utils.ts (1)
isDateRangeFilterValue(131-133)packages/features/data-table/lib/dateRange.ts (1)
recalculateDateRange(71-88)
🔇 Additional comments (13)
apps/web/playwright/insights.e2e.ts (1)
6-6: LGTM: Consolidated filter helper imports are correctSwitching to clearFilters/applySelectFilter aligns with the simplified helper API and reduces test flakiness and boilerplate.
packages/prisma/migrations/20250812084523_add_system_segment_id/migration.sql (2)
2-3: Schema change aligns with dual-ID preference modelAdding systemSegmentId and making segmentId nullable matches the repository upsert logic for system vs user segment preferences.
2-3: Unique constraint verified for UserFilterSegmentPreference upsert keyThe compound unique key on (userId, tableIdentifier) already exists:
- In schema.prisma:
• model UserFilterSegmentPreference has@@unique([userId, tableIdentifier])- In migrations:
•20250526143547_create_user_filter_segment_preference/migration.sqlcreates
CREATE UNIQUE INDEX "UserFilterSegmentPreference_userId_tableIdentifier_key" ON "UserFilterSegmentPreference"("userId","tableIdentifier");No further action needed here.
packages/lib/server/repository/filterSegment.type.ts (1)
60-63: LGTM: Preference input now accepts discriminated SegmentIdentifierSwitching segmentId to ZSegmentIdentifier.nullable() matches the new persistence model.
packages/features/data-table/components/segment/DuplicateSegmentDialog.tsx (2)
41-41: LGTM: setSegmentId now uses discriminated unionSetting { id, type: "user" } aligns with the new SegmentIdentifier shape.
91-97: LGTM: Localized label and added test idUsing t("name") and data-testid improves i18n and testability.
packages/features/data-table/hooks/useSegments.ts (2)
10-17: Fix confirmed: derived isSuccess when providedSegments are usedTreating providedSegments as a successful state prevents consumers from stalling when the query is disabled.
Also applies to: 62-63
42-54: User segments: consistent date-range normalizationNormalizing date-range filters at read time matches system behavior and simplifies downstream comparisons.
packages/features/data-table/lib/types.ts (3)
8-9: Introduces canonical prefix for system segmentsSYSTEM_SEGMENT_PREFIX centralizes ID construction across UI and routing.
260-275: System segment types are well-scoped for UI and transportSystemFilterSegment/Internal clearly separate public inputs (no perPage/tableIdentifier) from internal fields used in the table layer.
281-284: Good: unified CombinedFilterSegment and discriminated SegmentIdentifierThis aligns client/server and reduces ad-hoc ID parsing.
packages/features/data-table/DataTableProvider.tsx (2)
226-231: Good fix: explicit undefined checks for perPage/searchTermSwitching to explicit undefined checks ensures empty-string search terms and zero-like values are applied correctly. This resolves the earlier issue where empty strings couldn't clear an existing query.
121-129: Precedence check: preferredSegmentId prop vs fetchedPreferredSegmentIdCurrent flow prefers the prop as the initial default; later, fetchedPreferredSegmentId is only applied when no segmentId is present. Confirm this precedence is intended (URL > prop preferred > fetched preferred). If fetched preferred should override prop on load, we’ll need to tweak the initialization logic.
CarinaWolli
left a comment
CarinaWolli
left a comment
There was a problem hiding this comment.
Looks good and works great 🙏
github-actions
bot
added
the
bookings
5 participants
feat: implement system filter segments for data tables
Summary
This PR implements system filter segments for Cal.com's data table system, allowing users to access pre-defined filter combinations like "My Bookings" without having to create them manually. The implementation supports mixed segment ID types (numeric for user segments, string for system segments) and uses a dual database column approach for preference storage.
Key Changes:
systemSegmentIdVARCHAR column toUserFilterSegmentPreferencetable with constraints ensuring only one segment type per preferenceSystemFilterSegmenttype andCombinedFilterSegmentunion type to handle mixed ID types throughout the applicationuseSegmentshook,DataTableProvider, and repository methods to handle both user and system segmentsFilterSegmentSelectto group system segments separately with icons, and prevent editing/deleting of system segmentsReview & Testing Checklist for Human
systemSegmentIdcolumn and constraints were applied correctly, and that existing user preferences still workRecommended Test Plan:
Diagram
%%{ init : { "theme" : "default" }}%% graph TD BookingsView["apps/web/modules/bookings/views/<br/>bookings-listing-view.tsx"]:::major-edit DataTableProvider["packages/features/data-table/<br/>DataTableProvider.tsx"]:::major-edit useSegments["packages/features/data-table/hooks/<br/>useSegments.ts"]:::major-edit FilterSegmentSelect["packages/features/data-table/components/<br/>segment/FilterSegmentSelect.tsx"]:::major-edit SaveSegmentButton["packages/features/data-table/components/<br/>segment/SaveFilterSegmentButton.tsx"]:::minor-edit Types["packages/features/data-table/<br/>lib/types.ts"]:::major-edit Repository["packages/lib/server/repository/<br/>filterSegment.ts"]:::major-edit Schema["packages/prisma/<br/>schema.prisma"]:::minor-edit Migration["packages/prisma/migrations/<br/>add_default_segment_id/"]:::major-edit BookingsView -->|"defines default segments"| DataTableProvider DataTableProvider -->|"passes defaultSegments prop"| useSegments useSegments -->|"combines user + default segments"| FilterSegmentSelect FilterSegmentSelect -->|"groups and displays segments"| SaveSegmentButton Types -->|"defines CombinedFilterSegment"| useSegments Types -->|"defines DefaultFilterSegment"| BookingsView Repository -->|"handles mixed ID types"| useSegments Schema -->|"adds defaultSegmentId column"| Migration Migration -->|"database schema change"| Repository subgraph Legend L1[Major Edit]:::major-edit L2[Minor Edit]:::minor-edit L3[Context/No Edit]:::context end classDef major-edit fill:#90EE90 classDef minor-edit fill:#87CEEB classDef context fill:#FFFFFFNotes
Mixed ID Type Complexity: The most critical aspect to review is the handling of mixed segment ID types (number vs string) throughout the data flow. This required updates to URL parsing, state management, database queries, and UI components.
Frontend-Only Approach: Default segments are defined in component code rather than stored in the database, making them easier to deploy but requiring careful coordination between different views.
Date Range Recalculation: Default segments with relative date ranges (like "upcoming") are recalculated on each render to stay current, which should be monitored for performance impact.
Backward Compatibility: Existing user segments and preferences should continue working unchanged due to the dual-column database approach.
Link to Devin run: https://app.devin.ai/sessions/1c959909a25541de82f2ac0612e08a75
Requested by: @eunjae-lee