Test page recent payload: use ClickHouse to get the latest run ids #2614
Conversation
The Postgres query to get the latest run ids for the test page was very slow when there were a lot of runs and/or versions. This now uses the standard runs list we use everywhere else.
|
WalkthroughThis pull request refactors TestTaskPresenter to adopt a dependency injection pattern with replica-driven database access. The presenter no longer maintains an internal Prisma client; instead, it accepts injected dependencies: a PrismaClientOrTransaction replica instance and a ClickHouse client. Data fetching for task runs transitions from raw SQL queries to a repository-backed approach using RunsRepository, parameterized by projectId, organizationId, and environment details. All Prisma queries are redirected through the injected replica instance. The integration point in the route handler is updated to instantiate TestTaskPresenter with these new dependencies. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes The refactoring involves meaningful architectural changes—dependency injection, repository pattern introduction, and API signature modifications—across two interconnected files. The changes are heterogeneous (involving pattern shifts rather than repetitive edits) and require understanding the new data flow through RunsRepository and replica-based access patterns. However, the scope is narrowly focused, and the intent is clear and well-structured. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webapp/app/presenters/v3/TestTaskPresenter.server.ts(10 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.test.tasks.$taskParam/route.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/presenters/v3/TestTaskPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.test.tasks.$taskParam/route.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/presenters/v3/TestTaskPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.test.tasks.$taskParam/route.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/presenters/v3/TestTaskPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.test.tasks.$taskParam/route.tsx
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/presenters/v3/TestTaskPresenter.server.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/presenters/v3/TestTaskPresenter.server.ts
🧠 Learnings (1)
📚 Learning: 2025-07-12T18:06:04.133Z
Learnt from: matt-aitken
PR: triggerdotdev/trigger.dev#2264
File: apps/webapp/app/services/runsRepository.server.ts:172-174
Timestamp: 2025-07-12T18:06:04.133Z
Learning: In apps/webapp/app/services/runsRepository.server.ts, the in-memory status filtering after fetching runs from Prisma is intentionally used as a workaround for ClickHouse data delays. This approach is acceptable because the result set is limited to a maximum of 100 runs due to pagination, making the performance impact negligible.
Applied to files:
apps/webapp/app/presenters/v3/TestTaskPresenter.server.ts
🧬 Code graph analysis (2)
apps/webapp/app/presenters/v3/TestTaskPresenter.server.ts (3)
apps/webapp/app/db.server.ts (2)
PrismaClientOrTransaction(22-22)PrismaClient(370-370)internal-packages/clickhouse/src/index.ts (1)
ClickHouse(62-174)apps/webapp/app/services/runsRepository/runsRepository.server.ts (1)
RunsRepository(141-333)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.test.tasks.$taskParam/route.tsx (3)
apps/webapp/app/presenters/v3/TestTaskPresenter.server.ts (1)
TestTaskPresenter(118-359)apps/webapp/app/db.server.ts (1)
$replica(103-106)apps/webapp/app/services/clickhouseInstance.server.ts (1)
clickhouseClient(5-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- 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 (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- 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 - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.test.tasks.$taskParam/route.tsx (2)
76-77: LGTM!The imports are correct and align with the new dependency injection pattern in TestTaskPresenter. Both
$replicaandclickhouseClientare singleton instances, which is appropriate for shared database resources.
101-101: ****The environment object returned by
findEnvironmentBySlugdoes includeprojectIdandorganizationId. The function returnsAuthenticatedEnvironment, which is a Prisma payload that includes the fullRuntimeEnvironmentfields (id, type, projectId, organizationId) plus relations to project, organization, and orgMember. These fields remain accessible regardless of which relations are included.apps/webapp/app/presenters/v3/TestTaskPresenter.server.ts (5)
1-1: LGTM!The new imports support the ClickHouse integration and dependency injection pattern. All imports are properly typed and necessary for the refactored implementation.
Also applies to: 7-7, 9-11
26-27: Verify consistency with caller.The environment type now requires
projectIdandorganizationIdfields. This is necessary for the RunsRepository integration, but ensure the calling code in route.tsx provides an environment with these fields. (Related to verification comment in route.tsx.)
135-135: LGTM!The migration from internal
#prismaClientto injectedreplicais consistent across all Prisma queries. This aligns well with the dependency injection pattern.Also applies to: 152-152, 166-166, 180-180
197-245: Excellent performance optimization!The switch from raw Postgres SQL to ClickHouse-backed
RunsRepositoryaddresses the PR objective of improving query performance for tasks with many runs/versions. Key improvements:
- ClickHouse handles the heavy lifting of filtering and sorting run IDs
- Postgres fetch is limited to the filtered run IDs (max 10 with 30d period)
- Explicit
selectfields minimize data transferRunsRepositoryhas built-in fallback to Postgres if ClickHouse failsThe pattern aligns with the learning that small result sets (≤100 runs) make post-fetch operations efficient.
Based on learnings
273-278: LGTM!Normalizing nullable fields to
undefinedimproves type consistency and aligns with TypeScript best practices for optional properties. The pattern is consistently applied to both STANDARD and SCHEDULED trigger sources.Also applies to: 321-326
The Postgres query to get the latest run ids for the test page was very
slow when there were a lot of runs and/or versions.
This now uses the standard runs list we use everywhere else.