fix(webapp): logs page #2889
fix(webapp): logs page #2889
Conversation
added support for tasks_v1 for logs cleaned the No logs message extended LogsPresenter with BasePresenter for spans
|
|
Caution Review failedThe pull request is closed. WalkthroughThe changes add organization-aware event repository selection and runtime routing between ClickHouse v2, ClickHouse v1, and PostgreSQL (with PostgreSQL returning a ServiceValidationError). ClickHouse query builders and result shapes are versioned: metadata/attributes are replaced by attributes_text. Presenters, repositories, and routes select builders by store and handle validation errors. A new FEATURE_FLAG.hasLogsPageAccess gates Logs page access and is used in loaders and the side menu. UI: LogsTable drops the Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
✏️ Tip: You can disable this entire section by setting 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 review |
✅ Actions performedReview triggered.
|
PR Review: logs-page-fixesSummaryThis PR makes several improvements to the logs page functionality, including:
Issues Found1. Critical: Unused
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/presenters/v3/LogDetailPresenter.server.ts (1)
78-94: Remove dead code forlog.metadataparsing.The
metadatacolumn was removed from both query builders (getLogDetailQueryBuilderV1 and getLogDetailQueryBuilderV2) in commit c55ab1e. Thelog.metadatafield will always beundefined, making the parsing block on lines 78-84 and therawMetadataassignment on line 113 dead code. Either restore themetadatacolumn to the queries if it's needed, or remove the unused metadata parsing logic.
🤖 Fix all issues with AI agents
In `@apps/webapp/app/components/logs/LogsTable.tsx`:
- Around line 127-129: The empty-state message currently shown in LogsTable when
logs.length === 0 still mentions filters even though hasFilters was removed;
update the BlankState usage in LogsTable (or the BlankState component itself) to
show a generic message like "No logs found." or to accept a prop that
differentiates filtered vs. empty results; specifically, either restore a
boolean (e.g., hasFilters) passed to BlankState from LogsTable based on current
filter state or change the displayed text in the BlankState call inside
LogsTable to a neutral message so users who haven't applied filters aren't
misled.
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx:
- Around line 61-93: The loader currently only checks isAdmin and redirects
non-admins, bypassing the feature flag check; replace that simple admin check
with an awaited call to hasLogsPageAccess(userId, isAdmin, isImpersonating,
organizationSlug) and use its boolean result to decide access; specifically,
remove or replace the if (!isAdmin) { throw redirect("/") } logic with: await
hasLogsPageAccess(...) and if it returns false then throw redirect("/"); ensure
you pass the same userId, isAdmin, isImpersonating, and organizationSlug
variables and keep existing redirect behavior for denied access.
🧹 Nitpick comments (3)
apps/webapp/app/v3/eventRepository/index.server.ts (1)
27-42: Consider using$replicafor read operations.The function queries organization data using
prismadirectly, but other parts of the codebase (e.g.,RunsRepository, presenters) use$replicafor read operations to distribute load. Additionally, the error thrown is a genericErrorrather thanServiceValidationErrorused elsewhere in the codebase for validation errors.♻️ Suggested improvements
+import { $replica } from "~/db.server"; +import { ServiceValidationError } from "~/v3/services/baseService.server"; export async function getConfiguredEventRepository( organizationId: string ): Promise<{ repository: IEventRepository; store: string }> { - const organization = await prisma.organization.findFirst({ + const organization = await $replica.organization.findFirst({ select: { id: true, featureFlags: true, }, where: { id: organizationId, }, }); if (!organization) { - throw new Error('Organization not found when configuring event repository'); + throw new ServiceValidationError('Organization not found when configuring event repository'); }apps/webapp/app/presenters/v3/LogsListPresenter.server.ts (1)
545-555: Useconstfor the parsed attributes variable.The
attributesvariable is never reassigned, so it should be declared withconstinstead oflet.♻️ Suggested fix
if (log.status === "ERROR" && log.attributes_text) { try { - let attributes = JSON.parse(log.attributes_text) as ErrorAttributes; + const attributes = JSON.parse(log.attributes_text) as ErrorAttributes; if (attributes?.error?.message && typeof attributes.error.message === "string") { displayMessage = attributes.error.message; } } catch { // If attributes parsing fails, use the regular message } }internal-packages/clickhouse/src/taskEvents.ts (1)
353-375: Consider renaming the result type for clarity.The V1 detail builder uses
LogDetailV2Resultas its result type. While functionally correct (same schema), this naming could be confusing. Consider either:
- Creating a
LogDetailResulttype alias that both versions use- Renaming
LogDetailV2ResulttoLogDetailResultsince it's shared
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/env.server.tsapps/webapp/app/presenters/v3/LogDetailPresenter.server.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsxapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.tsxapps/webapp/app/v3/eventRepository/index.server.tsapps/webapp/app/v3/featureFlags.server.tsinternal-packages/clickhouse/src/index.tsinternal-packages/clickhouse/src/taskEvents.ts
💤 Files with no reviewable changes (1)
- apps/webapp/app/env.server.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects insteadEvery Trigger.dev task must be exported; use task() function with unique id and run async function
Files:
apps/webapp/app/presenters/v3/LogDetailPresenter.server.tsapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.tsxapps/webapp/app/v3/featureFlags.server.tsinternal-packages/clickhouse/src/taskEvents.tsapps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/v3/eventRepository/index.server.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsxinternal-packages/clickhouse/src/index.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/presenters/v3/LogDetailPresenter.server.tsapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.tsxapps/webapp/app/v3/featureFlags.server.tsapps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/v3/eventRepository/index.server.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/app/presenters/v3/LogDetailPresenter.server.tsapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.tsxapps/webapp/app/v3/featureFlags.server.tsinternal-packages/clickhouse/src/taskEvents.tsapps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/v3/eventRepository/index.server.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsxinternal-packages/clickhouse/src/index.ts
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webappIn webapp development, access environment variables via env export from apps/webapp/app/env.server.ts; never use process.env directly
Files:
apps/webapp/app/presenters/v3/LogDetailPresenter.server.tsapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.tsxapps/webapp/app/v3/featureFlags.server.tsapps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/v3/eventRepository/index.server.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Files:
apps/webapp/app/presenters/v3/LogDetailPresenter.server.tsapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.tsxapps/webapp/app/v3/featureFlags.server.tsapps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/v3/eventRepository/index.server.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx
**/*.{js,ts,jsx,tsx,json,md,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier
Files:
apps/webapp/app/presenters/v3/LogDetailPresenter.server.tsapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.tsxapps/webapp/app/v3/featureFlags.server.tsinternal-packages/clickhouse/src/taskEvents.tsapps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/v3/eventRepository/index.server.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsxinternal-packages/clickhouse/src/index.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
apps/webapp/app/presenters/v3/LogDetailPresenter.server.tsapps/webapp/app/v3/featureFlags.server.tsinternal-packages/clickhouse/src/taskEvents.tsapps/webapp/app/v3/eventRepository/index.server.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.tsinternal-packages/clickhouse/src/index.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: Import from@trigger.dev/coreusing subpaths only; never import from root of@trigger.dev/core
Always import task definitions from@trigger.dev/sdk, never from@trigger.dev/sdk/v3or deprecated client.defineJob pattern
Files:
apps/webapp/app/presenters/v3/LogDetailPresenter.server.tsapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.tsxapps/webapp/app/v3/featureFlags.server.tsinternal-packages/clickhouse/src/taskEvents.tsapps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/v3/eventRepository/index.server.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsxinternal-packages/clickhouse/src/index.ts
🧠 Learnings (19)
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/app/v3/presenters/**/*.server.{ts,tsx} : Organize presenters in the webapp following the pattern `app/v3/presenters/*/*.server.ts` to move complex loader code into classes
Applied to files:
apps/webapp/app/presenters/v3/LogDetailPresenter.server.tsapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.tsxapps/webapp/app/presenters/v3/LogsListPresenter.server.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Configure OpenTelemetry instrumentations and exporters in trigger.config.ts for enhanced logging
Applied to files:
apps/webapp/app/presenters/v3/LogDetailPresenter.server.ts
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/app/v3/services/**/*.server.{ts,tsx} : Organize services in the webapp following the pattern `app/v3/services/*/*.server.ts`
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.tsx
📚 Learning: 2025-12-08T15:19:56.823Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2760
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx:278-281
Timestamp: 2025-12-08T15:19:56.823Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx, the tableState search parameter uses intentional double-encoding: the parameter value contains a URL-encoded URLSearchParams string, so decodeURIComponent(value("tableState") ?? "") is required to fully decode it before parsing with new URLSearchParams(). This pattern allows bundling multiple filter/pagination params as a single search parameter.
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx
📚 Learning: 2026-01-12T11:01:34.792Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:01:34.792Z
Learning: Applies to internal-packages/database/prisma/migrations/**/*.sql : Edit database schema in internal-packages/database/prisma/schema.prisma; remove extraneous lines from migrations related to BackgroundWorker, TaskRun tags, waitpoints, and SecretStore indexes
Applied to files:
internal-packages/clickhouse/src/taskEvents.tsinternal-packages/clickhouse/src/index.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use `schemaTask()` from `trigger.dev/sdk/v3` with Zod schema for payload validation
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use the `task()` function from `trigger.dev/sdk/v3` to define tasks with id and run properties
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.tsinternal-packages/clickhouse/src/index.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Attach metadata to task runs using the metadata option when triggering, and access/update it inside runs using metadata functions
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.ts
📚 Learning: 2026-01-12T11:01:34.792Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:01:34.792Z
Learning: Applies to **/*.{ts,tsx} : Every Trigger.dev task must be exported; use task() function with unique id and run async function
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.ts
📚 Learning: 2026-01-12T11:01:34.792Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:01:34.792Z
Learning: Applies to **/*.{ts,tsx,js} : Always import task definitions from trigger.dev/sdk, never from trigger.dev/sdk/v3 or deprecated client.defineJob pattern
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.tsinternal-packages/clickhouse/src/index.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use `trigger.dev/sdk/v3` for all imports in Trigger.dev tasks
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Configure Trigger.dev project in `trigger.config.ts` using `defineConfig()` with project ref and task directories
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use logger methods (debug, log, info, warn, error) from `trigger.dev/sdk/v3` for structured logging in tasks
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.tsinternal-packages/clickhouse/src/index.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use `tasks.trigger()` with type-only imports to trigger tasks from backend code without importing the task implementation
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.tsinternal-packages/clickhouse/src/index.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Define global lifecycle functions (onStart, onSuccess, onFailure) in trigger.config.ts to apply to all tasks
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.ts
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx
📚 Learning: 2025-07-12T18:06:04.133Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 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/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use `schedules.task()` for scheduled/cron tasks instead of regular `task()`
Applied to files:
internal-packages/clickhouse/src/index.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use lifecycle functions (init, cleanup, onStart, onSuccess, onFailure, handleError) for task event handling
Applied to files:
internal-packages/clickhouse/src/index.ts
🧬 Code graph analysis (4)
apps/webapp/app/presenters/v3/LogDetailPresenter.server.ts (1)
apps/webapp/app/v3/eventRepository/index.server.ts (1)
getConfiguredEventRepository(27-61)
internal-packages/clickhouse/src/taskEvents.ts (2)
internal-packages/clickhouse/src/client/types.ts (1)
ClickhouseReader(60-199)internal-packages/clickhouse/src/index.ts (1)
ClickHouseSettings(2-2)
apps/webapp/app/v3/eventRepository/index.server.ts (3)
apps/webapp/app/v3/eventRepository/eventRepository.types.ts (1)
IEventRepository(314-411)apps/webapp/app/v3/eventRepository/clickhouseEventRepositoryInstance.server.ts (2)
clickhouseEventRepositoryV2(11-14)clickhouseEventRepository(6-9)apps/webapp/app/v3/eventRepository/eventRepository.server.ts (1)
eventRepository(1441-1441)
internal-packages/clickhouse/src/index.ts (1)
internal-packages/clickhouse/src/taskEvents.ts (4)
getLogsListQueryBuilderV1(329-351)getLogDetailQueryBuilderV1(353-375)getLogsListQueryBuilderV2(257-279)getLogDetailQueryBuilderV2(301-323)
⏰ 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). (20)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
🔇 Additional comments (22)
apps/webapp/app/components/logs/LogsTable.tsx (1)
211-234: LGTM!The
BlankStatecomponent is well-structured with proper loading state handling. ThecolSpan=6correctly accounts for all table columns including the menu column.apps/webapp/app/v3/featureFlags.server.ts (1)
9-9: LGTM!The new
hasLogsPageAccessfeature flag follows the established pattern, usesz.coerce.boolean()consistent with thehasQueryAccessflag, and integrates properly with the existing feature flag infrastructure.Also applies to: 17-17
apps/webapp/app/v3/eventRepository/index.server.ts (1)
44-61: LGTM!The resolution logic is well-documented with clear priority ordering, and the store selection matches the patterns used in
getEventRepositoryandgetV3EventRepository.apps/webapp/app/presenters/v3/LogsListPresenter.server.ts (4)
171-177: LGTM!The class correctly extends
BasePresenterand callssuper()in the constructor. This aligns with the presenter pattern documented in learnings forapps/webapp/app/v3/presenters/**/*.server.ts.
368-383: Store-based query builder selection is well-implemented.The conditional logic correctly routes to the appropriate ClickHouse version. Note that the
repositoryfromgetConfiguredEventRepositoryis unused here since the presenter directly uses the ClickHouse query builders.
394-424: LGTM!The conditional
inserted_atPREWHERE clause correctly applies partition pruning only for the V2 table (which is partitioned byinserted_at), whilestart_timefiltering in WHERE is applied for both versions. The inline comments clearly explain the reasoning.
137-152: LGTM!The
levelToKindsAndStatusesfunction provides a clean mapping from display levels to ClickHouse query constraints. The logic correctly handles the different level types including the special cases for ERROR and CANCELLED which map to statuses.internal-packages/clickhouse/src/taskEvents.ts (3)
257-279: LGTM!The V2 query builders are correctly renamed and target the
task_events_v2table. The column selection includingattributes_textis consistent across both list and detail builders.Also applies to: 301-323
329-375: Theattributes_textcolumn exists intask_events_v1and is properly selected.Verification confirms that
attributes_textis a MATERIALIZED column in thetask_events_v1table schema (attributes_text String MATERIALIZED toJSONString(attributes)), and all columns selected by bothgetLogsListQueryBuilderV1andgetLogDetailQueryBuilderV1are present in the schema. The queries are correct and will function as intended.
238-253: Schema change frommetadata/attributestoattributes_textis properly handled.The
LogsListResultschema has been updated to useattributes_textinstead of separate metadata and attributes fields. This is a breaking change, but verification confirms the only consumer—LogsListPresenter—has already been updated to correctly accessattributes_textrather than the old fields.internal-packages/clickhouse/src/index.ts (2)
26-29: LGTM!The imports are well-organized, clearly versioned, and properly separate V1 and V2 query builders for the respective task event paths.
209-228: LGTM!The version alignment is consistent:
taskEventsuses V1 builders targetingtask_events_v1table, whiletaskEventsV2uses V2 builders targetingtask_events_v2table. Settings are correctly passed through for both paths.apps/webapp/app/presenters/v3/LogDetailPresenter.server.ts (3)
5-6: LGTM!Proper imports for organization-based store configuration and error handling.
29-43: LGTM!The store detection and query builder selection logic is correct:
- PostgreSQL throws a clear error since logs aren't supported there
- V2 store uses V2 builder, otherwise falls back to V1 builder
The conditional branching is clean and maintainable.
87-91: LGTM!Correctly parses
attributes_textas the source for attributes data, aligning with the updated schema.apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.tsx (2)
10-10: LGTM!Correct import for error handling.
47-62: LGTM!The error handling pattern is clean and appropriate:
ServiceValidationErrormaps to HTTP 400 (bad request) with the error message- Other errors are re-thrown to preserve stack traces and allow upstream error handling
- This aligns with the presenter's store validation logic
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx (4)
3-3: LGTM!Correct import for error handling in the loader.
126-140: LGTM!The error handling pattern correctly catches
ServiceValidationErrorand transforms it into a structured error object that the UI can handle gracefully. Other errors are re-thrown to preserve error boundaries.
195-215: LGTM!Clean error-first rendering pattern that checks for the error object and displays a user-friendly callout before attempting to render the logs list.
230-230: LGTM!Good use of TypeScript's
Excludeutility type to ensure type safety by removing the error case from the list prop type, since error cases are handled separately in the parent component.apps/webapp/app/components/navigation/SideMenu.tsx (1)
272-281: LGTM!The visibility condition follows the established pattern used for the Query menu item (line 289) and correctly gates the Logs menu item behind:
- Admin access, or
- Impersonation, or
- The
hasLogsPageAccessfeature flagThis is consistent with the codebase conventions.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
...p/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/webapp/app/presenters/v3/LogDetailPresenter.server.ts`:
- Around line 39-43: The comment incorrectly states "only v2 is supported for
log details" even though the code branches on isClickhouseV2 and calls
clickhouse.taskEventsV2.logDetailQueryBuilder() or
clickhouse.taskEvents.logDetailQueryBuilder(); update the inline comment to
reflect that both v1 and v2 are supported and that the code selects the
appropriate query builder based on the store (isClickhouseV2) rather than saying
only v2 is supported.
🧹 Nitpick comments (1)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx (1)
104-113: Minor formatting: add space afterif.The condition at line 111 is missing a space after
if.Suggested fix
- if(!canAccess) { + if (!canAccess) {
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webapp/app/presenters/v3/LogDetailPresenter.server.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects insteadEvery Trigger.dev task must be exported; use task() function with unique id and run async function
Files:
apps/webapp/app/presenters/v3/LogDetailPresenter.server.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/presenters/v3/LogDetailPresenter.server.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/app/presenters/v3/LogDetailPresenter.server.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webappIn webapp development, access environment variables via env export from apps/webapp/app/env.server.ts; never use process.env directly
Files:
apps/webapp/app/presenters/v3/LogDetailPresenter.server.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Files:
apps/webapp/app/presenters/v3/LogDetailPresenter.server.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx
**/*.{js,ts,jsx,tsx,json,md,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier
Files:
apps/webapp/app/presenters/v3/LogDetailPresenter.server.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
apps/webapp/app/presenters/v3/LogDetailPresenter.server.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: Import from@trigger.dev/coreusing subpaths only; never import from root of@trigger.dev/core
Always import task definitions from@trigger.dev/sdk, never from@trigger.dev/sdk/v3or deprecated client.defineJob pattern
Files:
apps/webapp/app/presenters/v3/LogDetailPresenter.server.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx
🧠 Learnings (14)
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/app/v3/presenters/**/*.server.{ts,tsx} : Organize presenters in the webapp following the pattern `app/v3/presenters/*/*.server.ts` to move complex loader code into classes
Applied to files:
apps/webapp/app/presenters/v3/LogDetailPresenter.server.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Configure OpenTelemetry instrumentations and exporters in trigger.config.ts for enhanced logging
Applied to files:
apps/webapp/app/presenters/v3/LogDetailPresenter.server.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use `schemaTask()` from `trigger.dev/sdk/v3` with Zod schema for payload validation
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use the `task()` function from `trigger.dev/sdk/v3` to define tasks with id and run properties
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Attach metadata to task runs using the metadata option when triggering, and access/update it inside runs using metadata functions
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.ts
📚 Learning: 2026-01-12T11:01:34.792Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:01:34.792Z
Learning: Applies to **/*.{ts,tsx} : Every Trigger.dev task must be exported; use task() function with unique id and run async function
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.ts
📚 Learning: 2026-01-12T11:01:34.792Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:01:34.792Z
Learning: Applies to **/*.{ts,tsx,js} : Always import task definitions from trigger.dev/sdk, never from trigger.dev/sdk/v3 or deprecated client.defineJob pattern
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use `trigger.dev/sdk/v3` for all imports in Trigger.dev tasks
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Configure Trigger.dev project in `trigger.config.ts` using `defineConfig()` with project ref and task directories
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use logger methods (debug, log, info, warn, error) from `trigger.dev/sdk/v3` for structured logging in tasks
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use `tasks.trigger()` with type-only imports to trigger tasks from backend code without importing the task implementation
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Define global lifecycle functions (onStart, onSuccess, onFailure) in trigger.config.ts to apply to all tasks
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.ts
📚 Learning: 2025-12-08T15:19:56.823Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2760
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx:278-281
Timestamp: 2025-12-08T15:19:56.823Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx, the tableState search parameter uses intentional double-encoding: the parameter value contains a URL-encoded URLSearchParams string, so decodeURIComponent(value("tableState") ?? "") is required to fully decode it before parsing with new URLSearchParams(). This pattern allows bundling multiple filter/pagination params as a single search parameter.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx
📚 Learning: 2025-07-12T18:06:04.133Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 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/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx
🧬 Code graph analysis (3)
apps/webapp/app/presenters/v3/LogDetailPresenter.server.ts (2)
apps/webapp/app/v3/eventRepository/index.server.ts (1)
getConfiguredEventRepository(27-61)apps/webapp/app/v3/otlpExporter.server.ts (1)
store(113-123)
apps/webapp/app/presenters/v3/LogsListPresenter.server.ts (5)
apps/webapp/app/utils/logUtils.ts (1)
LogLevel(5-5)apps/webapp/app/models/runtimeEnvironment.server.ts (2)
displayableEnvironment(257-277)findDisplayableEnvironment(279-310)apps/webapp/app/v3/eventRepository/index.server.ts (1)
getConfiguredEventRepository(27-61)apps/webapp/app/v3/eventRepository/common.server.ts (1)
convertDateToNanoseconds(53-55)apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts (1)
convertDateToClickhouseDateTime(1931-1934)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx (4)
apps/webapp/app/v3/featureFlags.server.ts (1)
validateFeatureFlagValue(93-98)apps/webapp/app/utils/pathBuilder.ts (1)
EnvironmentParamSchema(26-28)apps/webapp/app/services/preferences/uiPreferences.server.ts (1)
setRootOnlyFilterPreference(40-44)apps/webapp/app/presenters/v3/LogsListPresenter.server.ts (1)
LogsList(99-99)
⏰ 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 (6, 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 (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 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 (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- 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 - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (15)
apps/webapp/app/presenters/v3/LogsListPresenter.server.ts (5)
11-23: LGTM - Imports are correctly organized.The new imports for
getConfiguredEventRepository,kindToLevel,LogLevel,LogLevelSchema, andBasePresenterare properly structured and use subpath exports as required by the coding guidelines.
171-177: LGTM - Class properly extends BasePresenter.The
LogsListPresenternow correctly extendsBasePresenterwith thesuper()call in the constructor, following the presenter pattern established in the codebase.
368-383: Store selection logic is well-structured.The dynamic query builder selection based on
storetype is clean. The error for PostgreSQL provides a clear message for unsupported configurations.
394-424: Time filtering correctly differentiates v1 and v2 behavior.The
inserted_atpartition pruning is correctly applied only for ClickHouse v2, whilestart_timefiltering is applied for both versions. This ensures backward compatibility with v1 while leveraging v2's partition optimization.
545-555: No action required —attributes_textis consistently available in both v1 and v2 query results. BothgetLogsListQueryBuilderV1andgetLogsListQueryBuilderV2selectattributes_textin their columns, and theLogsListResultschema properly defines it as a required string field.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx (5)
61-93: Access control implementation looks correct and addresses the previous review concern.The
hasLogsPageAccessfunction properly checks admin/impersonation status first, then falls back to organization feature flags. The loader now correctly calls this function.
135-148: Error handling pattern is well-implemented.The
.catch()block correctly catchesServiceValidationErrorand returns an error object instead of throwing, allowing graceful degradation in the UI. Other errors are re-thrown appropriately.
203-223: Error-first rendering handles the deferred error case correctly.The
"error" in resultcheck properly discriminates between success and error states before passing data toLogsList.
238-238: Type narrowing withExcludeis the correct approach.Using
Exclude<..., { error: string }>ensures type safety by removing the error variant from the union type after the error check.
346-354: No issue found:RunsFilterscomponent accepts thehasFiltersprop.The
RunsFilterscomponent includeshasFilters: boolean;in its type definition (line 326 ofapps/webapp/app/components/runs/v3/RunFilters.tsx), and the prop is used throughout the component's logic. The code correctly passes this prop.apps/webapp/app/presenters/v3/LogDetailPresenter.server.ts (5)
1-6: Imports look good.The new imports for
getConfiguredEventRepositoryandServiceValidationErrorare correctly added to support the organization-aware repository selection pattern.
29-37: Store detection and error handling are correctly implemented.The pattern aligns with
getConfiguredEventRepositoryusage in other parts of the codebase. ThrowingServiceValidationErrorfor PostgreSQL is appropriate since the logs page requires ClickHouse.
45-58: Query filters are correctly implemented.The combination of
spanId,traceId, andstartTimeuniquely identifies the log event, and the comment properly documents the reasoning.
88-105: Return object structure is well-implemented.The ID format using
::separator maintains consistency withLogsListPresenter. The safe duration conversion handles potential type variations from ClickHouse responses.
78-86: No issues found. Both ClickHouse v1 and v2 query builders return the sameLogDetailV2Resultschema, which explicitly definesattributes_text: z.string(). The code correctly assumesattributes_textexists in both versions, and the try-catch pattern appropriately handles any JSON parsing errors.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
Changelog
task_events_v1ortask_events_v2postgresTesting
To change between where the query looks for logs there is the following logic
The feature flags are:
{"hasLogsPageAccess": true}{"taskEventRepository": "clickhouse_v2"}The accepted values are for
taskEventRepository:postgres|clickhouse|clickhouse_v2Changing the taskEventRepository value in DB is reflected instantly in the application.