logs-page-fixes #2889
logs-page-fixes #2889
Conversation
added support for tasks_v1 for logs cleaned the No logs message extended LogsPresenter with BasePresenter for spans
|
WalkthroughThis PR introduces multi-backend event storage support by adding organization-based repository configuration that routes between ClickHouse V1, ClickHouse V2, and PostgreSQL. It renames and versions existing log query builders into V1 and V2 variants, updates the underlying data schema from metadata/attributes fields to a unified attributes_text field, implements error handling for unsupported backends, and gates logs page access through a new feature flag. The changes span presenters, routes, utility functions, and the ClickHouse query builder library. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Detailed notesThe review complexity stems from:
🚥 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
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.
| {logs.length === 0 ? ( | ||
| <BlankState isLoading={isLoading} onRefresh={() => window.location.reload()} /> | ||
| ) : ( |
There was a problem hiding this comment.
Empty state message may be inaccurate when no filters are applied.
Since hasFilters was removed, the message "No logs match your filters. Try refreshing or modifying your filters." displays even when no filters are applied. This could confuse users who haven't set any filters.
Consider differentiating the message based on whether filters are actually applied, or use a more generic message like "No logs found."
🤖 Prompt for AI Agents
In `@apps/webapp/app/components/logs/LogsTable.tsx` around lines 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.
| async function hasLogsPageAccess( | ||
| userId: string, | ||
| isAdmin: boolean, | ||
| isImpersonating: boolean, | ||
| organizationSlug: string | ||
| ): Promise<boolean> { | ||
| if (isAdmin || isImpersonating) { | ||
| return true; | ||
| } | ||
|
|
||
| // Check organization feature flags | ||
| const organization = await prisma.organization.findFirst({ | ||
| where: { | ||
| slug: organizationSlug, | ||
| members: { some: { userId } }, | ||
| }, | ||
| select: { | ||
| featureFlags: true, | ||
| }, | ||
| }); | ||
|
|
||
| if (!organization?.featureFlags) { | ||
| return false; | ||
| } | ||
|
|
||
| const flags = organization.featureFlags as Record<string, unknown>; | ||
| const hasLogsPageAccessResult = validateFeatureFlagValue( | ||
| FEATURE_FLAG.hasLogsPageAccess, | ||
| flags.hasLogsPageAccess | ||
| ); | ||
|
|
||
| return hasLogsPageAccessResult.success && hasLogsPageAccessResult.data === true; | ||
| } |
There was a problem hiding this comment.
hasLogsPageAccess is defined but never called.
This helper function is implemented but not used anywhere in the loader. The loader at lines 101-103 still only checks isAdmin and redirects non-admins to /:
if (!isAdmin) {
throw redirect("/");
}The feature flag check via hasLogsPageAccess is bypassed entirely. If the intent is to gate access based on the hasLogsPageAccess feature flag (as done in SideMenu.tsx), this function should be called in the loader.
Suggested fix
const isAdmin = user.admin || user.isImpersonating;
- if (!isAdmin) {
- throw redirect("/");
- }
+ const hasAccess = await hasLogsPageAccess(
+ userId,
+ user.admin,
+ user.isImpersonating,
+ organizationSlug
+ );
+
+ if (!hasAccess) {
+ throw redirect("/");
+ }🤖 Prompt for AI Agents
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.logs/route.tsx
around lines 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.
Closes #
✅ Checklist
Testing
[Describe the steps you took to test this change]
Changelog
[Short description of what has changed]
Screenshots
[Screenshots]
💯