test: e2e auth baseline tests + webapp testcontainer infrastructure#3438
test: e2e auth baseline tests + webapp testcontainer infrastructure#3438matt-aitken wants to merge 11 commits intomainfrom
Conversation
Adds the ability to spawn the built webapp as a child process in tests, plus a baseline of auth behaviour tests that will be used to verify the upcoming apiBuilder RBAC migration leaves auth behaviour unchanged. - internal-packages/testcontainers/src/webapp.ts: new helper that spawns build/server.js, waits for /healthcheck, and exposes a WebappInstance + startTestServer() convenience wrapper - internal-packages/testcontainers/package.json: add ./webapp sub-path export so tests can import from @internal/testcontainers/webapp - internal-packages/testcontainers/src/index.ts: export createPostgresContainer (needed by webapp.ts internally) - apps/webapp/test/helpers/seedTestEnvironment.ts: creates a minimal org/project/environment row set for use in auth tests - apps/webapp/test/api-auth.e2e.test.ts: 8 baseline tests covering API-key auth, JWT auth, missing/invalid credentials, and error shapes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
|
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 an E2E test suite for the webapp that starts a shared test server, seeds a development test environment via a new Prisma helper, and verifies API and JWT bearer authentication (including scope behavior). Introduces Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/webapp/test/helpers/seedTestEnvironment.ts (1)
10-11: Consider reusingcreateApiKeyForEnv/createPkApiKeyForEnvfor key generation.The generated keys (
tr_dev_${24-hex}/pk_dev_${24-hex}) diverge from the production format produced bycreateApiKeyForEnvinapps/webapp/app/models/api-key.server.ts(tr_dev_{20-char-alphanumeric}viaapiKeyId(20)). Functionally equivalent for bearer lookup today, but reusing the real helpers keeps this harness aligned if key parsing/validation ever tightens (length, charset, or prefix checks) and removes a subtle drift between test and prod auth paths.♻️ Proposed refactor
+import { createApiKeyForEnv, createPkApiKeyForEnv } from "~/models/api-key.server"; import type { PrismaClient } from "@trigger.dev/database"; import { randomBytes } from "crypto"; @@ - const apiKey = `tr_dev_${randomHex(24)}`; - const pkApiKey = `pk_dev_${randomHex(24)}`; + const apiKey = createApiKeyForEnv("DEVELOPMENT"); + const pkApiKey = createPkApiKeyForEnv("DEVELOPMENT");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/test/helpers/seedTestEnvironment.ts` around lines 10 - 11, Replace the ad-hoc key generation in seedTestEnvironment.ts (currently using `tr_dev_${randomHex(24)}` and `pk_dev_${randomHex(24)}`) with the production helpers `createApiKeyForEnv` and `createPkApiKeyForEnv` so test keys match real format; import those functions from apps/webapp/app/models/api-key.server.ts (which uses `apiKeyId(20)`) and call them to produce the `apiKey` and `pkApiKey` values used in the test seed, ensuring any future prefix/length/charset validation stays consistent between tests and production.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/webapp/test/helpers/seedTestEnvironment.ts`:
- Around line 10-11: Replace the ad-hoc key generation in seedTestEnvironment.ts
(currently using `tr_dev_${randomHex(24)}` and `pk_dev_${randomHex(24)}`) with
the production helpers `createApiKeyForEnv` and `createPkApiKeyForEnv` so test
keys match real format; import those functions from
apps/webapp/app/models/api-key.server.ts (which uses `apiKeyId(20)`) and call
them to produce the `apiKey` and `pkApiKey` values used in the test seed,
ensuring any future prefix/length/charset validation stays consistent between
tests and production.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a9944271-51a1-4a09-a908-c2e4e8e690d8
📒 Files selected for processing (1)
apps/webapp/test/helpers/seedTestEnvironment.ts
📜 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). (18)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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
Files:
apps/webapp/test/helpers/seedTestEnvironment.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/test/helpers/seedTestEnvironment.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Add crumbs as you write code using
//@Crumbscomments or `// `#region` `@crumbsblocks. These are temporary debug instrumentation and must be stripped usingagentcrumbs stripbefore merge.
Files:
apps/webapp/test/helpers/seedTestEnvironment.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/test/helpers/seedTestEnvironment.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/test/helpers/seedTestEnvironment.ts
**/*.ts{,x}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from
@trigger.dev/sdkwhen writing Trigger.dev tasks. Never use@trigger.dev/sdk/v3or deprecatedclient.defineJob.
Files:
apps/webapp/test/helpers/seedTestEnvironment.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: Access environment variables through theenvexport ofenv.server.tsinstead of directly accessingprocess.env
Use subpath exports from@trigger.dev/corepackage instead of importing from the root@trigger.dev/corepathUse named constants for sentinel/placeholder values (e.g.
const UNSET_VALUE = '__unset__') instead of raw string literals scattered across comparisons
Files:
apps/webapp/test/helpers/seedTestEnvironment.ts
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: internal-packages/run-engine/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:25.254Z
Learning: Applies to internal-packages/run-engine/src/engine/tests/**/*.test.ts : Implement tests for RunEngine in `src/engine/tests/` using testcontainers for Redis and PostgreSQL containerization
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/test/engine/taskIdentifierRegistry.test.ts:3-19
Timestamp: 2026-04-16T13:45:22.317Z
Learning: In `apps/webapp/test/engine/taskIdentifierRegistry.test.ts`, the `vi.mock` calls for `~/services/taskIdentifierCache.server` (stubbing `getTaskIdentifiersFromCache` and `populateTaskIdentifierCache`), `~/models/task.server` (stubbing `getAllTaskIdentifiers`), and `~/db.server` (stubbing `prisma` and `$replica`) are intentional. The suite uses real Postgres via testcontainers for all `TaskIdentifier` DB operations, but isolates the Redis cache layer and legacy query fallback as separate concerns not exercised in this test file. Do not flag these mocks as violations of the no-mocks policy in future reviews.
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/app/services/taskIdentifierRegistry.server.ts:24-67
Timestamp: 2026-04-13T21:44:00.032Z
Learning: In `apps/webapp/app/services/taskIdentifierRegistry.server.ts`, the sequential upsert/updateMany/findMany writes in `syncTaskIdentifiers` are intentionally NOT wrapped in a Prisma transaction. This function runs only during deployment-change events (low-concurrency path), and any partial `isInLatestDeployment` state is acceptable because it self-corrects on the next deployment. Do not flag this as a missing-transaction/atomicity issue in future reviews.
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3331
File: apps/webapp/test/engine/batchPayloads.test.ts:5-24
Timestamp: 2026-04-07T14:12:18.946Z
Learning: In `apps/webapp/test/engine/batchPayloads.test.ts`, using `vi.mock` for `~/v3/objectStore.server` (stubbing `hasObjectStoreClient` and `uploadPacketToObjectStore`), `~/env.server` (overriding offload thresholds), and `~/v3/tracer.server` (stubbing `startActiveSpan`) is intentional and acceptable. Simulating controlled transient upload failures (e.g., fail N times then succeed) to verify `p-retry` behavior cannot be reproduced with real services or testcontainers. This file is an explicit exception to the repo's general no-mocks policy.
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2026-04-15T15:39:31.575Z
Learning: Applies to apps/webapp/**/*.test.{ts,tsx} : Do not import `env.server.ts` directly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T15:39:06.868Z
Learning: Applies to **/*.test.{ts,tsx} : For Redis/PostgreSQL tests in vitest, use testcontainers helpers: `redisTest`, `postgresTest`, or `containerTest` imported from `internal/testcontainers`.
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: packages/redis-worker/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:43.173Z
Learning: Applies to packages/redis-worker/**/redis-worker/**/*.{test,spec}.{ts,tsx} : Use testcontainers for Redis in test files for redis-worker
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T10:48:02.687Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Tests should avoid mocks or stubs and use the helpers from `internal/testcontainers` when Redis or Postgres are needed
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-04-16T14:19:16.330Z
Learning: Applies to apps/webapp/**/*.test.{ts,tsx} : For testable code, never import `env.server.ts` in test files. Pass configuration as options instead (e.g., `realtimeClient.server.ts` takes config as constructor arg, `realtimeClientGlobal.server.ts` creates singleton with env config)
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T15:39:06.868Z
Learning: Applies to **/*.test.{ts,tsx} : Use vitest exclusively for testing. Never mock anything — use testcontainers instead.
📚 Learning: 2026-04-15T15:39:31.575Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2026-04-15T15:39:31.575Z
Learning: Applies to apps/webapp/**/*.test.{ts,tsx} : Do not import `env.server.ts` directly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable
Applied to files:
apps/webapp/test/helpers/seedTestEnvironment.ts
📚 Learning: 2026-04-15T15:39:31.575Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2026-04-15T15:39:31.575Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env`
Applied to files:
apps/webapp/test/helpers/seedTestEnvironment.ts
📚 Learning: 2026-04-16T14:19:16.330Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-04-16T14:19:16.330Z
Learning: Applies to apps/webapp/**/*.test.{ts,tsx} : For testable code, never import `env.server.ts` in test files. Pass configuration as options instead (e.g., `realtimeClient.server.ts` takes config as constructor arg, `realtimeClientGlobal.server.ts` creates singleton with env config)
Applied to files:
apps/webapp/test/helpers/seedTestEnvironment.ts
📚 Learning: 2026-04-16T13:45:22.317Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/test/engine/taskIdentifierRegistry.test.ts:3-19
Timestamp: 2026-04-16T13:45:22.317Z
Learning: In `apps/webapp/test/engine/taskIdentifierRegistry.test.ts`, the `vi.mock` calls for `~/services/taskIdentifierCache.server` (stubbing `getTaskIdentifiersFromCache` and `populateTaskIdentifierCache`), `~/models/task.server` (stubbing `getAllTaskIdentifiers`), and `~/db.server` (stubbing `prisma` and `$replica`) are intentional. The suite uses real Postgres via testcontainers for all `TaskIdentifier` DB operations, but isolates the Redis cache layer and legacy query fallback as separate concerns not exercised in this test file. Do not flag these mocks as violations of the no-mocks policy in future reviews.
Applied to files:
apps/webapp/test/helpers/seedTestEnvironment.ts
📚 Learning: 2026-03-02T12:42:47.652Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/supervisor/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:42:47.652Z
Learning: Applies to apps/supervisor/src/env.ts : Environment configuration should be defined in `src/env.ts`
Applied to files:
apps/webapp/test/helpers/seedTestEnvironment.ts
📚 Learning: 2026-04-16T14:21:17.695Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/app/components/logs/LogsTaskFilter.tsx:135-163
Timestamp: 2026-04-16T14:21:17.695Z
Learning: In `triggerdotdev/trigger.dev` PR `#3368`, the `TaskIdentifier` table has a `@unique([runtimeEnvironmentId, slug])` DB constraint, guaranteeing one row per (environment, slug). In components like `apps/webapp/app/components/logs/LogsTaskFilter.tsx` and `apps/webapp/app/components/runs/v3/RunFilters.tsx`, using `key={item.slug}` for SelectItem list items is correct and unique. Do NOT flag `key={item.slug}` as potentially non-unique — the old duplicate-(slug, triggerSource) issue only existed with the legacy `DISTINCT` query, which this registry replaces.
Applied to files:
apps/webapp/test/helpers/seedTestEnvironment.ts
📚 Learning: 2026-04-16T14:19:16.330Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-04-16T14:19:16.330Z
Learning: Applies to apps/webapp/**/*.server.ts : Always use `findFirst` instead of `findUnique` in Prisma queries. `findUnique` has an implicit DataLoader that batches concurrent calls and has active bugs even in Prisma 6.x (uppercase UUIDs returning null, composite key SQL correctness issues, 5-10x worse performance). `findFirst` is never batched and avoids this entire class of issues
Applied to files:
apps/webapp/test/helpers/seedTestEnvironment.ts
📚 Learning: 2026-04-20T15:08:55.358Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3417
File: apps/webapp/app/services/sessionsReplicationService.server.ts:204-215
Timestamp: 2026-04-20T15:08:55.358Z
Learning: In `apps/webapp/app/services/sessionsReplicationService.server.ts` and `apps/webapp/app/services/runsReplicationService.server.ts`, the `getKey` function in `ConcurrentFlushScheduler` uses `${item.event}_${item.session.id}` / `${item.event}_${item.run.id}` respectively. This pattern is intentionally kept identical across both replication services for consistency. Any change to the deduplication key shape (e.g., keying solely by session/run id) must be applied to both services together, never to one service in isolation. Tracking as a cross-service follow-up.
Applied to files:
apps/webapp/test/helpers/seedTestEnvironment.ts
📚 Learning: 2026-04-13T21:44:00.032Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/app/services/taskIdentifierRegistry.server.ts:24-67
Timestamp: 2026-04-13T21:44:00.032Z
Learning: In `apps/webapp/app/services/taskIdentifierRegistry.server.ts`, the sequential upsert/updateMany/findMany writes in `syncTaskIdentifiers` are intentionally NOT wrapped in a Prisma transaction. This function runs only during deployment-change events (low-concurrency path), and any partial `isInLatestDeployment` state is acceptable because it self-corrects on the next deployment. Do not flag this as a missing-transaction/atomicity issue in future reviews.
Applied to files:
apps/webapp/test/helpers/seedTestEnvironment.ts
📚 Learning: 2026-04-23T13:26:27.529Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3430
File: apps/webapp/app/presenters/v3/RunStreamPresenter.server.ts:0-0
Timestamp: 2026-04-23T13:26:27.529Z
Learning: In `apps/webapp/app/utils/sse.ts` (and callers such as `apps/webapp/app/presenters/v3/RunStreamPresenter.server.ts`), the string-reason type passed to `.abort()` is NOT load-bearing for heap retention in SSE streams on Node 20. Isolation testing (2000 requests × 200 KB payload) confirmed `.abort()` vs `.abort("string")` produces identical heap deltas once `AbortSignal.any` is removed. The actual root cause of the SSE memory leak was `AbortSignal.any`, which registers signals with a FinalizationRegistry that does not reliably release them on abort (see nodejs/node#54614 and `#55351`). Named sentinel constants (`ABORT_REASON_*`, exported from `sse.ts`) are a readability/style win per the CLAUDE.md named-constant guideline, not a correctness fix. Do not flag `.abort()` vs `.abort("reason string")` as a memory-retention issue in future reviews of this codebase.
Applied to files:
apps/webapp/test/helpers/seedTestEnvironment.ts
📚 Learning: 2026-03-26T10:02:25.354Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 3254
File: apps/webapp/app/services/platformNotifications.server.ts:363-385
Timestamp: 2026-03-26T10:02:25.354Z
Learning: In `triggerdotdev/trigger.dev`, the `getNextCliNotification` fallback in `apps/webapp/app/services/platformNotifications.server.ts` intentionally uses `prisma.orgMember.findFirst` (single org) when no `projectRef` is provided. This is acceptable for v1 because the CLI (`dev` and `login` commands) always passes `projectRef` in normal usage, making the fallback a rare edge case. Do not flag the single-org fallback as a multi-org correctness bug in this file.
Applied to files:
apps/webapp/test/helpers/seedTestEnvironment.ts
📚 Learning: 2026-04-07T14:12:18.946Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3331
File: apps/webapp/test/engine/batchPayloads.test.ts:5-24
Timestamp: 2026-04-07T14:12:18.946Z
Learning: In `apps/webapp/test/engine/batchPayloads.test.ts`, using `vi.mock` for `~/v3/objectStore.server` (stubbing `hasObjectStoreClient` and `uploadPacketToObjectStore`), `~/env.server` (overriding offload thresholds), and `~/v3/tracer.server` (stubbing `startActiveSpan`) is intentional and acceptable. Simulating controlled transient upload failures (e.g., fail N times then succeed) to verify `p-retry` behavior cannot be reproduced with real services or testcontainers. This file is an explicit exception to the repo's general no-mocks policy.
Applied to files:
apps/webapp/test/helpers/seedTestEnvironment.ts
📚 Learning: 2026-03-22T13:26:12.060Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/code/TextEditor.tsx:81-86
Timestamp: 2026-03-22T13:26:12.060Z
Learning: In the triggerdotdev/trigger.dev codebase, do not flag `navigator.clipboard.writeText(...)` calls for `missing-await`/`unhandled-promise` issues. These clipboard writes are intentionally invoked without `await` and without `catch` handlers across the project; keep that behavior consistent when reviewing TypeScript/TSX files (e.g., usages like in `apps/webapp/app/components/code/TextEditor.tsx`).
Applied to files:
apps/webapp/test/helpers/seedTestEnvironment.ts
📚 Learning: 2026-03-22T19:24:14.403Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/v3/services/alerts/deliverErrorGroupAlert.server.ts:200-204
Timestamp: 2026-03-22T19:24:14.403Z
Learning: In the triggerdotdev/trigger.dev codebase, webhook URLs are not expected to contain embedded credentials/secrets (e.g., fields like `ProjectAlertWebhookProperties` should only hold credential-free webhook endpoints). During code review, if you see logging or inclusion of raw webhook URLs in error messages, do not automatically treat it as a credential-leak/secrets-in-logs issue by default—first verify the URL does not contain embedded credentials (for example, no username/password in the URL, no obvious secret/token query params or fragments). If the URL is credential-free per this project’s conventions, allow the logging.
Applied to files:
apps/webapp/test/helpers/seedTestEnvironment.ts
- Capture spawn errors in a variable instead of throwing from the
proc.on('error') listener. A synchronous throw inside an EventEmitter
listener bypasses the surrounding try/catch and surfaces as an
uncaughtException, tearing down the test runner.
- Wrap resource acquisition in startTestServer() in a try/catch so that
a failure in startWebapp() (e.g. healthcheck timeout) tears down the
postgres container, network, and PrismaClient instead of leaking them.
Co-Authored-By: Matt Aitken <matt@mattaitken.com>
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.
E2e tests require a pre-built webapp (build/server.js) which is not present during normal CI test runs. Exclude them from the default include pattern so the sharded test jobs don't pick them up. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/e2e-webapp.yml (2)
87-88: No JUnit/blob reporter — CI loses structured failure output.The sibling webapp unit-tests workflow uses
--reporter=default --reporter=blobto support sharded merge and richer CI annotations. This job uses only--reporter=default, so on failure you lose the structured artifact that the rest of the pipeline relies on. Worth adding blob (or at least--reporter=junit) for consistency with the other jobs, especially if you later shard or aggregate these results.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-webapp.yml around lines 87 - 88, The Webapp E2E job ("🧪 Run Webapp E2E Tests") only passes --reporter=default to vitest, so CI won't produce the structured blob/JUnit artifacts; update the run command that invokes vitest (the line with pnpm exec vitest run test/api-auth.e2e.test.ts --reporter=default) to include the same reporters as the sibling unit-tests workflow (for example: --reporter=default --reporter=blob, or --reporter=default --reporter=junit) so CI receives the structured test output.
59-68: Minor: step-levelenv+ifreadability.The condition
if: ${{ env.DOCKERHUB_USERNAME }}works becauseDOCKERHUB_USERNAMEis surfaced at the job-levelenv:. Consider checkingsecrets.DOCKERHUB_USERNAME != ''directly, or making the intent explicit withif: ${{ env.DOCKERHUB_USERNAME != '' }}, since an empty string is currently treated as falsy by virtue of GHA coercion — works today but is easy to mis-edit later. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-webapp.yml around lines 59 - 68, The conditional checks on the DockerHub login steps are ambiguous because they rely on GHA coercion of env values; update the step-level if expressions for the "🐳 Login to DockerHub" and "🐳 Skipping DockerHub login (no secrets available)" steps to explicitly test for a non-empty value (for example use if: ${{ env.DOCKERHUB_USERNAME != '' }} or if: ${{ secrets.DOCKERHUB_USERNAME != '' }} for the login step and the inverse for the skip step) so the intent is explicit and resilient to future edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/e2e-webapp.yml:
- Around line 87-88: The Webapp E2E job ("🧪 Run Webapp E2E Tests") only passes
--reporter=default to vitest, so CI won't produce the structured blob/JUnit
artifacts; update the run command that invokes vitest (the line with pnpm exec
vitest run test/api-auth.e2e.test.ts --reporter=default) to include the same
reporters as the sibling unit-tests workflow (for example: --reporter=default
--reporter=blob, or --reporter=default --reporter=junit) so CI receives the
structured test output.
- Around line 59-68: The conditional checks on the DockerHub login steps are
ambiguous because they rely on GHA coercion of env values; update the step-level
if expressions for the "🐳 Login to DockerHub" and "🐳 Skipping DockerHub login
(no secrets available)" steps to explicitly test for a non-empty value (for
example use if: ${{ env.DOCKERHUB_USERNAME != '' }} or if: ${{
secrets.DOCKERHUB_USERNAME != '' }} for the login step and the inverse for the
skip step) so the intent is explicit and resilient to future edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 76a8e029-607d-45e6-87c9-cdff0546cc5e
📒 Files selected for processing (2)
.github/workflows/e2e-webapp.yml.github/workflows/unit-tests.yml
📜 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). (32)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 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 (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / e2e-webapp / 🧪 E2E Tests: Webapp
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: typecheck / typecheck
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: audit
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: internal-packages/run-engine/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:25.254Z
Learning: Applies to internal-packages/run-engine/src/engine/tests/**/*.test.ts : Implement tests for RunEngine in `src/engine/tests/` using testcontainers for Redis and PostgreSQL containerization
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/test/engine/taskIdentifierRegistry.test.ts:3-19
Timestamp: 2026-04-16T13:45:22.317Z
Learning: In `apps/webapp/test/engine/taskIdentifierRegistry.test.ts`, the `vi.mock` calls for `~/services/taskIdentifierCache.server` (stubbing `getTaskIdentifiersFromCache` and `populateTaskIdentifierCache`), `~/models/task.server` (stubbing `getAllTaskIdentifiers`), and `~/db.server` (stubbing `prisma` and `$replica`) are intentional. The suite uses real Postgres via testcontainers for all `TaskIdentifier` DB operations, but isolates the Redis cache layer and legacy query fallback as separate concerns not exercised in this test file. Do not flag these mocks as violations of the no-mocks policy in future reviews.
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 **/*.{test,spec}.{ts,tsx} : Use vitest for all tests in the Trigger.dev repository
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2026-04-15T15:39:31.575Z
Learning: Applies to apps/webapp/**/*.test.{ts,tsx} : Do not import `env.server.ts` directly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T15:39:06.868Z
Learning: Applies to **/*.test.{ts,tsx} : For Redis/PostgreSQL tests in vitest, use testcontainers helpers: `redisTest`, `postgresTest`, or `containerTest` imported from `internal/testcontainers`.
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T15:39:06.868Z
Learning: Applies to **/*.test.{ts,tsx} : Use vitest exclusively for testing. Never mock anything — use testcontainers instead.
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-04-16T14:19:16.330Z
Learning: Applies to apps/webapp/**/*.test.{ts,tsx} : For testable code, never import `env.server.ts` in test files. Pass configuration as options instead (e.g., `realtimeClient.server.ts` takes config as constructor arg, `realtimeClientGlobal.server.ts` creates singleton with env config)
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T10:48:02.687Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vitest for running unit tests
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3331
File: apps/webapp/test/engine/batchPayloads.test.ts:5-24
Timestamp: 2026-04-07T14:12:18.946Z
Learning: In `apps/webapp/test/engine/batchPayloads.test.ts`, using `vi.mock` for `~/v3/objectStore.server` (stubbing `hasObjectStoreClient` and `uploadPacketToObjectStore`), `~/env.server` (overriding offload thresholds), and `~/v3/tracer.server` (stubbing `startActiveSpan`) is intentional and acceptable. Simulating controlled transient upload failures (e.g., fail N times then succeed) to verify `p-retry` behavior cannot be reproduced with real services or testcontainers. This file is an explicit exception to the repo's general no-mocks policy.
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: packages/redis-worker/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:43.173Z
Learning: Applies to packages/redis-worker/**/redis-worker/**/*.{test,spec}.{ts,tsx} : Use testcontainers for Redis in test files for redis-worker
📚 Learning: 2026-03-02T12:43:25.254Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: internal-packages/run-engine/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:25.254Z
Learning: Applies to internal-packages/run-engine/src/engine/tests/**/*.test.ts : Implement tests for RunEngine in `src/engine/tests/` using testcontainers for Redis and PostgreSQL containerization
Applied to files:
.github/workflows/e2e-webapp.yml
🔇 Additional comments (3)
.github/workflows/unit-tests.yml (1)
13-15: LGTM — reusable workflow wiring is correct.
uses: ./.github/workflows/e2e-webapp.ymlwithsecrets: inheritmatches the pattern used by the other test jobs in this file, and the called workflow declareson: workflow_call. Partitioning withvitest.config.ts'sexclude: ["test/**/*.e2e.test.ts"]means the e2e file won't also run in thewebappunit job, so there's no duplicate execution..github/workflows/e2e-webapp.yml (2)
70-76: The pre-pulled image tags are correct. The code uses exactlypostgres:14(fromcreatePostgresContainerininternal-packages/testcontainers/src/utils.tsline 16) and testcontainers-node 10.28.0 hardcodestestcontainers/ryuk:0.11.0as the default reaper image. Both match the workflow pre-pulls, so the authenticated pull will cache these images successfully and avoid DockerHub rate limits on runtime pulls.> Likely an incorrect or invalid review comment.
78-85: The suggested filter is unnecessary and ineffective.
pnpm run generatewithout--filteralready only runs generate for packages that define it:@trigger.dev/database(Prisma) and@internal/llm-model-catalog. Since webapp depends directly on@internal/llm-model-catalogand indirectly on@trigger.dev/database(via@internal/testcontainers), both generate scripts are required for the E2E test to run. The--filter webapp...suggestion would not reduce work—it would run the same packages. The current approach is correct.> Likely an incorrect or invalid review comment.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| e2e-webapp: | ||
| uses: ./.github/workflows/e2e-webapp.yml | ||
| secrets: inherit |
There was a problem hiding this comment.
🚩 E2E tests run in the unit-tests workflow rather than a dedicated pipeline
The new e2e-webapp.yml workflow is called from .github/workflows/unit-tests.yml alongside the actual unit test jobs. E2E tests (starting containers, building the webapp, running a server) are significantly heavier and slower than unit tests. This coupling means E2E failures block the unit tests workflow. This is a design choice that may be intentional for simplicity, but a dedicated workflow or a separate CI job group might be more appropriate as E2E tests grow.
Was this helpful? React with 👍 or 👎 to provide feedback.
| run: | | ||
| echo "Pre-pulling Docker images with authenticated session..." | ||
| docker pull postgres:14 | ||
| docker pull redis:7-alpine |
There was a problem hiding this comment.
🚩 Pre-pulled Redis image tag may not match testcontainers default
The CI workflow pre-pulls redis:7-alpine (line 75), but @testcontainers/redis's RedisContainer uses a more specific default image tag (e.g., redis:7.2.4-alpine depending on the package version). If the tags don't match, Docker will still pull the correct image at test time — the pre-pull just won't provide the intended caching benefit. Consider checking the actual default image used by the installed @testcontainers/redis version and aligning the pre-pull tag.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Adds a minimal end-to-end test harness that spawns the compiled webapp as a child
process against a throwaway Postgres container, plus a baseline of 8 auth-behaviour
tests. These tests will be used as a regression check before and after the upcoming
apiBuilder RBAC migration to confirm auth behaviour is unchanged.
What's included
internal-packages/testcontainers/src/webapp.ts(new)Spawns
build/server.jswith a dynamically allocated port, polls/healthcheck,and exposes
WebappInstanceandstartTestServer()(postgres container + webapp +PrismaClient in one call). Key details:
process.execPathso the correct Node binary is found in forked test processesNODE_PATHtonode_modules/.pnpm/node_modulesso pnpm-hoisted transitivedeps (e.g.
eventsource-parser) resolve correctly inside the subprocessPORTandREMIX_APP_PORTso Vite's automatic.envloadingdoesn't override the dynamically allocated port
internal-packages/testcontainers/package.jsonAdds
./webappsub-path export so tests canimport from "@internal/testcontainers/webapp".internal-packages/testcontainers/src/index.tsExports
createPostgresContainer(used internally bywebapp.ts).apps/webapp/test/helpers/seedTestEnvironment.ts(new)Creates a minimal org → project → environment row set with random suffixes.
apps/webapp/test/api-auth.e2e.test.ts(new)8 tests across two suites:
How to run
Test plan