Tags listing now uses ClickHouse #2576
Conversation
|
WalkthroughShifts tag fetching from project-scoped to environment-scoped across UI and server. The RunFilters component now queries tags via an environment-based route with integrated time filters (period/from/to) and uses the returned tags array directly. A new loader for resources.environments.$envId.runs.tags validates auth, params, and time filters, then calls RunTagListPresenter. The presenter now uses RunsRepository.listTags with organizationId/environmentId and time range. RunsRepository interface and implementations (ClickHouse/Postgres) add listTags; ClickHouse uses a new tag query builder exposed via internal clickhouse package updates. The old project-scoped tags route is removed. tsconfig excludes tests. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/webapp/app/components/runs/v3/RunFilters.tsx (1)
838-852: Stop double encoding the tag search value.Line 840 wraps
searchValueinencodeURIComponent, butURLSearchParamsalready encodes for us. A query like “foo bar” becomesfoo%2520bar, and the loader only decodes once, so the presenter receivesfoo%20barand the ClickHouse search fails to match real tag names. Remove the manual encoding and letURLSearchParamshandle it.- if (searchValue) { - searchParams.set("name", encodeURIComponent(searchValue)); - } + if (searchValue) { + searchParams.set("name", searchValue); + }
🧹 Nitpick comments (1)
apps/webapp/app/services/runsRepository/runsRepository.server.ts (1)
316-332: Missing fallback logic for resilience.Unlike
listRunIds,listRuns, andcountRuns(lines 184-314), thelistTagsmethod does not include fallback logic to retry with Postgres if ClickHouse fails. This creates an inconsistency in resilience patterns.Apply this diff to add fallback logic:
async listTags(options: TagListOptions): Promise<TagList> { const repository = await this.#getRepository(); return startActiveSpan( "runsRepository.listTags", async () => { - return await repository.listTags(options); + try { + return await repository.listTags(options); + } catch (error) { + // If ClickHouse fails, retry with Postgres + if (repository.name === "clickhouse") { + this.logger?.warn("ClickHouse failed, retrying with Postgres", { error }); + return startActiveSpan( + "runsRepository.listTags.fallback", + async () => { + return await this.postgresRunsRepository.listTags(options); + }, + { + attributes: { + "repository.name": "postgres", + "fallback.reason": "clickhouse_error", + "fallback.error": error instanceof Error ? error.message : String(error), + organizationId: options.organizationId, + projectId: options.projectId, + environmentId: options.environmentId, + }, + } + ); + } + throw error; + } }, { attributes: { "repository.name": repository.name, organizationId: options.organizationId, projectId: options.projectId, environmentId: options.environmentId, }, } ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/webapp/app/components/runs/v3/RunFilters.tsx(6 hunks)apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts(2 hunks)apps/webapp/app/routes/resources.environments.$envId.runs.tags.tsx(1 hunks)apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.ai-filter.tsx(1 hunks)apps/webapp/app/routes/resources.projects.$projectParam.runs.tags.tsx(0 hunks)apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts(2 hunks)apps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts(2 hunks)apps/webapp/app/services/runsRepository/runsRepository.server.ts(4 hunks)apps/webapp/tsconfig.check.json(1 hunks)internal-packages/clickhouse/src/index.ts(2 hunks)internal-packages/clickhouse/src/taskRuns.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/webapp/app/routes/resources.projects.$projectParam.runs.tags.tsx
🧰 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:
internal-packages/clickhouse/src/taskRuns.tsapps/webapp/app/services/runsRepository/runsRepository.server.tsinternal-packages/clickhouse/src/index.tsapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.ai-filter.tsxapps/webapp/app/services/runsRepository/postgresRunsRepository.server.tsapps/webapp/app/presenters/v3/RunTagListPresenter.server.tsapps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.tsapps/webapp/app/routes/resources.environments.$envId.runs.tags.tsxapps/webapp/app/components/runs/v3/RunFilters.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/services/runsRepository/runsRepository.server.tsapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.ai-filter.tsxapps/webapp/app/services/runsRepository/postgresRunsRepository.server.tsapps/webapp/app/presenters/v3/RunTagListPresenter.server.tsapps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.tsapps/webapp/app/routes/resources.environments.$envId.runs.tags.tsxapps/webapp/app/components/runs/v3/RunFilters.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/services/runsRepository/runsRepository.server.tsapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.ai-filter.tsxapps/webapp/app/services/runsRepository/postgresRunsRepository.server.tsapps/webapp/app/presenters/v3/RunTagListPresenter.server.tsapps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.tsapps/webapp/app/routes/resources.environments.$envId.runs.tags.tsxapps/webapp/app/components/runs/v3/RunFilters.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/services/runsRepository/runsRepository.server.tsapps/webapp/app/services/runsRepository/postgresRunsRepository.server.tsapps/webapp/app/presenters/v3/RunTagListPresenter.server.tsapps/webapp/app/services/runsRepository/clickhouseRunsRepository.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/services/runsRepository/runsRepository.server.tsapps/webapp/app/services/runsRepository/postgresRunsRepository.server.tsapps/webapp/app/presenters/v3/RunTagListPresenter.server.tsapps/webapp/app/services/runsRepository/clickhouseRunsRepository.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/RunTagListPresenter.server.ts
🧬 Code graph analysis (9)
internal-packages/clickhouse/src/taskRuns.ts (1)
internal-packages/clickhouse/src/client/types.ts (1)
ClickhouseReader(20-75)
apps/webapp/app/services/runsRepository/runsRepository.server.ts (1)
apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts (2)
TagListOptions(7-19)TagList(23-23)
internal-packages/clickhouse/src/index.ts (1)
internal-packages/clickhouse/src/taskRuns.ts (1)
getTaskRunTagsQueryBuilder(118-127)
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.ai-filter.tsx (1)
packages/trigger-sdk/src/v3/tags.ts (1)
tags(12-14)
apps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts (2)
apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts (1)
TagListOptions(7-19)apps/webapp/app/services/runsRepository/runsRepository.server.ts (1)
TagListOptions(112-121)
apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts (3)
apps/webapp/app/services/runsRepository/runsRepository.server.ts (3)
TagListOptions(112-121)name(158-160)RunsRepository(141-333)apps/webapp/app/services/clickhouseInstance.server.ts (1)
clickhouseClient(5-5)apps/webapp/app/db.server.ts (1)
PrismaClient(370-370)
apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts (2)
apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts (1)
TagListOptions(7-19)apps/webapp/app/services/runsRepository/runsRepository.server.ts (1)
TagListOptions(112-121)
apps/webapp/app/routes/resources.environments.$envId.runs.tags.tsx (4)
apps/webapp/app/services/session.server.ts (1)
requireUserId(25-35)apps/webapp/app/db.server.ts (1)
$replica(103-106)apps/webapp/app/components/runs/v3/SharedFilters.tsx (1)
timeFilters(106-163)apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts (1)
RunTagListPresenter(26-64)
apps/webapp/app/components/runs/v3/RunFilters.tsx (3)
apps/webapp/app/hooks/useEnvironment.tsx (1)
useEnvironment(19-23)apps/webapp/app/hooks/useSearchParam.ts (1)
useSearchParams(7-70)apps/webapp/app/components/runs/v3/SharedFilters.tsx (1)
timeFilters(106-163)
⏰ 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 / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 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 (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/webapp/tsconfig.check.json (1)
12-13: LGTM!The test file exclusion pattern is appropriate and will prevent type-checking overhead on test files during the check process.
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.ai-filter.tsx (2)
68-75: LGTM!The shift to environment-scoped identifiers and addition of a 1-year time filter appropriately scopes tag queries to the current environment context.
77-77: Verified response structure RunTagListPresenter.call returnstagsas astring[], so replacingtags.tags.map(t => t.name)withtags.tagsis correct.apps/webapp/app/services/runsRepository/runsRepository.server.ts (1)
72-75: LGTM!Clean pagination type for offset-based queries.
| async listTags(options: TagListOptions) { | ||
| const queryBuilder = this.options.clickhouse.taskRuns | ||
| .tagQueryBuilder() | ||
| .where("organization_id = {organizationId: String}", { | ||
| organizationId: options.organizationId, | ||
| }) | ||
| .where("project_id = {projectId: String}", { | ||
| projectId: options.projectId, | ||
| }) | ||
| .where("environment_id = {environmentId: String}", { | ||
| environmentId: options.environmentId, | ||
| }); | ||
|
|
||
| const periodMs = options.period ? parseDuration(options.period) ?? undefined : undefined; | ||
| if (periodMs) { | ||
| queryBuilder.where("created_at >= fromUnixTimestamp64Milli({period: Int64})", { | ||
| period: new Date(Date.now() - periodMs).getTime(), | ||
| }); | ||
| } | ||
|
|
||
| if (options.from) { | ||
| queryBuilder.where("created_at >= fromUnixTimestamp64Milli({from: Int64})", { | ||
| from: options.from, | ||
| }); | ||
| } | ||
|
|
||
| if (options.to) { | ||
| queryBuilder.where("created_at <= fromUnixTimestamp64Milli({to: Int64})", { to: options.to }); | ||
| } | ||
|
|
||
| const [queryError, result] = await queryBuilder.execute(); | ||
|
|
||
| if (queryError) { | ||
| throw queryError; | ||
| } | ||
|
|
||
| if (result.length === 0) { | ||
| return { | ||
| tags: [], | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| tags: result.flatMap((row) => row.tags), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Honor query and pagination when listing tags.
listTags ignores options.query, options.limit, and options.offset, so /runs/tags always scans everything and the name= filter from the presenter is a no-op. That’s a regression from the previous implementation and makes hasMore meaningless. You need to push the filtering and pagination into ClickHouse before executing the query. For example, apply the name filter:
if (options.to) {
queryBuilder.where("created_at <= fromUnixTimestamp64Milli({to: Int64})", { to: options.to });
}
+
+ if (options.query) {
+ queryBuilder.where(
+ "arrayExists(tag -> ilike(tag, {query: String}), tags)",
+ { query: `%${options.query}%` }
+ );
+ }
+
+ if (options.limit !== undefined) {
+ queryBuilder.limit(options.limit + 1, options.offset ?? 0);
+ }Make sure the ClickHouse builder’s limit call (or equivalent) honours both limit and offset so the response aligns with the requested page.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async listTags(options: TagListOptions) { | |
| const queryBuilder = this.options.clickhouse.taskRuns | |
| .tagQueryBuilder() | |
| .where("organization_id = {organizationId: String}", { | |
| organizationId: options.organizationId, | |
| }) | |
| .where("project_id = {projectId: String}", { | |
| projectId: options.projectId, | |
| }) | |
| .where("environment_id = {environmentId: String}", { | |
| environmentId: options.environmentId, | |
| }); | |
| const periodMs = options.period ? parseDuration(options.period) ?? undefined : undefined; | |
| if (periodMs) { | |
| queryBuilder.where("created_at >= fromUnixTimestamp64Milli({period: Int64})", { | |
| period: new Date(Date.now() - periodMs).getTime(), | |
| }); | |
| } | |
| if (options.from) { | |
| queryBuilder.where("created_at >= fromUnixTimestamp64Milli({from: Int64})", { | |
| from: options.from, | |
| }); | |
| } | |
| if (options.to) { | |
| queryBuilder.where("created_at <= fromUnixTimestamp64Milli({to: Int64})", { to: options.to }); | |
| } | |
| const [queryError, result] = await queryBuilder.execute(); | |
| if (queryError) { | |
| throw queryError; | |
| } | |
| if (result.length === 0) { | |
| return { | |
| tags: [], | |
| }; | |
| } | |
| return { | |
| tags: result.flatMap((row) => row.tags), | |
| }; | |
| } | |
| async listTags(options: TagListOptions) { | |
| const queryBuilder = this.options.clickhouse.taskRuns | |
| .tagQueryBuilder() | |
| .where("organization_id = {organizationId: String}", { | |
| organizationId: options.organizationId, | |
| }) | |
| .where("project_id = {projectId: String}", { | |
| projectId: options.projectId, | |
| }) | |
| .where("environment_id = {environmentId: String}", { | |
| environmentId: options.environmentId, | |
| }); | |
| const periodMs = options.period ? parseDuration(options.period) ?? undefined : undefined; | |
| if (periodMs) { | |
| queryBuilder.where( | |
| "created_at >= fromUnixTimestamp64Milli({period: Int64})", | |
| { | |
| period: new Date(Date.now() - periodMs).getTime(), | |
| } | |
| ); | |
| } | |
| if (options.from) { | |
| queryBuilder.where( | |
| "created_at >= fromUnixTimestamp64Milli({from: Int64})", | |
| { from: options.from } | |
| ); | |
| } | |
| if (options.to) { | |
| queryBuilder.where( | |
| "created_at <= fromUnixTimestamp64Milli({to: Int64})", | |
| { to: options.to } | |
| ); | |
| } | |
| if (options.query) { | |
| queryBuilder.where( | |
| "arrayExists(tag -> ilike(tag, {query: String}), tags)", | |
| { query: `%${options.query}%` } | |
| ); | |
| } | |
| if (options.limit !== undefined) { | |
| // Fetch one extra to detect "hasMore" on the client | |
| queryBuilder.limit(options.limit + 1, options.offset ?? 0); | |
| } | |
| const [queryError, result] = await queryBuilder.execute(); | |
| if (queryError) { | |
| throw queryError; | |
| } | |
| if (result.length === 0) { | |
| return { | |
| tags: [], | |
| }; | |
| } | |
| return { | |
| tags: result.flatMap((row) => row.tags), | |
| }; | |
| } |
🤖 Prompt for AI Agents
In apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts
around lines 168 to 213, listTags currently ignores options.query, options.limit
and options.offset so name filtering and pagination happen in JS instead of
ClickHouse; update the ClickHouse query builder to (1) apply the name filter
when options.query is present (e.g., add a where clause that filters tag
name/keys by the provided query string), (2) push pagination into the builder by
calling its limit/offset (or equivalent) using options.limit and options.offset,
and (3) when computing hasMore, fetch one extra row (limit + 1) from ClickHouse
and derive hasMore from that extra row, returning only the requested page of
tags; ensure the parameter types and placeholders match the builder API and
preserve existing organization/project/environment/time filters.
| async listTags({ projectId, query, offset, limit }: TagListOptions) { | ||
| const tags = await this.options.prisma.taskRunTag.findMany({ | ||
| select: { | ||
| name: true, | ||
| }, | ||
| where: { | ||
| projectId, | ||
| name: query | ||
| ? { | ||
| startsWith: query, | ||
| mode: "insensitive", | ||
| } | ||
| : undefined, | ||
| }, | ||
| orderBy: { | ||
| id: "desc", | ||
| }, | ||
| take: limit + 1, | ||
| skip: offset, | ||
| }); | ||
|
|
||
| return { | ||
| tags: tags.map((tag) => tag.name), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Critical type mismatch and missing filter implementations.
The method signature destructures only { projectId, query, offset, limit } but TagListOptions (imported from runsRepository.server.ts at line 11) includes additional required fields: organizationId, environmentId, period, from, and to. This implementation:
- Ignores
organizationIdandenvironmentIdfilters entirely, which could return tags from wrong organizations/environments - Ignores time-range filters (
period,from,to), unlike the ClickHouse implementation - Uses
startsWithfor query filtering but theTagListOptionstype documents it as "case insensitive contains search" - Fetches
limit + 1rows (line 125) but doesn't return pagination metadata likehasMore
Apply this diff to align with the interface contract:
- async listTags({ projectId, query, offset, limit }: TagListOptions) {
+ async listTags({ organizationId, environmentId, projectId, query, period, from, to, offset, limit }: TagListOptions) {
+ // Build time filter conditions
+ const timeConditions: Prisma.TaskRunTagWhereInput = {};
+
+ if (period) {
+ const periodMs = parseDuration(period);
+ if (periodMs) {
+ timeConditions.createdAt = {
+ gte: new Date(Date.now() - periodMs),
+ };
+ }
+ }
+
+ if (from) {
+ timeConditions.createdAt = {
+ ...timeConditions.createdAt,
+ gte: new Date(from),
+ };
+ }
+
+ if (to) {
+ timeConditions.createdAt = {
+ ...timeConditions.createdAt,
+ lte: new Date(to),
+ };
+ }
+
const tags = await this.options.prisma.taskRunTag.findMany({
select: {
name: true,
},
where: {
+ project: {
+ organizationId,
+ },
projectId,
+ taskRun: {
+ runtimeEnvironmentId: environmentId,
+ },
name: query
? {
- startsWith: query,
+ contains: query,
mode: "insensitive",
}
: undefined,
+ ...timeConditions,
},
orderBy: {
id: "desc",
},
- take: limit + 1,
+ take: limit,
skip: offset,
});
return {
tags: tags.map((tag) => tag.name),
};
}Note: You'll need to import parseDuration from "parse-duration" at the top of the file if not already imported.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts
around lines 108–132, the listTags implementation only handles projectId, query,
offset, and limit but must also apply organizationId and environmentId filters,
honor time-range filters (period / from / to), use a case-insensitive "contains"
search for tag name, and return pagination metadata. Update the function
signature to destructure organizationId, environmentId, period, from, to;
compute an effective from/to range (if period is provided, compute from = now -
parseDuration(period)); add the where filters for organizationId and
environmentId; add createdAt: { gte: from, lte: to } when available; change name
filter to { contains: query, mode: "insensitive" } when query is present; keep
take: limit + 1 and determine hasMore = tags.length > limit, then slice to limit
before mapping names and return { tags, hasMore }. Also import parseDuration
from "parse-duration" at the top if not already imported.
| export type TagListOptions = { | ||
| organizationId: string; | ||
| projectId: string; | ||
| environmentId: string; | ||
| period?: string; | ||
| from?: number; | ||
| to?: number; | ||
| /** Performs a case insensitive contains search on the tag name */ | ||
| query?: string; | ||
| } & OffsetPagination; | ||
|
|
||
| export type TagList = { | ||
| tags: string[]; | ||
| }; |
There was a problem hiding this comment.
Type mismatch with presenter and misleading documentation.
Issues identified:
-
Type mismatch:
TagListOptions.fromandtoare typed asnumber(timestamps), butRunTagListPresenter.server.ts(line 6-18 in relevant snippets) usesDateobjects. This will cause type errors or runtime issues. -
Misleading documentation: Line 119 says "Performs a case insensitive contains search" but the Postgres implementation (postgresRunsRepository.server.ts) uses
startsWithfiltering, notcontains.
Apply this diff to align types:
export type TagListOptions = {
organizationId: string;
projectId: string;
environmentId: string;
period?: string;
- from?: number;
- to?: number;
+ /** Unix timestamp in milliseconds */
+ from?: number;
+ /** Unix timestamp in milliseconds */
+ to?: number;
/** Performs a case insensitive contains search on the tag name */
query?: string;
} & OffsetPagination;Also ensure the presenter converts Date objects to timestamps before calling the repository, or change the repository to accept Date objects.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type TagListOptions = { | |
| organizationId: string; | |
| projectId: string; | |
| environmentId: string; | |
| period?: string; | |
| from?: number; | |
| to?: number; | |
| /** Performs a case insensitive contains search on the tag name */ | |
| query?: string; | |
| } & OffsetPagination; | |
| export type TagList = { | |
| tags: string[]; | |
| }; | |
| export type TagListOptions = { | |
| organizationId: string; | |
| projectId: string; | |
| environmentId: string; | |
| period?: string; | |
| /** Unix timestamp in milliseconds */ | |
| from?: number; | |
| /** Unix timestamp in milliseconds */ | |
| to?: number; | |
| /** Performs a case insensitive contains search on the tag name */ | |
| query?: string; | |
| } & OffsetPagination; | |
| export type TagList = { | |
| tags: string[]; | |
| }; |
🤖 Prompt for AI Agents
In apps/webapp/app/services/runsRepository/runsRepository.server.ts around lines
112-125, update TagListOptions.by changing from?: number and to?: number to
from?: Date and to?: Date to match RunTagListPresenter.server.ts; change the
JSDoc for query to say "Performs a case-insensitive startsWith (prefix) search"
to reflect the Postgres implementation; then update the Postgres repository
implementation (postgresRunsRepository.server.ts) to accept Date values and
internally convert them to timestamps (e.g., .getTime()) when building DB
queries so types are consistent end-to-end.
| }; | ||
| }>; | ||
| countRuns(options: RunListInputOptions): Promise<number>; | ||
| listTags(options: TagListOptions): Promise<TagList>; |
There was a problem hiding this comment.
🧩 Analysis chain
Missing implementation consistency check.
Verify that both ClickHouse and Postgres implementations conform to the TagListOptions contract, especially regarding the organizationId, environmentId, and time filter parameters.
🏁 Script executed:
#!/bin/bash
# Description: Verify listTags implementations in both repositories
# Check ClickHouse implementation
echo "=== ClickHouse Implementation ==="
ast-grep --pattern 'class ClickHouseRunsRepository $$$ {
$$$
async listTags($$$) {
$$$
}
$$$
}'
# Check Postgres implementation
echo "=== Postgres Implementation ==="
ast-grep --pattern 'class PostgresRunsRepository $$$ {
$$$
async listTags($$$) {
$$$
}
$$$
}'Length of output: 53749
Ensure Postgres listTags respects TagListOptions filters
PostgresRunsRepository.listTags currently only filters by projectId, query, offset, and limit. It must also include organizationId, environmentId, and time-range filters (period, from, to) to conform to the TagListOptions contract.
🤖 Prompt for AI Agents
In apps/webapp/app/services/runsRepository/runsRepository.server.ts around line
138, update PostgresRunsRepository.listTags to honor the full TagListOptions
contract: extend the WHERE clause to include organization_id and environment_id
when provided, apply text search on query as already done, and add time-range
filtering using period/from/to by translating period into a from timestamp or
directly using provided from/to to filter the runs.created_at (or
tags.created_at if appropriate); ensure all filters are applied only when the
option is present, keep offset/limit and parameterization intact, and update any
total count logic to reflect these additional filters.
| export function getTaskRunTagsQueryBuilder(ch: ClickhouseReader, settings?: ClickHouseSettings) { | ||
| return ch.queryBuilder({ | ||
| name: "getTaskRunTags", | ||
| baseQuery: "SELECT DISTINCT tags FROM trigger_dev.task_runs_v2", | ||
| schema: z.object({ | ||
| tags: z.array(z.string()), | ||
| }), | ||
| settings, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Critical: Query returns arrays, not distinct tag strings.
The query SELECT DISTINCT tags FROM trigger_dev.task_runs_v2 operates on the tags array column (defined at line 32 as z.array(z.string())). This returns distinct arrays of tags, not distinct individual tag values. Additionally:
- No WHERE clause to filter by organizationId/environmentId/projectId
- No pagination (LIMIT/OFFSET) support
- No time-range filtering
Apply this diff to properly flatten and filter tags:
export function getTaskRunTagsQueryBuilder(ch: ClickhouseReader, settings?: ClickHouseSettings) {
return ch.queryBuilder({
name: "getTaskRunTags",
- baseQuery: "SELECT DISTINCT tags FROM trigger_dev.task_runs_v2",
+ baseQuery: "SELECT DISTINCT arrayJoin(tags) as tag FROM trigger_dev.task_runs_v2 FINAL WHERE _is_deleted = 0",
schema: z.object({
- tags: z.array(z.string()),
+ tag: z.string(),
}),
settings,
});
}The caller will need to:
- Add WHERE clauses for organization_id, project_id, environment_id
- Add time-range filters (created_at) if period/from/to are provided
- Add tag name filtering if query is provided (e.g.,
AND tag ILIKE {query: String}) - Add LIMIT and OFFSET for pagination
- Aggregate results into an array
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function getTaskRunTagsQueryBuilder(ch: ClickhouseReader, settings?: ClickHouseSettings) { | |
| return ch.queryBuilder({ | |
| name: "getTaskRunTags", | |
| baseQuery: "SELECT DISTINCT tags FROM trigger_dev.task_runs_v2", | |
| schema: z.object({ | |
| tags: z.array(z.string()), | |
| }), | |
| settings, | |
| }); | |
| } | |
| export function getTaskRunTagsQueryBuilder(ch: ClickhouseReader, settings?: ClickHouseSettings) { | |
| return ch.queryBuilder({ | |
| name: "getTaskRunTags", | |
| baseQuery: "SELECT DISTINCT arrayJoin(tags) as tag FROM trigger_dev.task_runs_v2 FINAL WHERE _is_deleted = 0", | |
| schema: z.object({ | |
| tag: z.string(), | |
| }), | |
| settings, | |
| }); | |
| } |
This is currently used in the dashboard only