-
-
Notifications
You must be signed in to change notification settings - Fork 745
Realtime and task run performance improvements #2158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…g for self-hosters)
🦋 Changeset detectedLatest commit: 39e97de The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Caution Review failedThe pull request is closed. WalkthroughThis change set implements a significant architectural refactor and feature expansion for task run querying, filtering, and real-time streaming within the application. The core changes include:
No breaking changes to public APIs are introduced, but the filtering and querying mechanisms are now environment-centric, more modular, and extensible, with improved performance and test coverage. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 16
🧹 Nitpick comments (16)
.claude/settings.local.json (2)
4-6
: Restrict broad shell command permissions
Allowinggrep:*
,rg:*
, andfind:*
without any constraints grants overly permissive shell access. Narrow the scope to only the flags and patterns you actually need, or consolidate into specific targeted entries.
9-9
: Broaden web-fetch domain if needed
WebFetch(domain:clickhouse.com)
may not cover subdomains (e.g.docs.clickhouse.com
). If you intend to fetch from any ClickHouse endpoint, consider a wildcard like*.clickhouse.com
.internal-packages/database/prisma/migrations/20250605220110_remove_unused_completed_at_idnex/migration.sql (1)
1-1
: Add IF EXISTS clause for safer migration.Consider adding
IF EXISTS
to prevent potential errors if the index doesn't exist, maintaining consistency with other migrations in this PR.-DROP INDEX CONCURRENTLY "TaskRun_completedAt_idx"; +DROP INDEX CONCURRENTLY IF EXISTS "TaskRun_completedAt_idx";apps/webapp/app/routes/realtime.v1.runs.ts (1)
12-12
: Ensure valid date format for createdAt filter.
Accepting arbitrary strings forcreatedAt
may lead to runtime errors when parsing downstream. Consider refining the Zod schema to enforce an ISO-8601 format (e.g.,z.string().datetime()
) or transforming to aDate
viaz.coerce.date()
.packages/core/src/v3/apiClient/types.ts (1)
44-45
: Strengthen typing and docs for new query params.
The addedcreatedAt?: string
andskipColumns?: string[]
enable filtering and bandwidth control, but consider:
- Using the
RealtimeRunSkipColumns
union type instead of a plainstring[]
forskipColumns
.- Adding JSDoc to clarify the expected format for
createdAt
(e.g., ISO-8601).apps/webapp/app/models/task.server.ts (1)
6-6
: Update the comment to reflect environment-scoped filtering.The comment still mentions "get all task identifiers for a project" but the function now takes
environmentId
and filters by runtime environment.- * @param prisma An efficient query to get all task identifiers for a project. + * @param prisma An efficient query to get all task identifiers for an environment.apps/webapp/app/services/clickhouseInstance.server.ts (1)
9-13
: Use consistent logging service instead of console.log.The file uses
console.log
while the rest of the codebase likely uses the logger service for consistency.Apply this diff to use consistent logging:
+ import { logger } from "~/services/logger.server";
if (!env.CLICKHOUSE_URL) { - console.log("🗃️ Clickhouse service not enabled"); + logger.info("🗃️ Clickhouse service not enabled"); return; } - console.log("🗃️ Clickhouse service enabled"); + logger.info("🗃️ Clickhouse service enabled");internal-packages/clickhouse/src/client/queryBuilder.ts (1)
41-46
: Improve type safety for the condition parameter.The
condition
parameter should have a more specific type thanany
to maintain type safety.- whereIf(condition: any, clause: string, params?: QueryParams): this { + whereIf(condition: unknown, clause: string, params?: QueryParams): this {apps/webapp/app/presenters/v3/UsagePresenter.server.ts (1)
121-183
: Consider extracting the common transformation logic to reduce duplication.The transformation logic for mapping database results to
TaskUsageItem
objects is duplicated between the ClickHouse path (lines 139-147) and the SQL path (lines 172-180). Both apply the same transformations including addingenv.CENTS_PER_RUN / 100
to the average cost.Extract the transformation into a helper function:
+function transformTaskUsageData(item: any): TaskUsageItem { + return { + taskIdentifier: item.task_identifier || item.taskIdentifier, + runCount: Number(item.run_count || item.runCount), + averageDuration: Number(item.average_duration || item.averageDuration), + averageCost: Number(item.average_cost || item.averageCost) + env.CENTS_PER_RUN / 100, + totalDuration: Number(item.total_duration || item.totalDuration), + totalCost: Number(item.total_cost || item.totalCost) + Number(item.total_base_cost || item.totalBaseCost), + }; +} async function getTaskUsageByOrganization( organizationId: string, startOfMonth: Date, endOfMonth: Date, replica: PrismaClientOrTransaction ) { if (clickhouseClient) { const [queryError, tasks] = await clickhouseClient.taskRuns.getTaskUsageByOrganization({ startTime: startOfMonth.getTime(), endTime: endOfMonth.getTime(), organizationId, }); if (queryError) { throw queryError; } - return tasks - .map((task) => ({ - taskIdentifier: task.task_identifier, - runCount: Number(task.run_count), - averageDuration: Number(task.average_duration), - averageCost: Number(task.average_cost) + env.CENTS_PER_RUN / 100, - totalDuration: Number(task.total_duration), - totalCost: Number(task.total_cost) + Number(task.total_base_cost), - })) - .sort((a, b) => b.totalCost - a.totalCost); + return tasks + .map(transformTaskUsageData) + .sort((a, b) => b.totalCost - a.totalCost); } else { return replica.$queryRaw<TaskUsageItem[]>` SELECT tr."taskIdentifier", COUNT(*) AS "runCount", AVG(tr."usageDurationMs") AS "averageDuration", SUM(tr."usageDurationMs") AS "totalDuration", AVG(tr."costInCents") / 100.0 AS "averageCost", SUM(tr."costInCents") / 100.0 AS "totalCost", SUM(tr."baseCostInCents") / 100.0 AS "totalBaseCost" FROM ${sqlDatabaseSchema}."TaskRun" tr JOIN ${sqlDatabaseSchema}."Project" pr ON pr.id = tr."projectId" JOIN ${sqlDatabaseSchema}."Organization" org ON org.id = pr."organizationId" JOIN ${sqlDatabaseSchema}."RuntimeEnvironment" env ON env."id" = tr."runtimeEnvironmentId" WHERE env.type <> 'DEVELOPMENT' AND tr."createdAt" > ${startOfMonth} AND tr."createdAt" < ${endOfMonth} AND org.id = ${organizationId} GROUP BY tr."taskIdentifier"; `.then((data) => { return data - .map((item) => ({ - taskIdentifier: item.taskIdentifier, - runCount: Number(item.runCount), - averageDuration: Number(item.averageDuration), - averageCost: Number(item.averageCost) + env.CENTS_PER_RUN / 100, - totalDuration: Number(item.totalDuration), - totalCost: Number(item.totalCost) + Number(item.totalBaseCost), - })) + .map(transformTaskUsageData) .sort((a, b) => b.totalCost - a.totalCost); }); } }apps/webapp/test/runsRepository.test.ts (1)
1408-1501
: Add test coverage for missing scenarios.The test suite is comprehensive but missing coverage for:
- Backward pagination direction (only forward is tested)
- The
period
filter option- Negative test cases (e.g., empty results, invalid inputs)
Would you like me to generate additional test cases to cover these scenarios? This would ensure complete coverage of the
RunsRepository
functionality.apps/webapp/app/presenters/v3/RunListPresenter.server.ts (1)
42-64
: Breaking change: Method signature updated to accept environmentIdThe
call
method now requiresenvironmentId
as the first parameter. This is a significant architectural change that aligns with the shift to environment-scoped queries.Ensure all callers of this method have been updated to pass the
environmentId
parameter.apps/webapp/app/services/runsRepository.server.ts (1)
131-135
: Enhance error handling with loggingThe error handling could be improved by utilizing the logger provided in options.
Consider adding error logging:
const [queryError, result] = await queryBuilder.execute(); if (queryError) { + this.options.logger?.error("Failed to query runs from ClickHouse", { + error: queryError, + options + }); throw queryError; }apps/webapp/app/presenters/v3/TaskListPresenter.server.ts (1)
34-98
: Smart performance optimization with deferred loadingThe method correctly implements deferred loading by returning promises without awaiting them. The comment on lines 76-77 clearly explains this design decision.
Consider documenting how consumers should handle potential errors from these deferred promises.
apps/webapp/app/services/realtimeClient.server.ts (1)
189-225
: Smart createdAt filter calculation with proper boundariesThe implementation correctly:
- Enforces a 7-day maximum age limit
- Handles both new filters and cached filters
- Provides sensible defaults
Consider extracting the magic number "24h" to a constant for maintainability.
apps/webapp/app/services/environmentMetricsRepository.server.ts (2)
67-67
: Use lowercase primitive types for consistency.TypeScript convention is to use lowercase primitive types instead of their object wrapper types.
Apply these changes throughout the file:
- count: BigInt; + count: bigint;- duration: Number; + duration: number;Also applies to: 110-110, 152-152, 274-274, 318-318
🧰 Tools
🪛 Biome (1.9.4)
[error] 67-67: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead(lint/complexity/noBannedTypes)
293-296
: Initialize all task run statuses to prevent undefined values.Currently only
COMPLETED_SUCCESSFULLY
is initialized to 0, but other statuses remain undefined. This could cause issues when accessing status counts.Initialize all possible task run statuses:
existingTask.push({ day: day.toISOString(), - ["COMPLETED_SUCCESSFULLY"]: 0, + PENDING: 0, + WAITING_FOR_DEPLOY: 0, + WAITING_TO_RESUME: 0, + QUEUED: 0, + EXECUTING: 0, + COMPLETED_SUCCESSFULLY: 0, + COMPLETED_WITH_ERRORS: 0, + SYSTEM_FAILURE: 0, + CRASHED: 0, + INTERRUPTED: 0, + CANCELED: 0, + EXPIRED: 0, } as { day: string } & Record<TaskRunStatus, number>);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
internal-packages/clickhouse/src/__snapshots__/taskRuns.test.ts.snap
is excluded by!**/*.snap
references/d3-chat/src/components/chat-container.tsx
is excluded by!references/**
references/d3-chat/src/trigger/chat.ts
is excluded by!references/**
references/hello-world/src/trigger/realtime.ts
is excluded by!references/**
📒 Files selected for processing (56)
.claude/settings.local.json
(1 hunks)apps/webapp/app/components/runs/v3/TaskRunsTable.tsx
(1 hunks)apps/webapp/app/env.server.ts
(2 hunks)apps/webapp/app/models/runtimeEnvironment.server.ts
(1 hunks)apps/webapp/app/models/task.server.ts
(2 hunks)apps/webapp/app/presenters/v3/ApiRunListPresenter.server.ts
(4 hunks)apps/webapp/app/presenters/v3/NextRunListPresenter.server.ts
(1 hunks)apps/webapp/app/presenters/v3/RunListPresenter.server.ts
(9 hunks)apps/webapp/app/presenters/v3/TaskListPresenter.server.ts
(2 hunks)apps/webapp/app/presenters/v3/UsagePresenter.server.ts
(2 hunks)apps/webapp/app/presenters/v3/ViewSchedulePresenter.server.ts
(3 hunks)apps/webapp/app/presenters/v3/WaitpointPresenter.server.ts
(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam._index/route.tsx
(2 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.next.runs._index/route.tsx
(2 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs._index/route.tsx
(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.schedules.$scheduleParam/route.tsx
(3 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.waitpoints.tokens.$waitpointParam/route.tsx
(0 hunks)apps/webapp/app/routes/api.v1.schedules.$scheduleId.activate.ts
(1 hunks)apps/webapp/app/routes/api.v1.schedules.$scheduleId.deactivate.ts
(1 hunks)apps/webapp/app/routes/api.v1.schedules.$scheduleId.ts
(1 hunks)apps/webapp/app/routes/realtime.v1.runs.ts
(1 hunks)apps/webapp/app/services/clickhouseInstance.server.ts
(1 hunks)apps/webapp/app/services/environmentMetricsRepository.server.ts
(1 hunks)apps/webapp/app/services/realtimeClient.server.ts
(10 hunks)apps/webapp/app/services/realtimeClientGlobal.server.ts
(1 hunks)apps/webapp/app/services/runsRepository.server.ts
(1 hunks)apps/webapp/test/runsRepository.test.ts
(1 hunks)apps/webapp/test/utils/replicationUtils.ts
(1 hunks)internal-packages/clickhouse/src/client/client.ts
(4 hunks)internal-packages/clickhouse/src/client/noop.ts
(1 hunks)internal-packages/clickhouse/src/client/queryBuilder.ts
(1 hunks)internal-packages/clickhouse/src/client/types.ts
(3 hunks)internal-packages/clickhouse/src/index.ts
(2 hunks)internal-packages/clickhouse/src/taskRuns.test.ts
(2 hunks)internal-packages/clickhouse/src/taskRuns.ts
(2 hunks)internal-packages/database/prisma/migrations/20250605220110_remove_unused_completed_at_idnex/migration.sql
(1 hunks)internal-packages/database/prisma/migrations/20250605220552_remove_unused_schedule_instance_id_index/migration.sql
(1 hunks)internal-packages/database/prisma/migrations/20250606131749_remove_task_run_project_id_index/migration.sql
(1 hunks)internal-packages/database/prisma/migrations/20250606132144_remove_task_run_project_id_task_identifier_index/migration.sql
(1 hunks)internal-packages/database/prisma/migrations/20250606132316_remove_task_run_project_id_status_index/migration.sql
(1 hunks)internal-packages/database/prisma/migrations/20250606132630_remove_task_run_project_id_task_identifier_status_index/migration.sql
(1 hunks)internal-packages/database/prisma/migrations/20250606132928_remove_project_id_created_at_task_identifier_index/migration.sql
(1 hunks)internal-packages/database/prisma/migrations/20250606133133_remove_task_run_project_id_id_desc_index/migration.sql
(1 hunks)internal-packages/database/prisma/migrations/20250609163214_add_task_run_environment_created_at_id_index/migration.sql
(1 hunks)internal-packages/database/prisma/migrations/20250609164201_add_task_run_run_tags_gin_index/migration.sql
(1 hunks)internal-packages/database/prisma/schema.prisma
(1 hunks)packages/core/src/v3/apiClient/index.ts
(5 hunks)packages/core/src/v3/apiClient/runStream.ts
(3 hunks)packages/core/src/v3/apiClient/stream.ts
(1 hunks)packages/core/src/v3/apiClient/types.ts
(1 hunks)packages/core/src/v3/isomorphic/duration.ts
(1 hunks)packages/core/src/v3/schemas/api.ts
(1 hunks)packages/core/test/duration.test.ts
(1 hunks)packages/react-hooks/src/hooks/useRealtime.ts
(12 hunks)packages/react-hooks/src/hooks/useTaskTrigger.ts
(2 hunks)packages/trigger-sdk/src/v3/runs.ts
(4 hunks)
💤 Files with no reviewable changes (1)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.waitpoints.tokens.$waitpointParam/route.tsx
🧰 Additional context used
🧬 Code Graph Analysis (14)
packages/react-hooks/src/hooks/useTaskTrigger.ts (1)
packages/core/src/v3/apiClient/runStream.ts (1)
RealtimeRunSkipColumns
(58-77)
internal-packages/clickhouse/src/client/noop.ts (2)
internal-packages/clickhouse/src/client/types.ts (3)
ClickhouseReader
(20-75)ClickhouseWriter
(85-94)ClickhouseQueryBuilderFunction
(16-18)internal-packages/clickhouse/src/client/queryBuilder.ts (1)
ClickhouseQueryBuilder
(7-93)
internal-packages/clickhouse/src/client/client.ts (2)
internal-packages/clickhouse/src/client/types.ts (1)
ClickhouseQueryBuilderFunction
(16-18)internal-packages/clickhouse/src/client/queryBuilder.ts (1)
ClickhouseQueryBuilder
(7-93)
internal-packages/clickhouse/src/index.ts (1)
internal-packages/clickhouse/src/taskRuns.ts (5)
getTaskRunsQueryBuilder
(97-104)getTaskActivityQueryBuilder
(120-147)getCurrentRunningStats
(162-186)getAverageDurations
(200-222)getTaskUsageByOrganization
(240-268)
internal-packages/clickhouse/src/taskRuns.test.ts (3)
internal-packages/testcontainers/src/index.ts (1)
clickhouseTest
(219-223)internal-packages/clickhouse/src/client/client.ts (2)
insert
(231-310)queryBuilder
(218-229)internal-packages/clickhouse/src/taskRuns.ts (2)
insertTaskRuns
(51-65)getTaskRunsQueryBuilder
(97-104)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam._index/route.tsx (2)
apps/webapp/app/presenters/v3/TaskListPresenter.server.ts (1)
taskListPresenter
(100-100)apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts (1)
environment
(2169-2192)
apps/webapp/app/services/realtimeClientGlobal.server.ts (2)
apps/webapp/app/env.server.ts (1)
env
(762-762)apps/webapp/app/services/realtimeClient.server.ts (1)
RealtimeClient
(69-526)
internal-packages/clickhouse/src/client/types.ts (1)
internal-packages/clickhouse/src/client/queryBuilder.ts (1)
ClickhouseQueryBuilder
(7-93)
apps/webapp/test/runsRepository.test.ts (3)
internal-packages/testcontainers/src/index.ts (2)
containerTest
(233-241)postgresContainer
(78-89)apps/webapp/test/utils/replicationUtils.ts (1)
setupClickhouseReplication
(7-54)apps/webapp/app/services/runsRepository.server.ts (1)
RunsRepository
(40-226)
apps/webapp/app/presenters/v3/UsagePresenter.server.ts (3)
internal-packages/clickhouse/src/taskRuns.ts (1)
getTaskUsageByOrganization
(240-268)apps/webapp/app/services/clickhouseInstance.server.ts (1)
clickhouseClient
(5-5)apps/webapp/app/db.server.ts (1)
sqlDatabaseSchema
(252-252)
packages/core/test/duration.test.ts (1)
packages/core/src/v3/isomorphic/duration.ts (5)
parseNaturalLanguageDuration
(1-63)safeParseNaturalLanguageDuration
(65-71)parseNaturalLanguageDurationAgo
(75-137)safeParseNaturalLanguageDurationAgo
(139-145)stringifyDuration
(147-167)
apps/webapp/app/presenters/v3/TaskListPresenter.server.ts (4)
apps/webapp/app/services/environmentMetricsRepository.server.ts (6)
DailyTaskActivity
(7-7)EnvironmentMetricsRepository
(11-29)CurrentRunningStats
(8-8)AverageDurations
(9-9)ClickHouseEnvironmentMetricsRepository
(176-268)PostgrestEnvironmentMetricsRepository
(38-170)apps/webapp/app/v3/models/workerDeployment.server.ts (1)
findCurrentWorkerFromEnvironment
(197-223)apps/webapp/app/services/clickhouseInstance.server.ts (1)
clickhouseClient
(5-5)apps/webapp/app/db.server.ts (1)
$replica
(102-105)
packages/react-hooks/src/hooks/useRealtime.ts (3)
packages/core/src/v3/apiClient/runStream.ts (2)
RealtimeRunSkipColumns
(58-77)RealtimeRun
(55-55)packages/core/src/v3/apiClient/index.ts (3)
RealtimeRunSkipColumns
(92-92)ApiClient
(138-1058)RealtimeRun
(126-126)packages/core/src/v3/types/tasks.ts (2)
AnyTask
(628-628)InferRunTypes
(904-914)
internal-packages/clickhouse/src/taskRuns.ts (1)
internal-packages/clickhouse/src/client/types.ts (1)
ClickhouseReader
(20-75)
🪛 Biome (1.9.4)
apps/webapp/app/presenters/v3/NextRunListPresenter.server.ts
[error] 166-166: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/webapp/app/services/environmentMetricsRepository.server.ts
[error] 67-67: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead
(lint/complexity/noBannedTypes)
[error] 110-110: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead
(lint/complexity/noBannedTypes)
[error] 152-152: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
[error] 274-274: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead
(lint/complexity/noBannedTypes)
[error] 318-318: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (90)
internal-packages/database/prisma/migrations/20250606131749_remove_task_run_project_id_index/migration.sql (1)
2-2
: Verify replacement index exists before removal
Before dropping"TaskRun_projectId_idx"
, ensure that the new environment-scoped index onruntimeEnvironmentId
(and any composite ordering/index oncreatedAt
orid
) has been created in an earlier migration. This prevents query slowdowns once the project‐based index is removed.internal-packages/database/prisma/migrations/20250606133133_remove_task_run_project_id_id_desc_index/migration.sql (1)
2-2
: Confirm prior creation of new environment-based index
Dropping"TaskRun_projectId_id_idx"
concurrently is safe, but double-check that the intended replacement index on(runtimeEnvironmentId, createdAt DESC, id DESC)
is already in place.internal-packages/database/prisma/migrations/20250606132144_remove_task_run_project_id_task_identifier_index/migration.sql (1)
2-2
: Ensure new lookup index covers taskIdentifier
Before removing"TaskRun_projectId_taskIdentifier_idx"
, verify that any new environment-scoped index supports efficient filtering bytaskIdentifier
(if still needed), or that queries have been updated accordingly.internal-packages/database/prisma/migrations/20250605220552_remove_unused_schedule_instance_id_index/migration.sql (1)
2-2
: Validate scheduling index replacement
The removal of"TaskRun_scheduleInstanceId_idx"
should follow the creation of any required environment-scoped schedule index (e.g., onruntimeEnvironmentId
plusscheduleInstanceId
if still used). Confirm that query paths remain covered.internal-packages/database/prisma/migrations/20250606132316_remove_task_run_project_id_status_index/migration.sql (1)
2-2
: Check that status filtering is supported post-drop
Dropping"TaskRun_projectId_status_idx"
is correct for the new environment approach, but ensure that status-based queries now leverage a replacement environment-scoped index or an alternative strategy.internal-packages/database/prisma/migrations/20250606132630_remove_task_run_project_id_task_identifier_status_index/migration.sql (1)
1-2
: LGTM! Safe migration approach.The migration correctly uses
CONCURRENTLY
to avoid blocking operations andIF EXISTS
to handle cases where the index might not exist. This aligns well with the broader refactoring from project-based to environment-based indexing.apps/webapp/app/routes/api.v1.schedules.$scheduleId.activate.ts (1)
62-62
: LGTM! Consistent environment scoping.The addition of
environmentId
parameter correctly uses the authenticated environment context and aligns with the broader refactoring to environment-based operations.apps/webapp/app/routes/api.v1.schedules.$scheduleId.deactivate.ts (1)
61-61
: LGTM! Consistent with activate route.The environment ID parameter addition follows the same correct pattern as the activate route, maintaining consistency across schedule operations.
internal-packages/database/prisma/migrations/20250606132928_remove_project_id_created_at_task_identifier_index/migration.sql (1)
1-2
: LGTM! Safe index removal.The migration correctly uses
CONCURRENTLY
to perform non-blocking index removal andIF EXISTS
to handle cases where the index may not exist. This aligns with the architectural shift to environment-scoped filtering.apps/webapp/app/routes/api.v1.schedules.$scheduleId.ts (1)
143-143
: LGTM! Consistent with environment-scoped refactoring.The addition of
environmentId
parameter correctly passes the authenticated environment's ID to the presenter, aligning with the broader architectural shift to environment-scoped filtering.internal-packages/database/prisma/migrations/20250609163214_add_task_run_environment_created_at_id_index/migration.sql (1)
1-2
: LGTM! Well-designed composite index for environment-scoped queries.The index effectively supports environment-scoped task run queries with proper descending order on
createdAt
andid
for efficient pagination. The use ofCONCURRENTLY
andIF NOT EXISTS
follows best practices for safe index creation.apps/webapp/app/components/runs/v3/TaskRunsTable.tsx (1)
542-542
: Removeenvironments
destructuring in BlankState.
Updating the destructuring to only extracttasks
,from
,to
, andotherFilters
aligns this component with the shift to a single-environment context.packages/react-hooks/src/hooks/useTaskTrigger.ts (2)
12-12
: ImportRealtimeRunSkipColumns
for skipColumns support.
The addition of theRealtimeRunSkipColumns
import correctly prepares the hook to expose the newskipColumns
option.
117-124
: Add optionalskipColumns
to task trigger options.
IntroducingskipColumns?: RealtimeRunSkipColumns
inUseRealtimeTaskTriggerOptions
properly extends the hook’s configurability for real-time subscriptions.internal-packages/database/prisma/migrations/20250609164201_add_task_run_run_tags_gin_index/migration.sql (1)
1-1
: Add GIN index onrunTags
.
Creating the concurrent GIN index withIF NOT EXISTS
onTaskRun.runTags
improves tag-based query performance without blocking writes.apps/webapp/app/models/task.server.ts (1)
10-10
: LGTM! Parameter and query changes are consistent.The function signature and SQL query have been properly updated together to support environment-scoped filtering instead of project-scoped filtering.
Also applies to: 19-19
packages/core/src/v3/apiClient/stream.ts (2)
105-109
: LGTM! Clean refactoring to more concise subscription call.The subscription logic remains the same while being more readable and concise.
114-114
: LGTM! Minor formatting improvement.apps/webapp/app/services/realtimeClientGlobal.server.ts (2)
7-7
: LGTM! Clean implementation of electric origin sharding support.The logic correctly checks for
ELECTRIC_ORIGIN_SHARDS
, splits by comma to create an array, and falls back to the singleELECTRIC_ORIGIN
when sharding is not configured.
10-10
: LGTM! Simplified variable assignment.Removing the redundant variable assignment improves code clarity.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam._index/route.tsx (2)
78-78
: LGTM! Added import for singleton presenter.
127-130
: LGTM! Proper migration to singleton pattern and environment-scoped parameters.The changes correctly:
- Replace direct instantiation with the singleton
taskListPresenter
- Switch from
projectId
toenvironmentType
parameter, aligning with the environment-scoped architectureThis improves performance by reusing the singleton instance and maintains consistency with the broader codebase refactoring.
apps/webapp/app/presenters/v3/WaitpointPresenter.server.ts (1)
81-85
: LGTM! Consistent with environment-scoped refactoring.The change correctly passes
environmentId
as the first argument torunPresenter.call
, aligning with the broader refactoring to shift from project-scoped to environment-scoped filtering. The parameter passing is consistent and theenvironmentId
is properly available from the method signature.apps/webapp/app/presenters/v3/ViewSchedulePresenter.server.ts (3)
11-11
: LGTM! Type definition updated correctly.The
ViewScheduleOptions
type is properly extended with the requiredenvironmentId
property, consistent with the environment-scoped refactoring.
21-21
: LGTM! Method signature updated consistently.The method signature correctly includes the new
environmentId
parameter, maintaining consistency with the updated type definition.
80-84
: LGTM! Presenter call updated correctly.The
runPresenter.call
is properly updated to passenvironmentId
as the first argument, aligning with the new environment-scoped filtering approach. The parameter destructuring and passing is correctly implemented.internal-packages/clickhouse/src/client/noop.ts (2)
3-3
: LGTM! Imports added correctly.The necessary imports for
ClickhouseQueryBuilderFunction
andClickhouseQueryBuilder
are properly added to support the newqueryBuilder
method.Also applies to: 7-7
14-22
: LGTM! QueryBuilder method implemented correctly.The
queryBuilder
method properly implements theClickhouseReader
interface. The implementation:
- Uses the correct method signature matching the interface
- Returns a function that creates a new
ClickhouseQueryBuilder
instance- Passes all required parameters (
name
,baseQuery
,this
,schema
,settings
) to the constructor- Provides appropriate no-op functionality for testing scenarios
This aligns well with the broader ClickHouse query builder ecosystem introduced in this PR.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.next.runs._index/route.tsx (2)
37-37
: LGTM! Imports updated correctly.The necessary imports for
$replica
,NextRunListPresenter
, andclickhouseClient
are properly added to support the new ClickHouse-backed run listing functionality.Also applies to: 43-44
130-131
: LGTM! Presenter instantiation and call updated correctly.The
NextRunListPresenter
is properly instantiated with both$replica
andclickhouseClient
parameters, and thecall
method correctly passesenvironment.id
as the first argument, consistent with the environment-scoped refactoring pattern.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.schedules.$scheduleParam/route.tsx (2)
48-48
: LGTM: Import addition aligns with environment-scoped architecture.The import of
findEnvironmentBySlug
is correctly added to support the new environment lookup functionality.
63-64
: LGTM: Parameter parsing correctly updated for environment scoping.The addition of
envParam
parsing aligns with the shift to environment-scoped data access.apps/webapp/app/models/runtimeEnvironment.server.ts (1)
279-310
: LGTM: Well-implemented environment retrieval function.The
findDisplayableEnvironment
function follows established patterns in the codebase:
- Proper database query with limited selection for performance
- Consistent return type (undefined instead of null)
- Reuses existing
displayableEnvironment
logic for consistency- Clear separation of concerns
This supports the broader architectural shift to environment-scoped data access.
apps/webapp/app/services/clickhouseInstance.server.ts (1)
5-30
: LGTM: Well-structured ClickHouse client singleton.The implementation demonstrates good practices:
- Proper singleton pattern using the utility function
- Conditional initialization based on environment configuration
- Comprehensive ClickHouse client configuration with all necessary parameters
- Clean separation of initialization logic
This establishes a solid foundation for ClickHouse integration across the application.
internal-packages/clickhouse/src/taskRuns.test.ts (2)
4-4
: LGTM: Import correctly added for new query builder functionality.The import of
getTaskRunsQueryBuilder
supports the new test case and aligns with the enhanced ClickHouse query capabilities.
254-346
: LGTM: Comprehensive test coverage for query builder functionality.The test case demonstrates excellent testing practices:
- Tests both query building (
build()
) and execution (execute()
)- Uses snapshot testing to validate generated query structure
- Covers conditional query building with
whereIf
- Tests both positive cases (finding results) and negative cases (empty results)
- Proper test data setup with realistic values
This provides solid coverage for the new ClickHouse query builder infrastructure.
internal-packages/clickhouse/src/index.ts (2)
5-13
: LGTM! Clean integration of query builder imports.The new imports properly extend the ClickHouse API with query builder functionality while maintaining consistency with existing patterns.
146-150
: Excellent exposure of query builder methods.The new query builder functions are properly exposed through the
taskRuns
getter, maintaining consistency with existing patterns by usingthis.reader
for all read operations.internal-packages/clickhouse/src/client/types.ts (2)
16-18
: Well-defined type for query builder function.The
ClickhouseQueryBuilderFunction
type properly encapsulates the optional settings parameter pattern and maintains type safety with the genericTOutput
parameter.
50-72
: Excellent interface extension for query builder functionality.The
queryBuilder
method signature is well-designed with:
- Clear documentation following established patterns
- Proper generic typing with Zod schema constraints
- Consistent parameter structure with the existing
query
method- Good example in the
baseQuery
documentationapps/webapp/test/utils/replicationUtils.ts (2)
45-49
: Proper cleanup pattern with afterEach hook.The cleanup implementation correctly stops the replication service after each test, preventing resource leaks and ensuring test isolation.
20-26
: Appropriate ClickHouse configuration for testing.The ClickHouse client setup with compression enabled and a descriptive service name is well-suited for test environments.
internal-packages/clickhouse/src/client/client.ts (2)
107-113
: Useful debug logging enhancement.The added debug logging provides comprehensive information for troubleshooting, including normalized query strings, parameters, and settings. This will be valuable for debugging ClickHouse query issues.
218-229
: Solid implementation of query builder method.The
queryBuilder
method implementation is well-designed:
- Proper settings merging with spread operator precedence
- Consistent parameter structure matching the interface
- Type safety maintained through generic constraints
- Clean factory function pattern returning configured builder instances
apps/webapp/app/presenters/v3/ApiRunListPresenter.server.ts (3)
158-161
: LGTM! Proper validation ensures environment exists.The validation logic correctly throws a
ServiceValidationError
when no environment is found, preventing downstream issues with undefined environment IDs.
138-161
:❓ Verification inconclusive
Environment filtering logic change may introduce breaking behavior.
The refactor correctly implements environment-scoped filtering, but when multiple environments are specified in
filter[env]
, only the first one is used (environments.at(0)?.id
). This could be a breaking change if the API previously supported filtering across multiple environments.Verify if this breaking change is intentional by checking how multiple environment filters were previously handled:
🏁 Script executed:
#!/bin/bash # Search for previous implementations of environment filtering in run list presenters rg -A 10 -B 5 "filter.*env.*environments" --type tsLength of output: 53
Ensure multi-environment filtering support is intentional
The refactored logic in
ApiRunListPresenter.server.ts
(lines 138–161) now only usesenvironments.at(0)?.id
when multiple values are passed infilter[env]
, dropping support for filtering across multiple environments. I couldn’t find any prior implementation of multi-environment filtering in the codebase—please confirm whether this narrowing to a single environment is an intentional breaking change or if we need to restore full multi-environment support.• File:
apps/webapp/app/presenters/v3/ApiRunListPresenter.server.ts
(lines 138–161)
212-212
:✅ Verification successful
Verify the RunListPresenter interface change.
The call signature change to pass
environmentId
as the first parameter aligns with the described refactor, but ensure all callers ofRunListPresenter.call
have been updated consistently.
🏁 Script executed:
#!/bin/bash # Search for other usages of RunListPresenter.call to verify consistent updates ast-grep --pattern 'RunListPresenter.call($$$)'Length of output: 49
I’ll search broadly for any
*.call
invocations on presenters to ensure nothing was missed:
🏁 Script executed:
#!/bin/bash # 1. Look for direct RunListPresenter.call usages rg -n "RunListPresenter\.call" -t ts || true # 2. Find all .call invocations on presenter instances rg -n "presenter\.call" -t ts || trueLength of output: 6774
I’ll verify where
ApiRunListPresenter
is referenced and how its.call
is invoked:
🏁 Script executed:
#!/bin/bash # Find all references to ApiRunListPresenter and show surrounding context rg -n "ApiRunListPresenter" -C3 -t tsLength of output: 4069
I’ll locate all definitions and usages of
RunListPresenter
to confirm its.call
signature and callers:
🏁 Script executed:
#!/bin/bash # 1. Find any files named RunListPresenter.* fd RunListPresenter -e ts || true # 2. Show the class definition(s) for RunListPresenter rg -n "class RunListPresenter" -C3 -t ts || true # 3. Find all instantiations of RunListPresenter rg -n "new RunListPresenter" -C3 -t ts || true # 4. Verify all .call invocations on those instances rg -n "RunListPresenter\.call" -C2 -t ts || true rg -n "presenter\.call" -C2 -t ts | grep -B1 "new RunListPresenter" || trueLength of output: 4362
All RunListPresenter.call usages now pass environmentId first
I’ve verified every instantiation of RunListPresenter and its .call(…) invocations; all four call sites have been updated to the new signature:
• apps/webapp/app/presenters/v3/ApiRunListPresenter.server.ts (line 212)
• apps/webapp/app/presenters/v3/WaitpointPresenter.server.ts (line 81)
• apps/webapp/app/presenters/v3/ViewSchedulePresenter.server.ts (line 80)
• apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs._index/route.tsx (line 124)No remaining callers of RunListPresenter.call were found—this change is complete.
packages/core/src/v3/apiClient/runStream.ts (2)
58-77
: Well-defined type for selective column skipping.The
RealtimeRunSkipColumns
type provides a clear enumeration of columns that can be skipped in real-time subscriptions, enabling performance optimizations for different use cases.
423-438
: Improved data transformation with consistent defaults.The nullish coalescing operators (
??
) ensure that numeric fields always have defined values (0) and boolean fields have sensible defaults (false
forisTest
). This prevents undefined values from breaking downstream consumers of the real-time stream data.packages/core/src/v3/schemas/api.ts (1)
873-888
: Schema improvements align with real-time stream enhancements.The reordering of fields and addition of default values ensures consistent data shapes when columns may be skipped in real-time streams. The defaults (
0
for numeric fields,false
forisTest
) match those used in thetransformRunShape
method, maintaining consistency across the data pipeline.internal-packages/database/prisma/schema.prisma (1)
684-685
: Excellent index optimization for environment-scoped queries.These new indexes strategically support the transition from project-based to environment-based filtering:
Line 684: The composite index
[runtimeEnvironmentId, createdAt(sort: Desc), id(sort: Desc)]
optimizes environment-scoped queries with time-based ordering and provides consistent pagination through the ID tiebreaker.Line 685: The GIN index
[runTags(ops: ArrayOps)]
enables efficient tag-based filtering on therunTags
array field, which aligns with the new filtering capabilities being added across the codebase.These indexes will significantly improve query performance for the new environment-centric data access patterns.
packages/trigger-sdk/src/v3/runs.ts (5)
18-18
: Good addition of the RealtimeRunSkipColumns import.This import supports the new column skipping functionality being added to real-time subscriptions.
350-361
: Well-designed skipColumns option with clear documentation.The addition of the
skipColumns
property toSubscribeToRunOptions
is implemented well:
- Type safety: Uses the imported
RealtimeRunSkipColumns
type- Optional parameter: Maintains backward compatibility
- Clear documentation: Includes usage example and default value
- Performance benefit: Allows clients to exclude unnecessary columns from real-time streams
This will help optimize real-time subscription performance by reducing payload size.
405-405
: Correct forwarding of skipColumns option.The
skipColumns
option is properly passed through to the API client, maintaining the chain of configuration from the SDK interface to the underlying implementation.
409-433
: Excellent design of the SubscribeToRunsFilterOptions interface.The new filtering options are well-structured:
- Duration-based filtering: The
createdAt
property uses intuitive duration strings like "1h", "30m"- Clear constraints: Maximum 1 week duration is documented
- Server-side calculation: Timestamp calculation on server prevents client-server time sync issues
- Consistent API: Uses the same
skipColumns
type as other subscription methodsThe documentation clearly explains the expected format and behavior.
466-475
: Clean API evolution with backward compatibility.The updated
subscribeToRunsWithTag
function signature maintains backward compatibility while adding powerful new filtering capabilities:
- Optional parameters: Both
filters
andoptions
are optional- Proper option forwarding: Signal handling is correctly preserved
- Type safety: Generic type parameter is properly propagated
The implementation correctly forwards all options to the underlying API client.
apps/webapp/app/env.server.ts (2)
38-40
: Well-documented addition for electric origin sharding.The
ELECTRIC_ORIGIN_SHARDS
environment variable is properly implemented:
- Clear documentation: Comments explain the purpose and provide a concrete example
- Optional configuration: Uses
.optional()
allowing for single-origin setups- Comma-separated format: Standard approach for multiple values in environment variables
This supports the multi-origin electric endpoint functionality for improved scalability and load distribution.
744-750
: Comprehensive Clickhouse configuration options.The Clickhouse environment variables provide thorough configuration control:
- Connection management: URL, keep-alive settings, and connection pooling
- Performance tuning: Max connections and compression settings
- Observability: Configurable log levels for debugging
- Sensible defaults: All variables have appropriate default values
- Consistent patterns: Follows the same naming and typing conventions as other database configurations
These variables properly support the new Clickhouse integration capabilities mentioned in the summary.
packages/react-hooks/src/hooks/useRealtime.ts (5)
3-9
: LGTM!The addition of
RealtimeRunSkipColumns
to the imports is appropriate for the new column skipping functionality.
39-44
: Well-documented optional property addition.The
skipColumns
option is properly typed and documented with a sensible default value.
352-376
: Excellent type definition with comprehensive documentation.The
UseRealtimeRunsWithTagOptions
type is well-structured with clear examples for thecreatedAt
duration format and proper documentation about server-side timestamp calculation.
115-117
: Consistent propagation of filtering options.The
skipColumns
andcreatedAt
filters are properly propagated through all the processing functions using a clean filters object pattern.Also applies to: 276-278, 440-442, 613-622, 656-672, 717-730
392-393
: Helpful documentation update.The example clearly demonstrates how to use the new
createdAt
filter option.apps/webapp/app/presenters/v3/NextRunListPresenter.server.ts (3)
165-180
: Code is already null-safe.The schedule ID lookup logic correctly handles the case where no schedule is found, using optional chaining on line 178.
🧰 Tools
🪛 Biome (1.9.4)
[error] 166-166: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
114-118
: Efficient concurrent data fetching.Good use of
Promise.all
to fetch independent data concurrently, improving performance.
229-293
: Well-structured response mapping.The return object properly handles optional fields, sorts collections for consistency, and provides a clear pagination structure.
internal-packages/clickhouse/src/client/queryBuilder.ts (1)
7-32
: Well-designed query builder with fluent interface.The class follows good design patterns with proper encapsulation, type safety through generics, and Zod schema validation.
apps/webapp/app/presenters/v3/UsagePresenter.server.ts (1)
143-143
: Verify the average cost calculation to avoid potential double counting.The code adds
env.CENTS_PER_RUN / 100
to the average cost after retrieving it from the database. This could lead to double counting if the database already includes the per-run cost in thecost_in_cents
calculation.Please verify whether the
cost_in_cents
field in both ClickHouse and PostgreSQL already includes the base per-run cost. If it does, this addition would result in incorrect cost calculations.Also applies to: 176-176
packages/core/src/v3/isomorphic/duration.ts (1)
3-14
: Well-implemented flexible duration parsing!The refactoring to support duration units in any order (e.g., "1h30m" or "30m1h") is a great improvement. The validation pattern ensures the entire string consists only of valid duration units, preventing partial matches.
apps/webapp/test/runsRepository.test.ts (1)
8-1501
: Excellent test coverage and structure!The test suite provides comprehensive coverage of the
RunsRepository
filtering capabilities with well-organized, focused tests. The use ofcontainerTest
for integration testing with real ClickHouse and PostgreSQL instances ensures the tests accurately reflect production behavior.packages/core/src/v3/apiClient/index.ts (5)
69-69
: LGTM!The import and re-export of
RealtimeRunSkipColumns
type are correctly implemented.Also applies to: 92-92
895-914
: LGTM!The
skipColumns
implementation insubscribeToRun
is well-structured and maintains backward compatibility.
919-925
: LGTM!The
skipColumns
support insubscribeToRunsWithTag
is consistent with the other subscription methods.
942-963
: LGTM!The
skipColumns
implementation insubscribeToBatch
follows the established pattern correctly.
1075-1081
: LGTM!The query parameter handling for
createdAt
andskipColumns
is implemented correctly.apps/webapp/app/presenters/v3/RunListPresenter.server.ts (2)
107-115
: LGTM!The environment validation logic properly ensures the environment exists and the user has access before proceeding.
218-218
: Good catch on field name correctionUsing
taskVersion
instead of the deprecatedversion
field name is the correct approach.apps/webapp/app/services/runsRepository.server.ts (3)
44-112
: Excellent query builder implementationThe query construction is well-structured with proper parameter binding and type safety. Good use of ClickHouse-specific functions like
hasAny
for array operations andfromUnixTimestamp64Milli
for timestamp conversions.
179-224
: LGTM!The two-phase query approach (ClickHouse for filtering/pagination, Prisma for detailed data) is an excellent architectural choice for performance.
145-176
:❌ Incorrect review comment
Review backward pagination logic
The backward pagination logic appears to have issues:
- When direction is "backward", we query for IDs > cursor but still order by DESC, which may not give the expected results
- The cursor calculations for backward pagination (lines 156-161) seem incorrect
Consider this corrected approach for backward pagination:
} else { queryBuilder .where("run_id > {runId: String}", { runId: options.page.cursor }) - .orderBy("run_id DESC") + .orderBy("run_id ASC") .limit(options.page.size + 1); }Then reverse the results for backward pagination before processing cursors.
🏁 Script executed:
#!/bin/bash # Search for similar pagination patterns in the codebase to verify the approach rg -A 10 -B 5 "direction.*backward.*cursor" --type tsLength of output: 55
🏁 Script executed:
#!/bin/bash # Extract the pagination query builder and cursor logic from the file sed -n '100,200p' apps/webapp/app/services/runsRepository.server.ts # Locate where queryBuilder is configured for cursor-based pagination rg -n "queryBuilder" -C5 apps/webapp/app/services/runsRepository.server.tsLength of output: 6741
Ignore backward pagination concern
The existing code correctly uses a consistent DESC order for both “forward” and “backward” queries. By querying
run_id > cursor
and then.orderBy("run_id DESC")
, you still get the newest‐first slice, so there’s no need to switch to ASC and reverse the array. ThenextCursor
/previousCursor
calculations align with this approach and will produce the intended pages.No changes required.
Likely an incorrect or invalid review comment.
apps/webapp/app/presenters/v3/TaskListPresenter.server.ts (3)
1-27
: Well-structured imports and type definitionsThe imports are properly organized and the type alias for
TaskActivity
improves code readability.
28-33
: Excellent use of dependency injection patternThe constructor properly implements dependency injection, making the class more testable and maintainable.
100-112
: Clean singleton implementation with smart fallbackThe factory function properly handles the choice between ClickHouse and PostgreSQL implementations, with a sensible fallback to PostgreSQL when ClickHouse is unavailable.
packages/core/test/duration.test.ts (2)
9-126
: Comprehensive test coverage for parseNaturalLanguageDurationExcellent test coverage including:
- Valid duration strings with all supported units
- Edge cases (zero values, large numbers)
- Invalid inputs (empty strings, negative numbers, invalid units)
- Proper time mocking setup
284-374
: Excellent consistency and formatting testsThe tests properly verify:
stringifyDuration
formatting with various inputs- Symmetry between forward and "ago" parsers
- Consistency between safe and unsafe versions
- Proper handling of invalid inputs across all functions
apps/webapp/app/services/realtimeClient.server.ts (4)
18-105
: Well-designed caching infrastructure with security considerationsGood implementation details:
- Security-conscious reserved columns prevent skipping critical fields
- Reasonable 7-day maximum age limit for createdAt filters
- Dual-store cache (memory + Redis) provides resilience
- Appropriate cache TTLs (1 week fresh, 2 weeks stale)
149-187
: Robust implementation of createdAt filteringThe implementation properly:
- Calculates and applies createdAt filters
- Handles cache storage after successful responses
- Uses non-blocking error handling to prevent request failures
- Logs errors for monitoring
301-321
: Secure column filtering implementationExcellent security considerations:
- Filters out reserved columns to prevent skipping critical fields
- Properly quotes column names to prevent SQL injection
- Maintains backward compatibility
480-488
:✅ Verification successful
Verify jumpHash implementation for consistent origin routing
The multi-origin support looks good with consistent hashing. However, please verify that the
jumpHash
function is properly implemented for distributed load balancing.
🏁 Script executed:
#!/bin/bash # Description: Verify the jumpHash implementation exists and is properly exported # Search for jumpHash implementation ast-grep --pattern 'export function jumpHash($$$) { $$$ }' # Also check for any existing tests fd -e test.ts -e spec.ts | xargs rg -l "jumpHash" -A 5Length of output: 153
🏁 Script executed:
#!/bin/bash # Locate jumpHash definition and inspect its implementation # Search for the function declaration rg -n "function jumpHash" -A5 -B5 # Also check for any named exports if not directly exported rg -n "export.*jumpHash" -A5 -B5Length of output: 2336
jumpHash implementation and export verified
- Defined in
packages/core/src/v3/serverOnly/jumpHash.ts
and re-exported viapackages/core/src/v3/serverOnly/index.ts
.- Behavior covered by existing tests in
packages/core/test/jumpHash.test.ts
.No further action required.
internal-packages/clickhouse/src/taskRuns.ts (1)
240-268
: Well-structured usage aggregation query.The implementation correctly:
- Filters out development environments
- Converts between Unix timestamps and ClickHouse timestamps
- Properly converts cents to dollars for cost calculations
- Orders by total cost for meaningful results
...outes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs._index/route.tsx
Show resolved
Hide resolved
.../_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.next.runs._index/route.tsx
Show resolved
Hide resolved
...gs.$organizationSlug.projects.$projectParam.env.$envParam.schedules.$scheduleParam/route.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
apps/webapp/app/services/realtimeClient.server.ts (1)
340-340
:⚠️ Potential issueInconsistency in electric origin usage.
The
#constructChunksElectricUrl
method still uses the old single-origin approach (this.options.electricOrigin
) while#constructRunsElectricUrl
uses the new multi-origin resolution. This could lead to inconsistent behavior.Apply this fix to ensure consistent multi-origin support:
- const electricUrl = new URL(`${this.options.electricOrigin}/v1/shape`); + const electricOrigin = this.#resolveElectricOrigin(environment.id); + const electricUrl = new URL(`${electricOrigin}/v1/shape`);And update the method signature to accept the environment parameter:
- #constructChunksElectricUrl(url: URL | string, whereClause: string, clientVersion?: string): URL { + #constructChunksElectricUrl(url: URL | string, environment: RealtimeEnvironment, whereClause: string, clientVersion?: string): URL {Then update the call site:
- const electricUrl = this.#constructChunksElectricUrl(url, whereClause, clientVersion); + const electricUrl = this.#constructChunksElectricUrl(url, environment, whereClause, clientVersion);
♻️ Duplicate comments (1)
apps/webapp/test/runsRepository.test.ts (1)
64-64
: Replace fixed delays with a more robust replication waiting mechanism.This issue was already identified in previous reviews. All tests use fixed 1-second delays to wait for ClickHouse replication, which can lead to flaky tests.
Also applies to: 171-171, 281-281, 391-391, 501-501, 610-610, 703-703, 807-807, 936-936, 1043-1043, 1150-1150, 1264-1264, 1384-1384, 1467-1467
🧹 Nitpick comments (1)
apps/webapp/test/runsRepository.test.ts (1)
1-1502
: Consider using constants for repeated values.The tests contain many magic values that are repeated throughout (e.g., "V2", "DEVELOPMENT", "test", "my-task"). Extracting these to constants would improve maintainability.
+const TEST_VALUES = { + ENGINE: "V2" as const, + ENVIRONMENT_TYPE: "DEVELOPMENT" as const, + QUEUE: "test" as const, + TASK_IDENTIFIER: "my-task" as const, + ORGANIZATION_TITLE: "test" as const, + PROJECT_NAME: "test" as const, +} as const;Then use these constants throughout the tests instead of hardcoded strings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
internal-packages/clickhouse/src/__snapshots__/taskRuns.test.ts.snap
is excluded by!**/*.snap
📒 Files selected for processing (5)
.gitignore
(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.schedules.$scheduleParam/route.tsx
(3 hunks)apps/webapp/app/services/realtimeClient.server.ts
(8 hunks)apps/webapp/test/runsRepository.test.ts
(1 hunks)packages/core/src/v3/isomorphic/duration.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.schedules.$scheduleParam/route.tsx
- packages/core/src/v3/isomorphic/duration.ts
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
apps/webapp/app/services/realtimeClient.server.ts (7)
48-49
: Good defensive programming with reserved columns and search params.The protection of reserved columns and search parameters prevents clients from accidentally breaking core functionality.
81-104
: Excellent caching architecture with multi-tier storage.The implementation using both memory and Redis cache stores with appropriate TTL values (1 week fresh, 2 weeks stale) provides good performance while ensuring data consistency across instances.
161-187
: Robust createdAt filtering with proper error handling.The implementation correctly handles both new and existing shape IDs, with appropriate error logging for cache operations. The asynchronous error handling using
tryCatch
is well implemented.
301-320
: Solid column filtering implementation with security considerations.The implementation correctly filters out reserved columns from the skip list and provides sensible defaults. The use of quoted column names prevents SQL injection.
474-482
: Robust multi-origin resolution using consistent hashing.The implementation using jump hash ensures consistent routing of environment IDs to electric origins, which is important for maintaining session affinity. Good fallback to the first origin if index is out of bounds.
522-528
: Good backward compatibility in shape ID extraction.The fallback from
handle
toshape_id
ensures compatibility with older clients while supporting the new parameter naming.
52-52
:✅ Verification successful
Verify multi-origin configuration handling.
The change from single string to string array for
electricOrigin
introduces complexity but enables load distribution. Ensure this change is backward compatible.
🏁 Script executed:
#!/bin/bash # Search for electricOrigin configuration usage to verify backward compatibility rg -A 3 -B 3 "electricOrigin.*=" --type tsLength of output: 3921
🏁 Script executed:
#!/bin/bash rg -n "resolveElectricOrigin" -A10 -B10 apps/webapp/app/services/realtimeClient.server.tsLength of output: 1387
✅ Multi-origin
electricOrigin
is backward compatible.The
#resolveElectricOrigin
method inrealtimeClient.server.ts
handles both a single string and an array, returning the original string when passed one origin and using jump hashing (with a fallback to the first element) for arrays. No further changes are needed.• apps/webapp/app/services/realtimeClient.server.ts (lines 474–482):
#resolveElectricOrigin(environmentId: string) { if (typeof this.options.electricOrigin === "string") { return this.options.electricOrigin; } const index = jumpHash(environmentId, this.options.electricOrigin.length); return this.options.electricOrigin[index] ?? this.options.electricOrigin[0]; }• apps/webapp/app/services/realtimeClientGlobal.server.ts: initialization uses
env.ELECTRIC_ORIGIN_SHARDS?.split(",") ?? env.ELECTRIC_ORIGIN
apps/webapp/test/runsRepository.test.ts (3)
178-188
: Well-structured test coverage for filtering scenarios.The test logic comprehensively covers the various filtering capabilities of RunsRepository, including task identifiers, versions, statuses, tags, and other parameters. The assertions correctly verify the expected filtering behavior.
1474-1499
: Excellent pagination test coverage.The pagination test properly validates cursor-based pagination with appropriate assertions for page size, cursor presence, and navigation between pages.
1276-1277
:✅ Verification successful
Verify timestamp format expectations.
The past review comment mentioned a timestamp conversion issue with division by 1000, but the current code appears to correctly pass milliseconds. Please confirm this issue has been resolved and that RunsRepository expects timestamps in milliseconds.
🏁 Script executed:
#!/bin/bash # Description: Verify RunsRepository timestamp parameter expectations # Search for the listRuns method implementation to check timestamp handling ast-grep --pattern 'listRuns($_) { $$$ }'Length of output: 45
To pinpoint how
listRuns
handlesfrom
/to
, let’s locate theRunsRepository
implementation and inspect itslistRuns
method as well as any division by 1000:
🏁 Script executed:
#!/bin/bash # 1. Find the RunsRepository source file repo_file=$(rg -l "class RunsRepository" --type ts) echo "RunsRepository implementation: $repo_file" # 2. Show 5 lines before and after the listRuns method rg -n -B5 -A5 "listRuns\(" "$repo_file" # 3. Check for any division by 1000 in that file rg -n "/ *1000" "$repo_file"Length of output: 805
Let’s pull the full
listRuns
implementation and theListRunsOptions
type to confirm howfrom
/to
are handled:
🏁 Script executed:
#!/bin/bash # 1. Locate RunsRepository and ListRunsOptions definitions repo_file=$(rg -l "class RunsRepository" --type ts) opts_file=$(rg -l "ListRunsOptions" --type ts | head -n1) echo "RunsRepository: $repo_file" echo "Options type: $opts_file" # 2. Dump listRuns method (2 lines before, 50 after) rg -n -B2 -A50 "async listRuns" "$repo_file" # 3. Show ListRunsOptions interface rg -n -B3 -A10 "interface ListRunsOptions" "$opts_file"Length of output: 2697
Timestamp units are correct
The
listRuns
method usesfromUnixTimestamp64Milli
for bothfrom
andto
, accepting Int64 milliseconds. SinceDate.getTime()
already returns milliseconds, no division by 1000 is required and the original issue is resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prepped and ready to go
This PR makes a number of performance improvements using the following changes:
We've also prepared a "next" runs list page that uses ClickHouse, so we can test against real usage before fully switching over.