refactor: convert insights services to use kysely#22166
refactor: convert insights services to use kysely#22166eunjae-lee wants to merge 4 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
🤖 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:
|
|
✅ No security or compliance issues detected. Reviewed everything up to 8fe9958. Security Overview
Detected Code Changes
Reply to this PR with |
E2E results are ready! |
- Remove insightsBooking.ts and insightsRouting.ts (now in PR #22166) - Remove corresponding test files to eliminate duplication - Funnel chart feature now depends on services from base branch Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (06/30/25)1 reviewer was added to this PR based on Keith Williams's automation. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic found 7 issues across 4 files. Review them in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai to give specific feedback.
| } | ||
|
|
||
| async buildAuthorizationConditions(): Promise<Prisma.BookingTimeStatusDenormalizedWhereInput> { | ||
| async buildAuthorizationConditions(): Promise<WhereCondition> { |
There was a problem hiding this comment.
Rule violated: Enforce Singular Naming for Single-Item Functions
Function name is plural (`buildAuthorizationConditions`) but it returns a single `WhereCondition`, violating the "Enforce Singular Naming for Single-Item Functions" rule and misleading callers about the expected return.
| } | ||
|
|
||
| async buildFilterConditions(): Promise<WhereCondition | null> { | ||
| if (!this.filters) { |
There was a problem hiding this comment.
Rule violated: Enforce Singular Naming for Single-Item Functions
Function name is plural (`buildFilterConditions`) but it returns a single `WhereCondition` (or null), violating the "Enforce Singular Naming for Single-Item Functions" rule and potentially confusing callers about the return type.
| ) => ReturnType<ExpressionBuilder<DB, "BookingTimeStatusDenormalized">["and"]> | ||
| ) { | ||
| const compiled = db.selectFrom("BookingTimeStatusDenormalized").selectAll().where(condition).compile(); | ||
| const where = compiled.sql.replace(/^select \* from "BookingTimeStatusDenormalized" where /, ""); |
There was a problem hiding this comment.
The regex only matches a very specific, lowercase select * from pattern; if Kysely changes its SQL casing, inserts extra whitespace, or prefixes the table with a schema, the replacement will fail and the tests will break. Making the pattern case-insensitive and tolerant of whitespace would make the helper more robust.
| const where = compiled.sql.replace(/^select \* from "BookingTimeStatusDenormalized" where /, ""); | |
| const where = compiled.sql.replace(/^select\s+\*\s+from\s+"BookingTimeStatusDenormalized"\s+where\s+/i, ""); |
| ) => ReturnType<ExpressionBuilder<DB, "RoutingFormResponseDenormalized">["and"]> | ||
| ) { | ||
| const compiled = db.selectFrom("RoutingFormResponseDenormalized").selectAll().where(condition).compile(); | ||
| const where = compiled.sql.replace(/^select \* from "RoutingFormResponseDenormalized" where /, ""); |
There was a problem hiding this comment.
The string replacement assumes the generated SQL will always have the exact prefix select * from "RoutingFormResponseDenormalized" where . This is fragile—any aliasing, whitespace change, capitalization difference, or addition (e.g., schema name, DISTINCT) will cause the regex not to match and the test will break even though the query is still correct.
| const where = compiled.sql.replace(/^select \* from "RoutingFormResponseDenormalized" where /, ""); | |
| const where = (compiled.sql.split(" where ")[1] ?? "").trim(); |
| eb.or([ | ||
| eb.and([eb("teamId", "=", options.teamId), eb("isTeamBooking", "=", true)]), | ||
| eb.and([eb("userId", "in", userIdsFromTeam), eb("isTeamBooking", "=", false)]), | ||
| ]); |
There was a problem hiding this comment.
If userIdsFromTeam is an empty array, this produces an SQL IN () clause, which is invalid in most databases and will throw at runtime. Guard against the empty array before adding this condition.
| eb.and([eb("userId", "in", userIdsFromTeam), eb("isTeamBooking", "=", false)]), | |
| ...(userIdsFromTeam.length > 0 ? [eb.and([eb("userId", "in", userIdsFromTeam), eb("isTeamBooking", "=", false)])] : []), |
|
|
||
| if (this.filters?.startDate) { | ||
| const startDate = this.filters.startDate; | ||
| conditions.push((eb) => eb("createdAt", ">=", new Date(startDate))); |
There was a problem hiding this comment.
Constructing dates directly from unchecked strings can yield "Invalid Date" values (e.g., when provided ISO with timezone or bad input) leading to SQL errors at runtime; input should be validated or parsed with zod before usage.
| query() { | ||
| if (!this.baseWhereConditions) { | ||
| throw new Error("Service must be initialized before building base query. Call init() first."); | ||
| } |
There was a problem hiding this comment.
Public methods (e.g., getDropOffData) call query() without ensuring init() has run, so any consumer that forgets to call init() first will hit this runtime error – a breaking change from the previous Prisma service that required no explicit initialization.
2 participants
refactor: convert insights services to use kysely
Summary
This PR converts the InsightsBookingService and InsightsRoutingService from Prisma to Kysely ORM. This is a foundational refactor that maintains the same service interfaces while migrating the underlying database query implementation.
The changes include:
insightsBooking.tsandinsightsRouting.tsnow use Kysely for database queries instead of PrismaThis PR is part of a larger effort to split PR #22106 - this contains just the service layer changes, with the UI features to be stacked on top.
Review & Testing Checklist for Human
TZ=UTC yarn test packages/lib/server/service/__tests__/insights*to ensure all test scenarios passRecommended Test Plan:
Diagram
%%{ init : { "theme" : "default" }}%% graph TB subgraph "Insights Services" IBS["packages/lib/server/service/insightsBooking.ts"]:::major-edit IRS["packages/lib/server/service/insightsRouting.ts"]:::major-edit end subgraph "Integration Tests" IBT["packages/lib/server/service/__tests__/insightsBooking.integration-test.ts"]:::major-edit IRT["packages/lib/server/service/__tests__/insightsRouting.integration-test.ts"]:::major-edit end subgraph "Database Layer" Kysely["Kysely ORM"]:::context DB["Database"]:::context end subgraph "Dependent Features" Funnel["Funnel Chart Feature"]:::context Router["tRPC Router"]:::context end IBS --> Kysely IRS --> Kysely Kysely --> DB IBT --> IBS IRT --> IRS Router --> IBS Router --> IRS Funnel --> Router 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