refactor: Routed to Per Period to use Insights Routing Service #23031
Conversation
WalkthroughThis PR refactors RoutedToPerPeriod to use the InsightsRouting service: client-side replaces infinite scrolling with a single debounced routing-parameter-driven query (useInsightsRoutingParameters, debouncedSearchQuery), tightens FormCard/DownloadButton props, and adapts data mapping to the new per-period shape. Server-side adds routedToPerPeriod input schemas, updates TRPC procedures to call InsightsRoutingBaseService (including CSV generation), implements getRoutedToPerPeriodData/getRoutedToPerPeriodCsvData, revises authorization/filter-condition builders, and removes the old routedToPerPeriod implementations from routing-events.ts. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (4)**/*Service.ts📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Files:
**/*.ts📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Files:
**/*.{ts,tsx,js,jsx}⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (5)📚 Learning: 2025-07-15T12:59:34.389ZApplied to files:
📚 Learning: 2025-07-15T12:58:40.539ZApplied to files:
📚 Learning: 2025-07-24T08:39:06.185ZApplied to files:
📚 Learning: 2025-08-08T09:29:11.681ZApplied to files:
📚 Learning: 2025-07-15T13:02:17.403ZApplied to files:
⏰ 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)
🔇 Additional comments (6)
✨ 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
|
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
eunjae-lee
changed the title
eunjae-lee
force-pushed
the
refactor/routed-to-per-period
branch
from
bcdb81f to
c7d0575
Compare
| private filters: InsightsRoutingServiceFilterOptions; | ||
| private cachedAuthConditions?: Prisma.Sql; | ||
| private cachedFilterConditions?: Prisma.Sql | null; | ||
|
|
There was a problem hiding this comment.
I've removed this cached xxx conditions variables here. We may add it back later on if really needed.
eunjae-lee
force-pushed
the
refactor/routed-to-per-period
branch
from
4aa9ae4 to
3fc353b
Compare
| } | ||
|
|
||
| async buildFilterConditions(): Promise<Prisma.Sql | null> { | ||
| async getFilterConditions(conditionsOptions?: GetConditionsOptions): Promise<Prisma.Sql | null> { |
There was a problem hiding this comment.
We used to cache the conditions so that we don't have to re-create them again, for example,
const service = new InsightsRoutingBaseService(...)
await service.getTableData(); // <- this builds all the conditions
await service.getTableData(); // <- if we call again, we use the cached conditions
However, we're introducing exclude parameters so that we can exclude some conditions from getFilterConditions result.
While we could still support cache for all cases, it can get complicated and we don't have any immediate use-case where we could benefit from this caching. So let's remove it for now, and revisit later if needed.
eunjae-lee
force-pushed
the
refactor/routed-to-per-period
branch
from
2d61d52 to
78bb92d
Compare
eunjae-lee
force-pushed
the
refactor/routed-to-per-period
branch
from
78bb92d to
19b7b1d
Compare
eunjae-lee
force-pushed
the
refactor/routed-to-per-period
branch
from
19b7b1d to
62d59b6
Compare
eunjae-lee
force-pushed
the
refactor/routed-to-per-period
branch
from
62d59b6 to
b02566b
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/features/insights/components/routing/RoutedToPerPeriod.tsx (1)
37-74: Download button never sets loading state to true
isDownloadingis never set totrue, so the spinner/disabled state never activates. Set it before the fetch and reset infinally.Apply this diff:
function DownloadButton({ selectedPeriod, searchQuery }: DownloadButtonProps) { const [isDownloading, setIsDownloading] = useState(false); const routingParams = useInsightsRoutingParameters(); const utils = trpc.useContext(); const handleDownload = async (e: React.MouseEvent) => { e.preventDefault(); // Prevent default form submission try { + setIsDownloading(true); const result = await utils.viewer.insights.routedToPerPeriodCsv.fetch({ ...routingParams, period: selectedPeriod, searchQuery: searchQuery || undefined, }); if (!result?.data) { throw new Error("No data received"); } downloadAsCsv(result.data, result.filename || "routing-data.csv"); } catch (error) { console.error("Download failed:", error); } finally { setIsDownloading(false); } };
🧹 Nitpick comments (7)
packages/lib/server/service/InsightsRoutingBaseService.ts (2)
562-581: Median calculation can be skewed for even N and unsorted inputYou sort by
total_bookings ASC(good), but for even-sized arrays you're implicitly picking the upper median; depending on UX intent, you may want the true median (average of two middles) or define this explicitly in UI copy.Optionally compute the median as the average of the two middle values for even
N.-const median = statsQuery[Math.floor(statsQuery.length / 2)]?.total_bookings || 0; +let median = 0; +if (stats.length > 0) { + const mid = Math.floor(stats.length / 2); + median = stats.length % 2 === 0 ? (stats[mid - 1].total_bookings + stats[mid].total_bookings) / 2 : stats[mid].total_bookings; +}Adjust performance labeling if median becomes non-integer.
615-644: CSV column ordering and completenessCSV headers derive from per-user stats order, which may vary. Consider generating a stable, global period list first and use it to order columns consistently across all rows.
- Build a sorted unique list of
period_startacross all stats.- Use that list to emit “Responses YYYY-MM-DD” columns in fixed order for every row, defaulting to “0”.
This ensures reproducible CSV structure regardless of user-specific periods.
packages/features/insights/server/trpc-router.ts (2)
941-961: CSV builder duplication and header completenessYou added a local
objectToCsvwhileRoutingEventsInsightsalready exposes CSV helpers. Duplicating increases drift risk and the current builder only uses headers from the first row, dropping extra keys in later rows.
- Reuse a shared CSV utility (e.g.,
@calcom/lib/csvUtilsor the helper inrouting-events) to centralize escaping/edge-cases.- If keeping local, compute the union of keys across all rows and use a stable column order.
Example improvement:
-const headers = Object.keys(data[0]); +const headers = Array.from( + data.reduce((s, row) => { Object.keys(row).forEach((k)=>s.add(k)); return s; }, new Set<string>()) +);
1123-1143: CSV escaping covers commas/quotes/newlines but can double-quote booleans/numbers inconsistentlySince the input type is
Record<string, string>, you're formatting everything as strings already, which is fine. If you expand to heterogeneous types later, consider explicit coercion and consistent quoting rules.Optionally, move this helper to a shared utility to avoid drift with other CSV exports.
packages/features/insights/components/routing/RoutedToPerPeriod.tsx (3)
200-207: Set-of-periods computed from periodStats is OK; consider stable sort/formattingCurrent logic uses
toISOString()perDateand sorts ascending. That’s fine. If you want locale-aware headers, consider formatting with the same day/week/month granularity the server used.No change required—just a note if you see UX inconsistency across locales.
169-176: Unused ref
tableContainerRefis assigned but never used. Remove it to reduce noise.Apply this diff:
- const tableContainerRef = useRef<HTMLDivElement>(null);And drop the
ref={tableContainerRef}usage below.
296-312: Week highlighting may not honor user’s weekStart
isCurrentPeriodassumes Sunday-based weeks. If your app supports different week starts (e.g., Monday), the highlight could be misleading.Consider deriving
weekStartfrom user settings/locale, or have the server return the current-period flag per header to avoid client-side calendar logic drift.
📜 Review details
Configuration used: .coderabbit.yaml
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 (5)
packages/features/insights/components/routing/RoutedToPerPeriod.tsx(10 hunks)packages/features/insights/server/raw-data.schema.ts(1 hunks)packages/features/insights/server/routing-events.ts(0 hunks)packages/features/insights/server/trpc-router.ts(3 hunks)packages/lib/server/service/InsightsRoutingBaseService.ts(6 hunks)
💤 Files with no reviewable changes (1)
- packages/features/insights/server/routing-events.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/insights/server/raw-data.schema.tspackages/features/insights/server/trpc-router.tspackages/lib/server/service/InsightsRoutingBaseService.tspackages/features/insights/components/routing/RoutedToPerPeriod.tsx
🧠 Learnings (3)
📚 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:
packages/features/insights/server/trpc-router.tspackages/features/insights/components/routing/RoutedToPerPeriod.tsx
📚 Learning: 2025-07-24T08:39:06.185Z
Learnt from: eunjae-lee
PR: calcom/cal.com#22702
File: packages/lib/server/service/insightsBooking.ts:120-124
Timestamp: 2025-07-24T08:39:06.185Z
Learning: In the InsightsBookingService (packages/lib/server/service/insightsBooking.ts), the constructor stores null for invalid options or filters but this is handled safely through null checks in buildFilterConditions() and buildAuthorizationConditions() methods. The service uses defensive programming to return safe fallback conditions (null or NOTHING_CONDITION) rather than throwing errors on invalid inputs.
Applied to files:
packages/lib/server/service/InsightsRoutingBaseService.ts
📚 Learning: 2025-07-15T12:58:40.539Z
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/RoutingFunnel.tsx:15-17
Timestamp: 2025-07-15T12:58:40.539Z
Learning: In the insights routing funnel component (packages/features/insights/components/RoutingFunnel.tsx), the useColumnFilters exclusions are intentionally different from the general useInsightsParameters exclusions. RoutingFunnel specifically excludes only ["createdAt"] while useInsightsParameters excludes ["bookingUserId", "formId", "createdAt", "eventTypeId"]. This difference is by design.
Applied to files:
packages/lib/server/service/InsightsRoutingBaseService.tspackages/features/insights/components/routing/RoutedToPerPeriod.tsx
⏰ 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). (7)
- GitHub Check: Production builds / Build Web App
- GitHub Check: Production builds / Build API v2
- GitHub Check: Production builds / Build Atoms
- GitHub Check: Tests / Unit
- GitHub Check: Linters / lint
- GitHub Check: Production builds / Build API v1
- GitHub Check: Type check / check-types
🔇 Additional comments (6)
packages/lib/server/service/InsightsRoutingBaseService.ts (1)
661-681: Flexible exclusions look goodThe new
GetConditionsOptions.excludesupport for omittingcreatedAtand selected filter IDs is clean and predictable. It reliably prevents conflicting date constraints when composing per-period queries.packages/features/insights/server/trpc-router.ts (2)
9-11: Schema swap LGTMSwitching inputs to
routedToPerPeriodInputSchemaandroutedToPerPeriodCsvInputSchemanarrows the contract and prevents oversized pagination in CSV endpoints.
952-956: Filename generation is clear and deterministicUsing
dayjswith the provided range fordownloadAsis a nice touch for traceability.packages/features/insights/components/routing/RoutedToPerPeriod.tsx (3)
84-121: FormCard refactor reads wellThe simplified props and explicit union for period handling remove a lot of plumbing and improve readability. Casting in
onValueChangeis pragmatic and safe with the provided options.
180-193: Query inputs and debouncing are wired correctlyUsing
useInsightsRoutingParameterswith a debouncedsearchQueryis a clean pattern. AddingstaleTimeandskipBatchis also appropriate for chart UX.
318-354: Nice: left-fixed user column and periodized cellsThe sticky user column and sparse rendering (omit zeros) make the table readable. Good use of
HoverCardfor details.
E2E results are ready! |
github-actions
bot
added
Low priority
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/21/25)1 reviewer was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/lib/server/service/InsightsRoutingBaseService.ts (1)
448-456: Nice fixes applied: NULL user guard, period recursion, and DISTINCT bookingId.
- Added AND "bookingUserId" IS NOT NULL to avoid NULL user pollution.
- Corrected date_range recursion to stop at end-of-period, fixing off-by-one.
- Aligned totals with COUNT(DISTINCT "bookingId").
All three address earlier review feedback.
Also applies to: 488-502, 549-551
🧹 Nitpick comments (4)
packages/lib/server/service/InsightsRoutingBaseService.ts (4)
131-137: Document GetConditionsOptions to avoid misuse.Add a brief JSDoc to clarify that createdAt is intentionally excluded when callers provide explicit date windows (e.g., per-period queries), and that columnFilterIds refers to DataTable column filter ids.
+/** + * Controls which filters are applied when building SQL conditions. + * - exclude.createdAt: omit the standard createdAt range filter. Callers must constrain dates themselves. + * - exclude.columnFilterIds: omit specific DataTable column filters by id (e.g., ["createdAt","bookingUserId"]). + */ type GetConditionsOptions = { exclude?: { createdAt?: boolean; columnFilterIds?: string[]; }; };
519-529: Consider filtering to routed rows for consistency and performance.booking_counts currently relies on COUNT(DISTINCT "bookingId") to ignore NULLs. Adding an explicit "bookingUid" IS NOT NULL can improve plan selectivity and make intent clearer, matching usersQuery/statsQuery.
FROM "RoutingFormResponseDenormalized" rfrd WHERE "bookingUserId" IN (SELECT user_id FROM all_users) + AND "bookingUid" IS NOT NULL AND date_trunc(${dayjsPeriod}, "createdAt") >= (SELECT MIN(period_start) FROM paginated_periods) AND date_trunc(${dayjsPeriod}, "createdAt") <= (SELECT MAX(period_start) FROM paginated_periods) AND ${baseConditions}
561-579: Median calculation: handle even-sized samples explicitly.Current median picks floor(n/2), which biases upwards for even n. Compute the canonical even median to avoid thresholds moving unexpectedly.
- const average = - statsQuery.reduce((sum, stat) => sum + Number(stat.total_bookings), 0) / statsQuery.length || 0; - const median = statsQuery[Math.floor(statsQuery.length / 2)]?.total_bookings || 0; + const totals = statsQuery.map((s) => Number(s.total_bookings)).sort((a, b) => a - b); + const n = totals.length; + const average = (totals.reduce((s, x) => s + x, 0) / (n || 1)) || 0; + const median = + n === 0 + ? 0 + : n % 2 === 1 + ? totals[(n - 1) >> 1] + : (totals[n / 2 - 1] + totals[n / 2]) / 2;Optional: If you want “median” to mean “exactly equal to the (possibly fractional) median,” that’s fine. Otherwise, treat values within the two middle integers as “median” when n is even.
850-859: Ensure array parameter is typed as text[] to avoid driver cast issues.Some Postgres drivers/prisma bindings can misinfer array types. Casting to text[] makes the @> operator deterministic.
- AND rrf."valueStringArray" @> ${filterValue.data.map(String)} + AND rrf."valueStringArray" @> ${Prisma.sql`ARRAY[${Prisma.join( + filterValue.data.map(String) + )}]::text[]`}
📜 Review details
Configuration used: .coderabbit.yaml
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 (2)
packages/features/insights/server/raw-data.schema.ts(1 hunks)packages/lib/server/service/InsightsRoutingBaseService.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/features/insights/server/raw-data.schema.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*Service.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/lib/server/service/InsightsRoutingBaseService.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/lib/server/service/InsightsRoutingBaseService.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:
packages/lib/server/service/InsightsRoutingBaseService.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/lib/server/service/InsightsRoutingBaseService.ts
🧠 Learnings (3)
📚 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:
packages/lib/server/service/InsightsRoutingBaseService.ts
📚 Learning: 2025-07-15T12:58:40.539Z
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/RoutingFunnel.tsx:15-17
Timestamp: 2025-07-15T12:58:40.539Z
Learning: In the insights routing funnel component (packages/features/insights/components/RoutingFunnel.tsx), the useColumnFilters exclusions are intentionally different from the general useInsightsParameters exclusions. RoutingFunnel specifically excludes only ["createdAt"] while useInsightsParameters excludes ["bookingUserId", "formId", "createdAt", "eventTypeId"]. This difference is by design.
Applied to files:
packages/lib/server/service/InsightsRoutingBaseService.ts
📚 Learning: 2025-07-24T08:39:06.185Z
Learnt from: eunjae-lee
PR: calcom/cal.com#22702
File: packages/lib/server/service/insightsBooking.ts:120-124
Timestamp: 2025-07-24T08:39:06.185Z
Learning: In the InsightsBookingService (packages/lib/server/service/insightsBooking.ts), the constructor stores null for invalid options or filters but this is handled safely through null checks in buildFilterConditions() and buildAuthorizationConditions() methods. The service uses defensive programming to return safe fallback conditions (null or NOTHING_CONDITION) rather than throwing errors on invalid inputs.
Applied to files:
packages/lib/server/service/InsightsRoutingBaseService.ts
🔇 Additional comments (5)
packages/lib/server/service/InsightsRoutingBaseService.ts (5)
4-4: Day.js usage is appropriate here.Day.js is only used for range normalization and CSV header formatting, not inside hot loops. No perf concerns.
645-658: Flexible condition composition looks good.getBaseConditions now composes auth and filter conditions with optional exclusions. The short-circuiting and NOTHING_CONDITION fallback are correct.
671-674: Column-filter exclusions are applied correctly.Filtering out exclude.columnFilterIds before building the filters prevents unintended double-filtering in per-period routes.
781-806: Authorization checks are sound and scope-aware.
- Owner/Admin check gates org/team scopes.
- User scope is correctly restricted to personal forms.
- Org scope includes all descendant teams plus the caller’s personal forms (intentional?).
Confirm the personal-forms union in org scope is desired for all tenants; otherwise, it can be surprising for auditors.
595-644: CSV header formatting is clear and consistent."Responses YYYY-MM-DD" per period_start is easy to consume. Nice map/reduce shaping to wide rows.
| limit = 10, | ||
| searchQuery, | ||
| }: { | ||
| period: "perDay" | "perWeek" | "perMonth"; | ||
| limit?: number; | ||
| searchQuery?: string; | ||
| }) { |
There was a problem hiding this comment.
CSV export currently returns only 10 users/periods due to default limit=10. Remove implicit limiting.
getRoutedToPerPeriodCsvData calls getRoutedToPerPeriodData without a limit, but the callee defaults limit to 10, so CSV is truncated. Make limit opt-in and allow “no limit” via null.
- async getRoutedToPerPeriodData({
- period,
- limit = 10,
- searchQuery,
- }: {
- period: "perDay" | "perWeek" | "perMonth";
- limit?: number;
- searchQuery?: string;
- }) {
+ async getRoutedToPerPeriodData({
+ period,
+ limit = null,
+ searchQuery,
+ }: {
+ period: "perDay" | "perWeek" | "perMonth";
+ limit?: number | null;
+ searchQuery?: string;
+ }) {
@@
- ${limit ? Prisma.sql`LIMIT ${limit}` : Prisma.empty}
+ ${limit != null ? Prisma.sql`LIMIT ${limit}` : Prisma.empty}
@@
- ${limit ? Prisma.sql`LIMIT ${limit}` : Prisma.empty}
+ ${limit != null ? Prisma.sql`LIMIT ${limit}` : Prisma.empty}And pass “no limit” explicitly from CSV:
- const data = await this.getRoutedToPerPeriodData({
- period,
- searchQuery,
- // No limit = get all data
- });
+ const data = await this.getRoutedToPerPeriodData({
+ period,
+ searchQuery,
+ // No limit = get all data
+ limit: null,
+ });Also applies to: 461-461, 510-510, 608-612
- Move getFailedBookingsByRoutingFormGroup logic to InsightsRoutingBaseService - Replace legacy getWhereForTeamOrAllTeams with getBaseConditions() - Add failedBookingsByFieldInputSchema with date filtering support - Update tRPC endpoint to use createInsightsRoutingService pattern - Update component to pass startDate/endDate parameters - Remove legacy method from routing-events.ts Follows same pattern as PR #23031 for RoutedToPerPeriod refactoring Co-Authored-By: [email protected] <[email protected]>
- Add missing import for useInsightsRoutingParameters - Update component to follow same pattern as PR #23031 - Add columnFilters support to failedBookingsByFieldInputSchema - Maintain routingFormId parameter from useInsightsParameters Co-Authored-By: [email protected] <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/lib/server/service/InsightsRoutingBaseService.ts (4)
448-456: Good fix: exclude NULL bookingUserIdThanks for adding AND "bookingUserId" IS NOT NULL alongside "bookingUid" IS NOT NULL. This prevents NULL user rows from propagating into combinations and stats.
488-503: Off-by-one in recursive period generation resolvedThe recursion now stops at date_trunc(period, endDate), eliminating the extra trailing period. Nice.
549-551: Metrics aligned — DISTINCT bookingId is correctSwitching to COUNT(DISTINCT "bookingId") keeps statsQuery consistent with booking_counts. Good catch.
608-612: CSV now fetches all rows — goodCalling getRoutedToPerPeriodData without limit fixes the earlier truncation issue for CSV exports.
🧹 Nitpick comments (6)
packages/lib/server/service/InsightsRoutingBaseService.ts (6)
4-4: Use UTC consistently for date mathImport looks good. For period-boundary correctness across timezones/DST, prefer using dayjs.utc(...) whenever computing start/end of periods and when formatting period labels (see comments below).
428-431: Guard missing dates and align to UTC to avoid off-by-one/timezone driftEven though filters declare dates as required, add a fast guard and compute boundaries in UTC to match DB semantics. This prevents accidental “default to now” and DST issues.
Apply:
- const dayjsPeriod = dayJsPeriodMap[period]; - const startDate = dayjs(this.filters.startDate).startOf(dayjsPeriod).toDate(); - const endDate = dayjs(this.filters.endDate).endOf(dayjsPeriod).toDate(); + const dayjsPeriod = dayJsPeriodMap[period]; + if (!this.filters.startDate || !this.filters.endDate) { + throw new Error("startDate and endDate are required for per-period queries."); + } + const startDate = dayjs.utc(this.filters.startDate).startOf(dayjsPeriod).toDate(); + const endDate = dayjs.utc(this.filters.endDate).endOf(dayjsPeriod).toDate();
560-564: Median for even N is incorrect; compute the true medianCurrent median picks the upper mid for even-length samples. If you need a true median, compute it precisely. Minimal change:
- const average = - statsQuery.reduce((sum, stat) => sum + Number(stat.total_bookings), 0) / statsQuery.length || 0; - const median = statsQuery[Math.floor(statsQuery.length / 2)]?.total_bookings || 0; + const totals = statsQuery.map((s) => Number(s.total_bookings)).sort((a, b) => a - b); + const n = totals.length; + const average = n ? totals.reduce((a, b) => a + b, 0) / n : 0; + const median = n + ? n % 2 + ? totals[(n - 1) / 2] + : (totals[n / 2 - 1] + totals[n / 2]) / 2 + : 0;Note: If you want to keep a discrete “is median” label, consider labeling the two middle values for even N instead of comparing to a fractional median.
621-631: Stabilize CSV period labels (UTC) to match DB period startsUse UTC when formatting period starts to avoid local-time shifts in CSV headers.
- [`Responses ${dayjs(periodStart).format("YYYY-MM-DD")}`]: total.toString(), + [`Responses ${dayjs.utc(periodStart).format("YYYY-MM-DD")}`]: total.toString(),
660-674: Column filters exclusion logic looks right; add a quick guard for empty arraysThe exclude handling is clear. Minor: if exclude.columnFilterIds is empty, the current code behaves correctly; no change needed.
781-806: Authorization checks are safe; minimal surface with NOTHING_CONDITION on failureOwner/admin verification on org/team, and strict fallbacks look good. Consider logging when returning NOTHING_CONDITION to aid debugging, but avoid leaking auth info to clients.
📜 Review details
Configuration used: Path: .coderabbit.yaml
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 (2)
packages/features/insights/server/raw-data.schema.ts(1 hunks)packages/lib/server/service/InsightsRoutingBaseService.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/features/insights/server/raw-data.schema.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*Service.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/lib/server/service/InsightsRoutingBaseService.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/lib/server/service/InsightsRoutingBaseService.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:
packages/lib/server/service/InsightsRoutingBaseService.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/lib/server/service/InsightsRoutingBaseService.ts
🧠 Learnings (4)
📚 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:
packages/lib/server/service/InsightsRoutingBaseService.ts
📚 Learning: 2025-07-15T12:58:40.539Z
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/RoutingFunnel.tsx:15-17
Timestamp: 2025-07-15T12:58:40.539Z
Learning: In the insights routing funnel component (packages/features/insights/components/RoutingFunnel.tsx), the useColumnFilters exclusions are intentionally different from the general useInsightsParameters exclusions. RoutingFunnel specifically excludes only ["createdAt"] while useInsightsParameters excludes ["bookingUserId", "formId", "createdAt", "eventTypeId"]. This difference is by design.
Applied to files:
packages/lib/server/service/InsightsRoutingBaseService.ts
📚 Learning: 2025-07-24T08:39:06.185Z
Learnt from: eunjae-lee
PR: calcom/cal.com#22702
File: packages/lib/server/service/insightsBooking.ts:120-124
Timestamp: 2025-07-24T08:39:06.185Z
Learning: In the InsightsBookingService (packages/lib/server/service/insightsBooking.ts), the constructor stores null for invalid options or filters but this is handled safely through null checks in buildFilterConditions() and buildAuthorizationConditions() methods. The service uses defensive programming to return safe fallback conditions (null or NOTHING_CONDITION) rather than throwing errors on invalid inputs.
Applied to files:
packages/lib/server/service/InsightsRoutingBaseService.ts
📚 Learning: 2025-07-15T13:02:17.403Z
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/lib/server/service/insightsRouting.ts:367-368
Timestamp: 2025-07-15T13:02:17.403Z
Learning: In the InsightsRoutingService (packages/lib/server/service/insightsRouting.ts), multi-select filter data is already validated by zod before reaching the buildFormFieldSqlCondition method, so null/undefined values are not present in filterValue.data arrays and don't need to be filtered out.
Applied to files:
packages/lib/server/service/InsightsRoutingBaseService.ts
⏰ 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)
- GitHub Check: Production builds / Build Docs
🔇 Additional comments (5)
packages/lib/server/service/InsightsRoutingBaseService.ts (5)
51-53: startDate/endDate made required — ensure upstream validation is strictMaking these required is good. Make sure upstream input schemas (TRPC zod) enforce ISO strings (or coerce to Date) to avoid runtime parse errors. See guard suggestion in getRoutedToPerPeriodData.
131-137: Flexible conditions builder looks goodThe exclude options (createdAt, columnFilterIds) give us precise control and replaced the earlier cached-conditions complexity nicely.
645-648: getBaseConditions combines auth+filters correctlyPassing through conditionsOptions into getFilterConditions keeps date-exclusion logic local to callers. Looks clean.
850-888: Form-field SQL builders look safe
- Multi/single-select and text/number paths use EXISTS subqueries and parameterized conditions; no raw column injection.
- Note: For multi-select, ensure rrf.valueStringArray is text[]; the parameter will bind correctly as a string[].
553-555: Verify timezone semantics for createdAt comparisonsPlease confirm the exact format and type of the
startDate/endDatevalues you’re interpolating. If they’re plain ISO strings including an explicit offset (e.g.2025-08-22T00:00:00Z), PostgreSQL will parse them correctly astimestamptzliterals. Otherwise, comparing atimestamptzcolumn to atimestamp(or to an unannotated string) can silently shift times when the server’s timezone or DST applies.Key locations to review:
packages/lib/server/service/InsightsRoutingBaseService.ts:553–555packages/lib/server/service/InsightsRoutingBaseService.ts:456–457packages/lib/server/service/InsightsRoutingBaseService.ts:667Suggested adjustment if your dates lack explicit offsets:
- AND "createdAt" >= ${startDate} - AND "createdAt" <= ${endDate} + AND "createdAt" >= ${startDate}::timestamptz + AND "createdAt" <= ${endDate}::timestamptzAnd for raw Prisma SQL snippets:
- Prisma.sql`"createdAt" >= ${this.filters.startDate}::timestamp - AND "createdAt" <= ${this.filters.endDate}::timestamp` + Prisma.sql`"createdAt" >= ${this.filters.startDate}::timestamptz + AND "createdAt" <= ${this.filters.endDate}::timestamptz`– If your inputs already include timezone offsets, you can omit casts altogether and rely on ISO8601 parsing.
– Otherwise, adding::timestamptzensures no off-by-hours when DST or differing DB timezones come into play.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/lib/server/service/__tests__/InsightsRoutingService.integration-test.ts (1)
615-619: Flaky assertion: relying on implicit row order from SELECT without ORDER BY.Postgres does not guarantee row order without
ORDER BY. The test may intermittently fail.Make the assertion order-insensitive:
- // Should only return the authorized user's form response - expect(results).toHaveLength(2); - expect(results[0]?.id).toBe(testData.formResponse.id); - expect(results[1]?.id).toBe(personalFormResponse.id); + // Should only return the authorized user's form response (order-insensitive) + expect(results).toHaveLength(2); + const actual = results.map((r) => r.id).sort((a, b) => a - b); + const expected = [testData.formResponse.id, personalFormResponse.id].sort((a, b) => a - b); + expect(actual).toEqual(expected);Alternatively, add
ORDER BY idto the query and keep a deterministic order.
🧹 Nitpick comments (3)
packages/lib/server/service/__tests__/InsightsRoutingService.integration-test.ts (3)
84-105: Fix test data mismatch: form response keys should match the created field IDs.Currently, the form
fields[].idvalues are random, but the response uses different random IDs. That disconnect can break future assertions or denormalization-dependent tests.Apply this diff to align the IDs:
@@ - // Create test routing form + // Create test routing form + const textFieldId = `text-field-id-${randomUUID()}`; + const emailFieldId = `email-field-id-${randomUUID()}`; const form = await prisma.app_RoutingForms_Form.create({ data: { name: "Test Routing Form", userId: user.id, teamId: team.id, fields: [ { - id: `text-field-id-${randomUUID()}`, + id: textFieldId, type: "text", label: "Name", required: true, }, { - id: `email-field-id-${randomUUID()}`, + id: emailFieldId, type: "email", label: "Email", required: true, }, ], }, }); @@ const formResponse = await prisma.app_RoutingForms_FormResponse.create({ data: { formFillerId: `test-filler-${randomUUID()}`, formId: form.id, response: { - [`text-field-id-${randomUUID()}`]: { + [textFieldId]: { label: "Name", value: "Test Name", }, - [`email-field-id-${randomUUID()}`]: { + [emailFieldId]: { label: "Email", value: "[email protected]", }, }, routedToBookingUid: booking.uid, }, });Also applies to: 113-121
149-182: Tighten setup/teardown for speed and resilience.
Setup creates many rows sequentially; teardown deletes them one-by-one. This is slow and brittle if a test fails before explicit cleanup.
Wrap deletions in a single transaction and prefer
deleteManywhere possible to reduce roundtrips.Consider using
try/finallyin each test (orafterEach) to guarantee cleanup on failures.Example teardown refactor:
- // Cleanup function + // Cleanup function const cleanup = async () => { - // Delete in reverse order to avoid foreign key constraints - for (const membership of additionalMemberships) { - await prisma.membership.delete({ - where: { id: membership.id }, - }); - } - for (const user of additionalUsers) { - await prisma.user.delete({ - where: { id: user.id }, - }); - } - for (const team of additionalTeams) { - await prisma.team.delete({ - where: { id: team.id }, - }); - } - - await prisma.membership.delete({ - where: { id: orgMembership.id }, - }); - await prisma.membership.delete({ - where: { id: membership.id }, - }); - await prisma.app_RoutingForms_FormResponse.delete({ - where: { id: formResponse.id }, - }); - await prisma.booking.delete({ - where: { id: booking.id }, - }); - await prisma.eventType.delete({ - where: { id: eventType.id }, - }); - await prisma.app_RoutingForms_Form.delete({ - where: { id: form.id }, - }); - await prisma.team.delete({ - where: { id: team.id }, - }); - await prisma.team.delete({ - where: { id: org.id }, - }); - await prisma.user.delete({ - where: { id: user.id }, - }); + // Delete in reverse dependency order inside a single transaction + await prisma.$transaction([ + prisma.membership.deleteMany({ where: { id: { in: additionalMemberships.map((m) => m.id) } } }), + prisma.user.deleteMany({ where: { id: { in: additionalUsers.map((u) => u.id) } } }), + prisma.team.deleteMany({ where: { id: { in: additionalTeams.map((t) => t.id) } } }), + prisma.membership.delete({ where: { id: orgMembership.id } }), + prisma.membership.delete({ where: { id: membership.id } }), + prisma.app_RoutingForms_FormResponse.delete({ where: { id: formResponse.id } }), + prisma.booking.delete({ where: { id: booking.id } }), + prisma.eventType.delete({ where: { id: eventType.id } }), + prisma.app_RoutingForms_Form.delete({ where: { id: form.id } }), + prisma.team.delete({ where: { id: team.id } }), + prisma.team.delete({ where: { id: org.id } }), + prisma.user.delete({ where: { id: user.id } }), + ]); };If you’d like, I can follow up with a helper to auto-register
cleanup()inafterEachfor each test case.Also applies to: 184-230
1086-1105: Snapshot assertions over generated SQL are brittle; prefer structure-aware assertions.Inline snapshots of
Prisma.sqlobjects can churn with minor whitespace or formatting changes. Since the code already returns{ strings, values }, asserting on key fragments or usingtoEqualwith a constructedPrisma.sql(as in other tests) is more stable.Example pattern:
- Assert
valuescontain the expected operands.- Assert
stringscontain the expected clause fragments in order (e.g.,EXISTS (...),ILIKE, etc.).No change required for this PR; flagging to reduce future maintenance noise.
Also applies to: 1141-1159, 1195-1213
📜 Review details
Configuration used: Path: .coderabbit.yaml
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 (1)
packages/lib/server/service/__tests__/InsightsRoutingService.integration-test.ts(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:
packages/lib/server/service/__tests__/InsightsRoutingService.integration-test.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:
packages/lib/server/service/__tests__/InsightsRoutingService.integration-test.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/lib/server/service/__tests__/InsightsRoutingService.integration-test.ts
packages/lib/server/service/__tests__/InsightsRoutingService.integration-test.ts
Show resolved
Hide resolved
* refactor: FailedBookingsByField to use Insights Routing Service - Move getFailedBookingsByRoutingFormGroup logic to InsightsRoutingBaseService - Replace legacy getWhereForTeamOrAllTeams with getBaseConditions() - Add failedBookingsByFieldInputSchema with date filtering support - Update tRPC endpoint to use createInsightsRoutingService pattern - Update component to pass startDate/endDate parameters - Remove legacy method from routing-events.ts Follows same pattern as PR #23031 for RoutedToPerPeriod refactoring Co-Authored-By: [email protected] <[email protected]> * fix: use useInsightsRoutingParameters hook in FailedBookingsByField - Add missing import for useInsightsRoutingParameters - Update component to follow same pattern as PR #23031 - Add columnFilters support to failedBookingsByFieldInputSchema - Maintain routingFormId parameter from useInsightsParameters Co-Authored-By: [email protected] <[email protected]> * refactor: remove separate routingFormId parameter - Remove routingFormId parameter from getFailedBookingsByFieldData method - Update component to only use insightsRoutingParameters - Remove routingFormId from failedBookingsByFieldInputSchema - Let getBaseConditions handle all filtering including routing form filtering - Simplify tRPC endpoint to not pass separate routingFormId Co-Authored-By: [email protected] <[email protected]> * clean up * fix * refactor: use RoutingFormResponseDenormalized directly in getFailedBookingsByFieldData - Replace complex CTE approach with direct denormalized table query - Join with App_RoutingForms_Form to access field definitions from JSON - Use RoutingFormResponseField for response values - Filter failed bookings with bookingUid IS NULL - Maintain same data structure for UI compatibility - Follow existing service patterns for authorization and date filtering Co-Authored-By: [email protected] <[email protected]> * fixing failed bookings query * fix query * fix integration test --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
What does this PR do?
Visual Demo (For contributors especially)
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist