Conversation
matt-aitken
commented
Mar 5, 2026
- Added versions filtering on the Errors list and page
- Added errors stacked bars to the graph on the individual error page
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds end-to-end error-group versioning, filtering, and alerting support. Frontend: new LogsVersionFilter, exported VersionsDropdown, status/version filters, chart legend class prop, ConfigureErrorAlerts UI, alert configuration routes, and storybook UnorderedList. Backend: presenters accept versions/statuses, ErrorGroupState types, ErrorGroupActions, DeliverErrorGroupAlertService, ErrorAlertEvaluator, alert worker tasks, and alert-channel presenter. ClickHouse: new per-version and "since" query builders. Database: Prisma/schema and SQL migrations for ErrorGroupState and alert config. Email template and email client integration for error-group alerts. Estimated code review effort🎯 5 (Critical) | ⏱️ ~180 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🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cd9b5ea to
e609e8b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
6875ac8 to
497f301
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
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/ErrorGroupPresenter.server.ts (1)
79-89:⚠️ Potential issue | 🔴 CriticalInitialize
stateingetSummary()now that it is required.
ErrorGroupSummarynow requiresstate, butgetSummary()still returns an object without that field. This breaks the file’s TypeScript contract beforecall()gets a chance to overwritesummary.state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/presenters/v3/ErrorGroupPresenter.server.ts` around lines 79 - 89, getSummary() currently returns an object missing the required state field of ErrorGroupSummary; update getSummary() to include a default state value (e.g., ErrorGroupState.Open or the appropriate ErrorGroupState enum member) in the returned summary object so the returned type satisfies ErrorGroupSummary before call() may overwrite summary.state; locate the getSummary function in ErrorGroupPresenter.server.ts and add the state property to the returned object referencing ErrorGroupState.
♻️ Duplicate comments (2)
apps/webapp/app/presenters/v3/ErrorGroupPresenter.server.ts (1)
133-148:⚠️ Potential issue | 🟠 MajorState lookup still needs
taskIdentifier.
ErrorGroupStateis keyed by(environmentId, taskIdentifier, errorFingerprint), but this code still fetches it with only(environmentId, errorFingerprint)and does so beforesummary.taskIdentifieris known. If the same fingerprint exists under multiple tasks, the page can surface the wrong resolved/ignored state. Fetchsummaryfirst, then look up state via the full composite key (findUniquefits here).Also applies to: 324-332
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/presenters/v3/ErrorGroupPresenter.server.ts` around lines 133 - 148, The state lookup is being called before the summary and without taskIdentifier; change the flow in ErrorGroupPresenter.server.ts so you await this.getSummary(...) first, extract summary.taskIdentifier, then fetch state using the composite key (environmentId, taskIdentifier, fingerprint) — replace the current this.getState(environmentId, fingerprint) call with a lookup that uses the full composite key (use the repository's findUnique or equivalent to query by environmentId, taskIdentifier, and errorFingerprint). Apply the same fix to the other occurrence mentioned (lines around the second getState call).apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx (1)
80-98:⚠️ Potential issue | 🟠 MajorReject empty task identifiers and non-positive ignore thresholds server-side.
These branches still allow an empty
taskIdentifierand zero/negative ignore thresholds through toErrorGroupActions. That lets crafted POSTs createErrorGroupStaterows the UI cannot reliably match, or ignore conditions that never behave as intended. Tighten this schema with a trimmed non-empty identifier and positive numeric validators.As per coding guidelines, "Use zod for validation in packages/core and apps/webapp".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx around lines 80 - 98, The discriminated union actionSchema allows empty taskIdentifier and non-positive ignore thresholds; update the schema so taskIdentifier uses a trimmed non-empty string (e.g., z.string().trim().min(1)) in every branch (resolve, ignore, unresolve) and make the numeric ignore fields (duration, occurrenceRate, totalOccurrences) use coercion plus positive checks (e.g., z.coerce.number().positive().optional()) so zero/negative values are rejected; keep resolvedInVersion and reason optional but consider trimming them as well for consistency.
🧹 Nitpick comments (1)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx (1)
820-845: Normalize dotted version strings used as chart series keys.Version strings like
20260304.1are used directly as object keys in the chart config (cfg[versions[i]]). While JavaScript bracket notation handles this correctly and recharts typically checks for direct properties before nested path traversal, this pattern is fragile. Recharts' accessor implementation could change, or future maintainers might not realize why this works. Normalize to internal series IDs (e.g., array indices) and use version strings only as labels:cfg[i] = { label: versions[i], color: getSeriesColor(i), }Also applies to: 890-899
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx around lines 820 - 845, The chart currently uses dotted version strings as object keys (cfg[versions[i]]) which can be fragile for recharts accessors; in ActivityChart replace those string keys with numeric series IDs (use i as the key) so cfg[i] = { label: versions[i], color: getSeriesColor(i) } and update the data transformation so each activity point maps its series value to the same numeric ID instead of the dotted version string; make sure to keep versions[i] as the human-readable label and apply the same change to the other chart config block that builds cfg (the second occurrence that also references cfg, versions, getSeriesColor, and activity/data).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/webapp/app/presenters/v3/ErrorGroupPresenter.server.ts`:
- Around line 79-89: getSummary() currently returns an object missing the
required state field of ErrorGroupSummary; update getSummary() to include a
default state value (e.g., ErrorGroupState.Open or the appropriate
ErrorGroupState enum member) in the returned summary object so the returned type
satisfies ErrorGroupSummary before call() may overwrite summary.state; locate
the getSummary function in ErrorGroupPresenter.server.ts and add the state
property to the returned object referencing ErrorGroupState.
---
Duplicate comments:
In `@apps/webapp/app/presenters/v3/ErrorGroupPresenter.server.ts`:
- Around line 133-148: The state lookup is being called before the summary and
without taskIdentifier; change the flow in ErrorGroupPresenter.server.ts so you
await this.getSummary(...) first, extract summary.taskIdentifier, then fetch
state using the composite key (environmentId, taskIdentifier, fingerprint) —
replace the current this.getState(environmentId, fingerprint) call with a lookup
that uses the full composite key (use the repository's findUnique or equivalent
to query by environmentId, taskIdentifier, and errorFingerprint). Apply the same
fix to the other occurrence mentioned (lines around the second getState call).
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx:
- Around line 80-98: The discriminated union actionSchema allows empty
taskIdentifier and non-positive ignore thresholds; update the schema so
taskIdentifier uses a trimmed non-empty string (e.g., z.string().trim().min(1))
in every branch (resolve, ignore, unresolve) and make the numeric ignore fields
(duration, occurrenceRate, totalOccurrences) use coercion plus positive checks
(e.g., z.coerce.number().positive().optional()) so zero/negative values are
rejected; keep resolvedInVersion and reason optional but consider trimming them
as well for consistency.
---
Nitpick comments:
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx:
- Around line 820-845: The chart currently uses dotted version strings as object
keys (cfg[versions[i]]) which can be fragile for recharts accessors; in
ActivityChart replace those string keys with numeric series IDs (use i as the
key) so cfg[i] = { label: versions[i], color: getSeriesColor(i) } and update the
data transformation so each activity point maps its series value to the same
numeric ID instead of the dotted version string; make sure to keep versions[i]
as the human-readable label and apply the same change to the other chart config
block that builds cfg (the second occurrence that also references cfg, versions,
getSeriesColor, and activity/data).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d9f30fbf-5e21-4716-a3ce-d6e0559ba25e
📒 Files selected for processing (2)
apps/webapp/app/presenters/v3/ErrorGroupPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx
📜 Review details
⏰ 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). (16)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{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 instead
**/*.{ts,tsx}: Use task export syntax: export const myTask = task({ id: 'my-task', run: async (payload) => { ... } })
Use Run Engine 2.0 (@internal/run-engine) and redis-worker for all new work - avoid DEPRECATED zodworker (Graphile-worker wrapper)
Prisma 6.14.0 client and schema use PostgreSQL in internal-packages/database - import only from Prisma client
Files:
apps/webapp/app/presenters/v3/ErrorGroupPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/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/ErrorGroupPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/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/ErrorGroupPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/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 webapp
Files:
apps/webapp/app/presenters/v3/ErrorGroupPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/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/ErrorGroupPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/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/ErrorGroupPresenter.server.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/presenters/v3/ErrorGroupPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx
apps/webapp/**/*.server.ts
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Access environment variables via the
envexport fromapp/env.server.ts, never useprocess.envdirectly
Files:
apps/webapp/app/presenters/v3/ErrorGroupPresenter.server.ts
apps/{webapp,supervisor}/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
When modifying only server components (apps/webapp/, apps/supervisor/) with no package changes, add a .server-changes/ file instead of a changeset
Files:
apps/webapp/app/presenters/v3/ErrorGroupPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: Always import from@trigger.dev/sdkfor Trigger.dev tasks - never use@trigger.dev/sdk/v3or deprecated client.defineJob
Import subpaths only from@trigger.dev/core, never import from root
Add crumbs as you write code using //@crumbscomments or //#region@crumbsblocks for agentcrumbs debug tracing
Files:
apps/webapp/app/presenters/v3/ErrorGroupPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx
apps/webapp/**/*.{ts,tsx,jsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Remix 2.1.0 is used in apps/webapp for the main API, dashboard, and orchestration with Express server
Files:
apps/webapp/app/presenters/v3/ErrorGroupPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx
🧠 Learnings (22)
📚 Learning: 2026-01-08T15:57:09.323Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/otel-metrics.mdc:0-0
Timestamp: 2026-01-08T15:57:09.323Z
Learning: Applies to **/*.ts : 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)
Applied to files:
apps/webapp/app/presenters/v3/ErrorGroupPresenter.server.ts
📚 Learning: 2026-03-04T16:40:11.313Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3172
File: apps/webapp/app/utils/semver.ts:19-33
Timestamp: 2026-03-04T16:40:11.313Z
Learning: In `apps/webapp/app/utils/semver.ts`, the versioning scheme used is date-based (e.g. `20260304.1`, `20260304.2`), not semantic versioning. Pre-release suffix handling (e.g. `-beta`, `-rc`) is not a concern for this file.
Applied to files:
apps/webapp/app/presenters/v3/ErrorGroupPresenter.server.ts
📚 Learning: 2026-03-13T13:37:49.562Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-13T13:37:49.562Z
Learning: Applies to **/*.{ts,tsx} : Prisma 6.14.0 client and schema use PostgreSQL in internal-packages/database - import only from Prisma client
Applied to files:
apps/webapp/app/presenters/v3/ErrorGroupPresenter.server.ts
📚 Learning: 2025-11-27T16:26:37.432Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T16:26:37.432Z
Learning: Applies to internal-packages/database/**/*.{ts,tsx} : Use Prisma for database interactions in internal-packages/database with PostgreSQL
Applied to files:
apps/webapp/app/presenters/v3/ErrorGroupPresenter.server.ts
📚 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/presenters/v3/ErrorGroupPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx
📚 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: Leverage the PostgreSQL database through the `trigger.dev/database` Prisma 6.14.0 client in the webapp for all data access patterns
Applied to files:
apps/webapp/app/presenters/v3/ErrorGroupPresenter.server.ts
📚 Learning: 2026-02-06T19:53:38.843Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/presenters/v3/DeploymentListPresenter.server.ts:233-237
Timestamp: 2026-02-06T19:53:38.843Z
Learning: When constructing Vercel dashboard URLs from deployment IDs, always strip the dpl_ prefix from the ID. Implement this by transforming the ID with .replace(/^dpl_/, "") before concatenating into the URL: https://vercel.com/${teamSlug}/${projectName}/${cleanedDeploymentId}. Consider centralizing this logic in a small helper (e.g., getVercelDeploymentId(id) or a URL builder) and add tests to verify both prefixed and non-prefixed inputs.
Applied to files:
apps/webapp/app/presenters/v3/ErrorGroupPresenter.server.ts
📚 Learning: 2026-03-02T12:42:56.114Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:42:56.114Z
Learning: Applies to apps/webapp/app/routes/**/*.ts : Use Remix flat-file route convention with dot-separated segments (e.g., `api.v1.tasks.$taskId.trigger.ts` for `/api/v1/tasks/:taskId/trigger`)
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.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/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx
📚 Learning: 2026-02-04T16:34:48.876Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/vercel.connect.tsx:13-27
Timestamp: 2026-02-04T16:34:48.876Z
Learning: In apps/webapp/app/routes/vercel.connect.tsx, configurationId may be absent for "dashboard" flows but must be present for "marketplace" flows. Enforce this with a Zod superRefine and pass installationId to repository methods only when configurationId is defined (omit the field otherwise).
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx
📚 Learning: 2026-03-13T13:37:49.562Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-13T13:37:49.562Z
Learning: Applies to apps/webapp/**/*.{ts,tsx,jsx,js} : Remix 2.1.0 is used in apps/webapp for the main API, dashboard, and orchestration with Express server
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx
📚 Learning: 2026-03-13T13:45:39.411Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3213
File: apps/webapp/app/routes/admin.llm-models.missing.$model.tsx:19-21
Timestamp: 2026-03-13T13:45:39.411Z
Learning: In `apps/webapp/app/routes/admin.llm-models.missing.$model.tsx`, the `decodeURIComponent(params.model ?? "")` call is intentionally unguarded. Remix route params are decoded at the routing layer before reaching the loader, so malformed percent-encoding is rejected upstream. The page is also admin-only, so the risk is minimal and no try-catch is warranted.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx
📚 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.errors.$fingerprint/route.tsx
📚 Learning: 2026-02-11T16:50:14.167Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3019
File: apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.$dashboardId.widgets.tsx:126-131
Timestamp: 2026-02-11T16:50:14.167Z
Learning: In apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.$dashboardId.widgets.tsx, MetricsDashboard entities are intentionally scoped to the organization level, not the project level. The dashboard lookup should filter by organizationId only (not projectId), allowing dashboards to be accessed across projects within the same organization. The optional projectId field on MetricsDashboard serves other purposes and should not be used as an authorization constraint.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx
📚 Learning: 2026-03-13T13:42:59.104Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3213
File: apps/webapp/app/routes/admin.api.v1.llm-models.$modelId.ts:40-43
Timestamp: 2026-03-13T13:42:59.104Z
Learning: In `apps/webapp/app/routes/admin.api.v1.llm-models.$modelId.ts` and `apps/webapp/app/routes/admin.api.v1.llm-models.ts`, the `startDate` field in `UpdateModelSchema` and `CreateModelSchema` intentionally uses `z.string().optional()` (or `.nullable().optional()`) without strict ISO datetime validation. Invalid date strings are rejected at the Prisma/DB layer. This is acceptable because these are admin-only API routes protected by Personal Access Token (PAT) authentication and are not user-facing.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/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 `schemaTask()` from `trigger.dev/sdk/v3` with Zod schema for payload validation
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx
📚 Learning: 2025-11-27T16:26:37.432Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T16:26:37.432Z
Learning: Applies to {packages/core,apps/webapp}/**/*.{ts,tsx} : Use zod for validation in packages/core and apps/webapp
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/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} : 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/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx
📚 Learning: 2025-07-12T18:00:06.163Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 2264
File: apps/webapp/app/utils/searchParams.ts:16-18
Timestamp: 2025-07-12T18:00:06.163Z
Learning: The `objectToSearchParams` function in `apps/webapp/app/utils/searchParams.ts` is used to generate URL parameters from objects and is separate from code that parses incoming search parameters. Changes to this function only affect places where it's used to create URLs, not places that parse search parameters from external sources.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx
📚 Learning: 2026-01-28T14:15:15.011Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 2953
File: apps/webapp/app/components/runs/v3/SharedFilters.tsx:441-452
Timestamp: 2026-01-28T14:15:15.011Z
Learning: In apps/webapp/app/components/runs/v3/SharedFilters.tsx, the maxPeriodDays limit for date ranges should only check the from date (via dateRangeToDays(fromValue)) because it enforces data retention limits—how far back in history queries can reach. The to date is irrelevant for retention-based limits.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx
📚 Learning: 2026-02-03T18:27:40.429Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx:553-555
Timestamp: 2026-02-03T18:27:40.429Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx, the menu buttons (e.g., Edit with PencilSquareIcon) in the TableCellMenu are intentionally icon-only with no text labels as a compact UI pattern. This is a deliberate design choice for this route; preserve the icon-only behavior for consistency in this file.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx
📚 Learning: 2026-02-11T16:37:32.429Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3019
File: apps/webapp/app/components/primitives/charts/Card.tsx:26-30
Timestamp: 2026-02-11T16:37:32.429Z
Learning: In projects using react-grid-layout, avoid relying on drag-handle class to imply draggability. Ensure drag-handle elements only affect dragging when the parent grid item is configured draggable in the layout; conditionally apply cursor styles based on the draggable prop. This improves correctness and accessibility.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
7f30042 to
c31d639
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
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/ErrorGroupPresenter.server.ts (1)
79-89:⚠️ Potential issue | 🔴 Critical
getSummary()return type mismatch: missing requiredstatefield.The return statement at lines 283–292 omits the required
state: ErrorGroupStateproperty declared in theErrorGroupSummarytype. TypeScript will report a type error here. Either initializestateingetSummary()using the fallback state object already defined at lines 153–166, or markstateoptional in the type definition and require it only aftercall()enriches the summary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/presenters/v3/ErrorGroupPresenter.server.ts` around lines 79 - 89, getSummary() currently returns an object missing the required state property from the ErrorGroupSummary type; either set the state field on the returned summary to the existing fallback state object (the fallback ErrorGroupState defined near the top of the file) by adding state: fallbackErrorGroupState to the summary returned by getSummary(), or change the ErrorGroupSummary type to make state optional (state?: ErrorGroupState) so callers that enrich via call() can provide it later—locate getSummary(), ErrorGroupSummary, and the fallback ErrorGroupState symbol names to implement the chosen fix.
🧹 Nitpick comments (6)
apps/webapp/app/v3/services/alerts/errorGroupWebhook.server.ts (1)
6-32: Use a type alias forErrorGroupAlertData.This exported payload shape should follow the repo’s TS rule to prefer
typeoverinterface.As per coding guidelines,
**/*.{ts,tsx}: Use types over interfaces for TypeScript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/v3/services/alerts/errorGroupWebhook.server.ts` around lines 6 - 32, Replace the exported interface ErrorGroupAlertData with an equivalent exported type alias named ErrorGroupAlertData; keep the exact same property shape (including nested error, organization, project, and dashboardUrl) but use the type ... = { ... } syntax to satisfy the repo rule preferring types over interfaces.apps/webapp/test/slackErrorAlerts.test.ts (1)
160-383: These cases still only assert “no throw”.Every test ends with
expect(true).toBe(true), so regressions in block composition, truncation, or wording will still pass as long as Slack accepts the request. If these are meant to stay manual visual checks, mark them that way; otherwise assert on the generated Slack payload before delivery.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/test/slackErrorAlerts.test.ts` around lines 160 - 383, Tests currently only assert no-throw (expect(true).toBe(true)); update each case to either mark them as manual/visual (e.g., use test.skip or add a clear comment) or add assertions against the generated Slack payload by stubbing/mocking the delivery path: capture the payload produced by DeliverErrorGroupAlertService.call (or the underlying method that formats the Slack message) when invoked with createMockErrorPayload inputs and assert expected fields (presence of classification text, stack truncation notice for long messages, correct taskIdentifier/errorType, and special character preservation) before the actual send; reference DeliverErrorGroupAlertService, createMockErrorPayload and the formatter/delivery method to locate where to intercept and assert.apps/webapp/test/webhookErrorAlerts.test.ts (1)
1-5: Keep this as a colocated unit test and drop the Prisma setup.The assertions only exercise
generateErrorGroupWebhookPayload()andWebhook.parse(), so the org/project setup is just DB churn. Fixed ids/slugs are enough here, and this suite will be easier to maintain next to the webhook generator it covers.As per coding guidelines,
Test files should live beside the files under test and use descriptive describe and it blocks.Also applies to: 92-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/test/webhookErrorAlerts.test.ts` around lines 1 - 5, This test should be converted into a pure colocated unit test: remove all Prisma setup/teardown and any imports/usage of prisma or getSecretStore, keep the file next to the webhook generator, and replace dynamic org/project DB setup with fixed test values (fixed ids/slugs) used as inputs to DeliverErrorGroupAlertService.generateErrorGroupWebhookPayload() and Webhook.parse(); ensure the test only imports and exercises generateErrorGroupWebhookPayload() and Webhook.parse(), and rename the describe/test blocks to a descriptive "describe/it" style that reflects it’s a unit test for the webhook payload generator.apps/webapp/app/v3/services/alerts/deliverErrorGroupAlert.server.ts (2)
282-285: Consider exposing Slack'sretryAfterfor smarter retry backoff.The
WebAPIRateLimitedErrorincludes aretryAftervalue (seconds). Exposing this in the error or as a custom error type could help the worker implement proper backoff rather than immediate retry.♻️ Optional: Include retryAfter in error
if (isWebAPIRateLimitedError(error)) { - throw new Error("Slack rate limited"); + const rateLimitError = error as WebAPIRateLimitedError; + throw new Error(`Slack rate limited, retry after ${rateLimitError.retryAfter}s`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/v3/services/alerts/deliverErrorGroupAlert.server.ts` around lines 282 - 285, When handling Slack rate limit errors inside deliverErrorGroupAlert (the catch that checks isWebAPIRateLimitedError(error)), extract the retryAfter value from the WebAPIRateLimitedError and throw a custom or enhanced error that includes that value (e.g., SlackRateLimitedError with a retryAfter property or attach retryAfter to the thrown Error). Update the throw in the isWebAPIRateLimitedError branch to include the numeric retryAfter seconds from the Slack error so callers (the worker) can apply proper backoff based on that field.
1-8: Remove unused imports.
WebAPIHTTPErrorandWebAPIRequestErrorare imported but not used anywhere in this file.♻️ Remove unused imports
import { type ChatPostMessageArguments, ErrorCode, - type WebAPIHTTPError, type WebAPIPlatformError, type WebAPIRateLimitedError, - type WebAPIRequestError, } from "@slack/web-api";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/v3/services/alerts/deliverErrorGroupAlert.server.ts` around lines 1 - 8, The import list at the top currently includes unused symbols WebAPIHTTPError and WebAPIRequestError; update the import statement that brings in ChatPostMessageArguments, ErrorCode, WebAPIHTTPError, WebAPIPlatformError, WebAPIRateLimitedError, WebAPIRequestError to remove WebAPIHTTPError and WebAPIRequestError so only used types (e.g., ChatPostMessageArguments, ErrorCode, WebAPIPlatformError, WebAPIRateLimitedError) remain; ensure no other references to those removed types exist in functions like deliverErrorGroupAlert or any helper functions before committing.apps/webapp/app/v3/services/alerts/errorAlertEvaluator.server.ts (1)
394-420: Consider parallelizing state updates for better throughput.The sequential
awaitinside the loop works correctly but may become a bottleneck if many errors transition toUNRESOLVEDin a single evaluation. Consider usingPromise.allor a batch update if performance becomes a concern.♻️ Optional: Parallelize updates
private async updateErrorGroupStates( alertableErrors: AlertableError[], stateMap: Map<string, ErrorGroupState> ): Promise<void> { - for (const alertable of alertableErrors) { + const updates = alertableErrors.map(async (alertable) => { const key = `${alertable.error.environment_id}:${alertable.error.task_identifier}:${alertable.error.error_fingerprint}`; const state = stateMap.get(key); - if (!state) continue; + if (!state) return; await this._prisma.errorGroupState.update({ - where: { id: state.id }, - data: { - status: "UNRESOLVED", - ignoredUntil: null, - ignoredUntilOccurrenceRate: null, - ignoredUntilTotalOccurrences: null, - ignoredAtOccurrenceCount: null, - ignoredAt: null, - ignoredReason: null, - ignoredByUserId: null, - resolvedAt: null, - resolvedInVersion: null, - resolvedBy: null, - }, - }); - } + where: { id: state.id }, + data: { + status: "UNRESOLVED", + ignoredUntil: null, + ignoredUntilOccurrenceRate: null, + ignoredUntilTotalOccurrences: null, + ignoredAtOccurrenceCount: null, + ignoredAt: null, + ignoredReason: null, + ignoredByUserId: null, + resolvedAt: null, + resolvedInVersion: null, + resolvedBy: null, + }, + }); + }); + + await Promise.all(updates); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/v3/services/alerts/errorAlertEvaluator.server.ts` around lines 394 - 420, The loop in updateErrorGroupStates performs sequential awaits on this._prisma.errorGroupState.update which can be slow; change it to collect update promises for each matching alertable (use the same key logic with alertableErrors and stateMap) and run them in parallel with await Promise.all(promises) (or use a single batch/transaction where appropriate), calling this._prisma.errorGroupState.update for each state but not awaiting inside the for-loop so throughput improves.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/webapp/app/presenters/v3/ErrorsListPresenter.server.ts`:
- Around line 235-245: The IN/NOT IN clauses built with queryBuilder are using
parentheses around the ClickHouse parameter placeholders which breaks runtime
substitution; in the code paths that check statusFilter.includeKeys and
statusFilter.excludeKeys (the queryBuilder.where calls referencing
"concat(task_identifier, '::', error_fingerprint)"), remove the surrounding
parentheses so the clauses use the same pattern as other IN usages (i.e., change
"IN ({statusIncludeKeys: Array(String)})" to "IN {statusIncludeKeys:
Array(String)}" and "NOT IN ({statusExcludeKeys: Array(String)})" to "NOT IN
{statusExcludeKeys: Array(String)}" while keeping the parameter objects
statusIncludeKeys/statusExcludeKeys).
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.errors._index/route.tsx:
- Around line 407-411: The hasFilters check (variable hasFilters) ignores
time-related query params so a custom period/from/to doesn't count as an active
filter; update the expression that computes hasFilters to also check
searchParams.has("period"), searchParams.has("from"), and searchParams.has("to")
(in the same boolean OR chain) so the "Clear all filters" UI appears when only
time params are set.
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx:
- Around line 767-771: The dialog is being closed unconditionally in the
fetcher.Form onSubmit handler (setTimeout(onClose, 100)) which discards input
even when the POST returns validation errors; remove that onSubmit close and
instead watch fetcher.data for a successful response (check fetcher.data?.ok ===
true) and call onClose() only when that condition becomes true (e.g., use an
effect that depends on fetcher.data to call onClose()). Ensure the fetcher.Form
submit stays intact and reference the existing fetcher.Form, onSubmit handler,
onClose callback, and fetcher.data in the change.
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.errors/route.tsx:
- Around line 97-127: The current action unconditionally creates email and Slack
channels from the posted emails[] and slackChannel even when those UI sections
are not editable; before creating channels in the emails loop and before the
slackChannel block, verify the corresponding availability/editable flag (the
same check used to render the UI, e.g., inspect editableTypes or the feature
availability predicate used elsewhere) and skip creation if that channel type is
not allowed; ensure the check happens prior to constructing
CreateAlertChannelOptions and calling service.call(project.externalRef, userId,
...), mirroring the UI logic so only permitted email (emails) and Slack
(slackChannel + slackIntegrationId) channels are upserted.
In `@apps/webapp/app/v3/services/alerts/errorGroupWebhook.server.ts`:
- Around line 51-52: The code constructs dates using new
Date(data.error.firstSeen) and new Date(data.error.lastSeen) where
firstSeen/lastSeen are millisecond-formatted strings; convert those string
timestamps to numbers before calling Date to avoid Invalid Date/JSON nulls.
Update the new Date calls in errorGroupWebhook.server.ts (the places referencing
data.error.firstSeen and data.error.lastSeen) to use
Number(data.error.firstSeen) and Number(data.error.lastSeen) (or parseInt) so
the Date constructor receives numeric milliseconds.
In `@apps/webapp/test/slackErrorAlerts.test.ts`:
- Around line 93-107: The test creates a live secret and DB metadata (using
secretStore.setSecret with secretKey and prisma.secretReference.create) but
never cleans them up; add teardown logic in afterAll to remove both the
persisted secret and its DB record: call the secret store deletion for secretKey
via secretStore.deleteSecret (or the store's remove API) and delete the created
secretReference by id via prisma.secretReference.delete({ where: { id:
<the-created-id> } }) (ensure you capture the created secretReference id when
creating it in the test so teardown can reference it); apply the same cleanup
for the other instance noted at lines ~138-157.
---
Outside diff comments:
In `@apps/webapp/app/presenters/v3/ErrorGroupPresenter.server.ts`:
- Around line 79-89: getSummary() currently returns an object missing the
required state property from the ErrorGroupSummary type; either set the state
field on the returned summary to the existing fallback state object (the
fallback ErrorGroupState defined near the top of the file) by adding state:
fallbackErrorGroupState to the summary returned by getSummary(), or change the
ErrorGroupSummary type to make state optional (state?: ErrorGroupState) so
callers that enrich via call() can provide it later—locate getSummary(),
ErrorGroupSummary, and the fallback ErrorGroupState symbol names to implement
the chosen fix.
---
Nitpick comments:
In `@apps/webapp/app/v3/services/alerts/deliverErrorGroupAlert.server.ts`:
- Around line 282-285: When handling Slack rate limit errors inside
deliverErrorGroupAlert (the catch that checks isWebAPIRateLimitedError(error)),
extract the retryAfter value from the WebAPIRateLimitedError and throw a custom
or enhanced error that includes that value (e.g., SlackRateLimitedError with a
retryAfter property or attach retryAfter to the thrown Error). Update the throw
in the isWebAPIRateLimitedError branch to include the numeric retryAfter seconds
from the Slack error so callers (the worker) can apply proper backoff based on
that field.
- Around line 1-8: The import list at the top currently includes unused symbols
WebAPIHTTPError and WebAPIRequestError; update the import statement that brings
in ChatPostMessageArguments, ErrorCode, WebAPIHTTPError, WebAPIPlatformError,
WebAPIRateLimitedError, WebAPIRequestError to remove WebAPIHTTPError and
WebAPIRequestError so only used types (e.g., ChatPostMessageArguments,
ErrorCode, WebAPIPlatformError, WebAPIRateLimitedError) remain; ensure no other
references to those removed types exist in functions like deliverErrorGroupAlert
or any helper functions before committing.
In `@apps/webapp/app/v3/services/alerts/errorAlertEvaluator.server.ts`:
- Around line 394-420: The loop in updateErrorGroupStates performs sequential
awaits on this._prisma.errorGroupState.update which can be slow; change it to
collect update promises for each matching alertable (use the same key logic with
alertableErrors and stateMap) and run them in parallel with await
Promise.all(promises) (or use a single batch/transaction where appropriate),
calling this._prisma.errorGroupState.update for each state but not awaiting
inside the for-loop so throughput improves.
In `@apps/webapp/app/v3/services/alerts/errorGroupWebhook.server.ts`:
- Around line 6-32: Replace the exported interface ErrorGroupAlertData with an
equivalent exported type alias named ErrorGroupAlertData; keep the exact same
property shape (including nested error, organization, project, and dashboardUrl)
but use the type ... = { ... } syntax to satisfy the repo rule preferring types
over interfaces.
In `@apps/webapp/test/slackErrorAlerts.test.ts`:
- Around line 160-383: Tests currently only assert no-throw
(expect(true).toBe(true)); update each case to either mark them as manual/visual
(e.g., use test.skip or add a clear comment) or add assertions against the
generated Slack payload by stubbing/mocking the delivery path: capture the
payload produced by DeliverErrorGroupAlertService.call (or the underlying method
that formats the Slack message) when invoked with createMockErrorPayload inputs
and assert expected fields (presence of classification text, stack truncation
notice for long messages, correct taskIdentifier/errorType, and special
character preservation) before the actual send; reference
DeliverErrorGroupAlertService, createMockErrorPayload and the formatter/delivery
method to locate where to intercept and assert.
In `@apps/webapp/test/webhookErrorAlerts.test.ts`:
- Around line 1-5: This test should be converted into a pure colocated unit
test: remove all Prisma setup/teardown and any imports/usage of prisma or
getSecretStore, keep the file next to the webhook generator, and replace dynamic
org/project DB setup with fixed test values (fixed ids/slugs) used as inputs to
DeliverErrorGroupAlertService.generateErrorGroupWebhookPayload() and
Webhook.parse(); ensure the test only imports and exercises
generateErrorGroupWebhookPayload() and Webhook.parse(), and rename the
describe/test blocks to a descriptive "describe/it" style that reflects it’s a
unit test for the webhook payload generator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1d4ea286-3ad8-4bf3-b751-ebc5f81426b2
📒 Files selected for processing (36)
apps/webapp/app/components/errors/ConfigureErrorAlerts.tsxapps/webapp/app/components/layout/AppLayout.tsxapps/webapp/app/components/logs/LogsVersionFilter.tsxapps/webapp/app/components/primitives/Popover.tsxapps/webapp/app/components/primitives/UnorderedList.tsxapps/webapp/app/components/primitives/charts/ChartLegendCompound.tsxapps/webapp/app/components/primitives/charts/ChartRoot.tsxapps/webapp/app/components/runs/v3/RunFilters.tsxapps/webapp/app/models/projectAlert.server.tsapps/webapp/app/presenters/v3/ErrorAlertChannelPresenter.server.tsapps/webapp/app/presenters/v3/ErrorGroupPresenter.server.tsapps/webapp/app/presenters/v3/ErrorsListPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors._index/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.connect-to-slack.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors/route.tsxapps/webapp/app/routes/storybook.unordered-list/route.tsxapps/webapp/app/routes/storybook/route.tsxapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/v3/alertsWorker.server.tsapps/webapp/app/v3/services/alerts/createAlertChannel.server.tsapps/webapp/app/v3/services/alerts/deliverErrorGroupAlert.server.tsapps/webapp/app/v3/services/alerts/errorAlertEvaluator.server.tsapps/webapp/app/v3/services/alerts/errorGroupWebhook.server.tsapps/webapp/app/v3/services/errorGroupActions.server.tsapps/webapp/test/errorGroupWebhook.test.tsapps/webapp/test/slackErrorAlerts.test.tsapps/webapp/test/webhookErrorAlerts.test.tsinternal-packages/clickhouse/src/errors.tsinternal-packages/clickhouse/src/index.tsinternal-packages/database/prisma/migrations/20260306102053_error_group_state/migration.sqlinternal-packages/database/prisma/schema.prismainternal-packages/emails/emails/alert-error-group.tsxinternal-packages/emails/src/index.tsxpackages/core/src/v3/schemas/webhooks.ts
✅ Files skipped from review due to trivial changes (3)
- apps/webapp/app/components/primitives/charts/ChartLegendCompound.tsx
- apps/webapp/app/routes/storybook.unordered-list/route.tsx
- apps/webapp/app/models/projectAlert.server.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- apps/webapp/app/routes/storybook/route.tsx
- apps/webapp/app/utils/pathBuilder.ts
- apps/webapp/app/components/layout/AppLayout.tsx
- apps/webapp/app/v3/services/alerts/createAlertChannel.server.ts
- apps/webapp/app/components/runs/v3/RunFilters.tsx
- apps/webapp/app/components/logs/LogsVersionFilter.tsx
- apps/webapp/app/components/primitives/charts/ChartRoot.tsx
- internal-packages/emails/src/index.tsx
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts/route.tsx
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.connect-to-slack.ts
- internal-packages/clickhouse/src/index.ts
- apps/webapp/app/v3/services/errorGroupActions.server.ts
- internal-packages/database/prisma/migrations/20260306102053_error_group_state/migration.sql
| if (statusFilter.includeKeys) { | ||
| queryBuilder.where( | ||
| "concat(task_identifier, '::', error_fingerprint) IN ({statusIncludeKeys: Array(String)})", | ||
| { statusIncludeKeys: statusFilter.includeKeys } | ||
| ); | ||
| } | ||
| if (statusFilter.excludeKeys) { | ||
| queryBuilder.where( | ||
| "concat(task_identifier, '::', error_fingerprint) NOT IN ({statusExcludeKeys: Array(String)})", | ||
| { statusExcludeKeys: statusFilter.excludeKeys } | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ClickHouse parameterized IN clause syntax with array parameters: is expr IN {ids:Array(String)}the correct form, and doesexpr IN ({ids:Array(String)}) change the semantics or break the query?
💡 Result:
No, expr IN {ids:Array(String)} is the correct syntax for ClickHouse parameterized queries with array parameters. The parameter {ids:Array(String)} is substituted as an array literal (e.g., ['a','b']), which the IN operator treats as a set of constants on the right side. expr IN ({ids:Array(String)}) adds unnecessary parentheses around the parameter. Since the parameter substitutes to an array literal [...] and IN expects a set or parenthesized list on the right, the extra parentheses would make it IN ([...]), which is invalid syntax and would break the query with a parsing error. For array parameters in IN: - Correct: column IN {ids:Array(String)} - Use has({ids:Array(String)}, column) if needing array membership check (left/right swapped). This syntax is confirmed in official docs for query parameters and GitHub issues validating IN ({param:Array(T)}) form (equivalent without extra parens).
Citations:
- 1: https://clickhouse.com/docs/en/sql-reference/syntax
- 2: https://clickhouse.com/docs/sql-reference/statements/set
- 3: https://clickhouse.com/docs/guides/developer/stored-procedures-and-prepared-statements
- 4: Parametrized view - what is the syntax to use in IN clause? Is it possible to use parameter in subquery? ClickHouse/ClickHouse#46741
- 5: https://github.com/ClickHouse/clickhouse-js/blob/main/examples/query_with_parameter_binding.ts
🏁 Script executed:
# Extract the specific lines from the file to verify the issue
sed -n '235,245p' apps/webapp/app/presenters/v3/ErrorsListPresenter.server.tsRepository: triggerdotdev/trigger.dev
Length of output: 532
🏁 Script executed:
# Search for other IN clause usages in the same file to confirm the existing pattern
rg -A 2 'IN \{' apps/webapp/app/presenters/v3/ErrorsListPresenter.server.tsRepository: triggerdotdev/trigger.dev
Length of output: 650
Fix ClickHouse IN clause syntax to match the existing pattern in this file.
The new IN ({statusIncludeKeys: Array(String)}) and NOT IN ({statusExcludeKeys: Array(String)}) clauses at lines 235-245 add unnecessary parentheses that break the query at runtime. All other IN clauses in this presenter use IN {param: Array(String)} without parentheses. Remove the parentheses to match the existing pattern and ensure the parameter substitution is valid.
🔧 Suggested fix
if (statusFilter.includeKeys) {
queryBuilder.where(
- "concat(task_identifier, '::', error_fingerprint) IN ({statusIncludeKeys: Array(String)})",
+ "concat(task_identifier, '::', error_fingerprint) IN {statusIncludeKeys: Array(String)}",
{ statusIncludeKeys: statusFilter.includeKeys }
);
}
if (statusFilter.excludeKeys) {
queryBuilder.where(
- "concat(task_identifier, '::', error_fingerprint) NOT IN ({statusExcludeKeys: Array(String)})",
+ "concat(task_identifier, '::', error_fingerprint) NOT IN {statusExcludeKeys: Array(String)}",
{ statusExcludeKeys: statusFilter.excludeKeys }
);
}📝 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.
| if (statusFilter.includeKeys) { | |
| queryBuilder.where( | |
| "concat(task_identifier, '::', error_fingerprint) IN ({statusIncludeKeys: Array(String)})", | |
| { statusIncludeKeys: statusFilter.includeKeys } | |
| ); | |
| } | |
| if (statusFilter.excludeKeys) { | |
| queryBuilder.where( | |
| "concat(task_identifier, '::', error_fingerprint) NOT IN ({statusExcludeKeys: Array(String)})", | |
| { statusExcludeKeys: statusFilter.excludeKeys } | |
| ); | |
| if (statusFilter.includeKeys) { | |
| queryBuilder.where( | |
| "concat(task_identifier, '::', error_fingerprint) IN {statusIncludeKeys: Array(String)}", | |
| { statusIncludeKeys: statusFilter.includeKeys } | |
| ); | |
| } | |
| if (statusFilter.excludeKeys) { | |
| queryBuilder.where( | |
| "concat(task_identifier, '::', error_fingerprint) NOT IN {statusExcludeKeys: Array(String)}", | |
| { statusExcludeKeys: statusFilter.excludeKeys } | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/webapp/app/presenters/v3/ErrorsListPresenter.server.ts` around lines 235
- 245, The IN/NOT IN clauses built with queryBuilder are using parentheses
around the ClickHouse parameter placeholders which breaks runtime substitution;
in the code paths that check statusFilter.includeKeys and
statusFilter.excludeKeys (the queryBuilder.where calls referencing
"concat(task_identifier, '::', error_fingerprint)"), remove the surrounding
parentheses so the clauses use the same pattern as other IN usages (i.e., change
"IN ({statusIncludeKeys: Array(String)})" to "IN {statusIncludeKeys:
Array(String)}" and "NOT IN ({statusExcludeKeys: Array(String)})" to "NOT IN
{statusExcludeKeys: Array(String)}" while keeping the parameter objects
statusIncludeKeys/statusExcludeKeys).
| const hasFilters = | ||
| searchParams.has("status") || | ||
| searchParams.has("tasks") || | ||
| searchParams.has("search") || | ||
| searchParams.has("period") || | ||
| searchParams.has("from") || | ||
| searchParams.has("to"); | ||
| searchParams.has("versions") || | ||
| searchParams.has("search"); |
There was a problem hiding this comment.
Include time params in hasFilters.
A custom period / from / to still changes the query, but this check only counts status/task/version/search. That makes “Clear all filters” disappear when time is the only active filter.
🔧 Suggested fix
const hasFilters =
+ searchParams.has("period") ||
+ searchParams.has("from") ||
+ searchParams.has("to") ||
searchParams.has("status") ||
searchParams.has("tasks") ||
searchParams.has("versions") ||
searchParams.has("search");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.errors._index/route.tsx
around lines 407 - 411, The hasFilters check (variable hasFilters) ignores
time-related query params so a custom period/from/to doesn't count as an active
filter; update the expression that computes hasFilters to also check
searchParams.has("period"), searchParams.has("from"), and searchParams.has("to")
(in the same boolean OR chain) so the "Clear all filters" UI appears when only
time params are set.
| <fetcher.Form | ||
| method="post" | ||
| onSubmit={() => { | ||
| setTimeout(onClose, 100); | ||
| }} |
There was a problem hiding this comment.
Close the dialog on success, not on submit.
This dismisses the form after 100 ms even when the action returns validation errors or the POST fails, so the user loses the input with no recovery path. Wait for fetcher.data?.ok === true before calling onClose().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.errors.$fingerprint/route.tsx
around lines 767 - 771, The dialog is being closed unconditionally in the
fetcher.Form onSubmit handler (setTimeout(onClose, 100)) which discards input
even when the POST returns validation errors; remove that onSubmit close and
instead watch fetcher.data for a successful response (check fetcher.data?.ok ===
true) and call onClose() only when that condition becomes true (e.g., use an
effect that depends on fetcher.data to call onClose()). Ensure the fetcher.Form
submit stays intact and reference the existing fetcher.Form, onSubmit handler,
onClose callback, and fetcher.data in the change.
| for (const email of emails) { | ||
| const options: CreateAlertChannelOptions = { | ||
| name: `Error alert to ${email}`, | ||
| alertTypes: ["ERROR_GROUP"], | ||
| environmentTypes, | ||
| deduplicationKey: `error-email:${email}:${environment.type}`, | ||
| channel: { type: "EMAIL", email }, | ||
| }; | ||
| const channel = await service.call(project.externalRef, userId, options); | ||
| processedChannelIds.add(channel.id); | ||
| } | ||
|
|
||
| if (slackChannel) { | ||
| const [channelId, channelName] = slackChannel.split("/"); | ||
| if (channelId && channelName) { | ||
| const options: CreateAlertChannelOptions = { | ||
| name: `Error alert to #${channelName}`, | ||
| alertTypes: ["ERROR_GROUP"], | ||
| environmentTypes, | ||
| deduplicationKey: `error-slack:${environment.type}`, | ||
| channel: { | ||
| type: "SLACK", | ||
| channelId, | ||
| channelName, | ||
| integrationId: slackIntegrationId, | ||
| }, | ||
| }; | ||
| const channel = await service.call(project.externalRef, userId, options); | ||
| processedChannelIds.add(channel.id); | ||
| } | ||
| } |
There was a problem hiding this comment.
Mirror the email/Slack availability checks in the action.
editableTypes only protects deletions. The create/update path still upserts email alerts whenever emails[] is posted and Slack alerts whenever slackChannel is posted, even if those sections were hidden or unavailable in the UI. A crafted POST can therefore persist channels the page intentionally disallows.
Also applies to: 141-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.errors/route.tsx
around lines 97 - 127, The current action unconditionally creates email and
Slack channels from the posted emails[] and slackChannel even when those UI
sections are not editable; before creating channels in the emails loop and
before the slackChannel block, verify the corresponding availability/editable
flag (the same check used to render the UI, e.g., inspect editableTypes or the
feature availability predicate used elsewhere) and skip creation if that channel
type is not allowed; ensure the check happens prior to constructing
CreateAlertChannelOptions and calling service.call(project.externalRef, userId,
...), mirroring the UI logic so only permitted email (emails) and Slack
(slackChannel + slackIntegrationId) channels are upserted.
| // Create secret reference for Slack token | ||
| const secretStore = getSecretStore("DATABASE"); | ||
| const secretKey = `slack-test-token-${Date.now()}`; | ||
|
|
||
| await secretStore.setSecret(secretKey, { | ||
| botAccessToken: process.env.TEST_SLACK_BOT_TOKEN!, | ||
| }); | ||
|
|
||
| const secretReference = await prisma.secretReference.create({ | ||
| data: { | ||
| key: secretKey, | ||
| provider: "DATABASE", | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Delete the stored bot token in teardown.
This writes a live TEST_SLACK_BOT_TOKEN into the secret store and creates a secretReference, but afterAll never removes either one. Repeated runs will leave orphaned secret metadata and a persisted real credential behind.
Also applies to: 138-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/webapp/test/slackErrorAlerts.test.ts` around lines 93 - 107, The test
creates a live secret and DB metadata (using secretStore.setSecret with
secretKey and prisma.secretReference.create) but never cleans them up; add
teardown logic in afterAll to remove both the persisted secret and its DB
record: call the secret store deletion for secretKey via
secretStore.deleteSecret (or the store's remove API) and delete the created
secretReference by id via prisma.secretReference.delete({ where: { id:
<the-created-id> } }) (ensure you capture the created secretReference id when
creating it in the test so teardown can reference it); apply the same cleanup
for the other instance noted at lines ~138-157.
| async evaluate(projectId: string, scheduledAt: number): Promise<void> { | ||
| const nextScheduledAt = Date.now(); | ||
|
|
||
| const channels = await this.resolveChannels(projectId); | ||
| if (channels.length === 0) { | ||
| logger.info("[ErrorAlertEvaluator] No active ERROR_GROUP channels, self-terminating", { | ||
| projectId, | ||
| }); | ||
| return; |
There was a problem hiding this comment.
🚩 Evaluator self-chain stops permanently if all retry attempts fail
The ErrorAlertEvaluator self-chains by enqueuing the next evaluation at the end of evaluate() (line 168). The job has maxAttempts: 3 (alertsWorker.server.ts:66). If all 3 retry attempts fail (e.g., due to a transient ClickHouse outage), selfChain() is never called, and the evaluator permanently stops for that project. It would only restart when a channel is created/enabled/updated via CreateAlertChannelService or the alerts page enable action. This is graceful degradation rather than a bug, but operators should be aware that extended ClickHouse outages could silently disable error alerting for all projects until channels are re-touched.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (submission.value.totalOccurrences) { | ||
| const qb = clickhouseClient.errors.listQueryBuilder(); | ||
| qb.where("project_id = {projectId: String}", { projectId: project.id }); | ||
| qb.where("environment_id = {environmentId: String}", { | ||
| environmentId: environment.id, | ||
| }); | ||
| qb.where("error_fingerprint = {fingerprint: String}", { fingerprint }); | ||
| qb.where("task_identifier = {taskIdentifier: String}", { | ||
| taskIdentifier: submission.value.taskIdentifier, | ||
| }); | ||
| qb.groupBy("error_fingerprint, task_identifier"); | ||
|
|
||
| const [err, results] = await qb.execute(); | ||
| if (!err && results && results.length > 0) { | ||
| occurrenceCountAtIgnoreTime = results[0].occurrence_count; | ||
| } | ||
| } |
There was a problem hiding this comment.
🚩 Total occurrence ignore threshold silently degrades if ClickHouse query fails at ignore time
In the fingerprint route action at lines 146-162, occurrenceCountAtIgnoreTime is only fetched from ClickHouse when submission.value.totalOccurrences is set. If the ClickHouse query fails (error or empty results), occurrenceCountAtIgnoreTime remains undefined, resulting in ignoredAtOccurrenceCount: null in the DB. In the evaluator's isIgnoreBreached() at line 220, the total-occurrences check is skipped when ignoredAtOccurrenceCount is null. This means a user who sets a total occurrence threshold but experiences a transient ClickHouse failure will have that threshold effectively disabled forever. There's no user-visible indication of this degradation.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const qb = this._clickhouse.errors.activeErrorsSinceQueryBuilder(); | ||
| qb.where("project_id = {projectId: String}", { projectId }); | ||
| qb.where("environment_id IN {envIds: Array(String)}", { envIds }); | ||
| qb.groupBy("environment_id, task_identifier, error_fingerprint"); | ||
| qb.having("toInt64(last_seen) > {scheduledAt: Int64}", { | ||
| scheduledAt, | ||
| }); |
There was a problem hiding this comment.
🚩 activeErrorsSinceQueryBuilder omits organization_id filter unlike other error queries
The getActiveErrors method at apps/webapp/app/v3/services/alerts/errorAlertEvaluator.server.ts:308-314 filters only by project_id and environment_id, while every other error query in the codebase (e.g., ErrorGroupPresenter.getSummary, ErrorsListPresenter.call) also includes organization_id. Since project_id is globally unique this won't cause incorrect results, but if the errors_v1 table is partitioned or indexed by organization_id first, omitting it could impact query performance on large datasets.
Was this helpful? React with 👍 or 👎 to provide feedback.
| export function getActiveErrorsSinceQueryBuilder( | ||
| ch: ClickhouseReader, | ||
| settings?: ClickHouseSettings | ||
| ) { | ||
| return ch.queryBuilder({ | ||
| name: "getActiveErrorsSince", | ||
| baseQuery: ` | ||
| SELECT | ||
| environment_id, | ||
| task_identifier, | ||
| error_fingerprint, | ||
| any(error_type) as error_type, | ||
| any(error_message) as error_message, | ||
| any(sample_stack_trace) as sample_stack_trace, | ||
| toString(toUnixTimestamp64Milli(min(first_seen))) as first_seen, | ||
| toString(toUnixTimestamp64Milli(max(last_seen))) as last_seen, | ||
| toUInt64(sumMerge(occurrence_count)) as occurrence_count | ||
| FROM trigger_dev.errors_v1 | ||
| `, | ||
| schema: ActiveErrorsSinceQueryResult, | ||
| settings, | ||
| }); |
There was a problem hiding this comment.
🚩 activeErrorsSinceQueryBuilder references sample_stack_trace on errors_v1 without a visible ClickHouse migration
The ActiveErrorsSinceQueryResult schema at internal-packages/clickhouse/src/errors.ts:358-370 expects a sample_stack_trace field from errors_v1, and the query uses any(sample_stack_trace). However, other existing queries against errors_v1 (e.g., getErrorGroupsListQueryBuilder, getErrorGroups) don't reference this column, and no ClickHouse migration adding it appears in this PR. If the column doesn't exist, the alert evaluator query will fail at runtime. If it does exist, the use of any() (vs anyMerge() used for other aggregate columns like sample_run_id) should be verified against the column's actual type.
Was this helpful? React with 👍 or 👎 to provide feedback.
| private async getErrorGroupStates( | ||
| activeErrors: ActiveErrorsSinceQueryResult[] | ||
| ): Promise<ErrorGroupState[]> { | ||
| if (activeErrors.length === 0) return []; | ||
|
|
||
| return this._replica.errorGroupState.findMany({ | ||
| where: { | ||
| OR: activeErrors.map((e) => ({ | ||
| environmentId: e.environment_id, | ||
| taskIdentifier: e.task_identifier, | ||
| errorFingerprint: e.error_fingerprint, | ||
| })), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🚩 Unbounded Prisma OR query in evaluator could generate very large SQL for high-volume projects
The getErrorGroupStates method at apps/webapp/app/v3/services/alerts/errorAlertEvaluator.server.ts:329-337 builds a Prisma OR clause from all active errors. For projects with many distinct error fingerprints across multiple environments, this could generate a very large SQL query. The same pattern appears in ErrorsListPresenter.getErrorGroupStates at apps/webapp/app/presenters/v3/ErrorsListPresenter.server.ts:502-508 but is bounded by page size (25). The evaluator's query has no such bound — it processes ALL errors with last_seen > scheduledAt.
Was this helpful? React with 👍 or 👎 to provide feedback.
| firstSeen: new Date().toISOString(), | ||
| lastSeen: new Date().toISOString(), |
There was a problem hiding this comment.
🚩 Test mock data uses ISO dates instead of millisecond timestamps
In apps/webapp/test/webhookErrorAlerts.test.ts:77-78, the createMockErrorPayload function sets firstSeen: new Date().toISOString() and lastSeen: new Date().toISOString(). However, generateErrorGroupWebhookPayload at apps/webapp/app/v3/services/alerts/errorGroupWebhook.server.ts:51-52 does new Date(Number(data.error.firstSeen)). Number("2024-01-01T00:00:00.000Z") produces NaN, leading to Invalid Date, which would cause Webhook.parse() to fail validation. The sibling test file errorGroupWebhook.test.ts correctly uses String(earlier) (millisecond timestamps). These tests require a database and may not run in standard CI, so this may not block the PR.
Was this helpful? React with 👍 or 👎 to provide feedback.