Automated Test: insights-performance-optimization

[admin-coderabbit]

This pull request was automatically created by @coderabbitai/e2e-reviewer.

Batch created pull request.

Summary by CodeRabbit

• Refactor

  • Refactored internal condition construction and query building to improve code maintainability and performance.
  • Updated service interface with new public options type for better type safety and clearer API contracts.
  • Reworked authorization and filtering logic to use improved query composition patterns.

• Tests

  • Updated integration tests to align with refactored service implementation.

[coderabbit-eval]

📝 Walkthrough

Walkthrough

This PR refactors InsightsBookingService to use Prisma.Sql fragments for constructing authorization and filter conditions instead of nested object literals. The service API exposes a new public options type, and corresponding test assertions are updated to validate against raw SQL expressions.

Changes

Cohort / File(s)	Summary
Service Refactoring packages/lib/server/service/insightsBooking.ts	Introduces InsightsBookingServicePublicOptions export; replaces condition construction from nested Prisma objects to Prisma.Sql fragments; refactors getAuthorizationConditions, getFilterConditions, and related builders to compose and return SQL expressions; updates membership role check from array.includes to direct boolean logic; removes findMany method in favor of getBaseConditions.
Test Updates packages/lib/server/service/__tests__/insightsBooking.integration-test.ts	Adds Prisma import and NOTHING_CONDITION constant; replaces object-based assertion patterns with Prisma.sql fragment comparisons; switches from ORM-based findMany calls to raw SQL query execution via getBaseConditions; updates test expectations to validate SQL expressions instead of nested authorization/filter structures.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐰 From objects nested deep and wide,
To SQL fragments, side by side,
Conditions flow as fragments pure,
Our queries fast, our types secure. ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Automated Test: insights-performance-optimization' is vague and does not clearly convey the specific changes made in this pull request.	Replace with a specific title that describes the main change, such as 'Refactor InsightsBookingService to use Prisma SQL fragments for authorization and filtering' or 'Replace object-based conditions with Prisma.sql fragments'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch insights-performance-optimization

---

_{Comment @coderabbit eval help to get the list of available commands and usage tips.}

[coderabbit-eval]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@packages/lib/server/service/insightsBooking.ts`:
- Around line 164-174: The generated SQL uses PostgreSQL ANY() without type
casts which breaks at runtime; update the two Prisma.sql clauses that reference
teamIds and uniqueUserIds (the initial element in the conditions array and the
pushed element for uniqueUserIds) to use IN with Prisma.join instead of ANY
(e.g., replace ("teamId" = ANY(${teamIds})) and ("userId" =
ANY(${uniqueUserIds})) with ("teamId" IN (${Prisma.join(teamIds)})) and
("userId" IN (${Prisma.join(uniqueUserIds)}))). Keep the surrounding logic that
builds the conditions array and the reduce() combining logic intact.

🧹 Nitpick comments (4)

packages/lib/server/service/insightsBooking.ts (3)

29-34: InsightsBookingServicePublicOptions is looser than the Zod schema.

The public type makes teamId optional for all scopes, but the Zod schema requires it for "team" scope. While safeParse handles validation at runtime, callers using this type won't get compile-time enforcement that teamId must be provided when scope === "team". Consider using a discriminated union type matching the schema, or at least adding a doc comment noting the Zod validation requirement.

♻️ Suggested type that mirrors the schema

-export type InsightsBookingServicePublicOptions = {
-  scope: "user" | "org" | "team";
-  userId: number;
-  orgId: number;
-  teamId?: number;
-};
+export type InsightsBookingServicePublicOptions =
+  | { scope: "user"; userId: number; orgId: number }
+  | { scope: "org"; userId: number; orgId: number }
+  | { scope: "team"; userId: number; orgId: number; teamId: number };

---

68-81: Dead code: authConditions is always truthy, making two branches unreachable.

getAuthorizationConditions() always returns a Prisma.Sql object (either real conditions or NOTHING_CONDITION). A Prisma.Sql instance is always truthy, so the else if (filterConditions) branch (line 76) and the else branch (line 78-80) are unreachable.

♻️ Simplified logic

   async getBaseConditions(): Promise<Prisma.Sql> {
     const authConditions = await this.getAuthorizationConditions();
     const filterConditions = await this.getFilterConditions();
 
-    if (authConditions && filterConditions) {
+    if (filterConditions) {
       return Prisma.sql`(${authConditions}) AND (${filterConditions})`;
-    } else if (authConditions) {
+    } else {
       return authConditions;
-    } else if (filterConditions) {
-      return filterConditions;
-    } else {
-      return NOTHING_CONDITION;
     }
   }

---

104-112: Falsy checks on filter IDs will silently skip 0 values.

if (this.filters.eventTypeId) and if (this.filters.memberUserId) use truthiness, so an ID of 0 would be ignored. While auto-increment IDs are typically positive, using explicit !== undefined (or != null) would be more defensive and express intent more clearly.

♻️ Proposed fix

-    if (this.filters.eventTypeId) {
+    if (this.filters.eventTypeId != null) {
       conditions.push(
         Prisma.sql`("eventTypeId" = ${this.filters.eventTypeId}) OR ("eventParentId" = ${this.filters.eventTypeId})`
       );
     }
 
-    if (this.filters.memberUserId) {
+    if (this.filters.memberUserId != null) {
       conditions.push(Prisma.sql`"userId" = ${this.filters.memberUserId}`);
     }

packages/lib/server/service/__tests__/insightsBooking.integration-test.ts (1)

332-343: Org scope assertion depends on query result ordering, which may be non-deterministic.

The toEqual check on the user IDs array ([testData.user.id, user2.id, user3.id]) assumes MembershipRepository.findAllByTeamIds returns users in a specific order. If the repository doesn't guarantee ordering, this test could be flaky. Since auto-increment IDs are assigned in creation order and the DB likely returns them sorted, this works in practice, but it's inherently fragile.

Consider sorting the IDs in the service's buildOrgAuthorizationCondition before constructing the SQL, which would make both the query and the test deterministic.

[coderabbit-eval]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

Prisma.sql ANY operator PostgreSQL array parameter binding

💡 Result:

For PostgreSQL, bind the whole JS array as a single parameter and use ANY($1) (often with an explicit cast):

Prisma TypedSQL (recommended)

SQL (.sql file):

SELECT id, name
FROM "User"
WHERE id = ANY($1);

TS:

const userIds = [1, 2, 3]
const users = await prisma.$queryRawTyped(getUsersByIds(userIds))

TypedSQL explicitly supports passing PostgreSQL array arguments this way. [1]

Prisma $queryRaw / Prisma.sql

const userIds = [1, 2, 3]

const rows = await prisma.$queryRaw`
  SELECT id, name
  FROM "User"
  WHERE id = ANY(${userIds}::int[]);
`

If your array is UUIDs:

WHERE id = ANY(${userIds}::uuid[]);

Why not IN ($1)?

IN ($1) treats $1 as a single scalar value, not an array; for “array parameter” semantics you typically want = ANY($1::type[]). [3]

Notes

• Prisma raw queries support parameter binding via tagged templates and also via $queryRawUnsafe("... $1 ...", value)-style parameterization. [2]

Sources: [1] Prisma TypedSQL docs (array args + ANY($1)), [2] Prisma raw queries docs (parameterized raw queries / tagged templates), [3] Stack Overflow discussion noting IN doesn’t work with array parameters and recommending = ANY($2::text[]).

---

🏁 Script executed:

# Find and examine the actual code file
cat -n packages/lib/server/service/insightsBooking.ts | sed -n '160,180p'

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 978

---

🏁 Script executed:

# Search for test cases or usage patterns with ANY in the codebase
rg "Prisma\.sql.*ANY" -A 3 -B 3

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 3112

---

🏁 Script executed:

# Check if Prisma.join is used elsewhere in the codebase
rg "Prisma\.join" -B 2 -A 2

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 1546

---

🏁 Script executed:

# Check the types of teamIds and uniqueUserIds
cat -n packages/lib/server/service/insightsBooking.ts | sed -n '140,175p'

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 1578

---

🏁 Script executed:

# Look at the full method signature and variable definitions
cat -n packages/lib/server/service/insightsBooking.ts | sed -n '130,180p'

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 2313

---

🏁 Script executed:

# Check Prisma version
rg '"prisma"' package.json

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 157

---

🏁 Script executed:

# Examine the test to see what types are being used
cat -n packages/lib/server/service/__tests__/insightsBooking.integration-test.ts | sed -n '1,50p'

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 1991

---

Add explicit type casts to ANY() operators, or use Prisma.join() with IN for safety.

The Prisma.sql("teamId" = ANY(${teamIds}))pattern is missing required PostgreSQL type casts. When usingANY()with array parameters in Prisma, you must explicitly cast the array type (e.g.,ANY(${teamIds}::uuid[])orANY(${teamIds}::text[])`). Without the cast, the query will fail at runtime.

A more robust alternative, already used elsewhere in the codebase, is IN with Prisma.join():

Prisma.sql`("teamId" IN (${Prisma.join(teamIds)}))`

Apply the same fix to line 168 for uniqueUserIds.

🤖 Prompt for AI Agents

In `@packages/lib/server/service/insightsBooking.ts` around lines 164 - 174, The
generated SQL uses PostgreSQL ANY() without type casts which breaks at runtime;
update the two Prisma.sql clauses that reference teamIds and uniqueUserIds (the
initial element in the conditions array and the pushed element for
uniqueUserIds) to use IN with Prisma.join instead of ANY (e.g., replace
("teamId" = ANY(${teamIds})) and ("userId" = ANY(${uniqueUserIds})) with
("teamId" IN (${Prisma.join(teamIds)})) and ("userId" IN
(${Prisma.join(uniqueUserIds)}))). Keep the surrounding logic that builds the
conditions array and the reduce() combining logic intact.