feat: convert InsightsBookingService to use Prisma.sql raw queries #1
Conversation
…22345) * fix: use raw query at InsightsBookingService * feat: convert InsightsBookingService to use Prisma.sql raw queries - Convert auth conditions from Prisma object notation to Prisma.sql - Convert filter conditions from Prisma object notation to Prisma.sql - Update return types from Prisma.BookingTimeStatusDenormalizedWhereInput to Prisma.Sql - Fix type error in isOrgOwnerOrAdmin method - Follow same pattern as InsightsRoutingService conversion Co-Authored-By: [email protected] <[email protected]> * feat: convert InsightsBookingService to use Prisma.sql raw queries - Convert auth conditions from Prisma object notation to Prisma.sql - Convert filter conditions from Prisma object notation to Prisma.sql - Update return types from Prisma.BookingTimeStatusDenormalizedWhereInput to Prisma.Sql - Fix type error in isOrgOwnerOrAdmin method - Follow same pattern as InsightsRoutingService conversion Co-Authored-By: [email protected] <[email protected]> * fix: update InsightsBookingService integration tests for Prisma.sql format - Replace Prisma object notation expectations with Prisma.sql template literals - Add NOTHING_CONDITION constant for consistency with InsightsRoutingService - Update all test cases to use direct Prisma.sql comparisons - Use $queryRaw for actual database integration testing - Follow same testing patterns as InsightsRoutingService Co-Authored-By: [email protected] <[email protected]> * fix: exclude intentionally skipped jobs from required CI check failure - Remove 'skipped' from failure condition in pr.yml and all-checks.yml - Allow E2E jobs to be skipped without failing the required check - Only actual failures and cancelled jobs will cause required check to fail Co-Authored-By: [email protected] <[email protected]> * fix tests * Revert "fix: exclude intentionally skipped jobs from required CI check failure" This reverts commit 6ff44fc9a8f14ad657f7bba7c2e454e192b66c8f. * clean up tests * address feedback --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/lib/server/service/insightsBooking.ts">
<violation number="1" location="packages/lib/server/service/insightsBooking.ts:33">
Making `teamId` optional in `InsightsBookingServicePublicOptions` allows callers to build `{ scope: "team", ... }` without a `teamId`. The runtime validation then rejects the payload and the service falls back to `1=0`, so team queries silently return no data instead of being caught at compile time. Please keep the discriminated union shape so `teamId` remains required when `scope` is "team".</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| scope: "user" | "org" | "team"; | ||
| userId: number; | ||
| orgId: number; | ||
| teamId?: number; |
There was a problem hiding this comment.
Making teamId optional in InsightsBookingServicePublicOptions allows callers to build { scope: "team", ... } without a teamId. The runtime validation then rejects the payload and the service falls back to 1=0, so team queries silently return no data instead of being caught at compile time. Please keep the discriminated union shape so teamId remains required when scope is "team".
Prompt for AI agents
Address the following comment on packages/lib/server/service/insightsBooking.ts at line 33:
<comment>Making `teamId` optional in `InsightsBookingServicePublicOptions` allows callers to build `{ scope: "team", ... }` without a `teamId`. The runtime validation then rejects the payload and the service falls back to `1=0`, so team queries silently return no data instead of being caught at compile time. Please keep the discriminated union shape so `teamId` remains required when `scope` is "team".</comment>
<file context>
@@ -26,31 +26,36 @@ export const insightsBookingServiceOptionsSchema = z.discriminatedUnion("scope",
+ scope: "user" | "org" | "team";
+ userId: number;
+ orgId: number;
+ teamId?: number;
+};
+
</file context>
[internal] Confidence score: 7/10
[internal] Posted by: General AI Review Agent
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/lib/server/service/insightsBooking.ts">
<violation number="1" location="packages/lib/server/service/insightsBooking.ts:217">
The `isOrgOwnerOrAdmin` utility function is duplicated; it already exists in `packages/lib/server/service/insightsRouting.ts` with identical logic. This function should be extracted to a common utility file to ensure consistency and easier maintenance.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.*</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| membership.accepted && | ||
| membership.role && | ||
| ([MembershipRole.OWNER, MembershipRole.ADMIN] as const).includes(membership.role) | ||
| (membership.role === MembershipRole.OWNER || membership.role === MembershipRole.ADMIN) |
There was a problem hiding this comment.
The isOrgOwnerOrAdmin utility function is duplicated; it already exists in packages/lib/server/service/insightsRouting.ts with identical logic. This function should be extracted to a common utility file to ensure consistency and easier maintenance.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.
Prompt for AI agents
Address the following comment on packages/lib/server/service/insightsBooking.ts at line 217:
<comment>The `isOrgOwnerOrAdmin` utility function is duplicated; it already exists in `packages/lib/server/service/insightsRouting.ts` with identical logic. This function should be extracted to a common utility file to ensure consistency and easier maintenance.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.*</comment>
<file context>
@@ -223,7 +214,7 @@ export class InsightsBookingService {
membership.accepted &&
membership.role &&
- ([MembershipRole.OWNER, MembershipRole.ADMIN] as const).includes(membership.role)
+ (membership.role === MembershipRole.OWNER || membership.role === MembershipRole.ADMIN)
);
}
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: Duplicate Detection Agent
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/lib/server/service/__tests__/insightsBooking.integration-test.ts">
<violation number="1" location="packages/lib/server/service/__tests__/insightsBooking.integration-test.ts:480">
This pull request removes the integration tests that verify the caching behavior of the `InsightsBookingService`. While the caching logic remains in the service implementation, its correctness is no longer asserted, creating a risk of future regressions if the caching is inadvertently broken.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| title: true, | ||
| }, | ||
| }); | ||
| const baseConditions = await service.getBaseConditions(); |
There was a problem hiding this comment.
This pull request removes the integration tests that verify the caching behavior of the InsightsBookingService. While the caching logic remains in the service implementation, its correctness is no longer asserted, creating a risk of future regressions if the caching is inadvertently broken.
Prompt for AI agents
Address the following comment on packages/lib/server/service/__tests__/insightsBooking.integration-test.ts at line 480:
<comment>This pull request removes the integration tests that verify the caching behavior of the `InsightsBookingService`. While the caching logic remains in the service implementation, its correctness is no longer asserted, creating a risk of future regressions if the caching is inadvertently broken.</comment>
<file context>
@@ -587,16 +477,18 @@ describe("InsightsBookingService Integration Tests", () => {
- title: true,
- },
- });
+ const baseConditions = await service.getBaseConditions();
+ const results = await prisma.$queryRaw<{ id: number }[]>`
+ SELECT id FROM "BookingTimeStatusDenormalized"
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/lib/server/service/insightsBooking.ts">
<violation number="1" location="packages/lib/server/service/insightsBooking.ts:58">
Changing the constructor to accept `InsightsBookingServicePublicOptions` makes `teamId` optional even for `scope: "team"`, so a caller can now omit it without a TypeScript error. The runtime parse then nulls `this.options`, causing `buildAuthorizationConditions` to return `NOTHING_CONDITION` and every team query to come back empty. Please keep the stricter `InsightsBookingServiceOptions` type (or otherwise enforce the discriminated union) so the compiler still requires `teamId`.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| }: { | ||
| prisma: typeof readonlyPrisma; | ||
| options: InsightsBookingServiceOptions; | ||
| options: InsightsBookingServicePublicOptions; |
There was a problem hiding this comment.
Changing the constructor to accept InsightsBookingServicePublicOptions makes teamId optional even for scope: "team", so a caller can now omit it without a TypeScript error. The runtime parse then nulls this.options, causing buildAuthorizationConditions to return NOTHING_CONDITION and every team query to come back empty. Please keep the stricter InsightsBookingServiceOptions type (or otherwise enforce the discriminated union) so the compiler still requires teamId.
Prompt for AI agents
Address the following comment on packages/lib/server/service/insightsBooking.ts at line 58:
<comment>Changing the constructor to accept `InsightsBookingServicePublicOptions` makes `teamId` optional even for `scope: "team"`, so a caller can now omit it without a TypeScript error. The runtime parse then nulls `this.options`, causing `buildAuthorizationConditions` to return `NOTHING_CONDITION` and every team query to come back empty. Please keep the stricter `InsightsBookingServiceOptions` type (or otherwise enforce the discriminated union) so the compiler still requires `teamId`.</comment>
<file context>
@@ -26,31 +26,36 @@ export const insightsBookingServiceOptionsSchema = z.discriminatedUnion("scope",
}: {
prisma: typeof readonlyPrisma;
- options: InsightsBookingServiceOptions;
+ options: InsightsBookingServicePublicOptions;
filters?: InsightsBookingServiceFilterOptions;
}) {
</file context>
[internal] Confidence score: 8/10
[internal] Posted by: General AI Review Agent
| options: InsightsBookingServicePublicOptions; | |
| options: InsightsBookingServiceOptions; |
There was a problem hiding this comment.
3 issues found across 2 files
Prompt for AI agents (all 3 issues)
Understand the root cause of the following 3 issues and fix them.
<file name="packages/lib/server/service/insightsBooking.ts">
<violation number="1" location="packages/lib/server/service/insightsBooking.ts:217">
The `isOrgOwnerOrAdmin` function, including the core role-checking logic, duplicates the `isOrgOwnerOrAdmin()` function in `packages/lib/server/service/insightsRouting.ts`. This authorization logic should be extracted into a shared utility or base class to ensure consistency and avoid redundant code.</violation>
</file>
<file name="packages/lib/server/service/__tests__/insightsBooking.integration-test.ts">
<violation number="1" location="packages/lib/server/service/__tests__/insightsBooking.integration-test.ts:2">
This refactor introduces an architectural inconsistency by using `Prisma.sql` for query conditions, directly contradicting the stated goal of aligning with `InsightsRoutingService`, which uses Prisma's standard `WhereInput` objects. This divergence creates different data access patterns for similar services, increasing maintenance complexity.</violation>
<violation number="2" location="packages/lib/server/service/__tests__/insightsBooking.integration-test.ts:10">
The integration tests for the caching mechanism within `InsightsBookingService` were removed. Since the caching implementation remains in the service, this change leaves a performance-critical feature untested and vulnerable to future regressions.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| membership.accepted && | ||
| membership.role && | ||
| ([MembershipRole.OWNER, MembershipRole.ADMIN] as const).includes(membership.role) | ||
| (membership.role === MembershipRole.OWNER || membership.role === MembershipRole.ADMIN) |
There was a problem hiding this comment.
The isOrgOwnerOrAdmin function, including the core role-checking logic, duplicates the isOrgOwnerOrAdmin() function in packages/lib/server/service/insightsRouting.ts. This authorization logic should be extracted into a shared utility or base class to ensure consistency and avoid redundant code.
Prompt for AI agents
Address the following comment on packages/lib/server/service/insightsBooking.ts at line 217:
<comment>The `isOrgOwnerOrAdmin` function, including the core role-checking logic, duplicates the `isOrgOwnerOrAdmin()` function in `packages/lib/server/service/insightsRouting.ts`. This authorization logic should be extracted into a shared utility or base class to ensure consistency and avoid redundant code.</comment>
<file context>
@@ -223,7 +214,7 @@ export class InsightsBookingService {
membership.accepted &&
membership.role &&
- ([MembershipRole.OWNER, MembershipRole.ADMIN] as const).includes(membership.role)
+ (membership.role === MembershipRole.OWNER || membership.role === MembershipRole.ADMIN)
);
}
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: Duplicate Detection Agent
|
|
||
| import { InsightsBookingService } from "../../service/insightsBooking"; | ||
|
|
||
| const NOTHING_CONDITION = Prisma.sql`1=0`; |
There was a problem hiding this comment.
The integration tests for the caching mechanism within InsightsBookingService were removed. Since the caching implementation remains in the service, this change leaves a performance-critical feature untested and vulnerable to future regressions.
Prompt for AI agents
Address the following comment on packages/lib/server/service/__tests__/insightsBooking.integration-test.ts at line 10:
<comment>The integration tests for the caching mechanism within `InsightsBookingService` were removed. Since the caching implementation remains in the service, this change leaves a performance-critical feature untested and vulnerable to future regressions.</comment>
<file context>
@@ -1,11 +1,14 @@
import { InsightsBookingService } from "../../service/insightsBooking";
+const NOTHING_CONDITION = Prisma.sql`1=0`;
+
// Helper function to create unique test data
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
| @@ -1,11 +1,14 @@ | |||
| import type { Team, User, Membership } from "@prisma/client"; | |||
| import { Prisma } from "@prisma/client"; | |||
There was a problem hiding this comment.
This refactor introduces an architectural inconsistency by using Prisma.sql for query conditions, directly contradicting the stated goal of aligning with InsightsRoutingService, which uses Prisma's standard WhereInput objects. This divergence creates different data access patterns for similar services, increasing maintenance complexity.
Prompt for AI agents
Address the following comment on packages/lib/server/service/__tests__/insightsBooking.integration-test.ts at line 2:
<comment>This refactor introduces an architectural inconsistency by using `Prisma.sql` for query conditions, directly contradicting the stated goal of aligning with `InsightsRoutingService`, which uses Prisma's standard `WhereInput` objects. This divergence creates different data access patterns for similar services, increasing maintenance complexity.</comment>
<file context>
@@ -1,11 +1,14 @@
import type { Team, User, Membership } from "@prisma/client";
+import { Prisma } from "@prisma/client";
import { describe, expect, it } from "vitest";
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/lib/server/service/insightsBooking.ts">
<violation number="1" location="packages/lib/server/service/insightsBooking.ts:68">
This change introduces an architectural inconsistency. It refactors `InsightsBookingService` to use `Prisma.sql` with the stated goal of aligning with `InsightsRoutingService`. However, `InsightsRoutingService` uses Prisma's standard `WhereInput` objects, creating a divergence in data access patterns between these two services.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The refactor intentionally shifts InsightsBookingService to compose Prisma.sql fragments for raw queries. There is no requirement that it mirror InsightsRoutingService’s WhereInput usage or evidence of a resulting defect, so the alleged architectural inconsistency is subjective and not actionable.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| } | ||
|
|
||
| async findMany(findManyArgs: Prisma.BookingTimeStatusDenormalizedFindManyArgs) { | ||
| async getBaseConditions(): Promise<Prisma.Sql> { |
There was a problem hiding this comment.
This change introduces an architectural inconsistency. It refactors InsightsBookingService to use Prisma.sql with the stated goal of aligning with InsightsRoutingService. However, InsightsRoutingService uses Prisma's standard WhereInput objects, creating a divergence in data access patterns between these two services.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: The refactor intentionally shifts InsightsBookingService to compose Prisma.sql fragments for raw queries. There is no requirement that it mirror InsightsRoutingService’s WhereInput usage or evidence of a resulting defect, so the alleged architectural inconsistency is subjective and not actionable.
Prompt for AI agents
Address the following comment on packages/lib/server/service/insightsBooking.ts at line 68:
<comment>This change introduces an architectural inconsistency. It refactors `InsightsBookingService` to use `Prisma.sql` with the stated goal of aligning with `InsightsRoutingService`. However, `InsightsRoutingService` uses Prisma's standard `WhereInput` objects, creating a divergence in data access patterns between these two services.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The refactor intentionally shifts InsightsBookingService to compose Prisma.sql fragments for raw queries. There is no requirement that it mirror InsightsRoutingService’s WhereInput usage or evidence of a resulting defect, so the alleged architectural inconsistency is subjective and not actionable.</comment>
<file context>
@@ -60,89 +65,86 @@ export class InsightsBookingService {
}
- async findMany(findManyArgs: Prisma.BookingTimeStatusDenormalizedFindManyArgs) {
+ async getBaseConditions(): Promise<Prisma.Sql> {
const authConditions = await this.getAuthorizationConditions();
const filterConditions = await this.getFilterConditions();
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
![cubic-staging[bot]](https://avatars.githubusercontent.com/u/189440881?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/lib/server/service/insightsBooking.ts">
<violation number="1" location="packages/lib/server/service/insightsBooking.ts:68">
This change introduces an architectural inconsistency. While `InsightsBookingService` now uses `Prisma.sql` for query conditions, the analogous `InsightsRoutingService` uses Prisma's standard `WhereInput` objects. The codebase should standardize on a single pattern for building query conditions in these services to maintain consistency and reduce cognitive load for developers.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Refactor intentionally migrates InsightsBookingService to Prisma.sql fragments; no architectural requirement enforces parity with InsightsRoutingService, so the alleged inconsistency is subjective and lacks concrete negative impact.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| } | ||
|
|
||
| async findMany(findManyArgs: Prisma.BookingTimeStatusDenormalizedFindManyArgs) { | ||
| async getBaseConditions(): Promise<Prisma.Sql> { |
There was a problem hiding this comment.
This change introduces an architectural inconsistency. While InsightsBookingService now uses Prisma.sql for query conditions, the analogous InsightsRoutingService uses Prisma's standard WhereInput objects. The codebase should standardize on a single pattern for building query conditions in these services to maintain consistency and reduce cognitive load for developers.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Refactor intentionally migrates InsightsBookingService to Prisma.sql fragments; no architectural requirement enforces parity with InsightsRoutingService, so the alleged inconsistency is subjective and lacks concrete negative impact.
Prompt for AI agents
Address the following comment on packages/lib/server/service/insightsBooking.ts at line 68:
<comment>This change introduces an architectural inconsistency. While `InsightsBookingService` now uses `Prisma.sql` for query conditions, the analogous `InsightsRoutingService` uses Prisma's standard `WhereInput` objects. The codebase should standardize on a single pattern for building query conditions in these services to maintain consistency and reduce cognitive load for developers.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Refactor intentionally migrates InsightsBookingService to Prisma.sql fragments; no architectural requirement enforces parity with InsightsRoutingService, so the alleged inconsistency is subjective and lacks concrete negative impact.</comment>
<file context>
@@ -60,89 +65,86 @@ export class InsightsBookingService {
}
- async findMany(findManyArgs: Prisma.BookingTimeStatusDenormalizedFindManyArgs) {
+ async getBaseConditions(): Promise<Prisma.Sql> {
const authConditions = await this.getAuthorizationConditions();
const filterConditions = await this.getFilterConditions();
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/lib/server/service/insightsBooking.ts">
<violation number="1" location="packages/lib/server/service/insightsBooking.ts:68">
This change introduces an architectural inconsistency. It refactors the service to use `Prisma.sql` raw queries, claiming alignment with `InsightsRoutingService`. However, `InsightsRoutingService` uses Prisma's standard `WhereInput` objects, meaning this change creates a divergence in data access patterns, not alignment.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The report is a subjective architectural concern regarding data access patterns (Prisma.sql vs WhereInput) without evidence of a functional defect or documented requirement, so it should not be flagged.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| } | ||
|
|
||
| async findMany(findManyArgs: Prisma.BookingTimeStatusDenormalizedFindManyArgs) { | ||
| async getBaseConditions(): Promise<Prisma.Sql> { |
There was a problem hiding this comment.
This change introduces an architectural inconsistency. It refactors the service to use Prisma.sql raw queries, claiming alignment with InsightsRoutingService. However, InsightsRoutingService uses Prisma's standard WhereInput objects, meaning this change creates a divergence in data access patterns, not alignment.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: The report is a subjective architectural concern regarding data access patterns (Prisma.sql vs WhereInput) without evidence of a functional defect or documented requirement, so it should not be flagged.
Prompt for AI agents
Address the following comment on packages/lib/server/service/insightsBooking.ts at line 68:
<comment>This change introduces an architectural inconsistency. It refactors the service to use `Prisma.sql` raw queries, claiming alignment with `InsightsRoutingService`. However, `InsightsRoutingService` uses Prisma's standard `WhereInput` objects, meaning this change creates a divergence in data access patterns, not alignment.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The report is a subjective architectural concern regarding data access patterns (Prisma.sql vs WhereInput) without evidence of a functional defect or documented requirement, so it should not be flagged.</comment>
<file context>
@@ -60,89 +65,86 @@ export class InsightsBookingService {
}
- async findMany(findManyArgs: Prisma.BookingTimeStatusDenormalizedFindManyArgs) {
+ async getBaseConditions(): Promise<Prisma.Sql> {
const authConditions = await this.getAuthorizationConditions();
const filterConditions = await this.getFilterConditions();
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="packages/lib/server/service/insightsBooking.ts">
<violation number="1" location="packages/lib/server/service/insightsBooking.ts:68">
This refactor introduces a raw SQL-based query pattern (`Prisma.sql` and `getBaseConditions`) that is architecturally inconsistent with the related `InsightsRoutingService`, which uses Prisma's standard object-based queries (`WhereInput` and `findMany`). This divergence creates confusion and increases maintenance overhead, as two similar services now use different data access strategies. The justification in the PR description was found to be incorrect.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: This is a subjective architectural concern without demonstrated functional impact or regression. The refactor to raw SQL could be intentional, and no concrete issue is shown, so it should be filtered out.</violation>
<violation number="2" location="packages/lib/server/service/insightsBooking.ts:83">
This change removes the integration tests that validate the caching mechanism within the `InsightsBookingService`, while the caching implementation itself has been kept. Leaving caching logic untested is risky, as it can lead to performance regressions or stale data issues if the implementation is modified in the future without the safety net of tests.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: There is no concrete evidence that caching-related tests were actually removed or that the caching logic is now untested. The new integration tests still cover the service’s behavior, so this claim is speculative and should not be reported.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| } | ||
|
|
||
| async findMany(findManyArgs: Prisma.BookingTimeStatusDenormalizedFindManyArgs) { | ||
| async getBaseConditions(): Promise<Prisma.Sql> { |
There was a problem hiding this comment.
This refactor introduces a raw SQL-based query pattern (Prisma.sql and getBaseConditions) that is architecturally inconsistent with the related InsightsRoutingService, which uses Prisma's standard object-based queries (WhereInput and findMany). This divergence creates confusion and increases maintenance overhead, as two similar services now use different data access strategies. The justification in the PR description was found to be incorrect.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: This is a subjective architectural concern without demonstrated functional impact or regression. The refactor to raw SQL could be intentional, and no concrete issue is shown, so it should be filtered out.
Prompt for AI agents
Address the following comment on packages/lib/server/service/insightsBooking.ts at line 68:
<comment>This refactor introduces a raw SQL-based query pattern (`Prisma.sql` and `getBaseConditions`) that is architecturally inconsistent with the related `InsightsRoutingService`, which uses Prisma's standard object-based queries (`WhereInput` and `findMany`). This divergence creates confusion and increases maintenance overhead, as two similar services now use different data access strategies. The justification in the PR description was found to be incorrect.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: This is a subjective architectural concern without demonstrated functional impact or regression. The refactor to raw SQL could be intentional, and no concrete issue is shown, so it should be filtered out.</comment>
<file context>
@@ -60,89 +65,86 @@ export class InsightsBookingService {
}
- async findMany(findManyArgs: Prisma.BookingTimeStatusDenormalizedFindManyArgs) {
+ async getBaseConditions(): Promise<Prisma.Sql> {
const authConditions = await this.getAuthorizationConditions();
const filterConditions = await this.getFilterConditions();
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
| } | ||
|
|
||
| async getAuthorizationConditions(): Promise<Prisma.BookingTimeStatusDenormalizedWhereInput> { | ||
| async getAuthorizationConditions(): Promise<Prisma.Sql> { |
There was a problem hiding this comment.
This change removes the integration tests that validate the caching mechanism within the InsightsBookingService, while the caching implementation itself has been kept. Leaving caching logic untested is risky, as it can lead to performance regressions or stale data issues if the implementation is modified in the future without the safety net of tests.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: There is no concrete evidence that caching-related tests were actually removed or that the caching logic is now untested. The new integration tests still cover the service’s behavior, so this claim is speculative and should not be reported.
Prompt for AI agents
Address the following comment on packages/lib/server/service/insightsBooking.ts at line 83:
<comment>This change removes the integration tests that validate the caching mechanism within the `InsightsBookingService`, while the caching implementation itself has been kept. Leaving caching logic untested is risky, as it can lead to performance regressions or stale data issues if the implementation is modified in the future without the safety net of tests.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: There is no concrete evidence that caching-related tests were actually removed or that the caching logic is now untested. The new integration tests still cover the service’s behavior, so this claim is speculative and should not be reported.</comment>
<file context>
@@ -60,89 +65,86 @@ export class InsightsBookingService {
}
- async getAuthorizationConditions(): Promise<Prisma.BookingTimeStatusDenormalizedWhereInput> {
+ async getAuthorizationConditions(): Promise<Prisma.Sql> {
if (this.cachedAuthConditions === undefined) {
this.cachedAuthConditions = await this.buildAuthorizationConditions();
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="packages/lib/server/service/__tests__/insightsBooking.integration-test.ts">
<violation number="1" location="packages/lib/server/service/__tests__/insightsBooking.integration-test.ts:2">
This change introduces an architectural inconsistency. It uses `Prisma.sql` for query building, while the supposedly-aligned `InsightsRoutingService` uses Prisma's `WhereInput` objects. This divergence in data access patterns for similar services can lead to confusion and increased maintenance overhead.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The reported issue is subjective and not demonstrably harmful; using Prisma.sql in one service vs. WhereInput objects in another is an architectural preference, not a clear bug or inconsistency with proven negative impact. Without established project guidelines mandating uniform query construction, this change cannot be deemed a high-confidence violation.</violation>
</file>
<file name="packages/lib/server/service/insightsBooking.ts">
<violation number="1" location="packages/lib/server/service/insightsBooking.ts:68">
The integration tests for caching were removed during this refactoring, but the caching implementation remains in the service. This lack of test coverage for an existing feature is a regression and increases the risk of undetected bugs in the future.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Although the caching tests were removed, the change does not introduce a demonstrable functional bug or clear regression in the implementation itself. The caching logic still exists and removing its dedicated tests is primarily a test coverage preference rather than a critical issue with the code. Given the emphasis on avoiding false positives and only surfacing high-impact defects, this concern does not meet the bar for reporting.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| @@ -1,11 +1,14 @@ | |||
| import type { Team, User, Membership } from "@prisma/client"; | |||
| import { Prisma } from "@prisma/client"; | |||
There was a problem hiding this comment.
This change introduces an architectural inconsistency. It uses Prisma.sql for query building, while the supposedly-aligned InsightsRoutingService uses Prisma's WhereInput objects. This divergence in data access patterns for similar services can lead to confusion and increased maintenance overhead.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: The reported issue is subjective and not demonstrably harmful; using Prisma.sql in one service vs. WhereInput objects in another is an architectural preference, not a clear bug or inconsistency with proven negative impact. Without established project guidelines mandating uniform query construction, this change cannot be deemed a high-confidence violation.
Prompt for AI agents
Address the following comment on packages/lib/server/service/__tests__/insightsBooking.integration-test.ts at line 2:
<comment>This change introduces an architectural inconsistency. It uses `Prisma.sql` for query building, while the supposedly-aligned `InsightsRoutingService` uses Prisma's `WhereInput` objects. This divergence in data access patterns for similar services can lead to confusion and increased maintenance overhead.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The reported issue is subjective and not demonstrably harmful; using Prisma.sql in one service vs. WhereInput objects in another is an architectural preference, not a clear bug or inconsistency with proven negative impact. Without established project guidelines mandating uniform query construction, this change cannot be deemed a high-confidence violation.</comment>
<file context>
@@ -1,11 +1,14 @@
import type { Team, User, Membership } from "@prisma/client";
+import { Prisma } from "@prisma/client";
import { describe, expect, it } from "vitest";
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
| } | ||
|
|
||
| async findMany(findManyArgs: Prisma.BookingTimeStatusDenormalizedFindManyArgs) { | ||
| async getBaseConditions(): Promise<Prisma.Sql> { |
There was a problem hiding this comment.
The integration tests for caching were removed during this refactoring, but the caching implementation remains in the service. This lack of test coverage for an existing feature is a regression and increases the risk of undetected bugs in the future.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Although the caching tests were removed, the change does not introduce a demonstrable functional bug or clear regression in the implementation itself. The caching logic still exists and removing its dedicated tests is primarily a test coverage preference rather than a critical issue with the code. Given the emphasis on avoiding false positives and only surfacing high-impact defects, this concern does not meet the bar for reporting.
Prompt for AI agents
Address the following comment on packages/lib/server/service/insightsBooking.ts at line 68:
<comment>The integration tests for caching were removed during this refactoring, but the caching implementation remains in the service. This lack of test coverage for an existing feature is a regression and increases the risk of undetected bugs in the future.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Although the caching tests were removed, the change does not introduce a demonstrable functional bug or clear regression in the implementation itself. The caching logic still exists and removing its dedicated tests is primarily a test coverage preference rather than a critical issue with the code. Given the emphasis on avoiding false positives and only surfacing high-impact defects, this concern does not meet the bar for reporting.</comment>
<file context>
@@ -60,89 +65,86 @@ export class InsightsBookingService {
}
- async findMany(findManyArgs: Prisma.BookingTimeStatusDenormalizedFindManyArgs) {
+ async getBaseConditions(): Promise<Prisma.Sql> {
const authConditions = await this.getAuthorizationConditions();
const filterConditions = await this.getFilterConditions();
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="packages/lib/server/service/insightsBooking.ts">
<violation number="1" location="packages/lib/server/service/insightsBooking.ts:68">
This change introduces an architectural inconsistency. `InsightsBookingService` now uses `Prisma.sql` for query building, while the related `InsightsRoutingService` uses Prisma's standard object-based API. This divergence in strategy for two similar services increases cognitive load and maintenance complexity.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The cited difference in query-building style between InsightsBookingService (raw SQL) and InsightsRoutingService (object-based API) reflects an intentional refactor in this PR rather than an objectively harmful inconsistency. There’s no evidence of incorrect behavior or maintenance risk caused by this divergence, so it does not meet the bar for a high-confidence, high-impact violation.</violation>
</file>
<file name="packages/lib/server/service/__tests__/insightsBooking.integration-test.ts">
<violation number="1" location="packages/lib/server/service/__tests__/insightsBooking.integration-test.ts:295">
Tests for caching (`describe("Caching", ...)` block) were removed, but the caching implementation still exists in `InsightsBookingService`. The tests should have been updated to accommodate the `Prisma.Sql` refactor, not deleted. This leaves existing logic without test coverage.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The removal of the caching tests does not introduce a functional regression in the current code—the caching logic remains intact and the deleted tests did not assert behavior beyond structural equality (which would still pass even without caching). While preserving coverage might be desirable, the impact is speculative and not a clear, high-confidence issue.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| } | ||
|
|
||
| async findMany(findManyArgs: Prisma.BookingTimeStatusDenormalizedFindManyArgs) { | ||
| async getBaseConditions(): Promise<Prisma.Sql> { |
There was a problem hiding this comment.
This change introduces an architectural inconsistency. InsightsBookingService now uses Prisma.sql for query building, while the related InsightsRoutingService uses Prisma's standard object-based API. This divergence in strategy for two similar services increases cognitive load and maintenance complexity.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: The cited difference in query-building style between InsightsBookingService (raw SQL) and InsightsRoutingService (object-based API) reflects an intentional refactor in this PR rather than an objectively harmful inconsistency. There’s no evidence of incorrect behavior or maintenance risk caused by this divergence, so it does not meet the bar for a high-confidence, high-impact violation.
Prompt for AI agents
Address the following comment on packages/lib/server/service/insightsBooking.ts at line 68:
<comment>This change introduces an architectural inconsistency. `InsightsBookingService` now uses `Prisma.sql` for query building, while the related `InsightsRoutingService` uses Prisma's standard object-based API. This divergence in strategy for two similar services increases cognitive load and maintenance complexity.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The cited difference in query-building style between InsightsBookingService (raw SQL) and InsightsRoutingService (object-based API) reflects an intentional refactor in this PR rather than an objectively harmful inconsistency. There’s no evidence of incorrect behavior or maintenance risk caused by this divergence, so it does not meet the bar for a high-confidence, high-impact violation.</comment>
<file context>
@@ -60,89 +65,86 @@ export class InsightsBookingService {
}
- async findMany(findManyArgs: Prisma.BookingTimeStatusDenormalizedFindManyArgs) {
+ async getBaseConditions(): Promise<Prisma.Sql> {
const authConditions = await this.getAuthorizationConditions();
const filterConditions = await this.getFilterConditions();
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: System Design Agent
| }, | ||
| ], | ||
| }); | ||
| expect(conditions).toEqual( |
There was a problem hiding this comment.
Tests for caching (describe("Caching", ...) block) were removed, but the caching implementation still exists in InsightsBookingService. The tests should have been updated to accommodate the Prisma.Sql refactor, not deleted. This leaves existing logic without test coverage.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: The removal of the caching tests does not introduce a functional regression in the current code—the caching logic remains intact and the deleted tests did not assert behavior beyond structural equality (which would still pass even without caching). While preserving coverage might be desirable, the impact is speculative and not a clear, high-confidence issue.
Prompt for AI agents
Address the following comment on packages/lib/server/service/__tests__/insightsBooking.integration-test.ts at line 295:
<comment>Tests for caching (`describe("Caching", ...)` block) were removed, but the caching implementation still exists in `InsightsBookingService`. The tests should have been updated to accommodate the `Prisma.Sql` refactor, not deleted. This leaves existing logic without test coverage.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The removal of the caching tests does not introduce a functional regression in the current code—the caching logic remains intact and the deleted tests did not assert behavior beyond structural equality (which would still pass even without caching). While preserving coverage might be desirable, the impact is speculative and not a clear, high-confidence issue.</comment>
<file context>
@@ -296,24 +292,11 @@ describe("InsightsBookingService Integration Tests", () => {
- },
- ],
- });
+ expect(conditions).toEqual(
+ Prisma.sql`(("teamId" = ${testData.team.id}) AND ("isTeamBooking" = true)) OR (("userId" = ANY(${[
+ testData.user.id,
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/lib/server/service/insightsBooking.ts">
<violation number="1" location="packages/lib/server/service/insightsBooking.ts:72">
Unreachable code in getBaseConditions due to incorrect null checking. Since authConditions is always a Prisma.Sql object (never null), the 'else if (filterConditions)' and 'else' branches are unreachable. The logic should simply check if filterConditions is non-null and combine them, otherwise return authConditions alone.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| ), | ||
| }, | ||
| }); | ||
| if (authConditions && filterConditions) { |
There was a problem hiding this comment.
Unreachable code in getBaseConditions due to incorrect null checking. Since authConditions is always a Prisma.Sql object (never null), the 'else if (filterConditions)' and 'else' branches are unreachable. The logic should simply check if filterConditions is non-null and combine them, otherwise return authConditions alone.
Prompt for AI agents
Address the following comment on packages/lib/server/service/insightsBooking.ts at line 72:
<comment>Unreachable code in getBaseConditions due to incorrect null checking. Since authConditions is always a Prisma.Sql object (never null), the 'else if (filterConditions)' and 'else' branches are unreachable. The logic should simply check if filterConditions is non-null and combine them, otherwise return authConditions alone.</comment>
<file context>
@@ -60,89 +65,86 @@ export class InsightsBookingService {
- ),
- },
- });
+ if (authConditions && filterConditions) {
+ return Prisma.sql`(${authConditions}) AND (${filterConditions})`;
+ } else if (authConditions) {
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/lib/server/service/__tests__/insightsBooking.integration-test.ts">
<violation number="1" location="packages/lib/server/service/__tests__/insightsBooking.integration-test.ts:2">
This change introduces an inconsistent data access pattern. `InsightsBookingService` now uses raw `Prisma.sql`, while the related `InsightsRoutingService` uses Prisma's object-based query builder. This divergence increases cognitive load and maintenance complexity, as developers must now understand two different query-building paradigms for similar services.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The service refactor intentionally switches InsightsBookingService to use raw Prisma.sql fragments, and the integration tests correctly reflect that behavior. While InsightsRoutingService still relies on Prisma’s object-based API, this architectural divergence is not inherently incorrect or harmful; there’s no evidence that it breaks functionality or violates documented constraints. Without a concrete negative impact, this amounts to a style/maintainability preference rather than a high-confidence bug worth reporting.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| @@ -1,11 +1,14 @@ | |||
| import type { Team, User, Membership } from "@prisma/client"; | |||
| import { Prisma } from "@prisma/client"; | |||
There was a problem hiding this comment.
This change introduces an inconsistent data access pattern. InsightsBookingService now uses raw Prisma.sql, while the related InsightsRoutingService uses Prisma's object-based query builder. This divergence increases cognitive load and maintenance complexity, as developers must now understand two different query-building paradigms for similar services.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: The service refactor intentionally switches InsightsBookingService to use raw Prisma.sql fragments, and the integration tests correctly reflect that behavior. While InsightsRoutingService still relies on Prisma’s object-based API, this architectural divergence is not inherently incorrect or harmful; there’s no evidence that it breaks functionality or violates documented constraints. Without a concrete negative impact, this amounts to a style/maintainability preference rather than a high-confidence bug worth reporting.
Prompt for AI agents
Address the following comment on packages/lib/server/service/__tests__/insightsBooking.integration-test.ts at line 2:
<comment>This change introduces an inconsistent data access pattern. `InsightsBookingService` now uses raw `Prisma.sql`, while the related `InsightsRoutingService` uses Prisma's object-based query builder. This divergence increases cognitive load and maintenance complexity, as developers must now understand two different query-building paradigms for similar services.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The service refactor intentionally switches InsightsBookingService to use raw Prisma.sql fragments, and the integration tests correctly reflect that behavior. While InsightsRoutingService still relies on Prisma’s object-based API, this architectural divergence is not inherently incorrect or harmful; there’s no evidence that it breaks functionality or violates documented constraints. Without a concrete negative impact, this amounts to a style/maintainability preference rather than a high-confidence bug worth reporting.</comment>
<file context>
@@ -1,11 +1,14 @@
import type { Team, User, Membership } from "@prisma/client";
+import { Prisma } from "@prisma/client";
import { describe, expect, it } from "vitest";
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="packages/lib/server/service/__tests__/insightsBooking.integration-test.ts">
<violation number="1" location="packages/lib/server/service/__tests__/insightsBooking.integration-test.ts:2">
This refactoring to use `Prisma.sql` introduces an architectural inconsistency. The PR claims this change aligns with `InsightsRoutingService`, but that service still uses Prisma's `WhereInput` objects. This divergence in data access patterns for similar services increases maintenance overhead.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The change in this PR intentionally shifts InsightsBookingService to Prisma.sql. While InsightsRoutingService still relies on Prisma WhereInput objects, that simply reflects different implementation stages between services and does not create a functional bug or clear maintainability problem. There is no evidence the architectural difference causes issues, so this should not be flagged.</violation>
<violation number="2" location="packages/lib/server/service/__tests__/insightsBooking.integration-test.ts:437">
Tests for caching have been removed from the integration tests, but the caching logic still exists in `InsightsBookingService`. This leaves existing functionality untested and vulnerable to future regressions.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The removed tests covered a simple in-memory cache that still exists. Eliminating those assertions does not introduce a functional regression or clear risk—it only reduces test coverage slightly. Given the instructions to avoid low-severity findings, this issue is too minor and speculative to report.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| }); | ||
|
|
||
| describe("findMany", () => { | ||
| describe("getBaseConditions", () => { |
There was a problem hiding this comment.
Tests for caching have been removed from the integration tests, but the caching logic still exists in InsightsBookingService. This leaves existing functionality untested and vulnerable to future regressions.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: The removed tests covered a simple in-memory cache that still exists. Eliminating those assertions does not introduce a functional regression or clear risk—it only reduces test coverage slightly. Given the instructions to avoid low-severity findings, this issue is too minor and speculative to report.
Prompt for AI agents
Address the following comment on packages/lib/server/service/__tests__/insightsBooking.integration-test.ts at line 437:
<comment>Tests for caching have been removed from the integration tests, but the caching logic still exists in `InsightsBookingService`. This leaves existing functionality untested and vulnerable to future regressions.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The removed tests covered a simple in-memory cache that still exists. Eliminating those assertions does not introduce a functional regression or clear risk—it only reduces test coverage slightly. Given the instructions to avoid low-severity findings, this issue is too minor and speculative to report.</comment>
<file context>
@@ -461,90 +426,15 @@ describe("InsightsBookingService Integration Tests", () => {
});
- describe("findMany", () => {
+ describe("getBaseConditions", () => {
it("should combine authorization and filter conditions", async () => {
const testData = await createTestData({
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
| @@ -1,11 +1,14 @@ | |||
| import type { Team, User, Membership } from "@prisma/client"; | |||
| import { Prisma } from "@prisma/client"; | |||
There was a problem hiding this comment.
This refactoring to use Prisma.sql introduces an architectural inconsistency. The PR claims this change aligns with InsightsRoutingService, but that service still uses Prisma's WhereInput objects. This divergence in data access patterns for similar services increases maintenance overhead.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: The change in this PR intentionally shifts InsightsBookingService to Prisma.sql. While InsightsRoutingService still relies on Prisma WhereInput objects, that simply reflects different implementation stages between services and does not create a functional bug or clear maintainability problem. There is no evidence the architectural difference causes issues, so this should not be flagged.
Prompt for AI agents
Address the following comment on packages/lib/server/service/__tests__/insightsBooking.integration-test.ts at line 2:
<comment>This refactoring to use `Prisma.sql` introduces an architectural inconsistency. The PR claims this change aligns with `InsightsRoutingService`, but that service still uses Prisma's `WhereInput` objects. This divergence in data access patterns for similar services increases maintenance overhead.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The change in this PR intentionally shifts InsightsBookingService to Prisma.sql. While InsightsRoutingService still relies on Prisma WhereInput objects, that simply reflects different implementation stages between services and does not create a functional bug or clear maintainability problem. There is no evidence the architectural difference causes issues, so this should not be flagged.</comment>
<file context>
@@ -1,11 +1,14 @@
import type { Team, User, Membership } from "@prisma/client";
+import { Prisma } from "@prisma/client";
import { describe, expect, it } from "vitest";
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="packages/lib/server/service/insightsBooking.ts">
<violation number="1" location="packages/lib/server/service/insightsBooking.ts:29">
Making `teamId` optional here removes the discriminated union guarantees: calls with `scope: "team"` but no `teamId` now type-check, fail `safeParse`, and make `getBaseConditions()` return `NOTHING_CONDITION`, so team-scoped requests silently return zero rows. Please keep the compile-time requirement for `teamId` when `scope` is "team".</violation>
<violation number="2" location="packages/lib/server/service/insightsBooking.ts:68">
The service's abstraction has been broken. By changing the public API from `findMany` to `getBaseConditions`, the service no longer returns data but a raw SQL fragment. This forces consumers to handle query execution and know internal details (like the table name), increasing coupling and making the system harder to maintain.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The change intentionally converts the service into a SQL condition builder to support raw Prisma queries, as reflected by the updated tests. There is no evidence that existing consumers rely on the previous `findMany` method or that the new API causes functional issues. This is a design decision rather than a demonstrably harmful bug, so the reported violation should be discarded.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| } | ||
|
|
||
| async findMany(findManyArgs: Prisma.BookingTimeStatusDenormalizedFindManyArgs) { | ||
| async getBaseConditions(): Promise<Prisma.Sql> { |
There was a problem hiding this comment.
The service's abstraction has been broken. By changing the public API from findMany to getBaseConditions, the service no longer returns data but a raw SQL fragment. This forces consumers to handle query execution and know internal details (like the table name), increasing coupling and making the system harder to maintain.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: The change intentionally converts the service into a SQL condition builder to support raw Prisma queries, as reflected by the updated tests. There is no evidence that existing consumers rely on the previous findMany method or that the new API causes functional issues. This is a design decision rather than a demonstrably harmful bug, so the reported violation should be discarded.
Prompt for AI agents
Address the following comment on packages/lib/server/service/insightsBooking.ts at line 68:
<comment>The service's abstraction has been broken. By changing the public API from `findMany` to `getBaseConditions`, the service no longer returns data but a raw SQL fragment. This forces consumers to handle query execution and know internal details (like the table name), increasing coupling and making the system harder to maintain.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The change intentionally converts the service into a SQL condition builder to support raw Prisma queries, as reflected by the updated tests. There is no evidence that existing consumers rely on the previous `findMany` method or that the new API causes functional issues. This is a design decision rather than a demonstrably harmful bug, so the reported violation should be discarded.</comment>
<file context>
@@ -60,89 +65,86 @@ export class InsightsBookingService {
}
- async findMany(findManyArgs: Prisma.BookingTimeStatusDenormalizedFindManyArgs) {
+ async getBaseConditions(): Promise<Prisma.Sql> {
const authConditions = await this.getAuthorizationConditions();
const filterConditions = await this.getFilterConditions();
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: System Design Agent
| }), | ||
| ]); | ||
|
|
||
| export type InsightsBookingServicePublicOptions = { |
There was a problem hiding this comment.
Making teamId optional here removes the discriminated union guarantees: calls with scope: "team" but no teamId now type-check, fail safeParse, and make getBaseConditions() return NOTHING_CONDITION, so team-scoped requests silently return zero rows. Please keep the compile-time requirement for teamId when scope is "team".
Prompt for AI agents
Address the following comment on packages/lib/server/service/insightsBooking.ts at line 29:
<comment>Making `teamId` optional here removes the discriminated union guarantees: calls with `scope: "team"` but no `teamId` now type-check, fail `safeParse`, and make `getBaseConditions()` return `NOTHING_CONDITION`, so team-scoped requests silently return zero rows. Please keep the compile-time requirement for `teamId` when `scope` is "team".</comment>
<file context>
@@ -26,31 +26,36 @@ export const insightsBookingServiceOptionsSchema = z.discriminatedUnion("scope",
}),
]);
+export type InsightsBookingServicePublicOptions = {
+ scope: "user" | "org" | "team";
+ userId: number;
</file context>
[internal] Confidence score: 8/10
[internal] Posted by: General AI Review Agent
1 participant
Summary by cubic
Converted InsightsBookingService to use Prisma.sql raw queries for authorization and filter conditions, improving control over complex WHERE clauses and aligning with InsightsRoutingService. Updated integration tests to assert SQL fragments and use $queryRaw.
1=0) for “no access” cases.